fix(eap): Normalize segment names for standalone spans#6163
Conversation
![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 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2efc5b5. Configure here.
| attributes | ||
| .0 | ||
| .insert(SENTRY__SEGMENT__NAME.to_owned(), attribute); | ||
| } |
There was a problem hiding this comment.
Duplicate transaction left unnormalized
Medium Severity
normalize_segment_name only rewrites sentry.segment.name, while write_legacy_attributes skips copying into sentry.transaction when that key already exists. Legacy V1→V2 conversion can populate both from data and sentry_tags, so one attribute can stay high-cardinality while the other is scrubbed.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2efc5b5. Configure here.
| /// by running [`normalize_transaction_name`] on it. | ||
| /// | ||
| /// This exists for parity with the legacy standalone span pipeline. | ||
| pub fn normalize_segment_name( |
There was a problem hiding this comment.
The code in this function is tortous and ugly because I need to get from an attribute to an Annotated<String> for normalize_transaction_name. Suggestions for improvements are very welcome.
| // V2 span without a name it's just invalid. | ||
| infer_name: false, | ||
| clear_web_vital_segment_info: false, | ||
| ..Default::default() |
There was a problem hiding this comment.
Using Default here instead of adding yet another false. I'm wondering if this normalization maybe should run for V2 spans though.
There was a problem hiding this comment.
Let's only add stuff when we actually need it, basically impossible to remove anything we add later on.
| } | ||
|
|
||
| assert spans_consumer.get_span() == { | ||
| "_meta": mock.ANY, |
There was a problem hiding this comment.
I'm mocking the _meta here because it differs slightly between the legacy and V2 pipelines, in an uninteresting way (attributes in V2 have an extra level of nesting for the value).
|
|
||
| // SENTRY__SEGMENT__NAME must exist and be a string. | ||
|
|
||
| let Some(annotated_attr) = attributes.remove(SENTRY__SEGMENT__NAME) else { |
There was a problem hiding this comment.
Seems easier if you just create a new temporary Annotated<String> and then later replace the value.
There was a problem hiding this comment.
It does. I think I was having tunnel vision about not cloning the existing value.
| // V2 span without a name it's just invalid. | ||
| infer_name: false, | ||
| clear_web_vital_segment_info: false, | ||
| ..Default::default() |
There was a problem hiding this comment.
Let's only add stuff when we actually need it, basically impossible to remove anything we add later on.
|
I am also not sure if this is something we should port at all, this is likely just been put in place for the future but not relevant for web vitals (actual usage). |
2 participants
In the legacy standalone span pipeline we run a normalization that scrubs unique identifiers out of transaction names and also applies rules from the project config to them. This PR enables the same normalization for the experimental V2 pipeline (but not for V2 spans in general).
ref: INGEST-943