From 97fa0e3846150929dec8735f7b6b01f2e568548f Mon Sep 17 00:00:00 2001 From: Shruthilaya Jaganathan Date: Thu, 1 May 2025 18:00:29 -0400 Subject: [PATCH 1/5] feat: Track alert misfires on EAP --- src/sentry/discover/compare_timeseries.py | 63 ++++++++++++++++--- .../discover/test_compare_timeseries.py | 45 ++++++++++++- 2 files changed, 97 insertions(+), 11 deletions(-) diff --git a/src/sentry/discover/compare_timeseries.py b/src/sentry/discover/compare_timeseries.py index b585762f83365e..65325d54afade2 100644 --- a/src/sentry/discover/compare_timeseries.py +++ b/src/sentry/discover/compare_timeseries.py @@ -12,7 +12,12 @@ from sentry.api.bases.organization import NoProjects from sentry.discover.translation.mep_to_eap import QueryParts, translate_mep_to_eap from sentry.exceptions import IncompatibleMetricsQuery -from sentry.incidents.models.alert_rule import AlertRule +from sentry.incidents.models.alert_rule import ( + AlertRule, + AlertRuleDetectionType, + AlertRuleThresholdType, + AlertRuleTrigger, +) from sentry.models.organization import Organization from sentry.search.eap.types import SearchResolverConfig from sentry.search.events.fields import get_function_alias @@ -148,7 +153,7 @@ def make_snql_request( return TSResultForComparison(result=results, agg_alias=get_function_alias(aggregate)) -def get_mismatch_type(mismatches: dict[int, dict[str, float]]): +def get_mismatch_type(mismatches: dict[int, dict[str, float]], total_buckets: int): all_snql_values_zero = True all_rpc_values_zero = True snql_always_lower = True @@ -178,7 +183,7 @@ def get_mismatch_type(mismatches: dict[int, dict[str, float]]): if snql_value >= rpc_value: snql_always_lower = False - if snql_value > 0 and rpc_value > 0 and diff > 0.2: + if diff > 0.2: has_high_diff = True if confidence == "low": @@ -196,10 +201,10 @@ def get_mismatch_type(mismatches: dict[int, dict[str, float]]): if all_rpc_values_zero: return MismatchType.RPC_ALWAYS_ZERO, many_low_conf_buckets, many_low_sample_rate_buckets - if snql_always_lower: + if snql_always_lower and total_buckets == len(mismatches): return MismatchType.SNQL_ALWAYS_LOWER, many_low_conf_buckets, many_low_sample_rate_buckets - if rpc_always_lower: + if rpc_always_lower and total_buckets == len(mismatches): return MismatchType.RPC_ALWAYS_LOWER, many_low_conf_buckets, many_low_sample_rate_buckets if has_high_diff: @@ -235,6 +240,9 @@ def fill_aligned_series(data: list[dict[str, Any]], alias: str, key: str): def assert_timeseries_close(aligned_timeseries, alert_rule): mismatches: dict[int, dict[str, float]] = {} + false_positive_misfire = 0 + false_negative_misfire = 0 + rule_triggers = AlertRuleTrigger.objects.get_for_alert_rule(alert_rule) missing_buckets = 0 all_zeros = True for timestamp, values in aligned_timeseries.items(): @@ -244,6 +252,42 @@ def assert_timeseries_close(aligned_timeseries, alert_rule): missing_buckets += 1 continue + if alert_rule.detection_type == AlertRuleDetectionType.STATIC: + for trigger in rule_triggers: + would_fire = False + threshold = trigger.alert_threshold + comparison_type = ( + alert_rule.threshold_type + if alert_rule.threshold_type is not None + else trigger.threshold_type + ) # greater or less than + + if ( + comparison_type == AlertRuleThresholdType.ABOVE.value and snql_value > threshold + ) or ( + comparison_type == AlertRuleThresholdType.BELOW.value and snql_value < threshold + ): + would_fire = True + + if would_fire: + if ( + comparison_type == AlertRuleThresholdType.ABOVE.value + and rpc_value < threshold + ) or ( + comparison_type == AlertRuleThresholdType.BELOW.value + and rpc_value > threshold + ): + false_negative_misfire += 1 + else: + if ( + comparison_type == AlertRuleThresholdType.ABOVE.value + and rpc_value > threshold + ) or ( + comparison_type == AlertRuleThresholdType.BELOW.value + and rpc_value < threshold + ): + false_positive_misfire += 1 + # If the sum is 0, we assume that the numbers must be 0, since we have all positive integers. We still do # check the sum in order to protect the division by zero in case for some reason we have -x + x inside of # the timeseries. @@ -264,6 +308,9 @@ def assert_timeseries_close(aligned_timeseries, alert_rule): "confidence": values["confidence"], } + sentry_sdk.set_tag("false_positive_misfires", false_positive_misfire) + sentry_sdk.set_tag("false_negative_misfires", false_negative_misfire) + if mismatches: with sentry_sdk.isolation_scope() as scope: scope.set_extra("mismatches", mismatches) @@ -274,24 +321,21 @@ def assert_timeseries_close(aligned_timeseries, alert_rule): scope.set_tag("buckets_mismatch.count", len(mismatches)) mismatch_type, many_low_conf_buckets, many_low_sample_rate_buckets = get_mismatch_type( - mismatches + mismatches, len(aligned_timeseries) ) scope.set_tag("mismatch_type", mismatch_type.value) scope.set_tag("many_low_conf_buckets", many_low_conf_buckets) scope.set_tag("many_low_sample_rate_buckets", many_low_sample_rate_buckets) sentry_sdk.capture_message("Timeseries mismatch", level="info") - logger.info("Alert %s has too many mismatches", alert_rule.id) return False, mismatches, all_zeros if missing_buckets > 1: sentry_sdk.capture_message("Multiple missing buckets", level="info") - logger.info("Alert %s has multiple missing buckets", alert_rule.id) return False, mismatches, all_zeros - logger.info("Alert %s timeseries is close", alert_rule.id) return True, mismatches, all_zeros @@ -321,6 +365,7 @@ def compare_timeseries_for_alert_rule(alert_rule: AlertRule): sentry_sdk.set_tag("organization", organization.slug) sentry_sdk.set_tag("alert_id", alert_rule.id) + sentry_sdk.set_tag("detection_type", alert_rule.detection_type) on_demand_metrics_enabled = features.has( "organizations:on-demand-metrics-extraction", diff --git a/tests/sentry/discover/test_compare_timeseries.py b/tests/sentry/discover/test_compare_timeseries.py index 2ff7f54a33d253..27a12a1261f397 100644 --- a/tests/sentry/discover/test_compare_timeseries.py +++ b/tests/sentry/discover/test_compare_timeseries.py @@ -4,8 +4,11 @@ import pytest -from sentry.discover.compare_timeseries import compare_timeseries_for_alert_rule -from sentry.incidents.models.alert_rule import AlertRule, AlertRuleProjects +from sentry.discover.compare_timeseries import ( + assert_timeseries_close, + compare_timeseries_for_alert_rule, +) +from sentry.incidents.models.alert_rule import AlertRule, AlertRuleProjects, AlertRuleThresholdType from sentry.snuba.dataset import Dataset from sentry.snuba.metrics.naming_layer.mri import TransactionMRI from sentry.snuba.models import SnubaQuery @@ -172,3 +175,41 @@ def test_compare_mri_alert(self): AlertRuleProjects.objects.create(alert_rule=alert_rule, project=self.project_1) result = compare_timeseries_for_alert_rule(alert_rule) assert result["skipped"] is True + + @mock.patch("sentry.discover.compare_timeseries.sentry_sdk.set_tag") + def test_false_positive_misfires(self, mock_set_tag): + alert_rule = self.create_alert_rule( + dataset=Dataset.PerformanceMetrics, + query_type=SnubaQuery.Type.PERFORMANCE, + aggregate="avg(transaction.duration)", + threshold_type=AlertRuleThresholdType.ABOVE, + ) + self.create_alert_rule_trigger(alert_rule=alert_rule, alert_threshold=75, label="critical") + + aligned_timeseries = { + 1745870400: {"rpc_value": 100, "snql_value": 50, "confidence": 0, "sampling_rate": 0}, + 1745874000: {"rpc_value": 100, "snql_value": 100, "confidence": 0, "sampling_rate": 0}, + } + + assert_timeseries_close(aligned_timeseries=aligned_timeseries, alert_rule=alert_rule) + mock_set_tag.assert_any_call("false_positive_misfires", 1) + mock_set_tag.assert_any_call("false_negative_misfires", 0) + + @mock.patch("sentry.discover.compare_timeseries.sentry_sdk.set_tag") + def test_false_negative_misfires(self, mock_set_tag): + alert_rule = self.create_alert_rule( + dataset=Dataset.PerformanceMetrics, + query_type=SnubaQuery.Type.PERFORMANCE, + aggregate="avg(transaction.duration)", + threshold_type=AlertRuleThresholdType.ABOVE, + ) + self.create_alert_rule_trigger(alert_rule=alert_rule, alert_threshold=75, label="critical") + + aligned_timeseries = { + 1745870400: {"rpc_value": 50, "snql_value": 100, "confidence": 0, "sampling_rate": 0}, + 1745874000: {"rpc_value": 100, "snql_value": 100, "confidence": 0, "sampling_rate": 0}, + } + + assert_timeseries_close(aligned_timeseries=aligned_timeseries, alert_rule=alert_rule) + mock_set_tag.assert_any_call("false_positive_misfires", 0) + mock_set_tag.assert_any_call("false_negative_misfires", 1) From 50f3e0518a11112c4d9ca4316336c8a5fef44177 Mon Sep 17 00:00:00 2001 From: Shruthilaya Jaganathan Date: Thu, 1 May 2025 18:04:00 -0400 Subject: [PATCH 2/5] account for that one missing bucket --- src/sentry/discover/compare_timeseries.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sentry/discover/compare_timeseries.py b/src/sentry/discover/compare_timeseries.py index 65325d54afade2..c6215bff7d75e8 100644 --- a/src/sentry/discover/compare_timeseries.py +++ b/src/sentry/discover/compare_timeseries.py @@ -201,10 +201,10 @@ def get_mismatch_type(mismatches: dict[int, dict[str, float]], total_buckets: in if all_rpc_values_zero: return MismatchType.RPC_ALWAYS_ZERO, many_low_conf_buckets, many_low_sample_rate_buckets - if snql_always_lower and total_buckets == len(mismatches): + if snql_always_lower and total_buckets - (mismatches) < 2: return MismatchType.SNQL_ALWAYS_LOWER, many_low_conf_buckets, many_low_sample_rate_buckets - if rpc_always_lower and total_buckets == len(mismatches): + if rpc_always_lower and total_buckets - len(mismatches) < 2: return MismatchType.RPC_ALWAYS_LOWER, many_low_conf_buckets, many_low_sample_rate_buckets if has_high_diff: From 4d9e8f61ce593f29bceb73ff093887b1a3f799cb Mon Sep 17 00:00:00 2001 From: Shruthilaya Jaganathan Date: Thu, 1 May 2025 18:04:37 -0400 Subject: [PATCH 3/5] account for that one missing bucket --- src/sentry/discover/compare_timeseries.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/discover/compare_timeseries.py b/src/sentry/discover/compare_timeseries.py index c6215bff7d75e8..2119fe8c47df84 100644 --- a/src/sentry/discover/compare_timeseries.py +++ b/src/sentry/discover/compare_timeseries.py @@ -201,7 +201,7 @@ def get_mismatch_type(mismatches: dict[int, dict[str, float]], total_buckets: in if all_rpc_values_zero: return MismatchType.RPC_ALWAYS_ZERO, many_low_conf_buckets, many_low_sample_rate_buckets - if snql_always_lower and total_buckets - (mismatches) < 2: + if snql_always_lower and total_buckets - len(mismatches) < 2: return MismatchType.SNQL_ALWAYS_LOWER, many_low_conf_buckets, many_low_sample_rate_buckets if rpc_always_lower and total_buckets - len(mismatches) < 2: From 8ef86638f703e0a948c2a53677afaef3b0094da7 Mon Sep 17 00:00:00 2001 From: Shruthilaya Jaganathan Date: Fri, 2 May 2025 08:15:42 -0400 Subject: [PATCH 4/5] don't raise no projects --- src/sentry/discover/compare_timeseries.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/sentry/discover/compare_timeseries.py b/src/sentry/discover/compare_timeseries.py index 2119fe8c47df84..f9308cd59f901f 100644 --- a/src/sentry/discover/compare_timeseries.py +++ b/src/sentry/discover/compare_timeseries.py @@ -9,7 +9,6 @@ from django.urls import reverse from sentry import features -from sentry.api.bases.organization import NoProjects from sentry.discover.translation.mep_to_eap import QueryParts, translate_mep_to_eap from sentry.exceptions import IncompatibleMetricsQuery from sentry.incidents.models.alert_rule import ( @@ -343,9 +342,9 @@ def compare_timeseries_for_alert_rule(alert_rule: AlertRule): snuba_query: SnubaQuery = alert_rule.snuba_query project = alert_rule.projects.first() if not project: - raise NoProjects + return {"is_close": False, "skipped": True, "mismatches": {}} - if snuba_query.aggregate in ["apdex()"]: + if "apdex" in snuba_query.aggregate: logger.info( "Skipping alert %s, %s aggregate not yet supported by RPC", alert_rule.id, From 915642ff7d9cf904e3ad041231883d720be27467 Mon Sep 17 00:00:00 2001 From: Shruthilaya Jaganathan Date: Mon, 5 May 2025 11:20:02 -0400 Subject: [PATCH 5/5] chore(alerts-comparison): Optionally get confidence --- src/sentry/discover/compare_timeseries.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sentry/discover/compare_timeseries.py b/src/sentry/discover/compare_timeseries.py index f9308cd59f901f..d4ce132a8bd26b 100644 --- a/src/sentry/discover/compare_timeseries.py +++ b/src/sentry/discover/compare_timeseries.py @@ -167,7 +167,7 @@ def get_mismatch_type(mismatches: dict[int, dict[str, float]], total_buckets: in snql_value = values["snql_value"] rpc_value = values["rpc_value"] diff = values["mismatch_percentage"] - confidence = values["confidence"] + confidence = values.get("confidence") sampling_rate = values.get("sampling_rate") if snql_value > 0: @@ -304,7 +304,7 @@ def assert_timeseries_close(aligned_timeseries, alert_rule): "snql_value": snql_value, "mismatch_percentage": diff, "sampling_rate": values.get("sampling_rate"), - "confidence": values["confidence"], + "confidence": values.get("confidence"), } sentry_sdk.set_tag("false_positive_misfires", false_positive_misfire) @@ -344,7 +344,7 @@ def compare_timeseries_for_alert_rule(alert_rule: AlertRule): if not project: return {"is_close": False, "skipped": True, "mismatches": {}} - if "apdex" in snuba_query.aggregate: + if "apdex" in snuba_query.aggregate or "percentile" in snuba_query.aggregate: logger.info( "Skipping alert %s, %s aggregate not yet supported by RPC", alert_rule.id,