Add limit to API for Alert Rules #70596
Conversation
… to match the combined-rules implementation today.
github-actions
bot
added
the
Scope: Backend
src/sentry/api/helpers/constants.py
Outdated
| @@ -0,0 +1,2 @@ | |||
| MAX_QUERY_SUBSCTIPTIONS_HEADER = "X-Sentry-Rule-Limit" | |||
There was a problem hiding this comment.
SUBSCTIPTIONS typo 😛 also should this be X-Sentry-Alert-Rule-Limit?
There was a problem hiding this comment.
at least i was consistent in my typo. 🤣 - ty - updating!
for X-Sentry-Alert-Rule-Limit do you know if the snuba subscription limit only impacts metric alerts? (i believe so, in which case agree that this is a better name)
There was a problem hiding this comment.
Yeah, if you look here it's checking the QuerySubscriptions which are only used for metric alerts.
| @@ -0,0 +1,2 @@ | |||
| MAX_QUERY_SUBSCTIPTIONS_HEADER = "X-Sentry-Rule-Limit" | |||
| ALERT_RULES_COUNT_HEADER = "X-Sentry-Alert-Rule-Hits" | |||
There was a problem hiding this comment.
Maybe this should be "remaining" instead of "hits" to match the API rate limits: https://docs.sentry.io/api/ratelimits/#headers
There was a problem hiding this comment.
Eh I could see it either way, up to you
There was a problem hiding this comment.
In this case, I don't think we should rename this because it already existed on the combined-rules api. i just also wanted to move this so it's consistent between alert-rules and combined-rules.
Description
Fixes: #67195
Adds the
x-sentry-rule-limitheader to the list views for alert rules (both the combined-rules and metric alert api).I tried adding an indicator to the UI, but it was pretty difficult to tell which page we're on in the UI, since we're handling it on the backend with a
Link. Rather than blocking the API fix on trying to figure out the UI, i figure it's better to get this out and we can add to the UI later.