Skip to content

feat(core): Fix span scope handling & transaction setting #10886

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

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

mydea
Copy link
Member

@mydea mydea commented Mar 1, 2024

This does the following things:

  1. Instead of setting transaction tag, we ensure to set the transaction on the event itself.
  2. This also allows us to remove code that lead to other bugs, which is that we needed to fork the scope for startInactiveSpan just to get the correct transaction tag.
  3. Add tests to ensure that the scope is correctly applied when using startInactiveSpan

Background

I initially implemented startInactiveSpan to fork the current scope & set the span on that to satisfy two things:

  1. We do not want to set this span as active on the actual current scope
  2. We needed a way to set the transaction tag on the span (if it was a transaction) in an event processor

For 2., in order for getActiveSpan() to work in there I had to make the span active in the forked scope.

This kind of worked, BUT I now noticed that this has the massive problem:

const scope = getCurrentScope();
const span = startInactiveSpan(...);
scope.setTag(...); // <-- this will not be applied to the span!

So I went in and fixed this - and since we talked about not needing to set the transaction tag anyhow (instead, just set the transaction itself!), I figured I can fix these in one go.

@mydea mydea requested review from lforst, Lms24, AbhiPrasad and s1gr1d March 1, 2024 12:17
@mydea mydea self-assigned this Mar 1, 2024
Copy link
Contributor

github-actions bot commented Mar 1, 2024

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped) 77.38 KB (-0.03% 🔽)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 68.59 KB (-0.03% 🔽)
@sentry/browser (incl. Tracing, Replay with Canvas) - Webpack (gzipped) 72.53 KB (-0.03% 🔽)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 62.15 KB (-0.03% 🔽)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 32.82 KB (-0.06% 🔽)
@sentry/browser (incl. browserTracingIntegration) - Webpack (gzipped) 32.82 KB (-0.06% 🔽)
@sentry/browser (incl. Feedback) - Webpack (gzipped) 31.06 KB (-0.01% 🔽)
@sentry/browser (incl. sendFeedback) - Webpack (gzipped) 31.06 KB (0%)
@sentry/browser - Webpack (gzipped) 22.3 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped) 75.82 KB (-0.02% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 67.45 KB (-0.02% 🔽)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 33.29 KB (+0.01% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped) 24.79 KB (-0.02% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 211.08 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 99.82 KB (+0.01% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 74.08 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 36.34 KB (-0.04% 🔽)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 68.87 KB (-0.03% 🔽)
@sentry/react - Webpack (gzipped) 22.33 KB (-0.01% 🔽)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 85.35 KB (-0.02% 🔽)
@sentry/nextjs Client - Webpack (gzipped) 49.68 KB (-0.03% 🔽)
@sentry-internal/feedback - Webpack (gzipped) 17.09 KB (0%)

This does the following things:
1. Instead of setting `transaction` tag, we ensure to set the `transaction` on the event itself.
2. This also allows us to remove code that lead to other bugs, which is that we needed to fork the scope for `startInactiveSpan` just to get the correct transaction tag.
3. Add tests to ensure that the scope is correctly applied when using `startInactiveSpan`
@mydea mydea force-pushed the fn/fix-inactive-span-lookup branch from 6a547af to 90968bd Compare March 1, 2024 12:51
@mydea
Copy link
Member Author

mydea commented Mar 4, 2024

@AbhiPrasad I noticed we are also setting transaction as tag on metrics summaries, not sure if we can/should also change this (I left it for now)

@mydea mydea merged commit 54b1cf5 into develop Mar 4, 2024
@mydea mydea deleted the fn/fix-inactive-span-lookup branch March 4, 2024 08:48
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