Created
February 16, 2015 09:12
-
-
Save peterbourgon/9aa3b32ecb6b4f78c4b4 to your computer and use it in GitHub Desktop.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
<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