feat(web-vitals): Emit web-vitals as metrics#6118
Conversation
| Ok(()) | ||
| } | ||
|
|
||
| fn produce_web_vital_metrics( |
There was a problem hiding this comment.
I think we should do these things now-a-days in the respective Forward::forward_store implementation of the processing pipeline.
Even if that means we need a separate story for transactions and spans atm.
@jjbayer wdyt?
There was a problem hiding this comment.
Yeah IMO we should put data type specifics into the processing pipeline as much as possible.
| pub(crate) mod store; | ||
| pub(crate) mod utils; |
There was a problem hiding this comment.
These should stay private, if we have to change visibility (especially scoped visibility) it's usually a sign of bad code organization.
There was a problem hiding this comment.
Done--but more generally, even pub(crate)?
There was a problem hiding this comment.
I'd try to avoid it, generally it can either be public or not, with a good module hierarchy it's generally not necessary (as child modules can access private modules of parents).
In this case I am a bit behind it, because I want every processor to be independent and avoid having processor start sharing code that isn't explicitly moved into a shared component, that was a big mistake of the old processing code.
| name: "browser.web_vital.lcp.value", | ||
| unit: MetricUnit::Duration(relay_metrics::DurationUnit::MilliSecond), | ||
| attribute_keys: &[ | ||
| "browser.web_vital.lcp.value", |
There was a problem hiding this comment.
I think these are missing the performance scores?
| name: "browser.web_vital.cls.value", | ||
| unit: MetricUnit::None, | ||
| attribute_keys: &[ | ||
| "browser.web_vital.cls.value", |
There was a problem hiding this comment.
Is attribute_value always also part of attribute_keys? In that case I would rename attribute_keys to something like additional_attributes and omit the value-holding attribute from it.
There was a problem hiding this comment.
On second inspection, why do we add the metric value as an attribute? Isn't that redundant?
There was a problem hiding this comment.
I'll be double-checking with the web-vitals folks about this, but this is as outlined in their rfc.
| name: web_vital.name.to_owned().into(), | ||
| ty: relay_event_schema::protocol::MetricType::Distribution.into(), | ||
| unit: web_vital.unit.into(), | ||
| value: Value::F64(value.as_f64().unwrap_or(0.0)).into(), |
There was a problem hiding this comment.
If the value is not a number, I would rather not emit a metric here at all. This is basically a product question: how do we want to surface unexpected data to the user? IMO silent failure is better than sending 0.0 because the latter skews the average, etc.
There was a problem hiding this comment.
Fair enough, I've moved it to filter it--I'll add some logging in case we see it in the wild
| Ok(()) | ||
| } | ||
|
|
||
| fn produce_web_vital_metrics( |
There was a problem hiding this comment.
Yeah IMO we should put data type specifics into the processing pipeline as much as possible.
| }; | ||
|
|
||
| message.try_accept(|span| { | ||
| self.produce_web_vital_metrics(scoping, received_at, &span)?; |
There was a problem hiding this comment.
Does this call need to be guarded by a global option or feature flag in any way?
There was a problem hiding this comment.
Yes, it should--once I've got the rest of the pinned down.
3 participants
No description provided.