Skip to content

Commit

Permalink
feat(alerts): advanced config (#27127)
Browse files Browse the repository at this point in the history
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
  • Loading branch information
anirudhpillai and github-actions[bot] authored Dec 23, 2024
1 parent e7d0a4c commit 1323e66
Show file tree
Hide file tree
Showing 17 changed files with 247 additions and 96 deletions.
27 changes: 26 additions & 1 deletion frontend/src/lib/components/Alerts/alertFormLogic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,16 @@ import { AlertType, AlertTypeWrite } from './types'

export type AlertFormType = Pick<
AlertType,
'name' | 'enabled' | 'created_at' | 'threshold' | 'condition' | 'subscribed_users' | 'checks' | 'config'
| 'name'
| 'enabled'
| 'created_at'
| 'calculation_interval'
| 'threshold'
| 'condition'
| 'subscribed_users'
| 'checks'
| 'config'
| 'skip_weekend'
> & {
id?: AlertType['id']
created_by?: AlertType['created_by'] | null
Expand Down Expand Up @@ -48,6 +57,7 @@ export const alertFormLogic = kea<alertFormLogicType>([
config: {
type: 'TrendsAlertConfig',
series_index: 0,
check_ongoing_interval: false,
},
threshold: { configuration: { type: InsightThresholdType.ABSOLUTE, bounds: {} } },
condition: {
Expand All @@ -56,6 +66,7 @@ export const alertFormLogic = kea<alertFormLogicType>([
subscribed_users: [],
checks: [],
calculation_interval: AlertCalculationInterval.DAILY,
skip_weekend: false,
insight: props.insightId,
} as AlertFormType),
errors: ({ name }) => ({
Expand All @@ -66,6 +77,20 @@ export const alertFormLogic = kea<alertFormLogicType>([
...alert,
subscribed_users: alert.subscribed_users?.map(({ id }) => id),
insight: props.insightId,
// can only skip weekends for hourly/daily alerts
skip_weekend:
(alert.calculation_interval === AlertCalculationInterval.DAILY ||
alert.calculation_interval === AlertCalculationInterval.HOURLY) &&
alert.skip_weekend,
// can only check ongoing interval for absolute value/increase alerts with upper threshold
config: {
...alert.config,
check_ongoing_interval:
(alert.condition.type === AlertConditionType.ABSOLUTE_VALUE ||
alert.condition.type === AlertConditionType.RELATIVE_INCREASE) &&
alert.threshold.configuration.bounds?.upper != null &&
alert.config.check_ongoing_interval,
},
}

// absolute value alert can only have absolute threshold
Expand Down
1 change: 1 addition & 0 deletions frontend/src/lib/components/Alerts/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export interface AlertTypeBase {
enabled: boolean
insight: QueryBasedInsightModel
config: AlertConfig
skip_weekend?: boolean
}

export interface AlertTypeWrite extends Omit<AlertTypeBase, 'insight'> {
Expand Down
55 changes: 55 additions & 0 deletions frontend/src/lib/components/Alerts/views/EditAlertModal.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { IconInfo } from '@posthog/icons'
import {
LemonBanner,
LemonCheckbox,
LemonInput,
LemonSegmentedButton,
LemonSelect,
SpinnerOverlay,
Tooltip,
} from '@posthog/lemon-ui'
import { useActions, useValues } from 'kea'
import { Form, Group } from 'kea-forms'
Expand Down Expand Up @@ -97,6 +99,11 @@ export function EditAlertModal({
const { alertSeries, isNonTimeSeriesDisplay, isBreakdownValid, formula } = useValues(trendsLogic)

const creatingNewAlert = alertForm.id === undefined
// can only check ongoing interval for absolute value/increase alerts with upper threshold
const can_check_ongoing_interval =
(alertForm?.condition.type === AlertConditionType.ABSOLUTE_VALUE ||
alertForm?.condition.type === AlertConditionType.RELATIVE_INCREASE) &&
alertForm.threshold.configuration.bounds?.upper != null

return (
<LemonModal onClose={onClose} isOpen={isOpen} width={600} simple title="">
Expand Down Expand Up @@ -320,7 +327,55 @@ export function EditAlertModal({
</div>
</div>
</div>

<div className="space-y-2">
<h3 className="text-muted-alt">Advanced</h3>
<Group name={['config']}>
<div className="flex gap-1">
<LemonField name="check_ongoing_interval">
<LemonCheckbox
checked={
can_check_ongoing_interval &&
alertForm?.config.check_ongoing_interval
}
data-attr="alertForm-check-ongoing-interval"
fullWidth
label="Check ongoing period"
disabledReason={
!can_check_ongoing_interval &&
'Can only alert for ongoing period when checking for absolute value/increase above threshold'
}
/>
</LemonField>
<Tooltip
title={`Checks the insight value for the on going period (current week/month) that hasn't yet completed. Use this if you want to be alerted right away when the insight value rises/increases above threshold`}
placement="right"
delayMs={0}
>
<IconInfo />
</Tooltip>
</div>
</Group>
<LemonField name="skip_weekend">
<LemonCheckbox
checked={
(alertForm?.calculation_interval === AlertCalculationInterval.DAILY ||
alertForm?.calculation_interval === AlertCalculationInterval.HOURLY) &&
alertForm?.skip_weekend
}
data-attr="alertForm-skip-weekend"
fullWidth
label="Skip checking on weekends"
disabledReason={
alertForm?.calculation_interval !== AlertCalculationInterval.DAILY &&
alertForm?.calculation_interval !== AlertCalculationInterval.HOURLY &&
'Can only skip weekend checking for hourly/daily alerts'
}
/>
</LemonField>
</div>
</div>

{alert && <AlertStateTable alert={alert} />}
</LemonModal.Content>

Expand Down
3 changes: 3 additions & 0 deletions frontend/src/queries/schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -12519,6 +12519,9 @@
"TrendsAlertConfig": {
"additionalProperties": false,
"properties": {
"check_ongoing_interval": {
"type": "boolean"
},
"series_index": {
"type": "integer"
},
Expand Down
1 change: 1 addition & 0 deletions frontend/src/queries/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2379,6 +2379,7 @@ export enum AlertCalculationInterval {
export interface TrendsAlertConfig {
type: 'TrendsAlertConfig'
series_index: integer
check_ongoing_interval?: boolean
}

export interface HogCompileResponse {
Expand Down
1 change: 1 addition & 0 deletions posthog/api/alert.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ class Meta:
"config",
"calculation_interval",
"snoozed_until",
"skip_weekend",
]
read_only_fields = [
"id",
Expand Down
1 change: 1 addition & 0 deletions posthog/api/test/test_alert.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ def test_create_and_delete_alert(self) -> None:
"last_checked_at": None,
"next_check_at": None,
"snoozed_until": None,
"skip_weekend": False,
}
assert response.status_code == status.HTTP_201_CREATED, response.content
assert response.json() == expected_alert_json
Expand Down
17 changes: 17 additions & 0 deletions posthog/migrations/0536_alertconfiguration_skip_weekend.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Generated by Django 4.2.15 on 2024-12-23 11:55

from django.db import migrations, models


class Migration(migrations.Migration):
dependencies = [
("posthog", "0535_alter_hogfunction_type"),
]

operations = [
migrations.AddField(
model_name="alertconfiguration",
name="skip_weekend",
field=models.BooleanField(blank=True, default=False, null=True),
),
]
2 changes: 1 addition & 1 deletion posthog/migrations/max_migration.txt
Original file line number Diff line number Diff line change
@@ -1 +1 @@
0535_alter_hogfunction_type
0536_alertconfiguration_skip_weekend
2 changes: 2 additions & 0 deletions posthog/models/alert.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ class AlertConfiguration(CreatedMetaFields, UUIDModel):
# UTC time until when we shouldn't check alert/notify user
snoozed_until = models.DateTimeField(null=True, blank=True)

skip_weekend = models.BooleanField(null=True, blank=True, default=False)

def __str__(self):
return f"{self.name} (Team: {self.team})"

Expand Down
1 change: 1 addition & 0 deletions posthog/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -1571,6 +1571,7 @@ class TrendsAlertConfig(BaseModel):
model_config = ConfigDict(
extra="forbid",
)
check_ongoing_interval: Optional[bool] = None
series_index: int
type: Literal["TrendsAlertConfig"] = "TrendsAlertConfig"

Expand Down
8 changes: 8 additions & 0 deletions posthog/tasks/alerts/checks.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
next_check_time,
send_notifications_for_breaches,
send_notifications_for_errors,
skip_because_of_weekend,
WRAPPER_NODE_KINDS,
)
from posthog.tasks.alerts.trends import check_trends_alert
Expand Down Expand Up @@ -216,6 +217,13 @@ def check_alert(alert_id: str, capture_ph_event: Callable = lambda *args, **kwar
)
return

if skip_because_of_weekend(alert):
logger.info(
"Skipping alert check because weekend checking is disabled",
alert=alert,
)
return

if alert.snoozed_until:
if alert.snoozed_until > now:
logger.info(
Expand Down
29 changes: 27 additions & 2 deletions posthog/tasks/alerts/test/test_alert_checks.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@ def set_thresholds(self, lower: Optional[int] = None, upper: Optional[int] = Non
data={"threshold": {"configuration": {"type": "absolute", "bounds": {"lower": lower, "upper": upper}}}},
)

def skip_weekend(self, skip: bool) -> None:
self.client.patch(
f"/api/projects/{self.team.id}/alerts/{self.alert['id']}",
data={"skip_weekend": skip},
)

def get_breach_description(self, mock_send_notifications_for_breaches: MagicMock, call_index: int) -> list[str]:
return mock_send_notifications_for_breaches.call_args_list[call_index].args[1]

Expand Down Expand Up @@ -130,7 +136,7 @@ def test_alert_is_triggered_for_value_below_lower_threshold(

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 interval (0) is less than lower threshold (1.0)" in anomalies
assert "The insight value ($pageview) for current 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
Expand Down Expand Up @@ -318,7 +324,7 @@ def test_alert_with_insight_with_filter(

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 interval (0) is less than lower threshold (1.0)" in anomalies
assert "The insight value ($pageview) for current interval (0) is less than lower threshold (1.0)" in anomalies

@patch("posthog.tasks.alerts.utils.EmailMessage")
def test_send_emails(
Expand Down Expand Up @@ -389,3 +395,22 @@ def test_alert_not_recalculated_when_is_calculating(
assert mock_send_notifications_for_breaches.call_count == 1
second_check = AlertCheck.objects.filter(alert_configuration=self.alert["id"]).latest("created_at")
assert first_check.id == second_check.id

def test_alert_is_not_checked_on_weekend_when_skip_weekends_is_true(
self, mock_send_notifications_for_breaches: MagicMock, mock_send_errors: MagicMock
) -> None:
self.skip_weekend(True)

# run on weekend
with freeze_time("2024-12-21T07:55:00.000Z"):
check_alert(self.alert["id"])

checks = AlertCheck.objects.filter(alert_configuration=self.alert["id"])
assert len(checks) == 0

# run on week day
with freeze_time("2024-12-19T07:55:00.000Z"):
check_alert(self.alert["id"])

checks = AlertCheck.objects.filter(alert_configuration=self.alert["id"])
assert len(checks) == 1
24 changes: 12 additions & 12 deletions posthog/tasks/alerts/test/test_trends_absolute_alerts.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ def create_alert(
lower: Optional[int] = None,
upper: Optional[int] = None,
calculation_interval: AlertCalculationInterval = AlertCalculationInterval.DAILY,
check_ongoing_interval: bool = False,
) -> dict:
alert = self.client.post(
f"/api/projects/{self.team.id}/alerts",
Expand All @@ -59,6 +60,7 @@ def create_alert(
"config": {
"type": "TrendsAlertConfig",
"series_index": series_index,
"check_ongoing_interval": check_ongoing_interval,
},
"condition": {"type": "absolute_value"},
"calculation_interval": calculation_interval,
Expand All @@ -69,7 +71,9 @@ def create_alert(
return alert

def create_time_series_trend_insight(
self, breakdown: Optional[BreakdownFilter] = None, interval: IntervalType = IntervalType.WEEK
self,
breakdown: Optional[BreakdownFilter] = None,
interval: IntervalType = IntervalType.WEEK,
) -> dict[str, Any]:
query_dict = TrendsQuery(
series=[
Expand Down Expand Up @@ -491,7 +495,7 @@ def test_trend_current_interval_high_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, upper=1)
alert = self.create_alert(insight, series_index=0, upper=1, check_ongoing_interval=True)

# around 8 AM on same day as check
with freeze_time(FROZEN_TIME - dateutil.relativedelta.relativedelta(hours=1)):
Expand Down Expand Up @@ -528,11 +532,11 @@ def test_trend_current_interval_high_threshold_breached(
ANY, ["The insight value (signed_up) for current week (2) is more than upper threshold (1.0)"]
)

def test_trend_current_interval_fallback_to_previous_high_threshold_breached(
def test_trend_current_interval_should_not_fallback_to_previous_high_threshold_breached(
self, mock_send_breaches: MagicMock, mock_send_errors: MagicMock
) -> None:
insight = self.create_time_series_trend_insight(interval=IntervalType.DAY)
alert = self.create_alert(insight, series_index=0, upper=1)
alert = self.create_alert(insight, series_index=0, upper=1, check_ongoing_interval=True)

# current day doesn't breach
with freeze_time(FROZEN_TIME - dateutil.relativedelta.relativedelta(hours=1)):
Expand Down Expand Up @@ -563,23 +567,19 @@ def test_trend_current_interval_fallback_to_previous_high_threshold_breached(
check_alert(alert["id"])

updated_alert = AlertConfiguration.objects.get(pk=alert["id"])
assert updated_alert.state == AlertState.FIRING
assert updated_alert.state == AlertState.NOT_FIRING

next_check = (FROZEN_TIME + dateutil.relativedelta.relativedelta(days=1)).replace(hour=1, tzinfo=pytz.UTC)
assert updated_alert.next_check_at is not None
assert updated_alert.next_check_at.hour == next_check.hour
assert updated_alert.next_check_at.date() == next_check.date()

alert_check = AlertCheck.objects.filter(alert_configuration=alert["id"]).latest("created_at")
assert alert_check.calculated_value == 2
assert alert_check.state == AlertState.FIRING
# has to have value for current period
assert alert_check.calculated_value == 1
assert alert_check.state == AlertState.NOT_FIRING
assert alert_check.error is None

# should be 'previous' week as current week check should fallback
mock_send_breaches.assert_called_once_with(
ANY, ["The insight value (signed_up) for previous day (2) is more than upper threshold (1.0)"]
)

def test_trend_current_interval_no_threshold_breached(
self, mock_send_breaches: MagicMock, mock_send_errors: MagicMock
) -> None:
Expand Down
Loading

0 comments on commit 1323e66

Please sign in to comment.