Skip to content

Instantly share code, notes, and snippets.

@peterbourgon
Created February 16, 2015 09:12
Show Gist options
  • Save peterbourgon/9aa3b32ecb6b4f78c4b4 to your computer and use it in GitHub Desktop.
Save peterbourgon/9aa3b32ecb6b4f78c4b4 to your computer and use it in GitHub Desktop.
<aybabtme> ptrb: i've thought of storing log entries into context.Context but found it would be too cumbersome `ctx.Value("log").(*log.Entry).Info("message")`
<aybabtme> ptrb: another thing i thought of doing was add a context.Context aware func to our logger, `log.Ctx(ctx).Info("message")`
<aybabtme> ptrb: but then it would mean all the fields in `ctx` would get logged, which im not sure we always want
<aybabtme> responding here to avoid derailing the PR more with my comments =P
<mattheath> what’s the advantage of logging with the context? is the aim being able to correlate specific log lines back to a specific request?
<mattheath> (which would be awesome)
<aybabtme> mattheath: have you read https://github.com/peterbourgon/gokit/pull/4#issuecomment-74424405? might provide ...context...
<aybabtme> =P
<mattheath> "context" nice ;)
<ptrb> aybabtme: I guess it's a tradeoff, do you complect your method definitions or the code within them
<ptrb> it's definitely a violation of bounded context to have func BusinessOperation() take a log object
<ptrb> ...or a context.Context, for that matter
<ptrb> but to get to your concrete example, I'd much rather do func Foo(ctx context.Context, ...) { e := ctx.Value("log").(*log.Entry); ... e.Info("message") }
<ptrb> than func Foo(ctx *context.Context, e *log.Entry, ...) { ... e.Info("message") }
<ptrb> because the former are chainable, i.e. composable
<ptrb> (among other reasons)
<aybabtme> i dont understand why they are chainable in this form and not in the other [
<ptrb> you can encode arbitrarily many contexts into a context.Context and keep the same signature
<ptrb> and (crucially) pass them to downstream functions without stripping the context of stuff you don't know about
<aybabtme> hum right
<ptrb> a logger or a log entry is just one of an arbitrarily large set of things that you might want to pass through a context chain
<aybabtme> however you lose type safety when you get back your logger from the context
<ptrb> aybabtme: not with a type assertion, non?
<aybabtme> if somehow something else than `*log.Entry` is at this value, the program will panic
<ptrb> well, that's fine.
<ptrb> gokit would own all of that
<aybabtme> you can 'l, ok := ctx.Value()` but its cumbersome =P
<mattheath> and you can wrap it so it wouldn’t panic
<mattheath> yeah ^^
<aybabtme> right, i dont think its a concern of gokit
<aybabtme> ptrb: sorry i abuse your patience. i didnt know what was meant with `bounded context violation` and googled http://martinfowler.com/bliki/BoundedContext.html however i dont see how passing log.Entry around violates it
<ptrb> func BusinessThing() is concerned with the business thing, its parameters should only be directly related to the business thing
<ptrb> when you need to pass a logger.Entry to it, you're violating its bounded context
<aybabtme> i see
<aybabtme> how about the recommandation to pass context.Context as the first arg?
<aybabtme> *recommendation
<ptrb> so that's one way -- that minimizes the violation of context to one flexible argument
<ptrb> and my gut reaction is that's a pretty reasonable compromise
<ptrb> another way is to use wrappers, I'll let tsenart explain that one :)
<ptrb> it's more pure, but more typing
<aybabtme> so your gripe is mostly that a 2nd arg is pushing the compromise?
<ptrb> aybabtme: yes, but also that a seond param of type *log.Entry is *really* pushing the compromise, because that's such a specific and concrete thing
<aybabtme> i think it would make sense to make a wrapper that recovers a `log.Entry` from a context, or returns a new one if none exists
<mattheath> yeah you could easily pass the context to the logger, and if it can’t recover a log entry from teh context just creates a new one
<ptrb> aybabtme: can you play.golang an example?
<aybabtme> sorry im diverting the conversation for my own needs, however talking about it helps me figure out this thing that's been annoying me for a while
<ptrb> of course! this is interesting to me too :)
<aybabtme> doesnt compile but that's the idea: http://play.golang.org/p/gSYedjlFbm
<ptrb> looking
<ptrb> aybabtme: yes, exactly. the BusinessThings might take other parameters after the context.Context
<ptrb> and the "log" string literal should be a package-level var, so users can change it, if they like
<ptrb> aybabtme: here's another admittedly more verbose strategy, which is perhaps more "pure" -- http://play.golang.org/p/Yqqr_DH8F7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment