From 5f6f8012639f5d8385b9943e46ddad9d8925c949 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Thu, 16 Dec 2021 15:29:01 +0100 Subject: [PATCH] feat: Extract normalized dist as metric (#1158) Apply normalization to event.dist and add as tag to every transaction metric. --- CHANGELOG.md | 1 + relay-general/src/store/mod.rs | 1 + relay-general/src/store/normalize.rs | 54 +++++++++++++++---- .../src/metrics_extraction/transactions.rs | 18 ++++++- 4 files changed, 63 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6844087743..93c0caad39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ - Extract session metrics from aggregate sessions. ([#1140](https://github.com/getsentry/relay/pull/1140)) - Prefix names of extracted metrics by `sentry.sessions.` or `sentry.transactions.`. ([#1147](https://github.com/getsentry/relay/pull/1147)) - Extract transaction duration as metric. ([#1148](https://github.com/getsentry/relay/pull/1148)) +- Extract normalized dist as metric. ([#1158](https://github.com/getsentry/relay/pull/1158)) ## 21.11.0 diff --git a/relay-general/src/store/mod.rs b/relay-general/src/store/mod.rs index 58639923de..f7b45f10c2 100644 --- a/relay-general/src/store/mod.rs +++ b/relay-general/src/store/mod.rs @@ -23,6 +23,7 @@ mod trimming; pub use self::clock_drift::ClockDriftProcessor; pub use self::geo::{GeoIpError, GeoIpLookup}; pub use normalize::breakdowns::BreakdownsConfig; +pub use normalize::normalize_dist; /// The config for store. #[derive(Serialize, Deserialize, Debug, Default)] diff --git a/relay-general/src/store/normalize.rs b/relay-general/src/store/normalize.rs index f624be7828..300a507972 100644 --- a/relay-general/src/store/normalize.rs +++ b/relay-general/src/store/normalize.rs @@ -66,6 +66,22 @@ fn is_valid_platform(platform: &str) -> bool { VALID_PLATFORMS.contains(&platform) } +pub fn normalize_dist(dist: &mut Option) { + let mut erase = false; + if let Some(val) = dist { + if val.is_empty() { + erase = true; + } + let trimmed = val.trim(); + if trimmed != val { + *val = trimmed.to_string() + } + } + if erase { + *dist = None; + } +} + /// The processor that normalizes events for store. pub struct NormalizeProcessor<'a> { config: Arc, @@ -120,15 +136,7 @@ impl<'a> NormalizeProcessor<'a> { /// Ensures that the `release` and `dist` fields match up. fn normalize_release_dist(&self, event: &mut Event) { - if event.dist.value().is_some() && event.release.value().is_empty() { - event.dist.set_value(None); - } - - if let Some(dist) = event.dist.value_mut() { - if dist.trim() != dist { - *dist = dist.trim().to_string(); - } - } + normalize_dist(event.dist.value_mut()); } /// Validates the timestamp range and sets a default value. @@ -1600,3 +1608,31 @@ fn test_past_timestamp() { } "###); } + +#[test] +fn test_normalize_dist_none() { + let mut dist = None; + normalize_dist(&mut dist); + assert_eq!(dist, None); +} + +#[test] +fn test_normalize_dist_empty() { + let mut dist = Some("".to_owned()); + normalize_dist(&mut dist); + assert_eq!(dist, None); +} + +#[test] +fn test_normalize_dist_trim() { + let mut dist = Some(" foo ".to_owned()); + normalize_dist(&mut dist); + assert_eq!(dist.unwrap(), "foo"); +} + +#[test] +fn test_normalize_dist_whitespace() { + let mut dist = Some(" ".to_owned()); + normalize_dist(&mut dist); + assert_eq!(dist.unwrap(), ""); // Not sure if this is what we want +} diff --git a/relay-server/src/metrics_extraction/transactions.rs b/relay-server/src/metrics_extraction/transactions.rs index 9f6eaaaf69..fafd48423d 100644 --- a/relay-server/src/metrics_extraction/transactions.rs +++ b/relay-server/src/metrics_extraction/transactions.rs @@ -5,6 +5,7 @@ use std::collections::BTreeSet; use { relay_common::UnixTimestamp, relay_general::protocol::{AsPair, Event, EventType}, + relay_general::store::normalize_dist, relay_metrics::{Metric, MetricUnit, MetricValue}, std::collections::BTreeMap, std::fmt::Write, @@ -34,7 +35,7 @@ fn metric_name(parts: &[&str]) -> String { } #[cfg(feature = "processing")] -fn transaction_status(transaction: &Event) -> Option { +fn extract_transaction_status(transaction: &Event) -> Option { use relay_general::{ protocol::{Context, ContextInner}, types::Annotated, @@ -49,6 +50,13 @@ fn transaction_status(transaction: &Event) -> Option { Some(span_status.to_string()) } +#[cfg(feature = "processing")] +fn extract_dist(transaction: &Event) -> Option { + let mut dist = transaction.dist.0.clone(); + normalize_dist(&mut dist); + dist +} + #[cfg(feature = "processing")] pub fn extract_transaction_metrics( config: &TransactionMetricsConfig, @@ -90,6 +98,9 @@ pub fn extract_transaction_metrics( if let Some(release) = event.release.as_str() { tags.insert("release".to_owned(), release.to_owned()); } + if let Some(dist) = extract_dist(event) { + tags.insert("dist".to_owned(), dist); + } if let Some(environment) = event.environment.as_str() { tags.insert("environment".to_owned(), environment.to_owned()); } @@ -182,7 +193,7 @@ pub fn extract_transaction_metrics( unit: MetricUnit::Duration(DurationPrecision::MilliSecond), value: MetricValue::Distribution(duration_millis as f64), timestamp, - tags: match transaction_status(event) { + tags: match extract_transaction_status(event) { Some(status) => with_tag(&tags, "transaction.status", status), None => tags.clone(), }, @@ -225,6 +236,7 @@ mod tests { "type": "transaction", "timestamp": "2021-04-26T08:00:00+0100", "release": "1.2.3", + "dist": "foo ", "environment": "fake_environment", "transaction": "mytransaction", "tags": { @@ -311,6 +323,7 @@ mod tests { for metric in metrics { assert!(matches!(metric.value, MetricValue::Distribution(_))); assert_eq!(metric.tags["release"], "1.2.3"); + assert_eq!(metric.tags["dist"], "foo"); assert_eq!(metric.tags["environment"], "fake_environment"); assert_eq!(metric.tags["transaction"], "mytransaction"); assert_eq!(metric.tags["fOO"], "bar"); @@ -369,6 +382,7 @@ mod tests { } assert_eq!(duration_metric.tags.len(), 4); + assert_eq!(duration_metric.tags["release"], "1.2.3"); assert_eq!(duration_metric.tags["transaction.status"], "ok"); assert_eq!(duration_metric.tags["environment"], "fake_environment"); assert_eq!(duration_metric.tags["transaction"], "mytransaction");