log15 icon indicating copy to clipboard operation
log15 copied to clipboard

poor library recommendation

Open tve opened this issue 10 years ago • 3 comments

The log15 docs recommend that library writers expose a Log variable that can be set to a logger. https://godoc.org/gopkg.in/inconshreveable/log15.v2#hdr-Library_Use I think this is pretty poor practice, actually, because it defeats context loggers which is one of the features of log15 I really like. The recommendation made is better than nothing but there should also be a proper recommendation, which is to embed a logger into the appropriate objects exported by the library so the client can set a logger on a per-request or per-thread of execution basis.

tve avatar Jul 17 '15 17:07 tve

@tve Thanks for pointing it out! That's not actually poor practice, it's just poorly communicated how those two things should tie together. Your library should have context loggers embedded in the appropriate objects, but each of those should be created from your top-level global Log variable. Like so:

var Log = log.New()

func init() {
    Log.SetHandler(log.DiscardHandler())
}

type Foo struct {
    // etc.
    log.Logger
}

func NewFoo() *Foo {
    return &Foo{
        // create your new context logger from the library-global logger
        Logger: Log.New("obj", "foo"),
    }
}

In this way, a user of your library can set the handler of the library's top-level Logger to change how all of the context loggers work in each of the objects it creates.

inconshreveable avatar Jul 18 '15 14:07 inconshreveable

Looks like I didn't explain the problem I'm seeing but your response does allow me to do what I'm looking for. I have the situation where my server receives a request, creates a context logger for the request with the request ID, and wants to pass that down into libraries so when they log stuff it can be related to the request. In the example you gave, this is possible because Logger is a public field of Foo, so after creating a Foo I can reset its Logger to be the request context logger. Would this be worth adding to the README?

tve avatar Aug 21 '15 18:08 tve

Yeah, i think that would make a great addition to the readme. Happy to take a patch with that!

inconshreveable avatar Aug 21 '15 18:08 inconshreveable