-
Notifications
You must be signed in to change notification settings - Fork 251
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
Fix(common): format logs in correct local timezone #1032
Conversation
Logs were formatted in UTC regardless of local timezone, resulting in incorrect times. We now format the datetime with rfc3339 which includes the correct timezone.
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.
Nice, thanks @timonv! 🥳
I left some cleanup related comments
Thanks for the feedback, added a clarifying comment, cleaned up and readded dim() |
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.
This looks great, thanks Timon! Thanks for the test, the screenshot and the thorough PR description as well 😍
* Fix logs formatted in wrong timezone Logs were formatted in UTC regardless of local timezone, resulting in incorrect times. We now format the datetime with rfc3339 which includes the correct timezone. * Remove mutex * Reset Cargo.toml to main * Add back datetime dim() formatting
Description of change
Logs were formatted in UTC regardless of local timezone, resulting in incorrect times. We now format the datetime with rfc3339 which includes the correct timezone.
Testing is a bit hacky with the mutex, as per the comment, Time uses libc to get the timezone. By forcing the env var to the expected timezone we get the result we want. Originally had more than one test, hence the mutex. Decided to leave it in, as any additional test using timezones would yield crazy racing errors. Happy to remove it if needed.
Closes #834
How has this been tested? (if applicable)
Added unit tests.
Having some issues running the full test suite / local env. Should be alright but timezones.