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

[v8] Browser & active spans #10944

Closed
mydea opened this issue Mar 6, 2024 · 0 comments · Fixed by #10986
Closed

[v8] Browser & active spans #10944

mydea opened this issue Mar 6, 2024 · 0 comments · Fixed by #10986

Comments

@mydea
Copy link
Member

mydea commented Mar 6, 2024

In Node & edge environments, we use async local storage to ensure we have actual isolation for active spans. So the following code:

startSpan({ name: 'outer 1' }, async () => {
  await waitForSeconds(2);
  console.log(getActiveSpan());
});

startSpan({ name: 'outer 2' }, async () => {
  await waitForSeconds(3);
  console.log(getActiveSpan());
});

Will actually work and log outer 1 and outer 2 correctly.
However, in browser (and theoretically any other environment where we don't have a custom Async Context Strategy (ACS)), this will not necessarily work as expected, because we cannot isolate the execution contexts, thus they may leak out into other parts of the code - so the above code may print out different spans, depending on what is globally active at the time.

This was always the case - also with the old performance APIs - but with the new APIs it is much easier to accidentally do this.

Previously, if you just did getActiveTransaction().startChild() or similar, stuff would work just fine - you had to actually go and do getCurrentScope().setSpan(span) to get potentially inconsistent behavior.

Since now startSpan (and startSpanManual) are the "default" APIs proposed for these things, I feel like maybe this will become a bigger problem in v8, and we should address this somehow.

Some options I see:

  1. We do nothing - this will basically work when passing a sync callback, and it may or may not work when passing async callbacks.
  2. In browser environment, we make startSpan and startSpanManual not actually update the active span. This would mean that the only active spans would ever be ones that we create (? or we provide a separate API/option for that...?), so pageload/navigation spans... we'd need some story for manual page load etc. instrumentation though as well 🤔
  3. Or we could do 2 only if an async callback is provided, not if a sync callback is provided. But I guess this would only make things even more confusing...

Basically, I think it would be safer to say that in browser we don't have a nested span structure, but we only have a single active span (pageload, navigation, ...) and all other spans are children of that. That of course also has downsides, but to me less severe ones. But 🤷 there is no ideal solution here 😬

s1gr1d added a commit that referenced this issue Mar 19, 2024
For showing current behavior with using differently timed async spans in
browser
ref #10986
ref #10944
mydea added a commit that referenced this issue Apr 10, 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
cadesalaberry pushed a commit to cadesalaberry/sentry-javascript that referenced this issue 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 issue 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 a pull request may close this issue.

1 participant