Browse Source

feat(insights): Update `GroupHistory.actor` to set null on delete (#29469)

We don't want to delete these rows if the actor that made the changes is deleted, just `SET_NULL`
instead. Also corrected some comments
Dan Fuller 3 years ago
parent
commit
1173127d80

+ 1 - 1
migrations_lockfile.txt

@@ -6,5 +6,5 @@ To resolve this, rebase against latest master and regenerate your migration. Thi
 will then be regenerated, and you should be able to merge without conflicts.
 
 nodestore: 0002_nodestore_no_dictfield
-sentry: 0240_grouphistory_index
+sentry: 0241_grouphistory_null_actor
 social_auth: 0001_initial

+ 2 - 2
src/sentry/api/endpoints/team_time_to_resolution.py

@@ -29,8 +29,8 @@ class TeamTimeToResolutionEndpoint(TeamEndpoint, EnvironmentMixin):
             )
             .annotate(bucket=TruncDay("date_added"))
             .values("bucket", "prev_history_date")
-            # We do the coalesce here to handle historical data. At some point every `RESOLVED` row
-            # will have a non-null `prev_history_date`, and at that point we could remove this.
+            # We need to coalesce here since we won't store the initial `UNRESOLVED` row for every
+            # group, since it's unnecessary and just takes extra storage.
             .annotate(
                 ttr=F("date_added") - Coalesce(F("prev_history_date"), F("group__first_seen"))
             )

+ 42 - 0
src/sentry/migrations/0241_grouphistory_null_actor.py

@@ -0,0 +1,42 @@
+# Generated by Django 2.2.24 on 2021-10-21 01:06
+
+import django.db.models.deletion
+from django.db import migrations
+
+import sentry.db.models.fields.foreignkey
+
+
+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.
+    # You'll also usually want to set this to `False` if you're writing a data
+    # migration, since we don't want the entire migration to run in one long-running
+    # transaction.
+    atomic = True
+
+    dependencies = [
+        ("sentry", "0240_grouphistory_index"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="grouphistory",
+            name="actor",
+            field=sentry.db.models.fields.foreignkey.FlexibleForeignKey(
+                null=True, on_delete=django.db.models.deletion.SET_NULL, to="sentry.Actor"
+            ),
+        ),
+    ]

+ 4 - 1
src/sentry/models/grouphistory.py

@@ -1,6 +1,7 @@
 from typing import TYPE_CHECKING, Optional, Union
 
 from django.db import models
+from django.db.models import SET_NULL
 from django.utils import timezone
 from django.utils.translation import ugettext_lazy as _
 
@@ -12,6 +13,8 @@ if TYPE_CHECKING:
 
 
 class GroupHistoryStatus:
+    # Note that we don't record the initial group creation unresolved here to save on creating a row
+    # for every group.
     UNRESOLVED = 0
     RESOLVED = 1
     SET_RESOLVED_IN_RELEASE = 11
@@ -78,7 +81,7 @@ class GroupHistory(Model):
     group = FlexibleForeignKey("sentry.Group", db_constraint=False)
     project = FlexibleForeignKey("sentry.Project", db_constraint=False)
     release = FlexibleForeignKey("sentry.Release", null=True, db_constraint=False)
-    actor = FlexibleForeignKey("sentry.Actor", null=True)
+    actor = FlexibleForeignKey("sentry.Actor", null=True, on_delete=SET_NULL)
 
     status = BoundedPositiveIntegerField(
         default=0,