opentelemetry-python-contrib icon indicating copy to clipboard operation
opentelemetry-python-contrib copied to clipboard

Audit and test otel instrumentation for httpx

Open SuryanarayanaPeri opened this issue 1 year ago • 11 comments
trafficstars

Description

Audit and test otel instrumentation for httpx

Fixes #979

Type of change

Please delete options that are not relevant.

  • [x] 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

SuryanarayanaPeri avatar Jul 19 '24 09:07 SuryanarayanaPeri

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: SuryanarayanaPeri / name: Suryanarayana Peri (51b2910b2b0c9ce343d014afe94320acce2fdbb4, d327a1dada46dac410bdaf5354636a679adc41c4, 102a8cad1877cda60f6d101c6fac57269f472a0f)

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

zhihali avatar Jul 21 '24 22:07 zhihali

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

SuryanarayanaPeri avatar Jul 22 '24 07:07 SuryanarayanaPeri

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

xrmx avatar Jul 24 '24 16:07 xrmx

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

brianwarner avatar Jul 24 '24 22:07 brianwarner

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

SuryanarayanaPeri avatar Jul 25 '24 07:07 SuryanarayanaPeri

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

SuryanarayanaPeri avatar Jul 31 '24 14:07 SuryanarayanaPeri

@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

xrmx avatar Jul 31 '24 14:07 xrmx

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

SuryanarayanaPeri avatar Aug 02 '24 11:08 SuryanarayanaPeri

Marking as a draft until test is fixed

emdneto avatar Aug 06 '24 15:08 emdneto

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

SuryanarayanaPeri avatar Sep 17 '24 13:09 SuryanarayanaPeri