Skip to content

feat(metric-issues): Add observed value to occurrence evidence_display - #120713

Draft
isaacwang-sentry wants to merge 2 commits into
masterfrom
isaacwang/metric-issue-observed-value-evidence
Draft

feat(metric-issues): Add observed value to occurrence evidence_display#120713
isaacwang-sentry wants to merge 2 commits into
masterfrom
isaacwang/metric-issue-observed-value-evidence

Conversation

@isaacwang-sentry

Copy link
Copy Markdown
Member

Metric issue occurrences shipped evidence_display=[] (with an XXX: may need to pass more info here for the front end note), so the value that actually breached was never exposed as curated evidence — it existed only inside the internal evidence_data payload. This is the Sentry half of the AIML-3164 work in the Autofix Support for Metric Issues project.

This emits a single important=True row:

Observed value (Number of events): 147

Why exactly one row, and why only the value

The monitor's definition — threshold, query, environment, detection type — is current detector state, served by the detector_context path (#120370 + its Seer-side rendering). The observed value is the point-in-time breach value, the one fact only the occurrence has. Duplicating the definition here would put two blocks in one LLM prompt that contradict each other as soon as a detector is edited after the occurrence was created.

One row also honors the IssueEvidence "only one important row per occurrence" convention, since space-constrained integrations surface only the first.

Two implementation details worth a look

Reading the value. build_evidence_display reads data_packet.packet.values["value"] directly rather than calling self.extract_value(), whose anomaly-detection branch wraps the result in a group-keyed dict. Both MetricUpdate variants (ProcessedSubscriptionUpdate, AnomalyDetectionUpdate) define values with a value: float key, so direct access is uniform. A test drives a real AnomalyDetectionUpdate through this path — asserting on detection_type="dynamic" in a title test would not have exercised it.

The aggregate helper returns two values. format_aggregate() is factored out of construct_title and returns (display label, normalized aggregate key). These diverge: the crash-rate branch strips the AS <CRASH_RATE_ALERT_AGGREGATE_ALIAS> suffix off the key, and it is that stripped key — not the display label — that get_alert_type_from_aggregate_dataset consumes. A display-only helper would have silently changed dynamic alert titles. The logic moved verbatim; construct_title behavior is unchanged.

Impact

  • No UI change. MetricIssue.category = GroupCategory.METRIC routes to metricConfig.tsx, which sets evidence: null in _categoryDefaults and does not re-enable it for IssueType.METRIC_ISSUE, so EventEvidence returns null regardless. (metricIssueConfig.tsx serves IssueCategory.METRIC_ALERT — a different category.) Enabling that section is a separate product/design call.
  • Jira ticket bodies will change — please sign off. Metric-issue actions without a metric-alert-specific handler (WEBHOOK, PLUGIN, ticketing) fall through to the generic issue-alert path, and jira/integration.py reads important_evidence_display. Slack/Discord/MSTeams/PagerDuty/Opsgenie/Email route through metric_alert_registry and are unaffected.
  • Newly created occurrences only. Stored occurrences keep evidence_display=[]; there is no backfill.

Tests

tests/sentry/incidents/test_metric_issue_detector_handler.py had no evidence_display assertions — the shared verify_issue_occurrence helper checks only evidence_data. Added coverage for the row's name/value, exactly-one-important, static / comparison-delta / dynamic variants, and all four format_aggregate branches (mapped, unmapped, MRI, equation, crash-rate alias).

46 passed. Reverting evidence_display to [] fails 9 of the new tests, so they check behavior rather than restate the implementation.

🤖 Generated with Claude Code

Metric issue occurrences shipped an empty evidence_display, so the value that actually breached was never exposed as curated evidence - it existed only inside the internal evidence_data payload.

Emit a single important row carrying the observed value labeled with the metric it measures. One row by design: the monitor definition (threshold, query, environment, detection type) is current detector state served by the detector-context path, while this is the point-in-time breach value, the one fact only the occurrence has. Keeping it to one row honors the one-important-row-per-occurrence convention, since space-constrained integrations surface only the first.

The value is read from values["value"] directly rather than through extract_value(), whose anomaly-detection branch wraps the result in a group-keyed dict. The aggregate-display logic is factored out of construct_title into format_aggregate(), which returns both the display label and the normalized aggregate key - the crash-rate branch strips an alias suffix off the key, and it is that stripped key, not the label, that dynamic alert classification consumes.

No UI change: metricConfig.tsx sets evidence to null for the METRIC category, so the Evidence section stays hidden. Applies to newly created occurrences only.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 28, 2026
…e union

DataPacket is invariant in its type parameter, so DataPacket[AnomalyDetectionUpdate] is not assignable to the DataPacket[MetricUpdate] that build_evidence_display accepts. Annotate the variable with the union instead, matching the existing pattern in tests/sentry/issues/test_ingest.py.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant