Drop zap logger dependency in favor of slog?

[stefanvanburen]

Not sure if you have the stomach for another breaking change after just releasing v2, but the only external dependency this package has is zap. Since you're targeting go 1.21, you could switch to using the builtin slog package for logging.

[francislavoie]

We use zap here because the primary consumer of this library is Caddy which is designed around using zap. But I agree we can probably switch to slog here since we already have the slog->zap bridge wired up in Caddy now.

[mholt]

We use zap for its efficiency though. My understanding is that slog is not designed for performance.

[mholt]

Okay, correction: it does have an optional API for zero-allocation logging. Could be worth looking into.

[jantytgat]

I'm considering switching from go-lego to this library, as it has a much cleaner interface that works well within my custom scenario. I'm trying to eliminate all custom loggers in favor of the slog package, so this would be nice!

Could I help in facilitating this change?

[jantytgat]

@mholt I created a fork and replaced all the zap-logger calls with similar calls for slog.
I also ran a quick test based on your "plumbing" example, and it seems to work even with my custom slog logger.

What do you think the best way is to move forward here?

[mholt]

@jantytgat Oh cool, thanks! Go ahead and submit a pull request from your fork, and I'll do a review :)

[francislavoie]

Will also need to figure out how to wire through the slog logger from Caddy into certmagic then through to acmez. Certmagic will likely need to accept taking both kinds of loggers or something, unless the logger can be adapted in certmagic before passing it down.

[jantytgat]

Will also need to figure out how to wire through the slog logger from Caddy into certmagic then through to acmez. Certmagic will likely need to accept taking both kinds of loggers or something, unless the logger can be adapted in certmagic before passing it down.

That should be as easy as:

package main

import (
    "log/slog"

    "go.uber.org/zap"
    "go.uber.org/zap/exp/zapslog"
)

func main() {
    zapL := zap.Must(zap.NewProduction())

    defer zapL.Sync()

    logger := slog.New(zapslog.NewHandler(zapL.Core(), nil))

    logger.Info(
        "incoming request",
        slog.String("method", "GET"),
        slog.String("path", "/api/user"),
        slog.Int("status", 200),
    )
}

Taken from https://betterstack.com/community/guides/logging/logging-in-go/#using-third-party-logging-backends-with-slog

This is partly how I created a "wrapper" around slog to allow for the log level TRACE (slog.Level(-8)), and to be able to get the logger pointer from a context if desired. (Which is not the case for this package)

[francislavoie]

Yeah we already have that in Caddy, i.e. ctx.Slogger(). But we'll need to also do slog.New(zapslog.NewHandler(logger.Core(), nil)) in certmagic to pass it into acmez until certmagic's config API is updated to take a slog (or optionally both in case we want to keep backwards compat for consumers of certmagic, idk).

[jantytgat]

Gotcha, so basically this is a breaking change, and should be in v3 (although v2 isn't that old)

[mholt]

CertMagic's exported API can still change; we can add to ACMEz's API but I don't want to make a breaking change. What would be breaking in acmez?

[francislavoie]

@mholt if the goal is to remove the zap dependency in acmez, a breaking change is necessary, because currently zap is part of the config API. So the change would be to swap the config to take in an slogger instead of a zap logger.

[mholt]

Ah, I was thinking of putting it side-by-side, maybe?

[jantytgat]

Hoe would you see that?

Have a method that takes zap, and another one to take slog.

Then have zap embedded as a handler for slog like in the example above, so we can have all logging calls based on slog?

[francislavoie]

If it's side by side, it doesn't solve anything cause we can't remove the dependency @mholt

[mholt]

Do I understand correctly that zap can consume slog but slog can't consume zap?

v3 is fine, I guess, just annoying 🙃 but we already had v2, so.

[jantytgat]

Yes, you can pass zap as a handler for slog, but not the other way around.

[mohammed90]

What about changing the field from *zap.Logger to an interface defined in acme{,z} which *zap.Logger implements but is also adaptable somehow to slog? This minimizes the breakage, though not completely eliminate it.

[jantytgat]

I'm not sure that would work, given the public api for slog and zap appear to be very different. (Unless I still don't fully grasp the implicit interfaces in go)

Basically what you are suggesting is to define a ln interface to which both could adhere. So far as function names, defining an interface might seem fine.

The biggest challenge would be the big differences in parameters passed into each implementation. Zap appears to use encoders, which slog doesn't have.

[jantytgat]

@mholt, given you're OK with a potential v3, I'll clean up all the references to zap in acmez as well (not only the acme package) and then create a pull request. We can move forward from there!

[jantytgat]

Pull request in #30

[jantytgat]

@stefanvanburen changes have been merged in master ;-)

[stefanvanburen]

Thanks for getting this over the line, looking forward to v3! :-)

[stefanvanburen]

hi @mholt, I think there's one last thing to do here: tag the v3 version in the repo. Otherwise go get will use the pseudo-commit versions:

$ go get github.com/mholt/acmez/v3
go: downloading github.com/mholt/acmez/v3 v3.0.0-20241214053340-45433dfc1161
go: added github.com/mholt/acmez/v3 v3.0.0-20241214053340-45433dfc1161

[mholt]

@jantytgat FYI, I think the suggestion in that article:

logger := slog.New(zapslog.NewHandler(zapL.Core(), nil))

is buggy. Passing a nil argument as a handler option results in a panic: caddyserver/caddy@ed1c594#commitcomment-150769508 -- one that I haven't encountered personally but I did question its veracity when I was propagating the changes through to Caddy.

[jantytgat]

I'll try to look at it asap.
Funny that it panics, because of two reasons:

• zapslog.NewHandler expects an array of options, hence the nil being passed.

Also, the handlers are processed using a for-loop, which shouldn't do anything when the input is nil.

• the package zapslog requires go 1.21, and you're already above that with caddy.

[mholt]

There's nothing you need to fix, I just wanted to let you know that the article was wrong.

The array isn't empty when nil is passed in, it has length one.