-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
ref(metrics): Use usage metric in the billing consumer #57759
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code change LGTM, see comment about rollout order.
@@ -90,6 +90,7 @@ | |||
"g:transactions/alert@none": PREFIX + 135, | |||
"d:transactions/duration_light@millisecond": PREFIX + 136, | |||
"c:transactions/usage@none": PREFIX + 137, | |||
# Last possible index: 199 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smart.
#: See https://github.com/getsentry/relay/blob/4f3e224d5eeea8922fe42163552e8f20db674e86/relay-server/src/metrics_extraction/transactions.rs#L71 | ||
TRANSACTION_METRICS_EXTRACTION_VERSION = 2 | ||
#: See https://github.com/getsentry/relay/blob/6181c6e80b9485ed394c40bc860586ae934704e2/relay-dynamic-config/src/metrics.rs#L85 | ||
TRANSACTION_METRICS_EXTRACTION_VERSION = 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will take a while for all project configs to update to version 3, so we will be in a state where Relay still emits outcomes (for rate limits) based on the old metric, but sentry already emits accepted outcomes based on the new metric. Can't think of an argument right now why this should matter, but it feels inconsistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's acceptable behavior, since both numbers are equal in practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we continue accepting transaction duration outcomes for some time after upgrading the version to 3?
Follow-up to #57666 and getsentry/relay#2571.
Uses the new usage metric in the billing consumer. This also updates the metric
extraction version to 3 so that the usage metric is used for rate limiting.