-
Notifications
You must be signed in to change notification settings - Fork 192
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
refactor: add structured logging with slog
#199
Conversation
slog
slog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
service/tcp.go
Outdated
// between Go's inlining/escape analysis and varargs functions like slog.Debug. | ||
if slog.Default().Enabled(context.TODO(), slog.LevelDebug) { | ||
args = append(args, slog.String("ID", cipherID)) | ||
slog.Debug(fmt.Sprintf("TCP: %s", template), args...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI. I guess it doesn't matter for debug, but LogAttrs
is the most efficient way to log: https://pkg.go.dev/golang.org/x/exp/slog#hdr-Attrs_and_Values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Done, even if it's just for debug.
This PR keeps the status quo of logging to the default logger. This doesn't change the fact that we log in the service library. Ideally the library part doesn't log, that should be up the client, but leaving it as a TODO to rethink that in a future PR.
Tint seems to be the go-to for tinted/colored logs if we want to keep that feature, but I could revert to just plain text logging.
Screenshot: