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.

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(metrics): Extract user satisfaction as tag [INGEST-589] #1197

Merged
merged 10 commits into from Mar 3, 2022

Conversation

jjbayer
Copy link
Member

@jjbayer jjbayer commented Feb 28, 2022

Use the config exposed in getsentry/sentry#32116 to extract user satisfaction tags for apdex and user misery.

let end = end.timestamp_millis();
Some(end.saturating_sub(start) as f64)
}
_ => None,
Copy link
Member Author

Choose a reason for hiding this comment

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

Open question: Should a transaction without duration count towards satisfied users?

Copy link
Member

Choose a reason for hiding this comment

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

If we have to choose between satisfied, tolerated, and frustrated, I say yes. However, do we know what cases cause transactions without duration count? If there are cases where SDKs aren't able to measure the duration, we may want to include an additional satisfaction value. I don't know how feasible this is though. If a transaction couldn't be measured, I don't think counting it as satisfaction provides any value. It could take a long time, and having another value could lead to users updating SDK code to correctly capture these transactions and have more valuable data.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do not have to choose, we can simply leave the metric untagged. So I will leave None here. This is an exceptional case anyway, because both timestamp and start_timestamp are required fields: https://develop.sentry.dev/sdk/event-payloads/transaction/.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now refactored so that we do an early exit when the transaction is invalid. Metrics for invalid transactions are dropped later on anyway.

@jjbayer
Copy link
Member Author

jjbayer commented Mar 1, 2022

@lynnagara, @wmak: We're trying to model user satisfaction (for Apdex and User Misery) as metrics tags here, and I want to be as consistent with the transactions-based functions as possible. Are the following assumptions correct?

  1. Every transaction in Snuba has a duration which defaults to zero, so when the user choses to apply the satisfaction threshold to the transaction duration, these zero values count towards “satisfied” transactions (and thereby increase the Apdex score).
  2. Not every transaction in Snuba has a measurements.lcp, so when the user choses to apply the satisfaction threshold to LCP instead, only transactions that actually have that measurement are considered for the Apdex score.

Here's what I base those assumptions on:

@jjbayer jjbayer marked this pull request as ready for review March 1, 2022 13:02
@jjbayer jjbayer requested a review from a team March 1, 2022 13:02
relay-server/src/metrics_extraction/transactions.rs Outdated Show resolved Hide resolved
let end = end.timestamp_millis();
Some(end.saturating_sub(start) as f64)
}
_ => None,
Copy link
Member

Choose a reason for hiding this comment

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

If we have to choose between satisfied, tolerated, and frustrated, I say yes. However, do we know what cases cause transactions without duration count? If there are cases where SDKs aren't able to measure the duration, we may want to include an additional satisfaction value. I don't know how feasible this is though. If a transaction couldn't be measured, I don't think counting it as satisfaction provides any value. It could take a long time, and having another value could lead to users updating SDK code to correctly capture these transactions and have more valuable data.

},
});

// User
if let Some(user) = event.user.value() {
if let Some(user_id) = user.id.as_str() {
// TODO: If we have a transaction duration of 0 or no transaction at all, does the user count as satisfied?
Copy link
Member

Choose a reason for hiding this comment

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

I think transactions of duration 0 should count as satisfied. No transaction shouldn't count towards user satisfaction, that's misleading because we actually don't have data.

@jjbayer jjbayer marked this pull request as draft March 2, 2022 10:47
@jjbayer
Copy link
Member Author

jjbayer commented Mar 3, 2022

@lynnagara, @wmak: Ignore my previous question, I figured out that Relay rejects invalid transactions, so handling of special cases (missing timestamp, missing start_timestamp) is not required when extracting metrics.

@jjbayer jjbayer marked this pull request as ready for review March 3, 2022 14:28
@jjbayer jjbayer requested review from untitaker and a team March 3, 2022 14:29
@jjbayer jjbayer merged commit f916258 into master Mar 3, 2022
@jjbayer jjbayer deleted the feat/metrics-extraction-user-satisfaction branch March 3, 2022 16:07
jan-auer added a commit that referenced this pull request Mar 9, 2022
* master:
  ref(make): Simplify M1 exports in Makefile (#1206)
  fix(metrics): Wait for project states during aggregator shutdown (#1205)
  fix(test): Find librdkafka on Apple M1 (#1204)
  build: Bump sentry-relay in dev dependencies to 0.8.9 (#1202)
  ref(metrics): Tag backdated bucket creations in statsd (#1200)
  feat(metrics): Extract user satisfaction as tag (#1197)
  fix(statsd): Add new metric_type tag to existing metrics (#1199)
  fix: Apply clippy 1.59 suggestions (#1198)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants