Add dual-semconv support to RPC attributes extractors

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

@@ -16,7 +16,23 @@
  */
 public interface RpcAttributesGetter<REQUEST, RESPONSE> {
 
-  @Nullable
+  /**
+   * Returns the stable semconv system name for the RPC framework (e.g. {@code "grpc"}, {@code
+   * "java_rmi"}, {@code "dotnet_wcf"}).
+   *
+   * @see <a
+   *     href="https://opentelemetry.io/docs/specs/semconv/attributes-registry/rpc/">rpc.system.name
+   *     spec</a>
+   */
+  @SuppressWarnings("deprecation")
+  default String getRpcSystemName(REQUEST request) {
+    return getSystem(request);
+  }
+
+  /**
+   * @deprecated Use {@link #getRpcSystemName(REQUEST)}. To be removed in 3.0.
+   */
+  @Deprecated
   String getSystem(REQUEST request);
 
   @Nullable
@@ -47,6 +63,7 @@ default Long getResponseSize(REQUEST request) {
    *     method is unavailable
    */
   @Nullable
+  // TODO remove default implementation
   default String getRpcMethod(REQUEST request) {
     return null;
   }
@@ -56,15 +73,16 @@ default String getRpcMethod(REQUEST request) {
    *
    * <p>This method should return {@code null} if there was no error.
    *
-   * <p>If this method is not implemented, or if it returns {@code null}, the exception class name
-   * will be used as error type.
+   * <p>If this method returns {@code null}, the exception class name will be used as error type if
+   * one was thrown.
    *
    * <p>The cardinality of the error type should be low. The instrumentations implementing this
    * method are recommended to document the custom values they support.
    *
-   * <p>Examples: {@code OK}, {@code CANCELLED}, {@code UNKNOWN}, {@code -32602}
+   * <p>Examples: {@code CANCELLED}, {@code UNKNOWN}, {@code -32602}
    */
   @Nullable
+  // TODO remove default implementation
   default String getErrorType(
       REQUEST request, @Nullable RESPONSE response, @Nullable Throwable error) {
     return null;

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

@@ -5,6 +5,10 @@
 
 package io.opentelemetry.instrumentation.api.incubator.semconv.rpc;
 
+import static io.opentelemetry.instrumentation.api.internal.SemconvStability.emitOldRpcSemconv;
+import static io.opentelemetry.instrumentation.api.internal.SemconvStability.emitStableRpcSemconv;
+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;
@@ -14,9 +18,15 @@
 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");
 
   private final RpcAttributesGetter<REQUEST, RESPONSE> getter;
@@ -25,12 +35,23 @@ abstract class RpcCommonAttributesExtractor<REQUEST, RESPONSE>
     this.getter = getter;
   }
 
-  @SuppressWarnings("deprecation") // for getMethod()
+  @SuppressWarnings("deprecation") // for getSystem(), getMethod()
   @Override
   public final void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
-    attributes.put(RPC_SYSTEM, getter.getSystem(request));
-    attributes.put(RPC_SERVICE, getter.getService(request));
-    attributes.put(RPC_METHOD, getter.getMethod(request));
+
+    if (emitStableRpcSemconv()) {
+      attributes.put(RPC_SYSTEM_NAME, getter.getRpcSystemName(request));
+      attributes.put(RPC_METHOD, getter.getRpcMethod(request));
+    }
+
+    if (emitOldRpcSemconv()) {
+      attributes.put(RPC_SYSTEM, getter.getSystem(request));
+      attributes.put(RPC_SERVICE, getter.getService(request));
+      if (!emitStableRpcSemconv()) {
+        // only set old rpc.method on spans when there's no clash with stable rpc.method
+        attributes.put(RPC_METHOD, getter.getMethod(request));
+      }
+    }
   }
 
   @Override
@@ -40,6 +61,13 @@ public final void onEnd(
       REQUEST request,
       @Nullable RESPONSE response,
       @Nullable Throwable error) {
-    // No response attributes
+    if (emitStableRpcSemconv()) {
+      String errorType = getter.getErrorType(request, response, error);
+      // fall back to exception class name
+      if (errorType == null && error != null) {
+        errorType = error.getClass().getName();
+      }
+      attributes.put(ERROR_TYPE, errorType);
+    }
   }
 }

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

@@ -5,6 +5,8 @@
 
 package io.opentelemetry.instrumentation.api.incubator.semconv.rpc;
 
+import static io.opentelemetry.instrumentation.api.internal.SemconvStability.emitStableRpcSemconv;
+
 import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;
 
 /** A {@link SpanNameExtractor} for RPC requests. */
@@ -28,6 +30,15 @@ private RpcSpanNameExtractor(RpcAttributesGetter<REQUEST, ?> getter) {
   @SuppressWarnings("deprecation") // for getMethod()
   @Override
   public String extract(REQUEST request) {
+    if (emitStableRpcSemconv()) {
+      String method = getter.getRpcMethod(request);
+      if (method != null) {
+        return method;
+      }
+      // fall back to rpc.system.name
+      return getter.getRpcSystemName(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,25 +5,36 @@
 
 package io.opentelemetry.instrumentation.api.incubator.semconv.rpc;
 
+import static io.opentelemetry.instrumentation.api.internal.SemconvStability.emitOldRpcSemconv;
+import static io.opentelemetry.instrumentation.api.internal.SemconvStability.emitStableRpcSemconv;
 import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;
+import static io.opentelemetry.semconv.ErrorAttributes.ERROR_TYPE;
 import static io.opentelemetry.semconv.incubating.RpcIncubatingAttributes.RPC_METHOD;
 import static io.opentelemetry.semconv.incubating.RpcIncubatingAttributes.RPC_SERVICE;
 import static io.opentelemetry.semconv.incubating.RpcIncubatingAttributes.RPC_SYSTEM;
 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 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> {
+
+    @Override
+    public String getRpcSystemName(Map<String, String> request) {
+      return "test";
+    }
 
     @Override
     public String getSystem(Map<String, String> request) {
@@ -40,18 +51,40 @@ 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");
+    }
   }
 
   @Test
   void server() {
-    testExtractor(RpcServerAttributesExtractor.create(TestGetter.INSTANCE));
+    testExtractor(RpcServerAttributesExtractor.create(new TestGetter()));
   }
 
   @Test
   void client() {
-    testExtractor(RpcClientAttributesExtractor.create(TestGetter.INSTANCE));
+    testExtractor(RpcClientAttributesExtractor.create(new TestGetter()));
   }
 
+  // Stable semconv keys
+  private static final AttributeKey<String> RPC_SYSTEM_NAME =
+      AttributeKey.stringKey("rpc.system.name");
+
   private static void testExtractor(AttributesExtractor<Map<String, String>, Void> extractor) {
     Map<String, String> request = new HashMap<>();
     request.put("service", "my.Service");
@@ -61,16 +94,89 @@ private static void testExtractor(AttributesExtractor<Map<String, String>, Void>
 
     AttributesBuilder attributes = Attributes.builder();
     extractor.onStart(attributes, context, request);
-    assertThat(attributes.build())
-        .containsOnly(
-            entry(RPC_SYSTEM, "test"),
-            entry(RPC_SERVICE, "my.Service"),
-            entry(RPC_METHOD, "Method"));
+
+    // Build expected entries list based on semconv mode
+    List<Map.Entry<? extends AttributeKey<?>, ?>> expectedEntries = new ArrayList<>();
+
+    if (emitStableRpcSemconv()) {
+      expectedEntries.add(entry(RPC_SYSTEM_NAME, "test"));
+      expectedEntries.add(entry(RPC_METHOD, "my.Service/Method"));
+    }
+
+    if (emitOldRpcSemconv()) {
+      expectedEntries.add(entry(RPC_SYSTEM, "test"));
+      expectedEntries.add(entry(RPC_SERVICE, "my.Service"));
+      if (!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);
+  }
+
+  @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());
+
+    Context context = Context.root();
+    AttributesBuilder attributes = Attributes.builder();
+    extractor.onStart(attributes, context, request);
     extractor.onEnd(attributes, context, request, null, null);
-    assertThat(attributes.build())
-        .containsOnly(
-            entry(RPC_SYSTEM, "test"),
-            entry(RPC_SERVICE, "my.Service"),
-            entry(RPC_METHOD, "Method"));
+
+    if (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());
+
+    Context context = Context.root();
+    AttributesBuilder attributes = Attributes.builder();
+    extractor.onStart(attributes, context, request);
+    extractor.onEnd(attributes, context, request, null, new IllegalArgumentException());
+
+    if (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());
+
+    Context context = Context.root();
+    AttributesBuilder attributes = Attributes.builder();
+    extractor.onStart(attributes, context, request);
+    extractor.onEnd(attributes, context, request, null, null);
+
+    if (emitStableRpcSemconv()) {
+      assertThat(attributes.build()).doesNotContainKey(ERROR_TYPE);
+    }
   }
 }