From f51ca9435f986e104e245dbd48eab880e03662e7 Mon Sep 17 00:00:00 2001 From: edwardgou-sentry <83961295+edwardgou-sentry@users.noreply.github.com> Date: Wed, 24 Jan 2024 11:18:30 -0500 Subject: [PATCH] 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 | 1 + py/CHANGELOG.md | 4 + relay-event-normalization/src/event.rs | 73 ++++++++++++++++++- .../src/normalize/mod.rs | 12 +++ 4 files changed, 87 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 867f362257..4a4d9e93c2 100644 --- a/CHANGELOG.md +++ b/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**: diff --git a/py/CHANGELOG.md b/py/CHANGELOG.md index c203ae7335..36a6e4a87b 100644 --- a/py/CHANGELOG.md +++ b/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 diff --git a/relay-event-normalization/src/event.rs b/relay-event-normalization/src/event.rs index e3d3edfca7..2e3da0878f 100644 --- a/relay-event-normalization/src/event.rs +++ b/relay-event-normalization/src/event.rs @@ -725,9 +725,6 @@ 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), @@ -735,6 +732,9 @@ pub fn normalize_measurements( }; 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::::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, + }, + }), + ))), + }, + }, + } + "###); + } } diff --git a/relay-event-normalization/src/normalize/mod.rs b/relay-event-normalization/src/normalize/mod.rs index 91cf4ab77c..ab52306700 100644 --- a/relay-event-normalization/src/normalize/mod.rs +++ b/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.