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.

Sign up for GitHub

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

Jump to bottom

Audit and test otel instrumentation for httpx #2723

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

Audit and test otel instrumentation for httpx #2723

wants to merge 3 commits into from

Conversation

SuryanarayanaPeri
Copy link
Contributor

@SuryanarayanaPeri SuryanarayanaPeri commented Jul 19, 2024 •
edited by xrmx
Loading

Description

Audit and test otel instrumentation for httpx

Fixes #979

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

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

  • Test A

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Jul 19, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

zhihali
zhihali reviewed Jul 21, 2024
View reviewed changes
Copy link
Contributor

@zhihali zhihali left a comment

Choose a reason for hiding this comment

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

Please sign the CLA, and I can see the lint test fail. Recomand to check the tox.ini and find the tox command and run the lint check command locally(or use pre-commit), after it passed then commit again.

instrumentation/opentelemetry-instrumentation-httpx/tests/test_httpx_integration.py Outdated
HTTPXClientInstrumentor().instrument(
tracer_provider=trace_api.NoOpTracerProvider()
)
async with httpx.AsyncClient() as client:
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your comments. I am working on your recommendations and will be submitting the PR again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

@zhihali
Copy link
Contributor

zhihali commented Jul 21, 2024

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

@emdneto emdneto requested a review from a team July 21, 2024 23:31
@xrmx xrmx changed the title httpx_issue_979 - Audit and test otel instrumentation for httpx Audit and test otel instrumentation for httpx Jul 22, 2024
@SuryanarayanaPeri
Copy link
Contributor Author

@zhihali - Thank you for your comments. I am working on your recommendations and will be submitting the PR again.

@xrmx xrmx marked this pull request as draft July 22, 2024 13:50
@xrmx
Copy link
Contributor

xrmx commented Jul 24, 2024 •
edited
Loading

@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.

@brianwarner
Copy link

@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 SuryanarayanaPeri marked this pull request as ready for review July 25, 2024 07:45
@SuryanarayanaPeri
Copy link
Contributor Author

@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 xrmx added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 25, 2024
@SuryanarayanaPeri
Copy link
Contributor Author

@xrmx -
The Tox tests for Python 3.10 are failing on my local machine after several changes were pushed to the repository in the past few days. Previously, they were running completely fine, but some of the tests are now failing, possibly due to the recent changes. The tests seem to be running fine with Python 3.9. Please help me with this issue and let me know if I am missing anything.

@xrmx
Copy link
Contributor

xrmx commented Jul 31, 2024

@SuryanarayanaPeri Not sure what are you talking about, here it is failing for this reason:
https://github.com/open-telemetry/opentelemetry-python-contrib/actions/runs/10090168599/job/27899069159#step:6:261

@SuryanarayanaPeri
Copy link
Contributor Author

@xrmx - I have made the lint related changes and now all the Checks are passed. Can you please review.

emdneto
emdneto reviewed Aug 6, 2024
View reviewed changes
Copy link
Member

@emdneto emdneto left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think the test is wrong. Please take a look at how other tests are implemented. The no_op_tracer_provider test should be run for both Async and Sync clients.

@emdneto emdneto marked this pull request as draft August 6, 2024 15:20
@emdneto
Copy link
Member

emdneto commented Aug 6, 2024 •
edited
Loading

Marking as a draft until test is fixed

@SuryanarayanaPeri
Copy link
Contributor Author

@emdneto
Can you please suggest the similar tests that are implemented for me to take as a reference to complete this issue.

  • Surya

emdneto left a comment •
I think the test is wrong. Please take a look at how other tests are implemented. The no_op_tracer_provider test should be run for both Async and Sync clients.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhihali zhihali zhihali left review comments

@emdneto emdneto emdneto left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Audit and test opentelemetry-instrumentation-httpx
5 participants
@SuryanarayanaPeri @zhihali @xrmx @brianwarner @emdneto