ref(spans): Modify segment attributes during conversion#6126
ref(spans): Modify segment attributes during conversion#6126loewenheim wants to merge 5 commits into
Conversation
Refactor of #6042. Instead of modifying `is_segment` and related fields/attributes as a span V2 normalization step, we do it when we convert from V1 to V2 in the new standalone span pipeline. To make this possible, the core of the function that already does this for the legacy pipeline is extracted to `relay-spans`. This PR actually does slightly more than move logic from #6042 around: it also ports some other small normalizations from the legacy pipeline to the V2 one. I had these noted down to investigate anyway, so this takes care of them in one fell swoop.
| 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(); | ||
| } |
There was a problem hiding this comment.
These normalizations would previously happen in the legacy standalone span pipeline, but not the V2 one.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Dav1dde
left a comment
Dav1dde
left a comment
There was a problem hiding this comment.
I think you wanted to port over the test from Dynamic Sampling as well?
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3e2bd9a. Configure here.
| // 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); |
There was a problem hiding this comment.
Native V2 web vital segments
Medium Severity
Removing normalize_web_vital_span_segment from shared V2 normalize_span leaves native Span V2 ingest (container items and similar paths) with no step that clears segment fields for ui.webvital.* / ui.interaction.* ops, because set_segment_attributes only runs when expanding legacy V1 JSON.
Reviewed by Cursor Bugbot for commit 3e2bd9a. Configure here.
There was a problem hiding this comment.
This is intentional. In V2 we simply require that SDKs set the is_segment field correctly.
| // 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) | ||
| }); |
There was a problem hiding this comment.
Bug: For legacy web vital spans, sentry.segment.id from the V1 data field can leak into V2 attributes because the explicit removal logic was removed.
Severity: LOW
Suggested Fix
Explicitly remove the SENTRY__SEGMENT__ID attribute from the V2 attributes of web vital spans after the initial conversion from the V1 data field. This ensures the attribute is stripped regardless of its source, restoring the intended behavior.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: relay-server/src/processing/spans/process.rs#L171-L175
Potential issue: The logic for converting legacy (V1) web vital spans to V2 format no
longer explicitly removes the `sentry.segment.id` attribute. If a V1 web vital span
contains `sentry.segment.id` within its `data` payload, this attribute is converted and
added to the V2 attributes. The new logic attempts to clear the top-level `segment_id`,
but this does not affect the attribute already present from the `data` field. As a
result, the `sentry.segment.id` attribute incorrectly remains on the V2 span, which
contradicts the intended behavior of stripping it from web vital spans.
Also affects:
relay-spans/src/segment.rs:1~37
2 participants
Refactor of #6042. Instead of modifying
is_segmentand related fields/attributes as a span V2 normalization step, we do it when we convert from V1 to V2 in the new standalone span pipeline. To make this possible, the core of the function that already does this for the legacy pipeline is extracted torelay-spans.This PR actually does slightly more than move logic from #6042 around: it also ports some other small normalizations from the legacy pipeline to the V2 one. I had these noted down to investigate anyway, so this takes care of them in one fell swoop.