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

Add new TLS related APIs on OTLP exporter builders. #5280

Merged
merged 20 commits into from
May 4, 2023
Merged

Add new TLS related APIs on OTLP exporter builders. #5280

merged 20 commits into from
May 4, 2023

Conversation

breedx-splk
Copy link
Contributor

Relates to #5211.

This is a follow-up from #5246 and provides new methods for configuring TLS on the OTLP exporters in a more DIY or BYO fashion. This allows users to preconfigure their own existing X509TrustManager, X509KeyManager, and/or SSLSocketFactory without necessarily having to manually deal with certificate bytes (existing APIs).

@breedx-splk breedx-splk requested a review from a team as a code owner March 9, 2023 23:40
@breedx-splk breedx-splk changed the title Add new TLS related APIs on OTLP exporters. Add new TLS related APIs on OTLP exporter builders. Mar 9, 2023
@breedx-splk
Copy link
Contributor Author

Ok, I still don't love having two ways of providing a trust manager (cert bytes, or instance along with the SSLSocketFactory), and I think there's probably room to improve guidance to the user....but I made a change so that it's just one new method now per builder. If we treat this as the pure BYO approach and the other pair as the previous, then I think that's what @jack-berg was asking for.

@breedx-splk breedx-splk reopened this Mar 14, 2023
@codecov
Copy link

codecov bot commented Mar 14, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (106941f) 91.30% compared to head (a566f5d) 91.32%.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #5280      +/-   ##
============================================
+ Coverage     91.30%   91.32%   +0.02%     
- Complexity     4879     4887       +8     
============================================
  Files           549      549              
  Lines         14379    14393      +14     
  Branches       1354     1354              
============================================
+ Hits          13129    13145      +16     
+ Misses          863      861       -2     
  Partials        387      387
Impacted Files Coverage Δ
...ry/exporter/internal/grpc/GrpcExporterBuilder.java 98.71% <100.00%> (+0.03%) ⬆️
...lp/http/metrics/OtlpHttpMetricExporterBuilder.java 95.23% <100.00%> (+0.23%) ⬆️
...r/otlp/http/trace/OtlpHttpSpanExporterBuilder.java 90.00% <100.00%> (+0.71%) ⬆️
...er/otlp/metrics/OtlpGrpcMetricExporterBuilder.java 100.00% <100.00%> (ø)
...porter/otlp/trace/OtlpGrpcSpanExporterBuilder.java 91.66% <100.00%> (+0.49%) ⬆️
...lp/http/logs/OtlpHttpLogRecordExporterBuilder.java 90.00% <100.00%> (+0.71%) ⬆️
...er/otlp/logs/OtlpGrpcLogRecordExporterBuilder.java 91.66% <100.00%> (+0.49%) ⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

jack-berg
jack-berg reviewed Mar 22, 2023
View reviewed changes
Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Couple of small comments but I'm aligned with this.

@chicobento curious if this satisfies your use case in #5211.

exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporterBuilder.java Show resolved Hide resolved
...all/src/main/java/io/opentelemetry/exporter/otlp/http/trace/OtlpHttpSpanExporterBuilder.java Outdated Show resolved Hide resolved
...all/src/main/java/io/opentelemetry/exporter/otlp/http/trace/OtlpHttpSpanExporterBuilder.java Outdated Show resolved Hide resolved
chicobento
chicobento reviewed Mar 24, 2023
View reviewed changes
...all/src/main/java/io/opentelemetry/exporter/otlp/http/trace/OtlpHttpSpanExporterBuilder.java Outdated Show resolved Hide resolved
...all/src/main/java/io/opentelemetry/exporter/otlp/http/trace/OtlpHttpSpanExporterBuilder.java
@@ -15,6 +15,8 @@
import io.opentelemetry.exporter.internal.otlp.traces.TraceRequestMarshaler;
import java.time.Duration;
import java.util.concurrent.TimeUnit;
import javax.net.ssl.SSLSocketFactory;
import javax.net.ssl.X509TrustManager;

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Would like to see same changes applied to jaeger-related classes such as JaegerRemoteSamplerBuilder and JaegerGrpcSpanExporterBuilder.

Copy link
Member

Choose a reason for hiding this comment

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

Let's do that in a separate PR.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2023

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Apr 7, 2023
@jack-berg jack-berg mentioned this pull request Apr 8, 2023
Merged
@jack-berg
Copy link
Member

Now that #5362 is merged, we should be able to rebase this and expose the new setSslContext(SSLContext, X509TrustManager) API to the builders.

@github-actions github-actions bot removed the Stale label Apr 14, 2023
@jack-berg
Copy link
Member

@jkwatson can you take a look? Would be good to get this in for the May release.

jkwatson
jkwatson approved these changes May 2, 2023
View reviewed changes
Copy link
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

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

Thanks!

@jack-berg jack-berg merged commit 5f9dc3f into open-telemetry:main May 4, 2023
@jack-berg jack-berg mentioned this pull request May 4, 2023
Closed
@chicobento chicobento mentioned this pull request May 4, 2023
Merged
@jack-berg jack-berg mentioned this pull request May 31, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chicobento chicobento chicobento left review comments

@jack-berg jack-berg jack-berg approved these changes

@jkwatson jkwatson jkwatson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@breedx-splk @jack-berg @jkwatson @chicobento