parser: Add IANA time_zone support for native timestamps#11913
Conversation
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
…p cases Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
…lookup Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
…on Windows Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughFluent Bit parsers now accept per-parser IANA Time_Zone, validate/load platform-specific zone data (Windows IANA→native or POSIX tzif), convert naive timestamps with the configured zone, wire Time_Zone into config/creation, update format parsers to use the new converter, and add unit/integration tests plus a CI change. ChangesIANA Timezone Parser Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/flb_parser.csrc/flb_parser.c:20:10: fatal error: 'fluent-bit/flb_info.h' file not found ... [truncated 713 characters] ... "/opt/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include" 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 |
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 791d8faa02
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
| http_port: ${FLUENT_BIT_HTTP_MONITORING_PORT} | ||
| parsers_file: ${PARSERS_FILE_TEST} |
There was a problem hiding this comment.
I'm thinking we can start using the defaulting options now as well?
There was a problem hiding this comment.
Not sure. This is the first time to use parsers_file in our integration tests. So, if we'll need another part of the integration test case, we'll have to implement and use the defaulting option here.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/flb_parser.c (1)
634-649:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCache the resolved timezone at parser creation.
flb_parser_tm2time_parser()and the missing-year branch inflb_parser_time_lookup()both reload and parse the zoneinfo data for every record, whileflb_parser_create_with_time_zone()only keeps the raw string. That puts blocking file I/O and TZif parsing on the parser hot path, and it means parser registration can still succeed on POSIX even when the zone file is unreadable or malformed. Resolve/parse the timezone once during creation, store the compiled state onstruct flb_parser, and reuse it from both runtime paths.Also applies to: 943-965, 1890-1905
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/flb_parser.c` around lines 634 - 649, The parser currently reparses zoneinfo on each record; modify struct flb_parser to hold a parsed/compiled timezone object (e.g., a tzif handle) and update flb_parser_create_with_time_zone() to resolve and parse the zone file once (failing creation if parsing fails), storing the compiled state on the flb_parser; then change flb_parser_tm2time_parser() and the missing-year branch in flb_parser_time_lookup() to use that stored tzif object instead of calling tzif_load/tzif_tm2time each time, and ensure you free/destroy the cached tzif in the parser destructor to avoid leaks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/flb_parser.c`:
- Around line 634-649: The parser currently reparses zoneinfo on each record;
modify struct flb_parser to hold a parsed/compiled timezone object (e.g., a tzif
handle) and update flb_parser_create_with_time_zone() to resolve and parse the
zone file once (failing creation if parsing fails), storing the compiled state
on the flb_parser; then change flb_parser_tm2time_parser() and the missing-year
branch in flb_parser_time_lookup() to use that stored tzif object instead of
calling tzif_load/tzif_tm2time each time, and ensure you free/destroy the cached
tzif in the parser destructor to avoid leaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 826d9b16-036e-4139-bbd2-fd91882ccaa8
📒 Files selected for processing (2)
src/flb_parser.ctests/internal/parser.c
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/internal/parser.c
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
This is 2nd attempt of adding IANA compatible time_zone support for system time.
This work is based on #11639 which is @arnikola's work.
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:
Example config files taken from #11639 (comment):
config.conf:parsers.confIf 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.
Summary by CodeRabbit
New Features
Tests