Implement spec changes for grpc server span error status (#9641)

open-telemetry · Oct 10, 2023 · 13585c6 · 13585c6
1 parent 6980b69
commit 13585c6
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 14 deletions.
diff --git a/...ary/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/GrpcSpanStatusExtractor.java b/...ary/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/GrpcSpanStatusExtractor.java
@@ -5,15 +5,29 @@
 
 package io.opentelemetry.instrumentation.grpc.v1_6;
 
+import com.google.errorprone.annotations.Immutable;
 import io.grpc.Status;
 import io.grpc.StatusException;
 import io.grpc.StatusRuntimeException;
 import io.opentelemetry.api.trace.StatusCode;
 import io.opentelemetry.instrumentation.api.instrumenter.SpanStatusBuilder;
 import io.opentelemetry.instrumentation.api.instrumenter.SpanStatusExtractor;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.function.Predicate;
 import javax.annotation.Nullable;
 
-final class GrpcSpanStatusExtractor implements SpanStatusExtractor<GrpcRequest, Status> {
+enum GrpcSpanStatusExtractor implements SpanStatusExtractor<GrpcRequest, Status> {
+  CLIENT(GrpcSpanStatusExtractor::isClientError),
+  SERVER(GrpcSpanStatusExtractor::isServerError);
+
+  private final ErrorStatusPredicate isError;
+
+  GrpcSpanStatusExtractor(ErrorStatusPredicate isError) {
+    this.isError = isError;
+  }
+
   @Override
   public void extract(
       SpanStatusBuilder spanStatusBuilder,
@@ -28,11 +42,35 @@ public void extract(
       }
     }
     if (status != null) {
-      if (!status.isOk()) {
+      if (isError.test(status)) {
         spanStatusBuilder.setStatus(StatusCode.ERROR);
       }
     } else {
       SpanStatusExtractor.getDefault().extract(spanStatusBuilder, request, status, error);
     }
   }
+
+  private static final Set<Status.Code> serverErrorStatuses = new HashSet<>();
+
+  static {
+    serverErrorStatuses.addAll(
+        Arrays.asList(
+            Status.Code.UNKNOWN,
+            Status.Code.DEADLINE_EXCEEDED,
+            Status.Code.UNIMPLEMENTED,
+            Status.Code.INTERNAL,
+            Status.Code.UNAVAILABLE,
+            Status.Code.DATA_LOSS));
+  }
+
+  private static boolean isServerError(Status status) {
+    return serverErrorStatuses.contains(status.getCode());
+  }
+
+  private static boolean isClientError(Status status) {
+    return !status.isOk();
+  }
+
+  @Immutable
+  private interface ErrorStatusPredicate extends Predicate<Status> {}
 }
diff --git a/...ibrary/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/GrpcTelemetryBuilder.java b/...ibrary/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/GrpcTelemetryBuilder.java
@@ -25,7 +25,6 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.function.Function;
-import java.util.stream.Stream;
 import javax.annotation.Nullable;
 
 /** A builder of {@link GrpcTelemetry}. */
@@ -167,20 +166,15 @@ public GrpcTelemetry build() {
     InstrumenterBuilder<GrpcRequest, Status> serverInstrumenterBuilder =
         Instrumenter.builder(openTelemetry, INSTRUMENTATION_NAME, serverSpanNameExtractor);
 
-    Stream.of(clientInstrumenterBuilder, serverInstrumenterBuilder)
-        .forEach(
-            instrumenter ->
-                instrumenter
-                    .setSpanStatusExtractor(new GrpcSpanStatusExtractor())
-                    .addAttributesExtractors(additionalExtractors));
-
     GrpcClientNetworkAttributesGetter netClientAttributesGetter =
         new GrpcClientNetworkAttributesGetter();
     GrpcNetworkServerAttributesGetter netServerAttributesGetter =
         new GrpcNetworkServerAttributesGetter();
     GrpcRpcAttributesGetter rpcAttributesGetter = GrpcRpcAttributesGetter.INSTANCE;
 
     clientInstrumenterBuilder
+        .setSpanStatusExtractor(GrpcSpanStatusExtractor.CLIENT)
+        .addAttributesExtractors(additionalExtractors)
         .addAttributesExtractor(RpcClientAttributesExtractor.create(rpcAttributesGetter))
         .addAttributesExtractor(ServerAttributesExtractor.create(netClientAttributesGetter))
         .addAttributesExtractors(additionalClientExtractors)
@@ -189,6 +183,8 @@ public GrpcTelemetry build() {
                 GrpcRpcAttributesGetter.INSTANCE, capturedClientRequestMetadata))
         .addOperationMetrics(RpcClientMetrics.get());
     serverInstrumenterBuilder
+        .setSpanStatusExtractor(GrpcSpanStatusExtractor.SERVER)
+        .addAttributesExtractors(additionalExtractors)
         .addAttributesExtractor(RpcServerAttributesExtractor.create(rpcAttributesGetter))
         .addAttributesExtractor(
             ServerAttributesExtractor.createForServerSide(netServerAttributesGetter))

diff --git a/....6/testing/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/AbstractGrpcTest.java b/....6/testing/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/AbstractGrpcTest.java
@@ -604,6 +604,7 @@ public void sayHello(
               assertThat(t.getStatus().getDescription()).isEqualTo(status.getDescription());
             });
 
+    boolean isServerError = status.getCode() != Status.Code.NOT_FOUND;
     testing()
         .waitAndAssertTraces(
             trace ->
@@ -635,7 +636,7 @@ public void sayHello(
                         span.hasName("example.Greeter/SayHello")
                             .hasKind(SpanKind.SERVER)
                             .hasParent(trace.getSpan(0))
-                            .hasStatus(StatusData.error())
+                            .hasStatus(isServerError ? StatusData.error() : StatusData.unset())
                             .hasAttributesSatisfyingExactly(
                                 equalTo(SemanticAttributes.RPC_SYSTEM, "grpc"),
                                 equalTo(SemanticAttributes.RPC_SERVICE, "example.Greeter"),
@@ -863,11 +864,19 @@ static class ErrorProvider implements ArgumentsProvider {
     public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
       return Stream.of(
           arguments(Status.UNKNOWN.withCause(new RuntimeException("some error"))),
-          arguments(Status.PERMISSION_DENIED.withCause(new RuntimeException("some error"))),
+          arguments(Status.DEADLINE_EXCEEDED.withCause(new RuntimeException("some error"))),
           arguments(Status.UNIMPLEMENTED.withCause(new RuntimeException("some error"))),
+          arguments(Status.INTERNAL.withCause(new RuntimeException("some error"))),
+          arguments(Status.UNAVAILABLE.withCause(new RuntimeException("some error"))),
+          arguments(Status.DATA_LOSS.withCause(new RuntimeException("some error"))),
+          arguments(Status.NOT_FOUND.withCause(new RuntimeException("some error"))),
           arguments(Status.UNKNOWN.withDescription("some description")),
-          arguments(Status.PERMISSION_DENIED.withDescription("some description")),
-          arguments(Status.UNIMPLEMENTED.withDescription("some description")));
+          arguments(Status.DEADLINE_EXCEEDED.withDescription("some description")),
+          arguments(Status.UNIMPLEMENTED.withDescription("some description")),
+          arguments(Status.INTERNAL.withDescription("some description")),
+          arguments(Status.UNAVAILABLE.withDescription("some description")),
+          arguments(Status.DATA_LOSS.withDescription("some description")),
+          arguments(Status.NOT_FOUND.withDescription("some description")));
     }
   }