proposal: log/slog: allow implementing slog.Handler and logr.LogSink in the same type

[pohly]

https://pkg.go.dev/golang.org/x/exp/slog#Handler and https://pkg.go.dev/github.com/go-logr/logr#LogSink are conceptually similar. If it was possible to implement both interfaces in the same type, then it would be simple to put either slog or logr as user-facing API on top of it.

This is currently not possible because of conflicting parameters for Enabled:

• slog: Enabled(context.Context, Level) bool
• logr: Enabled(level int) bool

Would it be possible to rename Handler.Enabled? Possible alternatives (just thinking out aloud):

• EnabledWithContext
• SlogEnabled
• LevelEnabled

Would it be useful? I think so - but probably not crucial.

My current workaround for https://github.com/go-logr/zapr/blob/master/zapr.go is a second type:

var _ logr.LogSink = &zapLogger{}
var _ logr.CallDepthLogSink = &zapLogger{}

func (zl *zapLogger) Handle(ctx context.Context, record slog.Record) error {
	return nil // TODO
}

func (zl *zapLogger) WithAttrs(attrs []slog.Attr) slog.Handler {
	return nil // TODO
}

func (zl *zapLogger) WithGroup(name string) slog.Handler {
	return nil // TODO
}

type zapHandler zapLogger

func (zh *zapHandler) Enabled(_ context.Context, slevel slog.Level) bool {
	if slevel >= 0 /* info and higher */ {
		return true
	}
	return (*zapLogger)(zh).Enabled(int(-slevel))
}

func (zh *zapHandler) Handle(ctx context.Context, record slog.Record) error {
	return (*zapLogger)(zh).Handle(ctx, record)
}

func (zh *zapHandler) WithAttrs(attrs []slog.Attr) slog.Handler {
	return (*zapLogger)(zh).WithAttrs(attrs)
}

func (zh *zapHandler) WithGroup(name string) slog.Handler {
	return (*zapLogger)(zh).WithGroup(name)
}

var _ slog.Handler = &zapHandler{}

cc @jba @rsc

[pohly]

When using a second type, conversion between LogSink and Handler has to be done through additional interfaces. That may be the right solution anyway, because it gives implementors who want to support both more flexibility.

package zapr // TODO: put this into logr

import (
	"github.com/go-logr/logr"
	"golang.org/x/exp/slog"
)

type SlogImplementor interface {
	GetSlogHandler() slog.Handler
}

type LogrImplementor interface {
	GetLogrLogSink() logr.LogSink
}

// logr.ToSlog
func ToSlog(logger logr.Logger) *slog.Logger {
	if slogImplementor, ok := logger.GetSink().(SlogImplementor); ok {
		handler := slogImplementor.GetSlogHandler()
		return slog.New(handler)
	}

	// TODO: implement Handler which can write to an arbitrary LogSink.
	//
	// Problem: LogSinks do their own stack unwinding and need to
	// know how many levels to skip to get across the slog.Logger
	// frontend into the user code. Probably doesn't have a good
	// solution.
	return nil
}

// logr.FromSlog
func FromSlog(logger *slog.Logger) logr.Logger {
	if logrImplementor, ok := logger.Handler().(LogrImplementor); ok {
		logSink := logrImplementor.GetLogrLogSink()
		return logr.New(logSink)
	}

	// TODO: implement LogSink which can write to an arbitrary Handler.
	//
	// TBD: the stored WithName prefix has to be added to the Record.Message
	// or added as a new Attr (but with which key?).
	return logr.Discard()
}

[xorkevin]

One could create an adapter type to wrap a logr.LogSink with the slog.Handler interface right?

[jba]

I don't see us changing the name of Enabled just for this, and it looks like there are several ways around it.
Any objection if I close this?

[pohly]

Yes, let's close. I've been using the adapter type approach and it's not too bad.