Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't include first-input entry duplicating event entry in INP entries? #547

Open
brendankenny opened this issue Oct 15, 2024 · 3 comments

Comments

@brendankenny
Copy link
Member

This is technically a breaking change, so if done it probably needs to be in v5. It's also pretty minor so it's not the end of the world if we close this as won't fix.

But it's a little strange that the INPMetric.entries array can include (basically) the same event twice if the event was also the first input. example:
web-vitals INP object, showing two identical 'pointerdown' event entries, except one is of entryType 'event' and the other entryType of 'first-input'

Maybe entries should be filtered of the first-input entry if there's an otherwise identical event entry? Or just drop the first-input entry if the INP was long enough to not need it for timing?

@philipwalton
Copy link
Member

There used to be code that tried to dedupe these at collection time, but it was changed in #442 to simplify things and address edge cases (e.g. turns out you can't just assume that if the first-input entry duration is longer than the passed durationThreshold value, then there will be an equivalent event entry).

I think it would be fine to filter these out at reporting time, but TBH I think there are some benefits to knowing if the INP event is also the first input.

@mmocny
Copy link
Member

mmocny commented Oct 15, 2024

turns out you can't just assume that if the first-input entry duration is longer than the passed durationThreshold value, then there will be an equivalent event entry

I can think of cases where the event wouldn't be reported until later, and I think there might be some cases where it get's filtered from getting interactionID but FID doesnt (the "state machine" for interactions is more complex than FID), but if you are saying the Event doesn't even get reported to Event Timing I would love a repro!

I think there are some benefits to knowing if the INP event is also the first input.

The aspiration in Chromium is to change the first-input event timing to just be the first event timing with non-0 interactionId, and potentially also to report "all parts" of that interaction (thought that might be breaking change and not worth it).

@philipwalton
Copy link
Member

I can think of cases where the event wouldn't be reported until later, and I think there might be some cases where it get's filtered from getting interactionID but FID doesn't (the "state machine" for interactions is more complex than FID), but if you are saying the Event doesn't even get reported to Event Timing I would love a repro!

I believe the issue with the old code was that it would:

  1. Assume that if a first-input entry was received that was greater than durationThreshold, it was safe to throw it away because that would mean a corresponding event entry would have already been received. This turned out to not be true, see Fix onINP() attribution error due to empty entries #452 for details.
  2. Assume that event entries are always dispatched before first-input entries, meaning if a first-input entry was received, you could scan through the existing entries and see if a matching one existed. I believe (but I can't fully remember) that this was also not always true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants