fix(aci): support notifications for non-dual written metric detectors #103938
Conversation
github-actions
bot
added
the
Scope: Backend
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| 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() | ||
|
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Issue alerts don't go through this code. Silly bot.
| 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) | ||
| ) |
There was a problem hiding this comment.
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
| 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¬ification_uuid={notification_uuid}" | ||
| ) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, they will see these links. Mia has worked on redirects, so they will link to the monitor.
Should probably be merged before #103937. If an open period comes in without a corresponding entry in the IGOP table, create a fake ID for the incident and pass it to the frontend, where it will be used for redirect logic. Do the same for detector IDs that don't have entries in the AlertRuleDetector table.