[Core] Avoid NPEs on creating events

When creating trace event format events, the code used to throw NullPointerExceptions when trying to construct an event from a JsonObject that was missing required members. This change adds static `fromJson` methods, which check the members exist, or otherwise throw an IllegalArgumentException. It also adds some basic tests for the Event classes. Signed-off-by: Sara Adams <sara.e.adams@gmail.com>
EngFlow · May 23, 2024 · 877d45f · 877d45f
1 parent b441e93
commit 877d45f
Show file tree

Hide file tree

Showing 9 changed files with 396 additions and 22 deletions.
diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/ProfileThread.java
@@ -117,14 +117,14 @@ public boolean addEvent(JsonObject event) {
       switch (event.get(TraceEventFormatConstants.EVENT_PHASE).getAsString()) {
         case TraceEventFormatConstants.PHASE_COMPLETE: // Complete events
           {
-            completeEvents.add(new CompleteEvent(event));
+            completeEvents.add(CompleteEvent.fromJson(event));
             break;
           }
 
         case "I": // Deprecated, fall-through
         case TraceEventFormatConstants.PHASE_INSTANT: // Instant events
           {
-            InstantEvent instantEvent = new InstantEvent(event);
+            InstantEvent instantEvent = InstantEvent.fromJson(event);
 
             List<InstantEvent> instantList =
                 instants.compute(
@@ -142,7 +142,7 @@ public boolean addEvent(JsonObject event) {
 
         case TraceEventFormatConstants.PHASE_COUNTER: // Counter events
           {
-            CounterEvent counterEvent = new CounterEvent(event);
+            CounterEvent counterEvent = CounterEvent.fromJson(event);
 
             List<CounterEvent> countList =
                 counts.compute(

diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/traceeventformat/CompleteEvent.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/traceeventformat/CompleteEvent.java
@@ -18,29 +18,91 @@
 
 import com.engflow.bazel.invocation.analyzer.time.TimeUtil;
 import com.engflow.bazel.invocation.analyzer.time.Timestamp;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
 import com.google.gson.JsonObject;
 import java.time.Duration;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 
+/**
+ * Complete events describe an event with a duration.
+ *
+ * @see <a
+ *     https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview#heading=h.lpfof2aylapb">specification</a>
+ */
 public class CompleteEvent {
+  @VisibleForTesting
+  static final List<String> REQUIRED_JSON_MEMBERS =
+      List.of(
+          TraceEventFormatConstants.EVENT_TIMESTAMP,
+          TraceEventFormatConstants.EVENT_DURATION,
+          TraceEventFormatConstants.EVENT_THREAD_ID,
+          TraceEventFormatConstants.EVENT_PROCESS_ID);
+
   @Nullable public final String name;
   @Nullable public final String category;
   public final Timestamp start;
   public final Duration duration;
   public final Timestamp end;
   public final int threadId;
   public final int processId;
-  public final ImmutableMap<String, String> args;
+  public final Map<String, String> args;
+
+  public static CompleteEvent fromJson(JsonObject object) {
+    Preconditions.checkNotNull(object);
+    List<String> missingMembers = Lists.newArrayList();
+    for (String requiredMember : REQUIRED_JSON_MEMBERS) {
+      if (!object.has(requiredMember)) {
+        missingMembers.add(requiredMember);
+      }
+    }
+    if (!missingMembers.isEmpty()) {
+      throw new IllegalArgumentException(
+          "Missing members: " + Arrays.toString(missingMembers.toArray()));
+    }
+
+    return new CompleteEvent(
+        object.has(TraceEventFormatConstants.EVENT_NAME)
+            ? object.get(TraceEventFormatConstants.EVENT_NAME).getAsString()
+            : null,
+        object.has(TraceEventFormatConstants.EVENT_CATEGORY)
+            ? object.get(TraceEventFormatConstants.EVENT_CATEGORY).getAsString()
+            : null,
+        Timestamp.ofMicros(object.get(TraceEventFormatConstants.EVENT_TIMESTAMP).getAsLong()),
+        TimeUtil.getDurationForMicros(
+            object.get(TraceEventFormatConstants.EVENT_DURATION).getAsLong()),
+        object.get(TraceEventFormatConstants.EVENT_THREAD_ID).getAsInt(),
+        object.get(TraceEventFormatConstants.EVENT_PROCESS_ID).getAsInt(),
+        object.has(TraceEventFormatConstants.EVENT_ARGUMENTS)
+            ? object
+                .get(TraceEventFormatConstants.EVENT_ARGUMENTS)
+                .getAsJsonObject()
+                .entrySet()
+                .stream()
+                .collect(toImmutableMap(e -> e.getKey(), e -> e.getValue().getAsString()))
+            : ImmutableMap.of());
+  }
 
+  /**
+   * Parses a {@link CompleteEvent} from a JsonObject.
+   *
+   * @deprecated Use {@link #fromJson(JsonObject)} instead.
+   */
+  @Deprecated
   public CompleteEvent(JsonObject object) {
     this(
         object.has(TraceEventFormatConstants.EVENT_NAME)
-            ? object.get(TraceEventFormatConstants.EVENT_NAME).getAsString().intern()
+            ? object.get(TraceEventFormatConstants.EVENT_NAME).getAsString()
             : null,
         object.has(TraceEventFormatConstants.EVENT_CATEGORY)
-            ? object.get(TraceEventFormatConstants.EVENT_CATEGORY).getAsString().intern()
+            ? object.get(TraceEventFormatConstants.EVENT_CATEGORY).getAsString()
             : null,
         Timestamp.ofMicros(object.get(TraceEventFormatConstants.EVENT_TIMESTAMP).getAsLong()),
         TimeUtil.getDurationForMicros(
@@ -53,9 +115,7 @@ public CompleteEvent(JsonObject object) {
                 .getAsJsonObject()
                 .entrySet()
                 .stream()
-                .collect(
-                    toImmutableMap(
-                        e -> e.getKey().intern(), e -> e.getValue().getAsString().intern()))
+                .collect(Collectors.toMap(e -> e.getKey(), e -> e.getValue().getAsString()))
             : ImmutableMap.of());
   }
 
@@ -66,15 +126,18 @@ public CompleteEvent(
       Duration duration,
       int threadId,
       int processId,
-      ImmutableMap<String, String> args) {
-    this.name = name;
-    this.category = category;
+      Map<String, String> args) {
+    this.name = name == null ? null : name.intern();
+    this.category = category == null ? null : category.intern();
     this.start = start;
     this.duration = duration;
     this.end = start.plus(duration);
     this.threadId = threadId;
     this.processId = processId;
-    this.args = args;
+    this.args =
+        args.entrySet().stream()
+            .collect(
+                Collectors.toUnmodifiableMap(e -> e.getKey().intern(), e -> e.getValue().intern()));
   }
 
   @Override

diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/traceeventformat/CounterEvent.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/traceeventformat/CounterEvent.java
@@ -15,25 +15,78 @@
 package com.engflow.bazel.invocation.analyzer.traceeventformat;
 
 import com.engflow.bazel.invocation.analyzer.time.Timestamp;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Lists;
 import com.google.gson.JsonObject;
+import java.util.Arrays;
+import java.util.List;
 
+/**
+ * Counter events can track a value or multiple values as they change over time.
+ *
+ * @see <a
+ *     href="https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview#heading=h.msg3086636uq">specification</a>
+ */
 public class CounterEvent {
+  @VisibleForTesting
+  static final List<String> REQUIRED_JSON_MEMBERS =
+      List.of(
+          TraceEventFormatConstants.EVENT_NAME,
+          TraceEventFormatConstants.EVENT_TIMESTAMP,
+          TraceEventFormatConstants.EVENT_ARGUMENTS);
+
   private final String name;
   private final Timestamp timestamp;
   private final double totalValue;
 
-  public CounterEvent(JsonObject event) {
-    this.name = event.get(TraceEventFormatConstants.EVENT_NAME).getAsString().intern();
-    this.timestamp =
-        Timestamp.ofMicros(event.get(TraceEventFormatConstants.EVENT_TIMESTAMP).getAsLong());
+  public static CounterEvent fromJson(JsonObject event) {
+    List<String> missingMembers = Lists.newArrayList();
+    for (String requiredMember : REQUIRED_JSON_MEMBERS) {
+      if (!event.has(requiredMember)) {
+        missingMembers.add(requiredMember);
+      }
+    }
+    if (!missingMembers.isEmpty()) {
+      throw new IllegalArgumentException(
+          "Missing members: " + Arrays.toString(missingMembers.toArray()));
+    }
 
     // For now we are treating all the different counts as a single metric by summing them
     // together. In the future we may want to respect each count individually.
-    this.totalValue =
+    var totalValue =
         event.get(TraceEventFormatConstants.EVENT_ARGUMENTS).getAsJsonObject().entrySet().stream()
             .mapToDouble(e -> e.getValue().getAsDouble())
             .sum();
+
+    return new CounterEvent(
+        event.get(TraceEventFormatConstants.EVENT_NAME).getAsString(),
+        Timestamp.ofMicros(event.get(TraceEventFormatConstants.EVENT_TIMESTAMP).getAsLong()),
+        totalValue);
+  }
+
+  /**
+   * Parses a {@link CounterEvent} from a JsonObject.
+   *
+   * @deprecated Use {@link #fromJson(JsonObject)} instead.
+   */
+  @Deprecated
+  public CounterEvent(JsonObject event) {
+    this(
+        event.get(TraceEventFormatConstants.EVENT_NAME).getAsString(),
+        Timestamp.ofMicros(event.get(TraceEventFormatConstants.EVENT_TIMESTAMP).getAsLong()),
+        // For now we are treating all the different counts as a single metric by summing them
+        // together. In the future we may want to respect each count individually.
+        event.get(TraceEventFormatConstants.EVENT_ARGUMENTS).getAsJsonObject().entrySet().stream()
+            .mapToDouble(e -> e.getValue().getAsDouble())
+            .sum());
+  }
+
+  private CounterEvent(String name, Timestamp timestamp, double totalValue) {
+    this.name = Preconditions.checkNotNull(name).intern();
+    this.timestamp = Preconditions.checkNotNull(timestamp);
+    this.totalValue = totalValue;
   }
 
   public String getName() {

diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/traceeventformat/InstantEvent.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/traceeventformat/InstantEvent.java
@@ -15,19 +15,67 @@
 package com.engflow.bazel.invocation.analyzer.traceeventformat;
 
 import com.engflow.bazel.invocation.analyzer.time.Timestamp;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Lists;
 import com.google.gson.JsonObject;
+import java.util.Arrays;
+import java.util.List;
 
+/**
+ * Instant events describe an event that has no duration.
+ *
+ * @see <a
+ *     https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview#heading=h.lenwiilchoxp">specification</a>
+ */
 public class InstantEvent {
+  @VisibleForTesting
+  static final List<String> REQUIRED_JSON_MEMBERS =
+      List.of(
+          TraceEventFormatConstants.EVENT_CATEGORY,
+          TraceEventFormatConstants.EVENT_NAME,
+          TraceEventFormatConstants.EVENT_TIMESTAMP);
+
   private final String category;
   private final String name;
   private final Timestamp timestamp;
 
+  public static InstantEvent fromJson(JsonObject event) {
+    List<String> missingMembers = Lists.newArrayList();
+    for (String requiredMember : REQUIRED_JSON_MEMBERS) {
+      if (!event.has(requiredMember)) {
+        missingMembers.add(requiredMember);
+      }
+    }
+    if (!missingMembers.isEmpty()) {
+      throw new IllegalArgumentException(
+          "Missing members: " + Arrays.toString(missingMembers.toArray()));
+    }
+
+    return new InstantEvent(
+        event.get(TraceEventFormatConstants.EVENT_CATEGORY).getAsString(),
+        event.get(TraceEventFormatConstants.EVENT_NAME).getAsString(),
+        Timestamp.ofMicros(event.get(TraceEventFormatConstants.EVENT_TIMESTAMP).getAsLong()));
+  }
+
+  /**
+   * Parses a {@link InstantEvent} from a JsonObject.
+   *
+   * @deprecated Use {@link #fromJson(JsonObject)} instead.
+   */
+  @Deprecated
   public InstantEvent(JsonObject event) {
-    this.category = event.get(TraceEventFormatConstants.EVENT_CATEGORY).getAsString().intern();
-    this.name = event.get(TraceEventFormatConstants.EVENT_NAME).getAsString().intern();
-    this.timestamp =
-        Timestamp.ofMicros(event.get(TraceEventFormatConstants.EVENT_TIMESTAMP).getAsLong());
+    this(
+        event.get(TraceEventFormatConstants.EVENT_CATEGORY).getAsString(),
+        event.get(TraceEventFormatConstants.EVENT_NAME).getAsString(),
+        Timestamp.ofMicros(event.get(TraceEventFormatConstants.EVENT_TIMESTAMP).getAsLong()));
+  }
+
+  private InstantEvent(String category, String name, Timestamp timestamp) {
+    this.category = Preconditions.checkNotNull(category).intern();
+    this.name = Preconditions.checkNotNull(name).intern();
+    this.timestamp = Preconditions.checkNotNull(timestamp);
   }
 
   public String getCategory() {

diff --git a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/traceeventformat/BUILD b/analyzer/javatests/com/engflow/bazel/invocation/analyzer/traceeventformat/BUILD
@@ -0,0 +1,12 @@
+java_test(
+    name = "traceeventformat",
+    srcs = glob(["**/*.java"]),
+    test_class = "com.engflow.bazel.invocation.analyzer.traceeventformat.TraceEventFormatTestSuite",
+    deps = [
+        "//analyzer/java/com/engflow/bazel/invocation/analyzer/time",
+        "//analyzer/java/com/engflow/bazel/invocation/analyzer/traceeventformat",
+        "//third_party/gson",
+        "//third_party/junit",
+        "//third_party/truth",
+    ],
+)