Ensure httpx.get etc. get instrumented #2011
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Could this patch be considered for merging? We hit this gap in instrumentation in our deployments which relied on |
|
At least a CHANGELOG entry is required. |
|
Possibly related to #2364 cc @jeremydvoss
I would accept this PR with a test 😄 |
8 tasks
|
Closing as superseded by #2538 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This closes #1742 — "httpx instrumentation doesn't work for httpx.get, httpx.post, etc.".
The
httpx.get,httpx.post, etc. methods ultimately usehttpx._api.request, which makes instantiateshttpx._api.Client. Buthttpx.Clientis modified during instrumentation without updating the value imported in thehttpx._apimodule.Fixing this was as easy as also patching the
Clientvalue inhttpx._api.Fixes #1742
Type of change
I am not sure if it counts as a feature or bug fix. I'm sure it could be argued both ways, but I decided to call it a bug fix.
How Has This Been Tested?
I made this change and confirmed that it caused spans from the HTTPX instrumentation to be nested properly in my observability backend. I can add tests with a bit of guidance about where to look/modify/what would be desired. But I'd like to get confirmation that this is likely to be accepted before putting in the effort.
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.
I don't think a documentation change is necessary; I'll update changelogs after adding unit tests once confirmed this is desirable.