From 871ba7574e8374baab594c86cd1484219adef7c2 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 2 Aug 2024 13:39:21 +0200 Subject: [PATCH] fix(spans): Mark all fields untrimmable (#3890) Mark all fields of a span as untrimmable to ensure that the span is either trimmed as a whole, or left intact. This PR is a temporary fix until we have a better way to atomically trim / not trim PRs. Note that when we trim a span from a transaction, no outcome is generated for the span. --- CHANGELOG.md | 1 + relay-event-normalization/src/trimming.rs | 39 ++++------------------- relay-event-schema/src/protocol/span.rs | 23 +++++++------ 3 files changed, 22 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b1d6bb630f..548f944d46 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ **Bug Fixes**: - Allow metrics summaries with only `count` (for sets). ([#3864](https://github.com/getsentry/relay/pull/3864)) +- Do not trim any span field. Remove entire span instead. ([#3890](https://github.com/getsentry/relay/pull/3890)) - Do not drop replays, profiles and standalone spans in proxy Relays. ([#3888](https://github.com/getsentry/relay/pull/3888)) **Internal**: diff --git a/relay-event-normalization/src/trimming.rs b/relay-event-normalization/src/trimming.rs index 56b44f98ef..8285aebbb3 100644 --- a/relay-event-normalization/src/trimming.rs +++ b/relay-event-normalization/src/trimming.rs @@ -948,19 +948,23 @@ mod tests { platform: Annotated::new("a".repeat(1024 * 100)), ..Default::default() }; - let spans = std::iter::repeat_with(|| Annotated::new(span.clone())) + let spans: Vec<_> = std::iter::repeat_with(|| Annotated::new(span.clone())) .take(10) .collect(); let mut event = Annotated::new(Event { - spans: Annotated::new(spans), + spans: Annotated::new(spans.clone()), ..Default::default() }); let mut processor = TrimmingProcessor::new(); processor::process_value(&mut event, &mut processor, ProcessingState::root()).unwrap(); - assert_eq!(event.0.unwrap().spans.0.unwrap().len(), 8); + let trimmed_spans = event.0.unwrap().spans.0.unwrap(); + assert_eq!(trimmed_spans.len(), 8); + + // The actual spans were not touched: + assert_eq!(trimmed_spans.as_slice(), &spans[0..8]); } #[test] @@ -1051,33 +1055,4 @@ mod tests { assert!(get_value!(event.spans[1].start_timestamp).is_some()); assert!(get_value!(event.spans[1].timestamp).is_some()); } - - #[test] - fn test_trim_false_contributes_to_budget() { - for span_id in ["short", "looooooooooooooooooooooooooong"] { - let original_span_id = SpanId(span_id.to_owned()); - let original_description = "a".repeat(900000); - - let mut event = Annotated::new(Event { - spans: Annotated::new(vec![Span { - span_id: original_span_id.clone().into(), - description: original_description.clone().into(), - ..Default::default() - } - .into()]), - ..Default::default() - }); - - let mut processor = TrimmingProcessor::new(); - processor::process_value(&mut event, &mut processor, ProcessingState::root()).unwrap(); - - assert_eq!(get_value!(event.spans[0].span_id!).as_ref(), span_id); - - // The amount of trimming on the description depends on the length of the span id. - assert_eq!( - get_value!(event.spans[0].description!).len(), - 1024 * 800 - 12 - span_id.len(), - ); - } - } } diff --git a/relay-event-schema/src/protocol/span.rs b/relay-event-schema/src/protocol/span.rs index 33410bba2b..d2412e9671 100644 --- a/relay-event-schema/src/protocol/span.rs +++ b/relay-event-schema/src/protocol/span.rs @@ -24,6 +24,7 @@ pub struct Span { /// The amount of time in milliseconds spent in this span, /// excluding its immediate child spans. + #[metastructure(trim = "false")] pub exclusive_time: Annotated, /// Span type (see `OperationType` docs). @@ -54,38 +55,42 @@ pub struct Span { pub is_segment: Annotated, /// The status of a span. + #[metastructure(trim = "false")] pub status: Annotated, /// Human readable description of a span (e.g. method URL). - #[metastructure(pii = "maybe")] + #[metastructure(pii = "maybe", trim = "false")] pub description: Annotated, /// Arbitrary tags on a span, like on the top-level event. - #[metastructure(pii = "maybe")] + #[metastructure(pii = "maybe", trim = "false")] pub tags: Annotated>, /// The origin of the span indicates what created the span (see [OriginType] docs). - #[metastructure(max_chars = 128, allow_chars = "a-zA-Z0-9_.")] + #[metastructure(max_chars = 128, allow_chars = "a-zA-Z0-9_.", trim = "false")] pub origin: Annotated, /// ID of a profile that can be associated with the span. + #[metastructure(trim = "false")] pub profile_id: Annotated, /// Arbitrary additional data on a span. /// /// Besides arbitrary user data, this object also contains SDK-provided fields used by the /// product (see ). - #[metastructure(pii = "true")] + #[metastructure(pii = "true", trim = "false")] pub data: Annotated, /// Tags generated by Relay. These tags are a superset of the tags set on span metrics. + #[metastructure(trim = "false")] pub sentry_tags: Annotated>, /// Timestamp when the span has been received by Sentry. + #[metastructure(trim = "false")] pub received: Annotated, /// Measurements which holds observed values such as web vitals. - #[metastructure(skip_serialization = "empty")] + #[metastructure(skip_serialization = "empty", trim = "false")] #[metastructure(omit_from_schema)] // we only document error events for now pub measurements: Annotated, @@ -93,22 +98,22 @@ pub struct Span { /// /// This shall move to a stable location once we have stabilized the /// interface. This is intentionally not typed today. - #[metastructure(skip_serialization = "empty")] + #[metastructure(skip_serialization = "empty", trim = "false")] pub _metrics_summary: Annotated, /// Platform identifier. /// /// See [`Event::platform`](`crate::protocol::Event::platform`). - #[metastructure(skip_serialization = "empty")] + #[metastructure(skip_serialization = "empty", trim = "false")] pub platform: Annotated, /// Whether the span is a segment span that was converted from a transaction. - #[metastructure(skip_serialization = "empty")] + #[metastructure(skip_serialization = "empty", trim = "false")] pub was_transaction: Annotated, // TODO remove retain when the api stabilizes /// Additional arbitrary fields for forwards compatibility. - #[metastructure(additional_properties, retain = "true", pii = "maybe")] + #[metastructure(additional_properties, retain = "true", pii = "maybe", trim = "false")] pub other: Object, }