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

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

Closed
pohly opened this issue Mar 18, 2023 · 4 comments

Comments

@pohly
Copy link

pohly commented Mar 18, 2023

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 pohly added the Proposal label Mar 18, 2023
@gopherbot gopherbot added this to the Proposal milestone Mar 18, 2023
@pohly
Copy link
Author

pohly commented Mar 18, 2023

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
Copy link

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

@jba
Copy link
Contributor

jba commented Mar 21, 2023

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
Copy link
Author

pohly commented Mar 21, 2023

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

@pohly pohly closed this as completed Mar 21, 2023
@golang golang locked and limited conversation to collaborators Mar 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants