From 86b14662119bd3ad546cc38c3585d8b336812328 Mon Sep 17 00:00:00 2001 From: kazukousen Date: Thu, 14 Dec 2023 00:02:02 +0900 Subject: [PATCH] contrib/internal/httptrace: Add DD_TRACE_SET_HTTP_ERROR_DISABLED env var --- contrib/internal/httptrace/config.go | 4 ++ contrib/internal/httptrace/config_test.go | 47 ++++++++-------- contrib/internal/httptrace/httptrace.go | 2 +- contrib/internal/httptrace/httptrace_test.go | 57 ++++++++++++++++++++ 4 files changed, 88 insertions(+), 22 deletions(-) diff --git a/contrib/internal/httptrace/config.go b/contrib/internal/httptrace/config.go index b545a447ce..311b86a529 100644 --- a/contrib/internal/httptrace/config.go +++ b/contrib/internal/httptrace/config.go @@ -22,6 +22,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" + // envSetHTTPErrorDisabled is the name of the env var used to disabled to set HTTP 5xx error to span tags + envSetHTTPErrorDisabled = "DD_TRACE_SET_HTTP_ERROR_DISABLED" ) // defaultQueryStringRegexp is the regexp used for query string obfuscation if `envQueryStringRegexp` is empty. @@ -31,6 +33,7 @@ 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 + setHTTPError bool } func newConfig() config { @@ -38,6 +41,7 @@ func newConfig() config { queryString: !internal.BoolEnv(envQueryStringDisabled, false), queryStringRegexp: defaultQueryStringRegexp, traceClientIP: internal.BoolEnv(envTraceClientIPEnabled, false), + setHTTPError: !internal.BoolEnv(envSetHTTPErrorDisabled, false), } if s, ok := os.LookupEnv(envQueryStringRegexp); !ok { return c diff --git a/contrib/internal/httptrace/config_test.go b/contrib/internal/httptrace/config_test.go index c052292346..8e429b64eb 100644 --- a/contrib/internal/httptrace/config_test.go +++ b/contrib/internal/httptrace/config_test.go @@ -6,7 +6,6 @@ package httptrace import ( - "os" "testing" "github.com/stretchr/testify/require" @@ -16,6 +15,8 @@ func TestConfig(t *testing.T) { defaultCfg := config{ queryString: true, queryStringRegexp: defaultQueryStringRegexp, + traceClientIP: false, + setHTTPError: true, } for _, tc := range []struct { name string @@ -38,40 +39,44 @@ func TestConfig(t *testing.T) { name: "disable-query", env: map[string]string{envQueryStringDisabled: "true"}, cfg: config{ + queryString: false, queryStringRegexp: defaultQueryStringRegexp, + traceClientIP: false, + setHTTPError: true, }, }, { name: "disable-query-obf", env: map[string]string{envQueryStringRegexp: ""}, cfg: config{ - queryString: true, + queryString: true, + queryStringRegexp: nil, + traceClientIP: false, + setHTTPError: true, }, }, + { + name: "disable-set-http-error", + env: map[string]string{envSetHTTPErrorDisabled: "true"}, + cfg: config{ + queryString: true, + queryStringRegexp: defaultQueryStringRegexp, + traceClientIP: false, + setHTTPError: false, + }, + }, + { + name: "disable-set-http-error-obf", + env: map[string]string{envSetHTTPErrorDisabled: ""}, + cfg: defaultCfg, + }, } { t.Run(tc.name, func(t *testing.T) { - defer cleanEnv()() for k, v := range tc.env { - os.Setenv(k, v) + t.Setenv(k, v) } c := newConfig() - require.Equal(t, tc.cfg.queryStringRegexp, c.queryStringRegexp) - require.Equal(t, tc.cfg.queryString, c.queryString) + require.Equal(t, tc.cfg, c) }) } } - -func cleanEnv() func() { - env := map[string]string{ - envQueryStringDisabled: os.Getenv(envQueryStringDisabled), - envQueryStringRegexp: os.Getenv(envQueryStringRegexp), - } - for k := range env { - os.Unsetenv(k) - } - return func() { - for k, v := range env { - os.Setenv(k, v) - } - } -} diff --git a/contrib/internal/httptrace/httptrace.go b/contrib/internal/httptrace/httptrace.go index 29df91d67c..872120a2a7 100644 --- a/contrib/internal/httptrace/httptrace.go +++ b/contrib/internal/httptrace/httptrace.go @@ -71,7 +71,7 @@ func FinishRequestSpan(s tracer.Span, status int, opts ...tracer.FinishOption) { statusStr = strconv.Itoa(status) } s.SetTag(ext.HTTPCode, statusStr) - if status >= 500 && status < 600 { + if cfg.setHTTPError && status >= 500 && status < 600 { s.SetTag(ext.Error, fmt.Errorf("%s: %s", statusStr, http.StatusText(status))) } s.Finish(opts...) diff --git a/contrib/internal/httptrace/httptrace_test.go b/contrib/internal/httptrace/httptrace_test.go index 8207bbdaa4..831febc8d6 100644 --- a/contrib/internal/httptrace/httptrace_test.go +++ b/contrib/internal/httptrace/httptrace_test.go @@ -18,6 +18,7 @@ import ( "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" "gopkg.in/DataDog/dd-trace-go.v1/internal" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" + "gopkg.in/DataDog/dd-trace-go.v1/internal/namingschema" "gopkg.in/DataDog/dd-trace-go.v1/internal/normalizer" "github.com/DataDog/appsec-internal-go/netip" @@ -146,6 +147,62 @@ func TestTraceClientIPFlag(t *testing.T) { } } +func TestSetHTTPErrorFlag(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + + type testCase struct { + name string + setHTTPErrorEnvVal string + expectSetHTTPError bool + } + + for _, tc := range []testCase{ + { + name: "Set HTTP Error Disabled Env set to true", + setHTTPErrorEnvVal: "true", + expectSetHTTPError: false, + }, + { + name: "Set HTTP Error Disabled Env set to false", + setHTTPErrorEnvVal: "false", + expectSetHTTPError: true, + }, + { + name: "Set HTTP Error Disabled Env unset", + setHTTPErrorEnvVal: "", + expectSetHTTPError: true, + }, + { + name: "Set HTTP Error Disabled Env set to non-boolean value", + setHTTPErrorEnvVal: "asdadsasd", + expectSetHTTPError: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv(envSetHTTPErrorDisabled, tc.setHTTPErrorEnvVal) + + // reset config based on new DD_TRACE_SET_HTTP_ERROR_DISABLED value + cfg = newConfig() + + s := tracer.StartSpan(namingschema.NewHTTPServerOp().GetName(), nil) + FinishRequestSpan(s, http.StatusServiceUnavailable, nil) + + spans := mt.FinishedSpans() + targetSpan := spans[0] + + assert.Equal(t, "503", targetSpan.Tag(ext.HTTPCode)) + if tc.expectSetHTTPError { + assert.Equal(t, "503: Service Unavailable", targetSpan.Tag(ext.Error).(error).Error()) + } else { + assert.NotContains(t, targetSpan.Tags(), ext.Error) + } + mt.Reset() + }) + } + +} + func TestURLTag(t *testing.T) { type URLTestCase struct { name, expectedURL, host, port, path, query, fragment string