fix(performance): Fix INP spans not retrieving correct origin transaction name #12358
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
size-limit report 📦
|
|
might have to figure out how to reduce bundle size on this one, or raise the threshold |
mydea
added a commit
that referenced
this pull request
Jun 5, 2024
Instead of #12358, this is a simpler change which ensures we pick the transaction from the scope instead. I also added tests for the various different scenarios, to ensure we see how they behave: 1. INP is emitted _during_ pageload (span is active) 2. INP is emitted _after_ pageload a. Pageload is parametrized (route) b. Pageload is unparametrized (URL)
mydea
requested changes
Jun 5, 2024
There was a problem hiding this comment.
I would prefer if we could instead do something like #12372, which is much simpler and should be good enough for the vast majority of cases I believe...? This change is pretty complex, subject to leak memory and increases bundle size, which is not ideal 😬
If @edwardgou-sentry you think the other PR is not sufficient here, let's connect on what other things we could explore to improve this!
👍 left a comment in the other pr! |
mydea
added a commit
that referenced
this pull request
Jun 6, 2024
Instead of #12358, this is a simpler change which ensures we pick the transaction from the scope instead. I also added tests for the various different scenarios, to ensure we see how they behave: 1. INP is emitted _during_ pageload (span is active) 2. INP is emitted _after_ pageload a. Pageload is parametrized (route) b. Pageload is unparametrized (URL)
mydea
added a commit
that referenced
this pull request
Jun 6, 2024
Instead of #12358, this is a simpler change which ensures we pick the transaction from the scope instead. I also added tests for the various different scenarios, to ensure we see how they behave: 1. INP is emitted _during_ pageload (span is active) 2. INP is emitted _after_ pageload a. Pageload is parametrized (route) b. Pageload is unparametrized (URL)
|
Closing this PR in favour of #12372 |
mydea
added a commit
that referenced
this pull request
Jun 6, 2024
Instead of #12358, this is a simpler change which ensures we pick the transaction from the scope instead. I also added tests for the various different scenarios, to ensure we see how they behave: 1. INP is emitted _during_ pageload (span is active) 2. INP is emitted _after_ pageload a. Pageload is parametrized (route) b. Pageload is unparametrized (URL)
mydea
added a commit
that referenced
this pull request
Jun 7, 2024
Instead of #12358, this is a simpler change which ensures we pick the transaction from the scope instead. I also added tests for the various different scenarios, to ensure we see how they behave: 1. INP is emitted _during_ pageload (span is active) 2. INP is emitted _after_ pageload a. Pageload is parametrized (route) b. Pageload is unparametrized (URL)
mydea
added a commit
that referenced
this pull request
Jun 7, 2024
Instead of #12358, this is a simpler change which ensures we pick the transaction from the scope instead. I also added tests for the various different scenarios, to ensure we see how they behave: 1. INP is emitted _during_ pageload (span is active) 2. INP is emitted _after_ pageload a. Pageload is parametrized (route) b. Pageload is unparametrized (URL)
mydea
added a commit
that referenced
this pull request
Jun 7, 2024
Instead of #12358, this is a simpler change which ensures we pick the transaction from the scope instead. I also added tests for the various different scenarios, to ensure we see how they behave: 1. INP is emitted _during_ pageload (span is active) 2. INP is emitted _after_ pageload a. Pageload is parametrized (route) b. Pageload is unparametrized (URL) When the pageload is unparametrized (default browser SDK), the transaction is not added to the DSC envelope header (which is correct and also what we do in other places). it is _always_ added to the span attributes now, though. If no span is active, it will use transactionName from the last active pageload/navigation span. There may be edge cases where this is not 100% correct (e.g. if the INP span is only emitted once the pageload is done but another navigation already started) but IMHO these are more edge cases and this change is probably fine for now..? (While at it, I also added an origin to the INP spans)
billyvg
pushed a commit
that referenced
this pull request
Jun 10, 2024
Instead of #12358, this is a simpler change which ensures we pick the transaction from the scope instead. I also added tests for the various different scenarios, to ensure we see how they behave: 1. INP is emitted _during_ pageload (span is active) 2. INP is emitted _after_ pageload a. Pageload is parametrized (route) b. Pageload is unparametrized (URL) When the pageload is unparametrized (default browser SDK), the transaction is not added to the DSC envelope header (which is correct and also what we do in other places). it is _always_ added to the span attributes now, though. If no span is active, it will use transactionName from the last active pageload/navigation span. There may be edge cases where this is not 100% correct (e.g. if the INP span is only emitted once the pageload is done but another navigation already started) but IMHO these are more edge cases and this change is probably fine for now..? (While at it, I also added an origin to the INP spans)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a
registerInpInteractionListenerhandler to browser tracing that caches an interactions to transaction name mapping (interactionIdToRouteNameMapping). This mapping allows us to fetch the correct origin transaction name that an INP occurred on, instead of looking for an active pageload/navigation span (which is inaccurate, and might not even exist at the time of INP).This also reduces the chances that
transactionwill be undefined for INP spans, which mitigates this issue: getsentry/sentry#71435