fix Sync hook used as async hook in opentelemetry-instrumentation-httpx causing TypeError #2794
Conversation
Signed-off-by: Shi, Stone <[email protected]>
| @@ -731,7 +731,9 @@ def _instrument(self, **kwargs): | |||
| self._original_async_client = httpx.AsyncClient | |||
| request_hook = kwargs.get("request_hook") | |||
| response_hook = kwargs.get("response_hook") | |||
| async_request_hook = kwargs.get("async_request_hook", request_hook) | |||
There was a problem hiding this comment.
I'm wondering if we should remove this logic of defaulting to request hook if async_request_hook does not exist. It doesn't make much intuitive sense to me. We can then probably just add a check to see if the hook coming from async_request_hook is indeed an async function with inspect.iscoroutinefunction as part of the inspect module and not assign it to _InstrumentedAsyncClient._request_hook if not.
There was a problem hiding this comment.
The easiest way is probably to leave it as None if not set; it will fail during the callable check when the hook is None. Also, this PR needs the async_response_hook fix as well. And of course, some tests would be nice to avoid problems with this again. Wdyt?
There was a problem hiding this comment.
leave it as None if not set; it will fail during the callable check when the hook is None.
Yes this and checking if async_request_hook is actually async is probably sufficient, and as always, adding more tests are always welcome.
There was a problem hiding this comment.
Thanks for your review. I will work on the changes.
|
Please also add a CHANGELOG entry :) |
co-authored-by: [email protected] Signed-off-by: Shi, Stone <[email protected]>
Co-authored by [email protected] Signed-off-by: Shi, Stone <[email protected]>
Signed-off-by: Shi, Stone <[email protected]>
Signed-off-by: Shi, Stone <[email protected]>
kayhern
force-pushed
the
fix-async-request-hook
branch
from
aa4d12f to
2602421
Compare
| if iscoroutinefunction(request_hook): | ||
| async_request_hook = kwargs.get("async_request_hook", request_hook) |
There was a problem hiding this comment.
From the previous review, I'm not sure we want this. I think the idea is to remove the default from the previous code here
| "async_response_hook", response_hook | ||
| ) | ||
| else: | ||
| async_response_hook = kwargs.get("async_response_hook") | ||
| if callable(request_hook): | ||
| _InstrumentedClient._request_hook = request_hook | ||
| if callable(async_request_hook): |
There was a problem hiding this comment.
And check here if it is a coroutine
There was a problem hiding this comment.
I believe you'd just need the below:
async_request_hook = kwargs.get("async_request_hook")
if callable(async_request_hook) and iscoroutinefunction(async_request_hook):
_InstrumentedAsyncClient._request_hook = async_request_hook
There was a problem hiding this comment.
Thanks @xrmx and @lzchen for your feedback. I've tried the code you mentioned but the testAsyncInstrumentationIntegration test case will fail with the change. The reason is in this case, request_hook is actually an aysnc request hook which we cannot set to None. Please correct me if I misunderstood.
|
Superseded by #2823 |
Fixes # (#2734)