Revert "feat(crons): Add QuotaConfig for check-in rate-limits (#60342)"

This reverts commit f6e945981af72d7c034ccbc446490115616c274e.

Co-authored-by: phacops <336345+phacops@users.noreply.github.com>

src/sentry/conf/server.py

@@ -1502,8 +1502,6 @@ SENTRY_FEATURES: dict[str, bool | None] = {
     "organizations:higher-ownership-limit": False,
     # Enable Monitors (Crons) view
     "organizations:monitors": False,
-    # Enable rate-limiting via relay for Monitors (crons)
-    "organizations:monitors-quota-rate-limit": False,
     # Enable Performance view
     "organizations:performance-view": True,
     # Enable profiling

src/sentry/features/__init__.py

@@ -125,7 +125,6 @@ default_manager.add("organizations:anr-analyze-frames", OrganizationFeature, Fea
 default_manager.add("organizations:device-classification", OrganizationFeature, FeatureHandlerStrategy.REMOTE)
 default_manager.add("organizations:device-class-synthesis", OrganizationFeature, FeatureHandlerStrategy.INTERNAL)
 default_manager.add("organizations:monitors", OrganizationFeature, FeatureHandlerStrategy.REMOTE)
-default_manager.add("organizations:monitors-quota-rate-limit", OrganizationFeature, FeatureHandlerStrategy.INTERNAL)
 default_manager.add("organizations:new-page-filter", OrganizationFeature, FeatureHandlerStrategy.REMOTE)
 default_manager.add("organizations:new-weekly-report", OrganizationFeature, FeatureHandlerStrategy.REMOTE)
 default_manager.add("organizations:noisy-alert-warning", OrganizationFeature, FeatureHandlerStrategy.REMOTE)

src/sentry/monitors/models.py

@@ -542,8 +542,6 @@ class MonitorEnvironmentManager(BaseManager["MonitorEnvironment"]):
     def ensure_environment(
         self, project: Project, monitor: Monitor, environment_name: str | None
     ) -> MonitorEnvironment:
-        from sentry.monitors.rate_limit import update_monitor_quota
-
         if not environment_name:
             environment_name = "production"
 
@@ -553,15 +551,9 @@ class MonitorEnvironmentManager(BaseManager["MonitorEnvironment"]):
         # TODO: assume these objects exist once backfill is completed
         environment = Environment.get_or_create(project=project, name=environment_name)
 
-        monitor_env, created = MonitorEnvironment.objects.get_or_create(
+        return MonitorEnvironment.objects.get_or_create(
             monitor=monitor, environment=environment, defaults={"status": MonitorStatus.ACTIVE}
-        )
-
-        # recompute per-project monitor check-in rate limit quota
-        if created:
-            update_monitor_quota(monitor_env)
-
-        return monitor_env
+        )[0]
 
 
 @region_silo_only_model

src/sentry/monitors/rate_limit.py

@@ -1,99 +0,0 @@
-from typing import Optional, Tuple
-
-from django.core.cache import cache
-
-from sentry import features
-from sentry.models.project import Project
-from sentry.monitors.models import MonitorEnvironment
-from sentry.tasks.relay import schedule_invalidate_project_config
-
-# This module is used by the Quotas system to inform relay of the allowed
-# number of check-ins that can be sent per project during the QUOTA_WINDOW.
-#
-# In an ideal world we would be rate-limiting monitors on the key of
-# (organization, monitor_slug, environment). Unfortunately due to how Relay
-# handles quota rate-limiting we're at best able to rate-limit per project.
-#
-# > Relay cannot do rate-limiting like this because it would need to fully
-# > unmarshal the event payload to rate-limit on the monitor slug. The project_id
-# > is available in the payload headers.
-#
-# To allow the monitors system to take-advantage of Relay's quota system to be
-# used for rate-limiting, we can rate-limit per project by providing relay with
-# a quota for the DataCategory.MONITOR that is calculated based on how many
-# monitors the project has.
-#
-# There are some caveats to this:
-#
-# - For upserted monitors we need to give some amount of minimum quota, to
-#   allow monitors to be created even when there are 0 monitors within a
-#   project (since otherwise it would be completely rate-limited)
-#
-# - It is possible that one monitor may saturate the quota, thus rate-limiting
-#   all other monitors within that project. This is an accepted side-effect of
-#   doing rate-limiting this way.
-#
-# - We give each monitor a 'grace' number of allowed check-ins. Ideally each
-#   monitor should only be checking in once per minute, so the quota could just
-#   be the number of monitors in the project. However,
-
-
-# Monitor check-in limits are per minute. This maps to the smallest check-in
-# window that we support.
-QUOTA_WINDOW = 60
-
-# Determines how many check-ins per-minute will be allowed per monitor. This is
-# used when computing the QuotaConfig for the DataCategory.MONITOR (check-ins)
-#
-# These are the 'grace' check-ins as described above.
-ALLOWED_CHECK_INS_PER_MONITOR = 5
-
-# The minimum rate-limit per project for the DataCategory.MONITOR. This value
-# should be high enough that it allows for a large number of monitors to be
-# upserted without hitting the project rate-limit.
-ALLOWED_MINIMUM = 50
-
-
-def get_project_monitor_quota(
-    project: Project,
-    cache_bust=False,
-) -> Tuple[Optional[int], Optional[int]]:
-    """
-    Determines the rate-limit for monitor check-ins across a particular
-    project.
-
-    :return: A (window, limit) tuple. (None, None) indicates no rate-limit
-    """
-    if not features.has(
-        "organizations:monitors-quota-rate-limit", organization=project.organization
-    ):
-        return (None, None)
-
-    limit = None
-    cache_key = f"project:{project.id}:monitor-env-count"
-
-    # Cache rate-limit computation. This function will be called often by the
-    # Quotas system.
-    if not cache_bust:
-        limit = cache.get(cache_key)
-
-    if limit is None:
-        monitor_count = MonitorEnvironment.objects.filter(monitor__project_id=project.id).count()
-        limit = monitor_count * ALLOWED_CHECK_INS_PER_MONITOR
-        cache.set(cache_key, limit, 600)
-
-    return (ALLOWED_MINIMUM + limit, QUOTA_WINDOW)
-
-
-def update_monitor_quota(monitor_env: MonitorEnvironment):
-    """
-    When new monitor environments are created we recompute the per-project
-    monitor check-in rate limit QuotaConfig in relay.
-    """
-    project = Project.objects.get_from_cache(id=monitor_env.monitor.project_id)
-
-    get_project_monitor_quota(project, cache_bust=True)
-    schedule_invalidate_project_config(
-        project_id=project.id,
-        trigger="monitors:monitor_created",
-    )

src/sentry/quotas/base.py

@@ -414,11 +414,6 @@ class Quota(Service):
                     reason_code="project_abuse_limit",
                 )
 
-    def get_monitor_quota(self, project):
-        from sentry.monitors.rate_limit import get_project_monitor_quota
-
-        return get_project_monitor_quota(project)
-
     def get_project_quota(self, project):
         from sentry.models.options.organization_option import OrganizationOption
         from sentry.models.organization import Organization

src/sentry/quotas/redis.py

@@ -87,22 +87,6 @@ class RedisQuota(Quota):
                     )
                 )
 
-        with sentry_sdk.start_span(op="redis.get_quotas.get_monitor_quota") as span:
-            span.set_tag("project.id", project.id)
-            mrlquota = self.get_monitor_quota(project)
-            if mrlquota[0] is not None:
-                results.append(
-                    QuotaConfig(
-                        id="mrl",
-                        limit=mrlquota[0],
-                        window=mrlquota[1],
-                        scope=QuotaScope.PROJECT,
-                        scope_id=project.id,
-                        categories=[DataCategory.MONITOR],
-                        reason_code="monitor_rate_limit",
-                    )
-                )
-
         if key and not keys:
             keys = [key]
         elif not keys:

tests/sentry/monitors/test_rate_limit.py

@@ -1,75 +0,0 @@
-from unittest import mock
-
-from sentry.monitors.models import (
-    Monitor,
-    MonitorEnvironment,
-    MonitorStatus,
-    MonitorType,
-    ScheduleType,
-)
-from sentry.monitors.rate_limit import get_project_monitor_quota
-from sentry.testutils.cases import TestCase
-from sentry.testutils.helpers.features import with_feature
-
-
-@mock.patch("sentry.monitors.rate_limit.QUOTA_WINDOW", 45)
-@mock.patch("sentry.monitors.rate_limit.ALLOWED_CHECK_INS_PER_MONITOR", 2)
-@mock.patch("sentry.monitors.rate_limit.ALLOWED_MINIMUM", 5)
-class MonitorRateLimit(TestCase):
-    def test_disabled(self):
-        """
-        Disabled without feature flag
-        """
-        limit, window = get_project_monitor_quota(self.project)
-        assert limit is None
-        assert window is None
-
-    @with_feature("organizations:monitors-quota-rate-limit")
-    def test_minimum(self):
-        """
-        Without any monitor environments we'll always return ALLOWED_MINIMUM.
-        """
-        limit, window = get_project_monitor_quota(self.project)
-        assert limit == 5
-        assert window == 45
-
-    @with_feature("organizations:monitors-quota-rate-limit")
-    def test_computed_from_environments(self):
-        """
-        Validate that the quota is computed from the total number of monitor
-        environments in a project.
-        """
-        monitor = Monitor.objects.create(
-            organization_id=self.organization.id,
-            project_id=self.project.id,
-            type=MonitorType.CRON_JOB,
-            config={"schedule": "*/5 * * * *", "schedule_type": ScheduleType.CRONTAB},
-        )
-
-        mon_env_count = 10
-        for i in range(mon_env_count):
-            env = self.create_environment(self.project, name=f"test-{i}")
-            MonitorEnvironment.objects.create(
-                monitor=monitor,
-                environment=env,
-                status=MonitorStatus.OK,
-            )
-
-        # The rate limit is per project, create another monitor in a different
-        # project to validate it is not counted
-        project2 = self.create_project(organization=self.organization)
-        monitor2 = Monitor.objects.create(
-            organization_id=self.organization.id,
-            project_id=project2.id,
-            type=MonitorType.CRON_JOB,
-            config={"schedule": "*/5 * * * *", "schedule_type": ScheduleType.CRONTAB},
-        )
-        MonitorEnvironment.objects.create(
-            monitor=monitor2,
-            environment=self.environment,
-            status=MonitorStatus.OK,
-        )
-
-        limit, window = get_project_monitor_quota(self.project, cache_bust=True)
-        assert limit == 25
-        assert window == 45

tests/sentry/quotas/test_redis.py

@@ -76,7 +76,6 @@ class RedisQuotaTest(TestCase):
         # AssertionError: reject-all quotas cannot be tracked
         self.get_project_quota.return_value = (100, 10)
         self.get_organization_quota.return_value = (1000, 10)
-        self.get_monitor_quota.return_value = (15, 60)
 
         # A negative quota means reject-all.
         self.organization.update_option("project-abuse-quota.error-limit", -1)
@@ -200,7 +199,6 @@ class RedisQuotaTest(TestCase):
         # AssertionError: reject-all quotas cannot be tracked
         self.get_project_quota.return_value = (100, 10)
         self.get_organization_quota.return_value = (1000, 10)
-        self.get_monitor_quota.return_value = (15, 60)
 
         self.organization.update_option("project-abuse-quota.transaction-limit", 600)
         with self.feature({"organizations:transaction-metrics-extraction": False}):
@@ -228,17 +226,9 @@ class RedisQuotaTest(TestCase):
         ) as self.get_organization_quota:
             yield
 
-    @pytest.fixture(autouse=True)
-    def _patch_get_monitor_quota(self):
-        with mock.patch.object(
-            RedisQuota, "get_monitor_quota", return_value=(0, 60)
-        ) as self.get_monitor_quota:
-            yield
-
     def test_uses_defined_quotas(self):
         self.get_project_quota.return_value = (200, 60)
         self.get_organization_quota.return_value = (300, 60)
-        self.get_monitor_quota.return_value = (15, 60)
         quotas = self.quota.get_quotas(self.project)
 
         assert quotas[0].id == "p"
@@ -251,11 +241,6 @@ class RedisQuotaTest(TestCase):
         assert quotas[1].scope_id == str(self.organization.id)
         assert quotas[1].limit == 300
         assert quotas[1].window == 60
-        assert quotas[2].id == "mrl"
-        assert quotas[2].scope == QuotaScope.PROJECT
-        assert quotas[2].scope_id == str(self.project.id)
-        assert quotas[2].limit == 15
-        assert quotas[2].window == 60
 
     @mock.patch("sentry.quotas.redis.is_rate_limited")
     @mock.patch.object(RedisQuota, "get_quotas", return_value=[])
@@ -268,14 +253,12 @@ class RedisQuotaTest(TestCase):
     def test_is_not_limited_without_rejections(self, is_rate_limited):
         self.get_organization_quota.return_value = (100, 60)
         self.get_project_quota.return_value = (200, 60)
-        self.get_monitor_quota.return_value = (15, 60)
         assert not self.quota.is_rate_limited(self.project).is_limited
 
     @mock.patch("sentry.quotas.redis.is_rate_limited", return_value=(True, False))
     def test_is_limited_on_rejections(self, is_rate_limited):
         self.get_organization_quota.return_value = (100, 60)
         self.get_project_quota.return_value = (200, 60)
-        self.get_monitor_quota.return_value = (15, 60)
         assert self.quota.is_rate_limited(self.project).is_limited
 
     @mock.patch.object(RedisQuota, "get_quotas")
@@ -331,7 +314,6 @@ class RedisQuotaTest(TestCase):
 
         self.get_project_quota.return_value = (200, 60)
         self.get_organization_quota.return_value = (300, 60)
-        self.get_monitor_quota.return_value = (15, 60)
 
         n = 10
         for _ in range(n):
@@ -345,13 +327,9 @@ class RedisQuotaTest(TestCase):
 
         usage = self.quota.get_usage(self.project.organization_id, all_quotas, timestamp=timestamp)
 
-        assert usage == [
-            n,  # project quota is consumed
-            n,  # organization quota is consumed
-            0,  # monitor quota is not consumed
-            0,  # unlimited quota is not consumed
-            0,  # dummy quota is not consumed
-        ]
+        # Only quotas with an ID are counted in Redis (via this ID). Assume the
+        # count for these quotas and None for the others.
+        assert usage == [n if q.id else None for q in quotas] + [0, 0]
 
     @mock.patch.object(RedisQuota, "get_quotas")
     def test_refund_defaults(self, mock_get_quotas):
@@ -456,7 +434,6 @@ class RedisQuotaTest(TestCase):
 
         self.get_project_quota.return_value = (200, 60)
         self.get_organization_quota.return_value = (300, 60)
-        self.get_monitor_quota.return_value = (15, 60)
 
         n = 10
         for _ in range(n):
@@ -475,10 +452,4 @@ class RedisQuotaTest(TestCase):
         # Only quotas with an ID are counted in Redis (via this ID). Assume the
         # count for these quotas and None for the others.
         # The ``- 1`` is because we refunded once.
-        assert usage == [
-            n - 1,  # project quota has been refunded one
-            n - 1,  # organization quota has been refunded one
-            0,  # monitor quota was not consumed
-            0,  # unlimited quota was not consumed
-            0,  # dummy quota was not consumed
-        ]
+        assert usage == [n - 1 if q.id else None for q in quotas] + [0, 0]