Refactor JaegerRemoteSampler to leverage senders by jack-berg · Pull Request #8046 · open-telemetry/opentelemetry-java

jack-berg · 2026-02-04T22:57:45Z

Contributes to #8001 by refactoring JaegerRemoteSampler to be built on top of the newly public sender APIs.

This simplifies the code, and allows JaegerRemoteSampler to be able to automatically take advantage of breaking out opentelemetry-exporter-sender-okhttp into separate versions for okhttp 4.x and 5.x.

jack-berg · 2026-02-04T22:58:26Z

exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporterBuilder.java

  public String toString() {
    return toString(true);
  }
-


Just moving this code to a more neutral SenderUtil class since its now being used outside GrpcExporterBuilder.

jack-berg · 2026-02-04T22:58:48Z

...rc/testGrpcSenderProvider/java/io/opentelemetry/exporter/internal/grpc/GrpcExporterTest.java

-import org.junit.jupiter.api.extension.RegisterExtension;
-import org.junitpioneer.jupiter.SetSystemProperty;
-
-class GrpcExporterTest {


Moved to SenderUtilTest

jack-berg · 2026-02-04T22:59:48Z

...n/java/io/opentelemetry/exporter/sender/grpc/managedchannel/internal/UpstreamGrpcSender.java

-          return new byte[] {}; // TODO: drain input to byte array
+        public byte[] parse(InputStream inputStream) {
+          try {
+            int bufLen = 4 * 0x400; // 4KB


Lifted from the now-deleted MarshallerRemoteSamplerServiceGrpc.

jack-berg · 2026-02-04T23:00:27Z

.../okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpGrpcSender.java

                    }));
  }

+  private static byte[] getResponseMessageBytes(byte[] bodyBytes) throws IOException {


Lifted from the now-deleted OkHttpGrpcService

codecov · 2026-02-04T23:07:51Z

Codecov Report

❌ Patch coverage is 89.84375% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (fff95e0) to head (3eeba43).
⚠️ Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
...pc/managedchannel/internal/UpstreamGrpcSender.java	44.44%	9 Missing and 1 partial ⚠️
...sion/trace/jaeger/sampler/JaegerRemoteSampler.java	90.62%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8046      +/-   ##
============================================
+ Coverage     90.20%   90.31%   +0.11%     
+ Complexity     7592     7584       -8     
============================================
  Files           841      839       -2     
  Lines         22911    22808     -103     
  Branches       2288     2274      -14     
============================================
- Hits          20666    20600      -66     
+ Misses         1529     1500      -29     
+ Partials        716      708       -8

☔ View full report in Codecov by Sentry.
📢 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.

Copilot

Pull request overview

This pull request refactors JaegerRemoteSampler to use the public sender APIs (GrpcSender and GrpcSenderProvider) instead of maintaining its own internal HTTP client implementations. This change addresses issue #8001 regarding OkHttp compatibility, allowing the sampler to automatically benefit from separate versions of opentelemetry-exporter-sender-okhttp for OkHttp 4.x and 5.x.

Changes:

Replaced internal OkHttpGrpcService and UpstreamGrpcService classes with the public GrpcSender API
Converted JaegerRemoteSampler from synchronous to asynchronous callback-based communication
Extracted common sender resolution logic into a new SenderUtil class shared between GrpcExporterBuilder and HttpExporterBuilder

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`JaegerRemoteSampler.java`	Refactored to use `GrpcSender` with async callbacks instead of synchronous `GrpcService` execution
`JaegerRemoteSamplerBuilder.java`	Updated to resolve and create `GrpcSender` using `GrpcSenderProvider` instead of building custom HTTP clients
`OkHttpGrpcService.java`	Deleted - functionality moved to `OkHttpGrpcSender`
`UpstreamGrpcService.java`	Deleted - functionality exists in `UpstreamGrpcSender`
`GrpcService.java`	Deleted - replaced by public `GrpcSender` interface
`MarshallerRemoteSamplerServiceGrpc.java`	Deleted - no longer needed with sender abstraction
`SamplingStrategyResponseUnMarshaler.java`	Simplified to static `read()` method, removing stateful instance pattern
`OkHttpGrpcSender.java`	Added response message extraction logic from deleted `OkHttpGrpcService`
`UpstreamGrpcSender.java`	Added input stream parsing implementation for response messages
`SenderUtil.java`	New utility class consolidating sender provider resolution logic
`GrpcExporterBuilder.java`	Refactored to delegate sender resolution to `SenderUtil`
`HttpExporterBuilder.java`	Refactored to delegate sender resolution to `SenderUtil`
`ManagedChannelUtil.java`	Removed `shutdownChannel()` method (now handled by senders)
`ImmutableGrpcSenderConfig.java`	Made public to support external usage by `JaegerRemoteSamplerBuilder`
`build.gradle.kts` (jaeger-remote-sampler)	Removed direct OkHttp dependency, added test configuration for GrpcSenderProvider
`build.gradle.kts` (exporters/common)	Reorganized test suites to consolidate sender provider tests
Test files	Updated to reflect field name changes (`delegate` → `grpcSender`) and use `GrpcStatusCode` enum

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

...r/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSampler.java

exporters/common/src/main/java/io/opentelemetry/exporter/internal/SenderUtil.java

...r/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSampler.java

.../okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpGrpcSender.java

…y-java into refactor-jaeger-remote-sampler

trask

nice

exporters/common/src/main/java/io/opentelemetry/exporter/internal/SenderUtil.java

...r/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSampler.java

trask · 2026-02-15T20:13:36Z

...r/src/main/java/io/opentelemetry/sdk/extension/trace/jaeger/sampler/JaegerRemoteSampler.java

+    if (logger.isLoggable(Level.FINEST)) {
+      logger.log(Level.FINEST, "Failed to execute " + type + "s. Details follow: " + e);
+    }


(or did you want to not log the stack trace above at SEVERE?)

Suggested change

if (logger.isLoggable(Level.FINEST)) {

logger.log(Level.FINEST, "Failed to execute " + type + "s. Details follow: " + e);

}

Copy / pasted from GrpcExporter, which has a similar, but not identical, harness / wrapper around GrpcSender.

Tracked the lineage of where this additional finest level logging came from and its ancient: https://github.com/open-telemetry/opentelemetry-java/pull/1803/changes#diff-b81bcc3e91a1ea9ee5f99b101f968b51a9396151014479db2f00e419394b573fR149

But it does seem duplicative. At the SEVERE level, the exception is included, and thus can be printed if the user has configured their logging framework to do so. At the FINEST level, the stacktrace is printed as part of the log message regardless of logging framework config.

Tracking here: #8098

.../okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpGrpcSender.java

trask · 2026-02-15T20:49:55Z

.../okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpGrpcSender.java

+    if (bodyBytes.length > 5) {
+      ByteArrayInputStream bodyStream = new ByteArrayInputStream(bodyBytes);
+      bodyStream.skip(5);
+      if (bodyBytes[0] == '1') {


(at least this is what AI tells me, and it seems believable, probably we need a compressed response test)

Suggested change

if (bodyBytes[0] == '1') {

if (bodyBytes[0] == 1) {

Yes - tracked down the relevant spec: https://github.com/grpc/grpc/blame/master/doc/PROTOCOL-HTTP2.md#L96

I'll note that this is an example of existing code that was just shuffled around, but yes we do need a compressed response test.

This indeed was a problem and I wrote a failing test specifically for JaegerRemoteSampler to verify in 3eeba43. It slipped under the radar for OTLP so far because we don't do anything with the responses. Can update AbstractGrpcTelemetryExporterTest to include tests once we actually deserialize responses via #4706.

.../okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpGrpcSender.java

zeitlinger

Nice cleanup — using the sender APIs removes a lot of duplicated HTTP client logic. One minor question below. (Also +1 to Copilot's comment about wrapping onResponse in a try-catch.)

zeitlinger · 2026-02-20T11:57:45Z

Minor question on JaegerRemoteSamplerBuilder.java line 113 — the // TODO(jack-berg): Make public on setHeaders():

Is there a plan for when to make this public? If it's needed for a known use case, might be worth doing in this PR to avoid shipping package-private API that callers can't use. If it's speculative, consider dropping it and adding when there's demand — avoids the dangling TODO.

jack-berg · 2026-02-20T14:39:46Z

Add the setHeaders() package private API to facilitate testing gzip resposne handling: https://github.com/open-telemetry/opentelemetry-java/pull/8046/changes#diff-70f38e17b79d7b258d48ea3fd0c7c4a86187766411401fc211ec442c173be753R246

There was an existing bug that in the response decoding causing this to not work. Shuffling around the code brought attention to it and didn't feel right to leave it broken: #8046 (comment)

setHeaders() is both speculative and known to be useful. Its speculative in the sense that nobody has formally asked for it. Its known to be useful because I wrote a test dependent on it which causes responses to be compressed, which is a reasonable ask. Also, it can be used for authentication purposes.

I'm happy to promote it to the public API because its consistent with setters we maintain in other network-interacting components like the OTLP exporters. But it wouldn't be right to extend a refactor PR to add new public API surface area. So left it for a followup.

Refactor JaegerRemoteSampler to leverage senders

a5f73ba

jack-berg requested a review from a team as a code owner February 4, 2026 22:57

jack-berg commented Feb 4, 2026

View reviewed changes

jack-berg requested a review from Copilot February 6, 2026 14:11

Copilot started reviewing on behalf of jack-berg February 6, 2026 14:12 View session

Copilot AI reviewed Feb 6, 2026

View reviewed changes

jack-berg added 2 commits February 11, 2026 09:21

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

e360ddf

…y-java into refactor-jaeger-remote-sampler

PR feedback

e876071

jack-berg mentioned this pull request Feb 13, 2026

Split out okhttp4 sender #8086

Open

trask reviewed Feb 15, 2026

View reviewed changes

PR feedback

e772b84

trask approved these changes Feb 17, 2026

View reviewed changes

jack-berg mentioned this pull request Feb 17, 2026

Do we need to log OTLP errors at SEVERE and FINEST? #8098

Open

Fix decompression

3eeba43

jack-berg merged commit d469a9c into open-telemetry:main Feb 17, 2026
27 checks passed

zeitlinger approved these changes Feb 20, 2026

View reviewed changes

	if (logger.isLoggable(Level.FINEST)) {
	logger.log(Level.FINEST, "Failed to execute " + type + "s. Details follow: " + e);
	}

Comments

Conversation

jack-berg commented Feb 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trask left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zeitlinger left a comment

Choose a reason for hiding this comment

Uh oh!

zeitlinger commented Feb 20, 2026

Uh oh!

jack-berg commented Feb 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Feb 4, 2026 •

edited

Loading