Skip to content

Commit

Permalink
fix(spans): Mark all fields untrimmable (#3890)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jjbayer authored Aug 2, 2024
1 parent 35bdc67 commit 871ba75
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 41 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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**:
Expand Down
39 changes: 7 additions & 32 deletions relay-event-normalization/src/trimming.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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(),
);
}
}
}
23 changes: 14 additions & 9 deletions relay-event-schema/src/protocol/span.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<f64>,

/// Span type (see `OperationType` docs).
Expand Down Expand Up @@ -54,61 +55,65 @@ pub struct Span {
pub is_segment: Annotated<bool>,

/// The status of a span.
#[metastructure(trim = "false")]
pub status: Annotated<SpanStatus>,

/// Human readable description of a span (e.g. method URL).
#[metastructure(pii = "maybe")]
#[metastructure(pii = "maybe", trim = "false")]
pub description: Annotated<String>,

/// Arbitrary tags on a span, like on the top-level event.
#[metastructure(pii = "maybe")]
#[metastructure(pii = "maybe", trim = "false")]
pub tags: Annotated<Object<JsonLenientString>>,

/// 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<OriginType>,

/// ID of a profile that can be associated with the span.
#[metastructure(trim = "false")]
pub profile_id: Annotated<EventId>,

/// Arbitrary additional data on a span.
///
/// Besides arbitrary user data, this object also contains SDK-provided fields used by the
/// product (see <https://develop.sentry.dev/sdk/performance/span-data-conventions/>).
#[metastructure(pii = "true")]
#[metastructure(pii = "true", trim = "false")]
pub data: Annotated<SpanData>,

/// Tags generated by Relay. These tags are a superset of the tags set on span metrics.
#[metastructure(trim = "false")]
pub sentry_tags: Annotated<Object<String>>,

/// Timestamp when the span has been received by Sentry.
#[metastructure(trim = "false")]
pub received: Annotated<Timestamp>,

/// 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<Measurements>,

/// Temporary protocol support for metric summaries.
///
/// 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<MetricsSummary>,

/// Platform identifier.
///
/// See [`Event::platform`](`crate::protocol::Event::platform`).
#[metastructure(skip_serialization = "empty")]
#[metastructure(skip_serialization = "empty", trim = "false")]
pub platform: Annotated<String>,

/// 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<bool>,

// 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<Value>,
}

Expand Down

0 comments on commit 871ba75

Please sign in to comment.