Merge branch 'master' into ref/axum

* master: ref(tx-processor): Apply transaction rules after UUID scrubbing (#1964)
getsentry · Mar 29, 2023 · e890a61 · e890a61
2 parents f49f6c1 + dac9b06
commit e890a61
Show file tree

Hide file tree

Showing 5 changed files with 212 additions and 29 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,7 +4,7 @@
 
 **Breaking Changes**:
 
-This release carries major changes to the web layer, including TCP and HTTP handling as well as all web endpoint handlers. Due to these changes, some functionality was retired and Relay responds differently in specific cases.
+This release contains major changes to the web layer, including TCP and HTTP handling as well as all web endpoint handlers. Due to these changes, some functionality was retired and Relay responds differently in specific cases.
 
 Configuration:
 - SSL support has been dropped. As per [official guidelines](https://docs.sentry.io/product/relay/operating-guidelines/), Relay should be operated behind a reverse proxy, which can perform SSL termination.
@@ -27,6 +27,7 @@ Metrics:
 **Internal**:
 
 - Upgrade the web framework and related dependencies. ([#1938](https://github.com/getsentry/relay/pull/1938))
+- Apply transaction clustering rules before UUID scrubbing rules. ([#1964](https://github.com/getsentry/relay/pull/1964))
 
 ## 23.3.1
 

diff --git a/py/CHANGELOG.md b/py/CHANGELOG.md
@@ -5,6 +5,7 @@
 - Add `thread.state` field to protocol. ([#1896](https://github.com/getsentry/relay/pull/1896))
 - PII scrub `span.data` by default. ([#1953](https://github.com/getsentry/relay/pull/1953))
 - Scrub sensitive cookies. ([#1951](https://github.com/getsentry/relay/pull/1951))
+- Apply transaction clustering rules before UUID scrubbing rules. ([#1964](https://github.com/getsentry/relay/pull/1964))
 
 ## 0.8.19
 

diff --git a/relay-general/src/store/transactions/processor.rs b/relay-general/src/store/transactions/processor.rs
@@ -57,7 +57,13 @@ impl<'r> TransactionsProcessor<'r> {
 
                 if let Some((rule, result)) = result {
                     if *transaction != result {
-                        meta.set_original_value(Some(transaction.clone()));
+                        // If another rule was applied before, we don't want to
+                        // rename the transaction name to keep the original one.
+                        // We do want to continue adding remarks though, in
+                        // order to keep track of all rules applied.
+                        if meta.original_value().is_none() {
+                            meta.set_original_value(Some(transaction.clone()));
+                        }
                         // add also the rule which was applied to the transaction name
                         meta.add_remark(Remark::new(RemarkType::Substituted, rule));
                         *transaction = result;
@@ -342,26 +348,28 @@ impl Processor for TransactionsProcessor<'_> {
         }
 
         if self.name_config.scrub_identifiers {
-            // Apply the rule if any found
-            self.apply_transaction_rename_rule(
-                &mut event.transaction,
-                event.transaction_info.value_mut(),
-            )?;
-
             // Normalize transaction names for URLs and Sanitized transaction sources.
             // This in addition to renaming rules can catch some high cardinality parts.
             if matches!(
                 event.get_transaction_source(),
                 &TransactionSource::Url | &TransactionSource::Sanitized
             ) {
                 scrub_identifiers(&mut event.transaction)?;
-                if self.name_config.mark_scrubbed_as_sanitized {
-                    event
-                        .transaction_info
-                        .get_or_insert_with(Default::default)
-                        .source
-                        .set_value(Some(TransactionSource::Sanitized));
-                }
+            }
+
+            self.apply_transaction_rename_rule(
+                &mut event.transaction,
+                event.transaction_info.value_mut(),
+            )?;
+
+            if matches!(event.get_transaction_source(), &TransactionSource::Url)
+                && self.name_config.mark_scrubbed_as_sanitized
+            {
+                event
+                    .transaction_info
+                    .get_or_insert_with(Default::default)
+                    .source
+                    .set_value(Some(TransactionSource::Sanitized));
             }
         }
 
@@ -449,7 +457,7 @@ mod tests {
     use super::*;
     use crate::processor::process_value;
     use crate::protocol::{Contexts, SpanId, TraceContext, TraceId, TransactionSource};
-    use crate::store::LazyGlob;
+    use crate::store::{LazyGlob, RedactionRule, RuleScope};
     use crate::testutils::assert_annotated_snapshot;
     use crate::types::Object;
 
@@ -1675,7 +1683,7 @@ mod tests {
         let json = r#"
         {
             "type": "transaction",
-            "transaction": "/foo/2fd4e1c67a2d28fced849ee1bb76e7391b93eb12/user/123/0/",
+            "transaction": "/foo/rule-target/user/123/0/",
             "transaction_info": {
               "source": "url"
             },
@@ -1722,8 +1730,8 @@ mod tests {
             &mut event,
             &mut TransactionsProcessor::new(TransactionNameConfig {
                 scrub_identifiers: true,
+                mark_scrubbed_as_sanitized: false, // ensure `source` is set by rule application
                 rules: rules.as_ref(),
-                ..Default::default()
             }),
             ProcessingState::root(),
         )
@@ -1758,12 +1766,18 @@ mod tests {
              "transaction": {
                "": {
                  "rem": [
+                   [
+                     "int",
+                     "s",
+                     22,
+                     25
+                   ],
                    [
                      "/foo/*/user/*/**",
                      "s"
                    ]
                  ],
-                 "val": "/foo/2fd4e1c67a2d28fced849ee1bb76e7391b93eb12/user/123/0/"
+                 "val": "/foo/rule-target/user/123/0/"
                }
              }
            }
@@ -1779,8 +1793,8 @@ mod tests {
             &mut event,
             &mut TransactionsProcessor::new(TransactionNameConfig {
                 scrub_identifiers: true,
+                mark_scrubbed_as_sanitized: false, // ensure `source` is set by rule application
                 rules: rules.as_ref(),
-                ..Default::default()
             }),
             ProcessingState::root(),
         )
@@ -1815,18 +1829,18 @@ mod tests {
             "transaction": {
               "": {
                 "rem": [
-                  [
-                    "/foo/*/**",
-                    "s"
-                  ],
                   [
                     "int",
                     "s",
-                    12,
-                    15
+                    22,
+                    25
+                  ],
+                  [
+                    "/foo/*/**",
+                    "s"
                   ]
                 ],
-                "val": "/foo/*/user/123/0/"
+                "val": "/foo/rule-target/user/123/0/"
               }
             }
           }
@@ -1910,7 +1924,7 @@ mod tests {
         let json = r#"
         {
             "type": "transaction",
-            "transaction": "/foo/2fd4e1c67a2d28fced849ee1bb76e7391b93eb12/user",
+            "transaction": "/foo/rule-target/user",
             "transaction_info": {
               "source": "url"
             },
@@ -1984,7 +1998,7 @@ mod tests {
                      "s"
                    ]
                  ],
-                 "val": "/foo/2fd4e1c67a2d28fced849ee1bb76e7391b93eb12/user"
+                 "val": "/foo/rule-target/user"
                }
              }
            }
@@ -2139,4 +2153,93 @@ mod tests {
         "open-12345-close",
         "open-12345-close"
     );
+
+    #[test]
+    fn test_scrub_identifiers_before_rules() {
+        // There's a rule matching the transaction name. However, the UUID
+        // should be scrubbed first. Scrubbing the UUID makes the rule to not
+        // match the transformed transaction name anymore.
+
+        let mut event = Annotated::<Event>::from_json(
+            r#"{
+                "type": "transaction",
+                "transaction": "/remains/rule-target/1234567890",
+                "transaction_info": {
+                    "source": "url"
+                },
+                "timestamp": "2021-04-26T08:00:00+0100",
+                "start_timestamp": "2021-04-26T07:59:01+0100",
+                "contexts": {
+                    "trace": {
+                        "trace_id": "4c79f60c11214eb38604f4ae0781bfb2",
+                        "span_id": "fa90fdead5f74053"
+                    }
+                }
+            }"#,
+        )
+        .unwrap();
+
+        process_value(
+            &mut event,
+            &mut TransactionsProcessor::new(TransactionNameConfig {
+                scrub_identifiers: true,
+                rules: &[TransactionNameRule {
+                    pattern: LazyGlob::new("/remains/*/1234567890/".to_owned()),
+                    expiry: Utc.with_ymd_and_hms(3000, 1, 1, 1, 1, 1).unwrap(),
+                    scope: RuleScope::default(),
+                    redaction: RedactionRule::default(),
+                }],
+                ..Default::default()
+            }),
+            ProcessingState::root(),
+        )
+        .unwrap();
+
+        // Annotate the snapshot instead of comparing transaction names, to also
+        // make sure the event's _meta is correct.
+        assert_annotated_snapshot!(event);
+    }
+
+    #[test]
+    fn test_scrub_identifiers_and_apply_rules() {
+        // Ensure rules are applied after scrubbing identifiers. Rules are only
+        // applied when `transaction.source="url"`, so this test ensures this
+        // value isn't set as part of identifier scrubbing.
+        let mut event = Annotated::<Event>::from_json(
+            r#"{
+                "type": "transaction",
+                "transaction": "/remains/rule-target/1234567890",
+                "transaction_info": {
+                    "source": "url"
+                },
+                "timestamp": "2021-04-26T08:00:00+0100",
+                "start_timestamp": "2021-04-26T07:59:01+0100",
+                "contexts": {
+                    "trace": {
+                        "trace_id": "4c79f60c11214eb38604f4ae0781bfb2",
+                        "span_id": "fa90fdead5f74053"
+                    }
+                }
+            }"#,
+        )
+        .unwrap();
+
+        process_value(
+            &mut event,
+            &mut TransactionsProcessor::new(TransactionNameConfig {
+                scrub_identifiers: true,
+                mark_scrubbed_as_sanitized: true,
+                rules: &[TransactionNameRule {
+                    pattern: LazyGlob::new("/remains/*/**".to_owned()),
+                    expiry: Utc.with_ymd_and_hms(3000, 1, 1, 1, 1, 1).unwrap(),
+                    scope: RuleScope::default(),
+                    redaction: RedactionRule::default(),
+                }],
+            }),
+            ProcessingState::root(),
+        )
+        .unwrap();
+
+        assert_annotated_snapshot!(event);
+    }
 }
diff --git a/...ay_general__store__transactions__processor__tests__scrub_identifiers_and_apply_rules.snap b/...ay_general__store__transactions__processor__tests__scrub_identifiers_and_apply_rules.snap
@@ -0,0 +1,41 @@
+---
+source: relay-general/src/store/transactions/processor.rs
+expression: event
+---
+{
+  "type": "transaction",
+  "transaction": "/remains/*/*",
+  "transaction_info": {
+    "source": "sanitized"
+  },
+  "timestamp": 1619420400.0,
+  "start_timestamp": 1619420341.0,
+  "contexts": {
+    "trace": {
+      "trace_id": "4c79f60c11214eb38604f4ae0781bfb2",
+      "span_id": "fa90fdead5f74053",
+      "op": "default",
+      "type": "trace"
+    }
+  },
+  "spans": [],
+  "_meta": {
+    "transaction": {
+      "": {
+        "rem": [
+          [
+            "int",
+            "s",
+            21,
+            31
+          ],
+          [
+            "/remains/*/**",
+            "s"
+          ]
+        ],
+        "val": "/remains/rule-target/1234567890"
+      }
+    }
+  }
+}
diff --git a/...relay_general__store__transactions__processor__tests__scrub_identifiers_before_rules.snap b/...relay_general__store__transactions__processor__tests__scrub_identifiers_before_rules.snap
@@ -0,0 +1,37 @@
+---
+source: relay-general/src/store/transactions/processor.rs
+expression: event
+---
+{
+  "type": "transaction",
+  "transaction": "/remains/rule-target/*",
+  "transaction_info": {
+    "source": "url"
+  },
+  "timestamp": 1619420400.0,
+  "start_timestamp": 1619420341.0,
+  "contexts": {
+    "trace": {
+      "trace_id": "4c79f60c11214eb38604f4ae0781bfb2",
+      "span_id": "fa90fdead5f74053",
+      "op": "default",
+      "type": "trace"
+    }
+  },
+  "spans": [],
+  "_meta": {
+    "transaction": {
+      "": {
+        "rem": [
+          [
+            "int",
+            "s",
+            21,
+            31
+          ]
+        ],
+        "val": "/remains/rule-target/1234567890"
+      }
+    }
+  }
+}