Don't create duplicate headers (#2727)

* Test infra * Update examples * Update instrumentation * Update tests * jaxrs-client fixes * Remove doRequest/doReusedRequest * Fix muzzle * some fixes * fix test * doc * not private * Apply to doRequestWithCallback also * Update doc * groovy * better * Don't hardcode traceparent * Reuse request in SpringRestTemplateTest
open-telemetry · Apr 12, 2021 · 3bc058b · 3bc058b
1 parent 5be065c
commit 3bc058b
Show file tree

Hide file tree

Showing 50 changed files with 674 additions and 509 deletions.
diff --git a/...rc/main/java/com/example/javaagent/instrumentation/DemoServlet3InstrumentationModule.java b/...rc/main/java/com/example/javaagent/instrumentation/DemoServlet3InstrumentationModule.java
@@ -23,7 +23,8 @@
 import net.bytebuddy.matcher.ElementMatcher;
 
 /**
- * This is a demo instrumentation which hooks into servlet invocation and modifies the http response.
+ * This is a demo instrumentation which hooks into servlet invocation and modifies the http
+ * response.
  */
 @AutoService(InstrumentationModule.class)
 public final class DemoServlet3InstrumentationModule extends InstrumentationModule {
@@ -78,12 +79,10 @@ public static void onEnter(@Advice.Argument(value = 1) ServletResponse response)
 
         HttpServletResponse httpServletResponse = (HttpServletResponse) response;
         if (!httpServletResponse.containsHeader("X-server-id")) {
-          httpServletResponse
-              .addHeader("X-server-id",
-                  Java8BytecodeBridge.currentSpan().getSpanContext().getTraceId());
+          httpServletResponse.setHeader(
+              "X-server-id", Java8BytecodeBridge.currentSpan().getSpanContext().getTraceId());
         }
       }
-
     }
   }
-}
+}
diff --git a/...ntation/akka-http-10.0/javaagent/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy b/...ntation/akka-http-10.0/javaagent/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy
@@ -9,41 +9,42 @@ import akka.actor.ActorSystem
 import akka.http.javadsl.Http
 import akka.http.javadsl.model.HttpMethods
 import akka.http.javadsl.model.HttpRequest
-import akka.http.javadsl.model.HttpResponse
 import akka.http.javadsl.model.headers.RawHeader
 import akka.stream.ActorMaterializer
 import io.opentelemetry.instrumentation.test.AgentTestTrait
 import io.opentelemetry.instrumentation.test.base.HttpClientTest
-import java.util.concurrent.CompletionStage
 import java.util.function.Consumer
 import spock.lang.Shared
 
-class AkkaHttpClientInstrumentationTest extends HttpClientTest implements AgentTestTrait {
+class AkkaHttpClientInstrumentationTest extends HttpClientTest<HttpRequest> implements AgentTestTrait {
 
   @Shared
   ActorSystem system = ActorSystem.create()
   @Shared
   ActorMaterializer materializer = ActorMaterializer.create(system)
 
   @Override
-  int doRequest(String method, URI uri, Map<String, String> headers) {
-    return sendRequest(method, uri, headers).toCompletableFuture().get().status().intValue()
-  }
-
-  @Override
-  void doRequestWithCallback(String method, URI uri, Map<String, String> headers = [:], Consumer<Integer> callback) {
-    sendRequest(method, uri, headers).thenAccept {
-      callback.accept(it.status().intValue())
-    }
-  }
-
-  private CompletionStage<HttpResponse> sendRequest(String method, URI uri, Map<String, String> headers) {
-    def request = HttpRequest.create(uri.toString())
+  HttpRequest buildRequest(String method, URI uri, Map<String, String> headers) {
+    return HttpRequest.create(uri.toString())
       .withMethod(HttpMethods.lookup(method).get())
       .addHeaders(headers.collect { RawHeader.create(it.key, it.value) })
+  }
 
+  @Override
+  int sendRequest(HttpRequest request, String method, URI uri, Map<String, String> headers) {
     return Http.get(system)
       .singleRequest(request, materializer)
+      .toCompletableFuture()
+      .get()
+      .status()
+      .intValue()
+  }
+
+  @Override
+  void sendRequestWithCallback(HttpRequest request, String method, URI uri, Map<String, String> headers, Consumer<Integer> callback) {
+    Http.get(system).singleRequest(request, materializer).thenAccept {
+      callback.accept(it.status().intValue())
+    }
   }
 
   // TODO(anuraaga): Context leak seems to prevent us from running asynchronous tests in a row.

diff --git a/...ion/apache-httpasyncclient-4.1/javaagent/src/test/groovy/ApacheHttpAsyncClientTest.groovy b/...ion/apache-httpasyncclient-4.1/javaagent/src/test/groovy/ApacheHttpAsyncClientTest.groovy
@@ -15,7 +15,7 @@ import org.apache.http.message.BasicHeader
 import spock.lang.AutoCleanup
 import spock.lang.Shared
 
-class ApacheHttpAsyncClientTest extends HttpClientTest implements AgentTestTrait {
+class ApacheHttpAsyncClientTest extends HttpClientTest<HttpUriRequest> implements AgentTestTrait {
 
   @Shared
   RequestConfig requestConfig = RequestConfig.custom()
@@ -31,13 +31,22 @@ class ApacheHttpAsyncClientTest extends HttpClientTest implements AgentTestTrait
   }
 
   @Override
-  int doRequest(String method, URI uri, Map<String, String> headers = [:]) {
-    return client.execute(buildRequest(method, uri, headers), null).get().statusLine.statusCode
+  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 doRequestWithCallback(String method, URI uri, Map<String, String> headers = [:], Consumer<Integer> callback) {
-    client.execute(buildRequest(method, uri, headers), new FutureCallback<HttpResponse>() {
+  void sendRequestWithCallback(HttpUriRequest request, String method, URI uri, Map<String, String> headers, Consumer<Integer> callback) {
+    client.execute(request, new FutureCallback<HttpResponse>() {
       @Override
       void completed(HttpResponse httpResponse) {
         callback.accept(httpResponse.statusLine.statusCode)
@@ -55,14 +64,6 @@ class ApacheHttpAsyncClientTest extends HttpClientTest implements AgentTestTrait
     })
   }
 
-  private static 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
   Integer statusOnRedirectError() {
     return 302

diff --git a/...e-httpclient/apache-httpclient-2.0/javaagent/src/test/groovy/CommonsHttpClientTest.groovy b/...e-httpclient/apache-httpclient-2.0/javaagent/src/test/groovy/CommonsHttpClientTest.groovy
@@ -16,7 +16,7 @@ import org.apache.commons.httpclient.methods.PutMethod
 import org.apache.commons.httpclient.methods.TraceMethod
 import spock.lang.Shared
 
-class CommonsHttpClientTest extends HttpClientTest implements AgentTestTrait {
+class CommonsHttpClientTest extends HttpClientTest<HttpMethod> implements AgentTestTrait {
   @Shared
   HttpClient client = new HttpClient()
 
@@ -30,42 +30,44 @@ class CommonsHttpClientTest extends HttpClientTest implements AgentTestTrait {
   }
 
   @Override
-  int doRequest(String method, URI uri, Map<String, String> headers = [:]) {
-    HttpMethod httpMethod
-
+  HttpMethod buildRequest(String method, URI uri, Map<String, String> headers) {
+    def request
     switch (method) {
       case "GET":
-        httpMethod = new GetMethod(uri.toString())
+        request = new GetMethod(uri.toString())
         break
       case "PUT":
-        httpMethod = new PutMethod(uri.toString())
+        request = new PutMethod(uri.toString())
         break
       case "POST":
-        httpMethod = new PostMethod(uri.toString())
+        request = new PostMethod(uri.toString())
         break
       case "HEAD":
-        httpMethod = new HeadMethod(uri.toString())
+        request = new HeadMethod(uri.toString())
         break
       case "DELETE":
-        httpMethod = new DeleteMethod(uri.toString())
+        request = new DeleteMethod(uri.toString())
         break
       case "OPTIONS":
-        httpMethod = new OptionsMethod(uri.toString())
+        request = new OptionsMethod(uri.toString())
         break
       case "TRACE":
-        httpMethod = new TraceMethod(uri.toString())
+        request = new TraceMethod(uri.toString())
         break
       default:
         throw new RuntimeException("Unsupported method: " + method)
     }
+    headers.each { request.setRequestHeader(it.key, it.value) }
+    return request
+  }
 
-    headers.each { httpMethod.setRequestHeader(it.key, it.value) }
-
+  @Override
+  int sendRequest(HttpMethod request, String method, URI uri, Map<String, String> headers) {
     try {
-      client.executeMethod(httpMethod)
-      return httpMethod.getStatusCode()
+      client.executeMethod(request)
+      return request.getStatusCode()
     } finally {
-      httpMethod.releaseConnection()
+      request.releaseConnection()
     }
   }
 
@@ -75,6 +77,13 @@ class CommonsHttpClientTest extends HttpClientTest implements AgentTestTrait {
     false
   }
 
+  @Override
+  boolean testReusedRequest() {
+    // apache commons throws an exception if the request is reused without being recycled first
+    // at which point this test is not useful (and requires re-populating uri)
+    false
+  }
+
   @Override
   boolean testCallback() {
     false

diff --git a/...metry/javaagent/instrumentation/apachehttpclient/v4_0/HostAndRequestAsHttpUriRequest.java b/...metry/javaagent/instrumentation/apachehttpclient/v4_0/HostAndRequestAsHttpUriRequest.java
@@ -51,8 +51,8 @@ public boolean isAborted() {
   }
 
   @Override
-  public void addHeader(String name, String value) {
-    actualRequest.addHeader(name, value);
+  public void setHeader(String name, String value) {
+    actualRequest.setHeader(name, value);
   }
 
   @Override

diff --git a/...entelemetry/javaagent/instrumentation/apachehttpclient/v4_0/HttpHeadersInjectAdapter.java b/...entelemetry/javaagent/instrumentation/apachehttpclient/v4_0/HttpHeadersInjectAdapter.java
@@ -14,6 +14,6 @@ class HttpHeadersInjectAdapter implements TextMapSetter<HttpUriRequest> {
 
   @Override
   public void set(HttpUriRequest carrier, String key, String value) {
-    carrier.addHeader(key, value);
+    carrier.setHeader(key, value);
   }
 }
diff --git a/...he-httpclient/apache-httpclient-4.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy b/...he-httpclient/apache-httpclient-4.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy
@@ -17,7 +17,7 @@ import org.apache.http.params.HttpParams
 import org.apache.http.protocol.BasicHttpContext
 import spock.lang.Shared
 
-abstract class ApacheHttpClientTest<T extends HttpRequest> extends HttpClientTest implements AgentTestTrait {
+abstract class ApacheHttpClientTest<T extends HttpRequest> extends HttpClientTest<T> implements AgentTestTrait {
   @Shared
   def client = new DefaultHttpClient()
 
@@ -28,29 +28,27 @@ abstract class ApacheHttpClientTest<T extends HttpRequest> extends HttpClientTes
 
   @Override
   boolean testCausality() {
-    return false
+    false
   }
 
   @Override
-  int doRequest(String method, URI uri, Map<String, String> headers = [:]) {
+  T buildRequest(String method, URI uri, Map<String, String> headers) {
     def request = createRequest(method, uri)
     headers.entrySet().each {
-      request.addHeader(new BasicHeader(it.key, it.value))
+      request.setHeader(new BasicHeader(it.key, it.value))
     }
+    return request
+  }
 
+  // compilation fails with @Override annotation on this method (groovy quirk?)
+  int sendRequest(T 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
   }
 
-  @Override
-  void doRequestWithCallback(String method, URI uri, Map<String, String> headers = [:], Consumer<Integer> callback) {
-    def request = createRequest(method, uri)
-    headers.entrySet().each {
-      request.addHeader(new BasicHeader(it.key, it.value))
-    }
-
+  // compilation fails with @Override annotation on this method (groovy quirk?)
+  void sendRequestWithCallback(T request, String method, URI uri, Map<String, String> headers, Consumer<Integer> callback) {
     executeRequestWithCallback(request, uri) {
       it.entity?.content?.close() // Make sure the connection is closed.
       callback.accept(it.statusLine.statusCode)

diff --git a/...entelemetry/javaagent/instrumentation/apachehttpclient/v5_0/HttpHeadersInjectAdapter.java b/...entelemetry/javaagent/instrumentation/apachehttpclient/v5_0/HttpHeadersInjectAdapter.java
@@ -14,6 +14,6 @@ class HttpHeadersInjectAdapter implements TextMapSetter<ClassicHttpRequest> {
 
   @Override
   public void set(ClassicHttpRequest carrier, String key, String value) {
-    carrier.addHeader(key, value);
+    carrier.setHeader(key, value);
   }
 }
diff --git a/...he-httpclient/apache-httpclient-5.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy b/...he-httpclient/apache-httpclient-5.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy
@@ -22,7 +22,7 @@ import org.apache.hc.core5.http.protocol.BasicHttpContext
 import spock.lang.AutoCleanup
 import spock.lang.Shared
 
-abstract class ApacheHttpClientTest<T extends HttpRequest> extends HttpClientTest implements AgentTestTrait {
+abstract class ApacheHttpClientTest<T extends HttpRequest> extends HttpClientTest<T> implements AgentTestTrait {
   @Shared
   @AutoCleanup
   CloseableHttpClient client
@@ -37,25 +37,23 @@ abstract class ApacheHttpClientTest<T extends HttpRequest> extends HttpClientTes
   }
 
   @Override
-  int doRequest(String method, URI uri, Map<String, String> headers = [:]) {
+  T buildRequest(String method, URI uri, Map<String, String> headers) {
     def request = createRequest(method, uri)
     headers.entrySet().each {
-      request.addHeader(new BasicHeader(it.key, it.value))
+      request.setHeader(new BasicHeader(it.key, it.value))
     }
+    return request
+  }
 
+  // compilation fails with @Override annotation on this method (groovy quirk?)
+  int sendRequest(T request, String method, URI uri, Map<String, String> headers) {
     def response = executeRequest(request, uri)
     response.close() // Make sure the connection is closed.
-
     return response.code
   }
 
-  @Override
-  void doRequestWithCallback(String method, URI uri, Map<String, String> headers = [:], Consumer<Integer> callback) {
-    def request = createRequest(method, uri)
-    headers.entrySet().each {
-      request.addHeader(new BasicHeader(it.key, it.value))
-    }
-
+  // compilation fails with @Override annotation on this method (groovy quirk?)
+  void sendRequestWithCallback(T request, String method, URI uri, Map<String, String> headers, Consumer<Integer> callback) {
     executeRequestWithCallback(request, uri) {
       it.close() // Make sure the connection is closed.
       callback.accept(it.code)