Enable Trace Propagation#65
Conversation
When the configuration parameter `EnableTracePropagation` is enabled, the traces generated by river workers will be *linked* to the traces that enqueued them. This is done by use of metadata parameters, following the example in River's documentation for the middleware functionality (https://riverqueue.com/docs/middleware). This uses a *link* instead of directly making a child span which is semantically more correct. Fixes riverqueue#41 (I believe, at least).
|
@codex review |
|
Just an FYI: we've rolled this code out in production at my company and it has been successful (traces from jobs are properly linking to whatever kicked them off). |
brandur
left a comment
brandur
left a comment
There was a problem hiding this comment.
Good stuff! Want to add a changelog entry?
| } | ||
| } | ||
| extracted := propagation.TraceContext{}.Extract(ctx, carrier) | ||
| return trace.SpanFromContext(extracted).SpanContext() |
There was a problem hiding this comment.
I think this piece is subtly not-quite-right in that the purpose of this function is always to pull a span context out of the given metadata and we should never be looking to get out of active context. What do you think about removing the ctx parameter from extractSpanContext in favor of using context.Background() here? If no span context is founded in metadata, then nothing should be extracted.
There was a problem hiding this comment.
Added in 0ea8d93 - along with a comment explaining what's goiung on.
There was a problem hiding this comment.
Can you take a look at the lint problem below? For this one, you probably just want to add a //nolint:contextcheck on the line.
|
|
||
| // EnableTracePropagation injects W3C trace context (traceparent/tracestate) | ||
| // into job metadata on insert and extracts it on work, adding a span link | ||
| // from the work span back to the span that enqueued the job. A link is used |
There was a problem hiding this comment.
Do you want to maybe move the sentence starting from "A link is used ..." from here down to this line?
startOpts = append(startOpts, trace.WithLinks(trace.Link{SpanContext: sc}))
It feels like rationalizing the use of a link here is a little bit TMI for the docblock and belongs more in an internal comment.
There was a problem hiding this comment.
Good call!
Added in 648c0f8. |
brandur
left a comment
brandur
left a comment
There was a problem hiding this comment.
Just a couple minor lint things.
| } | ||
| } | ||
| extracted := propagation.TraceContext{}.Extract(ctx, carrier) | ||
| return trace.SpanFromContext(extracted).SpanContext() |
There was a problem hiding this comment.
Can you take a look at the lint problem below? For this one, you probably just want to add a //nolint:contextcheck on the line.
Co-authored-by: Brandur <brandur@brandur.org>
This is no longer used. Also fixes lints.
|
Should be all fixed now! |
|
thx! |
2 participants
When the configuration parameter
EnableTracePropagationis enabled, the traces generated by river workers will be linked to the traces that enqueued them. This is done by use of metadata parameters, following the example in River's documentation for the middleware functionality (https://riverqueue.com/docs/middleware).This uses a link instead of directly making a child span which is semantically more correct—if a parent span is used, you get weird situations where a POST request that returns in 50ms somehow contains a trace that starts two hours later and takes 10 minutes. River jobs are async, after all.
Fixes #41 (I believe, at least).