feat(cloudflare): Add trace propagation for RPC method calls#20343
feat(cloudflare): Add trace propagation for RPC method calls#20343
Conversation
![sentry[bot]](https://avatars.github.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.github.com/in/1210556?s=60&v=4){kind=small}
size-limit report 📦
|
|
Moved to draft, as tests are failing and I have to change 1-2 things that could reduce the amount lines added |
…pagation (#20345) follow up to #19991 It is better to release it first with an option to be enabled, that would then also be in line with #20343, otherwise `.fetch()` RPC calls would work without any option and the actual Cap'n'Proto RPC calls wouldn't work without. That would be an odd experience. ### New option: `enableRpcTracePropagation` > `instrumentPrototypeMethods` has been deprecated in favor of `enableRpcTracePropagation` Replaces the deprecated `instrumentPrototypeMethods` option with a clearer name that describes what it actually does. This option must be enabled on **both** the caller (Worker) and receiver (Durable Object) sides for trace propagation to work. It is also worth to mention that the implementation of "instrumenting prototype methods" has changed to a Proxy. ```ts // Worker side export default Sentry.withSentry( (env) => ({ dsn: env.SENTRY_DSN, enableRpcTracePropagation: true, }), handler, ); // Durable Object side export const MyDurableObject = Sentry.instrumentDurableObjectWithSentry( (env) => ({ dsn: env.SENTRY_DSN, enableRpcTracePropagation: true, }), MyDurableObjectBase, ); ```
…pagation (#20345) follow up to #19991 It is better to release it first with an option to be enabled, that would then also be in line with #20343, otherwise `.fetch()` RPC calls would work without any option and the actual Cap'n'Proto RPC calls wouldn't work without. That would be an odd experience. ### New option: `enableRpcTracePropagation` > `instrumentPrototypeMethods` has been deprecated in favor of `enableRpcTracePropagation` Replaces the deprecated `instrumentPrototypeMethods` option with a clearer name that describes what it actually does. This option must be enabled on **both** the caller (Worker) and receiver (Durable Object) sides for trace propagation to work. It is also worth to mention that the implementation of "instrumenting prototype methods" has changed to a Proxy. ```ts // Worker side export default Sentry.withSentry( (env) => ({ dsn: env.SENTRY_DSN, enableRpcTracePropagation: true, }), handler, ); // Durable Object side export const MyDurableObject = Sentry.instrumentDurableObjectWithSentry( (env) => ({ dsn: env.SENTRY_DSN, enableRpcTracePropagation: true, }), MyDurableObjectBase, ); ```
JPeer264
force-pushed
the
jp/real-rpc
branch
3 times, most recently
from
8a3eeb3 to
08d395b
Compare
s1gr1d
left a comment
s1gr1d
left a comment
There was a problem hiding this comment.
It's a good approach for this - first I thought about using a more hidden Symbol or a non-enumerable property here but this wouldn't survive serialization with Cap'n Proto.
Just one comment to define the key a bit tighter.
| return false; | ||
| } | ||
| const sentry = (value as SentryRpcMeta).__sentry; | ||
| return typeof sentry === 'object' && sentry !== null; |
There was a problem hiding this comment.
You could make the check more strict here - just to make sure this is really from us.
| return typeof sentry === 'object' && sentry !== null; | |
| return ( | |
| typeof sentry === 'object' && | |
| sentry !== null && | |
| ('sentry-trace' in sentry || 'baggage' in sentry) | |
| ); |
There was a problem hiding this comment.
I would answer it like this one: #20343 (comment)
There was a problem hiding this comment.
yeah now with the longer key, it's VERY unlikely
| @@ -213,6 +228,13 @@ export function wrapMethodWithSentry<T extends OriginalMethod>( | |||
| }); | |||
| }; | |||
|
|
|||
| if (rpcMeta) { | |||
| return continueTrace( | |||
| { sentryTrace: rpcMeta['sentry-trace'] || '', baggage: rpcMeta.baggage || '' }, | |||
There was a problem hiding this comment.
If this is the only place we consume it, is there a reason it's called sentry-trace rather than sentryTrace? The css-case name makes sense when it's in HTTP headers, of course, but since this is always a plain old JS object, it seems like making it camelCase would be a little simpler.
| { sentryTrace: rpcMeta['sentry-trace'] || '', baggage: rpcMeta.baggage || '' }, | |
| rpcMeta, |
Possibly would require setting the default '' values in the extractRpcMeta method, and obviously updating the type everywhere else of course.
There was a problem hiding this comment.
The reason is that I retook the type SerializedTraceData from core, and there it is written in kebab case. I'd like to keep it with this case, even though I don't really like it and it adds couple of bytes 😭
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Bug: The implementation uses the key __sentry_rpc_meta__ to append RPC metadata, but tests and the PR description refer to __sentry. This mismatch will break metadata extraction.
Severity: MEDIUM
Suggested Fix
Ensure the key used for appending RPC metadata is consistent across the implementation, tests, and documentation. Align on either __sentry_rpc_meta__ or __sentry and update the code and tests accordingly to use the same constant.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/cloudflare/src/utils/rpcMeta.ts#L20-L22
Potential issue: The implementation in `rpcMeta.ts` appends Sentry RPC metadata to
function arguments using the key `__sentry_rpc_meta__`. However, the associated tests
and the pull request description indicate that the expected key is `__sentry`. This
discrepancy means that the logic to extract the metadata from the arguments list on the
receiving end will fail, as it will be looking for the wrong key. Consequently, the
metadata will not be correctly processed and will be passed as an extra argument to the
user's RPC method, which is not the intended behavior.
| get(target, prop) { | ||
| const value = Reflect.get(target, prop); |
There was a problem hiding this comment.
Bug: The proxy get trap for DurableObjectStub and JSRPC proxies omits the receiver argument in Reflect.get, which can cause 'Illegal invocation' errors on native getters.
Severity: HIGH
Suggested Fix
In the proxy get traps, change the call from Reflect.get(target, prop) to Reflect.get(target, prop, target). This ensures that when accessing getters, the this context refers to the original object (target) rather than the proxy, satisfying internal brand checks.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
packages/cloudflare/src/instrumentations/instrumentDurableObjectNamespace.ts#L47-L48
Potential issue: The proxy `get` handlers in `instrumentDurableObjectNamespace.ts` and
`instrumentEnv.ts` call `Reflect.get(target, prop)` without specifying a `receiver`.
This can cause 'Illegal invocation: function called with incorrect `this` reference'
errors when accessing properties on proxied Cloudflare objects like `DurableObjectStub`.
These objects may have native getters that perform internal brand checks on `this`. An
existing instrumentation for `DurableObjectStorage` in the codebase already accounts for
this by explicitly passing the `target` as the receiver, but this pattern was not
followed in the new code.
Also affects:
packages/cloudflare/src/instrumentations/worker/instrumentEnv.ts:62~63
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.
Reviewed by Cursor Bugbot for commit 7c90dce. Configure here.
3 participants
closes #19327
closes JS-1715
closes #16898
closes JS-680
closes #16760
closes JS-622
Summary
Adds trace propagation for Cloudflare Workers RPC method calls to Durable Objects.
This is admittedly a bit of a hack: Cap'n Proto (which powers Cloudflare RPC) has no native support for headers or metadata. To work around this, we append our trace data (sentry-trace + baggage) as a trailing argument object
{ __sentry: { trace, baggage } }to every RPC call. On the receiving DO side, we strip this argument before the user's method is invoked, so it's completely transparent.Caveat: If the Durable Object is not instrumented with Sentry, the trailing
__sentryargument will remain in the args array and be passed to the user's method. I would count this as ok since:...argsto retrieve all argumentsOtherwise, trace propagation should be seamless across Worker → DO and Worker → Worker → DO call chains.
How it works
As mentioned above a Sentry trace object is appended on each call