Respect Retry-After in OTLP HTTP senders#8633
Conversation
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8633 +/- ##
============================================
+ Coverage 91.60% 91.64% +0.03%
- Complexity 10343 10365 +22
============================================
Files 1013 1013
Lines 27352 27424 +72
Branches 3215 3222 +7
============================================
+ Hits 25057 25133 +76
+ Misses 1567 1559 -8
- Partials 728 732 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1249450 to
675f3c9
Compare
Pull request dashboard statusStatus last refreshed: 2026-07-25 03:14:14 UTC.
This automated status or its linked feedback items may be incorrect. If something looks wrong, please report it with the result you expected. If you believe this pull request is incorrectly routed as waiting on the author, comment |
| retryDelayNanos = OptionalLong.empty(); | ||
| try { | ||
| TimeUnit.NANOSECONDS.sleep(backoffNanos); | ||
| sleeper.sleep(backoffNanos); |
There was a problem hiding this comment.
We can't blindly sleep for whatever time the server tells us. We need to clamp this to something like Math.min(backoffNanos, timeout-elapsed)
| } | ||
|
|
||
| /** Returns whether the status code represents OTLP/HTTP throttling. */ | ||
| public static boolean isOtlpHttpThrottleStatusCode(int statusCode) { |
There was a problem hiding this comment.
I think this gates respecting Retry-After to only 429/503 status codes.
My read of the relevant section of the spec is that:
- Servers should use 429 or 503 when overloaded, and may include Retry-After
- Clients should honor Retry-After, so long as its a retryable status code.
| * Returns the delay specified by a {@code Retry-After} header, or empty if the value is absent or | ||
| * malformed. | ||
| */ | ||
| public static OptionalLong retryAfterNanos(@Nullable String retryAfter) { |
There was a problem hiding this comment.
Unit test the edge cases of this, and keep the integration tests a higher level validation that Retry-After is being honored generally
| } | ||
|
|
||
| @Test | ||
| void retryableThrottledError_retryAfterSeconds() { |
There was a problem hiding this comment.
Can we get away only with the coverage here and get rid of the JdkHttpSenderTest coverage?
Also, as mentioned in the other comment, if you unit test the retryAfterNanos method, you can reduce the coverage here to something more basic. I.e. the one positive case here + the malformedRetryAfterFallsBack negative case. Can delete retryAfterDate case since that would already be covered.
| } | ||
|
|
||
| /** Constructs a new retrier with an optional server-provided retry delay override. */ | ||
| public RetryInterceptor( |
There was a problem hiding this comment.
Too many overloads for an internal class. Let's reduce to the just one or two.
| } | ||
|
|
||
| // Visible for testing | ||
| JdkHttpSender( |
There was a problem hiding this comment.
Let's get rid of this extra overload.
|
|
||
| try { | ||
| Instant retryAfterInstant = | ||
| ZonedDateTime.parse(retryAfter, DateTimeFormatter.RFC_1123_DATE_TIME).toInstant(); |
There was a problem hiding this comment.
Can you evalutate what other otel languages are doing here and confirm we match?
2 participants
Summary:
Fixes #8449
Testing: