feat(browser): Add support for propagateTraceparent SDK option
#17509
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
size-limit report 📦
|
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.
|
Lms24
changed the title
propagateTraceparent SDK optionpropagateTraceparent SDK option
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Lms24
requested review from
markushi,
cleptric,
a team,
stephanie-anderson,
chargome and
mydea
and removed request for
a team and
stephanie-anderson
| ): void { | ||
| const originalHeaders = xhr.__sentry_xhr_v3__?.request_headers; | ||
|
|
||
| if (originalHeaders?.['sentry-trace']) { | ||
| if (originalHeaders?.['sentry-trace'] || !xhr.setRequestHeader) { | ||
| // bail if a sentry-trace header is already set |
There was a problem hiding this comment.
👀 we don't have this logic in sentry-java, but I'll add it too. There's an edge case: if the request has a sentry-trace header, but lacks baggage/traceparent header, we won't add baggage/traceparent here. But I think that's fine, otherwise you could end up in situations where the header values don't match up.
There was a problem hiding this comment.
if the request has a
sentry-traceheader, but lacks baggage/traceparent header, we won't add baggage/traceparent here
yes correct and I agree that this is fine. sentry-trace is still the minimum required header for our trace propagation to work according to our DSC dev spec. (Server) SDKs have to handle incoming requests where only sentry-trace is sent. This was primarily required for backwards compatibility but I think this case is another valid (though very edge-casey) reason to do so. So I think this should be safe.
My rationale here is: If users really attach their custom tracing headers (already unlikely but it happened), it's on them to do it properly. We export functions like Sentry.getTraceData() to make this as easy as possible. Everything we do in the SDK would be a "best guess" effort and likely leads to unexpected results.
| @@ -20,6 +20,9 @@ export const TRACEPARENT_REGEXP = new RegExp( | |||
| /** | |||
| * Extract transaction context data from a `sentry-trace` header. | |||
| * | |||
| * This is terrible naming but the function has nothing to do with the W3C traceparent header. | |||
This PR adds support for a new browser SDK init option,
propagateTraceparent, as spec'd out in our develop docsIf users opt into
propagateTraceparent, browser SDKs will attach a W3C complianttraceparentheader to outgoing fetch and XHR requests, in addition tosentry-traceandbaggageheaders.A couple of implementation remarks:
@sentry/core.instrumentFetchRequestfunction in core because it already took an optional param. Nothing major though in terms of breakage but this is a publicly exported function, so we can't just simply break the signature.generateSentryTraceHeaderreturns a random uuid if we're in TwP mode. If we were to generate a traceparent header from the same data set, we'd also have to create a random uuid, meaning we'd end up with sentry-trace and traceparent headers on the same request with different (non-existing) parentSpanIds. If we just take the finished header, and morph it into the other header we can avoid this entirely.parentSpanIdstays consistent during trace in TwP mode #17526 will improve this by properly generating atraceparentheader. Which requires some additional TwP trace propagation changes, so I decided to keep this separate.closes #17482