Skip to content

Commit 41f81ca

Browse files
authored
fix(ACI): Don't send None values to workflow engine processing (#91777)
If an alert rule (that got migrated to become a detector, data conditions, etc) has a comparison delta and ends up getting a `None` aggregation value we were sending that value along to the data packet which causes an error downstream. We can check that it's not None before sending. Fixes https://sentry.sentry.io/issues/6606822297/
1 parent 69ba9fa commit 41f81ca

File tree

2 files changed

+54
-24
lines changed

2 files changed

+54
-24
lines changed

src/sentry/incidents/subscription_processor.py

Lines changed: 33 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,14 @@ def get_comparison_aggregation_value(
312312

313313
if not comparison_aggregate:
314314
metrics.incr("incidents.alert_rules.skipping_update_comparison_value_invalid")
315+
logger.info(
316+
"No comparison aggregate",
317+
extra={
318+
"alert_rule_id": self.alert_rule.id,
319+
"subscription_id": subscription_update.get("subscription_id"),
320+
"organization_id": self.alert_rule.organization_id,
321+
},
322+
)
315323
return None
316324

317325
return (aggregation_value / comparison_aggregate) * 100
@@ -408,33 +416,34 @@ def process_update(self, subscription_update: QuerySubscriptionUpdate) -> None:
408416
},
409417
)
410418

411-
if features.has(
412-
"organizations:workflow-engine-metric-alert-processing",
413-
self.subscription.project.organization,
414-
):
415-
packet = MetricDetectorUpdate(
416-
entity=subscription_update.get("entity", ""),
417-
subscription_id=subscription_update["subscription_id"],
418-
values={"value": aggregation_value},
419-
timestamp=self.last_update,
420-
)
421-
data_packet = DataPacket[MetricDetectorUpdate](
422-
source_id=str(self.subscription.id), packet=packet
423-
)
424-
results = process_data_packets([data_packet], DATA_SOURCE_SNUBA_QUERY_SUBSCRIPTION)
419+
if aggregation_value is not None:
425420
if features.has(
426-
"organizations:workflow-engine-metric-alert-dual-processing-logs",
427-
self.alert_rule.organization,
421+
"organizations:workflow-engine-metric-alert-processing",
422+
self.subscription.project.organization,
428423
):
429-
logger.info(
430-
"dual processing results for alert rule",
431-
extra={
432-
"results": results,
433-
"num_results": len(results),
434-
"value": aggregation_value,
435-
"rule_id": self.alert_rule.id,
436-
},
424+
packet = MetricDetectorUpdate(
425+
entity=subscription_update.get("entity", ""),
426+
subscription_id=subscription_update["subscription_id"],
427+
values={"value": aggregation_value},
428+
timestamp=self.last_update,
429+
)
430+
data_packet = DataPacket[MetricDetectorUpdate](
431+
source_id=str(self.subscription.id), packet=packet
437432
)
433+
results = process_data_packets([data_packet], DATA_SOURCE_SNUBA_QUERY_SUBSCRIPTION)
434+
if features.has(
435+
"organizations:workflow-engine-metric-alert-dual-processing-logs",
436+
self.alert_rule.organization,
437+
):
438+
logger.info(
439+
"dual processing results for alert rule",
440+
extra={
441+
"results": results,
442+
"num_results": len(results),
443+
"value": aggregation_value,
444+
"rule_id": self.alert_rule.id,
445+
},
446+
)
438447

439448
has_anomaly_detection = features.has(
440449
"organizations:anomaly-detection-alerts", self.subscription.project.organization

tests/sentry/incidents/test_subscription_processor.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3855,6 +3855,27 @@ def test_process_data_packets_called(self, mock_process_data_packets):
38553855
assert data_packet_list[0].source_id == str(self.sub.id)
38563856
assert data_packet_list[0].packet["values"] == {"value": 10}
38573857

3858+
@with_feature("organizations:workflow-engine-metric-alert-processing")
3859+
@mock.patch("sentry.incidents.subscription_processor.process_data_packets")
3860+
@mock.patch(
3861+
"sentry.incidents.subscription_processor.SubscriptionProcessor.get_comparison_aggregation_value"
3862+
)
3863+
def test_process_data_packets_not_called(
3864+
self, mock_get_comparison_aggregation_value, mock_process_data_packets
3865+
):
3866+
rule = self.comparison_rule_above
3867+
trigger = self.trigger
3868+
3869+
detector = self.create_detector(name="hojicha", type=MetricIssue.slug)
3870+
data_source = self.create_data_source(source_id=str(self.sub.id))
3871+
data_source.detectors.set([detector])
3872+
3873+
mock_get_comparison_aggregation_value.return_value = None
3874+
self.send_update(
3875+
rule, trigger.alert_threshold + 1, timedelta(minutes=-10), subscription=self.sub
3876+
)
3877+
assert mock_process_data_packets.call_count == 0
3878+
38583879

38593880
class MetricsCrashRateAlertProcessUpdateTest(ProcessUpdateBaseClass, BaseMetricsTestCase):
38603881
@pytest.fixture(autouse=True)

0 commit comments

Comments
 (0)