diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/NoCacheActionsSuggestionProvider.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/NoCacheActionsSuggestionProvider.java new file mode 100644 index 0000000..34883da --- /dev/null +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/NoCacheActionsSuggestionProvider.java @@ -0,0 +1,127 @@ +/* + * Copyright 2023 EngFlow Inc. + * + * 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 + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package com.engflow.bazel.invocation.analyzer.suggestionproviders; + +import com.engflow.bazel.invocation.analyzer.Caveat; +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.RemoteCachingUsed; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableList; +import java.util.ArrayList; +import java.util.List; +import java.util.stream.Collectors; + +/** + * A {@link SuggestionProvider} that suggests making non-remote-cacheable actions cacheable when + * remote caching is used. + */ +public class NoCacheActionsSuggestionProvider extends SuggestionProviderBase { + private static final String ANALYZER_CLASSNAME = NoCacheActionsSuggestionProvider.class.getName(); + private static final String SUGGESTION_ID_NO_CACHE_ACTIONS = "NoCacheActions"; + + public static NoCacheActionsSuggestionProvider createDefault() { + return new NoCacheActionsSuggestionProvider(5); + } + + public static NoCacheActionsSuggestionProvider createVerbose() { + return new NoCacheActionsSuggestionProvider(Integer.MAX_VALUE); + } + + private final int maxActions; + + @VisibleForTesting + NoCacheActionsSuggestionProvider(int maxActions) { + this.maxActions = maxActions; + } + + @Override + public SuggestionOutput getSuggestions(DataManager dataManager) { + try { + boolean remoteCachingUsed = + dataManager.getDatum(RemoteCachingUsed.class).isRemoteCachingUsed(); + List suggestions = new ArrayList<>(); + List caveats = new ArrayList<>(); + if (remoteCachingUsed) { + var localActions = dataManager.getDatum(LocalActions.class); + if (!localActions.isEmpty()) { + var actionsWithoutRemoteCacheCheck = + localActions.stream() + .filter(action -> !action.hasRemoteCacheCheck()) + .sorted((a, b) -> b.getAction().duration.compareTo(a.getAction().duration)) + .collect(Collectors.toList()); + if (!actionsWithoutRemoteCacheCheck.isEmpty()) { + var targetLabelIncluded = + dataManager.getDatum(FlagValueExperimentalProfileIncludeTargetLabel.class); + if (!targetLabelIncluded.isProfileIncludeTargetLabelEnabled()) { + caveats.add( + SuggestionProviderUtil.createCaveat( + FlagValueExperimentalProfileIncludeTargetLabel.getNotSetButUsefulForStatement( + "investigating actions that are not using remote caching"), + false)); + } + if (actionsWithoutRemoteCacheCheck.size() > maxActions) { + caveats.add( + SuggestionProviderUtil.createCaveat( + String.format( + "Only the %d longest actions that did not check the remote cache of the" + + " %d found were listed.", + maxActions, actionsWithoutRemoteCacheCheck.size()), + true)); + } + StringBuilder recommendation = new StringBuilder(); + recommendation.append( + "Some actions did not check the remote cache. Likely the targets the actions were" + + " executed for include the tag `no-cache` or `no-remote-cache`. Investigate" + + " whether these tags can be removed:"); + actionsWithoutRemoteCacheCheck.stream() + .limit(maxActions) + .forEachOrdered( + action -> + recommendation.append( + "\n" + BazelEventsUtil.summarizeCompleteEvent(action.getAction()))); + suggestions.add( + SuggestionProviderUtil.createSuggestion( + SuggestionCategory.OTHER, + createSuggestionId(SUGGESTION_ID_NO_CACHE_ACTIONS), + "Investigate actions that are not using remote caching", + recommendation.toString(), + null, + ImmutableList.of( + "The profile suggests remote caching was used, but some actions did not" + + " check the remote cache. In most cases, enabling remote caching for" + + " actions improves build performance. An example case where using" + + " remote caching might not be beneficial is when the outputs are very" + + " large and the the cost of downloading them is higher than locally" + + " building the target."), + caveats)); + } + } + } + return SuggestionProviderUtil.createSuggestionOutput(ANALYZER_CLASSNAME, suggestions, null); + } catch (MissingInputException e) { + return SuggestionProviderUtil.createSuggestionOutputForMissingInput(ANALYZER_CLASSNAME, e); + } catch (Throwable t) { + return SuggestionProviderUtil.createSuggestionOutputForFailure(ANALYZER_CLASSNAME, t); + } + } +} diff --git a/analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/SuggestionProviderUtil.java b/analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/SuggestionProviderUtil.java index 0760e28..dd720df 100644 --- a/analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/SuggestionProviderUtil.java +++ b/analyzer/java/com/engflow/bazel/invocation/analyzer/suggestionproviders/SuggestionProviderUtil.java @@ -57,6 +57,9 @@ public static List getAllSuggestionProviders(boolean verbose new GarbageCollectionSuggestionProvider(), new JobsSuggestionProvider(), new UseSkymeldSuggestionProvider(), + verbose + ? NoCacheActionsSuggestionProvider.createVerbose() + : NoCacheActionsSuggestionProvider.createDefault(), new NegligiblePhaseSuggestionProvider(), new QueuingSuggestionProvider(), new IncompleteProfileSuggestionProvider(), diff --git a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/suggestionproviders/NoCacheActionsSuggestionProviderTest.java b/analyzer/javatests/com/engflow/bazel/invocation/analyzer/suggestionproviders/NoCacheActionsSuggestionProviderTest.java new file mode 100644 index 0000000..737805b --- /dev/null +++ b/analyzer/javatests/com/engflow/bazel/invocation/analyzer/suggestionproviders/NoCacheActionsSuggestionProviderTest.java @@ -0,0 +1,199 @@ +/* + * Copyright 2023 EngFlow Inc. + * + * 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 + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License + * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express + * or implied. See the License for the specific language governing permissions and limitations under + * the License. + */ + +package com.engflow.bazel.invocation.analyzer.suggestionproviders; + +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.CAT_REMOTE_ACTION_EXECUTION; +import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Mockito.when; + +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.RemoteCachingUsed; +import java.util.List; +import org.junit.Before; +import org.junit.Test; + +public class NoCacheActionsSuggestionProviderTest extends SuggestionProviderUnitTestBase { + // These variables are returned from calls to DataManager.getDatum for the associated types. They + // are set up with reasonable defaults before each test is run, but can be overridden within the + // tests when custom values are desired for the testing being conducted (without the need to + // re-initialize the mocking). + private RemoteCachingUsed remoteCachingUsed; + 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(RemoteCachingUsed.class)).thenAnswer(i -> remoteCachingUsed); + when(dataManager.getDatum(LocalActions.class)).thenAnswer(i -> localActions); + when(dataManager.getDatum(FlagValueExperimentalProfileIncludeTargetLabel.class)) + .thenAnswer(i -> actionsIncludeTargetName); + + suggestionProvider = NoCacheActionsSuggestionProvider.createDefault(); + } + + @Test + public void shouldNotReturnSuggestionIfRemoteCachingIsNotUsed() { + remoteCachingUsed = new RemoteCachingUsed(false); + + SuggestionOutput suggestionOutput = suggestionProvider.getSuggestions(dataManager); + + assertThat(suggestionOutput.getSuggestionList()).isEmpty(); + assertThat(suggestionOutput.hasFailure()).isFalse(); + } + + @Test + public void shouldNotReturnSuggestionForRemoteCachingInvocationWithoutLocalActions() { + remoteCachingUsed = new RemoteCachingUsed(true); + localActions = LocalActions.create(List.of()); + + SuggestionOutput suggestionOutput = suggestionProvider.getSuggestions(dataManager); + + assertThat(suggestionOutput.getSuggestionList()).isEmpty(); + assertThat(suggestionOutput.hasFailure()).isFalse(); + } + + @Test + public void shouldNotReturnSuggestionForRemoteCachingInvocationWithRemoteCacheCheck() { + remoteCachingUsed = new RemoteCachingUsed(true); + var thread = new EventThreadBuilder(1, 1); + var actionWithCacheCheck = + new LocalActions.LocalAction( + thread.actionProcessingAction("some remote action", "b", 20, 10), + List.of(thread.related(20, 2, CAT_REMOTE_ACTION_CACHE_CHECK))); + localActions = LocalActions.create(List.of(actionWithCacheCheck)); + + SuggestionOutput suggestionOutput = suggestionProvider.getSuggestions(dataManager); + + assertThat(suggestionOutput.getSuggestionList()).isEmpty(); + assertThat(suggestionOutput.hasFailure()).isFalse(); + } + + @Test + public void shouldReturnSuggestionForRemoteCachingInvocationWithoutRemoteCacheCheck() { + remoteCachingUsed = new RemoteCachingUsed(true); + var thread = new EventThreadBuilder(1, 1); + var actionWithRemoteCacheCheck = + new LocalActions.LocalAction( + thread.actionProcessingAction("action with RC check", "a", 10, 10), + List.of(thread.related(10, 2, CAT_REMOTE_ACTION_CACHE_CHECK))); + var actionWithoutRemoteCacheCheckLocal = + new LocalActions.LocalAction( + thread.actionProcessingAction("no RC check, local exec", "b", 20, 10), + List.of(thread.related(20, 2, CAT_LOCAL_ACTION_EXECUTION))); + var actionWithoutRemoteCacheCheckRemote = + new LocalActions.LocalAction( + thread.actionProcessingAction("no RC check, remote exec", "c", 30, 10), + List.of(thread.related(30, 2, CAT_REMOTE_ACTION_EXECUTION))); + + localActions = + LocalActions.create( + List.of( + actionWithRemoteCacheCheck, + actionWithoutRemoteCacheCheckLocal, + actionWithoutRemoteCacheCheckRemote)); + + SuggestionOutput suggestionOutput = suggestionProvider.getSuggestions(dataManager); + + assertThat(suggestionOutput.getAnalyzerClassname()) + .isEqualTo(NoCacheActionsSuggestionProvider.class.getName()); + assertThat(suggestionOutput.hasFailure()).isFalse(); + assertThat(suggestionOutput.getSuggestionList().size()).isEqualTo(1); + var suggestion = suggestionOutput.getSuggestion(0); + assertThat(suggestion.getRecommendation()) + .contains(actionWithoutRemoteCacheCheckLocal.getAction().name); + assertThat(suggestion.getRecommendation()) + .contains(actionWithoutRemoteCacheCheckRemote.getAction().name); + assertThat(suggestion.getRecommendation()) + .doesNotContain(actionWithRemoteCacheCheck.getAction().name); + assertThat(suggestion.getCaveatList()).isEmpty(); + } + + @Test + public void shouldReturnSuggestionForRemoteCachingInvocationWithTooManyMatches() { + suggestionProvider = new NoCacheActionsSuggestionProvider(1); + remoteCachingUsed = new RemoteCachingUsed(true); + var thread = new EventThreadBuilder(1, 1); + var actionWithRemoteCacheCheck = + new LocalActions.LocalAction( + thread.actionProcessingAction("action with RC check", "a", 10, 10), + List.of(thread.related(10, 2, CAT_REMOTE_ACTION_CACHE_CHECK))); + var actionWithoutRemoteCacheCheckLocal = + new LocalActions.LocalAction( + thread.actionProcessingAction("no RC check, local exec", "b", 20, 10), + List.of(thread.related(20, 2, CAT_LOCAL_ACTION_EXECUTION))); + var actionWithoutRemoteCacheCheckRemote = + new LocalActions.LocalAction( + thread.actionProcessingAction("no RC check, remote exec", "c", 30, 100), + List.of(thread.related(30, 2, CAT_REMOTE_ACTION_EXECUTION))); + + localActions = + LocalActions.create( + List.of( + actionWithRemoteCacheCheck, + actionWithoutRemoteCacheCheckLocal, + actionWithoutRemoteCacheCheckRemote)); + + SuggestionOutput suggestionOutput = suggestionProvider.getSuggestions(dataManager); + + assertThat(suggestionOutput.getAnalyzerClassname()) + .isEqualTo(NoCacheActionsSuggestionProvider.class.getName()); + assertThat(suggestionOutput.hasFailure()).isFalse(); + assertThat(suggestionOutput.getSuggestionList().size()).isEqualTo(1); + var suggestion = suggestionOutput.getSuggestion(0); + // This is the longest matching action. It is included. + assertThat(suggestion.getRecommendation()) + .contains(actionWithoutRemoteCacheCheckRemote.getAction().name); + // This is another matching action, but it exceeds the max count, so it's not included. + assertThat(suggestion.getRecommendation()) + .doesNotContain(actionWithoutRemoteCacheCheckLocal.getAction().name); + assertThat(suggestion.getRecommendation()) + .doesNotContain(actionWithRemoteCacheCheck.getAction().name); + assertThat(suggestion.getCaveatCount()).isEqualTo(1); + assertThat(suggestion.getCaveat(0).getSuggestVerboseMode()).isTrue(); + } + + @Test + public void shouldReturnSuggestionForRemoteCachingInvocationWithTargetNameCaveat() { + actionsIncludeTargetName = new FlagValueExperimentalProfileIncludeTargetLabel(false); + remoteCachingUsed = new RemoteCachingUsed(true); + var thread = new EventThreadBuilder(1, 1); + var matchingAction = + new LocalActions.LocalAction( + thread.actionProcessingAction("no RC check", "b", 20, 10), List.of()); + + localActions = LocalActions.create(List.of(matchingAction)); + + SuggestionOutput suggestionOutput = suggestionProvider.getSuggestions(dataManager); + + assertThat(suggestionOutput.getAnalyzerClassname()) + .isEqualTo(NoCacheActionsSuggestionProvider.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); + } +} diff --git a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/suggestionproviders/SuggestionProvidersTestSuite.java b/analyzer/javatests/com/engflow/bazel/invocation/analyzer/suggestionproviders/SuggestionProvidersTestSuite.java index 45a1810..52d454b 100644 --- a/analyzer/javatests/com/engflow/bazel/invocation/analyzer/suggestionproviders/SuggestionProvidersTestSuite.java +++ b/analyzer/javatests/com/engflow/bazel/invocation/analyzer/suggestionproviders/SuggestionProvidersTestSuite.java @@ -29,6 +29,7 @@ LocalActionsWithRemoteExecutionSuggestionProviderTest.class, MergedEventsSuggestionProviderTest.class, NegligiblePhaseSuggestionProviderTest.class, + NoCacheActionsSuggestionProviderTest.class, QueuingSuggestionProviderTest.class, UseSkymeldSuggestionProviderTest.class, })