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): Restore previously active span when passing a custom scope to startSpanManual #14901

Draft
wants to merge 3 commits into
base: lms/fix-core-restore-active-span-custom-scope
Choose a base branch
from

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Jan 3, 2025

This PR follows up on #14900 and proposes active span restoration when passing a custom scope to startSpanManual. This is unfortunately a bit trickier than for startSpan because there are two ways how users can end a span:

  • by using span.end()
  • by using the finish function passed to the startSpanManual callback

With the changes in this PR, this two ways of ending a span would diverge in behaviour:

startSpanManual({ scope: customScope }, (span, finish)  => {
  // ends the span and restores the previous active span on the scope
  finish();
})

startSpanManual({ scope: customScope }, span  => {
  // ends the span but DOES NOT restore the span on the scope
  span.end();
})

The alternative to the proposed change would be to monkey-patch span.end() to always restore the active span on custom scopes. Can also change the PR to do this if reviewers prefer it.

@Lms24 Lms24 changed the base branch from develop to lms/fix-core-restore-active-span-custom-scope January 3, 2025 13:43
@Lms24 Lms24 requested a review from mydea January 3, 2025 13:48
Copy link
Contributor

github-actions bot commented Jan 3, 2025

size-limit report 📦

Path Size % Change Change
@sentry/browser 22.77 KB - -
@sentry/browser - with treeshaking flags 21.52 KB - -
@sentry/browser (incl. Tracing) 35.35 KB - -
@sentry/browser (incl. Tracing, Replay) 72.56 KB - -
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 62.95 KB - -
@sentry/browser (incl. Tracing, Replay with Canvas) 76.95 KB - -
@sentry/browser (incl. Tracing, Replay, Feedback) 89.32 KB - -
@sentry/browser (incl. Feedback) 39.53 KB - -
@sentry/browser (incl. sendFeedback) 27.41 KB - -
@sentry/browser (incl. FeedbackAsync) 32.17 KB - -
@sentry/react 25.54 KB - -
@sentry/react (incl. Tracing) 38.16 KB - -
@sentry/vue 27.06 KB - -
@sentry/vue (incl. Tracing) 37.22 KB - -
@sentry/svelte 22.94 KB - -
CDN Bundle 24.14 KB - -
CDN Bundle (incl. Tracing) 35.72 KB +0.14% +49 B 🔺
CDN Bundle (incl. Tracing, Replay) 70.76 KB +0.08% +51 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) 75.96 KB +0.07% +49 B 🔺
CDN Bundle - uncompressed 70.59 KB - -
CDN Bundle (incl. Tracing) - uncompressed 106.11 KB +0.06% +56 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed 220.05 KB +0.03% +56 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 232.92 KB +0.03% +56 B 🔺
@sentry/nextjs (client) 38.48 KB - -
@sentry/sveltekit (client) 35.88 KB - -
@sentry/node 162.47 KB -0.01% -1 B 🔽
@sentry/node - without tracing 98.24 KB - -
@sentry/aws-serverless 128.09 KB +0.01% +1 B 🔺

View base workflow run

@Lms24 Lms24 self-assigned this Jan 3, 2025
@Lms24 Lms24 force-pushed the lms/fix-core-restore-active-span-custom-scope branch from c864c76 to 52b10a8 Compare January 10, 2025 10:58
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.

1 participant