Skip to content

fix(aci): support notifications for non-dual written metric detectors #103938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 25, 2025
Merged

fix(aci): support notifications for non-dual written metric detectors #103938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fc9de16
implement
mifu67 Nov 24, 2025
8a2f10e
revive test file + add test + handle missing ARD
mifu67 Nov 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/sentry/incidents/action_handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
DetailedIncidentSerializerResponse,
IncidentSerializer,
)
from sentry.incidents.endpoints.serializers.utils import get_fake_id_from_object_id
from sentry.incidents.grouptype import MetricIssue
from sentry.incidents.models.alert_rule import (
AlertRuleDetectionType,
Expand Down Expand Up @@ -603,15 +604,19 @@ def generate_incident_trigger_email_context(
incident_group_open_period = IncidentGroupOpenPeriod.objects.get(
group_open_period_id=metric_issue_context.open_period_identifier
)
incident_identifier = incident_group_open_period.incident_identifier
except IncidentGroupOpenPeriod.DoesNotExist:
raise ValueError("IncidentGroupOpenPeriod does not exist")
# the corresponding metric detector was not dual written
incident_identifier = get_fake_id_from_object_id(
metric_issue_context.open_period_identifier
)

alert_link = organization.absolute_url(
reverse(
"sentry-metric-alert",
kwargs={
"organization_slug": organization.slug,
"incident_id": incident_group_open_period.incident_identifier,
"incident_id": incident_identifier,
},
),
query=urlencode(alert_link_params),
Expand Down
12 changes: 8 additions & 4 deletions src/sentry/integrations/metric_alerts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@
from typing import NotRequired, TypedDict
from urllib import parse

import sentry_sdk
from django.db.models import Max
from django.urls import reverse
from django.utils.translation import gettext as _

from sentry import features
from sentry.constants import CRASH_RATE_ALERT_AGGREGATE_ALIAS
from sentry.incidents.endpoints.serializers.utils import get_fake_id_from_object_id
from sentry.incidents.logic import GetMetricIssueAggregatesParams, get_metric_issue_aggregates
from sentry.incidents.models.alert_rule import AlertRule, AlertRuleThresholdType
from sentry.incidents.models.incident import (
Expand Down Expand Up @@ -239,7 +239,8 @@ def incident_attachment_info(
if alert_rule_id is None:
raise ValueError("Alert rule id not found when querying for AlertRuleDetector")
except AlertRuleDetector.DoesNotExist:
raise ValueError("Alert rule detector not found when querying for AlertRuleDetector")
# the corresponding metric detector was not dual written
alert_rule_id = get_fake_id_from_object_id(alert_context.action_identifier_id)

workflow_engine_params = title_link_params.copy()

Comment on lines 239 to 246
Copy link

@sentry sentry bot Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: AlertRuleDetector.objects.values_list(...).get() returns None for issue rule detectors, causing ValueError instead of graceful fallback.
Severity: HIGH | Confidence: High

🔍 Detailed Analysis

The code in metric_alerts.py attempts to retrieve alert_rule_id from AlertRuleDetector. If an AlertRuleDetector exists but has rule_id set and alert_rule_id is NULL (indicating it's linked to an issue rule detector), the .get() call will return None. The subsequent if alert_rule_id is None: check then raises a ValueError, preventing the intended graceful fallback to a fake ID. This occurs when a detector is misconfigured or partially migrated, leading to an unexpected crash instead of a graceful fallback.

💡 Suggested Fix

Modify the code to handle alert_rule_id being None when rule_id is present, allowing the graceful fallback mechanism to activate.

🤖 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/integrations/metric_alerts.py#L239-L246

Potential issue: The code in `metric_alerts.py` attempts to retrieve `alert_rule_id`
from `AlertRuleDetector`. If an `AlertRuleDetector` exists but has `rule_id` set and
`alert_rule_id` is `NULL` (indicating it's linked to an issue rule detector), the
`.get()` call will return `None`. The subsequent `if alert_rule_id is None:` check then
raises a `ValueError`, preventing the intended graceful fallback to a fake ID. This
occurs when a detector is misconfigured or partially migrated, leading to an unexpected
crash instead of a graceful fallback.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 3334006

Copy link
Contributor Author

@mifu67 mifu67 Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue alerts don't go through this code. Silly bot.

Expand All @@ -248,8 +249,11 @@ def incident_attachment_info(
group_open_period_id=metric_issue_context.open_period_identifier
)
workflow_engine_params["alert"] = str(open_period_incident.incident_identifier)
except IncidentGroupOpenPeriod.DoesNotExist as e:
sentry_sdk.capture_exception(e)
except IncidentGroupOpenPeriod.DoesNotExist:
# the corresponding metric detector was not dual written
workflow_engine_params["alert"] = str(
get_fake_id_from_object_id(metric_issue_context.open_period_identifier)
)
Comment on lines 251 to +256
Copy link

@sentry sentry bot Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: API endpoints fail with 404 for metric alert links containing fake IDs due to missing conversion logic.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

Fake IDs generated for metric alerts are embedded in URLs and passed to API endpoints. The IncidentEndpoint.convert_args() method directly queries the database using Incident.objects.get(organization=organization, identifier=incident_identifier). Since fake IDs (e.g., original_id + 10^9) do not match any real Incident.identifier values, this lookup fails, raising Incident.DoesNotExist and resulting in a 404 error. There is no conversion logic in the API endpoint to transform fake IDs back to real IDs.

💡 Suggested Fix

Implement logic in IncidentEndpoint.convert_args() or a preceding layer to convert fake IDs back to real IDs using get_object_id_from_fake_id() before performing database lookups.

🤖 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/integrations/metric_alerts.py#L251-L256

Potential issue: Fake IDs generated for metric alerts are embedded in URLs and passed to
API endpoints. The `IncidentEndpoint.convert_args()` method directly queries the
database using `Incident.objects.get(organization=organization,
identifier=incident_identifier)`. Since fake IDs (e.g., `original_id + 10^9`) do not
match any real `Incident.identifier` values, this lookup fails, raising
`Incident.DoesNotExist` and resulting in a 404 error. There is no conversion logic in
the API endpoint to transform fake IDs back to real IDs.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 3334006

Copy link
Contributor Author

@mifu67 mifu67 Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#103937


title_link = build_title_link(alert_rule_id, organization, workflow_engine_params)

Expand Down
167 changes: 167 additions & 0 deletions tests/sentry/integrations/test_metric_alerts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
import uuid

import pytest

from sentry.incidents.endpoints.serializers.utils import get_fake_id_from_object_id
from sentry.incidents.grouptype import MetricIssue
from sentry.incidents.logic import CRITICAL_TRIGGER_LABEL
from sentry.incidents.models.alert_rule import AlertRuleDetectionType, AlertRuleThresholdType
from sentry.incidents.models.incident import IncidentStatus
from sentry.incidents.typings.metric_detector import AlertContext, MetricIssueContext
from sentry.integrations.metric_alerts import incident_attachment_info
from sentry.models.groupopenperiod import GroupOpenPeriod
from sentry.testutils.cases import BaseIncidentsTest, TestCase

pytestmark = pytest.mark.sentry_metrics


def incident_attachment_info_with_metric_value(incident, new_status, metric_value):
return incident_attachment_info(
organization=incident.organization,
alert_context=AlertContext.from_alert_rule_incident(incident.alert_rule),
metric_issue_context=MetricIssueContext.from_legacy_models(
incident, new_status, metric_value
),
)


class IncidentAttachmentInfoTest(TestCase, BaseIncidentsTest):
def setUp(self) -> None:
super().setUp()
self.group = self.create_group(type=MetricIssue.type_id)

def test_returns_correct_info_with_workflow_engine_dual_write(self) -> None:
"""
This tests that we look up the correct incident and alert rule during dual write ACI migration.
"""
alert_rule = self.create_alert_rule()
date_started = self.now
incident = self.create_incident(
self.organization,
title="Incident #1",
projects=[self.project],
alert_rule=alert_rule,
status=IncidentStatus.CLOSED.value,
date_started=date_started,
)
trigger = self.create_alert_rule_trigger(alert_rule, CRITICAL_TRIGGER_LABEL, 100)
self.create_alert_rule_trigger_action(
alert_rule_trigger=trigger, triggered_for_incident=incident
)
metric_value = 123
referrer = "metric_alert_custom"
notification_uuid = str(uuid.uuid4())

detector = self.create_detector(project=self.project)
self.create_alert_rule_detector(alert_rule_id=alert_rule.id, detector=detector)

open_period = (
GroupOpenPeriod.objects.filter(group=self.group).order_by("-date_started").first()
)
assert open_period is not None
self.create_incident_group_open_period(incident, open_period)

metric_issue_context = MetricIssueContext.from_legacy_models(
incident, IncidentStatus.CLOSED, metric_value
)
# Setting the open period identifier to the open period id, since we are testing the lookup
metric_issue_context.open_period_identifier = open_period.id
metric_issue_context.group = self.group
assert metric_issue_context.group is not None

data = incident_attachment_info(
organization=incident.organization,
alert_context=AlertContext(
name=alert_rule.name,
# Setting the action identifier id to the detector id since that's what the NOA does
action_identifier_id=detector.id,
threshold_type=AlertRuleThresholdType(alert_rule.threshold_type),
detection_type=AlertRuleDetectionType(alert_rule.detection_type),
comparison_delta=alert_rule.comparison_delta,
sensitivity=alert_rule.sensitivity,
resolve_threshold=alert_rule.resolve_threshold,
alert_threshold=None,
),
metric_issue_context=metric_issue_context,
notification_uuid=notification_uuid,
referrer=referrer,
)

assert data["title"] == f"Resolved: {alert_rule.name}"
assert data["status"] == "Resolved"
assert data["text"] == "123 events in the last 10 minutes"
# We still build the link using the alert_rule_id and the incident identifier
assert (
data["title_link"]
== f"http://testserver/organizations/baz/alerts/rules/details/{alert_rule.id}/?alert={incident.identifier}&referrer={referrer}&detection_type=static&notification_uuid={notification_uuid}"
)
assert (
data["logo_url"]
== "http://testserver/_static/{version}/sentry/images/sentry-email-avatar.png"
)

def test_returns_correct_info_placeholder_incident(self) -> None:
"""
Test that we use the fake incident ID to build the title link if no IGOP entry exists (if the detector was not dual written).
"""
alert_rule = self.create_alert_rule()
date_started = self.now
incident = self.create_incident(
self.organization,
title="Incident #1",
projects=[self.project],
alert_rule=alert_rule,
status=IncidentStatus.CLOSED.value,
date_started=date_started,
)
trigger = self.create_alert_rule_trigger(alert_rule, CRITICAL_TRIGGER_LABEL, 100)
self.create_alert_rule_trigger_action(
alert_rule_trigger=trigger, triggered_for_incident=incident
)
metric_value = 123
referrer = "metric_alert_custom"
notification_uuid = str(uuid.uuid4())

detector = self.create_detector(project=self.project)

open_period = (
GroupOpenPeriod.objects.filter(group=self.group).order_by("-date_started").first()
)
assert open_period is not None

metric_issue_context = MetricIssueContext.from_legacy_models(
incident, IncidentStatus.CLOSED, metric_value
)
# Setting the open period identifier to the open period id, since we are testing the lookup
metric_issue_context.open_period_identifier = open_period.id
metric_issue_context.group = self.group
assert metric_issue_context.group is not None

# XXX: for convenience, we populate the AlertContext with alert rule/incident information. In this test,
# we're just interested in how the method handles missing AlertRuleDetectors/IGOPs.
data = incident_attachment_info(
organization=incident.organization,
alert_context=AlertContext(
name=alert_rule.name,
# Setting the action identifier id to the detector id since that's what the NOA does
action_identifier_id=detector.id,
threshold_type=AlertRuleThresholdType(alert_rule.threshold_type),
detection_type=AlertRuleDetectionType(alert_rule.detection_type),
comparison_delta=alert_rule.comparison_delta,
sensitivity=alert_rule.sensitivity,
resolve_threshold=alert_rule.resolve_threshold,
alert_threshold=None,
),
metric_issue_context=metric_issue_context,
notification_uuid=notification_uuid,
referrer=referrer,
)

fake_alert_rule_id = get_fake_id_from_object_id(detector.id)
fake_incident_identifier = get_fake_id_from_object_id(open_period.id)

# Build the link using the fake alert_rule_id and the fake incident identifier
assert (
data["title_link"]
== f"http://testserver/organizations/baz/alerts/rules/details/{fake_alert_rule_id}/?alert={fake_incident_identifier}&referrer={referrer}&detection_type=static&notification_uuid={notification_uuid}"
)
Comment on lines +164 to +167
Copy link
Member

@ceorourke ceorourke Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgive me if I missed some context, but will users actually see these links? They won't work, right? Or will we be sending different links for single written detectors that work?

Copy link
Contributor Author

@mifu67 mifu67 Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they will see these links. Mia has worked on redirects, so they will link to the monitor.

Loading