NetClientAttributesAdapter - favor composition over inheritance (open…

…-telemetry#5030)

* first pass at separating NetAttributesAdapter interface and make NetClientAttributesExtractor concrete

* rename the implementations extractor -> adapter

* hide constructor and make factory method

* rename to client and add javadoc

* spotless

* finish javadoc thought

* rebase

* renamed NetClientAttributesAdapter to NetClientAttributesGetter

* fix lettuce

* code review comments

* code review comments -- renaming for consistency

* adapter -> getter

* fix ratpack

* adapter -> getter

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/PeerServiceAttributesExtractor.java

@@ -9,7 +9,7 @@
 
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.instrumentation.api.config.Config;
-import io.opentelemetry.instrumentation.api.instrumenter.net.NetClientAttributesExtractor;
+import io.opentelemetry.instrumentation.api.instrumenter.net.NetClientAttributesGetter;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.util.Map;
 import javax.annotation.Nullable;
@@ -29,24 +29,23 @@ public final class PeerServiceAttributesExtractor<REQUEST, RESPONSE>
       Config.get().getMap("otel.instrumentation.common.peer-service-mapping", emptyMap());
 
   private final Map<String, String> peerServiceMapping;
-  private final NetClientAttributesExtractor<REQUEST, RESPONSE> netClientAttributesExtractor;
+  private final NetClientAttributesGetter<REQUEST, RESPONSE> attributesGetter;
 
   // visible for tests
   PeerServiceAttributesExtractor(
       Map<String, String> peerServiceMapping,
-      NetClientAttributesExtractor<REQUEST, RESPONSE> netClientAttributesExtractor) {
+      NetClientAttributesGetter<REQUEST, RESPONSE> attributesGetter) {
     this.peerServiceMapping = peerServiceMapping;
-    this.netClientAttributesExtractor = netClientAttributesExtractor;
+    this.attributesGetter = attributesGetter;
   }
 
   /**
    * Returns a new {@link PeerServiceAttributesExtractor} that will use the passed {@code
    * netAttributesExtractor} instance to determine the value of the {@code peer.service} attribute.
    */
   public static <REQUEST, RESPONSE> PeerServiceAttributesExtractor<REQUEST, RESPONSE> create(
-      NetClientAttributesExtractor<REQUEST, RESPONSE> netAttributesExtractor) {
-    return new PeerServiceAttributesExtractor<>(
-        JAVAAGENT_PEER_SERVICE_MAPPING, netAttributesExtractor);
+      NetClientAttributesGetter<REQUEST, RESPONSE> attributesGetter) {
+    return new PeerServiceAttributesExtractor<>(JAVAAGENT_PEER_SERVICE_MAPPING, attributesGetter);
   }
 
   @Override
@@ -64,10 +63,10 @@ public void onEnd(
       return;
     }
 
-    String peerName = netClientAttributesExtractor.peerName(request, response);
+    String peerName = attributesGetter.peerName(request, response);
     String peerService = mapToPeerService(peerName);
     if (peerService == null) {
-      String peerIp = netClientAttributesExtractor.peerIp(request, response);
+      String peerIp = attributesGetter.peerIp(request, response);
       peerService = mapToPeerService(peerIp);
     }
     if (peerService != null) {

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/net/InetSocketAddressNetClientAttributesExtractor.java → instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/net/InetSocketAddressNetClientAttributesGetter.java

@@ -16,8 +16,8 @@
  * {@link InetSocketAddress} so this is a convenient alternative to {@link
  * NetClientAttributesExtractor}. There is no meaning to implement both in the same instrumentation.
  */
-public abstract class InetSocketAddressNetClientAttributesExtractor<REQUEST, RESPONSE>
-    extends NetClientAttributesExtractor<REQUEST, RESPONSE> {
+public abstract class InetSocketAddressNetClientAttributesGetter<REQUEST, RESPONSE>
+    implements NetClientAttributesGetter<REQUEST, RESPONSE> {
 
   @Nullable
   public abstract InetSocketAddress getAddress(REQUEST request, @Nullable RESPONSE response);

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/net/NetClientAttributesExtractor.java

@@ -14,46 +14,48 @@
  * Extractor of <a
  * href="https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/span-general.md#general-network-connection-attributes">Network
  * attributes</a>. It is common to have access to {@link java.net.InetSocketAddress}, in which case
- * it is more convenient to use {@link InetSocketAddressNetClientAttributesExtractor}.
+ * it is more convenient to use {@link InetSocketAddressNetClientAttributesGetter}.
+ *
+ * <p>This class delegates to a type-specific {@link NetClientAttributesGetter} for individual
+ * attribute extraction from request/response objects.
  */
-public abstract class NetClientAttributesExtractor<REQUEST, RESPONSE>
+public final class NetClientAttributesExtractor<REQUEST, RESPONSE>
     implements AttributesExtractor<REQUEST, RESPONSE> {
 
+  private final NetClientAttributesGetter<REQUEST, RESPONSE> getter;
+
+  public static <REQUEST, RESPONSE> NetClientAttributesExtractor<REQUEST, RESPONSE> create(
+      NetClientAttributesGetter<REQUEST, RESPONSE> getter) {
+    return new NetClientAttributesExtractor<>(getter);
+  }
+
+  private NetClientAttributesExtractor(NetClientAttributesGetter<REQUEST, RESPONSE> getter) {
+    this.getter = getter;
+  }
+
   @Override
-  public final void onStart(AttributesBuilder attributes, REQUEST request) {}
+  public void onStart(AttributesBuilder attributes, REQUEST request) {}
 
   @Override
-  public final void onEnd(
+  public void onEnd(
       AttributesBuilder attributes,
       REQUEST request,
       @Nullable RESPONSE response,
       @Nullable Throwable error) {
 
-    set(attributes, SemanticAttributes.NET_TRANSPORT, transport(request, response));
+    set(attributes, SemanticAttributes.NET_TRANSPORT, getter.transport(request, response));
 
-    String peerIp = peerIp(request, response);
-    String peerName = peerName(request, response);
+    String peerIp = getter.peerIp(request, response);
+    String peerName = getter.peerName(request, response);
 
     if (peerName != null && !peerName.equals(peerIp)) {
       set(attributes, SemanticAttributes.NET_PEER_NAME, peerName);
     }
     set(attributes, SemanticAttributes.NET_PEER_IP, peerIp);
 
-    Integer peerPort = peerPort(request, response);
+    Integer peerPort = getter.peerPort(request, response);
     if (peerPort != null && peerPort > 0) {
       set(attributes, SemanticAttributes.NET_PEER_PORT, (long) peerPort);
     }
   }
-
-  @Nullable
-  public abstract String transport(REQUEST request, @Nullable RESPONSE response);
-
-  @Nullable
-  public abstract String peerName(REQUEST request, @Nullable RESPONSE response);
-
-  @Nullable
-  public abstract Integer peerPort(REQUEST request, @Nullable RESPONSE response);
-
-  @Nullable
-  public abstract String peerIp(REQUEST request, @Nullable RESPONSE response);
 }

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/net/NetClientAttributesGetter.java

@@ -0,0 +1,31 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.instrumenter.net;
+
+import javax.annotation.Nullable;
+
+/**
+ * An interface for getting client-based network attributes. It adapts from a type-specific request
+ * and response into the 4 common network attribute values.
+ *
+ * <p>Instrumentation authors will create implementations of this interface for their specific
+ * library/framework. It will be used by the NetClientAttributesExtractor to obtain the various
+ * network attributes in a type-generic way.
+ */
+public interface NetClientAttributesGetter<REQUEST, RESPONSE> {
+
+  @Nullable
+  String transport(REQUEST request, @Nullable RESPONSE response);
+
+  @Nullable
+  String peerName(REQUEST request, @Nullable RESPONSE response);
+
+  @Nullable
+  Integer peerPort(REQUEST request, @Nullable RESPONSE response);
+
+  @Nullable
+  String peerIp(REQUEST request, @Nullable RESPONSE response);
+}

instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/PeerServiceAttributesExtractorTest.java

@@ -14,7 +14,7 @@
 
 import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.api.common.AttributesBuilder;
-import io.opentelemetry.instrumentation.api.instrumenter.net.NetClientAttributesExtractor;
+import io.opentelemetry.instrumentation.api.instrumenter.net.NetClientAttributesGetter;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.util.HashMap;
 import java.util.Map;
@@ -25,7 +25,7 @@
 
 @ExtendWith(MockitoExtension.class)
 class PeerServiceAttributesExtractorTest {
-  @Mock NetClientAttributesExtractor<String, String> netAttributesExtractor;
+  @Mock NetClientAttributesGetter<String, String> netAttributesExtractor;
 
   @Test
   void shouldNotSetAnyValueIfNetExtractorReturnsNulls() {

instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/net/InetSocketAddressNetClientAttributesExtractorTest.java → instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/net/InetSocketAddressNetClientAttributesGetterTest.java

@@ -17,12 +17,11 @@
 import org.mockito.junit.jupiter.MockitoExtension;
 
 @ExtendWith(MockitoExtension.class)
-class InetSocketAddressNetClientAttributesExtractorTest {
+class InetSocketAddressNetClientAttributesGetterTest {
 
-  private final InetSocketAddressNetClientAttributesExtractor<InetSocketAddress, InetSocketAddress>
-      extractor =
-          new InetSocketAddressNetClientAttributesExtractor<
-              InetSocketAddress, InetSocketAddress>() {
+  private final InetSocketAddressNetClientAttributesGetter<InetSocketAddress, InetSocketAddress>
+      getter =
+          new InetSocketAddressNetClientAttributesGetter<InetSocketAddress, InetSocketAddress>() {
             @Override
             public InetSocketAddress getAddress(
                 InetSocketAddress request, InetSocketAddress response) {
@@ -34,9 +33,12 @@ public String transport(InetSocketAddress request, InetSocketAddress response) {
               return SemanticAttributes.NetTransportValues.IP_TCP;
             }
           };
+  private final NetClientAttributesExtractor<InetSocketAddress, InetSocketAddress> extractor =
+      NetClientAttributesExtractor.create(getter);
 
   @Test
   void noInetSocketAddress() {
+
     AttributesBuilder attributes = Attributes.builder();
     extractor.onEnd(attributes, null, null, null);
     assertThat(attributes.build())

instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/net/NetClientAttributesExtractorTest.java

@@ -17,8 +17,8 @@
 
 class NetClientAttributesExtractorTest {
 
-  static class TestNetClientAttributesExtractor
-      extends NetClientAttributesExtractor<Map<String, String>, Map<String, String>> {
+  static class TestNetClientAttributesGetter
+      implements NetClientAttributesGetter<Map<String, String>, Map<String, String>> {
 
     @Override
     public String transport(Map<String, String> request, Map<String, String> response) {
@@ -64,7 +64,9 @@ void normal() {
     response.put("peerPort", "42");
     response.put("peerIp", "4.3.2.1");
 
-    TestNetClientAttributesExtractor extractor = new TestNetClientAttributesExtractor();
+    TestNetClientAttributesGetter getter = new TestNetClientAttributesGetter();
+    NetClientAttributesExtractor<Map<String, String>, Map<String, String>> extractor =
+        NetClientAttributesExtractor.create(getter);
 
     // when
     AttributesBuilder startAttributes = Attributes.builder();
@@ -97,7 +99,9 @@ public void doesNotSetDuplicateAttributes() {
     response.put("peerPort", "42");
     response.put("peerIp", "4.3.2.1");
 
-    TestNetClientAttributesExtractor extractor = new TestNetClientAttributesExtractor();
+    TestNetClientAttributesGetter getter = new TestNetClientAttributesGetter();
+    NetClientAttributesExtractor<Map<String, String>, Map<String, String>> extractor =
+        NetClientAttributesExtractor.create(getter);
 
     // when
     AttributesBuilder startAttributes = Attributes.builder();
@@ -124,7 +128,9 @@ public void doesNotSetNegativePort() {
     Map<String, String> response = new HashMap<>();
     response.put("peerPort", "-1");
 
-    TestNetClientAttributesExtractor extractor = new TestNetClientAttributesExtractor();
+    TestNetClientAttributesGetter getter = new TestNetClientAttributesGetter();
+    NetClientAttributesExtractor<Map<String, String>, Map<String, String>> extractor =
+        NetClientAttributesExtractor.create(getter);
 
     // when
     AttributesBuilder startAttributes = Attributes.builder();

instrumentation/akka/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/client/AkkaHttpClientSingletons.java

@@ -14,6 +14,7 @@
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpClientMetrics;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanNameExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.http.HttpSpanStatusExtractor;
+import io.opentelemetry.instrumentation.api.instrumenter.net.NetClientAttributesExtractor;
 import io.opentelemetry.javaagent.instrumentation.akkahttp.AkkaHttpUtil;
 
 public class AkkaHttpClientSingletons {
@@ -25,16 +26,16 @@ public class AkkaHttpClientSingletons {
     SETTER = new HttpHeaderSetter(GlobalOpenTelemetry.getPropagators());
     AkkaHttpClientAttributesExtractor httpAttributesExtractor =
         new AkkaHttpClientAttributesExtractor();
-    AkkaHttpNetAttributesExtractor netAttributesExtractor = new AkkaHttpNetAttributesExtractor();
+    AkkaHttpNetAttributesGetter netAttributesGetter = new AkkaHttpNetAttributesGetter();
     INSTRUMENTER =
         Instrumenter.<HttpRequest, HttpResponse>builder(
                 GlobalOpenTelemetry.get(),
                 AkkaHttpUtil.instrumentationName(),
                 HttpSpanNameExtractor.create(httpAttributesExtractor))
             .setSpanStatusExtractor(HttpSpanStatusExtractor.create(httpAttributesExtractor))
             .addAttributesExtractor(httpAttributesExtractor)
-            .addAttributesExtractor(netAttributesExtractor)
-            .addAttributesExtractor(PeerServiceAttributesExtractor.create(netAttributesExtractor))
+            .addAttributesExtractor(NetClientAttributesExtractor.create(netAttributesGetter))
+            .addAttributesExtractor(PeerServiceAttributesExtractor.create(netAttributesGetter))
             .addRequestMetrics(HttpClientMetrics.get())
             .newInstrumenter(SpanKindExtractor.alwaysClient());
   }

instrumentation/akka/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/client/AkkaHttpNetAttributesExtractor.java → instrumentation/akka/akka-http-10.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/akkahttp/client/AkkaHttpNetAttributesGetter.java

@@ -7,12 +7,12 @@
 
 import akka.http.scaladsl.model.HttpRequest;
 import akka.http.scaladsl.model.HttpResponse;
-import io.opentelemetry.instrumentation.api.instrumenter.net.NetClientAttributesExtractor;
+import io.opentelemetry.instrumentation.api.instrumenter.net.NetClientAttributesGetter;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import javax.annotation.Nullable;
 
-class AkkaHttpNetAttributesExtractor
-    extends NetClientAttributesExtractor<HttpRequest, HttpResponse> {
+class AkkaHttpNetAttributesGetter implements NetClientAttributesGetter<HttpRequest, HttpResponse> {
+
   @Override
   public String transport(HttpRequest httpRequest, @Nullable HttpResponse httpResponse) {
     return SemanticAttributes.NetTransportValues.IP_TCP;

instrumentation/apache-dubbo-2.7/library-autoconfigure/src/main/java/io/opentelemetry/instrumentation/apachedubbo/v2_7/DubboTracingBuilder.java

@@ -6,13 +6,14 @@
 package io.opentelemetry.instrumentation.apachedubbo.v2_7;
 
 import io.opentelemetry.api.OpenTelemetry;
-import io.opentelemetry.instrumentation.apachedubbo.v2_7.internal.DubboNetClientAttributesExtractor;
+import io.opentelemetry.instrumentation.apachedubbo.v2_7.internal.DubboNetClientAttributesGetter;
 import io.opentelemetry.instrumentation.apachedubbo.v2_7.internal.DubboNetServerAttributesExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
 import io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder;
 import io.opentelemetry.instrumentation.api.instrumenter.PeerServiceAttributesExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor;
+import io.opentelemetry.instrumentation.api.instrumenter.net.NetClientAttributesExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.rpc.RpcSpanNameExtractor;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.util.ArrayList;
@@ -56,8 +57,9 @@ public DubboTracing build() {
     SpanNameExtractor<DubboRequest> spanNameExtractor =
         RpcSpanNameExtractor.create(rpcAttributesExtractor);
 
-    DubboNetClientAttributesExtractor netClientAttributesExtractor =
-        new DubboNetClientAttributesExtractor();
+    DubboNetClientAttributesGetter netClientAttributesGetter = new DubboNetClientAttributesGetter();
+    NetClientAttributesExtractor<DubboRequest, Result> netClientAttributesExtractor =
+        NetClientAttributesExtractor.create(netClientAttributesGetter);
 
     InstrumenterBuilder<DubboRequest, Result> serverInstrumenterBuilder =
         Instrumenter.builder(openTelemetry, INSTRUMENTATION_NAME, spanNameExtractor);
@@ -79,7 +81,7 @@ public DubboTracing build() {
           AttributesExtractor.constant(SemanticAttributes.PEER_SERVICE, peerService));
     } else {
       clientInstrumenterBuilder.addAttributesExtractor(
-          PeerServiceAttributesExtractor.create(netClientAttributesExtractor));
+          PeerServiceAttributesExtractor.create(netClientAttributesGetter));
     }
 
     return new DubboTracing(

instrumentation/apache-dubbo-2.7/library-autoconfigure/src/main/java/io/opentelemetry/instrumentation/apachedubbo/v2_7/internal/DubboNetClientAttributesExtractor.java → instrumentation/apache-dubbo-2.7/library-autoconfigure/src/main/java/io/opentelemetry/instrumentation/apachedubbo/v2_7/internal/DubboNetClientAttributesGetter.java

@@ -6,13 +6,13 @@
 package io.opentelemetry.instrumentation.apachedubbo.v2_7.internal;
 
 import io.opentelemetry.instrumentation.apachedubbo.v2_7.DubboRequest;
-import io.opentelemetry.instrumentation.api.instrumenter.net.InetSocketAddressNetClientAttributesExtractor;
+import io.opentelemetry.instrumentation.api.instrumenter.net.InetSocketAddressNetClientAttributesGetter;
 import java.net.InetSocketAddress;
 import javax.annotation.Nullable;
 import org.apache.dubbo.rpc.Result;
 
-public final class DubboNetClientAttributesExtractor
-    extends InetSocketAddressNetClientAttributesExtractor<DubboRequest, Result> {
+public final class DubboNetClientAttributesGetter
+    extends InetSocketAddressNetClientAttributesGetter<DubboRequest, Result> {
 
   @Override
   @Nullable

instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientNetAttributesExtractor.java → instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientNetAttributesGetter.java

@@ -5,13 +5,13 @@
 
 package io.opentelemetry.javaagent.instrumentation.apachehttpasyncclient;
 
-import io.opentelemetry.instrumentation.api.instrumenter.net.NetClientAttributesExtractor;
+import io.opentelemetry.instrumentation.api.instrumenter.net.NetClientAttributesGetter;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import javax.annotation.Nullable;
 import org.apache.http.HttpResponse;
 
-final class ApacheHttpAsyncClientNetAttributesExtractor
-    extends NetClientAttributesExtractor<ApacheHttpClientRequest, HttpResponse> {
+final class ApacheHttpAsyncClientNetAttributesGetter
+    implements NetClientAttributesGetter<ApacheHttpClientRequest, HttpResponse> {
 
   @Override
   public String transport(ApacheHttpClientRequest request, @Nullable HttpResponse response) {