Fix Apache HttpClient host + absolute uri
MODIFY
instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientRequest.java
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(),

MODIFY
instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy
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

MODIFY
instrumentation/apache-httpclient/apache-httpclient-4.3/library/src/main/java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/ApacheHttpClientRequest.java
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;
+ }
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;
+ }
+ }
}

ADD
instrumentation/apache-httpclient/apache-httpclient-4.3/library/src/test/groovy/io/opentelemetry/instrumentation/apachehttpclient/v4_3/ApacheClientHostAbsoluteUriRequestContextTest.groovy
+/*
+ * 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
+ }
+}

ADD
instrumentation/apache-httpclient/apache-httpclient-4.3/library/src/test/groovy/io/opentelemetry/instrumentation/apachehttpclient/v4_3/ApacheClientHostAbsoluteUriRequestTest.groovy
+/*
+ * 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
+ }
+}

MODIFY
instrumentation/apache-httpclient/apache-httpclient-4.3/testing/src/main/groovy/io/opentelemetry/instrumentation/apachehttpclient/v4_3/ApacheHttpClientTest.groovy
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) {

MODIFY
instrumentation/apache-httpclient/apache-httpclient-5.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy
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())
}

Fix Apache HttpClient host + absolute uri (#3694)