-
Notifications
You must be signed in to change notification settings - Fork 4
server: unify and improve logging #56
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
base: main
Are you sure you want to change the base?
Conversation
3d95e1b
to
5836d93
Compare
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.
Could you also add logging for successful authorizations through tsidp?
I'd like to have an audit log of everyone who used tsidp to successfully login to some remote service.
1e92fef
to
56d3719
Compare
56d3719
to
4f4ca34
Compare
Hi @awly Thanks for the initial detailed review. I addressed the main issues:
|
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.
please generally avoid bundling multiple unrelated changes in one PR.
it makes it harder to review and slower for you to merge.
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.
Agreed. That is typically my preference too. The README, at this early stage in the project, I think we can be a bit more flexible on.
cur := slog.SetLogLoggerLevel(slog.LevelDebug) // force debug if this option is on | ||
slog.Debug(fmt.Sprintf(format, args...)) | ||
slog.SetLogLoggerLevel(cur) |
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.
this will flip the global logger level while a line is printed.
instead, create a dedicated slog.Logger
to use in tsnet.
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.
This was part of the examples in the slog documentation. I can create a separate logger instead if we prefer to do it that way instead.
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.
I looked into this. I would prefer to keep it this way.
Rationale:
- slog uses an internal, unexported handler to format logging. No way to keep consistency with the rest of the logging.
- it's ugly but less ugly than all the logs being in two different formats
-debug-tsnet
is meant for troubleshooting and is not a hot code path.
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.
oh, this actually sets the log level for log.Print*
functions, not for slog.Debug
. I don't think it matters here at all.
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.
Maybe?
This is a work around for the situation where -log info
(default), and -debug-tsnet
is used but doesn't print anything because slog filtered out debug level logs.
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.
ah, https://pkg.go.dev/log/slog#SetLogLoggerLevel says
Before SetDefault is called, slog top-level logging functions call the default log.Logger. In that mode, SetLogLoggerLevel sets the minimum level for those calls.
so because your code never calls slog.SetDefault
, this implicitly sets the slog log level too.
8b10e83
to
5a96c09
Compare
Switch to slog for structured logging with appropriate log levels for activity within the app. - log all token api errors at WARN level - move tsnet.Server logging to new flag: -debug-tsnet - move request/response logging to new flag: -debug-all-requests - unify API error handling logic to writeHTTPError - switch funnel error to http.StatusUnauthorized - update docker image to use new logging flags - update README.md with new flags and env vars Co-authored-by: Andrew Lytvynov <[email protected]> Signed-off-by: Benson Wong <[email protected]>
5a96c09
to
c460b6b
Compare
Signed-off-by: Benson Wong <[email protected]>
This commit switches tsidp to slog and collapses HTTP error logging to a single function.
Updates #25
Log output: