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
WIP: slog support #196
WIP: slog support #196
Changes from 2 commits
e40bcc0
b142419
ddaa1ac
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'm having a hard time really grasping why we want this
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.
See other comments ("avoid glue code").
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.
Can we maybe do this as 2 (or more) PRs?
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.
We can definitely start with step 1.
Do you want to do that here on some experimental branch or in a separate repo? I'm leaning towards doing it here, in the main package, because I expect that we'll need that eventually. We just shouldn't merge it into "master" yet.
I'm less sure about the order of the next two steps. Solving some of these issues will be hard and take time. The end result will still be less efficient. It would be simpler to do step 3 first and enhance klog and zapr. Then Kubernetes will be ready to support packages using slog as API much sooner.
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 just saw your comment below about context handling with the key defined in an internal package. That should work, so let me take back the "in the main package": let's start in this repo with "slogr" as package?
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'm OK to skip step 2 if we decide step 3 is the better answer.
This branch seems fine, and we can commit things and then revise, as long as we don't tag a release yet.
WRT which package -
logr.ToSlog
andlogr.FromSlog
are pleasant. I think aslogr
package is better if the API is really parallel to funcr, zapr, etc -slogr.New(slog.Handler)
.Maybe step 1 is to evaluate that difference.
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.
Instead of rewriting the entire branch for this PR, I created a new one for step 1: #205
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.
It would be good if
FromSlog(ToSlog(...))
and vice versa returned the original logger. This currently works when the backend supports both APIs. But if not, we end up layering slogSink on top of slogHandler or vice versa.Can be fixed with the "underlier" pattern.
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 skipped over one detail:
logr.Logger.level
gets lost during conversion.Suppose someone does this:
Should that info message then become a debug message (info - 4 = debug in slog)? I think the answer is yes.
This can be done in the
slogHandler
or by wrapping a realslog.Handler
(modify record), but it makes the conversion code more complex becauselogr.Logger.level
needs to be restored when going back from such aslog.Handler
to alogr.Logger
(FromSlog(ToSlog(logger.V(4))
==logger.V(4)
).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.
logr docs say:
"Negative V-levels have the same meaning as V(0)."
so you don't need to worry about this (minor point)
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.
True.
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.
At some point I want to discuss with you whether
V(-1)
should be supported, though.It think it is a valid use case to say "I want more output when calling this code here", which is
someCode(logger.V(-1))
. Right now we only support "I want less output":someCode(logger.V(1))
. Ironically, the latter use case is the one that Jordan was concerned about because when seeinglogger.V(5).Info
insomeCode
it's not obvious that running the binary with-v=5
won't produce that output. That's not a problem withsomeCode(logger.V(-1))
and it was a use case that scheduler folks wanted ("produce more output when scheduling a specific pod").That slog supports "more important than INFO" (=
slog.LevelWarn
= 4) and "less important than INFO" (=slog.LevelDebug
= -4) while logr only supports "less important than INFO (=V(4)
) is another aspect to consider. I understand that this was intentional, but it keeps coming up.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 have not heard that argument, and I am skeptical of it.
Every package is the center of their own universe, and thinks they know what's important. logr V(0) means "always log", unless somebody upstream from you has decided that you are less important than you think you are. I'm not sure we should be giving callsites the ability to override that.
I don't object to leaving this, other than "less is more", but simply negating the level seems correct-ish anyway. If you somehow had a V(-1) logr call it would become INFO+1 in slog.
Anyway, that's orthogonal to this PR. I'd prefer we strip this one of anything speculative, get the bare basics merged, and then see how to do better
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. I got carried away when you mentioned negative levels 😅
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 was thinking about adding slog support to funcr, but decided against it because:
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.
However, klog and zapr definitely should get extended. I've started doing that for zapr, using the same interface for conversion that I am proposing here.
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.
For zapr see go-logr/zapr#60
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 object to impl libraries supporting both logr and slog, but I don't know why it's logr's responsibility to participate.
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.
Because
logr.ToSlog
andlog.FromSlog
are using it to avoid glue code.slogr.New
andslogr.NewHandler
(your PR) don't, but I think they should if we go with that API.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.
my impl chose to store the Logger rather than the LogSink. I'm not sure it's better, but I am curious why you chose Sink? I chose Logger because it felt easier. (FWIW, my impl of logr->slog first held a slog.Logger, but apparently that's not what slog wants us to do).
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.
Mostly for symmetry: we are layering some high-level API on top of some low-level API. Doing the same here with
LogSink
as inslogSink
withslog.Handler
seemed appropriate.Is it really? Using
l.sink.Enabled/Info/Error
below is trivial and avoids calling code that doesn't add any value.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.
logr.Logger is not a no-op, though.
Similarly, Logger.WithCallDepth() does the LogSink interface test.
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.
True, I need to handle a nil sink. I wasn't trying to do anything with call stack unwinding, so I didn't need that.
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.
In all of these, ctx is unusued, so should be named
_
?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.
Okay.
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.
We need to document somewhere that the passed-in timestamp is ignored, and any
logr.LogSink
which logs the time is going to produce its own timestamp.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.
The README has a list of drawbacks that come from using glue code for "logr on slog" - this is another one which I need to add.
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.
Somewhere we need to handle caller info. My PR gets it right by hardcoding a depth, which I don't think slog will want to guarantee.
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 we need to call Callers() again and walk back until I find the PC they gave us, and add that many frames to the logr depth?
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.
Only if the code using the handler is using
slog.Logger
. We can't assume that, the code might also use something else with different offsets.Urgh. That probably works in most usages, but not when the record was produced in one goroutine and then gets logged somewhere else entirely.
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.
#201
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 see now what this is, and I am less enamored with it. I thought it was to accumulate Attrs into a "pseudo-struct" which would then fold into the parent Handler, but I see that's not the case.
e.g.
But that's not it. then I thought it would at least assemble a struct and log it as such:
But that doesn't seem right either, unless this adapter caches all the
WithAttrs
and then sends them all to logr when it is actually logged (which will break things like funcr's hooks).So a key-prefix seems about as good as we can get. That said, using a period as the delimiter means we get JSON like
{ "the.key1": 1, "the.key2": 2}
which is strictly VALID but seems to break jq:I didn't find any other "special" character that doesn't also break jq, so either we use something like underscore or we just document this oddity. Or we let users pass in the group-delimiter string.
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.
That's indeed what the slog.JSONLogger does:
=>
When wrapping zapr (the current one without slog support), I get instead:
With slog support there is at least a chance to implement
WithGroup
properly - but I haven't tried that yet (open TODO).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.
Once I found https://github.com/uber-go/zap/blob/v1.25.0/field.go#L334 it was trivial.
Now I get:
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.
How does that intersect with WithValues() ?
E.g.:
The only options I see are
Live with output
"g.b": 2, "g.c": 3
don't use logrHandler under slog, use native slog support
accumulate calls to WithAttr(), so it becomes:
I'm interested in this approach, but I think we can accept (0) until we do that. Fair?
#199
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.
Yes.