From bcd36b13c789316cc0cfec1c82e1a49bbddd2118 Mon Sep 17 00:00:00 2001 From: Eliott Bouhana Date: Fri, 17 May 2024 12:44:12 +0200 Subject: [PATCH 1/3] internal: stacktrace: skip internal packages Signed-off-by: Eliott Bouhana --- go.mod | 1 + go.sum | 2 + internal/stacktrace/event_test.go | 3 +- internal/stacktrace/stacktrace.go | 169 ++++++++++++++++--------- internal/stacktrace/stacktrace_test.go | 19 +-- 5 files changed, 122 insertions(+), 72 deletions(-) diff --git a/go.mod b/go.mod index 84820512e0..7612ea14da 100644 --- a/go.mod +++ b/go.mod @@ -31,6 +31,7 @@ require ( github.com/confluentinc/confluent-kafka-go/v2 v2.2.0 github.com/denisenkom/go-mssqldb v0.11.0 github.com/dimfeld/httptreemux/v5 v5.5.0 + github.com/eapache/queue/v2 v2.0.0-20230407133247-75960ed334e4 github.com/elastic/go-elasticsearch/v6 v6.8.5 github.com/elastic/go-elasticsearch/v7 v7.17.1 github.com/elastic/go-elasticsearch/v8 v8.4.0 diff --git a/go.sum b/go.sum index a9b50ebfca..bc579a77d1 100644 --- a/go.sum +++ b/go.sum @@ -1062,6 +1062,8 @@ github.com/eapache/go-xerial-snappy v0.0.0-20230731223053-c322873962e3 h1:Oy0F4A github.com/eapache/go-xerial-snappy v0.0.0-20230731223053-c322873962e3/go.mod h1:YvSRo5mw33fLEx1+DlK6L2VV43tJt5Eyel9n9XBcR+0= github.com/eapache/queue v1.1.0 h1:YOEu7KNc61ntiQlcEeUIoDTJ2o8mQznoNvUhiigpIqc= github.com/eapache/queue v1.1.0/go.mod h1:6eCeP0CKFpHLu8blIFXhExK/dRa7WDZfr6jVFPTqq+I= +github.com/eapache/queue/v2 v2.0.0-20230407133247-75960ed334e4 h1:8EXxF+tCLqaVk8AOC29zl2mnhQjwyLxxOTuhUazWRsg= +github.com/eapache/queue/v2 v2.0.0-20230407133247-75960ed334e4/go.mod h1:I5sHm0Y0T1u5YjlyqC5GVArM7aNZRUYtTjmJ8mPJFds= github.com/ebitengine/purego v0.6.0-alpha.5 h1:EYID3JOAdmQ4SNZYJHu9V6IqOeRQDBYxqKAg9PyoHFY= github.com/ebitengine/purego v0.6.0-alpha.5/go.mod h1:ah1In8AOtksoNK6yk5z1HTJeUkC1Ez4Wk2idgGslMwQ= github.com/elastic/elastic-transport-go/v8 v8.1.0 h1:NeqEz1ty4RQz+TVbUrpSU7pZ48XkzGWQj02k5koahIE= diff --git a/internal/stacktrace/event_test.go b/internal/stacktrace/event_test.go index 452ec3e0b2..c49a2175b2 100644 --- a/internal/stacktrace/event_test.go +++ b/internal/stacktrace/event_test.go @@ -20,8 +20,7 @@ func TestNewEvent(t *testing.T) { require.Equal(t, ExceptionEvent, event.Category) require.Equal(t, "go", event.Language) require.Equal(t, "message", event.Message) - require.GreaterOrEqual(t, len(event.Frames), 3) - require.Equal(t, "TestNewEvent", event.Frames[0].Function) + require.GreaterOrEqual(t, len(event.Frames), 2) } func TestEventToSpan(t *testing.T) { diff --git a/internal/stacktrace/stacktrace.go b/internal/stacktrace/stacktrace.go index 469540236e..368e41356b 100644 --- a/internal/stacktrace/stacktrace.go +++ b/internal/stacktrace/stacktrace.go @@ -12,9 +12,11 @@ import ( "os" "regexp" "runtime" + "strings" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" + "github.com/eapache/queue/v2" "github.com/hashicorp/go-secure-stdlib/parseutil" ) @@ -22,6 +24,16 @@ var ( enabled = true defaultTopFrameDepth = 8 defaultMaxDepth = 32 + + // internalPackagesPrefixes is the list of prefixes for internal packages that should be hidden in the stack trace + internalSymbolPrefixes = []string{ + "gopkg.in/DataDog/dd-trace-go.v1", + "github.com/DataDog/dd-trace-go", + "github.com/DataDog/go-libddwaf", + "github.com/DataDog/datadog-agent", + "github.com/DataDog/appsec-internal-go", + "github.com/DataDog/orchestrion", + } ) const ( @@ -116,86 +128,121 @@ func Capture() StackTrace { // SkipAndCapture creates a new stack trace from the current call stack, skipping the first `skip` frames func SkipAndCapture(skip int) StackTrace { - // callers() and getRealStackDepth() have to be used side by side to keep the same number of `skip`-ed frames - realDepth := getRealStackDepth(skip, defaultMaxDepth) - callers := callers(skip, realDepth, defaultMaxDepth, defaultTopFrameDepth) - frames := callersFrame(callers, defaultMaxDepth, defaultTopFrameDepth) - stack := make([]StackFrame, len(frames)) - - for i := 0; i < len(frames); i++ { - frame := frames[i] - - // If the top frames are separated from the bottom frames we have to stitch the real index together - frameIndex := i - if frameIndex >= defaultMaxDepth-defaultTopFrameDepth { - frameIndex = realDepth - defaultMaxDepth + i + return skipAndCapture(skip, defaultMaxDepth, internalSymbolPrefixes) +} + +func skipAndCapture(skip int, maxDepth int, symbolSkip []string) StackTrace { + iter := iterator(skip, maxDepth, symbolSkip) + stack := make([]StackFrame, defaultMaxDepth) + nbStoredFrames := 0 + topFramesQueue := queue.New[StackFrame]() + + // We have to make sure we don't store more than maxDepth frames + // if there is more than maxDepth frames, we get X frames from the bottom of the stack and Y from the top + for frame, ok := iter.Next(); ok; frame, ok = iter.Next() { + // we reach the top frames: start to use the queue + if nbStoredFrames >= defaultMaxDepth-defaultTopFrameDepth { + topFramesQueue.Add(frame) + // queue is full, remove the oldest frame + if topFramesQueue.Length() > defaultTopFrameDepth { + topFramesQueue.Remove() + } + continue } - parsedSymbol := parseSymbol(frame.Function) + // Bottom frames: directly store them in the stack + stack[nbStoredFrames] = frame + nbStoredFrames++ + } - stack[i] = StackFrame{ - Index: uint32(frameIndex), - Text: "", - File: frame.File, - Line: uint32(frame.Line), - Column: 0, // No column given by the runtime - Namespace: parsedSymbol.Package, - ClassName: parsedSymbol.Receiver, - Function: parsedSymbol.Function, - } + // Stitch the top frames to the stack + for topFramesQueue.Length() > 0 { + stack[nbStoredFrames] = topFramesQueue.Remove() + nbStoredFrames++ } - return stack + return stack[:nbStoredFrames] } -// getRealStackDepth returns the real depth of the stack, skipping the first `skip` frames -func getRealStackDepth(skip, increment int) int { - pcs := make([]uintptr, increment) +// framesIterator is an iterator over the frames of a call stack +// It skips internal packages and caches the frames to avoid multiple calls to runtime.Callers +// It also skips the first `skip` frames +// It is not thread-safe +type framesIterator struct { + skipPrefixes []string + cache []uintptr + frames *queue.Queue[runtime.Frame] + cacheDepth int + cacheSize int + currDepth int +} - depth := increment - for n := increment; n == increment; depth += n { - n = runtime.Callers(depth+skip, pcs[:]) +func iterator(skip, cacheSize int, internalPrefixSkip []string) framesIterator { + return framesIterator{ + skipPrefixes: internalPrefixSkip, + cache: make([]uintptr, cacheSize), + frames: queue.New[runtime.Frame](), + cacheDepth: skip, + cacheSize: cacheSize, + currDepth: 0, } - - return depth } -// callers returns an array of function pointers of size stackSize, skipping the first `skip` frames -// if realDepth of the current call stack is bigger that stackSize, we return the first stackSize - defaultTopFrameDepth frames -// and the last defaultTopFrameDepth frames of the whole stack -func callers(skip, realDepth, stackSize, topFrames int) []uintptr { - // The stack size is smaller than the max depth, return the whole stack - if realDepth <= stackSize { - pcs := make([]uintptr, realDepth) - runtime.Callers(skip, pcs[:]) - return pcs +// next returns the next runtime.Frame in the call stack, filling the cache if needed +func (it *framesIterator) next() (runtime.Frame, bool) { + if it.frames.Length() == 0 { + n := runtime.Callers(it.cacheDepth, it.cache) + if n == 0 { + return runtime.Frame{}, false + } + + frames := runtime.CallersFrames(it.cache[:n]) + for { + frame, more := frames.Next() + it.frames.Add(frame) + it.cacheDepth++ + if !more { + break + } + } } - // The stack is bigger than the max depth, proceed to find the N start frames and stitch them to the ones we have - pcs := make([]uintptr, stackSize) - runtime.Callers(skip, pcs[:stackSize-topFrames]) - runtime.Callers(skip+realDepth-topFrames, pcs[stackSize-topFrames:]) - return pcs + it.currDepth++ + return it.frames.Remove(), true } -// callersFrame returns an array of runtime.Frame from an array of function pointers -// There can be multiple frames for a single function pointer, so we have to cut things again to make sure the final -// stacktrace is the correct size -func callersFrame(pcs []uintptr, stackSize, topFrames int) []runtime.Frame { - frames := runtime.CallersFrames(pcs) - framesArray := make([]runtime.Frame, 0, len(pcs)) - +// Next returns the next StackFrame in the call stack, skipping internal packages and refurbishing the cache if needed +func (it *framesIterator) Next() (StackFrame, bool) { for { - frame, more := frames.Next() - framesArray = append(framesArray, frame) - if !more { - break + frame, ok := it.next() + if !ok { + return StackFrame{}, false } + + if it.skipSymbol(frame.Function) { + continue + } + + parsedSymbol := parseSymbol(frame.Function) + return StackFrame{ + Index: uint32(it.currDepth - 1), + Text: "", + File: frame.File, + Line: uint32(frame.Line), + Column: 0, // No column given by the runtime + Namespace: parsedSymbol.Package, + ClassName: parsedSymbol.Receiver, + Function: parsedSymbol.Function, + }, true } +} - if len(framesArray) > stackSize { - framesArray = append(framesArray[:stackSize-topFrames], framesArray[len(framesArray)-topFrames:]...) +func (it *framesIterator) skipSymbol(symbol string) bool { + for _, prefix := range it.skipPrefixes { + if strings.HasPrefix(symbol, prefix) { + return true + } } - return framesArray + return false } diff --git a/internal/stacktrace/stacktrace_test.go b/internal/stacktrace/stacktrace_test.go index c6455f222e..730d05325b 100644 --- a/internal/stacktrace/stacktrace_test.go +++ b/internal/stacktrace/stacktrace_test.go @@ -7,13 +7,14 @@ package stacktrace import ( "fmt" - "github.com/stretchr/testify/require" "runtime" "testing" + + "github.com/stretchr/testify/require" ) func TestNewStackTrace(t *testing.T) { - stack := Capture() + stack := skipAndCapture(defaultCallerSkip, defaultMaxDepth, nil) if len(stack) == 0 { t.Error("stacktrace should not be empty") } @@ -21,7 +22,7 @@ func TestNewStackTrace(t *testing.T) { func TestStackTraceCurrentFrame(t *testing.T) { // Check last frame for the values of the current function - stack := Capture() + stack := skipAndCapture(defaultCallerSkip, defaultMaxDepth, nil) require.GreaterOrEqual(t, len(stack), 3) frame := stack[0] @@ -35,7 +36,7 @@ func TestStackTraceCurrentFrame(t *testing.T) { type Test struct{} func (t *Test) Method() StackTrace { - return Capture() + return skipAndCapture(defaultCallerSkip, defaultMaxDepth, nil) } func TestStackMethodReceiver(t *testing.T) { @@ -53,7 +54,7 @@ func TestStackMethodReceiver(t *testing.T) { func recursive(i int) StackTrace { if i == 0 { - return Capture() + return skipAndCapture(defaultCallerSkip, defaultMaxDepth, nil) } return recursive(i - 1) @@ -153,15 +154,15 @@ func TestParseSymbol(t *testing.T) { } } -func recursiveBench(i int, b *testing.B) StackTrace { +func recursiveBench(i int, depth int, b *testing.B) StackTrace { if i == 0 { b.StartTimer() - stack := Capture() + stack := skipAndCapture(defaultCallerSkip, depth*2, nil) b.StopTimer() return stack } - return recursiveBench(i-1, b) + return recursiveBench(i-1, depth, b) } func BenchmarkCaptureStackTrace(b *testing.B) { @@ -169,7 +170,7 @@ func BenchmarkCaptureStackTrace(b *testing.B) { b.Run(fmt.Sprintf("%v", depth), func(b *testing.B) { defaultMaxDepth = depth * 2 // Making sure we are capturing the full stack for n := 0; n < b.N; n++ { - runtime.KeepAlive(recursiveBench(depth, b)) + runtime.KeepAlive(recursiveBench(depth, depth, b)) } }) } From 811ae04fa7aea3bfbf3dead8be27beebbda133a5 Mon Sep 17 00:00:00 2001 From: Eliott Bouhana Date: Fri, 17 May 2024 13:34:23 +0200 Subject: [PATCH 2/3] add method receiver templated test Signed-off-by: Eliott Bouhana --- internal/stacktrace/stacktrace_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/stacktrace/stacktrace_test.go b/internal/stacktrace/stacktrace_test.go index 730d05325b..4187fd06cf 100644 --- a/internal/stacktrace/stacktrace_test.go +++ b/internal/stacktrace/stacktrace_test.go @@ -137,11 +137,16 @@ func TestParseSymbol(t *testing.T) { "*Test", "Method", }}, - {"main-receiver-templated", "test.(*toto).templatedFunc[...]", symbol{ + {"main-receiver-templated-func", "test.(*toto).templatedFunc[...]", symbol{ "test", "*toto", "templatedFunc[...]", }}, + {"main-templated-receiver", "test.(*toto[...]).func", symbol{ + "test", + "*toto[...]", + "func", + }}, {"main-lambda", "test.main.func1", symbol{ "test", "", From cd6ecbc60cc9dd7b632062154971944f885700a3 Mon Sep 17 00:00:00 2001 From: Eliott Bouhana Date: Tue, 21 May 2024 10:57:02 +0200 Subject: [PATCH 3/3] change API of NewEvent to option pattern Signed-off-by: Eliott Bouhana --- internal/stacktrace/event.go | 41 ++++++++++++++++++++----------- internal/stacktrace/event_test.go | 6 +++-- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/internal/stacktrace/event.go b/internal/stacktrace/event.go index 832141cc45..feb1e2c581 100644 --- a/internal/stacktrace/event.go +++ b/internal/stacktrace/event.go @@ -11,7 +11,6 @@ import ( "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" "gopkg.in/DataDog/dd-trace-go.v1/internal" - "github.com/google/uuid" "github.com/tinylib/msgp/msgp" ) @@ -45,28 +44,42 @@ type Event struct { } // NewEvent creates a new stacktrace event with the given category, type and message -func NewEvent(eventCat EventCategory, eventType, message string) *Event { - return &Event{ +func NewEvent(eventCat EventCategory, options ...Options) *Event { + event := &Event{ Category: eventCat, - Type: eventType, Language: "go", - Message: message, Frames: SkipAndCapture(defaultCallerSkip), } + + for _, opt := range options { + opt(event) + } + + return event } -// IDLink returns a UUID to link the stacktrace event with other data. NOT thread-safe -func (e *Event) IDLink() string { - if e.ID != "" { - newUUID, err := uuid.NewUUID() - if err != nil { - return "" - } +// Options is a function type to set optional parameters for the event +type Options func(*Event) + +// WithType sets the type of the event +func WithType(eventType string) Options { + return func(event *Event) { + event.Type = eventType + } +} - e.ID = newUUID.String() +// WithMessage sets the message of the event +func WithMessage(message string) Options { + return func(event *Event) { + event.Message = message } +} - return e.ID +// WithID sets the id of the event +func WithID(id string) Options { + return func(event *Event) { + event.ID = id + } } // AddToSpan adds the event to the given span's root span as a tag if stacktrace collection is enabled diff --git a/internal/stacktrace/event_test.go b/internal/stacktrace/event_test.go index c49a2175b2..5da7febd9c 100644 --- a/internal/stacktrace/event_test.go +++ b/internal/stacktrace/event_test.go @@ -16,10 +16,12 @@ import ( ) func TestNewEvent(t *testing.T) { - event := NewEvent(ExceptionEvent, "", "message") + event := NewEvent(ExceptionEvent, WithMessage("message"), WithType("type"), WithID("id")) require.Equal(t, ExceptionEvent, event.Category) require.Equal(t, "go", event.Language) require.Equal(t, "message", event.Message) + require.Equal(t, "type", event.Type) + require.Equal(t, "id", event.ID) require.GreaterOrEqual(t, len(event.Frames), 2) } @@ -28,7 +30,7 @@ func TestEventToSpan(t *testing.T) { defer mt.Stop() span := ddtracer.StartSpan("op") - event := NewEvent(ExceptionEvent, "", "message") + event := NewEvent(ExceptionEvent, WithMessage("message")) AddToSpan(span, event) span.Finish()