feat(honeycomb-opentelemetry-web): Add data attributtes for LCP. #309
Conversation
wolfgangcodes
force-pushed
the
wg/lcp-data
branch
from
6b4a8de to
56a9c13
Compare
| @@ -280,41 +287,37 @@ export class WebVitalsInstrumentation extends InstrumentationAbstract { | |||
| private _setupWebVitalsCallbacks() { | |||
| if (this.vitalsToTrack.includes('CLS')) { | |||
| onCLS((vital) => { | |||
| this.onReportCLS(vital, this.clsOpts?.applyCustomAttributes); | |||
| this.onReportCLS(vital, this.clsOpts); | |||
There was a problem hiding this comment.
Update all the this.onREportXYZ to take opts as the second argument, to minimize changes for additional options.
| /** | ||
| * Will send the values of these data attributes if they appear on an LCP event | ||
| */ | ||
| dataAttributes?: string[]; |
There was a problem hiding this comment.
This pattern could be useful if we wanted to add in the ability to collect data- from nodes where attributes aren't available and we need to do a dom lookup. Maybe this is a way to let users opt-into something that might impact "performance"?
There was a problem hiding this comment.
Hmmmmm that's in interesting point, I agree that its nice to have this array if you only want to look up a certain subset of data- attributes. I was originally thinking that if any data- attrs are provided that we would just automatically send them all as attributes and then the array can filter them down if provided just to save the user a setup step but if this is more future proof than I'm happy with it!
There was a problem hiding this comment.
Ooo. default to sending all and filter down? I like that better for this instance.
| @@ -481,6 +482,25 @@ export class WebVitalsInstrumentation extends InstrumentationAbstract { | |||
| [`${attrPrefix}.resource_load_time`]: resourceLoadDuration, | |||
| }); | |||
|
|
|||
| lcpEntry?.element?.getAttributeNames().forEach((attrName) => { | |||
| // Skip non data-* attributes | |||
| if (!attrName.startsWith('data-')) { | |||
There was a problem hiding this comment.
I think it would be safer / more correct to use the dataset API in JavaScript rather than iterating over all the attributes if it's available!
There was a problem hiding this comment.
OH. Heck yeah!
README.md
Outdated
| @@ -98,6 +98,7 @@ Pass these options to the HoneycombWebSDK: | |||
| | enabled | optional | boolean | `true` | Where or not to enable this auto instrumentation. | | |||
| | lcp| optional| VitalOpts | `undefined` | Pass-through config options for web-vitals. See [ReportOpts](https://github.com/GoogleChrome/web-vitals?tab=readme-ov-file#reportopts). | |||
| | lcp.applyCustomAttributes| optional| function | `undefined` | A function for adding custom attributes to core web vitals spans. | |||
| | lcp.dataAttributes| optional| `string[]` | `undefined` | an array of data-* attribute names to filter. By default it will send all `data-*` attribute-value pairs with the key `lcp.element.data-someAttr`, `[]` will send none. | |||
There was a problem hiding this comment.
I don't think this is how the values end up being added as attrs, according to the code each one is an attribute by itself with .data. prefix, can we change that here?
There was a problem hiding this comment.
💯 I wanted to make sure we were 👍 with c3f6422 before updating all the docs. 😅
Which problem is this PR solving?
Adds
data-attributes to LCP web vitals and a configuration to specify which data attributes to collect.Closes #95
Short description of the changes
How to verify that this has the expected result
Add a
data-to the LCP element and see it's value as an attribute span underelement.data-ATTRIBUTE_NAME