diff --git a/CHANGELOG.md b/CHANGELOG.md index c3fe611b218..9bc9e14368e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ - Remove unused item types. ([#1211](https://github.com/getsentry/relay/pull/1211)) - Pin click dependency in requirements-dev.txt. ([#1214](https://github.com/getsentry/relay/pull/1214)) +- Use fully qualified metric resource identifiers (MRI) for metrics ingestion. For example, the sessions duration is now called `d:sessions/duration@s`. ([#1215](https://github.com/getsentry/relay/pull/1215)) ## 22.3.0 diff --git a/relay-metrics/src/protocol.rs b/relay-metrics/src/protocol.rs index 8e74fa95ae7..dcc1d5302af 100644 --- a/relay-metrics/src/protocol.rs +++ b/relay-metrics/src/protocol.rs @@ -407,6 +407,27 @@ pub struct Metric { } impl Metric { + /// Creates a new metric using the MRI naming format. + /// + /// MRI is the metric resource identifier in the format `:/@`. This name + /// ensures that just the name determines correct bucketing of metrics with name collisions. + pub fn new_mri( + namespace: impl fmt::Display, + name: impl fmt::Display, + unit: MetricUnit, + value: MetricValue, + timestamp: UnixTimestamp, + tags: BTreeMap, + ) -> Self { + Self { + name: format!("{}:{}/{}@{}", value.ty(), namespace, name, unit), + unit, + value, + timestamp, + tags, + } + } + fn parse_str(string: &str, timestamp: UnixTimestamp) -> Option { let mut components = string.split('|'); diff --git a/relay-server/src/metrics_extraction/sessions.rs b/relay-server/src/metrics_extraction/sessions.rs index 6234c7d3fd9..6d26a0c3fb0 100644 --- a/relay-server/src/metrics_extraction/sessions.rs +++ b/relay-server/src/metrics_extraction/sessions.rs @@ -18,11 +18,7 @@ fn nil_to_none(distinct_id: Option<&String>) -> Option<&String> { Some(distinct_id) } -/// Generate a sessions-related metric name -/// Would be nice to have this as a `const fn`, but [`Metric`] requires a [`String`] anyway. -fn metric_name(name: &str) -> String { - format!("sentry.sessions.{}", name) -} +const METRIC_NAMESPACE: &str = "sessions"; pub fn extract_session_metrics( attributes: &SessionAttributes, @@ -46,110 +42,119 @@ pub fn extract_session_metrics( // Always capture with "init" tag for the first session update of a session. This is used // for adoption and as baseline for crash rates. if session.total_count() > 0 { - target.push(Metric { - name: metric_name("session"), - unit: MetricUnit::None, - value: MetricValue::Counter(session.total_count() as f64), + target.push(Metric::new_mri( + METRIC_NAMESPACE, + "session", + MetricUnit::None, + MetricValue::Counter(session.total_count() as f64), timestamp, - tags: with_tag(&tags, "session.status", "init"), - }); + with_tag(&tags, "session.status", "init"), + )); if let Some(distinct_id) = nil_to_none(session.distinct_id()) { - target.push(Metric { - name: metric_name("user"), - unit: MetricUnit::None, - value: MetricValue::set_from_str(distinct_id), + target.push(Metric::new_mri( + METRIC_NAMESPACE, + "user", + MetricUnit::None, + MetricValue::set_from_str(distinct_id), timestamp, - tags: with_tag(&tags, "session.status", "init"), - }); + with_tag(&tags, "session.status", "init"), + )); } } // Mark the session as errored, which includes fatal sessions. if let Some(errors) = session.errors() { target.push(match errors { - SessionErrored::Individual(session_id) => Metric { - name: metric_name("session.error"), - unit: MetricUnit::None, - value: MetricValue::set_from_display(session_id), + SessionErrored::Individual(session_id) => Metric::new_mri( + METRIC_NAMESPACE, + "error", + MetricUnit::None, + MetricValue::set_from_display(session_id), timestamp, - tags: tags.clone(), - }, - SessionErrored::Aggregated(count) => Metric { - name: metric_name("session"), - unit: MetricUnit::None, - value: MetricValue::Counter(count as f64), + tags.clone(), + ), + SessionErrored::Aggregated(count) => Metric::new_mri( + METRIC_NAMESPACE, + "session", + MetricUnit::None, + MetricValue::Counter(count as f64), timestamp, - tags: with_tag(&tags, "session.status", "errored_preaggr"), - }, + with_tag(&tags, "session.status", "errored_preaggr"), + ), }); if let Some(distinct_id) = nil_to_none(session.distinct_id()) { - target.push(Metric { - name: metric_name("user"), - unit: MetricUnit::None, - value: MetricValue::set_from_str(distinct_id), + target.push(Metric::new_mri( + METRIC_NAMESPACE, + "user", + MetricUnit::None, + MetricValue::set_from_str(distinct_id), timestamp, - tags: with_tag(&tags, "session.status", "errored"), - }); + with_tag(&tags, "session.status", "errored"), + )); } } // Record fatal sessions for crash rate computation. This is a strict subset of errored // sessions above. if session.abnormal_count() > 0 { - target.push(Metric { - name: metric_name("session"), - unit: MetricUnit::None, - value: MetricValue::Counter(session.abnormal_count() as f64), + target.push(Metric::new_mri( + METRIC_NAMESPACE, + "session", + MetricUnit::None, + MetricValue::Counter(session.abnormal_count() as f64), timestamp, - tags: with_tag(&tags, "session.status", SessionStatus::Abnormal), - }); + with_tag(&tags, "session.status", SessionStatus::Abnormal), + )); if let Some(distinct_id) = nil_to_none(session.distinct_id()) { - target.push(Metric { - name: metric_name("user"), - unit: MetricUnit::None, - value: MetricValue::set_from_str(distinct_id), + target.push(Metric::new_mri( + METRIC_NAMESPACE, + "user", + MetricUnit::None, + MetricValue::set_from_str(distinct_id), timestamp, - tags: with_tag(&tags, "session.status", SessionStatus::Abnormal), - }); + with_tag(&tags, "session.status", SessionStatus::Abnormal), + )); } } + if session.crashed_count() > 0 { - target.push(Metric { - name: metric_name("session"), - unit: MetricUnit::None, - value: MetricValue::Counter(session.crashed_count() as f64), + target.push(Metric::new_mri( + METRIC_NAMESPACE, + "session", + MetricUnit::None, + MetricValue::Counter(session.crashed_count() as f64), timestamp, - tags: with_tag(&tags, "session.status", SessionStatus::Crashed), - }); + with_tag(&tags, "session.status", SessionStatus::Crashed), + )); if let Some(distinct_id) = nil_to_none(session.distinct_id()) { - target.push(Metric { - name: metric_name("user"), - unit: MetricUnit::None, - value: MetricValue::set_from_str(distinct_id), + target.push(Metric::new_mri( + METRIC_NAMESPACE, + "user", + MetricUnit::None, + MetricValue::set_from_str(distinct_id), timestamp, - tags: with_tag(&tags, "session.status", SessionStatus::Crashed), - }); + with_tag(&tags, "session.status", SessionStatus::Crashed), + )); } } // Count durations for all exited/crashed sessions. Note that right now, in the product we // really only use durations from session.status=exited, but decided it may be worth ingesting // this data in case we need it. If we need to cut cost, this is one place to start though. - // if session.status.is_terminal() { if let Some((duration, status)) = session.final_duration() { - target.push(Metric { - name: metric_name("session.duration"), - unit: MetricUnit::Duration(DurationPrecision::Second), - value: MetricValue::Distribution(duration), + target.push(Metric::new_mri( + METRIC_NAMESPACE, + "duration", + MetricUnit::Duration(DurationPrecision::Second), + MetricValue::Distribution(duration), timestamp, - tags: with_tag(&tags, "session.status", status), - }); + with_tag(&tags, "session.status", status), + )); } - // } } #[cfg(test)] @@ -210,14 +215,14 @@ mod tests { let session_metric = &metrics[0]; assert_eq!(session_metric.timestamp, started()); - assert_eq!(session_metric.name, "sentry.sessions.session"); + assert_eq!(session_metric.name, "c:sessions/session@"); assert!(matches!(session_metric.value, MetricValue::Counter(_))); assert_eq!(session_metric.tags["session.status"], "init"); assert_eq!(session_metric.tags["release"], "1.0.0"); let user_metric = &metrics[1]; assert_eq!(session_metric.timestamp, started()); - assert_eq!(user_metric.name, "sentry.sessions.user"); + assert_eq!(user_metric.name, "s:sessions/user@"); assert!(matches!(user_metric.value, MetricValue::Set(_))); assert_eq!(session_metric.tags["session.status"], "init"); assert_eq!(user_metric.tags["release"], "1.0.0"); @@ -281,13 +286,13 @@ mod tests { let session_metric = &metrics[expected_metrics - 2]; assert_eq!(session_metric.timestamp, started()); - assert_eq!(session_metric.name, "sentry.sessions.session.error"); + assert_eq!(session_metric.name, "s:sessions/error@"); assert!(matches!(session_metric.value, MetricValue::Set(_))); assert_eq!(session_metric.tags.len(), 1); // Only the release tag let user_metric = &metrics[expected_metrics - 1]; assert_eq!(session_metric.timestamp, started()); - assert_eq!(user_metric.name, "sentry.sessions.user"); + assert_eq!(user_metric.name, "s:sessions/user@"); assert!(matches!(user_metric.value, MetricValue::Set(_))); assert_eq!(user_metric.tags["session.status"], "errored"); assert_eq!(user_metric.tags["release"], "1.0.0"); @@ -317,19 +322,19 @@ mod tests { assert_eq!(metrics.len(), 4); - assert_eq!(metrics[0].name, "sentry.sessions.session.error"); - assert_eq!(metrics[1].name, "sentry.sessions.user"); + assert_eq!(metrics[0].name, "s:sessions/error@"); + assert_eq!(metrics[1].name, "s:sessions/user@"); assert_eq!(metrics[1].tags["session.status"], "errored"); let session_metric = &metrics[2]; assert_eq!(session_metric.timestamp, started()); - assert_eq!(session_metric.name, "sentry.sessions.session"); + assert_eq!(session_metric.name, "c:sessions/session@"); assert!(matches!(session_metric.value, MetricValue::Counter(_))); assert_eq!(session_metric.tags["session.status"], status.to_string()); let user_metric = &metrics[3]; assert_eq!(session_metric.timestamp, started()); - assert_eq!(user_metric.name, "sentry.sessions.user"); + assert_eq!(user_metric.name, "s:sessions/user@"); assert!(matches!(user_metric.value, MetricValue::Set(_))); assert_eq!(user_metric.tags["session.status"], status.to_string()); } @@ -359,7 +364,7 @@ mod tests { assert_eq!(metrics.len(), 1); let duration_metric = &metrics[0]; - assert_eq!(duration_metric.name, "sentry.sessions.session.duration"); + assert_eq!(duration_metric.name, "d:sessions/duration@s"); assert!(matches!( duration_metric.value, MetricValue::Distribution(_) @@ -402,7 +407,7 @@ mod tests { insta::assert_debug_snapshot!(metrics, @r###" [ Metric { - name: "sentry.sessions.session", + name: "c:sessions/session@", unit: None, value: Counter( 135.0, @@ -415,7 +420,7 @@ mod tests { }, }, Metric { - name: "sentry.sessions.session", + name: "c:sessions/session@", unit: None, value: Counter( 5.0, @@ -428,7 +433,7 @@ mod tests { }, }, Metric { - name: "sentry.sessions.session", + name: "c:sessions/session@", unit: None, value: Counter( 7.0, @@ -441,7 +446,7 @@ mod tests { }, }, Metric { - name: "sentry.sessions.session", + name: "c:sessions/session@", unit: None, value: Counter( 15.0, @@ -454,7 +459,7 @@ mod tests { }, }, Metric { - name: "sentry.sessions.user", + name: "s:sessions/user@", unit: None, value: Set( 3097475539, @@ -467,7 +472,7 @@ mod tests { }, }, Metric { - name: "sentry.sessions.session", + name: "c:sessions/session@", unit: None, value: Counter( 3.0, @@ -480,7 +485,7 @@ mod tests { }, }, Metric { - name: "sentry.sessions.user", + name: "s:sessions/user@", unit: None, value: Set( 3097475539, diff --git a/relay-server/src/metrics_extraction/transactions.rs b/relay-server/src/metrics_extraction/transactions.rs index ff8a48ebf2f..c20cc278386 100644 --- a/relay-server/src/metrics_extraction/transactions.rs +++ b/relay-server/src/metrics_extraction/transactions.rs @@ -10,7 +10,6 @@ use { }, relay_metrics::{Metric, MetricUnit, MetricValue}, std::fmt, - std::fmt::Write, }; /// The metric on which the user satisfaction threshold is applied. @@ -51,19 +50,7 @@ pub struct TransactionMetricsConfig { } #[cfg(feature = "processing")] -const METRIC_NAME_PREFIX: &str = "sentry.transactions"; - -/// Generate a transaction-related metric name -#[cfg(feature = "processing")] -fn metric_name(parts: &[&str]) -> String { - let mut name = METRIC_NAME_PREFIX.to_owned(); - for part in parts { - // Unwrapping here should be fine: - // https://github.com/rust-lang/rust/blob/1.57.0/library/alloc/src/string.rs#L2721-L2724 - write!(name, ".{}", part).unwrap(); - } - name -} +const METRIC_NAMESPACE: &str = "transactions"; #[cfg(feature = "processing")] fn extract_transaction_status(transaction: &Event) -> Option { @@ -259,38 +246,39 @@ pub fn extract_transaction_metrics( None => continue, }; - let name = metric_name(&["measurements", measurement_name]); let mut tags = tags.clone(); if let Some(rating) = get_measurement_rating(measurement_name, measurement) { tags.insert("measurement_rating".to_owned(), rating); } - push_metric(Metric { - name, - unit: MetricUnit::None, - value: MetricValue::Distribution(measurement), - timestamp: unix_timestamp, + push_metric(Metric::new_mri( + METRIC_NAMESPACE, + format_args!("measurements.{}", measurement_name), + MetricUnit::None, + MetricValue::Distribution(measurement), + unix_timestamp, tags, - }); + )); } } // Breakdowns if let Some(breakdowns_config) = breakdowns_config { - for (breakdown_name, measurements) in get_breakdown_measurements(event, breakdowns_config) { + for (breakdown, measurements) in get_breakdown_measurements(event, breakdowns_config) { for (measurement_name, annotated) in measurements.iter() { let measurement = match annotated.value().and_then(|m| m.value.value()) { Some(measurement) => *measurement, None => continue, }; - push_metric(Metric { - name: metric_name(&["breakdowns", breakdown_name, measurement_name]), - unit: MetricUnit::None, - value: MetricValue::Distribution(measurement), - timestamp: unix_timestamp, - tags: tags.clone(), - }); + push_metric(Metric::new_mri( + METRIC_NAMESPACE, + format_args!("breakdowns.{}.{}", breakdown, measurement_name), + MetricUnit::None, + MetricValue::Distribution(measurement), + unix_timestamp, + tags.clone(), + )); } } } @@ -309,32 +297,34 @@ pub fn extract_transaction_metrics( // Duration let duration_millis = get_duration_millis(start_timestamp, end_timestamp); - push_metric(Metric { - name: metric_name(&["transaction.duration"]), - unit: MetricUnit::Duration(DurationPrecision::MilliSecond), - value: MetricValue::Distribution(duration_millis), - timestamp: unix_timestamp, - tags: match extract_transaction_status(event) { + push_metric(Metric::new_mri( + METRIC_NAMESPACE, + "duration", + MetricUnit::Duration(DurationPrecision::MilliSecond), + MetricValue::Distribution(duration_millis), + unix_timestamp, + match extract_transaction_status(event) { Some(status) => with_tag(&tags, "transaction.status", status), None => tags_with_satisfaction.clone(), }, - }); + )); // User if let Some(user) = event.user.value() { if let Some(user_id) = user.id.as_str() { - push_metric(Metric { - name: metric_name(&["user"]), - unit: MetricUnit::None, - value: MetricValue::set_from_str(user_id), - timestamp: unix_timestamp, + push_metric(Metric::new_mri( + METRIC_NAMESPACE, + "user", + MetricUnit::None, + MetricValue::set_from_str(user_id), + unix_timestamp, // A single user might end up in multiple satisfaction buckets when they have // some satisfying transactions and some frustrating transactions. // This is OK as long as we do not add these numbers *after* aggregation: // total_users = uniqIf(user, satisfied) + uniqIf(user, tolerated) + uniqIf(user, frustrated) // total_users = uniq(user) - tags: tags_with_satisfaction, - }); + tags_with_satisfaction, + )); } } @@ -435,11 +425,11 @@ mod tests { r#" { "extractMetrics": [ - "sentry.transactions.measurements.foo", - "sentry.transactions.measurements.lcp", - "sentry.transactions.breakdowns.span_ops.ops.react.mount", - "sentry.transactions.transaction.duration", - "sentry.transactions.user" + "d:transactions/measurements.foo@", + "d:transactions/measurements.lcp@", + "d:transactions/breakdowns.span_ops.ops.react.mount@", + "d:transactions/duration@ms", + "s:transactions/user@" ], "extractCustomTags": ["fOO"] } @@ -457,18 +447,15 @@ mod tests { assert_eq!(metrics.len(), 5, "{:?}", metrics); - assert_eq!(metrics[0].name, "sentry.transactions.measurements.foo"); - assert_eq!(metrics[1].name, "sentry.transactions.measurements.lcp"); + assert_eq!(metrics[0].name, "d:transactions/measurements.foo@"); + assert_eq!(metrics[1].name, "d:transactions/measurements.lcp@"); assert_eq!( metrics[2].name, - "sentry.transactions.breakdowns.span_ops.ops.react.mount" + "d:transactions/breakdowns.span_ops.ops.react.mount@" ); let duration_metric = &metrics[3]; - assert_eq!( - duration_metric.name, - "sentry.transactions.transaction.duration" - ); + assert_eq!(duration_metric.name, "d:transactions/duration@ms"); if let MetricValue::Distribution(value) = duration_metric.value { assert_eq!(value, 59000.0); } else { @@ -476,7 +463,7 @@ mod tests { } let user_metric = &metrics[4]; - assert_eq!(user_metric.name, "sentry.transactions.user"); + assert_eq!(user_metric.name, "s:transactions/user@"); assert!(matches!(user_metric.value, MetricValue::Set(_))); assert_eq!(metrics[1].tags["measurement_rating"], "meh"); @@ -519,7 +506,7 @@ mod tests { r#" { "extractMetrics": [ - "sentry.transactions.transaction.duration" + "d:transactions/duration@ms" ] } "#, @@ -531,10 +518,7 @@ mod tests { assert_eq!(metrics.len(), 1); let duration_metric = &metrics[0]; - assert_eq!( - duration_metric.name, - "sentry.transactions.transaction.duration" - ); + assert_eq!(duration_metric.name, "d:transactions/duration@ms"); assert_eq!( duration_metric.unit, MetricUnit::Duration(DurationPrecision::MilliSecond) @@ -572,8 +556,8 @@ mod tests { r#" { "extractMetrics": [ - "sentry.transactions.transaction.duration", - "sentry.transactions.user" + "d:transactions/duration@ms", + "s:transactions/user@" ], "satisfactionThresholds": { "projectThreshold": { @@ -616,7 +600,7 @@ mod tests { r#" { "extractMetrics": [ - "sentry.transactions.transaction.duration" + "d:transactions/duration@ms" ], "satisfactionThresholds": { "projectThreshold": { @@ -664,7 +648,7 @@ mod tests { r#" { "extractMetrics": [ - "sentry.transactions.transaction.duration" + "d:transactions/duration@ms" ], "satisfactionThresholds": { "projectThreshold": { diff --git a/tests/integration/test_metrics.py b/tests/integration/test_metrics.py index 50d04a0b34b..a4b0191bb6b 100644 --- a/tests/integration/test_metrics.py +++ b/tests/integration/test_metrics.py @@ -231,7 +231,7 @@ def test_session_metrics_non_processing( ts = int(started.timestamp()) assert session_metrics == [ { - "name": "sentry.sessions.session", + "name": "c:sessions/session@", "tags": { "environment": "production", "release": "sentry-test@1.0.0", @@ -243,7 +243,7 @@ def test_session_metrics_non_processing( "value": 1.0, }, { - "name": "sentry.sessions.session.duration", + "name": "d:sessions/duration@s", "tags": { "environment": "production", "release": "sentry-test@1.0.0", @@ -256,7 +256,7 @@ def test_session_metrics_non_processing( "value": [1947.49], }, { - "name": "sentry.sessions.user", + "name": "s:sessions/user@", "tags": { "environment": "production", "release": "sentry-test@1.0.0", @@ -317,10 +317,10 @@ def test_metrics_extracted_only_once( metrics = metrics_by_name(metrics_consumer, 3, timeout=6) # if it is not 1 it means the session was extracted multiple times - assert metrics["sentry.sessions.session"]["value"] == 1.0 + assert metrics["c:sessions/session@"]["value"] == 1.0 # if the vector contains multiple duration we have the session extracted multiple times - assert len(metrics["sentry.sessions.session.duration"]["value"]) == 1 + assert len(metrics["d:sessions/duration@s"]["value"]) == 1 @pytest.mark.parametrize( @@ -360,11 +360,11 @@ def test_session_metrics_processing( metrics = metrics_by_name(metrics_consumer, 3) expected_timestamp = int(started.timestamp()) - assert metrics["sentry.sessions.session"] == { + assert metrics["c:sessions/session@"] == { "org_id": 1, "project_id": 42, "timestamp": expected_timestamp, - "name": "sentry.sessions.session", + "name": "c:sessions/session@", "type": "c", "unit": "", "value": 1.0, @@ -375,11 +375,11 @@ def test_session_metrics_processing( }, } - assert metrics["sentry.sessions.user"] == { + assert metrics["s:sessions/user@"] == { "org_id": 1, "project_id": 42, "timestamp": expected_timestamp, - "name": "sentry.sessions.user", + "name": "s:sessions/user@", "type": "s", "unit": "", "value": [1617781333], @@ -390,11 +390,11 @@ def test_session_metrics_processing( }, } - assert metrics["sentry.sessions.session.duration"] == { + assert metrics["d:sessions/duration@s"] == { "org_id": 1, "project_id": 42, "timestamp": expected_timestamp, - "name": "sentry.sessions.session.duration", + "name": "d:sessions/duration@s", "type": "d", "unit": "s", "value": [1947.49], @@ -464,10 +464,10 @@ def test_transaction_metrics( elif extract_metrics: config["transactionMetrics"] = { "extractMetrics": [ - "sentry.transactions.measurements.foo", - "sentry.transactions.measurements.bar", - "sentry.transactions.breakdowns.span_ops.total.time", - "sentry.transactions.breakdowns.span_ops.ops.react.mount", + "d:transactions/measurements.foo@", + "d:transactions/measurements.bar@", + "d:transactions/breakdowns.span_ops.total.time@", + "d:transactions/breakdowns.span_ops.ops.react.mount@", ] } @@ -512,33 +512,33 @@ def test_transaction_metrics( "tags": {"transaction": "/organizations/:orgId/performance/:eventSlug/"}, } - assert metrics["sentry.transactions.measurements.foo"] == { + assert metrics["d:transactions/measurements.foo@"] == { **common, - "name": "sentry.transactions.measurements.foo", + "name": "d:transactions/measurements.foo@", "type": "d", "unit": "", "value": [1.2, 2.2], } - assert metrics["sentry.transactions.measurements.bar"] == { + assert metrics["d:transactions/measurements.bar@"] == { **common, - "name": "sentry.transactions.measurements.bar", + "name": "d:transactions/measurements.bar@", "type": "d", "unit": "", "value": [1.3], } - assert metrics["sentry.transactions.breakdowns.span_ops.ops.react.mount"] == { + assert metrics["d:transactions/breakdowns.span_ops.ops.react.mount@"] == { **common, - "name": "sentry.transactions.breakdowns.span_ops.ops.react.mount", + "name": "d:transactions/breakdowns.span_ops.ops.react.mount@", "type": "d", "unit": "", "value": [9.910106, 9.910106], } - assert metrics["sentry.transactions.breakdowns.span_ops.total.time"] == { + assert metrics["d:transactions/breakdowns.span_ops.total.time@"] == { **common, - "name": "sentry.transactions.breakdowns.span_ops.total.time", + "name": "d:transactions/breakdowns.span_ops.total.time@", "type": "d", "unit": "", "value": [9.910106, 9.910106],