diff --git a/CHANGELOG.md b/CHANGELOG.md index 624a49b978b..0fdb3289b31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ - Correctly handle minidump objecstore upload failures. ([#6033](https://github.com/getsentry/relay/pull/6033)) - Add `client.address` attribute to known IP fields. ([#6058](https://github.com/getsentry/relay/pull/6058)) - Fix a bug in mobile attribute normalization. ([#6065](https://github.com/getsentry/relay/pull/6065)) +- Don't infer names during tag extraction in transaction processing. ([#6080](https://github.com/getsentry/relay/pull/6080)) **Internal**: diff --git a/relay-event-normalization/src/normalize/span/snapshots/relay_event_normalization__normalize__span__tag_extraction__tests__cache_extraction.snap b/relay-event-normalization/src/normalize/span/snapshots/relay_event_normalization__normalize__span__tag_extraction__tests__cache_extraction.snap index b46d10a9834..3aef7fc54b9 100644 --- a/relay-event-normalization/src/normalize/span/snapshots/relay_event_normalization__normalize__span__tag_extraction__tests__cache_extraction.snap +++ b/relay-event-normalization/src/normalize/span/snapshots/relay_event_normalization__normalize__span__tag_extraction__tests__cache_extraction.snap @@ -33,8 +33,7 @@ expression: event.to_json_pretty().unwrap() "cache.hit": "true", "cache.key": "[\"my_key\"]", "thread.name": "Thread-4 (process_request_thread)", - "thread.id": "6286962688", - "name": "cache.get_item" + "thread.id": "6286962688" }, "measurements": { "cache.item_size": { @@ -74,8 +73,7 @@ expression: event.to_json_pretty().unwrap() "cache.hit": "false", "cache.key": "[\"my_key\",\"my_key_2\"]", "thread.name": "Thread-4 (process_request_thread)", - "thread.id": "6286962688", - "name": "cache.get_item" + "thread.id": "6286962688" }, "measurements": { "cache.item_size": { @@ -114,8 +112,7 @@ expression: event.to_json_pretty().unwrap() "cache.hit": "false", "cache.key": "[\"my_key_2\"]", "thread.name": "Thread-4 (process_request_thread)", - "thread.id": "6286962688", - "name": "cache.get" + "thread.id": "6286962688" }, "measurements": { "cache.item_size": { diff --git a/relay-event-normalization/src/normalize/span/tag_extraction.rs b/relay-event-normalization/src/normalize/span/tag_extraction.rs index 543ecf3db3f..cb637ddbecc 100644 --- a/relay-event-normalization/src/normalize/span/tag_extraction.rs +++ b/relay-event-normalization/src/normalize/span/tag_extraction.rs @@ -16,9 +16,7 @@ use relay_event_schema::protocol::{ TraceContext, }; use relay_protocol::{Annotated, Empty, FiniteF64, Value}; -use relay_spans::name_for_span; -use sqlparser::ast::{ObjectName, Visitor}; -use sqlparser::ast::{ObjectNamePart, Visit}; +use sqlparser::ast::{ObjectName, ObjectNamePart, Visit, Visitor}; use url::Url; use crate::GeoIpLookup; @@ -1142,8 +1140,6 @@ pub fn extract_tags( && !name.is_empty() { span_tags.name = name.to_owned().into(); - } else if let Some(name) = name_for_span(span) { - span_tags.name = name.into(); } span_tags @@ -3485,35 +3481,4 @@ LIMIT 1 assert_eq!(tags.name.value(), Some(&"my name".to_owned())); } - - #[test] - fn generate_name_from_attributes() { - let span: Span = Annotated::::from_json( - r#"{ - "start_timestamp": 0, - "timestamp": 1, - "span_id": "922dda2462ea4ac2", - "data": { - "db.query.summary": "SELECT users" - }, - "op": "db" - }"#, - ) - .unwrap() - .into_value() - .unwrap(); - - let tags = extract_tags( - &span, - 200, - None, - None, - false, - None, - &[], - &GeoIpLookup::empty(), - ); - - assert_eq!(tags.name.value(), Some(&"SELECT users".to_owned())); - } } diff --git a/relay-spans/src/lib.rs b/relay-spans/src/lib.rs index 8a066268b35..ee867a9ba67 100644 --- a/relay-spans/src/lib.rs +++ b/relay-spans/src/lib.rs @@ -6,7 +6,6 @@ html_favicon_url = "https://raw.githubusercontent.com/getsentry/relay/master/artwork/relay-icon.png" )] -pub use crate::name::name_for_span; 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::v1_to_v2::span_v1_to_span_v2; diff --git a/relay-spans/src/name.rs b/relay-spans/src/name.rs index 4d1adc1f255..a44c912e18a 100644 --- a/relay-spans/src/name.rs +++ b/relay-spans/src/name.rs @@ -1,24 +1,7 @@ use relay_conventions::attributes::SENTRY__OP; use relay_conventions::name_for_op_and_attributes; -use relay_event_schema::protocol::{Attributes, Span}; -use relay_protocol::{Getter, GetterIter, Val}; - -/// Constructs a name attribute for a span, following the rules defined in sentry-conventions. -pub fn name_for_span(span: &Span) -> Option { - let op = span.op.value()?; - - let Some(data) = span.data.value() else { - return Some(name_for_op_and_attributes(op, &EmptyGetter {})); - }; - - Some(name_for_op_and_attributes( - op, - // SpanData's Getter impl treats dots in attribute names as object traversals. - // They have to be escaped in order for an attribute name with dots to be treated as a root - // attribute. - &EscapedGetter(data), - )) -} +use relay_event_schema::protocol::Attributes; +use relay_protocol::{Getter, Val}; /// Constructs a name attribute for a span, following the rules defined in sentry-conventions. pub fn name_for_attributes(attributes: &Attributes) -> Option { @@ -26,26 +9,6 @@ pub fn name_for_attributes(attributes: &Attributes) -> Option { Some(name_for_op_and_attributes(op, &AttributeGetter(attributes))) } -struct EmptyGetter {} - -impl Getter for EmptyGetter { - fn get_value(&self, _path: &str) -> Option> { - None - } -} - -struct EscapedGetter<'a, T: Getter>(&'a T); - -impl<'a, T: Getter> Getter for EscapedGetter<'a, T> { - fn get_value(&self, path: &str) -> Option> { - self.0.get_value(&path.replace(".", "\\.")) - } - - fn get_iter(&self, path: &str) -> Option> { - self.0.get_iter(&path.replace(".", "\\.")) - } -} - /// A custom getter for [`Attributes`] which only resolves values based on the attribute name. /// /// This [`Getter`] does not implement nested traversals, which is the behaviour required for @@ -60,20 +23,10 @@ impl<'a> Getter for AttributeGetter<'a> { #[cfg(test)] mod tests { - use relay_event_schema::protocol::SpanData; - use relay_protocol::{Annotated, Object, Value}; + use relay_protocol::Annotated; use super::*; - #[test] - fn test_span_falls_back_to_op_when_no_templates_defined() { - let span = Span { - op: Annotated::new("foo".to_owned()), - ..Default::default() - }; - assert_eq!(name_for_span(&span), Some("foo".to_owned())); - } - #[test] fn test_attributes_falls_back_to_op_when_no_templates_defined() { let attributes = Attributes::from([( @@ -84,32 +37,6 @@ mod tests { assert_eq!(name_for_attributes(&attributes), Some("foo".to_owned())); } - #[test] - fn test_span_uses_the_first_matching_template() { - let span = Span { - op: Annotated::new("db".to_owned()), - data: Annotated::new(SpanData { - other: Object::from([ - ( - "db.query.summary".to_owned(), - Value::String("SELECT users".to_owned()).into(), - ), - ( - "db.operation.name".to_owned(), - Value::String("INSERT".to_owned()).into(), - ), - ( - "db.collection.name".to_owned(), - Value::String("widgets".to_owned()).into(), - ), - ]), - ..Default::default() - }), - ..Default::default() - }; - assert_eq!(name_for_span(&span), Some("SELECT users".to_owned())); - } - #[test] fn test_attributes_uses_the_first_matching_template() { let attributes = Attributes::from([ @@ -137,28 +64,6 @@ mod tests { ); } - #[test] - fn test_span_uses_fallback_templates_when_data_is_missing() { - let span = Span { - op: Annotated::new("db".to_owned()), - data: Annotated::new(SpanData { - other: Object::from([ - ( - "db.operation.name".to_owned(), - Value::String("INSERT".to_owned()).into(), - ), - ( - "db.collection.name".to_owned(), - Value::String("widgets".to_owned()).into(), - ), - ]), - ..Default::default() - }), - ..Default::default() - }; - assert_eq!(name_for_span(&span), Some("INSERT widgets".to_owned())); - } - #[test] fn test_attributes_uses_fallback_templates_when_data_is_missing() { let attributes = Attributes::from([ @@ -182,15 +87,6 @@ mod tests { ); } - #[test] - fn test_span_falls_back_to_hardcoded_name_when_nothing_matches() { - let span = Span { - op: Annotated::new("db".to_owned()), - ..Default::default() - }; - assert_eq!(name_for_span(&span), Some("Database operation".to_owned())); - } - #[test] fn test_attributes_falls_back_to_hardcoded_name_when_nothing_matches() { let attributes = Attributes::from([( diff --git a/tests/integration/test_spans.py b/tests/integration/test_spans.py index 6abf5bcf004..a3203b004c1 100644 --- a/tests/integration/test_spans.py +++ b/tests/integration/test_spans.py @@ -9,7 +9,7 @@ from sentry_relay.consts import DataCategory from sentry_sdk.envelope import Envelope, Item, PayloadRef -from .asserts import matches_any +from .asserts import matches_any, time_within_delta from .test_store import make_transaction TEST_CONFIG = { @@ -60,6 +60,9 @@ def test_span_extraction( project_config["config"]["features"].append( "organizations:performance-issues-spans" ) + project_config["config"]["piiConfig"]["applications"]["$span.data.'http.route'"] = [ + "@anything:mask" + ] event = make_transaction({"event_id": "cbf6960622e14a45abc1f03b2055b186"}) event["contexts"]["trace"]["status"] = "success" @@ -96,6 +99,24 @@ def test_span_extraction( "timestamp": end.isoformat(), "trace_id": "ff62a8b040f340bda5d830223def1d81", }, + { + "description": "Some http operation", + # The combination of op, `http.request.method`, and + # `http.route` means this span has its name synthesized as + # `GET https://example.com`. + "op": "http.client", + "data": { + "http.request.method": "GET", + "http.route": "https://example.com", + }, + "origin": "auto", + "parent_span_id": "968cff94913ebb07", + "span_id": "cccccccccccccccc", + "start_timestamp": start.isoformat(), + "status": "success", + "timestamp": end.isoformat(), + "trace_id": "ff62a8b040f340bda5d830223def1d81", + }, ] user_id = str(uuid.uuid4()) @@ -123,82 +144,171 @@ def test_span_extraction( ("namespace", b"spans"), } - child_span = spans_consumer.get_span() - - del child_span["received"] - - expected_child_span = { - "attributes": { # Backfilled from `sentry_tags` - "sentry.category": {"type": "string", "value": "http"}, - "sentry.exclusive_time": {"type": "double", "value": 500.0}, - "sentry.normalized_description": {"type": "string", "value": "GET *"}, - "sentry.group": {"type": "string", "value": "37e3d9fab1ae9162"}, - "sentry.op": {"type": "string", "value": "http"}, - "sentry.origin": {"type": "string", "value": "manual"}, - "sentry.platform": {"type": "string", "value": "other"}, - "sentry.replay_id": { - "type": "string", - "value": "4c79f60c11214eb38604f4ae0781bfb2", + expected_child_spans = [ + { + "attributes": { # Backfilled from `sentry_tags` + "sentry.category": {"type": "string", "value": "http"}, + "sentry.exclusive_time": {"type": "double", "value": 500.0}, + "sentry.normalized_description": {"type": "string", "value": "GET *"}, + "sentry.group": {"type": "string", "value": "37e3d9fab1ae9162"}, + "sentry.op": {"type": "string", "value": "http"}, + "sentry.origin": {"type": "string", "value": "manual"}, + "sentry.platform": {"type": "string", "value": "other"}, + "sentry.replay_id": { + "type": "string", + "value": "4c79f60c11214eb38604f4ae0781bfb2", + }, + "sentry.sdk.name": {"type": "string", "value": "raven-node"}, + "sentry.sdk.version": {"type": "string", "value": "2.6.3"}, + "sentry.status": {"type": "string", "value": "ok"}, + "sentry.segment.name": {"type": "string", "value": "hi"}, + "sentry.trace.status": {"type": "string", "value": "ok"}, + "sentry.transaction": {"type": "string", "value": "hi"}, + "sentry.transaction.op": {"type": "string", "value": "hi"}, + "sentry.user": {"type": "string", "value": f"id:{user_id}"}, + "sentry.user.geo.city": {"type": "string", "value": "Vienna"}, + "sentry.user.geo.country_code": {"type": "string", "value": "AT"}, + "sentry.user.geo.region": {"type": "string", "value": "Austria"}, + "sentry.user.geo.subdivision": {"type": "string", "value": "Vienna"}, + "sentry.user.geo.subregion": {"type": "string", "value": "155"}, + "sentry.user.id": {"type": "string", "value": user_id}, + "sentry.user.ip": {"type": "string", "value": "192.168.0.1"}, + "user.geo.city": {"type": "string", "value": "Vienna"}, + "user.geo.country_code": {"type": "string", "value": "AT"}, + "user.geo.region": {"type": "string", "value": "Austria"}, + "user.geo.subdivision": {"type": "string", "value": "Vienna"}, + "user.id": {"type": "string", "value": user_id}, + "user.ip_address": {"type": "string", "value": "192.168.0.1"}, + "sentry.description": { + "type": "string", + "value": "GET /api/0/organizations/?member=1", + }, + "sentry.is_remote": {"type": "boolean", "value": False}, + "sentry.segment.id": {"type": "string", "value": "968cff94913ebb07"}, + "sentry.dsc.project_id": {"type": "string", "value": "42"}, + "sentry.dsc.trace_id": { + "type": "string", + "value": "a0fa8803753e40fd8124b21eeb2986b5", + }, + "sentry.dsc.transaction": {"type": "string", "value": "hi"}, }, - "sentry.sdk.name": {"type": "string", "value": "raven-node"}, - "sentry.sdk.version": {"type": "string", "value": "2.6.3"}, - "sentry.status": {"type": "string", "value": "ok"}, - "sentry.segment.name": {"type": "string", "value": "hi"}, - "sentry.trace.status": {"type": "string", "value": "ok"}, - "sentry.transaction": {"type": "string", "value": "hi"}, - "sentry.transaction.op": {"type": "string", "value": "hi"}, - "sentry.user": {"type": "string", "value": f"id:{user_id}"}, - "sentry.user.geo.city": {"type": "string", "value": "Vienna"}, - "sentry.user.geo.country_code": {"type": "string", "value": "AT"}, - "sentry.user.geo.region": {"type": "string", "value": "Austria"}, - "sentry.user.geo.subdivision": {"type": "string", "value": "Vienna"}, - "sentry.user.geo.subregion": {"type": "string", "value": "155"}, - "sentry.user.id": {"type": "string", "value": user_id}, - "sentry.user.ip": {"type": "string", "value": "192.168.0.1"}, - "user.geo.city": {"type": "string", "value": "Vienna"}, - "user.geo.country_code": {"type": "string", "value": "AT"}, - "user.geo.region": {"type": "string", "value": "Austria"}, - "user.geo.subdivision": {"type": "string", "value": "Vienna"}, - "user.id": {"type": "string", "value": user_id}, - "user.ip_address": {"type": "string", "value": "192.168.0.1"}, - "sentry.description": { - "type": "string", - "value": "GET /api/0/organizations/?member=1", + "downsampled_retention_days": 90, + "end_timestamp": end.timestamp(), + "event_id": "cbf6960622e14a45abc1f03b2055b186", + "is_segment": False, + "links": [ + { + "trace_id": "0f62a8b040f340bda5d830223def1d82", + "span_id": "cbbbbbbbbbbbbbbc", + "sampled": True, + "attributes": { + "span_key": {"type": "string", "value": "span_value"} + }, + }, + ], + "name": "http", + "organization_id": 1, + "parent_span_id": "968cff94913ebb07", + "project_id": 42, + "key_id": 123, + "retention_days": 90, + "accepted_outcome_emitted": relay_emits_accepted_outcome, + "span_id": "bbbbbbbbbbbbbbbb", + "start_timestamp": start.timestamp(), + "status": "ok", + "trace_id": "ff62a8b040f340bda5d830223def1d81", + "received": time_within_delta(), + }, + { + "_meta": { + "attributes": { + "http.route": { + "": { + "len": 19, + "rem": [ + [ + "@anything:mask", + "m", + 0, + 19, + ], + ], + }, + }, + }, }, - "sentry.is_remote": {"type": "boolean", "value": False}, - "sentry.segment.id": {"type": "string", "value": "968cff94913ebb07"}, - "sentry.dsc.project_id": {"type": "string", "value": "42"}, - "sentry.dsc.trace_id": { - "type": "string", - "value": "a0fa8803753e40fd8124b21eeb2986b5", + "attributes": { # Backfilled from `sentry_tags` + "http.request.method": {"type": "string", "value": "GET"}, + "http.route": {"type": "string", "value": "*******************"}, + "sentry.category": {"type": "string", "value": "http"}, + "sentry.exclusive_time": {"type": "double", "value": 500.0}, + "sentry.op": {"type": "string", "value": "http.client"}, + "sentry.origin": {"type": "string", "value": "auto"}, + "sentry.platform": {"type": "string", "value": "other"}, + "sentry.replay_id": { + "type": "string", + "value": "4c79f60c11214eb38604f4ae0781bfb2", + }, + "sentry.sdk.name": {"type": "string", "value": "raven-node"}, + "sentry.sdk.version": {"type": "string", "value": "2.6.3"}, + "sentry.status": {"type": "string", "value": "ok"}, + "sentry.segment.name": {"type": "string", "value": "hi"}, + "sentry.trace.status": {"type": "string", "value": "ok"}, + "sentry.transaction": {"type": "string", "value": "hi"}, + "sentry.transaction.op": {"type": "string", "value": "hi"}, + "sentry.user": {"type": "string", "value": f"id:{user_id}"}, + "sentry.user.geo.city": {"type": "string", "value": "Vienna"}, + "sentry.user.geo.country_code": {"type": "string", "value": "AT"}, + "sentry.user.geo.region": {"type": "string", "value": "Austria"}, + "sentry.user.geo.subdivision": {"type": "string", "value": "Vienna"}, + "sentry.user.geo.subregion": {"type": "string", "value": "155"}, + "sentry.user.id": {"type": "string", "value": user_id}, + "sentry.user.ip": {"type": "string", "value": "192.168.0.1"}, + "user.geo.city": {"type": "string", "value": "Vienna"}, + "user.geo.country_code": {"type": "string", "value": "AT"}, + "user.geo.region": {"type": "string", "value": "Austria"}, + "user.geo.subdivision": {"type": "string", "value": "Vienna"}, + "user.id": {"type": "string", "value": user_id}, + "user.ip_address": {"type": "string", "value": "192.168.0.1"}, + "sentry.description": { + "type": "string", + "value": "Some http operation", + }, + "sentry.is_remote": {"type": "boolean", "value": False}, + "sentry.segment.id": {"type": "string", "value": "968cff94913ebb07"}, + "sentry.dsc.project_id": {"type": "string", "value": "42"}, + "sentry.dsc.trace_id": { + "type": "string", + "value": "a0fa8803753e40fd8124b21eeb2986b5", + }, + "sentry.dsc.transaction": {"type": "string", "value": "hi"}, }, - "sentry.dsc.transaction": {"type": "string", "value": "hi"}, + "downsampled_retention_days": 90, + "end_timestamp": end.timestamp(), + "event_id": "cbf6960622e14a45abc1f03b2055b186", + "is_segment": False, + # Since `http.route` is redacted and the name is synthesized + # from it, it needs to be redacted too. + "name": "GET *******************", + "organization_id": 1, + "parent_span_id": "968cff94913ebb07", + "project_id": 42, + "key_id": 123, + "retention_days": 90, + "accepted_outcome_emitted": relay_emits_accepted_outcome, + "span_id": "cccccccccccccccc", + "start_timestamp": start.timestamp(), + "status": "ok", + "trace_id": "ff62a8b040f340bda5d830223def1d81", + "received": time_within_delta(), }, - "downsampled_retention_days": 90, - "end_timestamp": end.timestamp(), - "event_id": "cbf6960622e14a45abc1f03b2055b186", - "is_segment": False, - "links": [ - { - "trace_id": "0f62a8b040f340bda5d830223def1d82", - "span_id": "cbbbbbbbbbbbbbbc", - "sampled": True, - "attributes": {"span_key": {"type": "string", "value": "span_value"}}, - }, - ], - "name": "http", - "organization_id": 1, - "parent_span_id": "968cff94913ebb07", - "project_id": 42, - "key_id": 123, - "retention_days": 90, - "accepted_outcome_emitted": relay_emits_accepted_outcome, - "span_id": "bbbbbbbbbbbbbbbb", - "start_timestamp": start.timestamp(), - "status": "ok", - "trace_id": "ff62a8b040f340bda5d830223def1d81", - } - assert child_span == expected_child_span + ] + + child_span = spans_consumer.get_span() + assert child_span == expected_child_spans[0] + + child_span = spans_consumer.get_span() + assert child_span == expected_child_spans[1] start_timestamp = datetime.fromisoformat(event["start_timestamp"]).replace( tzinfo=timezone.utc @@ -292,7 +402,7 @@ def test_span_extraction( "org_id": 1, "outcome": 0, "project_id": 42, - "quantity": 2, + "quantity": 3, } ]