[RemoteCacheMetrics] Fix percentage bug and improve stats

This change

* fixes the percentage shown, it was showing the inverse value (cache miss % instead of cache hit %)
* adds absolute numbers for how many cache checks were performed and how many were misses
* filters out local actions that don't do remote cache checks, as these are not relevant

Contributes to #90

Signed-off-by: Sara Adams <sara.e.adams@gmail.com>

analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/LocalActions.java

@@ -2,6 +2,7 @@
 
 import static com.engflow.bazel.invocation.analyzer.bazelprofile.BazelProfileConstants.CAT_GENERAL_INFORMATION;
 import static com.engflow.bazel.invocation.analyzer.bazelprofile.BazelProfileConstants.CAT_LOCAL_ACTION_EXECUTION;
+import static com.engflow.bazel.invocation.analyzer.bazelprofile.BazelProfileConstants.CAT_REMOTE_ACTION_CACHE_CHECK;
 import static com.engflow.bazel.invocation.analyzer.bazelprofile.BazelProfileConstants.COMPLETE_SUBPROCESS_RUN;
 
 import com.engflow.bazel.invocation.analyzer.core.Datum;
@@ -105,12 +106,15 @@ public static class LocalAction implements Comparable<LocalAction> {
 
     private final CompleteEvent action;
     private final ImmutableList<CompleteEvent> relatedEvents;
+
+    private final boolean checksRemoteCache;
     private final boolean executedLocally;
 
     @VisibleForTesting
     public LocalAction(CompleteEvent action, List<CompleteEvent> relatedEvents) {
       this.action = action;
       this.relatedEvents = ImmutableList.copyOf(relatedEvents);
+      this.checksRemoteCache = relatedEvents.stream().anyMatch(e -> checksRemoteCache(e));
       this.executedLocally = relatedEvents.stream().anyMatch(e -> indicatesLocalExecution(e));
     }
 
@@ -122,6 +126,10 @@ public List<CompleteEvent> getRelatedEvents() {
       return relatedEvents;
     }
 
+    public boolean hasRemoteCacheCheck() {
+      return checksRemoteCache;
+    }
+
     public boolean isExecutedLocally() {
       return executedLocally;
     }
@@ -167,6 +175,10 @@ public int compareTo(LocalAction o) {
       return action.start.compareTo(o.action.start);
     }
 
+    public static boolean checksRemoteCache(CompleteEvent event) {
+      return CAT_REMOTE_ACTION_CACHE_CHECK.equals(event.category);
+    }
+
     public static boolean indicatesLocalExecution(CompleteEvent event) {
       return CAT_LOCAL_ACTION_EXECUTION.equals(event.category)
           || CAT_GENERAL_INFORMATION.equals(event.category)

analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/RemoteCacheMetrics.java

@@ -4,6 +4,7 @@
 import com.engflow.bazel.invocation.analyzer.time.DurationUtil;
 import com.google.common.base.Objects;
 import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
 import java.time.Duration;
 
 /**
@@ -12,30 +13,37 @@
  */
 public class RemoteCacheMetrics implements Datum {
 
-  private final Duration totalCacheCheck;
-  private final Duration totalDownloadOutputs;
-  private final Duration totalUploadOutputs;
+  private final int cacheChecks;
+  private final int cacheMisses;
+  private final Duration cacheCheckDuration;
+  private final Duration downloadOutputsDuration;
+  private final Duration uploadOutputsDuration;
 
   private final float percentCachedRemotely;
 
   RemoteCacheMetrics() {
-    this(Duration.ZERO, Duration.ZERO, Duration.ZERO, 0f);
+    this(0, 0, Duration.ZERO, Duration.ZERO, Duration.ZERO);
   }
 
   RemoteCacheMetrics(
-      Duration totalCacheCheck,
-      Duration totalDownloadOutputs,
-      Duration totalUploadOutputs,
-      float percentCachedRemotely) {
-    this.totalCacheCheck = Preconditions.checkNotNull(totalCacheCheck);
-    this.totalDownloadOutputs = Preconditions.checkNotNull(totalDownloadOutputs);
-    this.totalUploadOutputs = Preconditions.checkNotNull(totalUploadOutputs);
-    this.percentCachedRemotely = percentCachedRemotely;
+      int cacheChecks,
+      int cacheMisses,
+      Duration totalCacheCheckDuration,
+      Duration downloadOutputsDuration,
+      Duration totalUploadOutputs) {
+    this.cacheChecks = cacheChecks;
+    this.cacheMisses = cacheMisses;
+    this.percentCachedRemotely = 100f * (cacheChecks - cacheMisses) / cacheChecks;
+    this.cacheCheckDuration = Preconditions.checkNotNull(totalCacheCheckDuration);
+    this.downloadOutputsDuration = Preconditions.checkNotNull(downloadOutputsDuration);
+    this.uploadOutputsDuration = Preconditions.checkNotNull(totalUploadOutputs);
   }
 
   @Override
   public boolean isEmpty() {
-    return totalCacheCheck.isZero() && totalDownloadOutputs.isZero() && totalUploadOutputs.isZero();
+    return cacheCheckDuration.isZero()
+        && downloadOutputsDuration.isZero()
+        && uploadOutputsDuration.isZero();
   }
 
   @Override
@@ -52,11 +60,11 @@ public String getDescription() {
   public String toString() {
     return "RemoteCacheMetrics{"
         + "totalCacheCheck="
-        + totalCacheCheck
+        + cacheCheckDuration
         + ", totalDownloadOutputs="
-        + totalDownloadOutputs
+        + downloadOutputsDuration
         + ", totalUploadOutputs="
-        + totalUploadOutputs
+        + uploadOutputsDuration
         + ", percentCached="
         + percentCachedRemotely
         + '}';
@@ -72,27 +80,33 @@ public boolean equals(Object o) {
     }
     RemoteCacheMetrics that = (RemoteCacheMetrics) o;
     return Float.compare(that.percentCachedRemotely, percentCachedRemotely) == 0
-        && Objects.equal(totalCacheCheck, that.totalCacheCheck)
-        && Objects.equal(totalDownloadOutputs, that.totalDownloadOutputs)
-        && Objects.equal(totalUploadOutputs, that.totalUploadOutputs);
+        && Objects.equal(cacheCheckDuration, that.cacheCheckDuration)
+        && Objects.equal(downloadOutputsDuration, that.downloadOutputsDuration)
+        && Objects.equal(uploadOutputsDuration, that.uploadOutputsDuration);
   }
 
   @Override
   public int hashCode() {
     return Objects.hashCode(
-        totalCacheCheck, totalDownloadOutputs, totalUploadOutputs, percentCachedRemotely);
+        cacheCheckDuration, downloadOutputsDuration, uploadOutputsDuration, percentCachedRemotely);
   }
 
   @Override
   public String getSummary() {
+    String formattedPercentage = String.format("%,.2f%%", percentCachedRemotely);
+    var width = Math.max(formattedPercentage.length(), String.valueOf(cacheChecks).length());
     return String.format(
-        "Total Remote Cache Check Duration: %s\n"
-            + "Total Remote Download Outputs:     %s\n"
-            + "Total Remote Upload Outputs:       %s\n"
-            + "Percent cached remotely:           %,.2f%%",
-        DurationUtil.formatDuration(totalCacheCheck),
-        DurationUtil.formatDuration(totalDownloadOutputs),
-        DurationUtil.formatDuration(totalUploadOutputs),
-        percentCachedRemotely);
+        "Number of cache checks:             %s\n"
+            + "Number of cache misses:             %s\n"
+            + "Cache hit percentage:               %s\n"
+            + "Time spend checking for cache hits: %s\n"
+            + "Time spend downloading outputs:     %s\n"
+            + "Time spend uploading outputs:       %s\n",
+        Strings.padStart(String.valueOf(cacheChecks), width, ' '),
+        Strings.padStart(String.valueOf(cacheMisses), width, ' '),
+        Strings.padStart(formattedPercentage, width, ' '),
+        DurationUtil.formatDuration(cacheCheckDuration),
+        DurationUtil.formatDuration(downloadOutputsDuration),
+        DurationUtil.formatDuration(uploadOutputsDuration));
   }
 }

analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/RemoteCacheMetricsDataProvider.java

@@ -31,15 +31,14 @@ public List<DatumSupplierSpecification<?>> getSuppliers() {
   RemoteCacheMetrics derive()
       throws InvalidProfileException, MissingInputException, NullDatumException {
     var metrics =
-        getDataManager().getDatum(LocalActions.class).parallelStream()
+        getDataManager().getDatum(LocalActions.class).stream()
+            .filter(action -> action.hasRemoteCacheCheck())
+            .parallel()
             .map(this::coalesce)
             .collect(Collectors.toList());
     var summary = metrics.stream().reduce(RemoteCacheData.EMPTY, RemoteCacheData::plus);
     return new RemoteCacheMetrics(
-        summary.check,
-        summary.download,
-        summary.upload,
-        ((float) summary.uncached / metrics.size()) * 100f);
+        metrics.size(), summary.uncached, summary.check, summary.download, summary.upload);
   }
 
   RemoteCacheData coalesce(LocalAction action) {

analyzer/javatests/com/engflow/bazel/invocation/analyzer/dataproviders/RemoteCacheMetricsDataProviderTest.java

@@ -78,9 +78,10 @@ public void summarizeCacheData()
     Truth.assertThat(provider.derive())
         .isEqualTo(
             new RemoteCacheMetrics(
+                3,
+                1,
                 Duration.ofSeconds(1 + 4 + 16),
                 Duration.ofSeconds(2 + 8),
-                Duration.ofSeconds(32),
-                25.0f));
+                Duration.ofSeconds(32)));
   }
 }