Ignore null or blank string-valued span attributes. (#771)

* Ignore null or blank string-valued span attributes.

* also ignore null & blank strings on active span attributes.

* change to ignore at the AttributeValue level, rather than above

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() {}
 }

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();
   }
 

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

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;
     }
 

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.
@@ -81,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.
    *
+   * <p>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
@@ -354,6 +359,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 +458,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.
      *
+     * <p>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

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();
   }
 }

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(

sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java

@@ -18,11 +18,13 @@
 
 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;
 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;
@@ -325,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.");

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;
@@ -24,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;
@@ -159,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;
   }

sdk/src/test/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpanTest.java

@@ -286,6 +286,10 @@ public void setAttribute() {
     RecordEventsReadableSpan span = createTestRootSpan();
     try {
       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);

sdk/src/test/java/io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java

@@ -169,6 +169,17 @@ 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);
+    spanBuilder.setAttribute("nullStringAttributeValue", AttributeValue.stringAttributeValue(null));
+    spanBuilder.setAttribute("emptyStringAttributeValue", AttributeValue.stringAttributeValue(""));
+    RecordEventsReadableSpan span = (RecordEventsReadableSpan) spanBuilder.startSpan();
+    assertThat(span.toSpanData().getAttributes()).isEmpty();
+  }
+
   @Test
   public void droppingAttributes() {
     final int maxNumberOfAttrs = 8;