-
-
Notifications
You must be signed in to change notification settings - Fork 620
fix: timezone handling in logging, so local timezone is always used #3952
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
base: master
Are you sure you want to change the base?
Conversation
|
For comparison, here’s how it looked before the fix: |
54075f3 to
5cd4a8c
Compare
glookka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that tests fail, what's the point in reviewing?
- {"Variable_name":"timezone","Value":"UTC"},
+ {"Variable_name":"timezone","Value":"/etc/localtime"},
|
The test may need an update because the UTC value is even more incorrect, e.g. with the latest version it works so: The local timezone has nothing to do with UTC, but But you're right. It's better to fix it first and then review all at once. |
If it wasn't modified with config values or SET GLOBAL, it should show local time zone name. If it doesn't, it's a bug. If it shows '/etc/localtime' instead of a time zone name, it's a bug too. |
5cd4a8c to
6c8bbdf
Compare
|
I see that some pretty specific code was added ( |
I'm working on improving the |
|
I see that the original issue was updated and it seems that the problem is that the code couldn't detect that the time zone was set to EST and was using UTC. At the same time '/etc/localtime' contains UTC and $TZ is not set. But |
|
As we discussed, even though these changes seem to fix the issue, the implementation should use a different approach. I also committed the updated CLT test, which you can use as a reference. It passes with the changes in this PR and fails on the master branch. Feel free to close this PR if you prefer to start a new one and not continue here. |
The fix separates logging timezone from time function timezone and improves timezone name detection:
Separated logging and time function timezones:
g_hTimeZoneLocal(for logging): Always uses the actual system local timezone, determined by temporarily unsettingTZbefore callingcctz::local_time_zone()g_hTimeZone(for time functions likeNOW(),CURTIME()): RespectsTZenvironment variable ortimezoneconfig setting if set, otherwise uses system local timezoneImproved timezone name detection:
g_sTimeZoneNameto cache explicitly set timezone namesDetermineLocalTimeZoneName()to ignoreTZwhen detecting system timezone and addedbUseSystemDirparameter to force using system directory (/usr/share/zoneinfo/) instead of Manticore'sTZDIRGetTimeZoneName()to:/etc/localtimesymlink whencctzreturns a file path or "UTC"UTC+07,UTC+05:30) when timezone name cannot be determinedAdded UTC offset calculation:
GetUTCOffsetString()function calculates UTC offset from system timeUTC+07) and half-hour offsets (e.g.,UTC+05:30)Key Changes
SetTimeZoneLocal(): Temporarily unsetsTZto get actual system timezone for logging, then restores it for time functionsDetermineLocalTimeZoneName(): Now ignoresTZand can use system directory whenbUseSystemDir=trueGetTimeZoneName(): Enhanced to detect timezone names from/etc/localtimeand provide UTC offset fallbackGetUTCOffsetString(): New helper function for UTC offset calculationLogging now always uses the local time, no matter what
TZorsearchd.timezoneis set to. Example:Commands useful for manual testing: