-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(alerts): add a regular job to detect anomalies #22762
Changes from 29 commits
eb4cb49
ba51409
bb71f5a
1d57843
15e0b6b
71c44d9
5e59a5d
29e613b
b1a0219
0b2e57a
29d9305
0cd1a01
b8b4fec
3475c37
16227b9
712d780
f179e37
5cf4e46
d940441
d91bcc9
ed27736
a1966b4
4e97160
028d155
1920482
fa7dc4c
e5b44ca
b8d4e60
e3db36a
4183314
66f0c4a
d274bea
0e8d28a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -268,7 +268,7 @@ export const insightNavLogic = kea<insightNavLogicType>([ | |
}, | ||
})), | ||
urlToAction(({ actions }) => ({ | ||
'/insights/:shortId(/:mode)(/:subscriptionId)': ( | ||
'/insights/:shortId(/:mode)(/:itemId)': ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per #22554 (comment) |
||
_, // url params | ||
{ dashboard, ...searchParams }, // search params | ||
{ filters: _filters } // hash params | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4369,6 +4369,7 @@ export type HogFunctionInvocationGlobals = { | |
> | ||
} | ||
|
||
// TODO: move to schema.ts | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was planning to do it in a follow up PR, but yeah, I can fix it here, done |
||
export interface AnomalyCondition { | ||
absoluteThreshold: { | ||
lower?: number | ||
|
nikitaevg marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
from celery import shared_task | ||
from celery.canvas import group, chain | ||
from django.utils import timezone | ||
import math | ||
import structlog | ||
|
||
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.flagged_conversion_manager import ( | ||
conversion_to_query_based, | ||
) | ||
from posthog.models import Alert | ||
from posthog.schema import AnomalyCondition | ||
|
||
logger = structlog.get_logger(__name__) | ||
|
||
|
||
def check_all_alerts() -> None: | ||
alert_ids = list(Alert.objects.all().values_list("id", flat=True)) | ||
|
||
group_count = 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)) | ||
|
||
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.si(alert_id) for alert_id in alert_id_group]) | ||
groups.append(chained_calls) | ||
|
||
group(groups).apply_async() | ||
|
||
|
||
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, | ||
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.model_validate(alert.anomaly_condition) | ||
thresholds = anomaly_condition.absoluteThreshold | ||
|
||
result = calculation_result.result[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 threshold met", alert_id=alert.id) | ||
return | ||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this needs to be a task. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes sense, thanks, removed |
||
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}" | ||
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 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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a redundant field