feat: add attributions for core web vitals: LCP, CLS, and FID #432
Conversation
williazz
force-pushed
the
CWVAttributions
branch
from
d5496ff
to
7fd03d2
Compare
williazz
force-pushed
the
CWVAttributions
branch
4 times, most recently
from
40cf75b
to
2d52a51
Compare
| | FirstInputDelayEvent | ||
| | CumulativeLayoutShiftEvent = { | ||
| protected onload(): void { | ||
| onLCP((metric) => this.handleLCP(metric)); |
There was a problem hiding this comment.
Requires arrow function to preserve reference of keyword this so that context can still be found
| onCLS((metric) => this.handleCLS(metric)); | ||
| } | ||
|
|
||
| handleLCP(metric: LCPMetricWithAttribution | Metric) { |
There was a problem hiding this comment.
We will never expect any of these handlers to receive just Metric because we are using the attribution build of web-vitals. However, they left Metric in as an argument type, so we should still double check that attributions are attached
There was a problem hiding this comment.
question: Why do we need to double check that attributions are attached?
Looking at the type definition, it should always be LCPMetricWithAttribution, although TypeScript does think it could be Metric at this point. If we know it will always be LCPMetricWithAttribution then we can simplify this logic.
There was a problem hiding this comment.
That was my initial thought as well, but I was spooked into double checking because because TypeScript was complaining, like you said. I'm happy to make that change though
There was a problem hiding this comment.
Removed double check
|
issue: I think we should create browser integ tests for web vital attribution, especially considering different browsers support different features related to the collection of web vitals. |
| onCLS, | ||
| onFID, | ||
| onLCP | ||
| } from 'web-vitals/attribution'; |
There was a problem hiding this comment.
question: By how much does using the web-vitals attribution build (+ adding our own code to record attribution) increase the total size of the compressed aws-rum-web package?
At some point we will also want to create different builds, of different sizes, for aws-rum-web.
There was a problem hiding this comment.
I did not do an exact comparison. There was no documentation on size difference in the web vitals package for different builds, so I just did my best to keep my changes as small as possible.
I can check though.
There was a problem hiding this comment.
I used the bundle analyzer plugin to do a size comparison. I compared parsed sizes of the cwr.js, WebVitalsPlugin, and web-vitals package.
release/2.0.x
- total size = 212.76kb
- web vitals plugin = 1.24kb
- web-vitals package = 6.52kb
this pr
- total size = 215.82kb
- web vitals plugin = 9.56kb
- web-vitals package = 2.02kb
diff
- total size = + ~3kb
- web vitals plugins = + ~8kb
- web vitals package = -4kb?
Not sure why the attributions build is smaller than the normal build.
| onCLS((metric) => this.handleCLS(metric)); | ||
| } | ||
|
|
||
| handleLCP(metric: LCPMetricWithAttribution | Metric) { |
There was a problem hiding this comment.
question: Why do we need to double check that attributions are attached?
Looking at the type definition, it should always be LCPMetricWithAttribution, although TypeScript does think it could be Metric at this point. If we know it will always be LCPMetricWithAttribution then we can simplify this logic.
Currently looking into this. If the scope is big enough, it may have to be a separate PR @qhanam Update: Added integ test for attribution for LCP and CLS on chrome. FID is supported on Chrome, Firefox (and Safari + IE via polyfill). However, FID cannot be tested on testcafe because the web vitals package knows to ignore manually triggered user input. |
| @@ -52,7 +64,7 @@ describe('WebVitalsPlugin tests', () => { | |||
| record.mockClear(); | |||
| }); | |||
|
|
|||
| test('When web vitals are present then events are recorded', async () => { | |||
| test('When web vitals are present then LCP is recorded with attributions', async () => { | |||
There was a problem hiding this comment.
question: Do we need unit tests to cover case where attributions could not be collected?
There was a problem hiding this comment.
Yes, we need "when [web vital] does not have attribution then event is recorded without attribution". Thanks
There was a problem hiding this comment.
Correction: this test is not needed. Attributions object will always exist. Some properties are optional, such as LCP.element and all of the CLS fields. These fields will be defined as {element: undefined...} and dropped entirely during JSON.stringify().
williazz
force-pushed
the
CWVAttributions
branch
from
51e46d4
to
133521c
Compare
williazz
force-pushed
the
CWVAttributions
branch
from
fbeb308
to
26a50f4
Compare
Revision 1
Revision 2
Will be included in another PR
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.