search

GoogleChrome / web-vitals

Essential metrics for a healthy site.
https://web.dev/vitals
Apache License 2.0
7.47k stars 410 forks source link

Fix missing LoAF attribution when entries are dispatched before event entries #487

Closed brendankenny closed 3 months ago

brendankenny commented 3 months ago

fixes #485

As described there, I added an extra condition to keep any LoAFs around that are after any existing known interactions, in case those interactions then come in later.

I then realized that would leave the LoAF collection unbounded, since a page can have a bunch of slow animation frames without the user interacting. I added a MAX_UNPAIRED_LOAFS of 20, so it will only keep the 20 most recent LoAFs after any known user interaction events. The number isn't hugely important, that just allows a one second delay in event-timing reporting even if the page somehow has filled the time with the shortest (50ms) long animation frames possible. LoAF entries are fairly light weight, so that's not so bad.

As I was implementing this, however, I realized that even without this change the pendingLoAFs array can grow without bound on a page with e.g. animations and a busy main thread, since LoAFs are appended as they come in but clean up is only scheduled by user interactions. Seems like we should fix? @philipwalton @tunetheweb

My only idea right now is to split the event timing and LoAF cleanups into two separate functions, and run each as their respective entries come in, since that's the only time each list of entries grows.

brendankenny commented 3 months ago

Oh, also I could not get a test for this working reliably. Inherently flaky and subject to future Chrome scheduler changes anyways. I verified the issue repro no longer fails, but looking for other ideas on this. Time to bust out some unit tests? Override PerformanceObserver in an e2e test?

tunetheweb commented 3 months ago

As I was implementing this, however, I realized that even without this change the pendingLoAFs array can grow without bound on a page with e.g. animations and a busy main thread, since LoAFs are appended as they come in but clean up is only scheduled by user interactions. Seems like we should fix? @philipwalton @tunetheweb

Maybe we should do this clean up in handleLoAFEntries then (behind a whenIdle call)? At the moment we queue a clean up interactions when new interactions come in, so we should also queue LoAFs clean ups when they come in.

philipwalton commented 3 months ago

Updated, and also moved the whenIdle() call to within the main onINP() function (as mentioned here) to ensure that attribution is always processed synchronously with the INP dispatch.