Proposal: Remove log.DefaultLogger

[ChrisHines]

Rationale

Logging is an application concern and should be controlled by applications. Each application should decide how log events are filtered and routed. Widely reusable (open source) libraries shouldn't do any logging, preferring to return errors or provide some other mechanism for event notification and leave it to the application to hook that into its choice for logging if desired. Organizations may enforce a logging standard for all of their internal packages. In that case internal libraries can reasonably assume a standard approach to logging and produce log events in accordance, but the application should still control filtering and routing.

Recommended Best Practice

Each source of log events should provide a mechanism for accepting a Logger implementation from the main application. The scope of the accepted Logger determines the granularity of control over filtering and routing available. Some possibilities:

• Function arguments
• Context value or constructor argument
• Package scoped function or variable

Packages that do not create log events do not need any of these. Thus, log.DefaultLogger is not needed as package log is not a source of log events.

Of the above choices, a package scoped logger is attractive. Ignoring concurrency issues for the time being, each package that creates log events could include a declaration such as:

var Log log.Logger = log.NewDiscardLogger()

Now the application can control logging for each package by changing its Log variable. If an application wanted to identify the source package for each log event it could do so by supplying a different contextual logger to each package. For example:

packageA.Log = log.With(logger, "module", "packageA")

[ChrisHines]

One more thought. Organizations that wish to standardize their logging across projects will likely create an internal logging package built from gokit/log pieces and other packages (e.g. lumberjack). Such a package would define local logging policies. It would construct a base Logger from configuration data. It may choose to export a DefaultLogger if desired. In short, it encodes the local policy decisions that are out of scope for a general purpose library such as go-kit.

[harlow]

I like this a lot var Log log.Logger = log.NewDiscardLogger(). I'm curious what the concurrency issues are here?

[ChrisHines]

The concurrency issue arises if one goroutine assigns a new value to Log while another goroutine is logging with it. There could be a data race accessing the Log variable. I have an idea to handle this but didn't want to complicate the discussion here.

[harlow]

Ok I see. That makes sense 👍

[peterbourgon]

Logging is an application concern and should be controlled by applications. Each application should decide how log events are filtered and routed. Widely reusable (open source) libraries shouldn't do any logging, preferring to return errors or provide some other mechanism for event notification and leave it to the application to hook that into its choice for logging if desired.

I completely agree with the first two sentences, but the bolded one gives me pause. There are situations where this isn't practical. Consider a library that maintains a persistent connection to some backing service. Intermittent failures should be reported, so operators can correlate problems, but parameterizing a logger or reporting channel in the API likely violates principles of encapsulation.

That said — the package level Logger solves this problem quite elegantly. It could even be more sophisticated, where necessary. For example, if a library wants to emit multiple levels of log information, we could have multiple unexported package-level Loggers, set with a single exported function.

package mylib

var (
    logDebug = log.NewSwapLogger(log.NewDiscardLogger())
    logInfo  = log.NewSwapLogger(log.NewDiscardLogger())
    logError = log.NewSwapLogger(log.NewDiscardLogger())
)

func SetLog(debug, info, error log.Logger) {
    logDebug.Swap(debug)
    logInfo.Swap(info)
    logError.Swap(error)
}

So, 👍 from me on this idea!

[ChrisHines]

Consider a library that maintains a persistent connection to some backing service. Intermittent failures should be reported, so operators can correlate problems, but parameterizing a logger or reporting channel in the API likely violates principles of encapsulation.

Yes, there will be exceptions to the guidelines, but I think they should be rare. Libraries that choose not to follow the guidelines should understand that they are coupling themselves to a particular logging API and possibly making the library less suitable for certain uses. That is a valid choice, as long as it is made consciously.

[peterbourgon]

This is done!