ddtrace/tracer: add WithHTTPClient and increase default HTTP timeout

This change adds a new option `WithHTTPClient` and marks
`WithHTTPRoundTripper` as deprecated (now superseded). Additionally, the
default client timeout is increased to 2 seconds as it has proven to be
too short (see #181).

Closes #181

ddtrace/tracer/option.go

@@ -50,8 +50,8 @@ type config struct {
 	// propagator propagates span context cross-process
 	propagator Propagator
 
-	// httpRoundTripper defines the http.RoundTripper used by the agent transport.
-	httpRoundTripper http.RoundTripper
+	// httpClient specifies the HTTP client to be used by the agent's transport.
+	httpClient *http.Client
 
 	// hostname is automatically assigned when the DD_TRACE_REPORT_HOSTNAME is set to true,
 	// and is added as a special tag to the root span of traces.
@@ -243,12 +243,18 @@ func WithSampler(s Sampler) StartOption {
 	}
 }
 
-// WithHTTPRoundTripper allows customizing the underlying HTTP transport for
-// emitting spans. This is useful for advanced customization such as emitting
-// spans to a unix domain socket. The default should be used in most cases.
+// WithHTTPRoundTripper is deprecated. Please consider using WithHTTPClient instead.
 func WithHTTPRoundTripper(r http.RoundTripper) StartOption {
+	return WithHTTPClient(&http.Client{
+		Transport: r,
+		Timeout:   defaultHTTPTimeout,
+	})
+}
+
+// WithHTTPClient specifies the HTTP client to use when emitting spans to the agent.
+func WithHTTPClient(client *http.Client) StartOption {
 	return func(c *config) {
-		c.httpRoundTripper = r
+		c.httpClient = client
 	}
 }
 

ddtrace/tracer/option_test.go

@@ -38,7 +38,7 @@ func TestTracerOptionsDefaults(t *testing.T) {
 		assert.Equal("tracer.test", c.serviceName)
 		assert.Equal("localhost:8126", c.agentAddr)
 		assert.Equal("localhost:8125", c.dogstatsdAddr)
-		assert.Equal(nil, c.httpRoundTripper)
+		assert.Nil(nil, c.httpClient)
 	})
 
 	t.Run("analytics", func(t *testing.T) {

ddtrace/tracer/tracer.go

@@ -137,7 +137,7 @@ func newUnstartedTracer(opts ...StartOption) *tracer {
 		fn(c)
 	}
 	if c.transport == nil {
-		c.transport = newTransport(c.agentAddr, c.httpRoundTripper)
+		c.transport = newTransport(c.agentAddr, c.httpClient)
 	}
 	if c.propagator == nil {
 		c.propagator = NewPropagator(nil)

ddtrace/tracer/tracer_test.go

@@ -499,7 +499,7 @@ func TestTracerPrioritySampler(t *testing.T) {
 	addr := srv.Listener.Addr().String()
 
 	tr, _, flush, stop := startTestTracer(t,
-		withTransport(newHTTPTransport(addr, defaultRoundTripper)),
+		withTransport(newHTTPTransport(addr, defaultClient)),
 	)
 	defer stop()
 

ddtrace/tracer/transport.go

@@ -21,11 +21,11 @@ import (
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/version"
 )
 
-var (
+var defaultClient = &http.Client{
 	// We copy the transport to avoid using the default one, as it might be
 	// augmented with tracing and we don't want these calls to be recorded.
 	// See https://golang.org/pkg/net/http/#DefaultTransport .
-	defaultRoundTripper = &http.Transport{
+	Transport: &http.Transport{
 		Proxy: http.ProxyFromEnvironment,
 		DialContext: (&net.Dialer{
 			Timeout:   30 * time.Second,
@@ -36,14 +36,15 @@ var (
 		IdleConnTimeout:       90 * time.Second,
 		TLSHandshakeTimeout:   10 * time.Second,
 		ExpectContinueTimeout: 1 * time.Second,
-	}
-)
+	},
+	Timeout: defaultHTTPTimeout,
+}
 
 const (
 	defaultHostname    = "localhost"
 	defaultPort        = "8126"
 	defaultAddress     = defaultHostname + ":" + defaultPort
-	defaultHTTPTimeout = time.Second             // defines the current timeout before giving up with the send process
+	defaultHTTPTimeout = 2 * time.Second         // defines the current timeout before giving up with the send process
 	traceCountHeader   = "X-Datadog-Trace-Count" // header containing the number of traces in the payload
 )
 
@@ -64,16 +65,16 @@ type transport interface {
 // running on a non-default port, if it's located on another machine, or when
 // otherwise needing to customize the transport layer, for instance when using
 // a unix domain socket.
-func newTransport(addr string, roundTripper http.RoundTripper) transport {
-	if roundTripper == nil {
-		roundTripper = defaultRoundTripper
+func newTransport(addr string, client *http.Client) transport {
+	if client == nil {
+		client = defaultClient
 	}
-	return newHTTPTransport(addr, roundTripper)
+	return newHTTPTransport(addr, client)
 }
 
 // newDefaultTransport return a default transport for this tracing client
 func newDefaultTransport() transport {
-	return newHTTPTransport(defaultAddress, defaultRoundTripper)
+	return newHTTPTransport(defaultAddress, defaultClient)
 }
 
 type httpTransport struct {
@@ -83,7 +84,7 @@ type httpTransport struct {
 }
 
 // newHTTPTransport returns an httpTransport for the given endpoint
-func newHTTPTransport(addr string, roundTripper http.RoundTripper) *httpTransport {
+func newHTTPTransport(addr string, client *http.Client) *httpTransport {
 	// initialize the default EncoderPool with Encoder headers
 	defaultHeaders := map[string]string{
 		"Datadog-Meta-Lang":             "go",
@@ -101,11 +102,8 @@ func newHTTPTransport(addr string, roundTripper http.RoundTripper) *httpTranspor
 	}
 	return &httpTransport{
 		traceURL: fmt.Sprintf("http://%s/v0.4/traces", resolveAddr(addr)),
-		client: &http.Client{
-			Transport: roundTripper,
-			Timeout:   defaultHTTPTimeout,
-		},
-		headers: defaultHeaders,
+		client:   client,
+		headers:  defaultHeaders,
 	}
 }
 

ddtrace/tracer/transport_test.go

@@ -127,7 +127,7 @@ func TestTracesAgentIntegration(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		transport := newHTTPTransport(defaultAddress, defaultRoundTripper)
+		transport := newHTTPTransport(defaultAddress, defaultClient)
 		p, err := encode(tc.payload)
 		assert.NoError(err)
 		_, err = transport.send(p)
@@ -196,7 +196,7 @@ func TestTransportResponse(t *testing.T) {
 			}))
 			defer ln.Close()
 			addr := ln.Addr().String()
-			transport := newHTTPTransport(addr, defaultRoundTripper)
+			transport := newHTTPTransport(addr, defaultClient)
 			rc, err := transport.send(newPayload())
 			if tt.err != "" {
 				assert.Equal(tt.err, err.Error())
@@ -230,7 +230,7 @@ func TestTraceCountHeader(t *testing.T) {
 	assert.Nil(err)
 	assert.NotEmpty(port, "port should be given, as it's chosen randomly")
 	for _, tc := range testCases {
-		transport := newHTTPTransport(host, defaultRoundTripper)
+		transport := newHTTPTransport(host, defaultClient)
 		p, err := encode(tc.payload)
 		assert.NoError(err)
 		_, err = transport.send(p)
@@ -246,7 +246,7 @@ type recordingRoundTripper struct {
 
 func (r *recordingRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
 	r.reqs = append(r.reqs, req)
-	return defaultRoundTripper.RoundTrip(req)
+	return defaultClient.Transport.RoundTrip(req)
 }
 
 func TestCustomTransport(t *testing.T) {
@@ -263,7 +263,7 @@ func TestCustomTransport(t *testing.T) {
 	assert.NotEmpty(port, "port should be given, as it's chosen randomly")
 
 	customRoundTripper := new(recordingRoundTripper)
-	transport := newHTTPTransport(host, customRoundTripper)
+	transport := newHTTPTransport(host, &http.Client{Transport: customRoundTripper})
 	p, err := encode(getTestTrace(1, 1))
 	assert.NoError(err)
 	_, err = transport.send(p)
@@ -273,6 +273,24 @@ func TestCustomTransport(t *testing.T) {
 	assert.Len(customRoundTripper.reqs, 1)
 }
 
+func TestWithHTTPClient(t *testing.T) {
+	assert := assert.New(t)
+	srv := mockDatadogAPINewServer(t)
+	defer srv.Close()
+
+	u, err := url.Parse(srv.URL)
+	assert.NoError(err)
+	rt := new(recordingRoundTripper)
+	trc := newTracer(WithAgentAddr(u.Host), WithHTTPClient(&http.Client{Transport: rt}))
+	defer trc.Stop()
+
+	p, err := encode(getTestTrace(1, 1))
+	assert.NoError(err)
+	_, err = trc.config.transport.send(p)
+	assert.NoError(err)
+	assert.Len(rt.reqs, 1)
+}
+
 // TestTransportHTTPRace defines a regression tests where the request body was being
 // read even after http.Client.Do returns. See golang/go#33244
 func TestTransportHTTPRace(t *testing.T) {