feat(cloudflare)!: Replace enableRpcTracePropagation with rpcTracePropagationBindings - #23486
feat(cloudflare)!: Replace enableRpcTracePropagation with rpcTracePropagationBindings#23486JPeer264 wants to merge 7 commits into
Conversation
size-limit report 📦
|
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d68e2df. Configure here.
d68e2df to
74e4152
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
This mostly looks good, and does solve the intended problem. I think there's an improvement that ought to be done to make the gating match what the comment says, and a possible improvement to match core's trace propagation RegExp handling, but once those are addressed (or dismissed for good reason) then lgtm.
| * Propagation over `stub.fetch()` and service binding `fetch()` uses HTTP headers and is not | ||
| * affected by this option. |
There was a problem hiding this comment.
I could be misreading this, but it says here that it's not gating on this option, but then in instrumentEnv in packages/cloudflare/src/instrumentations/worker/instrumentEnv.ts on line 97, it does this:
if (!shouldPropagateRpcTrace(String(prop))) {
return item;
}which is where it uses this option, and that's before the instrumentDurableObjectNamespace(item) and instrumentFetcher calls. So I think it is affected by this option, then, and maybe shouldn't be?
Suggestion: always apply instrumentDurableObjectNamespace and the JSRPC fetch wrap, and move shouldPropagateRpcTrace(...) down so it guards only the appendRpcMeta decision, to around instrumentEnv.ts line 117 and the equivalent call in instrumentDurableObjectNamespace.ts on line 55.
That'll need the binding name threaded into instrumentDurableObjectNamespace, but that's a pretty minor change.
There was a problem hiding this comment.
That is an insane catch tbh! It's a good idea to always apply it on fetch calls, as this wouldn't break anything, even on uninstrumented workers. The actual RPC calls are the issue - I'll update that.
|
|
||
| // Strings must match a binding name exactly, without this, an entry of `DB` would also enable | ||
| // propagation for a binding named `MY_DB`. Regular expressions still give pattern matching. | ||
| return (bindingName: string) => stringMatchesSomePattern(bindingName, targets, true); |
There was a problem hiding this comment.
This should probably get the same defensive fix as core did in ee5a772, so that /g and /y flags don't mess things up. Either use matchesTracePropagationTargets (which is for this purpose) or call normalizeRegExpTarget like it does.
There was a problem hiding this comment.
Oh, also, this should be case-insensitive if the goal is to match tracePropagationTargets, since it became case insensitive in that commit as well.
There was a problem hiding this comment.
this should be case-insensitive if the goal is to match tracePropagationTargets, since it became case insensitive in that commit as well.
Yes I totally. I planned this to be in a separate PR so this PR can be backported to v10 with the same functionality and the case-insensitive change lives only in v11. Will prepare that today.
so that /g and /y flags don't mess things up
that is a good point, I would have missed that. Same as above I'll keep that in v11 to match things properly
| } | ||
|
|
||
| if (!options?.enableRpcTracePropagation) { | ||
| if (!shouldPropagateRpcTrace(String(prop))) { |
There was a problem hiding this comment.
See the comment on client.ts, this should move down so that it only guards what the JSDoc comment says gates on it.
Suggestion: always apply
instrumentDurableObjectNamespaceand the JSRPCfetchwrap, and moveshouldPropagateRpcTrace(...)down so it guards only theappendRpcMetadecision, to aroundinstrumentEnv.tsline 117 and the equivalent call ininstrumentDurableObjectNamespace.tson line 55.That'll need the binding name threaded into
instrumentDurableObjectNamespace, but that's a pretty minor change.
| return isObjectLike(item) || typeof item === 'function'; | ||
| } | ||
|
|
||
| const instrumentedBindings = new WeakMap<object, unknown>(); |
There was a problem hiding this comment.
This is shared across all envs, so it can leak to everything in the same isolate. Could be a problem, since this method is called from a bunch of different places, each with its own options object.
Recommend: key the cache on (binding, decision) or move it inside the per-instrumentEnv closure.
There was a problem hiding this comment.
Could you clarify that I think I can't follow on this. What is an "env" in this case? In practice each DurableObject, Workflow and WorkerEntrypoint to have separate isolates (at least with my tests on prod) and they do not share any memory - so in practice the options wouldn't change. If I read it correctly this comment would only be true if two DurableObjects with different options.rpcTracePropagationBindings occur on one isolate, right?
| ); | ||
| ``` | ||
|
|
||
| - The `instrumentPrototypeMethods` option of `instrumentDurableObjectWithSentry` was removed. A Durable Object instruments its RPC methods unconditionally now, so there is nothing to replace it with on the receiver. |
There was a problem hiding this comment.
I'd recommend adding a note here that a Durable Object's prototype methods are now replaced unconditionally, rather than just "there is nothing to replace it with on the receiver", because that sounds a bit like "nothing happens".
There was a problem hiding this comment.
This is being used as just a presence test, so it could use hasRpcMeta no?
There was a problem hiding this comment.
This logic is now gone, so we don't check for that anymore. I'm using the "check" from inside wrapMethodWithSentry now
| return captureMethod; | ||
| } | ||
|
|
||
| const tracedMethod = wrapMethodWithSentry( |
There was a problem hiding this comment.
low/question: Previously this method was only generated lazily, if the RPC trace prop was enabled, but now it's being built every time. Is it possible/worthwhile to generate both lazily, since only one of them ends up being used?
There was a problem hiding this comment.
Hm. I just realized that tracedMethod is kinda the same as boundMethod above, so it could be combined into one
| * ``` | ||
| */ | ||
| enableRpcTracePropagation?: boolean; | ||
| rpcTracePropagationTargets?: Array<string | RegExp>; |
There was a problem hiding this comment.
This could use the TracePropagationTargets type from core, clearer communication of intent.
There was a problem hiding this comment.
Oh, also, this is kind of confusingly named, since it's not matched against URLs, but just against binding env strings? Consider: rpcTracePropagationBindings
There was a problem hiding this comment.
This could use the TracePropagationTargets type from core, clearer communication of intent.
Makes sense. I'll use that one
this is kind of confusingly named, since it's not matched against URLs, but just against binding env strings? Consider: rpcTracePropagationBindings
That is an amazing name. Love it!
JPeer264
changed the title
8a2c709 to
7e69829
Compare
…pagationTargets `enableRpcTracePropagation: true` propagated trace context to every Durable Object namespace and service binding on `env`. RPC carries that context as a trailing argument, and only a Sentry-instrumented receiver strips it again, so any receiver the user does not own saw an extra argument. `rpcTracePropagationTargets` names the bindings to propagate to instead, mirroring `tracePropagationTargets`. Strings match a binding name exactly, regular expressions match by pattern. The option is now caller-only. Receivers no longer take a switch: a Durable Object instruments its RPC methods unconditionally, matching what a WorkerEntrypoint already did. `extractRpcMeta` only strips a trailing argument that actually carries `__sentry_rpc_meta__`, so a call arriving without metadata is untouched. BREAKING CHANGE: `enableRpcTracePropagation` is removed. Replace it on the caller with `rpcTracePropagationTargets` listing the bindings you call, and drop it from receivers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…racePropagationTargets
862d405 to
f6bdbcb
Compare
2 participants
closes #23233
This PR existed because of this comment: #23363 (comment)
We already have an option
tracePropagationTargetsand they are set to a safe default list in the browser (because in the browser we can't enable it for all addresses by default). To keep it consistentrpcTracePropagationTargetsis the equivalent for RPC calls, while this has the same concerns as the browser - we can't target all RPC calls OOTB, as this would lead to issues as seen in #23233We now remove the
enableRpcTracePropagation: booleanentirely with a allow listrpcTracePropagationTargetsoption, that is empty by default and is therefore the equivalent ofenableRpcTracePropagation: false(in a later PR we have a prefilled allowlist, like in the browser, when users do have our Vite plugin enabled).rpcTracePropagationTargetsonly has to be enabled on the sender worker. The receiving worker does not have to do anything, as we automatically detect if there is a payload from us included. If there is, we strip it - it should be safe enough to listen to this by default, as we already have the last RPC parameter properly prefixed.(when this is being backported to v10, we keep
enableRpcTracePropagation: boolean, deprecate it and overruleenableRpcTracePropagationwhenrpcTracePropagationTargetsis set (so the allow list is stronger than enabling it for everything - the receiving worker won't change for the backport)