Repository navigation
feat(api_core): add sync and async gRPC transport attempt logging interceptors - #19010
chalmerlowe wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces synchronous and asynchronous gRPC client interceptors (_GrpcAttemptLoggingInterceptor and _AsyncGrpcAttemptLoggingInterceptor) to log failed wire attempts at the DEBUG level, updating the OpenTelemetry interceptor retrieval functions to incorporate them. The review feedback recommends checking if the gRPC call or future has been cancelled before calling exception() or awaiting code() to avoid raising unhandled CancelledError or AioRpcError exceptions, ensuring robust fail-open behavior.
| exc_fn = getattr(resp, "exception", None) | ||
| if callable(exc_fn) and (resp_exc := exc_fn()) is not None: |
There was a problem hiding this comment.
When implementing a completion callback for a future-like object, always check if the future was cancelled using cancelled() before calling exception() to avoid raising an unnecessary exception (such as CancelledError or FutureCancelledError).
cancelled_fn = getattr(resp, "cancelled", None)
if callable(cancelled_fn) and cancelled_fn():
return
exc_fn = getattr(resp, "exception", None)
if callable(exc_fn) and (resp_exc := exc_fn()) is not None:References
- When implementing a completion callback for a concurrent.futures.Future, always check if the future was cancelled using future.cancelled() before calling future.exception() to avoid raising an unhandled CancelledError.
There was a problem hiding this comment.
Fixed in 42a6423 — broadened the fail-open catch in _inspect_sync_response to except (Exception, asyncio.CancelledError): and added a unit test covering cancelled Future inspection.
|
|
||
| async def _inspect_async_call(target_call: Any) -> None: | ||
| try: | ||
| code_fn = getattr(target_call, "code", None) |
There was a problem hiding this comment.
For async gRPC calls, calling await code_fn() on a cancelled call can raise a CancelledError or AioRpcError. To prevent unnecessary exceptions and ensure a clean fail-open behavior, check if the call was cancelled using cancelled() before awaiting its code or details.
cancelled_fn = getattr(target_call, "cancelled", None)
if callable(cancelled_fn) and cancelled_fn():
return
code_fn = getattr(target_call, "code", None)There was a problem hiding this comment.
Fixed in 42a6423 — broadened the fail-open catch in _inspect_async_call to except (Exception, asyncio.CancelledError): and added a unit test covering cancelled async Call inspection.
0b90e49 to
42a6423
Compare
42a6423 to
0cbf1b7
Compare
866406e to
56c1e04
Compare
Problem / Context
To capture transient wire failures and retries across synchronous and asynchronous gRPC transports without blocking in-flight
grpc.Futureorgrpc.aio.UnaryUnaryCallobjects,google.api_core._observabilityneeds gRPC client interceptors that inspect completed RPC attempts and emit structuredDEBUGlogs correlated with the active Tier 4 OpenTelemetry span.Bug: b/479827845
Solution
_GrpcAttemptLoggingInterceptorand_AsyncGrpcAttemptLoggingInterceptoringoogle/api_core/_observability.pyto log failed unary-unary gRPC wire attempts atDEBUGlevel vialog_transport_attempt.add_done_callbackwhen supported on the returned gRPC response/call object sogrpc.Futureandgrpc.aio.UnaryUnaryCallexecution remains non-blocking.get_otel_interceptorandget_otel_async_interceptorto sandwich the attempt logging interceptor between the outer OpenTelemetry tracing interceptor (so the Tier 4 span is active for trace correlation) and the inner_SuppressingClientInterceptor/_AsyncSuppressingClientInterceptor.tests/unit/test_observability.pycovering sync/async response callbacks, direct exceptions, fail-open inspection errors, and interceptor ordering.Verification
Relying on GitHub Actions CI for full matrix verification, alongside local unit test (
unit-3.14) and lint verification.