From 73e671d8f18ce7b3f77fd38f17012a86a563ae68 Mon Sep 17 00:00:00 2001
From: Giovanni Liva <giovanni.liva@dynatrace.com>
Date: Fri, 20 Mar 2020 18:08:31 +0100
Subject: [PATCH] Revise null values for AttributeValue (#1026)

---
 .../sdk/trace/SpanBuilderSdk.java             | 25 ++++++++++++++-----
 .../sdk/trace/SpanBuilderSdkTest.java         | 23 +++++++++++++++++
 2 files changed, 42 insertions(+), 6 deletions(-)

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 1d53c3c943..345cb3b7e5 100644
--- a/sdk/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java
+++ b/sdk/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java
@@ -17,8 +17,6 @@
 package io.opentelemetry.sdk.trace;
 
 import io.opentelemetry.common.AttributeValue;
-import io.opentelemetry.common.AttributeValue.Type;
-import io.opentelemetry.internal.StringUtils;
 import io.opentelemetry.internal.Utils;
 import io.opentelemetry.sdk.common.Clock;
 import io.opentelemetry.sdk.common.InstrumentationLibraryInfo;
@@ -169,14 +167,29 @@ final class SpanBuilderSdk implements Span.Builder {
   @Override
   public Span.Builder setAttribute(String key, AttributeValue value) {
     Utils.checkNotNull(key, "key");
-    Utils.checkNotNull(value, "value");
-    if (value.getType() == Type.STRING && StringUtils.isNullOrEmpty(value.getStringValue())) {
-      return this;
+    if (isRemovedValue(value)) {
+      attributes.remove(key);
+    } else {
+      attributes.putAttribute(key, value);
     }
-    attributes.putAttribute(key, value);
     return this;
   }
 
+  private static boolean isRemovedValue(AttributeValue value) {
+    if (value == null) {
+      return true;
+    }
+    switch (value.getType()) {
+      case STRING:
+        return value.getStringValue() == null;
+      case BOOLEAN:
+      case LONG:
+      case DOUBLE:
+        return false;
+    }
+    return false;
+  }
+
   @Override
   public Span.Builder setStartTimestamp(long startTimestamp) {
     Utils.checkArgument(startTimestamp >= 0, "Negative startTimestamp");
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 8654488875..fc52f9973d 100644
--- a/sdk/src/test/java/io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java
+++ b/sdk/src/test/java/io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java
@@ -180,6 +180,29 @@ public class SpanBuilderSdkTest {
     spanBuilder.setAttribute("nullStringAttributeValue", AttributeValue.stringAttributeValue(null));
     spanBuilder.setAttribute("emptyStringAttributeValue", AttributeValue.stringAttributeValue(""));
     RecordEventsReadableSpan span = (RecordEventsReadableSpan) spanBuilder.startSpan();
+    assertThat(span.toSpanData().getAttributes().size()).isEqualTo(2);
+    spanBuilder.setAttribute("emptyString", (String) null);
+    spanBuilder.setAttribute("emptyStringAttributeValue", (String) null);
+    assertThat(span.toSpanData().getAttributes()).isEmpty();
+  }
+
+  @Test
+  public void setAttribute_nullAttributeValue() throws Exception {
+    Span.Builder spanBuilder = tracerSdk.spanBuilder(SPAN_NAME);
+    spanBuilder.setAttribute("emptyString", "");
+    spanBuilder.setAttribute("nullString", (AttributeValue) null);
+    spanBuilder.setAttribute("nullStringAttributeValue", AttributeValue.stringAttributeValue(null));
+    spanBuilder.setAttribute("emptyStringAttributeValue", AttributeValue.stringAttributeValue(""));
+    spanBuilder.setAttribute("longAttribute", 0L);
+    spanBuilder.setAttribute("boolAttribute", false);
+    spanBuilder.setAttribute("doubleAttribute", 0.12345f);
+    RecordEventsReadableSpan span = (RecordEventsReadableSpan) spanBuilder.startSpan();
+    assertThat(span.toSpanData().getAttributes().size()).isEqualTo(5);
+    spanBuilder.setAttribute("emptyString", (AttributeValue) null);
+    spanBuilder.setAttribute("emptyStringAttributeValue", (AttributeValue) null);
+    spanBuilder.setAttribute("longAttribute", (AttributeValue) null);
+    spanBuilder.setAttribute("boolAttribute", (AttributeValue) null);
+    spanBuilder.setAttribute("doubleAttribute", (AttributeValue) null);
     assertThat(span.toSpanData().getAttributes()).isEmpty();
   }