From 8299673c3603fc062f9e3a816a005b47133f69f0 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 28 Feb 2025 09:22:05 -0800 Subject: [PATCH 01/11] inital commit --- src/sentry/sentry_apps/metrics.py | 42 +++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 src/sentry/sentry_apps/metrics.py diff --git a/src/sentry/sentry_apps/metrics.py b/src/sentry/sentry_apps/metrics.py new file mode 100644 index 00000000000000..8c3f7f0401934d --- /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 | None = None + + def get_metric_key(self, outcome: EventLifecycleOutcome) -> str: + tokens = ("sentry_app", 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, + } From b5e833bf35c3bc1f83dcd49d6e5d945b0323ffa8 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 28 Feb 2025 12:31:11 -0800 Subject: [PATCH 02/11] add context manager for event webhooks --- src/sentry/sentry_apps/tasks/sentry_apps.py | 183 ++++++++++++-------- src/sentry/utils/sentry_apps/webhooks.py | 119 +++++++------ 2 files changed, 169 insertions(+), 133 deletions(-) diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 0470c36acf0da5..c8bcfc88c0b3cd 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}" + org = None - if event not in VALID_EVENTS: - return - - 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]): @@ -498,14 +527,16 @@ def send_webhooks(installation: RpcSentryAppInstallation, event: str, **kwargs: organization_id=installation.organization_id, actor_id=installation.id ) except ServiceHook.DoesNotExist: - logger.info( + raise SentryAppSentryError( "send_webhooks.missing_servicehook", - extra={"installation_id": installation.id, "event": event}, + webhook_context={"installation_id": installation.id, "event": event}, ) - return None if event not in servicehook.events: - return None + raise SentryAppSentryError( + "send_webhooks.event_not_in_servicehook", + webhook_context={"installation_id": installation.id, "event": event}, + ) # The service hook applies to all projects if there are no # ServiceHookProject records. Otherwise we want check if diff --git a/src/sentry/utils/sentry_apps/webhooks.py b/src/sentry/utils/sentry_apps/webhooks.py index 094e6b5c5a6852..caa70787d90677 100644 --- a/src/sentry/utils/sentry_apps/webhooks.py +++ b/src/sentry/utils/sentry_apps/webhooks.py @@ -126,69 +126,74 @@ 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 + with SentryAppInteractionEvent( + operation_type=SentryAppInteractionType.SEND_WEBHOOK, event_type=event + ).capture() as lifecycle: + buffer = SentryAppWebhookRequestsBuffer(sentry_app) - 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) + 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) + # 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: + 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 From f3dde2d9fb6ada3f884e5f15273b13e538bb414e Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 28 Feb 2025 12:36:53 -0800 Subject: [PATCH 03/11] add context manager for event webhooks --- src/sentry/sentry_apps/tasks/sentry_apps.py | 101 ++++++++++---------- 1 file changed, 53 insertions(+), 48 deletions(-) diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index c8bcfc88c0b3cd..3ab59465e2d913 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -521,56 +521,61 @@ def notify_sentry_app(event: GroupEvent, futures: Sequence[RuleFuture]): def send_webhooks(installation: RpcSentryAppInstallation, event: str, **kwargs: Any) -> None: - servicehook: ServiceHook - try: - servicehook = ServiceHook.objects.get( - organization_id=installation.organization_id, actor_id=installation.id - ) - except ServiceHook.DoesNotExist: - raise SentryAppSentryError( - "send_webhooks.missing_servicehook", - webhook_context={"installation_id": installation.id, "event": event}, - ) + with SentryAppInteractionEvent( + operation_type=SentryAppInteractionType.SEND_WEBHOOK, event_type=event + ).capture() as lifecycle: + servicehook: ServiceHook + extras = {"installation_id": installation.id, "event": event} + lifecycle.add_extras(extras) - if event not in servicehook.events: - raise SentryAppSentryError( - "send_webhooks.event_not_in_servicehook", - webhook_context={"installation_id": installation.id, "event": event}, - ) + try: + servicehook = ServiceHook.objects.get( + organization_id=installation.organization_id, actor_id=installation.id + ) + except ServiceHook.DoesNotExist: + lifecycle.record_failure("send_webhooks.missing_servicehook") + raise SentryAppSentryError("send_webhooks.missing_servicehook", webhook_context=extras) + + if event not in servicehook.events: + extras + lifecycle.record_failure("send_webhooks.event_not_in_servicehook") + 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 - # the event is within the allowed projects. - project_limited = ServiceHookProject.objects.filter(service_hook_id=servicehook.id).exists() - - # TODO(nola): This is disabled for now, because it could potentially affect internal integrations w/ error.created - # # If the event is error.created & the request is going out to the Org that owns the Sentry App, - # # Make sure we don't send the request, to prevent potential infinite loops - # if ( - # event == "error.created" - # and installation.organization_id == installation.sentry_app.owner_id - # ): - # # We just want to exclude error.created from the project that the integration lives in - # # Need to first implement project mapping for integration partners - # metrics.incr( - # "webhook_request.dropped", - # tags={"sentry_app": installation.sentry_app.id, "event": event}, - # ) - # return - - if not project_limited: - resource, action = event.split(".") - - kwargs["resource"] = resource - kwargs["action"] = action - kwargs["install"] = installation - - request_data = AppPlatformEvent(**kwargs) - send_and_save_webhook_request( - installation.sentry_app, - request_data, - installation.sentry_app.webhook_url, - ) + # The service hook applies to all projects if there are no + # ServiceHookProject records. Otherwise we want check if + # the event is within the allowed projects. + project_limited = ServiceHookProject.objects.filter(service_hook_id=servicehook.id).exists() + + # TODO(nola): This is disabled for now, because it could potentially affect internal integrations w/ error.created + # # If the event is error.created & the request is going out to the Org that owns the Sentry App, + # # Make sure we don't send the request, to prevent potential infinite loops + # if ( + # event == "error.created" + # and installation.organization_id == installation.sentry_app.owner_id + # ): + # # We just want to exclude error.created from the project that the integration lives in + # # Need to first implement project mapping for integration partners + # metrics.incr( + # "webhook_request.dropped", + # tags={"sentry_app": installation.sentry_app.id, "event": event}, + # ) + # return + + if not project_limited: + resource, action = event.split(".") + + kwargs["resource"] = resource + kwargs["action"] = action + kwargs["install"] = installation + + request_data = AppPlatformEvent(**kwargs) + send_and_save_webhook_request( + installation.sentry_app, + request_data, + installation.sentry_app.webhook_url, + ) @instrumented_task( From 757890f5bf94f1391a3f0d72ac58abad2663ffc6 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 28 Feb 2025 12:51:32 -0800 Subject: [PATCH 04/11] typing --- src/sentry/sentry_apps/metrics.py | 2 +- src/sentry/sentry_apps/tasks/sentry_apps.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sentry/sentry_apps/metrics.py b/src/sentry/sentry_apps/metrics.py index 8c3f7f0401934d..c39a34464bde31 100644 --- a/src/sentry/sentry_apps/metrics.py +++ b/src/sentry/sentry_apps/metrics.py @@ -20,7 +20,7 @@ class SentryAppInteractionEvent(EventLifecycleMetric): """An event under the Sentry App umbrella""" operation_type: SentryAppInteractionType - event_type: str | None = None + event_type: str def get_metric_key(self, outcome: EventLifecycleOutcome) -> str: tokens = ("sentry_app", str(outcome)) diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 3ab59465e2d913..9982928b1afefc 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -525,8 +525,8 @@ def send_webhooks(installation: RpcSentryAppInstallation, event: str, **kwargs: operation_type=SentryAppInteractionType.SEND_WEBHOOK, event_type=event ).capture() as lifecycle: servicehook: ServiceHook - extras = {"installation_id": installation.id, "event": event} - lifecycle.add_extras(extras) + extras: dict[str, int | str] = {"installation_id": installation.id, "event": event} + lifecycle.add_extras(extras=extras) try: servicehook = ServiceHook.objects.get( From e9dacdff66d9a3a26446f562a8404bd6219879ea Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 28 Feb 2025 13:16:23 -0800 Subject: [PATCH 05/11] record halts in sending webhook --- src/sentry/sentry_apps/metrics.py | 2 +- src/sentry/sentry_apps/tasks/sentry_apps.py | 100 +++++++++----------- src/sentry/utils/sentry_apps/webhooks.py | 14 ++- 3 files changed, 59 insertions(+), 57 deletions(-) diff --git a/src/sentry/sentry_apps/metrics.py b/src/sentry/sentry_apps/metrics.py index c39a34464bde31..73a351307d0dfa 100644 --- a/src/sentry/sentry_apps/metrics.py +++ b/src/sentry/sentry_apps/metrics.py @@ -23,7 +23,7 @@ class SentryAppInteractionEvent(EventLifecycleMetric): event_type: str def get_metric_key(self, outcome: EventLifecycleOutcome) -> str: - tokens = ("sentry_app", str(outcome)) + tokens = ("sentry_app", self.operation_type, str(outcome)) return ".".join(tokens) def get_event_type(self) -> str: diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 9982928b1afefc..51042cc289e8d2 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -521,61 +521,53 @@ def notify_sentry_app(event: GroupEvent, futures: Sequence[RuleFuture]): def send_webhooks(installation: RpcSentryAppInstallation, event: str, **kwargs: Any) -> None: - with SentryAppInteractionEvent( - operation_type=SentryAppInteractionType.SEND_WEBHOOK, event_type=event - ).capture() as lifecycle: - servicehook: ServiceHook - extras: dict[str, int | str] = {"installation_id": installation.id, "event": event} - lifecycle.add_extras(extras=extras) + 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: - lifecycle.record_failure("send_webhooks.missing_servicehook") - raise SentryAppSentryError("send_webhooks.missing_servicehook", webhook_context=extras) - - if event not in servicehook.events: - extras - lifecycle.record_failure("send_webhooks.event_not_in_servicehook") - 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 - # the event is within the allowed projects. - project_limited = ServiceHookProject.objects.filter(service_hook_id=servicehook.id).exists() - - # TODO(nola): This is disabled for now, because it could potentially affect internal integrations w/ error.created - # # If the event is error.created & the request is going out to the Org that owns the Sentry App, - # # Make sure we don't send the request, to prevent potential infinite loops - # if ( - # event == "error.created" - # and installation.organization_id == installation.sentry_app.owner_id - # ): - # # We just want to exclude error.created from the project that the integration lives in - # # Need to first implement project mapping for integration partners - # metrics.incr( - # "webhook_request.dropped", - # tags={"sentry_app": installation.sentry_app.id, "event": event}, - # ) - # return - - if not project_limited: - resource, action = event.split(".") - - kwargs["resource"] = resource - kwargs["action"] = action - kwargs["install"] = installation - - request_data = AppPlatformEvent(**kwargs) - send_and_save_webhook_request( - installation.sentry_app, - request_data, - installation.sentry_app.webhook_url, - ) + try: + servicehook = ServiceHook.objects.get( + organization_id=installation.organization_id, actor_id=installation.id + ) + except ServiceHook.DoesNotExist: + raise SentryAppSentryError("send_webhooks.missing_servicehook", webhook_context=extras) + + if event not in servicehook.events: + 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 + # the event is within the allowed projects. + project_limited = ServiceHookProject.objects.filter(service_hook_id=servicehook.id).exists() + + # TODO(nola): This is disabled for now, because it could potentially affect internal integrations w/ error.created + # # If the event is error.created & the request is going out to the Org that owns the Sentry App, + # # Make sure we don't send the request, to prevent potential infinite loops + # if ( + # event == "error.created" + # and installation.organization_id == installation.sentry_app.owner_id + # ): + # # We just want to exclude error.created from the project that the integration lives in + # # Need to first implement project mapping for integration partners + # metrics.incr( + # "webhook_request.dropped", + # tags={"sentry_app": installation.sentry_app.id, "event": event}, + # ) + # return + + if not project_limited: + resource, action = event.split(".") + + kwargs["resource"] = resource + kwargs["action"] = action + kwargs["install"] = installation + + request_data = AppPlatformEvent(**kwargs) + + send_and_save_webhook_request( + installation.sentry_app, + request_data, + installation.sentry_app.webhook_url, + ) @instrumented_task( diff --git a/src/sentry/utils/sentry_apps/webhooks.py b/src/sentry/utils/sentry_apps/webhooks.py index caa70787d90677..2f065d3f692864 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 @@ -166,6 +166,7 @@ def send_and_save_webhook_request( 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 @@ -187,13 +188,22 @@ def send_and_save_webhook_request( 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) - response.raise_for_status() + try: + response.raise_for_status() + except RequestException as e: + lifecycle.record_halt(e) + raise return response From a26919de011f52657aca5d3101210d09cb1a45c4 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 28 Feb 2025 13:25:49 -0800 Subject: [PATCH 06/11] update tests --- tests/sentry/sentry_apps/tasks/test_sentry_apps.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index e7772628f857e9..57e5ca45fcfd31 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -34,6 +34,7 @@ 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.cases import TestCase @@ -656,7 +657,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 +670,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 From fe4de7fdf455e874d6af76015fb12873056dff22 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Mon, 3 Mar 2025 10:24:50 -0800 Subject: [PATCH 07/11] update tests --- src/sentry/sentry_apps/tasks/sentry_apps.py | 1 - .../sentry_apps/tasks/test_sentry_apps.py | 60 +++++++++++++++++-- 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 51042cc289e8d2..91db93267ffe47 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -530,7 +530,6 @@ def send_webhooks(installation: RpcSentryAppInstallation, event: str, **kwargs: ) except ServiceHook.DoesNotExist: raise SentryAppSentryError("send_webhooks.missing_servicehook", webhook_context=extras) - if event not in servicehook.events: raise SentryAppSentryError("send_webhooks.event_not_in_servicehook", webhook_context=extras) diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index 57e5ca45fcfd31..f7ea681dcfdcdb 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -1,5 +1,6 @@ from collections import namedtuple from datetime import datetime, timedelta +from functools import reduce from unittest.mock import ANY, patch import pytest @@ -19,6 +20,7 @@ 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 @@ -37,6 +39,7 @@ 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_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 @@ -315,7 +318,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(): @@ -342,12 +346,29 @@ def test_group_created_sends_webhook(self, safe_urlopen): "Sentry-Hook-Timestamp", "Sentry-Hook-Signature", } + assert_success_metric(mock_record) + started_calls = reduce( + lambda acc, calls: (acc + 1 if calls.args[0] == EventLifecycleOutcome.SUCCESS else acc), + mock_record.mock_calls, + 0, + ) + success_calls = reduce( + lambda acc, calls: (acc + 1 if calls.args[0] == EventLifecycleOutcome.SUCCESS else acc), + mock_record.mock_calls, + 0, + ) + + assert started_calls == 3 + assert success_calls == 3 - def test_does_not_process_disallowed_event(self, safe_urlopen): + @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") - 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() @@ -358,6 +379,20 @@ 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) + started_calls = reduce( + lambda acc, calls: (acc + 1 if calls.args[0] == EventLifecycleOutcome.SUCCESS else acc), + mock_record.mock_calls, + 0, + ) + success_calls = reduce( + lambda acc, calls: (acc + 1 if calls.args[0] == EventLifecycleOutcome.SUCCESS else acc), + mock_record.mock_calls, + 0, + ) + + assert started_calls == 1 + assert success_calls == 1 @patch("sentry.sentry_apps.tasks.sentry_apps._process_resource_change") def test_process_resource_change_bound_passes_retry_object(self, process, safe_urlopen): @@ -370,7 +405,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"] ) @@ -416,6 +452,21 @@ def test_error_created_sends_webhook(self, safe_urlopen): "Sentry-Hook-Signature", } + assert_success_metric(mock_record) + started_calls = reduce( + lambda acc, calls: (acc + 1 if calls.args[0] == EventLifecycleOutcome.SUCCESS else acc), + mock_record.mock_calls, + 0, + ) + success_calls = reduce( + lambda acc, calls: (acc + 1 if calls.args[0] == EventLifecycleOutcome.SUCCESS else acc), + mock_record.mock_calls, + 0, + ) + + assert started_calls == 3 + assert success_calls == 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") @@ -877,6 +928,7 @@ def test_ignore_issue_alert(self, safe_urlopen): self.sentry_app.update(status=SentryAppStatus.INTERNAL) data = {"issue": serialize(self.issue)} # we don't raise errors for unpublished and internal apps + # with pytest.raises(SentryAppSentryError): for i in range(3): send_webhooks( installation=self.install, event="event.alert", data=data, actor=self.user From 3804a47f780dd400c7570412aeff0f6954da7ee1 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Mon, 3 Mar 2025 15:45:18 -0800 Subject: [PATCH 08/11] create helper for asserting count --- src/sentry/testutils/asserts.py | 11 +++ .../sentry_apps/tasks/test_sentry_apps.py | 81 +++++++++---------- 2 files changed, 50 insertions(+), 42 deletions(-) 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/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index f7ea681dcfdcdb..33a45fe4b4d4ad 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -1,6 +1,5 @@ from collections import namedtuple from datetime import datetime, timedelta -from functools import reduce from unittest.mock import ANY, patch import pytest @@ -39,7 +38,11 @@ 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_failure_metric, assert_success_metric +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 @@ -347,26 +350,29 @@ def test_group_created_sends_webhook(self, mock_record, safe_urlopen): "Sentry-Hook-Signature", } assert_success_metric(mock_record) - started_calls = reduce( - lambda acc, calls: (acc + 1 if calls.args[0] == EventLifecycleOutcome.SUCCESS else acc), - mock_record.mock_calls, - 0, + + # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (success) -> SEND_WEBHOOK (success) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=3 ) - success_calls = reduce( - lambda acc, calls: (acc + 1 if calls.args[0] == EventLifecycleOutcome.SUCCESS else acc), - mock_record.mock_calls, - 0, + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=3 ) - assert started_calls == 3 - assert success_calls == 3 - @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 + ) + @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): @@ -380,20 +386,15 @@ def test_does_not_process_sentry_apps_without_issue_webhooks(self, mock_record, assert len(safe_urlopen.mock_calls) == 0 assert_success_metric(mock_record) - started_calls = reduce( - lambda acc, calls: (acc + 1 if calls.args[0] == EventLifecycleOutcome.SUCCESS else acc), - mock_record.mock_calls, - 0, + + # PREPARE_WEBHOOK (success) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=1 ) - success_calls = reduce( - lambda acc, calls: (acc + 1 if calls.args[0] == EventLifecycleOutcome.SUCCESS else acc), - mock_record.mock_calls, - 0, + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=1 ) - assert started_calls == 1 - assert success_calls == 1 - @patch("sentry.sentry_apps.tasks.sentry_apps._process_resource_change") def test_process_resource_change_bound_passes_retry_object(self, process, safe_urlopen): group = self.create_group(project=self.project) @@ -453,20 +454,15 @@ def test_error_created_sends_webhook(self, mock_record, safe_urlopen): } assert_success_metric(mock_record) - started_calls = reduce( - lambda acc, calls: (acc + 1 if calls.args[0] == EventLifecycleOutcome.SUCCESS else acc), - mock_record.mock_calls, - 0, + + # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (success) -> SEND_WEBHOOK (success) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=3 ) - success_calls = reduce( - lambda acc, calls: (acc + 1 if calls.args[0] == EventLifecycleOutcome.SUCCESS else acc), - mock_record.mock_calls, - 0, + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=3 ) - assert started_calls == 3 - assert success_calls == 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") @@ -525,8 +521,9 @@ def setUp(self): ) @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, safe_urlopen): + def test_sends_webhooks_to_all_installs(self, mock_record, safe_urlopen): one_min_ago = before_now(minutes=1).isoformat() event = self.store_event( data={ @@ -927,12 +924,12 @@ def test_ignore_issue_alert(self, safe_urlopen): 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 - # with pytest.raises(SentryAppSentryError): - 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 From 48a67f5ab8d75417574343dc15c5ba156cf937e8 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Mon, 3 Mar 2025 16:27:29 -0800 Subject: [PATCH 09/11] add testing for send webhook --- src/sentry/utils/sentry_apps/webhooks.py | 1 - .../sentry/sentry_apps/tasks/test_sentry_apps.py | 16 ++++++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/sentry/utils/sentry_apps/webhooks.py b/src/sentry/utils/sentry_apps/webhooks.py index 2f065d3f692864..4d4ca8f0a2ff27 100644 --- a/src/sentry/utils/sentry_apps/webhooks.py +++ b/src/sentry/utils/sentry_apps/webhooks.py @@ -138,7 +138,6 @@ def send_and_save_webhook_request( slug = sentry_app.slug_for_metrics url = url or sentry_app.webhook_url assert url is not None - try: response = safe_urlopen( url=url, diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index 33a45fe4b4d4ad..3af621cae5b442 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -99,8 +99,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): @@ -552,6 +552,18 @@ def test_sends_webhooks_to_all_installs(self, mock_record, 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 (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=3 + ) + assert_count_of_metric( + mock_record=mock_record, outcome=EventLifecycleOutcome.HALTED, outcome_count=2 + ) + @control_silo_test class TestInstallationWebhook(TestCase): From 56d5e904e3347d410bd883a07d6ee8d3fb540e7a Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Mon, 3 Mar 2025 17:58:19 -0800 Subject: [PATCH 10/11] fix and add tests for event webhook SLOs --- src/sentry/sentry_apps/tasks/sentry_apps.py | 1 - .../sentry_apps/tasks/test_sentry_apps.py | 132 +++++++++++++++++- 2 files changed, 126 insertions(+), 7 deletions(-) diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 91db93267ffe47..3500dbd186459e 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -523,7 +523,6 @@ 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 diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index 3af621cae5b442..9fcaccbd606884 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -26,6 +26,7 @@ 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, @@ -501,6 +502,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, @@ -520,10 +527,6 @@ def setUp(self): slug=self.sentry_app_2.slug, ) - @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): one_min_ago = before_now(minutes=1).isoformat() event = self.store_event( data={ @@ -564,6 +567,121 @@ def test_sends_webhooks_to_all_installs(self, mock_record, safe_urlopen): mock_record=mock_record, outcome=EventLifecycleOutcome.HALTED, outcome_count=2 ) + @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_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={ + "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 + + 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): @@ -927,9 +1045,10 @@ 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 """ @@ -942,6 +1061,7 @@ def test_ignore_issue_alert(self, safe_urlopen): 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 From 2614eb5dc66d0cf3990a6dada38126802abefad0 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 4 Mar 2025 10:31:07 -0800 Subject: [PATCH 11/11] add test for lifecycle halt for published apps --- .../sentry_apps/tasks/test_sentry_apps.py | 71 +++++++++++++++++-- 1 file changed, 67 insertions(+), 4 deletions(-) diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index 9fcaccbd606884..ee410f37b893be 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -360,6 +360,22 @@ def test_group_created_sends_webhook(self, mock_record, safe_urlopen): mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=3 ) + @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) @@ -550,21 +566,68 @@ def test_sends_webhooks_to_all_installs(self, mock_record, safe_urlopen): 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 - assert_success_metric(mock_record) - # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (success) x 2 -> SEND_WEBHOOK (halt) x2 + # 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=3 + 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=2 + mock_record=mock_record, outcome=EventLifecycleOutcome.HALTED, outcome_count=3 ) @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen", return_value=MockResponseInstance)