chore!: adopt log/slog, drop go-kit/log #4089
Conversation
The bulk of this change set was automated by the following script which is being used to aid in converting the various exporters/projects to use slog: https://gist.github.com/tjhop/49f96fb7ebbe55b12deee0b0312d8434 This commit includes several changes: - bump exporter-tookit to v0.13.1 for log/slog support - updates golangci-lint deprecated configs - enables sloglint linter - removes old go-kit/log linter configs - introduce some `if logger == nil { $newLogger }` additions to prevent nil references - converts cluster membership config to use a stdlib compatible slog adapter, rather than creating a custom io.Writer for use as the membership `logOutput` config Signed-off-by: TJ Hoplock <[email protected]>
|
cc: @SuperQ |
|
Not sure why the lint stage took so long, but it failed due to a timeout: I don't run into any lint issue locally -- don't know how much caching we do for golangci-lint, but maybe a rerun will work 🤞 |
|
Similarly, I don't run into this test issue locally, either 🤔 time=2024-10-31T03:41:02.745Z level=DEBUG source=dispatch.go:165 msg="Received alert" component=dispatcher alert=test1[5ae90ff][active]
time=2024-10-31T03:41:03.746Z level=DEBUG source=dispatch.go:531 msg=flushing component=dispatcher aggrGroup="{}:{alertname=\"test1\"}" alerts=[test1[5ae90ff][active]]
time=2024-10-31T03:41:03.747Z level=DEBUG source=notify.go:877 msg="Notify success" component=dispatcher receiver=default integration=webhook[0] aggrGroup="{}:{alertname=\"test1\"}" attempts=1 duration=897.598µs alerts=[test1[5ae90ff][active]]
cli_test.go:80:
collector "webhook":
interval [1,2]
---
- &{map[] 0001-01-01T00:00:00.000Z <nil> [] 0001-01-01T00:00:01.000Z <nil> <nil> { map[alertname:test1]}}[-9.223372036854776e+09:]
[ ✓ ]
Expected total of 1 alerts, got 2
received:
@ 1.019406235
- &{map[] 0001-01-01T00:00:00.000Z <nil> [] 2024-10-31T03:41:03.728Z <nil> <nil> { map[alertname:test1]}}[0.99986554:]
@ 2.087403109
- &{map[] 2024-10-31T03:41:04.728Z <nil> [] 2024-10-31T03:41:03.728Z <nil> <nil> { map[alertname:test1]}}[0.99986554:1.99986554]
FAIL
FAIL github.com/prometheus/alertmanager/test/cli/acceptance 2.814s |
|
Looks like flaky tests. Re-ran and everything is green now. |
| @@ -75,7 +74,8 @@ func (n *Notifier) Notify(ctx context.Context, as ...*types.Alert) (bool, error) | |||
| } | |||
| data := notify.GetTemplateData(ctx, n.tmpl, as, n.logger) | |||
|
|
|||
| level.Debug(n.logger).Log("incident", key) | |||
| // @tjhop: should this use `group` for the keyval like most other notify implementations? | |||
| n.logger.Debug("extracted group key", "incident", key) | |||
There was a problem hiding this comment.
We have mixed use of incident and group_key in a couple of places. It would be nice to make these consistent in a follow up PR.
There was a problem hiding this comment.
Agreed. I went with group_key in most places for sake of something to key the log message on since most of the previous calls were bare; the pushover implementation already used the incident key, so I left if for consistency in that regard.
If there's a preference what to align on, we can definitely circle back
| level.Error(n.logger).Log("err", err) | ||
| // @tjhop: should we `return false, err` here as we do in most | ||
| // other Notify() implementations? | ||
| n.logger.Error("error extracting group key", "err", err) |
There was a problem hiding this comment.
Yes I think so, this looks like an mistake!
There was a problem hiding this comment.
Ack 👍
We've been trying to keep these logging conversions relatively straightforward -- I'd vote to fix this and the other s/Info/Error/ logging calls in a separate PR
| @@ -446,7 +446,8 @@ Loop: | |||
| break Loop | |||
| case <-t.C: | |||
| if err := runMaintenance(doMaintenance); err != nil { | |||
| level.Info(s.logger).Log("msg", "Running maintenance failed", "err", err) | |||
| // @tjhop: this should probably log at error level | |||
| @@ -456,7 +457,8 @@ Loop: | |||
| return | |||
| } | |||
| if err := runMaintenance(doMaintenance); err != nil { | |||
| level.Info(s.logger).Log("msg", "Creating shutdown snapshot failed", "err", err) | |||
| // @tjhop: this should probably log at error level | |||
Signed-off-by: TJ Hoplock <[email protected]>
The bulk of this change set was automated by the following script which is being used to aid in converting the various exporters/projects to use slog:
https://gist.github.com/tjhop/49f96fb7ebbe55b12deee0b0312d8434
This commit includes several changes:
if logger == nil { $newLogger }additions to prevent nil referenceslogOutputconfig