Repository: rollbar/rollbar-go
Pull request #52 (2019-02-14 22:23:52)

Use []runtime.Frame instead of exporting Stack (#45)
This is my proposed solution to issue #45. I have tried to preserve as much as possible of the existing code structure to make it a less impactful change, but naturally it will be a breaking one no matter what. I concluded that it made the most sense to use `[]runtime.Frame` instead of e.g. `runtime.Frames` or `[]uintptr`. A runtime.Frame essentially holds the same information as the old `Frame`, with some additions.

With this change in place, it will be easier to extend the package to be able to get stack traces from other error interfaces, as well as to be able to take a stack trace straight (e.g. similar to what https://github.com/stvp/roll allows for).

While the tests are passing, I have yet to test it during some real usage. I'm planning on doing that over the next few days/week, and will report back once I know how it went (assuming this PR is still of interest).

**For now, it depends on #51** simply because that's the kind of environment I've worked in. If that one isn't of interest, it shouldn't be hard to just rebase this onto master (and is something that I'll of course do in that case).

Open to feedback if there is any change/piece of code that isn't up to par/to your liking! :)


Patch:
diff --git a/doc.go b/doc.go
index 3ef8b09..996100f 100644
--- a/doc.go
+++ b/doc.go
@@ -66,9 +66,9 @@ Due to the nature of the `error` type in Go, it can be difficult to attribute er
     type CauseStacker interface {
       error
       Cause() error
-      Stack() Stack
+      Stack() []runtime.Frame
     }
 
-One can implement this interface for custom Error types to be able to build up a chain of stack traces. In order to get stack the correct stacks, callers must call BuildStack on their own at the time that the cause is wrapped. This is the least intrusive mechanism for gathering this information due to the decisions made by the Go runtime to not track this information.
+One can implement this interface for custom Error types to be able to build up a chain of stack traces. In order to get the correct stack, callers are required to call runtime.Callers and build the runtime.Frame slice on their own at the time the cause is wrapped. This is the least intrusive mechanism for gathering this information due to the decisions made by the Go runtime to not track this information.
 */
 package rollbar
diff --git a/rollbar.go b/rollbar.go
index 7c4c0e6..30f24f3 100644
--- a/rollbar.go
+++ b/rollbar.go
@@ -5,6 +5,7 @@ import (
 	"net/http"
 	"os"
 	"regexp"
+	"runtime"
 )
 
 const (
@@ -519,9 +520,10 @@ func WrapAndWait(f func()) interface{} {
 }
 
 // CauseStacker is an interface that errors can implement to create a trace_chain.
-// Callers are required to call BuildStack on their own at the time the cause is wrapped.
+// Callers are required to call runtime.Callers and build the runtime.Frame slice
+// on their own at the time the cause is wrapped.
 type CauseStacker interface {
 	error
 	Cause() error
-	Stack() Stack
+	Stack() []runtime.Frame
 }
diff --git a/stack.go b/stack.go
index 27873a7..8d19d77 100644
--- a/stack.go
+++ b/stack.go
@@ -17,8 +17,8 @@ var (
 	}
 )
 
-// Frame represents one frame in a stack trace
-type Frame struct {
+// frame represents one frame in a stack trace
+type frame struct {
 	// Filename is the name of the file for this frame
 	Filename string `json:"filename"`
 	// Method is the name of the method for this frame
@@ -27,21 +27,16 @@ type Frame struct {
 	Line int `json:"lineno"`
 }
 
-// A Stack is a slice of frames
-type Stack []Frame
+// A stack is a slice of frames
+type stack []frame
 
-// BuildStack uses the Go runtime to construct a slice of frames optionally skipping the number of
-// frames specified by the input skip argument.
-func BuildStack(skip int) Stack {
-	stack := make(Stack, 0)
+// buildStack converts []runtime.Frame into a JSON serializable slice of frames
+func buildStack(frames []runtime.Frame) stack {
+	stack := make(stack, len(frames))
 
-	for i := skip; ; i++ {
-		pc, file, line, ok := runtime.Caller(i)
-		if !ok {
-			break
-		}
-		file = shortenFilePath(file)
-		stack = append(stack, Frame{file, functionName(pc), line})
+	for i, fr := range frames {
+		file := shortenFilePath(fr.File)
+		stack[i] = frame{file, functionName(fr.Function), fr.Line}
 	}
 
 	return stack
@@ -51,7 +46,7 @@ func BuildStack(skip int) Stack {
 // callstack, including file names. That ensure that there are no false duplicates
 // but also means that after changing the code (adding/removing lines), the
 // fingerprints will change. It's a trade-off.
-func (s Stack) Fingerprint() string {
+func (s stack) Fingerprint() string {
 	hash := crc32.NewIEEE()
 	for _, frame := range s {
 		fmt.Fprintf(hash, "%s%s%d", frame.Filename, frame.Method, frame.Line)
@@ -80,12 +75,10 @@ func shortenFilePath(s string) string {
 	return s
 }
 
-func functionName(pc uintptr) string {
-	fn := runtime.FuncForPC(pc)
-	if fn == nil {
+func functionName(pcFuncName string) string {
+	if pcFuncName == "" {
 		return "???"
 	}
-	name := fn.Name()
-	end := strings.LastIndex(name, string(os.PathSeparator))
-	return name[end+1:]
+	end := strings.LastIndex(pcFuncName, string(os.PathSeparator))
+	return pcFuncName[end+1:]
 }
diff --git a/transforms.go b/transforms.go
index 3af36c1..158566c 100644
--- a/transforms.go
+++ b/transforms.go
@@ -8,6 +8,7 @@ import (
 	"net/url"
 	"reflect"
 	"regexp"
+	"runtime"
 	"strings"
 	"time"
 )
@@ -191,10 +192,11 @@ func filterIp(ip string, captureIp captureIp) string {
 // method, the causes will be traversed until nil.
 func errorBody(configuration configuration, err error, skip int) (map[string]interface{}, string) {
 	var parent error
+	// allocate the slice at all times since it will get marshaled into JSON later
 	traceChain := []map[string]interface{}{}
 	fingerprint := ""
 	for {
-		stack := getOrBuildStack(err, parent, skip)
+		stack := buildStack(getOrBuildFrames(err, parent, 1 + skip))
 		traceChain = append(traceChain, buildTrace(err, stack))
 		if configuration.fingerprint {
 			fingerprint = fingerprint + stack.Fingerprint()
@@ -210,7 +212,7 @@ func errorBody(configuration configuration, err error, skip int) (map[string]int
 }
 
 // builds one trace element in trace_chain
-func buildTrace(err error, stack Stack) map[string]interface{} {
+func buildTrace(err error, stack stack) map[string]interface{} {
 	message := nilErrTitle
 	if err != nil {
 		message = err.Error()
@@ -231,20 +233,33 @@ func getCause(err error) error {
 	return nil
 }
 
-// gets Stack from errors that provide one of their own
-// otherwise, builds a new stack
-func getOrBuildStack(err error, parent error, skip int) Stack {
+// gets stack frames from errors that provide one of their own
+// otherwise, builds a new stack trace
+func getOrBuildFrames(err error, parent error, skip int) []runtime.Frame {
 	if cs, ok := err.(CauseStacker); ok {
-		if s := cs.Stack(); s != nil {
-			return s
-		}
-	} else {
-		if _, ok := parent.(CauseStacker); !ok {
-			return BuildStack(4 + skip)
+		return cs.Stack()
+	} else if _, ok := parent.(CauseStacker); !ok {
+		return getCallersFrames(1 + skip)
+	}
+
+	return nil
+}
+
+func getCallersFrames(skip int) []runtime.Frame {
+	pc := make([]uintptr, 100)
+	runtime.Callers(2 + skip, pc)
+	fr := runtime.CallersFrames(pc)
+	frames := make([]runtime.Frame, 0)
+
+	for frame, more := fr.Next(); frame != (runtime.Frame{}); frame, more = fr.Next() {
+		frames = append(frames, frame)
+
+		if !more {
+			break
 		}
 	}
 
-	return make(Stack, 0)
+	return frames
 }
 
 // Build a message inner-body for the given message string.


Test patch:
diff --git a/rollbar_test.go b/rollbar_test.go
index c9e292b..bf86c9f 100644
--- a/rollbar_test.go
+++ b/rollbar_test.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"net/http"
 	"os"
+	"runtime"
 	"testing"
 )
 
@@ -361,14 +362,14 @@ func TestFilterFlatten(t *testing.T) {
 type cs struct {
 	error
 	cause error
-	stack Stack
+	stack []runtime.Frame
 }
 
 func (cs cs) Cause() error {
 	return cs.cause
 }
 
-func (cs cs) Stack() Stack {
+func (cs cs) Stack() []runtime.Frame {
 	return cs.stack
 }
 
@@ -387,33 +388,36 @@ func TestGetCauseOfCauseStacker(t *testing.T) {
 }
 
 func TestGetOrBuildStackOfStdErrWithoutParent(t *testing.T) {
-	err := cs{fmt.Errorf(""), nil, BuildStack(0)}
-	if nil == getOrBuildStack(err, nil, 0) {
+	err := cs{fmt.Errorf(""), nil, getCallersFrames(0)}
+	if nil == getOrBuildFrames(err, nil, 0) {
 		t.Error("should build stack if parent is not a CauseStacker")
 	}
 }
 
 func TestGetOrBuildStackOfStdErrWithParent(t *testing.T) {
 	cause := fmt.Errorf("cause")
-	effect := cs{fmt.Errorf("effect"), cause, BuildStack(0)}
-	if 0 != len(getOrBuildStack(cause, effect, 0)) {
+	effect := cs{fmt.Errorf("effect"), cause, getCallersFrames(0)}
+	if 0 != len(getOrBuildFrames(cause, effect, 0)) {
 		t.Error("should return empty stack of stadard error if parent is CauseStacker")
 	}
 }
 
 func TestGetOrBuildStackOfCauseStackerWithoutParent(t *testing.T) {
 	cause := fmt.Errorf("cause")
-	effect := cs{fmt.Errorf("effect"), cause, BuildStack(0)}
-	if effect.Stack()[0] != getOrBuildStack(effect, nil, 0)[0] {
+	effect := cs{fmt.Errorf("effect"), cause, getCallersFrames(0)}
+	if len(effect.Stack()) == 0 {
+		t.Fatal("stack should not be empty")
+	}
+	if effect.Stack()[0] != getOrBuildFrames(effect, nil, 0)[0] {
 		t.Error("should use stack from effect")
 	}
 }
 
 func TestGetOrBuildStackOfCauseStackerWithParent(t *testing.T) {
 	cause := fmt.Errorf("cause")
-	effect := cs{fmt.Errorf("effect"), cause, BuildStack(0)}
-	effect2 := cs{fmt.Errorf("effect2"), effect, BuildStack(0)}
-	if effect.Stack()[0] != getOrBuildStack(effect2, effect, 0)[0] {
+	effect := cs{fmt.Errorf("effect"), cause, getCallersFrames(0)}
+	effect2 := cs{fmt.Errorf("effect2"), effect, getCallersFrames(0)}
+	if effect2.Stack()[0] != getOrBuildFrames(effect2, effect, 0)[0] {
 		t.Error("should use stack from effect2")
 	}
 }
@@ -441,8 +445,8 @@ func TestErrorBodyWithoutChain(t *testing.T) {
 
 func TestErrorBodyWithChain(t *testing.T) {
 	cause := fmt.Errorf("cause")
-	effect := cs{fmt.Errorf("effect1"), cause, BuildStack(0)}
-	effect2 := cs{fmt.Errorf("effect2"), effect, BuildStack(0)}
+	effect := cs{fmt.Errorf("effect1"), cause, getCallersFrames(0)}
+	effect2 := cs{fmt.Errorf("effect2"), effect, getCallersFrames(0)}
 	errorBody, fingerprint := errorBody(configuration{fingerprint: true}, effect2, 0)
 	if nil != errorBody["trace"] {
 		t.Error("should not have trace element")
@@ -463,7 +467,8 @@ func TestErrorBodyWithChain(t *testing.T) {
 	if "cause" != traces[2]["exception"].(map[string]interface{})["message"] {
 		t.Error("chain should contain cause third")
 	}
-	if effect2.Stack().Fingerprint()+effect.Stack().Fingerprint()+"0" != fingerprint {
+
+	if buildStack(effect2.Stack()).Fingerprint()+buildStack(effect.Stack()).Fingerprint()+"0" != fingerprint {
 		t.Error("fingerprint should be the fingerprints in chain concatenated together. got: ", fingerprint)
 	}
 }
diff --git a/stack_test.go b/stack_test.go
index 8a1c5ef..2420fdf 100644
--- a/stack_test.go
+++ b/stack_test.go
@@ -6,7 +6,8 @@ import (
 )
 
 func TestBuildStack(t *testing.T) {
-	frame := BuildStack(1)[0]
+	frame := buildStack(getCallersFrames(0))[0]
+
 	if !strings.HasSuffix(frame.Filename,"rollbar-go/stack_test.go") {
 		t.Errorf("got: %s", frame.Filename)
 	}
@@ -21,25 +22,25 @@ func TestBuildStack(t *testing.T) {
 func TestStackFingerprint(t *testing.T) {
 	tests := []struct {
 		Fingerprint string
-		Stack       Stack
+		Stack       stack
 	}{
 		{
 			"9344290d",
-			Stack{
-				Frame{"foo.go", "Oops", 1},
+			stack{
+				frame{"foo.go", "Oops", 1},
 			},
 		},
 		{
 			"a4d78b7",
-			Stack{
-				Frame{"foo.go", "Oops", 2},
+			stack{
+				frame{"foo.go", "Oops", 2},
 			},
 		},
 		{
 			"50e0fcb3",
-			Stack{
-				Frame{"foo.go", "Oops", 1},
-				Frame{"foo.go", "Oops", 2},
+			stack{
+				frame{"foo.go", "Oops", 1},
+				frame{"foo.go", "Oops", 2},
 			},
 		},
 	}