Skip to content

Commit

Permalink
Revert "Fix NPE in Apache HttpAsyncClient instrumentation (#3692)"
Browse files Browse the repository at this point in the history 
This reverts commit d305f31.
  • Loading branch information
@trask
trask authored Jul 28, 2021
1 parent d305f31 commit 1b8da11
Show file tree
Hide file tree
Showing 6 changed files with 54 additions and 272 deletions.
3 changes: 1 addition & 2 deletions ...javaagent/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientInstrumentation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,9 @@ public HttpHost getTarget() {

@Override
public HttpRequest generateRequest() throws IOException, HttpException {
HttpHost target = delegate.getTarget();
HttpRequest request = delegate.generateRequest();

ApacheHttpClientRequest otelRequest = new ApacheHttpClientRequest(target, request);
ApacheHttpClientRequest otelRequest = new ApacheHttpClientRequest(request);

if (instrumenter().shouldStart(parentContext, otelRequest)) {
wrappedFutureCallback.context = instrumenter().start(parentContext, otelRequest);
Expand Down
58 changes: 18 additions & 40 deletions ...pentelemetry/javaagent/instrumentation/apachehttpasyncclient/ApacheHttpClientRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@
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;
Expand All @@ -24,13 +25,24 @@ public final class ApacheHttpClientRequest {

private final HttpRequest delegate;

public ApacheHttpClientRequest(HttpHost httpHost, HttpRequest httpRequest) {
URI calculatedUri = getUri(httpRequest);
if (calculatedUri != null && httpHost != null) {
uri = getCalculatedUri(httpHost, calculatedUri);
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();
} else {
uri = calculatedUri;
RequestLine requestLine = httpRequest.getRequestLine();
try {
calculatedUri = new URI(requestLine.getUri());
} catch (URISyntaxException e) {
logger.debug(e.getMessage(), e);
calculatedUri = null;
}
}
uri = calculatedUri;
delegate = httpRequest;
}

Expand Down Expand Up @@ -121,38 +133,4 @@ public Integer getPeerPort() {
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;
}
}
}
139 changes: 27 additions & 112 deletions ...ion/apache-httpasyncclient-4.1/javaagent/src/test/groovy/ApacheHttpAsyncClientTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ 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
Expand All @@ -18,7 +17,7 @@ import org.apache.http.message.BasicHeader
import spock.lang.AutoCleanup
import spock.lang.Shared

abstract class ApacheHttpAsyncClientTest extends HttpClientTest<HttpUriRequest> implements AgentTestTrait {
class ApacheHttpAsyncClientTest extends HttpClientTest<HttpUriRequest> implements AgentTestTrait {

@Shared
RequestConfig requestConfig = RequestConfig.custom()
Expand All @@ -33,135 +32,51 @@ abstract class ApacheHttpAsyncClientTest extends HttpClientTest<HttpUriRequest>
client.start()
}

@Override
Integer responseCodeOnRedirectError() {
return 302
}

@Override
HttpUriRequest buildRequest(String method, URI uri, Map<String, String> headers) {
def request = createRequest(method, uri)
def request = new HttpUriRequest(method, uri)
headers.entrySet().each {
request.setHeader(new BasicHeader(it.key, it.value))
request.addHeader(new BasicHeader(it.key, it.value))
}
return request
}

@Override
Set<AttributeKey<?>> httpAttributes(URI uri) {
Set<AttributeKey<?>> extra = [
SemanticAttributes.HTTP_SCHEME,
SemanticAttributes.HTTP_TARGET
]
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)
return client.execute(request, null).get().statusLine.statusCode
}

@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)
}
}
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)
}

class ApacheClientHostAbsoluteUriRequest extends ApacheHttpAsyncClientTest {
@Override
void failed(Exception e) {
requestResult.complete(e)
}

@Override
HttpUriRequest createRequest(String method, URI uri) {
return new HttpUriRequest(method, new URI(uri.toString()))
@Override
void cancelled() {
throw new CancellationException()
}
})
}

@Override
HttpResponse executeRequest(HttpUriRequest request, URI uri) {
return client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request, null).get()
Integer responseCodeOnRedirectError() {
return 302
}

@Override
void executeRequestWithCallback(HttpUriRequest request, URI uri, FutureCallback<HttpResponse> callback) {
client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request, callback)
Set<AttributeKey<?>> httpAttributes(URI uri) {
Set<AttributeKey<?>> extra = [
SemanticAttributes.HTTP_SCHEME,
SemanticAttributes.HTTP_TARGET
]
super.httpAttributes(uri) + extra
}
}
34 changes: 8 additions & 26 deletions ...pentelemetry/javaagent/instrumentation/apachehttpclient/v4_0/ApacheHttpClientRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,14 @@ public final class ApacheHttpClientRequest {
private final HttpRequest delegate;

public ApacheHttpClientRequest(HttpHost httpHost, HttpRequest httpRequest) {
uri = getCalculatedUri(httpHost, httpRequest);
URI calculatedUri;
try {
calculatedUri = new URI(httpHost.toURI() + httpRequest.getRequestLine().getUri());
} catch (URISyntaxException e) {
logger.debug(e.getMessage(), e);
calculatedUri = null;
}
uri = calculatedUri;
delegate = httpRequest;
}

Expand Down Expand Up @@ -114,29 +121,4 @@ 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;
}
}
}
46 changes: 0 additions & 46 deletions ...he-httpclient/apache-httpclient-4.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,18 +94,11 @@ 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))
}

Expand All @@ -122,29 +115,9 @@ 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))
}

Expand All @@ -161,25 +134,6 @@ 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) {
Expand Down
Loading

0 comments on commit 1b8da11

Please sign in to comment.