feat(cloudflare): propagate tracing context through Queue messages#22303
feat(cloudflare): propagate tracing context through Queue messages#22303arthurfiorette wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
There are 4 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0bd582c. Configure here.
|
👋 @isaacs, @mydea, @andreiborza — Please review this PR when you get a chance! |
There was a problem hiding this comment.
One nit, one minor improvement, and one potentially substantial design choice. Looks good overall, though, and does indeed fix the bug!
Also, since this is a user-facing change (albeit small), we probably want to add a CHANGELOG.md entry like:
- feat(cloudflare): Add `enableQueueTracePropagation` to link Queue producer and consumer traces ([#22303](https://github.com/getsentry/sentry-javascript/pull/22303))| traceFlags: producerContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE, | ||
| isRemote: true, | ||
| }, | ||
| attributes: { [SEMANTIC_LINK_ATTRIBUTE_LINK_TYPE]: 'previous_trace' }, |
There was a problem hiding this comment.
This feels like it might be a weird fit. A previous_trace value here indicates that the link is part of a sequential chain of traces, for example a series of browser interactions that a user might perform. In the UI, it's going to be presented with that kind of sequential semantics.
However, this is more of a "fan out" relationship, not previous/next relationship, so I don't think previous_trace makes the most sense here. It might be better to just leave the type off, and let it be a plain old link.
@andreiborza @mydea Do you think I'm misreading this? What is your opinion on the semantics here?
| const sentryTrace = body[SENTRY_QUEUE_TRACE_KEY]; | ||
|
|
||
| // Once propagation is enabled this key is reserved transport metadata, even if its value was corrupted in transit. | ||
| delete body.__sentry_queue_trace__; |
There was a problem hiding this comment.
This should use the same const as the rest of the file. Also, delete can throw in strict mode if the object is frozen, so best to try/catch it.
| delete body.__sentry_queue_trace__; | |
| try { | |
| delete body[SENTRY_QUEUE_TRACE_KEY]; | |
| } catch { | |
| // best effort | |
| } |
| return startPublishSpan({ bindingName, bodySize: getBodySize(message) }, span => { | ||
| // Read trace data here so the carrier points to queue.publish, not its parent span. | ||
| const tracedMessage = enableTracePropagation | ||
| ? addQueueTraceContext(message, getTraceData()['sentry-trace']) | ||
| : message; | ||
|
|
||
| if (enableTracePropagation) { | ||
| span.setAttribute('messaging.message.body.size', getBodySize(tracedMessage)); | ||
| } |
There was a problem hiding this comment.
So, when trace propagation is enabled, we're calling getBodySize twice, and discarding the original value. It's not a huge cost, but it does incur a JSON.stringify, which can be excessive on large messages.
Consider something like this:
| return startPublishSpan({ bindingName, bodySize: getBodySize(message) }, span => { | |
| // Read trace data here so the carrier points to queue.publish, not its parent span. | |
| const tracedMessage = enableTracePropagation | |
| ? addQueueTraceContext(message, getTraceData()['sentry-trace']) | |
| : message; | |
| if (enableTracePropagation) { | |
| span.setAttribute('messaging.message.body.size', getBodySize(tracedMessage)); | |
| } | |
| return startPublishSpan({ bindingName, bodySize: undefined }, span => { | |
| // Read trace data here so the carrier points to queue.publish, not its parent span. | |
| const tracedMessage = enableTracePropagation | |
| ? addQueueTraceContext(message, getTraceData()['sentry-trace']) | |
| : message; | |
| span.setAttribute('messaging.message.body.size', getBodySize(tracedMessage)); |
| // Cloudflare Queues has no message-header API. Only record-like bodies can carry a named field; binary data and | ||
| // other structured-clone values must pass through untouched. | ||
| const prototype = Object.getPrototypeOf(body); | ||
| return prototype === Object.prototype || prototype === null; |
There was a problem hiding this comment.
Nice. Excludes all "fancy" objects. 👍

Adds opt-in distributed tracing across Cloudflare Queue producers and consumers.
When
enableQueueTracePropagationis enabled on both sides:sentry-traceandbaggageunder__sentry_queue_meta__.queue.processremains a root batch span and links to every producer represented in the batch.Span links are used instead of continuing a producer trace because a Queue batch can contain messages from unrelated traces. Selecting one producer as the parent would create false causal relationships for the other messages.
The option is disabled by default because Cloudflare Queues does not expose a message-header API. Producers must modify record-like message bodies, so consumers without matching instrumentation may observe the additional metadata field.
Closes #22298