Skip to content

fix(aci): Improve legacy model tracking#113110

Closed
kcons wants to merge 1 commit intomasterfrom
kcons/missedem
Closed

fix(aci): Improve legacy model tracking#113110
kcons wants to merge 1 commit intomasterfrom
kcons/missedem

Conversation

@kcons
Copy link
Copy Markdown
Member

@kcons kcons commented Apr 15, 2026

Our metric for tracking legacy model API use have a few holes, but I think this should close the biggest ones.

@kcons kcons requested a review from a team as a code owner April 15, 2026 20:33
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 15, 2026
404: RESPONSE_NOT_FOUND,
},
)
@track_alert_endpoint_execution("GET", "sentry-api-0-project-rule-group-history-index")
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit eb94516. Configure here.


return args, kwargs

report_used_legacy_models()
Copy link
Copy Markdown
Contributor

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.

@kcons
Copy link
Copy Markdown
Member Author

kcons commented Apr 15, 2026

ah right, I forgot about that.

@kcons kcons marked this pull request as draft April 15, 2026 20:39
@kcons kcons closed this Apr 15, 2026
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