Fix Apache HttpClient host + absolute uri (#3694)

instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientRequest.java

@@ -26,7 +26,12 @@ public final class ApacheHttpClientRequest {
   private final HttpRequest delegate;
 
   public ApacheHttpClientRequest(HttpHost httpHost, HttpRequest httpRequest) {
-    uri = getCalculatedUri(httpHost, httpRequest);
+    URI calculatedUri = getUri(httpRequest);
+    if (calculatedUri != null && httpHost != null) {
+      uri = getCalculatedUri(httpHost, calculatedUri);
+    } else {
+      uri = calculatedUri;
+    }
     delegate = httpRequest;
   }
 
@@ -116,15 +121,18 @@ public final class ApacheHttpClientRequest {
   }
 
   @Nullable
-  private static URI getCalculatedUri(HttpHost httpHost, HttpRequest httpRequest) {
-    final URI uri;
+  private static URI getUri(HttpRequest httpRequest) {
     try {
       // this can be relative or absolute
-      uri = new URI(httpRequest.getRequestLine().getUri());
+      return new URI(httpRequest.getRequestLine().getUri());
     } catch (URISyntaxException e) {
       logger.debug(e.getMessage(), e);
       return null;
     }
+  }
+
+  @Nullable
+  private static URI getCalculatedUri(HttpHost httpHost, URI uri) {
     try {
       return new URI(
           httpHost.getSchemeName(),

instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy

@@ -94,18 +94,12 @@ abstract class ApacheHttpClientTest<T extends HttpRequest> extends HttpClientTes
     }
     return builder.toString()
   }
-
-  static String absoluteUriWithNonResolvingHost(URI uri) {
-    // substituting a non-resolving host and port to make sure that the host and port from this uri are not used
-    return new URI(uri.getScheme(), uri.getAuthority(), "nowhere", 1, uri.getPath(), uri.getQuery(), uri.getFragment())
-      .toString()
-  }
 }
 
 class ApacheClientHostRequest extends ApacheHttpClientTest<BasicHttpRequest> {
   @Override
   BasicHttpRequest createRequest(String method, URI uri) {
-    // also testing with absolute path below
+    // also testing with an absolute path below
     return new BasicHttpRequest(method, fullPathFromURI(uri))
   }
 
@@ -125,7 +119,7 @@ class ApacheClientHostRequest extends ApacheHttpClientTest<BasicHttpRequest> {
 class ApacheClientHostAbsoluteUriRequest extends ApacheHttpClientTest<BasicHttpRequest> {
   @Override
   BasicHttpRequest createRequest(String method, URI uri) {
-    return new BasicHttpRequest(method, absoluteUriWithNonResolvingHost(uri))
+    return new BasicHttpRequest(method, uri.toString())
   }
 
   @Override
@@ -144,7 +138,7 @@ class ApacheClientHostAbsoluteUriRequest extends ApacheHttpClientTest<BasicHttpR
 class ApacheClientHostRequestContext extends ApacheHttpClientTest<BasicHttpRequest> {
   @Override
   BasicHttpRequest createRequest(String method, URI uri) {
-    // also testing with absolute path below
+    // also testing with an absolute path below
     return new BasicHttpRequest(method, fullPathFromURI(uri))
   }
 
@@ -164,7 +158,7 @@ class ApacheClientHostRequestContext extends ApacheHttpClientTest<BasicHttpReque
 class ApacheClientHostAbsoluteUriRequestContext extends ApacheHttpClientTest<BasicHttpRequest> {
   @Override
   BasicHttpRequest createRequest(String method, URI uri) {
-    return new BasicHttpRequest(method, absoluteUriWithNonResolvingHost(uri))
+    return new BasicHttpRequest(method, uri.toString())
   }
 
   @Override

instrumentation/apache-httpclient/apache-httpclient-4.3/library/src/main/java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/ApacheHttpClientRequest.java

@@ -12,7 +12,6 @@ import org.apache.http.Header;
 import org.apache.http.HttpHost;
 import org.apache.http.HttpRequest;
 import org.apache.http.ProtocolVersion;
-import org.apache.http.client.methods.HttpUriRequest;
 import org.checkerframework.checker.nullness.qual.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -26,18 +25,12 @@ public final class ApacheHttpClientRequest {
   private final HttpRequest delegate;
 
   ApacheHttpClientRequest(@Nullable HttpHost httpHost, HttpRequest httpRequest) {
-    URI calculatedUri = null;
-    if (httpRequest instanceof HttpUriRequest) {
-      calculatedUri = ((HttpUriRequest) httpRequest).getURI();
-    }
-    if (calculatedUri == null && httpHost != null) {
-      try {
-        calculatedUri = new URI(httpHost.toURI() + httpRequest.getRequestLine().getUri());
-      } catch (URISyntaxException e) {
-        // Ignore
-      }
+    URI calculatedUri = getUri(httpRequest);
+    if (calculatedUri != null && httpHost != null) {
+      uri = getCalculatedUri(httpHost, calculatedUri);
+    } else {
+      uri = calculatedUri;
     }
-    uri = calculatedUri;
     delegate = httpRequest;
   }
 
@@ -132,4 +125,32 @@ public final class ApacheHttpClientRequest {
         return null;
     }
   }
+
+  @Nullable
+  private static URI getUri(HttpRequest httpRequest) {
+    try {
+      // this can be relative or absolute
+      return new URI(httpRequest.getRequestLine().getUri());
+    } catch (URISyntaxException e) {
+      logger.debug(e.getMessage(), e);
+      return null;
+    }
+  }
+
+  @Nullable
+  private static URI getCalculatedUri(HttpHost httpHost, URI uri) {
+    try {
+      return new URI(
+          httpHost.getSchemeName(),
+          null,
+          httpHost.getHostName(),
+          httpHost.getPort(),
+          uri.getPath(),
+          uri.getQuery(),
+          uri.getFragment());
+    } catch (URISyntaxException e) {
+      logger.debug(e.getMessage(), e);
+      return null;
+    }
+  }
 }

instrumentation/apache-httpclient/apache-httpclient-4.3/library/src/test/groovy/io/opentelemetry/instrumentation/apachehttpclient/v4_3/ApacheClientHostAbsoluteUriRequestContextTest.groovy

@@ -0,0 +1,28 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.apachehttpclient.v4_3
+
+import io.opentelemetry.instrumentation.test.LibraryTestTrait
+import org.apache.http.client.config.RequestConfig
+import org.apache.http.impl.client.CloseableHttpClient
+
+class ApacheClientHostAbsoluteUriRequestContextTest extends AbstractApacheClientHostAbsoluteUriRequestContextTest implements LibraryTestTrait {
+  @Override
+  protected CloseableHttpClient createClient() {
+    def builder = ApacheHttpClientTracing.create(openTelemetry).newHttpClientBuilder()
+    builder.defaultRequestConfig = RequestConfig.custom()
+      .setMaxRedirects(maxRedirects())
+      .setConnectTimeout(CONNECT_TIMEOUT_MS)
+      .build()
+    return builder.build()
+  }
+
+  // library instrumentation doesn't have a good way of suppressing nested CLIENT spans yet
+  @Override
+  boolean testWithClientParent() {
+    false
+  }
+}

instrumentation/apache-httpclient/apache-httpclient-4.3/library/src/test/groovy/io/opentelemetry/instrumentation/apachehttpclient/v4_3/ApacheClientHostAbsoluteUriRequestTest.groovy

@@ -0,0 +1,28 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.apachehttpclient.v4_3
+
+import io.opentelemetry.instrumentation.test.LibraryTestTrait
+import org.apache.http.client.config.RequestConfig
+import org.apache.http.impl.client.CloseableHttpClient
+
+class ApacheClientHostAbsoluteUriRequestTest extends AbstractApacheClientHostAbsoluteUriRequestTest implements LibraryTestTrait {
+  @Override
+  protected CloseableHttpClient createClient() {
+    def builder = ApacheHttpClientTracing.create(openTelemetry).newHttpClientBuilder()
+    builder.defaultRequestConfig = RequestConfig.custom()
+      .setMaxRedirects(maxRedirects())
+      .setConnectTimeout(CONNECT_TIMEOUT_MS)
+      .build()
+    return builder.build()
+  }
+
+  // library instrumentation doesn't have a good way of suppressing nested CLIENT spans yet
+  @Override
+  boolean testWithClientParent() {
+    false
+  }
+}

instrumentation/apache-httpclient/apache-httpclient-4.3/testing/src/main/groovy/io/opentelemetry/instrumentation/apachehttpclient/v4_3/ApacheHttpClientTest.groovy

@@ -101,6 +101,7 @@ abstract class ApacheHttpClientTest<T extends HttpRequest> extends HttpClientTes
 abstract class AbstractApacheClientHostRequestTest extends ApacheHttpClientTest<BasicHttpRequest> {
   @Override
   BasicHttpRequest createRequest(String method, URI uri) {
+    // also testing with an absolute path below
     return new BasicHttpRequest(method, fullPathFromURI(uri))
   }
 
@@ -117,9 +118,29 @@ abstract class AbstractApacheClientHostRequestTest extends ApacheHttpClientTest<
   }
 }
 
+abstract class AbstractApacheClientHostAbsoluteUriRequestTest extends ApacheHttpClientTest<BasicHttpRequest> {
+  @Override
+  BasicHttpRequest createRequest(String method, URI uri) {
+    return new BasicHttpRequest(method, uri.toString())
+  }
+
+  @Override
+  HttpResponse executeRequest(BasicHttpRequest request, URI uri) {
+    return client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request)
+  }
+
+  @Override
+  void executeRequestWithCallback(BasicHttpRequest request, URI uri, Consumer<HttpResponse> callback) {
+    client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request) {
+      callback.accept(it)
+    }
+  }
+}
+
 abstract class AbstractApacheClientHostRequestContextTest extends ApacheHttpClientTest<BasicHttpRequest> {
   @Override
   BasicHttpRequest createRequest(String method, URI uri) {
+    // also testing with an absolute path below
     return new BasicHttpRequest(method, fullPathFromURI(uri))
   }
 
@@ -136,6 +157,25 @@ abstract class AbstractApacheClientHostRequestContextTest extends ApacheHttpClie
   }
 }
 
+abstract class AbstractApacheClientHostAbsoluteUriRequestContextTest extends ApacheHttpClientTest<BasicHttpRequest> {
+  @Override
+  BasicHttpRequest createRequest(String method, URI uri) {
+    return new BasicHttpRequest(method, uri.toString())
+  }
+
+  @Override
+  HttpResponse executeRequest(BasicHttpRequest request, URI uri) {
+    return client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request, new BasicHttpContext())
+  }
+
+  @Override
+  void executeRequestWithCallback(BasicHttpRequest request, URI uri, Consumer<HttpResponse> callback) {
+    client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request, {
+      callback.accept(it)
+    }, new BasicHttpContext())
+  }
+}
+
 abstract class AbstractApacheClientUriRequestTest extends ApacheHttpClientTest<HttpUriRequest> {
   @Override
   HttpUriRequest createRequest(String method, URI uri) {

instrumentation/apache-httpclient/apache-httpclient-5.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy

@@ -104,6 +104,7 @@ abstract class ApacheHttpClientTest<T extends HttpRequest> extends HttpClientTes
 class ApacheClientHostRequest extends ApacheHttpClientTest<ClassicHttpRequest> {
   @Override
   ClassicHttpRequest createRequest(String method, URI uri) {
+    // also testing with an absolute path below
     return new BasicClassicHttpRequest(method, fullPathFromURI(uri))
   }
 
@@ -123,10 +124,6 @@ class ApacheClientHostRequest extends ApacheHttpClientTest<ClassicHttpRequest> {
 class ApacheClientHostAbsoluteUriRequest extends ApacheHttpClientTest<ClassicHttpRequest> {
   @Override
   ClassicHttpRequest createRequest(String method, URI uri) {
-    // TODO(trask) substitute a non-resolving host and port to make sure that the host and port
-    //  from this uri are not used (currently that causes redirect tests to fail
-    //  because Apache HttpClient 5 appears to resolve relative redirects against this uri
-    //  instead of against the host, which is different from Apache HttpClient 4 behavior)
     return new BasicClassicHttpRequest(method, uri.toString())
   }
 
@@ -146,6 +143,7 @@ class ApacheClientHostAbsoluteUriRequest extends ApacheHttpClientTest<ClassicHtt
 class ApacheClientHostRequestContext extends ApacheHttpClientTest<ClassicHttpRequest> {
   @Override
   ClassicHttpRequest createRequest(String method, URI uri) {
+    // also testing with an absolute path below
     return new BasicClassicHttpRequest(method, fullPathFromURI(uri))
   }
 
@@ -165,10 +163,6 @@ class ApacheClientHostRequestContext extends ApacheHttpClientTest<ClassicHttpReq
 class ApacheClientHostAbsoluteUriRequestContext extends ApacheHttpClientTest<ClassicHttpRequest> {
   @Override
   ClassicHttpRequest createRequest(String method, URI uri) {
-    // TODO(trask) substitute a non-resolving host and port to make sure that the host and port
-    //  from this uri are not used (currently that causes redirect tests to fail
-    //  because Apache HttpClient 5 appears to resolve relative redirects against this uri
-    //  instead of against the host, which is different from Apache HttpClient 4 behavior)
     return new BasicClassicHttpRequest(method, uri.toString())
   }