Skip to content

fix(core): .set the sentry-trace header instead of .appending in fetch instrumentation #13907

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 13 commits into from
Oct 16, 2024
Merged

fix(core): .set the sentry-trace header instead of .appending in fetch instrumentation #13907

merged 13 commits into from
Oct 16, 2024

Conversation

lforst
Copy link
Contributor

@lforst lforst commented Oct 8, 2024 •
edited
Loading

Fixes #13870
Fixes #12350

Fixes issues where persistence of the options object passed to fetch() would cause the options object, or rather the attached sentry-trace and baggage headers to indefinitely grow in size.

@lforst lforst mentioned this pull request Oct 8, 2024
Closed
3 tasks
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 8, 2024 •
edited
Loading

size-limit report 📦

Path Size % Change Change
@sentry/browser 22.73 KB - -
@sentry/browser - with treeshaking flags 21.53 KB - -
@sentry/browser (incl. Tracing) 35.12 KB +0.34% +120 B 🔺
@sentry/browser (incl. Tracing, Replay) 71.83 KB +0.16% +111 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 62.23 KB +0.23% +141 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas) 76.19 KB +0.18% +135 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback) 88.97 KB +0.16% +141 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 90.8 KB +0.13% +116 B 🔺
@sentry/browser (incl. metrics) 27 KB - -
@sentry/browser (incl. Feedback) 39.87 KB - -
@sentry/browser (incl. sendFeedback) 27.38 KB - -
@sentry/browser (incl. FeedbackAsync) 32.17 KB - -
@sentry/react 25.49 KB - -
@sentry/react (incl. Tracing) 38.09 KB +0.31% +120 B 🔺
@sentry/vue 26.91 KB - -
@sentry/vue (incl. Tracing) 37.02 KB +0.33% +122 B 🔺
@sentry/svelte 22.87 KB - -
CDN Bundle 24.11 KB - -
CDN Bundle (incl. Tracing) 36.96 KB +0.33% +122 B 🔺
CDN Bundle (incl. Tracing, Replay) 71.62 KB +0.17% +124 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) 76.96 KB +0.19% +148 B 🔺
CDN Bundle - uncompressed 70.7 KB - -
CDN Bundle (incl. Tracing) - uncompressed 109.73 KB +0.35% +383 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed 222.17 KB +0.17% +383 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 235.38 KB +0.16% +383 B 🔺
@sentry/nextjs (client) 38.06 KB +0.32% +122 B 🔺
@sentry/sveltekit (client) 35.74 KB +0.4% +143 B 🔺
@sentry/node 125.15 KB +0.01% +3 B 🔺
@sentry/node - without tracing 94.25 KB - -
@sentry/aws-serverless 103.81 KB - -

View base workflow run

Lms24
Lms24 approved these changes Oct 8, 2024
View reviewed changes
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still curious how we'd attach headers multiple times but I think the change is fine.
I guess as long as we don't find out why this happene(d) testing against would be a bit artificial

@lforst
Copy link
Contributor Author

lforst commented Oct 8, 2024

I'm still curious how we'd attach headers multiple times but I think the change is fine.
I guess as long as we don't find out why this happene(d) testing against would be a bit artificial

I think I know a way this can happen. Basically, if you re-use request objects. I'll try to come up with tests.

@codecov Codecov
Copy link

codecov bot commented Oct 8, 2024 •
edited
Loading

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
536 1 535 129
View the top 1 failed tests by shortest run time 
tracing/metrics/web-vitals/test.ts captures time origin and navigation activationStart as span attributes
Stack Traces | 30s run time 
test.ts:62:11 captures time origin and navigation activationStart as span attributes

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

@lforst lforst merged commit 8d5a084 into develop Oct 16, 2024
147 of 148 checks passed
@lforst lforst deleted the lforst-set-headers-instead-of-append branch October 16, 2024 09:57
billyvg pushed a commit that referenced this pull request Oct 17, 2024
@billyvg
fix(core): .set the sentry-trace header instead of .appending i…
714f3ad 
…n fetch instrumentation (#13907)
billyvg pushed a commit that referenced this pull request Oct 17, 2024
@billyvg
fix(core): .set the sentry-trace header instead of .appending i…
6df99e4 
…n fetch instrumentation (#13907)
@github-actions github-actions bot mentioned this pull request Oct 21, 2024
Closed
3 tasks
@q1blue q1blue mentioned this pull request Nov 11, 2024
Open
This was referenced Nov 13, 2024
Closed
Closed
Closed
This was referenced May 2, 2025
Merged
Merged
Lms24 added a commit that referenced this pull request May 5, 2025
@Lms24
fix(browser): Respect manually set sentry tracing headers in XHR requ…
0335cf7 
…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`)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lms24 Lms24 Lms24 approved these changes

@AbhiPrasad AbhiPrasad AbhiPrasad approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@lforst @Lms24 @AbhiPrasad