time: time_tz: Handle conversion rules of windows and IANA tzinfo#11903
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds public APIs and implementations for Windows↔IANA timezone name conversions and fixed standard UTC offset lookups, backed by a static mapping table with thread-safe indexed initialization and binary-search lookups, plus unit tests and build integration. ChangesTime Zone Conversion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
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 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: ef28805e1c
ℹ️ 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".
ef28805 to
fd4c248
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/flb_time_tz.c (2)
510-578: ⚖️ Poor tradeoffOptional: linear scans over ~460 entries per call.
Each conversion does an O(n) walk of the table (~460 rows). If these helpers end up on a per-record path, consider a sorted table + binary search or a small hash to reduce lookup cost. Fine to defer if calls are infrequent (e.g., config-time resolution).
🤖 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_time_tz.c` around lines 510 - 578, The current lookup functions (flb_time_windows_zone_to_iana, flb_time_iana_zone_to_windows, flb_time_windows_zone_to_utc_offset, flb_time_iana_zone_to_utc_offset) perform linear scans over the windows_iana_timezones table (~460 entries) which is O(n) per call; replace these linear scans with a faster lookup by either (a) building two lookup tables at module init (e.g., a hash map for Windows->entry and one for IANA->entry) or (b) sorting the arrays and using binary_search; implement case-insensitive key handling for Windows names (strcasecmp semantics) when populating/looking up, keep the current function signatures and return semantics, and ensure the initialization is thread-safe or performed at startup before any lookups.
20-28: ⚡ Quick winInclude the public header instead of re-declaring the prototypes.
flb_time.halready declares these four functions, but this file forward-declares them locally (Lines 25-28) and never includes the header. If a public signature changes, the compiler won't catch a mismatch between the declaration and the definition here. Include<fluent-bit/flb_time.h>and drop the local prototypes.♻️ Proposed change
`#include` <fluent-bit/flb_compat.h> +#include <fluent-bit/flb_time.h> `#include` <stddef.h> `#include` <string.h> - -const char *flb_time_windows_zone_to_iana(const char *windows_zone); -const char *flb_time_iana_zone_to_windows(const char *iana_zone); -int flb_time_windows_zone_to_utc_offset(const char *windows_zone, long *offset); -int flb_time_iana_zone_to_utc_offset(const char *iana_zone, long *offset);🤖 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_time_tz.c` around lines 20 - 28, Remove the four redundant local forward declarations for flb_time_windows_zone_to_iana, flb_time_iana_zone_to_windows, flb_time_windows_zone_to_utc_offset, and flb_time_iana_zone_to_utc_offset and instead include the public header flb_time.h (e.g. add `#include` <fluent-bit/flb_time.h>) so the compilation uses the canonical prototypes; this ensures any signature changes are caught by the compiler and avoids duplicate declarations.
🤖 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.
Nitpick comments:
In `@src/flb_time_tz.c`:
- Around line 510-578: The current lookup functions
(flb_time_windows_zone_to_iana, flb_time_iana_zone_to_windows,
flb_time_windows_zone_to_utc_offset, flb_time_iana_zone_to_utc_offset) perform
linear scans over the windows_iana_timezones table (~460 entries) which is O(n)
per call; replace these linear scans with a faster lookup by either (a) building
two lookup tables at module init (e.g., a hash map for Windows->entry and one
for IANA->entry) or (b) sorting the arrays and using binary_search; implement
case-insensitive key handling for Windows names (strcasecmp semantics) when
populating/looking up, keep the current function signatures and return
semantics, and ensure the initialization is thread-safe or performed at startup
before any lookups.
- Around line 20-28: Remove the four redundant local forward declarations for
flb_time_windows_zone_to_iana, flb_time_iana_zone_to_windows,
flb_time_windows_zone_to_utc_offset, and flb_time_iana_zone_to_utc_offset and
instead include the public header flb_time.h (e.g. add `#include`
<fluent-bit/flb_time.h>) so the compilation uses the canonical prototypes; this
ensures any signature changes are caught by the compiler and avoids duplicate
declarations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93345a33-5ed7-48d2-bfc0-238106f3ab5a
📒 Files selected for processing (4)
include/fluent-bit/flb_time.hsrc/CMakeLists.txtsrc/flb_time_tz.ctests/internal/flb_time.c
fd4c248 to
b03eb52
Compare
b03eb52 to
9657cb8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/flb_time.c (1)
446-446: ⚖️ Poor tradeoffPrefer compiling
flb_time_tz.cas its own translation unit over#include-ing a.cfile.Including a
.cfile that contains externally-visible definitions is a code smell: it couples the two units, can confuse tooling/coverage, and risks ODR/duplicate-symbol issues if the file is ever picked up by the build glob. Since these are public APIs declared inflb_time.h, the cleaner approach is to addflb_time_tz.cto the build sources and drop this include.🤖 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_time.c` at line 446, Remove the line that `#include`"s" flb_time_tz.c from flb_time.c and instead compile flb_time_tz.c as a separate translation unit by adding flb_time_tz.c to the project's build sources (e.g., the Makefile/CMakeLists/meson build list). Ensure the public APIs declared in flb_time.h and implemented in flb_time_tz.c remain unique (no duplicate definitions) and update the build so linking picks up flb_time_tz.o; do not leave any remaining `#include` of the .c file.
🤖 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.
Nitpick comments:
In `@src/flb_time.c`:
- Line 446: Remove the line that `#include`"s" flb_time_tz.c from flb_time.c and
instead compile flb_time_tz.c as a separate translation unit by adding
flb_time_tz.c to the project's build sources (e.g., the
Makefile/CMakeLists/meson build list). Ensure the public APIs declared in
flb_time.h and implemented in flb_time_tz.c remain unique (no duplicate
definitions) and update the build so linking picks up flb_time_tz.o; do not
leave any remaining `#include` of the .c file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d15c7956-9dde-40b7-a083-93c1ffaabea3
📒 Files selected for processing (4)
include/fluent-bit/flb_time.hsrc/flb_time.csrc/flb_time_tz.ctests/internal/flb_time.c
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/internal/flb_time.c
- include/fluent-bit/flb_time.h
- src/flb_time_tz.c
9657cb8 to
8ef2b40
Compare
8ef2b40 to
a932a5e
Compare
a932a5e to
423ae51
Compare
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>
423ae51 to
46dfaaf
Compare
In the current Fluent Bit code base, there is no conversion rules between windows timezone and IANA provided timezone information.
So, we need to implement this conversion rule map as built-in capability in Fluent Bit.
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.
Summary by CodeRabbit
New Features
Improvements
Tests