Configurable Logging

[256dpi]

There are various locations where log.Printf I used to log errors, warnings and info messages. Since certmagic is used as a library one should be able to provide a custom logging interface if the logging to Stderr is not desired.

Similar to the already existing OnEvent(string, interface{}) callback I would propose the addition of the OnError(error) and OnInfo(string) callbacks. The default interface would log the errors to Stderr and info messages to Stdout.

I'm happy to put together a pull request once we agree on the final interface. 🚀

[mholt]

What about a package variable var Logger log.Logger or something? (or a struct field)

[256dpi]

Using log.Logger does not allow you to provide your own type that processes the log data. To overcome that we would need to create an interface that defines the logging methods. However, to be compatible with log.Logger it would need to look like this:

type Logger {
    Printf(format string, v ...interface{})
}

With this approach important metadata like the log level is part of the format and thus it is hard to clearly know if this is an important error or an info message.

On top of that, I think that errors should be treated differently. In my application, I log errors to centralized logging platform and send errors to sentry (to get email notifications). Having the options to treat them differently would be a huge win since certmagic is a crucial component.

Therefore I recommend again the addition of OnError(error) or similar to handle errors that prevented certmagic from doing its job and potentially caused the abortion of requests or connections.

Additionally we may continue with the Logger interface approach or a callback that is quicker to implement with a similar signature. Here I think it is also important that it should be safe to disable the informational logging without compromising stability. This means, all log info should be strictly informational and not convey any warnings that lead into future errors.

@mholt What is the purpose of the current OnEvent callback? Is this used by caddy to take actions after certain events? Is there potential to unify it with the logging?

Dave Cheney has a great post on the matter: https://dave.cheney.net/2015/11/05/lets-talk-about-logging.

[shravanshetty1]

type Logger interface {
	Printf(format string, v ...interface{})
}

var ErrLog Logger = log.New(os.Stderr,"[ERROR]",log.LstdFlags)
var InfoLog Logger = log.New(os.Stdout,"[INFO]",log.LstdFlags)
var WarnLog Logger = log.New(os.Stdout,"[WARNING]",log.LstdFlags)

This would be added to logging.go. All log.printf statements would be changed like this

log.Printf("[INFO][%s] Lock for '%s' is stale; removing then retrying: %s",
				fs, key, filename)

Will be changed to

InfoLog.Printf("[%s] Lock for '%s' is stale; removing then retrying: %s",
				fs, key, filename)

Same for error and warning logs.

Can i go ahead and make these changes?
@256dpi You can add instructions to send to sentry in your own printf func

[mholt]

Actually, since Caddy 2 uses zap for logging, I'd like CertMagic to be able to use that as well. If we're going to change how CertMagic does logging, let's use *zap.Logger so it's more compatible with Caddy 2 logs.

[desimone]

@mholt First of all, thanks for certmagic, it's a great project.

Would you be open to a logger interface so that users of other logging systems (we happen to use zerolog) can use their existing loggers? I'm super flexible as to what the interface actually looks like, and happy to take a stab at implementing it if you think this would be workable.

[mholt]

@desimone Hmm, as long as the interface preserves the zero-allocation benefits (as much as possible) and allows us to use zap logs flexibly, I'd be down for that, sure. I was going to spend more time on logging when I work on #71 which will probably use the same interface.

Honestly I was thinking of just using zap directly. It's virtually the same as zerolog in terms of benefits. It would be a lot simpler. Will that not be acceptable?

[desimone]

After digging in a bit more in how zap works, that might actually work just fine. Let me know how I can help.

…

On Mon, May 4, 2020 at 10:06 PM Matt Holt ***@***.***> wrote: @desimone <https://github.com/desimone> Hmm, as long as the interface preserves the zero-allocation benefits (as much as possible) and allows us to use zap logs flexibly, I'd be down for that, sure. I was going to spend more time on logging when I work on #71 <#71> which will probably use the same interface. Honestly I was thinking of just using zap directly. It's virtually the same as zerolog in terms of benefits. It would be a lot simpler. Will that not be acceptable? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALZFMMZJ3CAAKSDSM6ZJ7TRP6NE5ANCNFSM4GSTXIFQ> .

[mholt]

@desimone Did you have an interface in mind that would work with both and preserve the zero-alloc benefits? If so, what do you propose?

Otherwise, I'll be getting around to upgrading this library sooner or later -- I have a lot of catch-up to from the Caddy 2 release.

[mholt]

This is almost done, I'm just using zap loggers in the structs. Logging is optional, and if you set the loggers, that will enable logging. Pretty straightforward.