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

poor library recommendation #61

Open
tve opened this issue Jul 17, 2015 · 3 comments
Open

poor library recommendation #61

tve opened this issue Jul 17, 2015 · 3 comments

Comments

@tve
Copy link

tve commented Jul 17, 2015

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.

@inconshreveable
Copy link
Owner

@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.

warpfork added a commit to polydawn/repeatr that referenced this issue Aug 2, 2015
It'll be nice to have a little color through there, at least.  This may not be the ultimate state of this code, either, though -- this is an area where interactive use might still benefit from a special touch.  But, we don't have fully baked concepts of progress bars for most of this either, so, one step at a time.

Configuration is currently nonexistent.  I'm going to assume this is heading for human eyes.  There's a good suggestion of how to do configurability (at a library level, at least, which is also not exactly the domain problem here...) at inconshreveable/log15#61 (comment) which we may consider adopting as it becomes an issue.

Some sprintfs are used for the messages.  This is on the theory that those lines should be readable and meaningful without scanning down the whole context list (maybe I'd feel differently if context was before, then message, then parameters from the log call?)... this may not be a scalable or reasonable idea; it just looked weird to say "starting materialize" and *nothing else*.  So, going with it for now; may change.

Signed-off-by: Eric Myhre <[email protected]>
@tve
Copy link
Author

tve commented Aug 21, 2015

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?

@inconshreveable
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants