From 31869a3fc694d58a9ba22603811002eff9c17999 Mon Sep 17 00:00:00 2001 From: jack-berg <34418638+jack-berg@users.noreply.github.com> Date: Thu, 16 Jan 2025 18:02:31 -0600 Subject: [PATCH] Interpret timeout zero value as no limit (#7023) --- .../internal/grpc/GrpcExporterBuilder.java | 4 +- .../internal/http/HttpExporterBuilder.java | 4 +- .../AbstractGrpcTelemetryExporterTest.java | 58 +++++++++++++++++-- .../AbstractHttpTelemetryExporterTest.java | 49 +++++++++++++--- .../okhttp/internal/OkHttpGrpcSender.java | 8 ++- .../okhttp/internal/OkHttpHttpSender.java | 8 ++- .../zipkin/ZipkinSpanExporterBuilder.java | 7 ++- .../zipkin/ZipkinSpanExporterTest.java | 15 +++++ .../BatchLogRecordProcessorBuilder.java | 2 +- .../export/BatchSpanProcessorBuilder.java | 2 +- 10 files changed, 131 insertions(+), 26 deletions(-) diff --git a/exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporterBuilder.java b/exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporterBuilder.java index c5b04fd8db8..3ddf45c03e4 100644 --- a/exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporterBuilder.java +++ b/exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporterBuilder.java @@ -87,7 +87,7 @@ public GrpcExporterBuilder setChannel(ManagedChannel channel) { } public GrpcExporterBuilder setTimeout(long timeout, TimeUnit unit) { - timeoutNanos = unit.toNanos(timeout); + timeoutNanos = timeout == 0 ? Long.MAX_VALUE : unit.toNanos(timeout); return this; } @@ -96,7 +96,7 @@ public GrpcExporterBuilder setTimeout(Duration timeout) { } public GrpcExporterBuilder setConnectTimeout(long timeout, TimeUnit unit) { - connectTimeoutNanos = unit.toNanos(timeout); + connectTimeoutNanos = timeout == 0 ? Long.MAX_VALUE : unit.toNanos(timeout); return this; } diff --git a/exporters/common/src/main/java/io/opentelemetry/exporter/internal/http/HttpExporterBuilder.java b/exporters/common/src/main/java/io/opentelemetry/exporter/internal/http/HttpExporterBuilder.java index e88533ab8aa..a82da5afc9a 100644 --- a/exporters/common/src/main/java/io/opentelemetry/exporter/internal/http/HttpExporterBuilder.java +++ b/exporters/common/src/main/java/io/opentelemetry/exporter/internal/http/HttpExporterBuilder.java @@ -69,12 +69,12 @@ public HttpExporterBuilder(String exporterName, String type, String defaultEndpo } public HttpExporterBuilder setTimeout(long timeout, TimeUnit unit) { - timeoutNanos = unit.toNanos(timeout); + timeoutNanos = timeout == 0 ? Long.MAX_VALUE : unit.toNanos(timeout); return this; } public HttpExporterBuilder setConnectTimeout(long timeout, TimeUnit unit) { - connectTimeoutNanos = unit.toNanos(timeout); + connectTimeoutNanos = timeout == 0 ? Long.MAX_VALUE : unit.toNanos(timeout); return this; } diff --git a/exporters/otlp/testing-internal/src/main/java/io/opentelemetry/exporter/otlp/testing/internal/AbstractGrpcTelemetryExporterTest.java b/exporters/otlp/testing-internal/src/main/java/io/opentelemetry/exporter/otlp/testing/internal/AbstractGrpcTelemetryExporterTest.java index 1d7086c1c99..5aca2a4588e 100644 --- a/exporters/otlp/testing-internal/src/main/java/io/opentelemetry/exporter/otlp/testing/internal/AbstractGrpcTelemetryExporterTest.java +++ b/exporters/otlp/testing-internal/src/main/java/io/opentelemetry/exporter/otlp/testing/internal/AbstractGrpcTelemetryExporterTest.java @@ -815,13 +815,39 @@ 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(Long.MAX_VALUE, TimeUnit.NANOSECONDS))) + .doesNotThrowAnyException(); + assertThatCode( + () -> buildAndShutdown(exporterBuilder().setTimeout(Duration.ofNanos(Long.MAX_VALUE)))) + .doesNotThrowAnyException(); + assertThatCode( + () -> buildAndShutdown(exporterBuilder().setTimeout(Long.MAX_VALUE, TimeUnit.SECONDS))) + .doesNotThrowAnyException(); + assertThatCode(() -> buildAndShutdown(exporterBuilder().setTimeout(10, TimeUnit.MILLISECONDS))) + .doesNotThrowAnyException(); + 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(() -> exporterBuilder().setTimeout(Duration.ofMillis(10))) + assertThatCode( + () -> buildAndShutdown(exporterBuilder().setConnectTimeout(Duration.ofMillis(10)))) .doesNotThrowAnyException(); assertThatCode(() -> exporterBuilder().setEndpoint("http://localhost:4317")) @@ -846,6 +872,11 @@ void validConfig() { .doesNotThrowAnyException(); } + private void buildAndShutdown(TelemetryExporterBuilder builder) { + TelemetryExporter build = builder.build(); + build.shutdown().join(10, TimeUnit.MILLISECONDS); + } + @Test @SuppressWarnings({"PreferJavaTimeOverload", "NullAway"}) void invalidConfig() { @@ -858,6 +889,25 @@ void invalidConfig() { assertThatThrownBy(() -> exporterBuilder().setTimeout(null)) .isInstanceOf(NullPointerException.class) .hasMessage("timeout"); + assertThatThrownBy( + () -> + buildAndShutdown(exporterBuilder().setTimeout(Duration.ofSeconds(Long.MAX_VALUE)))) + .isInstanceOf(ArithmeticException.class); + + assertThatThrownBy(() -> exporterBuilder().setConnectTimeout(-1, TimeUnit.MILLISECONDS)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("timeout must be non-negative"); + assertThatThrownBy(() -> exporterBuilder().setConnectTimeout(1, null)) + .isInstanceOf(NullPointerException.class) + .hasMessage("unit"); + assertThatThrownBy(() -> exporterBuilder().setConnectTimeout(null)) + .isInstanceOf(NullPointerException.class) + .hasMessage("timeout"); + assertThatThrownBy( + () -> + buildAndShutdown( + exporterBuilder().setConnectTimeout(Duration.ofSeconds(Long.MAX_VALUE)))) + .isInstanceOf(ArithmeticException.class); assertThatThrownBy(() -> exporterBuilder().setEndpoint(null)) .isInstanceOf(NullPointerException.class) diff --git a/exporters/otlp/testing-internal/src/main/java/io/opentelemetry/exporter/otlp/testing/internal/AbstractHttpTelemetryExporterTest.java b/exporters/otlp/testing-internal/src/main/java/io/opentelemetry/exporter/otlp/testing/internal/AbstractHttpTelemetryExporterTest.java index 28e54dab222..6f622a6e70c 100644 --- a/exporters/otlp/testing-internal/src/main/java/io/opentelemetry/exporter/otlp/testing/internal/AbstractHttpTelemetryExporterTest.java +++ b/exporters/otlp/testing-internal/src/main/java/io/opentelemetry/exporter/otlp/testing/internal/AbstractHttpTelemetryExporterTest.java @@ -698,22 +698,39 @@ 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(Long.MAX_VALUE, TimeUnit.NANOSECONDS))) .doesNotThrowAnyException(); - assertThatCode(() -> exporterBuilder().setTimeout(Duration.ofMillis(10))) + assertThatCode( + () -> buildAndShutdown(exporterBuilder().setTimeout(Duration.ofNanos(Long.MAX_VALUE)))) .doesNotThrowAnyException(); - - assertThatCode(() -> exporterBuilder().setConnectTimeout(0, TimeUnit.MILLISECONDS)) + assertThatCode( + () -> buildAndShutdown(exporterBuilder().setTimeout(Long.MAX_VALUE, TimeUnit.SECONDS))) + .doesNotThrowAnyException(); + assertThatCode(() -> buildAndShutdown(exporterBuilder().setTimeout(10, TimeUnit.MILLISECONDS))) + .doesNotThrowAnyException(); + assertThatCode(() -> buildAndShutdown(exporterBuilder().setTimeout(Duration.ofMillis(10)))) + .doesNotThrowAnyException(); + 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")) @@ -738,6 +755,11 @@ void validConfig() { .doesNotThrowAnyException(); } + private void buildAndShutdown(TelemetryExporterBuilder builder) { + TelemetryExporter build = builder.build(); + build.shutdown().join(10, TimeUnit.MILLISECONDS); + } + @Test @SuppressWarnings({"PreferJavaTimeOverload", "NullAway"}) void invalidConfig() { @@ -750,6 +772,10 @@ void invalidConfig() { assertThatThrownBy(() -> exporterBuilder().setTimeout(null)) .isInstanceOf(NullPointerException.class) .hasMessage("timeout"); + assertThatThrownBy( + () -> + buildAndShutdown(exporterBuilder().setTimeout(Duration.ofSeconds(Long.MAX_VALUE)))) + .isInstanceOf(ArithmeticException.class); assertThatThrownBy(() -> exporterBuilder().setConnectTimeout(-1, TimeUnit.MILLISECONDS)) .isInstanceOf(IllegalArgumentException.class) @@ -760,6 +786,11 @@ void invalidConfig() { assertThatThrownBy(() -> exporterBuilder().setConnectTimeout(null)) .isInstanceOf(NullPointerException.class) .hasMessage("timeout"); + assertThatThrownBy( + () -> + buildAndShutdown( + exporterBuilder().setConnectTimeout(Duration.ofSeconds(Long.MAX_VALUE)))) + .isInstanceOf(ArithmeticException.class); assertThatThrownBy(() -> exporterBuilder().setEndpoint(null)) .isInstanceOf(NullPointerException.class) diff --git a/exporters/sender/okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpGrpcSender.java b/exporters/sender/okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpGrpcSender.java index f8b4996b90e..a0305cb863f 100644 --- a/exporters/sender/okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpGrpcSender.java +++ b/exporters/sender/okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpGrpcSender.java @@ -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); 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)); if (retryPolicy != null) { clientBuilder.addInterceptor( new RetryInterceptor(retryPolicy, OkHttpGrpcSender::isRetryable)); diff --git a/exporters/sender/okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpHttpSender.java b/exporters/sender/okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpHttpSender.java index 53d06120e2e..9161aa42088 100644 --- a/exporters/sender/okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpHttpSender.java +++ b/exporters/sender/okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpHttpSender.java @@ -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)) + .callTimeout(Duration.ofMillis(callTimeoutMillis)); if (proxyOptions != null) { builder.proxySelector(proxyOptions.getProxySelector()); diff --git a/exporters/zipkin/src/main/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterBuilder.java b/exporters/zipkin/src/main/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterBuilder.java index d13f1d4a825..0f1fae95b9a 100644 --- a/exporters/zipkin/src/main/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterBuilder.java +++ b/exporters/zipkin/src/main/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterBuilder.java @@ -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 meterProviderSupplier = GlobalOpenTelemetry::getMeterProvider; /** @@ -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; } @@ -212,7 +213,7 @@ public ZipkinSpanExporter build() { OkHttpSender.newBuilder() .endpoint(endpoint) .compressionEnabled(compressionEnabled) - .readTimeout((int) readTimeoutMillis) + .readTimeout(readTimeoutMillis) .build(); } OtelToZipkinSpanTransformer transformer = diff --git a/exporters/zipkin/src/test/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterTest.java b/exporters/zipkin/src/test/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterTest.java index 2c70691744c..b89c273bbbb 100644 --- a/exporters/zipkin/src/test/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterTest.java +++ b/exporters/zipkin/src/test/java/io/opentelemetry/exporter/zipkin/ZipkinSpanExporterTest.java @@ -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()) { diff --git a/sdk/logs/src/main/java/io/opentelemetry/sdk/logs/export/BatchLogRecordProcessorBuilder.java b/sdk/logs/src/main/java/io/opentelemetry/sdk/logs/export/BatchLogRecordProcessorBuilder.java index 6aafa9525aa..4cf30586367 100644 --- a/sdk/logs/src/main/java/io/opentelemetry/sdk/logs/export/BatchLogRecordProcessorBuilder.java +++ b/sdk/logs/src/main/java/io/opentelemetry/sdk/logs/export/BatchLogRecordProcessorBuilder.java @@ -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; } diff --git a/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessorBuilder.java b/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessorBuilder.java index c3235a60f4f..3ec3fce0778 100644 --- a/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessorBuilder.java +++ b/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessorBuilder.java @@ -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; }