ddtrace/tracer,profiler: share agent default URL resolution logic (#2719

)

Move the agent default URL resolution logic to internal so it can be shared
by the tracer and profiler.

This is motivated by the profiler failing to connect to the default trace agent
Unix domain socket in some situations. That gets picked up as a benefit of
sharing more logic, so this PR adds a test for that functionality.

The ddtrace/tracer library has code to resolve the agent URL from the program
environment, including checking for the default Unix domain socket. Right now
the profiler doesn't check for the default Unix domain socket and so will fail
to send profiles to the right place in some circumstances. The tracer and
profiler should ideally share all this logic.

ddtrace/tracer/option.go

@@ -100,10 +100,6 @@ var contribIntegrations = map[string]struct {
 }
 
 var (
-	// defaultSocketAPM specifies the socket path to use for connecting to the trace-agent.
-	// Replaced in tests
-	defaultSocketAPM = "/var/run/datadog/apm.socket"
-
 	// defaultSocketDSD specifies the socket path to use for connecting to the statsd server.
 	// Replaced in tests
 	defaultSocketDSD = "/var/run/datadog/dsd.socket"
@@ -437,10 +433,7 @@ func newConfig(opts ...StartOption) *config {
 		fn(c)
 	}
 	if c.agentURL == nil {
-		c.agentURL = resolveAgentAddr()
-		if url := internal.AgentURLFromEnv(); url != nil {
-			c.agentURL = url
-		}
+		c.agentURL = internal.AgentURLFromEnv()
 	}
 	if c.agentURL.Scheme == "unix" {
 		// If we're connecting over UDS we can just rely on the agent to provide the hostname

ddtrace/tracer/option_test.go

@@ -27,6 +27,7 @@ import (
 
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
+	"gopkg.in/DataDog/dd-trace-go.v1/internal"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/globalconfig"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/namingschema"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/telemetry"
@@ -827,9 +828,9 @@ func TestTracerOptionsDefaults(t *testing.T) {
 
 func TestDefaultHTTPClient(t *testing.T) {
 	defTracerClient := func(timeout int) *http.Client {
-		if _, err := os.Stat(defaultSocketAPM); err == nil {
+		if _, err := os.Stat(internal.DefaultTraceAgentUDSPath); err == nil {
 			// we have the UDS socket file, use it
-			return udsClient(defaultSocketAPM, 0)
+			return udsClient(internal.DefaultTraceAgentUDSPath, 0)
 		}
 		return defaultHTTPClient(time.Second * time.Duration(timeout))
 	}
@@ -853,8 +854,8 @@ func TestDefaultHTTPClient(t *testing.T) {
 			t.Fatal(err)
 		}
 		defer os.RemoveAll(f.Name())
-		defer func(old string) { defaultSocketAPM = old }(defaultSocketAPM)
-		defaultSocketAPM = f.Name()
+		defer func(old string) { internal.DefaultTraceAgentUDSPath = old }(internal.DefaultTraceAgentUDSPath)
+		internal.DefaultTraceAgentUDSPath = f.Name()
 		x := *defTracerClient(2)
 		y := *defaultHTTPClient(2)
 		compareHTTPClients(t, x, y)

ddtrace/tracer/transport.go

@@ -11,8 +11,6 @@ import (
 	"io"
 	"net"
 	"net/http"
-	"net/url"
-	"os"
 	"runtime"
 	"strconv"
 	"strings"
@@ -188,32 +186,3 @@ func (t *httpTransport) send(p *payload) (body io.ReadCloser, err error) {
 func (t *httpTransport) endpoint() string {
 	return t.traceURL
 }
-
-// resolveAgentAddr resolves the given agent address and fills in any missing host
-// and port using the defaults. Some environment variable settings will
-// take precedence over configuration.
-func resolveAgentAddr() *url.URL {
-	var host, port string
-	if v := os.Getenv("DD_AGENT_HOST"); v != "" {
-		host = v
-	}
-	if v := os.Getenv("DD_TRACE_AGENT_PORT"); v != "" {
-		port = v
-	}
-	if _, err := os.Stat(defaultSocketAPM); host == "" && port == "" && err == nil {
-		return &url.URL{
-			Scheme: "unix",
-			Path:   defaultSocketAPM,
-		}
-	}
-	if host == "" {
-		host = defaultHostname
-	}
-	if port == "" {
-		port = defaultPort
-	}
-	return &url.URL{
-		Scheme: "http",
-		Host:   fmt.Sprintf("%s:%s", host, port),
-	}
-}

ddtrace/tracer/transport_test.go

@@ -20,6 +20,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gopkg.in/DataDog/dd-trace-go.v1/internal"
 )
 
 // getTestSpan returns a Span with different fields set
@@ -102,7 +103,7 @@ func TestResolveAgentAddr(t *testing.T) {
 			if tt.envPort != "" {
 				t.Setenv("DD_TRACE_AGENT_PORT", tt.envPort)
 			}
-			c.agentURL = resolveAgentAddr()
+			c.agentURL = internal.AgentURLFromEnv()
 			if tt.inOpt != nil {
 				tt.inOpt(c)
 			}
@@ -111,12 +112,12 @@ func TestResolveAgentAddr(t *testing.T) {
 	}
 
 	t.Run("UDS", func(t *testing.T) {
-		old := defaultSocketAPM
+		old := internal.DefaultTraceAgentUDSPath
 		d, err := os.Getwd()
 		require.NoError(t, err)
-		defaultSocketAPM = d // Choose a file we know will exist
-		defer func() { defaultSocketAPM = old }()
-		c.agentURL = resolveAgentAddr()
+		internal.DefaultTraceAgentUDSPath = d // Choose a file we know will exist
+		defer func() { internal.DefaultTraceAgentUDSPath = old }()
+		c.agentURL = internal.AgentURLFromEnv()
 		assert.Equal(t, &url.URL{Scheme: "unix", Path: d}, c.agentURL)
 	})
 }

internal/agent.go

@@ -6,31 +6,70 @@
 package internal
 
 import (
+	"net"
 	"net/url"
 	"os"
 
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/log"
 )
 
-// AgentURLFromEnv determines the trace agent URL from environment variable
-// DD_TRACE_AGENT_URL. If the determined value is valid and the scheme is
-// supported (unix, http or https), it will return an *url.URL. Otherwise,
-// it returns nil.
+const (
+	DefaultAgentHostname  = "localhost"
+	DefaultTraceAgentPort = "8126"
+)
+
+// This is a variable rather than a constant so it can be replaced in unit tests
+var DefaultTraceAgentUDSPath = "/var/run/datadog/apm.socket"
+
+// AgentURLFromEnv resolves the URL for the trace agent based on
+// the default host/port and UDS path, and via standard environment variables.
+// AgentURLFromEnv has the following priority order:
+//   - First, DD_TRACE_AGENT_URL if it is set
+//   - Then, if either of DD_AGENT_HOST and DD_TRACE_AGENT_PORT are set,
+//     use http://DD_AGENT_HOST:DD_TRACE_AGENT_PORT,
+//     defaulting to localhost and 8126, respectively
+//   - Then, DefaultTraceAgentUDSPath, if the path exists
+//   - Finally, localhost:8126
 func AgentURLFromEnv() *url.URL {
-	agentURL := os.Getenv("DD_TRACE_AGENT_URL")
-	if agentURL == "" {
-		return nil
+	if agentURL := os.Getenv("DD_TRACE_AGENT_URL"); agentURL != "" {
+		u, err := url.Parse(agentURL)
+		if err != nil {
+			log.Warn("Failed to parse DD_TRACE_AGENT_URL: %v", err)
+		} else {
+			switch u.Scheme {
+			case "unix", "http", "https":
+				return u
+			default:
+				log.Warn("Unsupported protocol %q in Agent URL %q. Must be one of: http, https, unix.", u.Scheme, agentURL)
+			}
+		}
+	}
+
+	host, providedHost := os.LookupEnv("DD_AGENT_HOST")
+	port, providedPort := os.LookupEnv("DD_TRACE_AGENT_PORT")
+	if host == "" {
+		// We treat set but empty the same as unset
+		providedHost = false
+		host = DefaultAgentHostname
+	}
+	if port == "" {
+		// We treat set but empty the same as unset
+		providedPort = false
+		port = DefaultTraceAgentPort
 	}
-	u, err := url.Parse(agentURL)
-	if err != nil {
-		log.Warn("Failed to parse DD_TRACE_AGENT_URL: %v", err)
-		return nil
+	httpURL := &url.URL{
+		Scheme: "http",
+		Host:   net.JoinHostPort(host, port),
 	}
-	switch u.Scheme {
-	case "unix", "http", "https":
-		return u
-	default:
-		log.Warn("Unsupported protocol %q in Agent URL %q. Must be one of: http, https, unix.", u.Scheme, agentURL)
-		return nil
+	if providedHost || providedPort {
+		return httpURL
+	}
+
+	if _, err := os.Stat(DefaultTraceAgentUDSPath); err == nil {
+		return &url.URL{
+			Scheme: "unix",
+			Path:   DefaultTraceAgentUDSPath,
+		}
 	}
+	return httpURL
 }

internal/agent_test.go

@@ -11,26 +11,84 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestAgentURLFromEnv2(t *testing.T) {
+func TestAgentURLFromEnv(t *testing.T) {
 	for name, tc := range map[string]struct {
 		input string
 		want  string
 	}{
-		"empty":    {input: "", want: ""},
-		"protocol": {input: "bad://custom:1234", want: ""},
-		"invalid":  {input: "http://localhost%+o:8126", want: ""},
+		"empty": {input: "", want: "http://localhost:8126"},
+		// The next two are invalid, in which case we should fall back to the defaults
+		"protocol": {input: "bad://custom:1234", want: "http://localhost:8126"},
+		"invalid":  {input: "http://localhost%+o:8126", want: "http://localhost:8126"},
 		"http":     {input: "http://custom:1234", want: "http://custom:1234"},
 		"https":    {input: "https://custom:1234", want: "https://custom:1234"},
 		"unix":     {input: "unix:///path/to/custom.socket", want: "unix:///path/to/custom.socket"},
 	} {
 		t.Run(name, func(t *testing.T) {
 			t.Setenv("DD_TRACE_AGENT_URL", tc.input)
 			url := AgentURLFromEnv()
-			if tc.want == "" {
-				assert.Nil(t, url)
-			} else {
-				assert.Equal(t, tc.want, url.String())
-			}
+			assert.Equal(t, tc.want, url.String())
 		})
 	}
 }
+
+func TestAgentURLPriorityOrder(t *testing.T) {
+	makeTestUDS := func(t *testing.T) string {
+		// NB: We don't try to connect to this, we just check that a
+		// path exists
+		s := t.TempDir()
+		old := DefaultTraceAgentUDSPath
+		DefaultTraceAgentUDSPath = s
+		t.Cleanup(func() { DefaultTraceAgentUDSPath = old })
+		return s
+	}
+
+	t.Run("DD_TRACE_AGENT_URL", func(t *testing.T) {
+		t.Setenv("DD_TRACE_AGENT_URL", "https://foo:1234")
+		t.Setenv("DD_AGENT_HOST", "bar")
+		t.Setenv("DD_TRACE_AGENT_PORT", "5678")
+		_ = makeTestUDS(t)
+		url := AgentURLFromEnv()
+		assert.Equal(t, "https", url.Scheme)
+		assert.Equal(t, "foo:1234", url.Host)
+	})
+
+	t.Run("DD_AGENT_HOST-and-DD_TRACE_AGENT_PORT", func(t *testing.T) {
+		t.Setenv("DD_AGENT_HOST", "bar")
+		t.Setenv("DD_TRACE_AGENT_PORT", "5678")
+		_ = makeTestUDS(t)
+		url := AgentURLFromEnv()
+		assert.Equal(t, "http", url.Scheme)
+		assert.Equal(t, "bar:5678", url.Host)
+	})
+
+	t.Run("DD_AGENT_HOST", func(t *testing.T) {
+		t.Setenv("DD_AGENT_HOST", "bar")
+		_ = makeTestUDS(t)
+		url := AgentURLFromEnv()
+		assert.Equal(t, "http", url.Scheme)
+		assert.Equal(t, "bar:8126", url.Host)
+	})
+
+	t.Run("DD_TRACE_AGENT_PORT", func(t *testing.T) {
+		t.Setenv("DD_TRACE_AGENT_PORT", "5678")
+		_ = makeTestUDS(t)
+		url := AgentURLFromEnv()
+		assert.Equal(t, "http", url.Scheme)
+		assert.Equal(t, "localhost:5678", url.Host)
+	})
+
+	t.Run("UDS", func(t *testing.T) {
+		uds := makeTestUDS(t)
+		url := AgentURLFromEnv()
+		assert.Equal(t, "unix", url.Scheme)
+		assert.Equal(t, "", url.Host)
+		assert.Equal(t, uds, url.Path)
+	})
+
+	t.Run("nothing", func(t *testing.T) {
+		url := AgentURLFromEnv()
+		assert.Equal(t, "http", url.Scheme)
+		assert.Equal(t, "localhost:8126", url.Host)
+	})
+}

profiler/options.go

@@ -202,20 +202,11 @@ func defaultConfig() (*config, error) {
 		c.addProfileType(t)
 	}
 
-	agentHost, agentPort := defaultAgentHost, defaultAgentPort
-	if v := os.Getenv("DD_AGENT_HOST"); v != "" {
-		agentHost = v
-	}
-	if v := os.Getenv("DD_TRACE_AGENT_PORT"); v != "" {
-		agentPort = v
-	}
-	WithAgentAddr(net.JoinHostPort(agentHost, agentPort))(&c)
-	if url := internal.AgentURLFromEnv(); url != nil {
-		if url.Scheme == "unix" {
-			WithUDS(url.Path)(&c)
-		} else {
-			c.agentURL = url.String() + "/profiling/v1/input"
-		}
+	url := internal.AgentURLFromEnv()
+	if url.Scheme == "unix" {
+		WithUDS(url.Path)(&c)
+	} else {
+		c.agentURL = url.String() + "/profiling/v1/input"
 	}
 	if v := os.Getenv("DD_PROFILING_UPLOAD_TIMEOUT"); v != "" {
 		d, err := time.ParseDuration(v)
@@ -475,13 +466,23 @@ func WithHTTPClient(client *http.Client) Option {
 
 // WithUDS configures the HTTP client to dial the Datadog Agent via the specified Unix Domain Socket path.
 func WithUDS(socketPath string) Option {
-	return WithHTTPClient(&http.Client{
-		Transport: &http.Transport{
-			DialContext: func(_ context.Context, _, _ string) (net.Conn, error) {
-				return net.Dial("unix", socketPath)
+	return func(c *config) {
+		// The HTTP client needs a valid URL. The host portion of the
+		// url in particular can't just be the socket path, or else that
+		// will be interpreted as part of the request path and the
+		// request will fail.  Clean up the path here so we get
+		// something resembling the desired path in any profiler logs.
+		// TODO: copied from ddtrace/tracer, but is this correct?
+		cleanPath := fmt.Sprintf("UDS_%s", strings.NewReplacer(":", "_", "/", "_", `\`, "_").Replace(socketPath))
+		c.agentURL = "http://" + cleanPath + "/profiling/v1/input"
+		WithHTTPClient(&http.Client{
+			Transport: &http.Transport{
+				DialContext: func(_ context.Context, _, _ string) (net.Conn, error) {
+					return net.Dial("unix", socketPath)
+				},
 			},
-		},
-	})
+		})(c)
+	}
 }
 
 // withOutputDir writes a copy of all uploaded profiles to the given