contextual logging for slog about logr HOT 31 CLOSED

@veqryn: the main drawback of ContextWithLogrAndSlogr and the corresponding code in packages is the overhead. Modifying a logger, converting it, and creating two contexts instead of one can be significant work if the code that is being called then doesn't do much. We had cases in Kubernetes like that and had to reduce instrumentation at lower log levels to avoid performance regressions.

It could make sense that those helper methods live in logr, with better names and inlining some of the function calls.

The other drawback is that logr then has to depend on slog-context, a new package without a stable API. We could do that once it is 1.0, but then it's still not a nice dependency just for the context key.

I can also see a use for both of @thockin's "simple mode" ideas.

The first one doesn't work (extracting a logger cannot know which one to use). That leaves "Make logr.NewContext() store both our logr key and (if applicable) the slog-context key." Isn't that the same as your ContextWithLogrAndSlogr?

The "fancy" versions might be overkill in a world where devs are hopefully moving towards slog for most everything.

I agree, it's overkill. One argument against supporting logrus and zap the same way as slog and logr is that logrus and zap are logging implementations. If someone uses those, they decide to not be interoperable. Both logr and slog are abstractions and are interoperable with each other.

Regarding "slog for most everything", what do you think about recommending the usage of logr.Logger for code which accepts a context and wants to log through a logger in that context? If we can agree on that approach, the entire problem goes away...

from logr.

This is what I don't understand. On the one hand you are saying that logr is a dead-end because "it's not slog", but then so would be every other high-level API for slog.Handler that isn't in log/slog

In some sense, logr is a victim of its own success. Many aspects of logr made it into slog - so much so that the differences between slog.Logger and logr.Logger are pretty small. If you were a net-new user of Go, or even just starting a new app - would you choose to depend on built-in slog or on 3rd party logr? What does logr offer, beyond "kubernetes uses it", that would skew that decision? You cited skipping callers, and WithName(). I don't think WithName will matter a whole lot (I put it in the "cute" category, not "essential").

For "skipping callers" it was explicitly said that others can write a high-level API which supports that

I suspect this is going to be a decision that slog folks will come to regret. It seems so painfully obvious that wrappers are sometimes needed, and their answer is basically "too bad, so sad, go do it yourself". I don't know if this alone will make people choose logr, though. I suspect that eventually slog will HAVE to fix this. Same as having context support.

why not promote the existing solution that we already have instead of writing a new one with zero adoption?

Two answers.

1. Who do we think we are, telling slog-context how to build a library? @veqryn seems very nice, but they don't owe us anything. And there's no guarantee that slog-context "wins" and some other framework doesn't. How may times are we going to have this discussion with different, probably less pleasant projects? E.g. when slog itself decides to support Context, I doubt very much if they will care what we have done over here.

2. What's the long-term goal? My original goal with logr was, really, to push a Go-ecosystem-wide norm for logging. We can't claim full credit for slog, but certainly we had some influence. I call that a success! How much energy are we going to pour into logr, now that we have slog? It's a fun side-project (aka a distraction) but what's the opportunity cost? As slog (inevitably, IMO) fixes their missteps, there's less and less value in logr.

from logr.

PTAL at #252

from logr.

v1.4.0 is tagged.

from logr.

Option 3: None of the above. Developers who want to support contextual logging can do so with the logr.Logger API, which is more or less functionally equivalent to the slog.Logger API.

If this is the consensus, then we can do a few things in logr (all optional):

• document this recommendation more explicitly in slogr
• same in slog-context
• introduce log level constants in logr that match the ones in slog (e.g. const DEBUG int = 4 // -slog.DEBUG -> logger.V(logr.DEBUG).Info(...)) and/or aliases for V (logger.Debug().Info(...) - looks a bit weird?)

from logr.

Given that we already have context support for logr users, can we more clearly state the problem we are trying to solve? Is it:

a) to implement context support for slog users in general
b) to implement context support for users who store slog in Context via some external lib
c) to define a common key under which many slog+context libs store *slog.Logger?
d) something else?

Given that Go itself does not define a standard way to get a slog.Logger into and out of a Context, code which knows slog will have to either use the slog default Logger, pass *slog.Logger manually, or choose some non-standard lib (including DIY) for slog+context. It's not really logr's responsibility. That said, if we offer such a lib or throw what little weight we have behind one, it might help establish a norm.

Do we think slog-context has or might have critical mass for this?

Can we describe the desired UX for whomever is going to consume whatever it is we are conceptualizing? I feel like there are 3 categories of users:

1. Code that knows how to use slog and not logr. I think they want the equivalent of func NewContext(ctx context.Context, logger *slog.Logger) context.Context and func FromContext(ctx context.Context) (*slog.Logger, error) - but maybe they want to speak in terms of slog.Handler ?

2. Code that knows how to use logr and not slog. We already have logr.NewContext() and logr.FromContext().

3. Code that bridges slog and logr.

Do we feel like there's going to be a lot of back-and-forth between domains (1) and (2) or is it more like "this lib uses logr, so let me convert my slog to logr and stick it in context" ?

from logr.

Let's start with a problem statement: different packages defining their own keys for passing a logger (whether it's slog or logr) is a problem because it fragments the ecosystem. We have that problem with slog-context and logr.

My option 2 matches your point a - solve this for all slog users by trying to make logr the manager of "logger in context", regardless of the high-level API. The advantage of doing it in logr is that we can achieve interoperability with code which wants to use logr.Logger (like Kubernetes).

I'm not sure about your point b - is that "implement logr.Logger support for users who store slog in Context via some external lib"? My option 2 also would achieve that.

Point c is what slog-context already does. We don't need to replicate that in logr or slogr.

Point d might match my option 3: advocate to use logr.Logger when extracting a logger from a context.

from logr.

different packages defining their own keys for passing a logger (whether it's slog or logr) is a problem because it fragments the ecosystem.

I agree with a caveat. I'm not sure I buy that "passing a logger" is a generic statement. Surely we don't intend this to support any arbitrary logging API with the same key, with a matrix of transformations, do we? As fun as that sounds, it doesn't seem appropriate to store a log.Logger and fetch back a logr.Logger or slog.Logger or slog.Handler or zap.SugaredLogger or ...

So what are the reasonable bounds?

We have that problem with slog-context and logr.

From a strict POV, I don't think we have a "problem", per se. Both slog-context and logr work just fine with their own logger types. But, because logr and slog are so closely aligned, we're trying to do better, right? It's not exactly "arbitrary" but "a select list of implementations which choose to coordinate".

My option 2 matches your point a - solve this for all slog users ... The advantage of doing it in logr is that we can achieve interoperability with code which wants to use logr.Logger (like Kubernetes).

It feels like overreaching, to me. If I am a slog user, why is logr involved? If I were building slog-context, why should I take a dep on logr?

I'm not sure about your point b

I mistyped - I meant "to implement /context/ support for users who store slog in Context via some external lib"

Point c is what slog-context already does.

slog-context includes a lot more than that, though most of it is just wrapping and re-presenting a similar interface.

Point d might match my option 3: advocate to use logr.Logger when extracting a logger from a context.

I think, realistically, this is backwards. IMO slog obviates logr. We have no grounds on which to advocate for new users of logr, even if we prefer our own technical decisions :)

So if we think slog-context is the "winner" of the "slog-plus-context" debate (right up until Go decides to subsume it), we should put energy there.

Simple mode:

• Make logr.FromContext() read our own key for logr or else fall back on slog-context and use slogr (circular dep needs to be fixed) to produce a logger.Logger.
• Make logr.NewContext() store both our logr key and (if applicable) the slog-context key.

Fancy mode (hand-waving):

• Define a single unilog key and library which allows plug-in converter functions, which implementations offer to convert to/from other loggers they know.
• Example: logr registers conversion from logr.Logger to *slog.Logger and vice-versa (and maybe slog.Handler, too).
• unilog.NewContext[T] stores whatever it was given, on the assumption that the next caller will want that back
• unilog.FromContext[T] sees what was stored and figures how to convert from that to T via plugins

Extra points for adding a "cost" per conversion function and doing a shortest-path conversion. But realisitically, how many of these will we have in a single binary? I see https://pkg.go.dev/github.com/juju/zaputil/zapctx and logrus and zerolog all do things a little differently.

from logr.

Surely we don't intend this to support any arbitrary logging API with the same key, with a matrix of transformations, do we?

True. But slog and logr are conceptually similar enough that it is feasible and (my expectation) will win enough "market share" that covering those two would go a long way towards solving the problem.

It feels like overreaching, to me. If I am a slog user, why is logr involved? If I were building slog-context, why should I take a dep on logr?

The main advantage would be that a package using slog-context would work seamlessly in a Kubernetes binary that uses logr and klog's text logger or zapr. Without the integration, that binary can set the default slog logger (PR pending) but log entries then lack the context values.

So if we think slog-context is the "winner" of the "slog-plus-context" debate (right up until Go decides to subsume it), we should put energy there. Simple mode: ...

If the call sequence is logr.NewContext, slog-context.NewContext, logr.FromContext then looking first for the logr key will return the wrong logger. Same problem when reversing the order. It has to be a single key.

Fancy mode

That would work, but is more complex than doing it for logr and slog in logr. I'm just trying to be pragmatic here: IMHO accepting a dependency on slog is a small price to pay when (without changing program code) that enables using packages that use slog for contextual logging - if there are any.

I'm still not sure whether it's worth it. Option 3 seems perfectly fine to me, so it all depends on what other developers will pick for their packages.

from logr.

Ping @veqryn - we need your input.

from logr.

If the call sequence is logr.NewContext, slog-context.NewContext, logr.FromContext then looking first for the logr key will return the wrong logger. Same problem when reversing the order. It has to be a single key.

I feel like there's got to be some way, but I don't have time to prove it right now. If we get a logr, check to see if the logr.sloghandler == context's slog handler, and if not reconstruct the logr.

Writing it out, it starts to feel "too clever to be useful"

from logr.

#213 is a prior PR for option 1. I am going to close it while we figure out what to do in this issue.

from logr.

The main advantage would be that a package using slog-context would work seamlessly in a Kubernetes binary that uses logr and klog's text logger or zapr.

Perhaps I should have led with the inverse situation: Kubernetes packages like client-go will work seamlessly in a binary which uses slog + slog-context + some slog.Handler. That's probably more common than adding new dependencies to Kubernetes.

from logr.

Thank you for including me @pohly

So, just to recap:

• logr library has methods for storing and retrieving a logr.Logger from a context.Context
• slog-context library has methods for storing and retrieving a slog.Logger from a context.Context
• There exists some applications that presumably are using both a logr.Logger (perhaps in the main application) and also slog.Logger (perhaps in a library)
• We are exploring if we can make that easier or less error prone

And let me explore what integrating the two libraries looks like right now, without any changes to either library:

Primarily using logr workflow:

package main

import (
	"context"
	"log"
	"log/slog"
	"os"

	"github.com/go-logr/logr"
	"github.com/go-logr/logr/slogr"
	"github.com/go-logr/stdr"
	slogcontext "github.com/veqryn/slog-context"
)

func main() {
	logger := stdr.New(log.Default())                    // Create a logr.Logger
	ctx := logr.NewContext(context.Background(), logger) // Set it in the context

	slogger := slog.New(slogr.NewSlogHandler(logger)) // Convert logr.Logger to slog.Logger
	slog.SetDefault(slogger)                          // Set as default so that libraries that just use the default package level slog will still output the way we like
	ctx = slogcontext.ToCtx(ctx, slogger)             // Store the slog.Logger in the context too, so that libraries using slog.Logger from context output the way we like

	doSomething(ctx)
}

func doSomething(ctx context.Context) {
	logger := logr.FromContextOrDiscard(ctx).WithValues("myKey", "myVal") // Retrieve from context and store some attributes
	ctx = logr.NewContext(context.Background(), logger)                   // Set the new logr in the context, again
	ctx = slogcontext.ToCtx(ctx, slog.New(slogr.NewSlogHandler(logger)))  // Set the new slog in the context, again

	logger.Info("calling into library...")
	someLibraryWeHaveNoControlOver(ctx)
}

func someLibraryWeHaveNoControlOver(ctx context.Context) {
	slogcontext.Info(ctx, "in the library...")
}

And the primarily using slog-context workflow:

func main2() {
	logger := slog.New(slogcontext.NewHandler(slog.NewJSONHandler(os.Stdout, nil), nil)) // Create a slog.Logger
	slog.SetDefault(logger)                                                              // Set as default
	ctx := slogcontext.ToCtx(context.Background(), logger)                               // Store in the context

	// Convert slog.Logger to logr.Logger, and store the logr.Logger in the context too, so that libraries using logr.Logger from context output the way we like
	ctx = logr.NewContext(ctx, slogr.NewLogr(logger.Handler()))

	doSomething2(ctx)
}

func doSomething2(ctx context.Context) {
	ctx = slogcontext.With(ctx, "myKey", "myVal")                                // Retrieve from context and store some attributes, and store back in the context
	ctx = logr.NewContext(ctx, slogr.NewLogr(slogcontext.Logger(ctx).Handler())) // Convert the new slog to logr and set in the context

	slogcontext.Info(ctx, "calling into library...")
	someLibraryWeHaveNoControlOver2(ctx)
}

func someLibraryWeHaveNoControlOver2(ctx context.Context) {
	logr.FromContextOrDiscard(ctx).Info("in the library...")
}

We could cut down on some boilerplate with a few helper methods.

For the primarily logr workflow:

func doSomething(ctx context.Context) {
	logger := logr.FromContextOrDiscard(ctx).WithValues("myKey", "myVal")
	ctx = ContextWithLogrAndSlogr(ctx, logger)

	logger.Info("calling into library...")
	someLibraryWeHaveNoControlOver(ctx)
}

func ContextWithLogrAndSlogr(ctx context.Context, logger logr.Logger) context.Context {
	ctx = logr.NewContext(context.Background(), logger)
	return slogcontext.ToCtx(ctx, slog.New(slogr.NewSlogHandler(logger)))
}

For the primarily slog-context workflow:

func doSomething2(ctx context.Context) {
	ctx = ContextWithLogr(slogcontext.With(ctx, "myKey", "myVal"))

	slogcontext.Info(ctx, "calling into library...")
	someLibraryWeHaveNoControlOver2(ctx)
}

func ContextWithLogr(ctx context.Context) context.Context {
	return logr.NewContext(ctx, slogr.NewLogr(slogcontext.Logger(ctx).Handler()))
}

It could make sense that those helper methods live in logr, with better names and inlining some of the function calls.

I can also see a use for both of @thockin's "simple mode" ideas.

The "fancy" versions might be overkill in a world where devs are hopefully moving towards slog for most everything.

from logr.

@veqryn: the main drawback of ContextWithLogrAndSlogr and the corresponding code in packages is the overhead. Modifying a logger, converting it, and creating two contexts instead of one can be significant work if the code that is being called then doesn't do much. We had cases in Kubernetes like that and had to reduce instrumentation at lower log levels to avoid performance regressions.

I gave some thought to the above, and created this PR: veqryn/slog-context#1
There is only one context created, and only one context lookup. The overhead is now just an extra any on the context value.

I haven't added any documentation yet, and am open to renaming the methods. Just want to get a concrete idea out there right now, for us to talk about.

Regarding "slog for most everything", what do you think about recommending the usage of logr.Logger for code which accepts a context and wants to log through a logger in that context? If we can agree on that approach, the entire problem goes away...

I think that slog will gradually take over the ecosystem as the only structured logging interface, by virtue of being in the standard library.
I think most people that want to put a *slog.Logger into a context, will want to do just that, and work directly with the slog logger there, without anything else involved (like logr).

from logr.

I gave some thought to the above, and created this PR: veqryn/slog-context#1

I agree that this can work, but it has the drawbacks that I mentioned earlier:

• Each LogrToCtx call does extra allocations, which creates overhead in Kubernetes for no benefit (nothing needs the slog.Logger at the moment). On the bright side, if both are needed, then it's more efficient to only convert once.
• For interoperability, logr.NewContext and logr.FromContext* will have to call those functions and thus depend on slog-context and slog.

Perhaps I should put the idea that I had about converting on demand into code... stay tuned.

I think that slog will gradually take over the ecosystem as the only structured logging interface, by virtue of being in the standard library.

That may be the perception, but I think it's wrong: logr.Logger is designed as just one of many different high-level APIs on top of logr.Handler. It doesn't do everything that people will need (no support for contextual logging in the standard library, no support for logging helpers). That should leave room for alternatives. But you are right, many people will not think about it.

My hope was that in slog-context, we could point them in a better direction.

from logr.

Perhaps I should put the idea that I had about converting on demand into code... stay tuned.

See second commit in #237

from logr.

@thockin: that PR brings us back to my original implementation where everything is in the main package, which is required to avoid circular package dependencies. The slogr package still exists for backward compatibility.

from logr.

@pohly I can't comment on the packaging changes in #237, but I like the basic type switch for FromContext/SlogLoggerFromContext/NewContext/NewContextWithSlogLogger.

I think once you have a release with this, I can support it in slog-context and also link to it from my readme.
I wouldn't mind a callout for slog-context somewhere in your readme for folks who either want to work with slog in context directly, or who need the other features slog-context offers like pulling arbitrary attributes like OpenTelemetry tracing id's out of context and into slog log lines.

from logr.

I think once you have a release with this, I can support it in slog-context and also link to it from my readme. I wouldn't mind a callout for slog-context somewhere in your readme

So we cross-reference both efforts and then let developers decide which API they want to use, with an explanation of the pros and cons. That sounds like a plan to me.

from logr.

I think that slog will gradually take over the ecosystem

I agree. I know we think slog is "obviously" missing context support and other things, but really I don't think it's bad enough to prevent adoption. IMO, pushing logr as an interface is pretty much a dead-end. I can't say whether slog-context will be the dominant out-of-core helper lib.

If not for Kubernetes, would we be having this discussion?

I'll look at the PR, but here's something I don't have confidence in: Thru the slog design the Go team made the case that slog.Logger is but one possible high-level wrapper over slog.Hander. Do we think that "almost everyone" will use *slog.Logger as their primary interface? Or do we think that people will use other primary interfaces? I don't think we really have any way to know, and in fact what we do here may influence that.

from logr.

pushing logr as an interface is pretty much a dead-end [...] Thru the slog design the Go team made the case that slog.Logger is but one possible high-level wrapper over slog.Hander

This is what I don't understand. On the one hand you are saying that logr is a dead-end because "it's not slog", but then so would be every other high-level API for slog.Handler that isn't in log/slog.

Perhaps we need to distinguish more clearly what we are talking about: I see a future for logr.Logger as high-level API. I don't see a big need for logr.LogSink implementations, except for those which also fully support slog and provide some unique formatting that doesn't exist elsewhere in the Go ecosystem.

in fact what we do here may influence that

And that's a good thing, right? 😁

from logr.

If not for Kubernetes, would we be having this discussion?

Yes. logr.Logger supports several things that slog.Logger doesn't support (skipping callers, WithName). For "skipping callers" it was explicitly said that others can write a high-level API which supports that - so why not promote the existing solution that we already have instead of writing a new one with zero adoption?

from logr.

FWIW, if slog ever supports its use with context, I will happily call that a win.

from logr.

I suspect that eventually slog will HAVE to fix this. Same as having context support.

Some of their design decisions will make it hard to fix or more expensive (like using a pointer to an allocated struct instead of a small value).

I think the main reason for using the logr API is context support. It's more efficient if packages supporting that approach use the same logger instance and thus API.

Who do we think we are, telling slog-context how to build a library? [...] How may times are we going to have this discussion with different, probably less pleasant projects?

Hopefully not again. I've made enough noise about logr's slog support (added on https://github.com/golang/go/wiki/Resources-for-slog, posted in the issue) that other developers should be aware. Providing yet another package for "store slog.Logger in context" when there are already two (one which is in use, one which focuses on slog) seems stupid and hopefully won't get used even if someone publishes it.

How much energy are we going to pour into logr, now that we have slog? It's a fun side-project (aka a distraction) but what's the opportunity cost? As slog (inevitably, IMO) fixes their missteps, there's less and less value in logr.

We (or I, if you are loosing interest) will have to maintain it because Kubernetes depends on it. Once we are done with hashing out details of the slog interaction, I don't expect much further work.

from logr.

We (or I, if you are loosing interest) will have to maintain it because Kubernetes depends on it. Once we are done with hashing out details of the slog interaction, I don't expect much further work.

Or we convert to slog, like everyone else (my hypothesis)?

Basically, I'm not convinced that there won't be more things that slog changes that we have to keep pace with. I'm not deeply against this today, I really just want to understand how we see it playing out over the longer term.

So, is it a fair summary that our position is:

• logr will provide context support that converts to/from slog as needed
• slog-context will depend on logr
• any newcomers should also depend on logr
• if/when slog figures it out, we'll deal with it then

@veqryn are you good with that?

from logr.

Or we convert to slog, like everyone else (my hypothesis)?

That's something that we can discuss when slog reaches feature parity. Until then, it's a lot of code churn with no real benefit and some relevant drawbacks.

So, is it a fair summary that our position is: [...]

👍

from logr.

• logr will provide context support that converts to/from slog as needed
• slog-context will depend on logr (for slog in context support)
• any newcomers should also depend on logr (for slog in context support)
• if/when slog figures it out, we'll deal with it then

@veqryn are you good with that?

👍

from logr.

As far as I am concerned, we are done with this.

from logr.

@pohly what is the timeline for a new release?

from logr.

• fixed by #246

from logr.