Skip to content

feat(aci): Enforce quota limits when creating metric detectors #97445

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.

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

leedongwei
Copy link
Member

No description provided.

@leedongwei leedongwei requested review from a team as code owners August 7, 2025 22:43
@leedongwei leedongwei requested a review from a team August 7, 2025 22:43
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Aug 7, 2025
cursor[bot]

This comment was marked as outdated.

Copy link

codecov bot commented Aug 7, 2025

Codecov Report

❌ Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/sentry/quotas/base.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #97445      +/-   ##
==========================================
- Coverage   80.64%   80.58%   -0.07%     
==========================================
  Files        8572     8550      -22     
  Lines      377724   375362    -2362     
  Branches    24579    24431     -148     
==========================================
- Hits       304624   302471    -2153     
+ Misses      72734    72520     -214     
- Partials      366      371       +5     

AlertRule.objects.filter(
organization=organization,
)
.exclude(status=AlertRuleStatus.SNAPSHOT.value)
Copy link
Member

Choose a reason for hiding this comment

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

The default AlertRule manager already excludes snapshots, there's a separate AlertRule.objects_with_snapshots for inclusion. You could also use AlertRule.fetch_for_organization() since we have the org.


# Create 2 existing metric alert rules (1 active, 1 to be deleted)
self.create_alert_rule(organization=self.organization, status=AlertRuleStatus.PENDING)
self.create_alert_rule(organization=self.organization, status=AlertRuleStatus.SNAPSHOT)
Copy link
Member

Choose a reason for hiding this comment

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

nit: You could just do

alert_rule = self.create_alert_rule(organization=self.organization)
alert_rule.status = AlertRuleStatus.SNAPSHOT.value
.alert_rule.save()

instead of updating create_alert_rule and the test util for one test

Comment on lines +1679 to +1680
# Set limit to -1 (unlimited)
mock_get_limit.return_value = -1
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it already -1?

Comment on lines +318 to +338
if detector_type == MetricIssue.slug:
if not features.has("organizations:incidents", organization, actor=request.user):
raise ResourceDoesNotExist

if features.has(
"organizations:workflow-engine-metric-detector-limit",
organization,
actor=request.user,
):
alert_count = Detector.objects.filter(
project__organization=organization,
type=detector_type,
status=ObjectStatus.ACTIVE,
).count()
alert_limit = quotas.backend.get_metric_detector_limit(organization.id)

if alert_limit >= 0 and alert_count >= alert_limit:
return Response(
f"You may not exceed {alert_limit} metric detectors on your current plan.",
status=400,
)
Copy link
Contributor

@saponifi3d saponifi3d Aug 11, 2025

Choose a reason for hiding this comment

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

could we make this generic and use it in the detector validator? this could either be added to the detector handler if we need it to be a specific implementation.

class DetectorValidator(Serializer):
    def validate_quota(data):
        # example with a generic request to the quotas service with the detector type
        detector_limit = quotas.backend.get_detector_limit(organization.id, data.type)
        
        if detector_limit < 0 or detector_count < detector_limit:
               raise ValidatorError(
                   'Unable to create Detector, quota limit reached',
                   extra={"limit": detector_limit, "count": detector_count},
               )
        
    def validate(data):
        validate_quota(data)
        ....

It's not great since it will execute the quote limit every time the validator is invoked, but i think we'd want to verify the quote every time it's created / modified. We could also change the name from validate_quota to something else, so it doesn't automatically execute.

If we can't execute a general quote request like that example, we could add a method to the DetectorSettings class. That method could validate types individually, and we just use the code here to grab the handler from the registry.

class DetectorValidator(Serializer):
    def validate_quota(data):
        # example with detector handler
        quota_handler = grouptype.registry.get_by_slug(self.type).detector_settings.quota_handler

        try:
             has_quota = quota_handler()
        except:
             has_quota = False

        if not has_quota:
              raise ValidationError(f"You have exceeded quota on {data.type} detectors.")
        
    def validate(data):
        validate_quota(data)
        ....

Copy link
Member

@dashed dashed left a comment

Choose a reason for hiding this comment

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

Changes to src/sentry/quotas/base.py looks good to me. I can't speak for the rest of the changes.

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.

5 participants