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(tracing): Fix tracingOrigins not applying #6079

Merged
merged 2 commits into from
Oct 28, 2022

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Oct 28, 2022

In the process of working on #5285, we missed the fact that the first two PRs (#6039 and #6041) were interdependent, in that the former accidentally introduced a bug (#6077) which the latter then inadvertently fixed. This would have been fine, except that we published a release after merging the bug-creating PR but before merging the bug-fixing PR. Whoops.

This pulls just the bug-fixing part out of the second PR. It also adds tests to cover the buggy cases, using it.each to cover all of the different combinations of outcomes for shouldCreateSpanForRequest and shouldAttachHeaders. Finally, since I was already in the test file, I reorganized it a little:

  • it('does not create span if shouldCreateSpan returns false') -> absorbed into the it.each()
  • it('does not create span if there is no fetch data in handler data') -> added header check, became it('adds neither fetch request spans nor fetch request headers if there is no fetch data in handler data')
  • it('does not add fetch request spans if tracing is disabled') and it('does not add fetch request headers if tracing is disabled -> combined into it('adds neither fetch request spans nor fetch request headers if tracing is disabled')
  • it('adds sentry-trace header to fetch requests') -> absorbed into the it.each()
  • Similar changes made to XHR tests

Fixes #6077

h/t @timfish for the PR from which this is derived

@github-actions
Copy link
Contributor

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.49 KB (-0.01% 🔽)
@sentry/browser - ES5 CDN Bundle (minified) 60.35 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.12 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (minified) 53.68 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 19.86 KB (0%)
@sentry/browser - Webpack (minified) 65.09 KB (0%)
@sentry/react - Webpack (gzipped + minified) 19.89 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 45.78 KB (+0.07% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.24 KB (+0.09% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.62 KB (+0.07% 🔺)

@Lms24 Lms24 merged commit fecdc3e into master Oct 28, 2022
@Lms24 Lms24 deleted the kmclb-fix-tracingOrigins-not-applying branch October 28, 2022 07:18
lobsterkatie added a commit that referenced this pull request Nov 8, 2022
…g instrumentation (#6080)

As part of the work on #5285, this adds a new option, `tracePropagationTargets`, to our browser-side tracing instrumentation, to live alongside (and eventually take the place of) `tracingOrigins`. Its behavior matches that of `tracingOrigins`, but it has a much clearer name.

Switching from internal use of `tracingOrigins` to `tracePropagationTargets` to come in future PRs.

Note: This is what remained of #6041 after the fix in #6079 was split off. h/t @timfish for the initial work in that PR.
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.

7.17.1 attaching baggage header to third-party requests breaking CORS policies
2 participants