From c4412f2070c9b0af0e2003dd9429815355a44c4f Mon Sep 17 00:00:00 2001 From: Onur Kayabasi Date: Tue, 28 Jan 2025 23:41:02 +0100 Subject: [PATCH] Follow spec on span limits, batch processors (#7030) --- .../TracerProviderConfigurationTest.java | 4 +-- .../LoggerProviderConfigurationTest.java | 4 +-- .../sdk/logs/LogLimitsBuilder.java | 4 +-- .../BatchLogRecordProcessorBuilder.java | 2 ++ .../opentelemetry/sdk/logs/LogLimitsTest.java | 11 ++++++-- .../export/BatchLogRecordProcessorTest.java | 5 ++++ .../sdk/trace/SpanLimitsBuilder.java | 12 ++++----- .../export/BatchSpanProcessorBuilder.java | 2 ++ .../sdk/trace/config/SpanLimitsTest.java | 25 +++++++++++-------- .../trace/export/BatchSpanProcessorTest.java | 4 +++ 10 files changed, 49 insertions(+), 24 deletions(-) diff --git a/sdk-extensions/autoconfigure/src/test/java/io/opentelemetry/sdk/autoconfigure/TracerProviderConfigurationTest.java b/sdk-extensions/autoconfigure/src/test/java/io/opentelemetry/sdk/autoconfigure/TracerProviderConfigurationTest.java index d12ab1744f..e0ddcb438f 100644 --- a/sdk-extensions/autoconfigure/src/test/java/io/opentelemetry/sdk/autoconfigure/TracerProviderConfigurationTest.java +++ b/sdk-extensions/autoconfigure/src/test/java/io/opentelemetry/sdk/autoconfigure/TracerProviderConfigurationTest.java @@ -126,7 +126,7 @@ class TracerProviderConfigurationTest { Map properties = new HashMap<>(); properties.put("otel.bsp.schedule.delay", "100000"); properties.put("otel.bsp.max.queue.size", "2"); - properties.put("otel.bsp.max.export.batch.size", "3"); + properties.put("otel.bsp.max.export.batch.size", "2"); properties.put("otel.bsp.export.timeout", "4"); try (BatchSpanProcessor processor = @@ -144,7 +144,7 @@ class TracerProviderConfigurationTest { assertThat(worker) .extracting("exporterTimeoutNanos") .isEqualTo(TimeUnit.MILLISECONDS.toNanos(4)); - assertThat(worker).extracting("maxExportBatchSize").isEqualTo(3); + assertThat(worker).extracting("maxExportBatchSize").isEqualTo(2); assertThat(worker) .extracting("queue") .isInstanceOfSatisfying( diff --git a/sdk-extensions/autoconfigure/src/testFullConfig/java/io/opentelemetry/sdk/autoconfigure/LoggerProviderConfigurationTest.java b/sdk-extensions/autoconfigure/src/testFullConfig/java/io/opentelemetry/sdk/autoconfigure/LoggerProviderConfigurationTest.java index d3856cd93c..7a5473e8de 100644 --- a/sdk-extensions/autoconfigure/src/testFullConfig/java/io/opentelemetry/sdk/autoconfigure/LoggerProviderConfigurationTest.java +++ b/sdk-extensions/autoconfigure/src/testFullConfig/java/io/opentelemetry/sdk/autoconfigure/LoggerProviderConfigurationTest.java @@ -118,7 +118,7 @@ class LoggerProviderConfigurationTest { Map properties = new HashMap<>(); properties.put("otel.blrp.schedule.delay", "100000"); properties.put("otel.blrp.max.queue.size", "2"); - properties.put("otel.blrp.max.export.batch.size", "3"); + properties.put("otel.blrp.max.export.batch.size", "2"); properties.put("otel.blrp.export.timeout", "4"); try (BatchLogRecordProcessor processor = @@ -136,7 +136,7 @@ class LoggerProviderConfigurationTest { assertThat(worker) .extracting("exporterTimeoutNanos") .isEqualTo(TimeUnit.MILLISECONDS.toNanos(4)); - assertThat(worker).extracting("maxExportBatchSize").isEqualTo(3); + assertThat(worker).extracting("maxExportBatchSize").isEqualTo(2); assertThat(worker) .extracting("queue") .isInstanceOfSatisfying( diff --git a/sdk/logs/src/main/java/io/opentelemetry/sdk/logs/LogLimitsBuilder.java b/sdk/logs/src/main/java/io/opentelemetry/sdk/logs/LogLimitsBuilder.java index 22650d4d89..f4f74bf5ac 100644 --- a/sdk/logs/src/main/java/io/opentelemetry/sdk/logs/LogLimitsBuilder.java +++ b/sdk/logs/src/main/java/io/opentelemetry/sdk/logs/LogLimitsBuilder.java @@ -32,7 +32,7 @@ public final class LogLimitsBuilder { * @throws IllegalArgumentException if {@code maxNumberOfAttributes} is not positive. */ public LogLimitsBuilder setMaxNumberOfAttributes(int maxNumberOfAttributes) { - Utils.checkArgument(maxNumberOfAttributes > 0, "maxNumberOfAttributes must be greater than 0"); + Utils.checkArgument(maxNumberOfAttributes >= 0, "maxNumberOfAttributes must be non-negative"); this.maxNumAttributes = maxNumberOfAttributes; return this; } @@ -48,7 +48,7 @@ public final class LogLimitsBuilder { */ public LogLimitsBuilder setMaxAttributeValueLength(int maxAttributeValueLength) { Utils.checkArgument( - maxAttributeValueLength > -1, "maxAttributeValueLength must be non-negative"); + maxAttributeValueLength >= 0, "maxAttributeValueLength must be non-negative"); this.maxAttributeValueLength = maxAttributeValueLength; return this; } 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 4cf3058636..40e53302e6 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 @@ -98,9 +98,11 @@ public final class BatchLogRecordProcessorBuilder { * @param maxQueueSize the maximum number of Logs that are kept in the queue before start * dropping. * @return this. + * @throws IllegalArgumentException if {@code maxQueueSize} is not positive. * @see BatchLogRecordProcessorBuilder#DEFAULT_MAX_QUEUE_SIZE */ public BatchLogRecordProcessorBuilder setMaxQueueSize(int maxQueueSize) { + checkArgument(maxQueueSize > 0, "maxQueueSize must be positive."); this.maxQueueSize = maxQueueSize; return this; } diff --git a/sdk/logs/src/test/java/io/opentelemetry/sdk/logs/LogLimitsTest.java b/sdk/logs/src/test/java/io/opentelemetry/sdk/logs/LogLimitsTest.java index 250d49f840..4acb0666ba 100644 --- a/sdk/logs/src/test/java/io/opentelemetry/sdk/logs/LogLimitsTest.java +++ b/sdk/logs/src/test/java/io/opentelemetry/sdk/logs/LogLimitsTest.java @@ -6,6 +6,7 @@ package io.opentelemetry.sdk.logs; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; import org.junit.jupiter.api.Test; @@ -33,11 +34,17 @@ class LogLimitsTest { @Test void invalidLogLimits() { - assertThatThrownBy(() -> LogLimits.builder().setMaxNumberOfAttributes(0)) - .isInstanceOf(IllegalArgumentException.class); assertThatThrownBy(() -> LogLimits.builder().setMaxNumberOfAttributes(-1)) .isInstanceOf(IllegalArgumentException.class); assertThatThrownBy(() -> LogLimits.builder().setMaxAttributeValueLength(-1)) .isInstanceOf(IllegalArgumentException.class); } + + @Test + void validLogLimits() { + assertThatCode(() -> LogLimits.builder().setMaxNumberOfAttributes(0)) + .doesNotThrowAnyException(); + assertThatCode(() -> LogLimits.builder().setMaxAttributeValueLength(0)) + .doesNotThrowAnyException(); + } } diff --git a/sdk/logs/src/test/java/io/opentelemetry/sdk/logs/export/BatchLogRecordProcessorTest.java b/sdk/logs/src/test/java/io/opentelemetry/sdk/logs/export/BatchLogRecordProcessorTest.java index 4c17a1768f..e051eaf175 100644 --- a/sdk/logs/src/test/java/io/opentelemetry/sdk/logs/export/BatchLogRecordProcessorTest.java +++ b/sdk/logs/src/test/java/io/opentelemetry/sdk/logs/export/BatchLogRecordProcessorTest.java @@ -111,6 +111,10 @@ class BatchLogRecordProcessorTest { () -> BatchLogRecordProcessor.builder(mockLogRecordExporter).setExporterTimeout(null)) .isInstanceOf(NullPointerException.class) .hasMessage("timeout"); + assertThatThrownBy( + () -> BatchLogRecordProcessor.builder(mockLogRecordExporter).setMaxQueueSize(0)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("maxQueueSize must be positive."); } @Test @@ -337,6 +341,7 @@ class BatchLogRecordProcessorTest { .setExporterTimeout(exporterTimeoutMillis, TimeUnit.MILLISECONDS) .setScheduleDelay(1, TimeUnit.MILLISECONDS) .setMaxQueueSize(1) + .setMaxExportBatchSize(1) .build(); SdkLoggerProvider sdkLoggerProvider = SdkLoggerProvider.builder().addLogRecordProcessor(blp).build(); diff --git a/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SpanLimitsBuilder.java b/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SpanLimitsBuilder.java index 14b5f06f1a..353bc19b04 100644 --- a/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SpanLimitsBuilder.java +++ b/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SpanLimitsBuilder.java @@ -34,7 +34,7 @@ public final class SpanLimitsBuilder { * @throws IllegalArgumentException if {@code maxNumberOfAttributes} is not positive. */ public SpanLimitsBuilder setMaxNumberOfAttributes(int maxNumberOfAttributes) { - Utils.checkArgument(maxNumberOfAttributes > 0, "maxNumberOfAttributes must be greater than 0"); + Utils.checkArgument(maxNumberOfAttributes >= 0, "maxNumberOfAttributes must be non-negative"); this.maxNumAttributes = maxNumberOfAttributes; return this; } @@ -47,7 +47,7 @@ public final class SpanLimitsBuilder { * @throws IllegalArgumentException if {@code maxNumberOfEvents} is not positive. */ public SpanLimitsBuilder setMaxNumberOfEvents(int maxNumberOfEvents) { - Utils.checkArgument(maxNumberOfEvents > 0, "maxNumberOfEvents must be greater than 0"); + Utils.checkArgument(maxNumberOfEvents >= 0, "maxNumberOfEvents must be non-negative"); this.maxNumEvents = maxNumberOfEvents; return this; } @@ -60,7 +60,7 @@ public final class SpanLimitsBuilder { * @throws IllegalArgumentException if {@code maxNumberOfLinks} is not positive. */ public SpanLimitsBuilder setMaxNumberOfLinks(int maxNumberOfLinks) { - Utils.checkArgument(maxNumberOfLinks > 0, "maxNumberOfLinks must be greater than 0"); + Utils.checkArgument(maxNumberOfLinks >= 0, "maxNumberOfLinks must be non-negative"); this.maxNumLinks = maxNumberOfLinks; return this; } @@ -74,7 +74,7 @@ public final class SpanLimitsBuilder { */ public SpanLimitsBuilder setMaxNumberOfAttributesPerEvent(int maxNumberOfAttributesPerEvent) { Utils.checkArgument( - maxNumberOfAttributesPerEvent > 0, "maxNumberOfAttributesPerEvent must be greater than 0"); + maxNumberOfAttributesPerEvent >= 0, "maxNumberOfAttributesPerEvent must be non-negative"); this.maxNumAttributesPerEvent = maxNumberOfAttributesPerEvent; return this; } @@ -88,7 +88,7 @@ public final class SpanLimitsBuilder { */ public SpanLimitsBuilder setMaxNumberOfAttributesPerLink(int maxNumberOfAttributesPerLink) { Utils.checkArgument( - maxNumberOfAttributesPerLink > 0, "maxNumberOfAttributesPerLink must be greater than 0"); + maxNumberOfAttributesPerLink >= 0, "maxNumberOfAttributesPerLink must be non-negative"); this.maxNumAttributesPerLink = maxNumberOfAttributesPerLink; return this; } @@ -104,7 +104,7 @@ public final class SpanLimitsBuilder { */ public SpanLimitsBuilder setMaxAttributeValueLength(int maxAttributeValueLength) { Utils.checkArgument( - maxAttributeValueLength > -1, "maxAttributeValueLength must be non-negative"); + maxAttributeValueLength >= 0, "maxAttributeValueLength must be non-negative"); this.maxAttributeValueLength = maxAttributeValueLength; 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 3ec3fce077..8e9fe970d5 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 @@ -106,9 +106,11 @@ public final class BatchSpanProcessorBuilder { * @param maxQueueSize the maximum number of Spans that are kept in the queue before start * dropping. * @return this. + * @throws IllegalArgumentException if {@code maxQueueSize} is not positive. * @see BatchSpanProcessorBuilder#DEFAULT_MAX_QUEUE_SIZE */ public BatchSpanProcessorBuilder setMaxQueueSize(int maxQueueSize) { + checkArgument(maxQueueSize > 0, "maxQueueSize must be positive."); this.maxQueueSize = maxQueueSize; return this; } diff --git a/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/config/SpanLimitsTest.java b/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/config/SpanLimitsTest.java index ab749cea0f..73d9ac2e84 100644 --- a/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/config/SpanLimitsTest.java +++ b/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/config/SpanLimitsTest.java @@ -6,6 +6,7 @@ package io.opentelemetry.sdk.trace.config; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; import io.opentelemetry.sdk.trace.SpanLimits; @@ -46,27 +47,31 @@ class SpanLimitsTest { @Test void invalidSpanLimits() { - assertThatThrownBy(() -> SpanLimits.builder().setMaxNumberOfAttributes(0)) - .isInstanceOf(IllegalArgumentException.class); assertThatThrownBy(() -> SpanLimits.builder().setMaxNumberOfAttributes(-1)) .isInstanceOf(IllegalArgumentException.class); - assertThatThrownBy(() -> SpanLimits.builder().setMaxNumberOfEvents(0)) - .isInstanceOf(IllegalArgumentException.class); assertThatThrownBy(() -> SpanLimits.builder().setMaxNumberOfEvents(-1)) .isInstanceOf(IllegalArgumentException.class); - assertThatThrownBy(() -> SpanLimits.builder().setMaxNumberOfLinks(0)) - .isInstanceOf(IllegalArgumentException.class); assertThatThrownBy(() -> SpanLimits.builder().setMaxNumberOfLinks(-1)) .isInstanceOf(IllegalArgumentException.class); - assertThatThrownBy(() -> SpanLimits.builder().setMaxNumberOfAttributesPerEvent(0)) - .isInstanceOf(IllegalArgumentException.class); assertThatThrownBy(() -> SpanLimits.builder().setMaxNumberOfAttributesPerEvent(-1)) .isInstanceOf(IllegalArgumentException.class); - assertThatThrownBy(() -> SpanLimits.builder().setMaxNumberOfAttributesPerLink(0)) - .isInstanceOf(IllegalArgumentException.class); assertThatThrownBy(() -> SpanLimits.builder().setMaxNumberOfAttributesPerLink(-1)) .isInstanceOf(IllegalArgumentException.class); assertThatThrownBy(() -> SpanLimits.builder().setMaxAttributeValueLength(-1)) .isInstanceOf(IllegalArgumentException.class); } + + @Test + void validSpanLimits() { + assertThatCode(() -> SpanLimits.builder().setMaxNumberOfAttributes(0)) + .doesNotThrowAnyException(); + assertThatCode(() -> SpanLimits.builder().setMaxNumberOfEvents(0)).doesNotThrowAnyException(); + assertThatCode(() -> SpanLimits.builder().setMaxNumberOfLinks(0)).doesNotThrowAnyException(); + assertThatCode(() -> SpanLimits.builder().setMaxNumberOfAttributesPerEvent(0)) + .doesNotThrowAnyException(); + assertThatCode(() -> SpanLimits.builder().setMaxNumberOfAttributesPerLink(0)) + .doesNotThrowAnyException(); + assertThatCode(() -> SpanLimits.builder().setMaxAttributeValueLength(0)) + .doesNotThrowAnyException(); + } } diff --git a/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessorTest.java b/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessorTest.java index d9f48ecab1..23b8874790 100644 --- a/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessorTest.java +++ b/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/export/BatchSpanProcessorTest.java @@ -121,6 +121,9 @@ class BatchSpanProcessorTest { assertThatThrownBy(() -> BatchSpanProcessor.builder(mockSpanExporter).setExporterTimeout(null)) .isInstanceOf(NullPointerException.class) .hasMessage("timeout"); + assertThatThrownBy(() -> BatchSpanProcessor.builder(mockSpanExporter).setMaxQueueSize(0)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("maxQueueSize must be positive."); } @Test @@ -419,6 +422,7 @@ class BatchSpanProcessorTest { .setExporterTimeout(exporterTimeoutMillis, TimeUnit.MILLISECONDS) .setScheduleDelay(1, TimeUnit.MILLISECONDS) .setMaxQueueSize(1) + .setMaxExportBatchSize(1) .build(); sdkTracerProvider = SdkTracerProvider.builder().addSpanProcessor(bsp).build();