fix Sync hook used as async hook in opentelemetry-instrumentation-httpx causing TypeError

[shijiadong2022]

Fixes # (#2734)

[lzchen]

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.

[emdneto]

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?

[lzchen]

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.

[shijiadong2022]

Thanks for your review. I will work on the changes.

[lzchen]

Please also add a CHANGELOG entry :)

[xrmx]

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

[xrmx]

And check here if it is a coroutine

[lzchen]

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

[shijiadong2022]

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.

[xrmx]

Superseded by #2823