ref(profiles): Count processed profiles with metrics #2165
Conversation
jjbayer
changed the title
relay-server/src/actors/processor.rs
Outdated
| if !found_profile { | ||
| // Found first profile, keep it. | ||
| found_profile = true; | ||
| } else { | ||
| // We found a second profile, drop it. | ||
| return ItemAction::Drop(Outcome::Invalid(DiscardReason::Profiling( | ||
| relay_profiling::discard_reason(ProfileError::TooManyProfiles), | ||
| ))); | ||
| } |
There was a problem hiding this comment.
@phacops I added this logic as you suggested.
| struct ExtractInput<'a> { | ||
| aggregator_config: &'a AggregatorConfig, | ||
| config: &'a TransactionMetricsConfig, | ||
| event: &'a Event, | ||
| transaction_from_dsc: Option<&'a str>, | ||
| sampling_result: &'a SamplingResult, | ||
| has_profile: bool, | ||
| } |
There was a problem hiding this comment.
The linter finally started complaining about the number of arguments to extract_transaction_metrics_inner so I created this struct.
|
Letting one integration test fail deliberately. It will pass when #2166 is merged into this branch. |
relay-server/src/actors/processor.rs
Outdated
| ItemType::Profile => { | ||
| if !found_profile { | ||
| // Found first profile, keep it. | ||
| found_profile = true; |
There was a problem hiding this comment.
Relay also considers towards outcomes dropped profiles that could not parsed, even if they aren't dropped by dynamic sampling. #2054 mentions only "valid" profiles should be considered:
In other words, "processed profiles" should count all valid, non rate limited profiles seen by Relays, even if they were dropped by dynamic sampling.
There was a problem hiding this comment.
The wording in that PR description is too vague, it should say: "accepted processed profiles should count all valid non rate limited profiles seen by Relays" .
A better way to think of it is two data streams splitting ways (see diagram here), and getting counted towards a different category regardless of whether they're accepted, rate limited, or invalid.
With #2165, "processed" profiles are represented by their raw payload _before_ metrics extraction, and represented by the `transaction.duration` metric _after_ metrics extraction, as it is the case for transactions: ```mermaid flowchart LR . --"payload\n(DataCategory::Profile)"--> MetricsExtraction MetricsExtraction --"payloads\n(TransactionIndexed, ProfileIndexed)"-->.. MetricsExtraction --"metrics\n(Transaction, Profile)"-->... ``` To prevent double counting, emit any profile-related outcome after metrics extraction as `DataCategory::ProfileIndexed`. Co-authored-by: Iker Barriocanal <[email protected]>
Instead of counting processed profiles in two different places (see #2054), add a `has_profile` tag to the transaction counter metric, and define ``` processed_profiles := count(processed_transactions) WHERE has_profile = true ``` ## Changes This PR contains the following changes: * Revert PRs around counting profiles. * Add `has_profile(s)` tag on `transaction.duration` metric. #2165 * Update metrics billing consumer to emit `accepted` outcome for profiles. getsentry/sentry#50047 ## Caveats With this PR, rate limits will be applied consistently if they are issued for `DataCategory.Transaction`. However, if a rate limit (e.g. spike protection) is issued _only_ for `DataCategory.Profile`, it will not be enforced. ## Rollout Order 1. Merge and deploy getsentry/sentry#50047. The billing metrics consumer will now listen for profiles, but not receive any. 3. Deploy processing Relays. Profile counts will go down, because PoP-Relays are not sending the `has_profile` tag yet. 4. Deploy PoP Relays. Profile counts should go back to normal. ## In case of rollback 1. First revert the sentry change (getsentry/sentry#50047) and deploy it to stop counting profiles. 2. Then revert the Relay change (this PR) and deploy to processing Relays and PoPs. ref: #2158 --------- Co-authored-by: Iker Barriocanal <[email protected]>
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:
ProfileandProfileIndexed#skip-changelog