From eb4cb4992ce030a83e77a853cf34a26f404cb890 Mon Sep 17 00:00:00 2001 From: Nikita Vorobev Date: Thu, 6 Jun 2024 18:29:46 +0100 Subject: [PATCH 01/20] init --- .../src/scenes/insights/InsightPageHeader.tsx | 6 ++--- .../src/scenes/insights/insightSceneLogic.tsx | 26 +++++++------------ frontend/src/scenes/scenes.ts | 4 +-- 3 files changed, 14 insertions(+), 22 deletions(-) diff --git a/frontend/src/scenes/insights/InsightPageHeader.tsx b/frontend/src/scenes/insights/InsightPageHeader.tsx index f7bee0b63a58e..4cd44031ad169 100644 --- a/frontend/src/scenes/insights/InsightPageHeader.tsx +++ b/frontend/src/scenes/insights/InsightPageHeader.tsx @@ -33,7 +33,7 @@ import { ExporterFormat, InsightLogicProps, InsightModel, InsightShortId, ItemMo export function InsightPageHeader({ insightLogicProps }: { insightLogicProps: InsightLogicProps }): JSX.Element { // insightSceneLogic - const { insightMode, subscriptionId } = useValues(insightSceneLogic) + const { insightMode, itemId } = useValues(insightSceneLogic) const { setInsightMode } = useActions(insightSceneLogic) // insightLogic @@ -65,7 +65,7 @@ export function InsightPageHeader({ insightLogicProps }: { insightLogicProps: In isOpen={insightMode === ItemMode.Subscriptions} closeModal={() => push(urls.insightView(insight.short_id as InsightShortId))} insightShortId={insight.short_id} - subscriptionId={subscriptionId} + subscriptionId={itemId} /> push(urls.insightView(insight.short_id as InsightShortId))} insightShortId={insight.short_id as InsightShortId} - alertId={subscriptionId} + alertId={itemId} /> diff --git a/frontend/src/scenes/insights/insightSceneLogic.tsx b/frontend/src/scenes/insights/insightSceneLogic.tsx index 31be3b9c1df5a..af7bd39b69e13 100644 --- a/frontend/src/scenes/insights/insightSceneLogic.tsx +++ b/frontend/src/scenes/insights/insightSceneLogic.tsx @@ -32,10 +32,10 @@ export const insightSceneLogic = kea([ actions({ setInsightId: (insightId: InsightShortId) => ({ insightId }), setInsightMode: (insightMode: ItemMode, source: InsightEventSource | null) => ({ insightMode, source }), - setSceneState: (insightId: InsightShortId, insightMode: ItemMode, subscriptionId: string | undefined) => ({ + setSceneState: (insightId: InsightShortId, insightMode: ItemMode, itemId: string | undefined) => ({ insightId, insightMode, - subscriptionId, + itemId, }), setInsightLogicRef: (logic: BuiltLogic | null, unmount: null | (() => void)) => ({ logic, @@ -59,15 +59,11 @@ export const insightSceneLogic = kea([ setSceneState: (_, { insightMode }) => insightMode, }, ], - subscriptionId: [ + itemId: [ null as null | number | 'new', { - setSceneState: (_, { subscriptionId }) => - subscriptionId !== undefined - ? subscriptionId === 'new' - ? 'new' - : parseInt(subscriptionId, 10) - : null, + setSceneState: (_, { itemId }) => + itemId !== undefined ? (itemId === 'new' ? 'new' : parseInt(itemId, 10)) : null, }, ], insightLogicRef: [ @@ -174,8 +170,8 @@ export const insightSceneLogic = kea([ setSceneState: sharedListeners.reloadInsightLogic, })), urlToAction(({ actions, values }) => ({ - '/insights/:shortId(/:mode)(/:subscriptionId)': ( - { shortId, mode, subscriptionId }, // url params + '/insights/:shortId(/:mode)(/:itemId)': ( + { shortId, mode, itemId }, // url params { dashboard, ...searchParams }, // search params { filters: _filters, q }, // hash params { method, initial }, // "location changed" event payload @@ -209,12 +205,8 @@ export const insightSceneLogic = kea([ return } - if ( - insightId !== values.insightId || - insightMode !== values.insightMode || - subscriptionId !== values.subscriptionId - ) { - actions.setSceneState(insightId, insightMode, subscriptionId) + if (insightId !== values.insightId || insightMode !== values.insightMode || itemId !== values.itemId) { + actions.setSceneState(insightId, insightMode, itemId) } // capture any filters from the URL, either #filters={} or ?insight=X&bla=foo&bar=baz diff --git a/frontend/src/scenes/scenes.ts b/frontend/src/scenes/scenes.ts index 94983524158f6..f6b0ca72ec587 100644 --- a/frontend/src/scenes/scenes.ts +++ b/frontend/src/scenes/scenes.ts @@ -494,8 +494,8 @@ export const routes: Record = { [urls.insightEdit(':shortId' as InsightShortId)]: Scene.Insight, [urls.insightView(':shortId' as InsightShortId)]: Scene.Insight, [urls.insightSubcriptions(':shortId' as InsightShortId)]: Scene.Insight, - [urls.insightSubcription(':shortId' as InsightShortId, ':subscriptionId')]: Scene.Insight, - [urls.alert(':shortId' as InsightShortId, ':subscriptionId')]: Scene.Insight, + [urls.insightSubcription(':shortId' as InsightShortId, ':itemId')]: Scene.Insight, + [urls.alert(':shortId' as InsightShortId, ':itemId')]: Scene.Insight, [urls.alerts(':shortId' as InsightShortId)]: Scene.Insight, [urls.insightSharing(':shortId' as InsightShortId)]: Scene.Insight, [urls.savedInsights()]: Scene.SavedInsights, From ba51409809bc45f186f3e816a66692d4dd583681 Mon Sep 17 00:00:00 2001 From: Nikita Vorobev Date: Sat, 8 Jun 2024 16:46:05 +0100 Subject: [PATCH 02/20] initial version of the regular job --- .../test/subscriptions/test_subscriptions.py | 2 - .../lib/components/Alerts/views/EditAlert.tsx | 12 +- .../insights/InsightNav/insightNavLogic.tsx | 2 +- posthog/models/__init__.py | 4 +- posthog/models/alert.py | 17 +- posthog/tasks/__init__.py | 2 + posthog/tasks/detect_alerts_anomalies.py | 73 +++++++++ posthog/tasks/scheduled.py | 7 + posthog/tasks/tasks.py | 7 + .../test/test_detect_alerts_anomalies.py | 150 ++++++++++++++++++ posthog/templates/email/alert_anomaly.html | 10 ++ 11 files changed, 279 insertions(+), 7 deletions(-) create mode 100644 posthog/tasks/detect_alerts_anomalies.py create mode 100644 posthog/tasks/test/test_detect_alerts_anomalies.py create mode 100644 posthog/templates/email/alert_anomaly.html diff --git a/ee/tasks/test/subscriptions/test_subscriptions.py b/ee/tasks/test/subscriptions/test_subscriptions.py index 717ab7b42d592..5f6db8011b231 100644 --- a/ee/tasks/test/subscriptions/test_subscriptions.py +++ b/ee/tasks/test/subscriptions/test_subscriptions.py @@ -15,7 +15,6 @@ from posthog.models.exported_asset import ExportedAsset from posthog.models.insight import Insight from posthog.models.instance_setting import set_instance_setting -from posthog.models.subscription import Subscription from posthog.test.base import APIBaseTest @@ -24,7 +23,6 @@ @patch("ee.tasks.subscriptions.generate_assets") @freeze_time("2022-02-02T08:55:00.000Z") class TestSubscriptionsTasks(APIBaseTest): - subscriptions: list[Subscription] = None # type: ignore dashboard: Dashboard insight: Insight tiles: list[DashboardTile] = None # type: ignore diff --git a/frontend/src/lib/components/Alerts/views/EditAlert.tsx b/frontend/src/lib/components/Alerts/views/EditAlert.tsx index 6b60fcb84b778..8c2e42a8d5893 100644 --- a/frontend/src/lib/components/Alerts/views/EditAlert.tsx +++ b/frontend/src/lib/components/Alerts/views/EditAlert.tsx @@ -70,10 +70,18 @@ export function EditAlert({ id, insightShortId, onCancel, onDelete }: EditAlertP - + - + diff --git a/frontend/src/scenes/insights/InsightNav/insightNavLogic.tsx b/frontend/src/scenes/insights/InsightNav/insightNavLogic.tsx index b17a01d7c4d83..e4421f5778168 100644 --- a/frontend/src/scenes/insights/InsightNav/insightNavLogic.tsx +++ b/frontend/src/scenes/insights/InsightNav/insightNavLogic.tsx @@ -288,7 +288,7 @@ export const insightNavLogic = kea([ }, })), urlToAction(({ actions }) => ({ - '/insights/:shortId(/:mode)(/:subscriptionId)': ( + '/insights/:shortId(/:mode)(/:itemId)': ( _, // url params { dashboard, ...searchParams }, // search params { filters: _filters } // hash params diff --git a/posthog/models/__init__.py b/posthog/models/__init__.py index d0f9dcf893c80..b4d0df1bda701 100644 --- a/posthog/models/__init__.py +++ b/posthog/models/__init__.py @@ -13,7 +13,7 @@ ) from ..warehouse.models import DataWarehouseTable from ._deprecated_prompts import Prompt, PromptSequence, UserPromptState -from .alert import Alert +from .alert import Alert, AbsoluteThreshold, AnomalyCondition from .action import Action from .action.action_step import ActionStep from .activity_logging.activity_log import ActivityLog @@ -72,11 +72,13 @@ from .user_scene_personalisation import UserScenePersonalisation __all__ = [ + "AbsoluteThreshold", "Alert", "Action", "ActionStep", "ActivityLog", "Annotation", + "AnomalyCondition", "AsyncDeletion", "AsyncMigration", "AsyncMigrationError", diff --git a/posthog/models/alert.py b/posthog/models/alert.py index 5b8d9616bb06c..3ef6ee448499a 100644 --- a/posthog/models/alert.py +++ b/posthog/models/alert.py @@ -1,9 +1,24 @@ from django.db import models +from typing import Optional +from dataclasses import dataclass + + +@dataclass +class AbsoluteThreshold: + lower: Optional[float] = None + upper: Optional[float] = None + + +class AnomalyCondition: + absolute_threshold: AbsoluteThreshold + + def __init__(self, absoluteThreshold: dict): + self.absolute_threshold = AbsoluteThreshold(**absoluteThreshold) class Alert(models.Model): team: models.ForeignKey = models.ForeignKey("Team", on_delete=models.CASCADE) - insight = models.ForeignKey("posthog.Insight", on_delete=models.CASCADE) + insight: models.ForeignKey = models.ForeignKey("posthog.Insight", on_delete=models.CASCADE) name: models.CharField = models.CharField(max_length=100) target_value: models.TextField = models.TextField() diff --git a/posthog/tasks/__init__.py b/posthog/tasks/__init__.py index 5298d6bd84409..545846c8a844c 100644 --- a/posthog/tasks/__init__.py +++ b/posthog/tasks/__init__.py @@ -6,6 +6,7 @@ check_clickhouse_schema_drift, demo_create_data, demo_reset_master_team, + detect_alerts_anomalies, email, exporter, process_scheduled_changes, @@ -24,6 +25,7 @@ "check_clickhouse_schema_drift", "demo_create_data", "demo_reset_master_team", + "detect_alerts_anomalies", "email", "exporter", "process_scheduled_changes", diff --git a/posthog/tasks/detect_alerts_anomalies.py b/posthog/tasks/detect_alerts_anomalies.py new file mode 100644 index 0000000000000..f357a41ac0d97 --- /dev/null +++ b/posthog/tasks/detect_alerts_anomalies.py @@ -0,0 +1,73 @@ +import structlog +from celery import shared_task +from typing import cast +from posthog.schema import HogQLQueryResponse +from posthog.models import Alert, AnomalyCondition +from posthog.hogql_queries.query_runner import get_query_runner +from posthog.email import EmailMessage +from django.utils import timezone +from posthog.hogql_queries.legacy_compatibility.filter_to_query import filter_to_query + +logger = structlog.get_logger(__name__) + + +def check_all_alerts() -> None: + alerts = Alert.objects.all().only("id") + for alert in alerts: + logger.info("scheduling alert", alert_id=alert.id) + check_alert.delay(alert.id) + + +@shared_task(ignore_result=True) +def check_alert(id: int) -> None: + alert = Alert.objects.get(pk=id) + insight = alert.insight + if not insight.query: + insight.query = filter_to_query(insight.filters) + query_runner = get_query_runner(insight.query, alert.team) + response = cast(HogQLQueryResponse, query_runner.calculate()) + if not response.results: + raise RuntimeError(f"no results for alert {alert.id}") + + anomaly_condition = AnomalyCondition(**alert.anomaly_condition) + thresholds = anomaly_condition.absolute_threshold + + result = response.results[0] + aggregated_value = result["aggregated_value"] + anomalies_descriptions = [] + + if thresholds.lower is not None and aggregated_value < thresholds.lower: + anomalies_descriptions += [ + f"The trend value ({aggregated_value}) is below the lower threshold ({thresholds.lower})" + ] + if thresholds.upper is not None and aggregated_value > thresholds.upper: + anomalies_descriptions += [ + f"The trend value ({aggregated_value}) is above the upper threshold ({thresholds.upper})" + ] + + if not anomalies_descriptions: + logger.info("no anomalies", alert_id=alert.id) + return + + subject = f"PostHog alert {alert.name}" + campaign_key = f"alert-anomaly-notification-{alert.id}-{timezone.now().timestamp()}" + insight_url = f"/project/{alert.team.pk}/insights/{alert.insight.short_id}" + alert_url = f"{insight_url}/alerts/{alert.id}" + message = EmailMessage( + campaign_key=campaign_key, + subject=subject, + template_name="alert_anomaly", + template_context={ + "anomalies_descriptions": anomalies_descriptions, + "insight_url": insight_url, + "insight_name": alert.insight.name, + "alert_url": alert_url, + "alert_name": alert.name, + }, + ) + targets = list(filter(len, alert.target_value.split(","))) + if not targets: + raise RuntimeError(f"no targets configured for alert {alert.id}") + for target in targets: + message.add_recipient(email=target) + message.send() diff --git a/posthog/tasks/scheduled.py b/posthog/tasks/scheduled.py index 87a0a0a1725c1..7264b02e505f9 100644 --- a/posthog/tasks/scheduled.py +++ b/posthog/tasks/scheduled.py @@ -25,6 +25,7 @@ clickhouse_row_count, clickhouse_send_license_usage, delete_expired_exported_assets, + detect_alerts_anomalies, ee_persist_finished_recordings, find_flags_with_enriched_analytics, graphile_worker_queue_size, @@ -239,6 +240,12 @@ def setup_periodic_tasks(sender: Celery, **kwargs: Any) -> None: name="stop surveys that reached responses limits", ) + sender.add_periodic_task( + crontab(hour="*"), + detect_alerts_anomalies.s(), + name="detect alerts' anomalies and notify about them", + ) + if settings.EE_AVAILABLE: # every interval seconds, we calculate N replay embeddings # the goal is to process _enough_ every 24 hours that diff --git a/posthog/tasks/tasks.py b/posthog/tasks/tasks.py index 35023a0387a40..5de22a80b17b4 100644 --- a/posthog/tasks/tasks.py +++ b/posthog/tasks/tasks.py @@ -714,6 +714,13 @@ def stop_surveys_reached_target() -> None: stop_surveys_reached_target() +@shared_task(ignore_result=True) +def detect_alerts_anomalies() -> None: + from posthog.tasks.detect_alerts_anomalies import check_all_alerts + + check_all_alerts() + + def recompute_materialized_columns_enabled() -> bool: from posthog.models.instance_setting import get_instance_setting diff --git a/posthog/tasks/test/test_detect_alerts_anomalies.py b/posthog/tasks/test/test_detect_alerts_anomalies.py new file mode 100644 index 0000000000000..00fbc6023962c --- /dev/null +++ b/posthog/tasks/test/test_detect_alerts_anomalies.py @@ -0,0 +1,150 @@ +from unittest.mock import MagicMock, patch +import pytest +from typing import Optional +from freezegun import freeze_time +from posthog.models.instance_setting import set_instance_setting +from posthog.test.base import APIBaseTest, _create_event, flush_persons_and_events, ClickhouseDestroyTablesMixin +from posthog.api.test.dashboards import DashboardAPI +from posthog.schema import ChartDisplayType, EventsNode, TrendsQuery, TrendsFilter +from posthog.tasks.test.utils_email_tests import mock_email_messages +from posthog.tasks.detect_alerts_anomalies import check_all_alerts + + +@freeze_time("2024-06-02T08:55:00.000Z") +@patch("posthog.tasks.detect_alerts_anomalies.EmailMessage") +class TestDetectAlertsAnomaliesTasks(APIBaseTest, ClickhouseDestroyTablesMixin): + def setUp(self) -> None: + super().setUp() + set_instance_setting("EMAIL_HOST", "fake_host") + set_instance_setting("EMAIL_ENABLED", True) + self.settings(CELERY_TASK_ALWAYS_EAGER=True) + self.dashboard_api = DashboardAPI(self.client, self.team, self.assertEqual) + query_dict = TrendsQuery( + series=[ + EventsNode( + event="$pageview", + ), + ], + trendsFilter=TrendsFilter(display=ChartDisplayType.BoldNumber), + ).model_dump() + self.insight = self.dashboard_api.create_insight( + data={ + "name": "insight", + "query": query_dict, + } + )[1] + + self.alert = self.client.post( + f"/api/projects/{self.team.id}/alerts", + data={ + "name": "alert name", + "insight": self.insight["id"], + "target_value": "a@b.c,d@e.f", + "anomaly_condition": {"absoluteThreshold": {}}, + }, + ).json() + + def set_thresholds(self, lower: Optional[int] = None, upper: Optional[int] = None): + self.client.patch( + f"/api/projects/{self.team.id}/alerts/{self.alert['id']}", + data={"anomaly_condition": {"absoluteThreshold": {"lower": lower, "upper": upper}}}, + ) + + def test_alert_is_triggered_for_values_above_higher_threshold(self, MockEmailMessage: MagicMock): + mocked_email_messages = mock_email_messages(MockEmailMessage) + self.set_thresholds(upper=0) + + with freeze_time("2024-06-02T07:55:00.000Z"): + _create_event( + team=self.team, + event="$pageview", + distinct_id="1", + ) + flush_persons_and_events() + + check_all_alerts() + + assert len(mocked_email_messages) == 1 + assert mocked_email_messages[0].to == [ + {"recipient": "a@b.c", "raw_email": "a@b.c"}, + {"recipient": "d@e.f", "raw_email": "d@e.f"}, + ] + assert "The trend value (1) is above the upper threshold (0)" in mocked_email_messages[0].html_body + + def test_alert_is_triggered_for_value_below_lower_threshold(self, MockEmailMessage: MagicMock): + mocked_email_messages = mock_email_messages(MockEmailMessage) + self.set_thresholds(lower=1) + + check_all_alerts() + + assert len(mocked_email_messages) == 1 + assert "The trend value (0) is below the lower threshold (1)" in mocked_email_messages[0].html_body + + def test_alert_is_not_triggered_for_normal_values(self, MockEmailMessage: MagicMock): + mocked_email_messages = mock_email_messages(MockEmailMessage) + self.set_thresholds(lower=0, upper=1) + + check_all_alerts() + + assert len(mocked_email_messages) == 0 + + def test_error_while_calculating_no_alert(self, MockEmailMessage: MagicMock): + mocked_email_messages = mock_email_messages(MockEmailMessage) + query_dict = TrendsQuery( + series=[ + EventsNode( + event="$pageview", + ), + ], + ).model_dump() + insight = self.dashboard_api.create_insight( + data={ + "name": "insight", + "query": query_dict, + } + )[1] + + self.client.patch(f"/api/projects/{self.team.id}/alerts/{self.alert['id']}", data={"insight": insight["id"]}) + + # in production one alert failure won't cause an exception in check_all_alerts + # because execution won't be eager (see CELERY_TASK_ALWAYS_EAGER in the set up) + with pytest.raises(KeyError): + check_all_alerts() + + assert len(mocked_email_messages) == 0 + + def test_two_alerts_are_triggered(self, MockEmailMessage: MagicMock): + mocked_email_messages = mock_email_messages(MockEmailMessage) + self.set_thresholds(lower=1) + self.client.post( + f"/api/projects/{self.team.id}/alerts", + data={ + "name": "another alert name", + "insight": self.insight["id"], + "target_value": "email@address.com", + "anomaly_condition": {"absoluteThreshold": {"lower": 1}}, + }, + ).json() + + check_all_alerts() + + assert len(mocked_email_messages) == 2 + assert "The trend value (0) is below the lower threshold (1)" in mocked_email_messages[0].html_body + assert "The trend value (0) is below the lower threshold (1)" in mocked_email_messages[1].html_body + recipients = [[to["recipient"] for to in message.to] for message in mocked_email_messages] + recipients = sorted(recipients) + assert recipients == [["a@b.c", "d@e.f"], ["email@address.com"]] + + def test_alert_with_insight_with_filter(self, MockEmailMessage: MagicMock): + mocked_email_messages = mock_email_messages(MockEmailMessage) + insight = self.dashboard_api.create_insight( + data={"name": "insight", "filters": {"events": [{"id": "$pageview"}], "display": "BoldNumber"}} + )[1] + + self.client.patch(f"/api/projects/{self.team.id}/alerts/{self.alert['id']}", data={"insight": insight["id"]}) + self.set_thresholds(lower=1) + + check_all_alerts() + + assert len(mocked_email_messages) == 1 + assert "The trend value (0) is below the lower threshold (1)" in mocked_email_messages[0].html_body diff --git a/posthog/templates/email/alert_anomaly.html b/posthog/templates/email/alert_anomaly.html new file mode 100644 index 0000000000000..90ab186edba67 --- /dev/null +++ b/posthog/templates/email/alert_anomaly.html @@ -0,0 +1,10 @@ +{% extends "email/base.html" %} {% load posthog_assets %} {% block section %} +

+ The {{ alert_name }} alert detected following anomalies for {{ insight_name }}: +

    + {% for anomaly_description in anomalies_descriptions %} +
  • {{ anomaly_description }}
    • + {% endfor %} +
+

+{% endblock %}{% load posthog_filters %} \ No newline at end of file From bb71f5aac855aa3d573b8348ab5829d514534f85 Mon Sep 17 00:00:00 2001 From: Nikita Vorobev Date: Sat, 8 Jun 2024 17:17:22 +0100 Subject: [PATCH 03/20] small polishing --- posthog/tasks/detect_alerts_anomalies.py | 18 +++++++++++------- .../tasks/test/test_detect_alerts_anomalies.py | 17 +++++++++-------- posthog/templates/email/alert_anomaly.html | 2 +- 3 files changed, 21 insertions(+), 16 deletions(-) diff --git a/posthog/tasks/detect_alerts_anomalies.py b/posthog/tasks/detect_alerts_anomalies.py index f357a41ac0d97..407759159ccd3 100644 --- a/posthog/tasks/detect_alerts_anomalies.py +++ b/posthog/tasks/detect_alerts_anomalies.py @@ -1,12 +1,14 @@ +from typing import cast + import structlog from celery import shared_task -from typing import cast -from posthog.schema import HogQLQueryResponse -from posthog.models import Alert, AnomalyCondition -from posthog.hogql_queries.query_runner import get_query_runner -from posthog.email import EmailMessage from django.utils import timezone + +from posthog.email import EmailMessage from posthog.hogql_queries.legacy_compatibility.filter_to_query import filter_to_query +from posthog.hogql_queries.query_runner import get_query_runner +from posthog.models import Alert, AnomalyCondition +from posthog.schema import HogQLQueryResponse logger = structlog.get_logger(__name__) @@ -49,7 +51,7 @@ def check_alert(id: int) -> None: logger.info("no anomalies", alert_id=alert.id) return - subject = f"PostHog alert {alert.name}" + subject = f"PostHog alert {alert.name} has anomalies" campaign_key = f"alert-anomaly-notification-{alert.id}-{timezone.now().timestamp()}" insight_url = f"/project/{alert.team.pk}/insights/{alert.insight.short_id}" alert_url = f"{insight_url}/alerts/{alert.id}" @@ -67,7 +69,9 @@ def check_alert(id: int) -> None: ) targets = list(filter(len, alert.target_value.split(","))) if not targets: - raise RuntimeError(f"no targets configured for alert {alert.id}") + raise RuntimeError(f"no targets configured for the alert {alert.id}") for target in targets: message.add_recipient(email=target) + + logger.info(f"Send notifications about {len(anomalies_descriptions)} anomalies", alert_id=alert.id) message.send() diff --git a/posthog/tasks/test/test_detect_alerts_anomalies.py b/posthog/tasks/test/test_detect_alerts_anomalies.py index 00fbc6023962c..21551fa4e8b93 100644 --- a/posthog/tasks/test/test_detect_alerts_anomalies.py +++ b/posthog/tasks/test/test_detect_alerts_anomalies.py @@ -1,7 +1,9 @@ -from unittest.mock import MagicMock, patch import pytest from typing import Optional +from unittest.mock import MagicMock, patch + from freezegun import freeze_time + from posthog.models.instance_setting import set_instance_setting from posthog.test.base import APIBaseTest, _create_event, flush_persons_and_events, ClickhouseDestroyTablesMixin from posthog.api.test.dashboards import DashboardAPI @@ -50,6 +52,10 @@ def set_thresholds(self, lower: Optional[int] = None, upper: Optional[int] = Non data={"anomaly_condition": {"absoluteThreshold": {"lower": lower, "upper": upper}}}, ) + def get_recepients(self, mocked_email_messages) -> list[list[str]]: + recipients = [sorted([to["recipient"] for to in message.to]) for message in mocked_email_messages] + return sorted(recipients) + def test_alert_is_triggered_for_values_above_higher_threshold(self, MockEmailMessage: MagicMock): mocked_email_messages = mock_email_messages(MockEmailMessage) self.set_thresholds(upper=0) @@ -65,10 +71,7 @@ def test_alert_is_triggered_for_values_above_higher_threshold(self, MockEmailMes check_all_alerts() assert len(mocked_email_messages) == 1 - assert mocked_email_messages[0].to == [ - {"recipient": "a@b.c", "raw_email": "a@b.c"}, - {"recipient": "d@e.f", "raw_email": "d@e.f"}, - ] + assert self.get_recepients(mocked_email_messages) == [["a@b.c", "d@e.f"]] assert "The trend value (1) is above the upper threshold (0)" in mocked_email_messages[0].html_body def test_alert_is_triggered_for_value_below_lower_threshold(self, MockEmailMessage: MagicMock): @@ -131,9 +134,7 @@ def test_two_alerts_are_triggered(self, MockEmailMessage: MagicMock): assert len(mocked_email_messages) == 2 assert "The trend value (0) is below the lower threshold (1)" in mocked_email_messages[0].html_body assert "The trend value (0) is below the lower threshold (1)" in mocked_email_messages[1].html_body - recipients = [[to["recipient"] for to in message.to] for message in mocked_email_messages] - recipients = sorted(recipients) - assert recipients == [["a@b.c", "d@e.f"], ["email@address.com"]] + assert self.get_recepients(mocked_email_messages) == [["a@b.c", "d@e.f"], ["email@address.com"]] def test_alert_with_insight_with_filter(self, MockEmailMessage: MagicMock): mocked_email_messages = mock_email_messages(MockEmailMessage) diff --git a/posthog/templates/email/alert_anomaly.html b/posthog/templates/email/alert_anomaly.html index 90ab186edba67..c4ab2cea13ace 100644 --- a/posthog/templates/email/alert_anomaly.html +++ b/posthog/templates/email/alert_anomaly.html @@ -1,6 +1,6 @@ {% extends "email/base.html" %} {% load posthog_assets %} {% block section %}

- The {{ alert_name }} alert detected following anomalies for {{ insight_name }}: + Uh-oh, the {{ alert_name }} alert detected following anomalies for {{ insight_name }}:

    {% for anomaly_description in anomalies_descriptions %}
  • {{ anomaly_description }}
    • From 15e0b6b536a3e85f1f468a95e2ea40815743463c Mon Sep 17 00:00:00 2001 From: Nikita Vorobev Date: Sat, 8 Jun 2024 17:24:58 +0100 Subject: [PATCH 04/20] small test fix --- posthog/tasks/test/test_detect_alerts_anomalies.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/posthog/tasks/test/test_detect_alerts_anomalies.py b/posthog/tasks/test/test_detect_alerts_anomalies.py index 21551fa4e8b93..54bc6bf58e455 100644 --- a/posthog/tasks/test/test_detect_alerts_anomalies.py +++ b/posthog/tasks/test/test_detect_alerts_anomalies.py @@ -109,8 +109,8 @@ def test_error_while_calculating_no_alert(self, MockEmailMessage: MagicMock): self.client.patch(f"/api/projects/{self.team.id}/alerts/{self.alert['id']}", data={"insight": insight["id"]}) - # in production one alert failure won't cause an exception in check_all_alerts - # because execution won't be eager (see CELERY_TASK_ALWAYS_EAGER in the set up) + # in production failure of a single alert won't cause an exception in check_all_alerts + # because the execution won't be eager (see CELERY_TASK_ALWAYS_EAGER in the set up) with pytest.raises(KeyError): check_all_alerts() From 71c44d9084129f3303599faccb576c05a034a737 Mon Sep 17 00:00:00 2001 From: Nikita Vorobev Date: Sat, 8 Jun 2024 17:29:44 +0100 Subject: [PATCH 05/20] fix types in tests --- .../tasks/test/test_detect_alerts_anomalies.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/posthog/tasks/test/test_detect_alerts_anomalies.py b/posthog/tasks/test/test_detect_alerts_anomalies.py index 54bc6bf58e455..2f7e7e2b9924b 100644 --- a/posthog/tasks/test/test_detect_alerts_anomalies.py +++ b/posthog/tasks/test/test_detect_alerts_anomalies.py @@ -1,5 +1,5 @@ import pytest -from typing import Optional +from typing import Any, Optional from unittest.mock import MagicMock, patch from freezegun import freeze_time @@ -46,17 +46,17 @@ def setUp(self) -> None: }, ).json() - def set_thresholds(self, lower: Optional[int] = None, upper: Optional[int] = None): + def set_thresholds(self, lower: Optional[int] = None, upper: Optional[int] = None) -> None: self.client.patch( f"/api/projects/{self.team.id}/alerts/{self.alert['id']}", data={"anomaly_condition": {"absoluteThreshold": {"lower": lower, "upper": upper}}}, ) - def get_recepients(self, mocked_email_messages) -> list[list[str]]: + def get_recepients(self, mocked_email_messages: list[Any]) -> list[list[str]]: recipients = [sorted([to["recipient"] for to in message.to]) for message in mocked_email_messages] return sorted(recipients) - def test_alert_is_triggered_for_values_above_higher_threshold(self, MockEmailMessage: MagicMock): + def test_alert_is_triggered_for_values_above_higher_threshold(self, MockEmailMessage: MagicMock) -> None: mocked_email_messages = mock_email_messages(MockEmailMessage) self.set_thresholds(upper=0) @@ -74,7 +74,7 @@ def test_alert_is_triggered_for_values_above_higher_threshold(self, MockEmailMes assert self.get_recepients(mocked_email_messages) == [["a@b.c", "d@e.f"]] assert "The trend value (1) is above the upper threshold (0)" in mocked_email_messages[0].html_body - def test_alert_is_triggered_for_value_below_lower_threshold(self, MockEmailMessage: MagicMock): + def test_alert_is_triggered_for_value_below_lower_threshold(self, MockEmailMessage: MagicMock) -> None: mocked_email_messages = mock_email_messages(MockEmailMessage) self.set_thresholds(lower=1) @@ -83,7 +83,7 @@ def test_alert_is_triggered_for_value_below_lower_threshold(self, MockEmailMessa assert len(mocked_email_messages) == 1 assert "The trend value (0) is below the lower threshold (1)" in mocked_email_messages[0].html_body - def test_alert_is_not_triggered_for_normal_values(self, MockEmailMessage: MagicMock): + def test_alert_is_not_triggered_for_normal_values(self, MockEmailMessage: MagicMock) -> None: mocked_email_messages = mock_email_messages(MockEmailMessage) self.set_thresholds(lower=0, upper=1) @@ -91,7 +91,7 @@ def test_alert_is_not_triggered_for_normal_values(self, MockEmailMessage: MagicM assert len(mocked_email_messages) == 0 - def test_error_while_calculating_no_alert(self, MockEmailMessage: MagicMock): + def test_error_while_calculating_no_alert(self, MockEmailMessage: MagicMock) -> None: mocked_email_messages = mock_email_messages(MockEmailMessage) query_dict = TrendsQuery( series=[ @@ -116,7 +116,7 @@ def test_error_while_calculating_no_alert(self, MockEmailMessage: MagicMock): assert len(mocked_email_messages) == 0 - def test_two_alerts_are_triggered(self, MockEmailMessage: MagicMock): + def test_two_alerts_are_triggered(self, MockEmailMessage: MagicMock) -> None: mocked_email_messages = mock_email_messages(MockEmailMessage) self.set_thresholds(lower=1) self.client.post( @@ -136,7 +136,7 @@ def test_two_alerts_are_triggered(self, MockEmailMessage: MagicMock): assert "The trend value (0) is below the lower threshold (1)" in mocked_email_messages[1].html_body assert self.get_recepients(mocked_email_messages) == [["a@b.c", "d@e.f"], ["email@address.com"]] - def test_alert_with_insight_with_filter(self, MockEmailMessage: MagicMock): + def test_alert_with_insight_with_filter(self, MockEmailMessage: MagicMock) -> None: mocked_email_messages = mock_email_messages(MockEmailMessage) insight = self.dashboard_api.create_insight( data={"name": "insight", "filters": {"events": [{"id": "$pageview"}], "display": "BoldNumber"}} From 5e59a5d2dcb26d932453bed2a9e9210d42ba2321 Mon Sep 17 00:00:00 2001 From: Nikita Vorobev Date: Sat, 8 Jun 2024 17:34:52 +0100 Subject: [PATCH 06/20] fix the crontab schedule to every hour --- posthog/tasks/scheduled.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/posthog/tasks/scheduled.py b/posthog/tasks/scheduled.py index 5c4faf0ce680a..62482b40e8b6a 100644 --- a/posthog/tasks/scheduled.py +++ b/posthog/tasks/scheduled.py @@ -248,7 +248,7 @@ def setup_periodic_tasks(sender: Celery, **kwargs: Any) -> None: ) sender.add_periodic_task( - crontab(hour="*"), + crontab(hour="*", minute="30"), detect_alerts_anomalies.s(), name="detect alerts' anomalies and notify about them", ) From 29e613b9243877db4e41c5f1997547dfc9cb1a84 Mon Sep 17 00:00:00 2001 From: Nikita Vorobev Date: Sat, 8 Jun 2024 17:45:53 +0100 Subject: [PATCH 07/20] add a newline to the template --- posthog/tasks/scheduled.py | 2 +- posthog/templates/email/alert_anomaly.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/posthog/tasks/scheduled.py b/posthog/tasks/scheduled.py index 62482b40e8b6a..1c051739489c2 100644 --- a/posthog/tasks/scheduled.py +++ b/posthog/tasks/scheduled.py @@ -248,7 +248,7 @@ def setup_periodic_tasks(sender: Celery, **kwargs: Any) -> None: ) sender.add_periodic_task( - crontab(hour="*", minute="30"), + crontab(hour="*", minute="20"), detect_alerts_anomalies.s(), name="detect alerts' anomalies and notify about them", ) diff --git a/posthog/templates/email/alert_anomaly.html b/posthog/templates/email/alert_anomaly.html index c4ab2cea13ace..c50dafd157cc3 100644 --- a/posthog/templates/email/alert_anomaly.html +++ b/posthog/templates/email/alert_anomaly.html @@ -7,4 +7,4 @@ {% endfor %}

-{% endblock %}{% load posthog_filters %} \ No newline at end of file +{% endblock %}{% load posthog_filters %} From b1a0219a6252d46055cc4e62b0599f95bb611498 Mon Sep 17 00:00:00 2001 From: Nikita Vorobev Date: Tue, 11 Jun 2024 17:52:31 +0100 Subject: [PATCH 08/20] add a test to check insight date range --- .../tasks/test/test_detect_alerts_anomalies.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/posthog/tasks/test/test_detect_alerts_anomalies.py b/posthog/tasks/test/test_detect_alerts_anomalies.py index 2f7e7e2b9924b..7caa22ba34e65 100644 --- a/posthog/tasks/test/test_detect_alerts_anomalies.py +++ b/posthog/tasks/test/test_detect_alerts_anomalies.py @@ -74,6 +74,22 @@ def test_alert_is_triggered_for_values_above_higher_threshold(self, MockEmailMes assert self.get_recepients(mocked_email_messages) == [["a@b.c", "d@e.f"]] assert "The trend value (1) is above the upper threshold (0)" in mocked_email_messages[0].html_body + def test_alert_is_not_triggered_for_events_beyond_interval(self, MockEmailMessage: MagicMock) -> None: + mocked_email_messages = mock_email_messages(MockEmailMessage) + self.set_thresholds(upper=0) + + with freeze_time("2024-05-02T07:55:00.000Z"): + _create_event( + team=self.team, + event="$pageview", + distinct_id="1", + ) + flush_persons_and_events() + + check_all_alerts() + + assert len(mocked_email_messages) == 0 + def test_alert_is_triggered_for_value_below_lower_threshold(self, MockEmailMessage: MagicMock) -> None: mocked_email_messages = mock_email_messages(MockEmailMessage) self.set_thresholds(lower=1) From 29d93054554da2e70ee02265f6200ba55d5ef44a Mon Sep 17 00:00:00 2001 From: Nikita Vorobev Date: Tue, 11 Jun 2024 18:57:28 +0100 Subject: [PATCH 09/20] use the new display type naming --- posthog/tasks/test/test_detect_alerts_anomalies.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/posthog/tasks/test/test_detect_alerts_anomalies.py b/posthog/tasks/test/test_detect_alerts_anomalies.py index 7caa22ba34e65..dddf97c2e8dd7 100644 --- a/posthog/tasks/test/test_detect_alerts_anomalies.py +++ b/posthog/tasks/test/test_detect_alerts_anomalies.py @@ -27,7 +27,7 @@ def setUp(self) -> None: event="$pageview", ), ], - trendsFilter=TrendsFilter(display=ChartDisplayType.BoldNumber), + trendsFilter=TrendsFilter(display=ChartDisplayType.BOLD_NUMBER), ).model_dump() self.insight = self.dashboard_api.create_insight( data={ From b8b4fec43a9bc2daa8e8bc3c4eed2594e59fe163 Mon Sep 17 00:00:00 2001 From: Nikita Vorobev Date: Tue, 2 Jul 2024 20:53:51 +0100 Subject: [PATCH 10/20] address PR comments --- .../lib/components/Alerts/views/EditAlert.tsx | 2 +- frontend/src/queries/schema.json | 23 ++++++++++ frontend/src/queries/schema.ts | 9 ++++ frontend/src/types.ts | 1 + posthog/caching/calculate_results.py | 2 +- posthog/models/__init__.py | 4 +- posthog/models/alert.py | 15 ------- posthog/schema.py | 15 +++++++ posthog/tasks/detect_alerts_anomalies.py | 45 ++++++++++++------- .../test/test_detect_alerts_anomalies.py | 10 ++--- 10 files changed, 85 insertions(+), 41 deletions(-) diff --git a/frontend/src/lib/components/Alerts/views/EditAlert.tsx b/frontend/src/lib/components/Alerts/views/EditAlert.tsx index 8c2e42a8d5893..69d12f716623d 100644 --- a/frontend/src/lib/components/Alerts/views/EditAlert.tsx +++ b/frontend/src/lib/components/Alerts/views/EditAlert.tsx @@ -79,7 +79,7 @@ export function EditAlert({ id, insightShortId, onCancel, onDelete }: EditAlertP diff --git a/frontend/src/queries/schema.json b/frontend/src/queries/schema.json index f00512ba8906e..ef3d2392ba347 100644 --- a/frontend/src/queries/schema.json +++ b/frontend/src/queries/schema.json @@ -1,6 +1,19 @@ { "$schema": "http://json-schema.org/draft-07/schema#", "definitions": { + "AbsoluteThreshold": { + "additionalProperties": false, + "properties": { + "lower": { + "type": ["number", "null"] + }, + "upper": { + "type": ["number", "null"] + } + }, + "required": ["lower", "upper"], + "type": "object" + }, "ActionsNode": { "additionalProperties": false, "properties": { @@ -179,6 +192,16 @@ "enum": ["numeric", "duration", "duration_ms", "percentage", "percentage_scaled"], "type": "string" }, + "AnomalyCondition": { + "additionalProperties": false, + "properties": { + "absoluteThreshold": { + "$ref": "#/definitions/AbsoluteThreshold" + } + }, + "required": ["absoluteThreshold"], + "type": "object" + }, "AnyDataNode": { "anyOf": [ { diff --git a/frontend/src/queries/schema.ts b/frontend/src/queries/schema.ts index e34100a494b75..c5874d82aa8ca 100644 --- a/frontend/src/queries/schema.ts +++ b/frontend/src/queries/schema.ts @@ -1586,3 +1586,12 @@ export interface DashboardFilter { date_to?: string | null properties?: AnyPropertyFilter[] | null } + +export interface AbsoluteThreshold { + lower: number | null + upper: number | null +} + +export interface AnomalyCondition { + absoluteThreshold: AbsoluteThreshold +} diff --git a/frontend/src/types.ts b/frontend/src/types.ts index fdf011a2e8e19..ef0ca12120951 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -4273,6 +4273,7 @@ export type HogFunctionStatus = { }[] } +// TODO: move to schema.ts export interface AnomalyCondition { absoluteThreshold: { lower?: number diff --git a/posthog/caching/calculate_results.py b/posthog/caching/calculate_results.py index ee21f46cc6a3d..5f7507c85c02e 100644 --- a/posthog/caching/calculate_results.py +++ b/posthog/caching/calculate_results.py @@ -123,7 +123,7 @@ def get_cache_type(cacheable: Optional[FilterType] | Optional[dict]) -> CacheTyp def calculate_for_query_based_insight( - insight: Insight, *, dashboard: Optional[Dashboard] = None, execution_mode: ExecutionMode, user: User + insight: Insight, *, dashboard: Optional[Dashboard] = None, execution_mode: ExecutionMode, user: Optional[User] ) -> "InsightResult": from posthog.caching.fetch_from_cache import InsightResult, NothingInCacheResult from posthog.caching.insight_cache import update_cached_state diff --git a/posthog/models/__init__.py b/posthog/models/__init__.py index f2f36b0c7cb0e..389d573cb7e7b 100644 --- a/posthog/models/__init__.py +++ b/posthog/models/__init__.py @@ -13,7 +13,7 @@ ) from ..warehouse.models import DataWarehouseTable from ._deprecated_prompts import Prompt, PromptSequence, UserPromptState -from .alert import Alert, AbsoluteThreshold, AnomalyCondition +from .alert import Alert from .action import Action from .action.action_step import ActionStep from .activity_logging.activity_log import ActivityLog @@ -73,13 +73,11 @@ from .user_scene_personalisation import UserScenePersonalisation __all__ = [ - "AbsoluteThreshold", "Alert", "Action", "ActionStep", "ActivityLog", "Annotation", - "AnomalyCondition", "AsyncDeletion", "AsyncMigration", "AsyncMigrationError", diff --git a/posthog/models/alert.py b/posthog/models/alert.py index 3ef6ee448499a..1c324a32798f6 100644 --- a/posthog/models/alert.py +++ b/posthog/models/alert.py @@ -1,19 +1,4 @@ from django.db import models -from typing import Optional -from dataclasses import dataclass - - -@dataclass -class AbsoluteThreshold: - lower: Optional[float] = None - upper: Optional[float] = None - - -class AnomalyCondition: - absolute_threshold: AbsoluteThreshold - - def __init__(self, absoluteThreshold: dict): - self.absolute_threshold = AbsoluteThreshold(**absoluteThreshold) class Alert(models.Model): diff --git a/posthog/schema.py b/posthog/schema.py index c46df6df52699..4040a98c04691 100644 --- a/posthog/schema.py +++ b/posthog/schema.py @@ -12,6 +12,14 @@ class SchemaRoot(RootModel[Any]): root: Any +class AbsoluteThreshold(BaseModel): + model_config = ConfigDict( + extra="forbid", + ) + lower: Optional[float] = None + upper: Optional[float] = None + + class MathGroupTypeIndex(float, Enum): NUMBER_0 = 0 NUMBER_1 = 1 @@ -28,6 +36,13 @@ class AggregationAxisFormat(StrEnum): PERCENTAGE_SCALED = "percentage_scaled" +class AnomalyCondition(BaseModel): + model_config = ConfigDict( + extra="forbid", + ) + absoluteThreshold: AbsoluteThreshold + + class Kind(StrEnum): METHOD = "Method" FUNCTION = "Function" diff --git a/posthog/tasks/detect_alerts_anomalies.py b/posthog/tasks/detect_alerts_anomalies.py index 407759159ccd3..3351c6a172c25 100644 --- a/posthog/tasks/detect_alerts_anomalies.py +++ b/posthog/tasks/detect_alerts_anomalies.py @@ -1,14 +1,15 @@ -from typing import cast - import structlog from celery import shared_task from django.utils import timezone +from posthog.api.services.query import ExecutionMode +from posthog.caching.calculate_results import calculate_for_query_based_insight from posthog.email import EmailMessage -from posthog.hogql_queries.legacy_compatibility.filter_to_query import filter_to_query -from posthog.hogql_queries.query_runner import get_query_runner -from posthog.models import Alert, AnomalyCondition -from posthog.schema import HogQLQueryResponse +from posthog.hogql_queries.legacy_compatibility.flagged_conversion_manager import ( + conversion_to_query_based, +) +from posthog.models import Alert +from posthog.schema import AnomalyCondition logger = structlog.get_logger(__name__) @@ -17,24 +18,31 @@ def check_all_alerts() -> None: alerts = Alert.objects.all().only("id") for alert in alerts: logger.info("scheduling alert", alert_id=alert.id) - check_alert.delay(alert.id) + _check_alert_task.delay(alert.id) @shared_task(ignore_result=True) -def check_alert(id: int) -> None: +def _check_alert_task(id: int) -> None: + _check_alert(id) + + +def _check_alert(id: int) -> None: alert = Alert.objects.get(pk=id) insight = alert.insight - if not insight.query: - insight.query = filter_to_query(insight.filters) - query_runner = get_query_runner(insight.query, alert.team) - response = cast(HogQLQueryResponse, query_runner.calculate()) - if not response.results: + with conversion_to_query_based(insight): + calculation_result = calculate_for_query_based_insight( + insight, + execution_mode=ExecutionMode.RECENT_CACHE_CALCULATE_BLOCKING_IF_STALE, + user=None, + ) + + if not calculation_result.result: raise RuntimeError(f"no results for alert {alert.id}") - anomaly_condition = AnomalyCondition(**alert.anomaly_condition) - thresholds = anomaly_condition.absolute_threshold + anomaly_condition = AnomalyCondition.model_validate(alert.anomaly_condition) + thresholds = anomaly_condition.absoluteThreshold - result = response.results[0] + result = calculation_result.result[0] aggregated_value = result["aggregated_value"] anomalies_descriptions = [] @@ -51,6 +59,11 @@ def check_alert(id: int) -> None: logger.info("no anomalies", alert_id=alert.id) return + _send_notifications(alert, anomalies_descriptions) + + +# TODO: make it a task +def _send_notifications(alert: Alert, anomalies_descriptions: list[str]) -> None: subject = f"PostHog alert {alert.name} has anomalies" campaign_key = f"alert-anomaly-notification-{alert.id}-{timezone.now().timestamp()}" insight_url = f"/project/{alert.team.pk}/insights/{alert.insight.short_id}" diff --git a/posthog/tasks/test/test_detect_alerts_anomalies.py b/posthog/tasks/test/test_detect_alerts_anomalies.py index dddf97c2e8dd7..47881d551d191 100644 --- a/posthog/tasks/test/test_detect_alerts_anomalies.py +++ b/posthog/tasks/test/test_detect_alerts_anomalies.py @@ -72,7 +72,7 @@ def test_alert_is_triggered_for_values_above_higher_threshold(self, MockEmailMes assert len(mocked_email_messages) == 1 assert self.get_recepients(mocked_email_messages) == [["a@b.c", "d@e.f"]] - assert "The trend value (1) is above the upper threshold (0)" in mocked_email_messages[0].html_body + assert "The trend value (1) is above the upper threshold (0.0)" in mocked_email_messages[0].html_body def test_alert_is_not_triggered_for_events_beyond_interval(self, MockEmailMessage: MagicMock) -> None: mocked_email_messages = mock_email_messages(MockEmailMessage) @@ -97,7 +97,7 @@ def test_alert_is_triggered_for_value_below_lower_threshold(self, MockEmailMessa check_all_alerts() assert len(mocked_email_messages) == 1 - assert "The trend value (0) is below the lower threshold (1)" in mocked_email_messages[0].html_body + assert "The trend value (0) is below the lower threshold (1.0)" in mocked_email_messages[0].html_body def test_alert_is_not_triggered_for_normal_values(self, MockEmailMessage: MagicMock) -> None: mocked_email_messages = mock_email_messages(MockEmailMessage) @@ -148,8 +148,8 @@ def test_two_alerts_are_triggered(self, MockEmailMessage: MagicMock) -> None: check_all_alerts() assert len(mocked_email_messages) == 2 - assert "The trend value (0) is below the lower threshold (1)" in mocked_email_messages[0].html_body - assert "The trend value (0) is below the lower threshold (1)" in mocked_email_messages[1].html_body + assert "The trend value (0) is below the lower threshold (1.0)" in mocked_email_messages[0].html_body + assert "The trend value (0) is below the lower threshold (1.0)" in mocked_email_messages[1].html_body assert self.get_recepients(mocked_email_messages) == [["a@b.c", "d@e.f"], ["email@address.com"]] def test_alert_with_insight_with_filter(self, MockEmailMessage: MagicMock) -> None: @@ -164,4 +164,4 @@ def test_alert_with_insight_with_filter(self, MockEmailMessage: MagicMock) -> No check_all_alerts() assert len(mocked_email_messages) == 1 - assert "The trend value (0) is below the lower threshold (1)" in mocked_email_messages[0].html_body + assert "The trend value (0) is below the lower threshold (1.0)" in mocked_email_messages[0].html_body From 16227b9358cee004b31548c1f890d6a1c8ece60d Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Fri, 5 Jul 2024 15:10:35 +0200 Subject: [PATCH 11/20] Refactor things --- posthog/tasks/__init__.py | 2 - ...ct_alerts_anomalies.py => check_alerts.py} | 48 ++++++++++++++----- posthog/tasks/scheduled.py | 4 +- posthog/tasks/tasks.py | 7 --- ...erts_anomalies.py => test_check_alerts.py} | 4 +- 5 files changed, 39 insertions(+), 26 deletions(-) rename posthog/tasks/{detect_alerts_anomalies.py => check_alerts.py} (71%) rename posthog/tasks/test/{test_detect_alerts_anomalies.py => test_check_alerts.py} (98%) diff --git a/posthog/tasks/__init__.py b/posthog/tasks/__init__.py index bd79aedec977e..8e3e665eb7785 100644 --- a/posthog/tasks/__init__.py +++ b/posthog/tasks/__init__.py @@ -6,7 +6,6 @@ check_clickhouse_schema_drift, demo_create_data, demo_reset_master_team, - detect_alerts_anomalies, email, exporter, hog_functions, @@ -26,7 +25,6 @@ "check_clickhouse_schema_drift", "demo_create_data", "demo_reset_master_team", - "detect_alerts_anomalies", "email", "exporter", "hog_functions", diff --git a/posthog/tasks/detect_alerts_anomalies.py b/posthog/tasks/check_alerts.py similarity index 71% rename from posthog/tasks/detect_alerts_anomalies.py rename to posthog/tasks/check_alerts.py index 3351c6a172c25..44a811a55f1f5 100644 --- a/posthog/tasks/detect_alerts_anomalies.py +++ b/posthog/tasks/check_alerts.py @@ -1,5 +1,6 @@ import structlog from celery import shared_task +from celery.canvas import group, chain from django.utils import timezone from posthog.api.services.query import ExecutionMode @@ -15,20 +16,41 @@ def check_all_alerts() -> None: - alerts = Alert.objects.all().only("id") - for alert in alerts: - logger.info("scheduling alert", alert_id=alert.id) - _check_alert_task.delay(alert.id) + alert_ids = list(Alert.objects.all().values_list("id", flat=True)) + + group_count = 10 + chunk_size = 10 + + alert_id_groups = [alert_ids[i : i + group_count] for i in range(0, len(alert_ids), group_count)] + task_groups = group( + chain( + *( + check_alert_task.chunks( + [(alert_id,) for alert_id in g], + chunk_size, + ) + for g in alert_id_groups + ) + ) + ) + + task_groups.apply_async() @shared_task(ignore_result=True) -def _check_alert_task(id: int) -> None: - _check_alert(id) +def check_all_alerts_task() -> None: + check_all_alerts() -def _check_alert(id: int) -> None: - alert = Alert.objects.get(pk=id) +@shared_task(ignore_result=True) +def check_alert_task(alert_id: int) -> None: + check_alert(alert_id) + + +def check_alert(alert_id: int) -> None: + alert = Alert.objects.get(pk=alert_id) insight = alert.insight + with conversion_to_query_based(insight): calculation_result = calculate_for_query_based_insight( insight, @@ -37,7 +59,7 @@ def _check_alert(id: int) -> None: ) if not calculation_result.result: - raise RuntimeError(f"no results for alert {alert.id}") + raise RuntimeError(f"No results for alert {alert.id}") anomaly_condition = AnomalyCondition.model_validate(alert.anomaly_condition) thresholds = anomaly_condition.absoluteThreshold @@ -56,16 +78,16 @@ def _check_alert(id: int) -> None: ] if not anomalies_descriptions: - logger.info("no anomalies", alert_id=alert.id) + logger.info("No threshold met", alert_id=alert.id) return - _send_notifications(alert, anomalies_descriptions) + send_notifications(alert, anomalies_descriptions) # TODO: make it a task -def _send_notifications(alert: Alert, anomalies_descriptions: list[str]) -> None: +def send_notifications(alert: Alert, anomalies_descriptions: list[str]) -> None: subject = f"PostHog alert {alert.name} has anomalies" - campaign_key = f"alert-anomaly-notification-{alert.id}-{timezone.now().timestamp()}" + campaign_key = f"alert-anomaly-notification-{alert.id}-{timezone.now().isoformat()}" insight_url = f"/project/{alert.team.pk}/insights/{alert.insight.short_id}" alert_url = f"{insight_url}/alerts/{alert.id}" message = EmailMessage( diff --git a/posthog/tasks/scheduled.py b/posthog/tasks/scheduled.py index cdeb5718ad33d..9a1f4bd7bba06 100644 --- a/posthog/tasks/scheduled.py +++ b/posthog/tasks/scheduled.py @@ -7,6 +7,7 @@ from django.conf import settings from posthog.celery import app +from posthog.tasks.check_alerts import check_all_alerts_task from posthog.tasks.tasks import ( calculate_cohort, calculate_decide_usage, @@ -25,7 +26,6 @@ clickhouse_row_count, clickhouse_send_license_usage, delete_expired_exported_assets, - detect_alerts_anomalies, ee_persist_finished_recordings, find_flags_with_enriched_analytics, graphile_worker_queue_size, @@ -254,7 +254,7 @@ def setup_periodic_tasks(sender: Celery, **kwargs: Any) -> None: sender.add_periodic_task( crontab(hour="*", minute="20"), - detect_alerts_anomalies.s(), + check_all_alerts_task.s(), name="detect alerts' anomalies and notify about them", ) diff --git a/posthog/tasks/tasks.py b/posthog/tasks/tasks.py index 455ed086e3822..6a592febe4dea 100644 --- a/posthog/tasks/tasks.py +++ b/posthog/tasks/tasks.py @@ -782,13 +782,6 @@ def update_survey_iteration() -> None: update_survey_iteration() -@shared_task(ignore_result=True) -def detect_alerts_anomalies() -> None: - from posthog.tasks.detect_alerts_anomalies import check_all_alerts - - check_all_alerts() - - def recompute_materialized_columns_enabled() -> bool: from posthog.models.instance_setting import get_instance_setting diff --git a/posthog/tasks/test/test_detect_alerts_anomalies.py b/posthog/tasks/test/test_check_alerts.py similarity index 98% rename from posthog/tasks/test/test_detect_alerts_anomalies.py rename to posthog/tasks/test/test_check_alerts.py index 47881d551d191..b47b1c8ba94c5 100644 --- a/posthog/tasks/test/test_detect_alerts_anomalies.py +++ b/posthog/tasks/test/test_check_alerts.py @@ -5,15 +5,15 @@ from freezegun import freeze_time from posthog.models.instance_setting import set_instance_setting +from posthog.tasks.check_alerts import check_all_alerts from posthog.test.base import APIBaseTest, _create_event, flush_persons_and_events, ClickhouseDestroyTablesMixin from posthog.api.test.dashboards import DashboardAPI from posthog.schema import ChartDisplayType, EventsNode, TrendsQuery, TrendsFilter from posthog.tasks.test.utils_email_tests import mock_email_messages -from posthog.tasks.detect_alerts_anomalies import check_all_alerts @freeze_time("2024-06-02T08:55:00.000Z") -@patch("posthog.tasks.detect_alerts_anomalies.EmailMessage") +@patch("posthog.tasks.check_alerts.EmailMessage") class TestDetectAlertsAnomaliesTasks(APIBaseTest, ClickhouseDestroyTablesMixin): def setUp(self) -> None: super().setUp() From 712d780b4f5d45eb9202e3cabbee34305eaf57c7 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Fri, 5 Jul 2024 15:22:45 +0200 Subject: [PATCH 12/20] Fix scheduled task setup Somehow it didn't work for me --- posthog/tasks/scheduled.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/posthog/tasks/scheduled.py b/posthog/tasks/scheduled.py index 9a1f4bd7bba06..1a2bbbce86857 100644 --- a/posthog/tasks/scheduled.py +++ b/posthog/tasks/scheduled.py @@ -93,6 +93,8 @@ def setup_periodic_tasks(sender: Celery, **kwargs: Any) -> None: add_periodic_task_with_expiry(sender, 20, start_poll_query_performance.s(), "20 sec query performance heartbeat") + add_periodic_task_with_expiry(sender, 60 * 60, check_all_alerts_task.s(), "check all alerts") + # Update events table partitions twice a week sender.add_periodic_task( crontab(day_of_week="mon,fri", hour="0", minute="0"), @@ -252,12 +254,6 @@ def setup_periodic_tasks(sender: Celery, **kwargs: Any) -> None: name="update survey iteration based on date", ) - sender.add_periodic_task( - crontab(hour="*", minute="20"), - check_all_alerts_task.s(), - name="detect alerts' anomalies and notify about them", - ) - if settings.EE_AVAILABLE: # every interval seconds, we calculate N replay embeddings # the goal is to process _enough_ every 24 hours that From f179e37196fc9ef47f902914e63a51480b8d3b3b Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Fri, 5 Jul 2024 15:28:38 +0200 Subject: [PATCH 13/20] Refactor more --- posthog/tasks/alerts/__init__.py | 0 .../{check_alerts.py => alerts/checks.py} | 20 +++++++++---------- posthog/tasks/alerts/test/__init__.py | 0 .../test/test_checks.py} | 4 ++-- posthog/tasks/scheduled.py | 2 +- 5 files changed, 13 insertions(+), 13 deletions(-) create mode 100644 posthog/tasks/alerts/__init__.py rename posthog/tasks/{check_alerts.py => alerts/checks.py} (100%) create mode 100644 posthog/tasks/alerts/test/__init__.py rename posthog/tasks/{test/test_check_alerts.py => alerts/test/test_checks.py} (98%) diff --git a/posthog/tasks/alerts/__init__.py b/posthog/tasks/alerts/__init__.py new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/posthog/tasks/check_alerts.py b/posthog/tasks/alerts/checks.py similarity index 100% rename from posthog/tasks/check_alerts.py rename to posthog/tasks/alerts/checks.py index 44a811a55f1f5..181c3d85c0ac6 100644 --- a/posthog/tasks/check_alerts.py +++ b/posthog/tasks/alerts/checks.py @@ -37,16 +37,6 @@ def check_all_alerts() -> None: task_groups.apply_async() -@shared_task(ignore_result=True) -def check_all_alerts_task() -> None: - check_all_alerts() - - -@shared_task(ignore_result=True) -def check_alert_task(alert_id: int) -> None: - check_alert(alert_id) - - def check_alert(alert_id: int) -> None: alert = Alert.objects.get(pk=alert_id) insight = alert.insight @@ -84,6 +74,16 @@ def check_alert(alert_id: int) -> None: send_notifications(alert, anomalies_descriptions) +@shared_task(ignore_result=True) +def check_all_alerts_task() -> None: + check_all_alerts() + + +@shared_task(ignore_result=True) +def check_alert_task(alert_id: int) -> None: + check_alert(alert_id) + + # TODO: make it a task def send_notifications(alert: Alert, anomalies_descriptions: list[str]) -> None: subject = f"PostHog alert {alert.name} has anomalies" diff --git a/posthog/tasks/alerts/test/__init__.py b/posthog/tasks/alerts/test/__init__.py new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/posthog/tasks/test/test_check_alerts.py b/posthog/tasks/alerts/test/test_checks.py similarity index 98% rename from posthog/tasks/test/test_check_alerts.py rename to posthog/tasks/alerts/test/test_checks.py index b47b1c8ba94c5..972d422683603 100644 --- a/posthog/tasks/test/test_check_alerts.py +++ b/posthog/tasks/alerts/test/test_checks.py @@ -5,7 +5,7 @@ from freezegun import freeze_time from posthog.models.instance_setting import set_instance_setting -from posthog.tasks.check_alerts import check_all_alerts +from posthog.tasks.alerts.checks import check_all_alerts from posthog.test.base import APIBaseTest, _create_event, flush_persons_and_events, ClickhouseDestroyTablesMixin from posthog.api.test.dashboards import DashboardAPI from posthog.schema import ChartDisplayType, EventsNode, TrendsQuery, TrendsFilter @@ -13,7 +13,7 @@ @freeze_time("2024-06-02T08:55:00.000Z") -@patch("posthog.tasks.check_alerts.EmailMessage") +@patch("posthog.tasks.alerts.checks.EmailMessage") class TestDetectAlertsAnomaliesTasks(APIBaseTest, ClickhouseDestroyTablesMixin): def setUp(self) -> None: super().setUp() diff --git a/posthog/tasks/scheduled.py b/posthog/tasks/scheduled.py index 1a2bbbce86857..52056349e36aa 100644 --- a/posthog/tasks/scheduled.py +++ b/posthog/tasks/scheduled.py @@ -7,7 +7,7 @@ from django.conf import settings from posthog.celery import app -from posthog.tasks.check_alerts import check_all_alerts_task +from posthog.tasks.alerts.checks import check_all_alerts_task from posthog.tasks.tasks import ( calculate_cohort, calculate_decide_usage, From 5cf4e4629fdeb0873604b270fb574771ae1a8ccd Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Tue, 9 Jul 2024 17:29:56 +0200 Subject: [PATCH 14/20] Fix group setup --- posthog/tasks/alerts/checks.py | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/posthog/tasks/alerts/checks.py b/posthog/tasks/alerts/checks.py index 181c3d85c0ac6..e1d2b0e8cf7d7 100644 --- a/posthog/tasks/alerts/checks.py +++ b/posthog/tasks/alerts/checks.py @@ -23,15 +23,8 @@ def check_all_alerts() -> None: alert_id_groups = [alert_ids[i : i + group_count] for i in range(0, len(alert_ids), group_count)] task_groups = group( - chain( - *( - check_alert_task.chunks( - [(alert_id,) for alert_id in g], - chunk_size, - ) - for g in alert_id_groups - ) - ) + chain(check_alert_task.chunks([(alert_id,) for alert_id in alert_id_group], chunk_size)) + for alert_id_group in alert_id_groups ) task_groups.apply_async() From d9404415bac1805e35eefea1417daafb9ee1bfef Mon Sep 17 00:00:00 2001 From: Nikita Vorobev Date: Fri, 12 Jul 2024 09:30:54 +0100 Subject: [PATCH 15/20] address comments --- frontend/src/queries/schema.json | 1 - frontend/src/queries/schema.ts | 4 +- posthog/tasks/alerts/checks.py | 23 +++-- posthog/tasks/alerts/test/test_checks.py | 108 +++++++++++------------ 4 files changed, 66 insertions(+), 70 deletions(-) diff --git a/frontend/src/queries/schema.json b/frontend/src/queries/schema.json index 05f3a79f84963..c34c47b097433 100644 --- a/frontend/src/queries/schema.json +++ b/frontend/src/queries/schema.json @@ -11,7 +11,6 @@ "type": ["number", "null"] } }, - "required": ["lower", "upper"], "type": "object" }, "ActionsNode": { diff --git a/frontend/src/queries/schema.ts b/frontend/src/queries/schema.ts index 66027e597cd27..a59da9a330f7b 100644 --- a/frontend/src/queries/schema.ts +++ b/frontend/src/queries/schema.ts @@ -1527,8 +1527,8 @@ export interface DashboardFilter { } export interface AbsoluteThreshold { - lower: number | null - upper: number | null + lower?: number | null + upper?: number | null } export interface AnomalyCondition { diff --git a/posthog/tasks/alerts/checks.py b/posthog/tasks/alerts/checks.py index e1d2b0e8cf7d7..d6f811352e60c 100644 --- a/posthog/tasks/alerts/checks.py +++ b/posthog/tasks/alerts/checks.py @@ -2,6 +2,7 @@ from celery import shared_task from celery.canvas import group, chain from django.utils import timezone +import math from posthog.api.services.query import ExecutionMode from posthog.caching.calculate_results import calculate_for_query_based_insight @@ -19,15 +20,17 @@ def check_all_alerts() -> None: alert_ids = list(Alert.objects.all().values_list("id", flat=True)) group_count = 10 - chunk_size = 10 + # All groups but the last one will have a group_size size. + # The last group will have at most group_size size. + group_size = int(math.ceil(len(alert_ids) / group_count)) - alert_id_groups = [alert_ids[i : i + group_count] for i in range(0, len(alert_ids), group_count)] - task_groups = group( - chain(check_alert_task.chunks([(alert_id,) for alert_id in alert_id_group], chunk_size)) - for alert_id_group in alert_id_groups - ) + groups = [] + for i in range(0, len(alert_ids), group_size): + alert_id_group = alert_ids[i : i + group_size] + chained_calls = chain([check_alert_task.s(alert_id=alert_id) for alert_id in alert_id_group]) + groups.append(chained_calls) - task_groups.apply_async() + group(groups).apply_async() def check_alert(alert_id: int) -> None: @@ -72,9 +75,11 @@ def check_all_alerts_task() -> None: check_all_alerts() +# Note, check_alert_task is used in Celery chains. Celery chains pass the previous +# function call result to the next function as an argument, hence args and kwargs. @shared_task(ignore_result=True) -def check_alert_task(alert_id: int) -> None: - check_alert(alert_id) +def check_alert_task(*args, **kwargs) -> None: + check_alert(**kwargs) # TODO: make it a task diff --git a/posthog/tasks/alerts/test/test_checks.py b/posthog/tasks/alerts/test/test_checks.py index 972d422683603..fb5f93b3cb166 100644 --- a/posthog/tasks/alerts/test/test_checks.py +++ b/posthog/tasks/alerts/test/test_checks.py @@ -1,25 +1,25 @@ import pytest -from typing import Any, Optional +from typing import Optional from unittest.mock import MagicMock, patch from freezegun import freeze_time from posthog.models.instance_setting import set_instance_setting -from posthog.tasks.alerts.checks import check_all_alerts +from posthog.tasks.alerts.checks import send_notifications, check_alert from posthog.test.base import APIBaseTest, _create_event, flush_persons_and_events, ClickhouseDestroyTablesMixin from posthog.api.test.dashboards import DashboardAPI from posthog.schema import ChartDisplayType, EventsNode, TrendsQuery, TrendsFilter from posthog.tasks.test.utils_email_tests import mock_email_messages +from posthog.models import Alert @freeze_time("2024-06-02T08:55:00.000Z") -@patch("posthog.tasks.alerts.checks.EmailMessage") -class TestDetectAlertsAnomaliesTasks(APIBaseTest, ClickhouseDestroyTablesMixin): +@patch("posthog.tasks.alerts.checks.send_notifications") +class TestCheckAlertsTasks(APIBaseTest, ClickhouseDestroyTablesMixin): def setUp(self) -> None: super().setUp() set_instance_setting("EMAIL_HOST", "fake_host") set_instance_setting("EMAIL_ENABLED", True) - self.settings(CELERY_TASK_ALWAYS_EAGER=True) self.dashboard_api = DashboardAPI(self.client, self.team, self.assertEqual) query_dict = TrendsQuery( series=[ @@ -52,12 +52,10 @@ def set_thresholds(self, lower: Optional[int] = None, upper: Optional[int] = Non data={"anomaly_condition": {"absoluteThreshold": {"lower": lower, "upper": upper}}}, ) - def get_recepients(self, mocked_email_messages: list[Any]) -> list[list[str]]: - recipients = [sorted([to["recipient"] for to in message.to]) for message in mocked_email_messages] - return sorted(recipients) + def get_anomalies_descriptions(self, mock_send_notifications: MagicMock, call_index: int) -> list[str]: + return mock_send_notifications.call_args_list[call_index].args[1] - def test_alert_is_triggered_for_values_above_higher_threshold(self, MockEmailMessage: MagicMock) -> None: - mocked_email_messages = mock_email_messages(MockEmailMessage) + def test_alert_is_triggered_for_values_above_higher_threshold(self, mock_send_notifications: MagicMock) -> None: self.set_thresholds(upper=0) with freeze_time("2024-06-02T07:55:00.000Z"): @@ -68,14 +66,17 @@ def test_alert_is_triggered_for_values_above_higher_threshold(self, MockEmailMes ) flush_persons_and_events() - check_all_alerts() + check_alert(self.alert["id"]) - assert len(mocked_email_messages) == 1 - assert self.get_recepients(mocked_email_messages) == [["a@b.c", "d@e.f"]] - assert "The trend value (1) is above the upper threshold (0.0)" in mocked_email_messages[0].html_body + assert mock_send_notifications.call_count == 1 + alert = mock_send_notifications.call_args_list[0].args[0] + assert alert.id == self.alert["id"] - def test_alert_is_not_triggered_for_events_beyond_interval(self, MockEmailMessage: MagicMock) -> None: - mocked_email_messages = mock_email_messages(MockEmailMessage) + anomalies_descriptions = self.get_anomalies_descriptions(mock_send_notifications, call_index=0) + assert len(anomalies_descriptions) == 1 + assert "The trend value (1) is above the upper threshold (0.0)" in anomalies_descriptions[0] + + def test_alert_is_not_triggered_for_events_beyond_interval(self, mock_send_notifications: MagicMock) -> None: self.set_thresholds(upper=0) with freeze_time("2024-05-02T07:55:00.000Z"): @@ -86,35 +87,35 @@ def test_alert_is_not_triggered_for_events_beyond_interval(self, MockEmailMessag ) flush_persons_and_events() - check_all_alerts() + check_alert(self.alert["id"]) - assert len(mocked_email_messages) == 0 + assert mock_send_notifications.call_count == 0 - def test_alert_is_triggered_for_value_below_lower_threshold(self, MockEmailMessage: MagicMock) -> None: - mocked_email_messages = mock_email_messages(MockEmailMessage) + def test_alert_is_triggered_for_value_below_lower_threshold(self, mock_send_notifications: MagicMock) -> None: self.set_thresholds(lower=1) - check_all_alerts() + check_alert(self.alert["id"]) - assert len(mocked_email_messages) == 1 - assert "The trend value (0) is below the lower threshold (1.0)" in mocked_email_messages[0].html_body + assert mock_send_notifications.call_count == 1 + anomalies = self.get_anomalies_descriptions(mock_send_notifications, call_index=0) + assert "The trend value (0) is below the lower threshold (1.0)" in anomalies - def test_alert_is_not_triggered_for_normal_values(self, MockEmailMessage: MagicMock) -> None: - mocked_email_messages = mock_email_messages(MockEmailMessage) + def test_alert_is_not_triggered_for_normal_values(self, mock_send_notifications: MagicMock) -> None: self.set_thresholds(lower=0, upper=1) - check_all_alerts() + check_alert(self.alert["id"]) - assert len(mocked_email_messages) == 0 + assert mock_send_notifications.call_count == 0 - def test_error_while_calculating_no_alert(self, MockEmailMessage: MagicMock) -> None: - mocked_email_messages = mock_email_messages(MockEmailMessage) + def test_error_while_calculating_no_alert(self, mock_send_notifications: MagicMock) -> None: query_dict = TrendsQuery( series=[ EventsNode( event="$pageview", ), ], + # This query is not represented as a bold number, so calculating the insight aggregated value + # causes an error ).model_dump() insight = self.dashboard_api.create_insight( data={ @@ -125,35 +126,11 @@ def test_error_while_calculating_no_alert(self, MockEmailMessage: MagicMock) -> self.client.patch(f"/api/projects/{self.team.id}/alerts/{self.alert['id']}", data={"insight": insight["id"]}) - # in production failure of a single alert won't cause an exception in check_all_alerts - # because the execution won't be eager (see CELERY_TASK_ALWAYS_EAGER in the set up) with pytest.raises(KeyError): - check_all_alerts() - - assert len(mocked_email_messages) == 0 + check_alert(self.alert["id"]) + assert mock_send_notifications.call_count == 0 - def test_two_alerts_are_triggered(self, MockEmailMessage: MagicMock) -> None: - mocked_email_messages = mock_email_messages(MockEmailMessage) - self.set_thresholds(lower=1) - self.client.post( - f"/api/projects/{self.team.id}/alerts", - data={ - "name": "another alert name", - "insight": self.insight["id"], - "target_value": "email@address.com", - "anomaly_condition": {"absoluteThreshold": {"lower": 1}}, - }, - ).json() - - check_all_alerts() - - assert len(mocked_email_messages) == 2 - assert "The trend value (0) is below the lower threshold (1.0)" in mocked_email_messages[0].html_body - assert "The trend value (0) is below the lower threshold (1.0)" in mocked_email_messages[1].html_body - assert self.get_recepients(mocked_email_messages) == [["a@b.c", "d@e.f"], ["email@address.com"]] - - def test_alert_with_insight_with_filter(self, MockEmailMessage: MagicMock) -> None: - mocked_email_messages = mock_email_messages(MockEmailMessage) + def test_alert_with_insight_with_filter(self, mock_send_notifications: MagicMock) -> None: insight = self.dashboard_api.create_insight( data={"name": "insight", "filters": {"events": [{"id": "$pageview"}], "display": "BoldNumber"}} )[1] @@ -161,7 +138,22 @@ def test_alert_with_insight_with_filter(self, MockEmailMessage: MagicMock) -> No self.client.patch(f"/api/projects/{self.team.id}/alerts/{self.alert['id']}", data={"insight": insight["id"]}) self.set_thresholds(lower=1) - check_all_alerts() + check_alert(self.alert["id"]) + + assert mock_send_notifications.call_count == 1 + anomalies = self.get_anomalies_descriptions(mock_send_notifications, call_index=0) + assert "The trend value (0) is below the lower threshold (1.0)" in anomalies + + @patch("posthog.tasks.alerts.checks.EmailMessage") + def test_send_emails(self, MockEmailMessage: MagicMock, mock_send_notifications: MagicMock) -> None: + mocked_email_messages = mock_email_messages(MockEmailMessage) + alert = Alert.objects.get(pk=self.alert["id"]) + send_notifications(alert, ["first anomaly description", "second anomaly description"]) assert len(mocked_email_messages) == 1 - assert "The trend value (0) is below the lower threshold (1.0)" in mocked_email_messages[0].html_body + email = mocked_email_messages[0] + assert len(email.to) == 2 + assert email.to[0]["recipient"] == "a@b.c" + assert email.to[1]["recipient"] == "d@e.f" + assert "first anomaly description" in email.html_body + assert "second anomaly description" in email.html_body From ed27736f8074e371aa11304c47c0120c09f777bc Mon Sep 17 00:00:00 2001 From: Nikita Vorobev Date: Sat, 13 Jul 2024 11:23:25 +0100 Subject: [PATCH 16/20] fix typing --- posthog/tasks/alerts/checks.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/posthog/tasks/alerts/checks.py b/posthog/tasks/alerts/checks.py index d6f811352e60c..c1dc1fbaa92fa 100644 --- a/posthog/tasks/alerts/checks.py +++ b/posthog/tasks/alerts/checks.py @@ -1,8 +1,9 @@ -import structlog from celery import shared_task from celery.canvas import group, chain from django.utils import timezone import math +import structlog +from typing import Any from posthog.api.services.query import ExecutionMode from posthog.caching.calculate_results import calculate_for_query_based_insight @@ -78,7 +79,7 @@ def check_all_alerts_task() -> None: # Note, check_alert_task is used in Celery chains. Celery chains pass the previous # function call result to the next function as an argument, hence args and kwargs. @shared_task(ignore_result=True) -def check_alert_task(*args, **kwargs) -> None: +def check_alert_task(*args: Any, **kwargs: Any) -> None: check_alert(**kwargs) From 4e9716058c5c9c0626827cd96837f93f0ecc6891 Mon Sep 17 00:00:00 2001 From: Julian Bez Date: Wed, 17 Jul 2024 11:46:55 +0200 Subject: [PATCH 17/20] Revert "Fix scheduled task setup" This reverts commit 712d780b4f5d45eb9202e3cabbee34305eaf57c7. --- posthog/tasks/scheduled.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/posthog/tasks/scheduled.py b/posthog/tasks/scheduled.py index 52056349e36aa..1fd85e599f6d8 100644 --- a/posthog/tasks/scheduled.py +++ b/posthog/tasks/scheduled.py @@ -93,8 +93,6 @@ def setup_periodic_tasks(sender: Celery, **kwargs: Any) -> None: add_periodic_task_with_expiry(sender, 20, start_poll_query_performance.s(), "20 sec query performance heartbeat") - add_periodic_task_with_expiry(sender, 60 * 60, check_all_alerts_task.s(), "check all alerts") - # Update events table partitions twice a week sender.add_periodic_task( crontab(day_of_week="mon,fri", hour="0", minute="0"), @@ -254,6 +252,12 @@ def setup_periodic_tasks(sender: Celery, **kwargs: Any) -> None: name="update survey iteration based on date", ) + sender.add_periodic_task( + crontab(hour="*", minute="20"), + check_all_alerts_task.s(), + name="detect alerts' anomalies and notify about them", + ) + if settings.EE_AVAILABLE: # every interval seconds, we calculate N replay embeddings # the goal is to process _enough_ every 24 hours that From 028d155059def4cf31a5b78e949eed3ba25fe323 Mon Sep 17 00:00:00 2001 From: Nikita Vorobev Date: Thu, 18 Jul 2024 19:24:05 +0100 Subject: [PATCH 18/20] use si for chains --- posthog/tasks/alerts/checks.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/posthog/tasks/alerts/checks.py b/posthog/tasks/alerts/checks.py index c1dc1fbaa92fa..4ed6d29114401 100644 --- a/posthog/tasks/alerts/checks.py +++ b/posthog/tasks/alerts/checks.py @@ -3,7 +3,6 @@ from django.utils import timezone import math import structlog -from typing import Any from posthog.api.services.query import ExecutionMode from posthog.caching.calculate_results import calculate_for_query_based_insight @@ -28,7 +27,7 @@ def check_all_alerts() -> None: groups = [] for i in range(0, len(alert_ids), group_size): alert_id_group = alert_ids[i : i + group_size] - chained_calls = chain([check_alert_task.s(alert_id=alert_id) for alert_id in alert_id_group]) + chained_calls = chain([check_alert_task.si(alert_id) for alert_id in alert_id_group]) groups.append(chained_calls) group(groups).apply_async() @@ -76,11 +75,9 @@ def check_all_alerts_task() -> None: check_all_alerts() -# Note, check_alert_task is used in Celery chains. Celery chains pass the previous -# function call result to the next function as an argument, hence args and kwargs. @shared_task(ignore_result=True) -def check_alert_task(*args: Any, **kwargs: Any) -> None: - check_alert(**kwargs) +def check_alert_task(alert_id: int) -> None: + check_alert(alert_id) # TODO: make it a task From fa7dc4cfdfc62abe72f8f19a51fb03df5fcbead0 Mon Sep 17 00:00:00 2001 From: Nikita Vorobev Date: Thu, 18 Jul 2024 22:10:43 +0100 Subject: [PATCH 19/20] use timestamp for the campaign key --- posthog/tasks/alerts/checks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/posthog/tasks/alerts/checks.py b/posthog/tasks/alerts/checks.py index 4ed6d29114401..28b964b338af8 100644 --- a/posthog/tasks/alerts/checks.py +++ b/posthog/tasks/alerts/checks.py @@ -83,7 +83,7 @@ def check_alert_task(alert_id: int) -> None: # TODO: make it a task def send_notifications(alert: Alert, anomalies_descriptions: list[str]) -> None: subject = f"PostHog alert {alert.name} has anomalies" - campaign_key = f"alert-anomaly-notification-{alert.id}-{timezone.now().isoformat()}" + campaign_key = f"alert-anomaly-notification-{alert.id}-{timezone.now().timestamp()}" insight_url = f"/project/{alert.team.pk}/insights/{alert.insight.short_id}" alert_url = f"{insight_url}/alerts/{alert.id}" message = EmailMessage( From 66f0c4ac265d20eef0ae4ee0db4e9f9e99123880 Mon Sep 17 00:00:00 2001 From: Nikita Vorobev Date: Tue, 6 Aug 2024 18:21:59 +0100 Subject: [PATCH 20/20] brush up the PR --- frontend/src/types.ts | 9 +-------- posthog/tasks/alerts/checks.py | 1 - posthog/templates/email/alert_anomaly.html | 2 +- 3 files changed, 2 insertions(+), 10 deletions(-) diff --git a/frontend/src/types.ts b/frontend/src/types.ts index a81e03640e793..495e8b7daab90 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -33,6 +33,7 @@ import { Scene } from 'scenes/sceneTypes' import { QueryContext } from '~/queries/types' import type { + AnomalyCondition, DashboardFilter, DatabaseSchemaField, HogQLQuery, @@ -4370,14 +4371,6 @@ export type HogFunctionInvocationGlobals = { > } -// TODO: move to schema.ts -export interface AnomalyCondition { - absoluteThreshold: { - lower?: number - upper?: number - } -} - export interface AlertType { id: number name: string diff --git a/posthog/tasks/alerts/checks.py b/posthog/tasks/alerts/checks.py index 28b964b338af8..e5c34f578a048 100644 --- a/posthog/tasks/alerts/checks.py +++ b/posthog/tasks/alerts/checks.py @@ -80,7 +80,6 @@ def check_alert_task(alert_id: int) -> None: check_alert(alert_id) -# TODO: make it a task def send_notifications(alert: Alert, anomalies_descriptions: list[str]) -> None: subject = f"PostHog alert {alert.name} has anomalies" campaign_key = f"alert-anomaly-notification-{alert.id}-{timezone.now().timestamp()}" diff --git a/posthog/templates/email/alert_anomaly.html b/posthog/templates/email/alert_anomaly.html index c50dafd157cc3..49636488288dc 100644 --- a/posthog/templates/email/alert_anomaly.html +++ b/posthog/templates/email/alert_anomaly.html @@ -1,6 +1,6 @@ {% extends "email/base.html" %} {% load posthog_assets %} {% block section %}

- Uh-oh, the {{ alert_name }} alert detected following anomalies for {{ insight_name }}: + The {{ alert_name }} alert detected following anomalies for {{ insight_name }}:

    {% for anomaly_description in anomalies_descriptions %}
  • {{ anomaly_description }}