feat(incidents): Add is_metric_subscription_allowed; use it

Author: kcons

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,7 +229,11 @@ 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 subscription_update["timestamp"] <= self.last_update:

src/sentry/incidents/utils/subscription_limits.py

@@ -1,7 +1,33 @@
+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).
+    """
+    if not features.has("organizations:incidents", organization):
+        return False
+
+    if dataset == Dataset.Transactions.value:
+        return features.has("organizations:performance-view", organization)
+
+    if dataset == Dataset.EventsAnalyticsPlatform.value:
+        return features.has("organizations:visibility-explore-view", organization)
+
+    if dataset == Dataset.PerformanceMetrics.value:
+        return features.has("organizations:on-demand-metrics-extraction", organization)
+
+    return True
 
 
 def get_max_metric_alert_subscriptions(organization: Organization) -> int:

tests/sentry/incidents/subscription_processor/test_subscription_processor_aci.py

@@ -12,7 +12,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.subscription_limits import is_metric_subscription_allowed
 from sentry.incidents.utils.types import QuerySubscriptionUpdate
 from sentry.models.organization import Organization
 from sentry.seer.anomaly_detection.types import (
@@ -65,17 +66,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:
@@ -1014,90 +1016,94 @@ def test_ensure_case_when_no_metrics_index_not_found_is_handled_gracefully(
         )
 
 
-@patch("sentry.incidents.subscription_processor.metrics")
-class TestHasDowngraded:
+class TestIsMetricSubscriptionAllowed:
     org = MagicMock(spec=Organization)
 
     @contextmanager
     def fake_features(self, enabled: set[str]):
-        with patch("sentry.incidents.subscription_processor.features") as mock_features:
+        with patch("sentry.incidents.utils.subscription_limits.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:
+    # -- :incidents is the universal gate --
+
+    def test_no_incidents_disables_events(self) -> 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"
-        )
+            assert is_metric_subscription_allowed(Dataset.Events.value, self.org) is False
 
-    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_no_incidents_disables_transactions(self) -> None:
+        with self.fake_features({"organizations:performance-view"}):
+            assert is_metric_subscription_allowed(Dataset.Transactions.value, self.org) is False
 
-    def test_transactions_without_any_features(self, mock_metrics: MagicMock) -> None:
+    def test_no_incidents_disables_eap(self) -> None:
+        with self.fake_features({"organizations:visibility-explore-view"}):
+            assert (
+                is_metric_subscription_allowed(Dataset.EventsAnalyticsPlatform.value, self.org)
+                is False
+            )
+
+    def test_no_incidents_disables_performance_metrics(self) -> None:
+        with self.fake_features({"organizations:on-demand-metrics-extraction"}):
+            assert (
+                is_metric_subscription_allowed(Dataset.PerformanceMetrics.value, self.org) is False
+            )
+
+    def test_no_incidents_disables_unknown_dataset(self) -> 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"
-        )
+            assert is_metric_subscription_allowed("unknown_dataset", self.org) is False
 
-    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"
-        )
+    # -- Events --
+
+    def test_events_enabled(self) -> None:
+        with self.fake_features({"organizations:incidents"}):
+            assert is_metric_subscription_allowed(Dataset.Events.value, self.org) is True
+
+    # -- Transactions --
+
+    def test_transactions_without_performance_view(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, mock_metrics: MagicMock) -> None:
+    def test_transactions_with_both_features(self) -> None:
         with self.fake_features({"organizations:incidents", "organizations:performance-view"}):
-            assert has_downgraded(Dataset.Transactions.value, self.org) is False
+            assert is_metric_subscription_allowed(Dataset.Transactions.value, self.org) is True
 
-    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"
-        )
+    # -- 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_without_explore_view(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, mock_metrics: MagicMock) -> None:
+    def test_eap_with_both_features(self) -> None:
         with self.fake_features(
             {"organizations:incidents", "organizations:visibility-explore-view"}
         ):
-            assert has_downgraded(Dataset.EventsAnalyticsPlatform.value, self.org) is False
+            assert (
+                is_metric_subscription_allowed(Dataset.EventsAnalyticsPlatform.value, self.org)
+                is True
+            )
 
-    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"
-        )
+    # -- PerformanceMetrics --
 
-    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_performance_metrics_without_on_demand(self) -> None:
+        with self.fake_features({"organizations:incidents"}):
+            assert (
+                is_metric_subscription_allowed(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_performance_metrics_with_both_features(self) -> None:
+        with self.fake_features(
+            {"organizations:incidents", "organizations:on-demand-metrics-extraction"}
+        ):
+            assert (
+                is_metric_subscription_allowed(Dataset.PerformanceMetrics.value, self.org) is True
+            )
 
-    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},
-        )
+    # -- Unknown / other datasets --
+
+    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