Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3ff1113
fix Sync hook used as async hook
shijiadong2022 Jul 26, 2024
f3347b6
fix sync request as async request
shijiadong2022 Aug 19, 2024
e548302
change format of changelog
shijiadong2022 Aug 19, 2024
23d3131
Resolve merge conflict
shijiadong2022 Aug 20, 2024
2602421
Resolve conflict of changelog
shijiadong2022 Aug 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#2385](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2385))
- `opentelemetry-instrumentation-asyncio` Fixes async generator coroutines not being awaited
([#2792](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2792))
- `opentelemetry-instrumentation-httpx` Fix sync hook used as async hook causing `TypeError`
([#2794](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2794))
- `opentelemetry-instrumentation-tornado` Handle http client exception and record exception info into span
([#2563](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2563))

Expand Down
13 changes: 11 additions & 2 deletions ...n/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ async def async_response_hook(span, request, response):
"""
import logging
import typing
from inspect import iscoroutinefunction
from types import TracebackType

import httpx
Expand Down Expand Up @@ -731,8 +732,16 @@ 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

@emdneto emdneto Aug 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

async_response_hook = kwargs.get("async_response_hook", response_hook)
if iscoroutinefunction(request_hook):
async_request_hook = kwargs.get("async_request_hook", request_hook)
Comment on lines +735 to +736
Copy link
Contributor

Choose a reason for hiding this comment

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

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

else:
async_request_hook = kwargs.get("async_request_hook")
if iscoroutinefunction(response_hook):
async_response_hook = kwargs.get(
"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):
Copy link
Contributor

Choose a reason for hiding this comment

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

And check here if it is a coroutine

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Expand Down
24 changes: 24 additions & 0 deletions instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1214,3 +1214,27 @@ def test_basic_multiple(self):
self.perform_request(self.URL, client=self.client)
self.perform_request(self.URL, client=self.client2)
self.assert_span(num_spans=2)

def test_async_request_hook_with_sync_hook_value_provided(self):
HTTPXClientInstrumentor().instrument(
tracer_provider=self.tracer_provider,
request_hook=_request_hook,
)
client = self.create_client()
result = self.perform_request(self.URL, client=client)
self.assertEqual(result.text, "Hello!")
span = self.assert_span()
self.assertEqual(span.name, "GET")
HTTPXClientInstrumentor().uninstrument()

def test_async_response_hook_with_sync_hook_value_provided(self):
HTTPXClientInstrumentor().instrument(
tracer_provider=self.tracer_provider,
response_hook=_response_hook,
)
client = self.create_client()
result = self.perform_request(self.URL, client=client)
self.assertEqual(result.text, "Hello!")
span = self.assert_span()
self.assertEqual(span.name, "GET")
HTTPXClientInstrumentor().uninstrument()