diff --git a/src/sentry/sentry_apps/metrics.py b/src/sentry/sentry_apps/metrics.py new file mode 100644 index 00000000000000..73a351307d0dfa --- /dev/null +++ b/src/sentry/sentry_apps/metrics.py @@ -0,0 +1,42 @@ +from collections.abc import Mapping +from dataclasses import dataclass +from enum import StrEnum +from typing import Any + +from sentry.integrations.types import EventLifecycleOutcome +from sentry.integrations.utils.metrics import EventLifecycleMetric + + +class SentryAppInteractionType(StrEnum): + """Actions that Sentry Apps can do""" + + # Webhook actions + PREPARE_WEBHOOK = "prepare_webhook" + SEND_WEBHOOK = "send_webhook" + + +@dataclass +class SentryAppInteractionEvent(EventLifecycleMetric): + """An event under the Sentry App umbrella""" + + operation_type: SentryAppInteractionType + event_type: str + + def get_metric_key(self, outcome: EventLifecycleOutcome) -> str: + tokens = ("sentry_app", self.operation_type, str(outcome)) + return ".".join(tokens) + + def get_event_type(self) -> str: + return self.event_type if self.event_type else "" + + def get_metric_tags(self) -> Mapping[str, str]: + return { + "operation_type": self.operation_type, + "event_type": self.event_type, + } + + def get_extras(self) -> Mapping[str, Any]: + return { + "event_type": self.get_event_type(), + "operation_type": self.operation_type, + } diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 0470c36acf0da5..3500dbd186459e 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -5,6 +5,7 @@ from collections.abc import Mapping, Sequence from typing import Any +import sentry_sdk from celery import Task, current_task from django.urls import reverse from requests.exceptions import RequestException @@ -22,6 +23,7 @@ from sentry.models.organizationmapping import OrganizationMapping from sentry.models.project import Project from sentry.sentry_apps.api.serializers.app_platform_event import AppPlatformEvent +from sentry.sentry_apps.metrics import SentryAppInteractionEvent, SentryAppInteractionType from sentry.sentry_apps.models.sentry_app import VALID_EVENTS, SentryApp from sentry.sentry_apps.models.sentry_app_installation import SentryAppInstallation from sentry.sentry_apps.models.servicehook import ServiceHook, ServiceHookProject @@ -32,6 +34,7 @@ get_installation, get_installations_for_organization, ) +from sentry.sentry_apps.utils.errors import SentryAppSentryError from sentry.shared_integrations.exceptions import ApiHostError, ApiTimeoutError, ClientError from sentry.silo.base import SiloMode from sentry.tasks.base import instrumented_task, retry @@ -214,73 +217,88 @@ def _process_resource_change( retryer: Task | None = None, **kwargs: Any, ) -> None: - # The class is serialized as a string when enqueueing the class. - model: type[Event] | type[Model] = TYPES[sender] - instance: Event | Model | None = None - - project_id: int | None = kwargs.get("project_id", None) - group_id: int | None = kwargs.get("group_id", None) - if sender == "Error" and project_id and group_id: - # Read event from nodestore as Events are heavy in task messages. - nodedata = nodestore.backend.get(Event.generate_node_id(project_id, str(instance_id))) - if not nodedata: - extra = {"sender": sender, "action": action, "event_id": instance_id} - logger.info("process_resource_change.event_missing_event", extra=extra) + with SentryAppInteractionEvent( + operation_type=SentryAppInteractionType.PREPARE_WEBHOOK, + event_type=f"process_resource_change.{sender}_{action}", + ).capture() as lifecycle: + + # The class is serialized as a string when enqueueing the class. + model: type[Event] | type[Model] = TYPES[sender] + instance: Event | Model | None = None + + project_id: int | None = kwargs.get("project_id", None) + group_id: int | None = kwargs.get("group_id", None) + if sender == "Error" and project_id and group_id: + # Read event from nodestore as Events are heavy in task messages. + nodedata = nodestore.backend.get(Event.generate_node_id(project_id, str(instance_id))) + if not nodedata: + extra = {"sender": sender, "action": action, "event_id": instance_id} + lifecycle.record_failure( + failure_reason="process_resource_change.event_missing_event", extra=extra + ) + return + instance = Event( + project_id=project_id, group_id=group_id, event_id=str(instance_id), data=nodedata + ) + name = sender.lower() + else: + # Some resources are named differently than their model. eg. Group vs Issue. + # Looks up the human name for the model. Defaults to the model name. + name = RESOURCE_RENAMES.get(model.__name__, model.__name__.lower()) + + # By default, use Celery's `current_task` but allow a value to be passed for the + # bound Task. + retryer = retryer or current_task + + # We may run into a race condition where this task executes before the + # transaction that creates the Group has committed. + if not issubclass(model, Event): + try: + instance = model.objects.get(id=instance_id) + except model.DoesNotExist as e: + # Explicitly requeue the task, so we don't report this to Sentry until + # we hit the max number of retries. + return retryer.retry(exc=e) + + event = f"{name}.{action}" + lifecycle.add_extras(extras={"event_name": event, "instance_id": instance_id}) + + if event not in VALID_EVENTS: + lifecycle.record_failure( + failure_reason="invalid_event", + ) return - instance = Event( - project_id=project_id, group_id=group_id, event_id=str(instance_id), data=nodedata - ) - name = sender.lower() - else: - # Some resources are named differently than their model. eg. Group vs Issue. - # Looks up the human name for the model. Defaults to the model name. - name = RESOURCE_RENAMES.get(model.__name__, model.__name__.lower()) - - # By default, use Celery's `current_task` but allow a value to be passed for the - # bound Task. - retryer = retryer or current_task - - # We may run into a race condition where this task executes before the - # transaction that creates the Group has committed. - if not issubclass(model, Event): - try: - instance = model.objects.get(id=instance_id) - except model.DoesNotExist as e: - # Explicitly requeue the task, so we don't report this to Sentry until - # we hit the max number of retries. - return retryer.retry(exc=e) - - event = f"{name}.{action}" - if event not in VALID_EVENTS: - return - - org = None + org = None - if isinstance(instance, (Group, Event, GroupEvent)): - org = Organization.objects.get_from_cache( - id=Project.objects.get_from_cache(id=instance.project_id).organization_id - ) - assert org, "organization must exist to get related sentry app installations" - - installations = [ - installation - for installation in app_service.installations_for_organization(organization_id=org.id) - if event in installation.sentry_app.events - ] - - for installation in installations: - data = {} - if isinstance(instance, (Event, GroupEvent)): - assert instance.group_id, "group id is required to create webhook event data" - data[name] = _webhook_event_data(instance, instance.group_id, instance.project_id) - else: - data[name] = serialize(instance) - - # Trigger a new task for each webhook - send_resource_change_webhook.delay( - installation_id=installation.id, event=event, data=data + if isinstance(instance, (Group, Event, GroupEvent)): + org = Organization.objects.get_from_cache( + id=Project.objects.get_from_cache(id=instance.project_id).organization_id ) + assert org, "organization must exist to get related sentry app installations" + + installations = [ + installation + for installation in app_service.installations_for_organization( + organization_id=org.id + ) + if event in installation.sentry_app.events + ] + + for installation in installations: + data = {} + if isinstance(instance, (Event, GroupEvent)): + assert instance.group_id, "group id is required to create webhook event data" + data[name] = _webhook_event_data( + instance, instance.group_id, instance.project_id + ) + else: + data[name] = serialize(instance) + + # Trigger a new task for each webhook + send_resource_change_webhook.delay( + installation_id=installation.id, event=event, data=data + ) @instrumented_task( @@ -448,17 +466,28 @@ def get_webhook_data( def send_resource_change_webhook( installation_id: int, event: str, data: dict[str, Any], *args: Any, **kwargs: Any ) -> None: - installation = app_service.installation_by_id(id=installation_id) - if not installation: - logger.info( - "send_resource_change_webhook.missing_installation", - extra={"installation_id": installation_id, "event": event}, - ) - return + with SentryAppInteractionEvent( + operation_type=SentryAppInteractionType.SEND_WEBHOOK, event_type=event + ).capture() as lifecycle: + installation = app_service.installation_by_id(id=installation_id) + if not installation: + logger.info( + "send_resource_change_webhook.missing_installation", + extra={"installation_id": installation_id, "event": event}, + ) + return - send_webhooks(installation, event, data=data) + try: + send_webhooks(installation, event, data=data) + except SentryAppSentryError as e: + sentry_sdk.capture_exception(e) + lifecycle.record_failure(e) + return None + except (ApiHostError, ApiTimeoutError, RequestException, ClientError) as e: + lifecycle.record_halt(e) + raise - metrics.incr("resource_change.processed", sample_rate=1.0, tags={"change_event": event}) + metrics.incr("resource_change.processed", sample_rate=1.0, tags={"change_event": event}) def notify_sentry_app(event: GroupEvent, futures: Sequence[RuleFuture]): @@ -493,19 +522,15 @@ def notify_sentry_app(event: GroupEvent, futures: Sequence[RuleFuture]): def send_webhooks(installation: RpcSentryAppInstallation, event: str, **kwargs: Any) -> None: servicehook: ServiceHook + extras: dict[str, int | str] = {"installation_id": installation.id, "event": event} try: servicehook = ServiceHook.objects.get( organization_id=installation.organization_id, actor_id=installation.id ) except ServiceHook.DoesNotExist: - logger.info( - "send_webhooks.missing_servicehook", - extra={"installation_id": installation.id, "event": event}, - ) - return None - + raise SentryAppSentryError("send_webhooks.missing_servicehook", webhook_context=extras) if event not in servicehook.events: - return None + raise SentryAppSentryError("send_webhooks.event_not_in_servicehook", webhook_context=extras) # The service hook applies to all projects if there are no # ServiceHookProject records. Otherwise we want check if @@ -535,6 +560,7 @@ def send_webhooks(installation: RpcSentryAppInstallation, event: str, **kwargs: kwargs["install"] = installation request_data = AppPlatformEvent(**kwargs) + send_and_save_webhook_request( installation.sentry_app, request_data, diff --git a/src/sentry/testutils/asserts.py b/src/sentry/testutils/asserts.py index d5d451ef82edfd..0d989032cdd6cf 100644 --- a/src/sentry/testutils/asserts.py +++ b/src/sentry/testutils/asserts.py @@ -1,3 +1,5 @@ +from functools import reduce + from django.http import StreamingHttpResponse from sentry.integrations.types import EventLifecycleOutcome @@ -110,3 +112,12 @@ def assert_middleware_metrics(middleware_calls): assert end1.args[0] == EventLifecycleOutcome.SUCCESS assert start2.args[0] == EventLifecycleOutcome.STARTED assert end2.args[0] == EventLifecycleOutcome.SUCCESS + + +def assert_count_of_metric(mock_record, outcome, outcome_count): + calls = reduce( + lambda acc, calls: (acc + 1 if calls.args[0] == outcome else acc), + mock_record.mock_calls, + 0, + ) + assert calls == outcome_count diff --git a/src/sentry/utils/sentry_apps/webhooks.py b/src/sentry/utils/sentry_apps/webhooks.py index 094e6b5c5a6852..4d4ca8f0a2ff27 100644 --- a/src/sentry/utils/sentry_apps/webhooks.py +++ b/src/sentry/utils/sentry_apps/webhooks.py @@ -4,7 +4,7 @@ from collections.abc import Callable from typing import TYPE_CHECKING, Concatenate, ParamSpec, TypeVar -from requests import Response +from requests import RequestException, Response from requests.exceptions import ConnectionError, Timeout from rest_framework import status @@ -126,69 +126,83 @@ def send_and_save_webhook_request( :param url: The URL to hit for this webhook if it is different from `sentry_app.webhook_url`. :return: Webhook response """ - buffer = SentryAppWebhookRequestsBuffer(sentry_app) + from sentry.sentry_apps.metrics import SentryAppInteractionEvent, SentryAppInteractionType - org_id = app_platform_event.install.organization_id event = f"{app_platform_event.resource}.{app_platform_event.action}" - slug = sentry_app.slug_for_metrics - url = url or sentry_app.webhook_url - assert url is not None - - try: - response = safe_urlopen( - url=url, - data=app_platform_event.body, - headers=app_platform_event.headers, - timeout=options.get("sentry-apps.webhook.timeout.sec"), - ) - except (Timeout, ConnectionError) as e: - error_type = e.__class__.__name__.lower() - logger.info( - "send_and_save_webhook_request.timeout", - extra={ - "error_type": error_type, - "organization_id": org_id, - "integration_slug": sentry_app.slug, - "url": url, - }, - ) - track_response_code(error_type, slug, event) + with SentryAppInteractionEvent( + operation_type=SentryAppInteractionType.SEND_WEBHOOK, event_type=event + ).capture() as lifecycle: + buffer = SentryAppWebhookRequestsBuffer(sentry_app) + + org_id = app_platform_event.install.organization_id + slug = sentry_app.slug_for_metrics + url = url or sentry_app.webhook_url + assert url is not None + try: + response = safe_urlopen( + url=url, + data=app_platform_event.body, + headers=app_platform_event.headers, + timeout=options.get("sentry-apps.webhook.timeout.sec"), + ) + except (Timeout, ConnectionError) as e: + error_type = e.__class__.__name__.lower() + lifecycle.add_extras( + { + "event": "send_and_save_webhook_request.timeout", + "error_type": error_type, + "organization_id": org_id, + "integration_slug": sentry_app.slug, + "url": url, + }, + ) + track_response_code(error_type, slug, event) + buffer.add_request( + response_code=TIMEOUT_STATUS_CODE, + org_id=org_id, + event=event, + url=url, + headers=app_platform_event.headers, + ) + record_timeout(sentry_app, org_id, e) + lifecycle.record_halt(e) + # Re-raise the exception because some of these tasks might retry on the exception + raise + + track_response_code(response.status_code, slug, event) buffer.add_request( - response_code=TIMEOUT_STATUS_CODE, + response_code=response.status_code, org_id=org_id, event=event, url=url, + error_id=response.headers.get("Sentry-Hook-Error"), + project_id=response.headers.get("Sentry-Hook-Project"), + response=response, headers=app_platform_event.headers, ) - record_timeout(sentry_app, org_id, e) - # Re-raise the exception because some of these tasks might retry on the exception - raise - - track_response_code(response.status_code, slug, event) - buffer.add_request( - response_code=response.status_code, - org_id=org_id, - event=event, - url=url, - error_id=response.headers.get("Sentry-Hook-Error"), - project_id=response.headers.get("Sentry-Hook-Project"), - response=response, - headers=app_platform_event.headers, - ) - # we don't disable alert rules for internal integrations - # so we don't want to consider responses related to them - # for the purpose of disabling integrations - if app_platform_event.action != "event.alert": - record_response_for_disabling_integration(sentry_app, org_id, response) - - if response.status_code == status.HTTP_503_SERVICE_UNAVAILABLE: - raise ApiHostError.from_request(response.request) - - elif response.status_code == status.HTTP_504_GATEWAY_TIMEOUT: - raise ApiTimeoutError.from_request(response.request) - - elif 400 <= response.status_code < 500: - raise ClientError(response.status_code, url, response=response) - - response.raise_for_status() - return response + # we don't disable alert rules for internal integrations + # so we don't want to consider responses related to them + # for the purpose of disabling integrations + if app_platform_event.action != "event.alert": + record_response_for_disabling_integration(sentry_app, org_id, response) + + if response.status_code == status.HTTP_503_SERVICE_UNAVAILABLE: + lifecycle.record_halt(halt_reason="send_and_save_webhook_request.got-503") + raise ApiHostError.from_request(response.request) + + elif response.status_code == status.HTTP_504_GATEWAY_TIMEOUT: + lifecycle.record_halt(halt_reason="send_and_save_webhook_request.got-504") + raise ApiTimeoutError.from_request(response.request) + + elif 400 <= response.status_code < 500: + lifecycle.record_halt( + halt_reason=f"send_and_save_webhook_request.got-{response.status_code}" + ) + raise ClientError(response.status_code, url, response=response) + + try: + response.raise_for_status() + except RequestException as e: + lifecycle.record_halt(e) + raise + return response diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index e7772628f857e9..ee410f37b893be 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -19,12 +19,14 @@ 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.integrations.types import EventLifecycleOutcome 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 from sentry.sentry_apps.models.sentry_app import SentryApp from sentry.sentry_apps.models.sentry_app_installation import SentryAppInstallation +from sentry.sentry_apps.models.servicehook import ServiceHook from sentry.sentry_apps.tasks.sentry_apps import ( build_comment_webhook, installation_webhook, @@ -34,8 +36,14 @@ send_webhooks, workflow_notification, ) +from sentry.sentry_apps.utils.errors import SentryAppSentryError from sentry.shared_integrations.exceptions import ClientError from sentry.tasks.post_process import post_process_group +from sentry.testutils.asserts import ( + assert_count_of_metric, + assert_failure_metric, + assert_success_metric, +) from sentry.testutils.cases import TestCase from sentry.testutils.helpers import with_feature from sentry.testutils.helpers.datetime import before_now, freeze_time @@ -92,8 +100,8 @@ def __init__(self): MockResponseWithHeadersInstance = MockResponse( headers, html_content, "", True, 400, raiseStatusFalse, RequestMock() ) -MockResponseInstance = MockResponse({}, {}, "", True, 200, raiseStatusFalse, None) -MockResponse404 = MockResponse({}, {}, "", False, 404, raiseException, None) +MockResponseInstance = MockResponse({}, b"{}", "", True, 200, raiseStatusFalse, None) +MockResponse404 = MockResponse({}, b'{"bruh": "bruhhhhhhh"}', "", False, 404, raiseException, None) class TestSendAlertEvent(TestCase, OccurrenceTestMixin): @@ -314,7 +322,8 @@ def setUp(self): organization=self.organization, slug=self.sentry_app.slug ) - def test_group_created_sends_webhook(self, safe_urlopen): + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + def test_group_created_sends_webhook(self, mock_record, safe_urlopen): event = self.store_event(data={}, project_id=self.project.id) assert event.group is not None with self.tasks(): @@ -341,12 +350,48 @@ def test_group_created_sends_webhook(self, safe_urlopen): "Sentry-Hook-Timestamp", "Sentry-Hook-Signature", } + assert_success_metric(mock_record) + + # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (success) -> SEND_WEBHOOK (success) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=3 + ) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=3 + ) - def test_does_not_process_disallowed_event(self, safe_urlopen): + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + def test_does_not_process_no_event(self, mock_record, safe_urlopen): + process_resource_change_bound( + action="created", sender="Error", instance_id=123, project_id=1, group_id=1 + ) + assert len(safe_urlopen.mock_calls) == 0 + + assert_failure_metric(mock_record, "process_resource_change.event_missing_event") + # PREPARE_WEBHOOK (failure) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=1 + ) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.FAILURE, outcome_count=1 + ) + + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + def test_does_not_process_disallowed_event(self, mock_record, safe_urlopen): process_resource_change_bound("delete", "Group", self.create_group().id) assert len(safe_urlopen.mock_calls) == 0 + assert_failure_metric(mock_record, "invalid_event") + + # PREPARE_WEBHOOK (failure) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=1 + ) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.FAILURE, outcome_count=1 + ) - def test_does_not_process_sentry_apps_without_issue_webhooks(self, safe_urlopen): + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + def test_does_not_process_sentry_apps_without_issue_webhooks(self, mock_record, safe_urlopen): with assume_test_silo_mode_of(SentryApp): SentryAppInstallation.objects.all().delete() SentryApp.objects.all().delete() @@ -357,6 +402,15 @@ def test_does_not_process_sentry_apps_without_issue_webhooks(self, safe_urlopen) process_resource_change_bound("created", "Group", self.create_group().id) assert len(safe_urlopen.mock_calls) == 0 + assert_success_metric(mock_record) + + # PREPARE_WEBHOOK (success) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=1 + ) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=1 + ) @patch("sentry.sentry_apps.tasks.sentry_apps._process_resource_change") def test_process_resource_change_bound_passes_retry_object(self, process, safe_urlopen): @@ -369,7 +423,8 @@ def test_process_resource_change_bound_passes_retry_object(self, process, safe_u assert isinstance(task, Task) @with_feature("organizations:integrations-event-hooks") - def test_error_created_sends_webhook(self, safe_urlopen): + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + def test_error_created_sends_webhook(self, mock_record, safe_urlopen): sentry_app = self.create_sentry_app( organization=self.project.organization, events=["error.created"] ) @@ -415,6 +470,16 @@ def test_error_created_sends_webhook(self, safe_urlopen): "Sentry-Hook-Signature", } + assert_success_metric(mock_record) + + # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (success) -> SEND_WEBHOOK (success) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=3 + ) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=3 + ) + # TODO(nola): Enable this test whenever we prevent infinite loops w/ error.created integrations @pytest.mark.skip(reason="enable this when/if we do prevent infinite error.created loops") @with_feature("organizations:integrations-event-hooks") @@ -453,6 +518,12 @@ def test_integration_org_error_created_doesnt_send_webhook(self, safe_urlopen): class TestSendResourceChangeWebhook(TestCase): def setUp(self): + pass + + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen", return_value=MockResponse404) + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + @with_feature("organizations:integrations-event-hooks") + def test_sends_webhooks_to_all_installs(self, mock_record, safe_urlopen): self.project = self.create_project() self.sentry_app_1 = self.create_sentry_app( organization=self.project.organization, @@ -472,9 +543,116 @@ def setUp(self): slug=self.sentry_app_2.slug, ) + one_min_ago = before_now(minutes=1).isoformat() + event = self.store_event( + data={ + "message": "Foo bar", + "exception": {"type": "Foo", "value": "oh no"}, + "level": "error", + "timestamp": one_min_ago, + }, + project_id=self.project.id, + assert_no_errors=False, + ) + + with self.tasks(): + post_process_group( + is_new=True, + is_regression=False, + is_new_group_environment=False, + cache_key=write_event_to_cache(event), + group_id=event.group_id, + project_id=self.project.id, + eventstream_type=EventStreamEventType.Error, + ) + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen", return_value=MockResponse404) + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + @with_feature("organizations:integrations-event-hooks") + def test_record_lifecycle_error_from_pubished_apps(self, mock_record, safe_urlopen): + self.project = self.create_project() + self.sentry_app_1 = self.create_sentry_app( + organization=self.project.organization, + events=["issue.created"], + webhook_url="https://google.com", + published=True, + ) + self.install_1 = self.create_sentry_app_installation( + organization=self.project.organization, slug=self.sentry_app_1.slug + ) + self.sentry_app_2 = self.create_sentry_app( + organization=self.project.organization, + events=["issue.created"], + webhook_url="https://apple.com", + ) + self.install_2 = self.create_sentry_app_installation( + organization=self.project.organization, + slug=self.sentry_app_2.slug, + ) + + one_min_ago = before_now(minutes=1).isoformat() + event = self.store_event( + data={ + "message": "Foo bar", + "exception": {"type": "Foo", "value": "oh no"}, + "level": "error", + "timestamp": one_min_ago, + }, + project_id=self.project.id, + assert_no_errors=False, + ) + + with self.tasks(): + post_process_group( + is_new=True, + is_regression=False, + is_new_group_environment=False, + cache_key=write_event_to_cache(event), + group_id=event.group_id, + project_id=self.project.id, + eventstream_type=EventStreamEventType.Error, + ) + + assert len(safe_urlopen.mock_calls) == 2 + call_urls = [call.kwargs["url"] for call in safe_urlopen.mock_calls] + assert self.sentry_app_1.webhook_url in call_urls + assert self.sentry_app_2.webhook_url in call_urls + + # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (success) from unpublished app & (halt) from published app -> SEND_WEBHOOK (halt) x2 + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=5 + ) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=2 + ) + # Because we only propogate up errors from send_and_save_webhook_request for published apps, unpublished apps will be recorded as successes + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.HALTED, outcome_count=3 + ) + + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen", return_value=MockResponseInstance) + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") @with_feature("organizations:integrations-event-hooks") - def test_sends_webhooks_to_all_installs(self, safe_urlopen): + def test_sends_webhooks_to_all_installs_success(self, mock_record, safe_urlopen): + self.project = self.create_project() + self.sentry_app_1 = self.create_sentry_app( + organization=self.project.organization, + events=["issue.created"], + webhook_url="https://google.com", + ) + self.install_1 = self.create_sentry_app_installation( + organization=self.project.organization, slug=self.sentry_app_1.slug + ) + self.sentry_app_2 = self.create_sentry_app( + organization=self.project.organization, + events=["issue.created"], + webhook_url="https://apple.com", + ) + self.install_2 = self.create_sentry_app_installation( + organization=self.project.organization, + slug=self.sentry_app_2.slug, + ) + one_min_ago = before_now(minutes=1).isoformat() event = self.store_event( data={ @@ -503,6 +681,70 @@ def test_sends_webhooks_to_all_installs(self, safe_urlopen): assert self.sentry_app_1.webhook_url in call_urls assert self.sentry_app_2.webhook_url in call_urls + assert_success_metric(mock_record) + # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (success) x 2 -> SEND_WEBHOOK (success) x2 + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=5 + ) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=5 + ) + + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + @with_feature("organizations:integrations-event-hooks") + def test_sends_webhooks_with_send_webhook_sentry_failure(self, mock_record): + + self.project = self.create_project() + self.sentry_app_1 = self.create_sentry_app( + organization=self.project.organization, + events=["issue.created"], + webhook_url="https://google.com", + ) + self.install_1 = self.create_sentry_app_installation( + organization=self.project.organization, slug=self.sentry_app_1.slug + ) + with assume_test_silo_mode_of(ServiceHook): + servicehook = ServiceHook.objects.get( + organization_id=self.install_1.organization_id, actor_id=self.install_1.id + ) + servicehook.events = [] + servicehook.save() + + one_min_ago = before_now(minutes=1).isoformat() + event = self.store_event( + data={ + "message": "Foo bar", + "exception": {"type": "Foo", "value": "oh no"}, + "level": "error", + "timestamp": one_min_ago, + }, + project_id=self.project.id, + assert_no_errors=False, + ) + + with self.tasks(): + post_process_group( + is_new=True, + is_regression=False, + is_new_group_environment=False, + cache_key=write_event_to_cache(event), + group_id=event.group_id, + project_id=self.project.id, + eventstream_type=EventStreamEventType.Error, + ) + + assert_success_metric(mock_record) + # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (failure) x 1 + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=2 + ) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=1 + ) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.FAILURE, outcome_count=1 + ) + @control_silo_test class TestInstallationWebhook(TestCase): @@ -656,7 +898,8 @@ def test_does_not_send_if_no_service_hook_exists(self, safe_urlopen): install = self.create_sentry_app_installation( organization=self.project.organization, slug=sentry_app.slug ) - workflow_notification(install.id, self.issue.id, "assigned", self.user.id) + with pytest.raises(SentryAppSentryError): + workflow_notification(install.id, self.issue.id, "assigned", self.user.id) assert not safe_urlopen.called def test_does_not_send_if_event_not_in_app_events(self, safe_urlopen): @@ -668,7 +911,8 @@ def test_does_not_send_if_event_not_in_app_events(self, safe_urlopen): install = self.create_sentry_app_installation( organization=self.project.organization, slug=sentry_app.slug ) - workflow_notification(install.id, self.issue.id, "assigned", self.user.id) + with pytest.raises(SentryAppSentryError): + workflow_notification(install.id, self.issue.id, "assigned", self.user.id) assert not safe_urlopen.called @@ -864,20 +1108,23 @@ def test_timeout_disable(self, safe_urlopen): self.sentry_app.refresh_from_db() # reload to get updated events assert len(self.sentry_app.events) == 0 # check that events are empty / app is disabled - @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen", side_effect=Timeout) @override_settings(BROKEN_TIMEOUT_THRESHOLD=3) - def test_ignore_issue_alert(self, safe_urlopen): + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen", side_effect=Timeout) + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + def test_ignore_issue_alert(self, mock_record, safe_urlopen): """ Test that the integration is disabled after BROKEN_TIMEOUT_THRESHOLD number of timeouts """ with assume_test_silo_mode_of(SentryApp): self.sentry_app.update(status=SentryAppStatus.INTERNAL) data = {"issue": serialize(self.issue)} - # we don't raise errors for unpublished and internal apps - for i in range(3): - send_webhooks( - installation=self.install, event="event.alert", data=data, actor=self.user - ) + # event.alert is not in the servicehook events + with pytest.raises(SentryAppSentryError): + for i in range(3): + send_webhooks( + installation=self.install, event="event.alert", data=data, actor=self.user + ) + assert not safe_urlopen.called assert [len(item) == 0 for item in self.integration_buffer._get_broken_range_from_buffer()] assert len(self.integration_buffer._get_all_from_buffer()) == 0