Browse Source

chore(db): revert-revert - Split DB and state for removing Group.score column (#78880)

Had to revert https://github.com/getsentry/sentry/pull/78845.

Trying again except the `ScoreClause` class will be retained. I think
some in flight code elsewhere may have been referencing it but i'm still
not sure where. Either way, this will prevent
https://sentry.sentry.io/issues/5976379958/ in theory.
Leander Rodrigues 5 months ago
parent
commit
860c9b119a

+ 1 - 1
migrations_lockfile.txt

@@ -10,7 +10,7 @@ hybridcloud: 0016_add_control_cacheversion
 nodestore: 0002_nodestore_no_dictfield
 remote_subscriptions: 0003_drop_remote_subscription
 replays: 0004_index_together
-sentry: 0773_make_group_score_nullable
+sentry: 0774_drop_group_score_in_state_only
 social_auth: 0002_default_auto_field
 uptime: 0017_unique_on_timeout
 workflow_engine: 0009_detector_type

+ 0 - 7
src/sentry/buffer/base.py

@@ -148,7 +148,6 @@ class Buffer(Service):
         extra: dict[str, Any] | None = None,
         signal_only: bool | None = None,
     ) -> None:
-        from sentry.event_manager import ScoreClause
         from sentry.models.group import Group
 
         created = False
@@ -162,12 +161,6 @@ class Buffer(Service):
             # HACK(dcramer): this is gross, but we don't have a good hook to compute this property today
             # XXX(dcramer): remove once we can replace 'priority' with something reasonable via Snuba
             if model is Group:
-                if "last_seen" in update_kwargs and "times_seen" in update_kwargs:
-                    update_kwargs["score"] = ScoreClause(
-                        group=None,
-                        times_seen=update_kwargs["times_seen"],
-                        last_seen=update_kwargs["last_seen"],
-                    )
                 # XXX: create_or_update doesn't fire `post_save` signals, and so this update never
                 # ends up in the cache. This causes issues when handling issue alerts, and likely
                 # elsewhere. Use `update` here since we're already special casing, and we know that

+ 3 - 1
src/sentry/event_manager.py

@@ -319,7 +319,9 @@ class ScoreClause(Func):
     def __int__(self):
         # Calculate the score manually when coercing to an int.
         # This is used within create_or_update and friends
-        return self.group.get_score() if self.group else 0
+
+        # XXX: Since removing the 'score' column from 'Group', this now always returns 0.
+        return 0
 
     def as_sql(
         self,

+ 34 - 0
src/sentry/migrations/0774_drop_group_score_in_state_only.py

@@ -0,0 +1,34 @@
+# Generated by Django 5.1.1 on 2024-10-09 15:39
+
+from django.db import migrations
+
+from sentry.new_migrations.migrations import CheckedMigration
+
+
+class Migration(CheckedMigration):
+    # This flag is used to mark that a migration shouldn't be automatically run in production.
+    # 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 post deployment:
+    # - Large data migrations. Typically we want these to be run manually 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
+    #   run this outside deployments so that we don't block them. Note that while adding an index
+    #   is a schema change, it's completely safe to run the operation after the code has deployed.
+    # Once deployed, run these manually via: https://develop.sentry.dev/database-migrations/#migration-deployment
+
+    is_post_deployment = False
+
+    dependencies = [
+        ("sentry", "0773_make_group_score_nullable"),
+    ]
+
+    operations = [
+        migrations.SeparateDatabaseAndState(
+            database_operations=[],
+            state_operations=[
+                migrations.RemoveField(model_name="group", name="score"),
+            ],
+        )
+    ]

+ 0 - 12
src/sentry/models/group.py

@@ -1,7 +1,6 @@
 from __future__ import annotations
 
 import logging
-import math
 import re
 import warnings
 from collections import defaultdict, namedtuple
@@ -570,7 +569,6 @@ class Group(Model):
     active_at = models.DateTimeField(null=True, db_index=True)
     time_spent_total = BoundedIntegerField(default=0)
     time_spent_count = BoundedIntegerField(default=0)
-    score = BoundedIntegerField(default=0, null=True)
     # deprecated, do not use. GroupShare has superseded
     is_public = models.BooleanField(default=False, null=True)
     data: models.Field[dict[str, Any] | None, dict[str, Any]] = GzippedDictField(
@@ -624,9 +622,6 @@ class Group(Model):
             self.message = truncatechars(self.message.splitlines()[0], 255)
         if self.times_seen is None:
             self.times_seen = 1
-        self.score = type(self).calculate_score(
-            times_seen=self.times_seen, last_seen=self.last_seen
-        )
         super().save(*args, **kwargs)
 
     def get_absolute_url(
@@ -769,9 +764,6 @@ class Group(Model):
             # Otherwise it has not been shared yet.
             return None
 
-    def get_score(self):
-        return type(self).calculate_score(self.times_seen, self.last_seen)
-
     def get_latest_event(self) -> GroupEvent | None:
         if not hasattr(self, "_latest_event"):
             self._latest_event = self.get_latest_event_for_environments()
@@ -922,10 +914,6 @@ class Group(Model):
             referrer=referrer,
         )[self.id]
 
-    @classmethod
-    def calculate_score(cls, times_seen, last_seen):
-        return math.log(float(times_seen or 1)) * 600 + float(last_seen.strftime("%s"))
-
     def get_assignee(self) -> Team | RpcUser | None:
         from sentry.models.groupassignee import GroupAssignee
 

+ 0 - 3
src/sentry/tasks/unmerge.py

@@ -124,9 +124,6 @@ backfill_fields = {
         else data.get("first_release", None)
     ),
     "times_seen": lambda caches, data, event: data["times_seen"] + 1,
-    "score": lambda caches, data, event: Group.calculate_score(
-        data["times_seen"] + 1, data["last_seen"]
-    ),
 }
 
 

+ 0 - 2
src/sentry/testutils/cases.py

@@ -1452,7 +1452,6 @@ class SnubaTestCase(BaseTestCase):
                 "last_seen",
                 "first_seen",
                 "data",
-                "score",
                 "project_id",
                 "time_spent_total",
                 "time_spent_count",
@@ -1474,7 +1473,6 @@ class SnubaTestCase(BaseTestCase):
                 self.to_snuba_time_format(group.last_seen),
                 self.to_snuba_time_format(group.first_seen),
                 group.data,
-                group.score,
                 group.project.id,
                 group.time_spent_total,
                 group.time_spent_count,

+ 0 - 3
tests/snuba/tasks/test_unmerge.py

@@ -106,7 +106,6 @@ class UnmergeTestCase(TestCase, SnubaTestCase):
             "platform": "java",
             "message": "Hello from JavaScript",
             "level": logging.INFO,
-            "score": Group.calculate_score(3, now),
             "logger": "java",
             "times_seen": 3,
             "first_release": None,
@@ -132,7 +131,6 @@ class UnmergeTestCase(TestCase, SnubaTestCase):
                 platform="javascript",
                 message="Hello from JavaScript",
                 level=logging.INFO,
-                score=Group.calculate_score(3, now),
                 logger="javascript",
                 times_seen=1,
                 first_release=None,
@@ -167,7 +165,6 @@ class UnmergeTestCase(TestCase, SnubaTestCase):
             "active_at": now - timedelta(hours=2),
             "first_seen": now - timedelta(hours=2),
             "platform": "java",
-            "score": Group.calculate_score(3, now),
             "logger": "java",
             "times_seen": 3,
             "first_release": None,