fix(browser): Fix INP span creation & transaction tagging #12372
Conversation
size-limit report 📦
|
| @@ -83,7 +84,9 @@ function _trackINP(): () => void { | |||
| const activeSpan = getActiveSpan(); | |||
| const rootSpan = activeSpan ? getRootSpan(activeSpan) : undefined; | |||
|
|
|||
| const routeName = rootSpan ? spanToJSON(rootSpan).description : undefined; | |||
| // If there is no active span, we fall back to look at the transactionName on the scope | |||
There was a problem hiding this comment.
Hm, this may produce a lot of garbage data when the user does custom instrumentation. Normally I would say this is fine but "garbage data" in this case would be polluting the INP dashboard in Sentry massively with random crap and people have no way of figuring out why.
Could we just store the last active route on some global and read it here? That way we will never not receive route/url names - never ending up with crap in the INP dashboard.
There was a problem hiding this comment.
but this is exactly the semantics of the transactionName on the scope, isn't it? we use this literally to set this as "transaction" on the INP span :D makes sense to me to use the same thing we set as transaction on e.g. errors here?
There was a problem hiding this comment.
Ah I am sorry. That is correct. Mentally I was still in the old world where we had transactions on the scope 😄 All good then!
|
Hey @mydea one thing to keep in mind is that when INP is reported, it is not always on the last pageload/navigation.
The proper route for this interaction should be Another smaller/side point we should consider, INP is the p98 of all interactions, so we need a mechanism to know the route of the p98, not just the p100: |
This is a PR adjusting #12372 to keep transaction name for INP even if the route has changed in the meanwhile. Now, we keep a map of the last 10 interactionId <> route names, which we use instead (if possible).
|
Addressed concerns in #12378, which while not 100% should handle most cases. We can ship it like this for now and possibly revisit this later if we find this does not work perfectly in certain scecnarios. |
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)
This is a PR adjusting #12372 to keep transaction name for INP even if the route has changed in the meanwhile. Now, we keep a map of the last 10 interactionId <> route names, which we use instead (if possible).
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)
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:
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)