Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3036c4d
chore(stacktrace): Use internal/stacktrace package where it possible
kakkoyun Oct 22, 2025
dfb0b1a
chore(docs): Update documentation
kakkoyun Oct 22, 2025
8ba9e8b
Merge branch 'main' into kakkoyun/use_stacktrace_package
kakkoyun Oct 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ Targets:
format Format code
format/shell install shfmt
test Run all tests (core, integration, contrib)
test/unit Run unit tests
test/appsec Run tests with AppSec enabled
test/contrib Run contrib package tests
test/integration Run integration tests
Expand Down
44 changes: 10 additions & 34 deletions ddtrace/tracer/span.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,8 @@ import (
"encoding/json"
"fmt"
"reflect"
"runtime"
"runtime/pprof"
rt "runtime/trace"
"strconv"
"strings"
"sync"
"time"
Expand All @@ -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"

Expand Down Expand Up @@ -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.
Expand Down
11 changes: 4 additions & 7 deletions ddtrace/tracer/span_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -811,8 +812,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()
})
Expand All @@ -832,8 +831,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()
})
Expand Down
3 changes: 1 addition & 2 deletions ddtrace/tracer/tracer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
69 changes: 16 additions & 53 deletions instrumentation/errortrace/errortrace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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()
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kakkoyun Are the comment and the actual value out of sync?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to update the comment, good catch. SkipAndCaptureUnfiltered doesn't exist anymore.

Comment on lines +59 to +61
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Honor skip parameter when wrapping errors

The new WrapN implementation always calls stacktrace.CaptureRaw(int(skip) + 2) before symbolication. Because the stack collection now has an extra wrapper (CaptureRaw) compared to the old direct runtime.Callers call, the caller’s requested skip is now off by one: skip=1 returns a stack that still starts at the first helper rather than the helper’s caller. This regresses behaviour for any code that relies on skip to remove wrapper functions (e.g. WrapN(err, 1) should report the frame that invoked the helper). The inline comment even says “+4 … and this WrapN function” but only +2 is applied, confirming the mismatch. To preserve existing semantics, the capture should skip WrapN itself as well (skip+3 or skip+4 as documented) so the first frame corresponds to the requested depth.

Useful? React with 👍 / 👎.


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
Expand Down
Loading
Loading