ref(profiles): Counting profiles, V2 #2163
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced May 31, 2023
iker-barriocanal
approved these changes
Jun 1, 2023
olksdr
approved these changes
Jun 1, 2023
As described in #2158, track the number of processed profiles by tagging the corresponding transaction metric. Also: Do not accept multiple profiles per envelope. **Not in this PR:** * Distinguishing between `Profile` and `ProfileIndexed` * Rate limiting processed profiles based on metrics --------- Co-authored-by: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
jjbayer
changed the title
jjbayer
added a commit
to getsentry/sentry
that referenced
this pull request
Jun 5, 2023
…50047) Currently, accepted processed profiles are counted in Relay. To align processed transactions and processed profiles more consistently, we will change this by adding a tag to the `transaction.duration` metric (which we use for counting transactions), and defining `num_processed_profiles := num_processed_transactions with profiles` (see issue linked below for more information). This PR needs to be deployed _before_ getsentry/relay#2163 to prevent over-counting. ref: getsentry/relay#2158
phacops
approved these changes
Jun 5, 2023
jjbayer
added a commit
that referenced
this pull request
Jun 6, 2023
This was referenced Jun 13, 2023
jjbayer
added a commit
that referenced
this pull request
Jun 13, 2023
In #2163 we decided not to enforce rate limits on processed profiles with `limit > 0`, to prevent complexity (see PR description). I wrongly assumed that quotas (i.e. rate limits with `limit:0`) would be correctly enforced by dropping the profile envelope item before metrics are extracted. I overlooked the fact that PoP Relays (where metrics are extracted) do not _receive_ quota configuration, because they do not request it: https://github.com/getsentry/relay/blob/0505c88f932d89bd42ee75114440f7f08fe7d938/relay-server/src/actors/project_upstream.rs#L273 Note that this only affects rate limits that are issued _only_ for profiles. Rate limits issued for _transactions_ correctly drop the processed profiles. This PR fixes the issue by stripping the `has_profile` tag off metrics buckets if a `Profile` rate limit is active. Fixes #2207
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of counting processed profiles in two different places (see #2054), add a
has_profiletag to the transaction counter metric, and defineChanges
This PR contains the following changes:
has_profile(s)tag ontransaction.durationmetric. ref(profiles): Count processed profiles with metrics #2165acceptedoutcome for profiles. feat(profiles): Count processed profiles in billing metrics consumer sentry#50047Caveats
With this PR, rate limits will be applied consistently if they are issued for
DataCategory.Transaction. However, if a rate limit with a numeric value > 0 (e.g. spike protection) is issued only forDataCategory.Profile, it will not be enforced correctly.Rollout Order
has_profiletag yet.In case of rollback
ref: #2158