From 6c76bad65a616549d6e624acb2f6275e6fb22c1f Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Fri, 1 Sep 2023 13:14:31 -0700 Subject: [PATCH] middleware: Fix exception logging format on JSON views. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously (with ERROR_REPORTING = True), we’d stuff the entire traceback of the initial exception into the subject line of an error email, and then also send a separate email for the JSON 500 response. Instead, log one error with the standard Django format. Signed-off-by: Anders Kaseorg --- zerver/middleware.py | 24 +++++++++++---------- zerver/tests/test_integrations_dev_panel.py | 11 ++++++---- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/zerver/middleware.py b/zerver/middleware.py index 2d5af32824..ae5c886abe 100644 --- a/zerver/middleware.py +++ b/zerver/middleware.py @@ -2,7 +2,6 @@ import cProfile import logging import tempfile import time -import traceback from typing import Any, Callable, Dict, List, MutableMapping, Optional, Tuple from urllib.parse import urlencode, urljoin @@ -22,8 +21,7 @@ from django.utils.translation import gettext as _ from django.views.csrf import csrf_failure as html_csrf_failure from django_scim.middleware import SCIMAuthCheckMiddleware from django_scim.settings import scim_settings -from sentry_sdk import capture_exception, set_tag -from sentry_sdk.integrations.logging import ignore_logger +from sentry_sdk import set_tag from typing_extensions import Concatenate, ParamSpec from zerver.lib.cache import get_remote_cache_requests, get_remote_cache_time @@ -361,10 +359,6 @@ class LogRequests(MiddlewareMixin): class JsonErrorHandler(MiddlewareMixin): - def __init__(self, get_response: Callable[[HttpRequest], HttpResponseBase]) -> None: - super().__init__(get_response) - ignore_logger("zerver.middleware.json_error_handler") - def process_exception( self, request: HttpRequest, exception: Exception ) -> Optional[HttpResponse]: @@ -404,11 +398,19 @@ class JsonErrorHandler(MiddlewareMixin): exception=exception, ) return response + if RequestNotes.get_notes(request).error_format == "JSON" and not settings.TEST_SUITE: - capture_exception(exception) - json_error_logger = logging.getLogger("zerver.middleware.json_error_handler") - json_error_logger.error(traceback.format_exc(), extra=dict(request=request)) - return json_response(res_type="error", msg=_("Internal server error"), status=500) + response = json_response(res_type="error", msg=_("Internal server error"), status=500) + log_response( + "%s: %s", + response.reason_phrase, + request.path, + response=response, + request=request, + exception=exception, + ) + return response + return None diff --git a/zerver/tests/test_integrations_dev_panel.py b/zerver/tests/test_integrations_dev_panel.py index 39e52b5bba..8970172d99 100644 --- a/zerver/tests/test_integrations_dev_panel.py +++ b/zerver/tests/test_integrations_dev_panel.py @@ -30,11 +30,14 @@ class TestIntegrationsDevPanel(ZulipTestCase): # Intention of this test looks like to trigger ValidationError # so just testing ValidationError is printed along with Traceback in logs - self.assertTrue("ValidationError" in logs.output[0]) - self.assertTrue("Traceback (most recent call last)" in logs.output[0]) - self.assertEqual( - logs.output[1], "ERROR:django.request:Internal Server Error: /api/v1/external/airbrake" + self.assert_length(logs.output, 1) + self.assertTrue( + logs.output[0].startswith( + "ERROR:django.request:Internal Server Error: /api/v1/external/airbrake\n" + "Traceback (most recent call last):\n" + ) ) + self.assertTrue("ValidationError" in logs.output[0]) def test_check_send_webhook_fixture_message_for_success_without_headers(self) -> None: bot = get_user("webhook-bot@zulip.com", self.zulip_realm)