[Refactor] Extract common methods: summary for complete event, target…

… name missing info (#134)

This refactors some code, so that the way action processing events are
printed out consistently, rather than having duplicate code.
It also refactors the code, so it's easy to add caveats relating to a
profile not having the target names attached to actions.

Contributes to #133

---------

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

analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BUILD

@@ -38,6 +38,8 @@ java_library(
     visibility = ["//visibility:public"],
     deps = [
         ":types",
+        "//analyzer/java/com/engflow/bazel/invocation/analyzer/time",
         "//analyzer/java/com/engflow/bazel/invocation/analyzer/traceeventformat",
+        "//third_party/guava",
     ],
 )

analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelEventsUtil.java

@@ -23,8 +23,10 @@
 import static com.engflow.bazel.invocation.analyzer.bazelprofile.BazelProfileConstants.COMPLETE_REMOTE_EXECUTION_UPLOAD_TIME_UPLOAD;
 import static com.engflow.bazel.invocation.analyzer.bazelprofile.BazelProfileConstants.COMPLETE_REMOTE_EXECUTION_UPLOAD_TIME_UPLOAD_OUTPUTS;
 import static com.engflow.bazel.invocation.analyzer.bazelprofile.BazelProfileConstants.COMPLETE_SUBPROCESS_RUN;
+import static com.engflow.bazel.invocation.analyzer.time.DurationUtil.formatDuration;
 
 import com.engflow.bazel.invocation.analyzer.traceeventformat.CompleteEvent;
+import com.google.common.base.Strings;
 
 public final class BazelEventsUtil {
   private BazelEventsUtil() {}
@@ -73,4 +75,26 @@ public static boolean indicatesRemoteUploadOutputs(CompleteEvent event) {
         && (COMPLETE_REMOTE_EXECUTION_UPLOAD_TIME_UPLOAD_OUTPUTS.equals(event.name)
             || COMPLETE_REMOTE_EXECUTION_UPLOAD_TIME_UPLOAD.equals(event.name));
   }
+
+  public static String summarizeCompleteEvent(CompleteEvent event) {
+    var summary = new StringBuilder();
+    summary.append("\t- Action: \"");
+    summary.append(event.name);
+    summary.append("\"\n");
+    var forTarget = event.args.get(BazelProfileConstants.ARGS_CAT_ACTION_PROCESSING_TARGET);
+    if (!Strings.isNullOrEmpty(forTarget)) {
+      summary.append("\t\tTarget: \"");
+      summary.append(forTarget);
+      summary.append("\"\n");
+    }
+    var mnemonic = event.args.get(BazelProfileConstants.ARGS_CAT_ACTION_PROCESSING_MNEMONIC);
+    if (!Strings.isNullOrEmpty(mnemonic)) {
+      summary.append("\t\tMnemonic: \"");
+      summary.append(mnemonic);
+      summary.append("\"\n");
+    }
+    summary.append("\t\tAction duration: ");
+    summary.append(formatDuration(event.duration));
+    return summary.toString();
+  }
 }

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

@@ -15,6 +15,8 @@
 package com.engflow.bazel.invocation.analyzer.dataproviders;
 
 import com.engflow.bazel.invocation.analyzer.core.Datum;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
 import java.util.Objects;
 
 /**
@@ -49,6 +51,21 @@ public boolean isProfileIncludeTargetLabelEnabled() {
     return profileIncludeTargetLabelEnabled;
   }
 
+  /**
+   * Returns a statement to enable this flag, given the cause.
+   *
+   * <p>The {@param useCase} should be of the shape, so that it fits into: {@code String.format("It
+   * can help with %s.", useCase)}
+   */
+  public static String getNotSetButUsefulForStatement(String useCase) {
+    Preconditions.checkArgument(!Strings.isNullOrEmpty(useCase));
+    return String.format(
+        "The profile does not include which target each action was processed for,"
+            + " although that data can help with %s. It is added to the profile by using the"
+            + " Bazel flag `%s`. Also see %s",
+        useCase, FLAG_NAME, COMMAND_LINE_REFERENCE_URL);
+  }
+
   @Override
   public String getDescription() {
     return "Whether the Bazel flag `--experimental_profile_include_target_label` was enabled when"

analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/BUILD

@@ -4,6 +4,7 @@ java_library(
     visibility = ["//visibility:public"],
     deps = [
         "//analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile:types",
+        "//analyzer/java/com/engflow/bazel/invocation/analyzer/bazelprofile:util",
         "//analyzer/java/com/engflow/bazel/invocation/analyzer/core",
         "//analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders:types",
         "//analyzer/java/com/engflow/bazel/invocation/analyzer/dataproviders/remoteexecution:types",

analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/BottleneckSuggestionProvider.java

@@ -21,7 +21,7 @@
 import com.engflow.bazel.invocation.analyzer.Suggestion;
 import com.engflow.bazel.invocation.analyzer.SuggestionCategory;
 import com.engflow.bazel.invocation.analyzer.SuggestionOutput;
-import com.engflow.bazel.invocation.analyzer.bazelprofile.BazelProfileConstants;
+import com.engflow.bazel.invocation.analyzer.bazelprofile.BazelEventsUtil;
 import com.engflow.bazel.invocation.analyzer.core.DataManager;
 import com.engflow.bazel.invocation.analyzer.core.MissingInputException;
 import com.engflow.bazel.invocation.analyzer.dataproviders.ActionStats;
@@ -32,7 +32,6 @@
 import com.engflow.bazel.invocation.analyzer.time.DurationUtil;
 import com.engflow.bazel.invocation.analyzer.traceeventformat.PartialCompleteEvent;
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Strings;
 import java.time.Duration;
 import java.util.ArrayList;
 import java.util.Comparator;
@@ -138,12 +137,8 @@ public SuggestionOutput getSuggestions(DataManager dataManager) {
           && !flagForTargetInclusionEnabled.isProfileIncludeTargetLabelEnabled()) {
         caveats.add(
             SuggestionProviderUtil.createCaveat(
-                String.format(
-                    "The profile does not include which target each action was processed for,"
-                        + " although that data can help with breaking down bottlenecks. It is added"
-                        + " to the profile by using the Bazel flag `%s`. Also see %s",
-                    FlagValueExperimentalProfileIncludeTargetLabel.FLAG_NAME,
-                    FlagValueExperimentalProfileIncludeTargetLabel.COMMAND_LINE_REFERENCE_URL),
+                FlagValueExperimentalProfileIncludeTargetLabel.getNotSetButUsefulForStatement(
+                    "breaking down bottlenecks"),
                 false));
       }
       if (suggestions.size() < bottlenecks.size()) {
@@ -184,7 +179,7 @@ private Suggestion generateSuggestion(BottleneckStats bottleneck, Duration total
     StringBuilder recommendation =
         new StringBuilder(
             String.format(
-                "These actions are involved in a bottleneck preventing parallelization:\n" + "%s",
+                "These actions are involved in a bottleneck preventing parallelization:\n%s",
                 this.suggestTargetsOrActions(bottleneck)));
     final List<String> rationale = new ArrayList<>();
     rationale.add(
@@ -262,19 +257,7 @@ private String suggestTargetsOrActions(BottleneckStats bottleneck) {
         .map(
             event -> {
               StringBuilder sb = new StringBuilder();
-              sb.append("\t- Action: \"");
-              sb.append(event.completeEvent.name);
-              sb.append("\"\n");
-              var forTarget =
-                  event.completeEvent.args.get(
-                      BazelProfileConstants.ARGS_CAT_ACTION_PROCESSING_TARGET);
-              if (!Strings.isNullOrEmpty(forTarget)) {
-                sb.append("\t\tTarget: \"");
-                sb.append(forTarget);
-                sb.append("\"\n");
-              }
-              sb.append("\t\tAction duration: ");
-              sb.append(formatDuration(event.completeEvent.duration));
+              sb.append(BazelEventsUtil.summarizeCompleteEvent(event.completeEvent));
               if (event.isCropped()) {
                 sb.append("\n\t\tDuration within bottleneck: ");
                 sb.append(formatDuration(event.croppedDuration));

analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/InvestigateRemoteCacheMissesSuggestionProvider.java

@@ -14,20 +14,17 @@
 
 package com.engflow.bazel.invocation.analyzer.suggestionproviders;
 
-import static com.engflow.bazel.invocation.analyzer.time.DurationUtil.formatDuration;
-
 import com.engflow.bazel.invocation.analyzer.Caveat;
 import com.engflow.bazel.invocation.analyzer.SuggestionCategory;
 import com.engflow.bazel.invocation.analyzer.SuggestionOutput;
-import com.engflow.bazel.invocation.analyzer.bazelprofile.BazelProfileConstants;
+import com.engflow.bazel.invocation.analyzer.bazelprofile.BazelEventsUtil;
 import com.engflow.bazel.invocation.analyzer.core.DataManager;
 import com.engflow.bazel.invocation.analyzer.core.MissingInputException;
 import com.engflow.bazel.invocation.analyzer.core.SuggestionProvider;
 import com.engflow.bazel.invocation.analyzer.dataproviders.FlagValueExperimentalProfileIncludeTargetLabel;
 import com.engflow.bazel.invocation.analyzer.dataproviders.LocalActions;
 import com.engflow.bazel.invocation.analyzer.dataproviders.remoteexecution.RemoteCachingUsed;
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Strings;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.stream.Collectors;
@@ -77,30 +74,15 @@ public SuggestionOutput getSuggestions(DataManager dataManager) {
 
       String title = "Investigate remote cache misses";
       StringBuilder recommendation = new StringBuilder();
-      recommendation.append(
-          "The following actions with cache misses took the longest to execute:\n");
+      recommendation.append("The following actions with cache misses took the longest to execute:");
       cacheMisses.stream()
           .limit(maxActions)
           .forEachOrdered(
-              action -> {
-                var completeAction = action.getAction();
-                recommendation.append("\t- Action: \"");
-                recommendation.append(completeAction.name);
-                recommendation.append("\"\n");
-                var forTarget =
-                    completeAction.args.get(
-                        BazelProfileConstants.ARGS_CAT_ACTION_PROCESSING_TARGET);
-                if (!Strings.isNullOrEmpty(forTarget)) {
-                  recommendation.append("\t\tTarget: \"");
-                  recommendation.append(forTarget);
-                  recommendation.append("\"\n");
-                }
-                recommendation.append("\t\tAction duration: ");
-                recommendation.append(formatDuration(completeAction.duration));
-                recommendation.append("\n");
-              });
+              action ->
+                  recommendation.append(
+                      "\n" + BazelEventsUtil.summarizeCompleteEvent(action.getAction())));
       recommendation.append(
-          "Check https://bazel.build/remote/cache-remote#troubleshooting-cache-hits to learn more"
+          "\nCheck https://bazel.build/remote/cache-remote#troubleshooting-cache-hits to learn more"
               + " about how to debug remote cache misses. Increasing the cache hit rate can"
               + " significantly speed up builds.");
       var caveats = new ArrayList<Caveat>();
@@ -109,12 +91,8 @@ public SuggestionOutput getSuggestions(DataManager dataManager) {
       if (!targetLabelIncluded.isProfileIncludeTargetLabelEnabled()) {
         caveats.add(
             SuggestionProviderUtil.createCaveat(
-                String.format(
-                    "The profile does not include which target each action was processed for,"
-                        + " although that data can help with investigating remote cache misses. It"
-                        + " is added to the profile by using the Bazel flag `%s`. Also see %s",
-                    FlagValueExperimentalProfileIncludeTargetLabel.FLAG_NAME,
-                    FlagValueExperimentalProfileIncludeTargetLabel.COMMAND_LINE_REFERENCE_URL),
+                FlagValueExperimentalProfileIncludeTargetLabel.getNotSetButUsefulForStatement(
+                    "investigating remote cache misses"),
                 false));
       }
       if (cacheMisses.size() > maxActions) {

analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/LocalActionsWithRemoteExecutionSuggestionProvider.java

@@ -18,12 +18,13 @@
 import com.engflow.bazel.invocation.analyzer.Suggestion;
 import com.engflow.bazel.invocation.analyzer.SuggestionCategory;
 import com.engflow.bazel.invocation.analyzer.SuggestionOutput;
+import com.engflow.bazel.invocation.analyzer.bazelprofile.BazelEventsUtil;
 import com.engflow.bazel.invocation.analyzer.core.DataManager;
 import com.engflow.bazel.invocation.analyzer.core.MissingInputException;
 import com.engflow.bazel.invocation.analyzer.core.SuggestionProvider;
+import com.engflow.bazel.invocation.analyzer.dataproviders.FlagValueExperimentalProfileIncludeTargetLabel;
 import com.engflow.bazel.invocation.analyzer.dataproviders.LocalActions;
 import com.engflow.bazel.invocation.analyzer.dataproviders.remoteexecution.RemoteExecutionUsed;
-import com.engflow.bazel.invocation.analyzer.time.DurationUtil;
 import com.google.common.annotations.VisibleForTesting;
 import java.util.ArrayList;
 import java.util.List;
@@ -70,6 +71,16 @@ public SuggestionOutput getSuggestions(DataManager dataManager) {
                   .sorted((a, b) -> b.getAction().duration.compareTo(a.getAction().duration))
                   .collect(Collectors.toList());
           if (!locallyExecuted.isEmpty()) {
+            var targetLabelIncluded =
+                dataManager.getDatum(FlagValueExperimentalProfileIncludeTargetLabel.class);
+            if (!targetLabelIncluded.isProfileIncludeTargetLabelEnabled()) {
+              caveats.add(
+                  SuggestionProviderUtil.createCaveat(
+                      FlagValueExperimentalProfileIncludeTargetLabel.getNotSetButUsefulForStatement(
+                          "migrating locally executed actions to remote execution"),
+                      false));
+            }
+
             if (locallyExecuted.size() > maxActions) {
               caveats.add(
                   SuggestionProviderUtil.createCaveat(
@@ -87,14 +98,9 @@ public SuggestionOutput getSuggestions(DataManager dataManager) {
             locallyExecuted.stream()
                 .limit(maxActions)
                 .forEachOrdered(
-                    action -> {
-                      recommendation.append("\n\t- ");
-                      recommendation.append(action.getAction().name);
-                      recommendation.append(" (");
-                      recommendation.append(
-                          DurationUtil.formatDuration(action.getAction().duration));
-                      recommendation.append(")");
-                    });
+                    action ->
+                        recommendation.append(
+                            "\n" + BazelEventsUtil.summarizeCompleteEvent(action.getAction())));
             suggestions.add(
                 SuggestionProviderUtil.createSuggestion(
                     SuggestionCategory.OTHER,

analyzer/javatests/com/engflow/bazel/invocation/analyzer/bazelprofile/BazelEventsUtilTest.java

@@ -9,6 +9,7 @@
 import static com.engflow.bazel.invocation.analyzer.bazelprofile.BazelProfileConstants.CAT_REMOTE_OUTPUT_DOWNLOAD;
 import static com.google.common.truth.Truth.assertThat;
 
+import com.engflow.bazel.invocation.analyzer.time.DurationUtil;
 import com.engflow.bazel.invocation.analyzer.time.Timestamp;
 import com.engflow.bazel.invocation.analyzer.traceeventformat.CompleteEvent;
 import com.google.common.collect.ImmutableMap;
@@ -124,6 +125,54 @@ public void doesNotRemoteUploadOutputs() {
         .isFalse();
   }
 
+  @Test
+  public void summarizeCompleteActionWithoutArgs() {
+    var eventName = "some random name";
+    var eventCategory = "that specific category";
+    var eventDuration = Duration.ofSeconds(1234);
+    var event =
+        new CompleteEvent(
+            eventName,
+            eventCategory,
+            Timestamp.ofSeconds(0),
+            eventDuration,
+            1,
+            1,
+            ImmutableMap.of());
+    var summary = BazelEventsUtil.summarizeCompleteEvent(event);
+    assertThat(summary).contains(eventName);
+    assertThat(summary).contains(DurationUtil.formatDuration(eventDuration));
+    assertThat(summary).doesNotContain(eventCategory);
+  }
+
+  @Test
+  public void summarizeCompleteActionWithArgs() {
+    var eventName = "some random name";
+    var eventCategory = "that specific category";
+    var eventDuration = Duration.ofSeconds(1234);
+    var mnemonic = "indicative stuff";
+    var targetName = "for //target:foo";
+    var event =
+        new CompleteEvent(
+            eventName,
+            eventCategory,
+            Timestamp.ofSeconds(0),
+            eventDuration,
+            1,
+            1,
+            ImmutableMap.of(
+                BazelProfileConstants.ARGS_CAT_ACTION_PROCESSING_MNEMONIC,
+                mnemonic,
+                BazelProfileConstants.ARGS_CAT_ACTION_PROCESSING_TARGET,
+                targetName));
+    var summary = BazelEventsUtil.summarizeCompleteEvent(event);
+    assertThat(summary).contains(eventName);
+    assertThat(summary).contains(DurationUtil.formatDuration(eventDuration));
+    assertThat(summary).contains(mnemonic);
+    assertThat(summary).contains(targetName);
+    assertThat(summary).doesNotContain(eventCategory);
+  }
+
   private static CompleteEvent completeEvent(String name, String category) {
     return new CompleteEvent(
         name, category, Timestamp.ofSeconds(1), Duration.ofSeconds(1), 1, 1, ImmutableMap.of());

analyzer/javatests/com/engflow/bazel/invocation/analyzer/suggestionproviders/LocalActionsWithRemoteExecutionSuggestionProviderTest.java

@@ -23,6 +23,7 @@
 
 import com.engflow.bazel.invocation.analyzer.EventThreadBuilder;
 import com.engflow.bazel.invocation.analyzer.SuggestionOutput;
+import com.engflow.bazel.invocation.analyzer.dataproviders.FlagValueExperimentalProfileIncludeTargetLabel;
 import com.engflow.bazel.invocation.analyzer.dataproviders.LocalActions;
 import com.engflow.bazel.invocation.analyzer.dataproviders.remoteexecution.RemoteExecutionUsed;
 import java.util.List;
@@ -37,13 +38,17 @@ public class LocalActionsWithRemoteExecutionSuggestionProviderTest
   // re-initialize the mocking).
   private RemoteExecutionUsed remoteExecutionUsed;
   private LocalActions localActions;
+  private FlagValueExperimentalProfileIncludeTargetLabel actionsIncludeTargetName;
 
   @Before
   public void setup() throws Exception {
     // Create reasonable defaults and set up to return the class-variables when the associated types
     // are requested.
+    actionsIncludeTargetName = new FlagValueExperimentalProfileIncludeTargetLabel(true);
     when(dataManager.getDatum(RemoteExecutionUsed.class)).thenAnswer(i -> remoteExecutionUsed);
     when(dataManager.getDatum(LocalActions.class)).thenAnswer(i -> localActions);
+    when(dataManager.getDatum(FlagValueExperimentalProfileIncludeTargetLabel.class))
+        .thenAnswer(i -> actionsIncludeTargetName);
 
     suggestionProvider = LocalActionsWithRemoteExecutionSuggestionProvider.createDefault();
   }
@@ -152,4 +157,30 @@ public void shouldReturnSuggestionForRemoteExecutionInvocationWithTooManyLocalAc
     assertThat(suggestion.getCaveatCount()).isEqualTo(1);
     assertThat(suggestion.getCaveat(0).getSuggestVerboseMode()).isTrue();
   }
+
+  @Test
+  public void shouldReturnSuggestionForRemoteExecutionInvocationWithTargetNameCaveat() {
+    actionsIncludeTargetName = new FlagValueExperimentalProfileIncludeTargetLabel(false);
+    remoteExecutionUsed = new RemoteExecutionUsed(true);
+    var thread = new EventThreadBuilder(1, 1);
+    var matchingAction =
+        new LocalActions.LocalAction(
+            thread.actionProcessingAction("a local action", "a", 10, 10),
+            List.of(thread.related(10, 2, CAT_GENERAL_INFORMATION, COMPLETE_SUBPROCESS_RUN)));
+
+    localActions = LocalActions.create(List.of(matchingAction));
+
+    SuggestionOutput suggestionOutput = suggestionProvider.getSuggestions(dataManager);
+
+    assertThat(suggestionOutput.getAnalyzerClassname())
+        .isEqualTo(LocalActionsWithRemoteExecutionSuggestionProvider.class.getName());
+    assertThat(suggestionOutput.hasFailure()).isFalse();
+    assertThat(suggestionOutput.getSuggestionList().size()).isEqualTo(1);
+    var suggestion = suggestionOutput.getSuggestion(0);
+    assertThat(suggestion.getRecommendation()).contains(matchingAction.getAction().name);
+    assertThat(suggestion.getCaveatCount()).isEqualTo(1);
+    assertThat(suggestion.getCaveat(0).getSuggestVerboseMode()).isFalse();
+    assertThat(suggestion.getCaveat(0).getMessage())
+        .contains(FlagValueExperimentalProfileIncludeTargetLabel.FLAG_NAME);
+  }
 }