remote: Add interceptor for logging gRPC calls during remote execution/caching
This provides a io.grpc.ClientInterceptor implementation that can be used to log gRPC call information. The interceptor can select a logging handler to use based on the gRPC method being called (Watch, Execute, Write, etc) to build a LogEntry, which can then be logged after the call has finished. Unit tests for the interceptor are included.
In this change, the interceptor is never invoked, nor are there any handlers implemented for any gRPC methods. The interceptor also never tries to log any entries.
To avoid circular dependency issues (Remote library will depend on logger which depends on remote library for utils), I've factored out the utility classes from the remote library into their own directory/package as part of this change.
PiperOrigin-RevId: 187926516
diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD
index c3d1238..c197c10 100644
--- a/src/test/java/com/google/devtools/build/lib/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/BUILD
@@ -1199,9 +1199,12 @@
"//src/main/java/com/google/devtools/build/lib/remote",
"//src/main/java/com/google/devtools/build/lib/remote/blobstore",
"//src/main/java/com/google/devtools/build/lib/remote/blobstore/http",
+ "//src/main/java/com/google/devtools/build/lib/remote/logging",
+ "//src/main/java/com/google/devtools/build/lib/remote/util",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
"//src/main/java/com/google/devtools/common/options",
+ "//src/main/protobuf:remote_execution_log_java_proto",
"//third_party:api_client",
"//third_party:mockito",
"//third_party:netty",
diff --git a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamUploaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamUploaderTest.java
index 1991d7d..80febbc 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamUploaderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamUploaderTest.java
@@ -27,6 +27,8 @@
import com.google.common.util.concurrent.MoreExecutors;
import com.google.devtools.build.lib.analysis.BlazeVersionInfo;
import com.google.devtools.build.lib.remote.Retrier.RetryException;
+import com.google.devtools.build.lib.remote.util.DigestUtil;
+import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
import com.google.devtools.remoteexecution.v1test.Digest;
import com.google.devtools.remoteexecution.v1test.RequestMetadata;
diff --git a/src/test/java/com/google/devtools/build/lib/remote/CasPathConverterTest.java b/src/test/java/com/google/devtools/build/lib/remote/CasPathConverterTest.java
index 493041b..885ae95 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/CasPathConverterTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/CasPathConverterTest.java
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
import com.google.devtools.build.lib.remote.RemoteModule.CasPathConverter;
+import com.google.devtools.build.lib.remote.util.DigestUtil;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
diff --git a/src/test/java/com/google/devtools/build/lib/remote/ChunkerTest.java b/src/test/java/com/google/devtools/build/lib/remote/ChunkerTest.java
index ed1ccf6..2634cc9 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/ChunkerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/ChunkerTest.java
@@ -17,6 +17,7 @@
import static junit.framework.TestCase.fail;
import com.google.devtools.build.lib.remote.Chunker.Chunk;
+import com.google.devtools.build.lib.remote.util.DigestUtil;
import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
import com.google.devtools.remoteexecution.v1test.Digest;
import com.google.protobuf.ByteString;
diff --git a/src/test/java/com/google/devtools/build/lib/remote/FakeActionInputFileCache.java b/src/test/java/com/google/devtools/build/lib/remote/FakeActionInputFileCache.java
index c4c09ab..0fdf722 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/FakeActionInputFileCache.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/FakeActionInputFileCache.java
@@ -20,6 +20,7 @@
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.ActionInputFileCache;
import com.google.devtools.build.lib.actions.cache.Metadata;
+import com.google.devtools.build.lib.remote.util.DigestUtil;
import com.google.devtools.build.lib.skyframe.FileArtifactValue;
import com.google.devtools.build.lib.skyframe.FileContentsProxy;
import com.google.devtools.build.lib.vfs.FileStatus;
diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java
index 5b342e8..511e004 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteCacheTest.java
@@ -31,7 +31,9 @@
import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions;
import com.google.devtools.build.lib.authandtls.GoogleAuthUtils;
import com.google.devtools.build.lib.clock.JavaClock;
-import com.google.devtools.build.lib.remote.DigestUtil.ActionKey;
+import com.google.devtools.build.lib.remote.util.DigestUtil;
+import com.google.devtools.build.lib.remote.util.DigestUtil.ActionKey;
+import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
import com.google.devtools.build.lib.testutil.Scratch;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.FileSystem;
diff --git a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
index 49ee7cd..4f3252b 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/GrpcRemoteExecutionClientTest.java
@@ -44,6 +44,8 @@
import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionPolicy;
import com.google.devtools.build.lib.exec.util.FakeOwner;
+import com.google.devtools.build.lib.remote.util.DigestUtil;
+import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
index 4ed3dafb..4acdca8 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
@@ -44,8 +44,10 @@
import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionPolicy;
import com.google.devtools.build.lib.exec.util.FakeOwner;
-import com.google.devtools.build.lib.remote.DigestUtil.ActionKey;
import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode;
+import com.google.devtools.build.lib.remote.util.DigestUtil;
+import com.google.devtools.build.lib.remote.util.DigestUtil.ActionKey;
+import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
index bd6c12e..150e759 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
@@ -49,7 +49,8 @@
import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionPolicy;
import com.google.devtools.build.lib.exec.util.FakeOwner;
-import com.google.devtools.build.lib.remote.DigestUtil.ActionKey;
+import com.google.devtools.build.lib.remote.util.DigestUtil;
+import com.google.devtools.build.lib.remote.util.DigestUtil.ActionKey;
import com.google.devtools.build.lib.util.ExitCode;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.FileSystem;
diff --git a/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java
index f173c2e..0f24164 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/SimpleBlobStoreActionCacheTest.java
@@ -21,6 +21,8 @@
import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.clock.JavaClock;
import com.google.devtools.build.lib.remote.blobstore.ConcurrentMapBlobStore;
+import com.google.devtools.build.lib.remote.util.DigestUtil;
+import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
diff --git a/src/test/java/com/google/devtools/build/lib/remote/TreeNodeRepositoryTest.java b/src/test/java/com/google/devtools/build/lib/remote/TreeNodeRepositoryTest.java
index e03f85d..858c922 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/TreeNodeRepositoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/TreeNodeRepositoryTest.java
@@ -24,6 +24,7 @@
import com.google.devtools.build.lib.clock.BlazeClock;
import com.google.devtools.build.lib.exec.SingleBuildFileCache;
import com.google.devtools.build.lib.remote.TreeNodeRepository.TreeNode;
+import com.google.devtools.build.lib.remote.util.DigestUtil;
import com.google.devtools.build.lib.testutil.Scratch;
import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
import com.google.devtools.build.lib.vfs.Path;
diff --git a/src/test/java/com/google/devtools/build/lib/remote/logging/LoggingInterceptorTest.java b/src/test/java/com/google/devtools/build/lib/remote/logging/LoggingInterceptorTest.java
new file mode 100644
index 0000000..b9c3d54
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/remote/logging/LoggingInterceptorTest.java
@@ -0,0 +1,248 @@
+// Copyright 2018 The Bazel Authors. All rights reserved.
+//
+// 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.google.devtools.build.lib.remote.logging;
+
+import static com.google.common.collect.Iterators.advance;
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+import com.google.bytestream.ByteStreamGrpc;
+import com.google.bytestream.ByteStreamGrpc.ByteStreamBlockingStub;
+import com.google.bytestream.ByteStreamGrpc.ByteStreamImplBase;
+import com.google.bytestream.ByteStreamGrpc.ByteStreamStub;
+import com.google.bytestream.ByteStreamProto.ReadRequest;
+import com.google.bytestream.ByteStreamProto.ReadResponse;
+import com.google.bytestream.ByteStreamProto.WriteRequest;
+import com.google.bytestream.ByteStreamProto.WriteResponse;
+import com.google.devtools.build.lib.remote.logging.RemoteExecutionLog.LogEntry;
+import com.google.protobuf.ByteString;
+import io.grpc.Channel;
+import io.grpc.ClientInterceptors;
+import io.grpc.MethodDescriptor;
+import io.grpc.Server;
+import io.grpc.Status;
+import io.grpc.StatusRuntimeException;
+import io.grpc.inprocess.InProcessChannelBuilder;
+import io.grpc.inprocess.InProcessServerBuilder;
+import io.grpc.stub.StreamObserver;
+import io.grpc.util.MutableHandlerRegistry;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Mockito;
+
+/** Tests for {@link com.google.devtools.build.lib.remote.logging.LoggingInterceptor} */
+@RunWith(JUnit4.class)
+public class LoggingInterceptorTest {
+ private final String fakeServerName = "fake server for " + getClass();
+ private final MutableHandlerRegistry serviceRegistry = new MutableHandlerRegistry();
+ private Server fakeServer;
+
+ // This returns a logging interceptor where all calls are handled by the given handler.
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ private LoggingInterceptor getInterceptorWithAlwaysThisHandler(LoggingHandler handler) {
+ return new LoggingInterceptor() {
+ @Override
+ public <ReqT, RespT> LoggingHandler<ReqT, RespT> selectHandler(
+ MethodDescriptor<ReqT, RespT> method) {
+ return handler;
+ }
+ };
+ }
+
+ @Before
+ public final void setUp() throws Exception {
+ // Use a mutable service registry for later registering the service impl for each test case.
+ fakeServer =
+ InProcessServerBuilder.forName(fakeServerName)
+ .fallbackHandlerRegistry(serviceRegistry)
+ .directExecutor()
+ .build()
+ .start();
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ fakeServer.shutdownNow();
+ fakeServer.awaitTermination();
+ }
+
+ @Test
+ public void testCallOk() {
+ ReadRequest request = ReadRequest.newBuilder().setResourceName("test").build();
+ ReadResponse response =
+ ReadResponse.newBuilder().setData(ByteString.copyFromUtf8("abc")).build();
+
+ serviceRegistry.addService(
+ new ByteStreamImplBase() {
+ @Override
+ public void read(ReadRequest request, StreamObserver<ReadResponse> responseObserver) {
+ responseObserver.onNext(response);
+ responseObserver.onCompleted();
+ }
+ });
+
+ @SuppressWarnings("unchecked")
+ LoggingHandler<ReadRequest, ReadResponse> handler = Mockito.mock(LoggingHandler.class);
+ Mockito.when(handler.getEntry()).thenReturn(LogEntry.getDefaultInstance());
+
+ LoggingInterceptor interceptor = getInterceptorWithAlwaysThisHandler(handler);
+ Channel channel =
+ ClientInterceptors.intercept(
+ InProcessChannelBuilder.forName(fakeServerName).directExecutor().build(), interceptor);
+ ByteStreamBlockingStub stub = ByteStreamGrpc.newBlockingStub(channel);
+
+ stub.read(request).next();
+ verify(handler).handleReq(request);
+ verify(handler).handleResp(response);
+ verify(handler).getEntry();
+ }
+
+ @Test
+ public void testCallOkMultipleResponses() {
+ ReadRequest request = ReadRequest.newBuilder().setResourceName("test").build();
+ ReadResponse response1 =
+ ReadResponse.newBuilder().setData(ByteString.copyFromUtf8("abc")).build();
+ ReadResponse response2 =
+ ReadResponse.newBuilder().setData(ByteString.copyFromUtf8("def")).build();
+ serviceRegistry.addService(
+ new ByteStreamImplBase() {
+ @Override
+ public void read(ReadRequest request, StreamObserver<ReadResponse> responseObserver) {
+ responseObserver.onNext(response1);
+ responseObserver.onNext(response2);
+ responseObserver.onCompleted();
+ }
+ });
+
+ @SuppressWarnings("unchecked")
+ LoggingHandler<ReadRequest, ReadResponse> handler = Mockito.mock(LoggingHandler.class);
+ Mockito.when(handler.getEntry()).thenReturn(LogEntry.getDefaultInstance());
+
+ LoggingInterceptor interceptor = getInterceptorWithAlwaysThisHandler(handler);
+ Channel channel =
+ ClientInterceptors.intercept(
+ InProcessChannelBuilder.forName(fakeServerName).directExecutor().build(), interceptor);
+ ByteStreamBlockingStub stub = ByteStreamGrpc.newBlockingStub(channel);
+
+ // Read both responses.
+ advance(stub.read(request), 2);
+
+ ArgumentCaptor<ReadResponse> resultCaptor = ArgumentCaptor.forClass(ReadResponse.class);
+
+ verify(handler).handleReq(request);
+ verify(handler, times(2)).handleResp(resultCaptor.capture());
+ assertThat(resultCaptor.getAllValues().get(0)).isEqualTo(response1);
+ assertThat(resultCaptor.getAllValues().get(1)).isEqualTo(response2);
+ verify(handler).getEntry();
+ }
+
+ @Test
+ public void testCallOkMultipleRequests() {
+ WriteRequest request1 =
+ WriteRequest.newBuilder()
+ .setResourceName("test")
+ .setData(ByteString.copyFromUtf8("abc"))
+ .build();
+ WriteRequest request2 =
+ WriteRequest.newBuilder()
+ .setResourceName("test")
+ .setData(ByteString.copyFromUtf8("def"))
+ .build();
+ WriteResponse response = WriteResponse.newBuilder().setCommittedSize(6).build();
+ serviceRegistry.addService(
+ new ByteStreamImplBase() {
+ @Override
+ public StreamObserver<WriteRequest> write(StreamObserver<WriteResponse> streamObserver) {
+ return new StreamObserver<WriteRequest>() {
+ @Override
+ public void onNext(WriteRequest writeRequest) {}
+
+ @Override
+ public void onError(Throwable throwable) {}
+
+ @Override
+ public void onCompleted() {
+ streamObserver.onNext(response);
+ streamObserver.onCompleted();
+ }
+ };
+ }
+ });
+
+ @SuppressWarnings("unchecked")
+ LoggingHandler<WriteRequest, WriteResponse> handler = Mockito.mock(LoggingHandler.class);
+ Mockito.when(handler.getEntry()).thenReturn(LogEntry.getDefaultInstance());
+
+ LoggingInterceptor interceptor = getInterceptorWithAlwaysThisHandler(handler);
+ Channel channel =
+ ClientInterceptors.intercept(
+ InProcessChannelBuilder.forName(fakeServerName).directExecutor().build(), interceptor);
+ ByteStreamStub stub = ByteStreamGrpc.newStub(channel);
+
+ @SuppressWarnings("unchecked")
+ StreamObserver<WriteResponse> responseObserver = Mockito.mock(StreamObserver.class);
+ // Write both responses.
+ StreamObserver<WriteRequest> requester = stub.write(responseObserver);
+ requester.onNext(request1);
+ requester.onNext(request2);
+ requester.onCompleted();
+
+ ArgumentCaptor<WriteRequest> resultCaptor = ArgumentCaptor.forClass(WriteRequest.class);
+
+ verify(handler, times(2)).handleReq(resultCaptor.capture());
+ assertThat(resultCaptor.getAllValues().get(0)).isEqualTo(request1);
+ assertThat(resultCaptor.getAllValues().get(1)).isEqualTo(request2);
+ verify(handler).handleResp(response);
+ verify(handler).getEntry();
+ }
+
+ @Test
+ public void testCallWithError() {
+ ReadRequest request = ReadRequest.newBuilder().setResourceName("test").build();
+ Status error = Status.NOT_FOUND.withDescription("not found");
+
+ serviceRegistry.addService(
+ new ByteStreamImplBase() {
+ @Override
+ public void read(ReadRequest request, StreamObserver<ReadResponse> responseObserver) {
+ responseObserver.onError(error.asRuntimeException());
+ }
+ });
+
+ @SuppressWarnings("unchecked")
+ LoggingHandler<ReadRequest, ReadResponse> handler = Mockito.mock(LoggingHandler.class);
+ Mockito.when(handler.getEntry()).thenReturn(LogEntry.getDefaultInstance());
+
+ LoggingInterceptor interceptor = getInterceptorWithAlwaysThisHandler(handler);
+ Channel channel =
+ ClientInterceptors.intercept(
+ InProcessChannelBuilder.forName(fakeServerName).directExecutor().build(), interceptor);
+ ByteStreamBlockingStub stub = ByteStreamGrpc.newBlockingStub(channel);
+
+ assertThrows(StatusRuntimeException.class, () -> stub.read(request).next());
+
+ verify(handler).handleReq(request);
+ verify(handler, never()).handleResp(any());
+ verify(handler).getEntry();
+ }
+}
