feat(txname): Mark all URL transactions as sanitized (#2210)

In #2139, we transitioned to marking all URL transactions as "sanitized" once sentry provides the "ready" flag. However, this results in a bad onboarding experience for new projects because they still see `<< unparameterized >>` for the first ~10 hours. This PR removes the "ready" condition, such that _all_ transactions with source URL will now get their transaction name added as a metric tag. Since we were already doing this for all but the newest projects, this should not add much cardinality after all. But we have to keep an eye on the cardinality limiter in the first few hours / days after deploying this. Fixes #2186 --------- Co-authored-by: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
getsentry · Jun 13, 2023 · c057e37 · c057e37
1 parent 94c2a3a
commit c057e37
Show file tree

Hide file tree

Showing 4 changed files with 13 additions and 37 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,6 +17,7 @@
 **Internal**:
 
 - Extract app identifier from app context for profiles. ([#2172](https://github.com/getsentry/relay/pull/2172))
+- Mark all URL transactions as sanitized after applying rules. ([#2210](https://github.com/getsentry/relay/pull/2210))
 
 ## 23.5.2
 

diff --git a/relay-general/src/store/transactions/processor.rs b/relay-general/src/store/transactions/processor.rs
@@ -24,8 +24,6 @@ use crate::types::{
 pub struct TransactionNameConfig<'r> {
     /// Rules for identifier replacement that were discovered by Sentry's transaction clusterer.
     pub rules: &'r [TransactionNameRule],
-    /// True if URL transactions should be marked as sanitized, even if there are no rules.
-    pub ready: bool,
 }
 
 /// Rejects transactions based on required fields.
@@ -471,30 +469,30 @@ impl Processor for TransactionsProcessor<'_> {
                 .set_value(Some("<unlabeled transaction>".to_owned()))
         }
 
-        // If the project is marked as 'ready', always set the transaction source to sanitized.
-        let mut mark_as_sanitized = self.name_config.ready;
-
         if matches!(
             event.get_transaction_source(),
             &TransactionSource::Url | &TransactionSource::Sanitized
         ) {
             // Normalize transaction names for URLs and Sanitized transaction sources.
             // This in addition to renaming rules can catch some high cardinality parts.
-            scrub_identifiers(&mut event.transaction)?.then(|| {
-                mark_as_sanitized = true;
-            });
+            scrub_identifiers(&mut event.transaction)?;
         }
 
         if !self.name_config.rules.is_empty() {
             self.apply_transaction_rename_rule(
                 &mut event.transaction,
                 event.transaction_info.value_mut(),
             )?;
-
-            mark_as_sanitized = true;
         }
 
-        if mark_as_sanitized && matches!(event.get_transaction_source(), &TransactionSource::Url) {
+        if matches!(event.get_transaction_source(), &TransactionSource::Url) {
+            // Always mark URL transactions as sanitized, even if no modification were made by
+            // clusterer rules or regex matchers. This has the consequence that the transaction name
+            // is always extracted as a tag on transaction metrics.
+            // Instead of changing the source to "sanitized", we could have changed metrics extraction
+            // to also extract the transaction name for URL transactions. But this is the safer way,
+            // because the product currently uses queries that assume that `source:url` is equivalent
+            // to `transaction:<< unparameterized >>`.
             event
                 .transaction_info
                 .get_or_insert_with(Default::default)
@@ -1808,14 +1806,7 @@ mod tests {
 
         process_value(
             &mut event,
-            &mut TransactionsProcessor::new(
-                TransactionNameConfig {
-                    rules: &[],
-                    ready: true,
-                },
-                false,
-                None,
-            ),
+            &mut TransactionsProcessor::default(),
             ProcessingState::root(),
         )
         .unwrap();
@@ -1896,7 +1887,6 @@ mod tests {
             &mut TransactionsProcessor::new(
                 TransactionNameConfig {
                     rules: rules.as_ref(),
-                    ready: false,
                 },
                 false,
                 None,
@@ -1962,7 +1952,6 @@ mod tests {
             &mut TransactionsProcessor::new(
                 TransactionNameConfig {
                     rules: rules.as_ref(),
-                    ready: false,
                 },
                 false,
                 None,
@@ -2062,7 +2051,6 @@ mod tests {
             &mut TransactionsProcessor::new(
                 TransactionNameConfig {
                     rules: rules.as_ref(),
-                    ready: false,
                 },
                 false,
                 None,
@@ -2130,14 +2118,7 @@ mod tests {
 
         process_value(
             &mut event,
-            &mut TransactionsProcessor::new(
-                TransactionNameConfig {
-                    rules: &[rule],
-                    ready: false,
-                },
-                false,
-                None,
-            ),
+            &mut TransactionsProcessor::new(TransactionNameConfig { rules: &[rule] }, false, None),
             ProcessingState::root(),
         )
         .unwrap();
@@ -2364,7 +2345,6 @@ mod tests {
                         scope: TransactionNameRuleScope::default(),
                         redaction: RedactionRule::default(),
                     }],
-                    ready: false,
                 },
                 false,
                 None,
@@ -2412,7 +2392,6 @@ mod tests {
                         scope: TransactionNameRuleScope::default(),
                         redaction: RedactionRule::default(),
                     }],
-                    ready: false,
                 },
                 false,
                 None,
@@ -2894,7 +2873,6 @@ mod tests {
                         scope: TransactionNameRuleScope::default(),
                         redaction: RedactionRule::default(),
                     }],
-                    ..Default::default()
                 },
                 true,
                 None,
@@ -2995,7 +2973,6 @@ mod tests {
                         scope: TransactionNameRuleScope::default(),
                         redaction: RedactionRule::default(),
                     }],
-                    ..Default::default()
                 },
                 true,
                 Some(&Vec::from([SpanDescriptionRule {

diff --git a/...shots/relay_general__store__transactions__processor__tests__no_sanitized_if_no_rules.snap b/...shots/relay_general__store__transactions__processor__tests__no_sanitized_if_no_rules.snap
@@ -6,7 +6,7 @@ expression: event
   "type": "transaction",
   "transaction": "/remains/rule-target/whatever",
   "transaction_info": {
-    "source": "url"
+    "source": "sanitized"
   },
   "timestamp": 1619420400.0,
   "start_timestamp": 1619420341.0,

diff --git a/relay-server/src/actors/processor.rs b/relay-server/src/actors/processor.rs
@@ -2357,7 +2357,6 @@ impl EnvelopeProcessorService {
                 normalize_user_agent: Some(true),
                 transaction_name_config: TransactionNameConfig {
                     rules: &state.project_state.config.tx_name_rules,
-                    ready: state.project_state.config.tx_name_ready,
                 },
                 device_class_synthesis_config: state
                     .project_state
@@ -3716,7 +3715,6 @@ mod tests {
                                 substitution: "*".to_owned(),
                             },
                         }],
-                        ready: false,
                     },
                     ..Default::default()
                 };