From a452b23b04a854931936980278b59b3e133fb99e Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 12 Sep 2023 14:18:58 -0700 Subject: [PATCH] ref(rules): Migrate 'bad' rules to be disabled (#55941) Migrate alert rules that have no action or are exact duplicates of another rule in the same project to be disabled. We now prevent these from being created, this will just clean up old ones. Closes #55502 --- migrations_lockfile.txt | 2 +- ...550_migrate_no_action_dupe_issue_alerts.py | 92 +++++++++++++++++++ src/sentry/testutils/factories.py | 41 +++++---- ...550_migrate_no_action_dupe_issue_alerts.py | 25 +++++ 4 files changed, 142 insertions(+), 18 deletions(-) create mode 100644 src/sentry/migrations/0550_migrate_no_action_dupe_issue_alerts.py create mode 100644 tests/sentry/migrations/test_0550_migrate_no_action_dupe_issue_alerts.py diff --git a/migrations_lockfile.txt b/migrations_lockfile.txt index 279bd42b9a278a..d6d7aeaf57a017 100644 --- a/migrations_lockfile.txt +++ b/migrations_lockfile.txt @@ -8,5 +8,5 @@ will then be regenerated, and you should be able to merge without conflicts. feedback: 0001_feedback nodestore: 0002_nodestore_no_dictfield replays: 0003_add_size_to_recording_segment -sentry: 0549_re_add_groupsubscription_columns +sentry: 0550_migrate_no_action_dupe_issue_alerts social_auth: 0002_default_auto_field diff --git a/src/sentry/migrations/0550_migrate_no_action_dupe_issue_alerts.py b/src/sentry/migrations/0550_migrate_no_action_dupe_issue_alerts.py new file mode 100644 index 00000000000000..f28bed4da095a6 --- /dev/null +++ b/src/sentry/migrations/0550_migrate_no_action_dupe_issue_alerts.py @@ -0,0 +1,92 @@ +# Generated by Django 3.2.20 on 2023-09-12 18:15 + +from django.db import migrations + +from sentry.new_migrations.migrations import CheckedMigration +from sentry.utils.query import RangeQuerySetWrapperWithProgressBar + + +class ObjectStatus: + ACTIVE = 0 + HIDDEN = 1 + PENDING_DELETION = 2 + DELETION_IN_PROGRESS = 3 + + DISABLED = 1 + + +def has_duplicate_rule(apps, rule_data, project, rule_id): + Rule = apps.get_model("sentry", "Rule") + + matchers = {key for key in list(rule_data.keys()) if key not in ("name", "user_id")} + extra_fields = ["actions", "environment"] + matchers.update(extra_fields) + existing_rules = Rule.objects.exclude(id=rule_id).filter( + project=project, status=ObjectStatus.ACTIVE + ) + for existing_rule in existing_rules: + keys = 0 + matches = 0 + for matcher in matchers: + if existing_rule.data.get(matcher) and rule_data.get(matcher): + keys += 1 + + if existing_rule.data[matcher] == rule_data[matcher]: + matches += 1 + + elif matcher in extra_fields: + if not existing_rule.data.get(matcher) and not rule_data.get(matcher): + # neither rule has the matcher + continue + + elif matcher == "environment": + if existing_rule.environment_id and rule_data.get(matcher): + keys += 1 + if existing_rule.environment_id == rule_data.get(matcher): + matches += 1 + else: + keys += 1 + else: + # one rule has the matcher and the other one doesn't + keys += 1 + + if keys == matches: + return True + return False + + +def migrate_bad_rules(apps, schema_editor): + Rule = apps.get_model("sentry", "Rule") + + for rule in RangeQuerySetWrapperWithProgressBar(Rule.objects.all()): + if not rule.data.get("actions", []) or has_duplicate_rule( + apps, rule.data, rule.project, rule.id + ): + rule.status = ObjectStatus.DISABLED + rule.save(update_fields=["status"]) + + +class Migration(CheckedMigration): + # This flag is used to mark that a migration shouldn't be automatically run in production. For + # the most part, this should only be used for operations where it's safe to run the migration + # after your code has deployed. So this should not be used for most operations that alter the + # schema of a table. + # Here are some things that make sense to mark as dangerous: + # - Large data migrations. Typically we want these to be run manually by ops so that they can + # be monitored and not block the deploy for a long period of time while they run. + # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to + # have ops run this and not block the deploy. Note that while adding an index is a schema + # change, it's completely safe to run the operation after the code has deployed. + is_dangerous = True + + dependencies = [ + ("sentry", "0549_re_add_groupsubscription_columns"), + ] + + operations = [ + migrations.RunPython( + migrate_bad_rules, + migrations.RunPython.noop, + hints={"tables": ["sentry_rule"]}, + ), + ] diff --git a/src/sentry/testutils/factories.py b/src/sentry/testutils/factories.py index 88fd8cd5b63b66..ab5d6e6edb4b68 100644 --- a/src/sentry/testutils/factories.py +++ b/src/sentry/testutils/factories.py @@ -426,23 +426,27 @@ def create_project_bookmark(project, user): def create_project_rule( project, action_data=None, + allow_no_action_data=False, condition_data=None, name="", action_match="all", filter_match="all", **kwargs, ): - action_data = action_data or [ - { - "id": "sentry.rules.actions.notify_event.NotifyEventAction", - "name": "Send a notification (for all legacy integrations)", - }, - { - "id": "sentry.rules.actions.notify_event_service.NotifyEventServiceAction", - "service": "mail", - "name": "Send a notification via mail", - }, - ] + actions = None + if not allow_no_action_data: + action_data = action_data or [ + { + "id": "sentry.rules.actions.notify_event.NotifyEventAction", + "name": "Send a notification (for all legacy integrations)", + }, + { + "id": "sentry.rules.actions.notify_event_service.NotifyEventServiceAction", + "service": "mail", + "name": "Send a notification via mail", + }, + ] + actions = action_data condition_data = condition_data or [ { "id": "sentry.rules.conditions.first_seen_event.FirstSeenEventCondition", @@ -453,15 +457,18 @@ def create_project_rule( "name": "The event occurs", }, ] + data = { + "conditions": condition_data, + "action_match": action_match, + "filter_match": filter_match, + } + if actions: + data["actions"] = actions + return Rule.objects.create( label=name, project=project, - data={ - "conditions": condition_data, - "actions": action_data, - "action_match": action_match, - "filter_match": filter_match, - }, + data=data, **kwargs, ) diff --git a/tests/sentry/migrations/test_0550_migrate_no_action_dupe_issue_alerts.py b/tests/sentry/migrations/test_0550_migrate_no_action_dupe_issue_alerts.py new file mode 100644 index 00000000000000..4a611d359b64a4 --- /dev/null +++ b/tests/sentry/migrations/test_0550_migrate_no_action_dupe_issue_alerts.py @@ -0,0 +1,25 @@ +from sentry.constants import ObjectStatus +from sentry.testutils.cases import TestMigrations + + +class MigrateNoActionDupleIssueAlerts(TestMigrations): + migrate_from = "0549_re_add_groupsubscription_columns" + migrate_to = "0550_migrate_no_action_dupe_issue_alerts" + + def setup_initial_state(self): + self.no_action_rule = self.create_project_rule( + project=self.project, allow_no_action_data=True + ) + self.rule1 = self.create_project_rule(project=self.project) + self.rule2 = self.create_project_rule(project=self.project) + assert self.no_action_rule.status == ObjectStatus.ACTIVE + assert self.rule2.status == ObjectStatus.ACTIVE + assert self.rule1.status == ObjectStatus.ACTIVE + + def test(self): + self.no_action_rule.refresh_from_db() + self.rule1.refresh_from_db() + self.rule2.refresh_from_db() + assert self.no_action_rule.status == ObjectStatus.DISABLED + assert self.rule1.status == ObjectStatus.DISABLED + assert self.rule2.status == ObjectStatus.ACTIVE