fix(core): Respect manually set sentry tracing headers in fetch calls #16183
Conversation
Lms24
changed the title
size-limit report 📦
|
Lms24
changed the title
Lms24
force-pushed
the
lms/fix-core-fetch-custom-trace-headers
branch
from
844a168 to
e044661
Compare
| requests.forEach(request => { | ||
| const headers = request.headers(); | ||
|
|
||
| // No merged sentry trace headers | ||
| expect(headers['sentry-trace']).not.toContain(','); | ||
|
|
||
| // No multiple baggage entries | ||
| expect(headers['baggage'].match(/sentry-trace_id/g) ?? []).toHaveLength(1); | ||
| expect(headers['baggage'].match(/sentry-release/g) ?? []).toHaveLength(1); |
There was a problem hiding this comment.
this seems harmless but it demonstrates the behaviour change for reused request options:
previously, we'd simple disregard what sentry values were in the request options and always overwrite them. Now:
- we still don't blindly append our values to pre-existing ones
- but instead, we do nothing and just roll with the passed-in headers.
Result: All requests in this test sharing the same request options have the same sentry tracing headers (and hence their child trace trees have the same trace parent)
I think this is fine but it's still worth noting.
There was a problem hiding this comment.
Lgtm. I wonder whether there will ever be a problem because some downstream SDK wants to set a prefixed baggage entry in addition to what the core SDK sets, but it will not work because adding a prefixed baggage entry will just stomp on our logic... Might be worth checking with Krystof
|
Hmm I'm wondering what this downstream SDK would have to do to run into a problem here. This change basically prohibits modifying baggage (or sentry-trace) if the headers already exist on the request. My thinking is that if other higher-level SDKs add entries to the DSC (or baggage), they would do that by using |
|
Double checked with @krystofwoldrich - RN doesn't add request headers to fetch requests. |
…ests (#16184) Analogously to #16183 for `fetch`, this patch ensures that manually set `sentry-trace` and `baggage` headers in XHR requests are respected by our XHR instrumentation and not overwritten. This patch also fixes a bug where we'd append multiple `sentry-trace` or `baggage` values in case the header was somehow set more than once (analogously to #13907 for `fetch`)
This PR fixes a bit of a weird fetch instrumentation problem:
If users manually pass in
sentry-traceandbaggageheaders, we previously overwrote the values with the ones we computed. While in #13907 we specifically decided that we want to remove any previously set sentry tracing header values and replace them with the current ones, I think the main motivation was to avoid duplicated header values.With this PR, we:
nice side-effect: slight bundle size reduction :D
came up in https://linear.app/getsentry/issue/FE-380/debug-trace-context-propagation-issue-from-tanstack-to-go-backend