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

Add support for custom timestamp. Closes #103 #104

Merged
merged 3 commits into from Jan 10, 2022

Conversation

binaek
Copy link
Contributor

@binaek binaek commented Dec 15, 2021

No description provided.

@hashicorp-cla
Copy link

hashicorp-cla commented Dec 15, 2021

CLA assistant check
All committers have signed the CLA.

@binaek binaek mentioned this pull request Dec 15, 2021
@evanphx
Copy link
Contributor

evanphx commented Dec 15, 2021

Is there a reason you'd want the logs in a different timezone than the system itself? That's the reason we don't do this currently, we want the logs to always reflect the time of the system.

@binaek
Copy link
Contributor Author

binaek commented Dec 15, 2021

@evanphx I put in the UTC behind a flag for precisely the reason you mentioned. Most logs need to reflect the time of the system where it's logged from.

Having said that, UTC as a time zone removes a lot of ambiguities associated with local time zones and timestamps (not even bringing DST into the picture)

However, the motivation behind this PR is that we use hclog inside a library - which runs in an application which has its own logging and can be setup to log in UTC. The logs from our library interleave with the application's log and appear in the same log destination.

@evanphx
Copy link
Contributor

evanphx commented Dec 17, 2021

@binaek Gotcha, that makes sense. I'm wondering if rather than just a flag to set UTC, we should allow for someone to pass in a function that returns the current time. That would allow them maximum flexibility to calculate the current time in whatever form they want.

@binaek
Copy link
Contributor Author

binaek commented Dec 17, 2021

@evanphx That sounds like a good approach. Let me have a dig and update the PR.

@binaek
Copy link
Contributor Author

binaek commented Dec 21, 2021

@evanphx I have updated the PR with support for a custom time function - with a default which returns time.Now()

Take a look when you get the chance.

Copy link
Contributor

@evanphx evanphx left a comment

Choose a reason for hiding this comment

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

Super minor bits, no need to create a closure here, can just use time.Now directly.

global.go Outdated Show resolved Hide resolved
intlogger.go Outdated Show resolved Hide resolved
@binaek
Copy link
Contributor Author

binaek commented Dec 21, 2021

Huh! Not sure why I put the closure in there. Let me update the PR when I am at my desk

@binaek
Copy link
Contributor Author

binaek commented Dec 22, 2021

@evanphx PR updated

@binaek binaek changed the title Add support for UTC timestamp. Closes #103 Add support for custom timestamp. Closes #103 Dec 27, 2021
@binaek binaek requested a review from evanphx December 27, 2021 08:51
@binaek
Copy link
Contributor Author

binaek commented Jan 3, 2022

@evanphx let me know if you think this requires other changes

@evanphx evanphx merged commit 0cd5096 into hashicorp:master Jan 10, 2022
@binaek binaek deleted the utc_zone branch January 14, 2022 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants