Skip to content

Pass the response message to the aio gRPC client response hook - #4938

Open
alliasgher wants to merge 4 commits into
open-telemetry:mainfrom
alliasgher:fix-aio-unary-response-hook
Open

Pass the response message to the aio gRPC client response hook#4938
alliasgher wants to merge 4 commits into
open-telemetry:mainfrom
alliasgher:fix-aio-unary-response-hook

Conversation

@alliasgher

Copy link
Copy Markdown
Contributor

Fixes #3490

Description

The aio client's unary response_hook receives the gRPC status detail string instead of the response message, so it is '' on a successful call.

add_done_callback cannot take a coroutine, so _wrap_unary_response pre-fetches code and details and passes them into the callback factory. The callback then does response_hook(span, details)details was never the response, it just happened to be in scope.

This is an internal inconsistency rather than a design question. Both of the other paths already pass the deserialized message:

  • _client.py:112-113 (sync): if self._response_hook: self._call_response_hook(span, response)
  • _aio_client.py _wrap_stream_response: self._call_response_hook(span, response)

Only the aio unary path differs. It affects unary-unary and stream-unary.

Fix

await call to get the response before registering the callback. That is free latency-wise — await call.code() above it already blocks until the RPC has completed — and grpc.aio caches the unary result, so the caller's own await still returns the response. The extra await is skipped entirely when no hook is registered.

Two judgement calls I want to flag rather than have you find

  1. The hook no longer fires on error. Today it fires with '' on a non-OK status; now it fires only on OK. That matches the sync client, which never reaches its hook when the call raises, but it is a behaviour change for anyone relying on the empty-string call.

    I deliberately did not await call on the error path. Doing so raises AioRpcError inside the try, which the except grpc.aio.AioRpcError catches and re-raises from the interceptor rather than from the caller's own await — and add_done_callback is then never registered, so span.end() never runs and the span leaks.

  2. The hooks have no docstrings specifying their arguments, so the case for "response message" rests on parity with _client.py and _wrap_stream_response rather than on documented contract. Happy to be told the aio unary hook was meant to be different.

Tests

The existing test asserted the bug:

def response_hook(span, response):
    span.set_attribute("response_data", response)
...
    self.assertEqual(span.attributes["response_data"], "")

Passing a protobuf message to set_attribute is silently dropped, so it could not simply be left alone. The hook now mirrors the sync test's response.response_data, and on current main that fails with AttributeError: 'str' object has no attribute 'response_data'.

Added two cases: stream-unary (same bug, separate call path) and one asserting the hook does not fire on a failed RPC.

pytest tests/ in the grpc package: 142 passed. There are 10 pre-existing failures in TestOpenTelemetryServerInterceptorUnix in my environment (Unix domain sockets on macOS) — I diffed the failing-test sets with and without this change via junit XML and they are identical, so nothing here is a regression. Ruff check and format are clean.

I ran against grpcio 1.75.1 (the pinned test requirement). I did not separately run against the oldest supported grpcio in test-requirements-0.txt; add_done_callback and awaiting a completed unary call are long-stable, but flagging that I did not verify it.

add_done_callback cannot take a coroutine, so _wrap_unary_response
pre-fetched code and details and handed details -- the gRPC status detail
string -- to the response hook. Callers got '' instead of the response.

The sync client (_client.py) and the aio streaming path both pass the
deserialized message, so this was internally inconsistent.

Await the call to get the response before registering the callback. That is
free: await call.code() already blocks until the RPC has completed, and
grpc.aio caches the unary result, so the caller's own await still returns
it. The extra await is skipped when no hook is registered.

The hook is now called only on OK. Awaiting a failed call would raise
AioRpcError inside the try block, which would re-raise from the interceptor
rather than the caller and skip add_done_callback entirely, leaking the
span. This matches the sync client, which never reaches its hook on error.

@henry3260 henry3260 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just two nits, test_response_hook_not_called_on_error will be green in main, so we should assert that the hook is not called

alliasgher and others added 2 commits August 12, 2026 02:55
The previous test used the shared response_hook, which does
response.response_data. On main that raises AttributeError on the status
details string and _safe_invoke swallows it, so the attribute was absent
either way and the test passed without the fix.

Use a recording hook and assert it was never invoked, which fails on main
with [''] != []. Also assert the span is still ended, since not awaiting a
failed call is what keeps add_done_callback registered.
@alliasgher
alliasgher requested a review from henry3260 August 12, 2026 20:21
@xrmx xrmx moved this to Ready for review in Python PR digest Aug 13, 2026
@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Aug 14, 2026

Copy link
Copy Markdown

Pull request dashboard status

Waiting on the author · refreshed 2026-08-26 19:32 UTC

Respond to 2 review items (e.g. link a commit, explain why not, ask a follow-up):

  • Inline threads: 1
  • Top-level threads: 2
Status above doesn't look right?
  • Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
  • Should this be with reviewers? Comment /dashboard route:reviewers to route it to them.
  • Anything wrong — including the routing? Report it with what you expected; it helps us improve the dashboard.

@opentelemetry-pr-dashboard

Copy link
Copy Markdown

Hi @alliasgher — just a friendly reminder that this pull request is waiting on you. The dashboard status comment has the open items and is kept current.

  • Replying is enough to hand it off — answer, explain why no change is needed, or ask a follow-up. The dashboard routes it onward once nothing on the list is waiting on you.
  • To hand it back for any other reason, including the dashboard getting this wrong, comment /dashboard route:reviewers.

)
response_hook(span, details)
else:
response_hook(span, response)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this change the hook runs when RPC fails, after this change the hook will not. Since that is user visible, the changelog should warn about this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Ready for review

Development

Successfully merging this pull request may close these issues.

gRPC AIO Client Instrumentation Unary-Unary Response Hook Bad Arguments

4 participants