Add dual-semconv support to RPC attributes extractors

instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/rpc/RpcAttributesGetter.java

@@ -69,4 +69,31 @@ default String getErrorType(
       REQUEST request, @Nullable RESPONSE response, @Nullable Throwable error) {
     return null;
   }
+
+  /**
+   * Returns whether the RPC method is recognized as a predefined method by the RPC framework or
+   * library.
+   *
+   * <p>Some RPC frameworks or libraries provide a fixed set of recognized methods for client stubs
+   * and server implementations. Instrumentations for such frameworks MUST return {@code true} only
+   * when the method is recognized by the framework or library.
+   *
+   * <p>When the method is not recognized (for example, when the server receives a request for a
+   * method that is not predefined on the server), or when instrumentation is not able to reliably
+   * detect if the method is predefined, this method MUST return {@code false}.
+   *
+   * <p>When this method returns {@code false}, the {@code rpc.method} attribute will be set to
+   * {@code "_OTHER"} and the {@code rpc.method_original} attribute will be set to the original
+   * method name.
+   *
+   * <p>Note: If the RPC instrumentation could end up converting valid RPC methods to {@code
+   * "_OTHER"}, then it SHOULD provide a way to configure the list of recognized RPC methods.
+   *
+   * @param request the request object
+   * @return {@code true} if the method is recognized as predefined by the framework, {@code false}
+   *     otherwise
+   */
+  default boolean isPredefined(REQUEST request) {
+    return false;
+  }
 }

instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/rpc/RpcCommonAttributesExtractor.java

@@ -6,21 +6,32 @@
 package io.opentelemetry.instrumentation.api.incubator.semconv.rpc;
 
 import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;
+import static io.opentelemetry.semconv.ErrorAttributes.ERROR_TYPE;
 
 import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
+import io.opentelemetry.instrumentation.api.internal.SemconvStability;
 import javax.annotation.Nullable;
 
 abstract class RpcCommonAttributesExtractor<REQUEST, RESPONSE>
     implements AttributesExtractor<REQUEST, RESPONSE> {
 
-  // copied from RpcIncubatingAttributes
   static final AttributeKey<String> RPC_METHOD = AttributeKey.stringKey("rpc.method");
+
+  // Stable semconv keys
+  static final AttributeKey<String> RPC_SYSTEM_NAME = AttributeKey.stringKey("rpc.system.name");
+
+  // removed in stable semconv (merged into rpc.method)
   static final AttributeKey<String> RPC_SERVICE = AttributeKey.stringKey("rpc.service");
+
+  // use RPC_SYSTEM_NAME for stable semconv
   static final AttributeKey<String> RPC_SYSTEM = AttributeKey.stringKey("rpc.system");
 
+  static final AttributeKey<String> RPC_METHOD_ORIGINAL =
+      AttributeKey.stringKey("rpc.method_original");
+
   private final RpcAttributesGetter<REQUEST, RESPONSE> getter;
 
   RpcCommonAttributesExtractor(RpcAttributesGetter<REQUEST, RESPONSE> getter) {
@@ -30,9 +41,30 @@ abstract class RpcCommonAttributesExtractor<REQUEST, RESPONSE>
   @SuppressWarnings("deprecation") // for getMethod()
   @Override
   public final void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
-    internalSet(attributes, RPC_SYSTEM, getter.getSystem(request));
-    internalSet(attributes, RPC_SERVICE, getter.getService(request));
-    internalSet(attributes, RPC_METHOD, getter.getMethod(request));
+    String system = getter.getSystem(request);
+
+    if (SemconvStability.emitStableRpcSemconv()) {
+      internalSet(
+          attributes,
+          RPC_SYSTEM_NAME,
+          system == null ? null : SemconvStability.stableRpcSystemName(system));
+      String method = getter.getRpcMethod(request);
+      if (getter.isPredefined(request)) {
+        internalSet(attributes, RPC_METHOD, method);
+      } else {
+        internalSet(attributes, RPC_METHOD_ORIGINAL, method);
+        internalSet(attributes, RPC_METHOD, "_OTHER");
+      }
+    }
+
+    if (SemconvStability.emitOldRpcSemconv()) {
+      internalSet(attributes, RPC_SYSTEM, system);
+      internalSet(attributes, RPC_SERVICE, getter.getService(request));
+      if (!SemconvStability.emitStableRpcSemconv()) {
+        // only set old rpc.method on spans when there's no clash with stable rpc.method
+        internalSet(attributes, RPC_METHOD, getter.getMethod(request));
+      }
+    }
   }
 
   @Override
@@ -42,6 +74,13 @@ public final void onEnd(
       REQUEST request,
       @Nullable RESPONSE response,
       @Nullable Throwable error) {
-    // No response attributes
+    if (SemconvStability.emitStableRpcSemconv()) {
+      String errorType = getter.getErrorType(request, response, error);
+      // fall back to exception class name & _OTHER
+      if (errorType == null && error != null) {
+        errorType = error.getClass().getName();
+      }
+      internalSet(attributes, ERROR_TYPE, errorType);
+    }
   }
 }

instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/rpc/RpcSpanNameExtractor.java

@@ -6,6 +6,7 @@
 package io.opentelemetry.instrumentation.api.incubator.semconv.rpc;
 
 import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;
+import io.opentelemetry.instrumentation.api.internal.SemconvStability;
 
 /** A {@link SpanNameExtractor} for RPC requests. */
 public final class RpcSpanNameExtractor<REQUEST> implements SpanNameExtractor<REQUEST> {
@@ -28,6 +29,18 @@ private RpcSpanNameExtractor(RpcAttributesGetter<REQUEST, ?> getter) {
   @SuppressWarnings("deprecation") // for getMethod()
   @Override
   public String extract(REQUEST request) {
+    if (SemconvStability.emitStableRpcSemconv()) {
+      String method = getter.getRpcMethod(request);
+      if (method != null) {
+        return method;
+      }
+      String system = getter.getSystem(request);
+      if (system != null) {
+        return system;
+      }
+      return "RPC request";
+    }
+
     String service = getter.getService(request);
     String method = getter.getMethod(request);
     if (service == null || method == null) {

instrumentation-api-incubator/src/test/java/io/opentelemetry/instrumentation/api/incubator/semconv/rpc/RpcAttributesExtractorTest.java

@@ -5,23 +5,35 @@
 
 package io.opentelemetry.instrumentation.api.incubator.semconv.rpc;
 
+import static io.opentelemetry.instrumentation.api.incubator.semconv.rpc.RpcCommonAttributesExtractor.RPC_METHOD_ORIGINAL;
 import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;
+import static io.opentelemetry.semconv.ErrorAttributes.ERROR_TYPE;
 import static org.assertj.core.api.Assertions.entry;
 
+import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
+import io.opentelemetry.instrumentation.api.internal.SemconvStability;
 import io.opentelemetry.semconv.incubating.RpcIncubatingAttributes;
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
+import javax.annotation.Nullable;
 import org.junit.jupiter.api.Test;
 
 @SuppressWarnings("deprecation") // using deprecated semconv
 class RpcAttributesExtractorTest {
 
-  enum TestGetter implements RpcAttributesGetter<Map<String, String>, Void> {
-    INSTANCE;
+  private static class TestGetter implements RpcAttributesGetter<Map<String, String>, Void> {
+
+    private final boolean predefined;
+
+    private TestGetter(boolean predefined) {
+      this.predefined = predefined;
+    }
 
     @Override
     public String getSystem(Map<String, String> request) {
@@ -38,37 +50,160 @@ public String getService(Map<String, String> request) {
     public String getMethod(Map<String, String> request) {
       return request.get("method");
     }
+
+    @Nullable
+    @Override
+    public String getRpcMethod(Map<String, String> request) {
+      String service = getService(request);
+      String method = getMethod(request);
+      if (service == null || method == null) {
+        return null;
+      }
+      return service + "/" + method;
+    }
+
+    @Nullable
+    @Override
+    public String getErrorType(
+        Map<String, String> request, @Nullable Void response, @Nullable Throwable error) {
+      return request.get("errorType");
+    }
+
+    @Override
+    public boolean isPredefined(Map<String, String> stringStringMap) {
+      return predefined;
+    }
   }
 
   @Test
   void server() {
-    testExtractor(RpcServerAttributesExtractor.create(TestGetter.INSTANCE));
+    testExtractor(RpcServerAttributesExtractor.create(new TestGetter(false)), "my.Service/Method");
+  }
+
+  @Test
+  void serverPredefined() {
+    testExtractor(RpcServerAttributesExtractor.create(new TestGetter(true)), null);
   }
 
   @Test
   void client() {
-    testExtractor(RpcClientAttributesExtractor.create(TestGetter.INSTANCE));
+    testExtractor(RpcClientAttributesExtractor.create(new TestGetter(false)), "my.Service/Method");
+  }
+
+  @Test
+  void clientPredefined() {
+    testExtractor(RpcClientAttributesExtractor.create(new TestGetter(true)), null);
+  }
+
+  // Stable semconv keys
+  private static final AttributeKey<String> RPC_SYSTEM_NAME =
+      AttributeKey.stringKey("rpc.system.name");
+
+  // Old semconv keys (from RpcIncubatingAttributes)
+  private static final AttributeKey<String> RPC_SYSTEM = RpcIncubatingAttributes.RPC_SYSTEM;
+
+  private static final AttributeKey<String> RPC_SERVICE = RpcIncubatingAttributes.RPC_SERVICE;
+
+  private static final AttributeKey<String> RPC_METHOD = RpcIncubatingAttributes.RPC_METHOD;
+
+  private static void testExtractor(
+      AttributesExtractor<Map<String, String>, Void> extractor, @Nullable String originalMethod) {
+    Map<String, String> request = new HashMap<>();
+    request.put("service", "my.Service");
+    request.put("method", "Method");
+
+    Context context = Context.root();
+
+    AttributesBuilder attributes = Attributes.builder();
+    extractor.onStart(attributes, context, request);
+
+    // Build expected entries list based on semconv mode
+    List<Map.Entry<? extends AttributeKey<?>, ?>> expectedEntries = new ArrayList<>();
+
+    if (SemconvStability.emitStableRpcSemconv()) {
+      expectedEntries.add(entry(RPC_SYSTEM_NAME, "test"));
+      if (originalMethod != null) {
+        expectedEntries.add(entry(RPC_METHOD_ORIGINAL, originalMethod));
+        expectedEntries.add(entry(RPC_METHOD, "_OTHER"));
+      } else {
+        expectedEntries.add(entry(RPC_METHOD, "my.Service/Method"));
+      }
+    }
+
+    if (SemconvStability.emitOldRpcSemconv()) {
+      expectedEntries.add(entry(RPC_SYSTEM, "test"));
+      expectedEntries.add(entry(RPC_SERVICE, "my.Service"));
+      if (!SemconvStability.emitStableRpcSemconv()) {
+        expectedEntries.add(entry(RPC_METHOD, "Method"));
+      }
+    }
+
+    // safe conversion for test assertions
+    @SuppressWarnings({"unchecked", "rawtypes"})
+    Map.Entry<? extends AttributeKey<?>, ?>[] expectedArray =
+        (Map.Entry<? extends AttributeKey<?>, ?>[]) expectedEntries.toArray(new Map.Entry[0]);
+    assertThat(attributes.build()).containsOnly(expectedArray);
+
+    extractor.onEnd(attributes, context, request, null, null);
+    assertThat(attributes.build()).containsOnly(expectedArray);
   }
 
-  private static void testExtractor(AttributesExtractor<Map<String, String>, Void> extractor) {
+  @Test
+  void shouldExtractErrorType_getter() {
     Map<String, String> request = new HashMap<>();
     request.put("service", "my.Service");
     request.put("method", "Method");
+    request.put("errorType", "CANCELLED");
+
+    AttributesExtractor<Map<String, String>, Void> extractor =
+        RpcServerAttributesExtractor.create(new TestGetter(false));
 
     Context context = Context.root();
+    AttributesBuilder attributes = Attributes.builder();
+    extractor.onStart(attributes, context, request);
+    extractor.onEnd(attributes, context, request, null, null);
+
+    if (SemconvStability.emitStableRpcSemconv()) {
+      assertThat(attributes.build()).containsEntry(ERROR_TYPE, "CANCELLED");
+    }
+  }
+
+  @Test
+  void shouldExtractErrorType_exceptionClassName() {
+    Map<String, String> request = new HashMap<>();
+    request.put("service", "my.Service");
+    request.put("method", "Method");
 
+    AttributesExtractor<Map<String, String>, Void> extractor =
+        RpcServerAttributesExtractor.create(new TestGetter(false));
+
+    Context context = Context.root();
+    AttributesBuilder attributes = Attributes.builder();
+    extractor.onStart(attributes, context, request);
+    extractor.onEnd(attributes, context, request, null, new IllegalArgumentException());
+
+    if (SemconvStability.emitStableRpcSemconv()) {
+      assertThat(attributes.build())
+          .containsEntry(ERROR_TYPE, "java.lang.IllegalArgumentException");
+    }
+  }
+
+  @Test
+  void shouldNotExtractErrorType_noError() {
+    Map<String, String> request = new HashMap<>();
+    request.put("service", "my.Service");
+    request.put("method", "Method");
+
+    AttributesExtractor<Map<String, String>, Void> extractor =
+        RpcServerAttributesExtractor.create(new TestGetter(false));
+
+    Context context = Context.root();
     AttributesBuilder attributes = Attributes.builder();
     extractor.onStart(attributes, context, request);
-    assertThat(attributes.build())
-        .containsOnly(
-            entry(RpcIncubatingAttributes.RPC_SYSTEM, "test"),
-            entry(RpcIncubatingAttributes.RPC_SERVICE, "my.Service"),
-            entry(RpcIncubatingAttributes.RPC_METHOD, "Method"));
     extractor.onEnd(attributes, context, request, null, null);
-    assertThat(attributes.build())
-        .containsOnly(
-            entry(RpcIncubatingAttributes.RPC_SYSTEM, "test"),
-            entry(RpcIncubatingAttributes.RPC_SERVICE, "my.Service"),
-            entry(RpcIncubatingAttributes.RPC_METHOD, "Method"));
+
+    if (SemconvStability.emitStableRpcSemconv()) {
+      assertThat(attributes.build()).doesNotContainKey(ERROR_TYPE);
+    }
   }
 }