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.

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

feat(otlp-exporter-base): support CONNECT proxies #5054

Conversation

raphael-theriault-swi
Copy link
Contributor

@raphael-theriault-swi raphael-theriault-swi commented Oct 9, 2024

Which problem is this PR solving?

Currently it's possible to use HTTP CONNECT proxies with gRPC through the @grpc/grpc-js environment variables, however the same is not possible for the HTTP exporters.

At the moment this can't be configured with environment variables, and as far as I'm aware OTel doesn't define any for proxies ? The option could be exposed with the same HTTP(S)_PROXY variables as gRPC but I don't know if that's desirable.

I haven't updated the documentation yet in case there are any suggestions for better or additional ways to configure this.

Short description of the changes

This adds a new proxy?: string configuration value for OTLPExporterNodeBase. When provided, the transport uses a custom Agent which overrides the createConnection method to obtain the socket through an HTTP CONNECT request to the proxy url.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

Most of the transport unit tests have been mirrored and adapted to test similar proxied scenarios.

For testing the feature in practice one can use something like https://github.com/mock-server/mockserver or any other proxy with CONNECT support.

Checklist:

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

@raphael-theriault-swi raphael-theriault-swi requested a review from a team as a code owner October 9, 2024 00:49
Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.26%. Comparing base (f8ab559) to head (2ad170a).
Report is 32 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5054      +/-   ##
==========================================
- Coverage   93.39%   93.26%   -0.14%     
==========================================
  Files          46      317     +271     
  Lines         712     8211    +7499     
  Branches      120     1645    +1525     
==========================================
+ Hits          665     7658    +6993     
- Misses         47      553     +506     
Files with missing lines Coverage Δ
...-base/src/configuration/otlp-http-configuration.ts 100.00% <100.00%> (ø)
...ter-base/src/platform/node/OTLPExporterNodeBase.ts 40.47% <ø> (ø)
...-base/src/platform/node/http-exporter-transport.ts 93.75% <100.00%> (ø)
...ter-base/src/platform/node/http-transport-utils.ts 98.43% <100.00%> (ø)
...ages/otlp-exporter-base/src/platform/node/types.ts 100.00% <ø> (ø)

... and 262 files with indirect coverage changes

@pichlermarc
Copy link
Member

Hi @raphael-theriault-swi, I really appreciate you taking the time to work on this feature. However, we're currently going through a re-write of the exporters and adding this feature was not discussed prior to opening this PR. I do see potential in adding this feature, but I think it would better to wait until the re-write is complete to avoid unnecessary churn.

I do believe that once #4116 is exposed to the end-user we may be able to offer this feature as an extension transport. I am going to close this but would love to discuss adding this feature on an issue once we've established the new exporter design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants