Skip to content

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Dec 13, 2024

We've found a bug where if the transport is reused in different HTTP clients, the transport gets instrumented multiple times.

Run the following MRE:

from opentelemetry.sdk.trace import TracerProvider
from opentelemetry.sdk.trace.export import SimpleSpanProcessor, ConsoleSpanExporter
from opentelemetry.instrumentation.httpx import HTTPXClientInstrumentor
import httpx

exporter = ConsoleSpanExporter()
span_processor = SimpleSpanProcessor(exporter)
tracer_provider = TracerProvider()
tracer_provider.add_span_processor(span_processor)


transport = httpx.HTTPTransport()
client = httpx.Client(transport=transport)
client2 = httpx.Client(transport=transport)

HTTPXClientInstrumentor().instrument_client(client, tracer_provider=tracer_provider)
HTTPXClientInstrumentor().instrument_client(client2, tracer_provider=tracer_provider)

client.get("https://httpbin.org/get")

You'll see 2 spans.

This PR proposes adding the _is_instrumented_by_opentelemetry to the Transport as well.

@Kludex Kludex marked this pull request as draft December 13, 2024 17:26
@Kludex Kludex marked this pull request as ready for review December 13, 2024 18:28
@Kludex Kludex requested a review from a team as a code owner December 13, 2024 19:36
if getattr(
client._transport, "_is_instrumented_by_opentelemetry", False
):
_logger.warning(
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't log a warning if someone instruments a new client with the same transport. So I think we need to keep the attribute and warning on the client, but silently do nothing if the transport is already instrumented.

@Kludex Kludex force-pushed the instrument-transport-instead-of-client branch from 85dbb70 to 7660a00 Compare December 13, 2024 20:39
@xrmx
Copy link
Contributor

xrmx commented Dec 16, 2024

I think the title and description do not match the code, you are moving the flag that signals that the transport has already been instrumented to the transport itself.

@Kludex
Copy link
Member Author

Kludex commented Dec 16, 2024

I think the title and description do not match the code, you are moving the flag that signals that the transport has already been instrumented to the transport itself.

Right. The PR started as what the title suggests, and I modified the code given this comment: #3106 (comment)

@Kludex Kludex changed the title httpx: instrument transport instead of client httpx: add the is_instrumented flag to the transport Dec 16, 2024
@Kludex
Copy link
Member Author

Kludex commented Dec 16, 2024

@xrmx Should be better now. 🙏

@Kludex
Copy link
Member Author

Kludex commented Dec 18, 2024

@xrmx this is not ready, sorry.

I'm missing something, now I have 2 spans on my test, instead of 3 (before my changes were 3). I still need to fix it somewhere.

@xrmx xrmx marked this pull request as draft December 18, 2024 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants