opentelemetry-python-contrib icon indicating copy to clipboard operation
opentelemetry-python-contrib copied to clipboard

HTTPXClientInstrumentor does not instrument subclasses of httpx.Client created before instrumentation

Open palvarezcordoba opened this issue 1 year ago • 4 comments

When you call HTTPXClientInstrumentor.instrument, what happens in HTTPXClientInstrumentor._instrument, is that it creates subclasses of httpx.Clientand httpx.AsyncClient, and replaces httpx clients with those subclasses: https://github.com/open-telemetry/opentelemetry-python-contrib/blob/8fd2105ceae604cf39a67f1c4dd154753b43fcd1/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/init.py#L569-L570

The problem with this approach is that subclasses of httpx clients created before calling HTTPXClientInstrumentor.instrument will not be instrumented, because they are already subclasses of the original client class.

When I updated openai I found that it broke telemetry. The reason is that it now uses subclasses of httpx clients. So, if openai is imported before instrumenting httpx, telemetry is not going to work. You can check this issue: openai/openai-python#1144. To be clear, this is not a bug on openai. It is a problem in this package, which maybe I would not label as bug, but it is a surprising and unexpected behavior from a user's point of view, and as far as I know, it's not documented.

Steps to reproduce

Running this, openai httpx requests are instrumented:

from opentelemetry.instrumentation.httpx import HTTPXClientInstrumentor, _InstrumentedClient

HTTPXClientInstrumentor().instrument()
from openai._base_client import SyncHttpxClientWrapper
assert issubclass(SyncHttpxClientWrapper, _InstrumentedClient)

But this version is not instrumented, and the assertion fails:

from openai._base_client import SyncHttpxClientWrapper
from opentelemetry.instrumentation.httpx import HTTPXClientInstrumentor, _InstrumentedClient

HTTPXClientInstrumentor().instrument()
assert issubclass(SyncHttpxClientWrapper, _InstrumentedClient)

Again, not openai specific, it is the same with any subclass:

from opentelemetry.instrumentation.httpx import (
    HTTPXClientInstrumentor,
    _InstrumentedClient,
)
import httpx

class Client(httpx.Client):
    pass

HTTPXClientInstrumentor().instrument()
assert issubclass(Client, _InstrumentedClient)

What is the expected behavior? Subclasses of httpx's clients are instrumented even if they are created before instrumenting.

What is the actual behavior? Subclasses of httpx's clients are not instrumented if they are created before instrumenting.

Solution proposal

The code of _InstrumentedClient and _InstrumentedAsyncClient is this: https://github.com/open-telemetry/opentelemetry-python-contrib/blob/8fd2105ceae604cf39a67f1c4dd154753b43fcd1/instrumentation/opentelemetry-instrumentation-httpx/src/opentelemetry/instrumentation/httpx/init.py#L491-L526

It is subclassing to add some properties, and wrapping _transport in a SyncOpenTelemetryTransport or it's async version. One way to avoid this behavior would be to patch directly the client classes, instead of patching the httpx package to replace the original clients with the instrumented ones.

However, I'm not sure about what unintentional consecuences this could have. What do you think about this?

palvarezcordoba avatar Feb 10 '24 19:02 palvarezcordoba