Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Time_System_Timezone properly detect daylight savings #9201

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

braydonk
Copy link
Contributor

Previously, Time_System_Timezone would not work for timezone specifications like America/Toronto or EST5EDT because I neglected to set tm_isdst to -1, which prompts mktime to detect daylight savings time on its own.

This PR addresses this issue, and fixes the test data which was previously incorrect due to the existence of this bug.

#9197


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 configuration file for the change
  • Debug log output from testing the change
  • Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • Run local packaging test showing all targets (including any new ones) build.
  • Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • Documentation required for this feature

Backporting

  • Backport to latest stable release.

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.

Previously, this code did not declare ambiguous dst. This meant `mktime`
wasn't doing what was intended; we actually want `mktime` to determine
whether DST applies or not.

Signed-off-by: braydonk <braydonk@google.com>
The example data in these tests weren't actually accurate to the desired
output. This commit refactors the tests to use correct data, and to
switch to a table-test setup to allow easily specifying new test cases
    in the future.

Signed-off-by: braydonk <braydonk@google.com>
Copy link
Collaborator

@leonardo-albertovich leonardo-albertovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a deviation from the previous behavior and even though I don't have any experience with it, this stackoverflow response seems to make a compelling argument against it.

I don't mean to block this PR, on the contrary, I want someone with more experience to take a look at it and make an educated (and hopefully conservative) choice.

I personally think we should add a new option that users have to explicitly enable to switch the historic behavior but I understand that I could be wrong and an argument could be made about "the issue of adding an option each time we change a default behavior" so I urge others to get involved.

The reason I selected "request changes" was just so there is a visual cue to anyone else reviewing this so they pay attention to the potential issue.

Edit: It was brought to my attention that this was already discussed so there's no need to hold this PR.

@edsiper edsiper added this to the Fluent Bit v3.1.6 milestone Aug 13, 2024
@edsiper edsiper merged commit 05e823d into fluent:master Aug 14, 2024
42 of 43 checks passed
@braydonk
Copy link
Contributor Author

For posterity, I'll put here what I originally would have responded.


this stackoverflow response seems to make a compelling argument against it.

I saw this post as well. It makes a good point, and also shows that in the PR description when I say "prompts mktime to detect DST itself" is a bit of a misnomer; setting tm_isdst to -1 is actually telling mktime that the DST is unknown. However I think Fluent Bit is in the exceptional case where we can truly say DST is actually unknown.

When flb_strptime parses the time format, it will assume the data given is in UTC because making the assumption at the start would break the scenario where a Z specifier is present. That means once it gets to flb_parser_tm2time we truly have no way to tell if DST should or should not be in effect for the given time.

In the previous version, the bug was that the original tm that eventually made it down the callstack is a 0 initialized struct, meaning that the tm_isdst field that is never set when there's no Z specified would always be 0. So once it got to that point, it essentially treated it as if DST was never in effect. So this really should have existed from the start, and the previous behaviour was broken. I wish I could remember why I didn't do that in the first place, because once I went back and looked at it after that issue was opened I remembered all of these details. I wish I knew why it wasn't done before.

All this to say that I think this falls under a behaviour fix rather than a behaviour change; the original intention of this feature was to treat DST status as unknown and let mktime divine it; under most circumstances it will be correct, under ambiguous circumstances you will get weird values, but that's the way it goes with most anything dealing with timezones at the hours around DST shifts.

Apologies to anyone who was affected by this bug while it was in place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants