diff --git a/src/sentry/incidents/subscription_processor.py b/src/sentry/incidents/subscription_processor.py index 6276ab2701ef71..25751bbcc68eeb 100644 --- a/src/sentry/incidents/subscription_processor.py +++ b/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 diff --git a/src/sentry/incidents/utils/subscription_limits.py b/src/sentry/incidents/utils/subscription_limits.py index 92332aa5d6ab17..fc4a4edae41ae4 100644 --- a/src/sentry/incidents/utils/subscription_limits.py +++ b/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: diff --git a/tests/sentry/incidents/subscription_processor/test_subscription_processor_aci.py b/tests/sentry/incidents/subscription_processor/test_subscription_processor_aci.py index 04f154714cc371..80a5ec22eb6f7b 100644 --- a/tests/sentry/incidents/subscription_processor/test_subscription_processor_aci.py +++ b/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}, - ) diff --git a/tests/sentry/incidents/utils/test_subscription_limits.py b/tests/sentry/incidents/utils/test_subscription_limits.py new file mode 100644 index 00000000000000..924fc364e25dc4 --- /dev/null +++ b/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