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

Add basic proxy configuration to OtlpHttp{Signal}Exporters #6270

Merged
merged 10 commits into from
Mar 7, 2024

Conversation

jack-berg
Copy link
Member

Resolves #6204. Related to #6194.

Alternative to #6206.

@jack-berg jack-berg requested a review from a team March 5, 2024 13:28

@Override
public String toString() {
return "ProxyOptions{proxySelector=" + proxySelector + "}";
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know that proxySelector has a proper toString() method?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will depend on the implementation, but the static inner SimpleProxySelector below does have a proper toString. We try to provide toString for all sdk components so that users can call OpentelemetrySdk#toString and understand the full config.

Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 87.09677% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 91.02%. Comparing base (62a4810) to head (4a4eb8c).
Report is 11 commits behind head on main.

Files Patch % Lines
.../opentelemetry/sdk/common/export/ProxyOptions.java 69.23% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6270      +/-   ##
============================================
- Coverage     91.06%   91.02%   -0.04%     
- Complexity     5699     5708       +9     
============================================
  Files           621      622       +1     
  Lines         16679    16709      +30     
  Branches       1709     1711       +2     
============================================
+ Hits          15188    15210      +22     
- Misses          998     1004       +6     
- Partials        493      495       +2     

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


@Override
public void connectFailed(URI uri, SocketAddress sa, IOException e) {
// ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it make sense to at least log out some debug logs here to indicate that there was a problem? I am not sure how common that is in the opentelemetry project to log such things though. I am not sure how verbose the client implementations are about proxy failures.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, the static inner one in java.net just says /* ignore */ as well. I wonder if the user is supposed to care....

Copy link
Member Author

Choose a reason for hiding this comment

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

The purpose of this method seems to be to allow the selector to choose another proxy for subsequent requests in the case of error, i.e. for load balancing.

Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

I like it. Had a couple small suggestions, but I think this proxy support will be nice for quite a few enterprise users who have forced proxies for regulatory compliance reasons.


@Override
public void connectFailed(URI uri, SocketAddress sa, IOException e) {
// ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, the static inner one in java.net just says /* ignore */ as well. I wonder if the user is supposed to care....

Comment on lines +681 to +687
assertThat(exporter.export(telemetry).join(10, TimeUnit.SECONDS).isSuccess()).isTrue();
// assert that mock server received request
assertThat(clientAndServer.retrieveRecordedRequests(new org.mockserver.model.HttpRequest()))
.hasSize(1);
// assert that server received telemetry from proxy, and is as expected
List<U> expectedResourceTelemetry = toProto(telemetry);
assertThat(exportedResourceTelemetry).containsExactlyElementsOf(expectedResourceTelemetry);
Copy link
Contributor

Choose a reason for hiding this comment

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

If the proxy was ignored completely in the implementation (eg. setProxyOptions() was a noop) would this test still pass? If so, is there a way to validate that the proxy was actually used, not just that the data made it across?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the proxy was ignored completely in the implementation (eg. setProxyOptions() was a noop) would this test still pass?

No the test fails. Line 683 retrieve requests recorded by the proxy and verifies it received the expected request. If the implementation fails to route through the configured proxy, the mockserver proxy will never receive the request and line 683 fails.

@jack-berg
Copy link
Member Author

@open-telemetry/java-approvers planning on merging this for Friday's release if there are no additional comments.

@marcschumacher
Copy link
Contributor

No further open comments from my side.

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.

Proxy support
3 participants