Adding proxy functionality

[marcschumacher]

This PR adds functionality to enable the use of proxies with OpenTelemetry (logs, metrics and spans). It already adds the ability to configure it using

otel.exporter.otlp.[metrics|traces|logs].proxy.host = <proxy hostname>
otel.exporter.otlp.[metrics|traces|logs].proxy.port = <port number>

[linux-foundation-easycla]

• ✅login: marcschumacher / (db4da92)
• ✅login: marcschumacher / (db4da92, 20b39b0)
• ✅login: marcschumacher / (db4da92, 20b39b0, 1c19b98)
• ✅login: marcschumacher / (db4da92, 20b39b0, 1c19b98, 782b981)
• ✅login: marcschumacher / (db4da92, 20b39b0, 1c19b98, 782b981, 63b615a)
• ✅login: marcschumacher / (db4da92, 20b39b0, 1c19b98, 782b981, 63b615a, 54dc43d)

The committers listed above are authorized under a signed CLA.

[trask]

hi @marcschumacher! can you sign the CLA above?

It already adds the ability to configure it using

otel.exporter.otlp.[metrics|traces|logs].proxy.host = <proxy hostname>
otel.exporter.otlp.[metrics|traces|logs].proxy.port = <port number>

I'm not sure if we can support these properties before they're added to the specification: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md#configuration-options

and unfortunately there's a moratorium on new env vars until the new configuration mechanism is in place

[jack-berg]

Hi thanks for the contribution. The crux of this will be determine the appropriate format to expose proxy information. Given that we have strict backwards compatibility guarantees we'll be stuck supporting whatever we come up for the foreseeable future. I want to make sure that the configuration is exhaustive of what users would commonly expect, and plan on verifying this by evaluating how a number of the popular http client libraries expose proxy configuration. This will take some time to get to for me, so not realistic to get it done before the 2/9/24 release, but could be on the table for march.

[trask]

sometimes forward proxies require a username and password, though much less frequently.

maybe something like setProxy(ProxyOptions) and ProxyOptions.builder().setHost(...).setPort(...).build() to give room for expansion?

[jack-berg]

maybe something like setProxy(ProxyOptions) and ProxyOptions.builder().setHost(...).setPort(...).build() to give room for expansion?

Yup that's a good thought. If we determine there are say more than 2 parameters, and that the parameters are sometimes present and sometimes not, a build seems appropriate.

[marcschumacher]

Okay, I can file a change for this part. @jack-berg Would this give some more traction to the approval process also in terms of the discussion for being backwards compatible?

[marcschumacher]

maybe something like setProxy(ProxyOptions) and ProxyOptions.builder().setHost(...).setPort(...).build() to give room for expansion?

@trask As setting the host and port are essential, I would rather put this as parameter in the build method than to make it some kind of optional having to call setHost and setPort optionally. Or would you rather like to have validation for this inside the builder and still not supply them using the method?
In the end it would look like ProxyOptions.builder(String host, int port).build(), still being open for extension.

[jack-berg]

In the end it would look like ProxyOptions.builder(String host, int port).build(), still being open for extension.

Yup, that's right. Builders which have parameters the user is required to provide accept those as initialization arguments for the builder.

[marcschumacher]

I have replaced the proxyHost and proxyPort with a new ProxyOptions object, please check.

[marcschumacher]

hi @marcschumacher! can you sign the CLA above?
@trask I already reached out to our open source team about this. I am confident that there is no problem with this, but need to wait until they reply, which will probably happen today.

I'm not sure if we can support these properties before they're added to the specification: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md#configuration-options

How can this be added to the specification? Could we make this hidden properties to already be able to make use of it until this is approved? Or is there any other way to already use such configuration?

and unfortunately there's a open-telemetry/opentelemetry-specification#2891 (comment) until the open-telemetry/opentelemetry-specification#2920 is in place

I might be wrong, but I think the type of configuration I introduced does not apply to this moratorium, no?

It says it applies to:

Anything that is not statically typed. That is every environment variable must have a known expected type.

[codecov]

Codecov Report

Attention: 49 lines in your changes are missing coverage. Please review.

Comparison is base (3e7302e) 91.06% compared to head (54dc43d) 90.79%.
Report is 29 commits behind head on main.

Files	Patch %	Lines
.../opentelemetry/sdk/common/export/ProxyOptions.java	0.00%	13 Missing ⚠️
...orter/sender/okhttp/internal/OkHttpGrpcSender.java	0.00%	4 Missing and 1 partial ⚠️
...orter/sender/okhttp/internal/OkHttpHttpSender.java	0.00%	4 Missing and 1 partial ⚠️
...lp/http/logs/OtlpHttpLogRecordExporterBuilder.java	0.00%	3 Missing ⚠️
...lp/http/metrics/OtlpHttpMetricExporterBuilder.java	0.00%	3 Missing ⚠️
...r/otlp/http/trace/OtlpHttpSpanExporterBuilder.java	0.00%	3 Missing ⚠️
...er/otlp/logs/OtlpGrpcLogRecordExporterBuilder.java	0.00%	3 Missing ⚠️
...porter/otlp/trace/OtlpGrpcSpanExporterBuilder.java	0.00%	3 Missing ⚠️
...ry/exporter/sender/jdk/internal/JdkHttpSender.java	25.00%	2 Missing and 1 partial ⚠️
...ry/exporter/internal/grpc/GrpcExporterBuilder.java	50.00%	2 Missing ⚠️
... and 3 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6206      +/-   ##
============================================
- Coverage     91.06%   90.79%   -0.28%     
- Complexity     5672     5687      +15     
============================================
  Files           620      622       +2     
  Lines         16560    16703     +143     
  Branches       1690     1707      +17     
============================================
+ Hits          15080    15165      +85     
- Misses          993     1043      +50     
- Partials        487      495       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jack-berg]

I've analyzed various http client libraries and the options they expose for configuring proxies:

• Reactor Netty: type (http, socks4, socks5), host, port, non-proxy hosts (?), http headers, connect timeout, username, password.
• Apache HTTP client: host, port scheme, username, password.
• Jetty HTTP client: type (http, socks4, socks5), host, port, proxy credentials (realm, username, password), server credentials (realm, username, password).
• Jersey HTTP client: uri, username, password.
• Java HttpURLConnection: type (http, socks), host, port, SSLContext.
• OkHttp: type (http, socks), host, port, proxy authenticator (can modify headers if challenged), proxy selector (select eligible proxy for given route).
• Jdk 11+ HttpClient: type (http, socks), host, port, authentication through Authenticator interface when getRequestType() returns RequestorType.PROXY.

One recurring pattern is to expose these options through the java ProxySelector abstract class, which produces a list of relevant Proxy for a particular URI. It combines a variety of concepts into one interface, including host, port, and the ability load balance between multiple proxies.

Given that this concept is built into the JDK, and is used to build the proxy mechanisms for both HTTP client libraries we use for sender implementations, I think we should consider using it in our proxy configuration rather than trying to re-invent the wheel. Here's what I propose:

• Introduce a ProxyOptions class with initially just one option: ProxySelector getProxySelector(). This buys a lot in terms of configurability with limited API surface area.
• Update all the OtlpHttp{Signal}ExporterBuilders with a new setProxyOptions(ProxyOption) method. Punt on OtlpGrpc{Signal}ExporterBuilders until more research is done on how proxying intersects with gRPC.
• Punt on other proxy configuration options for now, including username / password.
• Introduce a ProxyOptions builder later when there are more options and more ways to construct. For now, just expose ProxyOptions.create(ProxySelector) and ProxyOoptions.create(InetSocketAddress). The latter is syntactic sugar for the simple case and implements ProxySelector getProxySelector() using ProxySelector.of(InetSocketAddress).

I would normally say that we should incubate this in an internal package, but don't feel that's necessary if we use ProxySelector instead of trying to invent something new.

@marcschumacher, @open-telemetry/java-approvers WDYT? If you agree with this direction, I can help refactor this PR. I've been working off this branch locally, including additional testing.

[marcschumacher]

@jack-berg This sounds like a great idea. Feel free to adjust my PR and let me know how I can further contribute!

[breedx-splk]

+1 yeah this sounds like a solid approach to me as well. I'm confident that users will ask for user/pass in short order, but I'm also confident that will be a nice/small/incremental add-on with the approach described above. I also think kicking the gRPC can (of worms) down the road is a good idea.

[jack-berg]

Opened a PR to update @marcschumacher's branch here.

Figured it was a significant enough set of changes that I out to give you the chance to review instead of just pushing commits to your branch. @marcschumacher if you can review / merge, its good from my perspective.

[trask]

btw, I ran across java.net.Authenticator while looking at this, I don't recall if we ever considered this instead of our io.opentelemetry.exporter.internal.auth.Authenticator?

[jack-berg]

btw, I ran across java.net.Authenticator while looking at this

I ran across that too @trask. I don't think it was considered, but we should.

[jack-berg]

@open-telemetry/java-approvers In an effort to get this change merge for upcoming release, I've opened an alternative PR #6270.

Please take a look @marcschumacher.