spiffe / go-spiffe

Golang library for SPIFFE support
Apache License 2.0
145 stars 77 forks source link

Support `*slog.Logger` as configured logger #281

Open strideynet opened 5 months ago

strideynet commented 5 months ago

Go recently introduced slog to the standard library, this is a structured logger unlike the existing slog package. I expect we'll see a large increase in folks using slog now that it is "blessed" as part of the standard library, especially in greenfield projects.

Today, go-spiffe accepts anything which implements the following interface as a configurable logger:

// Logger provides logging facilities to the library.
type Logger interface {
    Debugf(format string, args ...interface{})
    Infof(format string, args ...interface{})
    Warnf(format string, args ...interface{})
    Errorf(format string, args ...interface{})
}

Unfortunately, unlike many other popular logging libraries, *slog.Logger does not implement this interface. Today, you'd need to write a very simple adapter between slog and this interface.

Realistically, we have (two/three?) choices:

  1. Include a small "*slog.Logger <-> Logger" adapter as part of this module. This would be a small amount of work and would reduce the barrier to entry for slog.Logger users. It's unlikely this would require much future maintenance either. However, it would require bumping the minimum supported Go to 1.21.
  2. Rewrite go-spiffe to only accept *slog.Logger (or an interface matching this) as part of a breaking release. Until then, users will have to implement the adapter type themselves. This is the most opinionated option and banks heavily on slog becoming the most popular over time, it's particularly awkward for users who already heavily use a library other than slog who would then have to write an adapter. However, it would also provide an opportunity to switch go-spiffe's logging style to be more structured.
  3. Do nothing - users will just have to implement the adapter themselves if they want to use go-spiffe with slog.

I think (1) is the most reasonable thing to do in the near-term, but perhaps worth evaluating (2) in future. (3) is also acceptable, but I think we'll see more and more people mentioning this as time goes on. I'm happy to raise a PR with an implementation of (1).

azdagron commented 5 months ago

(2) seems like a non-starter without going through a normal deprecation cycle. It's a possibility though if we stage the changes over a few months and mark the related methods and interfaces as deprecated so that linters can help aid the change.

Since we've already moved to go1.21 on this library, instead of an external facing adapter, we could add WithSlogger options to the two spots that currently take loggers (via WithLogger options, which we could deprecate). Internally we could wrap it with an unexported adapter.

Once we deprecate the Logger interface and WithLogger options, we could switch the internal logging facilities to slog.Logger only and remove all support for the Logger interface/options.

strideynet commented 5 months ago

(2) seems like a non-starter without going through a normal deprecation cycle. It's a possibility though if we stage the changes over a few months and mark the related methods and interfaces as deprecated so that linters can help aid the change.

Agreed - this is probably a little "drastic" to pursue quickly and there's no real need to wrap this up quickly.

instead of an external facing adapter, we could add WithSlogger options to the two spots that currently take loggers (via WithLogger options, which we could deprecate). Internally we could wrap it with an unexported adapter.

I really like this as a midterm solution - avoids making the adapter part of the public API of the package and the maintenance concerns that introduces.