Skip to content

promslog: Use the default timezone (again) #739

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

Merged
merged 1 commit into from
Apr 10, 2025
Merged

promslog: Use the default timezone (again) #739

merged 1 commit into from
Apr 10, 2025

Conversation

beorn7
Copy link
Member

@beorn7 beorn7 commented Dec 5, 2024

This reverts commit 145b50a.

See discussion on Slack. We should first decide which log format we want before changing it from what is already released in prometheus/prometheus.

UPDATE:

This is the new commit description:

As per the dev-summit discussion, we want to use the default timezone
as selected by slog after all rather than a hardcoded UTC.

(This was accidentally introduced earlier when moving from gokit to
slog, but then switched back to hardcoded UTC in PR #735.)

Environments that have configured UTC anyway won't see any difference,
but those with other timezones configured will see the configured
timezone for the timestamps in their logs. This is in line with how
slog behaves by default.

Example timestamp before this change and also after this change
with UTC configured in the environment via a suitable way (e.g.
setting TZ=UTC on Linux):

time=2025-04-10T12:00:38.179Z

Example timestamp after this change with location Europe/Berlin,
i.e. timezone is CET or CEST:

time=2025-04-10T14:00:03.120+02:00

Note that the precise delta to UTC is in the timestamp. So this change
will also be transparent to the usual logs processing tools.

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

There is no need to revert this. This is a bug fix for the previous release where we did not correctly make timestamps UTC.

@beorn7
Copy link
Member Author

beorn7 commented Dec 5, 2024

It's a feature, not a bug… :)

But even if we come to the conclusion in the end that it is a bug fix, let's first come to that conclusion before we start cutting actual releases with the changed log format.

See also https://docs.google.com/document/d/1uurQCi5iVufhYHGlBZ8mJMK_freDFKPG0iYBQqJ9fvA/edit?pli=1&tab=t.0#bookmark=id.l370cy8m6ef4 .

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Approved at 2025-03-31 Dev Summit

@SuperQ
Copy link
Member

SuperQ commented Mar 31, 2025

I guess we need to do a bit of a rebase revert.

@beorn7
Copy link
Member Author

beorn7 commented Apr 10, 2025

Yeah, other things happened with fine tuning the timestamp format. I'll push a new commit shortly.

As per the dev-summit discussion, we want to use the default timezone
as selected by slog after all rather than a hardcoded UTC.

(This was accidentally introduced earlier when moving from gokit to
slog, but then switched back to hardcoded UTC in PR #735.)

Environments that have configured UTC anyway won't see any difference,
but those with other timezones configured will see the configured
timezone for the timestamps in their logs. This is in line with how
slog behaves by default.

Example timestamp _before_ this change and also _after_ this change
with UTC configured in the environment via a suitable way (e.g.
setting `TZ=UTC` on Linux):

    time=2025-04-10T12:00:38.179Z

Example timestamp _after_ this change with location Europe/Berlin,
i.e. timezone is CET or CEST:

    time=2025-04-10T14:00:03.120+02:00

Note that the precise delta to UTC is in the timestamp. So this change
will also be transparent to the usual logs processing tools.

Signed-off-by: beorn7 <[email protected]>
@beorn7 beorn7 changed the title Revert "fix(promslog): always use UTC for time (#735)" promslog: Use the default timezone (again) Apr 10, 2025
@beorn7
Copy link
Member Author

beorn7 commented Apr 10, 2025

Here we are. @SuperQ PHAL.

@beorn7 beorn7 merged commit 8de85c2 into main Apr 10, 2025
6 checks passed
@beorn7 beorn7 deleted the beorn7/log branch April 10, 2025 12:24
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.

2 participants