-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(browser): Ensure pageload trace remains active after pageload span finished #11600
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved this test to the trace-lifetime
directory where I plan on putting all tests from #11599
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice find
size-limit report 📦
|
Hmm, do we not need to extend this then to ensure we also update the propagation context when a navigation span starts? And add a test to cover this scenario:
--> I guess this is this test here: #11601 if that passes, all good I guess 😅 |
We already do this and #11601 adds a test for this scenario (already verified locally - the test passes).
Do you mean standalone spans? I'm not sure what the scenario is here and how we best test this, given that currently the only standalone spans are INP spans and they're not yet in v8 and a bit unpredictable afaict 🤔 |
All good, I saw the other test that covers this! Only thing missing then (which we can do in a follow up) is to also keep the DSC from the pageload/navigation spans on the propagation context when they end, but that's a further improvement 🚀 |
While working on integration tests for #11599, I found that initial pageload traces still ended after the pageload root span was ended due to us actively resetting the propagation context this way.
This PR removes the now incorrect propagationContext resetting logic for pageload spans and adds tests to cover this (and closely related) scenario.
ref #11599