Browse Source

feat(mail): Add support for target type and identifier to digests

Continuing to split out logic from https://github.com/getsentry/sentry/pull/17571. This adds
target_type and identifier to the digests key, and introduces a compatibility layer so that we can
continue to parse old keys.

I've kept this separate to the `unsplit_key` pr since we need to merge/deploy this first, otherwise
tasks that don't have this new code will fail if they receive a new key.

Depends on https://github.com/getsentry/sentry/pull/18250
jeffkwoh 4 years ago
parent
commit
e419abb133

+ 14 - 3
src/sentry/digests/notifications.py

@@ -11,6 +11,7 @@ from six.moves import reduce
 from sentry.app import tsdb
 from sentry.digests import Record
 from sentry.models import Project, Group, GroupStatus, Rule
+from sentry.utils import metrics
 from sentry.utils.dates import to_timestamp
 
 logger = logging.getLogger("sentry.digests")
@@ -19,10 +20,20 @@ Notification = namedtuple("Notification", "event rules")
 
 
 def split_key(key):
-    from sentry.plugins.base import plugins
+    from sentry.mail.adapter import ActionTargetType
+
+    key_parts = key.split(":", 4)
+    project_id = key_parts[2]
+    if len(key_parts) == 5:
+        target_type = ActionTargetType(key_parts[3])
+        target_identifier = key_parts[4] if key_parts[4] else None
+        metrics.incr("digests.new_key_seen")
+    else:
+        metrics.incr("digests.old_key_seen")
 
-    plugin_slug, _, project_id = key.split(":", 2)
-    return plugins.get(plugin_slug), Project.objects.get(pk=project_id)
+        target_type = ActionTargetType.ISSUE_OWNERS
+        target_identifier = None
+    return Project.objects.get(pk=project_id), target_type, target_identifier
 
 
 def unsplit_key(plugin, project):

+ 4 - 3
src/sentry/tasks/digests.py

@@ -37,16 +37,17 @@ def schedule_digests():
 @instrumented_task(name="sentry.tasks.digests.deliver_digest", queue="digests.delivery")
 def deliver_digest(key, schedule_timestamp=None):
     from sentry import digests
+    from sentry.mail.adapter import MailAdapter
 
     try:
-        plugin, project = split_key(key)
+        project, target_type, target_identifier = split_key(key)
     except Project.DoesNotExist as error:
         logger.info("Cannot deliver digest %r due to error: %s", key, error)
         digests.delete(key)
         return
 
     minimum_delay = ProjectOption.objects.get_value(
-        project, get_option_key(plugin.get_conf_key(), "minimum_delay")
+        project, get_option_key("mail", "minimum_delay")
     )
 
     with snuba.options_override({"consistent": True}):
@@ -58,4 +59,4 @@ def deliver_digest(key, schedule_timestamp=None):
             return
 
         if digest:
-            plugin.notify_digest(project, digest)
+            MailAdapter().notify_digest(project, digest, target_type, target_identifier)

+ 24 - 0
tests/sentry/digests/test_notifications.py

@@ -12,7 +12,9 @@ from sentry.digests.notifications import (
     group_records,
     sort_group_contents,
     sort_rule_groups,
+    split_key,
 )
+from sentry.mail.adapter import ActionTargetType
 from sentry.models import Rule
 from sentry.testutils import TestCase
 
@@ -112,3 +114,25 @@ class SortRecordsTestCase(TestCase):
                 (rules[0], OrderedDict(((groups[0], []),))),
             )
         )
+
+
+class SplitKeyTestCase(TestCase):
+    def test_old_style_key(self):
+        assert split_key("mail:p:{}".format(self.project.id)) == (
+            self.project,
+            ActionTargetType.ISSUE_OWNERS,
+            None,
+        )
+
+    def test_new_style_key_no_identifier(self):
+        assert split_key(
+            "mail:p:{}:{}:".format(self.project.id, ActionTargetType.ISSUE_OWNERS.value)
+        ) == (self.project, ActionTargetType.ISSUE_OWNERS, None)
+
+    def test_new_style_key_identifier(self):
+        identifier = "123"
+        assert split_key(
+            "mail:p:{}:{}:{}".format(
+                self.project.id, ActionTargetType.ISSUE_OWNERS.value, identifier
+            )
+        ) == (self.project, ActionTargetType.ISSUE_OWNERS, identifier)

+ 14 - 4
tests/sentry/tasks/test_digests.py

@@ -8,15 +8,13 @@ import sentry
 from sentry.digests.backends.redis import RedisBackend
 from sentry.digests.notifications import event_to_record
 from sentry.models.rule import Rule
-from sentry.plugins.sentry_mail.models import MailPlugin
 from sentry.tasks.digests import deliver_digest
 from sentry.testutils import TestCase
 from sentry.testutils.helpers.datetime import iso_format, before_now
 
 
 class DeliverDigestTest(TestCase):
-    @patch.object(sentry, "digests")
-    def test(self, digests):
+    def run_test(self, key, digests):
         """
         Simple integration test to make sure that digests are firing as expected.
         """
@@ -30,10 +28,22 @@ class DeliverDigestTest(TestCase):
             data={"timestamp": iso_format(before_now(days=1)), "fingerprint": ["group-2"]},
             project_id=self.project.id,
         )
-        key = "{}:p:{}".format(MailPlugin.slug, self.project.id)
+        key = "mail:p:{}".format(self.project.id)
         backend.add(key, event_to_record(event, [rule]), increment_delay=0, maximum_delay=0)
         backend.add(key, event_to_record(event_2, [rule]), increment_delay=0, maximum_delay=0)
         digests.digest = backend.digest
         with self.tasks():
             deliver_digest(key)
         assert "2 new alerts since" in mail.outbox[0].subject
+
+    @patch.object(sentry, "digests")
+    def test_old_key(self, digests):
+        self.run_test("mail:p:{}".format(self.project.id), digests)
+
+    @patch.object(sentry, "digests")
+    def test_new_key(self, digests):
+        self.run_test("mail:p:{}:IssueOwners:".format(self.project.id), digests)
+
+    @patch.object(sentry, "digests")
+    def test_member_key(self, digests):
+        self.run_test("mail:p:{}:Member:{}".format(self.project.id, self.user.id), digests)