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

Consider expose FID duration as first INP score, for cases when its under durationThreshold #228

Closed
mmocny opened this issue May 12, 2022 · 3 comments
Milestone

Comments

@mmocny
Copy link
Member

mmocny commented May 12, 2022

onINP relies on event timing durationThreshold and provides a default of 40ms (configurable down to 16ms).

There are cases where we report onFID but not onINP because of this threshold. first-input event timing actually does already expose the duration value so we can report the full latency exactly as INP would measure it, and not just the input delay.

Would help address GoogleChrome/web-vitals-extension#103

@philipwalton
Copy link
Member

philipwalton commented May 12, 2022

This is something I had originally considered but ultimately decided against for a few reasons:

  • first-input entries do not have an interactionId, which slightly complicates the logic. It also means the other event entries from that interaction are not captured, so you won't know the real processing/presentation time breakdown.
  • If we report the first-input entry it may give the false impression that INP was actually that low, whereas reporting 0 subtly communicates that INP was somewhere between 0 and durationThreshold (but we don't know exactly what it was).

I don't think either of these are really strong reasons not to, but I'm also hesitant to add code to the library for a use-case that really only benefits the extension (which can do this itself).

The other thing is, if you don't need the reportAllChanges feature (which most people don't), then a really simple solution to this problem is to just to the following:

onINP((metric) => {
  // If the metric show 0, fall back to the `first-input` entry.
  if (metric.value === 0) {
    const entry = performance.getEntriesByType('first-input')[0];
    metric.entries = [entry]
    metric.value = metric.delta = entry.duration;
  }
  console.log(metric);
});

But to add proper support for this in the library, then I'd have to integrate it into the logic that keeps track of the 10 longest interactions, which adds complexity that most people likely don't need.

Anyway, like I said above, I'm not 100% against adding this, but I want to make sure it's useful to regular users of the library and not just add it for the extension.


Tangentially, I wish we had made the first-input behavior part of the Event Timing API rather than its own separate entry type. E.g. it'd be so much easier if folks who want to understand the first input AND long inputs to do this:

new PerformanceObserver(console.log).observe({
  type: 'event',
  buffered: true,
  durationThreshold: 40,
  includeFirstInteraction: true, // Rather than a separate entry type.
});

@mmocny
Copy link
Member Author

mmocny commented May 16, 2022

first-input entries do not have an interactionId

Filed crbug.com/1325826

If we report the first-input entry it may give the false impression that INP was actually that low.

It is that low, at the time of the first interaction. I guess you mean that the second interaction, if it was <durationThreshold, may affect INP in ways we don't catch with onINP. Fair, but tbh, that doesn't convince me we shouldn't record the first-input.

only benefits the extension (which can do this itself)

I do agree the current demand is just the extension, and we can fix there. However, if any RUM providers start measuring INP, the fact that INP is an OPTIONAL metric, recording NULL in cases where there is an interaction will have a larger measurement effect than recording FID.duration instead of some slightly larger INP.

a really simple solution to this problem is to just to the following

I like it!

Tangentially, I wish we had made the first-input behavior part of the Event Timing API

I wasn't here for the history, but my understanding is that first-input shipped before event timing and we don't want to remove anything ever. So it wasn't so much that we added first-input, as we added event.

includeFirstInteraction: true

Interesting. Personally, I don't think we need this property. I agree that it would be nice if event timing could expose the first-input even if it was under 16ms. Perhaps we could change the durationThreshold minimum of 0 to allow actual 0 for the first-input only?

That wouldn't support the case you show above (asking for 40ms min and first-input) but can handle that in either of the above strategies you list.


Net/net, I think you are right that we don't need to add this to web-vitals.js. Even though I worry about the default behaviour, there are simple workarounds that can be used. We may be able to just fix the spec to make it work better by default.

@mmocny
Copy link
Member Author

mmocny commented May 16, 2022

One more consideration:

let po = new PerformanceObserver(list => {
  for (let entry of list.getEntries()) {
    if (entry.entryType != 'first-input' || !entry.interactionId)
      return;

    console.log(`[Event] entryType: ${entry.entryType}, type: ${entry.name}, id: ${entry.interactionId}, duration: ${entry.duration}`);
  }
});

po.observe({ type: 'event', durationThreshold: 40, buffered: true })
po.observe({ type: 'first-input', buffered: true})

...seems to work well enough. It's roughtly equivalent to includeFirstInteraction: true with the caveat that interactionId is not correct.

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

2 participants