parser: add IANA time_zone support for naive timestamps#11639
parser: add IANA time_zone support for naive timestamps#11639arnikola wants to merge 1 commit intofluent:4.2from
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e01d163f6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (parser->time_zone) { | ||
| char *old_tz = NULL; | ||
| const char *prev; | ||
| struct tm tmp; |
There was a problem hiding this comment.
Skip time_zone conversion when timestamp already has offset
The new time_zone branch always uses mktime in the configured zone, even when time_format includes %z/%Z and the parsed record already carries an explicit offset. In that case flb_parser_time_lookup() has already parsed the source zone, but this path ignores it and reinterprets the wall clock in parser->time_zone, producing shifted epochs (for example, a +0000 input gets converted as local New York time). Please either reject time_zone with %z/%Z formats in flb_parser_create() or bypass this branch when parser->time_with_tz is true.
Useful? React with 👍 / 👎.
| if (setenv("TZ", parser->time_zone, 1) != 0) { | ||
| flb_free(old_tz); | ||
| pthread_mutex_unlock(&flb_time_zone_mutex); | ||
| return (time_t) -1; | ||
| } |
There was a problem hiding this comment.
Avoid mutating process TZ during per-record parsing
This code changes TZ and calls tzset() for every parsed record, which is process-global state. The mutex only serializes this function, but other paths still call mktime without this lock (e.g. flb_parser_tm2time_simple for time_system_timezone), so concurrent parsing can observe the temporary timezone and compute wrong epochs. This creates cross-parser timestamp corruption when one thread uses time_zone and another thread parses local/system timestamps at the same time.
Useful? React with 👍 / 👎.
|
Example config files:
|
|
The patch direction looks great but we need to add Signed-off line on every commit. So, could you add this line? ref: https://github.com/fluent/fluent-bit/pull/11639/checks?check_run_id=69300784903 |
Add optional parser setting time_zone (IANA name, e.g. from tzdata) so naive timestamps from time_format are converted with local rules and DST, instead of only fixed offset or system timezone.
[N/A]
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.