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.
palantir · Apr 21, 2021 · b16226b · b16226b
1 parent afa62fc
commit b16226b
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 4 deletions.
diff --git a/changelog/@unreleased/pr-718.v2.yml 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
@@ -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
@@ -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
@@ -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();