feat(sentry apps): Capture failed requests data and add sorting (#34944)

*  Capture failed requests data and add sorting

src/sentry/api/endpoints/integrations/sentry_apps/requests.py

@@ -1,61 +1,72 @@
-from django.urls import reverse
+from dataclasses import dataclass
+from datetime import datetime
+from typing import Any, Mapping
+
 from rest_framework.request import Request
 from rest_framework.response import Response
 
-from sentry import eventstore
 from sentry.api.bases import SentryAppBaseEndpoint, SentryAppStatsPermission
-from sentry.models import Organization, Project
+from sentry.api.serializers import serialize
+from sentry.api.serializers.rest_framework import RequestSerializer
+from sentry.models import Organization
 from sentry.utils.sentry_apps import EXTENDED_VALID_EVENTS, SentryAppWebhookRequestsBuffer
 
+INVALID_DATE_FORMAT_MESSAGE = "Invalid date format. Format must be YYYY-MM-DD HH:MM:SS."
 
-class SentryAppRequestsEndpoint(SentryAppBaseEndpoint):
-    permission_classes = (SentryAppStatsPermission,)
 
-    def format_request(self, request: Request, sentry_app):
-        formatted_request = {
-            "webhookUrl": request.get("webhook_url"),
-            "sentryAppSlug": sentry_app.slug,
-            "eventType": request.get("event_type"),
-            "date": request.get("date"),
-            "responseCode": request.get("response_code"),
-        }
+def filter_by_date(request: Mapping[str, Any], start: float, end: float) -> bool:
+    date_str = request.get("date")
+    if not date_str:
+        return False
+    timestamp = datetime.strptime(date_str, "%Y-%m-%d %H:%M:%S.%f+00:00").replace(microsecond=0)
+    return start <= timestamp <= end
 
-        if "error_id" in request and "project_id" in request:
-            try:
-                project = Project.objects.get_from_cache(id=request["project_id"])
-                # Make sure the project actually belongs to the org that owns the Sentry App
-                if project.organization_id == sentry_app.owner_id:
-                    # Make sure the event actually exists
-                    event = eventstore.get_event_by_id(project.id, request["error_id"])
-                    if event is not None and event.group_id is not None:
-                        error_url = reverse(
-                            "sentry-organization-event-detail",
-                            args=[project.organization.slug, event.group_id, event.event_id],
-                        )
-                        formatted_request["errorUrl"] = error_url
-
-            except Project.DoesNotExist:
-                # If the project doesn't exist, don't add the error to the result
-                pass
-
-        if "organization_id" in request:
-            try:
-                org = Organization.objects.get_from_cache(id=request["organization_id"])
-                formatted_request["organization"] = {"name": org.name, "slug": org.slug}
-            except Organization.DoesNotExist:
-                # If the org somehow doesn't exist, just don't add it to the result
-                pass
 
-        return formatted_request
+def filter_by_organization(request: Mapping[str, Any], organization: Organization) -> bool:
+    if not organization:
+        return True
+    return request["organization_id"] == organization.id
+
+
+@dataclass
+class BufferedRequest:
+    id: int
+    data: Mapping[str, Any]
+
+    def __hash__(self):
+        return self.id
+
+
+class SentryAppRequestsEndpoint(SentryAppBaseEndpoint):
+    permission_classes = (SentryAppStatsPermission,)
 
     def get(self, request: Request, sentry_app) -> Response:
         """
         :qparam string eventType: Optionally specify a specific event type to filter requests
         :qparam bool errorsOnly: If this is true, only return error/warning requests (300-599)
+        :qparam string organizationSlug: Optionally specify an org slug to filter requests
+        :qparam string start: Optionally specify a date to begin at. Format must be YYYY-MM-DD HH:MM:SS
+        :qparam string end: Optionally specify a date to end at. Format must be YYYY-MM-DD HH:MM:SS
         """
+        date_format = "%Y-%m-%d %H:%M:%S"
+        now = datetime.now().strftime(date_format)
+        default_start = "2000-01-01 00:00:00"
 
         event_type = request.GET.get("eventType")
         errors_only = request.GET.get("errorsOnly")
+        org_slug = request.GET.get("organizationSlug")
+        start = request.GET.get("start", default_start)
+        end = request.GET.get("end", now)
+
+        try:
+            start = datetime.strptime(start, date_format)
+        except ValueError:
+            return Response({"detail": INVALID_DATE_FORMAT_MESSAGE}, status=400)
+
+        try:
+            end = datetime.strptime(end, date_format)
+        except ValueError:
+            return Response({"detail": INVALID_DATE_FORMAT_MESSAGE}, status=400)
 
         kwargs = {}
         if event_type:
@@ -66,9 +77,16 @@ class SentryAppRequestsEndpoint(SentryAppBaseEndpoint):
             kwargs["errors_only"] = True
 
         buffer = SentryAppWebhookRequestsBuffer(sentry_app)
+        organization = None
+        if org_slug:
+            try:
+                organization = Organization.objects.get(slug=org_slug)
+            except Organization.DoesNotExist:
+                return Response({"detail": "Invalid organization."}, status=400)
 
-        formatted_requests = [
-            self.format_request(req, sentry_app) for req in buffer.get_requests(**kwargs)
-        ]
+        filtered_requests = []
+        for i, req in enumerate(buffer.get_requests(**kwargs)):
+            if filter_by_date(req, start, end) and filter_by_organization(req, organization):
+                filtered_requests.append(BufferedRequest(id=i, data=req))
 
-        return Response(formatted_requests)
+        return Response(serialize(filtered_requests, request.user, RequestSerializer(sentry_app)))

src/sentry/api/serializers/rest_framework/sentry_app_request.py

@@ -0,0 +1,73 @@
+from __future__ import annotations
+
+from typing import Any, Mapping, MutableMapping
+
+from django.urls import reverse
+
+from sentry import eventstore
+from sentry.api.serializers import Serializer
+from sentry.models import Organization, Project, SentryApp
+from sentry.utils.sentry_apps.webhooks import TIMEOUT_STATUS_CODE
+
+
+class RequestSerializer(Serializer):
+    def __init__(self, sentry_app: SentryApp) -> None:
+        self.sentry_app = sentry_app
+
+    def get_attrs(self, item_list: list[Any], user: Any, **kwargs: Any) -> MutableMapping[Any, Any]:
+        project_ids = {item.data.get("project_id") for item in item_list}
+        projects = Project.objects.filter(id__in=project_ids)
+        projects_by_id = {project.id: project for project in projects}
+
+        organization_ids = {item.data.get("organization_id") for item in item_list}
+        organizations = Organization.objects.filter(id__in=organization_ids)
+        organizations_by_id = {organization.id: organization for organization in organizations}
+
+        return {
+            item: {
+                "organization": organizations_by_id.get(item.data.get("organization_id")),
+                "project": projects_by_id.get(item.data.get("project_id")),
+            }
+            for item in item_list
+        }
+
+    def serialize(
+        self, obj: Any, attrs: Mapping[Any, Any], user: Any, **kwargs: Any
+    ) -> Mapping[str, Any]:
+        organization = attrs.get("organization")
+        project = attrs.get("project")
+        response_code = obj.data.get("response_code")
+
+        data = {
+            "webhookUrl": obj.data.get("webhook_url"),
+            "sentryAppSlug": self.sentry_app.slug,
+            "eventType": obj.data.get("event_type"),
+            "date": obj.data.get("date"),
+            "responseCode": response_code,
+        }
+
+        if response_code >= 400 or response_code == TIMEOUT_STATUS_CODE:
+            # add error data to display in Sentry app dashboard
+            data.update(
+                {
+                    "requestBody": obj.data.get("request_body"),
+                    "requestHeaders": obj.data.get("request_headers"),
+                    "responseBody": obj.data.get("response_body"),
+                }
+            )
+
+        if project and "error_id" in obj.data:
+            # Make sure the project actually belongs to the org that owns the Sentry App
+            if project.organization_id == self.sentry_app.owner_id:
+                # Make sure the event actually exists
+                event = eventstore.get_event_by_id(project.id, obj.data["error_id"])
+                if event is not None and event.group_id is not None:
+                    data["errorUrl"] = reverse(
+                        "sentry-organization-event-detail",
+                        args=[project.organization.slug, event.group_id, event.event_id],
+                    )
+
+        if organization:
+            data["organization"] = {"name": organization.name, "slug": organization.slug}
+
+        return data

src/sentry/mediators/external_requests/util.py

@@ -9,6 +9,7 @@ from sentry.http import safe_urlopen
 from sentry.models import SentryApp
 from sentry.models.integrations.sentry_app import track_response_code
 from sentry.utils.sentry_apps import SentryAppWebhookRequestsBuffer
+from sentry.utils.sentry_apps.webhooks import TIMEOUT_STATUS_CODE
 
 logger = logging.getLogger(__name__)
 
@@ -60,7 +61,6 @@ def send_and_save_sentry_app_request(
 
     kwargs ends up being the arguments passed into safe_urlopen
     """
-
     buffer = SentryAppWebhookRequestsBuffer(sentry_app)
     slug = sentry_app.slug_for_metrics
 
@@ -77,8 +77,13 @@ def send_and_save_sentry_app_request(
             },
         )
         track_response_code(error_type, slug, event)
-        # Response code of 0 represents timeout
-        buffer.add_request(response_code=0, org_id=org_id, event=event, url=url)
+        buffer.add_request(
+            response_code=TIMEOUT_STATUS_CODE,
+            org_id=org_id,
+            event=event,
+            url=url,
+            headers=kwargs.get("headers"),
+        )
         # Re-raise the exception because some of these tasks might retry on the exception
         raise
 
@@ -90,6 +95,8 @@ def send_and_save_sentry_app_request(
         url=url,
         error_id=resp.headers.get("Sentry-Hook-Error"),
         project_id=resp.headers.get("Sentry-Hook-Project"),
+        response=resp,
+        headers=kwargs.get("headers"),
     )
     resp.raise_for_status()
     return resp

src/sentry/utils/sentry_apps/request_buffer.py

@@ -108,7 +108,19 @@ class SentryAppWebhookRequestsBuffer:
     def get_requests(self, event=None, errors_only=False):
         return self._get_requests(event=event, error=errors_only)
 
-    def add_request(self, response_code, org_id, event, url, error_id=None, project_id=None):
+    def add_request(
+        self,
+        response_code,
+        org_id,
+        event,
+        url,
+        error_id=None,
+        project_id=None,
+        response=None,
+        headers=None,
+    ):
+        from sentry.utils.sentry_apps.webhooks import TIMEOUT_STATUS_CODE
+
         if event not in EXTENDED_VALID_EVENTS:
             logger.warning(f"Event {event} is not a valid event that can be stored.")
             return
@@ -121,6 +133,29 @@ class SentryAppWebhookRequestsBuffer:
             "response_code": response_code,
             "webhook_url": url,
         }
+        MAX_SIZE = 1024
+        if response_code >= 400 or response_code == TIMEOUT_STATUS_CODE:
+            if headers:
+                request_data["request_headers"] = headers
+
+            if response is not None:
+                if response.content is not None:
+                    try:
+                        json.loads(response.content)
+                        # if the type is jsonifiable, treat it as such
+                        prettified_response_body = json.dumps(
+                            response.content, indent=2, separators=(",", ": ")
+                        )
+                        request_data["response_body"] = prettified_response_body[:MAX_SIZE]
+                    except (json.JSONDecodeError, TypeError):
+                        request_data["response_body"] = response.content[:MAX_SIZE]
+                if response.request is not None:
+                    request_body = response.request.body
+                    if request_body is not None:
+                        prettified_request_body = json.dumps(
+                            request_body, indent=2, separators=(",", ": ")
+                        )
+                        request_data["request_body"] = prettified_request_body[:MAX_SIZE]
 
         # Don't store the org id for internal apps because it will always be the org that owns the app anyway
         if not self.sentry_app.is_internal:
@@ -136,7 +171,7 @@ class SentryAppWebhookRequestsBuffer:
         self._add_to_buffer_pipeline(request_key, request_data, pipe)
 
         # If it's an error add it to the error buffer
-        if 400 <= response_code <= 599 or response_code == 0:
+        if 400 <= response_code <= 599 or response_code == TIMEOUT_STATUS_CODE:
             error_key = self._get_redis_key(event, error=True)
             self._add_to_buffer_pipeline(error_key, request_data, pipe)
 

src/sentry/utils/sentry_apps/webhooks.py

@@ -18,6 +18,7 @@ if TYPE_CHECKING:
 
 
 WEBHOOK_TIMEOUT = 5
+TIMEOUT_STATUS_CODE = 0
 
 logger = logging.getLogger("sentry.sentry_apps.webhooks")
 
@@ -55,7 +56,7 @@ def send_and_save_webhook_request(
     event = f"{app_platform_event.resource}.{app_platform_event.action}"
     slug = sentry_app.slug_for_metrics
     url = url or sentry_app.webhook_url
-
+    response = None
     try:
         response = safe_urlopen(
             url=url,
@@ -74,8 +75,13 @@ def send_and_save_webhook_request(
             },
         )
         track_response_code(error_type, slug, event)
-        # Response code of 0 represents timeout
-        buffer.add_request(response_code=0, org_id=org_id, event=event, url=url)
+        buffer.add_request(
+            response_code=TIMEOUT_STATUS_CODE,
+            org_id=org_id,
+            event=event,
+            url=url,
+            headers=app_platform_event.headers,
+        )
         # Re-raise the exception because some of these tasks might retry on the exception
         raise
 
@@ -87,6 +93,8 @@ def send_and_save_webhook_request(
         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,
     )
 
     if response.status_code == status.HTTP_503_SERVICE_UNAVAILABLE:

tests/sentry/api/endpoints/test_sentry_app_requests.py

@@ -1,5 +1,9 @@
+from datetime import datetime, timedelta
+
 from django.urls import reverse
+from freezegun import freeze_time
 
+from sentry.api.endpoints.integrations.sentry_apps.requests import INVALID_DATE_FORMAT_MESSAGE
 from sentry.testutils import APITestCase
 from sentry.testutils.helpers.datetime import before_now, iso_format
 from sentry.utils.sentry_apps import SentryAppWebhookRequestsBuffer
@@ -292,3 +296,89 @@ class GetSentryAppRequestsTest(SentryAppRequestsTest):
         assert response.data[0]["organization"]["slug"] == self.org.slug
         assert response.data[0]["sentryAppSlug"] == self.published_app.slug
         assert "errorUrl" not in response.data[0]
+
+    def test_org_slug_filter(self):
+        """Test that filtering by the qparam organizationSlug properly filters results"""
+        self.login_as(user=self.user)
+        buffer = SentryAppWebhookRequestsBuffer(self.published_app)
+        buffer.add_request(
+            response_code=200,
+            org_id=self.org.id,
+            event="issue.assigned",
+            url=self.published_app.webhook_url,
+        )
+        buffer.add_request(
+            response_code=500,
+            org_id=self.org.id,
+            event="issue.assigned",
+            url=self.published_app.webhook_url,
+        )
+
+        url = reverse("sentry-api-0-sentry-app-requests", args=[self.published_app.slug])
+        made_up_org_response = self.client.get(f"{url}?organizationSlug=madeUpOrg", format="json")
+        assert made_up_org_response.status_code == 400
+        assert made_up_org_response.data["detail"] == "Invalid organization."
+
+        org_response = self.client.get(f"{url}?organizationSlug={self.org.slug}", format="json")
+        assert org_response.status_code == 200
+        assert len(org_response.data) == 2
+
+        response = self.client.get(url, format="json")
+        assert response.status_code == 200
+        assert len(response.data) == 2
+
+    def test_date_filter(self):
+        """Test that filtering by the qparams start and end properly filters results"""
+        self.login_as(user=self.user)
+        buffer = SentryAppWebhookRequestsBuffer(self.published_app)
+        now = datetime.now() - timedelta(hours=1)
+        buffer.add_request(
+            response_code=200,
+            org_id=self.org.id,
+            event="issue.assigned",
+            url=self.published_app.webhook_url,
+        )
+        with freeze_time(now + timedelta(seconds=1)):
+            buffer.add_request(
+                response_code=200,
+                org_id=self.org.id,
+                event="issue.assigned",
+                url=self.published_app.webhook_url,
+            )
+        with freeze_time(now + timedelta(seconds=2)):
+            buffer.add_request(
+                response_code=200,
+                org_id=self.org.id,
+                event="issue.assigned",
+                url=self.published_app.webhook_url,
+            )
+
+        url = reverse("sentry-api-0-sentry-app-requests", args=[self.published_app.slug])
+        response = self.client.get(url, format="json")
+        assert response.status_code == 200
+        assert len(response.data) == 3
+
+        # test adding a start time
+        start_date = now.strftime("%Y-%m-%d %H:%M:%S")
+        start_date_response = self.client.get(f"{url}?start={start_date}", format="json")
+        assert start_date_response.status_code == 200
+        assert len(start_date_response.data) == 3
+
+        # test adding an end time
+        end_date = (now + timedelta(seconds=2)).strftime("%Y-%m-%d %H:%M:%S")
+        end_date_response = self.client.get(f"{url}?end={end_date}", format="json")
+        assert end_date_response.status_code == 200
+        assert len(end_date_response.data) == 2
+
+        # test adding a start and end time
+        new_start_date = (now + timedelta(seconds=1)).strftime("%Y-%m-%d %H:%M:%S")
+        new_end_date = (now + timedelta(seconds=2)).strftime("%Y-%m-%d %H:%M:%S")
+        start_end_date_response = self.client.get(
+            f"{url}?start={new_start_date}&end={new_end_date}", format="json"
+        )
+        assert start_end_date_response.status_code == 200
+        assert len(start_end_date_response.data) == 2
+
+        # test adding an improperly formatted end time
+        bad_date_format_response = self.client.get(f"{url}?end=2000-01- 00:00:00", format="json")
+        assert bad_date_format_response.data["detail"] == INVALID_DATE_FORMAT_MESSAGE

tests/sentry/tasks/test_sentry_apps.py

@@ -1,5 +1,6 @@
 from collections import namedtuple
-from unittest.mock import patch
+from copy import deepcopy
+from unittest.mock import MagicMock, patch
 
 import pytest
 from celery import Task
@@ -44,20 +45,77 @@ def raiseException():
     raise Exception
 
 
+class RequestMock:
+    def __init__(self):
+        self.body = "blah blah"
+
+
+class ResponseMock:
+    def __init__(self, headers, content, text, ok, status_code, request):
+        self.headers = headers
+        self.content = content
+        self.text = text
+        self.ok = ok
+        self.status_code = status_code
+        self.request = request
+
+    def get(self, request=None, content=None):
+        if request:
+            return self.request
+        if content:
+            return self.content
+        return None
+
+
+headers = {"Sentry-Hook-Error": "d5111da2c28645c5889d072017e3445d", "Sentry-Hook-Project": "1"}
+
+mock_fail_resp = ResponseMock(
+    headers=headers,
+    content={},
+    text="",
+    ok=False,
+    status_code=400,
+    request=RequestMock(),
+)
+
+mock_fail_html_resp = deepcopy(mock_fail_resp)
+mock_fail_html_resp.content = "a bunch of garbage HTML"
+
+MockFailureHTMLContentResponseInstance = MagicMock(
+    status_code=mock_fail_html_resp.status_code,
+    headers=mock_fail_html_resp.headers,
+    content=mock_fail_html_resp.content,
+    request=mock_fail_html_resp.request,
+)
+MockFailureHTMLContentResponseInstance.__iter__.return_value = []
+
+mock_fail_json_resp = deepcopy(mock_fail_resp)
+mock_fail_json_resp.content = '{"error": "bad request"}'
+
+MockFailureJSONContentResponseInstance = MagicMock(
+    status_code=mock_fail_json_resp.status_code,
+    headers=mock_fail_html_resp.headers,
+    content=mock_fail_json_resp.content,
+    request=mock_fail_json_resp.request,
+)
+MockFailureJSONContentResponseInstance.__iter__.return_value = []
+
+MockResponseWithHeadersInstance = MagicMock(
+    status_code=mock_fail_resp.status_code, headers=mock_fail_resp.headers
+)
+MockResponseWithHeadersInstance.__iter__.return_value = []
+
+MockFailureResponseInstance = MagicMock(
+    status_code=mock_fail_resp.status_code, headers=mock_fail_resp.headers
+)
+MockFailureResponseInstance.__iter__.return_value = []
+
+
 MockResponse = namedtuple(
     "MockResponse", ["headers", "content", "text", "ok", "status_code", "raise_for_status"]
 )
 MockResponseInstance = MockResponse({}, {}, "", True, 200, raiseStatusFalse)
-MockFailureResponseInstance = MockResponse({}, {}, "", False, 400, raiseStatusTrue)
 MockResponse404 = MockResponse({}, {}, "", False, 404, raiseException)
-MockResponseWithHeadersInstance = MockResponse(
-    {"Sentry-Hook-Error": "d5111da2c28645c5889d072017e3445d", "Sentry-Hook-Project": "1"},
-    {},
-    "",
-    False,
-    400,
-    raiseStatusTrue,
-)
 
 
 class TestSendAlertEvent(TestCase):
@@ -558,6 +616,52 @@ class TestWebhookRequests(TestCase):
         assert first_request["event_type"] == "issue.assigned"
         assert first_request["organization_id"] == self.install.organization.id
 
+    @patch(
+        "sentry.utils.sentry_apps.webhooks.safe_urlopen",
+        return_value=MockFailureHTMLContentResponseInstance,
+    )
+    def test_saves_error_if_webhook_request_with_html_content_fails(self, safe_urlopen):
+        data = {"issue": serialize(self.issue)}
+
+        with self.assertRaises(ClientError):
+            send_webhooks(
+                installation=self.install, event="issue.assigned", data=data, actor=self.user
+            )
+
+        requests = self.buffer.get_requests()
+        requests_count = len(requests)
+        first_request = requests[0]
+
+        assert safe_urlopen.called
+        assert requests_count == 1
+        assert first_request["response_code"] == 400
+        assert first_request["event_type"] == "issue.assigned"
+        assert first_request["organization_id"] == self.install.organization.id
+        assert first_request["response_body"] == mock_fail_html_resp.content
+
+    @patch(
+        "sentry.utils.sentry_apps.webhooks.safe_urlopen",
+        return_value=MockFailureJSONContentResponseInstance,
+    )
+    def test_saves_error_if_webhook_request_with_json_content_fails(self, safe_urlopen):
+        data = {"issue": serialize(self.issue)}
+
+        with self.assertRaises(ClientError):
+            send_webhooks(
+                installation=self.install, event="issue.assigned", data=data, actor=self.user
+            )
+
+        requests = self.buffer.get_requests()
+        requests_count = len(requests)
+        first_request = requests[0]
+
+        assert safe_urlopen.called
+        assert requests_count == 1
+        assert first_request["response_code"] == 400
+        assert first_request["event_type"] == "issue.assigned"
+        assert first_request["organization_id"] == self.install.organization.id
+        assert json.loads(first_request["response_body"]) == mock_fail_json_resp.content
+
     @patch("sentry.utils.sentry_apps.webhooks.safe_urlopen", return_value=MockResponseInstance)
     def test_saves_request_if_webhook_request_succeeds(self, safe_urlopen):
         data = {"issue": serialize(self.issue)}