json: fixes incorrect handling of error tag (#1415)

Signed-off-by: Adrian Cole <[email protected]>
openzipkin · Feb 13, 2024 · 7158ba2 · 7158ba2
1 parent 665e3e8
commit 7158ba2
Show file tree

Hide file tree

Showing 5 changed files with 97 additions and 18 deletions.
diff --git a/brave/src/main/java/brave/handler/MutableSpanBytesEncoder.java b/brave/src/main/java/brave/handler/MutableSpanBytesEncoder.java
@@ -22,6 +22,12 @@
 /** Similar to {@code zipkin2.MutableSpan.SpanBytesEncoder} except no Zipkin dependency. */
 public abstract class MutableSpanBytesEncoder {
 
+  /**
+   * Encodes a {@linkplain MutableSpan} into Zipkin's V2 json format.
+   *
+   * @param errorTag sets the tag for a {@linkplain MutableSpan#error()}, if the corresponding key
+   *                 doesn't already exist.
+   */
   public static MutableSpanBytesEncoder zipkinJsonV2(Tag<Throwable> errorTag) {
     if (errorTag == null) throw new NullPointerException("errorTag == null");
     return new ZipkinJsonV2(errorTag);

diff --git a/brave/src/main/java/brave/internal/codec/ZipkinV2JsonWriter.java b/brave/src/main/java/brave/internal/codec/ZipkinV2JsonWriter.java
@@ -88,19 +88,19 @@ public ZipkinV2JsonWriter(Tag<Throwable> errorTag) {
     }
     int tagCount = span.tagCount();
     String errorValue = errorTag.value(span.error(), null);
-    if (tagCount > 0 || errorValue != null) {
+    String errorTagName = errorValue != null ? errorTag.key() : null;
+    boolean writeError = errorTagName != null;
+    if (tagCount > 0 || writeError) {
       if (sizeInBytes > 1) sizeInBytes++; // ,
       sizeInBytes += 9; // "tags":{}
-      boolean foundError = false;
       for (int i = 0; i < tagCount; i++) {
         String key = span.tagKeyAt(i);
-        if (!foundError && key.equals("error")) foundError = true;
-        String value = span.tagValueAt(i);
-        sizeInBytes += tagSizeInBytes(key, value);
+        if (writeError && key.equals(errorTagName)) writeError = false;
+        sizeInBytes += tagSizeInBytes(key, span.tagValueAt(i));
       }
-      if (errorValue != null && !foundError) {
+      if (writeError) {
         tagCount++;
-        sizeInBytes += tagSizeInBytes(errorTag.key(), errorValue);
+        sizeInBytes += tagSizeInBytes(errorTagName, errorValue);
       }
       if (tagCount > 1) sizeInBytes += tagCount - 1; // comma to join elements
     }
@@ -179,20 +179,20 @@ public ZipkinV2JsonWriter(Tag<Throwable> errorTag) {
     }
     int tagCount = span.tagCount();
     String errorValue = errorTag.value(span.error(), null);
-    if (tagCount > 0 || errorValue != null) {
+    String errorTagName = errorValue != null ? errorTag.key() : null;
+    boolean writeError = errorTagName != null;
+    if (tagCount > 0 || writeError) {
       wroteField = writeFieldBegin(b, "tags", wroteField);
       b.writeByte('{');
-      boolean foundError = false;
       for (int i = 0; i < tagCount; ) {
         String key = span.tagKeyAt(i);
-        if (!foundError && key.equals("error")) foundError = true;
-        String value = span.tagValueAt(i);
-        writeKeyValue(b, key, value);
+        if (writeError && key.equals(errorTagName)) writeError = false;
+        writeKeyValue(b, key, span.tagValueAt(i));
         if (++i < tagCount) b.writeByte(',');
       }
-      if (errorValue != null && !foundError) {
+      if (writeError) {
         if (tagCount > 0) b.writeByte(',');
-        writeKeyValue(b, errorTag.key(), errorValue);
+        writeKeyValue(b, errorTagName, errorValue);
       }
       b.writeByte('}');
     }

diff --git a/...nal/codec/ZipkinJsonV2JsonWriterTest.java → ...nternal/codec/ZipkinV2JsonWriterTest.java b/...nal/codec/ZipkinJsonV2JsonWriterTest.java → ...nternal/codec/ZipkinV2JsonWriterTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2013-2023 The OpenZipkin Authors
+ * Copyright 2013-2024 The OpenZipkin Authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
  * in compliance with the License. You may obtain a copy of the License at
@@ -14,16 +14,18 @@
 package brave.internal.codec;
 
 import brave.Span;
+import brave.Tag;
 import brave.Tags;
 import brave.handler.MutableSpan;
 import brave.handler.MutableSpanTest;
+import brave.propagation.TraceContext;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.assertj.core.api.Assertions.assertThat;
 
-class ZipkinJsonV2JsonWriterTest {
+class ZipkinV2JsonWriterTest {
   ZipkinV2JsonWriter jsonWriter = new ZipkinV2JsonWriter(Tags.ERROR);
   WriteBuffer buffer = new WriteBuffer(new byte[512], 0);
 
@@ -81,6 +83,75 @@ class ZipkinJsonV2JsonWriterTest {
       .isEqualTo(string.getBytes(UTF_8).length);
   }
 
+  @Test void errorTag() {
+    MutableSpan span = new MutableSpan();
+    span.tag("a", "1");
+    span.tag("error", "true");
+    span.tag("b", "2");
+
+    jsonWriter.write(span, buffer);
+    String string = buffer.toString();
+    assertThat(string)
+      .isEqualTo("{\"tags\":{\"a\":\"1\",\"error\":\"true\",\"b\":\"2\"}}");
+
+    assertThat(jsonWriter.sizeInBytes(span))
+      .isEqualTo(string.getBytes(UTF_8).length);
+  }
+
+  @Test void error() {
+    MutableSpan span = new MutableSpan();
+    span.tag("a", "1");
+    span.tag("b", "2");
+    span.error(new RuntimeException("ice cream"));
+
+    jsonWriter.write(span, buffer);
+    String string = buffer.toString();
+    assertThat(string)
+      .isEqualTo("{\"tags\":{\"a\":\"1\",\"b\":\"2\",\"error\":\"ice cream\"}}");
+
+    assertThat(jsonWriter.sizeInBytes(span))
+      .isEqualTo(string.getBytes(UTF_8).length);
+  }
+
+  @Test void existingErrorTagWins() {
+    MutableSpan span = new MutableSpan();
+    span.tag("a", "1");
+    span.tag("error", "true");
+    span.tag("b", "2");
+    span.error(new RuntimeException("ice cream"));
+
+    jsonWriter.write(span, buffer);
+    String string = buffer.toString();
+    assertThat(string)
+      .isEqualTo("{\"tags\":{\"a\":\"1\",\"error\":\"true\",\"b\":\"2\"}}");
+
+    assertThat(jsonWriter.sizeInBytes(span))
+      .isEqualTo(string.getBytes(UTF_8).length);
+  }
+
+  @Test void differentErrorTagName() {
+    ZipkinV2JsonWriter jsonWriter = new ZipkinV2JsonWriter(new Tag<Throwable>("exception") {
+      @Override protected String parseValue(Throwable input, TraceContext context) {
+        return input.getMessage();
+      }
+    });
+
+    MutableSpan span = new MutableSpan();
+    span.tag("a", "1");
+    span.tag("error", "true");
+    span.tag("b", "2");
+    span.error(new RuntimeException("ice cream"));
+
+    jsonWriter.write(span, buffer);
+    String string = buffer.toString();
+    assertThat(string)
+      .isEqualTo(
+        "{\"tags\":{\"a\":\"1\",\"error\":\"true\",\"b\":\"2\",\"exception\":\"ice cream\"}}");
+
+    assertThat(jsonWriter.sizeInBytes(span))
+      .isEqualTo(string.getBytes(UTF_8).length);
+  }
+
   @Test void missingFields_testCases() {
     jsonWriter.write(MutableSpanTest.PERMUTATIONS.get(0).get(), buffer);
     assertThat(buffer.toString()).isEqualTo("{}");

diff --git a/instrumentation/benchmarks/src/main/java/brave/handler/MutableSpanBenchmarks.java b/instrumentation/benchmarks/src/main/java/brave/handler/MutableSpanBenchmarks.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2013-2019 The OpenZipkin Authors
+ * Copyright 2013-2024 The OpenZipkin Authors
  *
  * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
  * in compliance with the License. You may obtain a copy of the License at
@@ -76,9 +76,11 @@ public static MutableSpan newBigClientMutableSpan() {
     span.tag("http.path", "/thrift/shopForTalk");
     span.tag("http.status_code", "200");
     span.tag("http.url", "tbinary+h2c://abasdasgad.hsadas.ism/thrift/shopForTalk");
+    span.tag("error", "true");
     span.tag("instanceId", "line-wallet-api");
     span.tag("phase", "beta");
     span.tag("siteId", "shop");
+    span.error(new RuntimeException("ice cream"));
     return span;
   }
 

diff --git a/...mentation/benchmarks/src/main/java/brave/internal/codec/ZipkinV2JsonWriterBenchmarks.java b/...mentation/benchmarks/src/main/java/brave/internal/codec/ZipkinV2JsonWriterBenchmarks.java
@@ -70,7 +70,7 @@ public class ZipkinV2JsonWriterBenchmarks {
   public static void main(String[] args) throws RunnerException {
     Options opt = new OptionsBuilder()
       .addProfiler("gc")
-      .include(".*" + ZipkinV2JsonWriter.class.getSimpleName() + ".*")
+      .include(".*" + ZipkinV2JsonWriter.class.getSimpleName())
       .build();
 
     new Runner(opt).run();