Simplest version of console logging each and every interaction

[mmocny]

Fixes #109

[mmocny]

@tunetheweb would appreciate taking a look.

With the recent patches adding most of the work, the cleanest way to implement this change was to supply an alternative implementation onEachInteraction that has the same interface as web-vitals.js onINP.

The PR as is does not expose a configuration option for report-all nor does it mark when the new Interaction is or is not a new worst-case.

I think we could expose a settings option, but I'm partial to just getting the default right. Console logging is already a "debugging" feature.

However, perhaps right now the console.logging is a bit too spammy? Some options:

• Add a "debounce" to the INP report, similar to CLS, which summarizes recent interactions within a time window (once per second perhaps?).
• Switch to animation frame based reporting (one report per Next Paint, aka LoAF based), which seems like the best option but not easy to implement for chrome stable today.

[tunetheweb]

I think it's confusing to expose an interaction as "INP" when it's not the INP. For example:

The second one is less than the first one so is clearly not a new INP, but we've labelled it as INP.

So I think we should continue to log the INP metric, but log this as a separate "interaction" metric. Either always displaying the current INP like this:

[Web Vitals Extension] INP 48: Interaction: 48
[Web Vitals Extension] INP 48: Interaction: 40

Or logging these two types on separate lines like this

[Web Vitals Extension] Interaction: 48
[Web Vitals Extension] INP 48
[Web Vitals Extension] Interaction: 40

Note the second one has duplicated the first one (once as an "Interaction" and once as the "INP" value). Not sure if we can combine these to prevent that duplication:

[Web Vitals Extension] INP Interaction: 48
[Web Vitals Extension] Interaction: 40

WDYT?

I'm in two minds about exposing this as a separate config setting. I like the simplicity of not doing that, but worry about how noisy it gets on a highly interactive site. Will think on it some more...

[mmocny]

I think it's confusing to expose an interaction as "INP" when it's not the INP. For example:

Agreed. I can replace the word INP with the word Interaction. Not sure if we want to also expose the real INP value if it changes? That... does seem like it needs a setting.

So I think we should continue to log the INP metric, but log this as a separate "interaction" metric.

Makes sense. It should be easy to do it in one line by just saving the score, but, then you won't get attribution and you'll have to do mental gymnastics, so I'll try to just do it with separate lines.

Note the second one has duplicated the first one (once as an "Interaction" and once as the "INP" value). Not sure if we can combine these to prevent that duplication: WDYT?

That would be ideal-- let me see how much effort that is

I'm in two minds about exposing this as a separate config setting. I like the simplicity of not doing that, but worry about how noisy it gets on a highly interactive site. Will think on it some more...

Agreed! We could land without the option and then add an option if we get complaints. That way it gets discovered faster?

[mmocny]

• Updated the strings to be "Interaction" not "INP"
• Changed onEachInteraction to group interactions by interactionID, to reduce logging.
  • web-vitals.js already does this for INP.
• Because of that, I also changed to console.table a breakdown table for each part
  • also do this for INP. I think this is an improvement, see below:

One more feature to go: reduce the duplication when you get a new INP.

[mmocny]

Done. Found a surprisingly simple solution.

[brendankenny]

• fixed some minor lint stuff (092a60f)
• tracked down the negative durations (a0c1664)

[mmocny]

Thanks Brendan for the patches!

[mmocny]

I accepted the suggestion to remove the INP Interaction comparison, because I won't have the time to make improvements.

• onINP is registered before onEachInteraction, and so the perf observer callback should come first
• However, if there is any async "hop" for dispatching the callback, onEachInteraction will fire its callback first.
  • For example, in a prior iteration of this patch we would async hop for chrome.storage and I had to make sure not to do that.
  • onINP in web-vitals.js seems to unconditionally call report() synchronously, but I'm not sure if bindReporter has some reasons to delay dispatch...

[tunetheweb]

Cool. Good to merge from your side @mmocny ?

Will still wait until @brendankenny gives his OK too in case he has any last comments.

[mmocny]

Yep! Thanks!

On Tue, May 2, 2023, 13:59 Barry Pollard @.***> wrote:

Cool. Good to merge from your side @mmocny https://github.com/mmocny ?

Will still wait until @brendankenny https://github.com/brendankenny gives his OK too in case he has any last comments.

— Reply to this email directly, view it on GitHub https://github.com/GoogleChrome/web-vitals-extension/pull/120#issuecomment-1531345540, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADTZKVI5EPE2UZTZTMAIYDXEDZLRANCNFSM6AAAAAAXPQXN3A . You are receiving this because you were mentioned.Message ID: @.***>