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

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

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.