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

fix(outcomes): Do not report metrics-based outcomes for profiles #4365

Merged
merged 9 commits into from
Dec 12, 2024

Conversation

jjbayer
Copy link
Member

@jjbayer jjbayer commented Dec 10, 2024

Profiles are no longer dropped by dynamic sampling, so we do not need to track the virtual "total profile" category with a tag on metrics.

This also prevents double-reporting (the rate limiter on the payload also emits an outcome for the profile category).

ref: getsentry/sentry#81799

@jjbayer jjbayer marked this pull request as ready for review December 10, 2024 12:26
@jjbayer jjbayer requested a review from a team as a code owner December 10, 2024 12:26
Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

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

Nice!

Should double check if we can find any other mention of has_profile in (get)-sentry. I can't except in this test 👍 .

let has_profile = matches!(mri.name.as_ref(), "usage" | "duration")
&& self.tags.has_profile.is_some();
BucketSummary::Transactions { count, has_profile }
BucketSummary::Transactions(count)
Copy link
Member

Choose a reason for hiding this comment

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

Can use the same formatting as for spans here:

            MetricNamespace::Transactions => BucketSummary::Transactions(match self.value {
                MinimalValue::Counter(c) if mri.name == "usage" => c.to_f64() as usize,
                _ => 0,
            }),

@jjbayer
Copy link
Member Author

jjbayer commented Dec 10, 2024

Should double check if we can find any other mention of has_profile in (get)-sentry.

I found this constant, added in getsentry/sentry#64203.

@wmak is that list based on alert definitions that people actually use?

@jjbayer
Copy link
Member Author

jjbayer commented Dec 11, 2024

Decided we can merge this because the has_profile tag is only on c:transactions/usage@none, which is only used in the metrics billing consumer. I.e the tag cannot be accessed through the product.

@jjbayer jjbayer enabled auto-merge (squash) December 11, 2024 09:04
@jjbayer jjbayer merged commit e1524fc into master Dec 12, 2024
23 checks passed
@jjbayer jjbayer deleted the chore/outcomes-has-profile-tag branch December 12, 2024 08:01
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.

3 participants