golang/go

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

pohly opened this issue · 4 comments

pohly commented

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:

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 commented

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 commented

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

jba commented

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?

  • 👍2
pohly commented

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