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

httpx: add the is_instrumented flag to the transport #3106

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

Kludex
Copy link
Contributor

@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, "_is_instrumented_by_opentelemetry", False):
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
Contributor 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
CHANGELOG.md Outdated Show resolved Hide resolved
@Kludex
Copy link
Contributor Author

Kludex commented Dec 16, 2024

@xrmx Should be better now. 🙏

@Kludex
Copy link
Contributor 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