refs(alert_rules): Stop writing to unused incident fields. Modify data model to prepare for removing unused fields (#18875)

This refactors the `Incident` model to make `aggregation` and `query` nullable, removes the `groups`
relations and disables the constraints on `IncidentGroup`. We don't use any of these fields any
more, so may as well remove.

We also make `alert_rule` not null, since we will always have an alert rule available now. The data
migration will remove any incidents without alert rules - these are all test rows and are fine to
remove.

All tables involved are small, and so it's fine to do these operations as part of the migration.

bin/load-mocks

@@ -670,9 +670,7 @@ def main(num_events=1, extra_events=False):
                     org,
                     type_=IncidentType.DETECTED,
                     title="My Incident",
-                    query="",
                     date_started=datetime.utcnow().replace(tzinfo=utc),
-                    aggregation=QueryAggregations.TOTAL,
                     alert_rule=alert_rule,
                     projects=[project],
                 )

migrations_lockfile.txt

@@ -10,7 +10,7 @@ auth: 0008_alter_user_username_max_length
 contenttypes: 0002_remove_content_type_name
 jira_ac: 0001_initial
 nodestore: 0001_initial
-sentry: 0077_alert_query_col_drop_state
+sentry: 0078_incident_field_updates
 sessions: 0001_initial
 sites: 0002_alter_domain_unique
 social_auth: 0001_initial

src/sentry/api/endpoints/organization_incident_index.py

@@ -1,51 +1,12 @@
 from __future__ import absolute_import
 
-from rest_framework import serializers
-
 from sentry import features
 from sentry.api.bases.incident import IncidentPermission
 from sentry.api.bases.organization import OrganizationEndpoint
 from sentry.api.exceptions import ResourceDoesNotExist
 from sentry.api.paginator import OffsetPaginator
 from sentry.api.serializers import serialize
-from sentry.api.serializers.rest_framework import ListField
 from sentry.incidents.models import Incident, IncidentStatus
-from sentry.models.group import Group
-from sentry.models.project import Project
-from sentry.snuba.models import QueryAggregations
-
-
-class IncidentSerializer(serializers.Serializer):
-    projects = ListField(child=serializers.CharField(), required=False, default=[])
-    groups = ListField(child=serializers.CharField(), required=True, allow_null=False)
-    title = serializers.CharField(required=True)
-    query = serializers.CharField(required=False, allow_blank=True, allow_null=True)
-    aggregation = serializers.IntegerField(default=QueryAggregations.TOTAL.value)
-    dateStarted = serializers.DateTimeField(required=False)
-    dateDetected = serializers.DateTimeField(required=False, allow_null=True)
-
-    def validate_projects(self, slugs):
-        projects = Project.objects.filter(organization=self.context["organization"], slug__in=slugs)
-        if len(projects) != len(slugs):
-            raise serializers.ValidationError("Invalid project slug(s)")
-        return list(projects)
-
-    def validate_groups(self, group_ids):
-        groups = Group.objects.filter(
-            project__organization=self.context["organization"], id__in=group_ids
-        ).select_related("project")
-        if len(groups) != len(group_ids):
-            raise serializers.ValidationError("Invalid group id(s)")
-        return list(groups)
-
-    def validate_aggregation(self, aggregation):
-        try:
-            return QueryAggregations(aggregation)
-        except ValueError:
-            raise serializers.ValidationError(
-                "Invalid aggregation, valid values are %s"
-                % [item.value for item in QueryAggregations]
-            )
 
 
 class OrganizationIncidentIndexEndpoint(OrganizationEndpoint):

src/sentry/api/serializers/models/incident.py

@@ -5,13 +5,7 @@ from collections import defaultdict
 import six
 
 from sentry.api.serializers import Serializer, register, serialize
-from sentry.incidents.models import (
-    Incident,
-    IncidentGroup,
-    IncidentProject,
-    IncidentSeen,
-    IncidentSubscription,
-)
+from sentry.incidents.models import Incident, IncidentProject, IncidentSeen, IncidentSubscription
 from sentry.snuba.models import QueryDatasets
 from sentry.utils.db import attach_foreignkey
 
@@ -41,8 +35,6 @@ class IncidentSerializer(Serializer):
             "statusMethod": obj.status_method,
             "type": obj.type,
             "title": obj.title,
-            "query": obj.query,
-            "aggregation": obj.aggregation,
             "dateStarted": obj.date_started,
             "dateDetected": obj.date_detected,
             "dateCreated": obj.date_added,
@@ -62,15 +54,8 @@ class DetailedIncidentSerializer(IncidentSerializer):
                 )
             )
 
-        incident_groups = defaultdict(list)
-        for incident_id, group_id in IncidentGroup.objects.filter(
-            incident__in=item_list
-        ).values_list("incident_id", "group_id"):
-            incident_groups[incident_id].append(six.text_type(group_id))
-
         for item in item_list:
             results[item]["is_subscribed"] = item.id in subscribed_incidents
-            results[item]["groups"] = incident_groups.get(item.id, [])
         return results
 
     def _get_incident_seen_list(self, incident, user):
@@ -97,7 +82,6 @@ class DetailedIncidentSerializer(IncidentSerializer):
         context["isSubscribed"] = attrs["is_subscribed"]
         context["seenBy"] = seen_list["seen_by"]
         context["hasSeen"] = seen_list["has_seen"]
-        context["groups"] = attrs["groups"]
         context["alertRule"] = serialize(obj.alert_rule, user)
         # The query we should use to get accurate results in Discover.
         context["discoverQuery"] = self._build_discover_query(obj)

src/sentry/incidents/logic.py

@@ -21,7 +21,6 @@ from sentry.incidents.models import (
     Incident,
     IncidentActivity,
     IncidentActivityType,
-    IncidentGroup,
     IncidentProject,
     IncidentSnapshot,
     PendingIncidentSnapshot,
@@ -60,23 +59,14 @@ def create_incident(
     organization,
     type_,
     title,
-    query,
-    aggregation,
     date_started,
     date_detected=None,
     # TODO: Probably remove detection_uuid?
     detection_uuid=None,
     projects=None,
-    groups=None,
     user=None,
     alert_rule=None,
 ):
-    if groups:
-        group_projects = [g.project for g in groups]
-        if projects is None:
-            projects = []
-        projects = list(set(projects + group_projects))
-
     if date_detected is None:
         date_detected = date_started
 
@@ -87,8 +77,6 @@ def create_incident(
             status=IncidentStatus.OPEN.value,
             type=type_.value,
             title=title,
-            query=query,
-            aggregation=aggregation.value,
             date_started=date_started,
             date_detected=date_detected,
             alert_rule=alert_rule,
@@ -104,11 +92,6 @@ def create_incident(
                     sender=type(incident_project), instance=incident_project, created=True
                 )
 
-        if groups:
-            IncidentGroup.objects.bulk_create(
-                [IncidentGroup(incident=incident, group=group) for group in groups]
-            )
-
         create_incident_activity(incident, IncidentActivityType.DETECTED, user=user)
         analytics.record(
             "incident.created",
@@ -320,11 +303,6 @@ def build_incident_query_params(incident, start=None, end=None, windowed_stats=F
         incident, start, end, windowed_stats=windowed_stats
     )
 
-    group_ids = list(
-        IncidentGroup.objects.filter(incident=incident).values_list("group_id", flat=True)
-    )
-    if group_ids:
-        params["group_ids"] = group_ids
     project_ids = list(
         IncidentProject.objects.filter(incident=incident).values_list("project_id", flat=True)
     )

src/sentry/incidents/models.py

@@ -34,7 +34,7 @@ class IncidentGroup(Model):
     __core__ = False
 
     group = FlexibleForeignKey("sentry.Group", db_index=False, db_constraint=False)
-    incident = FlexibleForeignKey("sentry.Incident")
+    incident = FlexibleForeignKey("sentry.Incident", db_constraint=False)
 
     class Meta:
         app_label = "sentry"
@@ -163,8 +163,7 @@ class Incident(Model):
     projects = models.ManyToManyField(
         "sentry.Project", related_name="incidents", through=IncidentProject
     )
-    groups = models.ManyToManyField("sentry.Group", related_name="incidents", through=IncidentGroup)
-    alert_rule = FlexibleForeignKey("sentry.AlertRule", null=True, on_delete=models.SET_NULL)
+    alert_rule = FlexibleForeignKey("sentry.AlertRule", on_delete=models.PROTECT)
     # Incrementing id that is specific to the org.
     identifier = models.IntegerField()
     # Identifier used to match incoming events from the detection algorithm
@@ -174,10 +173,10 @@ class Incident(Model):
         default=IncidentStatusMethod.RULE_TRIGGERED.value
     )
     type = models.PositiveSmallIntegerField()
-    aggregation = models.PositiveSmallIntegerField(default=QueryAggregations.TOTAL.value)
+    aggregation = models.PositiveSmallIntegerField(default=QueryAggregations.TOTAL.value, null=True)
     title = models.TextField()
     # Query used to fetch events related to an incident
-    query = models.TextField()
+    query = models.TextField(null=True)
     # When we suspect the incident actually started
     date_started = models.DateTimeField(default=timezone.now)
     # When we actually detected the incident

src/sentry/incidents/subscription_processor.py

@@ -23,7 +23,6 @@ from sentry.incidents.models import (
     TriggerStatus,
 )
 from sentry.incidents.tasks import handle_trigger_action
-from sentry.snuba.subscriptions import aggregate_to_query_aggregation
 from sentry.utils import metrics, redis
 from sentry.utils.dates import to_datetime, to_timestamp
 from sentry.utils.compat import zip
@@ -185,10 +184,6 @@ class SubscriptionProcessor(object):
                     # TODO: Include more info in name?
                     self.alert_rule.name,
                     alert_rule=self.alert_rule,
-                    query=self.subscription.snuba_query.query,
-                    aggregation=aggregate_to_query_aggregation[
-                        self.alert_rule.snuba_query.aggregate
-                    ],
                     date_started=detected_at,
                     date_detected=detected_at,
                     projects=[self.subscription.project],

src/sentry/migrations/0078_incident_field_updates.py

@@ -0,0 +1,72 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.29 on 2020-05-15 20:50
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+import sentry.db.models.fields.foreignkey
+from sentry.utils.query import RangeQuerySetWrapperWithProgressBar
+
+
+def delete_incidents_with_no_alert_rule(apps, schema_editor):
+    # These are only test incidents that we don't care about, should be fine to remove
+    # these so that we can require there always be an AlertRule associated with
+    # Incidents going forward
+    Incident = apps.get_model("sentry", "Incident")
+    for incident in RangeQuerySetWrapperWithProgressBar(
+        Incident.objects.filter(alert_rule__isnull=True)
+    ):
+        incident.delete()
+
+
+class Migration(migrations.Migration):
+    # This flag is used to mark that a migration shouldn't be automatically run in
+    # production. We set this to True for operations that we think are risky and want
+    # someone from ops to run manually and monitor.
+    # General advice is that if in doubt, mark your migration as `is_dangerous`.
+    # Some things you should always mark as dangerous:
+    # - Large data migrations. Typically we want these to be run manually by ops so that
+    #   they can be monitored. Since data migrations will now hold a transaction open
+    #   this is even more important.
+    # - Adding columns to highly active tables, even ones that are NULL.
+    is_dangerous = False
+
+    # This flag is used to decide whether to run this migration in a transaction or not.
+    # By default we prefer to run in a transaction, but for migrations where you want
+    # to `CREATE INDEX CONCURRENTLY` this needs to be set to False. Typically you'll
+    # want to create an index concurrently when adding one to an existing table.
+    atomic = False
+
+    dependencies = [("sentry", "0077_alert_query_col_drop_state")]
+
+    operations = [
+        migrations.RunPython(
+            delete_incidents_with_no_alert_rule, reverse_code=migrations.RunPython.noop
+        ),
+        migrations.RemoveField(model_name="incident", name="groups"),
+        migrations.AlterField(
+            model_name="incident",
+            name="aggregation",
+            field=models.PositiveSmallIntegerField(default=0, null=True),
+        ),
+        migrations.AlterField(
+            model_name="incident",
+            name="alert_rule",
+            field=sentry.db.models.fields.foreignkey.FlexibleForeignKey(
+                on_delete=django.db.models.deletion.PROTECT, to="sentry.AlertRule"
+            ),
+        ),
+        migrations.AlterField(
+            model_name="incident", name="query", field=models.TextField(null=True)
+        ),
+        migrations.AlterField(
+            model_name="incidentgroup",
+            name="incident",
+            field=sentry.db.models.fields.foreignkey.FlexibleForeignKey(
+                db_constraint=False,
+                on_delete=django.db.models.deletion.CASCADE,
+                to="sentry.Incident",
+            ),
+        ),
+    ]

src/sentry/testutils/factories.py

@@ -31,7 +31,6 @@ from sentry.incidents.models import (
     AlertRuleTriggerAction,
     Incident,
     IncidentActivity,
-    IncidentGroup,
     IncidentProject,
     IncidentSeen,
     IncidentType,
@@ -812,7 +811,6 @@ class Factories(object):
         date_started=None,
         date_detected=None,
         date_closed=None,
-        groups=None,
         seen_by=None,
         alert_rule=None,
     ):
@@ -828,7 +826,6 @@ class Factories(object):
             detection_uuid=detection_uuid,
             status=status,
             title=title,
-            query=query,
             alert_rule=alert_rule,
             date_started=date_started or timezone.now(),
             date_detected=date_detected or timezone.now(),
@@ -837,9 +834,6 @@ class Factories(object):
         )
         for project in projects:
             IncidentProject.objects.create(incident=incident, project=project)
-        if groups:
-            for group in groups:
-                IncidentGroup.objects.create(incident=incident, group=group)
         if seen_by:
             for user in seen_by:
                 IncidentSeen.objects.create(incident=incident, user=user, last_seen=timezone.now())

tests/acceptance/test_incidents.py

@@ -5,9 +5,7 @@ import pytz
 
 from sentry.testutils import AcceptanceTestCase, SnubaTestCase
 from sentry.testutils.helpers.datetime import before_now
-from sentry.incidents.logic import create_alert_rule, create_incident
-from sentry.incidents.models import IncidentType
-from sentry.snuba.models import QueryAggregations
+from sentry.incidents.logic import create_alert_rule
 
 FEATURE_NAME = "organizations:incidents"
 
@@ -31,16 +29,12 @@ class OrganizationIncidentsListTest(AcceptanceTestCase, SnubaTestCase):
             self.organization, [self.project], "hello", "level:error", "count()", 10, 1
         )
 
-        incident = create_incident(
+        incident = self.create_incident(
             self.organization,
-            type_=IncidentType.DETECTED,
             title="Incident #1",
-            query="hello",
-            aggregation=QueryAggregations.TOTAL,
             date_started=timezone.now(),
             date_detected=timezone.now(),
             projects=[self.project],
-            groups=[self.group],
             alert_rule=alert_rule,
         )