Skip to content

Conversation

kcons
Copy link
Member

@kcons kcons commented Oct 14, 2025

There can be 10s-100s of thousands of IncidentTrigger rows associated with an AlertRuleTrigger.
In SENTRY-4A76, we see repeated failed attempts to delete an AlertRuleTrigger with 15k IncidentTriggers, and this change will hopefully help there.

@kcons kcons requested review from a team and cathteng October 14, 2025 19:02
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 14, 2025
Copy link

codecov bot commented Oct 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #101460   +/-   ##
========================================
  Coverage   81.02%    81.02%           
========================================
  Files        8701      8701           
  Lines      386021    386019    -2     
  Branches    24409     24409           
========================================
+ Hits       312774    312778    +4     
+ Misses      72896     72890    -6     
  Partials      351       351           

@ceorourke
Copy link
Member

I wonder if we instead change the order here https://github.com/getsentry/sentry/blob/master/src/sentry/deletions/defaults/alertrule.py#L15 to have Incident first that would delete the IncidentTrigger objects before getting to AlertRuleTrigger

@kcons
Copy link
Member Author

kcons commented Oct 14, 2025

I wonder if we instead change the order here https://github.com/getsentry/sentry/blob/master/src/sentry/deletions/defaults/alertrule.py#L15 to have Incident first that would delete the IncidentTrigger objects before getting to AlertRuleTrigger

I think it would, but we'd probably still not want to rely on cascades, and I think we don't index IncidentTrigger by incident id, so that path would likely be pretty slow.

@ceorourke
Copy link
Member

I wonder if we instead change the order here https://github.com/getsentry/sentry/blob/master/src/sentry/deletions/defaults/alertrule.py#L15 to have Incident first that would delete the IncidentTrigger objects before getting to AlertRuleTrigger

I think it would, but we'd probably still not want to rely on cascades, and I think we don't index IncidentTrigger by incident id, so that path would likely be pretty slow.

It is indexed https://github.com/getsentry/sentry/blob/master/src/sentry/incidents/models/incident.py#L322 but I'm ok with trying it on the AlertRuleTrigger 👍

@kcons kcons merged commit 740eeee into master Oct 15, 2025
66 checks passed
@kcons kcons deleted the kcons/beleete branch October 15, 2025 15:27
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.

2 participants