From bcc98da2ea02f6c260c0bb40664356ace64e97e6 Mon Sep 17 00:00:00 2001 From: Anirudh Pillai Date: Fri, 22 Nov 2024 20:55:18 +0000 Subject: [PATCH] fix(alerts): pass correct filters for aggregate trends (#26357) --- .../tasks/alerts/test/test_alert_checks.py | 6 +- .../test/test_trends_absolute_alerts.py | 112 ++++++++++++++++++ posthog/tasks/alerts/trends.py | 23 ++-- 3 files changed, 129 insertions(+), 12 deletions(-) diff --git a/posthog/tasks/alerts/test/test_alert_checks.py b/posthog/tasks/alerts/test/test_alert_checks.py index 617113b79e4..02faef2dc9b 100644 --- a/posthog/tasks/alerts/test/test_alert_checks.py +++ b/posthog/tasks/alerts/test/test_alert_checks.py @@ -100,7 +100,7 @@ class TestAlertChecks(APIBaseTest, ClickhouseDestroyTablesMixin): anomalies_descriptions = self.get_breach_description(mock_send_notifications_for_breaches, call_index=0) assert len(anomalies_descriptions) == 1 assert ( - "The insight value ($pageview) for previous day (1) is more than upper threshold (0.0)" + "The insight value ($pageview) for previous interval (1) is more than upper threshold (0.0)" in anomalies_descriptions[0] ) @@ -130,7 +130,7 @@ class TestAlertChecks(APIBaseTest, ClickhouseDestroyTablesMixin): assert mock_send_notifications_for_breaches.call_count == 1 anomalies = self.get_breach_description(mock_send_notifications_for_breaches, call_index=0) - assert "The insight value ($pageview) for previous day (0) is less than lower threshold (1.0)" in anomalies + assert "The insight value ($pageview) for previous interval (0) is less than lower threshold (1.0)" in anomalies def test_alert_triggers_but_does_not_send_notification_during_firing( self, mock_send_notifications_for_breaches: MagicMock, mock_send_errors: MagicMock @@ -318,7 +318,7 @@ class TestAlertChecks(APIBaseTest, ClickhouseDestroyTablesMixin): assert mock_send_notifications_for_breaches.call_count == 1 anomalies = self.get_breach_description(mock_send_notifications_for_breaches, call_index=0) - assert "The insight value ($pageview) for previous day (0) is less than lower threshold (1.0)" in anomalies + assert "The insight value ($pageview) for previous interval (0) is less than lower threshold (1.0)" in anomalies @patch("posthog.tasks.alerts.utils.EmailMessage") def test_send_emails( diff --git a/posthog/tasks/alerts/test/test_trends_absolute_alerts.py b/posthog/tasks/alerts/test/test_trends_absolute_alerts.py index 22ff95bb681..af4376fbe13 100644 --- a/posthog/tasks/alerts/test/test_trends_absolute_alerts.py +++ b/posthog/tasks/alerts/test/test_trends_absolute_alerts.py @@ -88,6 +88,34 @@ class TestTimeSeriesTrendsAbsoluteAlerts(APIBaseTest, ClickhouseDestroyTablesMix return insight + def create_aggregate_trend_insight(self, breakdown: Optional[BreakdownFilter] = None) -> dict[str, Any]: + query_dict = TrendsQuery( + series=[ + EventsNode( + event="signed_up", + math=BaseMathType.TOTAL, + ), + EventsNode( + event="$pageview", + name="Pageview", + math=BaseMathType.TOTAL, + ), + ], + breakdownFilter=breakdown, + trendsFilter=TrendsFilter(display=ChartDisplayType.ACTIONS_PIE), + interval=IntervalType.WEEK, + dateRange=InsightDateRange(date_from="-8w"), + ).model_dump() + + insight = self.dashboard_api.create_insight( + data={ + "name": "insight", + "query": query_dict, + } + )[1] + + return insight + def test_alert_lower_threshold_breached(self, mock_send_breaches: MagicMock, mock_send_errors: MagicMock) -> None: insight = self.create_time_series_trend_insight() alert = self.create_alert(insight, series_index=0, lower=1) @@ -295,3 +323,87 @@ class TestTimeSeriesTrendsAbsoluteAlerts(APIBaseTest, ClickhouseDestroyTablesMix assert alert_check.error is None mock_send_breaches.assert_not_called() + + def test_aggregate_trend_high_threshold_breached( + self, mock_send_breaches: MagicMock, mock_send_errors: MagicMock + ) -> None: + insight = self.create_aggregate_trend_insight() + alert = self.create_alert(insight, series_index=0, upper=1) + + with freeze_time(FROZEN_TIME - dateutil.relativedelta.relativedelta(weeks=4)): + _create_event( + team=self.team, + event="signed_up", + distinct_id="3", + properties={"$browser": "Firefox"}, + ) + _create_event( + team=self.team, + event="signed_up", + distinct_id="1", + properties={"$browser": "Chrome"}, + ) + _create_event( + team=self.team, + event="signed_up", + distinct_id="2", + properties={"$browser": "Chrome"}, + ) + flush_persons_and_events() + + check_alert(alert["id"]) + + updated_alert = AlertConfiguration.objects.get(pk=alert["id"]) + assert updated_alert.state == AlertState.FIRING + assert updated_alert.next_check_at == FROZEN_TIME + dateutil.relativedelta.relativedelta(days=1) + + alert_check = AlertCheck.objects.filter(alert_configuration=alert["id"]).latest("created_at") + assert alert_check.calculated_value == 3 + assert alert_check.state == AlertState.FIRING + assert alert_check.error is None + + mock_send_breaches.assert_called_once_with( + ANY, ["The insight value (signed_up) for previous interval (3) is more than upper threshold (1.0)"] + ) + + def test_aggregate_trend_with_breakdown_high_threshold_breached( + self, mock_send_breaches: MagicMock, mock_send_errors: MagicMock + ) -> None: + insight = self.create_aggregate_trend_insight(BreakdownFilter(breakdowns=[Breakdown(property="$browser")])) + alert = self.create_alert(insight, series_index=0, upper=1) + + with freeze_time(FROZEN_TIME - dateutil.relativedelta.relativedelta(weeks=4)): + _create_event( + team=self.team, + event="signed_up", + distinct_id="3", + properties={"$browser": "Firefox"}, + ) + _create_event( + team=self.team, + event="signed_up", + distinct_id="1", + properties={"$browser": "Chrome"}, + ) + _create_event( + team=self.team, + event="signed_up", + distinct_id="2", + properties={"$browser": "Chrome"}, + ) + flush_persons_and_events() + + check_alert(alert["id"]) + + updated_alert = AlertConfiguration.objects.get(pk=alert["id"]) + assert updated_alert.state == AlertState.FIRING + assert updated_alert.next_check_at == FROZEN_TIME + dateutil.relativedelta.relativedelta(days=1) + + alert_check = AlertCheck.objects.filter(alert_configuration=alert["id"]).latest("created_at") + assert alert_check.calculated_value == 2 + assert alert_check.state == AlertState.FIRING + assert alert_check.error is None + + mock_send_breaches.assert_called_once_with( + ANY, ["The insight value (signed_up - Chrome) for previous interval (2) is more than upper threshold (1.0)"] + ) diff --git a/posthog/tasks/alerts/trends.py b/posthog/tasks/alerts/trends.py index 7a87c4d54a4..0a744b4ebdf 100644 --- a/posthog/tasks/alerts/trends.py +++ b/posthog/tasks/alerts/trends.py @@ -58,19 +58,21 @@ def check_trends_alert(alert: AlertConfiguration, insight: Insight, query: Trend (query.breakdownFilter.breakdown and query.breakdownFilter.breakdown_type) or query.breakdownFilter.breakdowns ) + is_non_time_series = _is_non_time_series_trend(query) + match condition.type: case AlertConditionType.ABSOLUTE_VALUE: if threshold.type != InsightThresholdType.ABSOLUTE: raise ValueError(f"Absolute threshold not configured for alert condition ABSOLUTE_VALUE") - # want value for current interval (last hour, last day, last week, last month) - # depending on the alert calculation interval - if _is_non_time_series_trend(query): - filters_override = _date_range_override_for_intervals(query, last_x_intervals=2) - else: + if is_non_time_series: # for non time series, it's an aggregated value for full interval # so we need to compute full insight filters_override = None + else: + # want values back till previous interval (last hour, last day, last week, last month) + # depending on the alert calculation interval + filters_override = _date_range_override_for_intervals(query, last_x_intervals=2) calculation_result = calculate_for_query_based_insight( insight, @@ -83,18 +85,21 @@ def check_trends_alert(alert: AlertConfiguration, insight: Insight, query: Trend if not calculation_result.result: raise RuntimeError(f"No results found for insight with alert id = {alert.id}") + interval = query.interval if not is_non_time_series else None + if has_breakdown: # for breakdowns, we need to check all values in calculation_result.result breakdown_results = calculation_result.result for breakdown_result in breakdown_results: + # pick previous interval value prev_interval_value = _pick_interval_value_from_trend_result(query, breakdown_result, -1) breaches = _validate_bounds( threshold.bounds, prev_interval_value, threshold.type, condition.type, - query.interval, + interval, breakdown_result["label"], ) @@ -114,13 +119,13 @@ def check_trends_alert(alert: AlertConfiguration, insight: Insight, query: Trend prev_interval_value, threshold.type, condition.type, - query.interval, + interval, selected_series_result["label"], ) return AlertEvaluationResult(value=prev_interval_value, breaches=breaches) case AlertConditionType.RELATIVE_INCREASE: - if _is_non_time_series_trend(query): + if is_non_time_series: raise ValueError(f"Relative alerts not supported for non time series trends") # to measure relative increase, we can't alert until current interval has completed @@ -188,7 +193,7 @@ def check_trends_alert(alert: AlertConfiguration, insight: Insight, query: Trend return AlertEvaluationResult(value=(increase if not has_breakdown else None), breaches=breaches) case AlertConditionType.RELATIVE_DECREASE: - if _is_non_time_series_trend(query): + if is_non_time_series: raise ValueError(f"Relative alerts not supported for non time series trends") # to measure relative decrease, we can't alert until current interval has completed