feat(django): Add failed_request_status_codes

Author: mgaligniana

sentry_sdk/integrations/django/__init__.py

@@ -1,3 +1,4 @@
+import functools
 import inspect
 import sys
 import threading
@@ -6,7 +7,12 @@
 
 import sentry_sdk
 from sentry_sdk.consts import OP, SPANDATA, SPANNAME
-from sentry_sdk.integrations import DidNotEnable, Integration, _check_minimum_version
+from sentry_sdk.integrations import (
+    _DEFAULT_FAILED_REQUEST_STATUS_CODES,
+    DidNotEnable,
+    Integration,
+    _check_minimum_version,
+)
 from sentry_sdk.integrations._wsgi_common import (
     DEFAULT_HTTP_METHODS_TO_CAPTURE,
     RequestExtractor,
@@ -80,6 +86,7 @@
 from typing import TYPE_CHECKING
 
 if TYPE_CHECKING:
+    from collections.abc import Set
     from typing import Any, Callable, Dict, List, Optional, Union
 
     from django.core.handlers.wsgi import WSGIRequest
@@ -116,6 +123,11 @@ class DjangoIntegration(Integration):
     :param signals_spans: Whether to create spans for signals. Defaults to `True`.
     :param signals_denylist: A list of signals to ignore when creating spans.
     :param cache_spans: Whether to create spans for cache operations. Defaults to `False`.
+    :param failed_request_status_codes: Which HTTP error responses to report to Sentry.
+        Django answers some exceptions itself instead of failing: `raise Http404` gets
+        the user a 404 page, `PermissionDenied` a 403. Those are reported only if their
+        status code is in this set, which defaults to the 5xx range. Exceptions Django
+        gives up on end in a 500 and are always reported.
     """
 
     identifier = "django"
@@ -137,6 +149,8 @@ def __init__(
         db_transaction_spans: bool = False,
         signals_denylist: "Optional[list[signals.Signal]]" = None,
         http_methods_to_capture: "tuple[str, ...]" = DEFAULT_HTTP_METHODS_TO_CAPTURE,
+        *,
+        failed_request_status_codes: "Set[int]" = _DEFAULT_FAILED_REQUEST_STATUS_CODES,
     ) -> None:
         if transaction_style not in TRANSACTION_STYLE_VALUES:
             raise ValueError(
@@ -154,6 +168,8 @@ def __init__(
 
         self.http_methods_to_capture = tuple(map(str.upper, http_methods_to_capture))
 
+        self.failed_request_status_codes = failed_request_status_codes
+
     @staticmethod
     def setup_once() -> None:
         _check_minimum_version(DjangoIntegration, DJANGO_VERSION)
@@ -199,6 +215,8 @@ def sentry_patched_wsgi_handler(
 
         _patch_django_asgi_handler()
 
+        _patch_response_for_exception()
+
         signals.got_request_exception.connect(_got_request_exception)
 
         @add_global_event_processor
@@ -614,18 +632,89 @@ def _got_request_exception(request: "WSGIRequest" = None, **kwargs: "Any") -> No
     if integration is None:
         return
 
+    # Record that this exception is reported, so `_patch_response_for_exception`
+    # doesn't report it a second time.
+    with capture_internal_exceptions():
+        request._sentry_exception_reported = True
+
+    _capture_exception(sys.exc_info(), request, integration, handled=False)
+
+
+def _capture_exception(
+    exc_info: "Any",
+    request: "Optional[WSGIRequest]",
+    integration: "DjangoIntegration",
+    handled: bool,
+) -> None:
     if request is not None and integration.transaction_style == "url":
         scope = sentry_sdk.get_current_scope()
         _attempt_resolve_again(request, scope, integration.transaction_style)
 
     event, hint = event_from_exception(
-        sys.exc_info(),
-        client_options=client.options,
-        mechanism={"type": "django", "handled": False},
+        exc_info,
+        client_options=sentry_sdk.get_client().options,
+        mechanism={"type": "django", "handled": handled},
     )
     sentry_sdk.capture_event(event, hint=hint)
 
 
+def _patch_response_for_exception() -> None:
+    """
+    Report the errors Django answers itself.
+
+    Django deals with every exception in one function, which boils down to:
+
+        if isinstance(exc, Http404):             return <404 page>
+        if isinstance(exc, PermissionDenied):    return <403 page>
+        if isinstance(exc, SuspiciousOperation): return <400 page>
+        got_request_exception.send(...)          # Django gives up
+        return <500 page>
+
+    We only ever listened to that signal, so we heard about the exceptions Django
+    gives up on and about nothing else. Wrapping the function lets us see the rest
+    too, along with the status code Django picked for them.
+    """
+    try:
+        from django.core.handlers import exception as exception_handler
+    except ImportError:
+        # Django < 1.10 does this in `BaseHandler`, nothing to patch here
+        return
+
+    old_response_for_exception = getattr(
+        exception_handler, "response_for_exception", None
+    )
+    if old_response_for_exception is None:
+        return
+
+    @functools.wraps(old_response_for_exception)
+    def sentry_patched_response_for_exception(
+        request: "WSGIRequest", exc: Exception
+    ) -> "HttpResponse":
+        integration = sentry_sdk.get_client().get_integration(DjangoIntegration)
+        if integration is None:
+            return old_response_for_exception(request, exc)
+
+        # Clear the flag before delegating. The same request can reach this
+        # function twice: first when the view raises, then again if a middleware
+        # raises while handing the response back out. Without the reset, the
+        # first exception would keep the second one from being reported.
+        with capture_internal_exceptions():
+            request._sentry_exception_reported = False
+
+        response = old_response_for_exception(request, exc)
+
+        # The flag is set when Django gives up on the exception and fires
+        # `got_request_exception`, which means we reported it already.
+        if not getattr(request, "_sentry_exception_reported", False):
+            status_code = getattr(response, "status_code", None)
+            if status_code in integration.failed_request_status_codes:
+                _capture_exception(exc, request, integration, handled=True)
+
+        return response
+
+    exception_handler.response_for_exception = sentry_patched_response_for_exception
+
+
 class DjangoRequestExtractor(RequestExtractor):
     def __init__(self, request: "Union[WSGIRequest, ASGIRequest]") -> None:
         try:

tests/integrations/django/asgi/test_asgi.py

@@ -1126,3 +1126,37 @@ async def test_user_identity_error_event_data_collection(
         assert "id" not in event.get("user", {})
         assert "email" not in event.get("user", {})
         assert "username" not in event.get("user", {})
+
+
+@pytest.mark.parametrize("application", APPS)
+@pytest.mark.asyncio
+@pytest.mark.skipif(
+    django.VERSION < (3, 0), reason="Django ASGI support shipped in 3.0"
+)
+@pytest.mark.parametrize(
+    ("integration_kwargs", "expected_type"),
+    (
+        ({}, None),
+        ({"failed_request_status_codes": {403, *range(500, 600)}}, "PermissionDenied"),
+    ),
+)
+async def test_failed_request_status_codes(
+    sentry_init, capture_events, application, integration_kwargs, expected_type
+):
+    sentry_init(integrations=[DjangoIntegration(**integration_kwargs)])
+    events = capture_events()
+
+    comm = HttpCommunicator(application, "GET", "/permission-denied-exc")
+    response = await comm.get_response()
+    await comm.wait()
+
+    assert response["status"] == 403
+
+    if expected_type is None:
+        assert not events
+    else:
+        (event,) = events
+        (exception,) = event["exception"]["values"]
+        assert exception["type"] == expected_type
+        assert exception["mechanism"]["handled"] is True
+        assert event["transaction"] == "/permission-denied-exc"

tests/integrations/django/myapp/urls.py

@@ -102,6 +102,11 @@ def path(path, *args, **kwargs):
         views.permission_denied_exc,
         name="permission_denied_exc",
     ),
+    path(
+        "http404-exc",
+        views.http404_exc,
+        name="http404_exc",
+    ),
     path(
         "csrf-hello-not-exempt",
         views.csrf_hello_not_exempt,

tests/integrations/django/myapp/views.py

@@ -7,7 +7,12 @@
 from django.core.exceptions import PermissionDenied
 from django.db import transaction
 from django.dispatch import Signal
-from django.http import HttpResponse, HttpResponseNotFound, HttpResponseServerError
+from django.http import (
+    Http404,
+    HttpResponse,
+    HttpResponseNotFound,
+    HttpResponseServerError,
+)
 from django.shortcuts import render
 from django.template import Context, Template
 from django.template.response import TemplateResponse
@@ -343,6 +348,11 @@ def permission_denied_exc(*args, **kwargs):
     raise PermissionDenied("bye")
 
 
+@csrf_exempt
+def http404_exc(*args, **kwargs):
+    raise Http404("bye")
+
+
 def csrf_hello_not_exempt(*args, **kwargs):
     return HttpResponse("ok")
 

tests/integrations/django/test_basic.py

@@ -1761,6 +1761,96 @@ def test_does_not_capture_403(
         assert not events
 
 
+@pytest.mark.parametrize(
+    ("integration_kwargs", "endpoint", "status", "expected_type"),
+    (
+        # Django only turns exceptions into 4xx responses, so with the default
+        # (the 5xx range) none of them are reported
+        ({}, "permission_denied_exc", "403 forbidden", None),
+        ({}, "http404_exc", "404 not found", None),
+        (
+            {"failed_request_status_codes": set()},
+            "permission_denied_exc",
+            "403 forbidden",
+            None,
+        ),
+        (
+            {"failed_request_status_codes": {403, *range(500, 600)}},
+            "permission_denied_exc",
+            "403 forbidden",
+            "PermissionDenied",
+        ),
+        (
+            {"failed_request_status_codes": {404, *range(500, 600)}},
+            "http404_exc",
+            "404 not found",
+            "Http404",
+        ),
+        # Only the status codes that were opted into are reported
+        (
+            {"failed_request_status_codes": {403}},
+            "http404_exc",
+            "404 not found",
+            None,
+        ),
+    ),
+)
+def test_failed_request_status_codes(
+    sentry_init,
+    client,
+    capture_events,
+    integration_kwargs,
+    endpoint,
+    status,
+    expected_type,
+):
+    sentry_init(integrations=[DjangoIntegration(**integration_kwargs)])
+    events = capture_events()
+
+    _, response_status, _ = unpack_werkzeug_response(client.get(reverse(endpoint)))
+    assert response_status.lower() == status
+
+    # The test app's handler404 captures a message, ignore it here
+    error_events = [event for event in events if "exception" in event]
+
+    if expected_type is None:
+        assert not error_events
+    else:
+        (event,) = error_events
+        (exception,) = event["exception"]["values"]
+        assert exception["type"] == expected_type
+        assert exception["mechanism"]["type"] == "django"
+        assert exception["mechanism"]["handled"] is True
+
+
+@pytest.mark.parametrize(
+    "integration_kwargs",
+    (
+        {},
+        {"failed_request_status_codes": set()},
+        {"failed_request_status_codes": {404}},
+    ),
+)
+def test_failed_request_status_codes_unhandled_exception(
+    sentry_init, client, capture_events, integration_kwargs
+):
+    """
+    Exceptions Django gives up on are always reported, exactly once, no matter how
+    failed_request_status_codes is set.
+    """
+    sentry_init(integrations=[DjangoIntegration(**integration_kwargs)])
+    events = capture_events()
+
+    _, status, _ = unpack_werkzeug_response(client.get(reverse("view_exc")))
+    assert status.lower() == "500 internal server error"
+
+    (event,) = events
+    (exception,) = event["exception"]["values"]
+    assert exception["type"] == "ZeroDivisionError"
+    assert exception["mechanism"]["type"] == "django"
+    assert exception["mechanism"]["handled"] is False
+
+
 @pytest.mark.parametrize("span_streaming", [True, False])
 def test_render_spans(
     sentry_init,