contrib/go-chi: Apply DD_TRACE_HTTP_SERVER_ERROR_STATUSES (#2960)

DataDog · Nov 6, 2024 · 5dd43b0 · 5dd43b0
1 parent c9fc691
commit 5dd43b0
Show file tree

Hide file tree

Showing 6 changed files with 80 additions and 18 deletions.
diff --git a/contrib/go-chi/chi.v5/chi_test.go b/contrib/go-chi/chi.v5/chi_test.go
@@ -16,6 +16,7 @@ import (
 	"testing"
 
 	pappsec "gopkg.in/DataDog/dd-trace-go.v1/appsec"
+	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/namingschematest"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer"
@@ -207,6 +208,41 @@ func TestError(t *testing.T) {
 		wantErr := fmt.Sprintf("%d: %s", code, http.StatusText(code))
 		assert.Equal(wantErr, span.Tag(ext.Error).(error).Error())
 	})
+	t.Run("envvar", func(t *testing.T) {
+		assert := assert.New(t)
+		t.Setenv("DD_TRACE_HTTP_SERVER_ERROR_STATUSES", "200")
+		mt := mocktracer.Start()
+		defer mt.Stop()
+
+		// re-run config defaults based on new DD_TRACE_HTTP_SERVER_ERROR_STATUSES value
+		httptrace.ResetCfg()
+
+		router := chi.NewRouter()
+		router.Use(Middleware(
+			WithServiceName("foobar")))
+		code := 200
+		// a handler with an error and make the requests
+		router.Get("/err", func(w http.ResponseWriter, r *http.Request) {
+			http.Error(w, fmt.Sprintf("%d!", code), code)
+		})
+		r := httptest.NewRequest("GET", "/err", nil)
+		w := httptest.NewRecorder()
+		router.ServeHTTP(w, r)
+		response := w.Result()
+		defer response.Body.Close()
+		assert.Equal(response.StatusCode, code)
+
+		spans := mt.FinishedSpans()
+		assert.Len(spans, 1)
+		span := spans[0]
+		if span.Tag(ext.Error) == nil {
+			t.Fatal("Span missing error tags")
+		}
+		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()

diff --git a/contrib/go-chi/chi.v5/option.go b/contrib/go-chi/chi.v5/option.go
@@ -43,7 +43,6 @@ func defaults(cfg *config) {
 		cfg.analyticsRate = globalconfig.AnalyticsRate()
 	}
 	cfg.headerTags = globalconfig.HeaderTagMap()
-	cfg.isStatusError = isServerError
 	cfg.ignoreRequest = func(_ *http.Request) bool { return false }
 	cfg.modifyResourceName = func(s string) string { return s }
 	// for backward compatibility with modifyResourceName, initialize resourceName as nil.
@@ -97,10 +96,6 @@ func WithStatusCheck(fn func(statusCode int) bool) Option {
 	}
 }
 
-func isServerError(statusCode int) bool {
-	return statusCode >= 500 && statusCode < 600
-}
-
 // WithIgnoreRequest specifies a function to use for determining if the
 // incoming HTTP request tracing should be skipped.
 func WithIgnoreRequest(fn func(r *http.Request) bool) Option {

diff --git a/contrib/go-chi/chi/chi_test.go b/contrib/go-chi/chi/chi_test.go
@@ -15,6 +15,7 @@ import (
 	"testing"
 
 	pappsec "gopkg.in/DataDog/dd-trace-go.v1/appsec"
+	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/httptrace"
 	"gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/namingschematest"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer"
@@ -179,6 +180,40 @@ func TestError(t *testing.T) {
 		wantErr := fmt.Sprintf("%d: %s", code, http.StatusText(code))
 		assert.Equal(wantErr, span.Tag(ext.Error).(error).Error())
 	})
+	t.Run("envvar", func(t *testing.T) {
+		assert := assert.New(t)
+		t.Setenv("DD_TRACE_HTTP_SERVER_ERROR_STATUSES", "200")
+		mt := mocktracer.Start()
+		defer mt.Stop()
+
+		// re-run config defaults based on new DD_TRACE_HTTP_SERVER_ERROR_STATUSES value
+		httptrace.ResetCfg()
+
+		router := chi.NewRouter()
+		router.Use(Middleware(
+			WithServiceName("foobar")))
+		code := 200
+		// a handler with an error and make the requests
+		router.Get("/err", func(w http.ResponseWriter, r *http.Request) {
+			http.Error(w, fmt.Sprintf("%d!", code), code)
+		})
+		r := httptest.NewRequest("GET", "/err", nil)
+		w := httptest.NewRecorder()
+		router.ServeHTTP(w, r)
+		response := w.Result()
+		defer response.Body.Close()
+		assert.Equal(response.StatusCode, code)
+
+		spans := mt.FinishedSpans()
+		assert.Len(spans, 1)
+		span := spans[0]
+		if span.Tag(ext.Error) == nil {
+			t.Fatal("Span missing error tags")
+		}
+		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()

diff --git a/contrib/go-chi/chi/option.go b/contrib/go-chi/chi/option.go
@@ -41,7 +41,6 @@ func defaults(cfg *config) {
 		cfg.analyticsRate = globalconfig.AnalyticsRate()
 	}
 	cfg.headerTags = globalconfig.HeaderTagMap()
-	cfg.isStatusError = isServerError
 	cfg.ignoreRequest = func(_ *http.Request) bool { return false }
 	cfg.resourceNamer = func(r *http.Request) string {
 		resourceName := chi.RouteContext(r.Context()).RoutePattern()
@@ -99,10 +98,6 @@ func WithStatusCheck(fn func(statusCode int) bool) Option {
 	}
 }
 
-func isServerError(statusCode int) bool {
-	return statusCode >= 500 && statusCode < 600
-}
-
 // WithHeaderTags enables the integration to attach HTTP request headers as span tags.
 // Warning:
 // Using this feature can risk exposing sensitive data such as authorization tokens to Datadog.

diff --git a/contrib/internal/httptrace/config.go b/contrib/internal/httptrace/config.go
@@ -38,6 +38,11 @@ type config struct {
 	isStatusError     func(statusCode int) bool
 }
 
+// ResetCfg sets local variable cfg back to its defaults (mainly useful for testing)
+func ResetCfg() {
+	cfg = newConfig()
+}
+
 func newConfig() config {
 	c := config{
 		queryString:       !internal.BoolEnv(envQueryStringDisabled, false),

diff --git a/contrib/internal/httptrace/httptrace_test.go b/contrib/internal/httptrace/httptrace_test.go
@@ -91,10 +91,8 @@ func TestConfiguredErrorStatuses(t *testing.T) {
 
 		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()
+		// re-run config defaults based on new DD_TRACE_HTTP_SERVER_ERROR_STATUSES value
+		ResetCfg()
 
 		statuses := []int{0, 200, 400, 500}
 		r := httptest.NewRequest(http.MethodGet, "/test", nil)
@@ -123,10 +121,8 @@ func TestConfiguredErrorStatuses(t *testing.T) {
 
 		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()
+		// re-run config defaults based on new DD_TRACE_HTTP_SERVER_ERROR_STATUSES value
+		ResetCfg()
 
 		r := httptest.NewRequest(http.MethodGet, "/test", nil)
 		sp, _ := StartRequestSpan(r)