From f69d64ed0b0f33b472487583c511d11a421f5d5d Mon Sep 17 00:00:00 2001 From: Christinarlong <60594860+Christinarlong@users.noreply.github.com> Date: Fri, 22 Nov 2024 12:31:18 -0800 Subject: [PATCH] ref(sentry apps): Don't pass in event for send_alert_event (#80263) --- src/sentry/options/defaults.py | 7 + src/sentry/sentry_apps/tasks/__init__.py | 2 + src/sentry/sentry_apps/tasks/sentry_apps.py | 135 ++++++++++++++++-- .../sentry_apps/tasks/test_sentry_apps.py | 114 +++++++++++++-- 4 files changed, 239 insertions(+), 19 deletions(-) diff --git a/src/sentry/options/defaults.py b/src/sentry/options/defaults.py index 1ad12c2ae9bb50..58c544a62a7ac5 100644 --- a/src/sentry/options/defaults.py +++ b/src/sentry/options/defaults.py @@ -2921,6 +2921,13 @@ flags=FLAG_AUTOMATOR_MODIFIABLE, ) +# migrating send_alert_event task to not pass Event +register( + "sentryapps.send_alert_event.use-eventid", + type=Float, + default=0.0, + flags=FLAG_AUTOMATOR_MODIFIABLE, +) register( "transactions.do_post_process_in_save", default=0.0, diff --git a/src/sentry/sentry_apps/tasks/__init__.py b/src/sentry/sentry_apps/tasks/__init__.py index 62d7eee5405b7d..26909deb270a05 100644 --- a/src/sentry/sentry_apps/tasks/__init__.py +++ b/src/sentry/sentry_apps/tasks/__init__.py @@ -5,6 +5,7 @@ installation_webhook, process_resource_change_bound, send_alert_event, + send_alert_webhook, send_resource_change_webhook, workflow_notification, ) @@ -20,4 +21,5 @@ "send_resource_change_webhook", "workflow_notification", "process_service_hook", + "send_alert_webhook", ) diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 02ead328930772..62e35c5197cdaf 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -2,7 +2,7 @@ import logging from collections import defaultdict -from collections.abc import Mapping +from collections.abc import Mapping, Sequence from typing import Any from celery import Task, current_task @@ -13,8 +13,10 @@ from sentry.api.serializers import serialize from sentry.constants import SentryAppInstallationStatus from sentry.db.models.base import Model -from sentry.eventstore.models import Event, GroupEvent +from sentry.eventstore.models import BaseEvent, Event, GroupEvent +from sentry.features.rollout import in_random_rollout from sentry.hybridcloud.rpc.caching import region_caching_service +from sentry.issues.issue_occurrence import IssueOccurrence from sentry.models.activity import Activity from sentry.models.group import Group from sentry.models.organization import Organization @@ -34,6 +36,7 @@ from sentry.shared_integrations.exceptions import ApiHostError, ApiTimeoutError, ClientError from sentry.silo.base import SiloMode from sentry.tasks.base import instrumented_task, retry +from sentry.types.rules import RuleFuture from sentry.users.services.user.model import RpcUser from sentry.users.services.user.service import user_service from sentry.utils import metrics @@ -75,6 +78,8 @@ def _webhook_event_data( event: Event | GroupEvent, group_id: int, project_id: int ) -> dict[str, Any]: + from sentry.api.serializers.rest_framework import convert_dict_key_case, snake_to_camel_case + project = Project.objects.get_from_cache(id=project_id) organization = Organization.objects.get_from_cache(id=project.organization_id) @@ -91,6 +96,10 @@ def _webhook_event_data( "sentry-organization-event-detail", args=[organization.slug, group_id, event.event_id] ) ) + if hasattr(event, "occurrence") and event.occurrence is not None: + event_context["occurrence"] = convert_dict_key_case( + event.occurrence.to_dict(), snake_to_camel_case + ) # The URL has a regex OR in it ("|") which means `reverse` cannot generate # a valid URL (it can't know which option to pick). We have to manually @@ -100,6 +109,104 @@ def _webhook_event_data( return event_context +@instrumented_task(name="sentry.sentry_apps.tasks.sentry_apps.send_alert_webhook", **TASK_OPTIONS) +@retry_decorator +def send_alert_webhook( + rule: str, + sentry_app_id: int, + instance_id: str, + group_id: int, + occurrence_id: str, + additional_payload_key: str | None = None, + additional_payload: Mapping[str, Any] | None = None, + **kwargs: Any, +): + group = Group.objects.get_from_cache(id=group_id) + assert group, "Group must exist to get related attributes" + project = Project.objects.get_from_cache(id=group.project_id) + organization = Organization.objects.get_from_cache(id=project.organization_id) + extra = { + "sentry_app_id": sentry_app_id, + "project_slug": project.slug, + "organization_slug": organization.slug, + "rule": rule, + } + + sentry_app = app_service.get_sentry_app_by_id(id=sentry_app_id) + if sentry_app is None: + logger.info("event_alert_webhook.missing_sentry_app", extra=extra) + return + + installations = app_service.get_many( + filter=dict( + organization_id=organization.id, + app_ids=[sentry_app.id], + status=SentryAppInstallationStatus.INSTALLED, + ) + ) + if not installations: + logger.info("event_alert_webhook.missing_installation", extra=extra) + return + (install,) = installations + + nodedata = nodestore.backend.get( + BaseEvent.generate_node_id(project_id=project.id, event_id=instance_id) + ) + + if not nodedata: + extra = { + "event_id": instance_id, + "occurrence_id": occurrence_id, + "rule": rule, + "sentry_app": sentry_app.slug, + "group_id": group_id, + } + logger.info("send_alert_event.missing_event", extra=extra) + return + + occurrence = None + if occurrence_id: + occurrence = IssueOccurrence.fetch(occurrence_id, project_id=project.id) + + if not occurrence: + logger.info( + "send_alert_event.missing_occurrence", + extra={"occurrence_id": occurrence_id, "project_id": project.id}, + ) + return + + group_event = GroupEvent( + project_id=project.id, + event_id=instance_id, + group=group, + data=nodedata, + occurrence=occurrence, + ) + + event_context = _webhook_event_data(group_event, group.id, project.id) + + data = {"event": event_context, "triggered_rule": rule} + + # Attach extra payload to the webhook + if additional_payload_key and additional_payload: + data[additional_payload_key] = additional_payload + + request_data = AppPlatformEvent( + resource="event_alert", action="triggered", install=install, data=data + ) + + send_and_save_webhook_request(sentry_app, request_data) + + # On success, record analytic event for Alert Rule UI Component + if request_data.data.get("issue_alert"): + analytics.record( + "alert_rule_ui_component_webhook.sent", + organization_id=organization.id, + sentry_app_id=sentry_app_id, + event=f"{request_data.resource}.{request_data.action}", + ) + + @instrumented_task(name="sentry.sentry_apps.tasks.sentry_apps.send_alert_event", **TASK_OPTIONS) @retry_decorator def send_alert_event( @@ -426,7 +533,7 @@ def send_resource_change_webhook( metrics.incr("resource_change.processed", sample_rate=1.0, tags={"change_event": event}) -def notify_sentry_app(event: Event | GroupEvent, futures): +def notify_sentry_app(event: GroupEvent, futures: Sequence[RuleFuture]): for f in futures: if not f.kwargs.get("sentry_app"): continue @@ -446,12 +553,22 @@ def notify_sentry_app(event: Event | GroupEvent, futures): "settings": settings, } - send_alert_event.delay( - event=event, - rule=f.rule.label, - sentry_app_id=f.kwargs["sentry_app"].id, - **extra_kwargs, - ) + if in_random_rollout("sentryapps.send_alert_event.use-eventid"): + send_alert_webhook.delay( + instance_id=event.event_id, + group_id=event.group_id, + occurrence_id=event.occurrence_id if hasattr(event, "occurrence_id") else None, + rule=f.rule.label, + sentry_app_id=f.kwargs["sentry_app"].id, + **extra_kwargs, + ) + else: + send_alert_event.delay( + event=event, + rule=f.rule.label, + sentry_app_id=f.kwargs["sentry_app"].id, + **extra_kwargs, + ) def send_webhooks(installation: RpcSentryAppInstallation, event: str, **kwargs: Any) -> None: diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index cf3a15b61ab3ad..a0ddf7a6831555 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -12,11 +12,14 @@ from sentry import audit_log from sentry.api.serializers import serialize +from sentry.api.serializers.rest_framework import convert_dict_key_case, snake_to_camel_case from sentry.constants import SentryAppStatus +from sentry.eventstore.models import GroupEvent from sentry.eventstream.types import EventStreamEventType from sentry.integrations.models.utils import get_redis_key from sentry.integrations.notify_disable import notify_disable from sentry.integrations.request_buffer import IntegrationRequestBuffer +from sentry.issues.ingest import save_issue_occurrence from sentry.models.activity import Activity from sentry.models.auditlogentry import AuditLogEntry from sentry.models.rule import Rule @@ -28,6 +31,7 @@ notify_sentry_app, process_resource_change_bound, send_alert_event, + send_alert_webhook, send_webhooks, workflow_notification, ) @@ -47,6 +51,7 @@ from sentry.utils import json from sentry.utils.http import absolute_uri from sentry.utils.sentry_apps import SentryAppWebhookRequestsBuffer +from tests.sentry.issues.test_utils import OccurrenceTestMixin pytestmark = [requires_snuba] @@ -93,7 +98,7 @@ def __init__(self): MockResponse404 = MockResponse({}, {}, "", False, 404, raiseException, None) -class TestSendAlertEvent(TestCase): +class TestSendAlertEvent(TestCase, OccurrenceTestMixin): def setUp(self): self.sentry_app = self.create_sentry_app(organization=self.organization) self.rule = Rule.objects.create(project=self.project, label="Issa Rule") @@ -104,17 +109,36 @@ def setUp(self): @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen") def test_no_sentry_app(self, safe_urlopen): event = self.store_event(data={}, project_id=self.project.id) - send_alert_event(event, self.rule, 9999) + assert event.group is not None + group_event = GroupEvent.from_event(event, event.group) + send_alert_event(group_event, self.rule, 9999) + + assert not safe_urlopen.called + + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen") + def test_no_sentry_app_for_send_alert_event_v2(self, safe_urlopen): + event = self.store_event(data={}, project_id=self.project.id) + assert event.group is not None + group_event = GroupEvent.from_event(event, event.group) + send_alert_webhook( + instance_id=group_event.event_id, + group_id=group_event.group_id, + occurrence_id=None, + rule=self.rule, + sentry_app_id=9999, + ) assert not safe_urlopen.called @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen") def test_no_sentry_app_in_future(self, safe_urlopen): event = self.store_event(data={}, project_id=self.project.id) + assert event.group is not None + group_event = GroupEvent.from_event(event, event.group) rule_future = RuleFuture(rule=self.rule, kwargs={}) with self.tasks(): - notify_sentry_app(event, [rule_future]) + notify_sentry_app(group_event, [rule_future]) assert not safe_urlopen.called @@ -122,10 +146,12 @@ def test_no_sentry_app_in_future(self, safe_urlopen): def test_no_installation(self, safe_urlopen): sentry_app = self.create_sentry_app(organization=self.organization) event = self.store_event(data={}, project_id=self.project.id) + assert event.group is not None + group_event = GroupEvent.from_event(event, event.group) rule_future = RuleFuture(rule=self.rule, kwargs={"sentry_app": sentry_app}) with self.tasks(): - notify_sentry_app(event, [rule_future]) + notify_sentry_app(group_event, [rule_future]) assert not safe_urlopen.called @@ -134,14 +160,14 @@ def test_send_alert_event(self, safe_urlopen): event = self.store_event(data={}, project_id=self.project.id) assert event.group is not None group = event.group + group_event = GroupEvent.from_event(event, group) rule_future = RuleFuture(rule=self.rule, kwargs={"sentry_app": self.sentry_app}) with self.tasks(): - notify_sentry_app(event, [rule_future]) + notify_sentry_app(group_event, [rule_future]) ((args, kwargs),) = safe_urlopen.call_args_list data = json.loads(kwargs["data"]) - assert data == { "action": "triggered", "installation": {"uuid": self.install.uuid}, @@ -151,17 +177,18 @@ def test_send_alert_event(self, safe_urlopen): }, "actor": {"type": "application", "id": "sentry", "name": "Sentry"}, } - assert data["data"]["event"]["event_id"] == event.event_id + assert data["data"]["event"]["project"] == self.project.id + assert data["data"]["event"]["event_id"] == group_event.event_id assert data["data"]["event"]["url"] == absolute_uri( reverse( "sentry-api-0-project-event-details", - args=[self.organization.slug, self.project.slug, event.event_id], + args=[self.organization.slug, self.project.slug, group_event.event_id], ) ) assert data["data"]["event"]["web_url"] == absolute_uri( reverse( "sentry-organization-event-detail", - args=[self.organization.slug, group.id, event.event_id], + args=[self.organization.slug, group.id, group_event.event_id], ) ) assert data["data"]["event"]["issue_url"] == absolute_uri(f"/api/0/issues/{group.id}/") @@ -185,6 +212,9 @@ def test_send_alert_event(self, safe_urlopen): @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen", return_value=MockResponseInstance) def test_send_alert_event_with_additional_payload(self, safe_urlopen): event = self.store_event(data={}, project_id=self.project.id) + assert event.group is not None + + group_event = GroupEvent.from_event(event, event.group) settings = [ {"name": "alert_prefix", "value": "[Not Good]"}, {"name": "channel", "value": "#ignored-errors"}, @@ -199,7 +229,7 @@ def test_send_alert_event_with_additional_payload(self, safe_urlopen): ) with self.tasks(): - notify_sentry_app(event, [rule_future]) + notify_sentry_app(group_event, [rule_future]) ((args, kwargs),) = safe_urlopen.call_args_list payload = json.loads(kwargs["data"]) @@ -220,6 +250,70 @@ def test_send_alert_event_with_additional_payload(self, safe_urlopen): assert requests[0]["response_code"] == 200 assert requests[0]["event_type"] == "event_alert.triggered" + @override_options({"sentryapps.send_alert_event.use-eventid": 1.0}) + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen", return_value=MockResponseInstance) + def test_send_alert_event_with_groupevent(self, safe_urlopen): + event = self.store_event(data={}, project_id=self.project.id) + occurrence_data = self.build_occurrence_data( + event_id=event.event_id, project_id=self.project.id + ) + occurrence, group_info = save_issue_occurrence(occurrence_data=occurrence_data, event=event) + assert group_info is not None + + group_event = event.for_group(group_info.group) + group_event.occurrence = occurrence + rule_future = RuleFuture(rule=self.rule, kwargs={"sentry_app": self.sentry_app}) + + with self.tasks(): + notify_sentry_app(group_event, [rule_future]) + + ((args, kwargs),) = safe_urlopen.call_args_list + data = json.loads(kwargs["data"]) + assert data == { + "action": "triggered", + "installation": {"uuid": self.install.uuid}, + "data": { + "event": ANY, # tested below + "triggered_rule": self.rule.label, + }, + "actor": {"type": "application", "id": "sentry", "name": "Sentry"}, + } + assert data["data"]["event"]["project"] == self.project.id + assert data["data"]["event"]["event_id"] == group_event.event_id + assert data["data"]["event"]["url"] == absolute_uri( + reverse( + "sentry-api-0-project-event-details", + args=[self.organization.slug, self.project.slug, group_event.event_id], + ) + ) + assert data["data"]["event"]["web_url"] == absolute_uri( + reverse( + "sentry-organization-event-detail", + args=[self.organization.slug, group_event.group.id, group_event.event_id], + ) + ) + assert data["data"]["event"]["issue_url"] == absolute_uri( + f"/api/0/issues/{group_event.group.id}/" + ) + assert data["data"]["event"]["issue_id"] == str(group_event.group.id) + assert data["data"]["event"]["occurrence"] == convert_dict_key_case( + group_event.occurrence.to_dict(), snake_to_camel_case + ) + assert kwargs["headers"].keys() >= { + "Content-Type", + "Request-ID", + "Sentry-Hook-Resource", + "Sentry-Hook-Timestamp", + "Sentry-Hook-Signature", + } + + buffer = SentryAppWebhookRequestsBuffer(self.sentry_app) + requests = buffer.get_requests() + + assert len(requests) == 1 + assert requests[0]["response_code"] == 200 + assert requests[0]["event_type"] == "event_alert.triggered" + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen", return_value=MockResponseInstance) class TestProcessResourceChange(TestCase):