feat(ingest-metrics): Ensure billing metrics consumer avoids outcome double-billing#117186
Conversation
|
|
||
| manager.add("projects:workflow-engine-performance-detectors", ProjectFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) | ||
|
|
||
| manager.add("organizations:relay-generate-billing-outcome", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) |
There was a problem hiding this comment.
The feature flag also needs to be exposed to Relay, search for EXPOSABLE_FEATURES
| assert next_step.join.call_count == 1 | ||
|
|
||
|
|
||
| @with_feature("organizations:relay-generate-billing-outcome") |
There was a problem hiding this comment.
| @with_feature("organizations:relay-generate-billing-outcome") |
Mentioning the feature here should not be necessary because the consumer-side behavior is determined by the tag, not the feature flag, right? This might also be what confused sentry[bot] in their comment above.
| "has_transaction": PREFIX + 281, | ||
| "was_transaction": PREFIX + 282, | ||
| "is_segment": PREFIX + 283, | ||
| "billing_outcome_accepted": PREFIX + 284, |
There was a problem hiding this comment.
Not sure if you already bikeshedded on the name but billing_outcome_emitted would make slightly more sense to me (the outcome is Accepted but the thing relay does is emitting it).
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 978bfdc. Configure here.
|
|
||
| span_metric_id = SPAN_METRICS_NAMES["c:spans/usage@none"] | ||
| span_is_segment_tag = str(SHARED_TAG_STRINGS["is_segment"]) | ||
| billing_outcome_accepted_tag = str(SHARED_TAG_STRINGS["billing_outcome_emitted"]) |
There was a problem hiding this comment.
Bug: The variable billing_outcome_accepted_tag is misleadingly named, as it references the string "billing_outcome_emitted". If the external service sends "billing_outcome_accepted", the double-billing prevention will fail.
Severity: CRITICAL
Suggested Fix
Verify the exact tag name sent by the corresponding Relay change. If Relay sends "billing_outcome_accepted", update the string registered in src/sentry/sentry_metrics/indexer/strings.py to match. If Relay sends "billing_outcome_emitted", rename the variable billing_outcome_accepted_tag to billing_outcome_emitted_tag for clarity.
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: src/sentry/ingest/billing_metrics_consumer.py#L45
Potential issue: A new mechanism to prevent double-billing for span metrics relies on a
tag check. The variable `billing_outcome_accepted_tag` is used for this check, but it is
assigned the string value `"billing_outcome_emitted"`. The pull request description and
the variable's name imply that the tag sent by the external service (Relay) is
`"billing_outcome_accepted"`. If Relay sends a tag with this name, the consumer's check
for `"billing_outcome_emitted"` will not find it. This would cause the double-billing
prevention logic to silently fail for all metrics, defeating the purpose of the change.
Also affects:
src/sentry/sentry_metrics/indexer/strings.py:206~206
…double-billing (#117186) With relay producing billing outcomes [here](getsentry/relay#6066), introduce and check for a new tag ("billing_outcome_accepted") to avoid double-billing. I checked for prior string existence with: ``` SELECT string, count(*) FROM sentry_{perf,}stringindexer WHERE 1=1 AND string IN ( 'billing_outcome_accepted' ) GROUP BY string; ``` Guarded by a new temporary flag `organizations:relay-generate-billing-outcome`

With relay producing billing outcomes here, introduce and check for a new tag ("billing_outcome_accepted") to avoid double-billing.
I checked for prior string existence with:
Guarded by a new temporary flag
organizations:relay-generate-billing-outcome