fix(transactions): Defer dynamic sampling until metrics config is valid#6246
fix(transactions): Defer dynamic sampling until metrics config is valid#6246jjbayer wants to merge 5 commits into
Conversation
| ) -> (SamplingOutput, Option<CombinedMetricExtractionConfig<'_>>) { | ||
| let metrics_config = get_metrics_config(ctx); | ||
|
|
||
| if metrics_config.is_err() && !ctx.is_processing() { |
There was a problem hiding this comment.
The extra !ctx.is_processing check is here because I assume we still want sampling to happen in the final relay, even if no metrics could be extracted.
| let error = Error::from(limits); | ||
| let (indexed, metrics) = split_indexed_and_total(self, ctx, SamplingDecision::Keep); | ||
| let (indexed, metrics) = | ||
| split_indexed_and_total(self, ctx, SamplingDecision::Keep, get_metrics_config(ctx)); |
There was a problem hiding this comment.
This is a bit unfortunate: I have to call get_metrics_config again because this is a trait impl. I considered making Context an associated type, and passing in the metrics config via Context. But then again, get_metrics_config is a lightweight operation so maybe recomputation is OK here (ctx cannot change between computations).
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| if metrics_config.is_err() && !ctx.is_processing() { | ||
| // Defer dynamic sampling until the next relay. | ||
| return ( | ||
| SamplingOutput::Keep { | ||
| payload, | ||
| sample_rate: None, | ||
| }, | ||
| None, | ||
| ); |
There was a problem hiding this comment.
Bug: The fix to defer dynamic sampling for invalid metrics configs only applies to non-processing relays. Processing relays will still drop transactions without extracting metrics in this scenario.
Severity: MEDIUM
Suggested Fix
Remove the !ctx.is_processing() condition from the check in run_dynamic_sampling. This will ensure that both processing and non-processing relays defer dynamic sampling when the metrics extraction configuration is invalid, preventing data loss and creating consistent behavior across all relay types. Also, add a test case for processing relays with invalid metric configs to ensure correctness.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: relay-server/src/processing/transactions/process.rs#L224-L232
Potential issue: The change to defer dynamic sampling when `metrics_config` is invalid
is gated by `!ctx.is_processing()`. This means the fix only applies to non-processing
relays. For a processing relay, if the project's metric extraction configuration is
invalid (e.g., an unsupported version), dynamic sampling will still execute. If the
sampling decision is to drop the transaction, it will be dropped without any metrics
being extracted. This is inconsistent with the behavior of non-processing relays and
contrary to the PR's goal of preventing data loss in this scenario. The lack of test
coverage for processing relays suggests this was an oversight.
Did we get this right? 👍 / 👎 to inform future reviews.
2 participants
If metrics extraction config is invalid, delegate dynamic sampling to the next relay. This prevents dropping transactions for which no metrics were extracted yet.
Fixes RELAY-268