Merge pull request #1320 from DataDog/tyler/http-client-testing

Add tests for http clients

dd-java-agent/instrumentation/akka-http-10.0/src/test/groovy/AkkaHttpClientInstrumentationTest.groovy

@@ -9,7 +9,9 @@ import datadog.trace.api.DDSpanTypes
 import datadog.trace.bootstrap.instrumentation.api.Tags
 import datadog.trace.instrumentation.akkahttp.AkkaHttpClientDecorator
 import spock.lang.Shared
+import spock.lang.Timeout
 
+@Timeout(5)
 class AkkaHttpClientInstrumentationTest extends HttpClientTest {
 
   @Shared
@@ -56,6 +58,12 @@ class AkkaHttpClientInstrumentationTest extends HttpClientTest {
     false
   }
 
+  @Override
+  boolean testRemoteConnection() {
+    // Not sure how to properly set timeouts...
+    return false
+  }
+
   def "singleRequest exception trace"() {
     when:
     // Passing null causes NPE in singleRequest

dd-java-agent/instrumentation/apache-httpasyncclient-4/src/test/groovy/ApacheHttpAsyncClientCallbackTest.groovy

@@ -1,19 +1,28 @@
 import datadog.trace.agent.test.base.HttpClientTest
 import datadog.trace.instrumentation.apachehttpasyncclient.ApacheHttpAsyncClientDecorator
 import org.apache.http.HttpResponse
+import org.apache.http.client.config.RequestConfig
 import org.apache.http.concurrent.FutureCallback
 import org.apache.http.impl.nio.client.HttpAsyncClients
 import org.apache.http.message.BasicHeader
 import spock.lang.AutoCleanup
 import spock.lang.Shared
+import spock.lang.Timeout
 
 import java.util.concurrent.CompletableFuture
 
+@Timeout(5)
 class ApacheHttpAsyncClientCallbackTest extends HttpClientTest {
 
+  @Shared
+  RequestConfig requestConfig = RequestConfig.custom()
+    .setConnectTimeout(CONNECT_TIMEOUT_MS)
+    .setSocketTimeout(READ_TIMEOUT_MS)
+    .build()
+
   @AutoCleanup
   @Shared
-  def client = HttpAsyncClients.createDefault()
+  def client = HttpAsyncClients.custom().setDefaultRequestConfig(requestConfig).build()
 
   def setupSpec() {
     client.start()
@@ -59,4 +68,9 @@ class ApacheHttpAsyncClientCallbackTest extends HttpClientTest {
   Integer statusOnRedirectError() {
     return 302
   }
+
+  @Override
+  boolean testRemoteConnection() {
+    false // otherwise SocketTimeoutException for https requests
+  }
 }

dd-java-agent/instrumentation/apache-httpasyncclient-4/src/test/groovy/ApacheHttpAsyncClientNullCallbackTest.groovy

@@ -1,17 +1,26 @@
 import datadog.trace.agent.test.base.HttpClientTest
 import datadog.trace.instrumentation.apachehttpasyncclient.ApacheHttpAsyncClientDecorator
+import org.apache.http.client.config.RequestConfig
 import org.apache.http.impl.nio.client.HttpAsyncClients
 import org.apache.http.message.BasicHeader
 import spock.lang.AutoCleanup
 import spock.lang.Shared
+import spock.lang.Timeout
 
 import java.util.concurrent.Future
 
+@Timeout(5)
 class ApacheHttpAsyncClientNullCallbackTest extends HttpClientTest {
 
+  @Shared
+  RequestConfig requestConfig = RequestConfig.custom()
+    .setConnectTimeout(CONNECT_TIMEOUT_MS)
+    .setSocketTimeout(READ_TIMEOUT_MS)
+    .build()
+
   @AutoCleanup
   @Shared
-  def client = HttpAsyncClients.createDefault()
+  def client = HttpAsyncClients.custom().setDefaultRequestConfig(requestConfig).build()
 
   def setupSpec() {
     client.start()
@@ -45,4 +54,9 @@ class ApacheHttpAsyncClientNullCallbackTest extends HttpClientTest {
   Integer statusOnRedirectError() {
     return 302
   }
+
+  @Override
+  boolean testRemoteConnection() {
+    false // otherwise SocketTimeoutException for https requests
+  }
 }

dd-java-agent/instrumentation/apache-httpasyncclient-4/src/test/groovy/ApacheHttpAsyncClientTest.groovy

@@ -1,17 +1,26 @@
 import datadog.trace.agent.test.base.HttpClientTest
 import datadog.trace.instrumentation.apachehttpasyncclient.ApacheHttpAsyncClientDecorator
 import org.apache.http.HttpResponse
+import org.apache.http.client.config.RequestConfig
 import org.apache.http.concurrent.FutureCallback
 import org.apache.http.impl.nio.client.HttpAsyncClients
 import org.apache.http.message.BasicHeader
 import spock.lang.AutoCleanup
 import spock.lang.Shared
+import spock.lang.Timeout
 
+@Timeout(5)
 class ApacheHttpAsyncClientTest extends HttpClientTest {
 
+  @Shared
+  RequestConfig requestConfig = RequestConfig.custom()
+    .setConnectTimeout(CONNECT_TIMEOUT_MS)
+    .setSocketTimeout(READ_TIMEOUT_MS)
+    .build()
+
   @AutoCleanup
   @Shared
-  def client = HttpAsyncClients.createDefault()
+  def client = HttpAsyncClients.custom().setDefaultRequestConfig(requestConfig).build()
 
   def setupSpec() {
     client.start()
@@ -54,4 +63,9 @@ class ApacheHttpAsyncClientTest extends HttpClientTest {
   Integer statusOnRedirectError() {
     return 302
   }
+
+  @Override
+  boolean testRemoteConnection() {
+    false // otherwise SocketTimeoutException for https requests
+  }
 }

dd-java-agent/instrumentation/apache-httpclient-4/src/test/groovy/ApacheHttpClientResponseHandlerTest.groovy

@@ -4,8 +4,12 @@ import org.apache.http.HttpResponse
 import org.apache.http.client.ResponseHandler
 import org.apache.http.impl.client.DefaultHttpClient
 import org.apache.http.message.BasicHeader
+import org.apache.http.params.HttpConnectionParams
+import org.apache.http.params.HttpParams
 import spock.lang.Shared
+import spock.lang.Timeout
 
+@Timeout(5)
 class ApacheHttpClientResponseHandlerTest extends HttpClientTest {
 
   @Shared
@@ -19,6 +23,12 @@ class ApacheHttpClientResponseHandlerTest extends HttpClientTest {
     }
   }
 
+  def setupSpec() {
+    HttpParams httpParams = client.getParams()
+    HttpConnectionParams.setConnectionTimeout(httpParams, CONNECT_TIMEOUT_MS)
+    HttpConnectionParams.setSoTimeout(httpParams, READ_TIMEOUT_MS)
+  }
+
   @Override
   int doRequest(String method, URI uri, Map<String, String> headers, Closure callback) {
     def request = new HttpUriRequest(method, uri)

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

@@ -6,13 +6,22 @@ import org.apache.http.HttpResponse
 import org.apache.http.impl.client.DefaultHttpClient
 import org.apache.http.message.BasicHeader
 import org.apache.http.message.BasicHttpRequest
+import org.apache.http.params.HttpConnectionParams
+import org.apache.http.params.HttpParams
 import org.apache.http.protocol.BasicHttpContext
 import spock.lang.Shared
+import spock.lang.Timeout
 
 abstract class ApacheHttpClientTest<T extends HttpRequest> extends HttpClientTest {
   @Shared
   def client = new DefaultHttpClient()
 
+  def setupSpec() {
+    HttpParams httpParams = client.getParams()
+    HttpConnectionParams.setConnectionTimeout(httpParams, CONNECT_TIMEOUT_MS)
+    HttpConnectionParams.setSoTimeout(httpParams, READ_TIMEOUT_MS)
+  }
+
   @Override
   String component() {
     return ApacheHttpClientDecorator.DECORATE.component()
@@ -55,6 +64,7 @@ abstract class ApacheHttpClientTest<T extends HttpRequest> extends HttpClientTes
   }
 }
 
+@Timeout(5)
 class ApacheClientHostRequest extends ApacheHttpClientTest<BasicHttpRequest> {
   @Override
   BasicHttpRequest createRequest(String method, URI uri) {
@@ -65,8 +75,14 @@ class ApacheClientHostRequest extends ApacheHttpClientTest<BasicHttpRequest> {
   HttpResponse executeRequest(BasicHttpRequest request, URI uri) {
     return client.execute(new HttpHost(uri.getHost(), uri.getPort()), request)
   }
+
+  @Override
+  boolean testRemoteConnection() {
+    return false
+  }
 }
 
+@Timeout(5)
 class ApacheClientHostRequestContext extends ApacheHttpClientTest<BasicHttpRequest> {
   @Override
   BasicHttpRequest createRequest(String method, URI uri) {
@@ -77,8 +93,14 @@ class ApacheClientHostRequestContext extends ApacheHttpClientTest<BasicHttpReque
   HttpResponse executeRequest(BasicHttpRequest request, URI uri) {
     return client.execute(new HttpHost(uri.getHost(), uri.getPort()), request, new BasicHttpContext())
   }
+
+  @Override
+  boolean testRemoteConnection() {
+    return false
+  }
 }
 
+@Timeout(5)
 class ApacheClientHostRequestResponseHandler extends ApacheHttpClientTest<BasicHttpRequest> {
   @Override
   BasicHttpRequest createRequest(String method, URI uri) {
@@ -89,8 +111,14 @@ class ApacheClientHostRequestResponseHandler extends ApacheHttpClientTest<BasicH
   HttpResponse executeRequest(BasicHttpRequest request, URI uri) {
     return client.execute(new HttpHost(uri.getHost(), uri.getPort()), request, { response -> response })
   }
+
+  @Override
+  boolean testRemoteConnection() {
+    return false
+  }
 }
 
+@Timeout(5)
 class ApacheClientHostRequestResponseHandlerContext extends ApacheHttpClientTest<BasicHttpRequest> {
   @Override
   BasicHttpRequest createRequest(String method, URI uri) {
@@ -101,8 +129,14 @@ class ApacheClientHostRequestResponseHandlerContext extends ApacheHttpClientTest
   HttpResponse executeRequest(BasicHttpRequest request, URI uri) {
     return client.execute(new HttpHost(uri.getHost(), uri.getPort()), request, { response -> response }, new BasicHttpContext())
   }
+
+  @Override
+  boolean testRemoteConnection() {
+    return false
+  }
 }
 
+@Timeout(5)
 class ApacheClientUriRequest extends ApacheHttpClientTest<HttpUriRequest> {
   @Override
   HttpUriRequest createRequest(String method, URI uri) {
@@ -115,6 +149,7 @@ class ApacheClientUriRequest extends ApacheHttpClientTest<HttpUriRequest> {
   }
 }
 
+@Timeout(5)
 class ApacheClientUriRequestContext extends ApacheHttpClientTest<HttpUriRequest> {
   @Override
   HttpUriRequest createRequest(String method, URI uri) {
@@ -127,6 +162,7 @@ class ApacheClientUriRequestContext extends ApacheHttpClientTest<HttpUriRequest>
   }
 }
 
+@Timeout(5)
 class ApacheClientUriRequestResponseHandler extends ApacheHttpClientTest<HttpUriRequest> {
   @Override
   HttpUriRequest createRequest(String method, URI uri) {
@@ -139,6 +175,7 @@ class ApacheClientUriRequestResponseHandler extends ApacheHttpClientTest<HttpUri
   }
 }
 
+@Timeout(5)
 class ApacheClientUriRequestResponseHandlerContext extends ApacheHttpClientTest<HttpUriRequest> {
   @Override
   HttpUriRequest createRequest(String method, URI uri) {

dd-java-agent/instrumentation/commons-httpclient-2/src/test/groovy/CommonsHttpClientTest.groovy

@@ -10,11 +10,18 @@ import org.apache.commons.httpclient.methods.PostMethod
 import org.apache.commons.httpclient.methods.PutMethod
 import org.apache.commons.httpclient.methods.TraceMethod
 import spock.lang.Shared
+import spock.lang.Timeout
 
+@Timeout(5)
 class CommonsHttpClientTest extends HttpClientTest {
   @Shared
   HttpClient client = new HttpClient()
 
+  def setupSpec() {
+    client.setConnectionTimeout(CONNECT_TIMEOUT_MS)
+    client.setTimeout(READ_TIMEOUT_MS)
+  }
+
   @Override
   int doRequest(String method, URI uri, Map<String, String> headers, Closure callback) {
     HttpMethod httpMethod

dd-java-agent/instrumentation/google-http-client/src/test/groovy/AbstractGoogleHttpClientTest.groovy

@@ -23,6 +23,8 @@ abstract class AbstractGoogleHttpClientTest extends HttpClientTest {
     GenericUrl genericUrl = new GenericUrl(uri)
 
     HttpRequest request = requestFactory.buildRequest(method, genericUrl, null)
+    request.connectTimeout = CONNECT_TIMEOUT_MS
+    request.readTimeout = READ_TIMEOUT_MS
     request.getHeaders().putAll(headers)
     request.setThrowExceptionOnExecuteError(throwExceptionOnError)
 

dd-java-agent/instrumentation/google-http-client/src/test/groovy/GoogleHttpClientAsyncTest.groovy

@@ -1,6 +1,8 @@
 import com.google.api.client.http.HttpRequest
 import com.google.api.client.http.HttpResponse
+import spock.lang.Timeout
 
+@Timeout(5)
 class GoogleHttpClientAsyncTest extends AbstractGoogleHttpClientTest {
   @Override
   HttpResponse executeRequest(HttpRequest request) {

dd-java-agent/instrumentation/google-http-client/src/test/groovy/GoogleHttpClientTest.groovy

@@ -1,9 +1,12 @@
 import com.google.api.client.http.HttpRequest
 import com.google.api.client.http.HttpResponse
 import spock.lang.Retry
+import spock.lang.Timeout
 
-@Retry
+@Retry(condition = { !invocation.method.name.contains('circular redirects') })
+@Timeout(5)
 class GoogleHttpClientTest extends AbstractGoogleHttpClientTest {
+
   @Override
   HttpResponse executeRequest(HttpRequest request) {
     return request.execute()

dd-java-agent/instrumentation/http-url-connection/src/test/groovy/HttpUrlConnectionResponseCodeOnlyTest.groovy

@@ -1,6 +1,8 @@
 import datadog.trace.agent.test.base.HttpClientTest
 import datadog.trace.instrumentation.http_url_connection.HttpUrlConnectionDecorator
+import spock.lang.Timeout
 
+@Timeout(5)
 class HttpUrlConnectionResponseCodeOnlyTest extends HttpClientTest {
 
   @Override
@@ -8,6 +10,8 @@ class HttpUrlConnectionResponseCodeOnlyTest extends HttpClientTest {
     HttpURLConnection connection = uri.toURL().openConnection()
     try {
       connection.setRequestMethod(method)
+      connection.connectTimeout = CONNECT_TIMEOUT_MS
+      connection.readTimeout = READ_TIMEOUT_MS
       headers.each { connection.setRequestProperty(it.key, it.value) }
       connection.setRequestProperty("Connection", "close")
       return connection.getResponseCode()

dd-java-agent/instrumentation/http-url-connection/src/test/groovy/HttpUrlConnectionTest.groovy

@@ -5,6 +5,7 @@ import datadog.trace.bootstrap.instrumentation.api.Tags
 import datadog.trace.instrumentation.http_url_connection.HttpUrlConnectionDecorator
 import spock.lang.Ignore
 import spock.lang.Requires
+import spock.lang.Timeout
 import sun.net.www.protocol.https.HttpsURLConnectionImpl
 
 import static datadog.trace.agent.test.utils.ConfigUtils.withConfigOverride
@@ -12,6 +13,7 @@ import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace
 import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeScope
 import static datadog.trace.instrumentation.http_url_connection.HttpUrlConnectionInstrumentation.HttpUrlState.OPERATION_NAME
 
+@Timeout(5)
 class HttpUrlConnectionTest extends HttpClientTest {
 
   static final RESPONSE = "Hello."
@@ -25,6 +27,8 @@ class HttpUrlConnectionTest extends HttpClientTest {
       headers.each { connection.setRequestProperty(it.key, it.value) }
       connection.setRequestProperty("Connection", "close")
       connection.useCaches = true
+      connection.connectTimeout = CONNECT_TIMEOUT_MS
+      connection.readTimeout = READ_TIMEOUT_MS
       def parentSpan = activeScope()
       def stream = connection.inputStream
       assert activeScope() == parentSpan

dd-java-agent/instrumentation/http-url-connection/src/test/groovy/HttpUrlConnectionUseCachesFalseTest.groovy

@@ -1,8 +1,10 @@
 import datadog.trace.agent.test.base.HttpClientTest
 import datadog.trace.instrumentation.http_url_connection.HttpUrlConnectionDecorator
+import spock.lang.Timeout
 
 import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeScope
 
+@Timeout(5)
 class HttpUrlConnectionUseCachesFalseTest extends HttpClientTest {
 
   @Override
@@ -13,6 +15,8 @@ class HttpUrlConnectionUseCachesFalseTest extends HttpClientTest {
       headers.each { connection.setRequestProperty(it.key, it.value) }
       connection.setRequestProperty("Connection", "close")
       connection.useCaches = false
+      connection.connectTimeout = CONNECT_TIMEOUT_MS
+      connection.readTimeout = READ_TIMEOUT_MS
       def parentSpan = activeScope()
       def stream = connection.inputStream
       assert activeScope() == parentSpan

dd-java-agent/instrumentation/http-url-connection/src/test/groovy/SpringRestTemplateTest.groovy

@@ -4,13 +4,24 @@ import org.springframework.http.HttpEntity
 import org.springframework.http.HttpHeaders
 import org.springframework.http.HttpMethod
 import org.springframework.http.ResponseEntity
+import org.springframework.http.client.ClientHttpRequestFactory
+import org.springframework.http.client.SimpleClientHttpRequestFactory
 import org.springframework.web.client.RestTemplate
 import spock.lang.Shared
+import spock.lang.Timeout
 
+@Timeout(5)
 class SpringRestTemplateTest extends HttpClientTest {
 
   @Shared
-  RestTemplate restTemplate = new RestTemplate()
+  ClientHttpRequestFactory factory = new SimpleClientHttpRequestFactory()
+  @Shared
+  RestTemplate restTemplate = new RestTemplate(factory)
+
+  def setupSpec() {
+    factory.connectTimeout = CONNECT_TIMEOUT_MS
+    factory.readTimeout = READ_TIMEOUT_MS
+  }
 
   @Override
   int doRequest(String method, URI uri, Map<String, String> headers, Closure callback) {
@@ -36,4 +47,10 @@ class SpringRestTemplateTest extends HttpClientTest {
   boolean testConnectionFailure() {
     false
   }
+
+  @Override
+  boolean testRemoteConnection() {
+    // FIXME: exception wrapped in ResourceAccessException
+    return false
+  }
 }

dd-java-agent/instrumentation/jax-rs-client-1.1/src/test/groovy/JaxRsClientV1Test.groovy

@@ -5,13 +5,17 @@ import com.sun.jersey.api.client.filter.LoggingFilter
 import datadog.trace.agent.test.base.HttpClientTest
 import datadog.trace.instrumentation.jaxrs.v1.JaxRsClientV1Decorator
 import spock.lang.Shared
+import spock.lang.Timeout
 
+@Timeout(5)
 class JaxRsClientV1Test extends HttpClientTest {
 
   @Shared
   Client client = Client.create()
 
   def setupSpec() {
+    client.setConnectTimeout(CONNECT_TIMEOUT_MS)
+    client.setReadTimeout(READ_TIMEOUT_MS)
     // Add filters to ensure spans aren't duplicated.
     client.addFilter(new LoggingFilter())
     client.addFilter(new GZIPContentEncodingFilter())

dd-java-agent/instrumentation/jax-rs-client-2.0/jax-rs-client-2.0.gradle

@@ -33,7 +33,8 @@ dependencies {
   testCompile group: 'javax.ws.rs', name: 'javax.ws.rs-api', version: '2.0.1'
 
   testCompile group: 'org.glassfish.jersey.core', name: 'jersey-client', version: '2.0'
-  testCompile group: 'org.jboss.resteasy', name: 'resteasy-client', version: '3.0.0.Final'
+  testCompile group: 'org.jboss.resteasy', name: 'resteasy-client', version: '3.0.5.Final'
+  // ^ This version has timeouts https://issues.redhat.com/browse/RESTEASY-975
   testCompile group: 'org.apache.cxf', name: 'cxf-rt-rs-client', version: '3.1.0'
   // Doesn't work with CXF 3.0.x because their context is wrong:
   // https://github.com/apache/cxf/commit/335c7bad2436f08d6d54180212df5a52157c9f21

dd-java-agent/instrumentation/jax-rs-client-2.0/src/test/groovy/JaxRsClientAsyncTest.groovy

@@ -1,8 +1,11 @@
 import datadog.trace.agent.test.base.HttpClientTest
 import datadog.trace.instrumentation.jaxrs.JaxRsClientDecorator
 import org.apache.cxf.jaxrs.client.spec.ClientBuilderImpl
+import org.glassfish.jersey.client.ClientConfig
+import org.glassfish.jersey.client.ClientProperties
 import org.glassfish.jersey.client.JerseyClientBuilder
 import org.jboss.resteasy.client.jaxrs.ResteasyClientBuilder
+import spock.lang.Timeout
 
 import javax.ws.rs.client.AsyncInvoker
 import javax.ws.rs.client.Client
@@ -12,6 +15,7 @@ import javax.ws.rs.client.InvocationCallback
 import javax.ws.rs.client.WebTarget
 import javax.ws.rs.core.MediaType
 import javax.ws.rs.core.Response
+import java.util.concurrent.TimeUnit
 
 abstract class JaxRsClientAsyncTest extends HttpClientTest {
 
@@ -51,11 +55,15 @@ abstract class JaxRsClientAsyncTest extends HttpClientTest {
   abstract ClientBuilder builder()
 }
 
+@Timeout(5)
 class JerseyClientAsyncTest extends JaxRsClientAsyncTest {
 
   @Override
   ClientBuilder builder() {
-    return new JerseyClientBuilder()
+    ClientConfig config = new ClientConfig()
+    config.property(ClientProperties.CONNECT_TIMEOUT, CONNECT_TIMEOUT_MS)
+    config.property(ClientProperties.READ_TIMEOUT, READ_TIMEOUT_MS)
+    return new JerseyClientBuilder().withConfig(config)
   }
 
   boolean testCircularRedirects() {
@@ -63,11 +71,14 @@ class JerseyClientAsyncTest extends JaxRsClientAsyncTest {
   }
 }
 
+@Timeout(5)
 class ResteasyClientAsyncTest extends JaxRsClientAsyncTest {
 
   @Override
   ClientBuilder builder() {
     return new ResteasyClientBuilder()
+      .establishConnectionTimeout(CONNECT_TIMEOUT_MS, TimeUnit.MILLISECONDS)
+      .socketTimeout(READ_TIMEOUT_MS, TimeUnit.MILLISECONDS)
   }
 
   boolean testRedirects() {
@@ -75,6 +86,7 @@ class ResteasyClientAsyncTest extends JaxRsClientAsyncTest {
   }
 }
 
+@Timeout(5)
 class CxfClientAsyncTest extends JaxRsClientAsyncTest {
 
   @Override
@@ -89,4 +101,9 @@ class CxfClientAsyncTest extends JaxRsClientAsyncTest {
   boolean testConnectionFailure() {
     false
   }
+
+  boolean testRemoteConnection() {
+    // FIXME: span not reported correctly.
+    false
+  }
 }

dd-java-agent/instrumentation/jax-rs-client-2.0/src/test/groovy/JaxRsClientTest.groovy

@@ -1,8 +1,11 @@
 import datadog.trace.agent.test.base.HttpClientTest
 import datadog.trace.instrumentation.jaxrs.JaxRsClientDecorator
 import org.apache.cxf.jaxrs.client.spec.ClientBuilderImpl
+import org.glassfish.jersey.client.ClientConfig
+import org.glassfish.jersey.client.ClientProperties
 import org.glassfish.jersey.client.JerseyClientBuilder
 import org.jboss.resteasy.client.jaxrs.ResteasyClientBuilder
+import spock.lang.Timeout
 
 import javax.ws.rs.client.Client
 import javax.ws.rs.client.ClientBuilder
@@ -11,6 +14,7 @@ import javax.ws.rs.client.Invocation
 import javax.ws.rs.client.WebTarget
 import javax.ws.rs.core.MediaType
 import javax.ws.rs.core.Response
+import java.util.concurrent.TimeUnit
 
 abstract class JaxRsClientTest extends HttpClientTest {
 
@@ -41,11 +45,15 @@ abstract class JaxRsClientTest extends HttpClientTest {
   abstract ClientBuilder builder()
 }
 
+@Timeout(5)
 class JerseyClientTest extends JaxRsClientTest {
 
   @Override
   ClientBuilder builder() {
-    return new JerseyClientBuilder()
+    ClientConfig config = new ClientConfig()
+    config.property(ClientProperties.CONNECT_TIMEOUT, CONNECT_TIMEOUT_MS)
+    config.property(ClientProperties.READ_TIMEOUT, READ_TIMEOUT_MS)
+    return new JerseyClientBuilder().withConfig(config)
   }
 
   boolean testCircularRedirects() {
@@ -53,24 +61,29 @@ class JerseyClientTest extends JaxRsClientTest {
   }
 }
 
+@Timeout(5)
 class ResteasyClientTest extends JaxRsClientTest {
 
   @Override
   ClientBuilder builder() {
     return new ResteasyClientBuilder()
+      .establishConnectionTimeout(CONNECT_TIMEOUT_MS, TimeUnit.MILLISECONDS)
+      .socketTimeout(READ_TIMEOUT_MS, TimeUnit.MILLISECONDS)
   }
 
   boolean testRedirects() {
     false
   }
-
 }
 
+@Timeout(5)
 class CxfClientTest extends JaxRsClientTest {
 
   @Override
   ClientBuilder builder() {
     return new ClientBuilderImpl()
+//      .property(ClientImpl.HTTP_CONNECTION_TIMEOUT_PROP, (long) CONNECT_TIMEOUT_MS)
+//      .property(ClientImpl.HTTP_RECEIVE_TIMEOUT_PROP, (long) READ_TIMEOUT_MS)
   }
 
   boolean testRedirects() {
@@ -80,4 +93,9 @@ class CxfClientTest extends JaxRsClientTest {
   boolean testConnectionFailure() {
     false
   }
+
+  boolean testRemoteConnection() {
+    // FIXME: span not reported correctly.
+    false
+  }
 }

dd-java-agent/instrumentation/netty-4.0/src/test/groovy/Netty40ClientTest.groovy

@@ -7,6 +7,7 @@ import org.asynchttpclient.DefaultAsyncHttpClientConfig
 import org.asynchttpclient.Response
 import spock.lang.AutoCleanup
 import spock.lang.Shared
+import spock.lang.Timeout
 
 import java.util.concurrent.ExecutionException
 import java.util.concurrent.TimeUnit
@@ -16,6 +17,7 @@ import static datadog.trace.agent.test.utils.TraceUtils.basicSpan
 import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace
 import static org.asynchttpclient.Dsl.asyncHttpClient
 
+@Timeout(5)
 class Netty40ClientTest extends HttpClientTest {
 
   @Shared
@@ -60,6 +62,11 @@ class Netty40ClientTest extends HttpClientTest {
     false
   }
 
+  @Override
+  boolean testRemoteConnection() {
+    return false
+  }
+
   def "connection error (unopened port)"() {
     given:
     def uri = new URI("http://localhost:$UNUSABLE_PORT/")

dd-java-agent/instrumentation/netty-4.1/src/test/groovy/Netty41ClientTest.groovy

@@ -17,6 +17,7 @@ import org.asynchttpclient.DefaultAsyncHttpClientConfig
 import org.asynchttpclient.Response
 import spock.lang.Retry
 import spock.lang.Shared
+import spock.lang.Timeout
 
 import java.util.concurrent.ExecutionException
 import java.util.concurrent.TimeUnit
@@ -27,6 +28,7 @@ import static datadog.trace.agent.test.utils.TraceUtils.runUnderTrace
 import static org.asynchttpclient.Dsl.asyncHttpClient
 
 @Retry
+@Timeout(5)
 class Netty41ClientTest extends HttpClientTest {
 
   @Shared
@@ -70,6 +72,11 @@ class Netty41ClientTest extends HttpClientTest {
     false
   }
 
+  @Override
+  boolean testRemoteConnection() {
+    return false
+  }
+
   def "connection error (unopened port)"() {
     given:
     def uri = new URI("http://localhost:$UNUSABLE_PORT/")

dd-java-agent/instrumentation/okhttp-3/src/test/groovy/OkHttp3Test.groovy

@@ -6,10 +6,18 @@ import okhttp3.OkHttpClient
 import okhttp3.Request
 import okhttp3.RequestBody
 import okhttp3.internal.http.HttpMethod
+import spock.lang.Timeout
 
+import java.util.concurrent.TimeUnit
+
+@Timeout(5)
 class OkHttp3Test extends HttpClientTest {
 
-  def client = new OkHttpClient()
+  def client = new OkHttpClient.Builder()
+    .connectTimeout(CONNECT_TIMEOUT_MS, TimeUnit.MILLISECONDS)
+    .readTimeout(READ_TIMEOUT_MS, TimeUnit.MILLISECONDS)
+    .writeTimeout(READ_TIMEOUT_MS, TimeUnit.MILLISECONDS)
+    .build()
 
   @Override
   int doRequest(String method, URI uri, Map<String, String> headers, Closure callback) {

dd-java-agent/instrumentation/play-2.4/src/test/groovy/client/PlayWSClientTest.groovy

@@ -6,9 +6,11 @@ import play.libs.ws.WS
 import spock.lang.AutoCleanup
 import spock.lang.Shared
 import spock.lang.Subject
+import spock.lang.Timeout
 
 // Play 2.6+ uses a separately versioned client that shades the underlying dependency
 // This means our built in instrumentation won't work.
+@Timeout(5)
 class PlayWSClientTest extends HttpClientTest {
   @Subject
   @Shared
@@ -50,4 +52,9 @@ class PlayWSClientTest extends HttpClientTest {
   boolean testConnectionFailure() {
     false
   }
+
+  @Override
+  boolean testRemoteConnection() {
+    return false
+  }
 }

dd-java-agent/instrumentation/play-ws-1/src/test/groovy/PlayWSClientTest.groovy

@@ -8,9 +8,11 @@ import scala.concurrent.ExecutionContext
 import scala.concurrent.Future
 import scala.concurrent.duration.Duration
 import spock.lang.Shared
+import spock.lang.Timeout
 
 import java.util.concurrent.TimeUnit
 
+@Timeout(5)
 class PlayJavaWSClientTest extends PlayWSClientTestBase {
   @Shared
   StandaloneWSClient wsClient
@@ -37,6 +39,7 @@ class PlayJavaWSClientTest extends PlayWSClientTestBase {
   }
 }
 
+@Timeout(5)
 class PlayJavaStreamedWSClientTest extends PlayWSClientTestBase {
   @Shared
   StandaloneWSClient wsClient
@@ -66,6 +69,7 @@ class PlayJavaStreamedWSClientTest extends PlayWSClientTestBase {
   }
 }
 
+@Timeout(5)
 class PlayScalaWSClientTest extends PlayWSClientTestBase {
   @Shared
   play.api.libs.ws.StandaloneWSClient wsClient
@@ -96,6 +100,7 @@ class PlayScalaWSClientTest extends PlayWSClientTestBase {
   }
 }
 
+@Timeout(5)
 class PlayScalaStreamedWSClientTest extends PlayWSClientTestBase {
   @Shared
   play.api.libs.ws.StandaloneWSClient wsClient

dd-java-agent/instrumentation/ratpack-1.4/src/test/groovy/client/RatpackHttpClientTest.groovy

@@ -7,7 +7,11 @@ import ratpack.http.client.HttpClient
 import ratpack.test.exec.ExecHarness
 import spock.lang.AutoCleanup
 import spock.lang.Shared
+import spock.lang.Timeout
 
+import java.time.Duration
+
+@Timeout(5)
 class RatpackHttpClientTest extends HttpClientTest {
 
   @AutoCleanup
@@ -15,12 +19,16 @@ class RatpackHttpClientTest extends HttpClientTest {
   ExecHarness exec = ExecHarness.harness()
 
   @Shared
-  def client = HttpClient.of {}
+  def client = HttpClient.of {
+    it.readTimeout(Duration.ofSeconds(2))
+    // Connect timeout added in 1.5
+  }
 
   @Override
   int doRequest(String method, URI uri, Map<String, String> headers, Closure callback) {
     ExecResult<Integer> result = exec.yield {
       def resp = client.request(uri) { spec ->
+        spec.connectTimeout(Duration.ofSeconds(2))
         spec.method(method)
         spec.headers { headersSpec ->
           headers.entrySet().each {
@@ -55,4 +63,9 @@ class RatpackHttpClientTest extends HttpClientTest {
   boolean testConnectionFailure() {
     false
   }
+
+  @Override
+  boolean testRemoteConnection() {
+    return false
+  }
 }

dd-java-agent/instrumentation/spring-webflux-5/src/test/groovy/dd/trace/instrumentation/springwebflux/client/SpringWebfluxHttpClientTest.groovy

@@ -11,9 +11,11 @@ import org.springframework.http.HttpMethod
 import org.springframework.web.reactive.function.client.ClientResponse
 import org.springframework.web.reactive.function.client.WebClient
 import spock.lang.Shared
+import spock.lang.Timeout
 
 import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan
 
+@Timeout(5)
 class SpringWebfluxHttpClientTest extends HttpClientTest {
 
   @Shared
@@ -92,4 +94,9 @@ class SpringWebfluxHttpClientTest extends HttpClientTest {
   boolean testConnectionFailure() {
     false
   }
+
+  boolean testRemoteConnection() {
+    // FIXME: figure out how to configure timeouts.
+    false
+  }
 }

dd-java-agent/instrumentation/vertx/src/test/groovy/client/VertxHttpClientTest.groovy

@@ -4,7 +4,7 @@ import datadog.trace.agent.test.base.HttpClientTest
 import datadog.trace.instrumentation.netty41.client.NettyHttpClientDecorator
 import io.vertx.core.Vertx
 import io.vertx.core.VertxOptions
-import io.vertx.core.http.HttpClient
+import io.vertx.core.http.HttpClientOptions
 import io.vertx.core.http.HttpClientResponse
 import io.vertx.core.http.HttpMethod
 import spock.lang.Shared
@@ -16,9 +16,11 @@ import java.util.concurrent.CompletableFuture
 class VertxHttpClientTest extends HttpClientTest {
 
   @Shared
-  Vertx vertx = Vertx.vertx(new VertxOptions())
+  def vertx = Vertx.vertx(new VertxOptions())
   @Shared
-  HttpClient httpClient = vertx.createHttpClient()
+  def clientOptions = new HttpClientOptions().setConnectTimeout(CONNECT_TIMEOUT_MS).setIdleTimeout(READ_TIMEOUT_MS)
+  @Shared
+  def httpClient = vertx.createHttpClient(clientOptions)
 
   @Override
   int doRequest(String method, URI uri, Map<String, String> headers, Closure callback) {
@@ -53,4 +55,9 @@ class VertxHttpClientTest extends HttpClientTest {
   boolean testConnectionFailure() {
     false
   }
+
+  boolean testRemoteConnection() {
+    // FIXME: figure out how to configure timeouts.
+    false
+  }
 }

dd-java-agent/instrumentation/vertx/src/test/groovy/client/VertxRxCircuitBreakerWebClientTest.groovy

@@ -5,6 +5,7 @@ import datadog.trace.instrumentation.netty41.client.NettyHttpClientDecorator
 import io.vertx.circuitbreaker.CircuitBreakerOptions
 import io.vertx.core.VertxOptions
 import io.vertx.core.http.HttpMethod
+import io.vertx.ext.web.client.WebClientOptions
 import io.vertx.reactivex.circuitbreaker.CircuitBreaker
 import io.vertx.reactivex.core.Vertx
 import io.vertx.reactivex.ext.web.client.WebClient
@@ -19,7 +20,9 @@ class VertxRxCircuitBreakerWebClientTest extends HttpClientTest {
   @Shared
   Vertx vertx = Vertx.vertx(new VertxOptions())
   @Shared
-  WebClient client = WebClient.create(vertx)
+  def clientOptions = new WebClientOptions().setConnectTimeout(CONNECT_TIMEOUT_MS).setIdleTimeout(READ_TIMEOUT_MS)
+  @Shared
+  WebClient client = WebClient.create(vertx, clientOptions)
   @Shared
   CircuitBreaker breaker = CircuitBreaker.create("my-circuit-breaker", vertx,
     new CircuitBreakerOptions()
@@ -69,4 +72,9 @@ class VertxRxCircuitBreakerWebClientTest extends HttpClientTest {
   boolean testConnectionFailure() {
     false
   }
+
+  boolean testRemoteConnection() {
+    // FIXME: figure out how to configure timeouts.
+    false
+  }
 }

dd-java-agent/instrumentation/vertx/src/test/groovy/client/VertxRxWebClientTest.groovy

@@ -4,6 +4,7 @@ import datadog.trace.agent.test.base.HttpClientTest
 import datadog.trace.instrumentation.netty41.client.NettyHttpClientDecorator
 import io.vertx.core.VertxOptions
 import io.vertx.core.http.HttpMethod
+import io.vertx.ext.web.client.WebClientOptions
 import io.vertx.reactivex.core.Vertx
 import io.vertx.reactivex.ext.web.client.WebClient
 import spock.lang.Shared
@@ -15,7 +16,9 @@ class VertxRxWebClientTest extends HttpClientTest {
   @Shared
   Vertx vertx = Vertx.vertx(new VertxOptions())
   @Shared
-  WebClient client = WebClient.create(vertx)
+  def clientOptions = new WebClientOptions().setConnectTimeout(CONNECT_TIMEOUT_MS).setIdleTimeout(READ_TIMEOUT_MS)
+  @Shared
+  WebClient client = WebClient.create(vertx, clientOptions)
 
   @Override
   int doRequest(String method, URI uri, Map<String, String> headers, Closure callback) {
@@ -48,4 +51,9 @@ class VertxRxWebClientTest extends HttpClientTest {
   boolean testConnectionFailure() {
     false
   }
+
+  boolean testRemoteConnection() {
+    // FIXME: figure out how to configure timeouts.
+    false
+  }
 }

dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/base/HttpClientTest.groovy

@@ -8,6 +8,7 @@ import datadog.trace.api.DDSpanTypes
 import datadog.trace.api.DDTags
 import datadog.trace.bootstrap.instrumentation.api.Tags
 import spock.lang.AutoCleanup
+import spock.lang.Requires
 import spock.lang.Shared
 import spock.lang.Unroll
 
@@ -23,6 +24,8 @@ import static org.junit.Assume.assumeTrue
 @Unroll
 abstract class HttpClientTest extends AgentTestRunner {
   protected static final BODY_METHODS = ["POST", "PUT"]
+  protected static final CONNECT_TIMEOUT_MS = 1000
+  protected static final READ_TIMEOUT_MS = 2000
 
   @AutoCleanup
   @Shared
@@ -109,7 +112,7 @@ abstract class HttpClientTest extends AgentTestRunner {
       server.distributedRequestTrace(it, 0, trace(1).last())
       trace(1, size(2)) {
         basicSpan(it, 0, "parent")
-        clientSpan(it, 1, span(0), method, false)
+        clientSpan(it, 1, span(0), method)
       }
     }
 
@@ -179,7 +182,7 @@ abstract class HttpClientTest extends AgentTestRunner {
       trace(0, size(3)) {
         basicSpan(it, 0, "parent")
         basicSpan(it, 1, "child", span(0))
-        clientSpan(it, 2, span(0), method, false)
+        clientSpan(it, 2, span(0), method)
       }
     }
 
@@ -201,7 +204,7 @@ abstract class HttpClientTest extends AgentTestRunner {
     // only one trace (client).
     assertTraces(2) {
       trace(0, size(1)) {
-        clientSpan(it, 0, null, method, false)
+        clientSpan(it, 0, null, method)
       }
       trace(1, 1) {
         basicSpan(it, 0, "callback")
@@ -262,8 +265,7 @@ abstract class HttpClientTest extends AgentTestRunner {
 
   def "basic #method request with circular redirects"() {
     given:
-    assumeTrue(testRedirects())
+    assumeTrue(testRedirects() && testCircularRedirects())
-    assumeTrue(testCircularRedirects())
     def uri = server.address.resolve("/circular-redirect")
 
     when:
@@ -312,6 +314,77 @@ abstract class HttpClientTest extends AgentTestRunner {
     method = "GET"
   }
 
+  def "connection error dropped request"() {
+    given:
+    assumeTrue(testRemoteConnection())
+    // https://stackoverflow.com/a/100859
+    def uri = new URI("http://www.google.com:81/")
+
+    when:
+    runUnderTrace("parent") {
+      doRequest(method, uri)
+    }
+
+    then:
+    def ex = thrown(Exception)
+    def thrownException = ex instanceof ExecutionException ? ex.cause : ex
+    assertTraces(1) {
+      trace(0, size(2)) {
+        basicSpan(it, 0, "parent", null, thrownException)
+        clientSpan(it, 1, span(0), method, false, false, uri, null, thrownException)
+      }
+    }
+
+    where:
+    method = "HEAD"
+  }
+
+  def "connection error non routable address"() {
+    given:
+    assumeTrue(testRemoteConnection())
+    def uri = new URI("https://192.0.2.1/")
+
+    when:
+    runUnderTrace("parent") {
+      doRequest(method, uri)
+    }
+
+    then:
+    def ex = thrown(Exception)
+    def thrownException = ex instanceof ExecutionException ? ex.cause : ex
+    assertTraces(1) {
+      trace(0, size(2)) {
+        basicSpan(it, 0, "parent", null, thrownException)
+        clientSpan(it, 1, span(0), method, false, false, uri, null, thrownException)
+      }
+    }
+
+    where:
+    method = "HEAD"
+  }
+
+  // IBM JVM has different protocol support for TLS
+  @Requires({ !System.getProperty("java.vm.name").contains("IBM J9 VM") })
+  def "test https request"() {
+    given:
+    assumeTrue(testRemoteConnection())
+    def uri = new URI("https://www.google.com/")
+
+    when:
+    def status = doRequest(method, uri)
+
+    then:
+    status == 200
+    assertTraces(1) {
+      trace(0, size(1)) {
+        clientSpan(it, 0, null, method, false, false, uri)
+      }
+    }
+
+    where:
+    method = "HEAD"
+  }
+
   // parent span must be cast otherwise it breaks debugging classloading (junit loads it early)
   void clientSpan(TraceAssert trace, int index, Object parentSpan, String method = "GET", boolean renameService = false, boolean tagQueryString = false, URI uri = server.address.resolve("/success"), Integer status = 200, Throwable exception = null) {
     trace.span(index) {
@@ -320,7 +393,7 @@ abstract class HttpClientTest extends AgentTestRunner {
       } else {
         childOf((DDSpan) parentSpan)
       }
-      serviceName renameService ? "localhost" : "unnamed-java-app"
+      serviceName renameService ? uri.host : "unnamed-java-app"
       operationName expectedOperationName()
       resourceName "$method $uri.path"
       spanType DDSpanTypes.HTTP_CLIENT
@@ -328,9 +401,9 @@ abstract class HttpClientTest extends AgentTestRunner {
       tags {
         "$Tags.COMPONENT" component
         "$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT
-        "$Tags.PEER_HOSTNAME" "localhost"
+        "$Tags.PEER_HOSTNAME" uri.host
         "$Tags.PEER_HOST_IPV4" { it == null || it == "127.0.0.1" } // Optional
-        "$Tags.PEER_PORT" uri.port
+        "$Tags.PEER_PORT" uri.port > 0 ? uri.port : { it == null || it == 443 } // Optional
         "$Tags.HTTP_URL" "${uri.resolve(uri.path)}"
         "$Tags.HTTP_METHOD" method
         if (status) {
@@ -368,6 +441,10 @@ abstract class HttpClientTest extends AgentTestRunner {
     true
   }
 
+  boolean testRemoteConnection() {
+    true
+  }
+
   boolean testCallbackWithParent() {
     // FIXME: this hack is here because callback with parent is broken in play-ws when the stream()
     // function is used.  There is no way to stop a test from a derived class hence the flag

dd-java-agent/testing/src/main/groovy/datadog/trace/agent/test/server/http/TestHttpServer.groovy

@@ -22,17 +22,14 @@ import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.startSpan
 
 class TestHttpServer implements AutoCloseable {
 
-  static TestHttpServer httpServer(boolean start = true,
+  static TestHttpServer httpServer(@DelegatesTo(value = TestHttpServer, strategy = Closure.DELEGATE_FIRST) Closure spec) {
-                                   @DelegatesTo(value = TestHttpServer, strategy = Closure.DELEGATE_FIRST) Closure spec) {
 
     def server = new TestHttpServer()
     def clone = (Closure) spec.clone()
     clone.delegate = server
     clone.resolveStrategy = Closure.DELEGATE_FIRST
     clone(server)
-    if (start) {
     server.start()
-    }
     return server
   }
 

dd-java-agent/testing/src/test/groovy/server/ServerTest.groovy

@@ -18,12 +18,12 @@ class ServerTest extends AgentTestRunner {
 
   def "test server lifecycle"() {
     setup:
-    def server = httpServer(startAutomatically) {
+    def server = httpServer {
       handlers {}
     }
 
     expect:
-    server.internalServer.isRunning() == startAutomatically
+    server.internalServer.isRunning()
 
     when:
     server.start()
@@ -44,16 +44,13 @@ class ServerTest extends AgentTestRunner {
     server.internalServer.isRunning()
 
     when:
-    server.stop()
+    server.close()
 
     then:
     !server.internalServer.isRunning()
 
     cleanup:
-    server.stop()
+    server.close()
-
-    where:
-    startAutomatically << [true, false]
   }
 
   def "server 404's with no handlers"() {