feat(cloudflare): Propagate traceparent to RPC calls - via fetch#19991
feat(cloudflare): Propagate traceparent to RPC calls - via fetch#19991
Conversation
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Core
Deps
Other
Bug Fixes 🐛
Internal Changes 🔧
🤖 This preview updates automatically when you update the PR. |
size-limit report 📦
|
JPeer264
force-pushed
the
jp/rpc-instrument
branch
from
3bd82cc to
71a3936
Compare
JPeer264
changed the title
JPeer264
force-pushed
the
jp/rpc-instrument
branch
from
71a3936 to
3097f54
Compare
FYI, #19960 landed and I agree! Right now, our various request instrumentations combine the "logic"/"algorithm" code with platform specific code how to set headers. I think it would be good if we could strip out the algorithm part into a common helper, and only leave the platform-specific parts to the individual instrumentations. From my PoV a good improvement but something we can also tackle at a later point. Happy to leave this up to you :) |
| const newInit = addTraceHeaders(input, init); | ||
|
|
||
| return fetchFn(input, newInit); |
There was a problem hiding this comment.
m/q: is this how we add our tracing headers in our core/browser fetch instrumentation as well? Just wanted to double check because injecting headers into fetch arguments is a bit tricky with the various forms of input and init and the precedence.
I think adding headers to init is fine. Though us deep-copying headers from input to init, and then init again might be unexpected if one of the objects is reused by users across multiple requests. Anyway, not saying something is off but that we should double check that this is how we do it in the other SDKs as well.
There was a problem hiding this comment.
Fair enough, that would be indeed a bad behavior - I'll change the behavior to match the others
There was a problem hiding this comment.
@Lms24 the headers are now attached with the same function as the other SDKs: _INTERNAL_getTracingHeadersForFetchRequest.
JPeer264
force-pushed
the
jp/rpc-instrument
branch
from
3097f54 to
fb6a835
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@Lms24 I just reworked the way how tracing headers are generated. I actually exported
|
JPeer264
force-pushed
the
jp/rpc-instrument
branch
from
77c7260 to
ef05c42
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
dev-packages/cloudflare-integration-tests/suites/tracing/propagation/worker-do/wrangler.jsonc
Show resolved
Hide resolved
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
part of #15942 This is a basic instrumentation for `WorkerEntrypoint`. There is no extra `instrumentWorkerEntrypoint` exported, and should work directly with `withSentry` OOTB: ```js class MyWorker extends WorkerEntrypoint {} export default Sentry.withSentry(() => ({}), MyWorker) ``` To support the full `WorkerEntrypoint` there is the need to instrument [RPC methods](#16898). Without that we can't fully support `WorkerEntrypoint`s but just the basics of it. This can be added once #19991 lands (it is safe to review now and instrument the env in a separate PR) --- If you look at the instrumentations themselves, there is no `isInstrumented` or `markAsInstrumented`. The reason is that on the one hand this is not required, as every request is spawning a new instance of the `WorkerEntrypoint` and on the other hand it wouldn't work as it would mark it as instrumented, but on the request the `fetch` is overridden and not instrumented.
This PR is an extraction of #19991 It basically exports `getTracingHeadersForFetchRequest`, which was previously only exported for testing, but offers a great functionality if you want to add tracing headers to a request. I renamed it as `addTracingHeadersToFetchRequest` sounded a little misleading, as it didn't really add headers to the request, as it returned the extracted headers from the request (or init, if there are any). ### Open question I added `@hidden` and `@internal` to it, not sure if this is an approach we follow. I'm ok to remove it from the jsdoc
JPeer264
force-pushed
the
jp/rpc-instrument
branch
2 times, most recently
from
bc033a9 to
f62418d
Compare
JPeer264
force-pushed
the
jp/rpc-instrument
branch
from
f62418d to
179ec14
Compare
3 participants
relates to #19327
related to #16898 (it is not really closing it as we just add context propagation without adding spans for individual calls. It needs to be defined if we need it)
It is important to know that these RPC calls do only work with the
.fetchcall:This adds RPC fetch calls between:
This works by instrumenting
env(viainstrumentEnv), which then goes over the bindings and see if there is a DurableObject or a normal Fetcher (full list of current bindings: https://developers.cloudflare.com/workers/runtime-apis/bindings/). This got inspired by howotel-cf-workersinstruments their env: https://github.com/evanderkoogh/otel-cf-workers/blob/effeb549f0a4ed1c55ea0c4f0d8e8e37e5494fb3/src/instrumentation/env.tsWith this PR I added a lot of tests to check if trace propagation works (so this PR might look like it added a lot of LoC, but it is mostly tests). So I added it for
scheduleandqueue, but it is not possible foremailandtailwithwrangler dev.Potential things to change
Trace propagagtion
I added the
addTraceHeaders.tshelper, as there is currently no way to reuse the existing logic (it is baked-in into the fetch instrumentations). It would be nice once #19960 lands that we can reuse it in Cloudflare to reuse existing code. I tried to write couple of tests so we don't have duplicated headers.Adding extra spans
So there is actually a guide by OTel to add RPC spans, but was talking with someone from the OTel maintainers and they meant that this wouldn't be necessary as we already have an
http.serverspan from out instrumented DurableObjects (and other resources) - so it wouldn't add much of information.Without RPC span:
With RPC span: