-
Notifications
You must be signed in to change notification settings - Fork 81
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
WIP: slog support #196
WIP: slog support #196
Conversation
} | ||
|
||
func ExampleToSlog() { | ||
logger := logr.ToSlog(funcr.New(func(prefix, args string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about adding slog support to funcr, but decided against it because:
- here I want to test the code in slogHandler, which wouldn't be used if funcr itself supported slog
- the handlers that come with slog provide similar functionality, so I don't see the need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, klog and zapr definitely should get extended. I've started doing that for zapr, using the same interface for conversion that I am proposing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For zapr see go-logr/zapr#60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't object to impl libraries supporting both logr and slog, but I don't know why it's logr's responsibility to participate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because logr.ToSlog
and log.FromSlog
are using it to avoid glue code. slogr.New
and slogr.NewHandler
(your PR) don't, but I think they should if we go with that API.
This enables the use of the logr.Logger API on top of a slog.Handler. The other direction is also possible, but some information is lost. Instead, logr implementations should better get extended for usage as slog.Handler. Going back and forth between the two APIs is now part of logr.
This is mostly for convenience.
This needs to be explored further before this PR gets merged. My goal is to add a The alternative is to only have Yet another alternative would be to start with a separate repo and only move the code if needed. But this will cause work not just for us, but also others. The advantage would be that the API doesn't need to be exactly right immediately because a separate repo can start with v0.1.0 The motivation for having such a get function is to get rid of the klog dependency in modules like client-go. They need access to a global fallback logger because we don't require that all contexts in Kubernetes have a logger, so replacing the current |
The argument about having to convert in logr holds also without a global default logger: the same situation occurs when code using both APIs share the same context. It would be nice to support |
} | ||
|
||
return New(&slogSink{handler: handler}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good if FromSlog(ToSlog(...))
and vice versa returned the original logger. This currently works when the backend supports both APIs. But if not, we end up layering slogSink on top of slogHandler or vice versa.
Can be fixed with the "underlier" pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I skipped over one detail: logr.Logger.level
gets lost during conversion.
Suppose someone does this:
func foo(logger logr.Logger) {
// The Info call could be in some other library which only uses slog.
logr.ToSlog(logger).Info("hello")
}
func bar(logger logr.Logger) {
foo(logger.V(4))
}
Should that info message then become a debug message (info - 4 = debug in slog)? I think the answer is yes.
This can be done in the slogHandler
or by wrapping a real slog.Handler
(modify record), but it makes the conversion code more complex because logr.Logger.level
needs to be restored when going back from such a slog.Handler
to a logr.Logger
(FromSlog(ToSlog(logger.V(4))
== logger.V(4)
).
The same key must be used to ensure that the most recent logger gets retrieved. Converting on retrieval has the advantage that clients that only use one API don't need conversion.
I've pushed one commit adding support for that, |
There's a lot going on in this PR that I don't quite see why. I'm reading now, but if we could break it up into "bare minimum bridge logic" (which is what my own PR offers - bare minimum) and "other stuff to maybe make nicer UX", it would help. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How little can we get away with?
I think our PRs differ mostly in the UX. Can we step back and decide what we want the outermost API to be?
User story 1) I have a slog.Logger
and need to call code that uses logr.
User story 2) I have a logr.Logger
and need to call code that uses slog.
I envisioned:
func myCode(log slog.Logger) {
l := slogr.New(log.Handler())
codeThatWantsLogr(l)
}
func myCode(log logr.Logger) {
s := slog.New(slogr.NewHandler(log))
codeThatWantsSlog(l)
}
I guess you envisioned:
func myCode(log slog.Logger) {
l := logr.FromSlog(log)
codeThatWantsLogr(l)
}
func myCode(log logr.Logger) {
s := logr.ToSlog(log)
codeThatWantsSlog(l)
}
Questions:
- Do we want a
slog
dep in thelogr
package or in alogr/slogr
package or in a new reposlogr
? - How much do we encapsulate. Yours is more complete than mine. Mine is a bit more transparent.
- Do we really need the *Handler variants you have?
"log/slog" | ||
) | ||
|
||
// This file contains the version of NewContext and FromContext which supports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I get what you are doing here, but I am not sure why.
slog doesn't include "from context" functions - are you just "augmenting" their API by doing it here?
Do we really need this, or can we say "Go doesn't want to support it, go bother them" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I get what you are doing here, but I am not sure why.
slog doesn't include "from context" functions - are you just "augmenting" their API by doing it here?
Yes, and because any interoperable API has to know about both (see below). That cannot be in slog.
Do we really need this, or can we say "Go doesn't want to support it, go bother them" ?
That ship pretty much has sailed now. Suppose slog.NewContext
and slog.FromContext
were added now. Then consider this (simplified - no error handling) example:
func usingLogr(ctx context.Context) {
logger := logr.FromContext(ctx)
logger.Info("hello")
}
func usingSlog(ctx context.Context) {
logger := slog.FromContext(ctx)
logger.With("slogKey", "slogValue")
ctx = slog.NewContext(ctx, logger)
usingLogr(ctx)
}
func main() {
logger := zapr.NewLogger(...)
ctx := logr.NewContext(context.Background(), logger)
usingSlog(ctx)
}
What we want here is that "slogKey": "slogValue"
get added to the "hello" message. logr.FromContext
can't achieve that.
It can look for a logger with its own key and call slog.FromContext
. But the context has both and slog.FromContext
cannot decide which one is more recent and thus should be used.
This PR avoids that by using the same key for all kinds of loggers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But as soon as slog realizes this was the wrong decision, we're back to this problem, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My impression from the discussion was that the decision to not support passing via context is final.
|
||
// SlogImplementor is an interface that a logr.LogSink can implement | ||
// to support efficient logging through the slog.Logger API. | ||
type SlogImplementor interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having a hard time really grasping why we want this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comments ("avoid glue code").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we maybe do this as 2 (or more) PRs?
- Add trvially obvious bridge support with known gotchas.
- Make changes to logr to make cotchas less.
- Add optional interface indicating direct support for slog.
- Context mania
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can definitely start with step 1.
Do you want to do that here on some experimental branch or in a separate repo? I'm leaning towards doing it here, in the main package, because I expect that we'll need that eventually. We just shouldn't merge it into "master" yet.
I'm less sure about the order of the next two steps. Solving some of these issues will be hard and take time. The end result will still be less efficient. It would be simpler to do step 3 first and enhance klog and zapr. Then Kubernetes will be ready to support packages using slog as API much sooner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just saw your comment below about context handling with the key defined in an internal package. That should work, so let me take back the "in the main package": let's start in this repo with "slogr" as package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK to skip step 2 if we decide step 3 is the better answer.
This branch seems fine, and we can commit things and then revise, as long as we don't tag a release yet.
WRT which package - logr.ToSlog
and logr.FromSlog
are pleasant. I think a slogr
package is better if the API is really parallel to funcr, zapr, etc - slogr.New(slog.Handler)
.
Maybe step 1 is to evaluate that difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of rewriting the entire branch for this PR, I created a new one for step 1: #205
} | ||
|
||
func ExampleToSlog() { | ||
logger := logr.ToSlog(funcr.New(func(prefix, args string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't object to impl libraries supporting both logr and slog, but I don't know why it's logr's responsibility to participate.
return l.sink.Enabled(levelFromSlog(level)) | ||
} | ||
|
||
func (l *slogHandler) Handle(ctx context.Context, record slog.Record) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all of these, ctx is unusued, so should be named _
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay.
) | ||
|
||
type slogHandler struct { | ||
sink LogSink |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my impl chose to store the Logger rather than the LogSink. I'm not sure it's better, but I am curious why you chose Sink? I chose Logger because it felt easier. (FWIW, my impl of logr->slog first held a slog.Logger, but apparently that's not what slog wants us to do).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly for symmetry: we are layering some high-level API on top of some low-level API. Doing the same here with LogSink
as in slogSink
with slog.Handler
seemed appropriate.
I chose Logger because it felt easier.
Is it really? Using l.sink.Enabled/Info/Error
below is trivial and avoids calling code that doesn't add any value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logr.Logger is not a no-op, though.
logrLogger := logr.Discard() // nil LogSink
slogLogger := logr.ToSlog()
slogLogger.Info() // -> panic (I think?)
Similarly, Logger.WithCallDepth() does the LogSink interface test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, I need to handle a nil sink. I wasn't trying to do anything with call stack unwinding, so I didn't need that.
return &l | ||
} | ||
|
||
func (l slogHandler) WithGroup(name string) slog.Handler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now what this is, and I am less enamored with it. I thought it was to accumulate Attrs into a "pseudo-struct" which would then fold into the parent Handler, but I see that's not the case.
e.g.
log := getMySlogger()
log.WithGroup("grp").WithAttrs(slog.Int("a", 1), slog.Int("b", 2))
log.Info("The message", "arg", 31415)
=> {"msg": "The message", "grp": { "a": 1, "b": 2 }, "arg": 31415}`
But that's not it. then I thought it would at least assemble a struct and log it as such:
log := getMySlogger()
log.WithGroup("grp").LogAttrs(lvl, "The message", slog.Int("a", 1), slog.Int("b", 2))
=> {"msg": "The message", "grp": { "a": 1, "b": 2 }}`
But that doesn't seem right either, unless this adapter caches all the WithAttrs
and then sends them all to logr when it is actually logged (which will break things like funcr's hooks).
So a key-prefix seems about as good as we can get. That said, using a period as the delimiter means we get JSON like { "the.key1": 1, "the.key2": 2}
which is strictly VALID but seems to break jq:
$ echo '{ "a": "aval", "b.key": 1, "b.val": 2}' | jq .
{
"a": "aval",
"b.key": 1,
"b.val": 2
}
$ echo '{ "a": "aval", "b.key": 1, "b.val": 2}' | jq .a
"aval"
$ echo '{ "a": "aval", "b.key": 1, "b.val": 2}' | jq .b
null
$ echo '{ "a": "aval", "b.key": 1, "b.val": 2}' | jq .b.key
null
echo '{ "a": "aval", "b.key": 1, "b.val": 2}' | jq '.["b.key"]'
1
I didn't find any other "special" character that doesn't also break jq, so either we use something like underscore or we just document this oddity. Or we let users pass in the group-delimiter string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it would at least assemble a struct and log it as such
That's indeed what the slog.JSONLogger does:
package main
import (
"log/slog"
"os"
)
func main() {
logger := slog.New(slog.NewJSONHandler(os.Stdout, nil))
logger.WithGroup("group").With("hello", "world").Info("ping")
}
=>
{"time":"2023-08-04T08:33:43.56963767+02:00","level":"INFO","msg":"ping","group":{"hello":"world"}}
When wrapping zapr (the current one without slog support), I get instead:
{"level":"info","msg":"ping","group.hello":"world"}
With slog support there is at least a chance to implement WithGroup
properly - but I haven't tried that yet (open TODO).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once I found https://github.com/uber-go/zap/blob/v1.25.0/field.go#L334 it was trivial.
Now I get:
{"level":"info","msg":"ping","group":{"hello":"world"}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does that intersect with WithValues() ?
E.g.:
s := logr.ToSlog(myLogger)
s.With("a", 1) // calls Handler.WithAttrs(), which calls logr WithValues("a", 1)
s = s.WithGroup("g")
s.With("b", 2) // calls Handler.WithAttrs(), which calls logr WithValues("g.b", 2)
s.Info("msg", "c", 3) // calls Handler.Handle, which calls logr.Info("msg", "g.c", 3)
The only options I see are
-
Live with output
"g.b": 2, "g.c": 3
-
don't use logrHandler under slog, use native slog support
-
accumulate calls to WithAttr(), so it becomes:
s := logr.ToSlog(myLogger)
s.With("a", 1) // calls Handler.WithAttrs(), which calls logr WithValues("a", 1)
s = s.WithGroup("g")
s.With("b", 2) // calls Handler.WithAttrs(), which saves "b"=2
s.Info("msg", "c", 3) // calls Handler.Handle, which calls logr.Info("msg", "g", map[string]any{"b": 2, "c": 3})
- teach logr to have a "group" construct.
I'm interested in this approach, but I think we can accept (0) until we do that. Fair?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
l.callDepth = info.CallDepth | ||
} | ||
|
||
func (l *slogSink) WithCallDepth(depth int) LogSink { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in funcr I adopted the convention that methods which were going to clone the sink did:
func (sink slogSink) WithCallDepth(depth int) logr.LogSink {
sink.depth += depth
return &sink
}
This saves 1 LOC and I couldn't tell if it had any other side-effects. I don't REALLY care but it seems unfortunate to have both styles in the same codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with me, let's do the same here.
|
||
func (l *slogSink) Info(level int, msg string, kvList ...interface{}) { | ||
if l.prefix != "" { | ||
msg = l.prefix + ": " + msg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my PR I chose to track the "name" and report it as a distinct key-value pair ("logger":"my/logger/name"
) rather than mutating the user's message.
Do you have a reason to do it this way instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also added the V level as a key, since slog obscures that:
func (sink slogSink) Info(level int, msg string, kvList ...interface{}) {
args := make([]interface{}, 0, len(kvList)+4)
if len(sink.name) != 0 {
args = append(args, "logger", sink.name)
}
args = append(args, "vlevel", level)
args = append(args, kvList...)
sink.log(slog.Level(-level), msg, args...)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a reason to do it this way instead?
Not a strong one, just a slight concern about adding more "special" keys. That also applies to "vlevel". What if some program is already using that for something else?
"err" for slogSink.Error
has the same problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrt special keys:
We could choose to "group" keys (we didn't in the past, but there's always v2 :). We still have to pick a name for the group. We could name-prefix keys (e.g. "logr/name") but it seems unfortunate to embed "logr" into the keyspace.
I don't like mutating the message. I agree that "err" as a special key is clunky, but we need SOME special keys. slog does too: https://pkg.go.dev/golang.org/x/exp/slog#JSONHandler.Handle
Unless we namespace all the user-provided keys. This also feels orthogonal to getting slog support in - I am not against changing this, but lets consider that across all the impls we support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with using "logger" here.
} | ||
|
||
var _ LogSink = &slogSink{} | ||
var _ CallDepthLogSink = &slogSink{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented Underlier:
// Underlier exposes access to the underlying logging function. Since
// callers only have a logr.Logger, they have to know which
// implementation is in use, so this interface is less of an
// abstraction and more of a way to test type conversion.
type func (Underlier).GetUnderlying() invalid type
GetUnderlying() slog.Handler
}
func (sink slogSink) GetUnderlying() slog.Handler {
return sink.handler
}
var _ Underlier = &slogSink{}
|
||
func levelFromSlog(level slog.Level) int { | ||
if level >= 0 { | ||
// logr has no level lower than 0, so we have to truncate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logr docs say:
"Negative V-levels have the same meaning as V(0)."
so you don't need to worry about this (minor point)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point I want to discuss with you whether V(-1)
should be supported, though.
It think it is a valid use case to say "I want more output when calling this code here", which is someCode(logger.V(-1))
. Right now we only support "I want less output": someCode(logger.V(1))
. Ironically, the latter use case is the one that Jordan was concerned about because when seeing logger.V(5).Info
in someCode
it's not obvious that running the binary with -v=5
won't produce that output. That's not a problem with someCode(logger.V(-1))
and it was a use case that scheduler folks wanted ("produce more output when scheduling a specific pod").
That slog supports "more important than INFO" (= slog.LevelWarn
= 4) and "less important than INFO" (= slog.LevelDebug
= -4) while logr only supports "less important than INFO (= V(4)
) is another aspect to consider. I understand that this was intentional, but it keeps coming up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not heard that argument, and I am skeptical of it.
Every package is the center of their own universe, and thinks they know what's important. logr V(0) means "always log", unless somebody upstream from you has decided that you are less important than you think you are. I'm not sure we should be giving callsites the ability to override that.
I don't object to leaving this, other than "less is more", but simply negating the level seems correct-ish anyway. If you somehow had a V(-1) logr call it would become INFO+1 in slog.
Anyway, that's orthogonal to this PR. I'd prefer we strip this one of anything speculative, get the bare basics merged, and then see how to do better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I got carried away when you mentioned negative levels 😅
return l.sink.Enabled(levelFromSlog(level)) | ||
} | ||
|
||
func (l *slogHandler) Handle(ctx context.Context, record slog.Record) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to document somewhere that the passed-in timestamp is ignored, and any logr.LogSink
which logs the time is going to produce its own timestamp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The README has a list of drawbacks that come from using glue code for "logr on slog" - this is another one which I need to add.
return l.sink.Enabled(levelFromSlog(level)) | ||
} | ||
|
||
func (l *slogHandler) Handle(ctx context.Context, record slog.Record) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhere we need to handle caller info. My PR gets it right by hardcoding a depth, which I don't think slog will want to guarantee.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we need to call Callers() again and walk back until I find the PC they gave us, and add that many frames to the logr depth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My PR gets it right by hardcoding a depth, which I don't think slog will want to guarantee.
Only if the code using the handler is using slog.Logger
. We can't assume that, the code might also use something else with different offsets.
Maybe we need to call Callers() again and walk back until I find the PC they gave us, and add that many frames to the logr depth?
Urgh. That probably works in most usages, but not when the record was produced in one goroutine and then gets logged somewhere else entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we need examples and slog test and stuff, but that's after we agree on design
Not just that. Avoiding glue code (
I think that follows from the conceptual question above: is the focus more on conversion (back and forth) or more on "I just want a new logger"? Depending on that one API may be more suitable than the other.
My main argument at this point for having it in logr is that context support then works better - see the new(ish) "support slog.Logger and slog.Handler in context" commit that I added later yesterday.
I'm not sure what you mean with encapsulate.
Not sure. slog has two logging APIs ( |
I think you have looked at it longer than me, so I need some time to "catch
Hypothetically, could we do something like: logr/internal/ctximpl/context.go:
logr/logr.go:
logr/slogr/context.go:
Basically, the
How much of the complexity of the dual-universe do we try to make nicer (all
I can imagine a lot of things, but I don't think we should speculate without |
That looks doable.
Fair enough. So let's only support |
I need to point out one implication: because this code remains unchanged, the value must be a |
re Context - I don't yet have a clear metal model, so let's stack that for after the baseline discussion/bridge logic. |
Most of the functionality was merged into the separate |
This adds APIs for converting between a logr.Logger and a slog.Logger. Glue code is needed in some cases.
The reasons for doing this in logr itself are a) easy access to the new API calls and b) it enables a future logr extension where logr itself manages a global default logger which can be provided by either a slog.Handler or a logr.LogSink, with conversion as needed by a client.
Since it was created, this PR was split into smaller PRs and will not get merged itself. I'm keeping it around as reference until everything is merged:
Fixes: #171