Remove conditional requirement on network.peer.address and network.pe…

…er.port
open-telemetry · Oct 30, 2023 · f148592 · f148592
1 parent 6d8dda9
commit f148592
Show file tree

Hide file tree

Showing 42 changed files with 95 additions and 460 deletions.
diff --git a/...telemetry/instrumentation/api/instrumenter/http/ForwardedHostAddressAndPortExtractor.java b/...telemetry/instrumentation/api/instrumenter/http/ForwardedHostAddressAndPortExtractor.java
@@ -42,6 +42,13 @@ public void extract(AddressPortSink sink, REQUEST request) {
         return;
       }
     }
+
+    // try :authority (HTTP 2.0 pseudo-header)
+    for (String host : getter.getHttpRequestHeader(request, ":authority")) {
+      if (extractHost(sink, host, 0, host.length())) {
+        return;
+      }
+    }
   }
 
   private static boolean extractFromForwardedHeader(AddressPortSink sink, String forwarded) {

diff --git a/...telemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorBuilder.java b/...telemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorBuilder.java
@@ -135,7 +135,6 @@ InternalNetClientAttributesExtractor<REQUEST, RESPONSE> buildNetExtractor() {
   InternalNetworkAttributesExtractor<REQUEST, RESPONSE> buildNetworkExtractor() {
     return new InternalNetworkAttributesExtractor<>(
         netAttributesGetter,
-        AddressAndPortExtractor.noop(),
         serverAddressAndPortExtractor,
         /* captureNetworkTransportAndType= */ false,
         /* captureLocalSocketAttributes= */ false,

diff --git a/...telemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBuilder.java b/...telemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBuilder.java
@@ -16,7 +16,6 @@
 import io.opentelemetry.instrumentation.api.instrumenter.network.internal.InternalClientAttributesExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.network.internal.InternalNetworkAttributesExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.network.internal.InternalServerAttributesExtractor;
-import io.opentelemetry.instrumentation.api.instrumenter.network.internal.ServerAddressAndPortExtractor;
 import io.opentelemetry.instrumentation.api.instrumenter.url.internal.InternalUrlAttributesExtractor;
 import io.opentelemetry.instrumentation.api.internal.HttpConstants;
 import io.opentelemetry.instrumentation.api.internal.SemconvStability;
@@ -55,9 +54,7 @@ public final class HttpServerAttributesExtractorBuilder<REQUEST, RESPONSE> {
     clientAddressPortExtractor =
         new ClientAddressAndPortExtractor<>(
             netAttributesGetter, new ForwardedForAddressAndPortExtractor<>(httpAttributesGetter));
-    serverAddressPortExtractor =
-        new ServerAddressAndPortExtractor<>(
-            netAttributesGetter, new ForwardedHostAddressAndPortExtractor<>(httpAttributesGetter));
+    serverAddressPortExtractor = new ForwardedHostAddressAndPortExtractor<>(httpAttributesGetter);
   }
 
   /**
@@ -151,7 +148,6 @@ InternalNetServerAttributesExtractor<REQUEST, RESPONSE> buildNetExtractor() {
   InternalNetworkAttributesExtractor<REQUEST, RESPONSE> buildNetworkExtractor() {
     return new InternalNetworkAttributesExtractor<>(
         netAttributesGetter,
-        serverAddressPortExtractor,
         clientAddressPortExtractor,
         /* captureNetworkTransportAndType= */ false,
         /* captureLocalSocketAttributes= */ false,

diff --git a/...va/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesGetter.java b/...va/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesGetter.java
@@ -27,6 +27,7 @@ public interface HttpServerAttributesGetter<REQUEST, RESPONSE>
         io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesGetter<
             REQUEST, RESPONSE>,
         NetworkAttributesGetter<REQUEST, RESPONSE>,
+        // TODO remove ServerAttributesGetter from here
         ServerAttributesGetter<REQUEST, RESPONSE>,
         ClientAttributesGetter<REQUEST, RESPONSE> {
 
@@ -55,4 +56,32 @@ public interface HttpServerAttributesGetter<REQUEST, RESPONSE>
   default String getHttpRoute(REQUEST request) {
     return null;
   }
+
+  /**
+   * Returns the name of the local HTTP server that received the request.
+   *
+   * @deprecated This method is deprecated and will be removed without replacement. The {@link
+   *     HttpServerAttributesExtractor} now extracts the server address and port from the received
+   *     HTTP request's headers.
+   */
+  @Deprecated
+  @Nullable
+  @Override
+  default String getServerAddress(REQUEST request) {
+    return null;
+  }
+
+  /**
+   * Returns the port of the local HTTP server that received the request.
+   *
+   * @deprecated This method is deprecated and will be removed without replacement. The {@link
+   *     HttpServerAttributesExtractor} now extracts the server address and port from the received
+   *     HTTP request's headers.
+   */
+  @Deprecated
+  @Nullable
+  @Override
+  default Integer getServerPort(REQUEST request) {
+    return null;
+  }
 }
diff --git a/...a/io/opentelemetry/instrumentation/api/instrumenter/net/NetClientAttributesExtractor.java b/...a/io/opentelemetry/instrumentation/api/instrumenter/net/NetClientAttributesExtractor.java
@@ -50,7 +50,6 @@ private NetClientAttributesExtractor(NetClientAttributesGetter<REQUEST, RESPONSE
     internalNetworkExtractor =
         new InternalNetworkAttributesExtractor<>(
             getter,
-            AddressAndPortExtractor.noop(),
             serverAddressAndPortExtractor,
             /* captureNetworkTransportAndType= */ true,
             /* captureLocalSocketAttributes= */ false,

diff --git a/...a/io/opentelemetry/instrumentation/api/instrumenter/net/NetServerAttributesExtractor.java b/...a/io/opentelemetry/instrumentation/api/instrumenter/net/NetServerAttributesExtractor.java
@@ -52,7 +52,6 @@ private NetServerAttributesExtractor(NetServerAttributesGetter<REQUEST, RESPONSE
     internalNetworkExtractor =
         new InternalNetworkAttributesExtractor<>(
             getter,
-            serverAddressAndPortExtractor,
             clientAddressAndPortExtractor,
             /* captureNetworkTransportAndType= */ true,
             /* captureLocalSocketAttributes= */ true,

diff --git a/...io/opentelemetry/instrumentation/api/instrumenter/network/NetworkAttributesExtractor.java b/...io/opentelemetry/instrumentation/api/instrumenter/network/NetworkAttributesExtractor.java
@@ -37,7 +37,6 @@ public static <REQUEST, RESPONSE> NetworkAttributesExtractor<REQUEST, RESPONSE>
         new InternalNetworkAttributesExtractor<>(
             getter,
             AddressAndPortExtractor.noop(),
-            AddressAndPortExtractor.noop(),
             /* captureNetworkTransportAndType= */ true,
             /* captureLocalSocketAttributes= */ true,
             // capture the old net.sock.peer.name attr for backwards compatibility

diff --git a/...ava/io/opentelemetry/instrumentation/api/instrumenter/network/ServerAttributesGetter.java b/...ava/io/opentelemetry/instrumentation/api/instrumenter/network/ServerAttributesGetter.java
@@ -39,8 +39,6 @@ default Integer getServerPort(REQUEST request) {
     return null;
   }
 
-  // TODO deprecate
-
   /**
    * Returns an {@link InetSocketAddress} object representing the server socket address.
    *

diff --git a/...instrumentation/api/instrumenter/network/internal/InternalNetworkAttributesExtractor.java b/...instrumentation/api/instrumenter/network/internal/InternalNetworkAttributesExtractor.java
@@ -22,7 +22,6 @@
 public final class InternalNetworkAttributesExtractor<REQUEST, RESPONSE> {
 
   private final NetworkAttributesGetter<REQUEST, RESPONSE> getter;
-  private final AddressAndPortExtractor<REQUEST> logicalLocalAddressAndPortExtractor;
   private final AddressAndPortExtractor<REQUEST> logicalPeerAddressAndPortExtractor;
   private final boolean captureNetworkTransportAndType;
   private final boolean captureLocalSocketAttributes;
@@ -32,15 +31,13 @@ public final class InternalNetworkAttributesExtractor<REQUEST, RESPONSE> {
 
   public InternalNetworkAttributesExtractor(
       NetworkAttributesGetter<REQUEST, RESPONSE> getter,
-      AddressAndPortExtractor<REQUEST> logicalLocalAddressAndPortExtractor,
       AddressAndPortExtractor<REQUEST> logicalPeerAddressAndPortExtractor,
       boolean captureNetworkTransportAndType,
       boolean captureLocalSocketAttributes,
       boolean captureOldPeerDomainAttribute,
       boolean emitStableUrlAttributes,
       boolean emitOldHttpAttributes) {
     this.getter = getter;
-    this.logicalLocalAddressAndPortExtractor = logicalLocalAddressAndPortExtractor;
     this.logicalPeerAddressAndPortExtractor = logicalPeerAddressAndPortExtractor;
     this.captureNetworkTransportAndType = captureNetworkTransportAndType;
     this.captureLocalSocketAttributes = captureLocalSocketAttributes;
@@ -74,8 +71,7 @@ public void onEnd(AttributesBuilder attributes, REQUEST request, @Nullable RESPO
     }
 
     String localAddress = getter.getNetworkLocalAddress(request, response);
-    String logicalLocalAddress = logicalLocalAddressAndPortExtractor.extract(request).address;
-    if (localAddress != null && !localAddress.equals(logicalLocalAddress)) {
+    if (localAddress != null) {
       if (emitStableUrlAttributes && captureLocalSocketAttributes) {
         internalSet(attributes, NetworkAttributes.NETWORK_LOCAL_ADDRESS, localAddress);
       }
@@ -95,8 +91,7 @@ public void onEnd(AttributesBuilder attributes, REQUEST request, @Nullable RESPO
     }
 
     String peerAddress = getter.getNetworkPeerAddress(request, response);
-    String logicalPeerAddress = logicalPeerAddressAndPortExtractor.extract(request).address;
-    if (peerAddress != null && !peerAddress.equals(logicalPeerAddress)) {
+    if (peerAddress != null) {
       if (emitStableUrlAttributes) {
         internalSet(attributes, NetworkAttributes.NETWORK_PEER_ADDRESS, peerAddress);
       }
@@ -119,6 +114,7 @@ public void onEnd(AttributesBuilder attributes, REQUEST request, @Nullable RESPO
             getter.getNetworkPeerInetSocketAddress(request, response);
         if (peerSocketAddress != null) {
           String peerSocketDomain = InetSocketAddressUtil.getDomainName(peerSocketAddress);
+          String logicalPeerAddress = logicalPeerAddressAndPortExtractor.extract(request).address;
           if (peerSocketDomain != null && !peerSocketDomain.equals(logicalPeerAddress)) {
             internalSet(attributes, SemanticAttributes.NET_SOCK_PEER_NAME, peerSocketDomain);
           }

diff --git a/...metry/instrumentation/api/instrumenter/http/ForwardedHostAddressAndPortExtractorTest.java b/...metry/instrumentation/api/instrumenter/http/ForwardedHostAddressAndPortExtractorTest.java
@@ -109,6 +109,22 @@ void shouldParseHost(
     assertThat(sink.getPort()).isEqualTo(expectedPort);
   }
 
+  @ParameterizedTest
+  @ArgumentsSource(HostArgs.class)
+  void shouldParsePseudoAuthority(
+      List<String> headers, @Nullable String expectedAddress, @Nullable Integer expectedPort) {
+    doReturn(emptyList()).when(getter).getHttpRequestHeader(REQUEST, "forwarded");
+    doReturn(emptyList()).when(getter).getHttpRequestHeader(REQUEST, "x-forwarded-host");
+    doReturn(emptyList()).when(getter).getHttpRequestHeader(REQUEST, "host");
+    doReturn(headers).when(getter).getHttpRequestHeader(REQUEST, ":authority");
+
+    AddressAndPort sink = new AddressAndPort();
+    underTest.extract(sink, REQUEST);
+
+    assertThat(sink.getAddress()).isEqualTo(expectedAddress);
+    assertThat(sink.getPort()).isEqualTo(expectedPort);
+  }
+
   static final class HostArgs implements ArgumentsProvider {
 
     @Override

diff --git a/...pentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorTest.java b/...pentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorTest.java
@@ -111,18 +111,6 @@ public String getNetworkProtocolVersion(
         Map<String, Object> request, Map<String, Object> response) {
       return (String) request.get("protocolVersion");
     }
-
-    @Nullable
-    @Override
-    public String getServerAddress(Map<String, Object> request) {
-      return (String) request.get("serverAddress");
-    }
-
-    @Nullable
-    @Override
-    public Integer getServerPort(Map<String, Object> request) {
-      return (Integer) request.get("serverPort");
-    }
   }
 
   @Test
@@ -247,27 +235,6 @@ void extractNetHostAndPortFromHostHeader() {
             entry(SemanticAttributes.NET_HOST_PORT, 777L));
   }
 
-  @Test
-  void extractNetHostAndPortFromNetAttributesGetter() {
-    Map<String, Object> request = new HashMap<>();
-    request.put("header.host", "notthehost:77777"); // this should have lower precedence
-    request.put("serverAddress", "thehost");
-    request.put("serverPort", 777);
-
-    AttributesExtractor<Map<String, Object>, Map<String, Object>> extractor =
-        HttpServerAttributesExtractor.builder(new TestHttpServerAttributesGetter())
-            .setCapturedRequestHeaders(emptyList())
-            .setCapturedResponseHeaders(emptyList())
-            .build();
-
-    AttributesBuilder attributes = Attributes.builder();
-    extractor.onStart(attributes, Context.root(), request);
-    assertThat(attributes.build())
-        .containsOnly(
-            entry(SemanticAttributes.NET_HOST_NAME, "thehost"),
-            entry(SemanticAttributes.NET_HOST_PORT, 777L));
-  }
-
   @ParameterizedTest
   @ArgumentsSource(DefaultHostPortArgumentSource.class)
   void defaultHostPort(int hostPort, String scheme) {

diff --git a/.../opentelemetry/instrumentation/api/instrumenter/net/NetClientAttributesExtractorTest.java b/.../opentelemetry/instrumentation/api/instrumenter/net/NetClientAttributesExtractorTest.java
@@ -147,36 +147,6 @@ void empty() {
     assertThat(endAttributes.build()).isEmpty();
   }
 
-  @Test
-  void doesNotSetDuplicateSocketAddress() {
-    // given
-    Map<String, String> map = new HashMap<>();
-    map.put("netTransport", IP_TCP);
-    map.put("peerName", "1:2:3:4::");
-    map.put("peerPort", "42");
-    map.put("sockFamily", "inet6");
-    map.put("sockPeerAddr", "1:2:3:4::");
-    map.put("sockPeerName", "proxy.opentelemetry.io");
-    map.put("sockPeerPort", "123");
-
-    Context context = Context.root();
-
-    // when
-    AttributesBuilder startAttributes = Attributes.builder();
-    extractor.onStart(startAttributes, context, map);
-
-    AttributesBuilder endAttributes = Attributes.builder();
-    extractor.onEnd(endAttributes, context, map, map, null);
-
-    // then
-    assertThat(startAttributes.build())
-        .containsOnly(
-            entry(SemanticAttributes.NET_PEER_NAME, "1:2:3:4::"),
-            entry(SemanticAttributes.NET_PEER_PORT, 42L));
-
-    assertThat(endAttributes.build()).containsOnly(entry(SemanticAttributes.NET_TRANSPORT, IP_TCP));
-  }
-
   @Test
   void doesNotSetNegativePortValues() {
     // given

diff --git a/.../opentelemetry/instrumentation/api/instrumenter/net/NetServerAttributesExtractorTest.java b/.../opentelemetry/instrumentation/api/instrumenter/net/NetServerAttributesExtractorTest.java
@@ -163,36 +163,6 @@ void empty() {
     assertThat(endAttributes.build()).isEmpty();
   }
 
-  @Test
-  void doesNotSetDuplicatesSocketAddress() {
-    // given
-    Map<String, String> map = new HashMap<>();
-    map.put("netTransport", IP_TCP);
-    map.put("hostName", "4:3:2:1::");
-    map.put("hostPort", "80");
-    map.put("sockFamily", "inet6");
-    map.put("sockHostAddr", "4:3:2:1::");
-    map.put("sockHostPort", "8080");
-
-    Context context = Context.root();
-
-    // when
-    AttributesBuilder startAttributes = Attributes.builder();
-    extractor.onStart(startAttributes, context, map);
-
-    AttributesBuilder endAttributes = Attributes.builder();
-    extractor.onEnd(endAttributes, context, map, null, null);
-
-    // then
-    assertThat(startAttributes.build())
-        .containsOnly(
-            entry(SemanticAttributes.NET_TRANSPORT, IP_TCP),
-            entry(SemanticAttributes.NET_HOST_NAME, "4:3:2:1::"),
-            entry(SemanticAttributes.NET_HOST_PORT, 80L));
-
-    assertThat(endAttributes.build()).isEmpty();
-  }
-
   @Test
   void doesNotSetNegativePort() {
     // given

diff --git a/...y/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBothSemconvTest.java b/...y/instrumentation/api/instrumenter/http/HttpServerAttributesExtractorBothSemconvTest.java
@@ -103,18 +103,6 @@ public String getNetworkProtocolVersion(
         Map<String, Object> request, Map<String, Object> response) {
       return (String) request.get("protocolVersion");
     }
-
-    @Nullable
-    @Override
-    public String getServerAddress(Map<String, Object> request) {
-      return (String) request.get("serverAddress");
-    }
-
-    @Nullable
-    @Override
-    public Integer getServerPort(Map<String, Object> request) {
-      return (Integer) request.get("serverPort");
-    }
   }
 
   @Test

diff --git a/...instrumentation/api/instrumenter/http/HttpClientAttributesExtractorStableSemconvTest.java b/...instrumentation/api/instrumenter/http/HttpClientAttributesExtractorStableSemconvTest.java
@@ -372,7 +372,7 @@ void shouldExtractServerAddressAndPortFromHostHeader() {
   }
 
   @Test
-  void shouldNotExtractDuplicatePeerAddress() {
+  void shouldExtractPeerAddressEvenIfItDuplicatesServerAddress() {
     Map<String, String> request = new HashMap<>();
     request.put("networkPeerAddress", "1.2.3.4");
     request.put("networkPeerPort", "456");
@@ -395,6 +395,9 @@ void shouldNotExtractDuplicatePeerAddress() {
     AttributesBuilder endAttributes = Attributes.builder();
     extractor.onEnd(endAttributes, Context.root(), request, response, null);
     assertThat(endAttributes.build())
-        .containsOnly(entry(SemanticAttributes.HTTP_RESPONSE_STATUS_CODE, 200L));
+        .containsOnly(
+            entry(SemanticAttributes.HTTP_RESPONSE_STATUS_CODE, 200L),
+            entry(NetworkAttributes.NETWORK_PEER_ADDRESS, "1.2.3.4"),
+            entry(NetworkAttributes.NETWORK_PEER_PORT, 456L));
   }
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -39,8 +39,6 @@ default Integer getServerPort(REQUEST request) { @@
         return null;
       }
-      // TODO deprecate
       /**
        * Returns an {@link InetSocketAddress} object representing the server socket address.
        *
@@ Expand Down @@