fix(tracing-internal): Delay pageload transaction finish until document is interactive #10215
Conversation
…nt is interactive
lforst
force-pushed
the
lforst-idle-transaction-wait-until-interactive
branch
from
2aa8cdb to
3b5bb8d
Compare
lforst
changed the base branch from
lforst-absolutely-unhinged-nextjs-experiment
to
develop
size-limit report 📦
|
|
@AbhiPrasad and I had a chat on slack and we both deem this change useful. In a future PR we may want to create a span for DOM processing.
|
There was a problem hiding this comment.
I like the idea! Just a bit worried about adding another public method but since we'll need something like an IdleSpan anyway, I think we should be good.
A bit out of scope but as an idea: what if we add a 0ms span/mark to the transaction when we reach the interactive point? This should make the txn duration is more transparent to users.
also +1 on tests (maybe for these, such a span would even be helpful 🤔)
@Lms24 The "interactive" document status doesn't mean a lot I believe. I am not sure what it really indicates. The MDN docs say
and
But I think DOM interaction through JS is already possible before (?) 🤷. We already have a mark for the |
There was a problem hiding this comment.
cc @krystofwoldrich - not sure if this breaks anything with RN
| /** | ||
| * Restarts idle timeout, if there is no running idle timeout it will start one. | ||
| */ | ||
| private _restartIdleTimeout(endTimestamp?: Parameters<IdleTransaction['end']>[0]): void { | ||
| this.cancelIdleTimeout(); | ||
| this._idleTimeoutID = setTimeout(() => { | ||
| if (!this._finished && Object.keys(this.activities).length === 0) { | ||
| if (!this._finished && Object.keys(this.activities).length === 0 && this._autoFinishAllowed) { |
There was a problem hiding this comment.
do we need this check here? 🤔 as we are guarding even calling this function above?
There was a problem hiding this comment.
overall I think this makes sense! The whole idle transaction code is a bit confusing IMHO but I wouldn't spent too much time on this now as we'll rewrite this soonish with something like #9972 anyhow. I guess this should then be the default behavior in v8?
|
@AbhiPrasad Thanks for the ping. Looks good for RN. |
Explanation
Before this PR we automatically finished pageload transactions when there were no spans on the transaction for max 1 second, OR when there were no added/removed child spans consecutively in 3x5 seconds.
This caused inaccuracies in some cases, however, with the Next.js app router this causes problems consistently. Reason being that Next.js streams in the document (HTML) meaning the document
"interactive"readyStateis delayed by as long as the streaming response hasn't finished yet.The symptom of this is that, in Next.js applications (and also all other apps that have long document loading times), pageload transactions will be finished prematurely, because while the document is streaming in, it is possible that there are no additional spans on the transaction, meaning that the
IdleTransaction'sidletimeout(1s) kicks in and finishes the transaction.Ideally we fix this by delaying finishing pageload transactions until the document
readyStateis"interactive". Here is a great illustration on when exactly within the document loading pipeline this is: https://developer.mozilla.org/en-US/docs/Web/API/PerformanceNavigationTimingFor illustrative purposes of this change, here is what a pageload transaction looked like for a slow Next.js page before this PR:
Here is what it looks like after:
The difference is huge but it is arguably what the user would want. Notice that the request and response spans are completely missing in the first image because the transaction was finished before the data for these spans could be collected.
A piece of consideration: In traditional applications, this should not change the behaviour and more importantly the duration of pageload transactions by much. Only if you have an application that streams in HTML you will notice a difference, however, the new approach is much more accurate.