Skip to content
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.

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

fix(core): Ensure http.client span descriptions don't contain query params or fragments #15404

Merged
merged 4 commits into from
Feb 17, 2025

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Feb 13, 2025

This PR fixes an oversight with our fetch instrumentation in the core package. where we didn't strip query params and URL hash fragments from the span name (description) of http.client spans. With this fix, the span description now only contains the URL protocol, host and path as defined in our develop specification.

I initially thought this was a regression but after looking through various versions of our browser fetch and XHR instrumentations (all the way from v7.0.0 until today), it doesn't seem like we ever sanitized the URLs correctly. I'm not sure what the reason for not including browser was when working on getsentry/team-webplatform-meta#2. At least going forward, this should now be fixed.

Affected SDKs: @sentry/browser (+SDKs building on it), @sentry/cloudflare, @sentry/vercel-edge

This PR also adds a bunch of integration and unit tests (also to the unaffected Node SDK) that ensure that the behaviour is aligned in browser and node.

I noticed some discrepancies with span attributes, especially in Otel's node-fetch instrumentation but for now, all required attributes are there as far as I can tell. We can revisit if we get notifications from product that something is missing.

closes #15360

@Lms24 Lms24 self-assigned this Feb 13, 2025
Copy link
Contributor

github-actions bot commented Feb 13, 2025

size-limit report 📦

Path Size % Change Change
@sentry/browser 23.02 KB - -
@sentry/browser - with treeshaking flags 22.8 KB - -
@sentry/browser (incl. Tracing) 36.04 KB +0.22% +78 B 🔺
@sentry/browser (incl. Tracing, Replay) 73.03 KB +0.12% +85 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 66.5 KB +0.09% +57 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas) 77.28 KB +0.11% +85 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback) 90.23 KB +0.09% +78 B 🔺
@sentry/browser (incl. Feedback) 40.18 KB - -
@sentry/browser (incl. sendFeedback) 27.65 KB - -
@sentry/browser (incl. FeedbackAsync) 32.45 KB - -
@sentry/react 24.84 KB - -
@sentry/react (incl. Tracing) 37.92 KB +0.18% +69 B 🔺
@sentry/vue 27.2 KB - -
@sentry/vue (incl. Tracing) 37.73 KB +0.22% +84 B 🔺
@sentry/svelte 23.06 KB - -
CDN Bundle 24.22 KB - -
CDN Bundle (incl. Tracing) 36.06 KB +0.2% +71 B 🔺
CDN Bundle (incl. Tracing, Replay) 70.89 KB +0.1% +70 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) 76.06 KB +0.08% +60 B 🔺
CDN Bundle - uncompressed 70.83 KB - -
CDN Bundle (incl. Tracing) - uncompressed 107.09 KB +0.16% +169 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed 218.31 KB +0.08% +169 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 230.88 KB +0.08% +169 B 🔺
@sentry/nextjs (client) 38.91 KB +0.19% +75 B 🔺
@sentry/sveltekit (client) 36.45 KB +0.22% +81 B 🔺
@sentry/node 127.71 KB - -
@sentry/node - without tracing 97.99 KB -0.01% -1 B 🔽
@sentry/aws-serverless 107.42 KB -0.01% -1 B 🔽

View base workflow run

Copy link

codecov bot commented Feb 13, 2025

❌ 5 Tests Failed:

Tests completed Failed Passed Skipped
4568 5 4563 324
View the top 3 failed test(s) by shortest run time
transactions.test.ts API route transaction includes nest pipe span for invalid request
Stack Traces | 30s run time
transactions.test.ts:309:5 API route transaction includes nest pipe span for invalid request
transactions.test.ts API route transaction includes exception filter span for local filter in module registered after Sentry
Stack Traces | 30s run time
transactions.test.ts:164:5 API route transaction includes exception filter span for local filter in module registered after Sentry
transactions.test.ts API route transaction includes exception filter span for global filter in module registered after Sentry
Stack Traces | 30s run time
transactions.test.ts:125:5 API route transaction includes exception filter span for global filter in module registered after Sentry

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@Lms24 Lms24 force-pushed the lms/fix-strip-query-param branch 2 times, most recently from 9f55ad3 to 983576e Compare February 17, 2025 11:04
@Lms24 Lms24 force-pushed the lms/fix-strip-query-param branch from 983576e to c31d665 Compare February 17, 2025 13:39
@Lms24 Lms24 requested review from chargome and s1gr1d February 17, 2025 13:40
Copy link
Member

@s1gr1d s1gr1d left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for all the tests 🙌

@Lms24 Lms24 merged commit 6e6f85b into develop Feb 17, 2025
148 checks passed
@Lms24 Lms24 deleted the lms/fix-strip-query-param branch February 17, 2025 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SDK sends query params and fragment in span description of http.client spans
2 participants