From e2e01d7da98badc48dbc6bca4ef95e5a9175a34c Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 20 May 2025 13:52:54 -0700 Subject: [PATCH 1/4] remove org id --- .../incidents/subscription_processor.py | 34 +++++++++++-------- .../workflow_engine/processors/workflow.py | 19 +++-------- .../incidents/test_subscription_processor.py | 18 ++++++++-- .../processors/test_workflow.py | 10 ++---- 4 files changed, 43 insertions(+), 38 deletions(-) diff --git a/src/sentry/incidents/subscription_processor.py b/src/sentry/incidents/subscription_processor.py index aeb72208860477..be083da00e76e4 100644 --- a/src/sentry/incidents/subscription_processor.py +++ b/src/sentry/incidents/subscription_processor.py @@ -368,16 +368,6 @@ def process_update(self, subscription_update: QuerySubscriptionUpdate) -> None: "organizations:anomaly-detection-alerts", organization ) and features.has("organizations:anomaly-detection-rollout", organization) - alert_triggered_tags = { - "detection_type": self.alert_rule.detection_type, - "organization_id": None, - } - if features.has( - "organizations:workflow-engine-metric-alert-dual-processing-logs", - self.subscription.project.organization, - ): - alert_triggered_tags["organization_id"] = self.alert_rule.organization_id - potential_anomalies = None if ( has_anomaly_detection @@ -440,7 +430,7 @@ def process_update(self, subscription_update: QuerySubscriptionUpdate) -> None: ) and not self.check_trigger_matches_status(trigger, TriggerStatus.ACTIVE): metrics.incr( "incidents.alert_rules.threshold.alert", - tags=alert_triggered_tags, + tags={"detection_type": self.alert_rule.detection_type}, ) incident_trigger = self.trigger_alert_threshold( trigger, aggregation_value @@ -457,7 +447,7 @@ def process_update(self, subscription_update: QuerySubscriptionUpdate) -> None: ): metrics.incr( "incidents.alert_rules.threshold.resolve", - tags=alert_triggered_tags, + tags={"detection_type": self.alert_rule.detection_type}, ) incident_trigger = self.trigger_resolve_threshold( trigger, aggregation_value @@ -479,8 +469,16 @@ def process_update(self, subscription_update: QuerySubscriptionUpdate) -> None: # And the trigger is not yet active metrics.incr( "incidents.alert_rules.threshold.alert", - tags=alert_triggered_tags, + tags={"detection_type": self.alert_rule.detection_type}, ) + if features.has( + "organizations:workflow-engine-metric-alert-dual-processing-logs", + self.subscription.project.organization, + ): + metrics.incr( + "dual_processing.alert_rules.fire", + tags={"detection_type": self.alert_rule.detection_type}, + ) # triggering a threshold will create an incident and set the status to active incident_trigger = self.trigger_alert_threshold(trigger, aggregation_value) if incident_trigger is not None: @@ -497,8 +495,16 @@ def process_update(self, subscription_update: QuerySubscriptionUpdate) -> None: ): metrics.incr( "incidents.alert_rules.threshold.resolve", - tags=alert_triggered_tags, + tags={"detection_type": self.alert_rule.detection_type}, ) + if features.has( + "organizations:workflow-engine-metric-alert-dual-processing-logs", + self.subscription.project.organization, + ): + metrics.incr( + "dual_processing.alert_rules.fire", + tags={"detection_type": self.alert_rule.detection_type}, + ) incident_trigger = self.trigger_resolve_threshold( trigger, aggregation_value ) diff --git a/src/sentry/workflow_engine/processors/workflow.py b/src/sentry/workflow_engine/processors/workflow.py index d9b8ebcd3cfc05..cf38ea63710e7d 100644 --- a/src/sentry/workflow_engine/processors/workflow.py +++ b/src/sentry/workflow_engine/processors/workflow.py @@ -1,7 +1,6 @@ import logging from dataclasses import asdict, replace from enum import StrEnum -from typing import Any import sentry_sdk from django.db import router, transaction @@ -218,14 +217,6 @@ def process_workflows(event_data: WorkflowEventData) -> set[Workflow]: # TODO: remove fetching org, only used for feature flag checks organization = detector.project.organization - workflow_metric_tags: dict[str, Any] = { - "detector_type": detector.type, - "organization_id": None, - } - if features.has( - "organizations:workflow-engine-metric-alert-dual-processing-logs", organization - ): - workflow_metric_tags["organization_id"] = organization.id # Get the workflows, evaluate the when_condition_group, finally evaluate the actions for workflows that are triggered workflows = set( @@ -240,7 +231,7 @@ def process_workflows(event_data: WorkflowEventData) -> set[Workflow]: metrics.incr( "workflow_engine.process_workflows", len(workflows), - tags=workflow_metric_tags, + tags={"detector_type": detector.type}, ) logger.info( @@ -263,7 +254,7 @@ def process_workflows(event_data: WorkflowEventData) -> set[Workflow]: metrics.incr( "workflow_engine.process_workflows.triggered_workflows", len(triggered_workflows), - tags=workflow_metric_tags, + tags={"detector_type": detector.type}, ) logger.info( @@ -284,7 +275,7 @@ def process_workflows(event_data: WorkflowEventData) -> set[Workflow]: metrics.incr( "workflow_engine.process_workflows.actions", amount=len(actions), - tags=workflow_metric_tags, + tags={"detector_type": detector.type}, ) logger.info( @@ -309,7 +300,7 @@ def process_workflows(event_data: WorkflowEventData) -> set[Workflow]: metrics.incr( "workflow_engine.process_workflows.triggered_actions", amount=len(actions), - tags=workflow_metric_tags, + tags={"detector_type": detector.type}, ) logger.info( "workflow_engine.process_workflows.triggered_actions (batch)", @@ -324,7 +315,7 @@ def process_workflows(event_data: WorkflowEventData) -> set[Workflow]: # in order to check if workflow engine is firing 1:1 with the old system, we must only count once rather than each action metrics.incr( "workflow_engine.process_workflows.fired_actions", - tags=workflow_metric_tags, + tags={"detector_type": detector.type}, ) return triggered_workflows diff --git a/tests/sentry/incidents/test_subscription_processor.py b/tests/sentry/incidents/test_subscription_processor.py index 537a851b3fc244..5a3b8bc81bbc17 100644 --- a/tests/sentry/incidents/test_subscription_processor.py +++ b/tests/sentry/incidents/test_subscription_processor.py @@ -1075,7 +1075,11 @@ def test_alert_metrics(self, mock_metrics): [ call( "incidents.alert_rules.threshold.alert", - tags={"detection_type": "static", "organization_id": rule.organization_id}, + tags={"detection_type": "static"}, + ), + call( + "dual_processing.alert_rules.fire", + tags={"detection_type": "static"}, ), call("incidents.alert_rules.trigger", tags={"type": "fire"}), ], @@ -1271,12 +1275,20 @@ def test_resolve_metrics(self, mock_metrics): [ call( "incidents.alert_rules.threshold.alert", - tags={"detection_type": "static", "organization_id": rule.organization_id}, + tags={"detection_type": "static"}, + ), + call( + "dual_processing.alert_rules.fire", + tags={"detection_type": "static"}, ), call("incidents.alert_rules.trigger", tags={"type": "fire"}), call( "incidents.alert_rules.threshold.resolve", - tags={"detection_type": "static", "organization_id": rule.organization_id}, + tags={"detection_type": "static"}, + ), + call( + "dual_processing.alert_rules.fire", + tags={"detection_type": "static"}, ), call("incidents.alert_rules.trigger", tags={"type": "resolve"}), ] diff --git a/tests/sentry/workflow_engine/processors/test_workflow.py b/tests/sentry/workflow_engine/processors/test_workflow.py index 02eb37f48e315d..f7704f9ceb9d70 100644 --- a/tests/sentry/workflow_engine/processors/test_workflow.py +++ b/tests/sentry/workflow_engine/processors/test_workflow.py @@ -62,10 +62,6 @@ def setUp(self): id=1, is_new=False, is_regression=True, is_new_group_environment=False ), ) - self.workflow_metric_tags = { - "detector_type": self.error_detector.type, - "organization_id": None, - } def test_skips_disabled_workflows(self): workflow_triggers = self.create_data_condition_group() @@ -280,7 +276,7 @@ def test_metrics_with_workflows(self, mock_incr): mock_incr.assert_any_call( "workflow_engine.process_workflows", 1, - tags=self.workflow_metric_tags, + tags={"detector_type": self.error_detector.type}, ) @patch("sentry.utils.metrics.incr") @@ -290,7 +286,7 @@ def test_metrics_triggered_workflows(self, mock_incr): mock_incr.assert_any_call( "workflow_engine.process_workflows.triggered_workflows", 1, - tags=self.workflow_metric_tags, + tags={"detector_type": self.error_detector.type}, ) @with_feature("organizations:workflow-engine-process-workflows") @@ -302,7 +298,7 @@ def test_metrics_triggered_actions(self, mock_incr): mock_incr.assert_any_call( "workflow_engine.process_workflows.triggered_actions", amount=0, - tags=self.workflow_metric_tags, + tags={"detector_type": self.error_detector.type}, ) @with_feature("organizations:workflow-engine-process-workflows") From 90bb28afbed50e90da54d262a9579cc34a7ff78f Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 20 May 2025 13:55:55 -0700 Subject: [PATCH 2/4] remove unnecessary tag --- src/sentry/incidents/subscription_processor.py | 10 ++-------- tests/sentry/incidents/test_subscription_processor.py | 3 --- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/src/sentry/incidents/subscription_processor.py b/src/sentry/incidents/subscription_processor.py index be083da00e76e4..151ec34292d737 100644 --- a/src/sentry/incidents/subscription_processor.py +++ b/src/sentry/incidents/subscription_processor.py @@ -467,18 +467,12 @@ def process_update(self, subscription_update: QuerySubscriptionUpdate) -> None: ) and not self.check_trigger_matches_status(trigger, TriggerStatus.ACTIVE): # If the value has breached our threshold (above/below) # And the trigger is not yet active - metrics.incr( - "incidents.alert_rules.threshold.alert", - tags={"detection_type": self.alert_rule.detection_type}, - ) + metrics.incr("incidents.alert_rules.threshold.alert") if features.has( "organizations:workflow-engine-metric-alert-dual-processing-logs", self.subscription.project.organization, ): - metrics.incr( - "dual_processing.alert_rules.fire", - tags={"detection_type": self.alert_rule.detection_type}, - ) + metrics.incr("dual_processing.alert_rules.fire") # triggering a threshold will create an incident and set the status to active incident_trigger = self.trigger_alert_threshold(trigger, aggregation_value) if incident_trigger is not None: diff --git a/tests/sentry/incidents/test_subscription_processor.py b/tests/sentry/incidents/test_subscription_processor.py index 5a3b8bc81bbc17..c99fc303f8ff31 100644 --- a/tests/sentry/incidents/test_subscription_processor.py +++ b/tests/sentry/incidents/test_subscription_processor.py @@ -1079,7 +1079,6 @@ def test_alert_metrics(self, mock_metrics): ), call( "dual_processing.alert_rules.fire", - tags={"detection_type": "static"}, ), call("incidents.alert_rules.trigger", tags={"type": "fire"}), ], @@ -1279,7 +1278,6 @@ def test_resolve_metrics(self, mock_metrics): ), call( "dual_processing.alert_rules.fire", - tags={"detection_type": "static"}, ), call("incidents.alert_rules.trigger", tags={"type": "fire"}), call( @@ -1288,7 +1286,6 @@ def test_resolve_metrics(self, mock_metrics): ), call( "dual_processing.alert_rules.fire", - tags={"detection_type": "static"}, ), call("incidents.alert_rules.trigger", tags={"type": "resolve"}), ] From a63abe80176f861882cbeca53d8f038a54888cdf Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 20 May 2025 14:34:39 -0700 Subject: [PATCH 3/4] oops removed tags on the wrong metric --- src/sentry/incidents/subscription_processor.py | 10 +++++----- tests/sentry/incidents/test_subscription_processor.py | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/sentry/incidents/subscription_processor.py b/src/sentry/incidents/subscription_processor.py index 151ec34292d737..1266d9abbe746b 100644 --- a/src/sentry/incidents/subscription_processor.py +++ b/src/sentry/incidents/subscription_processor.py @@ -467,7 +467,10 @@ def process_update(self, subscription_update: QuerySubscriptionUpdate) -> None: ) and not self.check_trigger_matches_status(trigger, TriggerStatus.ACTIVE): # If the value has breached our threshold (above/below) # And the trigger is not yet active - metrics.incr("incidents.alert_rules.threshold.alert") + metrics.incr( + "incidents.alert_rules.threshold.alert", + tags={"detection_type": self.alert_rule.detection_type}, + ) if features.has( "organizations:workflow-engine-metric-alert-dual-processing-logs", self.subscription.project.organization, @@ -495,10 +498,7 @@ def process_update(self, subscription_update: QuerySubscriptionUpdate) -> None: "organizations:workflow-engine-metric-alert-dual-processing-logs", self.subscription.project.organization, ): - metrics.incr( - "dual_processing.alert_rules.fire", - tags={"detection_type": self.alert_rule.detection_type}, - ) + metrics.incr("dual_processing.alert_rules.fire") incident_trigger = self.trigger_resolve_threshold( trigger, aggregation_value ) diff --git a/tests/sentry/incidents/test_subscription_processor.py b/tests/sentry/incidents/test_subscription_processor.py index c99fc303f8ff31..df4713a529c8b8 100644 --- a/tests/sentry/incidents/test_subscription_processor.py +++ b/tests/sentry/incidents/test_subscription_processor.py @@ -1059,7 +1059,7 @@ def test_alert(self, create_metric_issue_mock, mock_metrics): [ call( "incidents.alert_rules.threshold.alert", - tags={"detection_type": "static", "organization_id": None}, + tags={"detection_type": "static"}, ), call("incidents.alert_rules.trigger", tags={"type": "fire"}), ], @@ -1252,12 +1252,12 @@ def test_resolve(self, create_metric_issue_mock, mock_metrics): [ call( "incidents.alert_rules.threshold.alert", - tags={"detection_type": "static", "organization_id": None}, + tags={"detection_type": "static"}, ), call("incidents.alert_rules.trigger", tags={"type": "fire"}), call( "incidents.alert_rules.threshold.resolve", - tags={"detection_type": "static", "organization_id": None}, + tags={"detection_type": "static"}, ), call("incidents.alert_rules.trigger", tags={"type": "resolve"}), ] From 6d61ff9cc09bb21b81d0d568ee8ef4916ca7fb3d Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 20 May 2025 14:55:25 -0700 Subject: [PATCH 4/4] fix test --- tests/sentry/workflow_engine/processors/test_workflow.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/sentry/workflow_engine/processors/test_workflow.py b/tests/sentry/workflow_engine/processors/test_workflow.py index f7704f9ceb9d70..5c6d20ae032e28 100644 --- a/tests/sentry/workflow_engine/processors/test_workflow.py +++ b/tests/sentry/workflow_engine/processors/test_workflow.py @@ -310,7 +310,6 @@ def test_metrics_issue_dual_processing_metrics(self, mock_incr): "workflow_engine.process_workflows.fired_actions", tags={ "detector_type": self.error_detector.type, - "organization_id": self.error_detector.project.organization_id, }, )