From 79ab580df864118a204e9459716a589661b860ac Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 17 Sep 2024 05:12:00 -0700 Subject: [PATCH] Internal change PiperOrigin-RevId: 675522967 Change-Id: I178fe567711ad8f551afd3b95567d4296f00fca0 --- .../build/lib/runtime/BlazeRuntime.java | 24 +++-- .../lib/runtime/CommonCommandOptions.java | 10 +++ .../runtime/InstrumentationOutputBuilder.java | 14 +++ .../runtime/InstrumentationOutputFactory.java | 71 ++++++++++++--- .../runtime/LocalInstrumentationOutput.java | 1 + .../build/lib/runtime/ServerBuilder.java | 18 ++++ .../InstrumentationOutputFactoryTest.java | 88 ++++++++++++++++--- 7 files changed, 196 insertions(+), 30 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java index 972b69cd5e5e63..9c9777b3e4343b 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java @@ -372,9 +372,12 @@ ProfilerStartedEvent initProfiler( env.getCommandId().toString(), commandOptions.profilesToRetain); profile = - instrumentationOutputFactory.createLocalInstrumentationOutput( + instrumentationOutputFactory.createInstrumentationOutput( profileName, profilePath, + options, + commandOptions.redirectLocalInstrumentationOutputWrites, + eventHandler, /* convenienceName= */ profileName, /* append= */ null, /* internal= */ null); @@ -386,11 +389,14 @@ ProfilerStartedEvent initProfiler( : Format.JSON_TRACE_FILE_FORMAT; var profilePath = workspace.getWorkspace().getRelative(commandOptions.profilePath); profile = - instrumentationOutputFactory.createLocalInstrumentationOutput( + instrumentationOutputFactory.createInstrumentationOutput( (format == Format.JSON_TRACE_FILE_COMPRESSED_FORMAT) ? "command.profile.gz" : "command.profile.json", profilePath, + options, + commandOptions.redirectLocalInstrumentationOutputWrites, + eventHandler, /* convenienceName= */ null, /* append= */ false, /* internal= */ true); @@ -1561,6 +1567,13 @@ public BlazeRuntime build() throws AbruptExitException { } ServerBuilder serverBuilder = new ServerBuilder(); serverBuilder.addQueryOutputFormatters(OutputFormatters.getDefaultFormatters()); + serverBuilder + .getInstrumentationOutputFactoryBuilder() + .setLocalInstrumentationOutputBuilderSupplier(LocalInstrumentationOutput.Builder::new); + serverBuilder + .getInstrumentationOutputFactoryBuilder() + .setBuildEventArtifactInstrumentationOutputBuilderSupplier( + BuildEventArtifactInstrumentationOutput.Builder::new); for (BlazeModule module : blazeModules) { module.serverInit(startupOptionsProvider, serverBuilder); } @@ -1632,12 +1645,7 @@ public BlazeRuntime build() throws AbruptExitException { serverBuilder.getBuildEventArtifactUploaderMap(), serverBuilder.getAuthHeadersProvidersMap(), serverBuilder.getRepositoryRemoteExecutorFactory(), - new InstrumentationOutputFactory.Builder() - .setLocalInstrumentationOutputBuilderSupplier( - LocalInstrumentationOutput.Builder::new) - .setBuildEventArtifactInstrumentationOutputBuilderSupplier( - BuildEventArtifactInstrumentationOutput.Builder::new) - .build()); + serverBuilder.createInstrumentationOutputFactory()); AutoProfiler.setClock(runtime.getClock()); BugReport.setRuntime(runtime); return runtime; diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java index 28e956ba41bd0e..b45f0fd92f5b3b 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java @@ -596,4 +596,14 @@ public ProfilerTaskConverter() { super(ProfilerTask.class, "profiler task"); } } + + @Option( + name = "redirect_local_instrumentation_output_writes", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.LOGGING, + effectTags = {OptionEffectTag.BAZEL_MONITORING}, + help = + "If true and supported, instrumentation output is redirected to be written locally on a" + + " different machine than where bazel is running on.") + public boolean redirectLocalInstrumentationOutputWrites; } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/InstrumentationOutputBuilder.java b/src/main/java/com/google/devtools/build/lib/runtime/InstrumentationOutputBuilder.java index 2cd747b31eca12..88a70f4ba0537c 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/InstrumentationOutputBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/InstrumentationOutputBuilder.java @@ -13,6 +13,8 @@ // limitations under the License. package com.google.devtools.build.lib.runtime; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.common.options.OptionsProvider; import com.google.errorprone.annotations.CanIgnoreReturnValue; /** Builds different {@link InstrumentationOutput} objects with correct input parameters. */ @@ -21,6 +23,18 @@ public interface InstrumentationOutputBuilder { @CanIgnoreReturnValue InstrumentationOutputBuilder setName(String name); + /** Sets the path to write the {@link InstrumentationOutput}. */ + @CanIgnoreReturnValue + default InstrumentationOutputBuilder setPath(Path path) { + return this; + } + + /** Provides the options necessary for building the {@link InstrumentationOutput}. */ + @CanIgnoreReturnValue + default InstrumentationOutputBuilder setOptions(OptionsProvider options) { + return this; + } + /** Builds the {@link InstrumentationOutput} object. */ InstrumentationOutput build(); } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/InstrumentationOutputFactory.java b/src/main/java/com/google/devtools/build/lib/runtime/InstrumentationOutputFactory.java index 33d24c5cc5ee60..c2c7b25254c860 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/InstrumentationOutputFactory.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/InstrumentationOutputFactory.java @@ -16,7 +16,10 @@ import static com.google.common.base.Preconditions.checkNotNull; import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader; +import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.common.options.OptionsProvider; import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.util.function.Supplier; import javax.annotation.Nullable; @@ -29,35 +32,69 @@ public final class InstrumentationOutputFactory { private final Supplier buildEventArtifactInstrumentationOutputBuilderSupplier; + @Nullable + final Supplier redirectInstrumentationOutputBuilderSupplier; + private InstrumentationOutputFactory( Supplier localInstrumentationOutputBuilderSupplier, Supplier - buildEventArtifactInstrumentationOutputBuilderSupplier) { + buildEventArtifactInstrumentationOutputBuilderSupplier, + @Nullable + Supplier redirectInstrumentationOutputBuilderSupplier) { this.localInstrumentationOutputBuilderSupplier = localInstrumentationOutputBuilderSupplier; this.buildEventArtifactInstrumentationOutputBuilderSupplier = buildEventArtifactInstrumentationOutputBuilderSupplier; + this.redirectInstrumentationOutputBuilderSupplier = + redirectInstrumentationOutputBuilderSupplier; } /** - * Creates {@link LocalInstrumentationOutput} with given parameters. + * Creates {@link LocalInstrumentationOutput} or an {@link InstrumentationOutput} object + * redirecting outputs to be written on a different machine. + * + *

If {@link #redirectInstrumentationOutputBuilderSupplier} is not provided but {@code + * isRedirect} is {@code true}, this method will default to return {@link + * LocalInstrumentationOutput}. * *

{@code name} and {@code path} are required since they indicate what the output is and where - * it is stored. + * it is stored. {@code options} might also be necessary for the redirect {@link + * InstrumentationOutput}. * - *

When the name of the instrumentation output is complicated, an optional {@code - * convenienceName} parameter can be passed in so that a symlink pointing to the output with such - * a simpler name is created. See {@link LocalInstrumentationOutput.Builder#setConvenienceName}. + *

For {@link LocalInstrumentationOutput}, there are two additional considerations: * - *

User can also pass in the optional {@code append} and {@code internal} {@code Boolean}s to - * control how {@code path} creates the {@link OutputStream}. See {@link - * LocalInstrumentationOutput#createOutputStream()} for more details. + *

    + *
  • When the name of the instrumentation output is complicated, an optional {@code + * convenienceName} parameter can be passed in so that a symlink pointing to the output with + * such a simpler name is created. See {@link + * LocalInstrumentationOutput.Builder#setConvenienceName}. + *
  • User can also pass in the optional {@code append} and {@code internal} {@code Boolean}s + * to control how {@code path} creates the {@link OutputStream}. See {@link + * LocalInstrumentationOutput#createOutputStream} for more details. + *
*/ - public LocalInstrumentationOutput createLocalInstrumentationOutput( + public InstrumentationOutput createInstrumentationOutput( String name, Path path, + OptionsProvider options, + boolean isRedirect, + EventHandler eventHandler, @Nullable String convenienceName, @Nullable Boolean append, @Nullable Boolean internal) { + if (isRedirect) { + if (redirectInstrumentationOutputBuilderSupplier != null) { + return redirectInstrumentationOutputBuilderSupplier + .get() + .setName(name) + .setPath(path) + .setOptions(options) + .build(); + } + eventHandler.handle( + Event.warn( + "Redirecting to write Instrumentation Output on a different machine is not" + + " supported. Defaulting to writing output locally.")); + } return localInstrumentationOutputBuilderSupplier .get() .setName(name) @@ -86,6 +123,9 @@ public static class Builder { private Supplier buildEventArtifactInstrumentationOutputBuilderSupplier; + @Nullable + private Supplier redirectInstrumentationOutputBuilderSupplier; + @CanIgnoreReturnValue public Builder setLocalInstrumentationOutputBuilderSupplier( Supplier localInstrumentationOutputBuilderSupplier) { @@ -102,6 +142,14 @@ public Builder setBuildEventArtifactInstrumentationOutputBuilderSupplier( return this; } + @CanIgnoreReturnValue + public Builder setRedirectInstrumentationOutputBuilderSupplier( + Supplier redirectInstrumentationOutputBuilderSupplier) { + this.redirectInstrumentationOutputBuilderSupplier = + redirectInstrumentationOutputBuilderSupplier; + return this; + } + public InstrumentationOutputFactory build() { return new InstrumentationOutputFactory( checkNotNull( @@ -109,7 +157,8 @@ public InstrumentationOutputFactory build() { "Cannot create InstrumentationOutputFactory without localOutputBuilderSupplier"), checkNotNull( buildEventArtifactInstrumentationOutputBuilderSupplier, - "Cannot create InstrumentationOutputFactory without bepOutputBuilderSupplier")); + "Cannot create InstrumentationOutputFactory without bepOutputBuilderSupplier"), + redirectInstrumentationOutputBuilderSupplier); } } } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/LocalInstrumentationOutput.java b/src/main/java/com/google/devtools/build/lib/runtime/LocalInstrumentationOutput.java index c6bc33f592ca78..62596b50bd1cf8 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/LocalInstrumentationOutput.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/LocalInstrumentationOutput.java @@ -85,6 +85,7 @@ public Builder setName(String name) { /** Sets the path to the local {@link InstrumentationOutput}. */ @CanIgnoreReturnValue + @Override public Builder setPath(Path path) { this.path = path; return this; diff --git a/src/main/java/com/google/devtools/build/lib/runtime/ServerBuilder.java b/src/main/java/com/google/devtools/build/lib/runtime/ServerBuilder.java index f9634eb57b2749..bb788c471f25cd 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/ServerBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/ServerBuilder.java @@ -40,6 +40,8 @@ public final class ServerBuilder { private final ImmutableMap.Builder authHeadersProvidersMap = ImmutableMap.builder(); private RepositoryRemoteExecutorFactory repositoryRemoteExecutorFactory; + private final InstrumentationOutputFactory.Builder instrumentationOutputFactoryBuilder = + new InstrumentationOutputFactory.Builder(); @VisibleForTesting public ServerBuilder() {} @@ -185,4 +187,20 @@ public ServerBuilder addAuthHeadersProvider( public ImmutableMap getAuthHeadersProvidersMap() { return authHeadersProvidersMap.buildOrThrow(); } + + /** + * Returns the builder for {@link InstrumentationOutputFactory} so that suppliers for different + * types of {@link InstrumentationOutputBuilder} can be added. + */ + public InstrumentationOutputFactory.Builder getInstrumentationOutputFactoryBuilder() { + return instrumentationOutputFactoryBuilder; + } + + /** + * Creates the {@link InstrumentationOutputFactory} so that user can choose to create the {@link + * InstrumentationOutputBuilder} object. + */ + public InstrumentationOutputFactory createInstrumentationOutputFactory() { + return instrumentationOutputFactoryBuilder.build(); + } } diff --git a/src/test/java/com/google/devtools/build/lib/runtime/InstrumentationOutputFactoryTest.java b/src/test/java/com/google/devtools/build/lib/runtime/InstrumentationOutputFactoryTest.java index 341ea8baea36b3..97ecc9abd351b0 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/InstrumentationOutputFactoryTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/InstrumentationOutputFactoryTest.java @@ -18,13 +18,21 @@ import static org.mockito.Mockito.mock; import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader; +import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.events.EventKind; +import com.google.devtools.build.lib.events.ExtendedEventHandler; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import com.google.devtools.common.options.OptionsProvider; +import com.google.errorprone.annotations.CanIgnoreReturnValue; +import com.google.testing.junit.testparameterinjector.TestParameter; +import com.google.testing.junit.testparameterinjector.TestParameterInjector; +import java.util.ArrayList; +import java.util.List; import org.junit.Test; import org.junit.runner.RunWith; -import org.junit.runners.JUnit4; -@RunWith(JUnit4.class) +@RunWith(TestParameterInjector.class) public class InstrumentationOutputFactoryTest { @Test public void testInstrumentationOutputFactory_cannotCreateFactorIfLocalSupplierUnset() { @@ -53,7 +61,9 @@ public void testInstrumentationOutputFactory_cannotCreateFactorIfBepSupplierUnse } @Test - public void testInstrumentationOutputFactory_successfulCreateFactory() { + public void testInstrumentationOutputFactory_successfulFactoryCreation( + @TestParameter boolean injectRedirectOutputBuilderSupplier, + @TestParameter boolean createRedirectOutput) { InstrumentationOutputFactory.Builder factoryBuilder = new InstrumentationOutputFactory.Builder(); factoryBuilder.setLocalInstrumentationOutputBuilderSupplier( @@ -61,15 +71,71 @@ public void testInstrumentationOutputFactory_successfulCreateFactory() { factoryBuilder.setBuildEventArtifactInstrumentationOutputBuilderSupplier( BuildEventArtifactInstrumentationOutput.Builder::new); + InstrumentationOutput fakeRedirectInstrumentationOutput = mock(InstrumentationOutput.class); + if (injectRedirectOutputBuilderSupplier) { + InstrumentationOutputBuilder fakeRedirectInstrumentationBuilder = + new InstrumentationOutputBuilder() { + @Override + @CanIgnoreReturnValue + public InstrumentationOutputBuilder setName(String name) { + return this; + } + + @Override + public InstrumentationOutput build() { + return fakeRedirectInstrumentationOutput; + } + }; + + factoryBuilder.setRedirectInstrumentationOutputBuilderSupplier( + () -> fakeRedirectInstrumentationBuilder); + } + + List warningEvents = new ArrayList<>(); + ExtendedEventHandler eventHandler = + new ExtendedEventHandler() { + @Override + public void post(Postable obj) {} + + @Override + public void handle(Event event) { + warningEvents.add(event); + } + }; + InstrumentationOutputFactory outputFactory = factoryBuilder.build(); - assertThat( - outputFactory.createLocalInstrumentationOutput( - /* name= */ "local", - new InMemoryFileSystem(DigestHashFunction.SHA256).getPath("/file"), - /* convenienceName= */ null, - /* append= */ null, - /* internal= */ null)) - .isNotNull(); + var instrumentationOutput = + outputFactory.createInstrumentationOutput( + /* name= */ "local", + new InMemoryFileSystem(DigestHashFunction.SHA256).getPath("/file"), + mock(OptionsProvider.class), + createRedirectOutput, + eventHandler, + /* convenienceName= */ null, + /* append= */ null, + /* internal= */ null); + + // Only when redirectOutputBuilderSupplier is provided to the factory, and we intend to create a + // RedirectOutputBuilder object, we expect a non-LocalInstrumentationOutput to be created. In + // all other scenarios, a LocalInstrumentationOutput is returned. + if (createRedirectOutput && injectRedirectOutputBuilderSupplier) { + assertThat(instrumentationOutput).isEqualTo(fakeRedirectInstrumentationOutput); + } else { + assertThat(instrumentationOutput).isInstanceOf(LocalInstrumentationOutput.class); + } + + // When user wants to create a redirectOutputBuilder object but its builder supplier is not + // provided, eventHandler should post a warning event. + if (createRedirectOutput && !injectRedirectOutputBuilderSupplier) { + assertThat(warningEvents) + .containsExactly( + Event.of( + EventKind.WARNING, + "Redirecting to write Instrumentation Output on a different machine is not" + + " supported. Defaulting to writing output locally.")); + } else { + assertThat(warningEvents).isEmpty(); + } assertThat( outputFactory.createBuildEventArtifactInstrumentationOutput( /* name= */ "bep", mock(BuildEventArtifactUploader.class)))