From 8299673c3603fc062f9e3a816a005b47133f69f0 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 28 Feb 2025 09:22:05 -0800 Subject: [PATCH 01/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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/32] 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) From c518499935682454e1ead9e868da96cac1a11916 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 4 Mar 2025 13:37:28 -0800 Subject: [PATCH 12/32] add context manager to alert prep task --- src/sentry/sentry_apps/tasks/sentry_apps.py | 148 +++++++++++--------- 1 file changed, 79 insertions(+), 69 deletions(-) diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 3500dbd186459e..069492dfec8236 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -123,91 +123,101 @@ def send_alert_webhook( 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: + with SentryAppInteractionEvent( + operation_type=SentryAppInteractionType.PREPARE_WEBHOOK, + event_type="send_alert_webhook", + ).capture() as lifecycle: + 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 = { - "event_id": instance_id, - "occurrence_id": occurrence_id, + "sentry_app_id": sentry_app_id, + "project_slug": project.slug, + "organization_slug": organization.slug, "rule": rule, - "sentry_app": sentry_app.slug, - "group_id": group_id, } - logger.info("send_alert_event.missing_event", extra=extra) - return + lifecycle.add_extras(extra) - occurrence = None - if occurrence_id: - occurrence = IssueOccurrence.fetch(occurrence_id, project_id=project.id) + sentry_app = app_service.get_sentry_app_by_id(id=sentry_app_id) + if sentry_app is None: + lifecycle.record_failure("event_alert_webhook.missing_sentry_app") + return - if not occurrence: - logger.info( - "send_alert_event.missing_occurrence", - extra={"occurrence_id": occurrence_id, "project_id": project.id}, + installations = app_service.get_many( + filter=dict( + organization_id=organization.id, + app_ids=[sentry_app.id], + status=SentryAppInstallationStatus.INSTALLED, ) + ) + if not installations: + lifecycle.record_failure("event_alert_webhook.missing_installation") return + (install,) = installations - group_event = GroupEvent( - project_id=project.id, - event_id=instance_id, - group=group, - data=nodedata, - occurrence=occurrence, - ) + nodedata = nodestore.backend.get( + BaseEvent.generate_node_id(project_id=project.id, event_id=instance_id) + ) - event_context = _webhook_event_data(group_event, group.id, project.id) + if not nodedata: + extra = { + "event_id": instance_id, + "occurrence_id": occurrence_id, + "rule": rule, + "sentry_app": sentry_app.slug, + "group_id": group_id, + } + lifecycle.record_failure(failure_reason="send_alert_event.missing_event", extra=extra) + return - data = {"event": event_context, "triggered_rule": rule} + occurrence = None + if occurrence_id: + occurrence = IssueOccurrence.fetch(occurrence_id, project_id=project.id) - # Attach extra payload to the webhook - if additional_payload_key and additional_payload: - data[additional_payload_key] = additional_payload + if not occurrence: + lifecycle.record_failure( + failure_reason="send_alert_event.missing_occurrence", + extra={"occurrence_id": occurrence_id, "project_id": project.id}, + ) + return - request_data = AppPlatformEvent( - resource="event_alert", action="triggered", install=install, data=data - ) + group_event = GroupEvent( + project_id=project.id, + event_id=instance_id, + group=group, + data=nodedata, + occurrence=occurrence, + ) - send_and_save_webhook_request(sentry_app, request_data) + event_context = _webhook_event_data(group_event, group.id, project.id) - # 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}", + 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 ) + try: + send_and_save_webhook_request(sentry_app, request_data) + except (ApiHostError, ApiTimeoutError, RequestException, ClientError) as e: + # record success as the preperation portion did not fail + lifecycle.record_success(e) + raise + + # 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}", + ) + def _process_resource_change( *, From b68ade67d6ce4b04f402e718f0a334f920347bea Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 4 Mar 2025 15:28:06 -0800 Subject: [PATCH 13/32] add context manager send alert webhook --- src/sentry/sentry_apps/tasks/sentry_apps.py | 5 +- .../sentry_apps/tasks/test_sentry_apps.py | 91 ++++++++++++++++++- 2 files changed, 89 insertions(+), 7 deletions(-) diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 069492dfec8236..7505b9f72605ac 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -141,7 +141,7 @@ def send_alert_webhook( sentry_app = app_service.get_sentry_app_by_id(id=sentry_app_id) if sentry_app is None: - lifecycle.record_failure("event_alert_webhook.missing_sentry_app") + lifecycle.record_failure("send_alert_event.missing_sentry_app") return installations = app_service.get_many( @@ -152,7 +152,7 @@ def send_alert_webhook( ) ) if not installations: - lifecycle.record_failure("event_alert_webhook.missing_installation") + lifecycle.record_failure("send_alert_event.missing_installation") return (install,) = installations @@ -503,6 +503,7 @@ def send_resource_change_webhook( def notify_sentry_app(event: GroupEvent, futures: Sequence[RuleFuture]): for f in futures: if not f.kwargs.get("sentry_app"): + logger.info("notify_sentry_app.future_missing_sentry_app", extra={"future": f}) continue extra_kwargs: dict[str, Any] = { diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index ee410f37b893be..432addbc78ba4a 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -113,7 +113,8 @@ def setUp(self): ) @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen") - def test_no_sentry_app_for_send_alert_event_v2(self, safe_urlopen): + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + def test_no_sentry_app_for_send_alert_event_v2(self, mock_record, 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) @@ -127,6 +128,42 @@ def test_no_sentry_app_for_send_alert_event_v2(self, safe_urlopen): assert not safe_urlopen.called + assert_failure_metric( + mock_record=mock_record, error_msg="send_alert_event.missing_sentry_app" + ) + # 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.utils.sentry_apps.webhooks.safe_urlopen", return_value=MockResponseInstance) + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + def test_missing_event(self, mock_record, safe_urlopen): + project = self.create_project() + issue = self.create_group(project=project) + + send_alert_webhook( + instance_id=123, + group_id=issue.id, + occurrence_id=None, + rule=self.rule, + sentry_app_id=self.sentry_app.id, + ) + + assert not safe_urlopen.called + + assert_failure_metric(mock_record=mock_record, error_msg="send_alert_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.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) @@ -140,7 +177,8 @@ def test_no_sentry_app_in_future(self, safe_urlopen): assert not safe_urlopen.called @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen") - def test_no_installation(self, safe_urlopen): + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + def test_no_installation(self, mock_record, 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 @@ -151,9 +189,20 @@ def test_no_installation(self, safe_urlopen): notify_sentry_app(group_event, [rule_future]) assert not safe_urlopen.called + assert_failure_metric( + mock_record=mock_record, error_msg="send_alert_event.missing_installation" + ) + # 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.utils.sentry_apps.webhooks.safe_urlopen", return_value=MockResponseInstance) - def test_send_alert_event(self, safe_urlopen): + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + def test_send_alert_event(self, mock_record, safe_urlopen): event = self.store_event(data={}, project_id=self.project.id) assert event.group is not None group = event.group @@ -206,8 +255,19 @@ def test_send_alert_event(self, safe_urlopen): assert requests[0]["response_code"] == 200 assert requests[0]["event_type"] == "event_alert.triggered" + # SLO validation + assert_success_metric(mock_record=mock_record) + # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (success) + 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=2 + ) + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen", return_value=MockResponseInstance) - def test_send_alert_event_with_additional_payload(self, safe_urlopen): + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + def test_send_alert_event_with_additional_payload(self, mock_record, safe_urlopen): event = self.store_event(data={}, project_id=self.project.id) assert event.group is not None @@ -247,8 +307,19 @@ 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" + # SLO validation + assert_success_metric(mock_record=mock_record) + # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (success) + 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=2 + ) + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen", return_value=MockResponseInstance) - def test_send_alert_event_with_groupevent(self, safe_urlopen): + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + def test_send_alert_event_with_groupevent(self, mock_record, 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 @@ -310,6 +381,16 @@ def test_send_alert_event_with_groupevent(self, safe_urlopen): assert requests[0]["response_code"] == 200 assert requests[0]["event_type"] == "event_alert.triggered" + # SLO validation + assert_success_metric(mock_record=mock_record) + # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (success) + 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=2 + ) + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen", return_value=MockResponseInstance) class TestProcessResourceChange(TestCase): From 72419db1be45b324ebd41da6c35b1183fdb9d177 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 4 Mar 2025 16:01:56 -0800 Subject: [PATCH 14/32] fix typing --- src/sentry/sentry_apps/tasks/sentry_apps.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 7505b9f72605ac..11b1692f072399 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -204,9 +204,9 @@ def send_alert_webhook( try: send_and_save_webhook_request(sentry_app, request_data) - except (ApiHostError, ApiTimeoutError, RequestException, ClientError) as e: + except (ApiHostError, ApiTimeoutError, RequestException, ClientError): # record success as the preperation portion did not fail - lifecycle.record_success(e) + lifecycle.record_success() raise # On success, record analytic event for Alert Rule UI Component From c261e2516401ad222f87db3b010b605441a063e0 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 4 Mar 2025 17:16:14 -0800 Subject: [PATCH 15/32] pr fixes for tasks --- src/sentry/sentry_apps/metrics.py | 16 ++++-- src/sentry/sentry_apps/tasks/sentry_apps.py | 49 +++++++++++-------- src/sentry/utils/sentry_apps/webhooks.py | 2 +- .../sentry_apps/tasks/test_sentry_apps.py | 13 ++--- 4 files changed, 49 insertions(+), 31 deletions(-) diff --git a/src/sentry/sentry_apps/metrics.py b/src/sentry/sentry_apps/metrics.py index 73a351307d0dfa..b3b8cdc2e78ad4 100644 --- a/src/sentry/sentry_apps/metrics.py +++ b/src/sentry/sentry_apps/metrics.py @@ -26,9 +26,6 @@ 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, @@ -37,6 +34,17 @@ def get_metric_tags(self) -> Mapping[str, str]: def get_extras(self) -> Mapping[str, Any]: return { - "event_type": self.get_event_type(), + "event_type": self.event_type, "operation_type": self.operation_type, } + + +class SentryAppWebhookFailureReason(StrEnum): + """Reasons why sentry app webhooks can fail""" + + MISSING_SENTRY_APP = "missing_sentry_app" + MISSING_INSTALLATION = "missing_installation" + MISSING_EVENT = "missing_event" + INVALID_EVENT = "invalid_event" + MISSING_SERVICEHOOK = "missing_servicehook" + EVENT_NOT_IN_SERVCEHOOK = "event_not_in_servicehook" diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 3500dbd186459e..1032cf9a3e246e 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -23,7 +23,11 @@ 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.metrics import ( + SentryAppInteractionEvent, + SentryAppInteractionType, + SentryAppWebhookFailureReason, +) 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 @@ -66,7 +70,7 @@ retry_decorator = retry( on=(RequestException, ApiHostError, ApiTimeoutError), - ignore=(ClientError,), + ignore=(ClientError, SentryAppSentryError), ) # We call some models by a different name, publicly, than their class name. @@ -232,11 +236,10 @@ def _process_resource_change( # 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 + raise SentryAppSentryError( + message=f"{SentryAppWebhookFailureReason.MISSING_EVENT}", + webhook_context={"sender": sender, "action": action, "event_id": instance_id}, ) - return instance = Event( project_id=project_id, group_id=group_id, event_id=str(instance_id), data=nodedata ) @@ -264,10 +267,9 @@ def _process_resource_change( lifecycle.add_extras(extras={"event_name": event, "instance_id": instance_id}) if event not in VALID_EVENTS: - lifecycle.record_failure( - failure_reason="invalid_event", + raise SentryAppSentryError( + message=f"{SentryAppWebhookFailureReason.INVALID_EVENT}", ) - return org = None @@ -308,9 +310,12 @@ def _process_resource_change( def process_resource_change_bound( self: Task, action: str, sender: str, instance_id: int, **kwargs: Any ) -> None: - _process_resource_change( - action=action, sender=sender, instance_id=instance_id, retryer=self, **kwargs - ) + try: + _process_resource_change( + action=action, sender=sender, instance_id=instance_id, retryer=self, **kwargs + ) + except SentryAppSentryError as e: + sentry_sdk.capture_exception(e) @instrumented_task( @@ -471,18 +476,18 @@ def send_resource_change_webhook( ).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}, + error = SentryAppSentryError( + message=f"{SentryAppWebhookFailureReason.MISSING_INSTALLATION}", + webhook_context={"installation_id": installation_id, "event": event}, ) - return + sentry_sdk.capture_exception(error) + raise error try: send_webhooks(installation, event, data=data) except SentryAppSentryError as e: sentry_sdk.capture_exception(e) - lifecycle.record_failure(e) - return None + raise except (ApiHostError, ApiTimeoutError, RequestException, ClientError) as e: lifecycle.record_halt(e) raise @@ -528,9 +533,13 @@ def send_webhooks(installation: RpcSentryAppInstallation, event: str, **kwargs: organization_id=installation.organization_id, actor_id=installation.id ) except ServiceHook.DoesNotExist: - raise SentryAppSentryError("send_webhooks.missing_servicehook", webhook_context=extras) + raise SentryAppSentryError( + message=SentryAppWebhookFailureReason.MISSING_SERVICEHOOK, webhook_context=extras + ) if event not in servicehook.events: - raise SentryAppSentryError("send_webhooks.event_not_in_servicehook", webhook_context=extras) + raise SentryAppSentryError( + message=SentryAppWebhookFailureReason.EVENT_NOT_IN_SERVCEHOOK, webhook_context=extras + ) # 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 4d4ca8f0a2ff27..b4bec519a07663 100644 --- a/src/sentry/utils/sentry_apps/webhooks.py +++ b/src/sentry/utils/sentry_apps/webhooks.py @@ -149,7 +149,7 @@ def send_and_save_webhook_request( error_type = e.__class__.__name__.lower() lifecycle.add_extras( { - "event": "send_and_save_webhook_request.timeout", + "reason": "send_and_save_webhook_request.timeout", "error_type": error_type, "organization_id": org_id, "integration_slug": sentry_app.slug, diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index ee410f37b893be..c29eac950f37fb 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -24,6 +24,7 @@ from sentry.models.activity import Activity from sentry.models.auditlogentry import AuditLogEntry from sentry.models.rule import Rule +from sentry.sentry_apps.metrics import SentryAppWebhookFailureReason 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 @@ -367,7 +368,9 @@ def test_does_not_process_no_event(self, mock_record, safe_urlopen): ) assert len(safe_urlopen.mock_calls) == 0 - assert_failure_metric(mock_record, "process_resource_change.event_missing_event") + assert_failure_metric( + mock_record, SentryAppSentryError(message=SentryAppWebhookFailureReason.MISSING_EVENT) + ) # PREPARE_WEBHOOK (failure) assert_count_of_metric( mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=1 @@ -380,7 +383,7 @@ def test_does_not_process_no_event(self, mock_record, safe_urlopen): 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") + assert_failure_metric(mock_record, SentryAppSentryError(message="invalid_event")) # PREPARE_WEBHOOK (failure) assert_count_of_metric( @@ -898,8 +901,7 @@ 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 ) - with pytest.raises(SentryAppSentryError): - workflow_notification(install.id, self.issue.id, "assigned", self.user.id) + 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): @@ -911,8 +913,7 @@ 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 ) - with pytest.raises(SentryAppSentryError): - workflow_notification(install.id, self.issue.id, "assigned", self.user.id) + workflow_notification(install.id, self.issue.id, "assigned", self.user.id) assert not safe_urlopen.called From ce6b02fdcd43e6d6e5f2db89ca1741954e6205e9 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 4 Mar 2025 17:52:26 -0800 Subject: [PATCH 16/32] fix tests via pr comments --- src/sentry/sentry_apps/metrics.py | 8 ++++++++ src/sentry/testutils/asserts.py | 12 ++++++++++++ src/sentry/utils/sentry_apps/webhooks.py | 11 ++++++++--- .../sentry_apps/tasks/test_sentry_apps.py | 18 ++++++++++++++++-- 4 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/sentry/sentry_apps/metrics.py b/src/sentry/sentry_apps/metrics.py index b3b8cdc2e78ad4..f0cff34b804529 100644 --- a/src/sentry/sentry_apps/metrics.py +++ b/src/sentry/sentry_apps/metrics.py @@ -42,9 +42,17 @@ def get_extras(self) -> Mapping[str, Any]: class SentryAppWebhookFailureReason(StrEnum): """Reasons why sentry app webhooks can fail""" + # Preperation fail MISSING_SENTRY_APP = "missing_sentry_app" MISSING_INSTALLATION = "missing_installation" MISSING_EVENT = "missing_event" INVALID_EVENT = "invalid_event" MISSING_SERVICEHOOK = "missing_servicehook" EVENT_NOT_IN_SERVCEHOOK = "event_not_in_servicehook" + + +class SentryAppWebhookHaltReason(StrEnum): + """Reasons why sentry app webhooks can halt""" + + GOT_CLIENT_ERROR = "got_client_error" + INTEGRATOR_ERROR = "integrator_error" diff --git a/src/sentry/testutils/asserts.py b/src/sentry/testutils/asserts.py index 0d989032cdd6cf..d48cd8f0b5be33 100644 --- a/src/sentry/testutils/asserts.py +++ b/src/sentry/testutils/asserts.py @@ -121,3 +121,15 @@ def assert_count_of_metric(mock_record, outcome, outcome_count): 0, ) assert calls == outcome_count + + +# Given messages_or_errors need to align 1:1 to the calls :bufo-big-eyes: +def assert_many_halt_metrics(mock_record, messages_or_errors): + halts = ( + call for call in mock_record.mock_calls if call.args[0] == EventLifecycleOutcome.HALTED + ) + for halt, error_msg in zip(halts, messages_or_errors): + if isinstance(error_msg, Exception): + assert isinstance(halt.args[1], type(error_msg)) + else: + assert halt.args[1] == error_msg diff --git a/src/sentry/utils/sentry_apps/webhooks.py b/src/sentry/utils/sentry_apps/webhooks.py index b4bec519a07663..bd42a9b6c38f05 100644 --- a/src/sentry/utils/sentry_apps/webhooks.py +++ b/src/sentry/utils/sentry_apps/webhooks.py @@ -15,6 +15,7 @@ from sentry.integrations.notify_disable import notify_disable from sentry.integrations.request_buffer import IntegrationRequestBuffer from sentry.models.organization import Organization +from sentry.sentry_apps.metrics import SentryAppWebhookHaltReason from sentry.sentry_apps.models.sentry_app import SentryApp, track_response_code from sentry.shared_integrations.exceptions import ApiHostError, ApiTimeoutError, ClientError from sentry.utils.audit import create_system_audit_entry @@ -187,16 +188,20 @@ 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") + lifecycle.record_halt( + halt_reason=f"send_and_save_webhook_request.{SentryAppWebhookHaltReason.INTEGRATOR_ERROR}" + ) 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") + lifecycle.record_halt( + halt_reason=f"send_and_save_webhook_request.{SentryAppWebhookHaltReason.INTEGRATOR_ERROR}" + ) 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}" + halt_reason=f"send_and_save_webhook_request.{SentryAppWebhookHaltReason.GOT_CLIENT_ERROR}_{response.status_code}" ) raise ClientError(response.status_code, url, response=response) diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index c29eac950f37fb..a7896ac1be2436 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -24,7 +24,7 @@ from sentry.models.activity import Activity from sentry.models.auditlogentry import AuditLogEntry from sentry.models.rule import Rule -from sentry.sentry_apps.metrics import SentryAppWebhookFailureReason +from sentry.sentry_apps.metrics import SentryAppWebhookFailureReason, SentryAppWebhookHaltReason 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 @@ -43,6 +43,7 @@ from sentry.testutils.asserts import ( assert_count_of_metric, assert_failure_metric, + assert_many_halt_metrics, assert_success_metric, ) from sentry.testutils.cases import TestCase @@ -383,8 +384,10 @@ def test_does_not_process_no_event(self, mock_record, safe_urlopen): 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, SentryAppSentryError(message="invalid_event")) + assert_failure_metric( + mock_record, SentryAppSentryError(message=SentryAppWebhookFailureReason.INVALID_EVENT) + ) # PREPARE_WEBHOOK (failure) assert_count_of_metric( mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=1 @@ -632,6 +635,14 @@ def test_record_lifecycle_error_from_pubished_apps(self, mock_record, safe_urlop assert_count_of_metric( mock_record=mock_record, outcome=EventLifecycleOutcome.HALTED, outcome_count=3 ) + assert_many_halt_metrics( + mock_record, + [ + f"send_and_save_webhook_request.{SentryAppWebhookHaltReason.GOT_CLIENT_ERROR}_404", + ClientError(status_code=404, url="example.com"), + f"send_and_save_webhook_request.{SentryAppWebhookHaltReason.GOT_CLIENT_ERROR}_404", + ], + ) @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen", return_value=MockResponseInstance) @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") @@ -737,6 +748,9 @@ def test_sends_webhooks_with_send_webhook_sentry_failure(self, mock_record): ) assert_success_metric(mock_record) + assert_failure_metric( + mock_record, SentryAppSentryError(SentryAppWebhookFailureReason.EVENT_NOT_IN_SERVCEHOOK) + ) # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (failure) x 1 assert_count_of_metric( mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=2 From 887b1a3ad97aba297745e87c0a81b4563e4f043b Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Wed, 5 Mar 2025 09:39:59 -0800 Subject: [PATCH 17/32] add assertionerror --- src/sentry/sentry_apps/tasks/sentry_apps.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 1032cf9a3e246e..e3b4795033bea2 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -70,7 +70,7 @@ retry_decorator = retry( on=(RequestException, ApiHostError, ApiTimeoutError), - ignore=(ClientError, SentryAppSentryError), + ignore=(ClientError, SentryAppSentryError, AssertionError), ) # We call some models by a different name, publicly, than their class name. @@ -314,8 +314,9 @@ def process_resource_change_bound( _process_resource_change( action=action, sender=sender, instance_id=instance_id, retryer=self, **kwargs ) - except SentryAppSentryError as e: + except (AssertionError, SentryAppSentryError) as e: sentry_sdk.capture_exception(e) + return @instrumented_task( From 91693e7892de658b9cdec22c9a1d2ee3fc71ab02 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Wed, 5 Mar 2025 09:49:14 -0800 Subject: [PATCH 18/32] typing --- tests/sentry/sentry_apps/tasks/test_sentry_apps.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index a7896ac1be2436..1f67b74f3627f9 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -639,7 +639,7 @@ def test_record_lifecycle_error_from_pubished_apps(self, mock_record, safe_urlop mock_record, [ f"send_and_save_webhook_request.{SentryAppWebhookHaltReason.GOT_CLIENT_ERROR}_404", - ClientError(status_code=404, url="example.com"), + ClientError(status_code="404", url="example.com"), f"send_and_save_webhook_request.{SentryAppWebhookHaltReason.GOT_CLIENT_ERROR}_404", ], ) From 46f0ceeff3208a70f9e9c4edae26ac81a8a94dcd Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Wed, 5 Mar 2025 10:56:31 -0800 Subject: [PATCH 19/32] pr schtuff --- src/sentry/sentry_apps/metrics.py | 1 + src/sentry/sentry_apps/tasks/sentry_apps.py | 20 +++--- .../sentry_apps/tasks/test_sentry_apps.py | 66 ++++++++++++++++++- 3 files changed, 73 insertions(+), 14 deletions(-) diff --git a/src/sentry/sentry_apps/metrics.py b/src/sentry/sentry_apps/metrics.py index f0cff34b804529..09bb964d143d1c 100644 --- a/src/sentry/sentry_apps/metrics.py +++ b/src/sentry/sentry_apps/metrics.py @@ -49,6 +49,7 @@ class SentryAppWebhookFailureReason(StrEnum): INVALID_EVENT = "invalid_event" MISSING_SERVICEHOOK = "missing_servicehook" EVENT_NOT_IN_SERVCEHOOK = "event_not_in_servicehook" + MISSING_ISSUE_OCCURRENCE = "missing_issue_occurrence" class SentryAppWebhookHaltReason(StrEnum): diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 4c860222e156ea..32eda9ba63cf03 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -137,7 +137,7 @@ def send_alert_webhook( organization = Organization.objects.get_from_cache(id=project.organization_id) extra = { "sentry_app_id": sentry_app_id, - "project_slug": project.slug, + "project_id": project.id, "organization_slug": organization.slug, "rule": rule, } @@ -145,8 +145,7 @@ def send_alert_webhook( sentry_app = app_service.get_sentry_app_by_id(id=sentry_app_id) if sentry_app is None: - lifecycle.record_failure("send_alert_event.missing_sentry_app") - return + raise SentryAppSentryError(message=SentryAppWebhookFailureReason.MISSING_SENTRY_APP) installations = app_service.get_many( filter=dict( @@ -156,8 +155,7 @@ def send_alert_webhook( ) ) if not installations: - lifecycle.record_failure("send_alert_event.missing_installation") - return + raise SentryAppSentryError(message=SentryAppWebhookFailureReason.MISSING_INSTALLATION) (install,) = installations nodedata = nodestore.backend.get( @@ -172,19 +170,19 @@ def send_alert_webhook( "sentry_app": sentry_app.slug, "group_id": group_id, } - lifecycle.record_failure(failure_reason="send_alert_event.missing_event", extra=extra) - return + raise SentryAppSentryError( + message=SentryAppWebhookFailureReason.MISSING_EVENT, webhook_context=extra + ) occurrence = None if occurrence_id: occurrence = IssueOccurrence.fetch(occurrence_id, project_id=project.id) if not occurrence: - lifecycle.record_failure( - failure_reason="send_alert_event.missing_occurrence", - extra={"occurrence_id": occurrence_id, "project_id": project.id}, + raise SentryAppSentryError( + message=SentryAppWebhookFailureReason.MISSING_ISSUE_OCCURRENCE, + webhook_context={"occurrence_id": occurrence_id, "project_id": project.id}, ) - return group_event = GroupEvent( project_id=project.id, diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index 0832d0a775250d..331bcb41ab80ae 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -131,7 +131,8 @@ def test_no_sentry_app_for_send_alert_event_v2(self, mock_record, safe_urlopen): assert not safe_urlopen.called assert_failure_metric( - mock_record=mock_record, error_msg="send_alert_event.missing_sentry_app" + mock_record=mock_record, + error_msg=f"send_alert_event.{SentryAppWebhookFailureReason.MISSING_SENTRY_APP}", ) # PREPARE_WEBHOOK (failure) assert_count_of_metric( @@ -157,7 +158,10 @@ def test_missing_event(self, mock_record, safe_urlopen): assert not safe_urlopen.called - assert_failure_metric(mock_record=mock_record, error_msg="send_alert_event.missing_event") + assert_failure_metric( + mock_record=mock_record, + error_msg=f"send_alert_event.{SentryAppWebhookFailureReason.MISSING_EVENT}", + ) # PREPARE_WEBHOOK (failure) assert_count_of_metric( mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=1 @@ -192,7 +196,8 @@ def test_no_installation(self, mock_record, safe_urlopen): assert not safe_urlopen.called assert_failure_metric( - mock_record=mock_record, error_msg="send_alert_event.missing_installation" + mock_record=mock_record, + error_msg=f"send_alert_event.{SentryAppWebhookFailureReason.MISSING_INSTALLATION}", ) # PREPARE_WEBHOOK (failure) assert_count_of_metric( @@ -393,6 +398,61 @@ def test_send_alert_event_with_groupevent(self, mock_record, safe_urlopen): mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=2 ) + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen", return_value=MockResponse404) + @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + def test_send_alert_event_with_3p_failure(self, mock_record, 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"}, + {"name": "best_emoji", "value": ":fire:"}, + {"name": "teamId", "value": 1}, + {"name": "assigneeId", "value": 3}, + ] + + rule_future = RuleFuture( + rule=self.rule, + kwargs={"sentry_app": self.sentry_app, "schema_defined_settings": settings}, + ) + + with self.tasks(): + notify_sentry_app(group_event, [rule_future]) + + ((args, kwargs),) = safe_urlopen.call_args_list + payload = json.loads(kwargs["data"]) + + assert payload["action"] == "triggered" + assert payload["data"]["triggered_rule"] == self.rule.label + assert payload["data"]["issue_alert"] == { + "id": self.rule.id, + "title": self.rule.label, + "sentry_app_id": self.sentry_app.id, + "settings": settings, + } + + 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" + + # SLO validation + assert_success_metric(mock_record=mock_record) + # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (halt) + 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.HALT, outcome_count=1 + ) + @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen", return_value=MockResponseInstance) class TestProcessResourceChange(TestCase): From 997e9546ee7bf9a13e9d3e15fdebf694e5ffe91f Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Wed, 5 Mar 2025 10:58:04 -0800 Subject: [PATCH 20/32] pr schtuff --- tests/sentry/sentry_apps/tasks/test_sentry_apps.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index 331bcb41ab80ae..e719a53286c4e3 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -43,6 +43,7 @@ from sentry.testutils.asserts import ( assert_count_of_metric, assert_failure_metric, + assert_halt_metric, assert_many_halt_metrics, assert_success_metric, ) @@ -442,6 +443,10 @@ def test_send_alert_event_with_3p_failure(self, mock_record, safe_urlopen): # SLO validation assert_success_metric(mock_record=mock_record) + assert_halt_metric( + mock_record=mock_record, + error_msg=f"send_and_save_webhook_request.{SentryAppWebhookHaltReason.GOT_CLIENT_ERROR}_{404}", + ) # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (halt) assert_count_of_metric( mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=2 From 85f71329307ca5c51b1a3ec4e116137f205a4edd Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Thu, 6 Mar 2025 02:50:30 -0800 Subject: [PATCH 21/32] shift up event naming to before lifecycle --- src/sentry/sentry_apps/metrics.py | 38 ++++++++++++++++ src/sentry/sentry_apps/tasks/sentry_apps.py | 44 ++++++++++--------- src/sentry/utils/sentry_apps/webhooks.py | 4 +- .../sentry_apps/tasks/test_sentry_apps.py | 10 +---- 4 files changed, 65 insertions(+), 31 deletions(-) diff --git a/src/sentry/sentry_apps/metrics.py b/src/sentry/sentry_apps/metrics.py index f0cff34b804529..2bc4068c2baeca 100644 --- a/src/sentry/sentry_apps/metrics.py +++ b/src/sentry/sentry_apps/metrics.py @@ -56,3 +56,41 @@ class SentryAppWebhookHaltReason(StrEnum): GOT_CLIENT_ERROR = "got_client_error" INTEGRATOR_ERROR = "integrator_error" + + +class SentryAppEventType(StrEnum): + """Events/features that Sentry Apps can do""" + + # event webhooks + ERROR_CREATED = "error.created" + ISSUE_CREATED = "issue.created" + + # issue alert webhooks + EVENT_ALERT_TRIGGERED = "event_alert.triggered" + + # external request webhooks + EXTERNAL_ISSUE_CREATED = "external_issue.created" + EXTERNAL_ISSUE_LINKED = "external_issue.linked" + SELECT_OPTIONS_REQUESTED = "select_options.requested" + + # metric alert webhooks + METRIC_ALERT_OPEN = "metric_alert.open" + METRIC_ALERT_RESOLVED = "metric_alert.resolved" + METRIC_ALERT_CRITICAL = "metric_alert.critical" + METRIC_ALERT_WARNING = "metric_alert.warning" + + # comment webhooks + COMMENT_CREATED = "comment.created" + COMMENT_UPDATED = "comment.updated" + COMMENT_DELETED = "comment.deleted" + + # installation webhooks + INSTALLATION_CREATED = "installation.created" + INSTALLATION_DELETED = "installation.deleted" + + # workflow notification + ISSUE_IGNORED = "issue.ignored" + ISSUE_ARCHIVED = "issue.archived" + ISSUE_UNRESOLVED = "issue.unresolved" + ISSUE_RESOLVED = "issue.resolved" + ISSUE_ASSIGNED = "issue.assigned" diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index e3b4795033bea2..d0a66a0893bc3d 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -24,6 +24,7 @@ from sentry.models.project import Project from sentry.sentry_apps.api.serializers.app_platform_event import AppPlatformEvent from sentry.sentry_apps.metrics import ( + SentryAppEventType, SentryAppInteractionEvent, SentryAppInteractionType, SentryAppWebhookFailureReason, @@ -221,17 +222,31 @@ def _process_resource_change( retryer: Task | None = None, **kwargs: Any, ) -> None: - 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 + # The class is serialized as a string when enqueueing the class. + model: type[Event] | type[Model] = TYPES[sender] + instance: Event | Model | None = None + # Make the event name first so we can add to metric tag + if sender == "Error": + 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()) + + event = f"{name}.{action}" + if event not in VALID_EVENTS: + raise SentryAppSentryError( + message=f"{SentryAppWebhookFailureReason.INVALID_EVENT}", + ) + with SentryAppInteractionEvent( + operation_type=SentryAppInteractionType.PREPARE_WEBHOOK, + event_type=SentryAppEventType(event), + ).capture(): 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))) @@ -243,11 +258,6 @@ def _process_resource_change( 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. @@ -263,14 +273,6 @@ def _process_resource_change( # 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: - raise SentryAppSentryError( - message=f"{SentryAppWebhookFailureReason.INVALID_EVENT}", - ) - org = None if isinstance(instance, (Group, Event, GroupEvent)): @@ -473,7 +475,7 @@ def send_resource_change_webhook( installation_id: int, event: str, data: dict[str, Any], *args: Any, **kwargs: Any ) -> None: with SentryAppInteractionEvent( - operation_type=SentryAppInteractionType.SEND_WEBHOOK, event_type=event + operation_type=SentryAppInteractionType.SEND_WEBHOOK, event_type=SentryAppEventType(event) ).capture() as lifecycle: installation = app_service.installation_by_id(id=installation_id) if not installation: diff --git a/src/sentry/utils/sentry_apps/webhooks.py b/src/sentry/utils/sentry_apps/webhooks.py index bd42a9b6c38f05..56427cbfddf2bd 100644 --- a/src/sentry/utils/sentry_apps/webhooks.py +++ b/src/sentry/utils/sentry_apps/webhooks.py @@ -15,7 +15,7 @@ from sentry.integrations.notify_disable import notify_disable from sentry.integrations.request_buffer import IntegrationRequestBuffer from sentry.models.organization import Organization -from sentry.sentry_apps.metrics import SentryAppWebhookHaltReason +from sentry.sentry_apps.metrics import SentryAppEventType, SentryAppWebhookHaltReason from sentry.sentry_apps.models.sentry_app import SentryApp, track_response_code from sentry.shared_integrations.exceptions import ApiHostError, ApiTimeoutError, ClientError from sentry.utils.audit import create_system_audit_entry @@ -131,7 +131,7 @@ def send_and_save_webhook_request( event = f"{app_platform_event.resource}.{app_platform_event.action}" with SentryAppInteractionEvent( - operation_type=SentryAppInteractionType.SEND_WEBHOOK, event_type=event + operation_type=SentryAppInteractionType.SEND_WEBHOOK, event_type=SentryAppEventType(event) ).capture() as lifecycle: buffer = SentryAppWebhookRequestsBuffer(sentry_app) diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index 1f67b74f3627f9..9b0085c6680427 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -385,15 +385,9 @@ 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, SentryAppSentryError(message=SentryAppWebhookFailureReason.INVALID_EVENT) - ) - # PREPARE_WEBHOOK (failure) + # We got an invalid event prior to lifecycle starting so we would exit early 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 + mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=0 ) @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") From 1ea4fab9876f05b97f96442c54bd8524646ec8de Mon Sep 17 00:00:00 2001 From: Christinarlong <60594860+Christinarlong@users.noreply.github.com> Date: Thu, 6 Mar 2025 09:47:46 -0800 Subject: [PATCH 22/32] Update src/sentry/sentry_apps/metrics.py Co-authored-by: Cathy Teng <70817427+cathteng@users.noreply.github.com> --- src/sentry/sentry_apps/metrics.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/sentry_apps/metrics.py b/src/sentry/sentry_apps/metrics.py index 2bc4068c2baeca..aa01ddaf610dc5 100644 --- a/src/sentry/sentry_apps/metrics.py +++ b/src/sentry/sentry_apps/metrics.py @@ -42,7 +42,7 @@ def get_extras(self) -> Mapping[str, Any]: class SentryAppWebhookFailureReason(StrEnum): """Reasons why sentry app webhooks can fail""" - # Preperation fail + # Preparation fail MISSING_SENTRY_APP = "missing_sentry_app" MISSING_INSTALLATION = "missing_installation" MISSING_EVENT = "missing_event" From cafe800ec9b4f0ff54ba5049bcbcf100706dd326 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Thu, 6 Mar 2025 10:54:34 -0800 Subject: [PATCH 23/32] update tests and remove extra params --- src/sentry/sentry_apps/tasks/sentry_apps.py | 49 +++++++------------ .../sentry_apps/tasks/test_sentry_apps.py | 18 ++++--- 2 files changed, 27 insertions(+), 40 deletions(-) diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 0f6b883faab580..c93d7695c8ab39 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -119,7 +119,7 @@ def _webhook_event_data( @instrumented_task(name="sentry.sentry_apps.tasks.sentry_apps.send_alert_webhook", **TASK_OPTIONS) @retry_decorator def send_alert_webhook( - rule: str, + rule_label: str, sentry_app_id: int, instance_id: str, group_id: int, @@ -130,17 +130,17 @@ def send_alert_webhook( ): with SentryAppInteractionEvent( operation_type=SentryAppInteractionType.PREPARE_WEBHOOK, - event_type="send_alert_webhook", + event_type=SentryAppEventType.EVENT_ALERT_TRIGGERED, ).capture() as lifecycle: 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 = { + extra: dict[str, int | str] = { "sentry_app_id": sentry_app_id, "project_id": project.id, "organization_slug": organization.slug, - "rule": rule, + "rule": rule_label, } lifecycle.add_extras(extra) @@ -164,16 +164,7 @@ def send_alert_webhook( ) if not nodedata: - extra = { - "event_id": instance_id, - "occurrence_id": occurrence_id, - "rule": rule, - "sentry_app": sentry_app.slug, - "group_id": group_id, - } - raise SentryAppSentryError( - message=SentryAppWebhookFailureReason.MISSING_EVENT, webhook_context=extra - ) + raise SentryAppSentryError(message=SentryAppWebhookFailureReason.MISSING_EVENT) occurrence = None if occurrence_id: @@ -181,8 +172,7 @@ def send_alert_webhook( if not occurrence: raise SentryAppSentryError( - message=SentryAppWebhookFailureReason.MISSING_ISSUE_OCCURRENCE, - webhook_context={"occurrence_id": occurrence_id, "project_id": project.id}, + message=SentryAppWebhookFailureReason.MISSING_ISSUE_OCCURRENCE ) group_event = GroupEvent( @@ -195,7 +185,7 @@ def send_alert_webhook( event_context = _webhook_event_data(group_event, group.id, project.id) - data = {"event": event_context, "triggered_rule": rule} + data = {"event": event_context, "triggered_rule": rule_label} # Attach extra payload to the webhook if additional_payload_key and additional_payload: @@ -205,21 +195,16 @@ def send_alert_webhook( resource="event_alert", action="triggered", install=install, data=data ) - try: - send_and_save_webhook_request(sentry_app, request_data) - except (ApiHostError, ApiTimeoutError, RequestException, ClientError): - # record success as the preperation portion did not fail - lifecycle.record_success() - raise + 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}", - ) + # 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}", + ) def _process_resource_change( @@ -531,7 +516,7 @@ def notify_sentry_app(event: GroupEvent, futures: Sequence[RuleFuture]): 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, + rule_label=f.rule.label, sentry_app_id=f.kwargs["sentry_app"].id, **extra_kwargs, ) diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index 8412d83b9c0983..c31622c4f1ef77 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -125,7 +125,7 @@ def test_no_sentry_app_for_send_alert_event_v2(self, mock_record, safe_urlopen): instance_id=group_event.event_id, group_id=group_event.group_id, occurrence_id=None, - rule=self.rule, + rule_label=self.rule.label, sentry_app_id=9999, ) @@ -133,7 +133,9 @@ def test_no_sentry_app_for_send_alert_event_v2(self, mock_record, safe_urlopen): assert_failure_metric( mock_record=mock_record, - error_msg=f"send_alert_event.{SentryAppWebhookFailureReason.MISSING_SENTRY_APP}", + error_msg=SentryAppSentryError( + message=SentryAppWebhookFailureReason.MISSING_SENTRY_APP + ), ) # PREPARE_WEBHOOK (failure) assert_count_of_metric( @@ -153,15 +155,14 @@ def test_missing_event(self, mock_record, safe_urlopen): instance_id=123, group_id=issue.id, occurrence_id=None, - rule=self.rule, + rule_label=self.rule.label, sentry_app_id=self.sentry_app.id, ) assert not safe_urlopen.called assert_failure_metric( - mock_record=mock_record, - error_msg=f"send_alert_event.{SentryAppWebhookFailureReason.MISSING_EVENT}", + mock_record, SentryAppSentryError(message=SentryAppWebhookFailureReason.MISSING_EVENT) ) # PREPARE_WEBHOOK (failure) assert_count_of_metric( @@ -198,7 +199,9 @@ def test_no_installation(self, mock_record, safe_urlopen): assert not safe_urlopen.called assert_failure_metric( mock_record=mock_record, - error_msg=f"send_alert_event.{SentryAppWebhookFailureReason.MISSING_INSTALLATION}", + error_msg=SentryAppSentryError( + message=SentryAppWebhookFailureReason.MISSING_INSTALLATION + ), ) # PREPARE_WEBHOOK (failure) assert_count_of_metric( @@ -438,7 +441,6 @@ def test_send_alert_event_with_3p_failure(self, mock_record, safe_urlopen): requests = buffer.get_requests() assert len(requests) == 1 - assert requests[0]["response_code"] == 200 assert requests[0]["event_type"] == "event_alert.triggered" # SLO validation @@ -455,7 +457,7 @@ def test_send_alert_event_with_3p_failure(self, mock_record, safe_urlopen): mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=1 ) assert_count_of_metric( - mock_record=mock_record, outcome=EventLifecycleOutcome.HALT, outcome_count=1 + mock_record=mock_record, outcome=EventLifecycleOutcome.HALTED, outcome_count=1 ) From 0a6c67bc3d6852b367a14aac64a54ef3b571904e Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Thu, 6 Mar 2025 14:06:28 -0800 Subject: [PATCH 24/32] remove extra context since sentry error will automagically catch --- src/sentry/sentry_apps/tasks/sentry_apps.py | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index d0a66a0893bc3d..b155b85ffa66bf 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -253,7 +253,6 @@ def _process_resource_change( if not nodedata: raise SentryAppSentryError( message=f"{SentryAppWebhookFailureReason.MISSING_EVENT}", - webhook_context={"sender": sender, "action": action, "event_id": instance_id}, ) instance = Event( project_id=project_id, group_id=group_id, event_id=str(instance_id), data=nodedata @@ -480,8 +479,7 @@ def send_resource_change_webhook( installation = app_service.installation_by_id(id=installation_id) if not installation: error = SentryAppSentryError( - message=f"{SentryAppWebhookFailureReason.MISSING_INSTALLATION}", - webhook_context={"installation_id": installation_id, "event": event}, + message=f"{SentryAppWebhookFailureReason.MISSING_INSTALLATION}" ) sentry_sdk.capture_exception(error) raise error @@ -530,19 +528,14 @@ 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: - raise SentryAppSentryError( - message=SentryAppWebhookFailureReason.MISSING_SERVICEHOOK, webhook_context=extras - ) + raise SentryAppSentryError(message=SentryAppWebhookFailureReason.MISSING_SERVICEHOOK) if event not in servicehook.events: - raise SentryAppSentryError( - message=SentryAppWebhookFailureReason.EVENT_NOT_IN_SERVCEHOOK, webhook_context=extras - ) + raise SentryAppSentryError(message=SentryAppWebhookFailureReason.EVENT_NOT_IN_SERVCEHOOK) # The service hook applies to all projects if there are no # ServiceHookProject records. Otherwise we want check if From d6dbd01128533f34c58f43bb3a648b98edd58ebb Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Thu, 6 Mar 2025 14:26:08 -0800 Subject: [PATCH 25/32] add ignore and capture --- src/sentry/tasks/base.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/sentry/tasks/base.py b/src/sentry/tasks/base.py index 3a2c404b4a7150..202c9766537189 100644 --- a/src/sentry/tasks/base.py +++ b/src/sentry/tasks/base.py @@ -144,6 +144,7 @@ def retry( on: type[Exception] | tuple[type[Exception], ...] = (Exception,), exclude: type[Exception] | tuple[type[Exception], ...] = (), ignore: type[Exception] | tuple[type[Exception], ...] = (), + ignore_and_capture: type[Exception] | tuple[type[Exception], ...] = (), ) -> Callable[..., Callable[..., Any]]: """ >>> @retry(on=(Exception,), exclude=(AnotherException,), ignore=(IgnorableException,)) @@ -161,6 +162,9 @@ def wrapped(*args, **kwargs): return func(*args, **kwargs) except ignore: return + except ignore_and_capture: + capture_exception() + return except exclude: raise except on as exc: From 33d5da0269282e33e5785d0453c310869d0ae9e6 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Thu, 6 Mar 2025 15:22:34 -0800 Subject: [PATCH 26/32] add tests for retry decorator --- tests/sentry/tasks/test_base.py | 58 ++++++++++++++++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/tests/sentry/tasks/test_base.py b/tests/sentry/tasks/test_base.py index 4db3893ae8cc34..b3c2181659a513 100644 --- a/tests/sentry/tasks/test_base.py +++ b/tests/sentry/tasks/test_base.py @@ -1,8 +1,10 @@ +from unittest.mock import patch + import pytest from django.test import override_settings from sentry.silo.base import SiloLimit, SiloMode -from sentry.tasks.base import instrumented_task +from sentry.tasks.base import instrumented_task, retry @instrumented_task(name="test.tasks.test_base.region_task", silo_mode=SiloMode.REGION) @@ -15,6 +17,30 @@ def control_task(param): return f"Control task {param}" +@retry(ignore_and_capture=(Exception,)) +@instrumented_task(name="test.tasks.test_base.ignore_and_capture_task", silo_mode=SiloMode.CONTROL) +def ignore_and_capture_retry_task(param): + raise Exception(param) + + +@retry(on=(Exception,)) +@instrumented_task(name="test.tasks.test_base.retry_task", silo_mode=SiloMode.CONTROL) +def retry_on_task(param): + raise Exception(param) + + +@retry(ignore=(Exception,)) +@instrumented_task(name="test.tasks.test_base.ignore_task", silo_mode=SiloMode.CONTROL) +def ignore_on_exception_task(param): + raise Exception(param) + + +@retry(exclude=(Exception,)) +@instrumented_task(name="test.tasks.test_base.exclude_task", silo_mode=SiloMode.CONTROL) +def exclude_on_exception_task(param): + raise Exception(param) + + @override_settings(SILO_MODE=SiloMode.REGION) def test_task_silo_limit_call_region(): result = region_task("hi") @@ -38,6 +64,36 @@ def test_task_silo_limit_call_monolith(): assert "Control task hi" == control_task("hi") +@patch("sentry.tasks.base.capture_exception") +def test_ignore_and_retry(capture_exception): + ignore_and_capture_retry_task("bruh") + + assert capture_exception.call_count == 1 + + +@patch("sentry.tasks.base.capture_exception") +def test_ignore_exception_retry(capture_exception): + ignore_on_exception_task("bruh") + + assert capture_exception.call_count == 0 + + +@patch("sentry.tasks.base.capture_exception") +def test_exclude_exception_retry(capture_exception): + with pytest.raises(Exception): + exclude_on_exception_task("bruh") + + assert capture_exception.call_count == 0 + + +@patch("sentry.tasks.base.capture_exception") +def test_retry_on(capture_exception): + with pytest.raises(Exception): + retry_on_task("bruh") + + assert capture_exception.call_count == 1 + + @pytest.mark.parametrize( "method_name", ( From eb8ced015ec947e72db0ac75382fdc28e6deef21 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Thu, 6 Mar 2025 15:58:19 -0800 Subject: [PATCH 27/32] add testing that we call retry --- tests/sentry/tasks/test_base.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/sentry/tasks/test_base.py b/tests/sentry/tasks/test_base.py index b3c2181659a513..527390c8d3b437 100644 --- a/tests/sentry/tasks/test_base.py +++ b/tests/sentry/tasks/test_base.py @@ -86,12 +86,15 @@ def test_exclude_exception_retry(capture_exception): assert capture_exception.call_count == 0 +@patch("sentry.tasks.base.current_task") @patch("sentry.tasks.base.capture_exception") -def test_retry_on(capture_exception): - with pytest.raises(Exception): - retry_on_task("bruh") +def test_retry_on(capture_exception, current_task): + + # In reality current_task.retry will cause the given exception to be re-raised but we patch it here so no need to .raises :bufo-shrug: + retry_on_task("bruh") assert capture_exception.call_count == 1 + assert current_task.retry.call_count == 1 @pytest.mark.parametrize( From c23e962c0bfb53acb701084171cec92a9f0c5da9 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Thu, 6 Mar 2025 17:36:16 -0800 Subject: [PATCH 28/32] remove try catches since the retry decorator willl handle for us, also instead of wrapping make new contxt manager --- src/sentry/sentry_apps/tasks/sentry_apps.py | 127 +++++++++--------- .../sentry_apps/tasks/test_sentry_apps.py | 34 +++-- 2 files changed, 78 insertions(+), 83 deletions(-) diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index b155b85ffa66bf..348758898538cb 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -5,7 +5,6 @@ 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 @@ -71,7 +70,8 @@ retry_decorator = retry( on=(RequestException, ApiHostError, ApiTimeoutError), - ignore=(ClientError, SentryAppSentryError, AssertionError), + ignore=(ClientError,), + ignore_and_capture=(SentryAppSentryError, AssertionError), ) # We call some models by a different name, publicly, than their class name. @@ -311,13 +311,9 @@ def _process_resource_change( def process_resource_change_bound( self: Task, action: str, sender: str, instance_id: int, **kwargs: Any ) -> None: - try: - _process_resource_change( - action=action, sender=sender, instance_id=instance_id, retryer=self, **kwargs - ) - except (AssertionError, SentryAppSentryError) as e: - sentry_sdk.capture_exception(e) - return + _process_resource_change( + action=action, sender=sender, instance_id=instance_id, retryer=self, **kwargs + ) @instrumented_task( @@ -475,25 +471,16 @@ def send_resource_change_webhook( ) -> None: with SentryAppInteractionEvent( operation_type=SentryAppInteractionType.SEND_WEBHOOK, event_type=SentryAppEventType(event) - ).capture() as lifecycle: + ).capture(): installation = app_service.installation_by_id(id=installation_id) if not installation: - error = SentryAppSentryError( + raise SentryAppSentryError( message=f"{SentryAppWebhookFailureReason.MISSING_INSTALLATION}" ) - sentry_sdk.capture_exception(error) - raise error - try: - send_webhooks(installation, event, data=data) - except SentryAppSentryError as e: - sentry_sdk.capture_exception(e) - raise - except (ApiHostError, ApiTimeoutError, RequestException, ClientError) as e: - lifecycle.record_halt(e) - raise + send_webhooks(installation, event, data=data) - 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]): @@ -527,51 +514,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(message=SentryAppWebhookFailureReason.MISSING_SERVICEHOOK) - if event not in servicehook.events: - raise SentryAppSentryError(message=SentryAppWebhookFailureReason.EVENT_NOT_IN_SERVCEHOOK) - - # 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, + if event not in VALID_EVENTS: + raise SentryAppSentryError( + message=f"{SentryAppWebhookFailureReason.INVALID_EVENT}", ) + with SentryAppInteractionEvent( + operation_type=SentryAppInteractionType.SEND_WEBHOOK, event_type=SentryAppEventType(event) + ).capture(): + servicehook: ServiceHook + try: + servicehook = ServiceHook.objects.get( + organization_id=installation.organization_id, actor_id=installation.id + ) + except ServiceHook.DoesNotExist: + raise SentryAppSentryError(message=SentryAppWebhookFailureReason.MISSING_SERVICEHOOK) + if event not in servicehook.events: + raise SentryAppSentryError( + message=SentryAppWebhookFailureReason.EVENT_NOT_IN_SERVCEHOOK + ) + + # 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( "sentry.sentry_apps.tasks.sentry_apps.create_or_update_service_hooks_for_sentry_app", diff --git a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py index 9b0085c6680427..f0f3b40d242215 100644 --- a/tests/sentry/sentry_apps/tasks/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/tasks/test_sentry_apps.py @@ -354,12 +354,12 @@ def test_group_created_sends_webhook(self, mock_record, safe_urlopen): } assert_success_metric(mock_record) - # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (success) -> SEND_WEBHOOK (success) + # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (success) -> SEND_WEBHOOK (success) -> SEND_WEBHOOK (success) assert_count_of_metric( - mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=3 + mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=4 ) assert_count_of_metric( - mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=3 + mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=4 ) @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") @@ -472,12 +472,12 @@ def test_error_created_sends_webhook(self, mock_record, safe_urlopen): assert_success_metric(mock_record) - # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (success) -> SEND_WEBHOOK (success) + # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (success) -> SEND_WEBHOOK (success) -> SEND_WEBHOOK (success) assert_count_of_metric( - mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=3 + mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=4 ) assert_count_of_metric( - mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=3 + mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=4 ) # TODO(nola): Enable this test whenever we prevent infinite loops w/ error.created integrations @@ -618,22 +618,20 @@ def test_record_lifecycle_error_from_pubished_apps(self, mock_record, safe_urlop 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 + # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (success) x2 -> SEND_WEBHOOK (success) x2 -> SEND_WEBHOOK (halt) x2 assert_count_of_metric( - mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=5 + mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=7 ) assert_count_of_metric( - mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=2 + mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=5 ) - # 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 + mock_record=mock_record, outcome=EventLifecycleOutcome.HALTED, outcome_count=2 ) assert_many_halt_metrics( mock_record, [ f"send_and_save_webhook_request.{SentryAppWebhookHaltReason.GOT_CLIENT_ERROR}_404", - ClientError(status_code="404", url="example.com"), f"send_and_save_webhook_request.{SentryAppWebhookHaltReason.GOT_CLIENT_ERROR}_404", ], ) @@ -690,12 +688,12 @@ def test_sends_webhooks_to_all_installs_success(self, mock_record, safe_urlopen) 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 + # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (success) x 2 -> SEND_WEBHOOK (success) x2 -> SEND_WEBHOOK (success) x2 assert_count_of_metric( - mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=5 + mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=7 ) assert_count_of_metric( - mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=5 + mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=7 ) @patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") @@ -745,12 +743,12 @@ def test_sends_webhooks_with_send_webhook_sentry_failure(self, mock_record): assert_failure_metric( mock_record, SentryAppSentryError(SentryAppWebhookFailureReason.EVENT_NOT_IN_SERVCEHOOK) ) - # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (failure) x 1 + # PREPARE_WEBHOOK (success) -> SEND_WEBHOOK (success) x 1 -> SEND_WEBHOOK (failure) assert_count_of_metric( - mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=2 + mock_record=mock_record, outcome=EventLifecycleOutcome.STARTED, outcome_count=3 ) assert_count_of_metric( - mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=1 + mock_record=mock_record, outcome=EventLifecycleOutcome.SUCCESS, outcome_count=2 ) assert_count_of_metric( mock_record=mock_record, outcome=EventLifecycleOutcome.FAILURE, outcome_count=1 From 83e94bd7d7ceba1f11bf6fa013036f529b1f9a43 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 7 Mar 2025 12:41:20 -0800 Subject: [PATCH 29/32] do event checking with cast --- src/sentry/sentry_apps/tasks/sentry_apps.py | 22 +++++++++++---------- src/sentry/utils/sentry_apps/webhooks.py | 17 +++++++++++++--- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 348758898538cb..9760b8f4b552f8 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -28,7 +28,7 @@ SentryAppInteractionType, SentryAppWebhookFailureReason, ) -from sentry.sentry_apps.models.sentry_app import VALID_EVENTS, SentryApp +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, ServiceHookProject from sentry.sentry_apps.services.app.model import RpcSentryAppInstallation @@ -234,15 +234,15 @@ def _process_resource_change( # Looks up the human name for the model. Defaults to the model name. name = RESOURCE_RENAMES.get(model.__name__, model.__name__.lower()) - event = f"{name}.{action}" - if event not in VALID_EVENTS: + try: + event = SentryAppEventType(f"{name}.{action}") + except ValueError as e: raise SentryAppSentryError( message=f"{SentryAppWebhookFailureReason.INVALID_EVENT}", - ) + ) from e with SentryAppInteractionEvent( - operation_type=SentryAppInteractionType.PREPARE_WEBHOOK, - event_type=SentryAppEventType(event), + operation_type=SentryAppInteractionType.PREPARE_WEBHOOK, event_type=event ).capture(): project_id: int | None = kwargs.get("project_id", None) group_id: int | None = kwargs.get("group_id", None) @@ -300,7 +300,7 @@ def _process_resource_change( # Trigger a new task for each webhook send_resource_change_webhook.delay( - installation_id=installation.id, event=event, data=data + installation_id=installation.id, event=str(event), data=data ) @@ -514,13 +514,15 @@ def notify_sentry_app(event: GroupEvent, futures: Sequence[RuleFuture]): def send_webhooks(installation: RpcSentryAppInstallation, event: str, **kwargs: Any) -> None: - if event not in VALID_EVENTS: + try: + event = SentryAppEventType(event) + except ValueError as e: raise SentryAppSentryError( message=f"{SentryAppWebhookFailureReason.INVALID_EVENT}", - ) + ) from e with SentryAppInteractionEvent( - operation_type=SentryAppInteractionType.SEND_WEBHOOK, event_type=SentryAppEventType(event) + operation_type=SentryAppInteractionType.SEND_WEBHOOK, event_type=event ).capture(): servicehook: ServiceHook try: diff --git a/src/sentry/utils/sentry_apps/webhooks.py b/src/sentry/utils/sentry_apps/webhooks.py index 56427cbfddf2bd..4f5dab90aa02b4 100644 --- a/src/sentry/utils/sentry_apps/webhooks.py +++ b/src/sentry/utils/sentry_apps/webhooks.py @@ -15,8 +15,13 @@ from sentry.integrations.notify_disable import notify_disable from sentry.integrations.request_buffer import IntegrationRequestBuffer from sentry.models.organization import Organization -from sentry.sentry_apps.metrics import SentryAppEventType, SentryAppWebhookHaltReason +from sentry.sentry_apps.metrics import ( + SentryAppEventType, + SentryAppWebhookFailureReason, + SentryAppWebhookHaltReason, +) from sentry.sentry_apps.models.sentry_app import SentryApp, track_response_code +from sentry.sentry_apps.utils.errors import SentryAppSentryError from sentry.shared_integrations.exceptions import ApiHostError, ApiTimeoutError, ClientError from sentry.utils.audit import create_system_audit_entry from sentry.utils.sentry_apps import SentryAppWebhookRequestsBuffer @@ -129,9 +134,15 @@ def send_and_save_webhook_request( """ from sentry.sentry_apps.metrics import SentryAppInteractionEvent, SentryAppInteractionType - event = f"{app_platform_event.resource}.{app_platform_event.action}" + try: + event = SentryAppEventType(f"{app_platform_event.resource}.{app_platform_event.action}") + except ValueError as e: + raise SentryAppSentryError( + message=f"{SentryAppWebhookFailureReason.INVALID_EVENT}", + ) from e + with SentryAppInteractionEvent( - operation_type=SentryAppInteractionType.SEND_WEBHOOK, event_type=SentryAppEventType(event) + operation_type=SentryAppInteractionType.SEND_WEBHOOK, event_type=event ).capture() as lifecycle: buffer = SentryAppWebhookRequestsBuffer(sentry_app) From 9dca10ea1ef313ad5f28c5e7031412a49e4eff67 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 7 Mar 2025 12:53:33 -0800 Subject: [PATCH 30/32] cast the event alert trifggerdd --- src/sentry/sentry_apps/tasks/sentry_apps.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index a9d526de7c1fb0..d8b0ac4e7091b0 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -203,7 +203,7 @@ def send_alert_webhook( "alert_rule_ui_component_webhook.sent", organization_id=organization.id, sentry_app_id=sentry_app_id, - event=f"{request_data.resource}.{request_data.action}", + event=str(SentryAppEventType.EVENT_ALERT_TRIGGERED), ) From 416e7cab76c3a119a173c5e0e356cf794a66b5da Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 11 Mar 2025 11:26:59 -0700 Subject: [PATCH 31/32] pr fixes --- src/sentry/sentry_apps/tasks/sentry_apps.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 85ba0b6d1e7621..9fad6a37c862f3 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -203,7 +203,7 @@ def send_alert_webhook( "alert_rule_ui_component_webhook.sent", organization_id=organization.id, sentry_app_id=sentry_app_id, - event=str(SentryAppEventType.EVENT_ALERT_TRIGGERED), + event=SentryAppEventType.EVENT_ALERT_TRIGGERED, ) @@ -491,8 +491,10 @@ def send_resource_change_webhook( def notify_sentry_app(event: GroupEvent, futures: Sequence[RuleFuture]): for f in futures: if not f.kwargs.get("sentry_app"): - logger.info("notify_sentry_app.future_missing_sentry_app", extra={"future": f}) - continue + logger.error( + "notify_sentry_app.future_missing_sentry_app", + extra={"event": event.as_dict(), "future": f, "event_id": event.event_id}, + ) extra_kwargs: dict[str, Any] = { "additional_payload_key": None, From f439a4466c14988ab6e5152116f45633a8a8edc0 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 14 Mar 2025 11:41:55 -0700 Subject: [PATCH 32/32] fix tests --- src/sentry/sentry_apps/tasks/sentry_apps.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sentry/sentry_apps/tasks/sentry_apps.py b/src/sentry/sentry_apps/tasks/sentry_apps.py index 9fad6a37c862f3..acd1d1b621fd8f 100644 --- a/src/sentry/sentry_apps/tasks/sentry_apps.py +++ b/src/sentry/sentry_apps/tasks/sentry_apps.py @@ -495,6 +495,7 @@ def notify_sentry_app(event: GroupEvent, futures: Sequence[RuleFuture]): "notify_sentry_app.future_missing_sentry_app", extra={"event": event.as_dict(), "future": f, "event_id": event.event_id}, ) + continue extra_kwargs: dict[str, Any] = { "additional_payload_key": None,