feat(alerts): Add ratelimiting for metric alerts #58032

isabellaenriquez · 2023-10-12T22:09:44Z

Ratelimiting is added (under an options-backed feature flag) to ensure metric alerts don't fire more than every 10 minutes. Action item from INC-454.

Closes #54730

codecov · 2023-10-12T23:45:22Z

Codecov Report

Merging #58032 (119ab22) into master (e88e83c) will increase coverage by 0.02%.
Report is 146 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 119ab22 differs from pull request most recent head f56d3e2. Consider uploading reports for the commit f56d3e2 to get more accurate results

@@            Coverage Diff             @@
##           master   #58032      +/-   ##
==========================================
+ Coverage   79.03%   79.06%   +0.02%     
==========================================
  Files        5131     5138       +7     
  Lines      223537   223607      +70     
  Branches    37640    37650      +10     
==========================================
+ Hits       176678   176784     +106     
+ Misses      41188    41158      -30     
+ Partials     5671     5665       -6

Files	Coverage Δ
src/sentry/incidents/subscription_processor.py	`89.41% <100.00%> (+0.24%)`	⬆️
src/sentry/options/defaults.py	`100.00% <100.00%> (ø)`

... and 21 files with indirect coverage changes

…t + clean up

src/sentry/incidents/subscription_processor.py

scttcper · 2023-10-13T20:46:06Z

this feels like something that will need to be on the docs since before it could fire every minute

JoshFerge

given that this is a very sensitive change, could we consider putting it behind an options backed feature flag? so if something goes wrong we can simply turn off? and test internally first?

JoshFerge · 2023-10-13T23:42:57Z

src/sentry/incidents/subscription_processor.py

+            [project.id for project in last_incident.projects.all()] if last_incident else []
+        )
+        minutes_since_last_incident = (
+            (timezone.now() - last_incident.date_added).seconds / 60 if last_incident else None


nit: I slightly prefer using timedelta to raw second math

could you elaborate what you mean by that? i believe the .seconds is a timedelta property

src/sentry/incidents/subscription_processor.py

wedamija

This is looking generally good to me. Since we need indexes here I'd recommend making a separate pr for them and merging that pr first

wedamija · 2023-10-14T00:00:20Z

src/sentry/incidents/subscription_processor.py

+
+        # If an incident was created for this rule, trigger type, and subscription
+        # within the last 10 minutes, don't make another one
+        last_incident: Incident | None = trigger.triggered_incidents.order_by("-date_added").first()


We might need to add an index on IncidentTrigger on alert_rule_trigger, date_added as well

not sure i understand the use of an additional index here. i know it would've been helpful had we kept the original approach with Incident.objects.filter(...), but since we use trigger.triggered_incidents.order_by("-date_added").first() to get last_incident, how does an additional index help us make this more efficent?

Ahh sorry, I misread the code and thought it was sorting on date_added in IncidentTrigger, but it's in Incident

We might need an index on the Incident table though... (id, date_added)

Here's what this query will look like

SELECT * FROM "sentry_incident" INNER JOIN "sentry_incidenttrigger" ON ("sentry_incident"."id" = "sentry_incidenttrigger"."incident_id") WHERE "sentry_incidenttrigger"."alert_rule_trigger_id" = 1 ORDER BY "sentry_incident"."date_added" DESC

This PR adds a new index to `Incident` to make querying for the metric alert rate limiting feature more efficient (#58032).

This PR gets rid of the index on `id, date_added`. This was originally added in #58207 to make a query in #58032 more efficient, however, after it was added, we determined that it was still really slow. We'll be using a different approach, so this one is no longer needed.

wedamija · 2023-10-19T22:21:06Z

Just a note to not merge this until the index is created

This PR adds a new index to `IncidentTrigger` to make querying for the metric alert rate limiting feature more efficient #58032. [Query in question](https://github.com/getsentry/sentry/pull/58032/files#diff-d262481ba0aaae3473d14f62d3cbb554e099bda1093454b7935890139f3fe5a8R558-R563)

add 10 minute rate limit

f564f30

isabellaenriquez self-assigned this Oct 12, 2023

vercel bot deployed to Preview October 12, 2023 22:09 View deployment

isabellaenriquez changed the title ~~feat(metric-alerts): Add ratelimiting~~ feat(alerts): Add ratelimiting for metric alerts Oct 12, 2023

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Oct 12, 2023

correct how minutes are calculated

dbd5446

vercel bot deployed to Preview October 12, 2023 22:46 View deployment

move out logic (tests still fail 😩 )

f111926

vercel bot deployed to Preview October 12, 2023 23:11 View deployment

account for distinct triggers

6fa26fd

vercel bot deployed to Preview October 13, 2023 17:12 View deployment

account for different subscriptions

254bf93

vercel bot deployed to Preview October 13, 2023 17:24 View deployment

Merge branch 'master' into isabella/metric-alert-rate-limit

50d6084

vercel bot deployed to Preview October 13, 2023 17:34 View deployment

isabellaenriquez added 2 commits October 13, 2023 11:20

add to trigger alerts before checking whether to make another inciden…

79b8339

…t + clean up

add tests for ratelimiting

e89d062

vercel bot deployed to Preview October 13, 2023 18:25 View deployment

isabellaenriquez marked this pull request as ready for review October 13, 2023 18:25

isabellaenriquez requested a review from a team as a code owner October 13, 2023 18:25

scefali reviewed Oct 13, 2023

View reviewed changes

src/sentry/incidents/subscription_processor.py Outdated Show resolved Hide resolved

scefali reviewed Oct 13, 2023

View reviewed changes

src/sentry/incidents/subscription_processor.py Outdated Show resolved Hide resolved

scefali reviewed Oct 13, 2023

View reviewed changes

src/sentry/incidents/subscription_processor.py Outdated Show resolved Hide resolved

scefali reviewed Oct 13, 2023

View reviewed changes

src/sentry/incidents/subscription_processor.py Outdated Show resolved Hide resolved

simplify logic a bit + add metric for rate limit

c1aedfd

vercel bot deployed to Preview October 13, 2023 19:43 View deployment

simplify MORE

5a57546

vercel bot deployed to Preview October 13, 2023 19:46 View deployment

isabellaenriquez requested a review from snigdhas October 13, 2023 21:14

JoshFerge reviewed Oct 13, 2023

View reviewed changes

src/sentry/incidents/subscription_processor.py Outdated Show resolved Hide resolved

wedamija reviewed Oct 14, 2023

View reviewed changes

add options flag

efaaf4c

vercel bot deployed to Preview October 16, 2023 16:56 View deployment

isabellaenriquez added 2 commits October 16, 2023 10:23

track project, trigger, and last_incident

58e1962

update tests

7aa233f

vercel bot deployed to Preview October 16, 2023 17:29 View deployment

isabellaenriquez mentioned this pull request Oct 16, 2023

feat(migrations): Add new index to Incident model #58207

Merged

isabellaenriquez added a commit that referenced this pull request Oct 17, 2023

feat(migrations): Add new index to Incident model (#58207)

998b5c3

This PR adds a new index to `Incident` to make querying for the metric alert rate limiting feature more efficient (#58032).

Merge branch 'master' into isabella/metric-alert-rate-limit

119ab22

vercel bot deployed to Preview October 17, 2023 16:30 View deployment

isabellaenriquez mentioned this pull request Oct 19, 2023

chore(migrations): Drop unused index in Incident table #58449

Merged

update last_incident retrieval

f56d3e2

vercel bot deployed to Preview October 19, 2023 16:40 View deployment

isabellaenriquez mentioned this pull request Oct 19, 2023

feat(migrations): Add new index to IncidentTrigger model #58467

Merged

wedamija approved these changes Oct 19, 2023

View reviewed changes

isabellaenriquez merged commit 03e3b04 into master Oct 23, 2023
50 checks passed

isabellaenriquez deleted the isabella/metric-alert-rate-limit branch October 23, 2023 20:18

isabellaenriquez mentioned this pull request Oct 23, 2023

Remove flag for metric alert rate limiting #58648

Closed

github-actions bot locked and limited conversation to collaborators Nov 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(alerts): Add ratelimiting for metric alerts #58032

feat(alerts): Add ratelimiting for metric alerts #58032

isabellaenriquez commented Oct 12, 2023 •

edited

Loading

codecov bot commented Oct 12, 2023 •

edited

Loading

scttcper commented Oct 13, 2023

JoshFerge left a comment

JoshFerge Oct 13, 2023

isabellaenriquez Oct 16, 2023

wedamija left a comment

wedamija Oct 14, 2023

isabellaenriquez Oct 16, 2023

wedamija Oct 16, 2023

wedamija commented Oct 19, 2023

feat(alerts): Add ratelimiting for metric alerts #58032

feat(alerts): Add ratelimiting for metric alerts #58032

Conversation

isabellaenriquez commented Oct 12, 2023 • edited Loading

codecov bot commented Oct 12, 2023 • edited Loading

Codecov Report

scttcper commented Oct 13, 2023

JoshFerge left a comment

Choose a reason for hiding this comment

JoshFerge Oct 13, 2023

Choose a reason for hiding this comment

isabellaenriquez Oct 16, 2023

Choose a reason for hiding this comment

wedamija left a comment

Choose a reason for hiding this comment

wedamija Oct 14, 2023

Choose a reason for hiding this comment

isabellaenriquez Oct 16, 2023

Choose a reason for hiding this comment

wedamija Oct 16, 2023

Choose a reason for hiding this comment

wedamija commented Oct 19, 2023

isabellaenriquez commented Oct 12, 2023 •

edited

Loading

codecov bot commented Oct 12, 2023 •

edited

Loading