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

test(V7/browser-integration-tests): Check for sentry-trace header in TwP #11555

Merged
merged 2 commits into from
Apr 12, 2024

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Apr 11, 2024

As a result of our investigation into tracing behaviour for fetch requests without active spans, this PR adjusts/adds tests around checking that a sentry-trace header is attached to outgoing requests in v7 of the SDK. As we also discovered today, this unfortunately doesn't work as "advertised in the basic TwP case:

  • [correct behaviour] BrowserTracing initialized; tracesSamplerate set: sentry-trace header is attached without a sampled flag.
  • [incorrect behaviour] BrowserTracing is initialized but no tracesSampleRate: We currently don't attach a sentry-trace header although we should.

@Lms24 Lms24 requested a review from mydea April 11, 2024 11:15
Comment on lines +8 to +10
integrations: [
new Sentry.BrowserTracing({ tracingOrigins: ['http://example.com'], startTransactionOnPageLoad: false }),
],
Copy link
Member Author

Choose a reason for hiding this comment

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

this test was broken before but still succeeded because our assertions didn't check anything specific other than ensuring that requests were made.

@Lms24 Lms24 changed the title test(V7/browser-integration-tests): Check for sentry-trace header in TwP test(V7/browser-integration-tests): Check for sentry-trace header in TwP Apr 11, 2024
Copy link
Contributor

github-actions bot commented Apr 11, 2024

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped) 80.58 KB (+0.12% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 71.55 KB (+0.11% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas) - Webpack (gzipped) 75.57 KB (+0.11% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 65.18 KB (+0.12% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 35.49 KB (+0.24% 🔺)
@sentry/browser (incl. browserTracingIntegration) - Webpack (gzipped) 35.37 KB (+0.24% 🔺)
@sentry/browser (incl. Feedback) - Webpack (gzipped) 31.57 KB (+0.15% 🔺)
@sentry/browser (incl. sendFeedback) - Webpack (gzipped) 31.58 KB (+0.16% 🔺)
@sentry/browser - Webpack (gzipped) 22.78 KB (+0.18% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped) 78.77 KB (+0.23% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 70.12 KB (+0.21% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 35.9 KB (+0.36% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped) 25.27 KB (+0.43% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 220.72 KB (+0.19% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 108.53 KB (+0.38% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 75.79 KB (+0.41% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 39.17 KB (+0.44% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 72.03 KB (+0.1% 🔺)
@sentry/react - Webpack (gzipped) 22.81 KB (+0.18% 🔺)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 89.87 KB (+0.07% 🔺)
@sentry/nextjs Client - Webpack (gzipped) 54.01 KB (+0.12% 🔺)
@sentry-internal/feedback - Webpack (gzipped) 17.32 KB (+0.09% 🔺)

@Lms24
Copy link
Member Author

Lms24 commented Apr 11, 2024

hmm why is size-check complaining? I guess we added more stuff in v7 recently and it just fluctuates a bit 🤔

@Lms24 Lms24 enabled auto-merge (squash) April 11, 2024 12:40
@Lms24 Lms24 disabled auto-merge April 11, 2024 13:24
@Lms24
Copy link
Member Author

Lms24 commented Apr 11, 2024

FYI I'm gonna hold off from merging this in to avoid blocking today's v7 release (#11540)

mydea added a commit that referenced this pull request Apr 11, 2024
This also forward-ports the tests from #11555, and adds some more tests.
@Lms24 Lms24 merged commit 8abd97c into v7 Apr 12, 2024
104 checks passed
@Lms24 Lms24 deleted the lms/test-twp-sentry-trace branch April 12, 2024 06:52
mydea added a commit that referenced this pull request Apr 12, 2024
This also forward-ports the tests from #11555, and adds some more tests.
mydea added a commit that referenced this pull request Apr 15, 2024
This also forward-ports the tests from
#11555, and adds some
more tests.

Node is a different story, will tackle it separately.
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request Apr 19, 2024
…ry#11561)

This also forward-ports the tests from
getsentry#11555, and adds some
more tests.

Node is a different story, will tackle it separately.
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.

2 participants