getsentry · jan-auer · Nov 16, 2023 · Nov 14, 2023 · Nov 14, 2023 · Nov 14, 2023
@@ -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
     """
@@ -288,6 +288,33 @@ def test_validate_sampling_configuration():
     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
+            }
+        ]
+    }"""
+    # Should NOT throw
+    sentry_relay.validate_sampling_configuration(config)
+
+
 def test_validate_project_config():
     config = {"allowedDomains": ["*"], "trustedRelays": [], "piiConfig": None}
     # Does not raise:

@@ -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)]

@@ -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")]

@@ -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();

@@ -7,22 +7,36 @@ use serde::{Deserialize, Serialize};
 
 use relay_protocol::RuleCondition;
 
+/// Maximum supported version of dynamic sampling.
+///
+/// The version is an integer scalar, incremented by one on each new version:
+///  - 1: Initial version that uses `rules_v2`.
+///  - 2: Moves back to `rules` and adds support for `RuleConfigs` with string comparisons.
+const SAMPLING_CONFIG_VERSION: u16 = 2;
+
 /// Represents the dynamic sampling configuration available to a project.
 ///
 /// Note: This comes from the organization data
 #[derive(Debug, Clone, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
 pub struct SamplingConfig {
+    /// The required version to run dynamic sampling.
+    ///
+    /// Defaults to legacy version (`1`) when missing.
+    #[serde(default = "SamplingConfig::legacy_version")]
+    pub version: u16,
+
     /// The ordered sampling rules for the project.
     ///
     /// This field will remain here to serve only for old customer Relays to which we will
     /// forward the sampling config. The idea is that those Relays will get the old rules as
     /// empty array, which will result in them not sampling and forwarding sampling decisions to
     /// upstream Relays.
-    #[serde(default, skip_deserializing)]
+    #[serde(default)]
     pub rules: Vec<SamplingRule>,
 
     /// The ordered sampling rules v2 for the project.
+    #[serde(default, skip_serializing)]
     pub rules_v2: Vec<SamplingRule>,
 
     /// Defines which population of items a dynamic sample rate applies to.
@@ -31,16 +45,42 @@ pub struct SamplingConfig {
 }
 
 impl SamplingConfig {
+    /// Creates an enabled configuration with empty defaults and the latest version.
+    pub fn new() -> Self {
+        Self::default()
+    }
+
     /// Returns `true` if any of the rules in this configuration is unsupported.
     pub fn unsupported(&self) -> bool {
-        !self.rules_v2.iter().all(SamplingRule::supported)
+        self.version > SAMPLING_CONFIG_VERSION || !self.rules.iter().all(SamplingRule::supported)
     }
 
     /// Filters the sampling rules by the given [`RuleType`].
     pub fn filter_rules(&self, rule_type: RuleType) -> impl Iterator<Item = &SamplingRule> {
-        self.rules_v2
-            .iter()
-            .filter(move |rule| rule.ty == rule_type)
+        self.rules.iter().filter(move |rule| rule.ty == rule_type)
+    }
+
+    /// Upgrades legacy sampling configs into the latest format.
+    pub fn normalize(&mut self) {
+        if self.version == Self::legacy_version() {
+            self.rules.append(&mut self.rules_v2);
+            self.version = SAMPLING_CONFIG_VERSION;
+        }
+    }
+
+    const fn legacy_version() -> u16 {
+        1
+    }
+}
+
+impl Default for SamplingConfig {
+    fn default() -> Self {
+        Self {
+            version: SAMPLING_CONFIG_VERSION,
+            rules: vec![],
+            rules_v2: vec![],
+            mode: SamplingMode::default(),
+        }
     }
 }
 
@@ -422,20 +462,9 @@ mod tests {
     }
 
     #[test]
-    fn test_sampling_config_with_rules_and_rules_v2_deserialization() {
+    fn test_legacy_deserialization() {
         let serialized_rule = r#"{
-               "rules": [
-                  {
-                     "sampleRate": 0.5,
-                     "type": "trace",
-                     "active": true,
-                     "condition": {
-                        "op": "and",
-                        "inner": []
-                     },
-                     "id": 1000
-                 }
-               ],
+               "rules": [],
                "rulesV2": [
                   {
                      "samplingValue":{
@@ -453,36 +482,37 @@ mod tests {
                ],
                "mode": "received"
         }"#;
-        let config: SamplingConfig = serde_json::from_str(serialized_rule).unwrap();
+        let mut config: SamplingConfig = serde_json::from_str(serialized_rule).unwrap();
+        config.normalize();
 
         // We want to make sure that we serialize an empty array of rule, irrespectively of the
         // received payload.
-        assert!(config.rules.is_empty());
+        assert_eq!(config.version, SAMPLING_CONFIG_VERSION);
         assert_eq!(
-            config.rules_v2[0].sampling_value,
+            config.rules[0].sampling_value,
             SamplingValue::SampleRate { value: 0.5 }
         );
+        assert!(config.rules_v2.is_empty());
     }
 
     #[test]
     fn test_sampling_config_with_rules_and_rules_v2_serialization() {
         let config = SamplingConfig {
-            rules: vec![],
-            rules_v2: vec![SamplingRule {
+            rules: vec![SamplingRule {
                 condition: RuleCondition::all(),
                 sampling_value: SamplingValue::Factor { value: 2.0 },
                 ty: RuleType::Transaction,
                 id: RuleId(1),
                 time_range: Default::default(),
                 decaying_fn: Default::default(),
             }],
-            mode: SamplingMode::Received,
+            ..SamplingConfig::new()
         };
 
         let serialized_config = serde_json::to_string_pretty(&config).unwrap();
         let expected_serialized_config = r#"{
-  "rules": [],
-  "rulesV2": [
+  "version": 2,
+  "rules": [
     {
       "condition": {
         "op": "and",

@@ -1,6 +1,6 @@
 {
-  "rules": [],
-  "rulesV2": [
+  "version": 2,
+  "rules": [
     {
       "condition": {
         "op": "and",
@@ -46,4 +46,4 @@
     }
   ],
   "mode": "total"
-}
+}