-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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
log/slog: enable setting level on default log.Logger #62418
Comments
CC @jba |
Here's something (gnarly): https://go.dev/play/p/h29CkeqMUOt The idea is to pass in
This doesn't preserve the |
What if the default handler used a
Would that work? |
I think so, it would work like this? Invoking // must configure both slog HandlerOptions and the log package to emit source location
h := slog.NewJSONHandler(w, &slog.HandlerOptions{
AddSource: true,
})
log.SetFlags(log.Llongfile)
// configure log package calls to emit at ERROR
slog.DefaultLevel.Set(slog.LevelError)
// install the new Handler
slog.SetDefault(slog.New(h)) Without invoking // configure just the log package to emit source location
log.SetFlags(log.Llongfile)
// configure log package calls to emit at ERROR
slog.DefaultLevel.Set(slog.LevelError)
// configure `log` output writer
log.SetOutput(w) I wonder if an example including |
From my point of view, I'd suggest adding a new API like Neither a global variable |
Change https://go.dev/cl/525096 mentions this issue: |
I've drafted a CL for this, check out if it's OK? |
Holding the CL until this proposal gets accepted. |
I think that would work, but a better name might be Examples:
l := slog.New(slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{
Level: slog.LevelInfo,
}))
slog.DefaultLogLoggerLevel.Set(slog.LevelError)
slog.SetDefault(l)
slog.DefaultLogLoggerLevel.Set(slog.LevelError) // Would require that `handlerWriter.level` is a `Leveler` rather than `Level` for this to take any effect without calling `slog.SetDefault()`
// slog.SetDefault(slog.Default()) // deadlock? Edit: On second thought, I think my second example is conflating setting the default slog handler level with the log logger's level.
slog.DefaultLogLoggerLevel.Set(slog.LevelError) This indicates to me that both |
This would work for my use case, but I don't believe it would solve setting the default handler's level, which I think makes sense to solve with a shared solution since they are related. I also think it might be a bit confusing that the |
I share this sentiment - "Default" might be confusing here. I agree with every reason for |
Since level is an attribute of |
@tiaguinho |
@AndrewHarrisSPU this is not fixed in func (h *commonHandler) enabled(l Level) bool {
minLevel := LevelInfo
if h.opts.Level != nil {
minLevel = h.opts.Level.Level()
}
return l >= minLevel
} The problem is with func (*defaultHandler) Enabled(_ context.Context, l Level) bool {
return l >= LevelInfo
} If this is changed to the same implementation in |
In both implementations the comparison is log.SetOutput(&handlerWriter{l.Handler(), LevelInfo, capturePC}) So there's no way with any honest handler implementation to adjust the level of stuff coming from |
Just an opinion from a
To draw a contrast, I think this proposal identifies something that is currently static and unavailable to configure. Offering a solution maybe shouldn't be read as endorsing global state, it's just that any solution must be global. |
@tiaguinho Two principles of slog's design are:
Although a lot of people have asked for these, it seems that the current design works well, so we don't have plans to change it. |
Thanks to all the participants here for thinking a lot of this through. The situation is confusing, because there are two directions to the slog-log "bridge," as @AndrewHarrisSPU calls it.
In both cases there is a hard-coded One other point: these two cases are (normally) mutually exclusive: your program has either called
and hold on to it after you call
produces the output
So I don't think that's a situation worth worrying about. To summarize:
That is why I suggested a single global Contrast that with adding a So a global |
@jba Thanks for the write up. I'm in favor of a single global |
@panjf2000 I realize I didn't address two of your concerns with a global
The
Again, the current value is the one that is in use. Given my answers here and my rationale above, are you okay with the global? |
Thanks for the clarification! Only one leftover for me, the case 1 you referred to in the previous comment, it's still not so clear to me how the global variable is going to steer the default logger of |
@panjf2000 instead of the current code
it will read from the global:
|
What should we call this global |
@panjf2000 maybe I didn't understand your question. But note that before SetDefault is called, there is no meaningful way to talk about the level of a log.Logger. See https://go.dev/play/p/kV4HauAYkLq:
outputs
The only place that a level matters is the minimum level of |
I like |
Yes, I was aware of that, and that's also my confusion because you said " |
The current doc for
The current doc for I think an example for |
I see. What if there's a use case where you want to know what the current level is without setting it? Not a use case I necessarily care about, just thinking about implications for other users of the API. Admittedly, your solution is a bit more concise: slog.SetLogLoggerLevel(slog.LevelError) vs slog.LogLoggerLevel().Set(slog.LevelError) |
I agree, I think |
So we now expose two new APIs for this? |
On the other hand, |
I think we can make SetLogLoggerLevel return the previous level when the input level is one of LevelDebug, LevelInfo, LevelWarn, and LevelError, otherwise, it does nothing and just returns the current level, just like runtime.GOMAXPROCS. |
This proposal has been added to the active column of the proposals project |
Have all remaining concerns about this proposal been addressed? Add the following to log/slog:
An implementation is in https://go.dev/cl/525096 |
Yes, that works for me! |
I thought we would change the global variable to a function, according to what @jba suggested. Also, I got no responses to my follow-up comment. |
Yes, it's going to be a function.
@panjf2000, runtime.GOMAXPROCS benefits from the fact that some arguments make no sense. Any integer can be a level, so we can't do the same thing. |
No change in consensus, so accepted. 🎉 // SetLogLoggerLevel controls the level for the bridge to the [log] package. |
The doc in the acceptance comment is a bit garbled. The proposed API is a function, not a global variable. // SetLogLoggerLevel controls the level for the bridge to the [log] package. |
I find TextHandler awkward to work with: h := slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{
Level: slog.LevelDebug,
})
slog.SetDefault(slog.New(h)) especially if you want to modify the default format. until Go 1.22 hits, a better option for me was just implementing Handler: package slog
import (
"context"
"fmt"
"log/slog"
)
type Handler struct { Level slog.Level }
func (Handler) WithAttrs([]slog.Attr) slog.Handler { return nil }
func (Handler) WithGroup(string) slog.Handler { return nil }
func (h Handler) Enabled(_ context.Context, l slog.Level) bool { return l >= h.Level }
func (Handler) Handle(_ context.Context, r slog.Record) error {
fmt.Print(r.Message)
r.Attrs(func(a slog.Attr) bool {
fmt.Print(" ", a)
return true
})
fmt.Println()
return nil
} note the entire interface has to be implemented because of #61892. then usage is more ergonomic: h := Handler{slog.LevelDebug}
slog.SetDefault(slog.New(h)) |
When slog.SetDefault has been changed and log.Fatal is used, the call depth was off by one. Fixes golang#62418
When slog.SetDefault has been changed and log.Fatal is used, the call depth was off by one. The handleWriter within slog expect the depth to be 4 levels deep. Fixes golang#62418
This makes all log functions keep a consistent call structure to be nice with the handleWriter in the slog package which expects a strict level of 4. Fixes golang#62418.
Change https://go.dev/cl/588335 mentions this issue: |
Edit: Official proposal in this comment
When
slog.SetDefault()
is run, it sets the log package's default Logger's output as well, configured to log atLevelInfo
. While this is probably a good default behavior, having a way to configure the level would be useful (such asLevelError
), especially considering thatlog.Printf()
by default logs to stderr, not stdout.While
slog.NewLogLogger()
exists, it returns a*log.Logger
, which cannot be used to update the default logger of the log package, as far as I can tell. (log.SetOuput(w io.Writer)
exists, butlog.SetDefault(*log.Logger)
does not).I propose exporting
handleWriter
to make this use case easier:Currently I can accomplish this by copying the handleWriter code, but it's a bit verbose in my opinion:
A couple more alternatives I've thought of:
slog.SetDefaultLogLogger(l, slog.LevelError)
, possibly confusing naming, but more concise than usingHandlerWriter
, not requiring computation ofcapturePC
.slog.NewLogLogger()
, explained above.slog.SetDefault()
updated to usel.Handler().Level()
ratherLevelInfo
. Would be a breaking change.slog.SetDefaultLogLoggerLevel(slog.LevelError)
, noop ifslog.SetDefault()
has not been called yet?The text was updated successfully, but these errors were encountered: