Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interpret timeout zero value as no limit #7023

Merged
merged 2 commits into from
Jan 17, 2025
+131 −26
Merged

Interpret timeout zero value as no limit #7023

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea48068
Interpret timeout zero value as no limit
jack-berg Jan 15, 2025
d386c51
Add unit tests
jack-berg Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...ers/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporterBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public GrpcExporterBuilder<T> setChannel(ManagedChannel channel) {
}

public GrpcExporterBuilder<T> setTimeout(long timeout, TimeUnit unit) {
timeoutNanos = unit.toNanos(timeout);
timeoutNanos = timeout == 0 ? Long.MAX_VALUE : unit.toNanos(timeout);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to think about negative values while we're at it...or leave for a follow-up effort?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return this;
}

Expand All @@ -96,7 +96,7 @@ public GrpcExporterBuilder<T> setTimeout(Duration timeout) {
}

public GrpcExporterBuilder<T> setConnectTimeout(long timeout, TimeUnit unit) {
connectTimeoutNanos = unit.toNanos(timeout);
connectTimeoutNanos = timeout == 0 ? Long.MAX_VALUE : unit.toNanos(timeout);
return this;
}

Expand Down
4 changes: 2 additions & 2 deletions ...ers/common/src/main/java/io/opentelemetry/exporter/internal/http/HttpExporterBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,12 @@ public HttpExporterBuilder(String exporterName, String type, String defaultEndpo
}

public HttpExporterBuilder<T> setTimeout(long timeout, TimeUnit unit) {
timeoutNanos = unit.toNanos(timeout);
timeoutNanos = timeout == 0 ? Long.MAX_VALUE : unit.toNanos(timeout);
return this;
}

public HttpExporterBuilder<T> setConnectTimeout(long timeout, TimeUnit unit) {
connectTimeoutNanos = unit.toNanos(timeout);
connectTimeoutNanos = timeout == 0 ? Long.MAX_VALUE : unit.toNanos(timeout);
return this;
}

Expand Down
28 changes: 24 additions & 4 deletions ...va/io/opentelemetry/exporter/otlp/testing/internal/AbstractGrpcTelemetryExporterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -815,13 +815,28 @@ void overrideHost() {
@Test
@SuppressWarnings("PreferJavaTimeOverload")
void validConfig() {
assertThatCode(() -> exporterBuilder().setTimeout(0, TimeUnit.MILLISECONDS))
// We must build exporters to test timeout settings, which intersect with underlying client
// implementations and may convert between Duration, int, and long, which may be susceptible to
// overflow exceptions.
assertThatCode(() -> buildAndShutdown(exporterBuilder().setTimeout(0, TimeUnit.MILLISECONDS)))
.doesNotThrowAnyException();
assertThatCode(() -> exporterBuilder().setTimeout(Duration.ofMillis(0)))
assertThatCode(() -> buildAndShutdown(exporterBuilder().setTimeout(Duration.ofMillis(0))))
.doesNotThrowAnyException();
assertThatCode(() -> exporterBuilder().setTimeout(10, TimeUnit.MILLISECONDS))
assertThatCode(() -> buildAndShutdown(exporterBuilder().setTimeout(10, TimeUnit.MILLISECONDS)))
.doesNotThrowAnyException();
assertThatCode(() -> exporterBuilder().setTimeout(Duration.ofMillis(10)))
assertThatCode(() -> buildAndShutdown(exporterBuilder().setTimeout(Duration.ofMillis(10))))
.doesNotThrowAnyException();
assertThatCode(
() -> buildAndShutdown(exporterBuilder().setConnectTimeout(0, TimeUnit.MILLISECONDS)))
.doesNotThrowAnyException();
assertThatCode(
() -> buildAndShutdown(exporterBuilder().setConnectTimeout(Duration.ofMillis(0))))
.doesNotThrowAnyException();
assertThatCode(
() -> buildAndShutdown(exporterBuilder().setConnectTimeout(10, TimeUnit.MILLISECONDS)))
.doesNotThrowAnyException();
assertThatCode(
() -> buildAndShutdown(exporterBuilder().setConnectTimeout(Duration.ofMillis(10))))
.doesNotThrowAnyException();

assertThatCode(() -> exporterBuilder().setEndpoint("http://localhost:4317"))
Expand All @@ -846,6 +861,11 @@ void validConfig() {
.doesNotThrowAnyException();
}

private void buildAndShutdown(TelemetryExporterBuilder<T> builder) {
TelemetryExporter<T> build = builder.build();
build.shutdown().join(10, TimeUnit.MILLISECONDS);
}

@Test
@SuppressWarnings({"PreferJavaTimeOverload", "NullAway"})
void invalidConfig() {
Expand Down
29 changes: 20 additions & 9 deletions ...va/io/opentelemetry/exporter/otlp/testing/internal/AbstractHttpTelemetryExporterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,22 +698,28 @@ void proxy() {
@Test
@SuppressWarnings("PreferJavaTimeOverload")
void validConfig() {
assertThatCode(() -> exporterBuilder().setTimeout(0, TimeUnit.MILLISECONDS))
// We must build exporters to test timeout settings, which intersect with underlying client
// implementations and may convert between Duration, int, and long, which may be susceptible to
// overflow exceptions.
assertThatCode(() -> buildAndShutdown(exporterBuilder().setTimeout(0, TimeUnit.MILLISECONDS)))
.doesNotThrowAnyException();
assertThatCode(() -> exporterBuilder().setTimeout(Duration.ofMillis(0)))
assertThatCode(() -> buildAndShutdown(exporterBuilder().setTimeout(Duration.ofMillis(0))))
.doesNotThrowAnyException();
assertThatCode(() -> exporterBuilder().setTimeout(10, TimeUnit.MILLISECONDS))
assertThatCode(() -> buildAndShutdown(exporterBuilder().setTimeout(10, TimeUnit.MILLISECONDS)))
.doesNotThrowAnyException();
assertThatCode(() -> exporterBuilder().setTimeout(Duration.ofMillis(10)))
assertThatCode(() -> buildAndShutdown(exporterBuilder().setTimeout(Duration.ofMillis(10))))
.doesNotThrowAnyException();

assertThatCode(() -> exporterBuilder().setConnectTimeout(0, TimeUnit.MILLISECONDS))
assertThatCode(
() -> buildAndShutdown(exporterBuilder().setConnectTimeout(0, TimeUnit.MILLISECONDS)))
.doesNotThrowAnyException();
assertThatCode(() -> exporterBuilder().setConnectTimeout(Duration.ofMillis(0)))
assertThatCode(
() -> buildAndShutdown(exporterBuilder().setConnectTimeout(Duration.ofMillis(0))))
.doesNotThrowAnyException();
assertThatCode(() -> exporterBuilder().setConnectTimeout(10, TimeUnit.MILLISECONDS))
assertThatCode(
() -> buildAndShutdown(exporterBuilder().setConnectTimeout(10, TimeUnit.MILLISECONDS)))
.doesNotThrowAnyException();
assertThatCode(() -> exporterBuilder().setConnectTimeout(Duration.ofMillis(10)))
assertThatCode(
() -> buildAndShutdown(exporterBuilder().setConnectTimeout(Duration.ofMillis(10))))
.doesNotThrowAnyException();

assertThatCode(() -> exporterBuilder().setEndpoint("http://localhost:4318"))
Expand All @@ -738,6 +744,11 @@ void validConfig() {
.doesNotThrowAnyException();
}

private void buildAndShutdown(TelemetryExporterBuilder<T> builder) {
TelemetryExporter<T> build = builder.build();
build.shutdown().join(10, TimeUnit.MILLISECONDS);
}

@Test
@SuppressWarnings({"PreferJavaTimeOverload", "NullAway"})
void invalidConfig() {
Expand Down
8 changes: 6 additions & 2 deletions ...http/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpGrpcSender.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,15 @@ public OkHttpGrpcSender(
@Nullable RetryPolicy retryPolicy,
@Nullable SSLContext sslContext,
@Nullable X509TrustManager trustManager) {
int callTimeoutMillis =
(int) Math.min(Duration.ofNanos(timeoutNanos).toMillis(), Integer.MAX_VALUE);
int connectTimeoutMillis =
(int) Math.min(Duration.ofNanos(connectTimeoutNanos).toMillis(), Integer.MAX_VALUE);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

OkHttpClient.Builder clientBuilder =
new OkHttpClient.Builder()
.dispatcher(OkHttpUtil.newDispatcher())
.callTimeout(Duration.ofNanos(timeoutNanos))
.connectTimeout(Duration.ofNanos(connectTimeoutNanos));
.callTimeout(Duration.ofMillis(callTimeoutMillis))
.connectTimeout(Duration.ofMillis(connectTimeoutMillis));
breedx-splk marked this conversation as resolved.
Show resolved Hide resolved
if (retryPolicy != null) {
clientBuilder.addInterceptor(
new RetryInterceptor(retryPolicy, OkHttpGrpcSender::isRetryable));
Expand Down
8 changes: 6 additions & 2 deletions ...http/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpHttpSender.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,15 @@ public OkHttpHttpSender(
@Nullable RetryPolicy retryPolicy,
@Nullable SSLContext sslContext,
@Nullable X509TrustManager trustManager) {
int callTimeoutMillis =
(int) Math.min(Duration.ofNanos(timeoutNanos).toMillis(), Integer.MAX_VALUE);
int connectTimeoutMillis =
(int) Math.min(Duration.ofNanos(connectionTimeoutNanos).toMillis(), Integer.MAX_VALUE);
OkHttpClient.Builder builder =
new OkHttpClient.Builder()
.dispatcher(OkHttpUtil.newDispatcher())
.connectTimeout(Duration.ofNanos(connectionTimeoutNanos))
.callTimeout(Duration.ofNanos(timeoutNanos));
.connectTimeout(Duration.ofMillis(connectTimeoutMillis))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as above with MAX_VALUE causing ISE.

.callTimeout(Duration.ofMillis(callTimeoutMillis));

if (proxyOptions != null) {
builder.proxySelector(proxyOptions.getProxySelector());
Expand Down
7 changes: 4 additions & 3 deletions ...ters/zipkin/src/main/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public final class ZipkinSpanExporterBuilder {
// compression is enabled by default, because this is the default of OkHttpSender,
// which is created when no custom sender is set (see OkHttpSender.Builder)
private boolean compressionEnabled = true;
private long readTimeoutMillis = TimeUnit.SECONDS.toMillis(10);
private int readTimeoutMillis = (int) TimeUnit.SECONDS.toMillis(10);
private Supplier<MeterProvider> meterProviderSupplier = GlobalOpenTelemetry::getMeterProvider;

/**
Expand Down Expand Up @@ -156,7 +156,8 @@ public ZipkinSpanExporterBuilder setCompression(String compressionMethod) {
public ZipkinSpanExporterBuilder setReadTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
this.readTimeoutMillis = unit.toMillis(timeout);
long timeoutMillis = timeout == 0 ? Long.MAX_VALUE : unit.toMillis(timeout);
this.readTimeoutMillis = (int) Math.min(timeoutMillis, Integer.MAX_VALUE);
return this;
}

Expand Down Expand Up @@ -212,7 +213,7 @@ public ZipkinSpanExporter build() {
OkHttpSender.newBuilder()
.endpoint(endpoint)
.compressionEnabled(compressionEnabled)
.readTimeout((int) readTimeoutMillis)
.readTimeout(readTimeoutMillis)
.build();
}
OtelToZipkinSpanTransformer transformer =
Expand Down
15 changes: 15 additions & 0 deletions exporters/zipkin/src/test/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,21 @@ void compressionEnabledAndDisabled() {
}
}

@Test
@SuppressWarnings("PreferJavaTimeOverload")
void readTimeout_Zero() {
ZipkinSpanExporter exporter =
ZipkinSpanExporter.builder().setReadTimeout(0, TimeUnit.SECONDS).build();

try {
assertThat(exporter)
.extracting("sender.delegate.client.readTimeoutMillis")
.isEqualTo(Integer.MAX_VALUE);
} finally {
exporter.shutdown();
}
}

@Test
void stringRepresentation() {
try (ZipkinSpanExporter exporter = ZipkinSpanExporter.builder().build()) {
Expand Down
2 changes: 1 addition & 1 deletion sdk/logs/src/main/java/io/opentelemetry/sdk/logs/export/BatchLogRecordProcessorBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ long getScheduleDelayNanos() {
public BatchLogRecordProcessorBuilder setExporterTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
exporterTimeoutNanos = unit.toNanos(timeout);
exporterTimeoutNanos = timeout == 0 ? Long.MAX_VALUE : unit.toNanos(timeout);
return this;
}

Expand Down
2 changes: 1 addition & 1 deletion sdk/trace/src/main/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessorBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ long getScheduleDelayNanos() {
public BatchSpanProcessorBuilder setExporterTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
exporterTimeoutNanos = unit.toNanos(timeout);
exporterTimeoutNanos = timeout == 0 ? Long.MAX_VALUE : unit.toNanos(timeout);
return this;
}

Expand Down
Loading