use log/slog for structured logging #3502
Conversation
There was a problem hiding this comment.
Thank you @seankhliao, I really appreciate you tackling this.
I'm mostly satisfied with how things look. I made a few comments, curious what you think.
cmd/dex/serve.go
Outdated
| "go_version", runtime.Version(), | ||
| "go_os", runtime.GOOS, | ||
| "go_arch", runtime.GOARCH, |
There was a problem hiding this comment.
nit: I wonder if using slog's group feature here would make sense. As far as I understand, that basically means using dot-notated labels (ie. go.version instead of go_version).
There was a problem hiding this comment.
I changed it to use groups.
It'll be go.version in the text output, and in json "go": {"version": ...}
cmd/dex/serve.go
Outdated
| formatter.f = &logrus.TextFormatter{DisableColors: true} | ||
| slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{ | ||
| Level: level, | ||
| }) | ||
| case "json": | ||
| formatter.f = &logrus.JSONFormatter{} | ||
| slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{ | ||
| Level: level, | ||
| }) |
There was a problem hiding this comment.
Although I don't think it makes a lot of difference, this changes where logs go (stdout instead of stderr).
We should at least highlight it somewhere.
WDYT @nabokihms ?
There was a problem hiding this comment.
I changed it to stdout. I just wasn't sure which one it should have gone to.
There was a problem hiding this comment.
Can be a problem, but mentioning it in the changelog should be fine enough.
There was a problem hiding this comment.
sorry I meant I changed it back to stderr.
| if c.BaseURL == "" { | ||
| return nil, fmt.Errorf("crowd: no baseURL provided for crowd connector") | ||
| } | ||
| return &crowdConnector{Config: *c, logger: logger}, nil | ||
| return &crowdConnector{Config: *c, logger: logger.With("connector_type", "atlassiancrowd", "connector_id", id)}, nil |
There was a problem hiding this comment.
Same thing about connectors: maybe use slog.Group?
There was a problem hiding this comment.
updated to use groups too
seankhliao
force-pushed
the
slog
branch
3 times, most recently
from
a5a5908
to
45e201f
Compare
Signed-off-by: Sean Liao <sean+git@liao.dev>
Overview
Switches pkg/log and logrus for the standard library's slog logger.
What this PR does / why we need it
This is more or less a mechanical replacement of the internal
log.Loggerinterface andlogruslogger with the standard library's slog.This appears to be the general community consensus on a standard logging interface, where slog.Logger is used as the common interface, and implementations can switch out the
slog.Handlerif necessary.Connectors have their type and id added as attributes.
Log lines are lowercased for consistency.
Closes #2020
Special notes for your reviewer
If dex is used as a library, this will be a breaking change.
Log line output will be different from before.