feat(sessions): Retire session duration metric (#1739)

The session duration metric has not been widely used in the product, suffers from inconsistencies between different kinds of sessions (e.g. no duration in request mode and javascript), but at the same time causes considerable storage cost. For these reasons, it has been decided to remove it entirely from session metrics. Fixes #1738
getsentry · Jan 12, 2023 · 84a5ca2 · 84a5ca2
1 parent 156c2ae
commit 84a5ca2
Show file tree

Hide file tree

Showing 4 changed files with 5 additions and 99 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,7 @@
 
 - Add support for `limits.keepalive_timeout` configuration. ([#1645](https://github.com/getsentry/relay/pull/1645))
 - Add support for decaying functions in dynamic sampling rules. ([#1692](https://github.com/getsentry/relay/pull/1692))
+- Stop extracting duration metric for session payloads. ([#1739](https://github.com/getsentry/relay/pull/1739))
 
 **Internal**:
 

diff --git a/relay-general/src/protocol/session.rs b/relay-general/src/protocol/session.rs
@@ -154,7 +154,6 @@ pub trait SessionLike {
     fn abnormal_count(&self) -> u32;
     fn crashed_count(&self) -> u32;
     fn all_errors(&self) -> Option<SessionErrored>;
-    fn final_duration(&self) -> Option<(f64, SessionStatus)>;
     fn abnormal_mechanism(&self) -> AbnormalMechanism;
 }
 
@@ -237,15 +236,6 @@ impl SessionLike for SessionUpdate {
         }
     }
 
-    fn final_duration(&self) -> Option<(f64, SessionStatus)> {
-        if self.status.is_terminal() {
-            if let Some(duration) = self.duration {
-                return Some((duration, self.status));
-            }
-        }
-        None
-    }
-
     fn all_errors(&self) -> Option<SessionErrored> {
         if self.errors > 0 || self.status.is_error() {
             Some(SessionErrored::Individual(self.session_id))
@@ -306,10 +296,6 @@ impl SessionLike for SessionAggregateItem {
         self.crashed
     }
 
-    fn final_duration(&self) -> Option<(f64, SessionStatus)> {
-        None
-    }
-
     fn all_errors(&self) -> Option<SessionErrored> {
         // 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

diff --git a/relay-server/src/metrics_extraction/sessions.rs b/relay-server/src/metrics_extraction/sessions.rs
@@ -4,15 +4,15 @@ use relay_common::{UnixTimestamp, Uuid};
 use relay_general::protocol::{
     AbnormalMechanism, SessionAttributes, SessionErrored, SessionLike, SessionStatus,
 };
-use relay_metrics::{DurationUnit, Metric, MetricNamespace, MetricUnit, MetricValue};
+use relay_metrics::{Metric, MetricNamespace, MetricUnit, MetricValue};
 
 use super::utils::with_tag;
 
 /// Namespace of session metricsfor the MRI.
 const METRIC_NAMESPACE: MetricNamespace = MetricNamespace::Sessions;
 
 /// Current version of metrics extraction.
-const EXTRACT_VERSION: u16 = 2;
+const EXTRACT_VERSION: u16 = 3;
 const EXTRACT_ABNORMAL_MECHANISM_VERSION: u16 = 2;
 
 /// Configuration for metric extraction from sessions.
@@ -201,20 +201,6 @@ pub fn extract_session_metrics<T: SessionLike>(
             ));
         }
     }
-
-    // Count durations only for exited sessions, since Sentry doesn't use durations for other types of sessions.
-    if let Some((duration, status)) = session.final_duration() {
-        if status == SessionStatus::Exited {
-            target.push(Metric::new_mri(
-                METRIC_NAMESPACE,
-                "duration",
-                MetricUnit::Duration(DurationUnit::Second),
-                MetricValue::Distribution(duration),
-                timestamp,
-                with_tag(&tags, "session.status", status),
-            ));
-        }
-    }
 }
 
 #[cfg(test)]
@@ -475,42 +461,6 @@ mod tests {
         }
     }
 
-    #[test]
-    fn test_extract_session_metrics_duration() {
-        let mut metrics = vec![];
-
-        let session = SessionUpdate::parse(
-            r#"{
-            "init": false,
-            "started": "2021-04-26T08:00:00+0100",
-            "attrs": {
-                "release": "1.0.0"
-            },
-            "did": "user123",
-            "status": "exited",
-            "duration": 123.4
-        }"#
-            .as_bytes(),
-        )
-        .unwrap();
-
-        extract_session_metrics(&session.attributes, &session, None, &mut metrics, true);
-
-        assert_eq!(metrics.len(), 2); // duration and user ID
-
-        let duration_metric = &metrics[1];
-        assert_eq!(duration_metric.name, "d:sessions/duration@second");
-        assert!(matches!(
-            duration_metric.value,
-            MetricValue::Distribution(_)
-        ));
-
-        let user_metric = &metrics[0];
-        assert_eq!(user_metric.name, "s:sessions/user@none");
-        assert!(matches!(user_metric.value, MetricValue::Set(_)));
-        assert!(!user_metric.tags.contains_key("session.status"));
-    }
-
     #[test]
     fn test_extract_session_metrics_aggregate() {
         let mut metrics = vec![];

diff --git a/tests/integration/test_metrics.py b/tests/integration/test_metrics.py
@@ -365,19 +365,6 @@ def test_session_metrics_non_processing(
                 "type": "c",
                 "value": 1.0,
             },
-            {
-                "name": "d:sessions/duration@second",
-                "tags": {
-                    "sdk": "raven-node/2.6.3",
-                    "environment": "production",
-                    "release": "sentry-test@1.0.0",
-                    "session.status": "exited",
-                },
-                "timestamp": ts,
-                "width": 1,
-                "type": "d",
-                "value": [1947.49],
-            },
             {
                 "name": "s:sessions/user@none",
                 "tags": {
@@ -437,14 +424,11 @@ def test_session_metrics_extracted_only_once(
 
     relay_chain.send_session(project_id, session_payload)
 
-    metrics = metrics_by_name(metrics_consumer, 3, timeout=6)
+    metrics = metrics_by_name(metrics_consumer, 2, timeout=6)
 
     # if it is not 1 it means the session was extracted multiple times
     assert metrics["c:sessions/session@none"]["value"] == 1.0
 
-    # if the vector contains multiple duration we have the session extracted multiple times
-    assert len(metrics["d:sessions/duration@second"]["value"]) == 1
-
 
 @pytest.mark.parametrize(
     "metrics_extracted", [True, False], ids=["extracted", "not extracted"]
@@ -480,7 +464,7 @@ def test_session_metrics_processing(
         metrics_consumer.assert_empty(timeout=2)
         return
 
-    metrics = metrics_by_name(metrics_consumer, 3)
+    metrics = metrics_by_name(metrics_consumer, 2)
 
     expected_timestamp = int(started.timestamp())
     assert metrics["c:sessions/session@none"] == {
@@ -512,21 +496,6 @@ def test_session_metrics_processing(
         },
     }
 
-    assert metrics["d:sessions/duration@second"] == {
-        "org_id": 1,
-        "project_id": 42,
-        "timestamp": expected_timestamp,
-        "name": "d:sessions/duration@second",
-        "type": "d",
-        "value": [1947.49],
-        "tags": {
-            "sdk": "raven-node/2.6.3",
-            "environment": "production",
-            "release": "sentry-test@1.0.0",
-            "session.status": "exited",
-        },
-    }
-
 
 @pytest.mark.parametrize(
     "extract_metrics,discard_data,with_external_relay",