Skip to content

Add setEnabledProtocols option to OTLP exporter builders#8610

Open
jack-berg wants to merge 3 commits into
open-telemetry:mainfrom
jack-berg:otlp-enabled-protocols
Open

Add setEnabledProtocols option to OTLP exporter builders#8610
jack-berg wants to merge 3 commits 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.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.64%. Comparing base (09d6c17) to head (dff3e33).

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 93.75% 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.64%   91.64%           
- Complexity    10348    10372   +24     
=========================================
  Files          1013     1013           
  Lines         27380    27441   +61     
  Branches       3218     3230   +12     
=========================================
+ Hits          25092    25149   +57     
- Misses         1558     1559    +1     
- Partials        730      733    +3     

☔ 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 last refreshed: 2026-07-26 18:31:03 UTC.

  • Waiting on: Reviewers
  • Next step: Review the latest changes.

This automated status or its linked feedback items may be incorrect. If something looks wrong, please report it with the result you expected.

@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.

@jack-berg

Copy link
Copy Markdown
Member Author

Hi @adhamahmad -

You make a good point. I updated the abstract http / grpc test classes to verify that when you restrict protocols to TLSv1.1, export fails. This ensures that the senders are indeed restricting based on protocols. But it doesn't demonstrate the actual motivating scenario from the issue. After reading your comment, I did try to update the abstract tests to recreate that situation but it proved impractical to configure the armeria test server used in those to downgrade to TLS 1.1, such that you could configure a client to use TLS 1.1 and get a successful export.

I ended up added a dedicated simpler scoped test to OkHttpHttpSenderTest in 9394db0 which recreates a server accepting only TLS 1.1 at a lower level. It looks somewhat similar to the test in your PR.

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