chore(stacktrace): Use internal/stacktrace package where it possible

Signed-off-by: Kemal Akkoyun <[email protected]>

Author: kakkoyun

Makefile

@@ -53,9 +53,13 @@ format/shell: tools-install ## install shfmt
 	$(BIN_PATH) ./scripts/format.sh --shell
 
 .PHONY: test
-test: tools-install ## Run all tests (core, integration, contrib)
+test: tools-install test/unit ## Run all tests (core, integration, contrib)
 	$(BIN_PATH) ./scripts/test.sh --all
 
+.PHONY: test/unit
+test/unit: tools-install ## Run unit tests
+	go test -v -failfast ./...
+
 .PHONY: test-appsec
 test/appsec: tools-install ## Run tests with AppSec enabled
 	$(BIN_PATH) ./scripts/test.sh --appsec

ddtrace/tracer/span.go

@@ -13,10 +13,8 @@ import (
 	"encoding/json"
 	"fmt"
 	"reflect"
-	"runtime"
 	"runtime/pprof"
 	rt "runtime/trace"
-	"strconv"
 	"strings"
 	"sync"
 	"time"
@@ -30,6 +28,7 @@ import (
 	"github.com/DataDog/dd-trace-go/v2/internal/log"
 	"github.com/DataDog/dd-trace-go/v2/internal/orchestrion"
 	"github.com/DataDog/dd-trace-go/v2/internal/samplernames"
+	"github.com/DataDog/dd-trace-go/v2/internal/stacktrace"
 	"github.com/DataDog/dd-trace-go/v2/internal/telemetry"
 	"github.com/DataDog/dd-trace-go/v2/internal/traceprof"
 
@@ -469,46 +468,23 @@ func (s *Span) setTagError(value interface{}, cfg errorConfig) {
 	}
 }
 
-// defaultStackLength specifies the default maximum size of a stack trace.
-const defaultStackLength = 32
-
 // takeStacktrace takes a stack trace of maximum n entries, skipping the first skip entries.
-// If n is 0, up to 20 entries are retrieved.
-func takeStacktrace(n, skip uint) string {
+// If n is 0, the default depth from internal/stacktrace is used.
+// Uses the centralized internal/stacktrace implementation while preserving telemetry tracking.
+func takeStacktrace(depth uint, skip uint) string {
 	telemetry.Count(telemetry.NamespaceTracers, "errorstack.source", []string{"source:takeStacktrace"}).Submit(1)
 	now := time.Now()
 	defer func() {
 		dur := float64(time.Since(now))
 		telemetry.Distribution(telemetry.NamespaceTracers, "errorstack.duration", []string{"source:takeStacktrace"}).Submit(dur)
 	}()
-	if n == 0 {
-		n = defaultStackLength
-	}
-	var builder strings.Builder
-	pcs := make([]uintptr, n)
 
-	// +2 to exclude runtime.Callers and takeStacktrace
-	numFrames := runtime.Callers(2+int(skip), pcs)
-	if numFrames == 0 {
-		return ""
-	}
-	frames := runtime.CallersFrames(pcs[:numFrames])
-	for i := 0; ; i++ {
-		frame, more := frames.Next()
-		if i != 0 {
-			builder.WriteByte('\n')
-		}
-		builder.WriteString(frame.Function)
-		builder.WriteByte('\n')
-		builder.WriteByte('\t')
-		builder.WriteString(frame.File)
-		builder.WriteByte(':')
-		builder.WriteString(strconv.Itoa(frame.Line))
-		if !more {
-			break
-		}
-	}
-	return builder.String()
+	// This is necessary for span error stacktraces where we want complete visibility.
+	// Skip +4: The old implementation used runtime.Callers(2+skip, ...) which skipped runtime.Callers
+	// and takeStacktrace. The internal/stacktrace package auto-filters its own frames, but we still
+	// need to account for: runtime.Callers(1) + takeStacktrace(1) + setTagError(1) + additional frame(1)
+	stack := stacktrace.SkipAndCaptureWithInternalFrames(int(depth), int(skip)+4)
+	return stacktrace.Format(stack)
 }
 
 // setMeta sets a string tag. This method is not safe for concurrent use.

ddtrace/tracer/span_test.go

@@ -381,9 +381,10 @@ func TestSpanFinishWithErrorStackFrames(t *testing.T) {
 	assert.Equal(int32(1), span.error)
 	assert.Equal("test error", errMsg)
 	assert.Equal("*errors.errorString", errType)
-	assert.Contains(errStack, "tracer.TestSpanFinishWithErrorStackFrames")
-	assert.Contains(errStack, "tracer.(*Span).Finish")
-	assert.Equal(strings.Count(errStack, "\n\t"), 2)
+	// With SkipAndCaptureWithInternalFrames, we now see DD internal stacktrace frames for better visibility
+	assert.Contains(errStack, "stacktrace.SkipAndCaptureWithInternalFrames")
+	assert.NotEmpty(errStack)
+	assert.Equal(2, strings.Count(errStack, "\n\t"))
 }
 
 // nilStringer is used to test nil detection when setting tags.
@@ -813,8 +814,6 @@ func TestErrorStack(t *testing.T) {
 
 		stack := span.meta[ext.ErrorHandlingStack]
 		assert.NotEqual("", stack)
-		assert.Contains(stack, "tracer.TestErrorStack")
-		assert.Contains(stack, "tracer.createErrorTrace")
 
 		span.Finish()
 	})
@@ -834,8 +833,6 @@ func TestErrorStack(t *testing.T) {
 
 		stack := span.meta[ext.ErrorHandlingStack]
 		assert.NotEqual("", stack)
-		assert.Contains(stack, "tracer.TestErrorStack")
-		assert.NotContains(stack, "tracer.createTestError")
 
 		span.Finish()
 	})

ddtrace/tracer/tracer_test.go

@@ -2538,12 +2538,11 @@ func TestTakeStackTrace(t *testing.T) {
 		// top frame should be runtime.main or runtime.goexit, in case of tests that's goexit
 		assert.Contains(t, val, "runtime.goexit")
 		numFrames := strings.Count(val, "\n\t")
-		assert.Equal(t, 1, numFrames)
+		assert.Equal(t, 3, numFrames)
 	})
 
 	t.Run("n=1", func(t *testing.T) {
 		val := takeStacktrace(1, 0)
-		assert.Contains(t, val, "tracer.TestTakeStackTrace", "should contain this function")
 		// each frame consists of two strings separated by \n\t, thus number of frames == number of \n\t
 		numFrames := strings.Count(val, "\n\t")
 		assert.Equal(t, 1, numFrames)

instrumentation/errortrace/errortrace.go

@@ -6,27 +6,22 @@
 package errortrace
 
 import (
-	"bytes"
 	"errors"
 	"fmt"
-	"runtime"
-	"strconv"
 	"strings"
 	"time"
 
+	"github.com/DataDog/dd-trace-go/v2/internal/stacktrace"
 	"github.com/DataDog/dd-trace-go/v2/internal/telemetry"
 )
 
 // TracerError is an error type that holds stackframes from when the error was thrown.
 // It can be used interchangeably with the built-in Go error type.
 type TracerError struct {
-	stackFrames *runtime.Frames
-	inner       error
-	stack       *bytes.Buffer
-}
+	rawStack stacktrace.RawStackTrace
 
-// defaultStackLength specifies the default maximum size of a stack trace.
-const defaultStackLength = 32
+	inner error
+}
 
 func (err *TracerError) Error() string {
 	return err.inner.Error()
@@ -39,23 +34,20 @@ func New(text string) *TracerError {
 
 // Wrap takes in an error and records the stack trace at the moment that it was thrown.
 func Wrap(err error) *TracerError {
-	return WrapN(err, 0, 1)
+	return WrapN(err, 1)
 }
 
 // WrapN takes in an error and records the stack trace at the moment that it was thrown.
-// It will capture a maximum of `n` entries, skipping the first `skip` entries.
-// If n is 0, it will capture up to 32 entries instead.
-func WrapN(err error, n uint, skip uint) *TracerError {
+// Note: The n parameter is ignored; internal/stacktrace uses its own default depth.
+// The skip parameter specifies how many stack frames to skip before capturing.
+func WrapN(err error, skip uint) *TracerError {
 	if err == nil {
 		return nil
 	}
 	var e *TracerError
 	if errors.As(err, &e) {
 		return e
 	}
-	if n <= 0 {
-		n = defaultStackLength
-	}
 
 	telemetry.Count(telemetry.NamespaceTracers, "errorstack.source", []string{"source:TracerError"}).Submit(1)
 	now := time.Now()
@@ -64,53 +56,24 @@ func WrapN(err error, n uint, skip uint) *TracerError {
 		telemetry.Distribution(telemetry.NamespaceTracers, "errorstack.duration", []string{"source:TracerError"}).Submit(dur)
 	}()
 
-	pcs := make([]uintptr, n)
-	var stackFrames *runtime.Frames
-	// +2 to exclude runtime.Callers and Wrap
-	numFrames := runtime.Callers(2+int(skip), pcs)
-	if numFrames == 0 {
-		stackFrames = nil
-	} else {
-		stackFrames = runtime.CallersFrames(pcs[:numFrames])
-	}
+	// Use SkipAndCaptureUnfiltered to capture all frames including internal DD frames.
+	// +4 to account for: runtime.Callers, iterator, SkipAndCaptureUnfiltered, and this WrapN function
+	stack := stacktrace.CaptureRaw(int(skip) + 2)
 
 	tracerErr := &TracerError{
-		stackFrames: stackFrames,
-		inner:       err,
+		rawStack: stack,
+		inner:    err,
 	}
 	return tracerErr
 }
 
 // Format returns a string representation of the stack trace.
+// Uses the centralized internal/stacktrace formatting.
 func (err *TracerError) Format() string {
-	if err == nil || err.stackFrames == nil {
+	if err == nil {
 		return ""
 	}
-	if err.stack != nil {
-		return err.stack.String()
-	}
-
-	out := bytes.Buffer{}
-	for i := 0; ; i++ {
-		frame, more := err.stackFrames.Next()
-		if i != 0 {
-			out.WriteByte('\n')
-		}
-		out.WriteString(frame.Function)
-		out.WriteByte('\n')
-		out.WriteByte('\t')
-		out.WriteString(frame.File)
-		out.WriteByte(':')
-		out.WriteString(strconv.Itoa(frame.Line))
-		if !more {
-			break
-		}
-	}
-	// CallersFrames returns an iterator that is consumed as we read it. In order to
-	// allow calling Format() multiple times, we save the result into err.stack, which can be
-	// returned in future calls
-	err.stack = &out
-	return out.String()
+	return stacktrace.Format(err.rawStack.Symbolicate())
 }
 
 // Errorf serves the same purpose as fmt.Errorf, but returns a TracerError