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

Update reportAllChanges behavior on INP #411

Closed
wants to merge 1 commit into from

Conversation

karreiro
Copy link

@karreiro karreiro commented Jan 8, 2024

Summary

Currently, the onINP function doesn't report all changes when the reportAllChanges flag is used. This is because the onINP module only tracks the top 10 longest interactions.

This PR updates the onINP module to report all INP interactions when the reportAllChanges flag is activated.

PR changes

Here's a quick rundown of the changes in this PR:

  • longestInteractionList has been renamed to interactionList
  • In the processEntry function, where we used to discard faster interactions, we now sort entries by startTime when the reportAllChanges flag is on
  • In the updateINPMetric function, we now grab the latest interaction when the reportAllChanges flag is on

Demos

Check out these demos:

  • Here's the onINP function after this PR:
  • Here's the onINP function after this PR with reportAllChanges set to false:
  • And here's the onINP function before this PR:

Copy link

google-cla bot commented Jan 8, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@tunetheweb
Copy link
Member

Hi @karreiro thanks for the PR but don't think this is one we want to merge.

I think this stems from a bit of misunderstanding about what "reportAllChanges" is intended to do.

It is intended to report ALL changes to the metric, NOT all inputs to the metric.

For example, if you have the following interactions as per your demo:

  • 312
  • 312
  • 104
  • 104
  • 1016
  • 1016

Then by default this would only report the largest (1016) on page unload (assuming the page had not been backgrounded earlier to force early reporting). With reportAllChanges it would report 312, and 1016 - and you can see this happens in your second demo.

The point of reportAllChanges is to allow you to early report your INP value, NOT to allow you to measure all interactions. 104 will never be the INP value in this case so it should never be reported as INP. reportAllChanges allows you to see when the first INP is set, and then see each time it increases.

This can be useful for early reporting (rather than waiting until a page hide), which is why it's important to still only report the must accurate known INP at that time, and each time that INP changes (which it wouldn't with your 104 examples).

This is also important for other metrics like LCP where it is not possible (or even desirable) to report "all the inputs" of LCP candidates throughout the life of the page, except for increasing ones.

In the web-vitals extension we recently added the logging of separate "interactions" as well as the INP to allow the type of monitoring you're looking for here. We haven't added that the web-vitals.js - and not sure we will since it's a bit outside it's direct use case or measuring in the field in as accurate a way as possible.

It is however apparent that we need to do a better job of documnting what "reportAllChanges" does in the README and/or code, as it's name could easily be taken to mean what you thought it meant.

@karreiro
Copy link
Author

karreiro commented Jan 9, 2024

Hi @tunetheweb, thanks for the review and the detailed explanation!

I was exploring this because in some situations, it could be handy to understand certain interactions. This understanding could help us figure out if we've managed to fix the root cause associated with a poor INP (INPMetric.entries[?].target), or if we're just dealing with a different problematic interaction.

But now, I totally understand the difference between interactions and INP, the purpose of web-vitals.js, and also the reportAllChanges flag. Thanks a lot for the clarification!

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

Successfully merging this pull request may close these issues.

None yet

2 participants