Zizzamia / perfume.js

Web performance library for measuring all performance vitals metrics
https://zizzamia.github.io/perfume/
MIT License
3.13k stars 112 forks source link

Question about Interaction to Next Paint (INP) implementation #194

Closed SuperOleg39 closed 2 years ago

SuperOleg39 commented 2 years ago

Hello!

Do you have a plans for Interaction to Next Paint (INP) metric implementation? Looks like implementation can be non-trivial, according to web-vitals package implementation.

I am interested in your opinion on this metric, as it will affect the size of the package badly, but it seems that this metric still have to start collecting in the future.

SuperOleg39 commented 2 years ago

@Zizzamia hello! What do you think?

Zizzamia commented 2 years ago

Ciao @SuperOleg39, thank you for the ping and sorry for the late reply.

Something I am been thinking for quite a while if it's time to make Perfume.js a superset of the Web Vitals package, by importing the package into Perfume and add some extra functionality on top of it.

This will help stay always align with Google updates, and also keep the extra features that Perfume wants to experiment and offers.

What's your thoughts on this?

SuperOleg39 commented 2 years ago

Hi! Thanks for the answer)

I see only one problem about web-vitals integration - result package size, because at this moment web-vitals have 2.5kb gzip size, аnd perfume.js is 2.3kb gzip (Bundlephobia info).

What do you think about size-limit integration before?

SuperOleg39 commented 2 years ago

@Zizzamia another reason to add web-vitals - https://github.com/GoogleChrome/web-vitals/blob/main/CHANGELOG.md#v300-2022-08-24

"Report TTFB after a bfcache restore" and new "attribution build" looks like very important and useful.

Zizzamia commented 2 years ago

Yeah, I think Perfume.js is better suited to be a superset of web-vitals going forward. Going to make the change pretty soon. Just need to find a few hours to make the switch.

Recently I got distracted on building a Performance library for React Native. But now I am back with more coding time on my hands.

Zizzamia commented 2 years ago

Alright, now Perfume.js is a super set of Web Vitals, starting from v8.0.0.

Please, if you see anything that is not working let me know and I will take care of it.

SuperOleg39 commented 1 year ago

Thank you! Will try to integrate soon.

SuperOleg39 commented 1 year ago

Only one problem, now perfume is 5kb gzip.

But I think than lazy loading solves this problem partially.

SuperOleg39 commented 1 year ago

Looks like First Paint is unsupported in web-vitals

SuperOleg39 commented 1 year ago

About Cumulative Layout Shift, there is a big change, when CLS fired after page visibility changed to hidden.

It is mean than we can send this metrics only when user leave/hide page twice, because if user will just close page after session, pagehide with sendBeacon will be sended before visibilitychange event (https://github.com/whatwg/html/issues/3957), and no information about CLS.

Maybe reportAllChanges for CLS will be a good default? https://github.com/GoogleChrome/web-vitals/pull/283

Upd. also ask question in web-vitals repo - https://github.com/GoogleChrome/web-vitals/issues/180#issuecomment-1337530037

SuperOleg39 commented 1 year ago

About Cumulative Layout Shift, there is a big change, when CLS fired after page visibility changed to hidden.

The same behaviour with Total Blocking Time

For now it is impossible to collect this metrics when user closing page, if batching is used https://github.com/GoogleChrome/web-vitals#batch-multiple-reports-together

I will try to make a MR with optional reportAllChanges integration`, and looks like we need the same behaviour for TBT

SuperOleg39 commented 1 year ago

@Zizzamia maybe we start with this changes - https://github.com/Zizzamia/perfume.js/pull/208/files

About TBT, looks like it fired 10+ seconds after FID (expect 5 second because of https://zizzamia.github.io/perfume/#/total-blocking-time/) - so nothing to change here)

Zizzamia commented 1 year ago

You right! The fact Perfume does not handle on his on Change Visibility, it does introduce a regression. Which is not good, and makes the data quite unuseful.

I am going to introduce back some fo the code related to Change Visibility that we used to have, and release a new version by the end of the year.

Thank you for keep trying Perfume.js!!!! Making Perfume.js a superset of web-vitals will require some extra love to make it right. ✨

SuperOleg39 commented 1 year ago

Will waiting for https://github.com/GoogleChrome/web-vitals/issues/180#issuecomment-1344720373

Zizzamia commented 1 year ago

@SuperOleg39 your work in testing both libraries is just the best OSS Christmas present. Thank you man.

I reverted some of the changes I initially made, and trying to recover the initial value of Perfume.js but still keeping the library as superset of web vitals.

https://github.com/Zizzamia/perfume.js/releases/tag/v8.1.6

Let me know if looks better on your side, and love your testing in making Perfume.js better. I will make sure to prioritize your advice going forward.

SuperOleg39 commented 1 year ago

Thank you for all this work!)

Test https://github.com/GoogleChrome/web-vitals/issues/180 PR with perfume.js@8.2.0 and reportAllChanges for CLS and INP - this metrics works perfect for my case!

Have only trouble with TBT - looks like this metric stop reported?

SuperOleg39 commented 1 year ago

Looks like TBT not reported, when:

SuperOleg39 commented 1 year ago

Found this code https://github.com/Zizzamia/perfume.js/blob/master/src/log.ts#L42

Zizzamia commented 1 year ago

Yeah the case you described is expected! That's because the reporting to be accurate Perfume needs to wait the 10s to be over.

Glad Perfume, now works well for you. Please let me know if there is anything I can help you with more.

SuperOleg39 commented 1 year ago

Thanks for the explanation)