feat: adding ability to track all changes for web vitals #981
Conversation
packages/web-sdk/src/instrumentations/webVitals/webVitalsWithAttribution.ts
Outdated
Show resolved
Hide resolved
packages/core/src/config/types.ts
Outdated
| * Report all changes for web vitals (default: false) | ||
| * | ||
| * In most cases, you only want the callback function to be called when the metric is ready to be reported. | ||
| * However, it is possible to report every change (e.g. each larger layout shift as it happens) |
There was a problem hiding this comment.
Let's add a comment that reportAllChanges isn't recommended to use in production and more for debugging purposes. See web-vitals docs
There was a problem hiding this comment.
Suggestion:
since we now have two props to configure web vitals we can add a config object bundling web-vitals updates. We can still keep the trackWebVitalsAttribution for quick enabling/disabling.
Then we would follow the shape of other config object which have multiple params.
Exmple:
webVitals {
enabled: boolean
reportAllWebVitalChanges: boolean
}There was a problem hiding this comment.
I think I understood what you wanted here - I did not want to create a breaking change by adding trackWebVitalsAttribution into this object, but wasnt too sure what else to add in here?
There was a problem hiding this comment.
Yes exactly.
Nit:
for consistency we can add an enabled attribute to the webVitalsobject as well
There was a problem hiding this comment.
One last thing:
Let's add the "Instrumentation" prefix to the config object for consistency with the other instrumentation options like the consoleInstrumentation .
And remove the webVitals part from the nested property bc it is already namespaced by the parent object.
webVitalsInstrumentation: {
reportAllChanges: true
}| @@ -15,8 +15,8 @@ export class WebVitalsInstrumentation extends BaseInstrumentation { | |||
|
|
|||
| private intializeWebVitalsInstrumentation() { | |||
| if (this.config.trackWebVitalsAttribution) { | |||
| return new WebVitalsWithAttribution(this.api.pushMeasurement); | |||
| return new WebVitalsWithAttribution(this.api.pushMeasurement, this.config.reportAllWebVitalChanges); | |||
There was a problem hiding this comment.
You need to update makeCoreConfig so the new property is added to the core config
Please add a small test to ensure that the trackWebVitalsAttribution is available in the config
|
Please also update the web-vitals cloud docs. |
|
please update the changelog |
- Refactor web vitals configuration to use a nested `webVitals` object - Add support for `reportAllWebVitalChanges` within the new configuration structure - Update instrumentation classes to use the new configuration format - Improve type safety and configuration flexibility for web vitals reporting
| it('enables web vitals feature when trackWebVitalsAttribution is true', () => { | ||
| const browserConfig = { | ||
| url: 'http://example.com/my-collector', | ||
| app: {}, | ||
| trackWebVitalsAttribution: true, | ||
| webVitals: { | ||
| reportAllWebVitalChanges: true, | ||
| }, | ||
| }; | ||
| const config = makeCoreConfig(browserConfig); | ||
|
|
||
| expect(config).toBeTruthy(); | ||
| expect(config?.trackWebVitalsAttribution).toBe(true); | ||
| expect(config?.webVitals?.reportAllWebVitalChanges).toBe(true); | ||
| }); | ||
|
|
There was a problem hiding this comment.
@codecapitano is this what you had in mind?
There was a problem hiding this comment.
Yes exactly.
Only some slight adjustments for consistency with the other options.
Let's add the "Instrumentation" prefix to the config object for consistency with the other instrumentation options like the consoleInstrumentation .
And remove the webVitals part from the nested property bc it is already namespaced by the parent object.
webVitalsInstrumentation: {
reportAllChanges: true
}
|
made changes to cloud docs here |
Why
solves #947
provides the option to enable the
trackAllChangesfeature in the web-vitals packageWhat
add
reportAllWebVitalChangesto the config object to enable that feature in the web-vitals package