diff --git a/cypress/e2e/alerts.cy.ts b/cypress/e2e/alerts.cy.ts index ea2dd44380ab1..110647969e6a7 100644 --- a/cypress/e2e/alerts.cy.ts +++ b/cypress/e2e/alerts.cy.ts @@ -15,19 +15,19 @@ describe('Alerts', () => { const createAlert = ( name: string = 'Alert name', - email: string = 'a@b.c', lowerThreshold: string = '100', upperThreshold: string = '200' ): void => { cy.get('[data-attr=more-button]').click() - cy.contains('Alerts').click() + cy.contains('Manage alerts').click() cy.contains('New alert').click() cy.get('[data-attr=alert-name]').clear().type(name) - cy.get('[data-attr=alert-notification-targets').clear().type(email) + cy.get('[data-attr=subscribed-users').click().type('{downarrow}{enter}') cy.get('[data-attr=alert-lower-threshold').clear().type(lowerThreshold) cy.get('[data-attr=alert-upper-threshold').clear().type(upperThreshold) cy.contains('Create alert').click() + cy.get('.Toastify__toast-body').should('contain', 'Alert saved') cy.url().should('not.include', '/new') cy.get('[aria-label="close"]').click() @@ -38,6 +38,7 @@ describe('Alerts', () => { cy.get('[data-attr=insight-edit-button]').click() cy.get('[data-attr=chart-filter]').click() cy.contains(displayType).click() + cy.get('.insight-empty-state').should('not.exist') cy.get('[data-attr=insight-save-button]').contains('Save').click() cy.url().should('not.include', '/edit') } @@ -45,7 +46,7 @@ describe('Alerts', () => { it('Should allow create and delete an alert', () => { cy.get('[data-attr=more-button]').click() // Alerts should be disabled for trends represented with graphs - cy.get('[data-attr=disabled-alerts-button]').should('exist') + cy.get('[data-attr=manage-alerts-button]').should('have.attr', 'aria-disabled', 'true') setInsightDisplayTypeAndSave('Number') @@ -54,10 +55,8 @@ describe('Alerts', () => { // Check the alert has the same values as when it was created cy.get('[data-attr=more-button]').click() - cy.contains('Alerts').click() cy.contains('Manage alerts').click() cy.get('[data-attr=alert-list-item]').contains('Alert name').click() - cy.get('[data-attr=alert-notification-targets]').should('have.value', 'a@b.c') cy.get('[data-attr=alert-name]').should('have.value', 'Alert name') cy.get('[data-attr=alert-lower-threshold').should('have.value', '100') cy.get('[data-attr=alert-upper-threshold').should('have.value', '200') @@ -90,7 +89,6 @@ describe('Alerts', () => { // Assert that saving an insight in an incompatible state removes alerts cy.get('[data-attr=more-button]').click() - cy.contains('Alerts').click() cy.contains('Manage alerts').click() cy.contains('Alert to be deleted because of a changed insight').should('not.exist') }) diff --git a/frontend/__snapshots__/exporter-exporter--user-paths-insight--light.png b/frontend/__snapshots__/exporter-exporter--user-paths-insight--light.png index 25984c46d9502..333981c508581 100644 Binary files a/frontend/__snapshots__/exporter-exporter--user-paths-insight--light.png and b/frontend/__snapshots__/exporter-exporter--user-paths-insight--light.png differ diff --git a/frontend/src/lib/api.ts b/frontend/src/lib/api.ts index 7b668a45bc434..9a5483f50dee0 100644 --- a/frontend/src/lib/api.ts +++ b/frontend/src/lib/api.ts @@ -10,6 +10,8 @@ import { SavedSessionRecordingPlaylistsResult } from 'scenes/session-recordings/ import { getCurrentExporterData } from '~/exporter/exporterViewLogic' import { + AlertType, + AlertTypeWrite, DatabaseSerializedFieldType, ErrorTrackingGroup, QuerySchema, @@ -19,7 +21,6 @@ import { import { ActionType, ActivityScope, - AlertType, AppMetricsTotalsV2Response, AppMetricsV2RequestParams, AppMetricsV2Response, @@ -730,12 +731,12 @@ class ApiRequest { } // # Alerts - public alerts(teamId?: TeamType['id']): ApiRequest { - return this.projectsDetail(teamId).addPathComponent('alerts') + public alerts(id: InsightModel['id'], teamId?: TeamType['id']): ApiRequest { + return this.insight(id, teamId).addPathComponent('alerts') } - public alert(id: AlertType['id'], teamId?: TeamType['id']): ApiRequest { - return this.alerts(teamId).addPathComponent(id) + public alert(id: AlertType['id'], insightId: InsightModel['id'], teamId?: TeamType['id']): ApiRequest { + return this.alerts(insightId, teamId).addPathComponent(id) } // Resource Access Permissions @@ -2264,20 +2265,20 @@ const api = { }, alerts: { - async get(alertId: AlertType['id']): Promise { - return await new ApiRequest().alert(alertId).get() + async get(insightId: number, alertId: AlertType['id']): Promise { + return await new ApiRequest().alert(alertId, insightId).get() }, - async create(data: Partial): Promise { - return await new ApiRequest().alerts().create({ data }) + async create(insightId: number, data: Partial): Promise { + return await new ApiRequest().alerts(insightId).create({ data }) }, - async update(alertId: AlertType['id'], data: Partial): Promise { - return await new ApiRequest().alert(alertId).update({ data }) + async update(insightId: number, alertId: AlertType['id'], data: Partial): Promise { + return await new ApiRequest().alert(alertId, insightId).update({ data }) }, async list(insightId: number): Promise> { - return await new ApiRequest().alerts().withQueryString(`insight=${insightId}`).get() + return await new ApiRequest().alerts(insightId).get() }, - async delete(alertId: AlertType['id']): Promise { - return await new ApiRequest().alert(alertId).delete() + async delete(insightId: number, alertId: AlertType['id']): Promise { + return await new ApiRequest().alert(alertId, insightId).delete() }, }, diff --git a/frontend/src/lib/components/Alerts/AlertDeletionWarning.tsx b/frontend/src/lib/components/Alerts/AlertDeletionWarning.tsx index cf1e5d0ad492c..d3c798462e792 100644 --- a/frontend/src/lib/components/Alerts/AlertDeletionWarning.tsx +++ b/frontend/src/lib/components/Alerts/AlertDeletionWarning.tsx @@ -7,8 +7,16 @@ import { alertsLogic } from './alertsLogic' export function AlertDeletionWarning(): JSX.Element | null { const { insightProps, insight } = useValues(insightLogic) + if (!insight?.short_id) { + return null + } + const { shouldShowAlertDeletionWarning } = useValues( - alertsLogic({ insightShortId: insight.short_id!, insightLogicProps: insightProps }) + alertsLogic({ + insightShortId: insight.short_id, + insightId: insight.id as number, + insightLogicProps: insightProps, + }) ) if (!shouldShowAlertDeletionWarning || !insight.short_id) { diff --git a/frontend/src/lib/components/Alerts/AlertsModal.tsx b/frontend/src/lib/components/Alerts/AlertsModal.tsx index d1ad9d9baf722..22e57b1112b82 100644 --- a/frontend/src/lib/components/Alerts/AlertsModal.tsx +++ b/frontend/src/lib/components/Alerts/AlertsModal.tsx @@ -1,4 +1,4 @@ -import { LemonButton, LemonButtonWithDropdown } from '@posthog/lemon-ui' +import { LemonButton } from '@posthog/lemon-ui' import { useActions, useValues } from 'kea' import { router } from 'kea-router' import { FEATURE_FLAGS } from 'lib/constants' @@ -17,11 +17,11 @@ import { ManageAlerts } from './views/ManageAlerts' export interface AlertsModalProps extends AlertsLogicProps { isOpen: boolean closeModal: () => void - alertId: number | 'new' | null + alertId?: string | null } export function AlertsModal(props: AlertsModalProps): JSX.Element { - const { closeModal, insightShortId, insightLogicProps, alertId, isOpen } = props + const { closeModal, insightId, insightShortId, insightLogicProps, alertId, isOpen } = props const { push } = useActions(router) const { userLoading } = useValues(userLogic) @@ -32,14 +32,16 @@ export function AlertsModal(props: AlertsModalProps): JSX.Element { {!alertId ? ( push(urls.alert(insightShortId, id.toString()))} + onSelect={(id) => push(urls.alert(insightShortId, id ?? 'new'))} /> ) : ( push(urls.alerts(insightShortId))} @@ -62,36 +64,19 @@ export function AlertsButton({ insight }: AlertsButtonProps): JSX.Element { if (!showAlerts) { return <> } - if (!areAlertsSupportedForInsight(insight.query)) { - return ( - - Alerts - - ) - } + return ( - push(urls.alerts(insight.short_id!))} fullWidth - dropdown={{ - actionable: true, - closeParentPopoverOnClickInside: true, - placement: 'right-start', - overlay: ( - <> - push(urls.alert(insight.short_id!, 'new'))} fullWidth> - New alert - - push(urls.alerts(insight.short_id!))} fullWidth> - Manage alerts - - - ), - }} + disabledReason={ + !areAlertsSupportedForInsight(insight.query) + ? 'Insights are only available for trends represented as a number. Change the insight representation to add alerts.' + : undefined + } > - Alerts - + Manage alerts + ) } diff --git a/frontend/src/lib/components/Alerts/alertLogic.ts b/frontend/src/lib/components/Alerts/alertLogic.ts index 5887329a815ec..66b3fe483c5cd 100644 --- a/frontend/src/lib/components/Alerts/alertLogic.ts +++ b/frontend/src/lib/components/Alerts/alertLogic.ts @@ -1,38 +1,38 @@ -import { connect, kea, key, path, props } from 'kea' +import { connect, kea, key, listeners, path, props } from 'kea' import { forms } from 'kea-forms' import { loaders } from 'kea-loaders' import { router, urlToAction } from 'kea-router' import api from 'lib/api' import { lemonToast } from 'lib/lemon-ui/LemonToast/LemonToast' -import { isEmail } from 'lib/utils' -import { getInsightId } from 'scenes/insights/utils' import { urls } from 'scenes/urls' -import { AlertType } from '~/types' +import { AlertType, AlertTypeWrite } from '~/queries/schema' import type { alertLogicType } from './alertLogicType' import { alertsLogic, AlertsLogicProps } from './alertsLogic' export interface AlertLogicProps extends AlertsLogicProps { - id: number | 'new' + id?: string } export const alertLogic = kea([ path(['lib', 'components', 'Alerts', 'alertLogic']), props({} as AlertLogicProps), - key(({ id, insightShortId }) => `${insightShortId}-${id ?? 'new'}`), + key(({ id, insightId }) => `${insightId}-${id ?? 'new'}`), connect(() => ({ - actions: [alertsLogic, ['loadAlerts']], + actions: [alertsLogic, ['loadAlerts'], router, ['push']], })), loaders(({ props }) => ({ alert: { __default: undefined as unknown as AlertType, loadAlert: async () => { - if (props.id && props.id !== 'new') { - return await api.alerts.get(props.id) + if (props.id) { + return await api.alerts.get(props.insightId, props.id) + } + return { + enabled: true, } - return { anomaly_condition: { absoluteThreshold: {} } } }, }, })), @@ -40,40 +40,43 @@ export const alertLogic = kea([ forms(({ props, actions }) => ({ alert: { defaults: {} as unknown as AlertType, - errors: ({ name, target_value }) => ({ + errors: ({ name }) => ({ name: !name ? 'You need to give your alert a name' : undefined, - target_value: !target_value - ? 'This field is required.' - : !target_value.split(',').every((email) => isEmail(email)) - ? 'All emails must be valid' - : undefined, }), submit: async (alert) => { - const insightId = await getInsightId(props.insightShortId) - - const payload = { + const payload: AlertTypeWrite = { ...alert, - insight: insightId, + subscribed_users: alert.subscribed_users?.map(({ id }) => id), + insight: props.insightId, } - const updatedAlert: AlertType = - props.id === 'new' ? await api.alerts.create(payload) : await api.alerts.update(props.id, payload) + try { + const updatedAlert: AlertType = !props.id + ? await api.alerts.create(props.insightId, payload) + : await api.alerts.update(props.insightId, props.id, payload) - actions.resetAlert() - - if (updatedAlert.id !== props.id) { - router.actions.replace(urls.alerts(props.insightShortId)) - } + actions.resetAlert() - actions.loadAlerts() - actions.loadAlertSuccess(updatedAlert) - lemonToast.success(`Alert saved.`) + actions.loadAlerts() + actions.loadAlertSuccess(updatedAlert) + lemonToast.success(`Alert saved.`) - return updatedAlert + return updatedAlert + } catch (error: any) { + const field = error.data?.attr?.replace(/_/g, ' ') + lemonToast.error(`Error saving alert: ${field}: ${error.detail}`) + throw error + } }, }, })), + listeners(({ props }) => ({ + submitAlertSuccess: () => { + router.actions.push(urls.alerts(props.insightShortId)) + }, + })), + urlToAction(({ actions }) => ({ '/*/*/alerts/:id': () => { actions.loadAlert() diff --git a/frontend/src/lib/components/Alerts/alertsLogic.ts b/frontend/src/lib/components/Alerts/alertsLogic.ts index 3e24f5daa07d9..467d7c616b42a 100644 --- a/frontend/src/lib/components/Alerts/alertsLogic.ts +++ b/frontend/src/lib/components/Alerts/alertsLogic.ts @@ -2,14 +2,15 @@ import { actions, afterMount, connect, kea, key, listeners, path, props, reducer import { loaders } from 'kea-loaders' import api from 'lib/api' import { insightVizDataLogic } from 'scenes/insights/insightVizDataLogic' -import { getInsightId } from 'scenes/insights/utils' +import { AlertType } from '~/queries/schema' import { isInsightVizNode, isTrendsQuery } from '~/queries/utils' -import { AlertType, ChartDisplayType, InsightLogicProps, InsightShortId } from '~/types' +import { ChartDisplayType, InsightLogicProps, InsightShortId } from '~/types' import type { alertsLogicType } from './alertsLogicType' export interface AlertsLogicProps { + insightId: number insightShortId: InsightShortId insightLogicProps: InsightLogicProps } @@ -27,9 +28,9 @@ export const areAlertsSupportedForInsight = (query?: Record | null) export const alertsLogic = kea([ path(['lib', 'components', 'Alerts', 'alertsLogic']), props({} as AlertsLogicProps), - key(({ insightShortId }) => `insight-${insightShortId}`), + key(({ insightId }) => `insight-${insightId}`), actions({ - deleteAlert: (id: number) => ({ id }), + deleteAlert: (id: string) => ({ id }), setShouldShowAlertDeletionWarning: (show: boolean) => ({ show }), }), @@ -41,11 +42,7 @@ export const alertsLogic = kea([ alerts: { __default: [] as AlertType[], loadAlerts: async () => { - const insightId = await getInsightId(props.insightShortId) - if (!insightId) { - return [] - } - const response = await api.alerts.list(insightId) + const response = await api.alerts.list(props.insightId) return response.results }, }, @@ -63,9 +60,9 @@ export const alertsLogic = kea([ ], }), - listeners(({ actions, values }) => ({ + listeners(({ actions, values, props }) => ({ deleteAlert: async ({ id }) => { - await api.alerts.delete(id) + await api.alerts.delete(props.insightId, id) }, setQuery: ({ query }) => { if (values.alerts.length === 0 || areAlertsSupportedForInsight(query)) { diff --git a/frontend/src/lib/components/Alerts/views/EditAlert.tsx b/frontend/src/lib/components/Alerts/views/EditAlert.tsx index 46d0dfd627cc1..eac20a8703086 100644 --- a/frontend/src/lib/components/Alerts/views/EditAlert.tsx +++ b/frontend/src/lib/components/Alerts/views/EditAlert.tsx @@ -1,11 +1,17 @@ -import { LemonInput } from '@posthog/lemon-ui' +import { LemonCheckbox, LemonInput } from '@posthog/lemon-ui' import { useActions, useValues } from 'kea' import { Form, Group } from 'kea-forms' +import { AlertStateIndicator } from 'lib/components/Alerts/views/ManageAlerts' +import { MemberSelectMultiple } from 'lib/components/MemberSelectMultiple' +import { TZLabel } from 'lib/components/TZLabel' +import { UserActivityIndicator } from 'lib/components/UserActivityIndicator/UserActivityIndicator' import { IconChevronLeft } from 'lib/lemon-ui/icons' import { LemonButton } from 'lib/lemon-ui/LemonButton' import { LemonField } from 'lib/lemon-ui/LemonField' import { LemonModal } from 'lib/lemon-ui/LemonModal' +import { AlertType } from '~/queries/schema' + import { alertLogic, AlertLogicProps } from '../alertLogic' import { alertsLogic } from '../alertsLogic' @@ -14,16 +20,54 @@ interface EditAlertProps extends AlertLogicProps { onDelete: () => void } +export function AlertState({ alert }: { alert: AlertType }): JSX.Element | null { + if (!alert.checks || alert.checks.length === 0) { + return null + } + + return ( +
+

+ Current status {alert.state === 'firing' ? 'firing' : 'not met'} + +

+ + + + + + + + + + + {alert.checks.map((check) => ( + + + + + + + ))} + +
StatusTimeValueTargets notified
{check.state === 'firing' ? 'Firing' : 'Not met'} + + {check.calculated_value}{check.targets_notified ? 'Yes' : 'No'}
+
+ ) +} + export function EditAlert(props: EditAlertProps): JSX.Element { const logic = alertLogic(props) const alertslogic = alertsLogic(props) const { alert, isAlertSubmitting, alertChanged } = useValues(logic) const { deleteAlert } = useActions(alertslogic) + const { setAlertValue } = useActions(logic) const id = props.id const _onDelete = (): void => { - if (id !== 'new') { + if (id) { deleteAlert(id) props.onDelete() } @@ -35,11 +79,11 @@ export function EditAlert(props: EditAlertProps): JSX.Element {
} onClick={props.onCancel} size="xsmall" /> -

{id === 'new' ? 'New' : 'Edit '} Alert

+

{!id ? 'New' : 'Edit '} Alert

- + {!alert ? (

Not found

@@ -47,42 +91,62 @@ export function EditAlert(props: EditAlertProps): JSX.Element {
) : ( <> - - - - - - - - - - - - - - - - - +
+ {alert?.created_by ? ( + + ) : null} + + + + + + + + + + u.id) ?? []} + idKey="id" + onChange={(value) => setAlertValue('subscribed_users', value)} + /> + + + + + + + + + + + +
+ )}
- {alert && id !== 'new' && ( + {alert && id && ( Delete alert @@ -92,7 +156,7 @@ export function EditAlert(props: EditAlertProps): JSX.Element { Cancel - {id === 'new' ? 'Create alert' : 'Save'} + {!id ? 'Create alert' : 'Save'} diff --git a/frontend/src/lib/components/Alerts/views/ManageAlerts.tsx b/frontend/src/lib/components/Alerts/views/ManageAlerts.tsx index 6f7c8f953ffa3..6d904f0133105 100644 --- a/frontend/src/lib/components/Alerts/views/ManageAlerts.tsx +++ b/frontend/src/lib/components/Alerts/views/ManageAlerts.tsx @@ -1,14 +1,26 @@ -import { IconEllipsis } from '@posthog/icons' +import { IconEllipsis, IconPause } from '@posthog/icons' import { useActions, useValues } from 'kea' +import { router } from 'kea-router' +import { IconPlayCircle } from 'lib/lemon-ui/icons' import { LemonButton } from 'lib/lemon-ui/LemonButton' import { LemonModal } from 'lib/lemon-ui/LemonModal' +import { LemonTag } from 'lib/lemon-ui/LemonTag' import { ProfileBubbles } from 'lib/lemon-ui/ProfilePicture' import { pluralize } from 'lib/utils' +import { urls } from 'scenes/urls' -import { AlertType } from '~/types' +import { AlertType } from '~/queries/schema' import { alertsLogic, AlertsLogicProps } from '../alertsLogic' +export function AlertStateIndicator({ alert }: { alert: AlertType }): JSX.Element { + return alert.state === 'firing' ? ( + + ) : ( + + ) +} + interface AlertListItemProps { alert: AlertType onClick: () => void @@ -16,15 +28,16 @@ interface AlertListItemProps { } export function AlertListItem({ alert, onClick, onDelete }: AlertListItemProps): JSX.Element { + const absoluteThreshold = alert.threshold?.configuration?.absoluteThreshold return ( : } sideAction={{ icon: , - dropdown: { overlay: ( <> @@ -45,9 +58,23 @@ export function AlertListItem({ alert, onClick, onDelete }: AlertListItemProps): >
-
{alert.name}
+
+ {alert.name} + {alert.enabled ? ( + <> + +
+ {absoluteThreshold?.lower && `Low ${absoluteThreshold.lower}`} + {absoluteThreshold?.lower && absoluteThreshold?.upper ? ' · ' : ''} + {absoluteThreshold?.upper && `High ${absoluteThreshold.upper}`} +
+ + ) : ( +
Disabled
+ )} +
- ({ email }))} /> + ({ email }))} />
) @@ -55,10 +82,11 @@ export function AlertListItem({ alert, onClick, onDelete }: AlertListItemProps): interface ManageAlertsProps extends AlertsLogicProps { onCancel: () => void - onSelect: (value: number | 'new') => void + onSelect: (value?: string) => void } export function ManageAlerts(props: ManageAlertsProps): JSX.Element { + const { push } = useActions(router) const logic = alertsLogic(props) const { alerts } = useValues(logic) @@ -67,15 +95,20 @@ export function ManageAlerts(props: ManageAlertsProps): JSX.Element { return ( <> -

Manage Alerts

+

+ Manage Alerts ALPHA +

+
+ With alerts, PostHog will monitor your insight and notify you when certain conditions are met. We do + not evaluate alerts in real-time, but rather on a schedule of once every hour. Please note that + alerts are in alpha and may not be fully reliable. +
{alerts.length ? (
- {alerts?.length} - {' active '} - {pluralize(alerts.length || 0, 'alert', 'alerts', false)} + {alerts?.length} {pluralize(alerts.length || 0, 'alert', 'alerts', false)}
{alerts.map((alert) => ( @@ -97,6 +130,9 @@ export function ManageAlerts(props: ManageAlertsProps): JSX.Element { + push(urls.alert(props.insightShortId, 'new'))}> + New alert + Close diff --git a/frontend/src/lib/components/MemberSelectMultiple.tsx b/frontend/src/lib/components/MemberSelectMultiple.tsx new file mode 100644 index 0000000000000..97900f97947b1 --- /dev/null +++ b/frontend/src/lib/components/MemberSelectMultiple.tsx @@ -0,0 +1,48 @@ +import { LemonInputSelect, ProfilePicture } from '@posthog/lemon-ui' +import { useActions, useValues } from 'kea' +import { fullName } from 'lib/utils' +import { useEffect } from 'react' +import { membersLogic } from 'scenes/organization/membersLogic' + +import { UserBasicType } from '~/types' + +type UserIdType = string | number + +export type MemberSelectMultipleProps = { + idKey: 'email' | 'uuid' | 'id' + value: UserIdType[] + onChange: (values: UserBasicType[]) => void +} + +export function MemberSelectMultiple({ idKey, value, onChange }: MemberSelectMultipleProps): JSX.Element { + const { filteredMembers, membersLoading } = useValues(membersLogic) + const { ensureAllMembersLoaded } = useActions(membersLogic) + + useEffect(() => { + ensureAllMembersLoaded() + }, []) + + const options = filteredMembers.map((member) => ({ + key: member.user[idKey].toString(), + label: fullName(member.user), + value: member.user[idKey], + icon: , + })) + + return ( + v.toString())} + loading={membersLoading} + onChange={(newValues: UserIdType[]) => { + const selectedUsers = filteredMembers.filter((member) => + newValues.includes(member.user[idKey].toString()) + ) + onChange(selectedUsers.map((member) => member.user)) + }} + mode="multiple" + options={options} + data-attr="subscribed-users" + /> + ) +} diff --git a/frontend/src/queries/schema.json b/frontend/src/queries/schema.json index 58d64587f477c..23b8ffec17f1e 100644 --- a/frontend/src/queries/schema.json +++ b/frontend/src/queries/schema.json @@ -1,18 +1,6 @@ { "$schema": "http://json-schema.org/draft-07/schema#", "definitions": { - "AbsoluteThreshold": { - "additionalProperties": false, - "properties": { - "lower": { - "type": ["number", "null"] - }, - "upper": { - "type": ["number", "null"] - } - }, - "type": "object" - }, "ActionsNode": { "additionalProperties": false, "properties": { @@ -195,14 +183,143 @@ "enum": ["numeric", "duration", "duration_ms", "percentage", "percentage_scaled"], "type": "string" }, - "AnomalyCondition": { + "AlertCheck": { "additionalProperties": false, "properties": { - "absoluteThreshold": { - "$ref": "#/definitions/AbsoluteThreshold" + "calculated_value": { + "type": "number" + }, + "created_at": { + "type": "string" + }, + "id": { + "type": "string" + }, + "state": { + "type": "string" + }, + "targets_notified": { + "type": "boolean" + } + }, + "required": ["id", "created_at", "calculated_value", "state", "targets_notified"], + "type": "object" + }, + "AlertCondition": { + "additionalProperties": false, + "type": "object" + }, + "AlertType": { + "additionalProperties": false, + "properties": { + "checks": { + "items": { + "$ref": "#/definitions/AlertCheck" + }, + "type": "array" + }, + "condition": { + "$ref": "#/definitions/AlertCondition" + }, + "created_at": { + "type": "string" + }, + "created_by": { + "$ref": "#/definitions/UserBasicType" + }, + "enabled": { + "type": "boolean" + }, + "id": { + "type": "string" + }, + "insight": { + "type": "number" + }, + "last_notified_at": { + "type": "string" + }, + "name": { + "type": "string" + }, + "state": { + "type": "string" + }, + "subscribed_users": { + "items": { + "$ref": "#/definitions/UserBasicType" + }, + "type": "array" + }, + "threshold": { + "additionalProperties": false, + "properties": { + "configuration": { + "$ref": "#/definitions/InsightThreshold" + } + }, + "required": ["configuration"], + "type": "object" + } + }, + "required": [ + "checks", + "condition", + "created_at", + "created_by", + "enabled", + "id", + "insight", + "last_notified_at", + "name", + "state", + "subscribed_users", + "threshold" + ], + "type": "object" + }, + "AlertTypeBase": { + "additionalProperties": false, + "properties": { + "condition": { + "$ref": "#/definitions/AlertCondition" + }, + "enabled": { + "type": "boolean" + }, + "insight": { + "type": "number" + }, + "name": { + "type": "string" } }, - "required": ["absoluteThreshold"], + "required": ["name", "condition", "enabled", "insight"], + "type": "object" + }, + "AlertTypeWrite": { + "additionalProperties": false, + "properties": { + "condition": { + "$ref": "#/definitions/AlertCondition" + }, + "enabled": { + "type": "boolean" + }, + "insight": { + "type": "number" + }, + "name": { + "type": "string" + }, + "subscribed_users": { + "items": { + "type": "integer" + }, + "type": "array" + } + }, + "required": ["condition", "enabled", "insight", "name", "subscribed_users"], "type": "object" }, "AnyDataNode": { @@ -4745,6 +4862,10 @@ } ] }, + "HedgehogColorOptions": { + "enum": ["green", "red", "blue", "purple", "dark", "light", "sepia", "invert", "invert-hue", "greyscale"], + "type": "string" + }, "HogLanguage": { "enum": ["hog", "hogJson", "hogQL", "hogQLExpr", "hogTemplate"], "type": "string" @@ -5478,6 +5599,15 @@ "InsightShortId": { "type": "string" }, + "InsightThreshold": { + "additionalProperties": false, + "properties": { + "absoluteThreshold": { + "$ref": "#/definitions/InsightsThresholdAbsolute" + } + }, + "type": "object" + }, "InsightVizNode": { "additionalProperties": false, "properties": { @@ -5780,6 +5910,18 @@ "required": ["kind"], "type": "object" }, + "InsightsThresholdAbsolute": { + "additionalProperties": false, + "properties": { + "lower": { + "type": "number" + }, + "upper": { + "type": "number" + } + }, + "type": "object" + }, "IntervalType": { "enum": ["minute", "hour", "day", "week", "month"], "type": "string" @@ -5970,6 +6112,32 @@ } ] }, + "MinimalHedgehogConfig": { + "additionalProperties": false, + "properties": { + "accessories": { + "items": { + "type": "string" + }, + "type": "array" + }, + "color": { + "anyOf": [ + { + "$ref": "#/definitions/HedgehogColorOptions" + }, + { + "type": "null" + } + ] + }, + "use_as_profile": { + "type": "boolean" + } + }, + "required": ["use_as_profile", "color", "accessories"], + "type": "object" + }, "MultipleBreakdownOptions": { "additionalProperties": false, "properties": { @@ -9264,6 +9432,35 @@ "required": ["results"], "type": "object" }, + "UserBasicType": { + "additionalProperties": false, + "properties": { + "distinct_id": { + "type": "string" + }, + "email": { + "type": "string" + }, + "first_name": { + "type": "string" + }, + "hedgehog_config": { + "$ref": "#/definitions/MinimalHedgehogConfig" + }, + "id": { + "type": "number" + }, + "is_email_verified": {}, + "last_name": { + "type": "string" + }, + "uuid": { + "type": "string" + } + }, + "required": ["distinct_id", "email", "first_name", "id", "uuid"], + "type": "object" + }, "VizSpecificOptions": { "additionalProperties": false, "description": "Chart specific rendering options. Use ChartRenderingMetadata for non-serializable values, e.g. onClick handlers", diff --git a/frontend/src/queries/schema.ts b/frontend/src/queries/schema.ts index 0429cfc1bea93..034cf4e76c532 100644 --- a/frontend/src/queries/schema.ts +++ b/frontend/src/queries/schema.ts @@ -28,6 +28,7 @@ import { SessionPropertyFilter, StickinessFilterType, TrendsFilterType, + UserBasicType, } from '~/types' export { ChartDisplayCategory } @@ -1688,11 +1689,47 @@ export interface DashboardFilter { properties?: AnyPropertyFilter[] | null } -export interface AbsoluteThreshold { - lower?: number | null - upper?: number | null +export interface InsightsThresholdAbsolute { + lower?: number + upper?: number } -export interface AnomalyCondition { - absoluteThreshold: AbsoluteThreshold +export interface InsightThreshold { + absoluteThreshold?: InsightsThresholdAbsolute + // More types of thresholds or conditions can be added here +} + +export interface AlertCondition { + // Conditions in addition to the separate threshold + // TODO: Think about things like relative thresholds, rate of change, etc. +} + +export interface AlertCheck { + id: string + created_at: string + calculated_value: number + state: string + targets_notified: boolean +} + +export interface AlertTypeBase { + name: string + condition: AlertCondition + enabled: boolean + insight: number +} + +export interface AlertTypeWrite extends AlertTypeBase { + subscribed_users: integer[] +} + +export interface AlertType extends AlertTypeBase { + id: string + subscribed_users: UserBasicType[] + threshold: { configuration: InsightThreshold } + created_by: UserBasicType + created_at: string + state: string + last_notified_at: string + checks: AlertCheck[] } diff --git a/frontend/src/scenes/insights/InsightPageHeader.tsx b/frontend/src/scenes/insights/InsightPageHeader.tsx index a2fd6491eacd7..86e730efafe5b 100644 --- a/frontend/src/scenes/insights/InsightPageHeader.tsx +++ b/frontend/src/scenes/insights/InsightPageHeader.tsx @@ -78,7 +78,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={itemId} + subscriptionId={typeof itemId === 'number' || itemId === 'new' ? itemId : null} /> push(urls.insightView(insight.short_id as InsightShortId))} isOpen={insightMode === ItemMode.Alerts} insightLogicProps={insightLogicProps} + insightId={insight.id as number} insightShortId={insight.short_id as InsightShortId} - alertId={itemId} + alertId={typeof itemId === 'string' ? itemId : null} /> @@ -150,7 +151,6 @@ export function InsightPageHeader({ insightLogicProps }: { insightLogicProps: In Share or embed - {exportContext ? ( ) : null} + )} diff --git a/frontend/src/scenes/insights/insightSceneLogic.tsx b/frontend/src/scenes/insights/insightSceneLogic.tsx index 23b8ddb6e8c0c..cf1c390cd0138 100644 --- a/frontend/src/scenes/insights/insightSceneLogic.tsx +++ b/frontend/src/scenes/insights/insightSceneLogic.tsx @@ -71,10 +71,16 @@ export const insightSceneLogic = kea([ }, ], itemId: [ - null as null | number | 'new', + null as null | string | number, { setSceneState: (_, { itemId }) => - itemId !== undefined ? (itemId === 'new' ? 'new' : parseInt(itemId, 10)) : null, + itemId !== undefined + ? itemId === 'new' + ? 'new' + : Number.isInteger(+itemId) + ? parseInt(itemId, 10) + : itemId + : null, }, ], insightLogicRef: [ diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 66d7f2cf51c7e..c60428f829eac 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -33,7 +33,6 @@ import { Scene } from 'scenes/sceneTypes' import { QueryContext } from '~/queries/types' import type { - AnomalyCondition, DashboardFilter, DatabaseSchemaField, HogQLQuery, @@ -4452,14 +4451,6 @@ export type HogFunctionInvocationGlobals = { > } -export interface AlertType { - id: number - name: string - insight?: number - target_value: string - anomaly_condition: AnomalyCondition -} - export type AppMetricsV2Response = { labels: string[] series: { diff --git a/latest_migrations.manifest b/latest_migrations.manifest index 323c6228e1202..af30ae6589b0d 100644 --- a/latest_migrations.manifest +++ b/latest_migrations.manifest @@ -5,7 +5,7 @@ contenttypes: 0002_remove_content_type_name ee: 0016_rolemembership_organization_member otp_static: 0002_throttling otp_totp: 0002_auto_20190420_0723 -posthog: 0459_convert_personsnode_insights_to_actorsquery +posthog: 0460_alertconfiguration_threshold_alertsubscription_and_more sessions: 0001_initial social_django: 0010_uid_db_index two_factor: 0007_auto_20201201_1019 diff --git a/posthog/api/__init__.py b/posthog/api/__init__.py index 1e6af969cbafd..9d61824e95d13 100644 --- a/posthog/api/__init__.py +++ b/posthog/api/__init__.py @@ -383,6 +383,20 @@ def api_not_found(request): ["team_id", "insight_id"], ) +project_insights_router.register( + "thresholds", + alert.ThresholdViewSet, + "project_insight_thresholds", + ["team_id", "insight_id"], +) + +project_insights_router.register( + "alerts", + alert.AlertViewSet, + "project_insight_alerts", + ["team_id", "insight_id"], +) + project_session_recordings_router.register( r"sharing", sharing.SharingConfigurationViewSet, diff --git a/posthog/api/alert.py b/posthog/api/alert.py index 8560b0304e198..0a655a8bd3d9d 100644 --- a/posthog/api/alert.py +++ b/posthog/api/alert.py @@ -1,37 +1,208 @@ from rest_framework import serializers, viewsets from rest_framework.exceptions import ValidationError +from rest_framework.response import Response from django.db.models import QuerySet from posthog.api.routing import TeamAndOrgViewSetMixin -from posthog.models.alert import Alert +from posthog.api.shared import UserBasicSerializer +from posthog.models import User +from posthog.models.alert import ( + AlertConfiguration, + AlertCheck, + Threshold, + AlertSubscription, + are_alerts_supported_for_insight, +) + + +class ThresholdSerializer(serializers.ModelSerializer): + class Meta: + model = Threshold + fields = [ + "id", + "created_at", + "name", + "configuration", + ] + read_only_fields = [ + "id", + "created_at", + ] + + def validate(self, data): + instance = Threshold(**data) + instance.clean() + return data + + +class AlertCheckSerializer(serializers.ModelSerializer): + targets_notified = serializers.SerializerMethodField() + + class Meta: + model = AlertCheck + fields = [ + "id", + "created_at", + "calculated_value", + "state", + "targets_notified", + ] + read_only_fields = fields + + def get_targets_notified(self, instance: AlertCheck) -> bool: + return instance.targets_notified != {} + + +class AlertSubscriptionSerializer(serializers.ModelSerializer): + user = serializers.PrimaryKeyRelatedField(queryset=User.objects.filter(is_active=True), required=True) + + class Meta: + model = AlertSubscription + fields = ["id", "user", "alert_configuration"] + read_only_fields = ["id", "alert_configuration"] + + def validate(self, data): + user: User = data["user"] + alert_configuration = data["alert_configuration"] + + if not user.teams.filter(pk=alert_configuration.team_id).exists(): + raise serializers.ValidationError("User does not belong to the same organization as the alert's team.") + + return data class AlertSerializer(serializers.ModelSerializer): + created_by = UserBasicSerializer(read_only=True) + checks = AlertCheckSerializer(many=True, read_only=True) + threshold = ThresholdSerializer() + subscribed_users = serializers.PrimaryKeyRelatedField( + queryset=User.objects.filter(is_active=True), + many=True, + required=True, + write_only=True, + allow_empty=False, + ) + class Meta: - model = Alert + model = AlertConfiguration fields = [ "id", + "created_by", + "created_at", "insight", "name", - "target_value", - "anomaly_condition", + "subscribed_users", + "threshold", + "condition", + "state", + "enabled", + "last_notified_at", + "checks", + ] + read_only_fields = [ + "id", + "created_at", + "state", + "last_notified_at", ] - read_only_fields = ["id"] - def create(self, validated_data: dict) -> Alert: + def to_representation(self, instance): + data = super().to_representation(instance) + data["subscribed_users"] = UserBasicSerializer(instance.subscribed_users.all(), many=True, read_only=True).data + return data + + def add_threshold(self, threshold_data, validated_data): + threshold_instance = Threshold.objects.create( + **threshold_data, + team_id=self.context["team_id"], + created_by=self.context["request"].user, + insight_id=validated_data["insight"].id, + ) + return threshold_instance + + def create(self, validated_data: dict) -> AlertConfiguration: validated_data["team_id"] = self.context["team_id"] - instance: Alert = super().create(validated_data) + validated_data["created_by"] = self.context["request"].user + subscribed_users = validated_data.pop("subscribed_users") + threshold_data = validated_data.pop("threshold", None) + + if threshold_data: + threshold_instance = self.add_threshold(threshold_data, validated_data) + validated_data["threshold"] = threshold_instance + + instance: AlertConfiguration = super().create(validated_data) + + for user in subscribed_users: + AlertSubscription.objects.create( + user=user, alert_configuration=instance, created_by=self.context["request"].user + ) + return instance + def update(self, instance, validated_data): + conditions_or_threshold_changed = False + + threshold_data = validated_data.pop("threshold", None) + if threshold_data is not None: + if threshold_data == {}: + instance.threshold = None + conditions_or_threshold_changed = True + elif instance.threshold: + previous_threshold_configuration = instance.threshold.configuration + threshold_instance = instance.threshold + for key, value in threshold_data.items(): + setattr(threshold_instance, key, value) + threshold_instance.save() + if previous_threshold_configuration != threshold_instance.configuration: + conditions_or_threshold_changed = True + else: + threshold_instance = self.add_threshold(threshold_data, validated_data) + validated_data["threshold"] = threshold_instance + conditions_or_threshold_changed = True + + subscribed_users = validated_data.pop("subscribed_users", None) + if subscribed_users is not None: + AlertSubscription.objects.filter(alert_configuration=instance).exclude(user__in=subscribed_users).delete() + for user in subscribed_users: + AlertSubscription.objects.get_or_create( + user=user, alert_configuration=instance, defaults={"created_by": self.context["request"].user} + ) + + if conditions_or_threshold_changed: + # If anything changed we set inactive, so it's firing and notifying with the new settings + instance.state = "inactive" + + return super().update(instance, validated_data) + + def validate_insight(self, value): + if value and not are_alerts_supported_for_insight(value): + raise ValidationError("Alerts are not supported for this insight.") + return value + + def validate_subscribed_users(self, value): + for user in value: + if not user.teams.filter(pk=self.context["team_id"]).exists(): + raise ValidationError("User does not belong to the same organization as the alert's team.") + return value + def validate(self, attrs): if attrs.get("insight") and attrs["insight"].team.id != self.context["team_id"]: raise ValidationError({"insight": ["This insight does not belong to your team."]}) + + if attrs.get("enabled") is not False and ( + AlertConfiguration.objects.filter(team_id=self.context["team_id"], enabled=True).count() + >= AlertConfiguration.ALERTS_PER_TEAM + ): + raise ValidationError( + {"alert": [f"Your team has reached the limit of {AlertConfiguration.ALERTS_PER_TEAM} enabled alerts."]} + ) + return attrs class AlertViewSet(TeamAndOrgViewSetMixin, viewsets.ModelViewSet): scope_object = "INTERNAL" - queryset = Alert.objects.all() + queryset = AlertConfiguration.objects.all().order_by("-created_at") serializer_class = AlertSerializer def safely_get_queryset(self, queryset) -> QuerySet: @@ -39,3 +210,23 @@ def safely_get_queryset(self, queryset) -> QuerySet: if "insight" in filters: queryset = queryset.filter(insight_id=filters["insight"]) return queryset + + def retrieve(self, request, *args, **kwargs): + instance = self.get_object() + instance.checks = instance.alertcheck_set.all().order_by("-created_at")[:5] + serializer = self.get_serializer(instance) + return Response(serializer.data) + + +class ThresholdWithAlertSerializer(ThresholdSerializer): + alerts = AlertSerializer(many=True, read_only=True, source="alertconfiguration_set") + + class Meta(ThresholdSerializer.Meta): + fields = [*ThresholdSerializer.Meta.fields, "alerts"] + read_only_fields = [*ThresholdSerializer.Meta.read_only_fields, "alerts"] + + +class ThresholdViewSet(TeamAndOrgViewSetMixin, viewsets.ReadOnlyModelViewSet): + scope_object = "INTERNAL" + queryset = Threshold.objects.all() + serializer_class = ThresholdWithAlertSerializer diff --git a/posthog/api/insight.py b/posthog/api/insight.py index 0073c2e67a563..6bf91e1649672 100644 --- a/posthog/api/insight.py +++ b/posthog/api/insight.py @@ -406,7 +406,7 @@ def update(self, instance: Insight, validated_data: dict, **kwargs) -> Insight: updated_insight = super().update(instance, validated_data) if not are_alerts_supported_for_insight(updated_insight): - instance.alert_set.all().delete() + instance.alertconfiguration_set.all().delete() self._log_insight_update(before_update, dashboards_before_change, updated_insight) diff --git a/posthog/api/test/test_alert.py b/posthog/api/test/test_alert.py index 87eab7245829d..49d502256294c 100644 --- a/posthog/api/test/test_alert.py +++ b/posthog/api/test/test_alert.py @@ -28,20 +28,33 @@ def setUp(self): def test_create_and_delete_alert(self) -> None: creation_request = { "insight": self.insight["id"], - "target_value": "test@posthog.com", + "subscribed_users": [ + self.user.id, + ], "name": "alert name", - "anomaly_condition": {}, + "threshold": {"configuration": {}}, } response = self.client.post(f"/api/projects/{self.team.id}/alerts", creation_request) expected_alert_json = { + "condition": {}, + "created_at": mock.ANY, + "created_by": mock.ANY, + "enabled": True, "id": mock.ANY, - "insight": self.insight["id"], - "target_value": "test@posthog.com", + "insight": mock.ANY, + "last_notified_at": None, "name": "alert name", - "anomaly_condition": {}, + "subscribed_users": mock.ANY, + "state": "inactive", + "threshold": { + "configuration": {}, + "created_at": mock.ANY, + "id": mock.ANY, + "name": "", + }, } - assert response.status_code == status.HTTP_201_CREATED + assert response.status_code == status.HTTP_201_CREATED, response.content assert response.json() == expected_alert_json alerts = self.client.get(f"/api/projects/{self.team.id}/alerts") @@ -55,9 +68,11 @@ def test_create_and_delete_alert(self) -> None: def test_incorrect_creation(self) -> None: creation_request = { - "target_value": "test@posthog.com", + "subscribed_users": [ + self.user.id, + ], + "threshold": {"configuration": {}}, "name": "alert name", - "anomaly_condition": {}, } response = self.client.post(f"/api/projects/{self.team.id}/alerts", creation_request) assert response.status_code == status.HTTP_400_BAD_REQUEST @@ -71,9 +86,11 @@ def test_incorrect_creation(self) -> None: ).json() creation_request = { "insight": str(another_team_insight["id"]), - "target_value": "test@posthog.com", + "subscribed_users": [ + self.user.id, + ], + "threshold": {"configuration": {}}, "name": "alert name", - "anomaly_condition": {}, } response = self.client.post(f"/api/projects/{self.team.id}/alerts", creation_request) assert response.status_code == status.HTTP_400_BAD_REQUEST @@ -81,9 +98,11 @@ def test_incorrect_creation(self) -> None: def test_create_and_list_alert(self) -> None: creation_request = { "insight": self.insight["id"], - "target_value": "test@posthog.com", + "subscribed_users": [ + self.user.id, + ], + "threshold": {"configuration": {}}, "name": "alert name", - "anomaly_condition": {}, } alert = self.client.post(f"/api/projects/{self.team.id}/alerts", creation_request).json() @@ -99,15 +118,35 @@ def test_create_and_list_alert(self) -> None: assert list_for_another_insight.status_code == status.HTTP_200_OK assert len(list_for_another_insight.json()["results"]) == 0 + def test_alert_limit(self) -> None: + with mock.patch("posthog.api.alert.AlertConfiguration.ALERTS_PER_TEAM") as alert_limit: + alert_limit.__get__ = mock.Mock(return_value=1) + + creation_request = { + "insight": self.insight["id"], + "subscribed_users": [ + self.user.id, + ], + "threshold": {"configuration": {}}, + "name": "alert name", + } + self.client.post(f"/api/projects/{self.team.id}/alerts", creation_request) + + alert_2 = self.client.post(f"/api/projects/{self.team.id}/alerts", creation_request).json() + + assert alert_2["code"] == "invalid_input" + def test_alert_is_deleted_on_insight_update(self) -> None: another_insight = self.client.post( f"/api/projects/{self.team.id}/insights", data=self.default_insight_data ).json() creation_request = { "insight": another_insight["id"], - "target_value": "test@posthog.com", + "subscribed_users": [ + self.user.id, + ], + "threshold": {"configuration": {}}, "name": "alert name", - "anomaly_condition": {}, } alert = self.client.post(f"/api/projects/{self.team.id}/alerts", creation_request).json() diff --git a/posthog/management/commands/test_migrations_are_safe.py b/posthog/management/commands/test_migrations_are_safe.py index 0a921cc98d2da..41ef0df6f90db 100644 --- a/posthog/management/commands/test_migrations_are_safe.py +++ b/posthog/management/commands/test_migrations_are_safe.py @@ -42,6 +42,8 @@ def validate_migration_sql(sql) -> bool: and "CREATE TABLE" not in operation_sql and "ADD CONSTRAINT" not in operation_sql and "-- not-null-ignore" not in operation_sql + # Ignore for brand-new tables + and (table_being_altered not in tables_created_so_far or table_being_altered not in new_tables) ): print( f"\n\n\033[91mFound a non-null field or default added to an existing model. This will lock up the table while migrating. Please add 'null=True, blank=True' to the field.\nSource: `{operation_sql}`" diff --git a/posthog/migrations/0460_alertconfiguration_threshold_alertsubscription_and_more.py b/posthog/migrations/0460_alertconfiguration_threshold_alertsubscription_and_more.py new file mode 100644 index 0000000000000..f53046b41046f --- /dev/null +++ b/posthog/migrations/0460_alertconfiguration_threshold_alertsubscription_and_more.py @@ -0,0 +1,161 @@ +# Generated by Django 4.2.14 on 2024-08-26 10:25 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import django.db.models.expressions +import posthog.models.utils + + +class Migration(migrations.Migration): + dependencies = [ + ("posthog", "0459_convert_personsnode_insights_to_actorsquery"), + ] + + operations = [ + migrations.CreateModel( + name="AlertConfiguration", + fields=[ + ("created_at", models.DateTimeField(auto_now_add=True)), + ( + "id", + models.UUIDField( + default=posthog.models.utils.UUIDT, editable=False, primary_key=True, serialize=False + ), + ), + ("name", models.CharField(blank=True, max_length=255)), + ("condition", models.JSONField(default=dict)), + ( + "state", + models.CharField( + choices=[("firing", "Firing"), ("inactive", "Inactive")], default="inactive", max_length=10 + ), + ), + ("enabled", models.BooleanField(default=True)), + ("last_notified_at", models.DateTimeField(blank=True, null=True)), + ( + "created_by", + models.ForeignKey( + blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL + ), + ), + ("insight", models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to="posthog.insight")), + ], + options={ + "abstract": False, + }, + ), + migrations.CreateModel( + name="Threshold", + fields=[ + ("created_at", models.DateTimeField(auto_now_add=True)), + ( + "id", + models.UUIDField( + default=posthog.models.utils.UUIDT, editable=False, primary_key=True, serialize=False + ), + ), + ("name", models.CharField(blank=True, max_length=255)), + ("configuration", models.JSONField(default=dict)), + ( + "created_by", + models.ForeignKey( + blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL + ), + ), + ("insight", models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to="posthog.insight")), + ("team", models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to="posthog.team")), + ], + options={ + "abstract": False, + }, + ), + migrations.CreateModel( + name="AlertSubscription", + fields=[ + ("created_at", models.DateTimeField(auto_now_add=True)), + ( + "id", + models.UUIDField( + default=posthog.models.utils.UUIDT, editable=False, primary_key=True, serialize=False + ), + ), + ("subscribed", models.BooleanField(default=True)), + ( + "alert_configuration", + models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to="posthog.alertconfiguration"), + ), + ( + "created_by", + models.ForeignKey( + blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL + ), + ), + ( + "user", + models.ForeignKey( + limit_choices_to={ + "is_active": True, + "organization_id": django.db.models.expressions.OuterRef( + "alert_configuration__team__organization_id" + ), + }, + on_delete=django.db.models.deletion.CASCADE, + related_name="alert_subscriptions", + to=settings.AUTH_USER_MODEL, + ), + ), + ], + options={ + "unique_together": {("user", "alert_configuration")}, + }, + ), + migrations.AddField( + model_name="alertconfiguration", + name="subscribed_users", + field=models.ManyToManyField( + related_name="alert_configurations", through="posthog.AlertSubscription", to=settings.AUTH_USER_MODEL + ), + ), + migrations.AddField( + model_name="alertconfiguration", + name="team", + field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to="posthog.team"), + ), + migrations.AddField( + model_name="alertconfiguration", + name="threshold", + field=models.ForeignKey( + blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, to="posthog.threshold" + ), + ), + migrations.CreateModel( + name="AlertCheck", + fields=[ + ( + "id", + models.UUIDField( + default=posthog.models.utils.UUIDT, editable=False, primary_key=True, serialize=False + ), + ), + ("created_at", models.DateTimeField(auto_now_add=True)), + ("calculated_value", models.FloatField(blank=True, null=True)), + ("condition", models.JSONField(default=dict)), + ("targets_notified", models.JSONField(default=dict)), + ("error", models.JSONField(blank=True, null=True)), + ( + "state", + models.CharField( + choices=[("firing", "Firing"), ("not_met", "Not Met")], default="not_met", max_length=10 + ), + ), + ( + "alert_configuration", + models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to="posthog.alertconfiguration"), + ), + ], + options={ + "abstract": False, + }, + ), + ] diff --git a/posthog/models/__init__.py b/posthog/models/__init__.py index 36bc77625ad95..674d665288439 100644 --- a/posthog/models/__init__.py +++ b/posthog/models/__init__.py @@ -17,7 +17,7 @@ from .action.action_step import ActionStep from .activity_logging.activity_log import ActivityLog from .activity_logging.notification_viewed import NotificationViewed -from .alert import Alert +from .alert import AlertConfiguration from .annotation import Annotation from .async_deletion import AsyncDeletion, DeletionType from .async_migration import AsyncMigration, AsyncMigrationError, MigrationStatus @@ -75,7 +75,7 @@ from .user_scene_personalisation import UserScenePersonalisation __all__ = [ - "Alert", + "AlertConfiguration", "Action", "ActionStep", "ActivityLog", diff --git a/posthog/models/alert.py b/posthog/models/alert.py index 973228820c752..807bc0a0437aa 100644 --- a/posthog/models/alert.py +++ b/posthog/models/alert.py @@ -1,20 +1,213 @@ +from datetime import datetime, UTC, timedelta +from typing import Any, Optional + from django.db import models +from django.core.exceptions import ValidationError + +from posthog.hogql_queries.legacy_compatibility.flagged_conversion_manager import conversion_to_query_based from posthog.models.insight import Insight +from posthog.models.utils import UUIDModel, CreatedMetaFields +from posthog.schema import AlertCondition, InsightThreshold def are_alerts_supported_for_insight(insight: Insight) -> bool: - query = insight.query - if query is None or query.get("kind") != "TrendsQuery": - return False - if query.get("trendsFilter", {}).get("display") != "BoldNumber": - return False + with conversion_to_query_based(insight): + query = insight.query + while query.get("source"): + query = query["source"] + if query is None or query.get("kind") != "TrendsQuery": + return False + if query.get("trendsFilter", {}).get("display") != "BoldNumber": + return False return True +class ConditionValidator: + def __init__(self, threshold: Optional[InsightThreshold], condition: AlertCondition): + self.threshold = threshold + self.condition = condition + + def validate(self, calculated_value: float) -> list[str]: + validators: Any = [ + self.validate_absolute_threshold, + ] + matches = [] + for validator in validators: + matches += validator(calculated_value) + return matches + + def validate_absolute_threshold(self, calculated_value: float) -> list[str]: + if not self.threshold or not self.threshold.absoluteThreshold: + return [] + + absolute_threshold = self.threshold.absoluteThreshold + if absolute_threshold.lower is not None and calculated_value < absolute_threshold.lower: + return [f"The trend value ({calculated_value}) is below the lower threshold ({absolute_threshold.lower})"] + if absolute_threshold.upper is not None and calculated_value > absolute_threshold.upper: + return [f"The trend value ({calculated_value}) is above the upper threshold ({absolute_threshold.upper})"] + return [] + + class Alert(models.Model): + """ + @deprecated("AlertConfiguration should be used instead.") + """ + team = models.ForeignKey("Team", on_delete=models.CASCADE) insight = models.ForeignKey("posthog.Insight", on_delete=models.CASCADE) name = models.CharField(max_length=100) target_value = models.TextField() anomaly_condition = models.JSONField(default=dict) + + +class Threshold(CreatedMetaFields, UUIDModel): + """ + Threshold holds the configuration for a threshold. This can either be attached to an alert, or used as a standalone + object for other purposes. + """ + + team = models.ForeignKey("Team", on_delete=models.CASCADE) + insight = models.ForeignKey("posthog.Insight", on_delete=models.CASCADE) + + name = models.CharField(max_length=255, blank=True) + configuration = models.JSONField(default=dict) + + def clean(self): + config = InsightThreshold.model_validate(self.configuration) + if not config or not config.absoluteThreshold: + return + if config.absoluteThreshold.lower is not None and config.absoluteThreshold.upper is not None: + if config.absoluteThreshold.lower > config.absoluteThreshold.upper: + raise ValidationError("Lower threshold must be less than upper threshold") + + +class AlertConfiguration(CreatedMetaFields, UUIDModel): + ALERTS_PER_TEAM = 10 + + team = models.ForeignKey("Team", on_delete=models.CASCADE) + insight = models.ForeignKey("posthog.Insight", on_delete=models.CASCADE) + + name = models.CharField(max_length=255, blank=True) + subscribed_users = models.ManyToManyField( + "posthog.User", + through="posthog.AlertSubscription", + through_fields=("alert_configuration", "user"), + related_name="alert_configurations", + ) + + # The threshold to evaluate the alert against. If null, the alert must have other conditions to trigger. + threshold = models.ForeignKey(Threshold, on_delete=models.CASCADE, null=True, blank=True) + condition = models.JSONField(default=dict) + + STATE_CHOICES = [ + ("firing", "Firing"), + ("inactive", "Inactive"), + ] + state = models.CharField(max_length=10, choices=STATE_CHOICES, default="inactive") + enabled = models.BooleanField(default=True) + + last_notified_at = models.DateTimeField(null=True, blank=True) + + def __str__(self): + return f"{self.name} (Team: {self.team})" + + def save(self, *args, **kwargs): + if not self.enabled: + # When disabling an alert, set the state to inactive + self.state = "inactive" + if "update_fields" in kwargs: + kwargs["update_fields"].append("state") + + super().save(*args, **kwargs) + + def evaluate_condition(self, calculated_value) -> list[str]: + threshold = InsightThreshold.model_validate(self.threshold.configuration) if self.threshold else None + condition = AlertCondition.model_validate(self.condition) + validator = ConditionValidator(threshold=threshold, condition=condition) + return validator.validate(calculated_value) + + def add_check( + self, *, calculated_value: Optional[float], error: Optional[dict] = None + ) -> tuple["AlertCheck", list[str]]: + """Add a new AlertCheck, managing state transitions and cooldown.""" + matches = self.evaluate_condition(calculated_value) if calculated_value is not None else [] + targets_notified = {} + + # Determine the appropriate state for this check + if matches: + if self.state != "firing": + # Transition to firing state and send a notification + check_state = "firing" + self.last_notified_at = datetime.now(UTC) + targets_notified = {"users": list(self.subscribed_users.all().values_list("email", flat=True))} + else: + check_state = "firing" # Already firing, no new notification + matches = [] # Don't send duplicate notifications + else: + check_state = "not_met" + self.state = "inactive" # Set the Alert to inactive if the threshold is no longer met + # Optionally send a resolved notification + + alert_check = AlertCheck.objects.create( + alert_configuration=self, + calculated_value=calculated_value, + condition=self.condition, + targets_notified=targets_notified, + state=check_state, + error=error, + ) + + # Update the Alert state + if check_state == "firing": + self.state = "firing" + elif check_state == "not_met": + self.state = "inactive" + + self.save() + return alert_check, matches + + +class AlertSubscription(CreatedMetaFields, UUIDModel): + user = models.ForeignKey( + "User", + on_delete=models.CASCADE, + limit_choices_to={ + "is_active": True, + "organization_id": models.OuterRef("alert_configuration__team__organization_id"), + }, + related_name="alert_subscriptions", + ) + alert_configuration = models.ForeignKey(AlertConfiguration, on_delete=models.CASCADE) + subscribed = models.BooleanField(default=True) + + def __str__(self): + return f"AlertSubscription for {self.alert_configuration.name} by {self.user.email}" + + class Meta: + unique_together = ["user", "alert_configuration"] + + +class AlertCheck(UUIDModel): + alert_configuration = models.ForeignKey(AlertConfiguration, on_delete=models.CASCADE) + created_at = models.DateTimeField(auto_now_add=True) + calculated_value = models.FloatField(null=True, blank=True) + condition = models.JSONField(default=dict) # Snapshot of the condition at the time of the check + targets_notified = models.JSONField(default=dict) + error = models.JSONField(null=True, blank=True) + + STATE_CHOICES = [ + ("firing", "Firing"), + ("not_met", "Not Met"), + ] + state = models.CharField(max_length=10, choices=STATE_CHOICES, default="not_met") + + def __str__(self): + return f"AlertCheck for {self.alert_configuration.name} at {self.created_at}" + + @classmethod + def clean_up_old_checks(cls) -> int: + retention_days = 14 + oldest_allowed_date = datetime.now(UTC) - timedelta(days=retention_days) + rows_count, _ = cls.objects.filter(created_at__lt=oldest_allowed_date).delete() + return rows_count diff --git a/posthog/schema.py b/posthog/schema.py index 56720edb0fae7..9a4055e6ae3e7 100644 --- a/posthog/schema.py +++ b/posthog/schema.py @@ -12,14 +12,6 @@ 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 @@ -36,11 +28,43 @@ class AggregationAxisFormat(StrEnum): PERCENTAGE_SCALED = "percentage_scaled" -class AnomalyCondition(BaseModel): +class AlertCheck(BaseModel): model_config = ConfigDict( extra="forbid", ) - absoluteThreshold: AbsoluteThreshold + calculated_value: float + created_at: str + id: str + state: str + targets_notified: bool + + +class AlertCondition(BaseModel): + pass + model_config = ConfigDict( + extra="forbid", + ) + + +class AlertTypeBase(BaseModel): + model_config = ConfigDict( + extra="forbid", + ) + condition: AlertCondition + enabled: bool + insight: float + name: str + + +class AlertTypeWrite(BaseModel): + model_config = ConfigDict( + extra="forbid", + ) + condition: AlertCondition + enabled: bool + insight: float + name: str + subscribed_users: list[int] class Kind(StrEnum): @@ -560,6 +584,19 @@ class GoalLine(BaseModel): value: float +class HedgehogColorOptions(StrEnum): + GREEN = "green" + RED = "red" + BLUE = "blue" + PURPLE = "purple" + DARK = "dark" + LIGHT = "light" + SEPIA = "sepia" + INVERT = "invert" + INVERT_HUE = "invert-hue" + GREYSCALE = "greyscale" + + class HogLanguage(StrEnum): HOG = "hog" HOG_JSON = "hogJson" @@ -701,6 +738,14 @@ class InsightNodeKind(StrEnum): LIFECYCLE_QUERY = "LifecycleQuery" +class InsightsThresholdAbsolute(BaseModel): + model_config = ConfigDict( + extra="forbid", + ) + lower: Optional[float] = None + upper: Optional[float] = None + + class IntervalType(StrEnum): MINUTE = "minute" HOUR = "hour" @@ -716,6 +761,15 @@ class LifecycleToggle(StrEnum): DORMANT = "dormant" +class MinimalHedgehogConfig(BaseModel): + model_config = ConfigDict( + extra="forbid", + ) + accessories: list[str] + color: Optional[HedgehogColorOptions] = None + use_as_profile: bool + + class MultipleBreakdownType(StrEnum): PERSON = "person" EVENT = "event" @@ -1258,6 +1312,20 @@ class TrendsQueryResponse(BaseModel): ) +class UserBasicType(BaseModel): + model_config = ConfigDict( + extra="forbid", + ) + distinct_id: str + email: str + first_name: str + hedgehog_config: Optional[MinimalHedgehogConfig] = None + id: float + is_email_verified: Optional[Any] = None + last_name: Optional[str] = None + uuid: str + + class ActionsPie(BaseModel): model_config = ConfigDict( extra="forbid", @@ -2474,6 +2542,13 @@ class InsightActorsQueryBase(BaseModel): response: Optional[ActorsQueryResponse] = None +class InsightThreshold(BaseModel): + model_config = ConfigDict( + extra="forbid", + ) + absoluteThreshold: Optional[InsightsThresholdAbsolute] = None + + class LifecycleFilter(BaseModel): model_config = ConfigDict( extra="forbid", @@ -3315,6 +3390,31 @@ class WebTopClicksQuery(BaseModel): useSessionsTable: Optional[bool] = None +class Threshold(BaseModel): + model_config = ConfigDict( + extra="forbid", + ) + configuration: InsightThreshold + + +class AlertType(BaseModel): + model_config = ConfigDict( + extra="forbid", + ) + checks: list[AlertCheck] + condition: AlertCondition + created_at: str + created_by: UserBasicType + enabled: bool + id: str + insight: float + last_notified_at: str + name: str + state: str + subscribed_users: list[UserBasicType] + threshold: Threshold + + class AnyResponseType( RootModel[ Union[ diff --git a/posthog/tasks/alerts/checks.py b/posthog/tasks/alerts/checks.py index e5c34f578a048..cbc4fb4604da5 100644 --- a/posthog/tasks/alerts/checks.py +++ b/posthog/tasks/alerts/checks.py @@ -1,107 +1,138 @@ +from datetime import datetime, timedelta, UTC +from typing import Optional + from celery import shared_task -from celery.canvas import group, chain +from celery.canvas import chain +from django.db import transaction from django.utils import timezone -import math import structlog +from sentry_sdk import capture_exception 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.errors import CHQueryErrorTooManySimultaneousQueries from posthog.hogql_queries.legacy_compatibility.flagged_conversion_manager import ( conversion_to_query_based, ) -from posthog.models import Alert -from posthog.schema import AnomalyCondition +from posthog.models import AlertConfiguration, Team +from posthog.models.alert import AlertCheck +from posthog.tasks.utils import CeleryQueue logger = structlog.get_logger(__name__) def check_all_alerts() -> None: - alert_ids = list(Alert.objects.all().values_list("id", flat=True)) + # TODO: Consider aligning insight calculation with cache warming of insights, see warming.py + # Currently it's implicitly aligned by alerts obviously also using cache if available + + # Use a fixed expiration time since tasks in the chain are executed sequentially + expire_after = datetime.now(UTC) + timedelta(minutes=30) + + teams = Team.objects.filter(alertconfiguration__isnull=False).distinct() - 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)) + for team in teams: + alert_ids = list(AlertConfiguration.objects.filter(team=team, enabled=True).values_list("id", flat=True)) - 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) + # We chain the task execution to prevent queries *for a single team* running at the same time + chain(*(check_alert_task.si(str(alert_id)).set(expires=expire_after) for alert_id in alert_ids))() - group(groups).apply_async() +@transaction.atomic +def check_alert(alert_id: str) -> None: + try: + alert = AlertConfiguration.objects.get(id=alert_id, enabled=True) + except AlertConfiguration.DoesNotExist: + logger.warning("Alert not found or not enabled", alert_id=alert_id) + return -def check_alert(alert_id: int) -> None: - alert = Alert.objects.get(pk=alert_id) insight = alert.insight + error: Optional[dict] = None + + try: + 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}") + + result = calculation_result.result[0] + aggregated_value = result["aggregated_value"] + except Exception as e: + event_id = capture_exception(e) + error = { + "sentry_event_id": event_id, + "message": str(e), + } + aggregated_value = None + + # Lock alert to prevent concurrent state changes + alert = AlertConfiguration.objects.select_for_update().get(id=alert_id, enabled=True) + check, matches = alert.add_check(calculated_value=aggregated_value, error=error) + + if not check.state == "firing": + logger.info("Check state is %s", check.state, alert_id=alert.id) + return - 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) + if not matches: + # We might be firing but have no (new) matches to notify about return - send_notifications(alert, anomalies_descriptions) + send_notifications(alert, matches) -@shared_task(ignore_result=True) +@shared_task( + ignore_result=True, + expires=60 * 60, +) def check_all_alerts_task() -> None: check_all_alerts() -@shared_task(ignore_result=True) -def check_alert_task(alert_id: int) -> None: +@shared_task( + ignore_result=True, + queue=CeleryQueue.ANALYTICS_LIMITED.value, # Important! Prevents Clickhouse from being overwhelmed + autoretry_for=(CHQueryErrorTooManySimultaneousQueries,), + retry_backoff=1, + retry_backoff_max=10, + max_retries=10, + expires=60 * 60, +) +def check_alert_task(alert_id: str) -> None: check_alert(alert_id) -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()}" +@shared_task(ignore_result=True) +def checks_cleanup_task() -> None: + AlertCheck.clean_up_old_checks() + + +def send_notifications(alert: AlertConfiguration, matches: list[str]) -> None: + subject = f"PostHog alert {alert.name} is firing" + campaign_key = f"alert-firing-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_name="alert_check_firing", template_context={ - "anomalies_descriptions": anomalies_descriptions, + "match_descriptions": matches, "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(","))) + targets = alert.subscribed_users.all().values_list("email", flat=True) 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) + logger.info(f"Send notifications about {len(matches)} anomalies", alert_id=alert.id) message.send() diff --git a/posthog/tasks/alerts/test/test_checks.py b/posthog/tasks/alerts/test/test_checks.py index fb5f93b3cb166..2504c90dc3a3f 100644 --- a/posthog/tasks/alerts/test/test_checks.py +++ b/posthog/tasks/alerts/test/test_checks.py @@ -1,16 +1,16 @@ -import pytest from typing import Optional from unittest.mock import MagicMock, patch from freezegun import freeze_time +from posthog.models.alert import AlertCheck from posthog.models.instance_setting import set_instance_setting 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 +from posthog.models import AlertConfiguration @freeze_time("2024-06-02T08:55:00.000Z") @@ -41,20 +41,29 @@ def setUp(self) -> None: data={ "name": "alert name", "insight": self.insight["id"], - "target_value": "a@b.c,d@e.f", - "anomaly_condition": {"absoluteThreshold": {}}, + "subscribed_users": [self.user.id], + "threshold": {"configuration": {"absoluteThreshold": {}}}, }, ).json() 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}}}, + data={"threshold": {"configuration": {"absoluteThreshold": {"lower": lower, "upper": upper}}}}, ) 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_not_triggered_when_disabled(self, mock_send_notifications: MagicMock) -> None: + self.set_thresholds(lower=1) + + self.client.patch(f"/api/projects/{self.team.id}/alerts/{self.alert['id']}", data={"enabled": False}) + + check_alert(self.alert["id"]) + + assert mock_send_notifications.call_count == 0 + def test_alert_is_triggered_for_values_above_higher_threshold(self, mock_send_notifications: MagicMock) -> None: self.set_thresholds(upper=0) @@ -70,7 +79,7 @@ def test_alert_is_triggered_for_values_above_higher_threshold(self, mock_send_no assert mock_send_notifications.call_count == 1 alert = mock_send_notifications.call_args_list[0].args[0] - assert alert.id == self.alert["id"] + assert str(alert.id) == self.alert["id"] anomalies_descriptions = self.get_anomalies_descriptions(mock_send_notifications, call_index=0) assert len(anomalies_descriptions) == 1 @@ -100,6 +109,77 @@ def test_alert_is_triggered_for_value_below_lower_threshold(self, mock_send_noti 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_triggers_but_does_not_send_notification_during_firing( + self, mock_send_notifications: MagicMock + ) -> None: + self.set_thresholds(lower=1) + + check_alert(self.alert["id"]) + + assert mock_send_notifications.call_count == 1 + assert AlertCheck.objects.filter(alert_configuration=self.alert["id"]).latest("created_at").state == "firing" + + with freeze_time("2024-06-02T09:00:00.000Z"): + check_alert(self.alert["id"]) + + assert mock_send_notifications.call_count == 1 + assert ( + AlertCheck.objects.filter(alert_configuration=self.alert["id"]).latest("created_at").state == "firing" + ) + + with freeze_time("2024-06-02T09:55:00.000Z"): + self.set_thresholds(lower=0) + + check_alert(self.alert["id"]) + + assert mock_send_notifications.call_count == 1 + assert ( + AlertCheck.objects.filter(alert_configuration=self.alert["id"]).latest("created_at").state == "not_met" + ) + + with freeze_time("2024-06-02T11:00:00.000Z"): + self.set_thresholds(lower=1) + + check_alert(self.alert["id"]) + + assert mock_send_notifications.call_count == 2 + assert ( + AlertCheck.objects.filter(alert_configuration=self.alert["id"]).latest("created_at").state == "firing" + ) + + # test clean up old checks (> 14 days) + with freeze_time("2024-06-20T11:00:00.000Z"): + AlertCheck.clean_up_old_checks() + assert AlertCheck.objects.filter(alert_configuration=self.alert["id"]).count() == 0 + + def test_alert_is_set_to_inactive_when_disabled(self, mock_send_notifications: MagicMock) -> None: + self.set_thresholds(lower=1) + + check_alert(self.alert["id"]) + + assert mock_send_notifications.call_count == 1 + assert AlertCheck.objects.filter(alert_configuration=self.alert["id"]).latest("created_at").state == "firing" + + self.client.patch(f"/api/projects/{self.team.id}/alerts/{self.alert['id']}", data={"enabled": False}) + + # Check that the alert is set to inactive and checks are not triggered + check_alert(self.alert["id"]) + + assert mock_send_notifications.call_count == 1 + assert AlertConfiguration.objects.get(pk=self.alert["id"]).state == "inactive" + + def test_alert_is_set_to_inactive_when_threshold_changes(self, mock_send_notifications: MagicMock) -> None: + self.set_thresholds(lower=1) + + check_alert(self.alert["id"]) + + assert mock_send_notifications.call_count == 1 + assert AlertCheck.objects.filter(alert_configuration=self.alert["id"]).latest("created_at").state == "firing" + + self.set_thresholds(lower=2) + + assert AlertConfiguration.objects.get(pk=self.alert["id"]).state == "inactive" + def test_alert_is_not_triggered_for_normal_values(self, mock_send_notifications: MagicMock) -> None: self.set_thresholds(lower=0, upper=1) @@ -124,12 +204,30 @@ def test_error_while_calculating_no_alert(self, mock_send_notifications: MagicMo } )[1] - self.client.patch(f"/api/projects/{self.team.id}/alerts/{self.alert['id']}", data={"insight": insight["id"]}) + # Change with ORM to bypass API validation + AlertConfiguration.objects.filter(pk=self.alert["id"]).update(insight=insight["id"]) - with pytest.raises(KeyError): - check_alert(self.alert["id"]) + check_alert(self.alert["id"]) assert mock_send_notifications.call_count == 0 + latest_alert_check = AlertCheck.objects.filter(alert_configuration=self.alert["id"]).latest("created_at") + assert latest_alert_check.error["message"] == "'aggregated_value'" + + # mock calculate_for_query_based_insight to raise a different exception + with patch( + "posthog.tasks.alerts.checks.calculate_for_query_based_insight" + ) as mock_calculate_for_query_based_insight: + mock_calculate_for_query_based_insight.side_effect = Exception("Some error") + + with freeze_time("2024-06-02T09:00:00.000Z"): + check_alert(self.alert["id"]) + assert mock_send_notifications.call_count == 0 + + latest_alert_check = AlertCheck.objects.filter(alert_configuration=self.alert["id"]).latest( + "created_at" + ) + assert latest_alert_check.error["message"] == "Some error" + 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"}} @@ -147,13 +245,12 @@ def test_alert_with_insight_with_filter(self, mock_send_notifications: MagicMock @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"]) + alert = AlertConfiguration.objects.get(pk=self.alert["id"]) send_notifications(alert, ["first anomaly description", "second anomaly description"]) assert len(mocked_email_messages) == 1 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 len(email.to) == 1 + assert email.to[0]["recipient"] == "user1@posthog.com" assert "first anomaly description" in email.html_body assert "second anomaly description" in email.html_body diff --git a/posthog/tasks/scheduled.py b/posthog/tasks/scheduled.py index 53127d9ce726d..df765689fb362 100644 --- a/posthog/tasks/scheduled.py +++ b/posthog/tasks/scheduled.py @@ -8,7 +8,7 @@ from posthog.caching.warming import schedule_warming_for_teams_task from posthog.celery import app -from posthog.tasks.alerts.checks import check_all_alerts_task +from posthog.tasks.alerts.checks import check_all_alerts_task, checks_cleanup_task from posthog.tasks.integrations import refresh_integrations from posthog.tasks.tasks import ( calculate_cohort, @@ -244,9 +244,15 @@ def setup_periodic_tasks(sender: Celery, **kwargs: Any) -> None: ) sender.add_periodic_task( - crontab(hour="*", minute="20"), + crontab(hour="*", minute="45"), check_all_alerts_task.s(), - name="detect alerts' anomalies and notify about them", + name="check alerts for matches and send notifications", + ) + + sender.add_periodic_task( + crontab(hour="8", minute="0"), + checks_cleanup_task.s(), + name="clean up old alert checks", ) if settings.EE_AVAILABLE: diff --git a/posthog/templates/email/alert_anomaly.html b/posthog/templates/email/alert_anomaly.html deleted file mode 100644 index 49636488288dc..0000000000000 --- a/posthog/templates/email/alert_anomaly.html +++ /dev/null @@ -1,10 +0,0 @@ -{% 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 %} diff --git a/posthog/templates/email/alert_check_firing.html b/posthog/templates/email/alert_check_firing.html new file mode 100644 index 0000000000000..52b10b504e175 --- /dev/null +++ b/posthog/templates/email/alert_check_firing.html @@ -0,0 +1,14 @@ +{% extends "email/base.html" %} {% load posthog_assets %} {% block section %} +

+ The {{ alert_name }} alert is firing for {{ insight_name }}: +

+
    + {% for item in match_descriptions %} +
  • {{ item }}
    • + {% endfor %} +
+
+ Manage alert +
+{% endblock %}{% load posthog_filters %}