Uh oh!
There was an error while loading. Please reload this page.
feat(cloudflare): Support rpc trace propagation for WorkerEntrypoint - #20523
Conversation
size-limit report 📦
|
7ea2cd5 to
ace755bCompare
nicohrubec
left a comment
There was a problem hiding this comment.
Overall looks lgtm, just not sure about the test
| const response = await runner.makeRequest<string>('get', '/rpc/hello'); | ||
| expect(response).toBe('Hello, World!'); | ||
| await runner.completed(); |
There was a problem hiding this comment.
h: Can we guarantee at this point that we actually get no RPC transaction? I think the runner just terminates as soon as we see the one expected envelope, so if the RPC transaction just arrives later this test will pass fine even though we don't have the behavior we want. If that's the case maybe we can come up with a pattern that makes this stable. Alternatively we could think about removing this E2E app and relying on unit tests to check this.
There was a problem hiding this comment.
I think I'll rewrite it and check if both events are arriving but have different trace id's, then we can be 100% sure
There was a problem hiding this comment.
I changed it to a .fetch RPC. Now we get a second trace and we can be sure the traces are not connected
👋 @s1gr1d — Please review this PR when you get a chance! |
11e5f85 to
9b3228aCompare| }); | ||
| } | ||
| if ('env' in obj) { |
There was a problem hiding this comment.
env should always be on the obj, right? Maybe it's worth just calling this unconditionally. And for extra safety, wrapping it in a try/catch:
try{Object.defineProperty(obj,'env',{ ... });}catch{}There was a problem hiding this comment.
It is not always guaranteed to be there. I like this approach of calling it unconditionally actually.
However, I'd leave it as is for now to be consistent with the code above. Once some performance metrics are added I'd like to see if there are some performance increases with this method.
closes#15942
closes JS-212
We already have support for
WorkerEntrypoint, but they were not yet fully supported because we didn't have trace propagation for them.This PR basically adds RPC trace propagation (and therefore full support for
WorkerEntrypoint) by instrumentingenvfor theWorkerEntrypoint. The base implementation is pretty straight forward, it is usinginstrumentEnvand overwrites theenvof the entrypoint instance.The rest are tests and different scenarios showing how it works. Most tests are the same as the normal exported handler tests:
worker-do-rpcis the same asworkerentrypoint-do-rpc, just with aWorkerEntrypointrather than theExportedHandler. The only additional test scenario isworker-workerentrypoint-rpcwhich showcases ifExportedHandlerandWorkerEntrypointare working out together.