Still record circuit breaker hits on 503+

These errors indicate failures from downstream loadbalancers that
indicate that the downstream application is gone/timing out.

Author: markstory

src/sentry/hybridcloud/apigateway/proxy.py

@@ -193,26 +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)
-        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()
 
         raise
 
-    if resp.status_code >= 500:
+    if resp.status_code >= 502:
         metrics.incr("apigateway.proxy.request_failed", tags=metric_tags)
+        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

@@ -355,7 +355,7 @@ def test_timeout_records_error(self) -> None:
     def test_connection_error_records_error(self) -> None:
         responses.add(
             responses.GET,
-            f"{self.REGION.address}/connect-error",
+            f"{self.CELL.address}/connect-error",
             body=ConnectionError(),
         )
         with patch("sentry.hybridcloud.apigateway.proxy.CircuitBreaker") as mock_breaker_class:
@@ -371,7 +371,7 @@ def test_connection_error_records_error(self) -> None:
     def test_connection_error_records_metric(self) -> None:
         responses.add(
             responses.GET,
-            f"{self.REGION.address}/connect-error",
+            f"{self.CELL.address}/connect-error",
             body=ConnectionError(),
         )
         with patch("sentry.hybridcloud.apigateway.proxy.metrics") as mock_metrics:
@@ -380,12 +380,30 @@ def test_connection_error_records_metric(self) -> None:
                 proxy_request(request, self.organization.slug, url_name)
         mock_metrics.incr.assert_any_call(
             "apigateway.proxy.connection_error",
-            tags={"region": self.REGION.name, "url_name": url_name},
+            tags={"region": self.CELL.name, "url_name": url_name},
         )
 
     @responses.activate
     @override_options(CB_ENABLED)
-    def test_5xx_response_does_not_record_error(self) -> None:
+    def test_504_response_does_not_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",