Add HttpClientTestOptions (#3714)

* Add HttpClientTestOptions

* Drift

instrumentation/armeria-1.3/library/src/test/java/io/opentelemetry/instrumentation/armeria/v1_3/ArmeriaHttpClientTest.java

@@ -8,6 +8,7 @@ package io.opentelemetry.instrumentation.armeria.v1_3;
 import com.linecorp.armeria.client.WebClientBuilder;
 import io.opentelemetry.instrumentation.testing.junit.InstrumentationExtension;
 import io.opentelemetry.instrumentation.testing.junit.http.HttpClientInstrumentationExtension;
+import io.opentelemetry.instrumentation.testing.junit.http.HttpClientTestOptions;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
 class ArmeriaHttpClientTest extends AbstractArmeriaHttpClientTest {
@@ -21,21 +22,16 @@ class ArmeriaHttpClientTest extends AbstractArmeriaHttpClientTest {
         ArmeriaTracing.create(testing.getOpenTelemetry()).newClientDecorator());
   }
 
-  // library instrumentation doesn't have a good way of suppressing nested CLIENT spans yet
   @Override
-  protected boolean testWithClientParent() {
+  protected void configure(HttpClientTestOptions options) {
-    return false;
+    super.configure(options);
-  }
+
+    // library instrumentation doesn't have a good way of suppressing nested CLIENT spans yet
+    options.disableTestWithClientParent();
 
     // Agent users have automatic propagation through executor instrumentation, but library users
     // should do manually using Armeria patterns.
-  @Override
+    options.disableTestCallbackWithParent();
-  protected boolean testCallbackWithParent() {
+    options.disableTestErrorWithCallback();
-    return false;
-  }
-
-  @Override
-  protected boolean testErrorWithCallback() {
-    return false;
   }
 }

instrumentation/armeria-1.3/testing/src/main/java/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpClientTest.java

@@ -14,6 +14,7 @@ import com.linecorp.armeria.common.RequestHeaders;
 import com.linecorp.armeria.common.util.Exceptions;
 import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpClientTest;
+import io.opentelemetry.instrumentation.testing.junit.http.HttpClientTestOptions;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.net.URI;
 import java.util.HashSet;
@@ -70,27 +71,20 @@ public abstract class AbstractArmeriaHttpClientTest extends AbstractHttpClientTe
                 requestResult.complete(() -> response.status().code(), throwable));
   }
 
+  @Override
+  protected void configure(HttpClientTestOptions options) {
     // Not supported yet: https://github.com/line/armeria/issues/2489
-  @Override
+    options.disableTestRedirects();
-  protected final boolean testRedirects() {
-    return false;
-  }
-
-  @Override
-  protected final boolean testReusedRequest() {
     // armeria requests can't be reused
-    return false;
+    options.disableTestReusedRequest();
-  }
 
-  @Override
-  protected Set<AttributeKey<?>> httpAttributes(URI uri) {
     Set<AttributeKey<?>> extra = new HashSet<>();
     extra.add(SemanticAttributes.HTTP_HOST);
     extra.add(SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH);
     extra.add(SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH);
     extra.add(SemanticAttributes.HTTP_SCHEME);
     extra.add(SemanticAttributes.HTTP_TARGET);
-    extra.addAll(super.httpAttributes(uri));
+    extra.addAll(HttpClientTestOptions.DEFAULT_HTTP_ATTRIBUTES);
-    return extra;
+    options.setHttpAttributes(unused -> extra);
   }
 }

testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpClientTest.groovy

@@ -227,6 +227,7 @@ abstract class HttpClientTest<REQUEST> extends InstrumentationSpecification {
   def setupSpec() {
     server = new HttpClientTestServer(openTelemetry)
     server.start()
+    junitTest.setupOptions()
     junitTest.setTesting(testRunner(), server)
   }
 

testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/AbstractHttpClientTest.java

@@ -42,12 +42,15 @@ import java.util.function.Consumer;
 import java.util.function.Supplier;
 import java.util.stream.IntStream;
 import org.checkerframework.checker.nullness.qual.Nullable;
+import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInstance;
 import org.junit.jupiter.api.condition.DisabledIfSystemProperty;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ValueSource;
 
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
 public abstract class AbstractHttpClientTest<REQUEST> {
   static final String BASIC_AUTH_KEY = "custom-authorization-header";
   static final String BASIC_AUTH_VAL = "plain text auth token";
@@ -136,6 +139,65 @@ public abstract class AbstractHttpClientTest<REQUEST> {
   private InstrumentationTestRunner testing;
   private HttpClientTestServer server;
 
+  private final HttpClientTestOptions options = new HttpClientTestOptions();
+
+  @BeforeAll
+  void setupOptions() {
+    // TODO(anuraaga): Have subclasses configure options directly and remove mapping of legacy
+    // protected methods.
+    options.setHttpAttributes(this::httpAttributes);
+    options.setExpectedClientSpanNameMapper(this::expectedClientSpanName);
+    Integer responseCodeOnError = responseCodeOnRedirectError();
+    if (responseCodeOnError != null) {
+      options.setResponseCodeOnRedirectError(responseCodeOnError);
+    }
+    options.setUserAgent(userAgent());
+    options.setClientSpanErrorMapper(this::clientSpanError);
+    options.setSingleConnectionFactory(this::createSingleConnection);
+    if (!testWithClientParent()) {
+      options.disableTestWithClientParent();
+    }
+    if (!testRedirects()) {
+      options.disableTestRedirects();
+    }
+    if (!testCircularRedirects()) {
+      options.disableTestCircularRedirects();
+    }
+    options.setMaxRedirects(maxRedirects());
+    if (!testReusedRequest()) {
+      options.disableTestReusedRequest();
+    }
+    if (!testConnectionFailure()) {
+      options.disableTestConnectionFailure();
+    }
+    if (testReadTimeout()) {
+      options.enableTestReadTimeout();
+    }
+    if (!testRemoteConnection()) {
+      options.disableTestRemoteConnection();
+    }
+    if (!testHttps()) {
+      options.disableTestHttps();
+    }
+    if (!testCausality()) {
+      options.disableTestCausality();
+    }
+    if (!testCausalityWithCallback()) {
+      options.disableTestCausalityWithCallback();
+    }
+    if (!testCallback()) {
+      options.disableTestCallback();
+    }
+    if (!testCallbackWithParent()) {
+      options.disableTestCallbackWithParent();
+    }
+    if (!testErrorWithCallback()) {
+      options.disableTestErrorWithCallback();
+    }
+
+    configure(options);
+  }
+
   @BeforeEach
   void verifyExtension() {
     if (testing == null) {
@@ -198,7 +260,7 @@ public abstract class AbstractHttpClientTest<REQUEST> {
   @ParameterizedTest
   @ValueSource(strings = {"PUT", "POST"})
   void shouldSuppressNestedClientSpanIfAlreadyUnderParentClientSpan(String method) {
-    assumeTrue(testWithClientParent());
+    assumeTrue(options.testWithClientParent);
 
     URI uri = resolveAddress("/success");
     int responseCode = runUnderParentClientSpan(() -> doRequest(method, uri));
@@ -219,8 +281,8 @@ public abstract class AbstractHttpClientTest<REQUEST> {
 
   @Test
   void requestWithCallbackAndParent() throws Throwable {
-    assumeTrue(testCallback());
+    assumeTrue(options.testCallback);
-    assumeTrue(testCallbackWithParent());
+    assumeTrue(options.testCallbackWithParent);
 
     String method = "GET";
     URI uri = resolveAddress("/success");
@@ -256,7 +318,7 @@ public abstract class AbstractHttpClientTest<REQUEST> {
 
   @Test
   void requestWithCallbackAndNoParent() throws Throwable {
-    assumeTrue(testCallback());
+    assumeTrue(options.testCallback);
 
     String method = "GET";
     URI uri = resolveAddress("/success");
@@ -289,7 +351,7 @@ public abstract class AbstractHttpClientTest<REQUEST> {
     // TODO quite a few clients create an extra span for the redirect
     // This test should handle both types or we should unify how the clients work
 
-    assumeTrue(testRedirects());
+    assumeTrue(options.testRedirects);
 
     String method = "GET";
     URI uri = resolveAddress("/redirect");
@@ -318,7 +380,7 @@ public abstract class AbstractHttpClientTest<REQUEST> {
     // TODO quite a few clients create an extra span for the redirect
     // This test should handle both types or we should unify how the clients work
 
-    assumeTrue(testRedirects());
+    assumeTrue(options.testRedirects);
 
     String method = "GET";
     URI uri = resolveAddress("/another-redirect");
@@ -345,8 +407,8 @@ public abstract class AbstractHttpClientTest<REQUEST> {
 
   @Test
   void circularRedirects() {
-    assumeTrue(testRedirects());
+    assumeTrue(options.testRedirects);
-    assumeTrue(testCircularRedirects());
+    assumeTrue(options.testCircularRedirects);
 
     String method = "GET";
     URI uri = resolveAddress("/circular-redirect");
@@ -358,7 +420,7 @@ public abstract class AbstractHttpClientTest<REQUEST> {
     } else {
       ex = thrown;
     }
-    Throwable clientError = clientSpanError(uri, ex);
+    Throwable clientError = options.clientSpanErrorMapper.apply(uri, ex);
 
     testing.waitAndAssertTraces(
         trace -> {
@@ -369,10 +431,10 @@ public abstract class AbstractHttpClientTest<REQUEST> {
           List<Consumer<SpanDataAssert>> assertions = new ArrayList<>();
           assertions.add(
               span ->
-                  assertClientSpan(span, uri, method, responseCodeOnRedirectError())
+                  assertClientSpan(span, uri, method, options.responseCodeOnRedirectError)
                       .hasParentSpanId(SpanId.getInvalid())
                       .hasEventsSatisfyingExactly(hasException(clientError)));
-          for (int i = 0; i < maxRedirects(); i++) {
+          for (int i = 0; i < options.maxRedirects; i++) {
             assertions.add(
                 span -> assertServerSpan(span).hasParentSpanId(traces.get(0).get(0).getSpanId()));
           }
@@ -382,7 +444,7 @@ public abstract class AbstractHttpClientTest<REQUEST> {
 
   @Test
   void redirectToSecuredCopiesAuthHeader() {
-    assumeTrue(testRedirects());
+    assumeTrue(options.testRedirects);
 
     String method = "GET";
     URI uri = resolveAddress("/to-secured");
@@ -439,7 +501,7 @@ public abstract class AbstractHttpClientTest<REQUEST> {
 
   @Test
   void reuseRequest() {
-    assumeTrue(testReusedRequest());
+    assumeTrue(options.testReusedRequest);
 
     String method = "GET";
     URI uri = resolveAddress("/success");
@@ -504,7 +566,7 @@ public abstract class AbstractHttpClientTest<REQUEST> {
 
   @Test
   void connectionErrorUnopenedPort() {
-    assumeTrue(testConnectionFailure());
+    assumeTrue(options.testConnectionFailure);
 
     String method = "GET";
     URI uri = URI.create("http://localhost:" + PortUtils.UNUSABLE_PORT + '/');
@@ -517,7 +579,7 @@ public abstract class AbstractHttpClientTest<REQUEST> {
     } else {
       ex = thrown;
     }
-    Throwable clientError = clientSpanError(uri, ex);
+    Throwable clientError = options.clientSpanErrorMapper.apply(uri, ex);
 
     testing.waitAndAssertTraces(
         trace -> {
@@ -544,9 +606,9 @@ public abstract class AbstractHttpClientTest<REQUEST> {
 
   @Test
   void connectionErrorUnopenedPortWithCallback() {
-    assumeTrue(testConnectionFailure());
+    assumeTrue(options.testConnectionFailure);
-    assumeTrue(testCallback());
+    assumeTrue(options.testCallback);
-    assumeTrue(testErrorWithCallback());
+    assumeTrue(options.testErrorWithCallback);
 
     String method = "GET";
     URI uri = URI.create("http://localhost:" + PortUtils.UNUSABLE_PORT + '/');
@@ -565,7 +627,7 @@ public abstract class AbstractHttpClientTest<REQUEST> {
     } else {
       ex = thrown;
     }
-    Throwable clientError = clientSpanError(uri, ex);
+    Throwable clientError = options.clientSpanErrorMapper.apply(uri, ex);
 
     testing.waitAndAssertTraces(
         trace -> {
@@ -591,7 +653,7 @@ public abstract class AbstractHttpClientTest<REQUEST> {
 
   @Test
   void connectionErrorNonRoutableAddress() {
-    assumeTrue(testRemoteConnection());
+    assumeTrue(options.testRemoteConnection);
 
     String method = "HEAD";
     URI uri = URI.create("https://192.0.2.1/");
@@ -604,7 +666,7 @@ public abstract class AbstractHttpClientTest<REQUEST> {
     } else {
       ex = thrown;
     }
-    Throwable clientError = clientSpanError(uri, ex);
+    Throwable clientError = options.clientSpanErrorMapper.apply(uri, ex);
 
     testing.waitAndAssertTraces(
         trace -> {
@@ -628,7 +690,7 @@ public abstract class AbstractHttpClientTest<REQUEST> {
 
   @Test
   void readTimedOut() {
-    assumeTrue(testReadTimeout());
+    assumeTrue(options.testReadTimeout);
 
     String method = "GET";
     URI uri = resolveAddress("/read-timeout");
@@ -641,7 +703,7 @@ public abstract class AbstractHttpClientTest<REQUEST> {
     } else {
       ex = thrown;
     }
-    Throwable clientError = clientSpanError(uri, ex);
+    Throwable clientError = options.clientSpanErrorMapper.apply(uri, ex);
 
     testing.waitAndAssertTraces(
         trace -> {
@@ -670,8 +732,8 @@ public abstract class AbstractHttpClientTest<REQUEST> {
       disabledReason = "IBM JVM has different protocol support for TLS")
   @Test
   void httpsRequest() {
-    assumeTrue(testRemoteConnection());
+    assumeTrue(options.testRemoteConnection);
-    assumeTrue(testHttps());
+    assumeTrue(options.testHttps);
 
     String method = "GET";
     URI uri = URI.create("https://localhost:" + server.httpsPort() + "/success");
@@ -702,7 +764,7 @@ public abstract class AbstractHttpClientTest<REQUEST> {
    */
   @Test
   void highConcurrency() {
-    assumeTrue(testCausality());
+    assumeTrue(options.testCausality);
 
     int count = 50;
     String method = "GET";
@@ -772,10 +834,10 @@ public abstract class AbstractHttpClientTest<REQUEST> {
 
   @Test
   void highConcurrencyWithCallback() {
-    assumeTrue(testCausality());
+    assumeTrue(options.testCausality);
-    assumeTrue(testCausalityWithCallback());
+    assumeTrue(options.testCausalityWithCallback);
-    assumeTrue(testCallback());
+    assumeTrue(options.testCallback);
-    assumeTrue(testCallbackWithParent());
+    assumeTrue(options.testCallbackWithParent);
 
     int count = 50;
     String method = "GET";
@@ -854,7 +916,8 @@ public abstract class AbstractHttpClientTest<REQUEST> {
    */
   @Test
   void highConcurrencyOnSingleConnection() {
-    SingleConnection singleConnection = createSingleConnection("localhost", server.httpPort());
+    SingleConnection singleConnection =
+        options.singleConnectionFactory.apply("localhost", server.httpPort());
     assumeTrue(singleConnection != null);
 
     int count = 50;
@@ -931,7 +994,7 @@ public abstract class AbstractHttpClientTest<REQUEST> {
   SpanDataAssert assertClientSpan(
       SpanDataAssert span, URI uri, String method, Integer responseCode) {
     Set<AttributeKey<?>> httpClientAttributes = httpAttributes(uri);
-    return span.hasName(expectedClientSpanName(uri, method))
+    return span.hasName(options.expectedClientSpanNameMapper.apply(uri, method))
         .hasKind(SpanKind.CLIENT)
         .hasAttributesSatisfying(
             attrs -> {
@@ -995,7 +1058,7 @@ public abstract class AbstractHttpClientTest<REQUEST> {
                         SemanticAttributes.HttpFlavorValues.HTTP_1_1);
               }
               if (httpClientAttributes.contains(SemanticAttributes.HTTP_USER_AGENT)) {
-                String userAgent = userAgent();
+                String userAgent = options.userAgent;
                 if (userAgent != null) {
                   // TODO(anuraaga): Remove after updating to SDK 1.5.0 which adds this into
                   // hasEntrySatisfying.
@@ -1157,6 +1220,8 @@ public abstract class AbstractHttpClientTest<REQUEST> {
     return true;
   }
 
+  protected void configure(HttpClientTestOptions options) {}
+
   // Workaround until release of
   // https://github.com/open-telemetry/opentelemetry-java/pull/3409
   // in 1.5

testing-common/src/main/java/io/opentelemetry/instrumentation/testing/junit/http/HttpClientTestOptions.java

@@ -0,0 +1,161 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.testing.junit.http;
+
+import io.opentelemetry.api.common.AttributeKey;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
+import java.net.URI;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.function.BiFunction;
+import java.util.function.Function;
+
+public final class HttpClientTestOptions {
+
+  public static final Set<AttributeKey<?>> DEFAULT_HTTP_ATTRIBUTES =
+      Collections.unmodifiableSet(
+          new HashSet<>(
+              Arrays.asList(
+                  SemanticAttributes.HTTP_URL,
+                  SemanticAttributes.HTTP_METHOD,
+                  SemanticAttributes.HTTP_FLAVOR,
+                  SemanticAttributes.HTTP_USER_AGENT)));
+
+  Function<URI, Set<AttributeKey<?>>> httpAttributes = unused -> DEFAULT_HTTP_ATTRIBUTES;
+
+  BiFunction<URI, String, String> expectedClientSpanNameMapper =
+      (uri, method) -> method != null ? "HTTP " + method : "HTTP request";
+
+  Integer responseCodeOnRedirectError = null;
+  String userAgent = null;
+
+  BiFunction<URI, Throwable, Throwable> clientSpanErrorMapper = (uri, exception) -> exception;
+
+  BiFunction<String, Integer, SingleConnection> singleConnectionFactory = (host, port) -> null;
+
+  boolean testWithClientParent = true;
+  boolean testRedirects = true;
+  boolean testCircularRedirects = true;
+  int maxRedirects = 2;
+  boolean testReusedRequest = true;
+  boolean testConnectionFailure = true;
+  boolean testReadTimeout = false;
+  boolean testRemoteConnection = true;
+  boolean testHttps = true;
+  boolean testCausality = true;
+  boolean testCausalityWithCallback = true;
+  boolean testCallback = true;
+  boolean testCallbackWithParent = true;
+  boolean testErrorWithCallback = true;
+
+  HttpClientTestOptions() {}
+
+  public HttpClientTestOptions setHttpAttributes(
+      Function<URI, Set<AttributeKey<?>>> httpAttributes) {
+    this.httpAttributes = httpAttributes;
+    return this;
+  }
+
+  public HttpClientTestOptions setExpectedClientSpanNameMapper(
+      BiFunction<URI, String, String> expectedClientSpanNameMapper) {
+    this.expectedClientSpanNameMapper = expectedClientSpanNameMapper;
+    return this;
+  }
+
+  public HttpClientTestOptions setResponseCodeOnRedirectError(int responseCodeOnRedirectError) {
+    this.responseCodeOnRedirectError = responseCodeOnRedirectError;
+    return this;
+  }
+
+  public HttpClientTestOptions setUserAgent(String userAgent) {
+    this.userAgent = userAgent;
+    return this;
+  }
+
+  public HttpClientTestOptions setClientSpanErrorMapper(
+      BiFunction<URI, Throwable, Throwable> clientSpanErrorMapper) {
+    this.clientSpanErrorMapper = clientSpanErrorMapper;
+    return this;
+  }
+
+  public HttpClientTestOptions setSingleConnectionFactory(
+      BiFunction<String, Integer, SingleConnection> singleConnectionFactory) {
+    this.singleConnectionFactory = singleConnectionFactory;
+    return this;
+  }
+
+  public HttpClientTestOptions setMaxRedirects(int maxRedirects) {
+    this.maxRedirects = maxRedirects;
+    return this;
+  }
+
+  public HttpClientTestOptions disableTestWithClientParent() {
+    testWithClientParent = false;
+    return this;
+  }
+
+  public HttpClientTestOptions disableTestRedirects() {
+    testRedirects = false;
+    return this;
+  }
+
+  public HttpClientTestOptions disableTestCircularRedirects() {
+    testCircularRedirects = false;
+    return this;
+  }
+
+  public HttpClientTestOptions disableTestReusedRequest() {
+    testReusedRequest = false;
+    return this;
+  }
+
+  public HttpClientTestOptions disableTestConnectionFailure() {
+    testConnectionFailure = false;
+    return this;
+  }
+
+  public HttpClientTestOptions enableTestReadTimeout() {
+    testReadTimeout = true;
+    return this;
+  }
+
+  public HttpClientTestOptions disableTestRemoteConnection() {
+    testRemoteConnection = false;
+    return this;
+  }
+
+  public HttpClientTestOptions disableTestHttps() {
+    testHttps = false;
+    return this;
+  }
+
+  public HttpClientTestOptions disableTestCausality() {
+    testCausality = false;
+    return this;
+  }
+
+  public HttpClientTestOptions disableTestCausalityWithCallback() {
+    testCausalityWithCallback = false;
+    return this;
+  }
+
+  public HttpClientTestOptions disableTestCallback() {
+    testCallback = false;
+    return this;
+  }
+
+  public HttpClientTestOptions disableTestCallbackWithParent() {
+    testCallbackWithParent = false;
+    return this;
+  }
+
+  public HttpClientTestOptions disableTestErrorWithCallback() {
+    testErrorWithCallback = false;
+    return this;
+  }
+}