From 0bb20bebc85c2ce2bafdab161c02b9c904dd8272 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Tue, 26 Nov 2024 17:28:31 -0500 Subject: [PATCH 1/8] fix(spans): Normalize INP spans further INP spans sets some top level span attributes inside span.data so make sure to pull them out to the top level. This has implications in the snuba consumer so that we do not redundant keys with slightly different names in span.data. --- .../src/services/processor/span/processing.rs | 114 +++++++++++++++++- 1 file changed, 111 insertions(+), 3 deletions(-) diff --git a/relay-server/src/services/processor/span/processing.rs b/relay-server/src/services/processor/span/processing.rs index ba41e57ce00..2083c3a2f76 100644 --- a/relay-server/src/services/processor/span/processing.rs +++ b/relay-server/src/services/processor/span/processing.rs @@ -18,12 +18,12 @@ use relay_event_normalization::{ }; use relay_event_schema::processor::{process_value, ProcessingAction, ProcessingState}; use relay_event_schema::protocol::{ - BrowserContext, IpAddr, Measurement, Measurements, Span, SpanData, + BrowserContext, EventId, IpAddr, Measurement, Measurements, Span, SpanData, }; use relay_log::protocol::{Attachment, AttachmentType}; use relay_metrics::{FractionUnit, MetricNamespace, MetricUnit, UnixTimestamp}; use relay_pii::PiiProcessor; -use relay_protocol::{Annotated, Empty}; +use relay_protocol::{Annotated, Empty, Value}; use relay_quotas::DataCategory; use relay_spans::otel_trace::Span as OtelSpan; use thiserror::Error; @@ -590,6 +590,28 @@ fn normalize( populate_ua_fields(span, user_agent.as_deref(), client_hints.as_deref()); + // INP spans sets some top level span attributes inside span.data so make sure to pull + // them out to the top level before further processing. + if let Annotated(Some(ref mut data), _) = span.data { + if let Some(exclusive_time) = match data.other.get("sentry.exclusive_time") { + Some(Annotated(Some(Value::I64(exclusive_time)), _)) => Some(*exclusive_time as f64), + Some(Annotated(Some(Value::U64(exclusive_time)), _)) => Some(*exclusive_time as f64), + Some(Annotated(Some(Value::F64(exclusive_time)), _)) => Some(*exclusive_time), + _ => None, + } { + data.other.remove("sentry.exclusive_time"); + span.exclusive_time = exclusive_time.into(); + } + + if let Some(profile_id) = match data.other.get("profile_id") { + Some(Annotated(Some(Value::String(profile_id)), _)) => profile_id.parse().map(|profile_id| EventId(profile_id)).ok(), + _ => None, + } { + data.other.remove("profile_id"); + span.profile_id = profile_id.into(); + } + } + if let Annotated(Some(ref mut measurement_values), ref mut meta) = span.measurements { normalize_measurements( measurement_values, @@ -771,7 +793,7 @@ mod tests { use once_cell::sync::Lazy; use relay_base_schema::project::ProjectId; use relay_event_schema::protocol::{ - Context, ContextInner, SpanId, Timestamp, TraceContext, TraceId, + Context, ContextInner, EventId, SpanData, SpanId, Timestamp, TraceContext, TraceId, }; use relay_event_schema::protocol::{Contexts, Event, Span}; use relay_protocol::get_value; @@ -1266,4 +1288,90 @@ mod tests { ); assert_eq!(get_value!(span.data.user_geo_city!), "Boxford"); } + + #[test] + fn exclusive_time_inside_span_data_i64() { + let mut span = Annotated::from_json( + r#"{ + "start_timestamp": 0, + "timestamp": 1, + "trace_id": "922dda2462ea4ac2b6a4b339bee90863", + "span_id": "922dda2462ea4ac2", + "data": { + "sentry.exclusive_time": 123 + } + }"#, + ) + .unwrap(); + + normalize(&mut span, normalize_config()).unwrap(); + + assert_eq!(*get_value!(span.exclusive_time!), 123.0); + } + + #[test] + fn exclusive_time_inside_span_data_f64() { + let mut span = Annotated::from_json( + r#"{ + "start_timestamp": 0, + "timestamp": 1, + "trace_id": "922dda2462ea4ac2b6a4b339bee90863", + "span_id": "922dda2462ea4ac2", + "data": { + "sentry.exclusive_time": 123.0 + } + }"#, + ) + .unwrap(); + + normalize(&mut span, normalize_config()).unwrap(); + + assert_eq!(*get_value!(span.exclusive_time!), 123.0); + } + + #[test] + fn normalize_inp_spans() { + /* + */ + let mut span = Annotated::from_json( + r#"{ + "data": { + "sentry.origin": "auto.http.browser.inp", + "sentry.op": "ui.interaction.click", + "release": "frontend@0735d75a05afe8d34bb0950f17c332eb32988862", + "environment": "prod", + "profile_id": "480ffcc911174ade9106b40ffbd822f5", + "replay_id": "f39c5eb6539f4e49b9ad2b95226bc120", + "transaction": "/replays", + "user_agent.original": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/131.0.0.0 Safari/537.36", + "sentry.exclusive_time": 128.0 + }, + "description": "div.app-3diuwe.e88zkai6 > span.app-ksj0rb.e88zkai4", + "op": "ui.interaction.click", + "parent_span_id": "88457c3c28f4c0c6", + "span_id": "be0e95480798a2a9", + "start_timestamp": 1732635523.5048, + "timestamp": 1732635523.6328, + "trace_id": "bdaf4823d1c74068af238879e31e1be9", + "origin": "auto.http.browser.inp", + "exclusive_time": 128, + "measurements": { + "inp": { + "value": 128, + "unit": "millisecond" + } + }, + "segment_id": "88457c3c28f4c0c6" + }"#, + ) + .unwrap(); + + normalize(&mut span, normalize_config()).unwrap(); + + let data = get_value!(span.data!); + assert!(!data.other.contains_key("sentry.exclusive_time")); + assert!(!data.other.contains_key("profile_id")); + + assert_eq!(get_value!(span.profile_id!), &EventId("480ffcc911174ade9106b40ffbd822f5".parse().unwrap())); + } } From 954d1a3f8312a8bb78ff902a42ab36aa2f96b026 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Tue, 26 Nov 2024 17:49:31 -0500 Subject: [PATCH 2/8] lint --- .../src/services/processor/span/processing.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/relay-server/src/services/processor/span/processing.rs b/relay-server/src/services/processor/span/processing.rs index 2083c3a2f76..0c63fbe31f5 100644 --- a/relay-server/src/services/processor/span/processing.rs +++ b/relay-server/src/services/processor/span/processing.rs @@ -604,7 +604,10 @@ fn normalize( } if let Some(profile_id) = match data.other.get("profile_id") { - Some(Annotated(Some(Value::String(profile_id)), _)) => profile_id.parse().map(|profile_id| EventId(profile_id)).ok(), + Some(Annotated(Some(Value::String(profile_id)), _)) => profile_id + .parse() + .map(|profile_id| EventId(profile_id)) + .ok(), _ => None, } { data.other.remove("profile_id"); @@ -1331,8 +1334,6 @@ mod tests { #[test] fn normalize_inp_spans() { - /* - */ let mut span = Annotated::from_json( r#"{ "data": { @@ -1372,6 +1373,9 @@ mod tests { assert!(!data.other.contains_key("sentry.exclusive_time")); assert!(!data.other.contains_key("profile_id")); - assert_eq!(get_value!(span.profile_id!), &EventId("480ffcc911174ade9106b40ffbd822f5".parse().unwrap())); + assert_eq!( + get_value!(span.profile_id!), + &EventId("480ffcc911174ade9106b40ffbd822f5".parse().unwrap()) + ); } } From 9424c89c3e775a03e0ba8a79178fbdb29ad1ac6e Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Wed, 27 Nov 2024 10:54:30 -0500 Subject: [PATCH 3/8] address PR comments --- relay-event-schema/src/protocol/span.rs | 8 +++ .../src/services/processor/span/processing.rs | 54 +++++++++---------- 2 files changed, 35 insertions(+), 27 deletions(-) diff --git a/relay-event-schema/src/protocol/span.rs b/relay-event-schema/src/protocol/span.rs index 140f53300de..47eb7d3803b 100644 --- a/relay-event-schema/src/protocol/span.rs +++ b/relay-event-schema/src/protocol/span.rs @@ -407,6 +407,14 @@ pub struct SpanData { #[metastructure(field = "user.roles")] pub user_roles: Annotated>, + /// Exclusive Time + #[metastructure(field = "sentry.exclusive_time")] + pub exclusive_time: Annotated, + + /// Profile ID + #[metastructure(field = "profile_id")] + pub profile_id: Annotated, + /// Replay ID #[metastructure(field = "sentry.replay.id", legacy_alias = "replay_id")] pub replay_id: Annotated, diff --git a/relay-server/src/services/processor/span/processing.rs b/relay-server/src/services/processor/span/processing.rs index 0c63fbe31f5..7b40d5d3d70 100644 --- a/relay-server/src/services/processor/span/processing.rs +++ b/relay-server/src/services/processor/span/processing.rs @@ -590,30 +590,7 @@ fn normalize( populate_ua_fields(span, user_agent.as_deref(), client_hints.as_deref()); - // INP spans sets some top level span attributes inside span.data so make sure to pull - // them out to the top level before further processing. - if let Annotated(Some(ref mut data), _) = span.data { - if let Some(exclusive_time) = match data.other.get("sentry.exclusive_time") { - Some(Annotated(Some(Value::I64(exclusive_time)), _)) => Some(*exclusive_time as f64), - Some(Annotated(Some(Value::U64(exclusive_time)), _)) => Some(*exclusive_time as f64), - Some(Annotated(Some(Value::F64(exclusive_time)), _)) => Some(*exclusive_time), - _ => None, - } { - data.other.remove("sentry.exclusive_time"); - span.exclusive_time = exclusive_time.into(); - } - - if let Some(profile_id) = match data.other.get("profile_id") { - Some(Annotated(Some(Value::String(profile_id)), _)) => profile_id - .parse() - .map(|profile_id| EventId(profile_id)) - .ok(), - _ => None, - } { - data.other.remove("profile_id"); - span.profile_id = profile_id.into(); - } - } + promote_span_data_fields(span); if let Annotated(Some(ref mut measurement_values), ref mut meta) = span.measurements { normalize_measurements( @@ -696,6 +673,31 @@ fn populate_ua_fields( } } +/// Promotes some fields from span.data as there are predefined places for certain fields. +fn promote_span_data_fields(span: &mut Span) { + // INP spans sets some top level span attributes inside span.data so make sure to pull + // them out to the top level before further processing. + if let Some(data) = span.data.value_mut() { + if let Some(exclusive_time) = match data.exclusive_time.value() { + Some(Value::I64(exclusive_time)) => Some(*exclusive_time as f64), + Some(Value::U64(exclusive_time)) => Some(*exclusive_time as f64), + Some(Value::F64(exclusive_time)) => Some(*exclusive_time), + _ => None, + } { + span.exclusive_time = exclusive_time.into(); + data.exclusive_time.set_value(None); + } + + if let Some(profile_id) = match data.profile_id.value() { + Some(Value::String(profile_id)) => profile_id.parse().map(EventId).ok(), + _ => None, + } { + span.profile_id = profile_id.into(); + data.profile_id.set_value(None); + } + } +} + fn scrub( annotated_span: &mut Annotated, project_config: &ProjectConfig, @@ -796,7 +798,7 @@ mod tests { use once_cell::sync::Lazy; use relay_base_schema::project::ProjectId; use relay_event_schema::protocol::{ - Context, ContextInner, EventId, SpanData, SpanId, Timestamp, TraceContext, TraceId, + Context, ContextInner, EventId, SpanId, Timestamp, TraceContext, TraceId, }; use relay_event_schema::protocol::{Contexts, Event, Span}; use relay_protocol::get_value; @@ -1370,8 +1372,6 @@ mod tests { normalize(&mut span, normalize_config()).unwrap(); let data = get_value!(span.data!); - assert!(!data.other.contains_key("sentry.exclusive_time")); - assert!(!data.other.contains_key("profile_id")); assert_eq!( get_value!(span.profile_id!), From 365648bce398ab5b876342a2203e98672c2c6e20 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Wed, 27 Nov 2024 10:55:26 -0500 Subject: [PATCH 4/8] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 85e58a4c886..7d7a2a21e87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ - Remove metrics summaries. ([#4278](https://github.com/getsentry/relay/pull/4278), [#4279](https://github.com/getsentry/relay/pull/4279)) - Use async `redis` for `projectconfig`. ([#4284](https://github.com/getsentry/relay/pull/4284)) +- Promote some `span.data` fields to the top level. ([#4298](https://github.com/getsentry/relay/pull/4298)) ## 24.11.0 From 03e8535c1e7cb0d82008c760805e607d6eefe9fd Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Wed, 27 Nov 2024 11:00:46 -0500 Subject: [PATCH 5/8] assert removed from span.data --- relay-server/src/services/processor/span/processing.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/relay-server/src/services/processor/span/processing.rs b/relay-server/src/services/processor/span/processing.rs index 7b40d5d3d70..5305f1b601c 100644 --- a/relay-server/src/services/processor/span/processing.rs +++ b/relay-server/src/services/processor/span/processing.rs @@ -1311,6 +1311,8 @@ mod tests { normalize(&mut span, normalize_config()).unwrap(); + let data = get_value!(span.data!); + assert_eq!(data.exclusive_time, Annotated::empty()); assert_eq!(*get_value!(span.exclusive_time!), 123.0); } @@ -1331,6 +1333,8 @@ mod tests { normalize(&mut span, normalize_config()).unwrap(); + let data = get_value!(span.data!); + assert_eq!(data.exclusive_time, Annotated::empty()); assert_eq!(*get_value!(span.exclusive_time!), 123.0); } @@ -1372,7 +1376,7 @@ mod tests { normalize(&mut span, normalize_config()).unwrap(); let data = get_value!(span.data!); - + assert_eq!(data.profile_id, Annotated::empty()); assert_eq!( get_value!(span.profile_id!), &EventId("480ffcc911174ade9106b40ffbd822f5".parse().unwrap()) From bfa157cea5c1a9d0828a4dd7d8af3394634e4b48 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Wed, 27 Nov 2024 11:01:41 -0500 Subject: [PATCH 6/8] assert exclusive time --- relay-server/src/services/processor/span/processing.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/relay-server/src/services/processor/span/processing.rs b/relay-server/src/services/processor/span/processing.rs index 5305f1b601c..c2eaadb612b 100644 --- a/relay-server/src/services/processor/span/processing.rs +++ b/relay-server/src/services/processor/span/processing.rs @@ -1376,6 +1376,10 @@ mod tests { normalize(&mut span, normalize_config()).unwrap(); let data = get_value!(span.data!); + + assert_eq!(data.exclusive_time, Annotated::empty()); + assert_eq!(*get_value!(span.exclusive_time!), 128.0); + assert_eq!(data.profile_id, Annotated::empty()); assert_eq!( get_value!(span.profile_id!), From d92911c33a803df81fd264120c7cfc3ce231d14f Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Wed, 27 Nov 2024 11:12:26 -0500 Subject: [PATCH 7/8] update insta snapshopts --- relay-event-schema/src/protocol/span.rs | 2 ++ relay-event-schema/src/protocol/span/convert.rs | 2 ++ ...ion__event__tests__extract_span_metrics_mobile.snap | 10 ++++++++++ 3 files changed, 14 insertions(+) diff --git a/relay-event-schema/src/protocol/span.rs b/relay-event-schema/src/protocol/span.rs index 47eb7d3803b..13ee234a4ff 100644 --- a/relay-event-schema/src/protocol/span.rs +++ b/relay-event-schema/src/protocol/span.rs @@ -876,6 +876,8 @@ mod tests { user_id: ~, user_name: ~, user_roles: ~, + exclusive_time: ~, + profile_id: ~, replay_id: ~, sdk_name: ~, sdk_version: ~, diff --git a/relay-event-schema/src/protocol/span/convert.rs b/relay-event-schema/src/protocol/span/convert.rs index 0c6c0f06392..bb1799bd43a 100644 --- a/relay-event-schema/src/protocol/span/convert.rs +++ b/relay-event-schema/src/protocol/span/convert.rs @@ -188,6 +188,8 @@ mod tests { user_id: ~, user_name: ~, user_roles: ~, + exclusive_time: ~, + profile_id: ~, replay_id: ~, sdk_name: "sentry.php", sdk_version: "1.2.3", diff --git a/relay-server/src/metrics_extraction/snapshots/relay_server__metrics_extraction__event__tests__extract_span_metrics_mobile.snap b/relay-server/src/metrics_extraction/snapshots/relay_server__metrics_extraction__event__tests__extract_span_metrics_mobile.snap index f471f6df99b..eb67ffe58c4 100644 --- a/relay-server/src/metrics_extraction/snapshots/relay_server__metrics_extraction__event__tests__extract_span_metrics_mobile.snap +++ b/relay-server/src/metrics_extraction/snapshots/relay_server__metrics_extraction__event__tests__extract_span_metrics_mobile.snap @@ -123,6 +123,8 @@ expression: "(&event.value().unwrap().spans, metrics.project_metrics)" user_id: ~, user_name: ~, user_roles: ~, + exclusive_time: ~, + profile_id: ~, replay_id: ~, sdk_name: ~, sdk_version: ~, @@ -478,6 +480,8 @@ expression: "(&event.value().unwrap().spans, metrics.project_metrics)" user_id: ~, user_name: ~, user_roles: ~, + exclusive_time: ~, + profile_id: ~, replay_id: ~, sdk_name: ~, sdk_version: ~, @@ -599,6 +603,8 @@ expression: "(&event.value().unwrap().spans, metrics.project_metrics)" user_id: ~, user_name: ~, user_roles: ~, + exclusive_time: ~, + profile_id: ~, replay_id: ~, sdk_name: ~, sdk_version: ~, @@ -771,6 +777,8 @@ expression: "(&event.value().unwrap().spans, metrics.project_metrics)" user_id: ~, user_name: ~, user_roles: ~, + exclusive_time: ~, + profile_id: ~, replay_id: ~, sdk_name: ~, sdk_version: ~, @@ -892,6 +900,8 @@ expression: "(&event.value().unwrap().spans, metrics.project_metrics)" user_id: ~, user_name: ~, user_roles: ~, + exclusive_time: ~, + profile_id: ~, replay_id: ~, sdk_name: ~, sdk_version: ~, From 3c038b4eaf663b4c6cc964bf8ae6d4a36af62ca9 Mon Sep 17 00:00:00 2001 From: Tony Xiao Date: Wed, 27 Nov 2024 11:17:32 -0500 Subject: [PATCH 8/8] update insta snapshots --- relay-spans/src/span.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/relay-spans/src/span.rs b/relay-spans/src/span.rs index 5017c0c7a00..dc2f50b1e84 100644 --- a/relay-spans/src/span.rs +++ b/relay-spans/src/span.rs @@ -614,6 +614,8 @@ mod tests { user_id: ~, user_name: ~, user_roles: ~, + exclusive_time: ~, + profile_id: ~, replay_id: ~, sdk_name: "sentry.php", sdk_version: ~,