Move logger context population so it can be reused by YARPC #155
Conversation
|
@sectioneight |
madhuravi
changed the title
| @@ -23,6 +23,9 @@ package fxcontext | |||
| import ( | |||
| gcontext "context" | |||
|
|
|||
| "github.com/opentracing/opentracing-go" | |||
| "github.com/uber/jaeger-client-go" | |||
|
|
|||
There was a problem hiding this comment.
nit: these go below fx imports
| // Note that opentracing.Tracer does not expose the tracer id | ||
| // We only inject tracing information for jaeger.Tracer | ||
| if jSpanCtx, ok := span.Context().(jaeger.SpanContext); ok { | ||
| traceLogger := c.Logger().With( |
There was a problem hiding this comment.
let's use the non-sugared version here for perf in the tight loop
There was a problem hiding this comment.
ulog.Logger With has some sentry magic. It adds the fields to sentry as well.
There was a problem hiding this comment.
Hmm, we should fix that. People should be able to add zap Fields directly and still get the same Sentry benefits. Particularly in a path like this (on the per-request loop) where we know the fields ahead of time, we really shouldn't be incurring the overhead of the interface{} conversion since we know it's expensive.
In other words, this should be something like:
traceLogger := c.Logger().Typed().With(
zap.StringField("traceID", traceID),
zap.StringField("spanID", spanID),
)
|
Let's also add some documentation to read me/godoc about this behavior so people don't have to dig through code to discover it |
|
@sectioneight Do you mean documentation to uhttp or context? Use for WithContextAwareLogger is only within UberFx. And for http, users will not have to set anything. It should just work. Do we want to just say that this is a thing we have with the http module and you can expect to see it in the logs? |
|
@madhuravi I meant that although it does "just work", we should document somewhere that we're auto-inserting trace/span IDs into the logs and the format they're in, where they come from, and why you might want to use them. |
No description provided.