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

feat(browser): Create spans as children of root span by default #10986

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

mydea
Copy link
Member

@mydea mydea commented Mar 8, 2024

You can opt out of this by setting parentSpanIsAlwaysRootSpan=false in your client options.

This means that in browser, any span will always be added to the active root span, not the active span. This way, we should be able to avoid problems with execution contexts etc.

Closes #10944

@mydea mydea requested review from lforst, Lms24 and AbhiPrasad March 8, 2024 13:05
@mydea mydea self-assigned this Mar 8, 2024
Copy link
Contributor

github-actions bot commented Mar 8, 2024

size-limit report 📦

Path Size
@sentry/browser 22.09 KB (+0.09% 🔺)
@sentry/browser (incl. Tracing) 31.78 KB (+0.31% 🔺)
@sentry/browser (incl. Tracing, Replay) 66.99 KB (+0.15% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 60.58 KB (+0.15% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas) 70.83 KB (+0.15% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback) 80.69 KB (+0.12% 🔺)
@sentry/browser (incl. Feedback) 35.64 KB (+0.07% 🔺)
@sentry/browser (incl. Feedback, Feedback Modal) 35.65 KB (+0.07% 🔺)
@sentry/browser (incl. Feedback, Feedback Modal, Feedback Screenshot) 35.66 KB (+0.06% 🔺)
@sentry/browser (incl. sendFeedback) 26.89 KB (+0.08% 🔺)
@sentry/react 24.78 KB (+0.1% 🔺)
@sentry/react (incl. Tracing) 34.7 KB (+0.31% 🔺)
@sentry/vue 25.64 KB (+0.53% 🔺)
@sentry/vue (incl. Tracing) 33.5 KB (+0.26% 🔺)
@sentry/svelte 22.22 KB (+0.08% 🔺)
CDN Bundle 24.14 KB (+0.07% 🔺)
CDN Bundle (incl. Tracing) 32.76 KB (+0.29% 🔺)
CDN Bundle (incl. Tracing, Replay) 66.47 KB (+0.14% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) 80.5 KB (+0.12% 🔺)
CDN Bundle - uncompressed 71.84 KB (+0.07% 🔺)
CDN Bundle (incl. Tracing) - uncompressed 98.11 KB (+0.28% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed 207.5 KB (+0.14% 🔺)
@sentry/nextjs (client) 33.88 KB (+0.3% 🔺)
@sentry/sveltekit (client) 32.3 KB (+0.3% 🔺)
@sentry/node 120.16 KB (+0.07% 🔺)

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Hmm I get why we need to do something about the current behaviour but at the same time it's ...sad that this is the consequence 😅

IMO, even in the browser, the parent<>child hierarchy makes sense for example for a span happening within a client-side data loading function. In this case we can't even set the flag because the span is created within our fetch instrumentation where we probably don't always want to set the flag. Also, setting the flag increases the bundle size footprint.

But 🤷 there is no ideal solution here 😬

Unfortunately, I agree, I also don't have a better solution for this.

We do nothing

Do we have concrete reports that the current behaviour is bothering people? Or some kind of indication how often we currently get the parent<>child mapping wrong? Maybe doing nothing is a feasible alternative?

@mydea
Copy link
Member Author

mydea commented Mar 8, 2024

Hmm I get why we need to do something about the current behaviour but at the same time it's ...sad that this is the consequence 😅

IMO, even in the browser, the parent<>child hierarchy makes sense for example for a span happening within a client-side data loading function. In this case we can't even set the flag because the span is created within our fetch instrumentation where we probably don't always want to set the flag. Also, setting the flag increases the bundle size footprint.

But 🤷 there is no ideal solution here 😬

Unfortunately, I agree, I also don't have a better solution for this.

We do nothing

Do we have concrete reports that the current behaviour is bothering people? Or some kind of indication how often we currently get the parent<>child mapping wrong? Maybe doing nothing is a feasible alternative?

So just to clarify, this setting is on client level, not on an individual span level - it would be either yes or no, basically, in this implementation!

I think we don't have that many reports of this because pre- startSpan this was less easily possible - basically, if you just did getActiveTransaction().startSpan() you'd always have inactive spans, and you needed to actually manually set this on the scope to run into this.

Now, as we basically promote startSpan() as the default method to use, it is much more easily possible to run into this.
Basically, if you have any component setup and you have two components running in parallel that create async spans- you'll already have an incorrect state, most likely 😬

With this change, in browser, you'll always have a hierarchy like this for the example you gave:

span name span bar
pageload xxxxxxxxxxxxxxxxxxxxxxxxxx
> data-loading-function xxxxxxxxx
> http-request-1 xxxx
> http-request-2 ____xxxxxx
> component-render ___________xxxxxxxxxxxxxxx

does this make sense? so the http requests would always be siblings of the data loading function span...

@@ -73,7 +73,7 @@ describe('Sentry.trackComponent()', () => {
description: '<Dummy$>',
op: 'ui.svelte.update',
origin: 'auto.ui.svelte',
parent_span_id: initSpanId,
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 (and src change) shows the actual change in semantics!

s1gr1d added a commit that referenced this pull request Mar 19, 2024
For showing current behavior with using differently timed async spans in
browser
ref #10986
ref #10944
@mydea mydea force-pushed the fn/browser-active-span branch from 9c599bd to 7798c67 Compare March 27, 2024 11:33
Copy link

codecov bot commented Mar 27, 2024

Bundle Report

Changes will decrease total bundle size by 618.37kB ⬇️

Bundle name Size Change
@sentry/types-cjs 35 bytes 0 bytes
@sentry/types-esm 35 bytes 0 bytes
@sentry/utils-cjs 179.0kB 0 bytes
@sentry-internal/tracing-cjs 107.96kB 0 bytes
@sentry/utils-esm 174.41kB 0 bytes
@sentry-internal/feedback-esm 65.28kB 0 bytes
@sentry/core-cjs 242.31kB 231 bytes ⬆️
@sentry/core-esm 238.74kB 231 bytes ⬆️
@sentry/node-experimental-esm 150.43kB 0 bytes
@sentry-internal/replay-cjs 306.18kB 0 bytes
@sentry/node-cjs 337.26kB 0 bytes
@sentry/node-esm 333.86kB 0 bytes
@sentry-internal/replay-esm 306.29kB 0 bytes
@sentry/aws-serverless-cjs 14.62kB 5.67kB ⬇️
@sentry-internal/replay-canvas-cjs 29.43kB 0 bytes
@sentry-internal/node-integration-tests-cjs 1.04kB 0 bytes
@sentry-internal/node-integration-tests-esm 888 bytes 0 bytes
@sentry-internal/replay-canvas-esm 29.34kB 0 bytes
@sentry/bun-cjs 13.53kB 0 bytes
@sentry/bun-esm 10.08kB 0 bytes
@sentry/google-cloud-serverless-esm 19.16kB 0 bytes
@sentry/google-cloud-serverless-cjs 23.0kB 0 bytes
@sentry/browser-cjs 107.29kB 137 bytes ⬆️
@sentry/browser-esm 104.48kB 137 bytes ⬆️
@sentry/svelte-esm 12.06kB 658 bytes ⬇️
@sentry/react-esm 41.18kB 0 bytes
@sentry/wasm-cjs 5.2kB 0 bytes
@sentry/svelte-cjs 13.19kB 649 bytes ⬇️
@sentry/astro-cjs 27.13kB 0 bytes
@sentry/astro-esm 23.39kB 0 bytes
@sentry/gatsby-cjs 2.88kB 0 bytes
@sentry/sveltekit-cjs 69.31kB 0 bytes
@sentry/wasm-esm 4.85kB 0 bytes
@sentry/vue-cjs 20.19kB 0 bytes
@sentry/vue-esm 18.85kB 0 bytes
@sentry/react-cjs 45.04kB 0 bytes
@sentry/sveltekit-esm 61.08kB 0 bytes
@sentry/nextjs-cjs 20.52kB 5.02kB ⬆️
@sentry/nextjs-esm 20.02kB 0 bytes
@sentry/profiling-node-cjs 25.5kB 0 bytes
@sentry/profiling-node-esm 25.52kB 0 bytes
@sentry-internal/integration-shims-cjs (removed) 3.65kB ⬇️
@sentry/node-experimental-cjs (removed) 157.11kB ⬇️
@sentry/gatsby-esm (removed) 2.27kB ⬇️
@sentry/remix-cjs (removed) 53.62kB ⬇️
@sentry/remix-esm (removed) 48.23kB ⬇️
@sentry/vercel-edge-cjs (removed) 18.23kB ⬇️
@sentry-internal/integration-shims-esm (removed) 2.99kB ⬇️
@sentry/vercel-edge-esm (removed) 16.13kB ⬇️
@sentry-internal/feedback-cjs (removed) 65.56kB ⬇️
@sentry/opentelemetry-cjs (removed) 71.63kB ⬇️
@sentry/opentelemetry-esm (removed) 70.52kB ⬇️
@sentry-internal/tracing-esm (removed) 107.22kB ⬇️

You can opt out of this by setting `parentSpanIsAlwaysRootSpan=false` in your client options.

update nested span handling in svelte
@mydea mydea force-pushed the fn/browser-active-span branch from 7798c67 to 4592d98 Compare April 10, 2024 10:02
@mydea mydea merged commit e4ec09e into develop Apr 10, 2024
97 checks passed
@mydea mydea deleted the fn/browser-active-span branch April 10, 2024 10:39
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request Apr 19, 2024
For showing current behavior with using differently timed async spans in
browser
ref getsentry#10986
ref getsentry#10944
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this pull request Apr 19, 2024
…entry#10986)

You can opt out of this by setting `parentSpanIsAlwaysRootSpan=false` in
your client options.

This means that in browser, any span will always be added to the active
root span, not the active span. This way, we should be able to avoid
problems with execution contexts etc.

Closes getsentry#10944
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.

[v8] Browser & active spans
2 participants