Skip to content
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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ jobs:
test:
strategy:
matrix:
version: [ '1.15', '1.16', '1.17', '1.18' ]
version: [ '1.15', '1.16', '1.17', '1.18', '1.19', '1.20', '1.21.0-rc.4' ]
thockin marked this conversation as resolved.
Show resolved Hide resolved
platform: [ ubuntu-latest, macos-latest, windows-latest ]
runs-on: ${{ matrix.platform }}
steps:
Expand Down
83 changes: 82 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,28 @@ received:
If the Go standard library had defined an interface for logging, this project
probably would not be needed. Alas, here we are.

When the Go developers started developing such an interface with
[slog](https://github.com/golang/go/issues/56345), they adopted some of the
logr design but also left out some parts and changed others:

| Feature | logr | slog |
|---------|------|------|
| High-level API | `Logger` (passed by value) | `Logger` (passed by [pointer](https://github.com/golang/go/issues/59126)) |
| Low-level API | `LogSink` | `Handler` |
| Stack unwinding | done by `LogSink` | done by `Logger` |
| Skipping helper functions | `WithCallDepth`, `WithCallStackHelper` | [not supported by Logger](https://github.com/golang/go/issues/59145) |
| Generating a value for logging on demand | `Marshaler` | `LogValuer` |
| Log levels | >= 0, higher meaning "less important" | positive and negative, with 0 for "info" and higher meaning "more important" |
| Error log entries | always logged, don't have a verbosity level | normal log entries with level >= `LevelError` |
| Passing logger via context | `NewContext`, `FromContext` | no API |
| Adding a name to a logger | `WithName` | no API |
| Grouping of key/value pairs | not supported | `WithGroup`, `GroupValue` |

The high-level slog API is explicitly meant to be one of many different APIs
that can be layered on top of a shared `slog.Handler`. logr is one such
alternative API, with interoperability as [described
below](#slog-interoperability).

### Inspiration

Before you consider this package, please read [this blog post by the
Expand Down Expand Up @@ -119,6 +141,63 @@ There are implementations for the following logging libraries:
- **github.com/go-kit/log**: [gokitlogr](https://github.com/tonglil/gokitlogr) (also compatible with github.com/go-kit/kit/log since v0.12.0)
- **bytes.Buffer** (writing to a buffer): [bufrlogr](https://github.com/tonglil/buflogr) (useful for ensuring values were logged, like during testing)

## slog interoperability

Interoperability goes both ways, using the `logr.Logger` API with a `slog.Handler`
and using the `slog.Logger` API with a `logr.LogSink`. logr provides `ToSlog` and
`FromSlog` API calls to convert between a `logr.Logger` and a `slog.Logger`. Because
the `slog.Logger` API is optional, there are also variants of these calls which
work directly with a `slog.Handler`.

Ideally, the backend should support both logr and slog. In that case, log calls
can go from the high-level API to the backend with no intermediate glue
code. Because of a conflict in the parameters of the common Enabled method, it
is [not possible to implement both interfaces in the same
type](https://github.com/golang/go/issues/59110). A second type and methods for
converting from one type to the other are needed. Here is an example:

```
// logSink implements logr.LogSink and logr.SlogImplementor.
type logSink struct { ... }

func (l *logSink) Enabled(lvl int) bool { ... }
...

// logHandler implements slog.Handler.
type logHandler logSink

func (l *logHandler) Enabled(ctx context.Context, slog.Level) bool { ... }
...

// Explicit support for converting between the two types is needed by logr
// because it cannot do type assertions.

func (l *logSink) GetSlogHandler() slog.Handler { return (*logHandler)(l) }
func (l *logHandler) GetLogrLogSink() logr.LogSink { return (*logSink)(l) }
```

Such a backend also should support values that implement specific interfaces
from both packages for logging (`logr.Marshaler`, `slog.LogValuer`). logr does not
convert between those.

If a backend only supports `logr.LogSink`, then `ToSlog` uses
[`slogHandler`](sloghandler.go) to implement the `logr.Handler` on top of that
`logr.LogSink`. This solution is problematic because there is no way to log the
correct call site. All log entries with `slog.Level` >= `slog.LevelInfo` (= 0)
and < `slog.LevelError` get logged as info message with logr level 0, >=
`slog.LevelError` as error message and negative levels as debug messages with
negated level (i.e. `slog.LevelDebug` = -4 becomes
`V(4).Info`). `slog.LogValuer` will not get used. Applications which care about
these aspects should switch to a logr implementation which supports slog.

If a backend only supports slog.Handler, then `FromSlog` uses
[`slogSink`](slogsink.go). This solution is more viable because call sites can
be logged correctly. However, `logr.Marshaler` will not get used. Types that
support `logr.Marshaler` should also support
`slog.LogValuer`. `logr.Logger.Error` logs with `slog.ErrorLevel`,
`logr.Logger.Info` with the negated level (i.e. `V(0).Info` uses `slog.Level` 0
= `slog.InfoLevel`, `V(4).Info` uses `slog.Level` -4 = `slog.DebugLevel`).

## FAQ

### Conceptual
Expand Down Expand Up @@ -242,7 +321,9 @@ Otherwise, you can start out with `0` as "you always want to see this",

Then gradually choose levels in between as you need them, working your way
down from 10 (for debug and trace style logs) and up from 1 (for chattier
info-type logs.)
info-type logs). For reference, slog pre-defines -4 for debug logs
(corresponds to 4 in logr), which matches what is
[recommended for Kubernetes](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/logging.md#what-method-to-use).

#### How do I choose my keys?

Expand Down
31 changes: 31 additions & 0 deletions context.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
Copyright 2019 The logr Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package logr

// contextKey is how we find loggers in a context.Context.
type contextKey struct{}

// notFoundError exists to carry an IsNotFound method.
type notFoundError struct{}

func (notFoundError) Error() string {
return "no logger was present"
}

func (notFoundError) IsNotFound() bool {
return true
}
52 changes: 52 additions & 0 deletions context_noslog.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
//go:build !go1.21
// +build !go1.21

/*
Copyright 2019 The logr Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package logr

import (
"context"
)

// This file contains the version of NewContext and FromContext which supports
// only storing and retrieving a Logger.

// FromContext returns a Logger from ctx or an error if no Logger is found.
func FromContext(ctx context.Context) (Logger, error) {
if v, ok := ctx.Value(contextKey{}).(Logger); ok {
return v, nil
}

return Logger{}, notFoundError{}
}

// FromContextOrDiscard returns a Logger from ctx. If no Logger is found, this
// returns a Logger that discards all log messages.
func FromContextOrDiscard(ctx context.Context) Logger {
if v, ok := ctx.Value(contextKey{}).(Logger); ok {
return v
}

return Discard()
}

// NewContext returns a new Context, derived from ctx, which carries the
// provided Logger.
func NewContext(ctx context.Context, logger Logger) context.Context {
return context.WithValue(ctx, contextKey{}, logger)
}
123 changes: 123 additions & 0 deletions context_slog.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
//go:build go1.21
// +build go1.21

/*
Copyright 2019 The logr Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package logr

import (
"context"
"log/slog"
)

// This file contains the version of NewContext and FromContext which supports
Copy link
Contributor

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" ?

Copy link
Contributor Author

@pohly pohly Aug 4, 2023

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

// storing different types of loggers and converts as needed when retrieving
// the most recent one.

// FromContext returns a Logger from ctx or an error if no Logger is found.
func FromContext(ctx context.Context) (Logger, error) {
l := ctx.Value(contextKey{})

switch l := l.(type) {
case Logger:
return l, nil
case *slog.Logger:
return FromSlog(l), nil
case slog.Handler:
return FromSlogHandler(l), nil
}

return Logger{}, notFoundError{}
}

// FromContextOrDiscard returns a Logger from ctx. If no Logger is found, this
// returns a Logger that discards all log messages.
func FromContextOrDiscard(ctx context.Context) Logger {
l, err := FromContext(ctx)
if err != nil {
return Discard()
}
return l
}

// SlogFromContext is a variant of FromContext that returns a slog.Logger.
func SlogFromContext(ctx context.Context) (*slog.Logger, error) {
l := ctx.Value(contextKey{})

switch l := l.(type) {
case Logger:
return ToSlog(l), nil
case *slog.Logger:
return l, nil
case slog.Handler:
return slog.New(l), nil
}

return nil, notFoundError{}
}

// SlogFromContextOrDiscard is a variant of FromContextOrDiscard that returns a slog.Logger.
func SlogFromContextOrDiscard(ctx context.Context) *slog.Logger {
l, err := SlogFromContext(ctx)
if err != nil {
return ToSlog(Discard()) // TODO: use something simpler
}
return l
}

// SlogHandlerFromContext is a variant of FromContext that returns a slog.Handler.
func SlogHandlerFromContext(ctx context.Context) (slog.Handler, error) {
l := ctx.Value(contextKey{})

switch l := l.(type) {
case Logger:
return ToSlogHandler(l), nil
case *slog.Logger:
return l.Handler(), nil
case slog.Handler:
return l, nil
}

return nil, notFoundError{}
}

// SlogHandlerFromContextOrDiscard is a variant of FromContextOrDiscard that returns a slog.Handler.
func SlogHandlerFromContextOrDiscard(ctx context.Context) slog.Handler {
l, err := SlogHandlerFromContext(ctx)
if err != nil {
return ToSlog(Discard()).Handler() // TODO: use something simpler
}
return l
}

// NewContext returns a new Context, derived from ctx, which carries the
// provided Logger.
func NewContext(ctx context.Context, logger Logger) context.Context {
return context.WithValue(ctx, contextKey{}, logger)
}

// SlogNewContext returns a new Context, derived from ctx, which carries the
// provided slog.Logger.
func SlogNewContext(ctx context.Context, logger *slog.Logger) context.Context {
return context.WithValue(ctx, contextKey{}, logger)
}

// SlogHandlerNewContext returns a new Context, derived from ctx, which carries the
// provided slog.Handler.
func SlogHandlerNewContext(ctx context.Context, handler slog.Handler) context.Context {
return context.WithValue(ctx, contextKey{}, handler)
}
43 changes: 0 additions & 43 deletions logr.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,10 +207,6 @@ limitations under the License.
// those.
package logr

import (
"context"
)

// New returns a new Logger instance. This is primarily used by libraries
// implementing LogSink, rather than end users. Passing a nil sink will create
// a Logger which discards all log lines.
Expand Down Expand Up @@ -397,45 +393,6 @@ func (l Logger) IsZero() bool {
return l.sink == nil
}

// contextKey is how we find Loggers in a context.Context.
type contextKey struct{}

// FromContext returns a Logger from ctx or an error if no Logger is found.
func FromContext(ctx context.Context) (Logger, error) {
if v, ok := ctx.Value(contextKey{}).(Logger); ok {
return v, nil
}

return Logger{}, notFoundError{}
}

// notFoundError exists to carry an IsNotFound method.
type notFoundError struct{}

func (notFoundError) Error() string {
return "no logr.Logger was present"
}

func (notFoundError) IsNotFound() bool {
return true
}

// FromContextOrDiscard returns a Logger from ctx. If no Logger is found, this
// returns a Logger that discards all log messages.
func FromContextOrDiscard(ctx context.Context) Logger {
if v, ok := ctx.Value(contextKey{}).(Logger); ok {
return v
}

return Discard()
}

// NewContext returns a new Context, derived from ctx, which carries the
// provided Logger.
func NewContext(ctx context.Context, logger Logger) context.Context {
return context.WithValue(ctx, contextKey{}, logger)
}

// RuntimeInfo holds information that the logr "core" library knows which
// LogSinks might want to know.
type RuntimeInfo struct {
Expand Down
Loading