Skip to content

Add setEnabledProtocols option to OTLP exporter builders#8610

Open
jack-berg wants to merge 1 commit into
open-telemetry:mainfrom
jack-berg:otlp-enabled-protocols
Open

Add setEnabledProtocols option to OTLP exporter builders#8610
jack-berg wants to merge 1 commit into
open-telemetry:mainfrom
jack-berg:otlp-enabled-protocols

Conversation

@jack-berg

Copy link
Copy Markdown
Member

Fixes #7573

@otelbot otelbot Bot added the api-change Changes to public API surface area label Jul 16, 2026
@otelbot

otelbot Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

⚠️ API changes detected — additional maintainer review required

@jack-berg @jkwatson

This PR modifies the public API surface area of the following module(s):

  • opentelemetry-exporter-otlp
  • opentelemetry-sdk-common

Please review the changes in docs/apidiffs/current_vs_latest/ carefully before approving.

ssl.trustManager(clientCertificate.certificate());
// Restrict server to TLS 1.2+ so enabledProtocols tests are meaningful
// regardless of SSL backend (JDK TLS or BoringSSL via netty-tcnative).
ssl.protocols("TLSv1.2", "TLSv1.3");

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was happy to see a nearly identical API from armeria

@jack-berg
jack-berg marked this pull request as ready for review July 16, 2026 20:21
@jack-berg
jack-berg requested a review from a team as a code owner July 16, 2026 20:21
@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.20290% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.63%. Comparing base (c12e11b) to head (edafbee).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ry/exporter/sender/jdk/internal/JdkHttpSender.java 80.00% 0 Missing and 1 partial ⚠️
...orter/sender/okhttp/internal/OkHttpGrpcSender.java 87.50% 0 Missing and 1 partial ⚠️
...orter/sender/okhttp/internal/OkHttpHttpSender.java 90.00% 0 Missing and 1 partial ⚠️
...ntelemetry/sdk/common/export/HttpSenderConfig.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##               main    #8610   +/-   ##
=========================================
  Coverage     91.63%   91.63%           
- Complexity    10326    10350   +24     
=========================================
  Files          1013     1013           
  Lines         27295    27356   +61     
  Branches       3205     3217   +12     
=========================================
+ Hits          25011    25069   +58     
- Misses         1558     1559    +1     
- Partials        726      728    +2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Jul 16, 2026

Copy link
Copy Markdown

Pull request dashboard status

Status: Waiting on @jack-berg to address or respond to review feedback.

Top-level feedback waiting on the author:

Please give each review feedback item a clear outcome: link to the commit that addresses it, explain why no change is needed, or ask a follow-up question.

@adhamahmad

Copy link
Copy Markdown

Hey @jack-berg, thanks for the detailed analysis.

I had one question about test coverage: is there a test that verifies the actual motivating scenario from #7573 — successfully reaching a real TLSv1/TLSv1.1-only server with enabledProtocols set and the JVM floor opened?

As written, enabledProtocols() only exercises TLS 1.2/1.3 against a TLS 1.2/1.3 server, which would still pass if setEnabledProtocols() were a no-op since that is already the default range.
I don't see a test covering the full "both gates open" scenario against a legacy server, which was the original bug report.

I ran into a related issue testing this in my PR: jdk.tls.disabledAlgorithms behavior is JVM-wide, and changing it at runtime can be problematic once TLS initialization has already happened. That made a legacy-server test order-dependent in practice.
If that is why this scenario was intentionally omitted, that makes sense as a stability tradeoff.

I just wanted to confirm whether that was the reasoning, or if I missed an existing test covering it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-change Changes to public API surface area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OkHttpHttpSender should support COMPATIBLE_TLS connectionSpec

2 participants