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 has been hidden.
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
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 has been hidden.
This comment has been hidden.
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 |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment has been hidden.
This comment has been hidden.
@paluszkiewiczB -trimpath build flag |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
Hi, thanks for adding structured logging. This will eliminate many unnecessary dependencies in our code. @jba, I have a question about style. In the proposal examples, all log messages start in lowercase, while the Google style guide suggests starting a log message in uppercase. What I am talking about is:
Style guide reference: https://google.github.io/styleguide/go/decisions#error-strings
I was not able to find any other recommendations in: Are there any suggestions, or should this be an internal team decision? |
@cooldarkdryplace, it should be a team decision. |
@jba Both |
@lmittmann, I'm not opposed to exporting Any API changes require a proposal at this point. Feel free. |
@jba My initial thought was that |
It's probably a hair more efficient, though I didn't benchmark it. Call it a premature optimization. |
I would like to ask: As time goes by, the written log file will expand infinitely, and it needs to be divided according to size or time (maybe there is compression to save disk space), will log/slog or other standard library consider providing such a function in the future? Similar to https://github.com/natefinch/lumberjack |
I might miss some comment but I search and there's no mention on how best to use slog on library. The simplest thing I could think would be for library to have package level *slog.Logger variable and has some function to let user of that library to supply their own logger so by default nothing is logged when that variable is not set. But currently slog code doesn't allow nil Logger for Debug(), Info() and the rest. So I wonder why is it like that? |
@cuishuang, splitting log files is not something we would add to slog. But it looks like you could pass a |
@bangzek, slog tries not to take a position on that question, but it should allow for many alternatives. If you choose to use a package-level variable, then it would need to be made concurrency-safe anyway, so you wouldn't be able to write |
Thanks for your answer! |
I have multiple sinks that I need to send each log entry to. My first thought was to write a wrapping Handler that dispatches to multiple Handlers, but was a bit surprised by the doc for Handler.WithAttrs, which says:
Does this mean that for each sink, my wrapping handler needs to clone the attrs slice before forwarding to each of the child handler's |
@shabbyrobe i've worked on such an handler and use it in production: https://github.com/samber/slog-multi It is called "Fanout". |
Thanks @samber. It looks though like that is potentially vulnerable to the exact thing I described. In your WithAttrs function in Again, unless I'm misunderstanding that comment, you'd need to do this instead, so that users who could be using arbitrary handlers not written by yourself can be safe from unexpected side effects:
Again, it's possible I'm simply misunderstanding that piece of documentation, but if I'm not, this is going to be a widely missed detail and I wonder if it's better for handlers to not presume ownership. |
@shabbyrobe, you are correct. You should make n-1 copies of the slice. (You can pass on the original to one lucky sub-handler, provided your wrapper doesn't change it.) |
Thanks for confirming @jba. What is the rationale for this design decision? I think this is hazardous and quite likely to be used wrong in this very scenario of fanning out, which is common. We've already seen an example, and I nearly did the exact same thing. |
So it's deliberate decision to err on nil Logger instead just doing nothing? If it's deliberate decision can I ask to add do nothing Logger in the stdlib so most library writer don't need to reinvent that wheel over and over again. Whether the package-level variable needs to be made concurrency-safe or not is a decision best left to the package author. In most cases, the added complexity is unnecessary. Typically, I set the package-level log decision once at the program's beginning, as there is no need for it to be modified once it's established. |
@shabbyrobe, sorry for the delay in responding. We let the handler own the argument for the common case where the handler wants to keep track of the attributes. It can simply store them without a copy. Many other handlers will wrap a single handler, and they can just pass on the argument. A handler that calls N others needs to make N-1 copies. I will mention this in a doc I'm putting together about writing handlers. |
You can make a proposal for Go 1.22. If we do see a lot of them arising in the wild over the next six months, I'd probably be in favor. |
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: