Implement Host header parsing in HTTP client attributes extractor (#7288

) Continuation (and refactoring) of #6892
open-telemetry · Nov 30, 2022 · 91d9f1d · 91d9f1d
1 parent e2f9603
commit 91d9f1d
Show file tree

Hide file tree

Showing 10 changed files with 195 additions and 44 deletions.
diff --git a/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractor.java b/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractor.java
@@ -82,7 +82,9 @@ public static <REQUEST, RESPONSE> HttpClientAttributesExtractorBuilder<REQUEST,
     super(httpAttributesGetter, capturedRequestHeaders, capturedResponseHeaders);
     internalNetExtractor =
         new InternalNetClientAttributesExtractor<>(
-            netAttributesGetter, this::shouldCapturePeerPort);
+            netAttributesGetter,
+            this::shouldCapturePeerPort,
+            new HttpNetNamePortGetter<>(httpAttributesGetter));
   }
 
   @Override

diff --git a/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpCommonAttributesExtractor.java b/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpCommonAttributesExtractor.java
@@ -9,12 +9,15 @@
 import static io.opentelemetry.instrumentation.api.instrumenter.http.CapturedHttpHeadersUtil.requestAttributeKey;
 import static io.opentelemetry.instrumentation.api.instrumenter.http.CapturedHttpHeadersUtil.responseAttributeKey;
 import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;
+import static java.util.logging.Level.FINE;
 
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
+import io.opentelemetry.instrumentation.api.instrumenter.net.internal.FallbackNamePortGetter;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.util.List;
+import java.util.logging.Logger;
 import javax.annotation.Nullable;
 
 /**
@@ -26,6 +29,8 @@ abstract class HttpCommonAttributesExtractor<
         REQUEST, RESPONSE, GETTER extends HttpCommonAttributesGetter<REQUEST, RESPONSE>>
     implements AttributesExtractor<REQUEST, RESPONSE> {
 
+  private static final Logger logger = Logger.getLogger(HttpCommonAttributesGetter.class.getName());
+
   final GETTER getter;
   private final List<String> capturedRequestHeaders;
   private final List<String> capturedResponseHeaders;
@@ -114,4 +119,43 @@ private static Long parseNumber(@Nullable String number) {
       return null;
     }
   }
+
+  static final class HttpNetNamePortGetter<REQUEST> implements FallbackNamePortGetter<REQUEST> {
+
+    private final HttpCommonAttributesGetter<REQUEST, ?> getter;
+
+    HttpNetNamePortGetter(HttpCommonAttributesGetter<REQUEST, ?> getter) {
+      this.getter = getter;
+    }
+
+    @Nullable
+    @Override
+    public String name(REQUEST request) {
+      String host = firstHeaderValue(getter.requestHeader(request, "host"));
+      if (host == null) {
+        return null;
+      }
+      int hostHeaderSeparator = host.indexOf(':');
+      return hostHeaderSeparator == -1 ? host : host.substring(0, hostHeaderSeparator);
+    }
+
+    @Nullable
+    @Override
+    public Integer port(REQUEST request) {
+      String host = firstHeaderValue(getter.requestHeader(request, "host"));
+      if (host == null) {
+        return null;
+      }
+      int hostHeaderSeparator = host.indexOf(':');
+      if (hostHeaderSeparator == -1) {
+        return null;
+      }
+      try {
+        return Integer.parseInt(host.substring(hostHeaderSeparator + 1));
+      } catch (NumberFormatException e) {
+        logger.log(FINE, e.getMessage(), e);
+      }
+      return null;
+    }
+  }
 }
diff --git a/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractor.java b/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerAttributesExtractor.java
@@ -79,7 +79,9 @@ public static <REQUEST, RESPONSE> HttpServerAttributesExtractorBuilder<REQUEST,
     super(httpAttributesGetter, capturedRequestHeaders, capturedResponseHeaders);
     internalNetExtractor =
         new InternalNetServerAttributesExtractor<>(
-            netAttributesGetter, this::shouldCaptureHostPort);
+            netAttributesGetter,
+            this::shouldCaptureHostPort,
+            new HttpNetNamePortGetter<>(httpAttributesGetter));
     this.httpRouteHolderGetter = httpRouteHolderGetter;
   }
 
@@ -95,7 +97,7 @@ public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST
     internalSet(attributes, SemanticAttributes.HTTP_ROUTE, getter.route(request));
     internalSet(attributes, SemanticAttributes.HTTP_CLIENT_IP, clientIp(request));
 
-    internalNetExtractor.onStart(attributes, request, host(request));
+    internalNetExtractor.onStart(attributes, request);
   }
 
   private boolean shouldCaptureHostPort(int port, REQUEST request) {
@@ -122,11 +124,6 @@ public void onEnd(
     internalSet(attributes, SemanticAttributes.HTTP_ROUTE, httpRouteHolderGetter.apply(context));
   }
 
-  @Nullable
-  private String host(REQUEST request) {
-    return firstHeaderValue(getter.requestHeader(request, "host"));
-  }
-
   @Nullable
   private String forwardedProto(REQUEST request) {
     // try Forwarded

diff --git a/...a/io/opentelemetry/instrumentation/api/instrumenter/net/NetClientAttributesExtractor.java b/...a/io/opentelemetry/instrumentation/api/instrumenter/net/NetClientAttributesExtractor.java
@@ -8,6 +8,7 @@
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
+import io.opentelemetry.instrumentation.api.instrumenter.net.internal.FallbackNamePortGetter;
 import io.opentelemetry.instrumentation.api.instrumenter.net.internal.InternalNetClientAttributesExtractor;
 import javax.annotation.Nullable;
 
@@ -31,7 +32,9 @@ public static <REQUEST, RESPONSE> NetClientAttributesExtractor<REQUEST, RESPONSE
   }
 
   private NetClientAttributesExtractor(NetClientAttributesGetter<REQUEST, RESPONSE> getter) {
-    internalExtractor = new InternalNetClientAttributesExtractor<>(getter, (port, request) -> true);
+    internalExtractor =
+        new InternalNetClientAttributesExtractor<>(
+            getter, (port, request) -> true, FallbackNamePortGetter.noop());
   }
 
   @Override

diff --git a/...a/io/opentelemetry/instrumentation/api/instrumenter/net/NetServerAttributesExtractor.java b/...a/io/opentelemetry/instrumentation/api/instrumenter/net/NetServerAttributesExtractor.java
@@ -8,6 +8,7 @@
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor;
+import io.opentelemetry.instrumentation.api.instrumenter.net.internal.FallbackNamePortGetter;
 import io.opentelemetry.instrumentation.api.instrumenter.net.internal.InternalNetServerAttributesExtractor;
 import javax.annotation.Nullable;
 
@@ -29,12 +30,13 @@ public static <REQUEST, RESPONSE> NetServerAttributesExtractor<REQUEST, RESPONSE
 
   private NetServerAttributesExtractor(NetServerAttributesGetter<REQUEST> getter) {
     internalExtractor =
-        new InternalNetServerAttributesExtractor<>(getter, (integer, request) -> true);
+        new InternalNetServerAttributesExtractor<>(
+            getter, (integer, request) -> true, FallbackNamePortGetter.noop());
   }
 
   @Override
   public void onStart(AttributesBuilder attributes, Context parentContext, REQUEST request) {
-    internalExtractor.onStart(attributes, request, null);
+    internalExtractor.onStart(attributes, request);
   }
 
   @Override

diff --git a/...o/opentelemetry/instrumentation/api/instrumenter/net/internal/FallbackNamePortGetter.java b/...o/opentelemetry/instrumentation/api/instrumenter/net/internal/FallbackNamePortGetter.java
@@ -0,0 +1,26 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.instrumenter.net.internal;
+
+import javax.annotation.Nullable;
+
+/**
+ * This class is internal and is hence not for public use. Its APIs are unstable and can change at
+ * any time.
+ */
+public interface FallbackNamePortGetter<REQUEST> {
+
+  @Nullable
+  String name(REQUEST request);
+
+  @Nullable
+  Integer port(REQUEST request);
+
+  @SuppressWarnings("unchecked")
+  static <REQUEST> FallbackNamePortGetter<REQUEST> noop() {
+    return (FallbackNamePortGetter<REQUEST>) NoopNamePortGetter.INSTANCE;
+  }
+}
diff --git a/...y/instrumentation/api/instrumenter/net/internal/InternalNetClientAttributesExtractor.java b/...y/instrumentation/api/instrumenter/net/internal/InternalNetClientAttributesExtractor.java
@@ -21,22 +21,24 @@ public final class InternalNetClientAttributesExtractor<REQUEST, RESPONSE> {
 
   private final NetClientAttributesGetter<REQUEST, RESPONSE> getter;
   private final BiPredicate<Integer, REQUEST> capturePeerPortCondition;
+  private final FallbackNamePortGetter<REQUEST> fallbackNamePortGetter;
 
   public InternalNetClientAttributesExtractor(
       NetClientAttributesGetter<REQUEST, RESPONSE> getter,
-      BiPredicate<Integer, REQUEST> capturePeerPortCondition) {
+      BiPredicate<Integer, REQUEST> capturePeerPortCondition,
+      FallbackNamePortGetter<REQUEST> fallbackNamePortGetter) {
     this.getter = getter;
     this.capturePeerPortCondition = capturePeerPortCondition;
+    this.fallbackNamePortGetter = fallbackNamePortGetter;
   }
 
   public void onStart(AttributesBuilder attributes, REQUEST request) {
-    String peerName = getter.peerName(request);
-    Integer peerPort = getter.peerPort(request);
-
-    // TODO: add host header parsing
+    String peerName = extractPeerName(request);
 
     if (peerName != null) {
       internalSet(attributes, SemanticAttributes.NET_PEER_NAME, peerName);
+
+      Integer peerPort = extractPeerPort(request);
       if (peerPort != null && peerPort > 0 && capturePeerPortCondition.test(peerPort, request)) {
         internalSet(attributes, SemanticAttributes.NET_PEER_PORT, (long) peerPort);
       }
@@ -47,13 +49,13 @@ public void onEnd(AttributesBuilder attributes, REQUEST request, @Nullable RESPO
 
     internalSet(attributes, SemanticAttributes.NET_TRANSPORT, getter.transport(request, response));
 
-    String peerName = getter.peerName(request);
-    Integer peerPort = getter.peerPort(request);
+    String peerName = extractPeerName(request);
 
     String sockPeerAddr = getter.sockPeerAddr(request, response);
     if (sockPeerAddr != null && !sockPeerAddr.equals(peerName)) {
       internalSet(attributes, SemanticAttributes.NET_SOCK_PEER_ADDR, sockPeerAddr);
 
+      Integer peerPort = extractPeerPort(request);
       Integer sockPeerPort = getter.sockPeerPort(request, response);
       if (sockPeerPort != null && sockPeerPort > 0 && !sockPeerPort.equals(peerPort)) {
         internalSet(attributes, SemanticAttributes.NET_SOCK_PEER_PORT, (long) sockPeerPort);
@@ -70,4 +72,20 @@ public void onEnd(AttributesBuilder attributes, REQUEST request, @Nullable RESPO
       }
     }
   }
+
+  private String extractPeerName(REQUEST request) {
+    String peerName = getter.peerName(request);
+    if (peerName == null) {
+      peerName = fallbackNamePortGetter.name(request);
+    }
+    return peerName;
+  }
+
+  private Integer extractPeerPort(REQUEST request) {
+    Integer peerPort = getter.peerPort(request);
+    if (peerPort == null) {
+      peerPort = fallbackNamePortGetter.port(request);
+    }
+    return peerPort;
+  }
 }
diff --git a/...y/instrumentation/api/instrumenter/net/internal/InternalNetServerAttributesExtractor.java b/...y/instrumentation/api/instrumenter/net/internal/InternalNetServerAttributesExtractor.java
@@ -6,35 +6,32 @@
 package io.opentelemetry.instrumentation.api.instrumenter.net.internal;
 
 import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;
-import static java.util.logging.Level.FINE;
 
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.instrumentation.api.instrumenter.net.NetServerAttributesGetter;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.util.function.BiPredicate;
-import java.util.logging.Logger;
-import javax.annotation.Nullable;
 
 /**
  * This class is internal and is hence not for public use. Its APIs are unstable and can change at
  * any time.
  */
 public final class InternalNetServerAttributesExtractor<REQUEST> {
 
-  private static final Logger logger =
-      Logger.getLogger(InternalNetServerAttributesExtractor.class.getName());
-
   private final NetServerAttributesGetter<REQUEST> getter;
   private final BiPredicate<Integer, REQUEST> captureHostPortCondition;
+  private final FallbackNamePortGetter<REQUEST> fallbackNamePortGetter;
 
   public InternalNetServerAttributesExtractor(
       NetServerAttributesGetter<REQUEST> getter,
-      BiPredicate<Integer, REQUEST> captureHostPortCondition) {
+      BiPredicate<Integer, REQUEST> captureHostPortCondition,
+      FallbackNamePortGetter<REQUEST> fallbackNamePortGetter) {
     this.getter = getter;
     this.captureHostPortCondition = captureHostPortCondition;
+    this.fallbackNamePortGetter = fallbackNamePortGetter;
   }
 
-  public void onStart(AttributesBuilder attributes, REQUEST request, @Nullable String hostHeader) {
+  public void onStart(AttributesBuilder attributes, REQUEST request) {
     internalSet(attributes, SemanticAttributes.NET_TRANSPORT, getter.transport(request));
 
     boolean setSockFamily = false;
@@ -51,24 +48,8 @@ public void onStart(AttributesBuilder attributes, REQUEST request, @Nullable Str
       }
     }
 
-    String hostName = getter.hostName(request);
-    Integer hostPort = getter.hostPort(request);
-
-    int hostHeaderSeparator = -1;
-    if (hostHeader != null) {
-      hostHeaderSeparator = hostHeader.indexOf(':');
-    }
-    if (hostName == null && hostHeader != null) {
-      hostName =
-          hostHeaderSeparator == -1 ? hostHeader : hostHeader.substring(0, hostHeaderSeparator);
-    }
-    if (hostPort == null && hostHeader != null && hostHeaderSeparator != -1) {
-      try {
-        hostPort = Integer.parseInt(hostHeader.substring(hostHeaderSeparator + 1));
-      } catch (NumberFormatException e) {
-        logger.log(FINE, e.getMessage(), e);
-      }
-    }
+    String hostName = extractHostName(request);
+    Integer hostPort = extractHostPort(request);
 
     if (hostName != null) {
       internalSet(attributes, SemanticAttributes.NET_HOST_NAME, hostName);
@@ -97,4 +78,20 @@ public void onStart(AttributesBuilder attributes, REQUEST request, @Nullable Str
       }
     }
   }
+
+  private String extractHostName(REQUEST request) {
+    String peerName = getter.hostName(request);
+    if (peerName == null) {
+      peerName = fallbackNamePortGetter.name(request);
+    }
+    return peerName;
+  }
+
+  private Integer extractHostPort(REQUEST request) {
+    Integer peerPort = getter.hostPort(request);
+    if (peerPort == null) {
+      peerPort = fallbackNamePortGetter.port(request);
+    }
+    return peerPort;
+  }
 }
diff --git a/...va/io/opentelemetry/instrumentation/api/instrumenter/net/internal/NoopNamePortGetter.java b/...va/io/opentelemetry/instrumentation/api/instrumenter/net/internal/NoopNamePortGetter.java
@@ -0,0 +1,24 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.instrumenter.net.internal;
+
+import javax.annotation.Nullable;
+
+enum NoopNamePortGetter implements FallbackNamePortGetter<Object> {
+  INSTANCE;
+
+  @Nullable
+  @Override
+  public String name(Object o) {
+    return null;
+  }
+
+  @Nullable
+  @Override
+  public Integer port(Object o) {
+    return null;
+  }
+}