From b16226bc57e9b7ee7b9ca8f337ef54944d380de3 Mon Sep 17 00:00:00 2001
From: Carter Kozak <ckozak@ckozak.net>
Date: Wed, 21 Apr 2021 08:47:14 -0400
Subject: [PATCH] Tracer gracefully handles null tags (#718)

Tracer gracefully handles null tags. Not a problem, but will be helpful once we start adding more standard http tags from nullable sources.
---
 changelog/@unreleased/pr-718.v2.yml           |  5 +++
 .../com/palantir/tracing/TagTranslator.java   |  5 ++-
 .../java/com/palantir/tracing/Tracer.java     |  4 ++-
 .../java/com/palantir/tracing/TracerTest.java | 31 +++++++++++++++++++
 4 files changed, 41 insertions(+), 4 deletions(-)
 create mode 100644 changelog/@unreleased/pr-718.v2.yml

diff --git a/changelog/@unreleased/pr-718.v2.yml b/changelog/@unreleased/pr-718.v2.yml
new file mode 100644
index 000000000..7145d0c29
--- /dev/null
+++ b/changelog/@unreleased/pr-718.v2.yml
@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: Tracer gracefully handles null tags. Not a problem, but will be helpful once we start adding more standard http tags from nullable sources.
+  links:
+  - https://github.com/palantir/tracing-java/pull/718
diff --git a/tracing/src/main/java/com/palantir/tracing/TagTranslator.java b/tracing/src/main/java/com/palantir/tracing/TagTranslator.java
index e3eb41aa8..1c2b1d523 100644
--- a/tracing/src/main/java/com/palantir/tracing/TagTranslator.java
+++ b/tracing/src/main/java/com/palantir/tracing/TagTranslator.java
@@ -59,10 +59,9 @@ default TagTranslator<S> andThen(TagTranslator<? super S> after) {
         return new CompositeTagTranslator<>(this, after);
     }
 
-    /**
-     * Tag adapter object which insulates the implementation of the underlying data structure from callers.
-     */
+    /** Tag adapter object which insulates the implementation of the underlying data structure from callers. */
     interface TagAdapter<T> {
+        /** No information will be recorded if either {@code key} or {@code value} are {@code null}. */
         void tag(T target, String key, String value);
 
         void tag(T target, Map<String, String> tags);
diff --git a/tracing/src/main/java/com/palantir/tracing/Tracer.java b/tracing/src/main/java/com/palantir/tracing/Tracer.java
index f79ccc08e..bf3f80064 100644
--- a/tracing/src/main/java/com/palantir/tracing/Tracer.java
+++ b/tracing/src/main/java/com/palantir/tracing/Tracer.java
@@ -604,7 +604,9 @@ private enum SpanBuilderTagAdapter implements TagTranslator.TagAdapter<Span.Buil
 
         @Override
         public void tag(Span.Builder target, String key, String value) {
-            target.putMetadata(key, value);
+            if (key != null && value != null) {
+                target.putMetadata(key, value);
+            }
         }
 
         @Override
diff --git a/tracing/src/test/java/com/palantir/tracing/TracerTest.java b/tracing/src/test/java/com/palantir/tracing/TracerTest.java
index e74022dd4..99254cbb0 100644
--- a/tracing/src/test/java/com/palantir/tracing/TracerTest.java
+++ b/tracing/src/test/java/com/palantir/tracing/TracerTest.java
@@ -58,6 +58,12 @@ public final class TracerTest {
     @Captor
     private ArgumentCaptor<Span> spanCaptor;
 
+    @After
+    public void before() {
+        Tracer.getAndClearTraceIfPresent();
+        Tracer.setSampler(AlwaysSampler.INSTANCE);
+    }
+
     @After
     public void after() {
         Tracer.initTraceWithSpan(Observability.SAMPLE, Tracers.randomId(), "op", SpanType.LOCAL);
@@ -259,6 +265,31 @@ public void testCompleteSpanWithMetadataIncludesMetadata() {
         assertThat(maybeSpan.get().getMetadata()).isEqualTo(metadata);
     }
 
+    @Test
+    public void testMetadataNullIgnored() {
+        Tracer.subscribe("1", observer1);
+        try {
+            Tracer.fastStartSpan("operation");
+            Tracer.fastCompleteSpan(
+                    new TagTranslator<String>() {
+                        @Override
+                        public <T> void translate(TagAdapter<T> adapter, T target, String _data) {
+                            adapter.tag(target, "foo", null);
+                            adapter.tag(target, null, "bar");
+                            adapter.tag(target, "baz", "bang");
+                        }
+                    },
+                    "str");
+        } finally {
+            Tracer.unsubscribe("1");
+        }
+
+        verify(observer1).consume(spanCaptor.capture());
+        Span span = spanCaptor.getValue();
+        assertThat(span.getOperation()).isEqualTo("operation");
+        assertThat(span.getMetadata()).isEqualTo(ImmutableMap.of("baz", "bang"));
+    }
+
     @Test
     public void testCompleteSpanWithoutMetadataHasNoMetadata() {
         assertThat(startAndCompleteSpan().getMetadata()).isEmpty();