fix(observability): ensure response hook only records OK on successful calls

Author: chalmerlowe

packages/google-api-core/google/api_core/_observability.py

@@ -184,15 +184,25 @@ def client_request_hook(span: Any, request: Any) -> None:
 def _grpc_client_response_hook(span: Any, response: Any) -> None:
     """OpenTelemetry gRPC client response hook to record response status code.
 
-    Note: Upstream OpenTelemetry gRPC instrumentation only invokes this response_hook
-    on successful RPC invocations. Failed RPCs raise an exception before this hook is reached.
-
     Args:
         span: The OpenTelemetry span.
         response: The gRPC response object or details.
     """
-    if span is not None and hasattr(span, "set_attribute"):
-        span.set_attribute("rpc.response.status_code", "OK")
+    if not span.is_recording():
+        return
+
+    # Verify the RPC succeeded before recording the OK response status.
+    # Upstream async instrumentation invokes this hook on both successes
+    # and failures, so check whether an error status was already recorded.
+    status = getattr(span, "status", None)
+    status_code = getattr(status, "status_code", None)
+    if (
+        getattr(status_code, "name", None) == "ERROR"
+        or getattr(status_code, "value", None) == 2
+    ):
+        return
+
+    span.set_attribute("rpc.response.status_code", "OK")
 
 
 def _get_tracer_provider(

packages/google-api-core/tests/unit/test_observability.py

@@ -531,11 +531,33 @@ def test_get_otel_async_interceptor_with_api_endpoint(monkeypatch):
 def test_grpc_client_response_hook_success():
     """Proves that _grpc_client_response_hook sets rpc.response.status_code to 'OK' on success."""
     mock_span = mock.Mock()
+    mock_span.is_recording.return_value = True
     _observability._grpc_client_response_hook(mock_span, mock.Mock())
     mock_span.set_attribute.assert_called_once_with("rpc.response.status_code", "OK")
 
 
-def test_grpc_client_response_hook_none_or_missing_set_attribute():
-    """Proves that _grpc_client_response_hook handles None or invalid span gracefully."""
-    _observability._grpc_client_response_hook(None, mock.Mock())
-    _observability._grpc_client_response_hook(object(), mock.Mock())
+def test_grpc_client_response_hook_not_recording():
+    """Proves that _grpc_client_response_hook skips non-recording spans."""
+    mock_span = mock.Mock()
+    mock_span.is_recording.return_value = False
+    _observability._grpc_client_response_hook(mock_span, mock.Mock())
+    mock_span.set_attribute.assert_not_called()
+
+
+def test_grpc_client_response_hook_error_status():
+    """Proves that _grpc_client_response_hook skips spans marked with ERROR status."""
+    mock_span = mock.Mock()
+    mock_span.is_recording.return_value = True
+    mock_span.status.status_code.name = "ERROR"
+    _observability._grpc_client_response_hook(mock_span, mock.Mock())
+    mock_span.set_attribute.assert_not_called()
+
+
+def test_grpc_client_response_hook_error_status_value():
+    """Proves that _grpc_client_response_hook skips spans with StatusCode.ERROR value (2)."""
+    mock_span = mock.Mock()
+    mock_span.is_recording.return_value = True
+    mock_span.status.status_code.name = "UNKNOWN"
+    mock_span.status.status_code.value = 2
+    _observability._grpc_client_response_hook(mock_span, mock.Mock())
+    mock_span.set_attribute.assert_not_called()