Skip to content

Commit

Permalink
ref(alerts): Removed unused code (#79700)
Browse files Browse the repository at this point in the history
Remove unused options (`include_all_projects` and `excluded_projects`)
for alert rules - there are no rules in existence that use these and
they're not published in the API docs. We'll be addressing what these
options intended to cover with ACI anyway, so it's nicer to remove them
now so I don't have to remember what to ignore as we make changes.
  • Loading branch information
ceorourke authored Oct 30, 2024
1 parent 1587e29 commit c8639b8
Show file tree
Hide file tree
Showing 10 changed files with 18 additions and 348 deletions.
121 changes: 2 additions & 119 deletions src/sentry/incidents/logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
AlertRuleActivity,
AlertRuleActivityType,
AlertRuleDetectionType,
AlertRuleExcludedProjects,
AlertRuleMonitorTypeInt,
AlertRuleProjects,
AlertRuleSeasonality,
Expand All @@ -38,7 +37,6 @@
AlertRuleThresholdType,
AlertRuleTrigger,
AlertRuleTriggerAction,
AlertRuleTriggerExclusion,
)
from sentry.incidents.models.alert_rule_activations import (
AlertRuleActivationCondition,
Expand Down Expand Up @@ -517,8 +515,6 @@ def create_alert_rule(
owner: Actor | None = None,
resolve_threshold: int | float | None = None,
environment: Environment | None = None,
include_all_projects: bool = False,
excluded_projects: Collection[Project] | None = None,
query_type: SnubaQuery.Type = SnubaQuery.Type.ERROR,
dataset: Dataset = Dataset.Events,
user: RpcUser | None = None,
Expand All @@ -536,8 +532,7 @@ def create_alert_rule(
Creates an alert rule for an organization.
:param organization:
:param projects: A list of projects to subscribe to the rule. This will be overridden
if `include_all_projects` is True
:param projects: A list of projects to subscribe to the rule
:param name: Name for the alert rule. This will be used as part of the
incident name, and must be unique per project
:param owner: Actor (sentry.types.actor.Actor) or None
Expand All @@ -550,10 +545,6 @@ def create_alert_rule(
subscription needs to exceed the threshold before triggering
:param resolve_threshold: Optional value that the subscription needs to reach to
resolve the alert
:param include_all_projects: Whether to include all current and future projects
from this organization
:param excluded_projects: List of projects to exclude if we're using
`include_all_projects`.
:param query_type: The SnubaQuery.Type of the query
:param dataset: The dataset that this query will be executed on
:param event_types: List of `EventType` that this alert will be related to
Expand Down Expand Up @@ -632,7 +623,6 @@ def create_alert_rule(
threshold_type=threshold_type.value,
resolve_threshold=resolve_threshold,
threshold_period=threshold_period,
include_all_projects=include_all_projects,
comparison_delta=comparison_delta,
monitor_type=monitor_type,
description=description,
Expand All @@ -642,20 +632,6 @@ def create_alert_rule(
**_owner_kwargs_from_actor(owner),
)

if include_all_projects:
# NOTE: This feature is not currently utilized.
excluded_projects = excluded_projects or ()
projects = list(
Project.objects.filter(organization=organization).exclude(
id__in=[p.id for p in excluded_projects]
)
)
exclusions = [
AlertRuleExcludedProjects(alert_rule=alert_rule, project=project)
for project in excluded_projects
]
AlertRuleExcludedProjects.objects.bulk_create(exclusions)

if alert_rule.detection_type == AlertRuleDetectionType.DYNAMIC.value:
# NOTE: if adding a new metric alert type, take care to check that it's handled here
send_new_rule_data(alert_rule, projects[0], snuba_query)
Expand Down Expand Up @@ -692,7 +668,6 @@ def create_alert_rule(
)

schedule_update_project_config(alert_rule, projects)

return alert_rule


Expand Down Expand Up @@ -761,8 +736,6 @@ def update_alert_rule(
threshold_type: AlertRuleThresholdType | None = None,
threshold_period: int | None = None,
resolve_threshold: int | float | NotSet = NOT_SET,
include_all_projects: bool | None = None,
excluded_projects: Iterable[Project] | None = None,
user: RpcUser | None = None,
event_types: Collection[SnubaQueryEventType.EventType] | None = None,
comparison_delta: int | None | NotSet = NOT_SET,
Expand All @@ -777,8 +750,6 @@ def update_alert_rule(
Updates an alert rule.
:param alert_rule: The alert rule to update
:param excluded_projects: List of projects to subscribe to the rule. Ignored if
`include_all_projects` is True
:param name: Name for the alert rule. This will be used as part of the
incident name, and must be unique per project.
:param owner: Actor (sentry.types.actor.Actor) or None
Expand All @@ -791,10 +762,6 @@ def update_alert_rule(
subscription needs to exceed the threshold before triggering
:param resolve_threshold: Optional value that the subscription needs to reach to
resolve the alert
:param include_all_projects: Whether to include all current and future projects
from this organization
:param excluded_projects: List of projects to exclude if we're using
`include_all_projects`. Ignored otherwise.
:param event_types: List of `EventType` that this alert will be related to
:param comparison_delta: An optional int representing the time delta to use to determine the
comparison period. In minutes.
Expand Down Expand Up @@ -829,8 +796,6 @@ def update_alert_rule(
updated_fields["resolve_threshold"] = resolve_threshold
if threshold_period:
updated_fields["threshold_period"] = threshold_period
if include_all_projects is not None:
updated_fields["include_all_projects"] = include_all_projects
if dataset is not None:
if dataset.value != snuba_query.dataset:
updated_query_fields["dataset"] = dataset
Expand Down Expand Up @@ -975,47 +940,13 @@ def update_alert_rule(
or aggregate is not None
or time_window is not None
or projects is not None
or include_all_projects is not None
or excluded_projects is not None
):
existing_subs = snuba_query.subscriptions.all().select_related("project")

new_projects: Iterable[Project] = ()
deleted_subs: Iterable[QuerySubscription] = ()

if not alert_rule.include_all_projects:
# We don't want to have any exclusion rows present if we're not in
# `include_all_projects` mode
get_excluded_projects_for_alert_rule(alert_rule).delete()

if alert_rule.include_all_projects:
# NOTE: This feature is not currently utilized.
if include_all_projects or excluded_projects is not None:
# If we're in `include_all_projects` mode, we want to just fetch
# projects that aren't already subscribed, and haven't been excluded so
# we can add them.
excluded_project_ids = (
{p.id for p in excluded_projects} if excluded_projects else set()
)
project_exclusions = get_excluded_projects_for_alert_rule(alert_rule)
project_exclusions.exclude(project_id__in=excluded_project_ids).delete()
existing_excluded_project_ids = {pe.project_id for pe in project_exclusions}
new_exclusions = [
AlertRuleExcludedProjects(alert_rule=alert_rule, project_id=project_id)
for project_id in excluded_project_ids
if project_id not in existing_excluded_project_ids
]
AlertRuleExcludedProjects.objects.bulk_create(new_exclusions)

new_projects = Project.objects.filter(organization=organization).exclude(
id__in={sub.project_id for sub in existing_subs} | excluded_project_ids
)
# If we're subscribed to any of the excluded projects then we want to
# remove those subscriptions
deleted_subs = [
sub for sub in existing_subs if sub.project_id in excluded_project_ids
]
elif projects is not None:
if projects is not None:
# All project slugs that currently exist for the alert rule
existing_project_slugs = {sub.project.slug for sub in existing_subs}

Expand Down Expand Up @@ -1135,12 +1066,6 @@ def delete_alert_rule(
tasks.auto_resolve_snapshot_incidents.apply_async(kwargs={"alert_rule_id": alert_rule.id})


def get_excluded_projects_for_alert_rule(
alert_rule: AlertRule,
) -> QuerySet[AlertRuleExcludedProjects]:
return AlertRuleExcludedProjects.objects.filter(alert_rule=alert_rule)


class AlertRuleTriggerLabelAlreadyUsedError(Exception):
pass

Expand All @@ -1158,15 +1083,13 @@ def create_alert_rule_trigger(
alert_rule: AlertRule,
label: str,
alert_threshold: int | float,
excluded_projects: Collection[Project] = (),
) -> AlertRuleTrigger:
"""
Creates a new AlertRuleTrigger
:param alert_rule: The alert rule to create the trigger for
:param label: A description of the trigger
:param alert_threshold: Value that the subscription needs to reach to trigger the
alert rule
:param excluded_projects: A list of Projects that should be excluded from this
trigger. These projects must be associate with the alert rule already
:return: The created AlertRuleTrigger
"""
Expand All @@ -1176,37 +1099,23 @@ def create_alert_rule_trigger(
if alert_rule.detection_type == AlertRuleDetectionType.DYNAMIC and alert_threshold != 0:
raise ValidationError(INVALID_ALERT_THRESHOLD)

excluded_subs: Iterable[QuerySubscription] = ()
if excluded_projects:
excluded_subs = _get_subscriptions_from_alert_rule(alert_rule, excluded_projects)

with transaction.atomic(router.db_for_write(AlertRuleTrigger)):
trigger = AlertRuleTrigger.objects.create(
alert_rule=alert_rule, label=label, alert_threshold=alert_threshold
)
if excluded_subs:
new_exclusions = [
AlertRuleTriggerExclusion(alert_rule_trigger=trigger, query_subscription=sub)
for sub in excluded_subs
]
AlertRuleTriggerExclusion.objects.bulk_create(new_exclusions)

return trigger


def update_alert_rule_trigger(
trigger: AlertRuleTrigger,
label: str | None = None,
alert_threshold: int | float | None = None,
excluded_projects: Collection[Project] = (),
) -> AlertRuleTrigger:
"""
:param trigger: The AlertRuleTrigger to update
:param label: A description of the trigger
:param alert_threshold: Value that the subscription needs to reach to trigger the
alert rule
:param excluded_projects: A list of Projects that should be excluded from this
trigger. These projects must be associate with the alert rule already
:return: The updated AlertRuleTrigger
"""

Expand All @@ -1226,36 +1135,10 @@ def update_alert_rule_trigger(
if alert_threshold is not None:
updated_fields["alert_threshold"] = alert_threshold

deleted_exclusion_ids = []
new_subs = []

if excluded_projects:
# We link projects to exclusions via QuerySubscriptions. Calculate which
# exclusions need to be deleted, and which need to be created.
excluded_subs = _get_subscriptions_from_alert_rule(trigger.alert_rule, excluded_projects)
existing_exclusions = AlertRuleTriggerExclusion.objects.filter(alert_rule_trigger=trigger)
new_sub_ids = {sub.id for sub in excluded_subs}
existing_sub_ids = {exclusion.query_subscription_id for exclusion in existing_exclusions}

deleted_exclusion_ids = [
e.id for e in existing_exclusions if e.query_subscription_id not in new_sub_ids
]
new_subs = [sub for sub in excluded_subs if sub.id not in existing_sub_ids]

with transaction.atomic(router.db_for_write(AlertRuleTrigger)):
if updated_fields:
trigger.update(**updated_fields)

if deleted_exclusion_ids:
AlertRuleTriggerExclusion.objects.filter(id__in=deleted_exclusion_ids).delete()

if new_subs:
new_exclusions = [
AlertRuleTriggerExclusion(alert_rule_trigger=trigger, query_subscription=sub)
for sub in new_subs
]
AlertRuleTriggerExclusion.objects.bulk_create(new_exclusions)

return trigger


Expand Down
23 changes: 1 addition & 22 deletions src/sentry/incidents/receivers.py
Original file line number Diff line number Diff line change
@@ -1,30 +1,9 @@
from datetime import datetime, timezone

from django.db.models.signals import post_save, pre_save
from django.db.models.signals import pre_save
from django.dispatch import receiver

from sentry.incidents.models.alert_rule import AlertRule
from sentry.incidents.models.incident import IncidentTrigger
from sentry.models.project import Project


@receiver(post_save, sender=Project, weak=False)
def add_project_to_include_all_rules(instance, created, **kwargs):
"""
If an alert rule is created for an org that should include all projects
When a Project model is saved, it will be subscribed to the AlertRule
NOTE: This feature is not currently utilized and may break the UX flow
"""
if not created:
return

alert_rules = AlertRule.objects.filter(
organization=instance.organization, include_all_projects=True
)
for alert_rule in alert_rules:
# NOTE: defaults to only subscribe if AlertRule.monitor_type === 'CONTINUOUS'
alert_rule.subscribe_projects(projects=[instance])


@receiver(pre_save, sender=IncidentTrigger)
Expand Down
10 changes: 2 additions & 8 deletions src/sentry/testutils/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -1557,9 +1557,7 @@ def create_alert_rule(
aggregate="count()",
time_window=10,
threshold_period=1,
include_all_projects=False,
environment=None,
excluded_projects=None,
date_added=None,
query_type=None,
dataset=Dataset.Events,
Expand Down Expand Up @@ -1595,8 +1593,6 @@ def create_alert_rule(
query_type=query_type,
dataset=dataset,
environment=environment,
include_all_projects=include_all_projects,
excluded_projects=excluded_projects,
user=user,
event_types=event_types,
comparison_delta=comparison_delta,
Expand Down Expand Up @@ -1637,13 +1633,11 @@ def create_alert_rule_activation(

@staticmethod
@assume_test_silo_mode(SiloMode.REGION)
def create_alert_rule_trigger(
alert_rule, label=None, alert_threshold=100, excluded_projects=None
):
def create_alert_rule_trigger(alert_rule, label=None, alert_threshold=100):
if not label:
label = petname.generate(2, " ", letters=10).title()

return create_alert_rule_trigger(alert_rule, label, alert_threshold, excluded_projects)
return create_alert_rule_trigger(alert_rule, label, alert_threshold)

@staticmethod
@assume_test_silo_mode(SiloMode.REGION)
Expand Down
14 changes: 10 additions & 4 deletions src/sentry/testutils/helpers/backups.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,11 @@
from sentry.backup.validate import validate
from sentry.data_secrecy.models import DataSecrecyWaiver
from sentry.db.models.paranoia import ParanoidModel
from sentry.incidents.models.alert_rule import AlertRuleMonitorTypeInt
from sentry.incidents.models.alert_rule import (
AlertRuleExcludedProjects,
AlertRuleMonitorTypeInt,
AlertRuleTriggerExclusion,
)
from sentry.incidents.models.incident import (
IncidentActivity,
IncidentSnapshot,
Expand Down Expand Up @@ -477,13 +481,15 @@ def create_exhaustive_organization(
alert = self.create_alert_rule(
organization=org,
projects=[project],
include_all_projects=True,
excluded_projects=[other_project],
user=owner,
)
AlertRuleExcludedProjects.objects.create(alert_rule=alert, project=other_project)
alert.user_id = owner_id
alert.save()
trigger = self.create_alert_rule_trigger(alert_rule=alert, excluded_projects=[project])
trigger = self.create_alert_rule_trigger(alert_rule=alert)
AlertRuleTriggerExclusion.objects.create(
alert_rule_trigger=trigger, query_subscription=alert.snuba_query.subscriptions.first()
)
self.create_alert_rule_trigger_action(alert_rule_trigger=trigger)
activated_alert = self.create_alert_rule(
organization=org,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
created: '2024-10-21T17:30:17.571525+00:00'
created: '2024-10-28T18:43:14.722559+00:00'
creator: sentry
source: tests/sentry/backup/test_sanitize.py
---
Expand Down Expand Up @@ -606,16 +606,6 @@ source: tests/sentry/backup/test_sanitize.py
sanitized_fields:
- date_added
- date_updated
- model_name: sentry.querysubscription
ordinal: 4
sanitized_fields:
- date_added
- date_updated
- model_name: sentry.querysubscription
ordinal: 5
sanitized_fields:
- date_added
- date_updated
- model_name: sentry.projectteam
ordinal: 1
sanitized_fields: []
Expand Down
Loading

0 comments on commit c8639b8

Please sign in to comment.