From a96cba50bcfdf5c22491e8f23e2fc51724ea961b Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Thu, 14 Apr 2022 12:27:53 +0200 Subject: [PATCH 1/5] feat(metrics): Add units in built-in measurements --- .../src/metrics_extraction/transactions.rs | 32 ++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/relay-server/src/metrics_extraction/transactions.rs b/relay-server/src/metrics_extraction/transactions.rs index 6bb1b88ecd5..d056396180f 100644 --- a/relay-server/src/metrics_extraction/transactions.rs +++ b/relay-server/src/metrics_extraction/transactions.rs @@ -152,6 +152,36 @@ fn extract_user_satisfaction( None } +/// Returns the unit of the provided metric. Defaults to None. +#[cfg(feature = "processing")] +fn get_metric_measurement_unit(metric: &String) -> MetricUnit { + match metric.as_str() { + // Web + "fcp" => MetricUnit::Duration(DurationUnit::MilliSecond), + "lcp" => MetricUnit::Duration(DurationUnit::MilliSecond), + "fid" => MetricUnit::Duration(DurationUnit::MilliSecond), + "fp" => MetricUnit::Duration(DurationUnit::MilliSecond), + "ttfb" => MetricUnit::Duration(DurationUnit::MilliSecond), + "ttfb.requesttime" => MetricUnit::Duration(DurationUnit::MilliSecond), + "cls" => MetricUnit::None, + + // Mobile + "app_start_cold" => MetricUnit::Duration(DurationUnit::MilliSecond), + "app_start_warm" => MetricUnit::Duration(DurationUnit::MilliSecond), + "frames_total" => MetricUnit::None, + "frames_slow" => MetricUnit::None, + "frames_frozen" => MetricUnit::None, + + // React-Native + "stall_count" => MetricUnit::None, + "stall_total_time" => MetricUnit::Duration(DurationUnit::MilliSecond), + "stall_longest_time" => MetricUnit::Duration(DurationUnit::MilliSecond), + + // Default + _ => MetricUnit::None, + } +} + #[cfg(feature = "processing")] pub fn extract_transaction_metrics( config: &TransactionMetricsConfig, @@ -251,7 +281,7 @@ fn extract_transaction_metrics_inner( push_metric(Metric::new_mri( METRIC_NAMESPACE, format_args!("measurements.{}", measurement_name), - MetricUnit::None, + get_metric_measurement_unit(&measurement_name), MetricValue::Distribution(measurement), unix_timestamp, tags, From 3cf763de5b91a89246fb3f87d3a853cad8c1b42e Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Thu, 14 Apr 2022 17:58:47 +0200 Subject: [PATCH 2/5] Fix metric unit in existing test The unit of `lcp` is `millisecond`, not `none`. --- .../src/metrics_extraction/transactions.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/relay-server/src/metrics_extraction/transactions.rs b/relay-server/src/metrics_extraction/transactions.rs index d056396180f..c0e23a1c6a6 100644 --- a/relay-server/src/metrics_extraction/transactions.rs +++ b/relay-server/src/metrics_extraction/transactions.rs @@ -453,7 +453,7 @@ mod tests { { "extractMetrics": [ "d:transactions/measurements.foo@none", - "d:transactions/measurements.lcp@none", + "d:transactions/measurements.lcp@millisecond", "d:transactions/breakdowns.span_ops.ops.react.mount@none", "d:transactions/duration@millisecond", "s:transactions/user@none" @@ -476,7 +476,10 @@ mod tests { assert_eq!(metrics.len(), 5, "{:?}", metrics); assert_eq!(metrics[0].name, "d:transactions/measurements.foo@none"); - assert_eq!(metrics[1].name, "d:transactions/measurements.lcp@none"); + assert_eq!( + metrics[1].name, + "d:transactions/measurements.lcp@millisecond" + ); assert_eq!( metrics[2].name, "d:transactions/breakdowns.span_ops.ops.react.mount@none" @@ -797,7 +800,7 @@ mod tests { r#" { "extractMetrics": [ - "d:transactions/measurements.lcp@none" + "d:transactions/measurements.lcp@millisecond" ] } "#, @@ -809,19 +812,19 @@ mod tests { [ { "condition": {"op": "gte", "name": "transaction.measurements.lcp", "value": 41}, - "targetMetrics": ["d:transactions/measurements.lcp@none"], + "targetMetrics": ["d:transactions/measurements.lcp@millisecond"], "targetTag": "satisfaction", "tagValue": "frustrated" }, { "condition": {"op": "gte", "name": "transaction.measurements.lcp", "value": 20}, - "targetMetrics": ["d:transactions/measurements.lcp@none"], + "targetMetrics": ["d:transactions/measurements.lcp@millisecond"], "targetTag": "satisfaction", "tagValue": "tolerated" }, { "condition": {"op": "and", "inner": []}, - "targetMetrics": ["d:transactions/measurements.lcp@none"], + "targetMetrics": ["d:transactions/measurements.lcp@millisecond"], "targetTag": "satisfaction", "tagValue": "satisfied" } @@ -843,7 +846,7 @@ mod tests { &[Metric::new_mri( METRIC_NAMESPACE, "measurements.lcp", - MetricUnit::None, + MetricUnit::Duration(DurationUnit::MilliSecond), MetricValue::Distribution(41.0), UnixTimestamp::from_secs(1619420402), { From 9183008a942db5b7d0fe46afc617afad015e2ed7 Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Thu, 14 Apr 2022 17:51:30 +0200 Subject: [PATCH 3/5] Add test --- .../src/metrics_extraction/transactions.rs | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/relay-server/src/metrics_extraction/transactions.rs b/relay-server/src/metrics_extraction/transactions.rs index c0e23a1c6a6..32cd0221312 100644 --- a/relay-server/src/metrics_extraction/transactions.rs +++ b/relay-server/src/metrics_extraction/transactions.rs @@ -513,6 +513,68 @@ mod tests { } } + #[test] + fn test_metric_measurement_units() { + let json = r#" + { + "type": "transaction", + "timestamp": "2021-04-26T08:00:00+0100", + "start_timestamp": "2021-04-26T07:59:01+0100", + "measurements": { + "fcp": {"value": 1.1}, + "stall_count": {"value": 3.3}, + "foo": {"value": 8.8} + } + } + "#; + + let config: TransactionMetricsConfig = serde_json::from_str( + r#" + { + "extractMetrics": [ + "d:transactions/measurements.fcp@millisecond", + "d:transactions/measurements.stall_count@none", + "d:transactions/measurements.foo@none" + ] + } + "#, + ) + .unwrap(); + + let event = Annotated::from_json(json).unwrap(); + + let mut metrics = vec![]; + extract_transaction_metrics(&config, None, &[], event.value().unwrap(), &mut metrics); + + assert_eq!(metrics.len(), 3, "{:?}", metrics); + + assert_eq!( + metrics[0].name, "d:transactions/measurements.fcp@millisecond", + "{:?}", + metrics[0] + ); + assert_eq!( + metrics[0].unit, + MetricUnit::Duration(DurationUnit::MilliSecond), + "{:?}", + metrics[0] + ); + + assert_eq!( + metrics[1].name, "d:transactions/measurements.foo@none", + "{:?}", + metrics[1] + ); + assert_eq!(metrics[1].unit, MetricUnit::None, "{:?}", metrics[1]); + + assert_eq!( + metrics[2].name, "d:transactions/measurements.stall_count@none", + "{:?}", + metrics[2] + ); + assert_eq!(metrics[2].unit, MetricUnit::None, "{:?}", metrics[2]); + } + #[test] fn test_transaction_duration() { let json = r#" From 8aae40b6cb9047d9b1d09a50bbe7d0b3c502b4ce Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Thu, 14 Apr 2022 19:15:10 +0200 Subject: [PATCH 4/5] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 955dcd98fef..c905b3ca7dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ **Features**: - Map Windows version from raw_description to version name (XP, Vista, 11, ...). ([#1219](https://github.com/getsentry/relay/pull/1219)) +- Add units in built-in measurements. ([#1229](https://github.com/getsentry/relay/pull/1229)) **Bug Fixes**: From f6e8d242dcc74f6724f7a56d7d8de96811514efc Mon Sep 17 00:00:00 2001 From: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com> Date: Thu, 14 Apr 2022 19:30:06 +0200 Subject: [PATCH 5/5] Fix lint --- relay-server/src/metrics_extraction/transactions.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/relay-server/src/metrics_extraction/transactions.rs b/relay-server/src/metrics_extraction/transactions.rs index 32cd0221312..6e13d10a768 100644 --- a/relay-server/src/metrics_extraction/transactions.rs +++ b/relay-server/src/metrics_extraction/transactions.rs @@ -154,8 +154,8 @@ fn extract_user_satisfaction( /// Returns the unit of the provided metric. Defaults to None. #[cfg(feature = "processing")] -fn get_metric_measurement_unit(metric: &String) -> MetricUnit { - match metric.as_str() { +fn get_metric_measurement_unit(metric: &str) -> MetricUnit { + match metric { // Web "fcp" => MetricUnit::Duration(DurationUnit::MilliSecond), "lcp" => MetricUnit::Duration(DurationUnit::MilliSecond), @@ -281,7 +281,7 @@ fn extract_transaction_metrics_inner( push_metric(Metric::new_mri( METRIC_NAMESPACE, format_args!("measurements.{}", measurement_name), - get_metric_measurement_unit(&measurement_name), + get_metric_measurement_unit(measurement_name), MetricValue::Distribution(measurement), unix_timestamp, tags,