Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
- Unset segment info for web vital spans. ([#6042](https://github.com/getsentry/relay/pull/6042))
- Set sentry.trace.status on segment spans. ([#6140](https://github.com/getsentry/relay/pull/6140))
- Don't modify segment information for V2 web vital spans. ([#6160](https://github.com/getsentry/relay/pull/6160))
- Normalize segment names for standalone spans. ([#6163](https://github.com/getsentry/relay/pull/6163))

**Internal**:

Expand Down
31 changes: 30 additions & 1 deletion relay-event-normalization/src/eap/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ use crate::span::tag_extraction::{
domain_from_scrubbed_http, domain_from_server_address, span_op_to_category,
sql_action_from_query, sql_tables_from_query,
};
use crate::{ClientHints, FromUserAgentInfo as _, RawUserAgentInfo};
use crate::{
ClientHints, FromUserAgentInfo as _, RawUserAgentInfo, TransactionNameRule,
normalize_transaction_name,
};

mod ai;
mod mobile;
Expand Down Expand Up @@ -849,6 +852,32 @@ pub fn normalize_web_vital_span_segment(span: &mut SpanV2) {
}
}

/// Normalize the [`SENTRY__SEGMENT__NAME`] attribute (aka the transaction)
/// by running [`normalize_transaction_name`] on it.
///
/// This exists for parity with the legacy standalone span pipeline.
pub fn normalize_segment_name(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

attributes: &mut Annotated<Attributes>,
tx_name_rules: &[TransactionNameRule],
) {
let Some(attributes) = attributes.value_mut() else {
return;
};

let Some(attr_value) = attributes.get_annotated_value_mut(SENTRY__SEGMENT__NAME) else {
return;
};

let mut segment_name = match &attr_value.0 {
Some(Value::String(s)) => Annotated(Some(s.to_owned()), attr_value.1.clone()),
_ => return,
};

normalize_transaction_name(&mut segment_name, tx_name_rules);

*attr_value = segment_name.map_value(Value::String);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2efc5b5. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ughhh.


/// Double writes sentry conventions attributes into legacy attributes.
///
/// This achieves backwards compatibility as it allows products to continue using legacy attributes
Expand Down
9 changes: 9 additions & 0 deletions relay-event-schema/src/protocol/attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,15 @@ impl Attributes {
Some(&self.0.get(key)?.value()?.value.value)
}

/// Returns the mutable attribute value as annotated.
pub fn get_annotated_value_mut<Q>(&mut self, key: &Q) -> Option<&mut Annotated<Value>>
where
String: Borrow<Q>,
Q: Ord + ?Sized,
{
Some(&mut self.0.get_mut(key)?.value_mut().as_mut()?.value.value)
}

/// Inserts an attribute with the given value into this collection.
pub fn insert<K: Into<String>, V: Into<AttributeValue>>(&mut self, key: K, value: V) {
fn inner(slf: &mut Attributes, key: String, value: AttributeValue) {
Expand Down
3 changes: 3 additions & 0 deletions relay-server/src/processing/spans/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,9 @@ struct Settings {
/// with the legacy pipeline. For V2 spans, we assume the SDK is already
/// sending the correct values.
clear_web_vital_segment_info: bool,
/// Normalize the segment name by scrubbing identifiers and applying rules
/// from the project config.
normalize_segment_name: bool,
}

/// Spans which have been parsed and expanded from their serialized state.
Expand Down
12 changes: 8 additions & 4 deletions relay-server/src/processing/spans/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,7 @@ fn expand_span_container(item: &Item) -> Result<(Settings, ContainerItems<SpanV2
infer_user_agent: is
.and_then(|is| is.infer_user_agent)
.is_some_and(|infer| infer.is_auto()),
// We don't want to infer names for V2 spans. If an SDK sent a
// V2 span without a name it's just invalid.
infer_name: false,
clear_web_vital_segment_info: false,
..Default::default()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Default here instead of adding yet another false. I'm wondering if this normalization maybe should run for V2 spans though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's only add stuff when we actually need it, basically impossible to remove anything we add later on.

},
// Unsupported, fall back to the safe default.
Some(_) => Default::default(),
Expand Down Expand Up @@ -155,6 +152,9 @@ fn expand_legacy_spans(
// We want to do this for V1 standalone spans for parity
// with the legacy pipeline.
clear_web_vital_segment_info: true,
// We want to do this for V1 standalone spans for parity
// with the legacy pipeline.
normalize_segment_name: true,
};

(settings, spans)
Expand Down Expand Up @@ -229,6 +229,7 @@ fn normalize_span(
hints: meta.client_hints(),
});
let performance_score = ctx.project_info.config().performance_score.as_ref();
let tx_name_rules = &ctx.project_info.config.tx_name_rules;

validate_timestamps(span)?;

Expand All @@ -241,6 +242,9 @@ fn normalize_span(
if settings.clear_web_vital_segment_info {
eap::normalize_web_vital_span_segment(span);
}
if settings.normalize_segment_name {
eap::normalize_segment_name(&mut span.attributes, tx_name_rules);
}
eap::normalize_span_category(&mut span.attributes);
eap::normalize_received(&mut span.attributes, meta.received_at());
eap::normalize_client_address(&mut span.attributes, meta.client_addr());
Expand Down
22 changes: 13 additions & 9 deletions tests/integration/test_spans_standalone.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from datetime import datetime, timezone
from unittest import mock

from sentry_sdk.envelope import Envelope, Item, PayloadRef

Expand Down Expand Up @@ -165,7 +166,7 @@ def test_lcp_span(
"release": "frontend@488531b11e6401fa530ac25554d44426e6ef0f0b",
"environment": "prod",
"replay_id": "3d76a6311de149b9b3f560827ea0ecf9",
"transaction": "/insights/projects/",
"transaction": "/insights/projects/b8686628-95f0-4be9-af6b-a98164504d8f",
"user_agent.original": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Firefox/42.0",
"client.address": "{{auto}}",
"sentry.exclusive_time": 0,
Expand Down Expand Up @@ -212,6 +213,7 @@ def test_lcp_span(
}

assert spans_consumer.get_span() == {
"_meta": mock.ANY,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

"attributes": {
"client.address": {"type": "string", "value": "127.0.0.1"},
"browser.web_vital.lcp.value": {"type": "double", "value": 548.0},
Expand Down Expand Up @@ -247,8 +249,8 @@ def test_lcp_span(
"value": "3d76a6311de149b9b3f560827ea0ecf9",
},
"sentry.report_event": {"type": "string", "value": "navigation"},
"sentry.segment.name": {"type": "string", "value": "/insights/projects/"},
"sentry.transaction": {"type": "string", "value": "/insights/projects/"},
"sentry.segment.name": {"type": "string", "value": "/insights/projects/*"},
"sentry.transaction": {"type": "string", "value": "/insights/projects/*"},
"user_agent.original": {
"type": "string",
"value": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) "
Expand Down Expand Up @@ -375,7 +377,7 @@ def test_cls_span(
"release": "frontend@488531b11e6401fa530ac25554d44426e6ef0f0b",
"environment": "prod",
"replay_id": "3d76a6311de149b9b3f560827ea0ecf9",
"transaction": "/insights/projects/",
"transaction": "/insights/projects/b8686628-95f0-4be9-af6b-a98164504d8f",
"user_agent.original": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Firefox/42.0",
"client.address": "{{auto}}",
"sentry.exclusive_time": 0,
Expand Down Expand Up @@ -423,6 +425,7 @@ def test_cls_span(
}

assert spans_consumer.get_span() == {
"_meta": mock.ANY,
"attributes": {
"client.address": {"type": "string", "value": "127.0.0.1"},
"browser.web_vital.cls.value": {"type": "double", "value": 0.1},
Expand Down Expand Up @@ -463,8 +466,8 @@ def test_cls_span(
"value": "3d76a6311de149b9b3f560827ea0ecf9",
},
"sentry.report_event": {"type": "string", "value": "navigation"},
"sentry.segment.name": {"type": "string", "value": "/insights/projects/"},
"sentry.transaction": {"type": "string", "value": "/insights/projects/"},
"sentry.segment.name": {"type": "string", "value": "/insights/projects/*"},
"sentry.transaction": {"type": "string", "value": "/insights/projects/*"},
"user_agent.original": {
"type": "string",
"value": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) "
Expand Down Expand Up @@ -590,7 +593,7 @@ def test_inp_span(
"release": "frontend@488531b11e6401fa530ac25554d44426e6ef0f0b",
"environment": "prod",
"replay_id": "3d76a6311de149b9b3f560827ea0ecf9",
"transaction": "/insights/projects/",
"transaction": "/insights/projects/b8686628-95f0-4be9-af6b-a98164504d8f",
"user_agent.original": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Firefox/42.0",
"client.address": "{{auto}}",
"sentry.exclusive_time": 104,
Expand Down Expand Up @@ -624,6 +627,7 @@ def test_inp_span(
}

assert spans_consumer.get_span() == {
"_meta": mock.ANY,
"attributes": {
"client.address": {"type": "string", "value": "127.0.0.1"},
"browser.web_vital.inp.value": {"type": "double", "value": 104.0},
Expand Down Expand Up @@ -653,8 +657,8 @@ def test_inp_span(
"type": "string",
"value": "3d76a6311de149b9b3f560827ea0ecf9",
},
"sentry.segment.name": {"type": "string", "value": "/insights/projects/"},
"sentry.transaction": {"type": "string", "value": "/insights/projects/"},
"sentry.segment.name": {"type": "string", "value": "/insights/projects/*"},
"sentry.transaction": {"type": "string", "value": "/insights/projects/*"},
"user_agent.original": {
"type": "string",
"value": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) "
Expand Down