From 121a6a0258827172d0c05403bdbed5f0184bba52 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 31 May 2024 13:06:18 +0200 Subject: [PATCH 01/11] fix(spans): Do not trim trace ID --- relay-event-derive/src/lib.rs | 21 +++++++ relay-event-normalization/src/trimming.rs | 71 +++++++++++++++++++---- relay-event-schema/src/processor/attrs.rs | 3 + relay-event-schema/src/protocol/span.rs | 2 +- 4 files changed, 86 insertions(+), 11 deletions(-) diff --git a/relay-event-derive/src/lib.rs b/relay-event-derive/src/lib.rs index 0812bba1f7..a313305d13 100644 --- a/relay-event-derive/src/lib.rs +++ b/relay-event-derive/src/lib.rs @@ -323,6 +323,7 @@ struct FieldAttrs { max_chars_allowance: Option, max_depth: Option, max_bytes: Option, + trim: Option, } impl FieldAttrs { @@ -366,6 +367,14 @@ impl FieldAttrs { quote!(crate::processor::Pii::False) }; + let trim = if let Some(trim) = self.trim { + quote!(#trim) + } else if let Some(ref parent_attrs) = inherit_from_field_attrs { + quote!(#parent_attrs.trim) + } else { + quote!(true) + }; + let retain = self.retain; let max_chars = if let Some(ref max_chars) = self.max_chars { @@ -421,6 +430,7 @@ impl FieldAttrs { max_bytes: #max_bytes, pii: #pii, retain: #retain, + trim: #trim, } }) } @@ -595,6 +605,17 @@ fn parse_field_attributes( panic!("Got non string literal for retain"); } } + } else if ident == "trim" { + match name_value.lit { + Lit::Str(litstr) => match litstr.value().as_str() { + "true" => rv.trim = None, + "false" => rv.trim = Some(false), + other => panic!("Unknown value {other}"), + }, + _ => { + panic!("Got non string literal for retain"); + } + } } else if ident == "legacy_alias" || ident == "skip_serialization" { // Skip } else { diff --git a/relay-event-normalization/src/trimming.rs b/relay-event-normalization/src/trimming.rs index dc210d6ed1..e83b88f31b 100644 --- a/relay-event-normalization/src/trimming.rs +++ b/relay-event-normalization/src/trimming.rs @@ -74,15 +74,16 @@ impl Processor for TrimmingProcessor { }); } - if self.remaining_size() == Some(0) { - // TODO: Create remarks (ensure they do not bloat event) - return Err(ProcessingAction::DeleteValueHard); - } - if self.remaining_depth(state) == Some(0) { - // TODO: Create remarks (ensure they do not bloat event) - return Err(ProcessingAction::DeleteValueHard); + if state.attrs().trim { + if self.remaining_size() == Some(0) { + // TODO: Create remarks (ensure they do not bloat event) + return Err(ProcessingAction::DeleteValueHard); + } + if self.remaining_depth(state) == Some(0) { + // TODO: Create remarks (ensure they do not bloat event) + return Err(ProcessingAction::DeleteValueHard); + } } - Ok(()) } @@ -127,6 +128,9 @@ impl Processor for TrimmingProcessor { meta: &mut Meta, state: &ProcessingState<'_>, ) -> ProcessingResult { + if !state.attrs().trim { + return Ok(()); + } if let Some(max_chars) = state.attrs().max_chars { trim_string(value, meta, max_chars, state.attrs().max_chars_allowance); } @@ -149,6 +153,10 @@ impl Processor for TrimmingProcessor { where T: ProcessValue, { + if !state.attrs().trim { + return Ok(()); + } + // If we need to check the bag size, then we go down a different path if !self.size_state.is_empty() { let original_length = value.len(); @@ -166,6 +174,12 @@ impl Processor for TrimmingProcessor { let item_state = state.enter_index(index, None, ValueType::for_field(item)); processor::process_value(item, self, &item_state)?; + + // Remaining size might now be zero because of `trim = "false"`. + if self.remaining_size().unwrap() == 0 { + split_index = Some(index); + break; + } } if let Some(split_index) = split_index { @@ -191,6 +205,10 @@ impl Processor for TrimmingProcessor { where T: ProcessValue, { + if !state.attrs().trim { + return Ok(()); + } + // If we need to check the bag size, then we go down a different path if !self.size_state.is_empty() { let original_length = value.len(); @@ -208,6 +226,12 @@ impl Processor for TrimmingProcessor { let item_state = state.enter_borrowed(key, None, ValueType::for_field(item)); processor::process_value(item, self, &item_state)?; + + // Remaining size might now be zero because of `trim = "false"`. + if self.remaining_size().unwrap() == 0 { + split_key = Some(key.to_owned()); + break; + } } if let Some(split_key) = split_key { @@ -230,6 +254,10 @@ impl Processor for TrimmingProcessor { _meta: &mut Meta, state: &ProcessingState<'_>, ) -> ProcessingResult { + if !state.attrs().trim { + return Ok(()); + } + match value { Value::Array(_) | Value::Object(_) => { if self.remaining_depth(state) == Some(1) { @@ -252,6 +280,10 @@ impl Processor for TrimmingProcessor { _meta: &mut Meta, state: &ProcessingState<'_>, ) -> ProcessingResult { + if !state.attrs().trim { + return Ok(()); + } + processor::apply(&mut stacktrace.frames, |frames, meta| { enforce_frame_hard_limit(frames, meta, 250); Ok(()) @@ -393,9 +425,10 @@ mod tests { use std::iter::repeat; use relay_event_schema::protocol::{ - Breadcrumb, Context, Contexts, Event, Exception, ExtraValue, Span, TagEntry, Tags, Values, + Breadcrumb, Context, Contexts, Event, Exception, ExtraValue, Span, TagEntry, Tags, TraceId, + Values, }; - use relay_protocol::{Map, Remark, SerializableAnnotated}; + use relay_protocol::{get_value, Map, Remark, SerializableAnnotated}; use similar_asserts::assert_eq; use crate::MaxChars; @@ -930,4 +963,22 @@ mod tests { assert_eq!(event.0.unwrap().spans.0.unwrap().len(), 8); } + + #[test] + fn test_too_many_spans_trimmed_trace_id() { + let span = Span { + trace_id: Annotated::new(TraceId("a".repeat(1024 * 900))), + + ..Default::default() + }; + let mut event = Annotated::new(Event { + spans: Annotated::new(vec![span.into()]), + ..Default::default() + }); + + let mut processor = TrimmingProcessor::new(); + processor::process_value(&mut event, &mut processor, ProcessingState::root()).unwrap(); + + assert!(get_value!(event.spans!).is_empty()); + } } diff --git a/relay-event-schema/src/processor/attrs.rs b/relay-event-schema/src/processor/attrs.rs index 5cfaa0beab..c80a9fe7c6 100644 --- a/relay-event-schema/src/processor/attrs.rs +++ b/relay-event-schema/src/processor/attrs.rs @@ -128,6 +128,8 @@ pub struct FieldAttrs { pub pii: Pii, /// Whether additional properties should be retained during normalization. pub retain: bool, + /// Whether the trimming processor is allowed to shorten or drop this field. + pub trim: bool, } /// A set of characters allowed or denied for a (string) field. @@ -167,6 +169,7 @@ impl FieldAttrs { max_bytes: None, pii: Pii::False, retain: false, + trim: true, } } diff --git a/relay-event-schema/src/protocol/span.rs b/relay-event-schema/src/protocol/span.rs index 08199c0f38..118c2eb34d 100644 --- a/relay-event-schema/src/protocol/span.rs +++ b/relay-event-schema/src/protocol/span.rs @@ -42,7 +42,7 @@ pub struct Span { pub parent_span_id: Annotated, /// The ID of the trace the span belongs to. - #[metastructure(required = "true")] + #[metastructure(required = "true", trim = "false")] pub trace_id: Annotated, /// A unique identifier for a segment within a trace (8 byte hexadecimal string). From e6d228208c57de3c67c095cd58e72a0e2cd44fa5 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 4 Jun 2024 09:00:21 +0200 Subject: [PATCH 02/11] wip: Size depleted vs. size exceeded --- relay-event-normalization/src/trimming.rs | 35 ++++++++++++++--------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/relay-event-normalization/src/trimming.rs b/relay-event-normalization/src/trimming.rs index e83b88f31b..c4ed933de6 100644 --- a/relay-event-normalization/src/trimming.rs +++ b/relay-event-normalization/src/trimming.rs @@ -11,7 +11,7 @@ use relay_protocol::{Annotated, Array, Empty, Meta, Object, RemarkType, Value}; struct SizeState { max_depth: Option, encountered_at_depth: usize, - size_remaining: Option, + size_remaining: Option, } /// Limits properties to a maximum size and depth. @@ -48,12 +48,20 @@ impl TrimmingProcessor { } #[inline] - fn remaining_size(&self) -> Option { + fn remaining_size(&self) -> Option { self.size_state .iter() .filter_map(|x| x.size_remaining) .min() } + + fn size_depleted(&self) -> bool { + matches!(self.remaining_size(), Some(n) if n <= 0) + } + + fn size_exceeded(&self) -> bool { + matches!(dbg!(self.remaining_size()), Some(n) if n < 0) + } } impl Processor for TrimmingProcessor { @@ -68,14 +76,14 @@ impl Processor for TrimmingProcessor { // XXX(iker): test setting only one of the two attributes. if state.attrs().max_bytes.is_some() || state.attrs().max_depth.is_some() { self.size_state.push(SizeState { - size_remaining: state.attrs().max_bytes, + size_remaining: state.attrs().max_bytes.map(|s| s as isize), encountered_at_depth: state.depth(), max_depth: state.attrs().max_depth, }); } if state.attrs().trim { - if self.remaining_size() == Some(0) { + if self.size_depleted() { // TODO: Create remarks (ensure they do not bloat event) return Err(ProcessingAction::DeleteValueHard); } @@ -115,7 +123,7 @@ impl Processor for TrimmingProcessor { let item_length = relay_protocol::estimate_size_flat(value) + 1; size_state.size_remaining = size_state .size_remaining - .map(|size| size.saturating_sub(item_length)); + .map(|size| size.saturating_sub_unsigned(item_length)); } } @@ -137,7 +145,7 @@ impl Processor for TrimmingProcessor { if let Some(size_state) = self.size_state.last() { if let Some(size_remaining) = size_state.size_remaining { - trim_string(value, meta, size_remaining, 0); + trim_string(value, meta, size_remaining.try_into().unwrap_or(0), 0); } } @@ -167,7 +175,7 @@ impl Processor for TrimmingProcessor { let mut split_index = None; for (index, item) in value.iter_mut().enumerate() { - if self.remaining_size().unwrap() == 0 { + if self.size_depleted() { split_index = Some(index); break; } @@ -175,8 +183,8 @@ impl Processor for TrimmingProcessor { let item_state = state.enter_index(index, None, ValueType::for_field(item)); processor::process_value(item, self, &item_state)?; - // Remaining size might now be zero because of `trim = "false"`. - if self.remaining_size().unwrap() == 0 { + // Remaining size might now be negative because of `trim = "false"`. + if self.size_exceeded() { split_index = Some(index); break; } @@ -219,7 +227,7 @@ impl Processor for TrimmingProcessor { let mut split_key = None; for (key, item) in value.iter_mut() { - if self.remaining_size().unwrap() == 0 { + if dbg!(self.size_depleted()) { split_key = Some(key.to_owned()); break; } @@ -227,8 +235,8 @@ impl Processor for TrimmingProcessor { let item_state = state.enter_borrowed(key, None, ValueType::for_field(item)); processor::process_value(item, self, &item_state)?; - // Remaining size might now be zero because of `trim = "false"`. - if self.remaining_size().unwrap() == 0 { + // Remaining size might now be negative because of `trim = "false"`. + if dbg!(self.size_exceeded()) { split_key = Some(key.to_owned()); break; } @@ -714,6 +722,7 @@ mod tests { .len(), 5000 ); + dbg!(i); assert_eq!( other .get("foo") @@ -961,7 +970,7 @@ mod tests { 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); + assert_eq!(event.0.unwrap().spans.0.unwrap().len(), 7); } #[test] From 1136f76d983422fa2f4ae240197d7e43ef2b6435 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 4 Jun 2024 09:02:48 +0200 Subject: [PATCH 03/11] Revert "wip: Size depleted vs. size exceeded" This reverts commit e6d228208c57de3c67c095cd58e72a0e2cd44fa5. --- relay-event-normalization/src/trimming.rs | 35 +++++++++-------------- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/relay-event-normalization/src/trimming.rs b/relay-event-normalization/src/trimming.rs index c4ed933de6..e83b88f31b 100644 --- a/relay-event-normalization/src/trimming.rs +++ b/relay-event-normalization/src/trimming.rs @@ -11,7 +11,7 @@ use relay_protocol::{Annotated, Array, Empty, Meta, Object, RemarkType, Value}; struct SizeState { max_depth: Option, encountered_at_depth: usize, - size_remaining: Option, + size_remaining: Option, } /// Limits properties to a maximum size and depth. @@ -48,20 +48,12 @@ impl TrimmingProcessor { } #[inline] - fn remaining_size(&self) -> Option { + fn remaining_size(&self) -> Option { self.size_state .iter() .filter_map(|x| x.size_remaining) .min() } - - fn size_depleted(&self) -> bool { - matches!(self.remaining_size(), Some(n) if n <= 0) - } - - fn size_exceeded(&self) -> bool { - matches!(dbg!(self.remaining_size()), Some(n) if n < 0) - } } impl Processor for TrimmingProcessor { @@ -76,14 +68,14 @@ impl Processor for TrimmingProcessor { // XXX(iker): test setting only one of the two attributes. if state.attrs().max_bytes.is_some() || state.attrs().max_depth.is_some() { self.size_state.push(SizeState { - size_remaining: state.attrs().max_bytes.map(|s| s as isize), + size_remaining: state.attrs().max_bytes, encountered_at_depth: state.depth(), max_depth: state.attrs().max_depth, }); } if state.attrs().trim { - if self.size_depleted() { + if self.remaining_size() == Some(0) { // TODO: Create remarks (ensure they do not bloat event) return Err(ProcessingAction::DeleteValueHard); } @@ -123,7 +115,7 @@ impl Processor for TrimmingProcessor { let item_length = relay_protocol::estimate_size_flat(value) + 1; size_state.size_remaining = size_state .size_remaining - .map(|size| size.saturating_sub_unsigned(item_length)); + .map(|size| size.saturating_sub(item_length)); } } @@ -145,7 +137,7 @@ impl Processor for TrimmingProcessor { if let Some(size_state) = self.size_state.last() { if let Some(size_remaining) = size_state.size_remaining { - trim_string(value, meta, size_remaining.try_into().unwrap_or(0), 0); + trim_string(value, meta, size_remaining, 0); } } @@ -175,7 +167,7 @@ impl Processor for TrimmingProcessor { let mut split_index = None; for (index, item) in value.iter_mut().enumerate() { - if self.size_depleted() { + if self.remaining_size().unwrap() == 0 { split_index = Some(index); break; } @@ -183,8 +175,8 @@ impl Processor for TrimmingProcessor { let item_state = state.enter_index(index, None, ValueType::for_field(item)); processor::process_value(item, self, &item_state)?; - // Remaining size might now be negative because of `trim = "false"`. - if self.size_exceeded() { + // Remaining size might now be zero because of `trim = "false"`. + if self.remaining_size().unwrap() == 0 { split_index = Some(index); break; } @@ -227,7 +219,7 @@ impl Processor for TrimmingProcessor { let mut split_key = None; for (key, item) in value.iter_mut() { - if dbg!(self.size_depleted()) { + if self.remaining_size().unwrap() == 0 { split_key = Some(key.to_owned()); break; } @@ -235,8 +227,8 @@ impl Processor for TrimmingProcessor { let item_state = state.enter_borrowed(key, None, ValueType::for_field(item)); processor::process_value(item, self, &item_state)?; - // Remaining size might now be negative because of `trim = "false"`. - if dbg!(self.size_exceeded()) { + // Remaining size might now be zero because of `trim = "false"`. + if self.remaining_size().unwrap() == 0 { split_key = Some(key.to_owned()); break; } @@ -722,7 +714,6 @@ mod tests { .len(), 5000 ); - dbg!(i); assert_eq!( other .get("foo") @@ -970,7 +961,7 @@ mod tests { let mut processor = TrimmingProcessor::new(); processor::process_value(&mut event, &mut processor, ProcessingState::root()).unwrap(); - assert_eq!(event.0.unwrap().spans.0.unwrap().len(), 7); + assert_eq!(event.0.unwrap().spans.0.unwrap().len(), 8); } #[test] From 513c6ecc63ef80a2ad3724995b26e888dc2d4857 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 4 Jun 2024 09:19:03 +0200 Subject: [PATCH 04/11] fix --- relay-event-normalization/src/trimming.rs | 52 ++++++++++++++++------- 1 file changed, 36 insertions(+), 16 deletions(-) diff --git a/relay-event-normalization/src/trimming.rs b/relay-event-normalization/src/trimming.rs index e83b88f31b..da2aacc487 100644 --- a/relay-event-normalization/src/trimming.rs +++ b/relay-event-normalization/src/trimming.rs @@ -167,19 +167,13 @@ impl Processor for TrimmingProcessor { let mut split_index = None; for (index, item) in value.iter_mut().enumerate() { - if self.remaining_size().unwrap() == 0 { + if self.remaining_size() == Some(0) { split_index = Some(index); break; } let item_state = state.enter_index(index, None, ValueType::for_field(item)); processor::process_value(item, self, &item_state)?; - - // Remaining size might now be zero because of `trim = "false"`. - if self.remaining_size().unwrap() == 0 { - split_index = Some(index); - break; - } } if let Some(split_index) = split_index { @@ -219,19 +213,13 @@ impl Processor for TrimmingProcessor { let mut split_key = None; for (key, item) in value.iter_mut() { - if self.remaining_size().unwrap() == 0 { + if self.remaining_size() == Some(0) { split_key = Some(key.to_owned()); break; } let item_state = state.enter_borrowed(key, None, ValueType::for_field(item)); processor::process_value(item, self, &item_state)?; - - // Remaining size might now be zero because of `trim = "false"`. - if self.remaining_size().unwrap() == 0 { - split_key = Some(key.to_owned()); - break; - } } if let Some(split_key) = split_key { @@ -966,9 +954,36 @@ mod tests { #[test] fn test_too_many_spans_trimmed_trace_id() { + let original_description = "a".repeat(819163); + let original_trace_id = TraceId("b".repeat(48)); let span = Span { - trace_id: Annotated::new(TraceId("a".repeat(1024 * 900))), + description: original_description.clone().into(), + trace_id: original_trace_id.clone().into(), + ..Default::default() + }; + let mut event = Annotated::new(Event { + spans: Annotated::new(vec![span.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].description!), + &original_description + ); + // Trace ID would be trimmed without `trim = "false"` + assert_eq!(get_value!(event.spans[0].trace_id!), &original_trace_id); + } + #[test] + fn test_too_many_spans_trimmed_trace_id_drop() { + let original_description = "a".repeat(819191); + let original_trace_id = TraceId("b".repeat(48)); + let span = Span { + description: original_description.clone().into(), + trace_id: original_trace_id.clone().into(), ..Default::default() }; let mut event = Annotated::new(Event { @@ -979,6 +994,11 @@ mod tests { let mut processor = TrimmingProcessor::new(); processor::process_value(&mut event, &mut processor, ProcessingState::root()).unwrap(); - assert!(get_value!(event.spans!).is_empty()); + assert_eq!( + get_value!(event.spans[0].description!), + &original_description + ); + // Trace ID would be dropped without `trim = "false"` + assert_eq!(get_value!(event.spans[0].trace_id!), &original_trace_id); } } From a18f3f36cc5e9518415dbced4e0867c979c3dd32 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 4 Jun 2024 10:05:03 +0200 Subject: [PATCH 05/11] more fields --- relay-event-normalization/src/trimming.rs | 16 ++++++++++++---- relay-event-schema/src/protocol/span.rs | 5 ++++- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/relay-event-normalization/src/trimming.rs b/relay-event-normalization/src/trimming.rs index da2aacc487..4d0c965949 100644 --- a/relay-event-normalization/src/trimming.rs +++ b/relay-event-normalization/src/trimming.rs @@ -413,8 +413,8 @@ mod tests { use std::iter::repeat; use relay_event_schema::protocol::{ - Breadcrumb, Context, Contexts, Event, Exception, ExtraValue, Span, TagEntry, Tags, TraceId, - Values, + Breadcrumb, Context, Contexts, Event, Exception, ExtraValue, Span, SpanId, TagEntry, Tags, + TraceId, Values, }; use relay_protocol::{get_value, Map, Remark, SerializableAnnotated}; use similar_asserts::assert_eq; @@ -980,10 +980,15 @@ mod tests { #[test] fn test_too_many_spans_trimmed_trace_id_drop() { let original_description = "a".repeat(819191); - let original_trace_id = TraceId("b".repeat(48)); + let original_span_id = SpanId("b".repeat(48)); + let original_trace_id = TraceId("c".repeat(48)); + let original_segment_id = SpanId("d".repeat(48)); let span = Span { description: original_description.clone().into(), + span_id: original_span_id.clone().into(), trace_id: original_trace_id.clone().into(), + segment_id: original_segment_id.clone().into(), + is_segment: false.into(), ..Default::default() }; let mut event = Annotated::new(Event { @@ -998,7 +1003,10 @@ mod tests { get_value!(event.spans[0].description!), &original_description ); - // Trace ID would be dropped without `trim = "false"` + // These fields would be dropped without `trim = "false"` + assert_eq!(get_value!(event.spans[0].span_id!), &original_span_id); assert_eq!(get_value!(event.spans[0].trace_id!), &original_trace_id); + assert_eq!(get_value!(event.spans[0].segment_id!), &original_segment_id); + assert_eq!(get_value!(event.spans[0].is_segment!), &false); } } diff --git a/relay-event-schema/src/protocol/span.rs b/relay-event-schema/src/protocol/span.rs index 118c2eb34d..b59cc81076 100644 --- a/relay-event-schema/src/protocol/span.rs +++ b/relay-event-schema/src/protocol/span.rs @@ -35,10 +35,11 @@ pub struct Span { pub op: Annotated, /// The Span id. - #[metastructure(required = "true")] + #[metastructure(required = "true", trim = "false")] pub span_id: Annotated, /// The ID of the span enclosing this span. + #[metastructure(trim = "false")] pub parent_span_id: Annotated, /// The ID of the trace the span belongs to. @@ -49,9 +50,11 @@ pub struct Span { /// /// For spans embedded in transactions, the `segment_id` is the `span_id` of the containing /// transaction. + #[metastructure(trim = "false")] pub segment_id: Annotated, /// Whether or not the current span is the root of the segment. + #[metastructure(trim = "false")] pub is_segment: Annotated, /// The status of a span. From d52a4a981c405d0649b0e32ce1f9a0287ceca0b3 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 4 Jun 2024 10:15:14 +0200 Subject: [PATCH 06/11] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4cb086f04b..4d7a8d2f60 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - Apply globally defined metric tags to legacy transaction metrics. ([#3615](https://github.com/getsentry/relay/pull/3615)) - Limit the maximum size of spans in an transaction to 800 kib. ([#3645](https://github.com/getsentry/relay/pull/3645)) - Scrub identifiers in spans with `op:db` and `db_system:redis`. ([#3642](https://github.com/getsentry/relay/pull/3642)) +- Stop trimming important span fields by marking them `trim = "false"`. ([#3670](https://github.com/getsentry/relay/pull/3670)) **Features**: From baa617623a573e7193d40fe3b38b318e8f9cb8fb Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 4 Jun 2024 11:08:11 +0200 Subject: [PATCH 07/11] fix: op --- relay-event-normalization/src/trimming.rs | 12 +++++++++--- relay-event-schema/src/protocol/span.rs | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/relay-event-normalization/src/trimming.rs b/relay-event-normalization/src/trimming.rs index 4d0c965949..c948b8226b 100644 --- a/relay-event-normalization/src/trimming.rs +++ b/relay-event-normalization/src/trimming.rs @@ -128,13 +128,14 @@ impl Processor for TrimmingProcessor { meta: &mut Meta, state: &ProcessingState<'_>, ) -> ProcessingResult { - if !state.attrs().trim { - return Ok(()); - } if let Some(max_chars) = state.attrs().max_chars { trim_string(value, meta, max_chars, state.attrs().max_chars_allowance); } + if !state.attrs().trim { + return Ok(()); + } + if let Some(size_state) = self.size_state.last() { if let Some(size_remaining) = size_state.size_remaining { trim_string(value, meta, size_remaining, 0); @@ -983,12 +984,14 @@ mod tests { let original_span_id = SpanId("b".repeat(48)); let original_trace_id = TraceId("c".repeat(48)); let original_segment_id = SpanId("d".repeat(48)); + let original_op = "e".repeat(129); let span = Span { description: original_description.clone().into(), span_id: original_span_id.clone().into(), trace_id: original_trace_id.clone().into(), segment_id: original_segment_id.clone().into(), is_segment: false.into(), + op: original_op.clone().into(), ..Default::default() }; let mut event = Annotated::new(Event { @@ -1008,5 +1011,8 @@ mod tests { assert_eq!(get_value!(event.spans[0].trace_id!), &original_trace_id); assert_eq!(get_value!(event.spans[0].segment_id!), &original_segment_id); assert_eq!(get_value!(event.spans[0].is_segment!), &false); + + // span.op is trimmed to its max_chars, but not dropped: + assert_eq!(get_value!(event.spans[0].op!).len(), 128); } } diff --git a/relay-event-schema/src/protocol/span.rs b/relay-event-schema/src/protocol/span.rs index b59cc81076..a7830fe23d 100644 --- a/relay-event-schema/src/protocol/span.rs +++ b/relay-event-schema/src/protocol/span.rs @@ -31,7 +31,7 @@ pub struct Span { pub description: Annotated, /// Span type (see `OperationType` docs). - #[metastructure(max_chars = 128)] + #[metastructure(max_chars = 128, trim = "false")] pub op: Annotated, /// The Span id. From 1267c6141207062f7e8ddaffe046db9c417ec75f Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 4 Jun 2024 11:27:29 +0200 Subject: [PATCH 08/11] test --- relay-event-normalization/src/trimming.rs | 88 +++++++++++++++++------ relay-event-schema/src/protocol/span.rs | 8 +-- 2 files changed, 69 insertions(+), 27 deletions(-) diff --git a/relay-event-normalization/src/trimming.rs b/relay-event-normalization/src/trimming.rs index c948b8226b..c5bb4a9ee1 100644 --- a/relay-event-normalization/src/trimming.rs +++ b/relay-event-normalization/src/trimming.rs @@ -957,13 +957,19 @@ mod tests { fn test_too_many_spans_trimmed_trace_id() { let original_description = "a".repeat(819163); let original_trace_id = TraceId("b".repeat(48)); - let span = Span { - description: original_description.clone().into(), - trace_id: original_trace_id.clone().into(), - ..Default::default() - }; let mut event = Annotated::new(Event { - spans: Annotated::new(vec![span.into()]), + spans: Annotated::new(vec![ + Span { + description: original_description.clone().into(), + ..Default::default() + } + .into(), + Span { + trace_id: original_trace_id.clone().into(), + ..Default::default() + } + .into(), + ]), ..Default::default() }); @@ -975,27 +981,34 @@ mod tests { &original_description ); // Trace ID would be trimmed without `trim = "false"` - assert_eq!(get_value!(event.spans[0].trace_id!), &original_trace_id); + assert_eq!(get_value!(event.spans[1].trace_id!), &original_trace_id); } #[test] fn test_too_many_spans_trimmed_trace_id_drop() { - let original_description = "a".repeat(819191); + let original_description = "a".repeat(819163); let original_span_id = SpanId("b".repeat(48)); let original_trace_id = TraceId("c".repeat(48)); let original_segment_id = SpanId("d".repeat(48)); let original_op = "e".repeat(129); - let span = Span { - description: original_description.clone().into(), - span_id: original_span_id.clone().into(), - trace_id: original_trace_id.clone().into(), - segment_id: original_segment_id.clone().into(), - is_segment: false.into(), - op: original_op.clone().into(), - ..Default::default() - }; + let mut event = Annotated::new(Event { - spans: Annotated::new(vec![span.into()]), + spans: Annotated::new(vec![ + Span { + description: original_description.clone().into(), + ..Default::default() + } + .into(), + Span { + span_id: original_span_id.clone().into(), + trace_id: original_trace_id.clone().into(), + segment_id: original_segment_id.clone().into(), + is_segment: false.into(), + op: original_op.clone().into(), + ..Default::default() + } + .into(), + ]), ..Default::default() }); @@ -1007,12 +1020,41 @@ mod tests { &original_description ); // These fields would be dropped without `trim = "false"` - assert_eq!(get_value!(event.spans[0].span_id!), &original_span_id); - assert_eq!(get_value!(event.spans[0].trace_id!), &original_trace_id); - assert_eq!(get_value!(event.spans[0].segment_id!), &original_segment_id); - assert_eq!(get_value!(event.spans[0].is_segment!), &false); + assert_eq!(get_value!(event.spans[1].span_id!), &original_span_id); + assert_eq!(get_value!(event.spans[1].trace_id!), &original_trace_id); + assert_eq!(get_value!(event.spans[1].segment_id!), &original_segment_id); + assert_eq!(get_value!(event.spans[1].is_segment!), &false); // span.op is trimmed to its max_chars, but not dropped: - assert_eq!(get_value!(event.spans[0].op!).len(), 128); + assert_eq!(get_value!(event.spans[1].op!).len(), 128); + } + + #[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 a7830fe23d..53b7078c9c 100644 --- a/relay-event-schema/src/protocol/span.rs +++ b/relay-event-schema/src/protocol/span.rs @@ -26,10 +26,6 @@ pub struct Span { /// excluding its immediate child spans. pub exclusive_time: Annotated, - /// Human readable description of a span (e.g. method URL). - #[metastructure(pii = "maybe")] - pub description: Annotated, - /// Span type (see `OperationType` docs). #[metastructure(max_chars = 128, trim = "false")] pub op: Annotated, @@ -60,6 +56,10 @@ pub struct Span { /// The status of a span. pub status: Annotated, + /// Human readable description of a span (e.g. method URL). + #[metastructure(pii = "maybe")] + pub description: Annotated, + /// Arbitrary tags on a span, like on the top-level event. #[metastructure(pii = "maybe")] pub tags: Annotated>, From 84224e9eef04b576306ca401a5d9b4fa04ed571f Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 4 Jun 2024 11:28:37 +0200 Subject: [PATCH 09/11] fix: panic message --- relay-event-derive/src/lib.rs | 2 +- relay-protocol-derive/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/relay-event-derive/src/lib.rs b/relay-event-derive/src/lib.rs index a313305d13..814802af2b 100644 --- a/relay-event-derive/src/lib.rs +++ b/relay-event-derive/src/lib.rs @@ -613,7 +613,7 @@ fn parse_field_attributes( other => panic!("Unknown value {other}"), }, _ => { - panic!("Got non string literal for retain"); + panic!("Got non string literal for trim"); } } } else if ident == "legacy_alias" || ident == "skip_serialization" { diff --git a/relay-protocol-derive/src/lib.rs b/relay-protocol-derive/src/lib.rs index 4fb7be5959..11f9b2d6ab 100644 --- a/relay-protocol-derive/src/lib.rs +++ b/relay-protocol-derive/src/lib.rs @@ -730,7 +730,7 @@ fn parse_field_attributes( rv.skip_serialization = FromStr::from_str(&litstr.value()) .expect("Unknown value for skip_serialization"); } - _ => panic!("Got non string literal for legacy_alias"), + _ => panic!("Got non string literal for skip_serialization"), } } } From d97f11e17f3e2e8dbf91b035cbba875eb8b8ea1d Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 4 Jun 2024 11:59:31 +0200 Subject: [PATCH 10/11] snapshots --- relay-event-normalization/src/normalize/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/relay-event-normalization/src/normalize/mod.rs b/relay-event-normalization/src/normalize/mod.rs index 57c43b4fa8..884c95ecbe 100644 --- a/relay-event-normalization/src/normalize/mod.rs +++ b/relay-event-normalization/src/normalize/mod.rs @@ -1706,7 +1706,6 @@ mod tests { timestamp: ~, start_timestamp: ~, exclusive_time: ~, - description: ~, op: "app_start_cold", span_id: ~, parent_span_id: ~, @@ -1714,6 +1713,7 @@ mod tests { segment_id: ~, is_segment: ~, status: ~, + description: ~, tags: ~, origin: ~, profile_id: ~, @@ -1751,7 +1751,6 @@ mod tests { timestamp: ~, start_timestamp: ~, exclusive_time: ~, - description: ~, op: "app.start.cold", span_id: ~, parent_span_id: ~, @@ -1759,6 +1758,7 @@ mod tests { segment_id: ~, is_segment: ~, status: ~, + description: ~, tags: ~, origin: ~, profile_id: ~, @@ -1796,7 +1796,6 @@ mod tests { timestamp: ~, start_timestamp: ~, exclusive_time: ~, - description: ~, op: "app.start.warm", span_id: ~, parent_span_id: ~, @@ -1804,6 +1803,7 @@ mod tests { segment_id: ~, is_segment: ~, status: ~, + description: ~, tags: ~, origin: ~, profile_id: ~, From 41379d3b6e115db487263c794f098b7e478e7049 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 4 Jun 2024 12:09:59 +0200 Subject: [PATCH 11/11] all the snapshots --- relay-event-schema/src/protocol/span.rs | 2 +- .../src/protocol/span/convert.rs | 2 +- ...t__tests__extract_span_metrics_mobile.snap | 22 +++++++++---------- .../metrics_extraction/transactions/mod.rs | 2 +- relay-spans/src/span.rs | 2 +- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/relay-event-schema/src/protocol/span.rs b/relay-event-schema/src/protocol/span.rs index 53b7078c9c..933d71f06d 100644 --- a/relay-event-schema/src/protocol/span.rs +++ b/relay-event-schema/src/protocol/span.rs @@ -453,11 +453,11 @@ mod tests { "timestamp": 0.0, "start_timestamp": -63158400.0, "exclusive_time": 1.23, - "description": "desc", "op": "operation", "span_id": "fa90fdead5f74052", "trace_id": "4c79f60c11214eb38604f4ae0781bfb2", "status": "ok", + "description": "desc", "origin": "auto.http", "measurements": { "memory": { diff --git a/relay-event-schema/src/protocol/span/convert.rs b/relay-event-schema/src/protocol/span/convert.rs index 1246e3c649..d283c2acd6 100644 --- a/relay-event-schema/src/protocol/span/convert.rs +++ b/relay-event-schema/src/protocol/span/convert.rs @@ -248,7 +248,6 @@ mod tests { timestamp: ~, start_timestamp: ~, exclusive_time: 123.4, - description: "my 1st transaction", op: "myop", span_id: SpanId( "fa90fdead5f74052", @@ -264,6 +263,7 @@ mod tests { ), is_segment: true, status: Ok, + description: "my 1st transaction", tags: ~, origin: "manual", profile_id: EventId( 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 954b08971b..48ef885df0 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 @@ -12,7 +12,6 @@ expression: "(&event.value().unwrap().spans, metrics)" 2020-08-21T02:18:20Z, ), exclusive_time: 2000.0, - description: ~, op: "app.start.cold", span_id: SpanId( "bd429c44b67a3eb4", @@ -24,6 +23,7 @@ expression: "(&event.value().unwrap().spans, metrics)" segment_id: ~, is_segment: ~, status: ~, + description: ~, tags: ~, origin: ~, profile_id: ~, @@ -60,7 +60,6 @@ expression: "(&event.value().unwrap().spans, metrics)" 2020-08-21T02:18:20Z, ), exclusive_time: 3000.0, - description: ~, op: "ui.load.initial_display", span_id: SpanId( "bd429c44b67a3eb2", @@ -72,6 +71,7 @@ expression: "(&event.value().unwrap().spans, metrics)" segment_id: ~, is_segment: ~, status: ~, + description: ~, tags: ~, origin: ~, profile_id: ~, @@ -185,7 +185,6 @@ expression: "(&event.value().unwrap().spans, metrics)" 2020-08-21T02:18:20Z, ), exclusive_time: 3000.0, - description: "Cold Start", op: "app.start.cold", span_id: SpanId( "bd429c44b67a3eb2", @@ -197,6 +196,7 @@ expression: "(&event.value().unwrap().spans, metrics)" segment_id: ~, is_segment: ~, status: ~, + description: "Cold Start", tags: ~, origin: ~, profile_id: ~, @@ -235,7 +235,6 @@ expression: "(&event.value().unwrap().spans, metrics)" 2020-08-21T02:18:20Z, ), exclusive_time: 3000.0, - description: "Custom Op", op: "custom.op", span_id: SpanId( "bd429c44b67a3eb2", @@ -247,6 +246,7 @@ expression: "(&event.value().unwrap().spans, metrics)" segment_id: ~, is_segment: ~, status: ~, + description: "Custom Op", tags: ~, origin: ~, profile_id: ~, @@ -282,7 +282,6 @@ expression: "(&event.value().unwrap().spans, metrics)" 2020-08-21T02:18:20Z, ), exclusive_time: 3000.0, - description: "io.sentry.android.core.SentryPerformanceProvider.onCreate", op: "contentprovider.load", span_id: SpanId( "bd429c44b67a3eb2", @@ -294,6 +293,7 @@ expression: "(&event.value().unwrap().spans, metrics)" segment_id: ~, is_segment: ~, status: ~, + description: "io.sentry.android.core.SentryPerformanceProvider.onCreate", tags: ~, origin: ~, profile_id: ~, @@ -331,7 +331,6 @@ expression: "(&event.value().unwrap().spans, metrics)" 2020-08-21T02:18:20Z, ), exclusive_time: 3000.0, - description: "io.sentry.samples.android.MyApplication.onCreate", op: "application.load", span_id: SpanId( "bd429c44b67a3eb2", @@ -343,6 +342,7 @@ expression: "(&event.value().unwrap().spans, metrics)" segment_id: ~, is_segment: ~, status: ~, + description: "io.sentry.samples.android.MyApplication.onCreate", tags: ~, origin: ~, profile_id: ~, @@ -380,7 +380,6 @@ expression: "(&event.value().unwrap().spans, metrics)" 2020-08-21T02:18:20Z, ), exclusive_time: 3000.0, - description: "io.sentry.samples.android.MainActivity.onCreate", op: "activity.load", span_id: SpanId( "bd429c44b67a3eb2", @@ -392,6 +391,7 @@ expression: "(&event.value().unwrap().spans, metrics)" segment_id: ~, is_segment: ~, status: ~, + description: "io.sentry.samples.android.MainActivity.onCreate", tags: ~, origin: ~, profile_id: ~, @@ -480,7 +480,6 @@ expression: "(&event.value().unwrap().spans, metrics)" 2020-08-21T02:18:20Z, ), exclusive_time: 3000.0, - description: "Process Initialization", op: "process.load", span_id: SpanId( "bd429c44b67a3eb2", @@ -492,6 +491,7 @@ expression: "(&event.value().unwrap().spans, metrics)" segment_id: ~, is_segment: ~, status: ~, + description: "Process Initialization", tags: ~, origin: ~, profile_id: ~, @@ -578,7 +578,6 @@ expression: "(&event.value().unwrap().spans, metrics)" 2020-08-21T02:18:20Z, ), exclusive_time: 3000.0, - description: "somebackup.212321", op: "file.read", span_id: SpanId( "bd429c44b67a3eb2", @@ -590,6 +589,7 @@ expression: "(&event.value().unwrap().spans, metrics)" segment_id: ~, is_segment: ~, status: ~, + description: "somebackup.212321", tags: ~, origin: ~, profile_id: ~, @@ -628,7 +628,6 @@ expression: "(&event.value().unwrap().spans, metrics)" 2020-08-21T02:18:20Z, ), exclusive_time: 3000.0, - description: "www.example.com", op: "http.client", span_id: SpanId( "bd429c44b67a3eb2", @@ -640,6 +639,7 @@ expression: "(&event.value().unwrap().spans, metrics)" segment_id: ~, is_segment: ~, status: ~, + description: "www.example.com", tags: ~, origin: ~, profile_id: ~, @@ -728,7 +728,6 @@ expression: "(&event.value().unwrap().spans, metrics)" 2020-08-21T02:18:20Z, ), exclusive_time: 3000.0, - description: "www.example.com", op: "http.client", span_id: SpanId( "bd429c44b67a3eb2", @@ -740,6 +739,7 @@ expression: "(&event.value().unwrap().spans, metrics)" segment_id: ~, is_segment: ~, status: ~, + description: "www.example.com", tags: ~, origin: ~, profile_id: ~, diff --git a/relay-server/src/metrics_extraction/transactions/mod.rs b/relay-server/src/metrics_extraction/transactions/mod.rs index 4df374bd7e..d57fc1dae3 100644 --- a/relay-server/src/metrics_extraction/transactions/mod.rs +++ b/relay-server/src/metrics_extraction/transactions/mod.rs @@ -641,7 +641,6 @@ mod tests { 2020-08-21T02:18:20Z, ), exclusive_time: 2000.0, - description: "", op: "react.mount", span_id: SpanId( "bd429c44b67a3eb4", @@ -655,6 +654,7 @@ mod tests { segment_id: ~, is_segment: ~, status: ~, + description: "", tags: ~, origin: ~, profile_id: ~, diff --git a/relay-spans/src/span.rs b/relay-spans/src/span.rs index d043efbbf1..507e868f56 100644 --- a/relay-spans/src/span.rs +++ b/relay-spans/src/span.rs @@ -602,7 +602,6 @@ mod tests { 1970-01-01T00:02:03Z, ), exclusive_time: 500.0, - description: ~, op: "myop", span_id: SpanId( "fa90fdead5f74052", @@ -618,6 +617,7 @@ mod tests { ), is_segment: ~, status: Ok, + description: ~, tags: ~, origin: ~, profile_id: EventId(