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

use log/slog for structured logging #3502

Merged
merged 1 commit into from
Jun 1, 2024
Merged

Conversation

seankhliao
Copy link
Contributor

@seankhliao seankhliao commented May 3, 2024

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.Logger interface and logrus logger 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.Handler if 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.

Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
cmd/dex/serve.go Outdated Show resolved Hide resolved
connector/atlassiancrowd/atlassiancrowd.go Outdated Show resolved Hide resolved
@seankhliao seankhliao force-pushed the slog branch 3 times, most recently from a5a5908 to 45e201f Compare May 13, 2024 11:46
connector/ldap/ldap.go Outdated Show resolved Hide resolved
Signed-off-by: Sean Liao <sean+git@liao.dev>
@seankhliao
Copy link
Contributor Author

i rebased to resolve a conflict with new additions to the google connector

@nabokihms
Copy link
Member

@sagikazarmark from my point of view, looks good enough to be merged. Anything else from your side?

Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

LGTM thanks @seankhliao

@nabokihms nabokihms merged commit 0b6a783 into dexidp:master Jun 1, 2024
9 checks passed
@seankhliao seankhliao deleted the slog branch June 1, 2024 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/new-feature Release note: Exciting New Features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce structured logging
3 participants