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.
klochek
force-pushed
the
christopherklochek/ingest-962-implement-double-writes-for-web-vitals
branch
from
4eb2c66 to
2c157e7
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2c157e7. Configure here.
| s.send_to_store(span.wrap(item)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Skips trace metrics feature flag
Medium Severity
Derived web-vital trace metrics are written straight to the store via produce_webvitals_metrics, so they never pass the trace-metrics processor’s Feature::TraceMetricsIngestion gate or its inbound filtering. Projects with trace metric ingestion disabled can still get these Kafka trace items.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 2c157e7. Configure here.
3 participants
No description provided.