fix(browser): Ensure pageload & navigation spans have correct data #16279
Conversation
size-limit report 📦
|
mydea
force-pushed
the
fn/browser-span-data
branch
2 times, most recently
from
285cf52 to
aa79811
Compare
mydea
force-pushed
the
fn/browser-span-data
branch
from
aa79811 to
8102ffc
Compare
|
|
||
| scope.setSDKProcessingMetadata({ | ||
| normalizedRequest: undefined, | ||
| }); | ||
| }, | ||
| }); | ||
|
|
||
| setActiveIdleSpan(client, idleSpan); | ||
|
|
||
| // We store the normalized request data on the scope, so we get the request data at time of span creation | ||
| // otherwise, the URL etc. may already be of the following navigation, and we'd report the wrong URL | ||
| getCurrentScope().setSDKProcessingMetadata({ | ||
| normalizedRequest: getHttpRequestData(), | ||
| }); | ||
|
|
There was a problem hiding this comment.
m/q: maybe I'm missing some timing information but isn't there a good chance that setting the normalizedRequest to undefined in 360 applies to the same scope where we previously set it (370)?
There was a problem hiding this comment.
argh, actually that does not work I just noticed, it fixed one test but broke another one - as this resets the request before we even process the transaction 😬 need to do this in a different way... basically the problem was apparent in some e2e tests. I think it becomes problematic in certain cases, e.g. nextjs pages router, where routing instrumentation triggers navigation spans before the url is updated 🤔
| @@ -459,16 +469,18 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio | |||
| return; | |||
| } | |||
|
|
|||
| if (from !== to) { | |||
| startingUrl = undefined; | |||
| startingUrl = undefined; | |||
There was a problem hiding this comment.
m/q: why did we remove the from !== to check?
There was a problem hiding this comment.
sorry, forgot to mention this - we actually have the same check in the history handler code, where we do not even trigger the handler in this case!
| // We wait a tick here to ensure that WINDOW.location.pathname is updated | ||
| setTimeout(() => { |
There was a problem hiding this comment.
m: I'm a bit worried about delaying the span start for a tick. I guess this comes down to the router or user implementation but what if users synchronously push a new state and e.g. make a fetch call directly afterwards? Would we still catch the span?
i think alternatively, we could still start the span directly but call updateSpanName once more in the setTimeout closure.
Not ideal in terms of DSC consistency but I'd rather have the span than miss it. WDYT?
There was a problem hiding this comment.
hmm, valid. one thing that would work as expected is if we just would use the to attribute from the history API, this is available already. I was afraid to do that because 🤷 if that is consistent with what we want/need there...?
|
OK, so I updated this based on feedback, to a more reliable heuristic I think:
|
mydea
force-pushed
the
fn/browser-span-data
branch
from
4472bc8 to
cee11ea
Compare
This reverts commit 5cd662a.
| @@ -459,16 +469,18 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio | |||
| return; | |||
| } | |||
|
|
|||
| if (from !== to) { | |||
| startingUrl = undefined; | |||
| startingUrl = undefined; | |||
This PR fixes two things about our idle spans emitted from
browserTracingIntegration:window.location.pathnameat the time when thepopstateevent is emitted - but at this point, this may not be updated yet. So anavigationtransaction would possibly have the pathname of the previous page as transaction name.HttpContextintegration at event processing time. However, at this time thewindow.locationdata is also usually already of the following navigation, so the pageload would often have wrong request data associated to it. Now, we store this on the current scope at span creation time to ensure it is actually correct.