From cef2e3734a219a2ea376299b5245a290b33e2721 Mon Sep 17 00:00:00 2001 From: Rachel Yang Date: Thu, 5 Dec 2024 15:56:11 -0500 Subject: [PATCH 1/9] WIP: http/net error codes --- contrib/internal/httptrace/before_handle.go | 8 ++- contrib/net/http/http.go | 9 +-- contrib/net/http/option.go | 10 +++ contrib/net/http/trace_test.go | 77 +++++++++++++++++++++ 4 files changed, 98 insertions(+), 6 deletions(-) diff --git a/contrib/internal/httptrace/before_handle.go b/contrib/internal/httptrace/before_handle.go index ba0458f47d..5403d7ba7c 100644 --- a/contrib/internal/httptrace/before_handle.go +++ b/contrib/internal/httptrace/before_handle.go @@ -6,6 +6,7 @@ package httptrace import ( + "fmt" "net/http" "gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/options" @@ -36,7 +37,7 @@ type ServeConfig struct { // 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 + IsStatusError func(int) bool } // BeforeHandle contains functionality that should be executed before a http.Handler runs. @@ -61,7 +62,10 @@ func BeforeHandle(cfg *ServeConfig, w http.ResponseWriter, r *http.Request) (htt rw, ddrw := wrapResponseWriter(w) rt := r.WithContext(ctx) closeSpan := func() { - FinishRequestSpan(span, ddrw.status, cfg.isStatusError, cfg.FinishOpts...) + if cfg.IsStatusError != nil && cfg.IsStatusError(ddrw.status) { + span.SetTag(ext.Error, fmt.Errorf("%d: %s", ddrw.status, http.StatusText(ddrw.status))) + } + FinishRequestSpan(span, ddrw.status, cfg.IsStatusError, cfg.FinishOpts...) } afterHandle := closeSpan handled := false diff --git a/contrib/net/http/http.go b/contrib/net/http/http.go index 5aedb032a4..2069e81a83 100644 --- a/contrib/net/http/http.go +++ b/contrib/net/http/http.go @@ -60,10 +60,11 @@ func (mux *ServeMux) ServeHTTP(w http.ResponseWriter, r *http.Request) { copy(so, mux.cfg.spanOpts) so = append(so, httptrace.HeaderTagsFromRequest(r, mux.cfg.headerTags)) TraceAndServe(mux.ServeMux, w, r, &ServeConfig{ - Service: mux.cfg.serviceName, - Resource: resource, - SpanOpts: so, - Route: route, + Service: mux.cfg.serviceName, + Resource: resource, + SpanOpts: so, + Route: route, + IsStatusError: mux.cfg.isStatusError, }) } diff --git a/contrib/net/http/option.go b/contrib/net/http/option.go index 22e63ee3b0..105e84fcf1 100644 --- a/contrib/net/http/option.go +++ b/contrib/net/http/option.go @@ -35,6 +35,7 @@ type config struct { finishOpts []ddtrace.FinishOption ignoreRequest func(*http.Request) bool resourceNamer func(*http.Request) string + isStatusError func(int) bool headerTags *internal.LockMap } @@ -51,6 +52,7 @@ func defaults(cfg *config) { cfg.analyticsRate = globalconfig.AnalyticsRate() } cfg.serviceName = namingschema.ServiceName(defaultServiceName) + cfg.isStatusError = func(status int) bool { return status >= 500 && status < 600 } cfg.headerTags = globalconfig.HeaderTagMap() cfg.spanOpts = []ddtrace.StartSpanOption{tracer.Measured()} if !math.IsNaN(cfg.analyticsRate) { @@ -86,6 +88,14 @@ func WithHeaderTags(headers []string) Option { } } +// WithStatusCheck sets a span to be an error if the passed function +// returns true for a given status code. +func WithStatusCheck(checker func(statusCode int) bool) Option { + return func(cfg *config) { + cfg.isStatusError = checker + } +} + // WithAnalytics enables Trace Analytics for all started spans. func WithAnalytics(on bool) MuxOption { return func(cfg *config) { diff --git a/contrib/net/http/trace_test.go b/contrib/net/http/trace_test.go index e00aaa95fe..33d3434ad3 100644 --- a/contrib/net/http/trace_test.go +++ b/contrib/net/http/trace_test.go @@ -10,6 +10,7 @@ import ( "io" "net/http" "net/http/httptest" + "os" "testing" "time" @@ -299,6 +300,82 @@ func TestTraceAndServe(t *testing.T) { assert.Equal("/path?", span.Tag(ext.HTTPURL)) assert.Equal("200", span.Tag(ext.HTTPCode)) }) + + t.Run("isStatusError", func(t *testing.T) { + mt := mocktracer.Start() + assert := assert.New(t) + defer mt.Stop() + + handler := func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadRequest) + } + r, err := http.NewRequest("GET", "/", nil) + assert.NoError(err) + w := httptest.NewRecorder() + TraceAndServe(http.HandlerFunc(handler), w, r, &ServeConfig{ + Service: "service", + Resource: "resource", + IsStatusError: func(i int) bool { return i >= 400 }, + }) + + spans := mt.FinishedSpans() + assert.Len(spans, 1) + assert.Equal("400", spans[0].Tag(ext.HTTPCode)) + assert.Equal("400: Bad Request", spans[0].Tag(ext.Error).(error).Error()) + }) + + t.Run("isStatusErrorEnv", func(t *testing.T) { + mt := mocktracer.Start() + assert := assert.New(t) + defer mt.Stop() + + // Set environment variable to treat 500 as an error + t.Setenv("DD_TRACE_HTTP_SERVER_ERROR_STATUSES", "500") + + handler := func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadRequest) + } + r, err := http.NewRequest("GET", "/", nil) + assert.NoError(err) + w := httptest.NewRecorder() + TraceAndServe(http.HandlerFunc(handler), w, r, &ServeConfig{ + Service: "service", + Resource: "resource", + IsStatusError: func(i int) bool { return i >= 400 }, + }) + + spans := mt.FinishedSpans() + assert.Len(spans, 1) + assert.Equal("400", spans[0].Tag(ext.HTTPCode)) + assert.Equal("400: Bad Request", spans[0].Tag(ext.Error).(error).Error()) + }) + + t.Run("isStatusErrorClientError", func(t *testing.T) { + mt := mocktracer.Start() + assert := assert.New(t) + defer mt.Stop() + + // Set environment variable to treat 500-510 as client errors + os.Setenv("DD_TRACE_HTTP_CLIENT_ERROR_STATUSES", "500-510") + defer os.Unsetenv("DD_TRACE_HTTP_CLIENT_ERROR_STATUSES") // Ensure cleanup after the test + + handler := func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadRequest) + } + r, err := http.NewRequest("GET", "/", nil) + assert.NoError(err) + w := httptest.NewRecorder() + // Define ServeConfig with custom IsStatusError logic + TraceAndServe(http.HandlerFunc(handler), w, r, &ServeConfig{ + Service: "service", + Resource: "resource", + IsStatusError: func(i int) bool { return i >= 400 && i < 500 }, // Treat only 400-499 as client errors + }) + spans := mt.FinishedSpans() + assert.Len(spans, 1) + assert.Equal("400", spans[0].Tag(ext.HTTPCode)) + assert.Equal("400: Bad Request", spans[0].Tag(ext.Error).(error).Error()) + }) } func TestTraceAndServeHost(t *testing.T) { From 307741cba29fdc4f6db47f7cd57fcbcbc0824130 Mon Sep 17 00:00:00 2001 From: Rachel Yang Date: Tue, 17 Dec 2024 01:35:59 +0900 Subject: [PATCH 2/9] Update contrib/internal/httptrace/before_handle.go Co-authored-by: Mikayla Toffler <46911781+mtoffl01@users.noreply.github.com> --- contrib/internal/httptrace/before_handle.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/contrib/internal/httptrace/before_handle.go b/contrib/internal/httptrace/before_handle.go index 5403d7ba7c..b6ea960f66 100644 --- a/contrib/internal/httptrace/before_handle.go +++ b/contrib/internal/httptrace/before_handle.go @@ -62,9 +62,6 @@ func BeforeHandle(cfg *ServeConfig, w http.ResponseWriter, r *http.Request) (htt rw, ddrw := wrapResponseWriter(w) rt := r.WithContext(ctx) closeSpan := func() { - if cfg.IsStatusError != nil && cfg.IsStatusError(ddrw.status) { - span.SetTag(ext.Error, fmt.Errorf("%d: %s", ddrw.status, http.StatusText(ddrw.status))) - } FinishRequestSpan(span, ddrw.status, cfg.IsStatusError, cfg.FinishOpts...) } afterHandle := closeSpan From 9c8a30d0ec6afaa5aaf211a4836abeafb29fe4a2 Mon Sep 17 00:00:00 2001 From: Rachel Yang Date: Tue, 17 Dec 2024 02:10:58 +0900 Subject: [PATCH 3/9] removing test case --- contrib/internal/httptrace/before_handle.go | 1 - contrib/net/http/trace_test.go | 28 --------------------- 2 files changed, 29 deletions(-) diff --git a/contrib/internal/httptrace/before_handle.go b/contrib/internal/httptrace/before_handle.go index b6ea960f66..f3be037d4b 100644 --- a/contrib/internal/httptrace/before_handle.go +++ b/contrib/internal/httptrace/before_handle.go @@ -6,7 +6,6 @@ package httptrace import ( - "fmt" "net/http" "gopkg.in/DataDog/dd-trace-go.v1/contrib/internal/options" diff --git a/contrib/net/http/trace_test.go b/contrib/net/http/trace_test.go index 33d3434ad3..4806d66da4 100644 --- a/contrib/net/http/trace_test.go +++ b/contrib/net/http/trace_test.go @@ -10,7 +10,6 @@ import ( "io" "net/http" "net/http/httptest" - "os" "testing" "time" @@ -349,33 +348,6 @@ func TestTraceAndServe(t *testing.T) { assert.Equal("400", spans[0].Tag(ext.HTTPCode)) assert.Equal("400: Bad Request", spans[0].Tag(ext.Error).(error).Error()) }) - - t.Run("isStatusErrorClientError", func(t *testing.T) { - mt := mocktracer.Start() - assert := assert.New(t) - defer mt.Stop() - - // Set environment variable to treat 500-510 as client errors - os.Setenv("DD_TRACE_HTTP_CLIENT_ERROR_STATUSES", "500-510") - defer os.Unsetenv("DD_TRACE_HTTP_CLIENT_ERROR_STATUSES") // Ensure cleanup after the test - - handler := func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusBadRequest) - } - r, err := http.NewRequest("GET", "/", nil) - assert.NoError(err) - w := httptest.NewRecorder() - // Define ServeConfig with custom IsStatusError logic - TraceAndServe(http.HandlerFunc(handler), w, r, &ServeConfig{ - Service: "service", - Resource: "resource", - IsStatusError: func(i int) bool { return i >= 400 && i < 500 }, // Treat only 400-499 as client errors - }) - spans := mt.FinishedSpans() - assert.Len(spans, 1) - assert.Equal("400", spans[0].Tag(ext.HTTPCode)) - assert.Equal("400: Bad Request", spans[0].Tag(ext.Error).(error).Error()) - }) } func TestTraceAndServeHost(t *testing.T) { From fce88207e647bcadf167519f2e44b1937d85efb8 Mon Sep 17 00:00:00 2001 From: Rachel Yang Date: Tue, 17 Dec 2024 02:17:05 +0900 Subject: [PATCH 4/9] changes to option.go --- contrib/net/http/option.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/contrib/net/http/option.go b/contrib/net/http/option.go index 105e84fcf1..b35fe8f5d1 100644 --- a/contrib/net/http/option.go +++ b/contrib/net/http/option.go @@ -52,7 +52,7 @@ func defaults(cfg *config) { cfg.analyticsRate = globalconfig.AnalyticsRate() } cfg.serviceName = namingschema.ServiceName(defaultServiceName) - cfg.isStatusError = func(status int) bool { return status >= 500 && status < 600 } + cfg.isStatusError = isServerError cfg.headerTags = globalconfig.HeaderTagMap() cfg.spanOpts = []ddtrace.StartSpanOption{tracer.Measured()} if !math.IsNaN(cfg.analyticsRate) { @@ -62,6 +62,10 @@ func defaults(cfg *config) { cfg.resourceNamer = func(_ *http.Request) string { return "" } } +func isServerError(status int) bool { + return status >= 500 && status < 600 +} + // WithIgnoreRequest holds the function to use for determining if the // incoming HTTP request should not be traced. func WithIgnoreRequest(f func(*http.Request) bool) MuxOption { @@ -90,9 +94,9 @@ func WithHeaderTags(headers []string) Option { // WithStatusCheck sets a span to be an error if the passed function // returns true for a given status code. -func WithStatusCheck(checker func(statusCode int) bool) Option { +func WithStatusCheck(fn func(statusCode int) bool) Option { return func(cfg *config) { - cfg.isStatusError = checker + cfg.isStatusError = fn } } From d76ce950af64fd81ef821040094bfba9d8173e2e Mon Sep 17 00:00:00 2001 From: Rachel Yang Date: Tue, 17 Dec 2024 18:58:01 +0900 Subject: [PATCH 5/9] fixing test case --- contrib/net/http/trace_test.go | 42 ++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/contrib/net/http/trace_test.go b/contrib/net/http/trace_test.go index 4806d66da4..a4045458d0 100644 --- a/contrib/net/http/trace_test.go +++ b/contrib/net/http/trace_test.go @@ -331,22 +331,46 @@ func TestTraceAndServe(t *testing.T) { // Set environment variable to treat 500 as an error t.Setenv("DD_TRACE_HTTP_SERVER_ERROR_STATUSES", "500") - handler := func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusBadRequest) - } - r, err := http.NewRequest("GET", "/", nil) - assert.NoError(err) - w := httptest.NewRecorder() - TraceAndServe(http.HandlerFunc(handler), w, r, &ServeConfig{ + cfg := &ServeConfig{ Service: "service", Resource: "resource", - IsStatusError: func(i int) bool { return i >= 400 }, - }) + IsStatusError: func(i int) bool { return i == 400 }, + } + + // Test a 400 response, which should be reported as an error + handler400 := func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadRequest) // 400 + } + + r400, err := http.NewRequest("GET", "/", nil) + assert.NoError(err) + w400 := httptest.NewRecorder() + TraceAndServe(http.HandlerFunc(handler400), w400, r400, cfg) spans := mt.FinishedSpans() assert.Len(spans, 1) assert.Equal("400", spans[0].Tag(ext.HTTPCode)) assert.Equal("400: Bad Request", spans[0].Tag(ext.Error).(error).Error()) + + // Reset the tracer + mt.Reset() + + // Test a 500 response, which should NOT be reported as an error, + // even though the environment variable says 500 is an error. + handler500 := func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) // 500 + } + + r500, err := http.NewRequest("GET", "/", nil) + assert.NoError(err) + w500 := httptest.NewRecorder() + TraceAndServe(http.HandlerFunc(handler500), w500, r500, cfg) + + spans = mt.FinishedSpans() + assert.Len(spans, 1) + assert.Equal("500", spans[0].Tag(ext.HTTPCode)) + // Confirm that the span is NOT marked as an error. + assert.Nil(spans[0].Tag(ext.Error)) }) } From 301ffb89a9eb9c0d8015c9db9c344e250c153182 Mon Sep 17 00:00:00 2001 From: Rachel Yang Date: Thu, 19 Dec 2024 01:39:12 +0900 Subject: [PATCH 6/9] system tests fix --- contrib/net/http/option.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/contrib/net/http/option.go b/contrib/net/http/option.go index b35fe8f5d1..408b93b66f 100644 --- a/contrib/net/http/option.go +++ b/contrib/net/http/option.go @@ -52,7 +52,6 @@ func defaults(cfg *config) { cfg.analyticsRate = globalconfig.AnalyticsRate() } cfg.serviceName = namingschema.ServiceName(defaultServiceName) - cfg.isStatusError = isServerError cfg.headerTags = globalconfig.HeaderTagMap() cfg.spanOpts = []ddtrace.StartSpanOption{tracer.Measured()} if !math.IsNaN(cfg.analyticsRate) { @@ -62,10 +61,6 @@ func defaults(cfg *config) { cfg.resourceNamer = func(_ *http.Request) string { return "" } } -func isServerError(status int) bool { - return status >= 500 && status < 600 -} - // WithIgnoreRequest holds the function to use for determining if the // incoming HTTP request should not be traced. func WithIgnoreRequest(f func(*http.Request) bool) MuxOption { From 040eb86ce93a7c41a8569d598c00a9872e05a06f Mon Sep 17 00:00:00 2001 From: Rachel Yang Date: Sat, 21 Dec 2024 00:05:01 +0900 Subject: [PATCH 7/9] added test case --- contrib/net/http/trace_test.go | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/contrib/net/http/trace_test.go b/contrib/net/http/trace_test.go index a4045458d0..7a3f2c3fa3 100644 --- a/contrib/net/http/trace_test.go +++ b/contrib/net/http/trace_test.go @@ -300,7 +300,7 @@ func TestTraceAndServe(t *testing.T) { assert.Equal("200", span.Tag(ext.HTTPCode)) }) - t.Run("isStatusError", func(t *testing.T) { + t.Run("integrationLevelErrorHandling", func(t *testing.T) { mt := mocktracer.Start() assert := assert.New(t) defer mt.Stop() @@ -323,7 +323,35 @@ func TestTraceAndServe(t *testing.T) { assert.Equal("400: Bad Request", spans[0].Tag(ext.Error).(error).Error()) }) - t.Run("isStatusErrorEnv", func(t *testing.T) { + t.Run("envLevelErrorHandling", func(t *testing.T) { + mt := mocktracer.Start() + assert := assert.New(t) + defer mt.Stop() + + t.Setenv("DD_TRACE_HTTP_SERVER_ERROR_STATUSES", "500") + + cfg := &ServeConfig{ + Service: "service", + Resource: "resource", + IsStatusError: nil, // No integration-level configuration + } + + handler := func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) // 500 + } + + r, err := http.NewRequest("GET", "/", nil) + assert.NoError(err) + w := httptest.NewRecorder() + TraceAndServe(http.HandlerFunc(handler), w, r, cfg) + + spans := mt.FinishedSpans() + assert.Len(spans, 1) + assert.Equal("500", spans[0].Tag(ext.HTTPCode)) + assert.Equal("500: Internal Server Error", spans[0].Tag(ext.Error).(error).Error()) + }) + + t.Run("integrationOverridesEnvConfig", func(t *testing.T) { mt := mocktracer.Start() assert := assert.New(t) defer mt.Stop() From 25c62bfd7183dec697e39f4a2696ced31e788795 Mon Sep 17 00:00:00 2001 From: Rachel Yang Date: Sat, 21 Dec 2024 01:07:38 +0900 Subject: [PATCH 8/9] Update trace_test.go --- contrib/net/http/trace_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/contrib/net/http/trace_test.go b/contrib/net/http/trace_test.go index 7a3f2c3fa3..a8621a8820 100644 --- a/contrib/net/http/trace_test.go +++ b/contrib/net/http/trace_test.go @@ -333,7 +333,6 @@ func TestTraceAndServe(t *testing.T) { cfg := &ServeConfig{ Service: "service", Resource: "resource", - IsStatusError: nil, // No integration-level configuration } handler := func(w http.ResponseWriter, r *http.Request) { From 031dbfc84c45fe468b778c8cb4d1d5fd6ba62432 Mon Sep 17 00:00:00 2001 From: Rachel Yang Date: Wed, 8 Jan 2025 10:02:34 -0500 Subject: [PATCH 9/9] Update trace_test.go --- contrib/net/http/trace_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/contrib/net/http/trace_test.go b/contrib/net/http/trace_test.go index a8621a8820..f85a592861 100644 --- a/contrib/net/http/trace_test.go +++ b/contrib/net/http/trace_test.go @@ -312,8 +312,6 @@ func TestTraceAndServe(t *testing.T) { assert.NoError(err) w := httptest.NewRecorder() TraceAndServe(http.HandlerFunc(handler), w, r, &ServeConfig{ - Service: "service", - Resource: "resource", IsStatusError: func(i int) bool { return i >= 400 }, }) @@ -359,8 +357,6 @@ func TestTraceAndServe(t *testing.T) { t.Setenv("DD_TRACE_HTTP_SERVER_ERROR_STATUSES", "500") cfg := &ServeConfig{ - Service: "service", - Resource: "resource", IsStatusError: func(i int) bool { return i == 400 }, }