feat: do not report error stack for status code errors (#3837)

Co-authored-by: kakkoyun <[email protected]>

Author: hannahkm

contrib/gofiber/fiber.v2/fiber.go

@@ -98,7 +98,7 @@ func Middleware(opts ...Option) func(c *fiber.Ctx) error {
 			span.SetTag(ext.Error, err)
 		} else if cfg.isStatusError(status) {
 			// mark 5xx server error
-			span.SetTag(ext.Error, fmt.Errorf("%d: %s", status, http.StatusText(status)))
+			span.SetTag(ext.ErrorNoStackTrace, fmt.Errorf("%d: %s", status, http.StatusText(status)))
 		}
 		return err
 	}

contrib/net/http/internal/wrap/roundtrip.go

@@ -174,7 +174,7 @@ func ObserveRoundTrip(cfg *config.RoundTripperConfig, req *http.Request) (*http.
 			span.SetTag(ext.HTTPCode, strconv.Itoa(resp.StatusCode))
 			if cfg.IsStatusError(resp.StatusCode) {
 				span.SetTag("http.errors", resp.Status)
-				span.SetTag(ext.Error, fmt.Errorf("%d: %s", resp.StatusCode, http.StatusText(resp.StatusCode)))
+				span.SetTag(ext.ErrorNoStackTrace, fmt.Errorf("%d: %s", resp.StatusCode, http.StatusText(resp.StatusCode)))
 			}
 		}
 

contrib/valyala/fasthttp/fasthttp.go

@@ -55,7 +55,7 @@ func WrapHandler(h fasthttp.RequestHandler, opts ...Option) fasthttp.RequestHand
 		span.SetTag(ext.ResourceName, cfg.resourceNamer(fctx))
 		status := fctx.Response.StatusCode()
 		if cfg.isStatusError(status) {
-			span.SetTag(ext.Error, fmt.Errorf("%d: %s", status, string(fctx.Response.Body())))
+			span.SetTag(ext.ErrorNoStackTrace, fmt.Errorf("%d: %s", status, string(fctx.Response.Body())))
 		}
 		span.SetTag(ext.HTTPCode, strconv.Itoa(status))
 	}

ddtrace/ext/tags.go

@@ -87,12 +87,19 @@ const (
 	// ErrorType specifies the error type.
 	ErrorType = "error.type"
 
-	// ErrorStack specifies the stack dump.
+	// ErrorStack specifies the stack dump when the error is thrown.
 	ErrorStack = "error.stack"
 
+	// ErrorHandlingStack specifies the stack dump when the error is captured.
+	ErrorHandlingStack = "error.handling_stack"
+
 	// ErrorDetails holds details about an error which implements a formatter.
+	// Deprecated: Use ErrorStack instead. This tag is not supported by Error Tracking.
 	ErrorDetails = "error.details"
 
+	// ErrorNoStackTrace is a tag that specifies that the error stack trace should not be captured.
+	ErrorNoStackTrace = "error.no_stack_trace"
+
 	// Environment specifies the environment to use with a trace.
 	Environment = "env"
 

ddtrace/tracer/span.go

@@ -197,6 +197,11 @@ func (s *Span) SetTag(key string, value interface{}) {
 			noDebugStack: s.noDebugStack,
 		})
 		return
+	case ext.ErrorNoStackTrace:
+		s.setTagError(value, errorConfig{
+			noDebugStack: true,
+		})
+		return
 	case ext.Component:
 		integration, ok := value.(string)
 		if ok {
@@ -430,23 +435,23 @@ func (s *Span) setTagError(value interface{}, cfg errorConfig) {
 		setError(true)
 		s.setMeta(ext.ErrorMsg, v.Error())
 		s.setMeta(ext.ErrorType, reflect.TypeOf(v).String())
+		if cfg.noDebugStack {
+			return
+		}
 		switch err := v.(type) {
 		case xerrors.Formatter:
-			s.setMeta(ext.ErrorDetails, fmt.Sprintf("%+v", v))
+			s.setMeta(ext.ErrorStack, fmt.Sprintf("%+v", v))
 		case fmt.Formatter:
 			// pkg/errors approach
-			s.setMeta(ext.ErrorDetails, fmt.Sprintf("%+v", v))
+			s.setMeta(ext.ErrorStack, fmt.Sprintf("%+v", v))
 		case *errortrace.TracerError:
 			// instrumentation/errortrace approach
-			s.setMeta(ext.ErrorDetails, fmt.Sprintf("%+v", v))
-			if !cfg.noDebugStack {
-				s.setMeta(ext.ErrorStack, err.Format())
-			}
+			s.setMeta(ext.ErrorStack, fmt.Sprintf("%+v", v))
+			s.setMeta(ext.ErrorHandlingStack, err.Format())
 			return
 		}
-		if !cfg.noDebugStack {
-			s.setMeta(ext.ErrorStack, takeStacktrace(cfg.stackFrames, cfg.stackSkip))
-		}
+		stack := takeStacktrace(cfg.stackFrames, cfg.stackSkip)
+		s.setMeta(ext.ErrorHandlingStack, stack)
 	case nil:
 		// no error
 		setError(false)

ddtrace/tracer/span_test.go

@@ -339,7 +339,7 @@ func TestSpanFinishWithError(t *testing.T) {
 	assert.Equal(int32(1), span.error)
 	assert.Equal("test error", span.meta[ext.ErrorMsg])
 	assert.Equal("*errors.errorString", span.meta[ext.ErrorType])
-	assert.NotEmpty(span.meta[ext.ErrorStack])
+	assert.NotEmpty(span.meta[ext.ErrorHandlingStack])
 }
 
 func TestSpanFinishWithErrorNoDebugStack(t *testing.T) {
@@ -365,9 +365,9 @@ func TestSpanFinishWithErrorStackFrames(t *testing.T) {
 	assert.Equal(int32(1), span.error)
 	assert.Equal("test error", span.meta[ext.ErrorMsg])
 	assert.Equal("*errors.errorString", span.meta[ext.ErrorType])
-	assert.Contains(span.meta[ext.ErrorStack], "tracer.TestSpanFinishWithErrorStackFrames")
-	assert.Contains(span.meta[ext.ErrorStack], "tracer.(*Span).Finish")
-	assert.Equal(strings.Count(span.meta[ext.ErrorStack], "\n\t"), 2)
+	assert.Contains(span.meta[ext.ErrorHandlingStack], "tracer.TestSpanFinishWithErrorStackFrames")
+	assert.Contains(span.meta[ext.ErrorHandlingStack], "tracer.(*Span).Finish")
+	assert.Equal(strings.Count(span.meta[ext.ErrorHandlingStack], "\n\t"), 2)
 }
 
 // nilStringer is used to test nil detection when setting tags.
@@ -413,7 +413,7 @@ func TestSpanSetTag(t *testing.T) {
 	assert.Equal(int32(1), span.error)
 	assert.Equal("abc", span.meta[ext.ErrorMsg])
 	assert.Equal("*errors.errorString", span.meta[ext.ErrorType])
-	assert.NotEmpty(span.meta[ext.ErrorStack])
+	assert.NotEmpty(span.meta[ext.ErrorHandlingStack])
 
 	span.SetTag(ext.Error, "something else")
 	assert.Equal(int32(1), span.error)
@@ -523,7 +523,7 @@ func TestSpanSetTagError(t *testing.T) {
 	t.Run("on", func(t *testing.T) {
 		span := newBasicSpan("web.request")
 		span.setTagError(errors.New("error value with trace"), errorConfig{noDebugStack: false})
-		assert.NotEmpty(t, span.meta[ext.ErrorStack])
+		assert.NotEmpty(t, span.meta[ext.ErrorHandlingStack])
 	})
 }
 
@@ -793,10 +793,11 @@ func TestErrorStack(t *testing.T) {
 		assert.Equal("Something wrong", span.meta[ext.ErrorMsg])
 		assert.Equal("*errortrace.TracerError", span.meta[ext.ErrorType])
 
-		stack := span.meta[ext.ErrorStack]
+		stack := span.meta[ext.ErrorHandlingStack]
 		assert.NotEqual("", stack)
 		assert.Contains(stack, "tracer.TestErrorStack")
 		assert.Contains(stack, "tracer.createErrorTrace")
+
 		span.Finish()
 	})
 
@@ -813,10 +814,11 @@ func TestErrorStack(t *testing.T) {
 		assert.Equal("Something wrong", span.meta[ext.ErrorMsg])
 		assert.Equal("*errors.errorString", span.meta[ext.ErrorType])
 
-		stack := span.meta[ext.ErrorStack]
+		stack := span.meta[ext.ErrorHandlingStack]
 		assert.NotEqual("", stack)
 		assert.Contains(stack, "tracer.TestErrorStack")
-		assert.NotContains(stack, "tracer.createTestError") // this checks our old behavior
+		assert.NotContains(stack, "tracer.createTestError")
+
 		span.Finish()
 	})
 }
@@ -835,7 +837,7 @@ func TestSpanError(t *testing.T) {
 	assert.Equal(int32(1), span.error)
 	assert.Equal("Something wrong", span.meta[ext.ErrorMsg])
 	assert.Equal("*errors.errorString", span.meta[ext.ErrorType])
-	assert.NotEqual("", span.meta[ext.ErrorStack])
+	assert.NotEqual("", span.meta[ext.ErrorHandlingStack])
 	span.Finish()
 
 	// operating on a finished span is a no-op
@@ -866,7 +868,7 @@ func TestSpanError_Typed(t *testing.T) {
 	assert.Equal(int32(1), span.error)
 	assert.Equal("boom", span.meta[ext.ErrorMsg])
 	assert.Equal("*tracer.boomError", span.meta[ext.ErrorType])
-	assert.NotEqual("", span.meta[ext.ErrorStack])
+	assert.NotEqual("", span.meta[ext.ErrorHandlingStack])
 }
 
 func TestSpanErrorNil(t *testing.T) {
@@ -978,7 +980,6 @@ func TestSpanErrorStackMetrics(t *testing.T) {
 		}
 
 		assert.Equal(0.0, telemetryClient.Count(telemetry.NamespaceTracers, "errorstack.source", []string{"source:takeStacktrace"}).Get())
-		assert.Equal(0.0, telemetryClient.Distribution(telemetry.NamespaceTracers, "errorstack.duration", []string{"source:takeStacktrace"}).Get())
 
 		assert.Equal(5.0, telemetryClient.Count(telemetry.NamespaceTracers, "errorstack.source", []string{"source:TracerError"}).Get())
 		if !windows {
@@ -987,6 +988,24 @@ func TestSpanErrorStackMetrics(t *testing.T) {
 	})
 }
 
+func TestSpanErrorNoStackTrace(t *testing.T) {
+	t.Run("default", func(t *testing.T) {
+		assert := assert.New(t)
+		tracer, _, _, stop, err := startTestTracer(t, WithDebugStack(true))
+		assert.Nil(err)
+		defer stop()
+
+		span := tracer.StartSpan("operation")
+		span.SetTag(ext.ErrorNoStackTrace, errors.New("test"))
+		span.Finish()
+
+		assert.Equal(int32(1), span.error)
+		assert.Equal("", span.meta[ext.ErrorStack])
+		assert.Equal("test", span.meta[ext.ErrorMsg])
+		assert.Equal("*errors.errorString", span.meta[ext.ErrorType])
+	})
+}
+
 func TestUniqueTagKeys(t *testing.T) {
 	assert := assert.New(t)
 	span := newBasicSpan("web.request")

instrumentation/httptrace/httptrace.go

@@ -161,14 +161,14 @@ func FinishRequestSpan(s *tracer.Span, status int, errorFn func(int) bool, opts
 	if status == 0 {
 		if fn(status) {
 			statusStr = "0"
-			s.SetTag(ext.Error, fmt.Errorf("%s: %s", statusStr, http.StatusText(status)))
+			s.SetTag(ext.ErrorNoStackTrace, fmt.Errorf("%s: %s", statusStr, http.StatusText(status)))
 		} else {
 			statusStr = "200"
 		}
 	} else {
 		statusStr = strconv.Itoa(status)
 		if fn(status) {
-			s.SetTag(ext.Error, fmt.Errorf("%s: %s", statusStr, http.StatusText(status)))
+			s.SetTag(ext.ErrorNoStackTrace, fmt.Errorf("%s: %s", statusStr, http.StatusText(status)))
 		}
 	}
 	fc := &tracer.FinishConfig{}