From ffb1fb4af4ae08b7d988df6eab22967d5afdd187 Mon Sep 17 00:00:00 2001 From: jwatson Date: Fri, 17 Jan 2020 11:58:29 -0800 Subject: [PATCH 1/3] Ignore null or blank string-valued span attributes. --- .../io/opentelemetry/internal/StringUtils.java | 4 ++++ .../java/io/opentelemetry/trace/DefaultTracer.java | 1 - api/src/main/java/io/opentelemetry/trace/Span.java | 9 +++++++-- .../io/opentelemetry/internal/StringUtilsTest.java | 14 ++++++++++---- .../io/opentelemetry/sdk/trace/SpanBuilderSdk.java | 4 ++++ .../sdk/trace/SpanBuilderSdkTest.java | 9 +++++++++ 6 files changed, 34 insertions(+), 7 deletions(-) diff --git a/api/src/main/java/io/opentelemetry/internal/StringUtils.java b/api/src/main/java/io/opentelemetry/internal/StringUtils.java index aa58546f8f0..627ef8e9283 100644 --- a/api/src/main/java/io/opentelemetry/internal/StringUtils.java +++ b/api/src/main/java/io/opentelemetry/internal/StringUtils.java @@ -41,5 +41,9 @@ private static boolean isPrintableChar(char ch) { return ch >= ' ' && ch <= '~'; } + public static boolean isNullOrBlank(String value) { + return value == null || value.length() == 0; + } + private StringUtils() {} } diff --git a/api/src/main/java/io/opentelemetry/trace/DefaultTracer.java b/api/src/main/java/io/opentelemetry/trace/DefaultTracer.java index b94cdf2fe81..11bab3947e2 100644 --- a/api/src/main/java/io/opentelemetry/trace/DefaultTracer.java +++ b/api/src/main/java/io/opentelemetry/trace/DefaultTracer.java @@ -134,7 +134,6 @@ public NoopSpanBuilder addLink(Link link) { @Override public NoopSpanBuilder setAttribute(String key, String value) { Utils.checkNotNull(key, "key"); - Utils.checkNotNull(value, "value"); return this; } diff --git a/api/src/main/java/io/opentelemetry/trace/Span.java b/api/src/main/java/io/opentelemetry/trace/Span.java index 7dc79cbe9e5..960098d8bb8 100644 --- a/api/src/main/java/io/opentelemetry/trace/Span.java +++ b/api/src/main/java/io/opentelemetry/trace/Span.java @@ -17,6 +17,7 @@ package io.opentelemetry.trace; import java.util.Map; +import javax.annotation.Nullable; import javax.annotation.concurrent.ThreadSafe; /** @@ -30,6 +31,7 @@ */ @ThreadSafe public interface Span { + /** * Type of span. Can be used to specify additional relationships between spans in addition to a * parent/child relationship. @@ -354,6 +356,7 @@ enum Kind { * @since 0.1.0 */ interface Builder { + /** * Sets the parent {@code Span} to use. If not set, the value of {@code Tracer.getCurrentSpan()} * at {@link #startSpan()} time will be used as parent. @@ -452,14 +455,16 @@ interface Builder { * Sets an attribute to the newly created {@code Span}. If {@code Span.Builder} previously * contained a mapping for the key, the old value is replaced by the specified value. * + *

If a null or empty String {@code value} is passed in, the attribute will be silently + * dropped. Note: this behavior could change in the future. + * * @param key the key for this attribute. * @param value the value for this attribute. * @return this. * @throws NullPointerException if {@code key} is {@code null}. - * @throws NullPointerException if {@code value} is {@code null}. * @since 0.3.0 */ - Builder setAttribute(String key, String value); + Builder setAttribute(String key, @Nullable String value); /** * Sets an attribute to the newly created {@code Span}. If {@code Span.Builder} previously diff --git a/api/src/test/java/io/opentelemetry/internal/StringUtilsTest.java b/api/src/test/java/io/opentelemetry/internal/StringUtilsTest.java index 8d9c12d519b..4702102a019 100644 --- a/api/src/test/java/io/opentelemetry/internal/StringUtilsTest.java +++ b/api/src/test/java/io/opentelemetry/internal/StringUtilsTest.java @@ -16,8 +16,7 @@ package io.opentelemetry.internal; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; +import static com.google.common.truth.Truth.assertThat; import org.junit.Test; import org.junit.runner.RunWith; @@ -29,7 +28,14 @@ public final class StringUtilsTest { @Test public void isPrintableString() { - assertTrue(StringUtils.isPrintableString("abcd")); - assertFalse(StringUtils.isPrintableString("\2ab\3cd")); + assertThat(StringUtils.isPrintableString("abcd")).isTrue(); + assertThat(StringUtils.isPrintableString("\2ab\3cd")).isFalse(); + } + + @Test + public void isNullOrEmpty() throws Exception { + assertThat(StringUtils.isNullOrBlank("")).isTrue(); + assertThat(StringUtils.isNullOrBlank(null)).isTrue(); + assertThat(StringUtils.isNullOrBlank("hello")).isFalse(); } } diff --git a/sdk/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java b/sdk/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java index d00af339861..e3d41c84729 100644 --- a/sdk/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java +++ b/sdk/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java @@ -16,6 +16,7 @@ package io.opentelemetry.sdk.trace; +import io.opentelemetry.internal.StringUtils; import io.opentelemetry.internal.Utils; import io.opentelemetry.sdk.common.Clock; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; @@ -137,6 +138,9 @@ public Span.Builder addLink(Link link) { @Override public Span.Builder setAttribute(String key, String value) { + if (StringUtils.isNullOrBlank(value)) { + return this; + } return setAttribute(key, AttributeValue.stringAttributeValue(value)); } diff --git a/sdk/src/test/java/io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java b/sdk/src/test/java/io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java index f6d456fe557..8bd2fb21a68 100644 --- a/sdk/src/test/java/io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java +++ b/sdk/src/test/java/io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java @@ -169,6 +169,15 @@ public void setAttribute() { } } + @Test + public void setAttribute_nullStringValue() throws Exception { + Span.Builder spanBuilder = tracerSdk.spanBuilder(SPAN_NAME); + spanBuilder.setAttribute("emptyString", ""); + spanBuilder.setAttribute("nullString", (String) null); + RecordEventsReadableSpan span = (RecordEventsReadableSpan) spanBuilder.startSpan(); + assertThat(span.toSpanData().getAttributes()).isEmpty(); + } + @Test public void droppingAttributes() { final int maxNumberOfAttrs = 8; From 71b9b77da949061995cb35e24fd90b3f05fcc903 Mon Sep 17 00:00:00 2001 From: jwatson Date: Fri, 17 Jan 2020 13:10:31 -0800 Subject: [PATCH 2/3] also ignore null & blank strings on active span attributes. --- api/src/main/java/io/opentelemetry/trace/DefaultSpan.java | 1 - api/src/main/java/io/opentelemetry/trace/Span.java | 5 ++++- .../test/java/io/opentelemetry/trace/DefaultSpanTest.java | 2 ++ .../io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java | 4 ++++ .../sdk/trace/RecordEventsReadableSpanTest.java | 2 ++ 5 files changed, 12 insertions(+), 2 deletions(-) diff --git a/api/src/main/java/io/opentelemetry/trace/DefaultSpan.java b/api/src/main/java/io/opentelemetry/trace/DefaultSpan.java index a8266d7135d..9f823c8d777 100644 --- a/api/src/main/java/io/opentelemetry/trace/DefaultSpan.java +++ b/api/src/main/java/io/opentelemetry/trace/DefaultSpan.java @@ -74,7 +74,6 @@ static DefaultSpan createRandom() { @Override public void setAttribute(String key, String value) { Utils.checkNotNull(key, "key"); - Utils.checkNotNull(value, "value"); } @Override diff --git a/api/src/main/java/io/opentelemetry/trace/Span.java b/api/src/main/java/io/opentelemetry/trace/Span.java index 960098d8bb8..5ee1377ba2e 100644 --- a/api/src/main/java/io/opentelemetry/trace/Span.java +++ b/api/src/main/java/io/opentelemetry/trace/Span.java @@ -83,11 +83,14 @@ enum Kind { * Sets an attribute to the {@code Span}. If the {@code Span} previously contained a mapping for * the key, the old value is replaced by the specified value. * + *

If a null or empty String {@code value} is passed in, the attribute will be silently + * dropped. Note: this behavior could change in the future. + * * @param key the key for this attribute. * @param value the value for this attribute. * @since 0.1.0 */ - void setAttribute(String key, String value); + void setAttribute(String key, @Nullable String value); /** * Sets an attribute to the {@code Span}. If the {@code Span} previously contained a mapping for diff --git a/api/src/test/java/io/opentelemetry/trace/DefaultSpanTest.java b/api/src/test/java/io/opentelemetry/trace/DefaultSpanTest.java index 18bb48c036e..d49abdad7a4 100644 --- a/api/src/test/java/io/opentelemetry/trace/DefaultSpanTest.java +++ b/api/src/test/java/io/opentelemetry/trace/DefaultSpanTest.java @@ -53,6 +53,8 @@ public void doNotCrash() { "MyStringAttributeKey", AttributeValue.stringAttributeValue("MyStringAttributeValue")); span.setAttribute("MyBooleanAttributeKey", AttributeValue.booleanAttributeValue(true)); span.setAttribute("MyLongAttributeKey", AttributeValue.longAttributeValue(123)); + span.setAttribute("NullString", (String) null); + span.setAttribute("EmptyString", ""); span.addEvent("event"); span.addEvent("event", 0); span.addEvent( diff --git a/sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java b/sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java index 687792a46d0..1f9fd3af0cf 100644 --- a/sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java +++ b/sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java @@ -18,6 +18,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.EvictingQueue; +import io.opentelemetry.internal.StringUtils; import io.opentelemetry.sdk.common.Clock; import io.opentelemetry.sdk.common.InstrumentationLibraryInfo; import io.opentelemetry.sdk.resources.Resource; @@ -303,6 +304,9 @@ Clock getClock() { @Override public void setAttribute(String key, String value) { + if (StringUtils.isNullOrBlank(value)) { + return; + } setAttribute(key, AttributeValue.stringAttributeValue(value)); } diff --git a/sdk/src/test/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpanTest.java b/sdk/src/test/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpanTest.java index 9ddf3f91472..30b093b7601 100644 --- a/sdk/src/test/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpanTest.java +++ b/sdk/src/test/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpanTest.java @@ -286,6 +286,8 @@ public void setAttribute() { RecordEventsReadableSpan span = createTestRootSpan(); try { span.setAttribute("StringKey", "StringVal"); + span.setAttribute("NullStringKey", (String) null); + span.setAttribute("EmptyStringKey", ""); span.setAttribute("LongKey", 1000L); span.setAttribute("DoubleKey", 10.0); span.setAttribute("BooleanKey", false); From 0e77fcb292510ff2ea9979757e2e3a9c0735e56d Mon Sep 17 00:00:00 2001 From: jwatson Date: Fri, 17 Jan 2020 14:30:09 -0800 Subject: [PATCH 3/3] change to ignore at the AttributeValue level, rather than above --- .../main/java/io/opentelemetry/trace/AttributeValue.java | 6 +++--- .../opentelemetry/sdk/trace/RecordEventsReadableSpan.java | 7 ++++--- .../java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java | 7 ++++--- .../sdk/trace/RecordEventsReadableSpanTest.java | 2 ++ .../io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java | 2 ++ 5 files changed, 15 insertions(+), 9 deletions(-) diff --git a/api/src/main/java/io/opentelemetry/trace/AttributeValue.java b/api/src/main/java/io/opentelemetry/trace/AttributeValue.java index 298545c7644..7e8167e7055 100644 --- a/api/src/main/java/io/opentelemetry/trace/AttributeValue.java +++ b/api/src/main/java/io/opentelemetry/trace/AttributeValue.java @@ -17,7 +17,7 @@ package io.opentelemetry.trace; import com.google.auto.value.AutoValue; -import io.opentelemetry.internal.Utils; +import javax.annotation.Nullable; import javax.annotation.concurrent.Immutable; /** @@ -152,8 +152,7 @@ abstract static class AttributeValueString extends AttributeValue { AttributeValueString() {} static AttributeValue create(String stringValue) { - return new AutoValue_AttributeValue_AttributeValueString( - Utils.checkNotNull(stringValue, "stringValue")); + return new AutoValue_AttributeValue_AttributeValueString(stringValue); } @Override @@ -162,6 +161,7 @@ public final Type getType() { } @Override + @Nullable public abstract String getStringValue(); } diff --git a/sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java b/sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java index 1f9fd3af0cf..fae3b265be1 100644 --- a/sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java +++ b/sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java @@ -24,6 +24,7 @@ import io.opentelemetry.sdk.resources.Resource; import io.opentelemetry.sdk.trace.config.TraceConfig; import io.opentelemetry.trace.AttributeValue; +import io.opentelemetry.trace.AttributeValue.Type; import io.opentelemetry.trace.EndSpanOptions; import io.opentelemetry.trace.Event; import io.opentelemetry.trace.Link; @@ -304,9 +305,6 @@ Clock getClock() { @Override public void setAttribute(String key, String value) { - if (StringUtils.isNullOrBlank(value)) { - return; - } setAttribute(key, AttributeValue.stringAttributeValue(value)); } @@ -329,6 +327,9 @@ public void setAttribute(String key, boolean value) { public void setAttribute(String key, AttributeValue value) { Preconditions.checkNotNull(key, "key"); Preconditions.checkNotNull(value, "value"); + if (value.getType() == Type.STRING && StringUtils.isNullOrBlank(value.getStringValue())) { + return; + } synchronized (lock) { if (hasEnded) { logger.log(Level.FINE, "Calling setAttribute() on an ended Span."); diff --git a/sdk/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java b/sdk/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java index e3d41c84729..080829b110e 100644 --- a/sdk/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java +++ b/sdk/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java @@ -25,6 +25,7 @@ import io.opentelemetry.sdk.trace.Sampler.Decision; import io.opentelemetry.sdk.trace.config.TraceConfig; import io.opentelemetry.trace.AttributeValue; +import io.opentelemetry.trace.AttributeValue.Type; import io.opentelemetry.trace.DefaultSpan; import io.opentelemetry.trace.Link; import io.opentelemetry.trace.Span; @@ -138,9 +139,6 @@ public Span.Builder addLink(Link link) { @Override public Span.Builder setAttribute(String key, String value) { - if (StringUtils.isNullOrBlank(value)) { - return this; - } return setAttribute(key, AttributeValue.stringAttributeValue(value)); } @@ -163,6 +161,9 @@ public Span.Builder setAttribute(String key, boolean value) { public Span.Builder setAttribute(String key, AttributeValue value) { Utils.checkNotNull(key, "key"); Utils.checkNotNull(value, "value"); + if (value.getType() == Type.STRING && StringUtils.isNullOrBlank(value.getStringValue())) { + return this; + } attributes.putAttribute(key, value); return this; } diff --git a/sdk/src/test/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpanTest.java b/sdk/src/test/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpanTest.java index 30b093b7601..0fa27a3a634 100644 --- a/sdk/src/test/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpanTest.java +++ b/sdk/src/test/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpanTest.java @@ -288,6 +288,8 @@ public void setAttribute() { span.setAttribute("StringKey", "StringVal"); span.setAttribute("NullStringKey", (String) null); span.setAttribute("EmptyStringKey", ""); + span.setAttribute("NullStringAttributeValue", AttributeValue.stringAttributeValue(null)); + span.setAttribute("EmptyStringAttributeValue", AttributeValue.stringAttributeValue("")); span.setAttribute("LongKey", 1000L); span.setAttribute("DoubleKey", 10.0); span.setAttribute("BooleanKey", false); diff --git a/sdk/src/test/java/io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java b/sdk/src/test/java/io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java index 8bd2fb21a68..da0d7c05c7a 100644 --- a/sdk/src/test/java/io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java +++ b/sdk/src/test/java/io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java @@ -174,6 +174,8 @@ public void setAttribute_nullStringValue() throws Exception { Span.Builder spanBuilder = tracerSdk.spanBuilder(SPAN_NAME); spanBuilder.setAttribute("emptyString", ""); spanBuilder.setAttribute("nullString", (String) null); + spanBuilder.setAttribute("nullStringAttributeValue", AttributeValue.stringAttributeValue(null)); + spanBuilder.setAttribute("emptyStringAttributeValue", AttributeValue.stringAttributeValue("")); RecordEventsReadableSpan span = (RecordEventsReadableSpan) spanBuilder.startSpan(); assertThat(span.toSpanData().getAttributes()).isEmpty(); }