Renovates gRPC instrumentation in preparation of standard parsing

.mvn/wrapper/maven-wrapper.properties

@@ -1,2 +1,2 @@
 distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.6.3/apache-maven-3.6.3-bin.zip
-wrapperUrl=https://repo.maven.apache.org/maven2/io/takari/maven-wrapper/0.5.6/maven-wrapper-0.5.6.jar
+wrapperUrl=https://repo.maven.apache.org/maven2/io/takari/maven-wrapper/0.5.6/maven-wrapper-0.5.6.jar

instrumentation/benchmarks/pom.xml

@@ -248,7 +248,7 @@
     </dependency>
     <dependency>
       <groupId>io.grpc</groupId>
-      <artifactId>grpc-core</artifactId>
+      <artifactId>grpc-testing</artifactId>
       <version>${grpc.version}</version>
     </dependency>
   </dependencies>

instrumentation/benchmarks/src/main/java/brave/grpc/GrpcPropagationBenchmarks.java

@@ -21,9 +21,12 @@
 import brave.propagation.TraceContext.Extractor;
 import brave.propagation.TraceContext.Injector;
 import brave.propagation.TraceContextOrSamplingFlags;
+import io.grpc.CallOptions;
 import io.grpc.Metadata;
 import io.grpc.Metadata.Key;
 import io.grpc.MethodDescriptor;
+import io.grpc.internal.NoopClientCall;
+import io.grpc.internal.NoopServerCall;
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
@@ -84,15 +87,14 @@ public class GrpcPropagationBenchmarks {
     bothNameToKey = nameToKey(both);
 
   static final GrpcServerRequest
-    incomingB3 = new GrpcServerRequest(b3NameToKey, methodDescriptor, new Metadata()),
-    incomingBoth = new GrpcServerRequest(bothNameToKey, methodDescriptor, new Metadata()),
-    incomingBothNoTags = new GrpcServerRequest(b3NameToKey, methodDescriptor, new Metadata()),
-    nothingIncoming = new GrpcServerRequest(emptyMap(), methodDescriptor, new Metadata());
+    incomingB3 = new GrpcServerRequest(b3NameToKey, new NoopServerCall<>(), new Metadata()),
+    incomingBoth = new GrpcServerRequest(bothNameToKey, new NoopServerCall<>(), new Metadata()),
+    incomingBothNoTags = new GrpcServerRequest(b3NameToKey, new NoopServerCall<>(), new Metadata()),
+    nothingIncoming = new GrpcServerRequest(emptyMap(), new NoopServerCall<>(), new Metadata());
 
   static final byte[] tagsBytes;
 
   static {
-
     try {
       ByteArrayOutputStream bytes = new ByteArrayOutputStream();
       bytes.write(0); // version
@@ -107,19 +109,18 @@ public class GrpcPropagationBenchmarks {
     }
 
     contextWithTags = context.toBuilder().extra(singletonList(new TagsBin(tagsBytes))).build();
-    b3Injector.inject(context, new GrpcClientRequest(b3NameToKey, methodDescriptor)
-      .metadata(incomingB3.metadata));
-    bothInjector.inject(contextWithTags,
-      new GrpcClientRequest(bothNameToKey, methodDescriptor)
-        .metadata(incomingBoth.metadata));
-    bothInjector.inject(context,
-      new GrpcClientRequest(bothNameToKey, methodDescriptor)
-        .metadata(incomingBothNoTags.metadata));
+    b3Injector.inject(context, noopRequest(b3NameToKey, incomingB3.headers));
+    bothInjector.inject(contextWithTags, noopRequest(bothNameToKey, incomingBoth.headers));
+    bothInjector.inject(context, noopRequest(bothNameToKey, incomingBothNoTags.headers));
+  }
+
+  static GrpcClientRequest noopRequest(Map<String, Key<String>> nameToKey, Metadata headers) {
+    return new GrpcClientRequest(nameToKey, methodDescriptor, CallOptions.DEFAULT,
+      new NoopClientCall<>(), headers);
   }
 
   @Benchmark public void inject_b3() {
-    GrpcClientRequest request =
-      new GrpcClientRequest(b3NameToKey, methodDescriptor).metadata(new Metadata());
+    GrpcClientRequest request = noopRequest(b3NameToKey, new Metadata());
     b3Injector.inject(context, request);
   }
 
@@ -132,14 +133,12 @@ public class GrpcPropagationBenchmarks {
   }
 
   @Benchmark public void inject_both() {
-    GrpcClientRequest request =
-      new GrpcClientRequest(bothNameToKey, methodDescriptor).metadata(new Metadata());
+    GrpcClientRequest request = noopRequest(bothNameToKey, new Metadata());
     bothInjector.inject(contextWithTags, request);
   }
 
   @Benchmark public void inject_both_no_tags() {
-    GrpcClientRequest request =
-      new GrpcClientRequest(bothNameToKey, methodDescriptor).metadata(new Metadata());
+    GrpcClientRequest request = noopRequest(bothNameToKey, new Metadata());
     bothInjector.inject(context, request);
   }
 

instrumentation/grpc/RATIONALE.md

@@ -0,0 +1,27 @@
+# brave-instrumentation-grpc rationale
+Please see [RPC](../rpc/RATIONALE.md) for basic rational about RPC instrumentation.
+
+## Why don't we record exceptions thrown by `ClientCall.Listener.onClose()` or `ServerCall.close()`
+`ClientCall.Listener.onClose()` or `ServerCall.close()` could throw an
+exception after the corresponding call succeeded. However, we do not catch this
+and add it to the span.
+
+The reason could be more obvious if you consider the synchronous alternative:
+
+```java
+futureStub.hello().thenApply(response -> throw new IllegalStateException("I'm a bad user callback"))
+// vs
+response = stub.hello();
+throw new IllegalStateException("I'm a bad user code");
+```
+
+In short, the reason we don't fail the CLIENT or SERVER span is that it its
+success or failure is independent of the 3rd party (possibly user) callbacks.
+
+This doesn't mean it won't be an error recorded in the trace, either! We have
+no insight to layers over gRPC, which themselves could be instrumented and
+record the application exception.
+
+In short, we choose to not mask the gRPC status with a potential application
+exception on close. In worst case, if there is no instrumentation for the layer
+that throws, its trace and span ID could be known via log correlation.

instrumentation/grpc/src/it/grpc12/pom.xml

@@ -89,9 +89,14 @@
         <artifactId>maven-compiler-plugin</artifactId>
         <version>@maven-compiler-plugin.version@</version>
         <configuration>
-          <excludes>
-            <exclude>**/IT*.java</exclude>
-          </excludes>
+          <!-- Only compiling types involved in integration tests avoids an otherwise unnecessary
+               Mockito dependency -->
+          <includes>
+            <include>**/BaseIT*.java</include>
+            <include>**/GreeterImpl.java</include>
+            <include>**/PickUnusedPort.java</include>
+            <include>**/TestServer.java</include>
+          </includes>
         </configuration>
       </plugin>
       <plugin>

instrumentation/grpc/src/it/grpc12/src/test/resources/log4j2.properties

@@ -0,0 +1,8 @@
+appenders=console
+appender.console.type=Console
+appender.console.name=STDOUT
+appender.console.layout.type=PatternLayout
+appender.console.layout.pattern=%d{ABSOLUTE} %-5p [%t] %C{2} (%F:%L) [%X{traceId}/%X{spanId}] - %m%n
+rootLogger.level=info
+rootLogger.appenderRefs=stdout
+rootLogger.appenderRef.stdout.ref=STDOUT

instrumentation/grpc/src/main/java/brave/grpc/GrpcClientRequest.java

@@ -13,72 +13,112 @@
  */
 package brave.grpc;
 
-import brave.internal.Nullable;
 import brave.propagation.Propagation.Setter;
 import brave.rpc.RpcClientRequest;
+import io.grpc.CallOptions;
+import io.grpc.Channel;
 import io.grpc.ClientCall;
+import io.grpc.ClientInterceptor;
 import io.grpc.Metadata;
 import io.grpc.Metadata.Key;
 import io.grpc.MethodDescriptor;
 import java.util.Map;
 
 // intentionally not yet public until we add tag parsing functionality
 final class GrpcClientRequest extends RpcClientRequest {
-  static final Setter<GrpcClientRequest, String> SETTER =
-    new Setter<GrpcClientRequest, String>() { // retrolambda no like
-      @Override public void put(GrpcClientRequest request, String key, String value) {
-        request.setMetadata(key, value);
-      }
+  static final Setter<GrpcClientRequest, String> SETTER = new Setter<GrpcClientRequest, String>() {
+    @Override public void put(GrpcClientRequest request, String key, String value) {
+      request.propagationField(key, value);
+    }
 
-      @Override public String toString() {
-        return "GrpcClientRequest::setMetadata";
-      }
-    };
+    @Override public String toString() {
+      return "GrpcClientRequest::propagationField";
+    }
+  };
 
   final Map<String, Key<String>> nameToKey;
-  final String fullMethodName;
-  @Nullable volatile Metadata metadata; // nullable due to lifecycle of gRPC request
+  final MethodDescriptor<?, ?> methodDescriptor;
+  final CallOptions callOptions;
+  final ClientCall<?, ?> call;
+  final Metadata headers;
 
-  GrpcClientRequest(Map<String, Key<String>> nameToKey, MethodDescriptor<?, ?> methodDescriptor) {
+  GrpcClientRequest(Map<String, Key<String>> nameToKey, MethodDescriptor<?, ?> methodDescriptor,
+    CallOptions callOptions, ClientCall<?, ?> call, Metadata headers) {
     if (nameToKey == null) throw new NullPointerException("nameToKey == null");
     if (methodDescriptor == null) throw new NullPointerException("methodDescriptor == null");
+    if (callOptions == null) throw new NullPointerException("callOptions == null");
+    if (call == null) throw new NullPointerException("call == null");
+    if (headers == null) throw new NullPointerException("headers == null");
     this.nameToKey = nameToKey;
-    this.fullMethodName = methodDescriptor.getFullMethodName();
+    this.methodDescriptor = methodDescriptor;
+    this.callOptions = callOptions;
+    this.call = call;
+    this.headers = headers;
   }
 
+  /** Returns the {@link #call()} */
   @Override public Object unwrap() {
-    return this;
+    return call;
   }
 
   @Override public String method() {
-    return GrpcParser.method(fullMethodName);
+    return GrpcParser.method(methodDescriptor.getFullMethodName());
   }
 
   @Override public String service() {
-    return GrpcParser.service(fullMethodName);
+    // MethodDescriptor.getServiceName() is not in our floor version: gRPC 1.2
+    return GrpcParser.service(methodDescriptor.getFullMethodName());
   }
 
-  /** Call on {@link ClientCall#start(ClientCall.Listener, Metadata)} */
-  GrpcClientRequest metadata(Metadata metadata) {
-    this.metadata = metadata;
-    return this;
+  /**
+   * Returns the {@linkplain MethodDescriptor method descriptor} passed to {@link
+   * ClientInterceptor#interceptCall}.
+   *
+   * @since 5.12
+   */
+  public MethodDescriptor<?, ?> methodDescriptor() {
+    return methodDescriptor;
   }
 
-  void setMetadata(String name, String value) {
-    if (name == null) throw new NullPointerException("name == null");
-    if (value == null) throw new NullPointerException("value == null");
+  /**
+   * Returns the {@linkplain CallOptions call options} passed to {@link
+   * ClientInterceptor#interceptCall}.
+   *
+   * @since 5.12
+   */
+  public CallOptions callOptions() {
+    return callOptions;
+  }
 
-    Metadata metadata = this.metadata;
-    if (metadata == null) {
-      assert false : "This code should never be called when null";
-      return;
-    }
-    Key<String> key = nameToKey.get(name);
+  /**
+   * Returns the {@linkplain ClientCall client call} generated by {@link Channel#newCall} during
+   * {@link ClientInterceptor#interceptCall}.
+   *
+   * @since 5.12
+   */
+  public ClientCall<?, ?> call() {
+    return call;
+  }
+
+  /**
+   * Returns the {@linkplain Metadata headers}  passed to {@link ClientCall#start(ClientCall.Listener,
+   * Metadata)}.
+   *
+   * @since 5.12
+   */
+  public Metadata headers() {
+    return headers;
+  }
+
+  void propagationField(String keyName, String value) {
+    if (keyName == null) throw new NullPointerException("keyName == null");
+    if (value == null) throw new NullPointerException("value == null");
+    Key<String> key = nameToKey.get(keyName);
     if (key == null) {
-      assert false : "We currently don't support setting metadata except propagation fields";
+      assert false : "We currently don't support setting headers except propagation fields";
       return;
     }
-    metadata.removeAll(key);
-    metadata.put(key, value);
+    headers.removeAll(key);
+    headers.put(key, value);
   }
 }

instrumentation/grpc/src/main/java/brave/grpc/GrpcClientResponse.java

@@ -0,0 +1,84 @@
+/*
+ * Copyright 2013-2020 The OpenZipkin Authors
+ *
+ * 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 brave.grpc;
+
+import brave.Response;
+import brave.Span;
+import brave.internal.Nullable;
+import io.grpc.ClientCall;
+import io.grpc.Metadata;
+import io.grpc.Status;
+
+// intentionally not yet public until we add tag parsing functionality
+final class GrpcClientResponse extends Response {
+  final GrpcClientRequest request;
+  @Nullable final Status status;
+  @Nullable final Metadata trailers;
+  @Nullable final Throwable error;
+
+  GrpcClientResponse(GrpcClientRequest request,
+    @Nullable Status status, @Nullable Metadata trailers, @Nullable Throwable error) {
+    if (request == null) throw new NullPointerException("request == null");
+    this.request = request;
+    this.status = status;
+    this.trailers = trailers;
+    this.error = error != null ? error : status != null ? status.getCause() : null;
+  }
+
+  @Override public Span.Kind spanKind() {
+    return Span.Kind.CLIENT;
+  }
+
+  /** Returns the {@link #status()} */
+  @Override @Nullable public Status unwrap() {
+    return status;
+  }
+
+  @Override public GrpcClientRequest request() {
+    return request;
+  }
+
+  @Override @Nullable public Throwable error() {
+    return error;
+  }
+
+  /**
+   * Returns the string form of the {@link Status#getCode()} or {@code null} when not {@link
+   * Status#isOk()} or {@link #error()}.
+   */
+  @Nullable public String errorCode() {
+    if (status == null || status.isOk()) return null;
+    return status.getCode().name();
+  }
+
+  /**
+   * Returns the status passed to {@link ClientCall.Listener#onClose(Status, Metadata)} or {@code
+   * null} on {@link #error()}.
+   *
+   * @since 5.12
+   */
+  @Nullable public Status status() {
+    return status;
+  }
+
+  /**
+   * Returns the trailers passed to {@link ClientCall.Listener#onClose(Status, Metadata)} or {@code
+   * null} on {@link #error()}.
+   *
+   * @since 5.12
+   */
+  @Nullable public Metadata trailers() {
+    return trailers;
+  }
+}