Browse Source

ref(migration): Create shared model NotificationAction (#45509)

This PR introduces a new model `NotificationAction` in place of the one
in [this PR](https://github.com/getsentry/getsentry/pull/9583). Since a
lot of the shape is shared among `AlertRuleTriggerAction` an abstract
model was used for the shared fields.

The way they register handlers is different and I wasn't able to pull
that into the abstractio. However, they share the enums which will much
more explicitly suggest adding handlers for both metric alerts, and
these notifications should we extend the list of services in the future.
Leander Rodrigues 2 years ago
parent
commit
2523ae97d8

+ 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: 0378_remove_dynamic_sampling_depricated_data
+sentry: 0379_create_notificationaction_model
 social_auth: 0001_initial

+ 3 - 0
src/sentry/incidents/logic.py

@@ -1105,6 +1105,9 @@ def create_alert_rule_trigger_action(
     :return: The created action
     """
     target_display = None
+    if type.value in AlertRuleTriggerAction.EXEMPT_SERVICES:
+        raise InvalidTriggerActionError("Selected notification service is exempt from alert rules")
+
     if type.value in AlertRuleTriggerAction.INTEGRATION_TYPES:
         if target_type != AlertRuleTriggerAction.TargetType.SPECIFIC:
             raise InvalidTriggerActionError("Must specify specific target type")

+ 9 - 27
src/sentry/incidents/models.py

@@ -20,6 +20,7 @@ from sentry.db.models import (
 from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey
 from sentry.db.models.manager import BaseManager
 from sentry.models import Team
+from sentry.models.notificationaction import AbstractNotificationAction, ActionService, ActionTarget
 from sentry.services.hybrid_cloud.user import user_service
 from sentry.snuba.models import QuerySubscription
 from sentry.utils import metrics
@@ -524,7 +525,7 @@ class AlertRuleTriggerExclusion(Model):
 
 
 @region_silo_only_model
-class AlertRuleTriggerAction(Model):
+class AlertRuleTriggerAction(AbstractNotificationAction):
     """
     This model represents an action that occurs when a trigger is fired. This is
     typically some sort of notification.
@@ -532,28 +533,16 @@ class AlertRuleTriggerAction(Model):
 
     __include_in_export__ = True
 
-    _type_registrations = {}
+    # Aliases from NotificationAction
+    Type = ActionService
+    TargetType = ActionTarget
 
-    # Which sort of action to take
-    class Type(Enum):
-        EMAIL = 0
-        PAGERDUTY = 1
-        SLACK = 2
-        MSTEAMS = 3
-        SENTRY_APP = 4
+    _type_registrations = {}
 
     INTEGRATION_TYPES = frozenset((Type.PAGERDUTY.value, Type.SLACK.value, Type.MSTEAMS.value))
 
-    class TargetType(Enum):
-        # A direct reference, like an email address, Slack channel, or PagerDuty service
-        SPECIFIC = 0
-        # A specific user. This could be used to grab the user's email address.
-        USER = 1
-        # A specific team. This could be used to send an email to everyone associated
-        # with a team.
-        TEAM = 2
-        # A Sentry App instead of any of the above.
-        SENTRY_APP = 3
+    # ActionService items which are not supported for AlertRuleTriggerActions
+    EXEMPT_SERVICES = frozenset((Type.SENTRY_NOTIFICATION.value,))
 
     TypeRegistration = namedtuple(
         "TypeRegistration",
@@ -561,14 +550,7 @@ class AlertRuleTriggerAction(Model):
     )
 
     alert_rule_trigger = FlexibleForeignKey("sentry.AlertRuleTrigger")
-    integration = FlexibleForeignKey("sentry.Integration", null=True)
-    sentry_app = FlexibleForeignKey("sentry.SentryApp", null=True)
-    type = models.SmallIntegerField()
-    target_type = models.SmallIntegerField()
-    # Identifier used to perform the action on a given target
-    target_identifier = models.TextField(null=True)
-    # Human readable name to display in the UI
-    target_display = models.TextField(null=True)
+
     date_added = models.DateTimeField(default=timezone.now)
     sentry_app_config = JSONField(null=True)
 

+ 108 - 0
src/sentry/migrations/0379_create_notificationaction_model.py

@@ -0,0 +1,108 @@
+# Generated by Django 2.2.28 on 2023-03-13 17:01
+
+import django.db.models.deletion
+from django.db import migrations, models
+
+import sentry.db.models.fields.bounded
+import sentry.db.models.fields.foreignkey
+import sentry.db.models.fields.hybrid_cloud_foreign_key
+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. 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 = False
+
+    dependencies = [
+        ("sentry", "0378_remove_dynamic_sampling_depricated_data"),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name="NotificationAction",
+            fields=[
+                (
+                    "id",
+                    sentry.db.models.fields.bounded.BoundedBigAutoField(
+                        primary_key=True, serialize=False
+                    ),
+                ),
+                ("type", models.SmallIntegerField()),
+                ("target_type", models.SmallIntegerField()),
+                ("target_identifier", models.TextField(null=True)),
+                ("target_display", models.TextField(null=True)),
+                (
+                    "integration_id",
+                    sentry.db.models.fields.hybrid_cloud_foreign_key.HybridCloudForeignKey(
+                        "sentry.Integration",
+                        blank=True,
+                        db_index=True,
+                        null=True,
+                        on_delete="CASCADE",
+                    ),
+                ),
+                (
+                    "sentry_app_id",
+                    sentry.db.models.fields.hybrid_cloud_foreign_key.HybridCloudForeignKey(
+                        "sentry.SentryApp",
+                        blank=True,
+                        db_index=True,
+                        null=True,
+                        on_delete="CASCADE",
+                    ),
+                ),
+                ("trigger_type", models.SmallIntegerField()),
+                (
+                    "organization",
+                    sentry.db.models.fields.foreignkey.FlexibleForeignKey(
+                        on_delete=django.db.models.deletion.CASCADE, to="sentry.Organization"
+                    ),
+                ),
+            ],
+            options={
+                "db_table": "sentry_notificationaction",
+            },
+        ),
+        migrations.CreateModel(
+            name="NotificationActionProject",
+            fields=[
+                (
+                    "id",
+                    sentry.db.models.fields.bounded.BoundedBigAutoField(
+                        primary_key=True, serialize=False
+                    ),
+                ),
+                (
+                    "action",
+                    sentry.db.models.fields.foreignkey.FlexibleForeignKey(
+                        on_delete=django.db.models.deletion.CASCADE, to="sentry.NotificationAction"
+                    ),
+                ),
+                (
+                    "project",
+                    sentry.db.models.fields.foreignkey.FlexibleForeignKey(
+                        on_delete=django.db.models.deletion.CASCADE, to="sentry.Project"
+                    ),
+                ),
+            ],
+            options={
+                "db_table": "sentry_notificationactionproject",
+            },
+        ),
+        migrations.AddField(
+            model_name="notificationaction",
+            name="projects",
+            field=models.ManyToManyField(
+                through="sentry.NotificationActionProject", to="sentry.Project"
+            ),
+        ),
+    ]

+ 219 - 0
src/sentry/models/notificationaction.py

@@ -0,0 +1,219 @@
+from __future__ import annotations
+
+import logging
+from collections import defaultdict
+from enum import IntEnum
+from typing import Callable, Iterable, List, Tuple
+
+from django.db import models
+from pyparsing import MutableMapping
+
+from sentry.db.models import FlexibleForeignKey, Model, sane_repr
+from sentry.db.models.base import region_silo_only_model
+from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey
+
+logger = logging.getLogger(__name__)
+
+
+class FlexibleIntEnum(IntEnum):
+    @classmethod
+    def as_choices(cls) -> Iterable[Tuple[int, str]]:
+        raise NotImplementedError
+
+    @classmethod
+    def get_name(cls, value: int) -> str | None:
+        return dict(cls.as_choices()).get(value)
+
+    @classmethod
+    def get_value(cls, name: str) -> int | None:
+        invert_choices = {v: k for k, v in dict(cls.as_choices()).items()}
+        return invert_choices.get(name)
+
+
+class ActionService(FlexibleIntEnum):
+    """
+    The available services to fire action notifications
+    """
+
+    EMAIL = 0
+    PAGERDUTY = 1
+    SLACK = 2
+    MSTEAMS = 3
+    SENTRY_APP = 4
+    SENTRY_NOTIFICATION = 5  # Use personal notification platform (src/sentry/notifications)
+
+    @classmethod
+    def as_choices(cls) -> Iterable[Tuple[int, str]]:
+        return (
+            (cls.EMAIL, "email"),
+            (cls.PAGERDUTY, "pagerduty"),
+            (cls.SLACK, "slack"),
+            (cls.MSTEAMS, "msteams"),
+            (cls.SENTRY_APP, "sentry_app"),
+            (cls.SENTRY_NOTIFICATION, "sentry_notification"),
+        )
+
+
+class ActionTarget(FlexibleIntEnum):
+    """
+    Explains the contents of target_identifier
+    """
+
+    # The target_identifier is a direct reference used by the service (e.g. email address, slack channel id)
+    SPECIFIC = 0
+    # The target_identifier is an id from the User model in Sentry
+    USER = 1
+    # The target_identifier is an id from the Team model in Sentry
+    TEAM = 2
+    # The target_identifier is an id from the SentryApp model in Sentry
+    SENTRY_APP = 3
+
+    @classmethod
+    def as_choices(cls) -> Iterable[Tuple[int, str]]:
+        return (
+            (cls.SPECIFIC, "specific"),
+            (cls.USER, "user"),
+            (cls.TEAM, "team"),
+            (cls.SENTRY_APP, "sentry_app"),
+        )
+
+
+class AbstractNotificationAction(Model):
+    """
+    Abstract model meant to retroactively create a contract for notification actions
+    (e.g. metric alerts, spike protection, etc.)
+    """
+
+    integration = FlexibleForeignKey("sentry.Integration", null=True)
+    sentry_app = FlexibleForeignKey("sentry.SentryApp", null=True)
+
+    # The type of service which will receive the action notification (e.g. slack, pagerduty, etc.)
+    type = models.SmallIntegerField(choices=ActionService.as_choices())
+    # The type of target which the service uses for routing (e.g. user, team)
+    target_type = models.SmallIntegerField(choices=ActionTarget.as_choices())
+    # Identifier of the target for the given service (e.g. slack channel id, pagerdutyservice id)
+    target_identifier = models.TextField(null=True)
+    # User-friendly name of the target (e.g. #slack-channel, pagerduty-service-name)
+    target_display = models.TextField(null=True)
+
+    @property
+    def service_type(self) -> ActionService:
+        """
+        Used for disambiguity of self.type
+        """
+        return self.type
+
+    class Meta:
+        abstract = True
+
+
+class ActionTrigger(FlexibleIntEnum):
+    """
+    The possible sources of action notifications.
+    Use values less than 100 here to avoid conflicts with getsentry's trigger values.
+    """
+
+    AUDIT_LOG = 0
+
+    @classmethod
+    def as_choices(cls) -> Iterable[Tuple[int, str]]:
+        return ((cls.AUDIT_LOG, "audit-log"),)
+
+
+class TriggerGenerator:
+    """
+    Allows NotificationAction.trigger_type to enforce extra triggers via
+    NotificationAction.register_trigger_type
+    """
+
+    def __iter__(self):
+        yield from NotificationAction._trigger_types
+
+
+@region_silo_only_model
+class NotificationActionProject(Model):
+    __include_in_export__ = True
+
+    project = FlexibleForeignKey("sentry.Project")
+    action = FlexibleForeignKey("sentry.NotificationAction")
+
+    class Meta:
+        app_label = "sentry"
+        db_table = "sentry_notificationactionproject"
+
+
+@region_silo_only_model
+class NotificationAction(AbstractNotificationAction):
+    """
+    Generic notification action model to programmatically route depending on the trigger (or source) for the notification
+    """
+
+    __include_in_export__ = True
+    __repr__ = sane_repr("id", "trigger_type", "service_type", "target_display")
+
+    _handlers: MutableMapping[ActionTrigger, MutableMapping[ActionTarget, Callable]] = defaultdict(
+        dict
+    )
+    _trigger_types: List[Tuple[int, str]] = ActionTrigger.as_choices()
+
+    organization = FlexibleForeignKey("sentry.Organization")
+    projects = models.ManyToManyField("sentry.Project", through=NotificationActionProject)
+    # TODO(Leander): After adding HybridCloudForeignKeys to AlertRuleTriggerAction, we can remove these lines
+    integration = None
+    integration_id = HybridCloudForeignKey(
+        "sentry.Integration", blank=True, null=True, on_delete="CASCADE"
+    )
+    sentry_app = None
+    sentry_app_id = HybridCloudForeignKey(
+        "sentry.SentryApp", blank=True, null=True, on_delete="CASCADE"
+    )
+
+    # The type of trigger which controls when the actions will go off (e.g. spike-protecion)
+    trigger_type = models.SmallIntegerField(choices=TriggerGenerator())
+
+    class Meta:
+        app_label = "sentry"
+        db_table = "sentry_notificationaction"
+
+    @classmethod
+    def register_trigger_type(
+        cls,
+        value: int,
+        display_text: str,
+    ):
+        """
+        This method is used for adding trigger types to this model from getsentry.
+        If the trigger is relevant to sentry as well, directly modify ActionTrigger.
+        """
+        cls._trigger_types = cls._trigger_types + ((value, display_text),)
+
+    @classmethod
+    def register_handler(
+        cls,
+        trigger_type: ActionTrigger,
+        service_type: ActionService,
+    ):
+        def inner(handler):
+            if service_type not in cls._handlers[trigger_type]:
+                cls._handlers[trigger_type][service_type] = handler
+            else:
+                raise AttributeError(
+                    f"Conflicting handler for trigger:service pair ({trigger_type}:{service_type})"
+                )
+            return handler
+
+        return inner
+
+    def fire(self, *args, **kwargs):
+        handler = NotificationAction._handlers[self.trigger_type].get(self.service_type)
+        if handler:
+            return handler(action=self, *args, **kwargs)
+        else:
+            logger.error(
+                "missing_handler",
+                extra={
+                    "id": self.id,
+                    "service_type": self.service_type,
+                    "trigger_type": self.trigger_type,
+                },
+            )

+ 33 - 0
src/sentry/testutils/factories.py

@@ -96,6 +96,12 @@ from sentry.models import (
 )
 from sentry.models.apikey import ApiKey
 from sentry.models.integrations.integration_feature import Feature, IntegrationTypes
+from sentry.models.notificationaction import (
+    ActionService,
+    ActionTarget,
+    ActionTrigger,
+    NotificationAction,
+)
 from sentry.models.releasefile import update_artifact_index
 from sentry.signals import project_created
 from sentry.snuba.dataset import Dataset
@@ -1390,3 +1396,30 @@ class Factories:
             owner = kwargs.pop("owner")
             kwargs["owner_id"] = owner.id if not isinstance(owner, int) else owner
         return SavedSearch.objects.create(name=name, **kwargs)
+
+    @staticmethod
+    @exempt_from_silo_limits()
+    def create_notification_action(
+        organization: Organization = None, projects: List[Project] = None, **kwargs
+    ):
+        if not organization:
+            organization = Factories.create_organization()
+
+        if not projects:
+            projects = []
+
+        action_kwargs = {
+            "organization": organization,
+            "type": ActionService.SENTRY_NOTIFICATION,
+            "target_type": ActionTarget.USER,
+            "target_identifier": 1,
+            "target_display": "Sentry User",
+            "trigger_type": ActionTrigger.AUDIT_LOG,
+            **kwargs,
+        }
+
+        action = NotificationAction.objects.create(**action_kwargs)
+        action.projects.add(*projects)
+        action.save()
+
+        return action

+ 5 - 0
src/sentry/testutils/fixtures.py

@@ -357,6 +357,11 @@ class Fixtures:
             alert_rule_trigger, target_identifier=target_identifier, **kwargs
         )
 
+    def create_notification_action(self, organization=None, projects=None, **kwargs):
+        return Factories.create_notification_action(
+            organization=organization, projects=projects, **kwargs
+        )
+
     def create_external_user(self, user=None, organization=None, integration=None, **kwargs):
         if not user:
             user = self.user

+ 12 - 0
tests/sentry/incidents/test_logic.py

@@ -1158,6 +1158,18 @@ class CreateAlertRuleTriggerActionTest(BaseAlertRuleTriggerActionTest, TestCase)
         assert action.target_type == target_type.value
         assert action.target_identifier == target_identifier
 
+    def test_exempt_service(self):
+        service_type = AlertRuleTriggerAction.Type.SENTRY_NOTIFICATION
+        target_type = AlertRuleTriggerAction.TargetType.SPECIFIC
+
+        with pytest.raises(InvalidTriggerActionError):
+            create_alert_rule_trigger_action(
+                trigger=self.trigger,
+                type=service_type,
+                target_type=target_type,
+                target_identifier=1,
+            )
+
     @responses.activate
     def test_slack(self):
         integration = Integration.objects.create(

+ 71 - 0
tests/sentry/models/test_notificationaction.py

@@ -0,0 +1,71 @@
+from collections import defaultdict
+from unittest.mock import MagicMock, patch
+
+import pytest
+from django.forms import ValidationError
+
+from sentry.models.notificationaction import NotificationAction
+from sentry.models.notificationaction import logger as NotificationActionLogger
+from sentry.testutils import TestCase
+
+
+@patch.dict(NotificationAction._handlers, defaultdict(dict))
+class NotificationActionTest(TestCase):
+    def setUp(self):
+        self.organization = self.create_organization(name="night city")
+        self.projects = [
+            self.create_project(name="netrunner", organization=self.organization),
+            self.create_project(name="edgerunner", organization=self.organization),
+        ]
+        self.notif_action = self.create_notification_action(
+            organization=self.organization, projects=self.projects
+        )
+        self.illegal_trigger = (-1, "sandevistan")
+        self.illegal_service = (-1, "braindance")
+        # Reset registers for tests
+
+    @patch.object(NotificationActionLogger, "error")
+    def test_register_handler_for_fire(self, mock_error_logger):
+        self.notif_action.type = self.illegal_service[0]
+        self.notif_action.trigger_type = self.illegal_trigger[0]
+        self.notif_action.save()
+        mock_handler = MagicMock()
+        NotificationAction.register_handler(
+            trigger_type=self.illegal_trigger[0],
+            service_type=self.illegal_service[0],
+        )(mock_handler)
+
+        self.notif_action.fire()
+        assert not mock_error_logger.called
+        assert mock_handler.called
+
+    def test_register_handler_for_overlap(self):
+        mock_handler = MagicMock()
+        NotificationAction.register_handler(
+            trigger_type=self.illegal_trigger[0],
+            service_type=self.illegal_service[0],
+        )(mock_handler)
+        with pytest.raises(AttributeError):
+            NotificationAction.register_handler(
+                trigger_type=self.illegal_trigger[0],
+                service_type=self.illegal_service[0],
+            )(mock_handler)
+
+    def test_register_trigger_type(self):
+        self.notif_action.trigger_type = self.illegal_trigger[0]
+        self.notif_action.save()
+        try:
+            self.notif_action.full_clean()
+        except ValidationError as err:
+            assert dict(err)["trigger_type"]
+        NotificationAction.register_trigger_type(*self.illegal_trigger)
+        self.notif_action.full_clean()
+
+    @patch.object(NotificationActionLogger, "error")
+    def test_fire_fails_silently(self, mock_error_logger):
+        self.notif_action.trigger_type = self.illegal_trigger[0]
+        self.notif_action.save()
+        # Misconfigured/missing handlers shouldn't raise errors, but should log errors
+        self.notif_action.fire()
+        assert mock_error_logger.called
+        mock_error_logger.reset_mock()