diff --git a/src/sentry/hybridcloud/apigateway/proxy.py b/src/sentry/hybridcloud/apigateway/proxy.py index aae730476e26b1..9f13ce9b8a6f2c 100644 --- a/src/sentry/hybridcloud/apigateway/proxy.py +++ b/src/sentry/hybridcloud/apigateway/proxy.py @@ -14,7 +14,7 @@ from django.http.response import HttpResponseBase from requests import Response as ExternalResponse from requests import request as external_request -from requests.exceptions import Timeout +from requests.exceptions import ConnectionError, Timeout from sentry import options from sentry.api.exceptions import RequestTimeout @@ -193,18 +193,22 @@ def proxy_cell_request(request: HttpRequest, cell: Cell, url_name: str) -> HttpR ) except Timeout: metrics.incr("apigateway.proxy.request_timeout", tags=metric_tags) - try: - if circuit_breaker is not None: - circuit_breaker.record_error() - except Exception: - logger.exception("Failed to record circuitbreaker failure") + if circuit_breaker is not None: + circuit_breaker.record_error() # remote silo timeout. Use DRF timeout instead raise RequestTimeout() + except ConnectionError: + metrics.incr("apigateway.proxy.connection_error", tags=metric_tags) + if circuit_breaker is not None: + circuit_breaker.record_error() + + raise - if resp.status_code >= 500 and circuit_breaker is not None: + if resp.status_code >= 502: metrics.incr("apigateway.proxy.request_failed", tags=metric_tags) - circuit_breaker.record_error() + if circuit_breaker is not None: + circuit_breaker.record_error() new_headers = clean_outbound_headers(resp.headers) resp.headers.clear() diff --git a/tests/sentry/hybridcloud/apigateway/test_proxy.py b/tests/sentry/hybridcloud/apigateway/test_proxy.py index c1458db1ba4f71..c8a66042dfc74d 100644 --- a/tests/sentry/hybridcloud/apigateway/test_proxy.py +++ b/tests/sentry/hybridcloud/apigateway/test_proxy.py @@ -7,7 +7,7 @@ from django.core.files.uploadedfile import SimpleUploadedFile from django.http import HttpResponse from django.test.client import RequestFactory -from requests.exceptions import Timeout +from requests.exceptions import ConnectionError, Timeout from sentry.api.exceptions import RequestTimeout from sentry.hybridcloud.apigateway.proxy import proxy_request @@ -352,7 +352,58 @@ def test_timeout_records_error(self) -> None: @responses.activate @override_options(CB_ENABLED) - def test_5xx_response_records_error(self) -> None: + def test_connection_error_records_error(self) -> None: + responses.add( + responses.GET, + f"{self.CELL.address}/connect-error", + body=ConnectionError(), + ) + with patch("sentry.hybridcloud.apigateway.proxy.CircuitBreaker") as mock_breaker_class: + mock_breaker = self._make_breaker_mock(allow_request=True) + mock_breaker_class.return_value = mock_breaker + request = RequestFactory().get("http://sentry.io/connect-error") + with pytest.raises(ConnectionError): + proxy_request(request, self.organization.slug, url_name) + mock_breaker.record_error.assert_called_once() + + @responses.activate + @override_options(CB_ENABLED) + def test_connection_error_records_metric(self) -> None: + responses.add( + responses.GET, + f"{self.CELL.address}/connect-error", + body=ConnectionError(), + ) + with patch("sentry.hybridcloud.apigateway.proxy.metrics") as mock_metrics: + request = RequestFactory().get("http://sentry.io/connect-error") + with pytest.raises(ConnectionError): + proxy_request(request, self.organization.slug, url_name) + mock_metrics.incr.assert_any_call( + "apigateway.proxy.connection_error", + tags={"region": self.CELL.name, "url_name": url_name}, + ) + + @responses.activate + @override_options(CB_ENABLED) + def test_504_response_does_record_error(self) -> None: + responses.add( + responses.GET, + f"{self.CELL.address}/server-error", + status=504, + body=json.dumps({"detail": "gateway timeout"}), + content_type="application/json", + ) + with patch("sentry.hybridcloud.apigateway.proxy.CircuitBreaker") as mock_breaker_class: + mock_breaker = self._make_breaker_mock(allow_request=True) + mock_breaker_class.return_value = mock_breaker + request = RequestFactory().get("http://sentry.io/server-error") + resp = proxy_request(request, self.organization.slug, url_name) + assert resp.status_code == 504 + mock_breaker.record_error.assert_called_once() + + @responses.activate + @override_options(CB_ENABLED) + def test_500_response_does_not_record_error(self) -> None: responses.add( responses.GET, f"{self.CELL.address}/server-error", @@ -366,7 +417,7 @@ def test_5xx_response_records_error(self) -> None: request = RequestFactory().get("http://sentry.io/server-error") resp = proxy_request(request, self.organization.slug, url_name) assert resp.status_code == 500 - mock_breaker.record_error.assert_called_once() + mock_breaker.record_error.assert_not_called() @responses.activate @override_options(CB_ENABLED)