fix(cells) Don't record proxied request failures towards circuit breaker (#111639)

We don't want to trip the breaker when the proxied requests fail with
500s. We're mostly interested in preventing resource exhaustion from
slow requests/connection errors here.

Refs INFRENG-275

Author: markstory

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()

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)