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

Implement Client Key and Certificate File Support for All OTLP Exporters #4116

Merged
merged 32 commits into from
Aug 20, 2024
Merged

Implement Client Key and Certificate File Support for All OTLP Exporters #4116

merged 32 commits into from
Aug 20, 2024

Conversation

sandy2008
Copy link
Contributor

@sandy2008 sandy2008 commented Aug 11, 2024 •
edited
Loading

Description

This PR extends the support for client key files and client certificate files to all OTLP exporters, building on the initial work from #3590, which is no longer maintained.

Type of change

  • New feature (non-breaking change that adds functionality)

How Has This Been Tested?

Tests have been updated and validated using the following commands:

  • tox -e opentelemetry-exporter-otlp-proto-grpc
  • tox -e opentelemetry-exporter-otlp-proto-http

Does This PR Require a Contrib Repo Change?

  • No.

Checklist:

  • Followed the project's style guidelines
  • Updated the changelogs
  • Added unit tests
  • Updated documentation

License Information

THIS SOFTWARE IS CONTRIBUTED SUBJECT TO THE TERMS OF THE APACHE LICENSE V.2.0. YOU MAY OBTAIN A COPY OF THE LICENSE AT https://github.com/open-telemetry/opentelemetry-python/blob/master/LICENSE.

THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.

chittalpatel and others added 8 commits December 15, 2023 09:57
@sandy2008 sandy2008 marked this pull request as ready for review August 11, 2024 14:53
@sandy2008 sandy2008 requested a review from a team August 11, 2024 14:53
This was referenced Aug 11, 2024
Open
Closed
Closed
@emdneto emdneto added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Aug 12, 2024
pmcollins
pmcollins reviewed Aug 13, 2024
View reviewed changes
Copy link
Member

@pmcollins pmcollins left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on. I believe you've brought these changes in from another PR, but I've made some comments below.

...pentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/exporter.py Outdated Show resolved Hide resolved
...pentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/exporter.py Show resolved Hide resolved
...pentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/exporter.py Outdated Show resolved Hide resolved
...xporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_log_exporter/__init__.py Show resolved Hide resolved
@sandy2008
refactor(otlp exporter): refactor client certificate and key handling
e59453d 
- Moved `client_key_file` and `client_certificate_file` to local variables as suggested.
- Added a comment explaining the retention of these as instance variables for testing purposes.
- Addressed all review comments to improve code readability and maintainability.
@sandy2008 sandy2008 requested a review from pmcollins August 14, 2024 08:27
@sandy2008
Copy link
Contributor Author

@pmcollins Thank you for your comments, resolved accordingly :)

@sandy2008
refactor(otlp exporter): refactor client certificate and key handling
d254a53 
- Moved `client_key_file` and `client_certificate_file` to local variables as suggested.
- Added a comment explaining the retention of these as instance variables for testing purposes.
- Addressed all review comments to improve code readability and maintainability.
@sandy2008 sandy2008 force-pushed the 2991-client-key-and-certificate-for-otlp-exporters branch from 92ebea0 to d254a53 Compare August 14, 2024 09:04
pmcollins
pmcollins reviewed Aug 14, 2024
View reviewed changes
Copy link
Member

@pmcollins pmcollins left a comment

Choose a reason for hiding this comment

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

Thanks for considering my feedback. Can you push your PR updates -- I'm not seeing them here.

...xporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_log_exporter/__init__.py Outdated Show resolved Hide resolved
@sandy2008
Copy link
Contributor Author

Thanks for considering my feedback. Can you push your PR updates -- I'm not seeing them here.

Hi @pmcollins :) Thank you for your reply!
I will adjust based on the new comment :) btw, I think I pushed the codes, did I miss or misunderstand any part of the comments..?

@pmcollins
Copy link
Member

Thanks for considering my feedback. Can you push your PR updates -- I'm not seeing them here.

Hi @pmcollins :) Thank you for your reply! I will adjust based on the new comment :) btw, I think I pushed the codes, did I miss or misunderstand any part of the comments..?

Oh sorry, it's fine -- I think it was a problem on my end.

@sandy2008
Copy link
Contributor Author

@pmcollins Hi, please help review again :)

@sandy2008 sandy2008 requested a review from pmcollins August 16, 2024 16:18
@sandy2008
Copy link
Contributor Author

sandy2008 commented Aug 16, 2024 •
edited
Loading

@pmcollins Hi! I think all the above comments & changes are resolved & made :)
Please kindly take a look if you have got some time :)

@sandy2008
Copy link
Contributor Author

@pmcollins Hi, all the issues have been resolved :)

@sandy2008 sandy2008 requested a review from pmcollins August 20, 2024 12:20
pmcollins
pmcollins approved these changes Aug 20, 2024
View reviewed changes
Copy link
Member

@pmcollins pmcollins left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the changes.

@sandy2008
Copy link
Contributor Author

LGTM. Thanks for the changes.

@pmcollins Thanks! What is the step before we can get it merged btw :)

@pmcollins
Copy link
Member

LGTM. Thanks for the changes.

@pmcollins Thanks! What is the step before we can get it merged btw :)

No problem at all. Leighton and/or Diego will take it from here.

@sandy2008
Copy link
Contributor Author

LGTM. Thanks for the changes.

@pmcollins Thanks! What is the step before we can get it merged btw :)

No problem at all. Leighton and/or Diego will take it from here.

Thanks! @lzchen , @ocelotl Please help merge it if you have time :)!

@lzchen lzchen merged commit e8cf94c into open-telemetry:main Aug 20, 2024
376 checks passed
hyoinandout pushed a commit to hyoinandout/opentelemetry-python that referenced this pull request Aug 29, 2024
@sandy2008 @hyoinandout
Implement Client Key and Certificate File Support for All OTLP Export…
e746026 
…ers (open-telemetry#4116)
@emdneto emdneto mentioned this pull request Sep 5, 2024
Closed
@emdneto emdneto mentioned this pull request Oct 10, 2024
Closed
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzchen lzchen lzchen approved these changes

@emdneto emdneto emdneto approved these changes

@ocelotl ocelotl ocelotl approved these changes

@sokoide sokoide sokoide approved these changes

@pmcollins pmcollins pmcollins approved these changes

@xrmx xrmx xrmx approved these changes

Assignees
No one assigned
Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@sandy2008 @pmcollins @xrmx @sokoide @ocelotl @emdneto @lzchen @chittalpatel @srikanthccv