opentracing: don't use net/http.DefaultClient|DefaultTransport about dd-trace-go HOT 7 CLOSED

I've not really dug deeper into the problem, but if the cause is indeed (as it seems to be) the fact that our transport uses http.DefaultTransport, then this might be your fix:

diff --git a/tracer/transport.go b/tracer/transport.go
index e526ad5..2c23656 100644
--- a/tracer/transport.go
+++ b/tracer/transport.go
@@ -74,6 +74,9 @@ func newHTTPTransport(hostname, port string) *httpTransport {
 		"Datadog-Meta-Lang-Interpreter": ext.Interpreter,
 		"Datadog-Meta-Tracer-Version":   ext.TracerVersion,
 	}
+	// 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.
+	transport := *(http.DefaultTransport.(*http.Transport)) // copy
 
 	return &httpTransport{
 		traceURL:         fmt.Sprintf("http://%s:%s/v0.3/traces", hostname, port),
@@ -82,7 +85,8 @@ func newHTTPTransport(hostname, port string) *httpTransport {
 		legacyServiceURL: fmt.Sprintf("http://%s:%s/v0.2/services", hostname, port),
 		getEncoder:       msgpackEncoderFactory,
 		client: &http.Client{
-			Timeout: defaultHTTPTimeout,
+			Transport: &transport,
+			Timeout:   defaultHTTPTimeout,
 		},
 		headers:           defaultHeaders,
 		compatibilityMode: false,

I hope this helps. A PR would be much appreciated, if this fixes the problem.

As a side note, it might come more natural to your application to create it's own "traced transport" as opposed to using the default one.

from dd-trace-go.

I see 2 majors issues in this code

transport := *(http.DefaultTransport.(*http.Transport))

• http.DefaultTransport is a http.RoundTripper, not always a http.Transport (the code will panic)
• it could be a http.Transport, with values different from the default ones (which could change the behavior of the DataDog tracer)

I suggest to copy/paste the declaration:

&Transport{
        Proxy: ProxyFromEnvironment,
        DialContext: (&net.Dialer{
                Timeout:   30 * time.Second,
                KeepAlive: 30 * time.Second,
                DualStack: true,
        }).DialContext,
        MaxIdleConns:          100,
        IdleConnTimeout:       90 * time.Second,
        TLSHandshakeTimeout:   10 * time.Second,
        ExpectContinueTimeout: 1 * time.Second,
}

I think it will "fix" my problem.
Do you want a PR for this ?

As a side note, it might come more natural to your application to create it's own "traced transport" as opposed to using the default one.

I'm not sure to understand.
In my code I do:

tr, closeTR, err := ddtrace_opentracing.NewTracer(cfg)
///
opentracing.SetGlobalTracer(tr)

Should I do differently ?

from dd-trace-go.

http.DefaultTransport is a http.RoundTripper, not always a http.Transport

Nice catch! But can you please enlighten me as to when it is not an http.Transport ?

I suggest to copy/paste the declaration

Sure, I am happy with this alternative (given that the above question can be answered).

Do you want a PR for this ?

You are welcome to open a PR if you have tried the suggested solution and if it fixes your problem. I think this is definitely something we want to have.

Should I do differently ?

Sorry, I might have not been clear enough. I just meant that as you have said yourself, it might be best for you to wrap and use a custom transport instead of wrapping the default one. Nevertheless, our code should account for both situations. There is nothing you need to change. You are doing everything right.

Thanks for reporting :-)

from dd-trace-go.

http.DefaultTransport is a http.RoundTripper, not always a http.Transport

Nice catch! But can you please enlighten me as to when it is not an http.Transport ?

http.DefaultTransport is a global variable with type http.RoundTripper.
It means you can change it to any variable with a type that implements http.RoundTripper.
See #172
I implemented my own http.RoundTripper, and of course it's not the same type as http.Transport.

You are welcome to open a PR if you have tried the suggested solution and if it fixes your problem. I think this is definitely something we want to have.

I'll test this solution, and let you know.

from dd-trace-go.

It means you can change it to any variable with a type that implements http.RoundTripper.

Ah yes of course, doooh. Sorry about that. Copying is of course fine in that case.

from dd-trace-go.

It worked, everything is fixed.

My change:

diff --git a/vendor/github.com/DataDog/dd-trace-go/tracer/transport.go b/vendor/github.com/DataDog/dd-trace-go/tracer/transport.go
index e526ad5..ac6da7e 100644
--- a/vendor/github.com/DataDog/dd-trace-go/tracer/transport.go
+++ b/vendor/github.com/DataDog/dd-trace-go/tracer/transport.go
@@ -4,6 +4,7 @@ import (
        "errors"
        "fmt"
        "log"
+       "net"
        "net/http"
        "strconv"
        "time"
@@ -82,6 +83,18 @@ func newHTTPTransport(hostname, port string) *httpTransport {
                legacyServiceURL: fmt.Sprintf("http://%s:%s/v0.2/services", hostname, port),
                getEncoder:       msgpackEncoderFactory,
                client: &http.Client{
+                       Transport: &http.Transport{
+                               Proxy: http.ProxyFromEnvironment,
+                               DialContext: (&net.Dialer{
+                                       Timeout:   30 * time.Second,
+                                       KeepAlive: 30 * time.Second,
+                                       DualStack: true,
+                               }).DialContext,
+                               MaxIdleConns:          100,
+                               IdleConnTimeout:       90 * time.Second,
+                               TLSHandshakeTimeout:   10 * time.Second,
+                               ExpectContinueTimeout: 1 * time.Second,
+                       },
                        Timeout: defaultHTTPTimeout,
                },
                headers:           defaultHeaders,

Is it still OK for a PR ?
Maybe the library should also allow to provide a custom http.RoundTripper/Client.

About the last part in my first message
#180 (comment)
I don't really understand how the DataDog web interfaces decides which "root trace/span" is displayed.
Previously there were 2 "root trace/span" in my code:

• http.request_out
• amqp.consume

The web interface displayed http.request_out only.

After removing http.request_out (and waiting ~15 minutes), amqp.consume was displayed.
This is not very important, because the issue is now fixed, but I'd like to understand the cause.

from dd-trace-go.

The PR #183

from dd-trace-go.