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

Allow instrumenting a single httpx client, fix tests for OTel 1.28 #575

Merged
merged 8 commits into from
Nov 12, 2024

Conversation

alexmojaki
Copy link
Contributor

No description provided.

Copy link

cloudflare-workers-and-pages bot commented Nov 6, 2024

Deploying logfire-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: a85e4c4
Status: ✅  Deploy successful!
Preview URL: https://1340ee60.logfire-docs.pages.dev
Branch Preview URL: https://alex-otel-1-28.logfire-docs.pages.dev

View logs

@samuelcolvin
Copy link
Member

samuelcolvin commented Nov 7, 2024

The failure is a key error here:

    @pytest.mark.anyio
    async def test_httpx_instrumentation(exporter: TestExporter):
        # The purpose of this mock transport is to ensure that the traceparent header is provided
        # without needing to actually make a network request
        def handler(request: Request):
            return httpx.Response(200, headers=request.headers)
    
        transport = httpx.MockTransport(handler)
    
        with logfire.span('test span') as span:
            assert span.context
            trace_id = span.context.trace_id
            with httpx.Client(transport=transport) as client:
                response = client.get('https://example.org/')
                # Validation of context propagation: ensure that the traceparent header contains the trace ID
                print(list(response.headers.keys()))
>               traceparent_header = response.headers['traceparent']

It looks like the traceparent really isn't being set. Could it be that the httpx instrumentation really isn't compatible with the most recent release?

@sydney-runkle
Copy link
Member

Could this be related to #529?

@@ -5,7 +5,7 @@ build-backend = "hatchling.build"
[project]
name = "logfire"
version = "2.1.2"
description = "The best Python observability tool! 🪵🔥"
description = "The best Python observability tool!"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pyright was failing to parse this file because of the emojis: microsoft/pyright#9412

Copy link
Member

Choose a reason for hiding this comment

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

This is sad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is closed, upgrading pyright should allow reverting this.

Copy link

codecov bot commented Nov 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (15ff969) to head (a85e4c4).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #575   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          133       133           
  Lines        10257     10263    +6     
  Branches      1399      1399           
=========================================
+ Hits         10257     10263    +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alexmojaki alexmojaki changed the title WIP test fixes for OTEL 1.28 Allow instrumenting a single httpx client, fix tests for OTel 1.28 Nov 12, 2024
@alexmojaki
Copy link
Contributor Author

Yes, OTEL fixed #529, and in the process that broke tests, but in a test-specific way. They now patch the HTTPTransport class if you don't pass a client, and we are using MockTransport in the test. So I added the ability to instrument a single client, meaning that its transport gets patched directly. The problem wasn't specifically about traceparent, there just wasn't any instrumentation at all without the test change.

@alexmojaki alexmojaki marked this pull request as ready for review November 12, 2024 09:33
@alexmojaki alexmojaki requested a review from Kludex November 12, 2024 09:33
logfire/_internal/integrations/httpx.py Outdated Show resolved Hide resolved
logfire/_internal/integrations/httpx.py Outdated Show resolved Hide resolved
@@ -5,7 +5,7 @@ build-backend = "hatchling.build"
[project]
name = "logfire"
version = "2.1.2"
description = "The best Python observability tool! 🪵🔥"
description = "The best Python observability tool!"
Copy link
Member

Choose a reason for hiding this comment

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

This is sad.

pyproject.toml Show resolved Hide resolved
Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Maybe we want to test with AsyncClient as well?

But all good. Thanks.

@Kludex
Copy link
Member

Kludex commented Nov 12, 2024

I've created #581 to document this. I can handle that. 👍

@alexmojaki alexmojaki enabled auto-merge (squash) November 12, 2024 11:52
@alexmojaki alexmojaki disabled auto-merge November 12, 2024 12:33
@alexmojaki alexmojaki merged commit d8042bf into main Nov 12, 2024
20 checks passed
@alexmojaki alexmojaki deleted the alex/otel-1.28 branch November 12, 2024 14:19
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.

4 participants