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

fix(promslog): always use UTC for time #735

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

tjhop
Copy link
Contributor

@tjhop tjhop commented Dec 4, 2024

@SuperQ and I have both called this out in a few places; finally remembering to fix it.

Prometheus has a UTC policy for logging because UTC is also used for metrics and so they should correlate.

Before:

=== RUN   TestDynamicLevels/slog_log_style
time=2024-12-02T13:51:08.463-05:00 level=INFO source=slog_test.go:120 msg=info hello=world
time=2024-12-02T13:51:08.463-05:00 level=DEBUG source=slog_test.go:121 msg=debug hello=world

After:

=== RUN   TestDynamicLevels/slog_log_style
time=2024-12-04T19:51:59.813Z level=INFO source=slog_test.go:120 msg=info hello=world
time=2024-12-04T19:51:59.813Z level=DEBUG source=slog_test.go:121 msg=debug hello=world

@SuperQ and I have both called this out in a few places; finally
remembering to fix it.

Prometheus has a UTC policy for logging because UTC is also used for
metrics and so they should correlate.

Before:
```
=== RUN   TestDynamicLevels/slog_log_style
time=2024-12-02T13:51:08.463-05:00 level=INFO source=slog_test.go:120 msg=info hello=world
time=2024-12-02T13:51:08.463-05:00 level=DEBUG source=slog_test.go:121 msg=debug hello=world
```

After:
```
=== RUN   TestDynamicLevels/slog_log_style
time=2024-12-04T19:51:59.813Z level=INFO source=slog_test.go:120 msg=info hello=world
time=2024-12-04T19:51:59.813Z level=DEBUG source=slog_test.go:121 msg=debug hello=world
```

Signed-off-by: TJ Hoplock <[email protected]>
@SuperQ SuperQ merged commit 145b50a into prometheus:main Dec 4, 2024
7 checks passed
@beorn7
Copy link
Member

beorn7 commented Dec 5, 2024

I don't think we should revert the behavior as it is done here.

We had a bunch of feature requests to use the timezone configured in the environment for logging. I closed them as completed once the slog transition was done.

Log format is exempt from the stability guarantees, so we are technically allowed to change it without a major version bump. But we should still only do it as an informed decision.

At the very least, we should give users an option to use the default slog behavior again.

But I would even argue that the current behavior (prior to this PR) is fine. Those people that have seen the light will configure UTC on their servers and get UTC logging. Those that set a local timezone on their server apparently want it that way, so why log in UTC?

I also don't think that using UTC consistently for time series means we have to only use UTC timestamps for logs. The reasoning is quite different for each case.

beorn7 added a commit that referenced this pull request 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.
beorn7 added a commit that referenced this pull request 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.

Signed-off-by: beorn7 <[email protected]>
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.

3 participants