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.

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

Require exporter timeouts to be positive #6850

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public final class OtlpHttpLogRecordExporterBuilder {
*/
public OtlpHttpLogRecordExporterBuilder setTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
checkArgument(timeout > 0, "timeout must be positive");
delegate.setTimeout(timeout, unit);
return this;
}
Expand All @@ -78,7 +78,7 @@ public OtlpHttpLogRecordExporterBuilder setTimeout(Duration timeout) {
*/
public OtlpHttpLogRecordExporterBuilder setConnectTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
checkArgument(timeout > 0, "timeout must be positive");
delegate.setConnectTimeout(timeout, unit);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public final class OtlpHttpMetricExporterBuilder {
*/
public OtlpHttpMetricExporterBuilder setTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
checkArgument(timeout > 0, "timeout must be positive");
delegate.setTimeout(timeout, unit);
return this;
}
Expand All @@ -92,7 +92,7 @@ public OtlpHttpMetricExporterBuilder setTimeout(Duration timeout) {
*/
public OtlpHttpMetricExporterBuilder setConnectTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
checkArgument(timeout > 0, "timeout must be positive");
delegate.setConnectTimeout(timeout, unit);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public final class OtlpHttpSpanExporterBuilder {
*/
public OtlpHttpSpanExporterBuilder setTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
checkArgument(timeout > 0, "timeout must be positive");
delegate.setTimeout(timeout, unit);
return this;
}
Expand All @@ -78,7 +78,7 @@ public OtlpHttpSpanExporterBuilder setTimeout(Duration timeout) {
*/
public OtlpHttpSpanExporterBuilder setConnectTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
checkArgument(timeout > 0, "timeout must be positive");
delegate.setConnectTimeout(timeout, unit);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public OtlpGrpcLogRecordExporterBuilder setChannel(ManagedChannel channel) {
*/
public OtlpGrpcLogRecordExporterBuilder setTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
checkArgument(timeout > 0, "timeout must be positive");
delegate.setTimeout(timeout, unit);
return this;
}
Expand All @@ -115,7 +115,7 @@ public OtlpGrpcLogRecordExporterBuilder setTimeout(Duration timeout) {
*/
public OtlpGrpcLogRecordExporterBuilder setConnectTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
checkArgument(timeout > 0, "timeout must be positive");
delegate.setConnectTimeout(timeout, unit);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ public OtlpGrpcMetricExporterBuilder setChannel(ManagedChannel channel) {
*/
public OtlpGrpcMetricExporterBuilder setTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
checkArgument(timeout > 0, "timeout must be positive");
delegate.setTimeout(timeout, unit);
return this;
}
Expand All @@ -129,7 +129,7 @@ public OtlpGrpcMetricExporterBuilder setTimeout(Duration timeout) {
*/
public OtlpGrpcMetricExporterBuilder setConnectTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
checkArgument(timeout > 0, "timeout must be positive");
delegate.setConnectTimeout(timeout, unit);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public OtlpGrpcSpanExporterBuilder setChannel(ManagedChannel channel) {
*/
public OtlpGrpcSpanExporterBuilder setTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
checkArgument(timeout > 0, "timeout must be positive");
delegate.setTimeout(timeout, unit);
return this;
}
Expand All @@ -111,7 +111,7 @@ public OtlpGrpcSpanExporterBuilder setTimeout(Duration timeout) {
*/
public OtlpGrpcSpanExporterBuilder setConnectTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
checkArgument(timeout > 0, "timeout must be positive");
delegate.setConnectTimeout(timeout, unit);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -814,9 +814,9 @@ void overrideHost() {
@Test
@SuppressWarnings("PreferJavaTimeOverload")
void validConfig() {
assertThatCode(() -> exporterBuilder().setTimeout(0, TimeUnit.MILLISECONDS))
assertThatCode(() -> exporterBuilder().setTimeout(1, TimeUnit.MILLISECONDS))
.doesNotThrowAnyException();
assertThatCode(() -> exporterBuilder().setTimeout(Duration.ofMillis(0)))
assertThatCode(() -> exporterBuilder().setTimeout(Duration.ofMillis(1)))
.doesNotThrowAnyException();
assertThatCode(() -> exporterBuilder().setTimeout(10, TimeUnit.MILLISECONDS))
.doesNotThrowAnyException();
Expand Down Expand Up @@ -848,9 +848,9 @@ void validConfig() {
@Test
@SuppressWarnings({"PreferJavaTimeOverload", "NullAway"})
void invalidConfig() {
assertThatThrownBy(() -> exporterBuilder().setTimeout(-1, TimeUnit.MILLISECONDS))
assertThatThrownBy(() -> exporterBuilder().setTimeout(0, TimeUnit.MILLISECONDS))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("timeout must be non-negative");
.hasMessage("timeout must be positive");
assertThatThrownBy(() -> exporterBuilder().setTimeout(1, null))
.isInstanceOf(NullPointerException.class)
.hasMessage("unit");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -728,18 +728,18 @@ void proxy() {
@Test
@SuppressWarnings("PreferJavaTimeOverload")
void validConfig() {
assertThatCode(() -> exporterBuilder().setTimeout(0, TimeUnit.MILLISECONDS))
assertThatCode(() -> exporterBuilder().setTimeout(1, TimeUnit.MILLISECONDS))
.doesNotThrowAnyException();
assertThatCode(() -> exporterBuilder().setTimeout(Duration.ofMillis(0)))
assertThatCode(() -> exporterBuilder().setTimeout(Duration.ofMillis(1)))
.doesNotThrowAnyException();
assertThatCode(() -> exporterBuilder().setTimeout(10, TimeUnit.MILLISECONDS))
.doesNotThrowAnyException();
assertThatCode(() -> exporterBuilder().setTimeout(Duration.ofMillis(10)))
.doesNotThrowAnyException();

assertThatCode(() -> exporterBuilder().setConnectTimeout(0, TimeUnit.MILLISECONDS))
assertThatCode(() -> exporterBuilder().setConnectTimeout(1, TimeUnit.MILLISECONDS))
.doesNotThrowAnyException();
assertThatCode(() -> exporterBuilder().setConnectTimeout(Duration.ofMillis(0)))
assertThatCode(() -> exporterBuilder().setConnectTimeout(Duration.ofMillis(1)))
.doesNotThrowAnyException();
assertThatCode(() -> exporterBuilder().setConnectTimeout(10, TimeUnit.MILLISECONDS))
.doesNotThrowAnyException();
Expand Down Expand Up @@ -771,19 +771,19 @@ void validConfig() {
@Test
@SuppressWarnings({"PreferJavaTimeOverload", "NullAway"})
void invalidConfig() {
assertThatThrownBy(() -> exporterBuilder().setTimeout(-1, TimeUnit.MILLISECONDS))
assertThatThrownBy(() -> exporterBuilder().setTimeout(0, TimeUnit.MILLISECONDS))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("timeout must be non-negative");
.hasMessage("timeout must be positive");
assertThatThrownBy(() -> exporterBuilder().setTimeout(1, null))
.isInstanceOf(NullPointerException.class)
.hasMessage("unit");
assertThatThrownBy(() -> exporterBuilder().setTimeout(null))
.isInstanceOf(NullPointerException.class)
.hasMessage("timeout");

assertThatThrownBy(() -> exporterBuilder().setConnectTimeout(-1, TimeUnit.MILLISECONDS))
assertThatThrownBy(() -> exporterBuilder().setConnectTimeout(0, TimeUnit.MILLISECONDS))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("timeout must be non-negative");
.hasMessage("timeout must be positive");
assertThatThrownBy(() -> exporterBuilder().setConnectTimeout(1, null))
.isInstanceOf(NullPointerException.class)
.hasMessage("unit");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ public ZipkinSpanExporterBuilder setCompression(String compressionMethod) {
*/
public ZipkinSpanExporterBuilder setReadTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
checkArgument(timeout > 0, "timeout must be positive");
this.readTimeoutMillis = unit.toMillis(timeout);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,9 @@ void testShutdown() throws IOException {
@SuppressWarnings({"PreferJavaTimeOverload", "deprecation"})
// we have to use the deprecated setEncoder overload to test it
void invalidConfig() {
assertThatThrownBy(() -> ZipkinSpanExporter.builder().setReadTimeout(-1, TimeUnit.MILLISECONDS))
assertThatThrownBy(() -> ZipkinSpanExporter.builder().setReadTimeout(0, TimeUnit.MILLISECONDS))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("timeout must be non-negative");
.hasMessage("timeout must be positive");

assertThatThrownBy(() -> ZipkinSpanExporter.builder().setReadTimeout(1, null))
.isInstanceOf(NullPointerException.class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ long getScheduleDelayNanos() {
*/
public BatchLogRecordProcessorBuilder setExporterTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
checkArgument(timeout > 0, "timeout must be positive");
exporterTimeoutNanos = unit.toNanos(timeout);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,9 @@ void builderInvalidConfig() {
assertThatThrownBy(
() ->
BatchLogRecordProcessor.builder(mockLogRecordExporter)
.setExporterTimeout(-1, TimeUnit.MILLISECONDS))
.setExporterTimeout(0, TimeUnit.MILLISECONDS))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("timeout must be non-negative");
.hasMessage("timeout must be positive");
assertThatThrownBy(
() ->
BatchLogRecordProcessor.builder(mockLogRecordExporter).setExporterTimeout(1, null))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ long getScheduleDelayNanos() {
*/
public BatchSpanProcessorBuilder setExporterTimeout(long timeout, TimeUnit unit) {
requireNonNull(unit, "unit");
checkArgument(timeout >= 0, "timeout must be non-negative");
checkArgument(timeout > 0, "timeout must be positive");
exporterTimeoutNanos = unit.toNanos(timeout);
return this;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,9 @@ void builderInvalidConfig() {
assertThatThrownBy(
() ->
BatchSpanProcessor.builder(mockSpanExporter)
.setExporterTimeout(-1, TimeUnit.MILLISECONDS))
.setExporterTimeout(0, TimeUnit.MILLISECONDS))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("timeout must be non-negative");
.hasMessage("timeout must be positive");
assertThatThrownBy(
() -> BatchSpanProcessor.builder(mockSpanExporter).setExporterTimeout(1, null))
.isInstanceOf(NullPointerException.class)
Expand Down
Loading