fix: retry OTLP gRPC responses with trailer status - #8854
efegokdemir wants to merge 8 commits into
Conversation
Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
Pull request dashboard statusWaiting on the author · refreshed 2026-10-01 11:21 UTC Investigate required status check failures. Status above doesn't look right?
|
Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8854 +/- ##
============================================
- Coverage 91.44% 91.43% -0.01%
- Complexity 10667 10682 +15
============================================
Files 1007 1007
Lines 28686 28731 +45
Branches 3676 3685 +9
============================================
+ Hits 26231 26270 +39
- Misses 1657 1660 +3
- Partials 798 801 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| private static Response prepareResponseForRetry(Response response, long maxResponseBodySize) | ||
| throws IOException { | ||
| if (response.header(GRPC_STATUS) != null) { | ||
| return response; | ||
| } | ||
|
|
||
| ResponseBody body = response.body(); | ||
| Buffer buffer = new Buffer(); | ||
| long readUpTo = | ||
| maxResponseBodySize >= Long.MAX_VALUE - 5 ? Long.MAX_VALUE : maxResponseBodySize + 6; | ||
| while (buffer.size() < readUpTo) { | ||
| long read = body.source().read(buffer, readUpTo - buffer.size()); | ||
| if (read == -1L) { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| boolean responseBodyTooLarge = buffer.size() > maxResponseBodySize; | ||
| Headers trailers = responseBodyTooLarge ? Headers.of() : response.trailers(); | ||
| Buffer replacementBuffer = buffer; | ||
| ResponseBody replacementBody = | ||
| new ResponseBody() { | ||
| @Override | ||
| public long contentLength() { | ||
| return replacementBuffer.size(); | ||
| } | ||
|
|
||
| @Override | ||
| public MediaType contentType() { | ||
| return body.contentType(); | ||
| } | ||
|
|
||
| @Override | ||
| public BufferedSource source() { | ||
| return replacementBuffer; | ||
| } | ||
| }; | ||
| Response.Builder responseBuilder = response.newBuilder(); | ||
| response.close(); | ||
| responseBuilder.body(replacementBody); | ||
| String grpcStatus = trailers.get(GRPC_STATUS); | ||
| if (grpcStatus != null) { | ||
| responseBuilder.header(GRPC_STATUS, grpcStatus); | ||
| } | ||
| String grpcMessage = trailers.get(GRPC_MESSAGE); | ||
| if (grpcMessage != null) { | ||
| responseBuilder.header(GRPC_MESSAGE, grpcMessage); | ||
| } | ||
| return RetryUtil.retryableGrpcStatusCodes().contains(grpcStatus); | ||
| return responseBuilder.build(); | ||
| } |
There was a problem hiding this comment.
prepareResponseForRetry and handleResponse now both have to understand gRPC wire format: frame layout, the read-past-limit trick for size enforcement, and headers-vs-trailers resolution. I get why it landed this way:Response.trailers() isn't available until the body is consumed, so if retry is driven from an OkHttp interceptor the trailer-hoisting has to happen inside the interceptor. But that this bug forces us to duplicate wire-format handling in two places is a signal that retry belongs one layer out, driven from the resolved GrpcResponse in handleResponse, not from the raw HTTP Response.
JdkHttpSender already does retry inline in the sender, so there's precedent. The reusable bits of RetryInterceptor (backoff, jitter, exception predicate) can be extracted into a shared helper used by both OkHttpGrpcSender and OkHttpHttpSender, so we don't lose the code reuse.
There was a problem hiding this comment.
Implemented in a90f00d. Retry now runs after canonical gRPC decoding has produced a resolved GrpcResponse, so trailer-based retry no longer requires raw-response trailer hoisting or duplicated gRPC wire parsing. Backoff, jitter, exception policy, attempt limits, interruption, and HTTP retry behavior are shared through RetryState. The focused OkHttp tests, Spotless, module check, and git diff --check pass.
There was a problem hiding this comment.
Düzeltme: değişiklikler doğru yazar kimliğiyle yeniden push edildi. Güncel HEAD: a7ee760.
a90f00d to
a7ee760
Compare
Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
|
Fixed the patch-caused response-size regression in
Validation: |
|
I inspected the current required-status-check and failed matrix logs. The failures are broad across macOS, Ubuntu, and Windows and across JDK 8, 11, 17, 21, 25, and 27. The logs show unrelated SDK metrics stress-test timeouts (for example SynchronousInstrumentStressTest partialWriteStressTest) and the required-status-check itself is an intentional |
This branch has not been deployed
2 participants
Problem
OkHttpGrpcSender.isRetryableonly inspected thegrpc-statusresponse header. When a retryable status such asUNAVAILABLEwas delivered in HTTP/2 trailers, the exporter reported the status but did not retry.Change
Read
grpc-statusfrom trailers when it is absent from headers, matching the existing status-resolution path. An IOException while reading trailers remains non-retryable. Added a regression test and an Unreleased changelog entry.Fixes #8843
Validation
upstream/main: failed as expected (falseinstead oftrue).JAVA_HOME=/opt/homebrew/opt/openjdk@21 ./gradlew :exporters:sender:okhttp:test --tests io.opentelemetry.exporter.sender.okhttp.internal.OkHttpGrpcSenderTest: passed (49 tests).JAVA_HOME=/opt/homebrew/opt/openjdk@21 ./gradlew :exporters:sender:okhttp:spotlessCheck: passed.git diff --check: passed.:exporters:sender:okhttp:check: one unrelated existing timing-sensitive failure inshutdown_CompletableResultCodeShouldWaitForThreads; the changed regression and remaining tests passed.Provenance
Codex assisted with repository analysis, implementation, regression testing, and validation. The submitter reviewed the complete diff and is responsible for the contribution.