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

Support *slog.Logger as configured logger #281

Open
strideynet opened this issue Apr 16, 2024 · 2 comments
Open

Support *slog.Logger as configured logger #281

strideynet opened this issue Apr 16, 2024 · 2 comments

Comments

@strideynet
Copy link
Contributor

strideynet commented Apr 16, 2024

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

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

(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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants