Fix and Feature: standardize config behavior across tracing libs

contrib/internal/httptrace/config.go

@@ -8,6 +8,8 @@ package httptrace
 import (
 	"os"
 	"regexp"
+	"strconv"
+	"strings"
 
 	"gopkg.in/DataDog/dd-trace-go.v1/internal"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/log"
@@ -22,6 +24,8 @@ const (
 	envQueryStringRegexp = "DD_TRACE_OBFUSCATION_QUERY_STRING_REGEXP"
 	// envTraceClientIPEnabled is the name of the env var used to specify whether or not to collect client ip in span tags
 	envTraceClientIPEnabled = "DD_TRACE_CLIENT_IP_ENABLED"
+	// envServerErrorStatuses is the name of the env var used to specify error status codes on http server spans
+	envServerErrorStatuses = "DD_TRACE_HTTP_SERVER_ERROR_STATUSES"
 )
 
 // defaultQueryStringRegexp is the regexp used for query string obfuscation if `envQueryStringRegexp` is empty.
@@ -31,13 +35,19 @@ type config struct {
 	queryStringRegexp *regexp.Regexp // specifies the regexp to use for query string obfuscation.
 	queryString       bool           // reports whether the query string should be included in the URL span tag.
 	traceClientIP     bool
+	isStatusError     func(statusCode int) bool
 }
 
 func newConfig() config {
 	c := config{
 		queryString:       !internal.BoolEnv(envQueryStringDisabled, false),
 		queryStringRegexp: defaultQueryStringRegexp,
 		traceClientIP:     internal.BoolEnv(envTraceClientIPEnabled, false),
+		isStatusError:     isServerError,
+	}
+	v := os.Getenv(envServerErrorStatuses)
+	if fn := GetErrorCodesFromInput(v); fn != nil {
+		c.isStatusError = fn
 	}
 	if s, ok := os.LookupEnv(envQueryStringRegexp); !ok {
 		return c
@@ -51,3 +61,62 @@ func newConfig() config {
 	}
 	return c
 }
+
+func isServerError(statusCode int) bool {
+	return statusCode >= 500 && statusCode < 600
+}
+
+// GetErrorCodesFromInput parses a comma-separated string s to determine which codes are to be considered errors
+// Its purpose is to support the DD_TRACE_HTTP_SERVER_ERROR_STATUSES env var
+// If error condition cannot be determined from s, `nil` is returned
+// e.g, input of "100,200,300-400" returns a function that returns true on 100, 200, and all values between 300-400, inclusive
+// any input that cannot be translated to integer values returns nil
+func GetErrorCodesFromInput(s string) func(statusCode int) bool {
+	if s == "" {
+		return nil
+	}
+	var codes []int
+	var ranges [][]int
+	vals := strings.Split(s, ",")
+	for _, val := range vals {
+		// "-" indicates a range of values
+		if strings.Contains(val, "-") {
+			bounds := strings.Split(val, "-")
+			if len(bounds) != 2 {
+				log.Debug("Trouble parsing %v due to entry %v, using default error status determination logic", s, val)
+				return nil
+			}
+			before, err := strconv.Atoi(bounds[0])
+			if err != nil {
+				log.Debug("Trouble parsing %v due to entry %v, using default error status determination logic", s, val)
+				return nil
+			}
+			after, err := strconv.Atoi(bounds[1])
+			if err != nil {
+				log.Debug("Trouble parsing %v due to entry %v, using default error status determination logic", s, val)
+				return nil
+			}
+			ranges = append(ranges, []int{before, after})
+		} else {
+			intVal, err := strconv.Atoi(val)
+			if err != nil {
+				log.Debug("Trouble parsing %v due to entry %v, using default error status determination logic", s, val)
+				return nil
+			}
+			codes = append(codes, intVal)
+		}
+	}
+	return func(statusCode int) bool {
+		for _, c := range codes {
+			if c == statusCode {
+				return true
+			}
+		}
+		for _, bounds := range ranges {
+			if statusCode >= bounds[0] && statusCode <= bounds[1] {
+				return true
+			}
+		}
+		return false
+	}
+}

contrib/internal/httptrace/httptrace.go

@@ -65,15 +65,21 @@ func StartRequestSpan(r *http.Request, opts ...ddtrace.StartSpanOption) (tracer.
 // code. Any further span finish option can be added with opts.
 func FinishRequestSpan(s tracer.Span, status int, opts ...tracer.FinishOption) {
 	var statusStr string
+	// if status is 0, treat it like 200 unless 0 was called out in DD_TRACE_HTTP_SERVER_ERROR_STATUSES
 	if status == 0 {
-		statusStr = "200"
+		if cfg.isStatusError(status) {
+			statusStr = "0"
+			s.SetTag(ext.Error, fmt.Errorf("%s: %s", statusStr, http.StatusText(status)))
+		} else {
+			statusStr = "200"
+		}
 	} else {
 		statusStr = strconv.Itoa(status)
+		if cfg.isStatusError(status) {
+			s.SetTag(ext.Error, fmt.Errorf("%s: %s", statusStr, http.StatusText(status)))
+		}
 	}
 	s.SetTag(ext.HTTPCode, statusStr)
-	if status >= 500 && status < 600 {
-		s.SetTag(ext.Error, fmt.Errorf("%s: %s", statusStr, http.StatusText(status)))
-	}
 	s.Finish(opts...)
 }
 

contrib/internal/httptrace/httptrace_test.go

@@ -6,9 +6,11 @@
 package httptrace
 
 import (
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"os"
 	"strconv"
 	"testing"
 
@@ -25,6 +27,117 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func TestGetErrorCodesFromInput(t *testing.T) {
+	codesOnly := "400,401,402"
+	rangesOnly := "400-405,408-410"
+	mixed := "400,403-405,407-410,412"
+	invalid1 := "1,100-200-300-"
+	invalid2 := "abc:@3$5^,"
+	empty := ""
+	t.Run("codesOnly", func(t *testing.T) {
+		fn := GetErrorCodesFromInput(codesOnly)
+		for i := 400; i <= 402; i++ {
+			assert.True(t, fn(i))
+		}
+		assert.False(t, fn(500))
+		assert.False(t, fn(0))
+	})
+	t.Run("rangesOnly", func(t *testing.T) {
+		fn := GetErrorCodesFromInput(rangesOnly)
+		for i := 400; i <= 405; i++ {
+			assert.True(t, fn(i))
+		}
+		for i := 408; i <= 410; i++ {
+			assert.True(t, fn(i))
+		}
+		assert.False(t, fn(406))
+		assert.False(t, fn(411))
+		assert.False(t, fn(500))
+	})
+	t.Run("mixed", func(t *testing.T) {
+		fn := GetErrorCodesFromInput(mixed)
+		assert.True(t, fn(400))
+		assert.False(t, fn(401))
+		for i := 403; i <= 405; i++ {
+			assert.True(t, fn(i))
+		}
+		assert.False(t, fn(406))
+		for i := 407; i <= 410; i++ {
+			assert.True(t, fn(i))
+		}
+		assert.False(t, fn(411))
+		assert.False(t, fn(500))
+	})
+	// invalid entries below should result in nils
+	t.Run("invalid1", func(t *testing.T) {
+		fn := GetErrorCodesFromInput(invalid1)
+		assert.Nil(t, fn)
+	})
+	t.Run("invalid2", func(t *testing.T) {
+		fn := GetErrorCodesFromInput(invalid2)
+		assert.Nil(t, fn)
+	})
+	t.Run("empty", func(t *testing.T) {
+		fn := GetErrorCodesFromInput(empty)
+		assert.Nil(t, fn)
+	})
+}
+
+func TestConfiguredErrorStatuses(t *testing.T) {
+	defer os.Unsetenv("DD_TRACE_HTTP_SERVER_ERROR_STATUSES")
+	t.Run("configured", func(t *testing.T) {
+		mt := mocktracer.Start()
+		defer mt.Stop()
+
+		os.Setenv("DD_TRACE_HTTP_SERVER_ERROR_STATUSES", "199-399,400,501")
+
+		// reset config based on new DD_TRACE_HTTP_SERVER_ERROR_STATUSES value
+		oldConfig := cfg
+		defer func() { cfg = oldConfig }()
+		cfg = newConfig()
+
+		statuses := []int{0, 200, 400, 500}
+		r := httptest.NewRequest(http.MethodGet, "/test", nil)
+		for i, status := range statuses {
+			sp, _ := StartRequestSpan(r)
+			FinishRequestSpan(sp, status)
+			spans := mt.FinishedSpans()
+			require.Len(t, spans, i+1)
+
+			switch status {
+			case 0:
+				assert.Equal(t, "200", spans[i].Tag(ext.HTTPCode))
+				assert.Nil(t, spans[i].Tag(ext.Error))
+			case 200, 400:
+				assert.Equal(t, strconv.Itoa(status), spans[i].Tag(ext.HTTPCode))
+				assert.Equal(t, fmt.Errorf("%s: %s", strconv.Itoa(status), http.StatusText(status)), spans[i].Tag(ext.Error).(error))
+			case 500:
+				assert.Equal(t, strconv.Itoa(status), spans[i].Tag(ext.HTTPCode))
+				assert.Nil(t, spans[i].Tag(ext.Error))
+			}
+		}
+	})
+	t.Run("zero", func(t *testing.T) {
+		mt := mocktracer.Start()
+		defer mt.Stop()
+
+		os.Setenv("DD_TRACE_HTTP_SERVER_ERROR_STATUSES", "0")
+
+		// reset config based on new DD_TRACE_HTTP_SERVER_ERROR_STATUSES value
+		oldConfig := cfg
+		defer func() { cfg = oldConfig }()
+		cfg = newConfig()
+
+		r := httptest.NewRequest(http.MethodGet, "/test", nil)
+		sp, _ := StartRequestSpan(r)
+		FinishRequestSpan(sp, 0)
+		spans := mt.FinishedSpans()
+		require.Len(t, spans, 1)
+		assert.Equal(t, "0", spans[0].Tag(ext.HTTPCode))
+		assert.Equal(t, fmt.Errorf("0: %s", http.StatusText(0)), spans[0].Tag(ext.Error).(error))
+	})
+}
+
 func TestHeaderTagsFromRequest(t *testing.T) {
 	mt := mocktracer.Start()
 	defer mt.Stop()

contrib/net/http/http_test.go

@@ -520,10 +520,14 @@ func handler200(w http.ResponseWriter, _ *http.Request) {
 	w.Write([]byte("OK\n"))
 }
 
-func handler500(w http.ResponseWriter, _ *http.Request) {
+func handler500(w http.ResponseWriter, r *http.Request) {
 	http.Error(w, "500!", http.StatusInternalServerError)
 }
 
+func handler400(w http.ResponseWriter, r *http.Request) {
+	http.Error(w, "400!", http.StatusBadRequest)
+}
+
 func BenchmarkHttpServeTrace(b *testing.B) {
 	tracer.Start(tracer.WithLogger(log.DiscardLogger{}))
 	defer tracer.Stop()

contrib/net/http/option.go

@@ -8,7 +8,9 @@ package http
 import (
 	"math"
 	"net/http"
+	"os"
 
+	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
@@ -18,7 +20,13 @@ import (
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/normalizer"
 )
 
-const defaultServiceName = "http.router"
+const (
+	defaultServiceName = "http.router"
+	// envClientQueryStringEnabled is the name of the env var used to specify whether query string collection is enabled for http client spans.
+	envClientQueryStringEnabled = "DD_TRACE_HTTP_CLIENT_TAG_QUERY_STRING"
+	// envClientErrorStatuses is the name of the env var that specifies error status codes on http client spans
+	envClientErrorStatuses = "DD_TRACE_HTTP_CLIENT_ERROR_STATUSES"
+)
 
 type config struct {
 	serviceName   string
@@ -146,6 +154,8 @@ type roundTripperConfig struct {
 	spanOpts      []ddtrace.StartSpanOption
 	propagation   bool
 	errCheck      func(err error) bool
+	queryString   bool // reports whether the query string is included in the URL tag for http client spans
+	isStatusError func(statusCode int) bool
 }
 
 func newRoundTripperConfig() *roundTripperConfig {
@@ -156,14 +166,22 @@ func newRoundTripperConfig() *roundTripperConfig {
 	defaultSpanNamer := func(_ *http.Request) string {
 		return spanName
 	}
-	return &roundTripperConfig{
+
+	c := &roundTripperConfig{
 		serviceName:   namingschema.ServiceNameOverrideV0("", ""),
 		analyticsRate: globalconfig.AnalyticsRate(),
 		resourceNamer: defaultResourceNamer,
 		propagation:   true,
 		spanNamer:     defaultSpanNamer,
 		ignoreRequest: func(_ *http.Request) bool { return false },
+		queryString:   internal.BoolEnv(envClientQueryStringEnabled, true),
+		isStatusError: isClientError,
+	}
+	v := os.Getenv(envClientErrorStatuses)
+	if fn := httptrace.GetErrorCodesFromInput(v); fn != nil {
+		c.isStatusError = fn
 	}
+	return c
 }
 
 // A RoundTripperOption represents an option that can be passed to
@@ -264,3 +282,7 @@ func RTWithErrorCheck(fn func(err error) bool) RoundTripperOption {
 		cfg.errCheck = fn
 	}
 }
+
+func isClientError(statusCode int) bool {
+	return statusCode >= 400 && statusCode < 500
+}