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

feat: Capture Web vitals #702

Closed
wants to merge 3 commits into from
Closed

Conversation

mhennoch
Copy link
Contributor

@mhennoch mhennoch commented Oct 15, 2021

This PR is an initial try to upstream web-vitals capturing.

Web Vitals is an initiative by Google to provide unified guidance for quality signals that are essential to delivering a great user experience on the web. Currently the core web vitals are:

  • Largest Contentful Paint (LCP)
  • First Input Delay (FID)
  • Cumulative Layout Shift (CLS)

Google has a library that can be used to capture them. It currently has 1.4m weekly downloads. This instrumentation is basically just a wrapper to that library. Ran into several issues so opening this not so polished PR to get some feedback. Main Issues:

  1. Currently only signal that is stable enough is span but it doesn't really make sense to send them as spans. in theory LCP and FID could be made into spans as there is a start time and duration but CLS is just a score eg. 0.1234. We have been using 0 duration spans for these where the real value is sent as span attribute. Eventually these should probably be metrics/logs. Any better ideas for now? Discussed at SIG, if I take ownership it is fine to use spans for now
  2. I have no access to event handlers set by web-vitals library to remove them in disable method. Currently browser instrumentations are enabled twice if you don't use {enabled: false} in config. So in order to not have double the spans I keep the enable status myself. Can't access parent's _enabled or super.enable. Not sure what the usecase for enable/disable is especially in browser. Related issues instrumentation-document-load doesn't support re-enables #616, #2410 Fixed with #2610
  3. Can't unit tests FID as it doesn't seem to happen if event is created by JS eg. document.body.click(). I think these should be tested with selenium only (needs #2570). Currently have some very basic unit tests only.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 15, 2021

CLA Signed

The committers are authorized under a signed CLA.

@mhennoch mhennoch changed the title Capture Web vitals feat: Capture Web vitals Oct 15, 2021
@codecov
Copy link

codecov bot commented Oct 15, 2021

Codecov Report

Merging #702 (4362761) into main (3d4a07f) will decrease coverage by 0.86%.
The diff coverage is n/a.

❗ Current head 4362761 differs from pull request most recent head 79f987f. Consider uploading reports for the commit 79f987f to get more accurate results

@@            Coverage Diff             @@
##             main     #702      +/-   ##
==========================================
- Coverage   96.87%   96.00%   -0.87%     
==========================================
  Files          11       11              
  Lines         640      676      +36     
  Branches      126      133       +7     
==========================================
+ Hits          620      649      +29     
- Misses         20       27       +7     
Impacted Files Coverage Δ
...entelemetry-instrumentation-fastify/src/version.ts
...entelemetry-instrumentation-fastify/src/version.ts
...-instrumentation-web-vitals/src/instrumentation.ts 81.08% <0.00%> (ø)
...elemetry-instrumentation-web-vitals/src/version.ts 100.00% <0.00%> (ø)

@kkruk-sumo
Copy link
Contributor

This is great and I was looking for it for some time. Did you think about adding these metrics into documentLoad spans from the document-load instrumentation. I added FP and FCP there.

Do you know the size of the library made by Google?

Also I was looking into this library long time ago and I remember that a user need to click on a page in order to get these metrics. Is is still true? Because of that I didn't manage to use it in documentLoad spans.

@mhennoch
Copy link
Contributor Author

mhennoch commented Nov 3, 2021

Did you think about adding these metrics into documentLoad spans from the document-load instrumentation. I added FP and FCP there.

I did think about it but I think the general consensus is to keep things separated. I asked a related question in here. Also it doesn't really make sense to add them to the documentLoad spans. CLS can change much later than documentLoad for example.

Do you know the size of the library made by Google?

From their readme: The web-vitals library is a tiny (~1K) modular library.

Also I was looking into this library long time ago and I remember that a user need to click on a page in order to get these metrics. Is is still true? Because of that I didn't manage to use it in documentLoad spans.

By definition you can't have FID without input. Also LCP can change until there is a click etc. on the document and CLS can change until page hidden event so I guess there is no other way.

I'll undraft it soon so we can push this forward.

@mhennoch mhennoch marked this pull request as ready for review November 3, 2021 13:04
@mhennoch mhennoch requested a review from a team November 3, 2021 13:04
@dyladan
Copy link
Member

dyladan commented Nov 3, 2021

I agree metrics would make much more sense for these types of things. Using the metrics API might be difficult because it is changing, but i'm not sure span is a good fit. Might you want to wait until the API stabilizes? It shouldn't be too long.

@bradfrosty
Copy link

For some added context, I made an issue last January to add this feature. The reasoning at the time was due to browser specific polyfills needed to capture the metrics.

I think it would make sense to add these as events on the document load plugin, or via the metrics API as @dyladan said.

@dyladan
Copy link
Member

dyladan commented Nov 10, 2021

Events on the document load make sense to me as well, but that makes analysis harder unless your backend understands what the events are. There are many off-the-shelf tools to process and analyze metrics. Far fewer can generate metrics from span events.

There is a module of the collector which generates metrics from spans (like request counts, request durations, etc). Maybe it could be extended to generate metrics from predefined span events? This would likely require a large spec effort, but would be very powerful.

@bradfrosty
Copy link

Good call re: making the analysis harder since not all backends read the events. Useful for looking at when analyzing the trace, but these metrics are more useful in charts.

Maybe it could be extended to generate metrics from predefined span events?

This would be really really useful and remove some of the redundancy between span events and metric instrumentation.

@mhennoch
Copy link
Contributor Author

I think these should ideally be standalone events that get made into metrics in the backend also. You do want to see these as metrics but also on your session/trace timeline as events. They can also have some extra information eg. which element caused the paint etc. Don't think we can add them as events on docload spans as we would have to keep the span open for some unknown amount of time( first user interaction for lcp and fid or page hidden). Sending data on leaving the page can be unreliable and we might lose that span altogether. This was also discussed briefly in RUM SIG and we decided to make some examples and go to the spec SIG with them to find out what they think.

@github-actions
Copy link
Contributor

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Jan 17, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2022

This PR was closed because it has been stale for 14 days with no activity.

@github-actions github-actions bot closed this Feb 7, 2022
@pkanal pkanal mentioned this pull request Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants