Fix NPE in Apache HttpAsyncClient instrumentation (#3692)

* Fix NPE in Apache HttpAsyncClient instrumentation

* Fix Apache HttpClient host + absolute uri

* Add similar test for Apache HttpClient 5

* Better tests

* Sync with 4.0 and 4.3

* Fix

* sync

* Elasticsearch twist

* Remove so-called optimization path

instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientInstrumentation.java

@@ -100,9 +100,10 @@ public class ApacheHttpAsyncClientInstrumentation implements TypeInstrumentation
 
     @Override
     public HttpRequest generateRequest() throws IOException, HttpException {
+      HttpHost target = delegate.getTarget();
       HttpRequest request = delegate.generateRequest();
 
-      ApacheHttpClientRequest otelRequest = new ApacheHttpClientRequest(request);
+      ApacheHttpClientRequest otelRequest = new ApacheHttpClientRequest(target, request);
 
       if (instrumenter().shouldStart(parentContext, otelRequest)) {
         wrappedFutureCallback.context = instrumenter().start(parentContext, otelRequest);

instrumentation/apache-httpasyncclient-4.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpClientRequest.java

@@ -9,10 +9,9 @@ import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.net.URI;
 import java.net.URISyntaxException;
 import org.apache.http.Header;
+import org.apache.http.HttpHost;
 import org.apache.http.HttpRequest;
 import org.apache.http.ProtocolVersion;
-import org.apache.http.RequestLine;
-import org.apache.http.client.methods.HttpUriRequest;
 import org.checkerframework.checker.nullness.qual.Nullable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -25,24 +24,13 @@ public final class ApacheHttpClientRequest {
 
   private final HttpRequest delegate;
 
-  public ApacheHttpClientRequest(HttpRequest httpRequest) {
-    URI calculatedUri;
-    if (httpRequest instanceof HttpUriRequest) {
-      // Note: this is essentially an optimization: HttpUriRequest allows quicker access to required
-      // information. The downside is that we need to load HttpUriRequest which essentially means we
-      // depend on httpasyncclient library depending on httpclient library. Currently this seems to
-      // be the case.
-      calculatedUri = ((HttpUriRequest) httpRequest).getURI();
+  public ApacheHttpClientRequest(HttpHost httpHost, HttpRequest httpRequest) {
+    URI calculatedUri = getUri(httpRequest);
+    if (calculatedUri != null && httpHost != null) {
+      uri = getCalculatedUri(httpHost, calculatedUri);
     } else {
-      RequestLine requestLine = httpRequest.getRequestLine();
-      try {
-        calculatedUri = new URI(requestLine.getUri());
-      } catch (URISyntaxException e) {
-        logger.debug(e.getMessage(), e);
-        calculatedUri = null;
-      }
+      uri = calculatedUri;
     }
-    uri = calculatedUri;
     delegate = httpRequest;
   }
 
@@ -133,4 +121,38 @@ 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 {
+      String path = uri.getPath();
+      if (!path.startsWith("/")) {
+        // elasticsearch RestClient sends relative urls
+        // TODO(trask) add test for this and extend to Apache 4, 4.3 and 5
+        path = "/" + path;
+      }
+      return new URI(
+          httpHost.getSchemeName(),
+          null,
+          httpHost.getHostName(),
+          httpHost.getPort(),
+          path,
+          uri.getQuery(),
+          uri.getFragment());
+    } catch (URISyntaxException e) {
+      logger.debug(e.getMessage(), e);
+      return null;
+    }
+  }
 }

instrumentation/apache-httpasyncclient-4.1/javaagent/src/test/groovy/ApacheHttpAsyncClientTest.groovy

@@ -9,6 +9,7 @@ import io.opentelemetry.instrumentation.test.base.HttpClientTest
 import io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpClientTest
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes
 import java.util.concurrent.CancellationException
+import org.apache.http.HttpHost
 import org.apache.http.HttpResponse
 import org.apache.http.client.config.RequestConfig
 import org.apache.http.concurrent.FutureCallback
@@ -17,7 +18,7 @@ import org.apache.http.message.BasicHeader
 import spock.lang.AutoCleanup
 import spock.lang.Shared
 
-class ApacheHttpAsyncClientTest extends HttpClientTest<HttpUriRequest> implements AgentTestTrait {
+abstract class ApacheHttpAsyncClientTest extends HttpClientTest<HttpUriRequest> implements AgentTestTrait {
 
   @Shared
   RequestConfig requestConfig = RequestConfig.custom()
@@ -32,45 +33,20 @@ class ApacheHttpAsyncClientTest extends HttpClientTest<HttpUriRequest> implement
     client.start()
   }
 
-  @Override
-  HttpUriRequest buildRequest(String method, URI uri, Map<String, String> headers) {
-    def request = new HttpUriRequest(method, uri)
-    headers.entrySet().each {
-      request.addHeader(new BasicHeader(it.key, it.value))
-    }
-    return request
-  }
-
-  @Override
-  int sendRequest(HttpUriRequest request, String method, URI uri, Map<String, String> headers) {
-    return client.execute(request, null).get().statusLine.statusCode
-  }
-
-  @Override
-  void sendRequestWithCallback(HttpUriRequest request, String method, URI uri, Map<String, String> headers, AbstractHttpClientTest.RequestResult requestResult) {
-    client.execute(request, new FutureCallback<HttpResponse>() {
-      @Override
-      void completed(HttpResponse httpResponse) {
-        requestResult.complete(httpResponse.statusLine.statusCode)
-      }
-
-      @Override
-      void failed(Exception e) {
-        requestResult.complete(e)
-      }
-
-      @Override
-      void cancelled() {
-        throw new CancellationException()
-      }
-    })
-  }
-
   @Override
   Integer responseCodeOnRedirectError() {
     return 302
   }
 
+  @Override
+  HttpUriRequest buildRequest(String method, URI uri, Map<String, String> headers) {
+    def request = createRequest(method, uri)
+    headers.entrySet().each {
+      request.setHeader(new BasicHeader(it.key, it.value))
+    }
+    return request
+  }
+
   @Override
   Set<AttributeKey<?>> httpAttributes(URI uri) {
     Set<AttributeKey<?>> extra = [
@@ -79,4 +55,113 @@ class ApacheHttpAsyncClientTest extends HttpClientTest<HttpUriRequest> implement
     ]
     super.httpAttributes(uri) + extra
   }
+
+  // compilation fails with @Override annotation on this method (groovy quirk?)
+  int sendRequest(HttpUriRequest request, String method, URI uri, Map<String, String> headers) {
+    def response = executeRequest(request, uri)
+    response.entity?.content?.close() // Make sure the connection is closed.
+    return response.statusLine.statusCode
+  }
+
+  // compilation fails with @Override annotation on this method (groovy quirk?)
+  void sendRequestWithCallback(HttpUriRequest request, String method, URI uri, Map<String, String> headers, AbstractHttpClientTest.RequestResult requestResult) {
+    try {
+      executeRequestWithCallback(request, uri, new FutureCallback<HttpResponse>() {
+        @Override
+        void completed(HttpResponse httpResponse) {
+          httpResponse.entity?.content?.close() // Make sure the connection is closed.
+          requestResult.complete(httpResponse.statusLine.statusCode)
+        }
+
+        @Override
+        void failed(Exception e) {
+          requestResult.complete(e)
+        }
+
+        @Override
+        void cancelled() {
+          requestResult.complete(new CancellationException())
+        }
+      })
+    } catch (Throwable throwable) {
+      requestResult.complete(throwable)
+    }
+  }
+
+  abstract HttpUriRequest createRequest(String method, URI uri)
+
+  abstract HttpResponse executeRequest(HttpUriRequest request, URI uri)
+
+  abstract void executeRequestWithCallback(HttpUriRequest request, URI uri, FutureCallback<HttpResponse> callback)
+
+  static String fullPathFromURI(URI uri) {
+    StringBuilder builder = new StringBuilder()
+    if (uri.getPath() != null) {
+      builder.append(uri.getPath())
+    }
+
+    if (uri.getQuery() != null) {
+      builder.append('?')
+      builder.append(uri.getQuery())
+    }
+
+    if (uri.getFragment() != null) {
+      builder.append('#')
+      builder.append(uri.getFragment())
+    }
+    return builder.toString()
+  }
+}
+
+class ApacheClientUriRequest extends ApacheHttpAsyncClientTest {
+  @Override
+  HttpUriRequest createRequest(String method, URI uri) {
+    return new HttpUriRequest(method, uri)
+  }
+
+  @Override
+  HttpResponse executeRequest(HttpUriRequest request, URI uri) {
+    return client.execute(request, null).get()
+  }
+
+  @Override
+  void executeRequestWithCallback(HttpUriRequest request, URI uri, FutureCallback<HttpResponse> callback) {
+    client.execute(request, callback)
+  }
+}
+
+class ApacheClientHostRequest extends ApacheHttpAsyncClientTest {
+  @Override
+  HttpUriRequest createRequest(String method, URI uri) {
+    // also testing with absolute path below
+    return new HttpUriRequest(method, new URI(fullPathFromURI(uri)))
+  }
+
+  @Override
+  HttpResponse executeRequest(HttpUriRequest request, URI uri) {
+    return client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request, null).get()
+  }
+
+  @Override
+  void executeRequestWithCallback(HttpUriRequest request, URI uri, FutureCallback<HttpResponse> callback) {
+    client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request, callback)
+  }
+}
+
+class ApacheClientHostAbsoluteUriRequest extends ApacheHttpAsyncClientTest {
+
+  @Override
+  HttpUriRequest createRequest(String method, URI uri) {
+    return new HttpUriRequest(method, new URI(uri.toString()))
+  }
+
+  @Override
+  HttpResponse executeRequest(HttpUriRequest request, URI uri) {
+    return client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request, null).get()
+  }
+
+  @Override
+  void executeRequestWithCallback(HttpUriRequest request, URI uri, FutureCallback<HttpResponse> callback) {
+    client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request, callback)
+  }
 }

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 final class ApacheHttpClientRequest {
         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 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 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

@@ -120,6 +120,29 @@ 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) {
@@ -139,6 +162,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) {