Refactor JaegerRemoteSampler to leverage senders#8046
Refactor JaegerRemoteSampler to leverage senders#8046jack-berg wants to merge 1 commit intoopen-telemetry:mainfrom
Conversation
| public String toString() { | ||
| return toString(true); | ||
| } | ||
|
|
There was a problem hiding this comment.
Just moving this code to a more neutral SenderUtil class since its now being used outside GrpcExporterBuilder.
| import org.junit.jupiter.api.extension.RegisterExtension; | ||
| import org.junitpioneer.jupiter.SetSystemProperty; | ||
|
|
||
| class GrpcExporterTest { |
There was a problem hiding this comment.
Moved to SenderUtilTest
| return new byte[] {}; // TODO: drain input to byte array | ||
| public byte[] parse(InputStream inputStream) { | ||
| try { | ||
| int bufLen = 4 * 0x400; // 4KB |
There was a problem hiding this comment.
Lifted from the now-deleted MarshallerRemoteSamplerServiceGrpc.
| })); | ||
| } | ||
|
|
||
| private static byte[] getResponseMessageBytes(byte[] bodyBytes) throws IOException { |
There was a problem hiding this comment.
Lifted from the now-deleted OkHttpGrpcService
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8046 +/- ##
============================================
+ Coverage 90.17% 90.25% +0.08%
+ Complexity 7479 7452 -27
============================================
Files 834 832 -2
Lines 22559 22432 -127
Branches 2239 2222 -17
============================================
- Hits 20342 20246 -96
+ Misses 1515 1492 -23
+ Partials 702 694 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
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
OkHttpGrpcServiceandUpstreamGrpcServiceclasses with the publicGrpcSenderAPI - Converted
JaegerRemoteSamplerfrom synchronous to asynchronous callback-based communication - Extracted common sender resolution logic into a new
SenderUtilclass shared betweenGrpcExporterBuilderandHttpExporterBuilder
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.
| default: | ||
| logger.log( | ||
| Level.WARNING, | ||
| "Failed to export " |
There was a problem hiding this comment.
Inconsistent error message: "Failed to export" should be "Failed to execute" to match the UNIMPLEMENTED and UNAVAILABLE cases (lines 96 and 105) and maintain consistency with the previous implementation.
| "Failed to export " | |
| "Failed to execute " |
| private static void onError(Throwable e) { | ||
| logger.log( | ||
| Level.SEVERE, | ||
| "Failed to export " |
There was a problem hiding this comment.
Inconsistent error message: "Failed to export" should be "Failed to execute" to maintain consistency with the message patterns in the rest of the method and with the previous implementation.
| if (configuredSender.isEmpty()) { | ||
| LOGGER.log( | ||
| Level.WARNING, | ||
| OLD_GRPC_SPI_PROPERTY | ||
| + " was used to set GrpcSenderProvider. Please use " | ||
| + GRPC_SPI_PROPERTY | ||
| + " instead. " | ||
| + OLD_GRPC_SPI_PROPERTY | ||
| + " will be removed after 1.61.0"); | ||
| } |
There was a problem hiding this comment.
Logic error in warning condition: The warning message says "was used to set GrpcSenderProvider" but the condition is actually checking if configuredSender.isEmpty() after reading from OLD_GRPC_SPI_PROPERTY. This means the warning will be logged when the old property is NOT set, which is incorrect. The condition should be !configuredSender.isEmpty() to log the warning when the old property was actually used.
| if (configuredSender.isEmpty()) { | ||
| LOGGER.log( | ||
| Level.WARNING, | ||
| OLD_HTTP_SPI_PROPERTY | ||
| + " was used to set HttpSenderProvider. Please use " | ||
| + HTTP_SPI_PROPERTY | ||
| + " instead. " | ||
| + OLD_HTTP_SPI_PROPERTY | ||
| + " will be removed after 1.61.0"); | ||
| } |
There was a problem hiding this comment.
Logic error in warning condition: The warning message says "was used to set HttpSenderProvider" but the condition is actually checking if configuredSender.isEmpty() after reading from OLD_HTTP_SPI_PROPERTY. This means the warning will be logged when the old property is NOT set, which is incorrect. The condition should be !configuredSender.isEmpty() to log the warning when the old property was actually used.
| private void onResponse(GrpcResponse grpcResponse) { | ||
| GrpcStatusCode statusCode = grpcResponse.getStatusCode(); | ||
|
|
||
| if (statusCode == GrpcStatusCode.OK) { | ||
| try { | ||
| SamplingStrategyResponse strategyResponse = | ||
| SamplingStrategyResponseUnMarshaler.read(grpcResponse.getResponseMessage()); | ||
| sampler = updateSampler(strategyResponse); | ||
| } catch (IOException e) { | ||
| logger.log(Level.WARNING, "Failed to unmarshal strategy response", e); | ||
| } | ||
| } catch (Throwable e) { // keep the timer thread alive | ||
| logger.log(Level.WARNING, "Failed to update sampler", e); | ||
| return; | ||
| } | ||
|
|
||
| switch (statusCode) { | ||
| case UNIMPLEMENTED: | ||
| logger.log( | ||
| Level.SEVERE, | ||
| "Failed to execute " | ||
| + type | ||
| + "s. Server responded with UNIMPLEMENTED. " | ||
| + "Full error message: " | ||
| + grpcResponse.getStatusDescription()); | ||
| break; | ||
| case UNAVAILABLE: | ||
| logger.log( | ||
| Level.SEVERE, | ||
| "Failed to execute " | ||
| + type | ||
| + "s. Server is UNAVAILABLE. " | ||
| + "Make sure your service is running and reachable from this network. " | ||
| + "Full error message:" | ||
| + grpcResponse.getStatusDescription()); | ||
| break; | ||
| default: | ||
| logger.log( | ||
| Level.WARNING, | ||
| "Failed to export " | ||
| + type | ||
| + "s. Server responded with gRPC status code " | ||
| + statusCode.name() | ||
| + ". Error message: " | ||
| + grpcResponse.getStatusDescription()); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing exception handling in async callbacks: The previous synchronous implementation had a broad catch (Throwable e) block to prevent exceptions from terminating the scheduled polling thread. With the new asynchronous design, if an uncaught exception occurs in the onResponse callback (e.g., from updateSampler), it could propagate to the underlying HTTP client's thread pool and potentially cause issues. Consider wrapping the entire callback body in a try-catch block to match the previous behavior and ensure robustness.
| + type | ||
| + "s. Server is UNAVAILABLE. " | ||
| + "Make sure your service is running and reachable from this network. " | ||
| + "Full error message:" |
There was a problem hiding this comment.
Inconsistent spacing in error message: Line 109 is missing a space before the colon ("Full error message:" vs "Full error message: " on line 99). This inconsistency makes the error message formatting differ between error cases.
| + "Full error message:" | |
| + "Full error message: " |
| private static byte[] getResponseMessageBytes(byte[] bodyBytes) throws IOException { | ||
| if (bodyBytes.length > 5) { | ||
| ByteArrayInputStream bodyStream = new ByteArrayInputStream(bodyBytes); | ||
| bodyStream.skip(5); |
There was a problem hiding this comment.
Method getResponseMessageBytes ignores exceptional return value of ByteArrayInputStream.skip.
| bodyStream.skip(5); | |
| long skipped = bodyStream.skip(5); | |
| if (skipped != 5) { | |
| throw new IOException("Invalid response"); | |
| } |
1 participant
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-okhttpinto separate versions for okhttp 4.x and 5.x.