feat(incidents): Add is_metric_subscription_allowed; use it

src/sentry/incidents/subscription_processor.py

@@ -15,6 +15,7 @@
     get_comparison_aggregation_value,
     get_crash_rate_alert_metrics_aggregation_value_helper,
 )
+from sentry.incidents.utils.subscription_limits import is_metric_subscription_allowed
 from sentry.incidents.utils.types import (
     DATA_SOURCE_SNUBA_QUERY_SUBSCRIPTION,
     AnomalyDetectionUpdate,
@@ -53,46 +54,6 @@ class MetricIssueDetectorConfig(TypedDict):
     detection_type: Literal["static", "percent", "dynamic"]
 
 
-def has_downgraded(dataset: str, organization: Organization) -> bool:
-    """
-    Check if the organization has downgraded since the subscription was created.
-    """
-    supports_metrics_issues = features.has("organizations:incidents", organization)
-    if dataset == Dataset.Events.value and not supports_metrics_issues:
-        metrics.incr("incidents.alert_rules.ignore_update_missing_incidents")
-        return True
-
-    supports_performance_view = features.has("organizations:performance-view", organization)
-    if dataset == Dataset.Transactions.value and not (
-        supports_metrics_issues and supports_performance_view
-    ):
-        metrics.incr("incidents.alert_rules.ignore_update_missing_incidents_performance")
-        return True
-
-    supports_explore_view = features.has("organizations:visibility-explore-view", organization)
-    if dataset == Dataset.EventsAnalyticsPlatform.value and not (
-        supports_metrics_issues and supports_explore_view
-    ):
-        metrics.incr("incidents.alert_rules.ignore_update_missing_incidents_eap")
-        return True
-
-    if dataset == Dataset.PerformanceMetrics.value and not features.has(
-        "organizations:on-demand-metrics-extraction", organization
-    ):
-        metrics.incr("incidents.alert_rules.ignore_update_missing_on_demand")
-        return True
-
-    if not supports_metrics_issues:
-        # These should probably be downgraded, but we should know the impact first.
-        metrics.incr(
-            "incidents.alert_rules.no_incidents_not_downgraded",
-            sample_rate=1.0,
-            tags={"dataset": dataset},
-        )
-
-    return False
-
-
 class SubscriptionProcessor:
     """
     Class for processing subscription updates for workflow engine.
@@ -268,9 +229,20 @@ def process_update(self, subscription_update: QuerySubscriptionUpdate) -> bool:
         dataset = self.subscription.snuba_query.dataset
         organization = self.subscription.project.organization
 
-        if has_downgraded(dataset, organization):
+        if not is_metric_subscription_allowed(dataset, organization):
+            metrics.incr(
+                "incidents.alert_rules.ignore_update_not_enabled",
+                tags={"dataset": dataset},
+            )
             return False
 
+        if not features.has("organizations:incidents", organization):
+            metrics.incr(
+                "incidents.alert_rules.no_incidents_not_downgraded",
+                sample_rate=1.0,
+                tags={"dataset": dataset},
+            )
+
         if subscription_update["timestamp"] <= self.last_update:
             metrics.incr("incidents.alert_rules.skipping_already_processed_update")
             return False

src/sentry/incidents/utils/subscription_limits.py

@@ -1,7 +1,35 @@
+from __future__ import annotations
+
 from django.conf import settings
 
-from sentry import options
+from sentry import features, options
 from sentry.models.organization import Organization
+from sentry.snuba.dataset import Dataset
+
+
+def is_metric_subscription_allowed(dataset: str, organization: Organization) -> bool:
+    """
+    Check whether the given organization is allowed to have a metric alert
+    subscription for the given dataset.
+
+    Returns True if allowed, False if the organization lacks the required features
+    (e.g. after a plan downgrade).
+    """
+    has_incidents = features.has("organizations:incidents", organization)
+    if dataset == Dataset.Events.value:
+        return has_incidents
+
+    if dataset == Dataset.Transactions.value:
+        return has_incidents and features.has("organizations:performance-view", organization)
+
+    if dataset == Dataset.EventsAnalyticsPlatform.value:
+        return has_incidents and features.has("organizations:visibility-explore-view", organization)
+
+    if dataset == Dataset.PerformanceMetrics.value:
+        return features.has("organizations:on-demand-metrics-extraction", organization)
+
+    # Other datasets (e.g. Metrics/sessions) aren't gated here but probably should be.
+    return True
 
 
 def get_max_metric_alert_subscriptions(organization: Organization) -> int:

tests/sentry/incidents/subscription_processor/test_subscription_processor_aci.py

@@ -1,6 +1,5 @@
 import copy
 import math
-from contextlib import contextmanager
 from datetime import timedelta
 from functools import cached_property
 from unittest.mock import MagicMock, call, patch
@@ -12,9 +11,8 @@
 from urllib3.response import HTTPResponse
 
 from sentry.constants import ObjectStatus
-from sentry.incidents.subscription_processor import SubscriptionProcessor, has_downgraded
+from sentry.incidents.subscription_processor import SubscriptionProcessor
 from sentry.incidents.utils.types import QuerySubscriptionUpdate
-from sentry.models.organization import Organization
 from sentry.seer.anomaly_detection.types import (
     AnomalyDetectionSeasonality,
     AnomalyDetectionSensitivity,
@@ -65,17 +63,18 @@ def test_missing_project(self, mock_metrics: MagicMock) -> None:
         self.sub.project.delete()
         assert self.send_update(self.critical_threshold + 1) is False
 
-    @patch("sentry.incidents.subscription_processor.has_downgraded", return_value=True)
-    def test_process_update_returns_false_when_downgraded(
-        self, mock_has_downgraded: MagicMock
-    ) -> None:
+    @patch(
+        "sentry.incidents.subscription_processor.is_metric_subscription_allowed",
+        return_value=False,
+    )
+    def test_process_update_returns_false_when_downgraded(self, mock_is_enabled: MagicMock) -> None:
         message = self.build_subscription_update(
             self.sub, value=self.critical_threshold + 1, time_delta=timedelta()
         )
 
         with self.capture_on_commit_callbacks(execute=True):
             assert SubscriptionProcessor.process(self.sub, message) is False
-        mock_has_downgraded.assert_called_once()
+        mock_is_enabled.assert_called_once()
 
     @patch("sentry.incidents.subscription_processor.metrics")
     def test_invalid_aggregation_value(self, mock_metrics: MagicMock) -> None:
@@ -1012,92 +1011,3 @@ def test_ensure_case_when_no_metrics_index_not_found_is_handled_gracefully(
                 call("incidents.alert_rules.skipping_update_invalid_aggregation_value"),
             ]
         )
-
-
-@patch("sentry.incidents.subscription_processor.metrics")
-class TestHasDowngraded:
-    org = MagicMock(spec=Organization)
-
-    @contextmanager
-    def fake_features(self, enabled: set[str]):
-        with patch("sentry.incidents.subscription_processor.features") as mock_features:
-            mock_features.has.side_effect = lambda name, *a, **kw: name in enabled
-            yield
-
-    def test_events_without_incidents_feature(self, mock_metrics: MagicMock) -> None:
-        with self.fake_features(set()):
-            assert has_downgraded(Dataset.Events.value, self.org) is True
-        mock_metrics.incr.assert_called_with(
-            "incidents.alert_rules.ignore_update_missing_incidents"
-        )
-
-    def test_events_with_incidents_feature(self, mock_metrics: MagicMock) -> None:
-        with self.fake_features({"organizations:incidents"}):
-            assert has_downgraded(Dataset.Events.value, self.org) is False
-
-    def test_transactions_without_any_features(self, mock_metrics: MagicMock) -> None:
-        with self.fake_features(set()):
-            assert has_downgraded(Dataset.Transactions.value, self.org) is True
-        mock_metrics.incr.assert_called_with(
-            "incidents.alert_rules.ignore_update_missing_incidents_performance"
-        )
-
-    def test_transactions_with_only_performance_view(self, mock_metrics: MagicMock) -> None:
-        with self.fake_features({"organizations:performance-view"}):
-            assert has_downgraded(Dataset.Transactions.value, self.org) is True
-        mock_metrics.incr.assert_called_with(
-            "incidents.alert_rules.ignore_update_missing_incidents_performance"
-        )
-
-    def test_transactions_with_both_features(self, mock_metrics: MagicMock) -> None:
-        with self.fake_features({"organizations:incidents", "organizations:performance-view"}):
-            assert has_downgraded(Dataset.Transactions.value, self.org) is False
-
-    def test_eap_without_features(self, mock_metrics: MagicMock) -> None:
-        with self.fake_features(set()):
-            assert has_downgraded(Dataset.EventsAnalyticsPlatform.value, self.org) is True
-        mock_metrics.incr.assert_called_with(
-            "incidents.alert_rules.ignore_update_missing_incidents_eap"
-        )
-
-    def test_eap_with_only_explore_view(self, mock_metrics: MagicMock) -> None:
-        with self.fake_features({"organizations:visibility-explore-view"}):
-            assert has_downgraded(Dataset.EventsAnalyticsPlatform.value, self.org) is True
-        mock_metrics.incr.assert_called_with(
-            "incidents.alert_rules.ignore_update_missing_incidents_eap"
-        )
-
-    def test_eap_with_both_features(self, mock_metrics: MagicMock) -> None:
-        with self.fake_features(
-            {"organizations:incidents", "organizations:visibility-explore-view"}
-        ):
-            assert has_downgraded(Dataset.EventsAnalyticsPlatform.value, self.org) is False
-
-    def test_performance_metrics_without_on_demand_feature(self, mock_metrics: MagicMock) -> None:
-        with self.fake_features(set()):
-            assert has_downgraded(Dataset.PerformanceMetrics.value, self.org) is True
-        mock_metrics.incr.assert_called_with(
-            "incidents.alert_rules.ignore_update_missing_on_demand"
-        )
-
-    def test_performance_metrics_with_on_demand_feature(self, mock_metrics: MagicMock) -> None:
-        with self.fake_features({"organizations:on-demand-metrics-extraction"}):
-            assert has_downgraded(Dataset.PerformanceMetrics.value, self.org) is False
-
-    def test_unknown_dataset_not_downgraded(self, mock_metrics: MagicMock) -> None:
-        with self.fake_features(set()):
-            assert has_downgraded("unknown_dataset", self.org) is False
-        mock_metrics.incr.assert_called_with(
-            "incidents.alert_rules.no_incidents_not_downgraded",
-            sample_rate=1.0,
-            tags={"dataset": "unknown_dataset"},
-        )
-
-    def test_no_incidents_not_downgraded_emits_metric(self, mock_metrics: MagicMock) -> None:
-        with self.fake_features({"organizations:on-demand-metrics-extraction"}):
-            assert has_downgraded(Dataset.PerformanceMetrics.value, self.org) is False
-        mock_metrics.incr.assert_called_with(
-            "incidents.alert_rules.no_incidents_not_downgraded",
-            sample_rate=1.0,
-            tags={"dataset": Dataset.PerformanceMetrics.value},
-        )

tests/sentry/incidents/utils/test_subscription_limits.py

@@ -0,0 +1,100 @@
+from contextlib import contextmanager
+from unittest.mock import MagicMock, patch
+
+from sentry.incidents.utils.subscription_limits import is_metric_subscription_allowed
+from sentry.models.organization import Organization
+from sentry.snuba.dataset import Dataset
+
+
+class TestIsMetricSubscriptionAllowed:
+    org = MagicMock(spec=Organization)
+
+    @contextmanager
+    def fake_features(self, enabled: set[str]):
+        with patch("sentry.incidents.utils.subscription_limits.features") as mock_features:
+            mock_features.has.side_effect = lambda name, *a, **kw: name in enabled
+            yield
+
+    # -- Events: requires :incidents --
+
+    def test_events_without_incidents(self) -> None:
+        with self.fake_features(set()):
+            assert is_metric_subscription_allowed(Dataset.Events.value, self.org) is False
+
+    def test_events_with_incidents(self) -> None:
+        with self.fake_features({"organizations:incidents"}):
+            assert is_metric_subscription_allowed(Dataset.Events.value, self.org) is True
+
+    # -- Transactions: requires :incidents + :performance-view --
+
+    def test_transactions_without_any_features(self) -> None:
+        with self.fake_features(set()):
+            assert is_metric_subscription_allowed(Dataset.Transactions.value, self.org) is False
+
+    def test_transactions_with_only_performance_view(self) -> None:
+        with self.fake_features({"organizations:performance-view"}):
+            assert is_metric_subscription_allowed(Dataset.Transactions.value, self.org) is False
+
+    def test_transactions_with_only_incidents(self) -> None:
+        with self.fake_features({"organizations:incidents"}):
+            assert is_metric_subscription_allowed(Dataset.Transactions.value, self.org) is False
+
+    def test_transactions_with_both_features(self) -> None:
+        with self.fake_features({"organizations:incidents", "organizations:performance-view"}):
+            assert is_metric_subscription_allowed(Dataset.Transactions.value, self.org) is True
+
+    # -- EAP: requires :incidents + :visibility-explore-view --
+
+    def test_eap_without_any_features(self) -> None:
+        with self.fake_features(set()):
+            assert (
+                is_metric_subscription_allowed(Dataset.EventsAnalyticsPlatform.value, self.org)
+                is False
+            )
+
+    def test_eap_with_only_explore_view(self) -> None:
+        with self.fake_features({"organizations:visibility-explore-view"}):
+            assert (
+                is_metric_subscription_allowed(Dataset.EventsAnalyticsPlatform.value, self.org)
+                is False
+            )
+
+    def test_eap_with_only_incidents(self) -> None:
+        with self.fake_features({"organizations:incidents"}):
+            assert (
+                is_metric_subscription_allowed(Dataset.EventsAnalyticsPlatform.value, self.org)
+                is False
+            )
+
+    def test_eap_with_both_features(self) -> None:
+        with self.fake_features(
+            {"organizations:incidents", "organizations:visibility-explore-view"}
+        ):
+            assert (
+                is_metric_subscription_allowed(Dataset.EventsAnalyticsPlatform.value, self.org)
+                is True
+            )
+
+    # -- PerformanceMetrics: requires :on-demand-metrics-extraction only --
+
+    def test_performance_metrics_without_on_demand(self) -> None:
+        with self.fake_features(set()):
+            assert (
+                is_metric_subscription_allowed(Dataset.PerformanceMetrics.value, self.org) is False
+            )
+
+    def test_performance_metrics_with_on_demand(self) -> None:
+        with self.fake_features({"organizations:on-demand-metrics-extraction"}):
+            assert (
+                is_metric_subscription_allowed(Dataset.PerformanceMetrics.value, self.org) is True
+            )
+
+    # -- Unknown / other datasets: always allowed --
+
+    def test_unknown_dataset_without_incidents(self) -> None:
+        with self.fake_features(set()):
+            assert is_metric_subscription_allowed("unknown_dataset", self.org) is True
+
+    def test_unknown_dataset_with_incidents(self) -> None:
+        with self.fake_features({"organizations:incidents"}):
+            assert is_metric_subscription_allowed("unknown_dataset", self.org) is True