Repository navigation
feat(cloudflare): propagate tracing context through Queue messages - #25306
arthurfiorette wants to merge 10 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
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.
| * ); | ||
| * ``` | ||
| */ | ||
| 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.
…ueue-links # Conflicts: # CHANGELOG.md # packages/cloudflare/src/instrumentations/worker/instrumentEnv.ts # packages/cloudflare/test/instrumentations/instrumentEnv.test.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 27dd16c. Configure here.
|
Review comments fixed @JPeer264! |
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