Refactored telemetry into separate class. Fixed review comments.

Author: AlexeyKuznetsov-DD

dd-java-agent/src/main/java/datadog/trace/bootstrap/BootstrapInitializationTelemetry.java

@@ -1,12 +1,14 @@
 package datadog.trace.bootstrap;
 
+import static java.util.Collections.emptyList;
+import static java.util.Collections.singletonList;
+
 import datadog.json.JsonWriter;
 import datadog.trace.bootstrap.environment.EnvironmentVariables;
 import de.thetaphi.forbiddenapis.SuppressForbidden;
 import java.io.Closeable;
 import java.io.OutputStream;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -93,37 +95,31 @@ public void finish() {}
   public static final class JsonBased extends BootstrapInitializationTelemetry {
     private final JsonSender sender;
 
-    private final Map<String, String> meta;
-    private final Map<String, List<String>> points;
+    private final Telemetry telemetry;
 
     // one way false to true
     private volatile boolean incomplete = false;
 
     JsonBased(JsonSender sender) {
       this.sender = sender;
-      this.meta = new LinkedHashMap<>();
-      this.points = new LinkedHashMap<>();
-
-      setMetaInfo("success", "success", "Successfully configured ddtrace package");
+      this.telemetry = new Telemetry();
     }
 
     @Override
     public void initMetaInfo(String attr, String value) {
-      synchronized (this.meta) {
-        this.meta.put(attr, value);
-      }
+      telemetry.setMetadata(attr, value);
     }
 
     @Override
     public void onAbort(String reasonCode) {
-      onPoint("library_entrypoint.abort", "reason:" + reasonCode);
+      onPoint("library_entrypoint.abort", singletonList("reason:" + reasonCode));
       markIncomplete();
-      setMetaInfo("abort", mapResultClass(reasonCode), reasonCode);
+      setResultMeta("abort", mapResultClass(reasonCode), reasonCode);
     }
 
     @Override
     public void onError(Throwable t) {
-      setMetaInfo("error", "internal_error", t.getMessage());
+      setResultMeta("error", "internal_error", t.getMessage());
 
       List<String> causes = new ArrayList<>();
 
@@ -141,13 +137,13 @@ public void onError(Throwable t) {
         causes = causes.subList(numCauses - maxTags, numCauses);
       }
 
-      onPoint("library_entrypoint.error", causes.toArray(new String[0]));
+      onPoint("library_entrypoint.error", causes);
     }
 
     @Override
     public void onError(String reasonCode) {
-      onPoint("library_entrypoint.error", "error_type:" + reasonCode);
-      setMetaInfo("error", mapResultClass(reasonCode), reasonCode);
+      onPoint("library_entrypoint.error", singletonList("error_type:" + reasonCode));
+      setResultMeta("error", mapResultClass(reasonCode), reasonCode);
     }
 
     private int maxTags() {
@@ -170,7 +166,7 @@ public void onFatalError(Throwable t) {
       markIncomplete();
     }
 
-    private void setMetaInfo(String result, String resultClass, String resultReason) {
+    private void setResultMeta(String result, String resultClass, String resultReason) {
       initMetaInfo("result", result);
       initMetaInfo("result_class", resultClass);
       initMetaInfo("result_reason", resultReason);
@@ -193,10 +189,8 @@ private String mapResultClass(String reasonCode) {
       }
     }
 
-    private void onPoint(String name, String... tags) {
-      synchronized (this.points) {
-        this.points.put(name, Arrays.asList(tags));
-      }
+    private void onPoint(String name, List<String> tags) {
+      telemetry.addPoint(name, tags);
     }
 
     @Override
@@ -207,15 +201,85 @@ public void markIncomplete() {
     @Override
     public void finish() {
       if (!this.incomplete) {
-        onPoint("library_entrypoint.complete");
+        onPoint("library_entrypoint.complete", emptyList());
       }
 
-      this.sender.send(meta, points);
+      this.sender.send(telemetry);
+    }
+  }
+
+  public static class Telemetry {
+    private final Map<String, String> metadata;
+    private final Map<String, List<String>> points;
+
+    public Telemetry() {
+      metadata = new LinkedHashMap<>();
+      points = new LinkedHashMap<>();
+
+      setResults("success", "success", "Successfully configured ddtrace package");
+    }
+
+    public void setMetadata(String name, String value) {
+      synchronized (metadata) {
+        metadata.put(name, value);
+      }
+    }
+
+    public void setResults(String result, String resultClass, String resultReason) {
+      synchronized (metadata) {
+        metadata.put("result", result);
+        metadata.put("result_class", resultClass);
+        metadata.put("result_reason", resultReason);
+      }
+    }
+
+    public void addPoint(String name, List<String> tags) {
+      synchronized (points) {
+        points.put(name, tags);
+      }
+    }
+
+    public byte[] json() {
+      try (JsonWriter writer = new JsonWriter()) {
+        writer.beginObject();
+        writer.name("metadata").beginObject();
+        synchronized (metadata) {
+          for (Map.Entry<String, String> entry : metadata.entrySet()) {
+            writer.name(entry.getKey());
+            writer.value(entry.getValue());
+          }
+
+          metadata.clear();
+        }
+        writer.endObject();
+
+        writer.name("points").beginArray();
+        synchronized (points) {
+          for (Map.Entry<String, List<String>> entry : points.entrySet()) {
+            writer.beginObject();
+            writer.name("name").value(entry.getKey());
+            if (!entry.getValue().isEmpty()) {
+              writer.name("tags").beginArray();
+              for (String tag : entry.getValue()) {
+                writer.value(tag);
+              }
+              writer.endArray();
+            }
+            writer.endObject();
+          }
+
+          points.clear();
+        }
+        writer.endArray();
+        writer.endObject();
+
+        return writer.toByteArray();
+      }
     }
   }
 
   public interface JsonSender {
-    void send(Map<String, String> meta, Map<String, List<String>> points);
+    void send(Telemetry telemetry);
   }
 
   public static final class ForwarderJsonSender implements JsonSender {
@@ -226,24 +290,21 @@ public static final class ForwarderJsonSender implements JsonSender {
     }
 
     @Override
-    public void send(Map<String, String> meta, Map<String, List<String>> points) {
-      ForwarderJsonSenderThread t = new ForwarderJsonSenderThread(forwarderPath, meta, points);
+    public void send(Telemetry telemetry) {
+      ForwarderJsonSenderThread t = new ForwarderJsonSenderThread(forwarderPath, telemetry);
       t.setDaemon(true);
       t.start();
     }
   }
 
   public static final class ForwarderJsonSenderThread extends Thread {
     private final String forwarderPath;
-    private final Map<String, String> meta;
-    private final Map<String, List<String>> points;
+    private final Telemetry telemetry;
 
-    public ForwarderJsonSenderThread(
-        String forwarderPath, Map<String, String> meta, Map<String, List<String>> points) {
+    public ForwarderJsonSenderThread(String forwarderPath, Telemetry telemetry) {
       super("dd-forwarder-json-sender");
       this.forwarderPath = forwarderPath;
-      this.meta = meta;
-      this.points = points;
+      this.telemetry = telemetry;
     }
 
     @SuppressForbidden
@@ -253,7 +314,7 @@ public void run() {
 
       // Run forwarder and mute tracing for subprocesses executed in by dd-java-agent.
       try (final Closeable ignored = muteTracing()) {
-        byte[] payload = json();
+        byte[] payload = telemetry.json();
 
         Process process = builder.start();
         try (OutputStream out = process.getOutputStream()) {
@@ -265,39 +326,6 @@ public void run() {
       }
     }
 
-    private byte[] json() {
-      try (JsonWriter writer = new JsonWriter()) {
-        writer.beginObject();
-        writer.name("metadata").beginObject();
-        synchronized (this.meta) {
-          for (Map.Entry<String, String> entry : meta.entrySet()) {
-            writer.name(entry.getKey());
-            writer.value(entry.getValue());
-          }
-        }
-        writer.endObject();
-
-        writer.name("points").beginArray();
-        synchronized (this.points) {
-          for (Map.Entry<String, List<String>> entry : points.entrySet()) {
-            writer.beginObject();
-            writer.name("name").value(entry.getKey());
-            writer.name("tags").beginArray();
-            for (String tag : entry.getValue()) {
-              writer.value(tag);
-            }
-            writer.endArray();
-            writer.endObject();
-          }
-          this.points.clear();
-        }
-        writer.endArray();
-        writer.endObject();
-
-        return writer.toByteArray();
-      }
-    }
-
     @SuppressForbidden
     private Closeable muteTracing() {
       try {

dd-java-agent/src/test/groovy/datadog/trace/bootstrap/BootstrapInitializationTelemetryTest.groovy

@@ -1,7 +1,11 @@
 package datadog.trace.bootstrap
 
+import datadog.trace.bootstrap.BootstrapInitializationTelemetry.Telemetry
+import groovy.json.JsonBuilder
 import spock.lang.Specification
 
+import static java.nio.charset.StandardCharsets.UTF_8
+
 class BootstrapInitializationTelemetryTest extends Specification {
   Capture capture
   def initTelemetry
@@ -29,8 +33,7 @@ class BootstrapInitializationTelemetryTest extends Specification {
     initTelemetry.finish()
 
     then:
-    assertMeta("success", "success", "Successfully configured ddtrace package")
-    assertPoints(true, "library_entrypoint.complete", [])
+    assertJson("success", "success", "Successfully configured ddtrace package", [[name: "library_entrypoint.complete"]])
   }
 
   def "test non fatal error as text"() {
@@ -39,8 +42,10 @@ class BootstrapInitializationTelemetryTest extends Specification {
     initTelemetry.finish()
 
     then:
-    assertMeta("error", "unknown", "some reason")
-    assertPoints(true, "library_entrypoint.error", ["error_type:some reason"])
+    assertJson("error", "unknown", "some reason", [
+      [name: "library_entrypoint.error", tags: ["error_type:some reason"]],
+      [name: "library_entrypoint.complete"]
+    ])
   }
 
   def "test non fatal error as exception"() {
@@ -49,8 +54,10 @@ class BootstrapInitializationTelemetryTest extends Specification {
     initTelemetry.finish()
 
     then:
-    assertMeta("error", "internal_error", "non fatal error")
-    assertPoints(true, "library_entrypoint.error", ["error_type:java.lang.Exception"])
+    assertJson("error", "internal_error", "non fatal error", [
+      [name: "library_entrypoint.error", tags: ["error_type:java.lang.Exception"]],
+      [name: "library_entrypoint.complete"]
+    ])
   }
 
   def "test abort"() {
@@ -59,8 +66,7 @@ class BootstrapInitializationTelemetryTest extends Specification {
     initTelemetry.finish()
 
     then:
-    assertMeta("abort", resultClass, reasonCode)
-    assertPoints(false, "library_entrypoint.abort", ["reason:${reasonCode}"])
+    assertJson("abort", resultClass, reasonCode, [[name: "library_entrypoint.abort", tags: ["reason:${reasonCode}"]]])
 
     where:
     reasonCode            | resultClass
@@ -76,8 +82,7 @@ class BootstrapInitializationTelemetryTest extends Specification {
     initTelemetry.finish()
 
     then:
-    assertMeta("error", "internal_error", "fatal error")
-    assertPoints(false, "library_entrypoint.error", ["error_type:java.lang.Exception"])
+    assertJson("error", "internal_error", "fatal error", [[name: "library_entrypoint.error", tags: ["error_type:java.lang.Exception"]]])
   }
 
   def "test unwind root cause"() {
@@ -86,36 +91,30 @@ class BootstrapInitializationTelemetryTest extends Specification {
     initTelemetry.finish()
 
     then:
-    assertMeta("error", "internal_error", "top cause")
-    assertPoints(true, "library_entrypoint.error", ["error_type:java.io.FileNotFoundException", "error_type:java.lang.Exception"])
-  }
-
-  def assertMeta(String result, String resultClass, String resultReason) {
-    def meta = capture.meta
-
-    assert meta.get("result") == result
-    assert meta.get("result_class") == resultClass
-    assert meta.get("result_reason") == resultReason
-
-    return true
+    assertJson("error", "internal_error", "top cause", [
+      [name: "library_entrypoint.error", tags: ["error_type:java.io.FileNotFoundException", "error_type:java.lang.Exception"]],
+      [name: "library_entrypoint.complete"]
+    ])
   }
 
-  def assertPoints(boolean complete, String point, List<String> tags) {
-    def points = capture.points
+  private boolean assertJson(String result, String resultClass, String resultReason, List points) {
+    def expectedJson = """{"metadata":{"result":"${result}","result_class":"${resultClass}","result_reason":"${resultReason}"},"points":${new JsonBuilder(points)}}"""
+    def actualJson = capture.json()
 
-    assert points.containsKey("library_entrypoint.complete") == complete
-    assert points.get(point) == tags
+    assert expectedJson == actualJson
 
     return true
   }
 
   static class Capture implements BootstrapInitializationTelemetry.JsonSender {
-    Map<String, String> meta
-    Map<String, List<String>> points
+    Telemetry telemetry
+
+    void send(Telemetry telemetry) {
+      this.telemetry = telemetry
+    }
 
-    void send(Map<String, String> meta, Map<String, List<String>> points) {
-      this.meta = meta
-      this.points = points
+    String json() {
+      return new String(telemetry.json(), UTF_8)
     }
   }
 }