From 65814ff7d87387070fef3c572bfb897a42541cf5 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Wed, 18 May 2022 14:19:06 +0200 Subject: [PATCH 1/7] fix(metrics): Count preaggregated crashed+abnormal towards errored_preaggr --- relay-general/src/protocol/session.rs | 16 ++++++++++------ relay-server/src/metrics_extraction/sessions.rs | 16 +++++++++++++++- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/relay-general/src/protocol/session.rs b/relay-general/src/protocol/session.rs index 373757c107..8898e742b0 100644 --- a/relay-general/src/protocol/session.rs +++ b/relay-general/src/protocol/session.rs @@ -96,7 +96,8 @@ fn is_false(val: &bool) -> bool { pub enum SessionErrored { /// Contains the UUID for a single errored session. Individual(Uuid), - /// Contains the number of errored sessions in an aggregate. + /// Contains the number of all errored sessions in an aggregate. + /// errored, crashed, abnormal all count towards errored sessions. Aggregated(u32), } @@ -107,7 +108,7 @@ pub trait SessionLike { fn total_count(&self) -> u32; fn abnormal_count(&self) -> u32; fn crashed_count(&self) -> u32; - fn errors(&self) -> Option; + fn all_errors(&self) -> Option; fn final_duration(&self) -> Option<(f64, SessionStatus)>; } @@ -196,7 +197,7 @@ impl SessionLike for SessionUpdate { None } - fn errors(&self) -> Option { + fn all_errors(&self) -> Option { if self.errors > 0 || self.status.is_error() { Some(SessionErrored::Individual(self.session_id)) } else { @@ -256,9 +257,12 @@ impl SessionLike for SessionAggregateItem { None } - fn errors(&self) -> Option { - if self.errored > 0 { - Some(SessionErrored::Aggregated(self.errored)) + fn all_errors(&self) -> Option { + // Errors contain crashed & abnormal as well. + // See https://github.com/getsentry/snuba/blob/c45f2a8636f9ea3dfada4e2d0ae5efef6c6248de/snuba/migrations/snuba_migrations/sessions/0003_sessions_matview.py#L80-L81 + let all_errored = self.abnormal + self.crashed + self.errored; + if all_errored > 0 { + Some(SessionErrored::Aggregated(all_errored)) } else { None } diff --git a/relay-server/src/metrics_extraction/sessions.rs b/relay-server/src/metrics_extraction/sessions.rs index 31167ecf7d..751563889b 100644 --- a/relay-server/src/metrics_extraction/sessions.rs +++ b/relay-server/src/metrics_extraction/sessions.rs @@ -106,7 +106,7 @@ pub fn extract_session_metrics( } // Mark the session as errored, which includes fatal sessions. - if let Some(errors) = session.errors() { + if let Some(errors) = session.all_errors() { target.push(match errors { SessionErrored::Individual(session_id) => Metric::new_mri( METRIC_NAMESPACE, @@ -466,6 +466,20 @@ mod tests { "session.status": "init", }, }, + Metric { + name: "c:sessions/session@none", + unit: None, + value: Counter( + 12.0, + ), + timestamp: UnixTimestamp(1581084960), + tags: { + "environment": "development", + "release": "my-project-name@1.0.0", + "sdk": "sentry-test/1.0", + "session.status": "errored_preaggr", + }, + }, Metric { name: "c:sessions/session@none", unit: None, From f958659e5eea34207fdca5dd132c97acd92c9a81 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Wed, 18 May 2022 15:43:13 +0200 Subject: [PATCH 2/7] doc: changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index aed3494e12..cba0d7361d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ **Compatibility:** This version of Relay requires Sentry server `22.6.0` or newer. +**Features**: + +- Session metrics extraction: Count crashed+abnormal towards errored_preaggr. ([#1274](https://github.com/getsentry/relay/pull/1274)) + **Internal**: - Add version 3 to the project configs endpoint. This allows returning pending results which need to be polled later and avoids blocking batched requests on single slow entries. ([#1263](https://github.com/getsentry/relay/pull/1263)) From 0bad9407d36b3353a72686e6c6749f612b3a9c02 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Wed, 18 May 2022 16:08:10 +0200 Subject: [PATCH 3/7] feat(metrics): Always extract distinct_id --- .../src/metrics_extraction/sessions.rs | 27 +++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/relay-server/src/metrics_extraction/sessions.rs b/relay-server/src/metrics_extraction/sessions.rs index 751563889b..81e1f5c240 100644 --- a/relay-server/src/metrics_extraction/sessions.rs +++ b/relay-server/src/metrics_extraction/sessions.rs @@ -197,6 +197,25 @@ pub fn extract_session_metrics( with_tag(&tags, "session.status", status), )); } + + // Some SDKs only provide user information on subsequent updates (not when init==true), so + // collect the user ID here as well. + // We could also add the user ID to the "init" tag, but collecting it under a separate "ok" tag + // might help us with validation / debugging. + if let Some(distinct_id) = nil_to_none(session.distinct_id()) { + if session.total_count() == 0 && matches!(session.all_errors(), None) { + // Assuming that aggregate items always have a total_count > 0, + // this is a session update to a healthy, individual session (init == false). + target.push(Metric::new_mri( + METRIC_NAMESPACE, + "user", + MetricUnit::None, + MetricValue::set_from_str(distinct_id), + timestamp, + with_tag(&tags, "session.status", SessionStatus::Ok), + )); + } + } } #[cfg(test)] @@ -292,8 +311,12 @@ mod tests { extract_session_metrics(&session.attributes, &session, None, &mut metrics); - // A none-initial update will not trigger any metric if it's not errored/crashed - assert_eq!(metrics.len(), 0); + // A none-initial update which is not errored/crashed/abnormal will only emit a user metric. + assert_eq!(metrics.len(), 1); + let user_metric = &metrics[0]; + assert_eq!(user_metric.name, "s:sessions/user@none"); + assert!(matches!(user_metric.value, MetricValue::Set(_))); + assert_eq!(user_metric.tags["session.status"], "ok"); } #[test] From 08a3fd1b590688410d48584ada2affb6312a0cb8 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Wed, 18 May 2022 16:27:06 +0200 Subject: [PATCH 4/7] doc: changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cba0d7361d..14d009e7f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ **Features**: +- Session metrics extraction: Always extract distinct_id. ([#1275](https://github.com/getsentry/relay/pull/1275)) + +**Bug Fixes**: + - Session metrics extraction: Count crashed+abnormal towards errored_preaggr. ([#1274](https://github.com/getsentry/relay/pull/1274)) **Internal**: From 193fd09d183d14a936ff4b8624f6888e4709002f Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Wed, 18 May 2022 17:00:43 +0200 Subject: [PATCH 5/7] fix: test --- relay-server/src/metrics_extraction/sessions.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/relay-server/src/metrics_extraction/sessions.rs b/relay-server/src/metrics_extraction/sessions.rs index 81e1f5c240..4fce7e741a 100644 --- a/relay-server/src/metrics_extraction/sessions.rs +++ b/relay-server/src/metrics_extraction/sessions.rs @@ -429,7 +429,7 @@ mod tests { extract_session_metrics(&session.attributes, &session, None, &mut metrics); - assert_eq!(metrics.len(), 1); + assert_eq!(metrics.len(), 2); // duration and user ID let duration_metric = &metrics[0]; assert_eq!(duration_metric.name, "d:sessions/duration@second"); @@ -437,6 +437,11 @@ mod tests { duration_metric.value, MetricValue::Distribution(_) )); + + let user_metric = &metrics[1]; + assert_eq!(user_metric.name, "s:sessions/user@none"); + assert!(matches!(user_metric.value, MetricValue::Set(_))); + assert_eq!(user_metric.tags["session.status"], "ok"); } #[test] From f1e0697494a5805f8107f820db13a5574d9c321c Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Thu, 19 May 2022 08:53:33 +0200 Subject: [PATCH 6/7] Update relay-server/src/metrics_extraction/sessions.rs Co-authored-by: Jan Michael Auer --- relay-server/src/metrics_extraction/sessions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/relay-server/src/metrics_extraction/sessions.rs b/relay-server/src/metrics_extraction/sessions.rs index 4fce7e741a..6ff7aad944 100644 --- a/relay-server/src/metrics_extraction/sessions.rs +++ b/relay-server/src/metrics_extraction/sessions.rs @@ -203,7 +203,7 @@ pub fn extract_session_metrics( // We could also add the user ID to the "init" tag, but collecting it under a separate "ok" tag // might help us with validation / debugging. if let Some(distinct_id) = nil_to_none(session.distinct_id()) { - if session.total_count() == 0 && matches!(session.all_errors(), None) { + if session.total_count() == 0 && session.all_errors().is_none() { // Assuming that aggregate items always have a total_count > 0, // this is a session update to a healthy, individual session (init == false). target.push(Metric::new_mri( From 243e85292d785a1010256eb6b2e1748191ec6708 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Thu, 19 May 2022 08:56:55 +0200 Subject: [PATCH 7/7] doc: Update changelog --- CHANGELOG.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14d009e7f4..2b0da4287d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,12 +4,10 @@ **Compatibility:** This version of Relay requires Sentry server `22.6.0` or newer. -**Features**: - -- Session metrics extraction: Always extract distinct_id. ([#1275](https://github.com/getsentry/relay/pull/1275)) - **Bug Fixes**: +- Session metrics extraction: Count distinct_ids from all session updates to prevent undercounting users. ([#1275](https://github.com/getsentry/relay/pull/1275)) + - Session metrics extraction: Count crashed+abnormal towards errored_preaggr. ([#1274](https://github.com/getsentry/relay/pull/1274)) **Internal**: