Skip to content
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.

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

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

Closed
horw opened this issue Jul 24, 2024 · 5 comments · Fixed by #2823
Closed

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

horw opened this issue Jul 24, 2024 · 5 comments · Fixed by #2823
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@horw
Copy link

horw commented Jul 24, 2024

Describe your environment

OS: Ubuntu
Python 3.10.12
Name: opentelemetry-instrumentation-httpx
Version: 0.46b0

What happened?

code from lib

request_hook = kwargs.get("request_hook")
response_hook = kwargs.get("response_hook")
async_request_hook = kwargs.get("async_request_hook", request_hook)
async_response_hook = kwargs.get("async_response_hook", response_hook

If you set up request_hook, it will also be used for async_request_hook if you don't initialize async_request_hook. This could lead to issues because you don't want to pass an async function to request_hook. If a non-async function is passed, it may not work as expected. It seems that there needs to be a wrapper around request_hook to check if it is not async and then wrap it accordingly.

Steps to Reproduce

def hook(span, req):
    pass

HTTPXClientInstrumentor().instrument(request_hook=hook)
    
with Client() as client:
    client.post("http://localhost:9999/hello")
async with AsyncClient() as client:
    await client.post("http://localhost:9999/hello")

Expected Result

Both requests completed

Actual Result

await self._request_hook(span, request_info)
TypeError: object NoneType can't be used in 'await' expression

Additional context

No response

Would you like to implement a fix?

None

@horw horw added the bug Something isn't working label Jul 24, 2024
@shijiadong2022
Copy link
Contributor

Can you assign this one to me?

@xrmx
Copy link
Contributor

xrmx commented Jul 24, 2024

@shijiadong2022 looking forward your PR

@emdneto emdneto changed the title httpx instrumentation Sync hook used as async hook in opentelemetry-instrumentation-httpx causing TypeError Jul 24, 2024
@emdneto emdneto added good first issue Good for newcomers help wanted Extra attention is needed labels Jul 25, 2024
@xrmx
Copy link
Contributor

xrmx commented Jul 31, 2024

@shijiadong2022 have you started working on this?

@shijiadong2022
Copy link
Contributor

I am working on it and will submit a PR soon

xrmx added a commit to xrmx/opentelemetry-python-contrib that referenced this issue Aug 25, 2024
Don't default to sync hooks for async hooks, then check that they are
actually async.

Fixes open-telemetry#2734
@xrmx
Copy link
Contributor

xrmx commented Aug 26, 2024

@horw Could you please double check if the fix works for you? To have the desired outcome with the updated code you should provide yourself the async_request_hook callback e.g. HTTPXClientInstrumentor().instrument(request_hook=hook, async_request_hook=async_request_hook)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants