diff --git a/relay-event-normalization/src/eap/mod.rs b/relay-event-normalization/src/eap/mod.rs index 56cf1651b18..4734004dc81 100644 --- a/relay-event-normalization/src/eap/mod.rs +++ b/relay-event-normalization/src/eap/mod.rs @@ -800,26 +800,6 @@ fn normalize_http_attributes( } } -/// Makes sure web vital spans are not identified with segments. -/// -/// This was ported from the legacy pipeline for behavior parity. -/// At some point in the future, web vital spans will become metrics, -/// and this will become academic. -pub fn normalize_web_vital_span_segment(span: &mut SpanV2) { - let Some(attributes) = span.attributes.value_mut() else { - return; - }; - - if let Some(op) = attributes.get_value(SENTRY__OP) - && let Some(op_name) = op.as_str() - && (op_name.starts_with("ui.interaction.") || op_name.starts_with("ui.webvital.")) - { - span.is_segment = None.into(); - span.parent_span_id = None.into(); - attributes.remove(SENTRY__SEGMENT__ID); - } -} - /// Double writes sentry conventions attributes into legacy attributes. /// /// This achieves backwards compatibility as it allows products to continue using legacy attributes diff --git a/relay-server/src/processing/legacy_spans/normalize.rs b/relay-server/src/processing/legacy_spans/normalize.rs index 00aba6c5d4b..435746c22e0 100644 --- a/relay-server/src/processing/legacy_spans/normalize.rs +++ b/relay-server/src/processing/legacy_spans/normalize.rs @@ -13,7 +13,7 @@ use relay_event_normalization::{ use relay_event_schema::processor::{ProcessingState, process_value}; use relay_event_schema::protocol::{BrowserContext, EventId, IpAddr, Span, SpanData}; use relay_metrics::UnixTimestamp; -use relay_protocol::{Annotated, Empty, Value}; +use relay_protocol::{Annotated, Value}; use relay_sampling::DynamicSamplingContext; /// Config needed to normalize a standalone span. @@ -62,33 +62,7 @@ pub struct NormalizeSpanConfig<'a> { fn set_segment_attributes(span: &mut Annotated) { let Some(span) = span.value_mut() else { return }; - - // Identify INP spans or other WebVital spans and make sure they are not wrapped in a segment. - if let Some(span_op) = span.op.value() - && (span_op.starts_with("ui.interaction.") || span_op.starts_with("ui.webvital.")) - { - span.is_segment = None.into(); - span.parent_span_id = None.into(); - span.segment_id = None.into(); - return; - } - - let Some(span_id) = span.span_id.value() else { - return; - }; - - if let Some(segment_id) = span.segment_id.value() { - // The span is a segment if and only if the segment_id matches the span_id. - span.is_segment = (segment_id == span_id).into(); - } else if span.parent_span_id.is_empty() { - // If the span has no parent, it is automatically a segment: - span.is_segment = true.into(); - } - - // If the span is a segment, always set the segment_id to the current span_id: - if span.is_segment.value() == Some(&true) { - span.segment_id = span.span_id.clone(); - } + relay_spans::set_segment_attributes(span); } /// Normalizes a standalone span. diff --git a/relay-server/src/processing/spans/process.rs b/relay-server/src/processing/spans/process.rs index 81b525f803a..7c991d479ae 100644 --- a/relay-server/src/processing/spans/process.rs +++ b/relay-server/src/processing/spans/process.rs @@ -167,7 +167,12 @@ fn expand_legacy_span(item: &Item) -> Result> { // We can't enable `infer_name` here: // These spans haven't been PII scrubbed yet, so inferring a name would risk // leaking PII. - .map_value(|span| relay_spans::span_v1_to_span_v2(span, false)); + .map_value(|mut span| { + // Set some segment-related attributes before converting to V2. + // This exists for parity with the legacy standalone span pipeline. + relay_spans::set_segment_attributes(&mut span); + relay_spans::span_v1_to_span_v2(span, false) + }); Ok(WithHeader::new(span)) } @@ -234,7 +239,6 @@ fn normalize_span( // because category derivation depends on having the sentry.op attribute // available. eap::normalize_sentry_op(&mut span.attributes); - eap::normalize_web_vital_span_segment(span); eap::normalize_span_category(&mut span.attributes); eap::normalize_received(&mut span.attributes, meta.received_at()); eap::normalize_client_address(&mut span.attributes, meta.client_addr()); diff --git a/relay-spans/src/lib.rs b/relay-spans/src/lib.rs index 1c93e613187..5b1ac525269 100644 --- a/relay-spans/src/lib.rs +++ b/relay-spans/src/lib.rs @@ -10,6 +10,7 @@ pub use crate::description::derive_description_for_v2_span; pub use crate::name::name_for_attributes; pub use crate::op::derive_op_for_v2_span; pub use crate::otel_to_sentry_v2::otel_to_sentry_span as otel_to_sentry_span_v2; +pub use crate::segment::set_segment_attributes; pub use crate::v1_to_v2::span_v1_to_span_v2; pub use opentelemetry_proto::tonic::trace::v1 as otel_trace; @@ -18,4 +19,5 @@ mod description; mod name; mod op; mod otel_to_sentry_v2; +mod segment; mod v1_to_v2; diff --git a/relay-spans/src/segment.rs b/relay-spans/src/segment.rs new file mode 100644 index 00000000000..6f8bdac54b5 --- /dev/null +++ b/relay-spans/src/segment.rs @@ -0,0 +1,37 @@ +use relay_event_schema::protocol::Span; +use relay_protocol::Empty; + +/// Normalizes various segment-related fields on a span: +/// +/// * Web vital spans have `is_segment`, `parent_span_id`, and `segment_id` erased. +/// * Spans whose `span_id` is equal to the `segment_id` or which don't have a `parent_span_id` have +/// `is_segment` set to `true`. +/// * Finally, segment spans have the `segment_id` set to their own `span_id`. +pub fn set_segment_attributes(span: &mut Span) { + // Identify INP spans or other WebVital spans and make sure they are not wrapped in a segment. + if let Some(span_op) = span.op.value() + && (span_op.starts_with("ui.interaction.") || span_op.starts_with("ui.webvital.")) + { + span.is_segment = None.into(); + span.parent_span_id = None.into(); + span.segment_id = None.into(); + return; + } + + let Some(span_id) = span.span_id.value() else { + return; + }; + + if let Some(segment_id) = span.segment_id.value() { + // The span is a segment if and only if the segment_id matches the span_id. + span.is_segment = (segment_id == span_id).into(); + } else if span.parent_span_id.is_empty() { + // If the span has no parent, it is automatically a segment: + span.is_segment = true.into(); + } + + // If the span is a segment, always set the segment_id to the current span_id: + if span.is_segment.value() == Some(&true) { + span.segment_id = span.span_id.clone(); + } +} diff --git a/tests/integration/test_spans_standalone.py b/tests/integration/test_spans_standalone.py index 6b34c6f0856..50e47809b02 100644 --- a/tests/integration/test_spans_standalone.py +++ b/tests/integration/test_spans_standalone.py @@ -1,9 +1,12 @@ from datetime import datetime, timezone from sentry_sdk.envelope import Envelope, Item, PayloadRef +from sentry_relay.consts import DataCategory from .asserts import time_within_delta, time_within +from .test_dynamic_sampling import add_sampling_config + import pytest # Some profiles from Sentry @@ -1192,5 +1195,137 @@ def test_name_inference( } +@pytest.mark.parametrize("sampled", [False, True]) +def test_inp_span_is_segment_ds( + mini_sentry, + relay, + relay_with_processing, + metrics_consumer, + outcomes_consumer, + sampled, +): + """ + Verifies that the `is_segment` tag on metrics is correctly unset + regardless of whether the span is sampled. + + This is specifically related to https://github.com/getsentry/relay/pull/6042. + and https://github.com/getsentry/relay/pull/6126. + If we ever decide to get rid of the functionality introduced in those PRs, this + test becomes obsolete. + """ + metrics_consumer = metrics_consumer() + outcomes_consumer = outcomes_consumer() + + project_id = 42 + project_config = mini_sentry.add_full_project_config(project_id) + project_config["config"]["performanceScore"] = { + "profiles": performance_score_profiles + } + project_config["config"].setdefault("features", []).append( + "organizations:relay-generate-billing-outcome" + ) + project_config["config"].setdefault("features", []).append( + "projects:span-v2-experimental-processing" + ) + + add_sampling_config( + project_config, sample_rate=1.0 if sampled else 0.0, rule_type="project" + ) + + relay = relay_with_processing() + + ts = datetime.now(timezone.utc) + + envelope = envelope_with_spans( + { + "data": { + "sentry.op": "ui.interaction.click", + "release": "frontend@488531b11e6401fa530ac25554d44426e6ef0f0b", + "environment": "prod", + "replay_id": "3d76a6311de149b9b3f560827ea0ecf9", + "transaction": "/insights/projects/", + "user_agent.original": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Firefox/42.0", + "client.address": "{{auto}}", + "sentry.exclusive_time": 104, + }, + "description": "", + "op": "ui.interaction.click", + "parent_span_id": "8a6626cc9bdd5d9b", + "span_id": "a6f029fbe0e2389a", + "start_timestamp": ts.timestamp() - 0.5, + "timestamp": ts.timestamp(), + "trace_id": "d3d20f000885466b8c8f947c9b92b8d3", + "origin": "auto.http.browser.inp", + "exclusive_time": 104, + "measurements": {"inp": {"value": 104, "unit": "millisecond"}}, + "segment_id": "8a6626cc9bdd5d9b", + "is_segment": True, + }, + trace_info={ + "trace_id": "d3d20f000885466b8c8f947c9b92b8d3", + "public_key": project_config["publicKeys"][0]["publicKey"], + "transaction": "/insights/projects/", + }, + ) + + relay.send_envelope(project_id, envelope) + + assert metrics_consumer.get_metrics(with_headers=False) == [ + { + "org_id": 1, + "project_id": 42, + "name": "c:spans/count_per_root_project@none", + "type": "c", + "value": 1.0, + "timestamp": time_within_delta(ts), + "tags": { + "decision": "keep" if sampled else "drop", + "is_segment": "false", + "target_project_id": "42", + "transaction": "/insights/projects/", + }, + "retention_days": 90, + "received_at": time_within(ts, precision="s"), + }, + { + "org_id": 1, + "project_id": 42, + "name": "c:spans/usage@none", + "type": "c", + "value": 1.0, + "timestamp": time_within_delta(ts), + "tags": {"is_segment": "false", "billing_outcome_emitted": "true"}, + "retention_days": 90, + "received_at": time_within(ts, precision="s"), + }, + ] + + expected_outcomes = [ + { + "category": DataCategory.SPAN.value, + "key_id": 123, + "org_id": 1, + "outcome": 0, + "project_id": 42, + "quantity": 1, + } + ] + + if not sampled: + expected_outcomes.append( + { + "category": DataCategory.SPAN_INDEXED.value, + "key_id": 123, + "org_id": 1, + "outcome": 1, + "project_id": 42, + "quantity": 1, + "reason": "Sampled:0", + } + ) + + assert outcomes_consumer.get_aggregated_outcomes() == expected_outcomes + + def _if_dict(cond, then): return then if cond else {} diff --git a/tests/integration/test_spansv2.py b/tests/integration/test_spansv2.py index aee8f0af526..97370ea0a7f 100644 --- a/tests/integration/test_spansv2.py +++ b/tests/integration/test_spansv2.py @@ -470,6 +470,7 @@ def test_spansv2_ds_drop(mini_sentry, relay, span, rule_type): "timestamp": ts.timestamp() + 0.5, "trace_id": "5b8efff798038103d269b633813fc60c", "span_id": "eee19b7ec3c1b176", + "parent_span_id": "eee19b7ec3c1b177", "op": "some op", "description": "some description", "data": {"foo": "bar"},