Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: use go-logr/logr with log/slog backend #1791
refactor: use go-logr/logr with log/slog backend #1791
Changes from all commits
769b3fc
9af4e20
829c8e6
8ba4107
8513a61
1edbe52
aa4336b
56088c2
e02a73a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 feel uncertain about anywhere where we see something like
logger.V(1)
. Doesn't the call toV()
change the verbosity level? Except for in the startup logs, (as mentioned here) we want to use the level specified by whoever installed Kargo. i.e. If obtained fromlogging.LoggerFromContext
, it should already be configured to the correct verbosity.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.
As far as I understand it, based on the docs here
logr
doesn't provide named levels (and therefore an API likelogr.Debug
). So,logr.Info
is equivalent tologr.V(0).Info
at verbosity level 0.If we, for example, wanted to map
logr
levels directly toslog.Level
as defined here, theInfo
level would remain aslogr.Info
orlogr.V(0).Info
andDebug
level would becomelogr.V(4).Info
, sincelogr
negates the verbosity level to matchslog
docs.Because we customise names of the level keys here,
logr.V(1).Info()
is equivalent to aDebug
log, with verbosity 1.The call to
V()
does change the verbosity level of that particular log, but has nothing to do withslog.HandlerOptions.Level
. Ifslog.HandlerOptions.Level
is set toLevelInfo
(slog.Level(0)
), logs withV(1)
effectively won't be printed.Sorry, I'm finding this quite tricky to explain well. I tried to give some examples under the "Level names" heading in the PR description. Let me know if this message helps at all, or just makes everything more confusing.
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.
🤦♂️ You're right. Although I'm comfortable with the notion of an abstraction over the underlying logger (mainly from my Java days), having not used logr before, I wasn't at all prepared for how wildly different the interface is from what I am accustomed to. My initial reaction is that needing to remember numeric levels as I code seems painful in comparison to
.Debug()
or.Warn()
. Let me cozy up with the logr docs and try to wrap my head around why this is supposed to be better before commenting further.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.
@hiddeco pointed out that realistically, we'd use constants we defined ourselves for the levels.
I'm still left feeling that
logger.V(logging.LevelDebug).Info(...)
is all at once wordier and less expressive thanlogger.Debug(...)
.I'm totally bought into the benefits of logr, but just cannot get over the interface.
Wondering what @jessesuen and @hiddeco might think about a thin wrapper around
logr.Logger
in our existinglogging
package.logging.DefaultLogger()
andlogging.LoggerFromContext(ctx)
could be modified to return something that looks like this:To be clear... I'm not proposing an "abstraction over an abstraction." I'm proposing fully taking advantage of the logr abstraction (i.e. we can still swap logging back ends no problem), but merely enhancing their interface without obscuring access in case we sometimes need to "drop-down" to it.
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.
On one hand, I feel averse of this idea because it does not allow you to log errors at debug level, which a real
logr.Logger
implementation does allow you to do.I also think (and know based on experience), the problem can largely be worked around by using a modular approach in your application architecture. Allowing you to e.g. inject a
logr.Logger
instance at a predefined level, so that you have to think less about the level which you are logging at.On the other hand, is the question how often you really have the desire to log an error at debug level, and isn't this approach really uncommon in our landscape.
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.
Keep in mind, we wouldn't lose access to the underlying
logr.Logger
.To be clear, my concern isn't so much about obtaining a logger that logs at a certain level of verbosity. My concern is the logr interface makes it harder to express the relative importance of an individual message.
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.
And lol at that link being almost exactly what I described.
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 don't have a philosophical attachment to the principles of
go-logr
. A lot of the stuff in the go-logr FAQ is quite principled. I'm not a go-logr purist so we don't have to fully accept their interface as-is. The main use case I was concerned with was that we didn't have a way of setting a logger tosigs.k8s.io/controller-runtime/pkg/log
, which wants alogr.Logger
.I'm fine with creating the wrapper that embeds logr.Logger, especially if it maintains our code readability while also addressing the initial use case. The underlying
logr.Logger
is always available if we wish to use the lower interface.FWIW
logger.Debug(...)
is definitely preferrable overlogger.V(logging.LevelDebug).Info(...)
and so if we had to switch all our logging statements to the 2nd version, that might be reason enough to create our wrapper.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.
Thanks @jessesuen.
@bakunowski are you game for adding a wrapper like the one @hiddeco linked to in our existing
logging
package?