-
Notifications
You must be signed in to change notification settings - Fork 17.4k
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: structured, leveled logging #56345
Comments
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This is a huge API surface without any real production testing (AIUI). Perhaps it might be better to land it under golang.org/x for some time? Eg, like context, xerrors changes. |
It's available under golang.org/x/exp/slog |
This comment was marked as resolved.
This comment was marked as resolved.
I love most of what this does, but I don't support its addition as it stands. Specifically, I have issues with the option to use inline key-value pairs in the log calls. I believe the attributes system alone is fine. Logging does not need the breakage that key-value args like that allow. The complexity in the documentation around Log should be a warning sign.
The suggestion was that potential problems with key-value misalignment will all be solved by vet checks. As I mentioned in this thread of the discussion, relying on vet should be viewed a warning as to potential problems with the design, not a part of the design itself. Vet should not be a catch-all, and we should do what we can to avoid requiring vet warnings to safely develop go. An accidentally deleted/missed or added/extra argument in key value logging would offset the keys and values after it. That could easily bog down a logging system trying to index all the new "keys" it is getting. It could also lead to data being exposed in a way that it should not be. I acknowledge that neither of these examples are all that likely in a well defined system, or somewhere with good practices around code reviewing etc.. But they are possible. |
@deefdragon Doesn't this concern apply to Printf as well? Is the difference the dependency on these logs by external systems..? |
Based on that the Go standard library is very often being recommended as source of idiomatic code, and this package aspires to be merged as part of it, I would like you explain to me the involvement of context package. If some object uses logger, isn't it its dependency? Shouldn't we make the dependencies explicit? Isn't this logger smuggling bad practice? If passing the logger by context is idiomatic, is *sql.DB too? Why has the logger stored context? It violates the most famous sentence from the documentation for context
Logger in context containing its own context inside ... Frankly, I'm a bit confused. |
@hherman1 The same concern does apply to printf, tho it's not as bad compared to logging. With printf, most statements are consumed as a single chunk, and only consumed locally by the programmer. Being misaligned is easy enough for a human to notice, parse, and correct. In the case of Sprintf, where it might not be consumed by the programmer, and instead be used as an argument to something, the "testing" during development that is making sure the program starts would likely catch most misalignment. Being off by one in a log is much harder to catch as it has no real impact in the program's execution. You only notice there is an issue when you have to go through your logs. |
I think I share some of @prochac 's concerns regarding context. Maybe I'm being a bit of a luddite, but recommending that the logger is passed around inside context rather than via explicit dependency injection, smells a bit funny to me. Context, from what I have always followed, is for request-scoped information, rather than dependencies. And the more clarity surfacing dependencies the better. IE just assuming the right logger is in context, and getting the default one because it's still getting some logger |
I think there are two approaches here:
I've used both patterns frequently in high-scale production services; and both have their places. I'd definitely like to see slog promote context-propagated logging as the observability benefits are huge. |
Appreciate your explanation @v3n . I'm still having a slightly hard time understanding the benefit of the logger itself being passed around in context. I understand propagating the log correlation information via context, and we currently use the otelzap implementation that does this sort of thing via ErrorContext(ctx, ...) etc logging methods. I like the WithContext methods proposed here, passing the context to the logger, in similar fashion. It's more the logger itself being passed around inside the context that feels a bit odd to me The zap and otelzap libraries do allow for the same kind of thing, whereby you can get the logger from context etc (and I'm sure others do), it's just this being in the std library it's more of a recommendation for this kind of pattern |
I still want a standard handler for |
@deefdragon, we'll have a vet check for that. |
@seankhliao, such a handler seems easy to write, and it's not clear to me yet whether there is enough demand to include it. Let's hold off for now; we can always add it later. |
@prochac, that is a design principle, not a hard-and-fast rule. It is there to steer people away from buggy code, but that has to be weighed against other factors. In this case, we knew that passing tracing information to logging was an important feature, but we didn't want to add a context argument to every log output method. This was our solution. |
@mminklet, scoping a logger to a request is a common pattern, and is probably the main application of the ability to add a Logger to a context. It doesn't preclude dependency injection; if that works for you, stick with it. |
This is a significant proposal. @jba can you do a video talk on this. And, perhaps, a blog post? |
@jba As I said in my original post, I don't think that's a good solution.
|
I like this in general. One API nit from an experiment in s3-upload-proxy: it would be good to have a way to convert a string into to the level (say you want to allow users set an environment variable like LOG_LEVEL=debug and have that translated to Other libraries (logrus, zerolog, zap) call that function |
|
The additional '+'/'-' terms put a twist on this, I think it'd be nice to have. (I had this laying around: https://go.dev/play/p/Izwzgd8Kmc9) |
Three comments on the proposal: One thing which irritated me with zap was the existence of both sugared and attribute log methods. My second observation is that we have 10 Attr constructors, one for each common type we want to log, + any. Finally, I think it is very good (but perhaps overdue) that we are moving towards a canonical production strength logging library in stdlib. Most libraries need some level of logging, if only for debugging. And not having a standard interface of |
I disagree. There is only one
The answer is, "hopefully." With the current implementation, you can only reduce the API surface at a considerable time penalty. But that may change before this API is frozen. See this item in the discussion.
According to my analysis, |
This proposal has been added to the active column of the proposals project |
I'm with @prochac and @mminklet in that passing logger in context seems awkward. However, I see your point about context propagation and @v3n point:
But then @jba (apologies if this was already discussed in #54763, I tried to & failed to find it there) did you consider something like context-propagated log-context, as opposed to propagating the whole logger? I frequently wish I could do easily is adding log-context to an already existing logger, like: logger = slow.WithContext(ctx).With("method", r.Method ...)
.....
err := doSomething(c ctx) err {
doSomethingElse(c ctx) {
somethingVeryDeep(c ctx) {
// do some other work
slog.AddToContext(ctx, "workResult", ok)
}
somethingVeryDeepAgain(c ctx) {
// do some work
slog.AddToContext(ctx, "otherWorkResult", ok)
}
} ()
} ()
if err != nil {
logger.Error("requestFailed") // `slog` extract values for me.
} This would allow me to log once per request/error instead of tens of scattered log-lines which I then need to correlate with request/span ids. I think this could also support https://go-review.googlesource.com/c/proposal/+/444415/5/design/56345-structured-logging.md#754 ctx, span := tracer.Start(ctx, name, opts) since
|
There may be a hint of namespace clobbring in ReplaceAttr, where matching on a key string is invoked before group prefixes are applied (example: https://go.dev/play/p/yFNXLz3gklD). I think it's a reasonable behavior; the version of A version of |
Add a package for testing that a slog.Handler implementation satisfies that interface's documented requirements. Code copied from x/exp/slog/slogtest. Updates golang#56345. Change-Id: I89e94d93bfbe58e3c524758f7ac3c3fba2a2ea96 Reviewed-on: https://go-review.googlesource.com/c/go/+/487895 TryBot-Result: Gopher Robot <[email protected]> Run-TryBot: Jonathan Amsterdam <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
The slog structured logging package. This code was copied from the slog directory of the x/exp repo at commit 642cacee5cc05231f45555a333d07f1005ffc287, with the following changes: - Change import paths. - Delete unused files list.go, list_test.go. - Rename example_depth_test.go to example_wrap_test.go and adjust example output. - Change the tag safe_values to safe_slog_values. - Make captureHandler goroutine-safe to fix a race condition in benchmarks. - Other small changes as suggested in review comments. Also, add dependencies to go/build/deps_test.go. Also, add new API for the API checker. Updates golang#56345. Change-Id: Id8d720967571ced5c5f32c84a8dd9584943cd7df Reviewed-on: https://go-review.googlesource.com/c/go/+/477295 TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]> Run-TryBot: Jonathan Amsterdam <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
Use the new functions in 1.20 (unsafe.StringData, etc.) instead of StringHeader and StructHeader from the reflect package. Updates golang#56345. Change-Id: I84d0db7b203aeffe45ce8b06beb7b4ee17e19949 Reviewed-on: https://go-review.googlesource.com/c/go/+/478055 TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Alan Donovan <[email protected]> Run-TryBot: Jonathan Amsterdam <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]>
Make it clear that "qualified by the given name" doesn't imply any particular implementation, but depends on how the Handler treats groups. Updates golang#56345. Change-Id: Idf52553108bc1a7d2091a06cc4fc4cc0fc52cc14 Reviewed-on: https://go-review.googlesource.com/c/go/+/478056 Reviewed-by: Alan Donovan <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Run-TryBot: Jonathan Amsterdam <[email protected]>
- Remove the norace_test.go files, moving their contents elsewhere. - Rename the internal/testutil package to internal/slogtest. - Remove value_unsafe.go, moving its contents to value.go. Updates golang#56345. Change-Id: I2a24ace5aea47f7a3067cd671f606c4fb279d744 Reviewed-on: https://go-review.googlesource.com/c/go/+/478197 Run-TryBot: Jonathan Amsterdam <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]>
Bench log file is created non-portably, only works on system where "/tmp" existed and "/" is path separator. Fixing this by using portable methods from std lib. Updates golang#56345 Change-Id: I1f6b6b97b913ca56a6053beca7025652618ecbf3 Reviewed-on: https://go-review.googlesource.com/c/go/+/478355 Run-TryBot: Ian Lance Taylor <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]> Auto-Submit: Ian Lance Taylor <[email protected]> Reviewed-by: Jonathan Amsterdam <[email protected]> Run-TryBot: Cuong Manh Le <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
Delete the set of bytes that need quoting in TextHandler, because it is almost identical to the set for JSON. Use JSONHandler's safeSet with a few exceptions. Updates golang#56345. Change-Id: Iff6d309c067affef2e5ecfcebd6e1bb8f00f95b9 Reviewed-on: https://go-review.googlesource.com/c/go/+/478198 Reviewed-by: Ian Lance Taylor <[email protected]> Run-TryBot: Jonathan Amsterdam <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
Give an example illustrating the problem with dots inside groups or keys. Clarify that to fix it in general, you need to do more than escape the keys, since that won't distinguish the group "a.b" from the two groups "a" and "b". Updates golang#56345. Change-Id: Ide301899c548d50b0a1f18e93e93d6e11ad485cf Reviewed-on: https://go-review.googlesource.com/c/go/+/478199 Reviewed-by: Alan Donovan <[email protected]> Run-TryBot: Jonathan Amsterdam <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
Add a suite of benchmarks for the LogAttrs method, which is intended to be fast. Updates golang#56345. Change-Id: If43f9f250bd588247c539bed87f81be7f5428c6d Reviewed-on: https://go-review.googlesource.com/c/go/+/478200 TryBot-Result: Gopher Robot <[email protected]> Run-TryBot: Jonathan Amsterdam <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
Format Group values like a []Attr, rather than a *Attr. Also, use fmt.Append in Value.append. Updates golang#56345. Change-Id: I9db1a8ec47f8e99c1ac3225d78e152013116bff3 Reviewed-on: https://go-review.googlesource.com/c/go/+/479515 Run-TryBot: Jonathan Amsterdam <[email protected]> Reviewed-by: Alan Donovan <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
Comparing two Values with == is sensitive to the internal representation of Values, and may not correspond to equality on the Go values they represent. For example, StringValue("X") != StringValue(strings.ToUpper("x")) because Go ends up doing a pointer comparison on the data stored in the Values. So make Values non-comparable by adding a non-comparable field. Updates golang#56345. Change-Id: Ieedbf454e631cda10bc6fcf470b57d3f1d2182cc Reviewed-on: https://go-review.googlesource.com/c/go/+/479516 Run-TryBot: Jonathan Amsterdam <[email protected]> Reviewed-by: Alan Donovan <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
Add a package for testing that a slog.Handler implementation satisfies that interface's documented requirements. Code copied from x/exp/slog/slogtest. Updates golang#56345. Change-Id: I89e94d93bfbe58e3c524758f7ac3c3fba2a2ea96 Reviewed-on: https://go-review.googlesource.com/c/go/+/487895 TryBot-Result: Gopher Robot <[email protected]> Run-TryBot: Jonathan Amsterdam <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
It seems that |
They were very controversial. I don't see that changing. |
https://pkg.go.dev/github.com/go-logr/logr has
Storing and retrieving a |
github.com/veqryn/slog-context has support for two different workflows for using slog and context: storing and retrieving the logger from Context and/or storing and retrieving attributes and values from Context. (These workflows can be used individually or together at the same time.) Attributes Extracted from Context Workflow:Using the In that same workflow, the Logger in Context Workflow:Using |
This only works when the code "that you don't control" is careful about always passing the context into any log calls that it invokes, right? So it won't work for e.g.
This won't be interoperable with Kubernetes packages, which use go-logr/logr for this (wrapped by klog, but that's an implementation detail). My point is: without a convention that is followed by all reusable Go packages, these mechanisms work in scenarios where one controls the entire code base, but not when combining code from different sources. |
Maybe it could be worth it adding to |
@antichris: We had considered that and decided against it at the time because it's just syntactic sugar and not necessarily a good solution because of that additional allocation. That constructing a But perhaps that's okay. I think it's fine to simply use the I'm not sure whether this issue is the right place to discuss this, though. I've created go-logr/logr#234 for further discussion and tracking. |
in the (initial proposal) it was part of the plan but it could cause some performance issue. There should be a way to consider correlation like, like |
I did a benchmark here on zax and it shows storing fields instead of the whole logger object gives better performance :
|
Regarding performance: this depends on usage patterns. If all call chains have additional values, then extracting those is going to be faster. If you have many different values and each call chain only has one of those values, then checking for all of them on each log call is probably slower. |
@pohly and I talked a bit and agreed to coordinate our repos. You can use
|
Hello, everyone! I've been trying out slog and hit some bumps with how the API is designed. Essentially, for my use case I need to check a certain log level and dispatch an alert, and still log normally. A middleware for the default handler basically. Should be something very straight forward, but in the current state, there's no (proper) way to wrap the default handler. The handlers that slog allows to be constructed don't log in the pretty ProblemI tried a different approach but hit another roadblock. type WrapperHandler struct {
slog.Handler
}
func (w *WrapperHandler) Handle(ctx context.Context, record slog.Record) error {
doSomething()
return w.Handler.Handle(ctx, record)
}
func main() {
wrap := &WrapperHandler{slog.Default().Handler()}
slog.SetDefault(slog.New(wrap))
slog.Info("TEST") // <-- Deadlock here!
} From what I've gathered, it seems to be because of what is being done in func SetDefault(l *Logger) {
// ...
// This bit here
if _, ok := l.Handler().(*defaultHandler); !ok {
capturePC := log.Flags()&(log.Lshortfile|log.Llongfile) != 0
log.SetOutput(&handlerWriter{l.Handler(), LevelInfo, capturePC})
log.SetFlags(0) // we want just the log message, no time or location
}
} If I got this right, this was added for compatibility with log but... it smells like bad practice to me. IMO this should NOT be done by SetDefault (a separate function for that), or WorkaroundIf I were to do the following, which feels like a very very bad thing to do, it now works. type WrapperHandler struct {
slog.Handler
}
func (w *WrapperHandler) Handle(ctx context.Context, record slog.Record) error {
doSomething()
return w.Handler.Handle(ctx, record)
}
func main() {
wrap := &WrapperHandler(slog.Default().Handler())
*slog.Default() = *slog.New(wrap) // Bypass SetDefault using pointer hacks (not thread-safe obviously)
slog.Info("TEST") // Works just fine
} RemarksI'm sorry if these things have been answered already, or if I'm overlooking something, I read the entire documentation and looked through the thread and didn't find this being mentioned. I can't really wrap (pun intended) my head around why extending/wrapping the default logger is so obtuse. I'm aware I could build my own logging package around slog, or use one of the great existing ones, but considering how feature complete slog looks I would rather stick with it as much as possible. Edit: After some more digging I found this to be related to #62418 and #61892 |
We propose a new package providing structured logging with levels. Structured logging adds key-value pairs to a human-readable output message to enable fast, accurate processing of large amounts of log data.
See the design doc for details.
The text was updated successfully, but these errors were encountered: