Audit and test otel instrumentation for httpx #2723
Conversation
| HTTPXClientInstrumentor().instrument( | ||
| tracer_provider=trace_api.NoOpTracerProvider() | ||
| ) | ||
| async with httpx.AsyncClient() as client: |
There was a problem hiding this comment.
I think if you write test under the async test class, you should have test under sync as well. This will ensure that the instrumentation behaves correctly in both modes of the HTTPX client.
There was a problem hiding this comment.
Thank you for your comments. I am working on your recommendations and will be submitting the PR again.
There was a problem hiding this comment.
@zhihali - I have reviewed the code and don't think the test should be under sync as well. Can you please let me know why you think the test should be under sync too.
|
Also a small suggestion is include the related issue's link to the description, would be easier for reviewer to check. #979 instead of # 979 |
xrmx
changed the title
|
@zhihali - Thank you for your comments. I am working on your recommendations and will be submitting the PR again. |
brianwarner
force-pushed
the
httpx_issue_979
branch
from
8c73723
to
5a78c38
Compare
|
@brianwarner since it looks like you are the boss of the people doing this kind of PRs could you please sort out the CLA or even setup an internal review before opening PRs here? That would save quite a bit of time on this side. Thanks. |
Hi @xrmx, sure, I can provide some context here. I'm not the boss, but I am responsible for managing our internal processes. We've been configured in EasyCLA as a corporate contributor for quite some time, and I've enabled as much automation as the platform allows. However, in EasyCLA, every new contributor must verify that they want to be covered by the CCLA, and they're not prompted to do this until the PR is opened. There are also occasional config issues to dig through, but these also don't surface until the PR is opened. Either way, I'm helping contributors fix the issues on the back end. As for reviews, we have an internal process where commits are first staged against an internal mirror of this project, run through various checks, and then get pushed to the public fork. Any requested changes are first staged internally, and when they're approved I push them out to the public fork. If there's something additional you'd like to see from contributors, I can provide that feedback to the teams doing the work. |
SuryanarayanaPeri
force-pushed
the
httpx_issue_979
branch
from
0398478
to
2352199
Compare
|
@zhihali - I have made the changes to the lint and see all the tests are successful, except for the "changelog". I am not able to add the labels. Can you please review and let me know if all looks good. |
|
@xrmx - |
|
@SuryanarayanaPeri Not sure what are you talking about, here it is failing for this reason: |
brianwarner
force-pushed
the
httpx_issue_979
branch
from
2352199
to
0ff708c
Compare
Signed-off-by: Peri, Suryanarayana <[email protected]>
Signed-off-by: Peri, Suryanarayana <[email protected]>
…single quote for the client url Signed-off-by: Peri, Suryanarayana <[email protected]>
SuryanarayanaPeri
force-pushed
the
httpx_issue_979
branch
from
0ff708c
to
102a8ca
Compare
|
@xrmx - I have made the lint related changes and now all the Checks are passed. Can you please review. |
|
Marking as a draft until test is fixed |
|
@emdneto
emdneto left a comment • |
Description
Audit and test otel instrumentation for httpx
Fixes #979
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.