From a92a8d28677b6bf39b4e69cc2c69d5e620feac9e Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Tue, 19 May 2026 16:23:39 +0200 Subject: [PATCH 1/5] Add attribute_info_with_fragment function --- relay-conventions/src/lib.rs | 45 +++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/relay-conventions/src/lib.rs b/relay-conventions/src/lib.rs index 96e5b403c9a..c2f3d443924 100644 --- a/relay-conventions/src/lib.rs +++ b/relay-conventions/src/lib.rs @@ -121,10 +121,18 @@ pub struct AttributeInfo { } /// Returns information about an attribute, as defined in `sentry-conventions`. -pub fn attribute_info(key: &str) -> Option<&'static AttributeInfo> { +/// +/// If the matched attribute contains a placeholder (``), the second returned +/// value is the part of the attribute key that was inserted for the placeholder. +pub fn attribute_info_with_fragment(key: &str) -> Option<(&'static AttributeInfo, Option<&str>)> { ATTRIBUTES.find(key) } +/// Returns information about an attribute, as defined in `sentry-conventions`. +pub fn attribute_info(key: &str) -> Option<&'static AttributeInfo> { + attribute_info_with_fragment(key).map(|(info, _)| info) +} + /// Special path segment in attribute keys that matches any value. const PLACEHOLDER_SEGMENT: &str = ""; @@ -134,19 +142,29 @@ struct Node { } impl Node { - fn find(&self, key: &str) -> Option<&T> { + fn find<'a>(&self, key: &'a str) -> Option<(&T, Option<&'a str>)> { if key.is_empty() { - return self.info.as_ref(); + return self.info.as_ref().map(|info| (info, None)); } let (prefix, suffix) = key.split_once('.').unwrap_or((key, "")); - for candidate in [prefix, PLACEHOLDER_SEGMENT] { - if let Some(info) = self - .children - .get(candidate) - .and_then(|child| child.find(suffix)) - { - return Some(info); - } + + // First try a literal lookup. + if let Some(info) = self + .children + .get(prefix) + .and_then(|child| child.find(suffix)) + { + return Some(info); + } + + // If the literal lookup doesn't succeed, try a placeholder + // lookup and bubble up the current `prefix` if it succeeds. + if let Some((info, _)) = self + .children + .get(PLACEHOLDER_SEGMENT) + .and_then(|child| child.find(suffix)) + { + return Some((info, Some(prefix))); } None } @@ -182,7 +200,8 @@ mod tests { #[test] fn test_url_path_parameter() { // See https://github.com/getsentry/sentry-conventions/blob/d80504a40ba3a0a23eb746e2608425cf8d8e68bf/model/attributes/url/url__path__parameter__%5Bkey%5D.json. - let info = attribute_info("url.path.parameter.'id=123'").unwrap(); + let (info, fragment) = attribute_info_with_fragment("url.path.parameter.'id=123'").unwrap(); + assert_eq!(fragment, Some("'id=123'")); insta::assert_debug_snapshot!(info, @r###" AttributeInfo { @@ -216,7 +235,7 @@ mod tests { #[test] fn test_hypothetical() { - assert_eq!(ROOT.find("foo.bar"), Some(&2)); + assert_eq!(ROOT.find("foo.bar"), Some((&2, Some("foo")))); } struct GetterMap<'a>(HashMap<&'a str, Val<'a>>); From fa2bedecd8c2f30d6e54b620470bece4fdd835e7 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Wed, 20 May 2026 17:47:40 +0200 Subject: [PATCH 2/5] Handle placeholders in normalization --- relay-conventions/build/attributes.rs | 43 +++++- relay-conventions/build/build.rs | 7 +- relay-conventions/src/lib.rs | 51 ++++++- relay-event-normalization/src/eap/mod.rs | 166 +++++++++++++++++---- tests/integration/test_spans_standalone.py | 7 +- 5 files changed, 233 insertions(+), 41 deletions(-) diff --git a/relay-conventions/build/attributes.rs b/relay-conventions/build/attributes.rs index 7f6c56333f3..929b001b65e 100644 --- a/relay-conventions/build/attributes.rs +++ b/relay-conventions/build/attributes.rs @@ -72,6 +72,36 @@ pub struct Attribute { pub alias: Vec, } +/// Sanity check: if an attribute +/// * is deprecated, +/// * has status "backfill" or "normalize", +/// * and it contains a placeholder while its replacement doesn't, or vice versa, +/// +/// then we panic. There is no general way to normalize such attributes, so it's +/// better to reject them at compile time. +pub fn check_attribute(attribute: &Attribute) { + let Attribute { + key, + brief: _, + pii: _, + deprecation, + alias: _, + } = attribute; + + if let Some(deprecation) = deprecation + && let Some(replacement) = deprecation.replacement.as_ref() + && deprecation.status.is_some() + { + let attribute_contains_placeholder = key.contains(""); + let replacement_contains_placeholder = replacement.contains(""); + + assert_eq!( + attribute_contains_placeholder, replacement_contains_placeholder, + r"One of attribute `{key}` and its replacement `{replacement}` contains a placeholder and the other doesn't. Such attributes can't be automatically backfilled or normalized by Relay." + ) + } +} + /// Formats an attribute's deprecation information as a `WriteBehavior`. fn format_write_behavior(deprecation: Option<&Deprecation>) -> String { let Some((status, replacement)) = @@ -80,12 +110,21 @@ fn format_write_behavior(deprecation: Option<&Deprecation>) -> String { return "WriteBehavior::CurrentName".to_owned(); }; + let name = if replacement.contains("") { + format!( + "ReplacementName::Dynamic(crate::interpolate::{})", + name_fn(replacement) + ) + } else { + format!("ReplacementName::Static({replacement:?})") + }; + match status { DeprecationStatus::Backfill => { - format!("WriteBehavior::BothNames({:?})", replacement) + format!("WriteBehavior::BothNames({name})") } DeprecationStatus::Normalize => { - format!("WriteBehavior::NewName({:?})", replacement) + format!("WriteBehavior::NewName({name})") } } } diff --git a/relay-conventions/build/build.rs b/relay-conventions/build/build.rs index 9d85cd61fc3..1855f347b99 100644 --- a/relay-conventions/build/build.rs +++ b/relay-conventions/build/build.rs @@ -26,10 +26,7 @@ fn main() { } fn write_attribute_rs(crate_dir: &Path) { - use attributes::{ - Attribute, RawNode, constant_pair, format_attribute_info, format_constant, - format_interpolating_fn, parse_segments, write_canonical_fn, - }; + use attributes::*; let attribute_consts_path = Path::new(&env::var("OUT_DIR").unwrap()).join("attribute_consts.rs"); @@ -52,6 +49,8 @@ fn write_attribute_rs(crate_dir: &Path) { let contents = std::fs::read_to_string(file.path()).unwrap(); let attr: Attribute = serde_json::from_str(&contents).unwrap(); + check_attribute(&attr); + // Write attribute constant writeln!(&mut attribute_consts_file, "{}\n", format_constant(&attr)).unwrap(); diff --git a/relay-conventions/src/lib.rs b/relay-conventions/src/lib.rs index c2f3d443924..57bdc50f6eb 100644 --- a/relay-conventions/src/lib.rs +++ b/relay-conventions/src/lib.rs @@ -95,6 +95,29 @@ pub enum Pii { Maybe, } +/// The name of the replacement of a deprecated attribute. +#[derive(Clone, Copy)] +pub enum ReplacementName { + /// The replacement attribute has a fixed name, + /// i.e., doesn't contain a placeholder. + Static(&'static str), + /// The replacement attribute contains a placeholder. + /// + /// This means its name can't be used "as-is"; a value + /// has to be inserted into the placeholder. The contained + /// function performs this insertion. + Dynamic(fn(&str) -> String), +} + +impl fmt::Debug for ReplacementName { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Static(arg0) => f.debug_tuple("Static").field(arg0).finish(), + Self::Dynamic(_) => f.debug_tuple("Dynamic").finish(), + } + } +} + /// Under which names an attribute should be saved. #[derive(Debug, Clone, Copy)] pub enum WriteBehavior { @@ -103,10 +126,10 @@ pub enum WriteBehavior { /// This is the only choice for attributes that aren't deprecated. CurrentName, /// Save the attribute under its replacement name instead. - NewName(&'static str), + NewName(ReplacementName), /// Save the attribute under both its current name and /// its replacement name. - BothNames(&'static str), + BothNames(ReplacementName), } /// Information about an attribute, as defined in `sentry-conventions`. @@ -183,10 +206,12 @@ mod tests { fn test_http_response_content_length() { let info = attribute_info("http.response_content_length").unwrap(); - insta::assert_debug_snapshot!(info, @r#" + insta::assert_debug_snapshot!(info, @r###" AttributeInfo { write_behavior: BothNames( - "http.response.body.size", + Static( + "http.response.body.size", + ), ), pii: Maybe, aliases: [ @@ -194,7 +219,7 @@ mod tests { "http.response.header.content-length", ], } - "#); + "###); } #[test] @@ -214,6 +239,22 @@ mod tests { "###); } + /// Tests that `cls.source.` is rewritten to `browser.web_vital.cls.source.`. + #[test] + fn test_cls_source_key() { + let (info, fragment) = attribute_info_with_fragment("cls.source.foobar").unwrap(); + + let WriteBehavior::BothNames(ReplacementName::Dynamic(name_fn)) = info.write_behavior + else { + unreachable!(); + }; + + assert_eq!( + name_fn(fragment.unwrap()), + "browser.web_vital.cls.source.foobar" + ); + } + const ROOT: Node = Node { info: None, children: phf_map! { diff --git a/relay-event-normalization/src/eap/mod.rs b/relay-event-normalization/src/eap/mod.rs index dc298e87e6c..e5605b6444c 100644 --- a/relay-event-normalization/src/eap/mod.rs +++ b/relay-event-normalization/src/eap/mod.rs @@ -7,8 +7,8 @@ use std::net::IpAddr; use chrono::{DateTime, Utc}; use relay_common::time::UnixTimestamp; -use relay_conventions::consts::*; use relay_conventions::{AttributeInfo, WriteBehavior}; +use relay_conventions::{ReplacementName, consts::*}; use relay_event_schema::protocol::{AttributeType, Attributes, BrowserContext, Geo}; use relay_protocol::{Annotated, ErrorKind, Meta, Remark, RemarkType, Value}; use relay_sampling::DynamicSamplingContext; @@ -396,12 +396,14 @@ pub fn normalize_dsc( /// Attributes with a status of `"backfill"` will be copied to their replacement name if the /// replacement name is not present. In any case, the original name is left alone. pub fn normalize_attribute_names(attributes: &mut Annotated) { - normalize_attribute_names_inner(attributes, relay_conventions::attribute_info) + normalize_attribute_names_inner(attributes, relay_conventions::attribute_info_with_fragment) } +type AttributeInfoFn = fn(&str) -> Option<(&'static AttributeInfo, Option<&str>)>; + fn normalize_attribute_names_inner( attributes: &mut Annotated, - attribute_info: fn(&str) -> Option<&'static AttributeInfo>, + attribute_info: AttributeInfoFn, ) { let Some(attributes) = attributes.value_mut() else { return; @@ -410,7 +412,7 @@ fn normalize_attribute_names_inner( let attribute_names: Vec<_> = attributes.0.keys().cloned().collect(); for name in attribute_names { - let Some(attribute_info) = attribute_info(&name) else { + let Some((attribute_info, fragment)) = attribute_info(&name) else { continue; }; @@ -421,26 +423,54 @@ fn normalize_attribute_names_inner( continue; }; + let new_name = resolve_attribute_name(new_name, fragment); + let mut meta = Meta::default(); // TODO: Possibly add a new RemarkType for "renamed/moved" meta.add_remark(Remark::new(RemarkType::Removed, "attribute.deprecated")); let new_attribute = std::mem::replace(old_attribute, Annotated(None, meta)); - if !attributes.contains_key(new_name) { - attributes.0.insert(new_name.to_owned(), new_attribute); + if !attributes.contains_key(&*new_name) { + attributes.0.insert(new_name.into_owned(), new_attribute); } } WriteBehavior::BothNames(new_name) => { - if !attributes.contains_key(new_name) + let new_name = resolve_attribute_name(new_name, fragment); + if !attributes.contains_key(&*new_name) && let Some(current_attribute) = attributes.0.get(&name).cloned() { - attributes.0.insert(new_name.to_owned(), current_attribute); + attributes + .0 + .insert(new_name.into_owned(), current_attribute); } } } } } +/// Resolves the name of a replacement attribute for rewriting. +/// +/// There are two cases to consider: +/// - `name` is `Static` and `fragment` is `None`: This means that both +/// the source and target attribute don't have a placeholder. +/// - `name` is `Dynamic` and `fragment` is `Some`: This means that both the +/// source and target attribute do have a placeholder. +fn resolve_attribute_name(name: ReplacementName, fragment: Option<&str>) -> Cow<'static, str> { + match (name, fragment) { + // Neither the original nor the replacement attribute contains a placeholder. + // Simply use the replacement attribute's static name. + (ReplacementName::Static(name), None) => Cow::Borrowed(name), + // Both the original and replacement attribute contain a placeholder. + // Use the replacement's interpolation function and the matched fragment + // to obtain the new name. + (ReplacementName::Dynamic(name_fn), Some(fragment)) => Cow::Owned(name_fn(fragment)), + // The other cases would mean that either the original attribute contains a placeholder + // and the replacement doesn't, or vice versa. This is ruled out by a compile-time check + // in `relay-conventions`. + _ => unreachable!(), + } +} + /// Normalizes the values of a set of attributes if present in the span. /// /// Each span type has a set of important attributes containing the main relevant information displayed @@ -1252,30 +1282,77 @@ mod tests { #[test] fn test_normalize_attributes() { - fn mock_attribute_info(name: &str) -> Option<&'static AttributeInfo> { + fn replace_key(fragment: &str) -> String { + format!("placeholder.replaced.{fragment}") + } + + fn backfill_key(fragment: &str) -> String { + format!("placeholder.backfilled.{fragment}") + } + + fn mock_attribute_info(name: &str) -> Option<(&'static AttributeInfo, Option<&str>)> { use relay_conventions::Pii; match name { - "replace.empty" => Some(&AttributeInfo { - write_behavior: WriteBehavior::NewName("replaced"), - pii: Pii::Maybe, - aliases: &["replaced"], - }), - "replace.existing" => Some(&AttributeInfo { - write_behavior: WriteBehavior::NewName("not.replaced"), - pii: Pii::Maybe, - aliases: &["not.replaced"], - }), - "backfill.empty" => Some(&AttributeInfo { - write_behavior: WriteBehavior::BothNames("backfilled"), - pii: Pii::Maybe, - aliases: &["backfilled"], - }), - "backfill.existing" => Some(&AttributeInfo { - write_behavior: WriteBehavior::BothNames("not.backfilled"), - pii: Pii::Maybe, - aliases: &["not.backfilled"], - }), + "replace.empty" => Some(( + &AttributeInfo { + write_behavior: WriteBehavior::NewName(ReplacementName::Static("replaced")), + pii: Pii::Maybe, + aliases: &["replaced"], + }, + None, + )), + "replace.existing" => Some(( + &AttributeInfo { + write_behavior: WriteBehavior::NewName(ReplacementName::Static( + "not.replaced", + )), + pii: Pii::Maybe, + aliases: &["not.replaced"], + }, + None, + )), + "backfill.empty" => Some(( + &AttributeInfo { + write_behavior: WriteBehavior::BothNames(ReplacementName::Static( + "backfilled", + )), + pii: Pii::Maybe, + aliases: &["backfilled"], + }, + None, + )), + "backfill.existing" => Some(( + &AttributeInfo { + write_behavior: WriteBehavior::BothNames(ReplacementName::Static( + "not.backfilled", + )), + pii: Pii::Maybe, + aliases: &["not.backfilled"], + }, + None, + )), + _ if let Some(fragment) = name.strip_prefix("placeholder.replace.") => Some(( + &AttributeInfo { + write_behavior: WriteBehavior::NewName(ReplacementName::Dynamic( + replace_key, + )), + pii: Pii::Maybe, + aliases: &["placeholder.replaced."], + }, + Some(fragment), + )), + _ if let Some(fragment) = name.strip_prefix("placeholder.backfill.") => Some(( + &AttributeInfo { + write_behavior: WriteBehavior::BothNames(ReplacementName::Dynamic( + backfill_key, + )), + pii: Pii::Maybe, + aliases: &["placeholder.backfilled."], + }, + Some(fragment), + )), + _ => None, } } @@ -1289,6 +1366,10 @@ mod tests { "replace.existing".to_owned(), Annotated::new("Should be removed".to_owned().into()), ), + ( + "placeholder.replace.foo".to_owned(), + Annotated::new("Should be moved".to_owned().into()), + ), ( "not.replaced".to_owned(), Annotated::new("Should be left alone".to_owned().into()), @@ -1301,6 +1382,10 @@ mod tests { "backfill.existing".to_owned(), Annotated::new("Should be left alone".to_owned().into()), ), + ( + "placeholder.backfill.bar".to_owned(), + Annotated::new("Should be copied".to_owned().into()), + ), ( "not.backfilled".to_owned(), Annotated::new("Should be left alone".to_owned().into()), @@ -1331,6 +1416,19 @@ mod tests { "type": "string", "value": "Should be left alone" }, + "placeholder.backfill.bar": { + "type": "string", + "value": "Should be copied" + }, + "placeholder.backfilled.bar": { + "type": "string", + "value": "Should be copied" + }, + "placeholder.replace.foo": null, + "placeholder.replaced.foo": { + "type": "string", + "value": "Should be moved" + }, "replace.empty": null, "replace.existing": null, "replaced": { @@ -1338,6 +1436,16 @@ mod tests { "value": "Should be moved" }, "_meta": { + "placeholder.replace.foo": { + "": { + "rem": [ + [ + "attribute.deprecated", + "x" + ] + ] + } + }, "replace.empty": { "": { "rem": [ diff --git a/tests/integration/test_spans_standalone.py b/tests/integration/test_spans_standalone.py index 74514bf5d77..58261ce666c 100644 --- a/tests/integration/test_spans_standalone.py +++ b/tests/integration/test_spans_standalone.py @@ -417,10 +417,15 @@ def test_cls_span( if mode == "v2": cls_backfill = { "browser.web_vital.cls.value": {"type": "double", "value": 0.1}, - "browser.web_vital.cls.source.": { + "browser.web_vital.cls.source.1": { "type": "string", "value": "AppContainer > NavContent > MobileTopbar > StyledButton", }, + "browser.web_vital.cls.source.2": { + "type": "string", + "value": "div.app-1azrk9k.etjky0h0 > AppContainer > BodyContainer > BaseFooter", + }, + "browser.web_vital.cls.source.3": {"type": "string", "value": ""}, } assert spans_consumer.get_span() == { From 91d16cb6f94c112527ce6288adf8fbc0ba07581d Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Wed, 20 May 2026 18:47:15 +0200 Subject: [PATCH 3/5] Be more defensive --- relay-conventions/src/lib.rs | 11 +++++---- relay-event-normalization/src/eap/mod.rs | 30 +++++++++++++++++++----- 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/relay-conventions/src/lib.rs b/relay-conventions/src/lib.rs index 57bdc50f6eb..058b0014fa4 100644 --- a/relay-conventions/src/lib.rs +++ b/relay-conventions/src/lib.rs @@ -172,10 +172,13 @@ impl Node { let (prefix, suffix) = key.split_once('.').unwrap_or((key, "")); // First try a literal lookup. - if let Some(info) = self - .children - .get(prefix) - .and_then(|child| child.find(suffix)) + // If the prefix is `""`, we skip this and fall through + // to the second attempt. + if prefix != PLACEHOLDER_SEGMENT + && let Some(info) = self + .children + .get(prefix) + .and_then(|child| child.find(suffix)) { return Some(info); } diff --git a/relay-event-normalization/src/eap/mod.rs b/relay-event-normalization/src/eap/mod.rs index e5605b6444c..ade28bbfc7a 100644 --- a/relay-event-normalization/src/eap/mod.rs +++ b/relay-event-normalization/src/eap/mod.rs @@ -423,7 +423,14 @@ fn normalize_attribute_names_inner( continue; }; - let new_name = resolve_attribute_name(new_name, fragment); + let Some(new_name) = resolve_attribute_name(new_name, fragment) else { + relay_log::error!( + attribute = name, + ?fragment, + "Attribute placeholder mismatch" + ); + continue; + }; let mut meta = Meta::default(); // TODO: Possibly add a new RemarkType for "renamed/moved" @@ -435,7 +442,15 @@ fn normalize_attribute_names_inner( } } WriteBehavior::BothNames(new_name) => { - let new_name = resolve_attribute_name(new_name, fragment); + let Some(new_name) = resolve_attribute_name(new_name, fragment) else { + relay_log::error!( + attribute = name, + ?fragment, + "Attribute placeholder mismatch" + ); + continue; + }; + if !attributes.contains_key(&*new_name) && let Some(current_attribute) = attributes.0.get(&name).cloned() { @@ -455,19 +470,22 @@ fn normalize_attribute_names_inner( /// the source and target attribute don't have a placeholder. /// - `name` is `Dynamic` and `fragment` is `Some`: This means that both the /// source and target attribute do have a placeholder. -fn resolve_attribute_name(name: ReplacementName, fragment: Option<&str>) -> Cow<'static, str> { +fn resolve_attribute_name( + name: ReplacementName, + fragment: Option<&str>, +) -> Option> { match (name, fragment) { // Neither the original nor the replacement attribute contains a placeholder. // Simply use the replacement attribute's static name. - (ReplacementName::Static(name), None) => Cow::Borrowed(name), + (ReplacementName::Static(name), None) => Some(Cow::Borrowed(name)), // Both the original and replacement attribute contain a placeholder. // Use the replacement's interpolation function and the matched fragment // to obtain the new name. - (ReplacementName::Dynamic(name_fn), Some(fragment)) => Cow::Owned(name_fn(fragment)), + (ReplacementName::Dynamic(name_fn), Some(fragment)) => Some(Cow::Owned(name_fn(fragment))), // The other cases would mean that either the original attribute contains a placeholder // and the replacement doesn't, or vice versa. This is ruled out by a compile-time check // in `relay-conventions`. - _ => unreachable!(), + _ => None, } } From 11764be2edd12447176784988e5a8874401acc1c Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Wed, 20 May 2026 18:51:01 +0200 Subject: [PATCH 4/5] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b89549f1f8..0294ceff3bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - Apply timestamp validations to transaction spans. ([#6005](https://github.com/getsentry/relay/pull/6005)) - Obtain PII values for `SpanData` fields from `sentry-conventions`. ([#5997](https://github.com/getsentry/relay/pull/5997)) - Add `sentry.dsc.transaction` and `sentry.dsc.trace_id` to all spans. ([#6001](https://github.com/getsentry/relay/pull/6001), [#6004](https://github.com/getsentry/relay/pull/6004), [#6008](https://github.com/getsentry/relay/pull/6008)) +- Correctly handle attributes with placeholders during normalization. ([#6012](https://github.com/getsentry/relay/pull/6012)) ## 26.5.0 From ec9a8ff7b7a4678fb735fbd04c7bce937629a47d Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Wed, 20 May 2026 18:54:26 +0200 Subject: [PATCH 5/5] Add edge case test --- relay-conventions/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/relay-conventions/src/lib.rs b/relay-conventions/src/lib.rs index 1593ee53b98..c633230ba5c 100644 --- a/relay-conventions/src/lib.rs +++ b/relay-conventions/src/lib.rs @@ -287,6 +287,7 @@ mod tests { #[test] fn test_hypothetical() { assert_eq!(ROOT.find("foo.bar"), Some((&2, Some("foo")))); + assert_eq!(ROOT.find(".bar"), Some((&2, Some("")))); } struct GetterMap<'a>(HashMap<&'a str, Val<'a>>);