Properly finish spans and support latest apache httpclient5 (#8272)

Author: amarziali

dd-java-agent/instrumentation/apache-httpclient-5/src/main/java/datadog/trace/instrumentation/apachehttpclient5/ApacheHttpAsyncClientInstrumentation.java

@@ -21,6 +21,7 @@
 import net.bytebuddy.matcher.ElementMatcher;
 import org.apache.hc.core5.concurrent.FutureCallback;
 import org.apache.hc.core5.http.nio.AsyncRequestProducer;
+import org.apache.hc.core5.http.protocol.BasicHttpContext;
 import org.apache.hc.core5.http.protocol.HttpContext;
 
 @AutoService(InstrumenterModule.class)
@@ -91,14 +92,18 @@ public static class ClientAdvice {
     @Advice.OnMethodEnter(suppress = Throwable.class)
     public static AgentScope methodEnter(
         @Advice.Argument(value = 0, readOnly = false) AsyncRequestProducer requestProducer,
-        @Advice.Argument(3) HttpContext context,
+        @Advice.Argument(value = 3, readOnly = false) HttpContext context,
         @Advice.Argument(value = 4, readOnly = false) FutureCallback<?> futureCallback) {
 
       final AgentScope parentScope = activeScope();
       final AgentSpan clientSpan = startSpan(HTTP_REQUEST);
       final AgentScope clientScope = activateSpan(clientSpan);
       DECORATE.afterStart(clientSpan);
 
+      if (context == null) {
+        context = new BasicHttpContext();
+      }
+
       requestProducer = new DelegatingRequestProducer(clientSpan, requestProducer);
       futureCallback =
           new TraceContinuedFutureCallback<>(parentScope, clientSpan, context, futureCallback);

dd-java-agent/instrumentation/apache-httpclient-5/src/main/java/datadog/trace/instrumentation/apachehttpclient5/TraceContinuedFutureCallback.java

@@ -35,7 +35,7 @@ public TraceContinuedFutureCallback(
 
   @Override
   public void completed(final T result) {
-    DECORATE.onResponse(clientSpan, getResponseFromHttpContext());
+    DECORATE.onResponse(clientSpan, extractHttpResponse(result));
     DECORATE.beforeFinish(clientSpan);
     clientSpan.finish(); // Finish span before calling delegate
 
@@ -51,7 +51,7 @@ public void completed(final T result) {
 
   @Override
   public void failed(final Exception ex) {
-    DECORATE.onResponse(clientSpan, getResponseFromHttpContext());
+    DECORATE.onResponse(clientSpan, extractHttpResponse(null));
     DECORATE.onError(clientSpan, ex);
     DECORATE.beforeFinish(clientSpan);
     clientSpan.finish(); // Finish span before calling delegate
@@ -68,7 +68,7 @@ public void failed(final Exception ex) {
 
   @Override
   public void cancelled() {
-    DECORATE.onResponse(clientSpan, getResponseFromHttpContext());
+    DECORATE.onResponse(clientSpan, extractHttpResponse(null));
     DECORATE.beforeFinish(clientSpan);
     clientSpan.finish(); // Finish span before calling delegate
 
@@ -101,7 +101,16 @@ private void cancelDelegate() {
   }
 
   @Nullable
-  private HttpResponse getResponseFromHttpContext() {
-    return (HttpResponse) context.getAttribute(HttpCoreContext.HTTP_RESPONSE);
+  private HttpResponse extractHttpResponse(Object futureResult) {
+    if (context != null) {
+      Object fromContext = context.getAttribute(HttpCoreContext.HTTP_RESPONSE);
+      if (fromContext instanceof HttpResponse) {
+        return (HttpResponse) fromContext;
+      }
+    }
+    if (futureResult instanceof HttpResponse) {
+      return (HttpResponse) futureResult;
+    }
+    return null;
   }
 }

dd-java-agent/instrumentation/apache-httpclient-5/src/test/groovy/ApacheHttpAsyncClient5Test.groovy

@@ -57,7 +57,7 @@ abstract class ApacheHttpAsyncClient5Test<T extends HttpRequest> extends HttpCli
   }
 }
 
-class ApacheHttpAsyncClient5NamingV0ForkedTest extends ApacheHttpAsyncClient5Test implements TestingGenericHttpNamingConventions.ClientV0 {
+class ApacheHttpAsyncClient5NamingV0Test extends ApacheHttpAsyncClient5Test implements TestingGenericHttpNamingConventions.ClientV0 {
 }
 
 class ApacheHttpAsyncClient5NamingV1ForkedTest extends ApacheHttpAsyncClient5Test implements TestingGenericHttpNamingConventions.ClientV1 {

dd-java-agent/instrumentation/apache-httpclient-5/src/test/groovy/ApacheHttpClientTest.groovy

@@ -208,7 +208,7 @@ abstract class ApacheClientResponseHandlerAll extends ApacheHttpClientTest<Class
 }
 
 @Timeout(5)
-class ApacheClientResponseHandlerAllV0ForkedTest extends ApacheClientResponseHandlerAll {
+class ApacheClientResponseHandlerAllV0Test extends ApacheClientResponseHandlerAll {
 }
 
 @Timeout(5)