-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Commit a7ebc26 disables Open Telemetry's httpx instrumentation in some scenarios #1144
Comments
Thank you for reporting, @palvarezcordoba ! cc @RobertCraigie , can you take a look? |
Thanks for the detailed report and for bisecting @palvarezcordoba! Unfortunately I don't think there's anything we can realistically do here and the fact that open telemetry worked independently of the import order was pure coincidence. It looks like I think we'll want to add a section to the README.md showcasing instrumentation and mentioning these points cc @rattrayalex |
@RobertCraigie Yeah, I didn't expect the issue to be solved here, although it would have been nice. Writing an example in the README would be very good. Your default clients are in For that reason, an example would be appreciated. Thanks, @rattrayalex and @RobertCraigie for your fast response. |
Ah of course, you shouldn't have to touch anything in import httpx
from opentelemetry.instrumentation.httpx import HTTPXClientInstrumentor
from openai import OpenAI
http_client = httpx.Client(transport=telemetry_transport)
HTTPXClientInstrumentor.instrument_client(http_client)
openai_client = OpenAI(http_client=http_client)
# use `openai_client` as normal
Sure we can update the changelog, the only reason it wasn't included in the first place is that we were unaware it would break this case. |
@RobertCraigie Where does telemetry_transport come from in that code? I'm testing this out in our samples, as I'm not seeing good traces for the OpenAI calls currently. |
Ah sorry it comes from this example: https://github.com/open-telemetry/opentelemetry-python-contrib/tree/main/instrumentation/opentelemetry-instrumentation-httpx#using-transports-directly import httpx
from opentelemetry.instrumentation.httpx import (
SyncOpenTelemetryTransport,
)
transport = httpx.HTTPTransport()
telemetry_transport = SyncOpenTelemetryTransport(transport) |
Sadly it's a bit more involved.
This seems just as fragile as |
I'm using the |
Confirm this is an issue with the Python library and not an underlying OpenAI API
Describe the bug
This commit a7ebc26, which was introduced in PR #966, for release v1.3.9, disables httpx instrumentation in some cases.
Specifically, it is disabled when openai is imported before instrumenting httpx.
This is because
opentelemetry.instrumentation.httpx .HTTPXClientInstrumentor._instrument
creates subclasses ofhttpx.Client
andhttpx.AsyncClient
. And then replaces the original clients with those subclasses, which adds telemetry.In the above commit, openai creates
SyncHttpxClientWrapper
andAsyncHttpxClientWrapper
subclasses of httpx's clients.That means that when we instrument first, and then import openai, the client wrappers inherit from the instrumented clients.
When we import openai first, and then instrument httpx, the wrapper clients inherit from the original httpx clients.
Maybe this should be addressed at the opentelemetry-python-contrib side, but even so, v1.3.9 broke telemetry.
Could the change implemented in #966 be implemented in a backward-compatible way?
If not, at the very least, the changelog should be updated, warning about this.
To Reproduce
HTTPXClientInstrumentor
fromopentelemetry.instrumentation.httpx
to instrument httpx.Code snippets
Running this, openai httpx requests are instrumented:
But this version is not instrumented, and the assertion fails:
OS
any
Python version
any
Library version
openai from v1.3.9.0 to v1.12.0
The text was updated successfully, but these errors were encountered: