log/slog: structured, leveled logging

jba commented 1 year ago

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.

jba commented 1 year ago

@ainar-g, I believe the right way to deal with that deadlock is to modify the log package. That will have to wait until slog is in the standard library.

chrisguiney commented 1 year ago

I have only seen one person speak in favor of inlined key values, and many 👍 and comments discussing disapproval of inline key-value.

This concern reminds me of the discussion we had on the error values proposal about the %w format directive for fmt.Errorf. Many people felt that it was not type-safe or general enough and advocated for a function like errors.Wrap. In the end we stuck with %w over the objections, and that seems to have been the right call: it is indeed type-unsafe, but that doesn't actually seem to bother anyone in practice (and there is a vet check—sound familiar?). And it is so much easier to use than a function.

So @deefdragon, while I agree that on this issue there has been more disapproval than approval, we have to take into account the millions of Go programmers who don't follow these issues (and those who do—thanks @hherman1) who rely on Go to be lightweight and easy to read and write. Without inlined key-value pairs, the API would feel cumbersome, and many would avoid it. Structured logging is already more verbose than printf-style logging; we don't want to add to that burden. The situation is actually better than with the error proposal, because slog does provide the alternative.

We also know from the existence of other popular structured logging packages that use key-value pairs—go-kit/log, logr, hclog and Zap's SugaredLogger—that people are generally fine with them.

I find comments like these to be somewhat concerning, but also very confusing.

Is feedback actually being sought for this proposal, or is the discussion merely a formality? This comment reads like you've already made all decisions, and aren't open to changing anything as a result of community feedback. It's not clear to me how the community can contribute to this proposal, if at all.

we have to take into account the millions of Go programmers who don't follow these issues

This seems like it can be used to justify any decision.

mkungla commented 1 year ago

// Handle handles the Record.
// It will only be called if Enabled returns true.
// Handle methods that produce output should observe the following rules:
//   - If r.Time is the zero time, ignore the time.
//   - If an Attr's key is the empty string, ignore the Attr.
Handle(r Record) error

It will only be called if Enabled returns true.

That is misleading, there is no checks for Enabled on available handlers which call directly commonHandler.handle. commonHandler.handler does not have enabled check. This check is performed by Logger call chain using Logger.logDepth where enabled is checked. That can lead some undesired side effects; e.g.

slog.SetDefault(slog.New(slog.HandlerOptions{
  Level: slog.LevelWarn,
}.NewTextHandler(os.Stdout)))

slog.Info("slog.Info: that is ignored as it should")
slog.Warn("slog.Warn: that is logged as it should", "info.enabled", slog.Default().Enabled(slog.LevelInfo))
log.Print("log.Print: did not expect to see that in logs")

slog.Default().Handler().Handle(
  slog.NewRecord(
    time.Now(),
    slog.LevelInfo,
    "call.Handle: I guess thats ok if that is printed always",
    0, 
    nil,
))

time=2022-12-19T22:13:49.033+02:00 level=WARN msg="slog.Warn: that is logged as it should" info.enabled=false
time=2022-12-19T22:13:49.033+02:00 level=INFO msg="log.Print: did not expect to see that in logs"
time=2022-12-19T22:13:49.033+02:00 level=INFO msg="call.Handle: I guess thats ok if that is printed always"

Primarly it conserns log.Output set by func SetDefault(l *Logger)

// It is used to link the default log.Logger to the default slog.Logger.
func (w *handlerWriter) Write(buf []byte) (int, error) {
  ...
  r := NewRecord(time.Now(), LevelInfo, string(buf), depth, nil)
  return origLen, w.h.Handle(r)
}

So shouldn't this handlerWriter call have something like

func (w *handlerWriter) Write(buf []byte) (int, error) {
  if !w.h.Enabled(LevelInfo) {
    return 0, nil
  }
  ...
}

abh commented 1 year ago

I have only seen one person speak in favor of inlined key values, and many 👍 and comments discussing disapproval of inline key-value.

I didn't think it was a popularity contest or a vote, but I for one prefer inline key-value. Logging is a small bit of extra book keeping to track what's going on. If it's too verbose it easily overwhelms whatever logic that the program is actually meant to do.

You can add a kv(args) function as a helper if the risk of off by one errors in your logs is significant for your program.

jba commented 1 year ago

Is feedback actually being sought for this proposal, or is the discussion merely a formality? This comment reads like you've already made all decisions, and aren't open to changing anything as a result of community feedback. It's not clear to me how the community can contribute to this proposal, if at all.

@chrisguiney, even a cursory reading of this issue and the prior discussion would provide plenty of evidence to contradict your claims. To name a few things we changed or added as a result of community feedback: groups, WithContext, and level numbering (twice, or maybe three times). There were also times when we didn't think a request rose to the level of an API change, but we did add an example or some documentation.

But @abh nailed it: this is not a popularity contest or a vote. We use this feedback to make choices, but we also rely on our experience and intuition, as well as data gathered from other places, like open-source code.

jba commented 1 year ago

It will only be called if Enabled returns true.

That is misleading

@mkungla, the comment is right, but the implementation is flawed. Stay tuned for a fix.

mminklet commented 1 year ago

@jba tbf to @chrisguiney point I also get an impression that you're not addressing concerns that have been raised multiple times about passing the logger inside context. You've only described it as a 'common practice', which doesn't seem to mesh with my experience, and suggested that context-propagated logging requires the logger being passed around in the context, which I also disagree with

who rely on Go to be lightweight and easy to read and write

Passing dependencies round in an opaque container, and relying on a global default for backup is not easy to read imo. Having to fight anti patterns being introduced to the codebase is hard enough without the blessing of the std library using context as a dependency container. I think that considering it's not hard to do that yourself if that's what you prefer, and it's clearly a contentious issue, it should be left out of the std lib.

deefdragon commented 1 year ago

Is feedback actually being sought for this proposal, or is the discussion merely a formality? This comment reads like you've already made all decisions, and aren't open to changing anything as a result of community feedback. It's not clear to me how the community can contribute to this proposal, if at all.

@chrisguiney, even a cursory reading of this issue and the prior discussion would provide plenty of evidence to contradict your claims. To name a few things we changed or added as a result of community feedback: groups, WithContext, and level numbering (twice, or maybe three times). There were also times when we didn't think a request rose to the level of an API change, but we did add an example or some documentation.

But @abh nailed it: this is not a popularity contest or a vote. We use this feedback to make choices, but we also rely on our experience and intuition, as well as data gathered from other places, like open-source code.

I largely agree with what @chrisguiney brought up. The previous times I or others brought up the KV issue (here and the original discussion), I felt that the issue was largely brushed off and ignored with a very "we know better" attitude (I do acknowledge my comparative lack of experience, but that attitude is not appreciated).

Up until my original comments, there was little support in the proposal discussion for KV. My API Change Proposal brought up individuals who do want KV arguments which is great, and part of why I made that proposal. It also brought up your comments @jba about libraries that already support that API format, which is I assume, the original supporting argument for KV. Something we did not have as far as I know.

It is now obvious that there is support for including KV in some manner for ease of use reasons. And I respect that, as it does make sense for those who use that format. I still believe however, that the existing proposed API is not the correct way to go about this. One of go's core features is its type system. The existing API ignores that strength and protection.

As I mentioned at the bottom of my suggested changes, I would support (and am now making an addition to my original proposal to include) a WithKV(kvs ...any) method.

This means that calls that wish to use sugared logging are a tiny bit more complex than the existing API, but that every call to a base logging method is now type safe.

Example:

logger.Info("Some Message", 
    "foo", "Foo attribute",
    slog.Int("count",1234),
    "bat", "bat attribute",
    slog.Bool("enabled",false),
    "bar", "Bar attribute")

becomes

logger.WithKV(
    "foo", "Foo attribute",
    "bat", "bat attribute",
    "bar", "Bar attribute"
).Info("Some Message", 
    slog.Int("count",1234),
    slog.Bool("enabled",false))

While sugaring is one extra function call chain, it is still very simple. The loss of type safety and misalignment problems inherent to KV are isolated to a single function, instead of every logging function.

This would also allow for simplification of the existing KV rules. If WithKV does not accept attributes, then the length of the parameter array must be even, and a value will always be followed by another key (or nothing).

ancientlore commented 1 year ago

While sugaring is one extra function call chain, it is still very simple. The loss of type safety and misalignment problems inherent to KV are isolated to a single function, instead of every logging function.

The WithKV syntax you propose might be kind of heavy since it creates a new Logger to achieve this. I haven't studied the implementation in detail, but it has to deal with concurrency considerations as new loggers can be created and used in different goroutines, particularly for HTTP servers. I rarely want a new logger just for one call to Info.

I was initially a bit apprehensive about the paired key/value style of logging when I first saw it in gokit. But I've come to see it as quite convenient. I don't know how often people would blend the two styles in a single call (maybe for groups), but the rules are very straightforward and I like that both styles are supported.

antichris commented 1 year ago

I'm with https://github.com/golang/go/issues/56345#issuecomment-1302023793, https://github.com/golang/go/issues/56345#issuecomment-1303467620, https://github.com/golang/go/issues/56345#issuecomment-1304889194, https://github.com/golang/go/issues/56345#issuecomment-1312273752 and https://github.com/golang/go/issues/56345#issuecomment-1320459849 for renaming the <level>(string, ...any) convenience funcs to <level>f(...), and introducing <level>(string, ...Attr) as the primary, recommended API.

People who love that sloppy convenience would still be able to happily do their

l.Infof("just", "don't", "you", "go", "crosseyed", "now", "trying", "to", "get", "these", "key", "value", "pairs", "right")

footguns that they like so much, while the rest of us could just as comfortably wield the more performant <level>(string, ...Attr) as a first-class citizen to do our

l.Info("message", slog.String("it", "is"), slog.String("very", "easy"), slog.String("now", "to"), slog.String("get", "these"), slog.String("key", "value"), slog.String("pairs", "right"))

logging.

The stdlib log also has four versions of each of Print, Fatal and Panic, so it's not like this kind of API bloat was unprecedented.

Alternatively: get rid of all convenience shortcuts. If one wants convenience, they'd have to implement a wrapper for slog of their own. Either way, no one would be more privileged than everyone else.

antichris commented 1 year ago

While on the topic of convenience, I might be missing something, but, at this point, it seems to me that the use cases might be common enough to warrant the inclusion of something along the lines of

type HandleFunc = func(next slog.Handler, r slog.Record) error

type FuncHandler struct {
    Func HandleFunc
    slog.Handler
}

func (h FuncHandler) Handle(r slog.Record) error {
    if h.Func == nil {
        return h.Handler.Handle(r)
    }
    return h.Func(h.Handler, r)
}

func (h FuncHandler) WithAttrs(attrs []slog.Attr) slog.Handler {
    return FuncHandler{h.Func, h.Handler.WithAttrs(attrs)}
}

func (h FuncHandler) WithGroup(name string) slog.Handler {
    return FuncHandler{h.Func, h.Handler.WithGroup(name)}
}

This would enable applications like fetching a request ID from the context, a common requirement, judging by the comments here.

I'd love to hear if my thinking's wrong, as I'm still a bit uncertain about how exactly are all the pieces of slog designed to fit together in daily use.

Edit: here's the FuncHandler API tightened up slightly.

```go func NewFuncHandler(h Handler, f HandleFunc) Handler { if f == nil { return h } return &FuncHandler{h, f} } type HandleFunc = func(next Handler, r Record) error type FuncHandler struct { h Handler f HandleFunc } func (h *FuncHandler) Enabled(l Level) bool { return h.h.Enabled(l) } func (h *FuncHandler) Handle(r Record) error { return h.f(h.h, r) } func (h *FuncHandler) WithAttrs(attrs []Attr) Handler { return NewFuncHandler(h.h.WithAttrs(attrs), h.f) } func (h *FuncHandler) WithGroup(name string) Handler { return NewFuncHandler(h.h.WithGroup(name), h.f) } func (h *FuncHandler) Handler() Handler { return h.h } ``` Playground:

AndrewHarrisSPU commented 1 year ago

@antichris

The FuncHandler looks like a pretty plausible idea. I think the record should be cloned - a record may be forwarded to multiple middleware or backends concurrently, but IIRC the AddAttrs method touches shared backing memory.

antichris commented 1 year ago

@AndrewHarrisSPU, AFAIK, a Logger only has one Handler. If the latter does some fancy fan-out, cloning the request, I believe, should be its responsibility. So, in regard to my FuncHandler sketch, I think it would be the specific HandleFunc implementation that should take care of that — if/when it forwards the record to another Handler besides the next.

AndrewHarrisSPU commented 1 year ago

a Logger only has one Handler. If the latter does some fancy fan-out, cloning the request, I believe, should be its responsibility.

The docs are pretty explicit on this:

A Record holds information about a log event. Copies of a Record share state. Do not modify a Record after handing out a copy to it. Use Record.Clone to create a copy with no shared state.

There's no guarantee that the Handler is being fed by a slog.Logger, and there's no guarantee an earlier or later Handler isn't multiplexing or mutating a Record if this isn't followed. I don't know how paranoid we need to be about middleware, but I think there's a spectrum of techniques to achieve roughly similar effects - the details matter, but some of them don't have exactly this concurrency concern.

antichris commented 1 year ago

There's no guarantee that the Handler is being fed by a slog.Logger, and there's no guarantee an earlier or later Handler isn't multiplexing or mutating a Record if this isn't followed.

Yes. In such a scenario, we can't even hope to safely clone a Record that our Handler has been passed, as it may already be being mutated concurrently by someone else somewhere. It's all out of the hands of our Handler. It couldn't even ever have been: it is the caller's responsibility to ensure its concurrent callees don't get races in the data it passes to them. This is not a problem that would be unique to a given implementation of Handler, to slog or even Go itself.

jba commented 1 year ago

It will only be called if Enabled returns true.

That is misleading

@mkungla, the comment is right, but the implementation is flawed. Stay tuned for a fix.

@mkungla, the fix is https://go.dev/cl/459558. Let me know if you notice any similar problems.

jba commented 1 year ago

I also get an impression that you're not addressing concerns that have been raised multiple times about passing the logger inside context

@mminklet, apologies if it seems like we've been brushing aside those concerns. We've tried to avoid integrating context even more deeply by making it first argument to each logging function, as some have requested. We thought having two functions that act independently from the rest of the package was a good compromise.

You've only described it as a 'common practice', which doesn't seem to mesh with my experience

That's a fair criticism. I've been led to this pattern myself many times, and know others who have as well, but a claim like that deserves more evidence. So I grepped my corpus of about 3,000 packages that import Zap, looking for code like

context.WithValue(... log...)

I found a little over 100 packages that do that. I didn't examine them all, but here are a few I looked at:

People do seem to reinvent this wheel quite a lot, so I think we are doing a service by adding it to the package.

I particularly like the doc comment on Kochava's package:

Generally speaking this is a bad use of the context package, but utility won out over passing both a context and a logger around all the time. In particular, this is useful for passing a request-scoped logger through different http.Handler implementations...

I agree with all of that except the word "bad." I like your word better—antipattern—because it's value neutral. (Some things that start with "anti-" are good, like antidotes, antipasto, ....) The problem with these labels, though, is that they reduce engineering tradeoffs to a dichotomy. Is it problematic to have a bag of stuff that you pass around for any function to grab whatever they want out of? Sure; it introduces coupling in way that's hard to see by reading the code. But is it also problematic to plumb a logger through multiple layers of code to add a debug message so you can track down a bug? Might you risk introducing more bugs just to find the first? Logging is one of those things—there aren't many of them—where you don't need it until you really do. I think that justifies passing loggers around in contexts.

jba commented 1 year ago

ignored with a very "we know better" attitude

@deefdragon, I'm sorry if my comments ever came across that way. It was certainly never my intent.

I think the problem with WithKV is that it adds verbosity, even just a little, to something that is supposed to have none.

jba commented 1 year ago

@antichris, I like the idea of FuncHandler, but sometimes you want to implement Enabled instead of Handle. I agree that the other functions are usually boilerplate. So we'd need two of these, or maybe one that takes two functions.

I don't think this is ready to include in this proposal, but maybe later when we have more experience.

jba commented 1 year ago

I think the record should be cloned

If [a Handler] does some fancy fan-out, cloning the request, I believe, should be its responsibility

I don't know how paranoid we need to be about middleware

@AndrewHarrisSPU, I agree with @antichris here: don't clone a Record unless you both mutate it and keep the original. Otherwise you will slow down your logging unnecessarily and write more code than you need.

prochac commented 1 year ago

So it's like an endorsement of that antipattern by Go team, right? The uber/zap logger is leveraging this antipattern, and it's fast thanks to that*, and people who need a fast logger will use it anyway. Don't we want just simple man's structured logger? The right one? json package can be considered slow, but its behavior that it doesn't pollute the structure on error is neet.

*) not just, but the gain is by prebuilding the log row a head. It also leads to that it doesn't override log fileds. uber/zap duplicating log fields https://go.dev/play/p/UqbREFOCyS9 while logrus is overriding https://go.dev/play/p/Z5fRdmCtg2K

People are really catchy on things they shouldn't. Example: The pkg package https://github.com/golang-standards/project-layout/issues/10 or calling Go Golang in spoken conversation, just because of one small thing, golang.org url

I really don't want to see *sql.DB in context one day, with an argument that std lib does it too.

darkfeline commented 1 year ago

I disagree with the claim that carrying loggers in a context is an antipattern. Logging is precisely the kind of cross-cutting concern where it makes sense to carry in a context.

(The alternative would be every single library, wrapper, retry function, etc. having to pass a specific logger dependency which is infeasible. See this for more examples of this pattern.)

neild commented 1 year ago

I can say that a primary motivation for adding support for values in contexts was to carry RPC trace IDs through the call stack. Each inbound and outbound RPC has a trace ID associated with it, and the context provides a way to associate the inbound and outbound calls. Other systems inside Google mostly use thread-local storage for this purpose. Go needs to pass this information explicitly. The Context type provided a way to carry the trace ID through layers of call stack that don't directly interact with RPCs. For example, a call stack might consist of an RPC handler function, an intermediate package, a database package, and an outbound RPC call made to the database service. The intermediate package doesn't need to know that it is being called by an RPC handler or that the database package makes outbound RPCs--it just plumbs through a Context.

Using the Context to hold call-scoped logging information feels very much in the same mold, and would permit a function to pass logging configuration through intermediate packages which don't themselves log.

szmcdull commented 1 year ago

If GO supports thread-local storage, that would be very helpful.

AndrewHarrisSPU commented 1 year ago

A goofy idea: https://go.dev/play/p/psdD7KDF5fp

@jellevandenhooff made the point that merging two loggers is a bit awkward (https://github.com/golang/go/issues/56345#issuecomment-1336539184). The idea here is a lazier store for attributes than Handlers that preformat eagerly WithAttrs, WithGroup methods. In theory there could be some utility regarding what gets put on a Context. By delaying preformatting, there's some opportunity to perform a few different flavors of joining Handlers. Or, extreme late-binding, using ReplaceAttr to expand attribute values with data extracted from a Context per-Record.

08d2 commented 1 year ago

I disagree with the claim that carrying loggers in a context is an antipattern. Logging is precisely the kind of cross-cutting concern where it makes sense to carry in a context.

Contexts are for request-scoped data, not cross-cutting concerns.

mminklet commented 1 year ago

The alternative would be every single library, wrapper, retry function, etc. having to pass a specific logger dependency which is infeasible.

I don't think that's true, as evidenced by the fact most have made it this far without needing to do it.

The anti-pattern I'm talking about is having opaque dependencies being passed around (you hope) opening the door to this pattern being adopted more widely. Go is good because it's easy to read, and this makes it harder to see where things are coming from imo. I feel like this is going to create a lot of instances of people wondering why the logger isn't configured because they've assumed what they're getting back from context is correct and not the default.

The Context type provided a way to carry the trace ID through layers of call stack that don't directly interact with RPCs.

Totally, and we use that ourselves, as trace ids etc are precisely the right type of information that belongs in context.

People do seem to reinvent this wheel quite a lot, so I think we are doing a service by adding it to the package.

While I appreciate you taking the time to respond with so much detail, this is where we disagree. People are able to 'reinvent the wheel' with this quite trivially without making the decision to add it into the standard library - that isn't just 'doing a service', it's an endorsement. However, I don't feel like my arguments are making much difference, and that's fair enough, so I'll leave it here.

chrisguiney commented 1 year ago

Totally, and we use that ourselves, as trace ids etc are precisely the right type of information that belongs in context.

If a logger is annotated with request scoped key/value pairs -- an example from the proposal:

func handle(w http.ResponseWriter, r *http.Request) {
    rlogger := slog.FromContext(r.Context()).With(
        "method", r.Method,
        "url", r.URL,
        "traceID", getTraceID(r),
    )
    ctx := slog.NewContext(r.Context(), rlogger)
    // ... use slog.FromContext(ctx) ...
}

What fundamentally makes storing rlogger in a context different than storing trace ids?

In one of the services I manage, there are two logs: a request log, and a service log. The request log gets created by a middleware handler passed down via context, writing the log record after the upstream handler completes. Handlers may add messages or tags as desired. It will be written as a single json message to a rotated append only file at the end of the request.

The service log is passed as a dependency to any struct/handler/etc that may be doing asynchronous work and is not directly related to handling requests. For example, an i/o error refreshing a cache in the background. These get logged immediately to stderr.

This has been a very useful distinction, and having the request logger passed via context hasn't been an issue. It can create a hidden dependency that may need to be accounted for in other contexts (e.g., under test). In practice though, it's been fine.

mminklet commented 1 year ago

What fundamentally makes storing rlogger in a context different than storing trace ids?

To me, one is request scoped information, one is a service (I mean, not exactly but sort of). I think loggers are a dependency in the same way http/grpc/etc clients are, you could make the same argument that passing information for the http client in context is fine, so why not the client itself?

If that pattern works for you, more power to you. My concern is the misuse of it, and it comes from my experience of fighting against painful 'fancy' dependency injection implementations that have come back to bite us. I have absolutely no issue with the go community doing this should they wish, my issue is it being in the std library and the potential for that to be an endorsement for misuse of context

However, as I say, I'm not going to continue to die on this hill, people in here are far more knowledgeable about it than me and otherwise I think this logger looks brilliant. I'm just a bit wary of Go losing it's readability.

antichris commented 1 year ago

I think a great deal of confusion here, at least in part, arises from the design choices of the context-/per-request-scoped record-supplementing attribute collection (keep this in mind)

being named Logger,
including the Handler, which is the actual, active logging service component, and
having methods that form the Records to be passed to that service.

Some have suggested passing the Attrs directly in the context. But that would require either (potentially) multiple expensive context value lookups or a single established Attr container value — which a Logger is.

If we were to strip Logger of the Handler and of the methods that take Attr values with a Level, form Records, and pass them to that Handler, we would need to introduce one more component to handle these tasks. Which might not be that bad if it really did make the design clearer. But that would also be yet another moving piece to manage and keep track of. And another method for specifying context-specific Handler configuration (e.g., Level sensitivity) would then be needed.

I also can see how all of this may not be obvious at first glance, but I get this design (or, at least, I think I do), I can see that there are compromises that must be and have been made.

alexec commented 1 year ago

This was bugging me too, but in Java. I wrote up a language agnostic spec for "semantic logging":

https://github.com/semantic-logs/spec

This dovetails into the topic of zero-copy logging.

burakkoken commented 1 year ago

Hi, first of all, thanks for this great proposal. But I share the same concerns regarding context.Context, custom handler and formatting output. And I would like to share my thoughts about these topics.

context.Context

Let's say we have some attributes like traceId and spanId for tracking our request flow.

To do so, the first option is we create a logger with these attributes and put it into context. And then we should get the logger from context and use the logger methods for logging across all components we have in our app. In my opinion, getting the logger from passed context is not efficient way to log our log data.

The second option is that we clone a logger using WithContext method in each package and method we are using logger. Also, this is not good option from my perspective.

My suggestion is we can add some extra logger methods which take type of context.Context parameter. They will be equivalent to the existing logger methods. So we will not need to use WithContext method anymore.

We can use the similar naming strategy in android logger.

// L is equivalent to Log method.  The only difference is it takes the type of context.Context as its first parameter.
func (l *Logger) L(ctx context.Context, level Level, msg string, args ...any)
func (l *Logger) Log(level Level, msg string, args ...any)

// A is equivalent to Attrs method.  The only difference is it takes the type of context.Context as its first parameter.
// It might be good to change LogAttrs method name to Attrs.
func (l *Logger) A(ctx context.Context, level Level, msg string, attrs ...Attr)
func (l *Logger) Attrs(level Level, msg string, attrs ...Attr)

// D is equivalent to Debug method.  The only difference is it takes the type of context.Context as its first parameter.
func (l *Logger) D(ctx context.Context, msg string, args ...any)
func (l *Logger) Debug(msg string, args ...any)

// I is equivalent to Info method.  The only difference is it takes the type of context.Context as its first parameter.
func (l *Logger) I(ctx context.Context, msg string, args ...any)
func (l *Logger) Info(msg string, args ...any)

// W is equivalent to Warn method.  The only difference is it takes the type of context.Context as its first parameter.
func (l *Logger) W(ctx context.Context, msg string, args ...any)
func (l *Logger) Warn(msg string, args ...any)

// E is equivalent to Error method.  The only difference is it takes the type of context.Context as its first parameter.
func (l *Logger) E(ctx context.Context, msg string, err error, args ...any)
func (l *Logger) Error(msg string, err error, args ...any)

// D is equivalent to Debug method.  The only difference is it takes the type of context.Context as its first parameter.
func D(ctx context.Context, msg string, args ...any)
func Debug(msg string, args ...any)

// I is equivalent to Info method. The only difference is it takes the type of context.Context as its first parameter.
func I(ctx context.Context, msg string, args ...any)
func Info(msg string, args ...any)

// W is equivalent to Warn method. The only difference is it takes the type of context.Context as its first parameter.
func W(ctx context.Context, msg string, args ...any)
func Warn(msg string, args ...any)

// E is equivalent to Error method. The only difference is it takes the type of context.Context as its first parameter.
func E(ctx context.Context, msg string, err error, args ...any)
func Error(msg string, err error, args ...any)

// L is equivalent to Log method. The only difference is it takes the type of context.Context as its first parameter.
func L(ctx context.Context, level Level, msg string, args ...any)
func Log(level Level, msg string, args ...any)

// A is equivalent to Attrs method.  The only difference is it takes the type of context.Context as its first parameter.
// It might be good to change LogAttrs method name to Attrs.
func A(ctx context.Context, level Level, msg string, attrs ...Attr)
func Attrs(level Level, msg string, attrs ...Attr)

Custom Handler Implementation

My concern with custom handler implementation is that it's difficult to create a custom handler, maybe we should simplify the Handler interface. What I mean is WithAttrs and WithGroup method can be removed because nobody would like to implement this method every time. Instead, Group and attributes can be handled and stored in struct Record by the slog library so newly created handlers can use them directly.

Formatting log output

My last concern is about formatting our log outputs. According to this design, when we would like to change our log pattern or format, we also have to create a custom handler again and moreover we cannot change the format for the existing handlers.

As I mentioned before, creating a custom handler takes some efforts and it is a bit difficult. My suggestion is to we can add some extra methods to Handler interface to be able to format our log outputs easily.

It might look like as follows.


type Handler interface {
    Handle(record Record) error
    SetFormatter(formatter Formatter)
    Formatter() Formatter
    Enabled(Level) bool
}

type Formatter interface {
    Format(record Record) string
}

These are only my thoughts I've had. I would like hear your thoughts and feedbacks.

jba commented 1 year ago

my issue is it being in the std library and the potential for that to be an endorsement for misuse of context

@mminklet, if someone makes that argument for including something in a context, I would reply by pointing out the differences between that something and logging. As I mentioned above, logging is the sort of thing that may be ignored through many layers of calls, only to be needed somewhere deep. In particular, when you add logging to find a bug, it is important to be able to make that change surgically, without disrupting lots of other code and potentially introducing more bugs. Very few things have that character.

"Because the standard library does it that way" is always a weak argument, except for conventions like naming and argument order. Why does the standard library do it that way? Do those same reasons apply to the current case, or are other forces at play?

jba commented 1 year ago

getting the logger from passed context is not efficient way to log our log data.

@burakkoken, That is true, but usually the extra time won't matter. If it does in some inner loop, you can always retrieve the logger from the context outside the loop.

we can add some extra logger methods which take type of context.Context parameter.

It seems simpler to have a the single WithContext method instead of all those extra methods. You can use WithContext fluent-style to get a similar effect to those methods:

logger.WithContext(ctx).Info(...)

WithAttrs and WithGroup method can be removed because nobody would like to implement this method every time

Unfortunately those need to be there so Handlers have an opportunity to pre-format parts of their output. That can make a big difference in performance. The implementation of the built-in handlers shows one way to implement those methods efficiently. Your Formatter interface would also need WithAttrs and WithGroup so it could make the same optimization, so ultimately it wouldn't make things simpler.

jba commented 1 year ago

To summarize the current state of the proposal:

We are proposing a package for structured logging, log/slog, whose API can be viewed at the design document or pkg.go.dev.

I'll cover only the major concerns here.

Many objected to the alternating keys and values API, feeling it is error-prone and not type-safe. Some prefer an API that only used Attrs, like the current LogAttrs method. We answered that this puts too much burden on typical users, and that other successful logging packages have the same API. (This comment is a good reflection of our views.) Others prefer a fluent API like log.String("name", "Al").Int("count", 3).Info(message). We pointed out that that API must sacrifice safety for performance.

Concerns were raised about including a Logger in a context, summarized here. Some argued that only log values should be in the context, not the logger itself. We answered that that still leaves open the problem of obtaining the right handler for the current log line, and putting the Logger in the context provides both attributes and handler. There were also concerns about the appropriateness of putting something other than request-scoped data in a context. We answered that logging is a reasonable exception to that rule, and the existence of numerous packages that put logging into a context suggests that it is a valuable feature.

Some objected to our design for adding context information to a Logger (the WithContext method). But the only alternative seems to be to add a context argument to every log method, and we did not want to force contexts on users who don't need them.

Some people pointed out that it is hard to change the level of a Handler. We answered that the Handler.Enabled method is more general than levels, and added an example that shows how to achieve the desired result.

Are there any other concerns with adopting this proposal?

beoran commented 1 year ago

@jba Since this logging package is now nearing completion, what are the performance characteristics, compared to other logging packages? Could you be as so kind as to resume them, please?

I assume the performance and memory use cannot be as good as those of Zerolog for safety reasons, but I would hope this package has a good performance, because for implementing servers, I had problems before with Logrus consuming up to 30% of CPU and memory resources. Now if slog becomes part of the standard library, that makes it easier to teach to new Go programmers to use it, so that's why I am willing to use it, if the performance of slog is sufficient.

QuantumGhost commented 1 year ago

This proposal has not mentioned how to handle slices.

For example, in zerolog Logger has method like Strs and Ints for log []string and []int respectively. It also has Array as a generic array logging mechanism.

It seems that the current implementation handling slices automatically, but it's not mentioned in the proposal.

alexec commented 1 year ago

This has been really helpful in specifying what Semantic Logs might be. Interestingly, its helped highlight:

There are a defect set of core fields such as time and msg that are common across programming languages.
Logging thread or logger appears to be an anti-pattern. trace_id and source and line is preferred.
JSON log encoding is very popular, maybe more popular that logfmt. It creates significantly larger logs.

https://github.com/semantic-logs/spec

08d2 commented 1 year ago

@jba

Are there any other concerns with adopting this proposal?

The proposal defines structured logging as a specific concrete implementation, and does not provide any kind of structured logging interface. As a consequence, code which wants to use structured logging via this package needs to depend on a concrete value rather than an abstract capability. This produces tight coupling between code modules (packages, etc.) that use structured logging, via a specific version of the slog package. It also makes it infeasible to extend the capabilities of the package with patterns like decorators, middlewares, and so on. An abstract logging interface is, for me, a necessary part of any proposal that extends the stdlib. Any proposal that doesn't include such an interface is concerning. Others may disagree, of course.

mminklet commented 1 year ago

In go you generally create the interface where you want to use it, rather than at the producer/implementation level, which means you shouldn't need a logging interface to be supplied for you

08d2 commented 1 year ago

Is a structured logger not a behavior with arbitrarily many implementations, like io.Writer or http.Handler?

AndrewHarrisSPU commented 1 year ago

@08d2

It also makes it infeasible to extend the capabilities of the package with patterns like decorators, middlewares, and so on

For architectural flexibility, the Handler is the interesting component. We could say the Logger implementation is a decorator on a Handler. Handlers are pretty adaptable for middleware.

In contrast, the Logger, TextHandler, and JSONHandler implementations could be pruned from slog and I think we'd have a good idea of what the missing foliage looked like. The library offers the concrete implementations up front, but the abstractions are also there.

08d2 commented 1 year ago

@AndrewHarrisSPU

We could say the Logger implementation is a decorator on a Handler. Handlers are pretty adaptable for middleware.

My goal is to ensure that code which should emit structured logs can take some well-defined interface as a dependency, and call methods on that interface to emit logs, without specific knowledge of its implementation(s). Can a slog.Handler serve this purpose? I don't think so, but maybe I'm missing something.

deefdragon commented 1 year ago

I've done some work to write my own front end logger as I have a priority for the type-safety. In the process, I'm finding myself re-writing a lot of the context related stuff. I'm worried that any 3rd parties that attempt to log using the context (or provide extra fields to it) will end up pulling in the default slog package using slog.FromContext(ctx).

My understand is that we cant use pure interfaces everywhere for logging because we don't have the escape analysis but I am wondering if there is a way we can use them to allow custom front ends to be more easily used/stored into context where they will properly be used downstream.

AndrewHarrisSPU commented 1 year ago

@08d2

My goal is to ensure that code which should emit structured logs can take some well-defined interface as a dependency, and call methods on that interface to emit logs, without specific knowledge of its implementation(s). Can a slog.Handler serve this purpose?

Definitely - aside from a few context management methods (essentially, serving to keep the slog context key internal), anything a Logger does could be rewritten to just use the underlying Handler in situ, it all terminates in a call to a Handler.

I think it'd be reasonable and more direct to pass a Handler rather than a Logger in a number of cases, but practically it sort of doesn't matter much - Logger.Handler provides the underlying Handler in any case.

szmcdull commented 1 year ago

@jba Since this logging package is now nearing completion, what are the performance characteristics, compared to other logging packages? Could you be as so kind as to resume them, please?

I assume the performance and memory use cannot be as good as those of Zerolog for safety reasons, but I would hope this package has a good performance, because for implementing servers, I had problems before with Logrus consuming up to 30% of CPU and memory resources. Now if slog becomes part of the standard library, that makes it easier to teach to new Go programmers to use it, so that's why I am willing to use it, if the performance of slog is sufficient.

As per Logrus, are you using WatchConfig()? WatchConfig() uses fsnotify, which consumes lot of kernel CPU when you put the log file in a large directory (with many files in it). In such case, every change of every file in the directory will be processed and notified through the kernel, and then filtered by Logrus.

ChrisHines commented 1 year ago

As I suggested in https://github.com/golang/go/issues/56345#issuecomment-1352047462, I have been carefully considering this proposal based on my experience contributing to several Go logging packages over the years, both open and closed source. I have not fully organized all of my thoughts yet and I have not had time to keep up with the discussion here. I am sure some of what I say below has already been said before, but I may be able to add another perspective based on a long running non-public project I've been a part of lately.

Context Support

Passing a logger in a context.Context is a common practice ...

Although true, it's not a good idea to make it the preferred approach.

I have strong opinions about this, opinions that are biased by my experience with a non-public structured logging package that adopted this practice over seven years ago in November 2015, prior even to the addition of the context package to the standard library. This logging package is universally used in a large project consisting of dozens of services built from about 1,000 packages. The project has been built and maintained by many different people over a number of years. It truly fits the "programming integrated over time" description of software engineering.

I only joined this project about two years ago. Shortly after joining I was tasked with leading a team to improve our logging practices within the project. To avoid going on a long tangent, I'll just say that we wanted to make our log data more consistent and make it easier for developers to maintain that consistency in the future. This is a big effort that requires reviewing and updating existing code that logs and establishing APIs and processes to put some guide rails on log data content.

As the team took stock of the status quo at the beginning of this effort we became convinced that the practice of passing loggers in contexts was problematic from a software engineering perspective. It hides the flow of loggers and encourages introducing contexts in code that doesn't need them. In October 2021 I wrote a document and gave a presentation to the wider project team arguing that we should eliminate passing loggers in contexts.

At the time we had about 300 functions that took a logger as an explicit parameter and about 1250 functions that extracted a logger from a context with our version of FromContext. As we studied our code we began to realize the cognitive load introduced by passing loggers in contexts.

Any function that accepts a context.Context might log, or might not.
- At a function call site:
- Does it use the context for cancelation?
- Does it use the context for logging?
- Do you need to pass a logger in the context?
- When editing a function body:
- Can you rely on the context containing an appropriate logger?
- If there isn't one, is it OK to use the default logger in this function?
- If a function doesn't do any logging itself but calls other functions that take a context:
- Does it need to extract a logger from the context, add some attributes, and put it in a new context to pass down?
  - What's the cost if that isn't needed?
  - What if some lower function gets logging added later?

We also discovered that the idea of passing loggers in contexts had become common enough in our code that developers had written functions with a context parameter for the sole purpose of getting a logger. I don't have a count of those, but it bothers me every time I see it.

We felt the practice of passing loggers in contexts was harmful enough that we are now undergoing an effort to eliminate it from our code base and we have written a static analysis tool to help developers with that effort.

I fear that should the standard library normalize the practice of passing loggers in contexts by providing convenient tools to do it that the practice will become too ubiquitous in the Go community and ecosystem to avoid and that will be a step in the wrong direction. It seems to me that projects that want to pass loggers in contexts can easily write their own LoggerFromContext and NewLoggerContext functions without loss of functionality. That, of course, means there isn't a common contextKey for loggers shared across the Go ecosystem, but why do we need that anyway?

I think slog.FromContext, slog.NewContext and slog.Ctx should be separated from the main slog proposal. They aren't integral to the main slog functionality and are of dubious value that can be debated in a separate proposal if we want.

Although my current project isn't public and is only one corner of the Go world, please take the lessons we've learned after seven years of experience with similar functions on a large project. We regret having those functions, they are now deprecated in our code, and we are actively working to eliminate their use.

dottedmag commented 1 year ago

I'd like to add a counter-point to https://github.com/golang/go/issues/56345#issuecomment-1379438240

A project I'm involved with is approximately twice as small as the one mentioned in that comment: we've got approximately 800 functions logging via logger obtained from context and ~100 functions taking a plain logger (zap in our case). This project is 6.5 years old and has seen quite a bit of churn w.r.t. active contributors during this time.

By applying some rules we've successfully used loggers passed through contexts and don't have any plans to stop doing that:

All contexts in the codebase inherit from the root context created in main. This is not a requirement enforced for the sake of logging, so adding a logger to the root context is a freebie that ensures there are no contexts without logger, and appropriate logger is always available either in the function, or in one of the callers.
One of mistakes reviewers watch for is passing a context solely to use a logger. This is not a huge burden during reviews, and we have a plan to make a linter to automate it if it proves to be more error-prone or time-consuming.

Hence we avoid most of the cognitive load mentioned in the previous comment:

At a function call site:

Does it use the context for cancelation?

It does, no exceptions. In exceedingly rare situations when a context was used for cancellation and then the code was changed to not do so it's a small and quick change. I remember 2 cases that involved 1 function each over the project lifetime.

Does it use the context for logging?

Maybe, it does not matter.

Do you need to pass a logger in the context?

It is already there, no need to do anything.

When editing a function body:

Can you rely on the context containing an appropriate logger?

Yes.

If a function doesn't do any logging itself but calls other functions that take a context:

Does it need to extract a logger from the context, add some attributes, and put it in a new context to pass down?

Yes, and this is a decision that has to be made anyway, no matter if the logger is passed via context or explicitly.

What if some lower function gets logging added later?

Over 6.5 years I remember maybe ~10 times when a code change had to pass a logger where it wasn't needed before, an I review ~50% of all changes going into the system.

darkfeline commented 1 year ago

Re: https://github.com/golang/go/issues/56345#issuecomment-1379438240

Any function that accepts a context.Context might log, or might not.

At a function call site:

Does it use the context for cancelation?

Does it use the context for logging?

I don't really understand why this is a consideration. I never wonder while calling a function whether it logs or not, let alone whether it is using the context for logging.

* Do you need to pass a logger in the context?

I don't think this matters. If a logger is not set, then the log is discarded. As a library/function author, I do not know or care where the log ends up. I ask for something to be logged, and the upstream user/caller decides whether and/or how the log message gets handled.

When editing a function body:

Can you rely on the context containing an appropriate logger?

Again, it doesn't matter. It's up to the caller to decide that, not the function. If a logger is not set then the message is discarded (or printed to stderr, depending on what choice is made for the ultimate default logger).

* If there isn't one, is it OK to use the default logger in this function?

The function doesn't decide which logger to use.

If a function doesn't do any logging itself but calls other functions that take a context:

Does it need to extract a logger from the context, add some attributes, and put it in a new context to pass down?

It doesn't need to know anything. This is why tying the logger to the context is a good idea. If I want to call a function that needs to log through a retry function that does not need to log, I do not need to rewrite the retry function to take a logger parameter to then pass to the final function. The retry function can pass the context through, happily oblivious of whether a logger is set or not.

  * What's the cost if that isn't needed?
  * What if some lower function gets logging added later?

Again, the beauty of tying it to the context is that it doesn't matter. This is only a concern if you're passing the logger around manually.

flowchartsman commented 1 year ago

Is there a particular implementation decision to have WithAttrs([]Attr) and not WithAttrs(...Attr)? It seems like that would be more consistent.

golang / go