[fix][internal/httptrace]: integration-level error codes override glo…

…bal (#2946)

contrib/emicklei/go-restful.v3/restful.go

@@ -48,7 +48,7 @@ func FilterFunc(configOpts ...Option) restful.FilterFunction {
 		spanOpts = append(spanOpts, httptrace.HeaderTagsFromRequest(req.Request, cfg.headerTags))
 		span, ctx := httptrace.StartRequestSpan(req.Request, spanOpts...)
 		defer func() {
-			httptrace.FinishRequestSpan(span, resp.StatusCode(), tracer.WithError(resp.Error()))
+			httptrace.FinishRequestSpan(span, resp.StatusCode(), nil, tracer.WithError(resp.Error()))
 		}()
 
 		// pass the span through the request context

contrib/emicklei/go-restful/restful.go

@@ -48,7 +48,7 @@ func FilterFunc(configOpts ...Option) restful.FilterFunction {
 		spanOpts = append(spanOpts, httptrace.HeaderTagsFromRequest(req.Request, cfg.headerTags))
 		span, ctx := httptrace.StartRequestSpan(req.Request, spanOpts...)
 		defer func() {
-			httptrace.FinishRequestSpan(span, resp.StatusCode(), tracer.WithError(resp.Error()))
+			httptrace.FinishRequestSpan(span, resp.StatusCode(), nil, tracer.WithError(resp.Error()))
 		}()
 
 		// pass the span through the request context
@@ -61,7 +61,7 @@ func FilterFunc(configOpts ...Option) restful.FilterFunction {
 func Filter(req *restful.Request, resp *restful.Response, chain *restful.FilterChain) {
 	span, ctx := httptrace.StartRequestSpan(req.Request, tracer.ResourceName(req.SelectedRoutePath()))
 	defer func() {
-		httptrace.FinishRequestSpan(span, resp.StatusCode(), tracer.WithError(resp.Error()))
+		httptrace.FinishRequestSpan(span, resp.StatusCode(), nil, tracer.WithError(resp.Error()))
 	}()
 
 	// pass the span through the request context

contrib/gin-gonic/gin/gintrace.go

@@ -54,7 +54,7 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc {
 		opts = append(opts, httptrace.HeaderTagsFromRequest(c.Request, cfg.headerTags))
 		span, ctx := httptrace.StartRequestSpan(c.Request, opts...)
 		defer func() {
-			httptrace.FinishRequestSpan(span, c.Writer.Status())
+			httptrace.FinishRequestSpan(span, c.Writer.Status(), nil)
 		}()
 
 		// pass the span through the request context

contrib/go-chi/chi.v5/chi.go

@@ -7,7 +7,6 @@
 package chi // import "gopkg.in/DataDog/dd-trace-go.v1/contrib/go-chi/chi.v5"
 
 import (
-	"fmt"
 	"math"
 	"net/http"
 
@@ -56,11 +55,7 @@ func Middleware(opts ...Option) func(next http.Handler) http.Handler {
 			ww := middleware.NewWrapResponseWriter(w, r.ProtoMajor)
 			defer func() {
 				status := ww.Status()
-				var opts []tracer.FinishOption
-				if cfg.isStatusError(status) {
-					opts = []tracer.FinishOption{tracer.WithError(fmt.Errorf("%d: %s", status, http.StatusText(status)))}
-				}
-				httptrace.FinishRequestSpan(span, status, opts...)
+				httptrace.FinishRequestSpan(span, status, cfg.isStatusError)
 			}()
 
 			// pass the span through the request context

contrib/go-chi/chi.v5/chi_test.go

@@ -138,19 +138,10 @@ func TestWithModifyResourceName(t *testing.T) {
 }
 
 func TestError(t *testing.T) {
-	assertSpan := func(assert *assert.Assertions, spans []mocktracer.Span, code int) {
-		assert.Len(spans, 1)
-		if len(spans) < 1 {
-			t.Fatalf("no spans")
-		}
-		span := spans[0]
+	assertSpan := func(assert *assert.Assertions, span mocktracer.Span, code int) {
 		assert.Equal("http.request", span.OperationName())
 		assert.Equal("foobar", span.Tag(ext.ServiceName))
-
 		assert.Equal(strconv.Itoa(code), span.Tag(ext.HTTPCode))
-
-		wantErr := fmt.Sprintf("%d: %s", code, http.StatusText(code))
-		assert.Equal(wantErr, span.Tag(ext.Error).(error).Error())
 	}
 
 	t.Run("default", func(t *testing.T) {
@@ -176,7 +167,11 @@ func TestError(t *testing.T) {
 
 		// verify the errors and status are correct
 		spans := mt.FinishedSpans()
-		assertSpan(assert, spans, code)
+		assert.Len(spans, 1)
+		span := spans[0]
+		assertSpan(assert, span, code)
+		wantErr := fmt.Sprintf("%d: %s", code, http.StatusText(code))
+		assert.Equal(wantErr, span.Tag(ext.Error).(error).Error())
 	})
 
 	t.Run("custom", func(t *testing.T) {
@@ -206,7 +201,66 @@ func TestError(t *testing.T) {
 
 		// verify the errors and status are correct
 		spans := mt.FinishedSpans()
-		assertSpan(assert, spans, code)
+		assert.Len(spans, 1)
+		span := spans[0]
+		assertSpan(assert, span, code)
+		wantErr := fmt.Sprintf("%d: %s", code, http.StatusText(code))
+		assert.Equal(wantErr, span.Tag(ext.Error).(error).Error())
+	})
+	t.Run("integration overrides global", func(t *testing.T) {
+		assert := assert.New(t)
+		mt := mocktracer.Start()
+		defer mt.Stop()
+
+		t.Setenv("DD_TRACE_HTTP_SERVER_ERROR_STATUSES", "500")
+
+		// setup
+		router := chi.NewRouter()
+		router.Use(Middleware(
+			WithServiceName("foobar"),
+			WithStatusCheck(func(statusCode int) bool {
+				return statusCode == 404
+			}),
+		))
+		code := 404
+		// a handler with an error and make the requests
+		router.Get("/404", func(w http.ResponseWriter, r *http.Request) {
+			http.Error(w, fmt.Sprintf("%d!", code), code)
+		})
+		r := httptest.NewRequest("GET", "/404", nil)
+		w := httptest.NewRecorder()
+		router.ServeHTTP(w, r)
+		response := w.Result()
+		defer response.Body.Close()
+		assert.Equal(response.StatusCode, code)
+
+		// verify the errors and status are correct
+		spans := mt.FinishedSpans()
+		assert.Len(spans, 1)
+		span := spans[0]
+		assertSpan(assert, span, code)
+		wantErr := fmt.Sprintf("%d: %s", code, http.StatusText(code))
+		assert.Equal(wantErr, span.Tag(ext.Error).(error).Error())
+
+		mt.Reset()
+
+		code = 500
+		router.Get("/500", func(w http.ResponseWriter, r *http.Request) {
+			http.Error(w, fmt.Sprintf("%d!", code), code)
+		})
+		r = httptest.NewRequest("GET", "/500", nil)
+		w = httptest.NewRecorder()
+		router.ServeHTTP(w, r)
+		response = w.Result()
+		defer response.Body.Close()
+		assert.Equal(response.StatusCode, 500)
+
+		// verify that span does not have error tag
+		spans = mt.FinishedSpans()
+		assert.Len(spans, 1)
+		span = spans[0]
+		assertSpan(assert, span, 500)
+		assert.Empty(span.Tag(ext.Error))
 	})
 }
 

contrib/go-chi/chi/chi.go

@@ -7,7 +7,6 @@
 package chi // import "gopkg.in/DataDog/dd-trace-go.v1/contrib/go-chi/chi"
 
 import (
-	"fmt"
 	"math"
 	"net/http"
 
@@ -56,11 +55,7 @@ func Middleware(opts ...Option) func(next http.Handler) http.Handler {
 			ww := middleware.NewWrapResponseWriter(w, r.ProtoMajor)
 			defer func() {
 				status := ww.Status()
-				var opts []tracer.FinishOption
-				if cfg.isStatusError(status) {
-					opts = []tracer.FinishOption{tracer.WithError(fmt.Errorf("%d: %s", status, http.StatusText(status)))}
-				}
-				httptrace.FinishRequestSpan(span, status, opts...)
+				httptrace.FinishRequestSpan(span, status, cfg.isStatusError)
 			}()
 
 			// pass the span through the request context

contrib/go-chi/chi/chi_test.go

@@ -110,19 +110,10 @@ func TestTrace200(t *testing.T) {
 }
 
 func TestError(t *testing.T) {
-	assertSpan := func(assert *assert.Assertions, spans []mocktracer.Span, code int) {
-		assert.Len(spans, 1)
-		if len(spans) < 1 {
-			t.Fatalf("no spans")
-		}
-		span := spans[0]
+	assertSpan := func(assert *assert.Assertions, span mocktracer.Span, code int) {
 		assert.Equal("http.request", span.OperationName())
 		assert.Equal("foobar", span.Tag(ext.ServiceName))
-
 		assert.Equal(strconv.Itoa(code), span.Tag(ext.HTTPCode))
-
-		wantErr := fmt.Sprintf("%d: %s", code, http.StatusText(code))
-		assert.Equal(wantErr, span.Tag(ext.Error).(error).Error())
 	}
 
 	t.Run("default", func(t *testing.T) {
@@ -148,7 +139,11 @@ func TestError(t *testing.T) {
 
 		// verify the errors and status are correct
 		spans := mt.FinishedSpans()
-		assertSpan(assert, spans, code)
+		assert.Len(spans, 1)
+		span := spans[0]
+		assertSpan(assert, span, code)
+		wantErr := fmt.Sprintf("%d: %s", code, http.StatusText(code))
+		assert.Equal(wantErr, span.Tag(ext.Error).(error).Error())
 	})
 
 	t.Run("custom", func(t *testing.T) {
@@ -178,7 +173,66 @@ func TestError(t *testing.T) {
 
 		// verify the errors and status are correct
 		spans := mt.FinishedSpans()
-		assertSpan(assert, spans, code)
+		assert.Len(spans, 1)
+		span := spans[0]
+		assertSpan(assert, span, code)
+		wantErr := fmt.Sprintf("%d: %s", code, http.StatusText(code))
+		assert.Equal(wantErr, span.Tag(ext.Error).(error).Error())
+	})
+	t.Run("integration overrides global", func(t *testing.T) {
+		assert := assert.New(t)
+		mt := mocktracer.Start()
+		defer mt.Stop()
+
+		t.Setenv("DD_TRACE_HTTP_SERVER_ERROR_STATUSES", "500")
+
+		// setup
+		router := chi.NewRouter()
+		router.Use(Middleware(
+			WithServiceName("foobar"),
+			WithStatusCheck(func(statusCode int) bool {
+				return statusCode == 404
+			}),
+		))
+		code := 404
+		// a handler with an error and make the requests
+		router.Get("/404", func(w http.ResponseWriter, r *http.Request) {
+			http.Error(w, fmt.Sprintf("%d!", code), code)
+		})
+		r := httptest.NewRequest("GET", "/404", nil)
+		w := httptest.NewRecorder()
+		router.ServeHTTP(w, r)
+		response := w.Result()
+		defer response.Body.Close()
+		assert.Equal(response.StatusCode, code)
+
+		// verify the errors and status are correct
+		spans := mt.FinishedSpans()
+		assert.Len(spans, 1)
+		span := spans[0]
+		assertSpan(assert, span, code)
+		wantErr := fmt.Sprintf("%d: %s", code, http.StatusText(code))
+		assert.Equal(wantErr, span.Tag(ext.Error).(error).Error())
+
+		mt.Reset()
+
+		code = 500
+		router.Get("/500", func(w http.ResponseWriter, r *http.Request) {
+			http.Error(w, fmt.Sprintf("%d!", code), code)
+		})
+		r = httptest.NewRequest("GET", "/500", nil)
+		w = httptest.NewRecorder()
+		router.ServeHTTP(w, r)
+		response = w.Result()
+		defer response.Body.Close()
+		assert.Equal(response.StatusCode, 500)
+
+		// verify that span does not have error tag
+		spans = mt.FinishedSpans()
+		assert.Len(spans, 1)
+		span = spans[0]
+		assertSpan(assert, span, 500)
+		assert.Empty(span.Tag(ext.Error))
 	})
 }
 

contrib/internal/httptrace/before_handle.go

@@ -35,6 +35,8 @@ type ServeConfig struct {
 	FinishOpts []ddtrace.FinishOption
 	// SpanOpts specifies any options to be applied to the request starting span.
 	SpanOpts []ddtrace.StartSpanOption
+	// isStatusError allows customization of error code determination.
+	isStatusError func(int) bool
 }
 
 // BeforeHandle contains functionality that should be executed before a http.Handler runs.
@@ -58,9 +60,8 @@ func BeforeHandle(cfg *ServeConfig, w http.ResponseWriter, r *http.Request) (htt
 	span, ctx := StartRequestSpan(r, opts...)
 	rw, ddrw := wrapResponseWriter(w)
 	rt := r.WithContext(ctx)
-
 	closeSpan := func() {
-		FinishRequestSpan(span, ddrw.status, cfg.FinishOpts...)
+		FinishRequestSpan(span, ddrw.status, cfg.isStatusError, cfg.FinishOpts...)
 	}
 	afterHandle := closeSpan
 	handled := false

contrib/internal/httptrace/httptrace.go

@@ -62,20 +62,26 @@ func StartRequestSpan(r *http.Request, opts ...ddtrace.StartSpanOption) (tracer.
 }
 
 // FinishRequestSpan finishes the given HTTP request span and sets the expected response-related tags such as the status
-// code. Any further span finish option can be added with opts.
-func FinishRequestSpan(s tracer.Span, status int, opts ...tracer.FinishOption) {
+// code. If not nil, errorFn will override the isStatusError method on httptrace for determining error codes. Any further span finish option can be added with opts.
+func FinishRequestSpan(s tracer.Span, status int, errorFn func(int) bool, opts ...tracer.FinishOption) {
 	var statusStr string
+	var fn func(int) bool
+	if errorFn == nil {
+		fn = cfg.isStatusError
+	} else {
+		fn = errorFn
+	}
 	// if status is 0, treat it like 200 unless 0 was called out in DD_TRACE_HTTP_SERVER_ERROR_STATUSES
 	if status == 0 {
-		if cfg.isStatusError(status) {
+		if fn(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) {
+		if fn(status) {
 			s.SetTag(ext.Error, fmt.Errorf("%s: %s", statusStr, http.StatusText(status)))
 		}
 	}

contrib/internal/httptrace/httptrace_test.go

@@ -100,7 +100,7 @@ func TestConfiguredErrorStatuses(t *testing.T) {
 		r := httptest.NewRequest(http.MethodGet, "/test", nil)
 		for i, status := range statuses {
 			sp, _ := StartRequestSpan(r)
-			FinishRequestSpan(sp, status)
+			FinishRequestSpan(sp, status, nil)
 			spans := mt.FinishedSpans()
 			require.Len(t, spans, i+1)
 
@@ -130,7 +130,7 @@ func TestConfiguredErrorStatuses(t *testing.T) {
 
 		r := httptest.NewRequest(http.MethodGet, "/test", nil)
 		sp, _ := StartRequestSpan(r)
-		FinishRequestSpan(sp, 0)
+		FinishRequestSpan(sp, 0, nil)
 		spans := mt.FinishedSpans()
 		require.Len(t, spans, 1)
 		assert.Equal(t, "0", spans[0].Tag(ext.HTTPCode))