feat(web-vitals): Emit web-vitals as metrics by klochek · Pull Request #6118 · getsentry/relay

klochek · 2026-06-23T02:20:12Z

No description provided.

linear-code · 2026-06-23T02:20:16Z

Dav1dde · 2026-06-23T10:50:01Z

        Ok(())
    }

+    fn produce_web_vital_metrics(


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?

Yeah IMO we should put data type specifics into the processing pipeline as much as possible.

Dav1dde · 2026-06-23T10:50:50Z

+pub(crate) mod store;
+pub(crate) mod utils;


These should stay private, if we have to change visibility (especially scoped visibility) it's usually a sign of bad code organization.

Done--but more generally, even pub(crate)?

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.

Dav1dde · 2026-06-23T10:53:27Z

+        name: "browser.web_vital.lcp.value",
+        unit: MetricUnit::Duration(relay_metrics::DurationUnit::MilliSecond),
+        attribute_keys: &[
+            "browser.web_vital.lcp.value",


I think these are missing the performance scores?

jjbayer · 2026-06-23T11:32:08Z

+        name: "browser.web_vital.cls.value",
+        unit: MetricUnit::None,
+        attribute_keys: &[
+            "browser.web_vital.cls.value",


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.

On second inspection, why do we add the metric value as an attribute? Isn't that redundant?

I'll be double-checking with the web-vitals folks about this, but this is as outlined in their rfc.

jjbayer · 2026-06-23T11:35:31Z

+                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(),


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.

Fair enough, I've moved it to filter it--I'll add some logging in case we see it in the wild

jjbayer · 2026-06-23T11:40:25Z

        Ok(())
    }

+    fn produce_web_vital_metrics(


Yeah IMO we should put data type specifics into the processing pipeline as much as possible.

jjbayer · 2026-06-23T11:40:50Z

        };

        message.try_accept(|span| {
+            self.produce_web_vital_metrics(scoping, received_at, &span)?;


Does this call need to be guarded by a global option or feature flag in any way?

Yes, it should--once I've got the rest of the pinned down.

cursor

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.}

cursor · 2026-07-06T20:09:07Z

+            s.send_to_store(span.wrap(item));
+        }
+    }
+}


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)

relay-server/src/processing/spans/mod.rs#L276-L278

relay-server/src/processing/legacy_spans/mod.rs#L204-L206

^{Reviewed by Cursor Bugbot for commit 2c157e7. Configure here.}

This we'll have to check with Ben, but seems okay?

Dav1dde · 2026-07-07T07:09:16Z

+    span: &Managed<Box<crate::services::store::StoreSpanV2>>,
+    metrics: Vec<TraceMetric>,
+) {
+    use crate::processing::trace_metrics;


This doesn't seem necessary, the code is already in this module

Dav1dde · 2026-07-07T07:09:49Z


+#[cfg(feature = "processing")]
+/// Produce the supplied webvital trace metrics to kafka.
+pub fn produce_webvitals_metrics(


Maybe can move it into mod store and just pub use here?

Dav1dde · 2026-07-07T07:10:29Z

+            s.send_to_store(span.wrap(item));
+        }
+    }
+}


This we'll have to check with Ben, but seems okay?

Dav1dde · 2026-07-07T07:12:16Z

+/// Extract any web vitals metrics for the supplied v2 span.  Bad or missing metrics will be
+/// silently dropped.
+pub fn extract_web_vital_metrics(span: &SpanV2) -> Option<Vec<TraceMetric>> {
+    let Some(attrs) = &span.attributes.0 else {


let attrs = span.attributes.0.as_ref()? should do

Dav1dde · 2026-07-07T07:15:45Z

+    let op_name = attrs.get_value("sentry.op")?;
+
+    if !WEB_VITAL_SPAN_NAMES.contains(&op_name.as_str().unwrap_or_default()) {


Suggested change

let op_name = attrs.get_value("sentry.op")?;

if !WEB_VITAL_SPAN_NAMES.contains(&op_name.as_str().unwrap_or_default()) {

let op_name = attrs.get_value("sentry.op")?.as_str()?;

if !WEB_VITAL_SPAN_NAMES.contains(&op_name) {

return None;

}

Dav1dde · 2026-07-07T07:17:37Z

+        }
+
+        // This is for attribution: we'll be able to tell on a metric if it came from relay.
+        attributes.insert("sentry.metric.source", "span");


This (sentry.metric.source) needs to be defined sentry-conventions if it isn't already.

Dav1dde · 2026-07-07T07:20:09Z

+        // CLS webvitals are a little weird, in that they can have an arbitrary number of
+        // "source" attributes (with a .N postfix, 0-based, monotonically increasing), so we
+        // exhaustively look for them here.
+        if web_vital.attribute_value == "browser.web_vital.cls.value" {


Suggested change

if web_vital.attribute_value == "browser.web_vital.cls.value" {

if web_vital.attribute_value == BROWSER__WEB_VITAL__CLS__VALUE {

Dav1dde · 2026-07-07T07:20:32Z

+        // exhaustively look for them here.
+        if web_vital.attribute_value == "browser.web_vital.cls.value" {
+            for i in 0..MAX_CLS_SOURCES {
+                let attr_key = format!("browser.web_vital.cls.source.{i}");


Suggested change

let attr_key = format!("browser.web_vital.cls.source.{i}");

let attr_key = browser__web_vital__cls__source__key(i);

(Needs a small change first that the function accepts impl Display).

Dav1dde · 2026-07-07T07:21:13Z

+                if let Some(v) = attrs.get_attribute(&attr_key) {
+                    attributes.insert(attr_key, v.value.clone());
+                } else {
+                    break;
+                }


Just a small style nit, in Relay we often use match statements for small if-else constructs like this

Dav1dde · 2026-07-07T07:22:04Z

+    "sentry.pageload.span_id",
+    "sentry.origin",
+    "sentry.transaction",
+    "user_agent.original",
+    "sentry.release",
+    "sentry.environment",
+    "sentry.sdk.name",
+    "sentry.sdk.version",
+    "sentry.platform",


These also all need to come from conventions

loewenheim · 2026-07-07T07:51:51Z

+
+
+# Test standalone spans (just lcp)
+def test_v1_standalone_span(mini_sentry, relay_with_processing, items_consumer):


You could parametrize this test with mode (like some tests in test_spans_standalone) to see whether the legacy and experimental standalone span pipeline work alike.

loewenheim

Looks good, the main concern is around using attribute constants from relay-conventions (and possibly defining things you need in sentry-conventions) as @Dav1dde mentioned.

logaretm

Just caught a small issue with the CLS source starting index, otherwise LGTM.

logaretm · 2026-07-07T19:16:23Z

+                if let Some(v) = attrs.get_attribute(&attr_key) {
+                    attributes.insert(attr_key, v.value.clone());
+                } else {
+                    break;


The SDK is sending the CLS starting from 1 rather than 0 so this would actually skip all incoming sources from an actual SDK because there is no 0 index.

While this looked weird to me at first, it is according to spec tho. So we should start the loop at 1 instead. Maybe let's throw in a test as well for this.

logaretm · 2026-07-07T19:32:15Z

+        attributes.insert("sentry.metric.source", "span");
+
+        let trace_metric = TraceMetric {
+            timestamp: span.start_timestamp.clone(),


I thought about usingend_timestamp instead but both aren't really accurate so it doesn't matter, both are equally bad at approximating the actual metric timestamp. Just wanted to note this down.

Dav1dde reviewed Jun 23, 2026

View reviewed changes

jjbayer reviewed Jun 23, 2026

View reviewed changes

klochek added 3 commits July 6, 2026 08:59

feat(web-vitals): Emit web-vitals as metrics

ce9908d

various fixes

50c25d3

various fixes

2c157e7

klochek force-pushed the christopherklochek/ingest-962-implement-double-writes-for-web-vitals branch from 4eb2c66 to 2c157e7 Compare July 6, 2026 19:15

klochek marked this pull request as ready for review July 6, 2026 20:05

klochek requested a review from a team as a code owner July 6, 2026 20:05

klochek requested a review from logaretm July 6, 2026 20:05

cursor Bot reviewed Jul 6, 2026

View reviewed changes

Dav1dde reviewed Jul 7, 2026

View reviewed changes

Dav1dde mentioned this pull request Jul 7, 2026

ref(conventions): Make key fn accept std::fmt::Display #6185

Merged

loewenheim reviewed Jul 7, 2026

View reviewed changes

logaretm reviewed Jul 7, 2026

View reviewed changes

		let op_name = attrs.get_value("sentry.op")?;

		if !WEB_VITAL_SPAN_NAMES.contains(&op_name.as_str().unwrap_or_default()) {

-    let op_name = attrs.get_value("sentry.op")?;
-    if !WEB_VITAL_SPAN_NAMES.contains(&op_name.as_str().unwrap_or_default()) {
+    let op_name = attrs.get_value("sentry.op")?.as_str()?;
+    if !WEB_VITAL_SPAN_NAMES.contains(&op_name) {
+        return None;
+    }

	if web_vital.attribute_value == "browser.web_vital.cls.value" {
	if web_vital.attribute_value == BROWSER__WEB_VITAL__CLS__VALUE {

	let attr_key = format!("browser.web_vital.cls.source.{i}");
	let attr_key = browser__web_vital__cls__source__key(i);



		# Test standalone spans (just lcp)
		def test_v1_standalone_span(mini_sentry, relay_with_processing, items_consumer):

Uh oh!

Conversation

klochek commented Jun 23, 2026

Uh oh!

linear-code Bot commented Jun 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot Jul 6, 2026

Choose a reason for hiding this comment

Skips trace metrics feature flag

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loewenheim left a comment

Choose a reason for hiding this comment

Uh oh!

logaretm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers