Fix Apache HttpClient host + absolute uri

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

@@ -26,14 +26,7 @@ public final class ApacheHttpClientRequest {
   private final HttpRequest delegate;
 
   public ApacheHttpClientRequest(HttpHost httpHost, HttpRequest httpRequest) {
-    URI calculatedUri;
-    try {
-      calculatedUri = new URI(httpHost.toURI() + httpRequest.getRequestLine().getUri());
-    } catch (URISyntaxException e) {
-      logger.debug(e.getMessage(), e);
-      calculatedUri = null;
-    }
-    uri = calculatedUri;
+    uri = getCalculatedUri(httpHost, httpRequest);
     delegate = httpRequest;
   }
 
@@ -121,4 +114,29 @@ public Integer getPeerPort() {
         return null;
     }
   }
+
+  @Nullable
+  private static URI getCalculatedUri(HttpHost httpHost, HttpRequest httpRequest) {
+    final URI uri;
+    try {
+      // this can be relative or absolute
+      uri = new URI(httpRequest.getRequestLine().getUri());
+    } catch (URISyntaxException e) {
+      logger.debug(e.getMessage(), e);
+      return null;
+    }
+    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.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy

@@ -94,11 +94,18 @@ 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 an absolute path below
     return new BasicHttpRequest(method, fullPathFromURI(uri))
   }
 
@@ -115,9 +122,29 @@ class ApacheClientHostRequest extends ApacheHttpClientTest<BasicHttpRequest> {
   }
 }
 
+class ApacheClientHostAbsoluteUriRequest extends ApacheHttpClientTest<BasicHttpRequest> {
+  @Override
+  BasicHttpRequest createRequest(String method, URI uri) {
+    return new BasicHttpRequest(method, absoluteUriWithNonResolvingHost(uri))
+  }
+
+  @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)
+    }
+  }
+}
+
 class ApacheClientHostRequestContext extends ApacheHttpClientTest<BasicHttpRequest> {
   @Override
   BasicHttpRequest createRequest(String method, URI uri) {
+    // also testing with an absolute path below
     return new BasicHttpRequest(method, fullPathFromURI(uri))
   }
 
@@ -134,6 +161,25 @@ class ApacheClientHostRequestContext extends ApacheHttpClientTest<BasicHttpReque
   }
 }
 
+class ApacheClientHostAbsoluteUriRequestContext extends ApacheHttpClientTest<BasicHttpRequest> {
+  @Override
+  BasicHttpRequest createRequest(String method, URI uri) {
+    return new BasicHttpRequest(method, absoluteUriWithNonResolvingHost(uri))
+  }
+
+  @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())
+  }
+}
+
 class ApacheClientUriRequest 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))
   }
 
@@ -120,9 +121,33 @@ 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())
+  }
+
+  @Override
+  ClassicHttpResponse executeRequest(ClassicHttpRequest request, URI uri) {
+    return client.execute(new HttpHost(uri.getScheme(), uri.getHost(), uri.getPort()), request)
+  }
+
+  @Override
+  void executeRequestWithCallback(ClassicHttpRequest request, URI uri, Consumer<ClassicHttpResponse> callback) {
+    client.execute(new HttpHost(uri.getScheme(), uri.getHost(), uri.getPort()), request) {
+      callback.accept(it)
+    }
+  }
+}
+
 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))
   }
 
@@ -139,6 +164,29 @@ 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())
+  }
+
+  @Override
+  ClassicHttpResponse executeRequest(ClassicHttpRequest request, URI uri) {
+    return client.execute(new HttpHost(uri.getScheme(), uri.getHost(), uri.getPort()), request, new BasicHttpContext())
+  }
+
+  @Override
+  void executeRequestWithCallback(ClassicHttpRequest request, URI uri, Consumer<ClassicHttpResponse> callback) {
+    client.execute(new HttpHost(uri.getScheme(), uri.getHost(), uri.getPort()), request, new BasicHttpContext()) {
+      callback.accept(it)
+    }
+  }
+}
+
 class ApacheClientUriRequest extends ApacheHttpClientTest<ClassicHttpRequest> {
   @Override
   ClassicHttpRequest createRequest(String method, URI uri) {