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

Feature Request: Use log/slog instead of Zap #254

Closed
stephenafamo opened this issue Oct 27, 2023 · 8 comments
Closed

Feature Request: Use log/slog instead of Zap #254

stephenafamo opened this issue Oct 27, 2023 · 8 comments
Labels
feature request Request for new feature or functionality

Comments

@stephenafamo
Copy link

Now that structured logging is part of the standard libarary, are there any plans to switch to log/slog?

@stephenafamo stephenafamo added the feature request Request for new feature or functionality label Oct 27, 2023
@francislavoie
Copy link
Member

Since Caddy is the primary consumer of certmagic and the whole Caddy plugin ecosystem uses zap, it would be very difficult to change that at this point.

I could see a Caddy v3 causing us to change over, but I don't think we will take the effort to do that anytime soon.

@stephenafamo
Copy link
Author

I guess the ideal situation is to have a way to create a zapcore.Core that pipes output to a slog.Handler and then create a *zap.Logger from that Core that will be passed to certmagic.

@francislavoie
Copy link
Member

I guess so. I don't know how interop between them looks like, I haven't looked into it. But that sounds reasonable for now.

@mholt
Copy link
Member

mholt commented Oct 27, 2023

Are std libs lobs zero-allocation?

If they aren't, I don't think moving will have any performance benefit.

@stephenafamo
Copy link
Author

For the user, it wouldn't be about performance but about interoperability.

For example, if I am starting a new project now, I will use slog for my structured logging. It would be nice that if I need to configure certmagic I can pass in my already existing logger.

I understand going all in on zap before slog was available, or even just in the interest of performance.

Thinking about this more, perhaps there is no need for certmagic and the rest of the Caddy ecosystem to change, there just needs to be an easy way to create a zap logger from slog.

@mholt
Copy link
Member

mholt commented Oct 27, 2023

Understood; but we can't impact the TLS server's performance like that -- logs that allocate would significantly reduce the performance of the server given how many logs we emit.

But, yeah, if what you're actually asking for is interop, then maybe a feature request for zap to integrate with slog in some way would be the best way to go. It wouldn't necessarily be as fast of course, but then CertMagic wouldn't have to change and you could still use your slog with CertMagic.

I'll close this but we can reopen if something becomes actionable for us 👍

@mholt mholt closed this as not planned Won't fix, can't repro, duplicate, stale Oct 27, 2023
@francislavoie
Copy link
Member

francislavoie commented Oct 27, 2023

Did some quick googling...

slog recommends using the Logger.LogAttrs API for zero-allocation logging (instead of Logger.Log).

Apparently there's interop between slog can use zap as a logging backend if a Handler is provided. https://github.com/chanchal1987/zaphandler is an example.

@francislavoie
Copy link
Member

Actually, looks like zap added some compat in uber-go/zap#1246

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for new feature or functionality
Projects
None yet
Development

No branches or pull requests

3 participants