Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sessions): Retire session duration metric #1739

Merged
merged 3 commits into from
Jan 12, 2023
Merged

feat(sessions): Retire session duration metric #1739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6405712
feat(sessions): Retire session duration metric
jjbayer Jan 12, 2023
5e65234
ref: Bump session extraction version so we turn it off in external re…
jjbayer Jan 12, 2023
17e237f
changelog and test fix
jjbayer Jan 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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**:

Expand Down
14 changes: 0 additions & 14 deletions relay-general/src/protocol/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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
Expand Down
54 changes: 2 additions & 52 deletions relay-server/src/metrics_extraction/sessions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version bump will allow us to turn off session metrics extraction in outdated external relays, if there is still a significant amount of duration buckets coming from those outdated relays.

const EXTRACT_ABNORMAL_MECHANISM_VERSION: u16 = 2;

/// Configuration for metric extraction from sessions.
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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![];
Expand Down
35 changes: 2 additions & 33 deletions tests/integration/test_metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down Expand Up @@ -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"]
Expand Down Expand Up @@ -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"] == {
Expand Down Expand Up @@ -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",
Expand Down