fix(webvitals): Drop negative web vitals from measurements (#2982)

Add `allow_negative` to `BuiltinMeasurementKey`. Filter out negative
BuiltinMeasurements if `allow_negative` is false.

Author: edwardgou-sentry

CHANGELOG.md

@@ -11,6 +11,7 @@
 - Proactively move on-disk spool to memory. ([#2949](https://github.com/getsentry/relay/pull/2949))
 - Default missing `Event.platform` and `Event.level` fields during light normalization. ([#2961](https://github.com/getsentry/relay/pull/2961))
 - Copy event measurements to span & normalize span measurements. ([#2953](https://github.com/getsentry/relay/pull/2953))
+- Add `allow_negative` to `BuiltinMeasurementKey`. Filter out negative BuiltinMeasurements if `allow_negative` is false. ([#2982](https://github.com/getsentry/relay/pull/2982))
 - Add possiblity to block metrics or their tags with glob-patterns. ([#2954](https://github.com/getsentry/relay/pull/2954), [#2973](https://github.com/getsentry/relay/pull/2973))
 
 **Bug Fixes**:

py/CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog
 
+## Unreleased
+
+- Add `allow_negative` to `BuiltinMeasurementKey`. Filter out negative BuiltinMeasurements if `allow_negative` is false. ([#2982](https://github.com/getsentry/relay/pull/2982))
+
 ## 0.8.44
 
 ### Various fixes & improvements

relay-event-normalization/src/event.rs

@@ -725,16 +725,16 @@ pub fn normalize_measurements(
 ) {
     normalize_mobile_measurements(measurements);
     normalize_units(measurements);
-    if let Some(measurements_config) = measurements_config {
-        remove_invalid_measurements(measurements, meta, measurements_config, max_mri_len);
-    }
 
     let duration_millis = match (start_timestamp, end_timestamp) {
         (Some(start), Some(end)) => relay_common::time::chrono_to_positive_millis(end - start),
         _ => 0.0,
     };
 
     compute_measurements(duration_millis, measurements);
+    if let Some(measurements_config) = measurements_config {
+        remove_invalid_measurements(measurements, meta, measurements_config, max_mri_len);
+    }
 }
 
 /// Computes performance score measurements.
@@ -994,6 +994,16 @@ fn remove_invalid_measurements(
         // Check if this is a builtin measurement:
         for builtin_measurement in measurements_config.builtin_measurement_keys() {
             if builtin_measurement.name() == name {
+                let value = measurement.value.value().unwrap_or(&0.0);
+                // Drop negative values if the builtin measurement does not allow them.
+                if !builtin_measurement.allow_negative() && *value < 0.0 {
+                    meta.add_error(Error::invalid(format!(
+                        "Negative value for measurement {name} not allowed: {value}",
+                    )));
+                    removed_measurements
+                        .insert(name.clone(), Annotated::new(std::mem::take(measurement)));
+                    return false;
+                }
                 // If the unit matches a built-in measurement, we allow it.
                 // If the name matches but the unit is wrong, we do not even accept it as a custom measurement,
                 // and just drop it instead.
@@ -2543,4 +2553,61 @@ mod tests {
             "invalid transaction event: timestamp is too stale"
         );
     }
+
+    #[test]
+    fn test_filter_negative_web_vital_measurements() {
+        let json = r#"
+        {
+            "type": "transaction",
+            "timestamp": "2021-04-26T08:00:05+0100",
+            "start_timestamp": "2021-04-26T08:00:00+0100",
+            "measurements": {
+                "ttfb": {"value": -100, "unit": "millisecond"}
+            }
+        }
+        "#;
+        let mut event = Annotated::<Event>::from_json(json).unwrap().0.unwrap();
+
+        // Allow ttfb as a builtinMeasurement with allow_negative defaulted to false.
+        let project_measurement_config: MeasurementsConfig = serde_json::from_value(json!({
+            "builtinMeasurements": [
+                {"name": "ttfb", "unit": "millisecond"},
+            ],
+        }))
+        .unwrap();
+
+        let dynamic_measurement_config =
+            DynamicMeasurementsConfig::new(Some(&project_measurement_config), None);
+
+        normalize_event_measurements(&mut event, Some(dynamic_measurement_config), None);
+
+        insta::assert_ron_snapshot!(SerializableAnnotated(&Annotated::new(event)), {}, @r###"
+        {
+          "type": "transaction",
+          "timestamp": 1619420405.0,
+          "start_timestamp": 1619420400.0,
+          "measurements": {},
+          "_meta": {
+            "measurements": {
+              "": Meta(Some(MetaInner(
+                err: [
+                  [
+                    "invalid_data",
+                    {
+                      "reason": "Negative value for measurement ttfb not allowed: -100",
+                    },
+                  ],
+                ],
+                val: Some({
+                  "ttfb": {
+                    "unit": "millisecond",
+                    "value": -100.0,
+                  },
+                }),
+              ))),
+            },
+          },
+        }
+        "###);
+    }
 }

relay-event-normalization/src/normalize/mod.rs

@@ -36,6 +36,12 @@ mod contexts;
 pub struct BuiltinMeasurementKey {
     name: String,
     unit: MetricUnit,
+    #[serde(skip_serializing_if = "is_false")]
+    allow_negative: bool,
+}
+
+fn is_false(b: &bool) -> bool {
+    !b
 }
 
 impl BuiltinMeasurementKey {
@@ -44,6 +50,7 @@ impl BuiltinMeasurementKey {
         Self {
             name: name.into(),
             unit,
+            allow_negative: false,
         }
     }
 
@@ -56,6 +63,11 @@ impl BuiltinMeasurementKey {
     pub fn unit(&self) -> &MetricUnit {
         &self.unit
     }
+
+    /// Return true if the built in measurement key allows negative values.
+    pub fn allow_negative(&self) -> &bool {
+        &self.allow_negative
+    }
 }
 
 /// Configuration for measurements normalization.