Interpret timeout zero value as no limit#7023
Conversation
| int callTimeoutMillis = | ||
| (int) Math.min(Duration.ofNanos(timeoutNanos).toMillis(), Integer.MAX_VALUE); | ||
| int connectTimeoutMillis = | ||
| (int) Math.min(Duration.ofNanos(connectTimeoutNanos).toMillis(), Integer.MAX_VALUE); |
There was a problem hiding this comment.
Internally, OkHttp converts durations in int type and time unit millis. Duration allows us to specify a value that overflows OkHttp's internal logic, so we need to add some bounds.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7023 +/- ##
============================================
- Coverage 89.95% 89.84% -0.11%
- Complexity 6636 6656 +20
============================================
Files 745 748 +3
Lines 20010 20087 +77
Branches 1962 1971 +9
============================================
+ Hits 17999 18048 +49
- Misses 1415 1444 +29
+ Partials 596 595 -1 ☔ View full report in Codecov by Sentry. |
breedx-splk
left a comment
breedx-splk
left a comment
There was a problem hiding this comment.
Looks good. I had a small question about negative values and maybe see an issue with MAX_VALUE for Integer in okhttp...but otherwise seems like a good change.
|
|
||
| public GrpcExporterBuilder<T> setTimeout(long timeout, TimeUnit unit) { | ||
| timeoutNanos = unit.toNanos(timeout); | ||
| timeoutNanos = timeout == 0 ? Long.MAX_VALUE : unit.toNanos(timeout); |
There was a problem hiding this comment.
Do we want to think about negative values while we're at it...or leave for a follow-up effort?
There was a problem hiding this comment.
Do we want to think about negative values while we're at it...or leave for a follow-up effort?
Negative values are rejected upstream of this, e.g.: https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/http/trace/OtlpHttpSpanExporterBuilder.java#L81
We discussed whether to centralize this type of parameter validation in a central place or duplicate it many times in upper layers, and decided on duplicating in upper layers. The idea being, that its much more discoverable for users to discover the requirements of the parameter if the validation is directly in the builder, vs. having to step through an obscure chain of abstractions to reach GrpcExporterBuilder.
We could of course have the checks at multiple layers, but that comes with its own problems of duplication and more edge cases to test if you take a strict view of coverage.
| .dispatcher(OkHttpUtil.newDispatcher()) | ||
| .connectTimeout(Duration.ofNanos(connectionTimeoutNanos)) | ||
| .callTimeout(Duration.ofNanos(timeoutNanos)); | ||
| .connectTimeout(Duration.ofMillis(connectTimeoutMillis)) |
There was a problem hiding this comment.
Same issue as above with MAX_VALUE causing ISE.
2 participants
Implementation of spec PR: open-telemetry/opentelemetry-specification#4331
Supersedes #6850.