feat(protocol): Support comparisons in rule conditions on strings (#2730

) Adds the ability to run all comparison operators (`>`, `>=`, `<`, `<=`) on strings. This applies to dynamic sampling and metric extraction. Since this change breaks backwards compatibility, the versioning for metrics extraction and dynamic sampling were bumped. Dynamic sampling configs did not have a versioning schema before. The last breaking change introduced a `rules_v2` array. To fix this, there is now versioning built into the new config. Additionally, an `ErrorBoundary` is added around sampling configuration to prevent accidental deserialization failures. Backwards compatibility is as follows: - New Relay <> Old Sentry: Rules are automatically upgraded from `rules_v2`. - Old Relay <> New Sentry: Cannot see the new rules and forwards events.
getsentry · Nov 16, 2023 · 063df70 · 063df70
1 parent 01640e3
commit 063df70
Show file tree

Hide file tree

Showing 17 changed files with 251 additions and 250 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@
 **Internal**:
 
 - Support `device.model` in dynamic sampling and metric extraction. ([#2728](https://github.com/getsentry/relay/pull/2728))
+- Support comparison operators (`>`, `>=`, `<`, `<=`) for strings in dynamic sampling and metric extraction rules. Previously, these comparisons were only possible on numbers. ([#2730](https://github.com/getsentry/relay/pull/2730))
 
 ## 23.11.0
 

diff --git a/py/tests/test_processing.py b/py/tests/test_processing.py
@@ -248,7 +248,7 @@ def test_invalid_sampling_condition():
         sentry_relay.validate_rule_condition(condition)
 
 
-def test_validate_sampling_configuration():
+def test_validate_legacy_sampling_configuration():
     """
     Tests that a valid sampling rule configuration passes
     """
@@ -264,9 +264,9 @@ def test_validate_sampling_configuration():
                 "condition": {
                     "op": "custom",
                     "name": "event.legacy_browser",
-                    "value":["ie10"]
+                    "value": ["ie10"]
                 },
-                "id":1
+                "id": 1
             },
             {
                 "type": "trace",
@@ -277,10 +277,37 @@ def test_validate_sampling_configuration():
                 "condition": {
                     "op": "eq",
                     "name": "event.release",
-                    "value":["1.1.*"],
+                    "value": ["1.1.*"],
+                    "options": {"ignoreCase": true}
+                },
+                "id": 2
+            }
+        ]
+    }"""
+    # Should NOT throw
+    sentry_relay.validate_sampling_configuration(config)
+
+
+def test_validate_sampling_configuration():
+    """
+    Tests that a valid sampling rule configuration passes
+    """
+    config = """{
+        "version": 2,
+        "rules": [
+            {
+                "type": "trace",
+                "samplingValue": {
+                    "type": "sampleRate",
+                    "value": 0.9
+                },
+                "condition": {
+                    "op": "eq",
+                    "name": "event.release",
+                    "value": ["1.1.*"],
                     "options": {"ignoreCase": true}
                 },
-                "id":2
+                "id": 2
             }
         ]
     }"""

diff --git a/relay-cabi/include/relay.h b/relay-cabi/include/relay.h
@@ -15,7 +15,8 @@
 /**
  * Classifies the type of data that is being ingested.
  */
-enum RelayDataCategory {
+enum RelayDataCategory
+{
   /**
    * Reserved and unused.
    */
@@ -106,7 +107,8 @@ typedef int8_t RelayDataCategory;
 /**
  * Controls the globbing behaviors.
  */
-enum GlobFlags {
+enum GlobFlags
+{
   /**
    * When enabled `**` matches over path separators and `*` does not.
    */
@@ -129,7 +131,8 @@ typedef uint32_t GlobFlags;
 /**
  * Represents all possible error codes.
  */
-enum RelayErrorCode {
+enum RelayErrorCode
+{
   RELAY_ERROR_CODE_NO_ERROR = 0,
   RELAY_ERROR_CODE_PANIC = 1,
   RELAY_ERROR_CODE_UNKNOWN = 2,
@@ -154,7 +157,8 @@ typedef uint32_t RelayErrorCode;
  * Values from <https://github.com/open-telemetry/opentelemetry-specification/blob/8fb6c14e4709e75a9aaa64b0dbbdf02a6067682a/specification/api-tracing.md#status>
  * Mapping to HTTP from <https://github.com/open-telemetry/opentelemetry-specification/blob/8fb6c14e4709e75a9aaa64b0dbbdf02a6067682a/specification/data-http.md#status>
  */
-enum RelaySpanStatus {
+enum RelaySpanStatus
+{
   /**
    * The operation completed successfully.
    *
@@ -279,7 +283,8 @@ typedef struct RelayStoreNormalizer RelayStoreNormalizer;
  *  - When obtained as instance through return values, always free the string.
  *  - When obtained as pointer through field access, never free the string.
  */
-typedef struct RelayStr {
+typedef struct RelayStr
+{
   /**
    * Pointer to the UTF-8 encoded string data.
    */
@@ -303,7 +308,8 @@ typedef struct RelayStr {
  *  - When obtained as instance through return values, always free the buffer.
  *  - When obtained as pointer through field access, never free the buffer.
  */
-typedef struct RelayBuf {
+typedef struct RelayBuf
+{
   /**
    * Pointer to the raw data.
    */
@@ -321,7 +327,8 @@ typedef struct RelayBuf {
 /**
  * Represents a key pair from key generation.
  */
-typedef struct RelayKeyPair {
+typedef struct RelayKeyPair
+{
   /**
    * The public key used for verifying Relay signatures.
    */
@@ -335,7 +342,8 @@ typedef struct RelayKeyPair {
 /**
  * A 16-byte UUID.
  */
-typedef struct RelayUuid {
+typedef struct RelayUuid
+{
   /**
    * UUID bytes in network byte order (big endian).
    */

diff --git a/relay-dynamic-config/src/metrics.rs b/relay-dynamic-config/src/metrics.rs
@@ -84,7 +84,8 @@ pub struct CustomMeasurementConfig {
 ///  - 3:
 ///      - Emit a `usage` metric and use it for rate limiting.
 ///      - Delay metrics extraction for indexed transactions.
-const TRANSACTION_EXTRACT_VERSION: u16 = 3;
+///  - 4: Adds support for `RuleConfigs` with string comparisons.
+const TRANSACTION_EXTRACT_VERSION: u16 = 4;
 
 /// Deprecated. Defines whether URL transactions should be considered low cardinality.
 #[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq)]

diff --git a/relay-dynamic-config/src/project.rs b/relay-dynamic-config/src/project.rs
@@ -46,8 +46,8 @@ pub struct ProjectConfig {
     #[serde(skip_serializing_if = "Vec::is_empty")]
     pub quotas: Vec<Quota>,
     /// Configuration for sampling traces, if not present there will be no sampling.
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub dynamic_sampling: Option<SamplingConfig>,
+    #[serde(alias = "dynamicSampling", skip_serializing_if = "Option::is_none")]
+    pub sampling: Option<ErrorBoundary<SamplingConfig>>,
     /// Configuration for measurements.
     /// NOTE: do not access directly, use [`relay_event_normalization::DynamicMeasurementsConfig`].
     #[serde(skip_serializing_if = "Option::is_none")]
@@ -97,6 +97,10 @@ impl ProjectConfig {
 
         metrics::convert_conditional_tagging(self);
         defaults::add_span_metrics(self);
+
+        if let Some(ErrorBoundary::Ok(ref mut sampling_config)) = self.sampling {
+            sampling_config.normalize();
+        }
     }
 }
 
@@ -111,7 +115,7 @@ impl Default for ProjectConfig {
             datascrubbing_settings: DataScrubbingConfig::default(),
             event_retention: None,
             quotas: Vec::new(),
-            dynamic_sampling: None,
+            sampling: None,
             measurements: None,
             breakdowns_v2: None,
             performance_score: Default::default(),
@@ -150,7 +154,7 @@ pub struct LimitedProjectConfig {
     #[serde(skip_serializing_if = "DataScrubbingConfig::is_disabled")]
     pub datascrubbing_settings: DataScrubbingConfig,
     #[serde(skip_serializing_if = "Option::is_none")]
-    pub dynamic_sampling: Option<SamplingConfig>,
+    pub sampling: Option<ErrorBoundary<SamplingConfig>>,
     #[serde(skip_serializing_if = "SessionMetricsConfig::is_disabled")]
     pub session_metrics: SessionMetricsConfig,
     #[serde(skip_serializing_if = "Option::is_none")]

diff --git a/relay-protocol/src/condition.rs b/relay-protocol/src/condition.rs
@@ -4,7 +4,7 @@
 
 use relay_common::glob3::GlobPatterns;
 use serde::{Deserialize, Serialize};
-use serde_json::{Number, Value};
+use serde_json::Value;
 
 use crate::{Getter, Val};
 
@@ -106,12 +106,12 @@ macro_rules! impl_cmp_condition {
             /// Path of the field that should match the value.
             pub name: String,
             /// The numeric value to check against.
-            pub value: Number,
+            pub value: Value,
         }
 
         impl $struct_name {
             /// Creates a new condition that comparison condition.
-            pub fn new(field: impl Into<String>, value: impl Into<Number>) -> Self {
+            pub fn new(field: impl Into<String>, value: impl Into<Value>) -> Self {
                 Self {
                     name: field.into(),
                     value: value.into(),
@@ -136,6 +136,8 @@ macro_rules! impl_cmp_condition {
                     a $operator b
                 } else if let (Some(a), Some(b)) = (value.as_f64(), self.value.as_f64()) {
                     a $operator b
+                } else if let (Some(a), Some(b)) = (value.as_str(), self.value.as_str()) {
+                    a $operator b
                 } else {
                     false
                 }
@@ -527,7 +529,7 @@ impl RuleCondition {
     ///
     /// let condition = RuleCondition::gt("obj.length", 10);
     /// ```
-    pub fn gt(field: impl Into<String>, value: impl Into<Number>) -> Self {
+    pub fn gt(field: impl Into<String>, value: impl Into<Value>) -> Self {
         Self::Gt(GtCondition::new(field, value))
     }
 
@@ -540,7 +542,7 @@ impl RuleCondition {
     ///
     /// let condition = RuleCondition::gte("obj.length", 10);
     /// ```
-    pub fn gte(field: impl Into<String>, value: impl Into<Number>) -> Self {
+    pub fn gte(field: impl Into<String>, value: impl Into<Value>) -> Self {
         Self::Gte(GteCondition::new(field, value))
     }
 
@@ -553,7 +555,7 @@ impl RuleCondition {
     ///
     /// let condition = RuleCondition::lt("obj.length", 10);
     /// ```
-    pub fn lt(field: impl Into<String>, value: impl Into<Number>) -> Self {
+    pub fn lt(field: impl Into<String>, value: impl Into<Value>) -> Self {
         Self::Lt(LtCondition::new(field, value))
     }
 
@@ -566,7 +568,7 @@ impl RuleCondition {
     ///
     /// let condition = RuleCondition::lte("obj.length", 10);
     /// ```
-    pub fn lte(field: impl Into<String>, value: impl Into<Number>) -> Self {
+    pub fn lte(field: impl Into<String>, value: impl Into<Value>) -> Self {
         Self::Lte(LteCondition::new(field, value))
     }
 
@@ -934,6 +936,7 @@ mod tests {
                     & RuleCondition::eq_ignore_case("trace.user.segment", "vip"),
             ),
             ("match no conditions", RuleCondition::all()),
+            ("string cmp", RuleCondition::gt("trace.transaction", "t")),
         ];
 
         let dsc = mock_dsc();