Repository navigation
feat(cloudflare): propagate tracing context through Queue messages - #25306
arthurfiorette wants to merge 7 commits into
Conversation
- Replace `enableQueueTracePropagation` with `queueTracePropagationBindings`, matching `rpcTracePropagationBindings`: producers opt in per binding, consumers always strip and link. - Drop the `previous_trace` link type, a batch fans in producers rather than chaining traces. - Strip the carrier with `Reflect.deleteProperty` on the shared key so frozen bodies cannot throw. - Compute the publish body size once when propagation is enabled. - Move the queue integration assertions to span streaming. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
HI @arthurfiorette thanks for opening another PR for this! I'll leave the review up to @JPeer264 who has more context on the cloudflare specific details. Good idea to use span links. I think we should still set the |
…ueue-links # Conflicts: # CHANGELOG.md
|
I'll wait on reviewing that PR until #21777 is finished. In that PR is an open point on how we deal with span links for queues. This might be then also relevant for this PR. I think by mid next week I can start reviewing this. Hope the timeline is ok for you. |
JPeer264
left a comment
There was a problem hiding this comment.
Sorry that it took a while, but I was finally able to finish of the remaining work of the other PR, that blocked this PR.
It is good that we add a new queueTracePropagationBindings option, since the current one is only focusing on rpc (I originally thought of having a one size fits all option, but I don't think that would have been a good idea).
I added some comments, so please take a look at them. The rest of the code looks good, great work - thank you.
|
|
||
| ### Other Changes | ||
|
|
||
| - feat(cloudflare): Add `queueTracePropagationBindings` to link Queue producer and consumer traces ([#25306](https://github.com/getsentry/sentry-javascript/pull/25306)) |
There was a problem hiding this comment.
You can remove this, we auto generate it based on the PR title, then you won't have merge conflicts like now
| traceId: producerContext.traceId, | ||
| spanId: producerContext.spanId, | ||
| traceFlags: producerContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE, | ||
| isRemote: true, |
There was a problem hiding this comment.
I think we can remove the isRemote: true. Was there a specific point, why this has been added?
| isRemote: true, |
| links.push({ | ||
| context: { |
There was a problem hiding this comment.
The main reason why I held back reviewing this PR. Please add the link type attribute message_origin (https://develop.sentry.dev/sdk/telemetry/traces/span-links/#message_origin):
| links.push({ | |
| context: { | |
| links.push({ | |
| attributes: { | |
| [SENTRY_LINK_TYPE]: 'message_origin' | |
| }, | |
| context: { |
Please also make sure this is reflected in the tests.
| return undefined; | ||
| } | ||
|
|
||
| return { |
There was a problem hiding this comment.
There is a limit of 128kb in Cloudflare for messages: https://developers.cloudflare.com/queues/platform/limits/
We might need to document this in the code that this could be a caveat. It's not a lot of extra bytes, but may be causing issues in some cases (it's definitely going to be added in our docs then)
| return item; | ||
| } | ||
|
|
||
| if (isQueue(item)) { |
There was a problem hiding this comment.
I don't think we would need this new logic - it could stay as it was before if I'm not mistaken. We also shouldn't optimize for a changing config.
With the logic before we're caching per binding object, and each binding (queue) would have its own flag if it has trace propagation enabled or not. So I guess the following would work as well:
if (isQueue(item)) {
const bindingName = typeof prop === 'string' ? prop : String(prop);
const instrumented = instrumentQueueProducer(item, bindingName, shouldPropagateQueueTrace(bindingName));
instrumentedBindings.set(item, instrumented);
return instrumented;
}| * ); | ||
| * ``` | ||
| */ | ||
| queueTracePropagationBindings?: TracePropagationTargets; |
There was a problem hiding this comment.
note to myself. We might need to extend this in our Vite plugin to automatically add these in case they're in the same deployment
| @@ -1,18 +1,14 @@ | |||
| import { matchesTracePropagationTargets } from '@sentry/core'; | |||
| import { matchesTracePropagationTargets, type TracePropagationTargets } from '@sentry/core'; | |||
There was a problem hiding this comment.
Super low priority. Technically this file is doing more now than just rpcPropagation , so we could rename it to something more generic, but not a must. You can decide as you see fit.
This extends #22303, which was auto-closed for inactivity after its review comments slipped through my GitHub inbox. It is now synced with
developand addresses that review.Adds opt-in trace propagation from Cloudflare Queue producers to consumers.
queueTracePropagationBindings, plain-object message bodies carry the producer'ssentry-traceunder__sentry_queue_trace__.queue.processremains a root span and links to every distinct producer in the batch.Span links are used instead of continuing a producer trace because a Queue batch can contain messages from unrelated traces. Picking one producer as the parent would create false causal relationships for the other messages.
Propagation is opt-in per binding because Cloudflare Queues has no message-header API. The SDK has to modify the message body, so consumers without Sentry would see the extra field.
Changes since #22303:
enableQueueTracePropagation: booleanbecamequeueTracePropagationBindings, mirroringrpcTracePropagationBindings(feat(cloudflare)!: Replace enableRpcTracePropagation with rpcTracePropagationBindings #23486). Only the producer opts in, since it is the side that changes the payload. Consumers always strip and link, the same way RPC receivers handle__sentry_rpc_meta__.sentry.link.type: previous_trace. A batch fans in producers rather than forming a previous/next chain.Fixes #22309