HTTPX instrumentor wraps transport repeatedly

[xoob]

The new HTTPX instrumentor wraps client._transport repeatedly if the same client is re-used for multiple requests. @jomasti

Describe your environment

{'implementation_name': 'cpython',
 'implementation_version': '3.7.9',
 'os_name': 'posix',
 'platform_machine': 'x86_64',
 'platform_python_implementation': 'CPython',
 'platform_release': '20.5.0',
 'platform_system': 'Darwin',
 'platform_version': 'Darwin Kernel Version 20.5.0: Sat May  8 05:10:33 PDT '
                     '2021; root:xnu-7195.121.3~9/RELEASE_X86_64',
 'python_full_version': '3.7.9',
 'python_version': '3.7',
 'sys_platform': 'darwin'}

httpx==0.18.2
opentelemetry-instrumentation-httpx==0.23.dev0

Steps to reproduce

Create an instance of httpx.Client and make multiple calls on it. Observe that the number of nested spans increases with each successive call, because the wrapped transport is re-wrapped with each request.

HTTPXClientInstrumentor().instrument()
...
shared_client = httpx.Client()
shared_client.post('https://example.org')
shared_client.get('https://example.org/foo')
shared_client.get('https://example.org/bar')

What is the expected behavior?
A single span should appear per request.

What is the actual behavior?
One, two, then three spans appear per request.

Additional context
The problem is in instrumented_sync_send where the transport is indiscriminately wrapped, even if it already is an instance of SyncOpenTelemetryTransport.

transport = instance._transport or httpx.HTTPTransport()
# instance._transport starts as httpx.HTTPTransport, then becomes
# SyncOpenTelemetryTransport

[lzchen]

I believe calling instrument() and instrument_client(client) will also generate duplicate spans.

[jomasti]

Oh, yeah, my bad. This is due to me changing up the approach of wrapping __init__ to send instead after it was requested the the instrumentation also affect already created clients. I should have had a test with multiple requests that would have caught this.

I think we can easily add an attribute that says it's been instrumented and checked. The only issue left after that still would be the leftover attribute and transport on those instances. It's tricky (I think), and makes me rethink the approach of using the transport. Maybe this instrumentation should just wrap the send function without the transport usage. The only thing lost would be the manual creation use case, which isn't so bad.

I won't have the time to get a PR up for this, so I would appreciate if someone else could look into it.

[lzchen]

@jomasti
I was looking into HTTPx instrumentation after working on this. Do you think replicating what flask is doing would solve this use case? Instead of wrapping send and transport each time upon calls, we simply replace the function library and it is wrapped upon instantiation?

[jomasti]

@lzchen So subclassing both client classes and replacing them in the module? Seems like it should work and would be easier to clean up when removing the instrumentation.