Skip to content

fix(aci): Improve legacy model tracking #113110

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

Closed
kcons wants to merge 1 commit into from
+7 −0
Closed

fix(aci): Improve legacy model tracking #113110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions src/sentry/incidents/endpoints/bases.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from sentry.workflow_engine.endpoints.utils.ids import to_valid_int_id
from sentry.workflow_engine.models.alertrule_detector import AlertRuleDetector
from sentry.workflow_engine.models.detector import Detector
from sentry.workflow_engine.utils.legacy_metric_tracking import report_used_legacy_models


class OrganizationAlertRuleBaseEndpoint(OrganizationEndpoint):
Expand Down Expand Up @@ -150,6 +151,7 @@ def convert_args(

return args, kwargs

report_used_legacy_models()
Copy link
Copy Markdown
Contributor

@cursor cursor bot Apr 15, 2026

Choose a reason for hiding this comment

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

ContextVar reset in decorator nullifies convert_args tracking

High Severity

report_used_legacy_models() in convert_args sets _legacy_models_used to True, but convert_args runs before the endpoint handler method. The track_alert_endpoint_execution decorator on the handler resets the ContextVar to False at the start of its wrapper. This means the legacy-model flag is always wiped before the decorator reads it, so legacy_models will always be reported as false — defeating the entire purpose of this tracking improvement. The same issue applies to the newly decorated rule history/stats endpoints, where the pre-existing report_used_legacy_models() in WorkflowEngineRuleEndpoint.convert_args is similarly reset.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit eb94516. Configure here.

try:
kwargs["alert_rule"] = AlertRule.objects.get(
projects=project, id=validated_alert_rule_id
Expand Down Expand Up @@ -203,6 +205,7 @@ def convert_args(

return args, kwargs

report_used_legacy_models()
try:
kwargs["alert_rule"] = AlertRule.objects.get(
organization=organization, id=validated_alert_rule_id
Expand Down
2 changes: 2 additions & 0 deletions src/sentry/rules/history/endpoints/project_rule_group_history.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from sentry.rules.history import fetch_rule_groups_paginated
from sentry.rules.history.base import RuleGroupHistory
from sentry.workflow_engine.models.workflow import Workflow
from sentry.workflow_engine.utils.legacy_metric_tracking import track_alert_endpoint_execution


class RuleGroupHistoryResponse(TypedDict):
Expand Down Expand Up @@ -78,6 +79,7 @@ class ProjectRuleGroupHistoryIndexEndpoint(WorkflowEngineRuleEndpoint):
404: RESPONSE_NOT_FOUND,
},
)
@track_alert_endpoint_execution("GET", "sentry-api-0-project-rule-group-history-index")
Copy link
Copy Markdown
Contributor

@sentry sentry bot Apr 15, 2026

Choose a reason for hiding this comment

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

Bug: The @track_alert_endpoint_execution decorator resets the _legacy_models_used context variable prematurely, causing metrics to always incorrectly report that legacy models were not used.
Severity: MEDIUM

Suggested Fix

Modify the @track_alert_endpoint_execution decorator to read the value of the _legacy_models_used context variable before resetting it. The value should be captured after the endpoint handler runs but before the context variable is reset, ensuring the correct state is recorded in the metric.

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/rules/history/endpoints/project_rule_group_history.py#L82

Potential issue: The `@track_alert_endpoint_execution` decorator introduces a timing bug
related to the `_legacy_models_used` context variable. The decorator's wrapper function
immediately resets this variable to `False` by calling `_legacy_models_used.set(False)`
before the actual endpoint handler is executed. The value is correctly set to `True` in
the `convert_args` method, which runs before the decorator's wrapper. However, because
the reset happens before the handler runs and the value is read, the decorator always
captures `False`. This results in the `alert_endpoint.executed` metric consistently and
incorrectly reporting `legacy_models: false`, defeating the purpose of the legacy model
usage tracking.

Did we get this right? 👍 / 👎 to inform future reviews.

def get(self, request: Request, project: Project, rule: Rule | Workflow) -> Response:
per_page = self.get_per_page(request)
cursor = self.get_cursor_from_request(request)
Expand Down
2 changes: 2 additions & 0 deletions src/sentry/rules/history/endpoints/project_rule_stats.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
from sentry.rules.history import fetch_rule_hourly_stats
from sentry.rules.history.base import TimeSeriesValue
from sentry.workflow_engine.models.workflow import Workflow
from sentry.workflow_engine.utils.legacy_metric_tracking import track_alert_endpoint_execution


class TimeSeriesValueResponse(TypedDict):
Expand Down Expand Up @@ -61,6 +62,7 @@ class ProjectRuleStatsIndexEndpoint(WorkflowEngineRuleEndpoint):
404: RESPONSE_NOT_FOUND,
},
)
@track_alert_endpoint_execution("GET", "sentry-api-0-project-rule-stats-index")
def get(self, request: Request, project: Project, rule: Rule | Workflow) -> Response:
"""
Note that results are returned in hourly buckets.
Expand Down
Loading