Repository: rollbar/rollbar-go
Pull request #53 (2019-02-19 22:22:05)

Add support for pkg/errors stack trace extraction
This PR concludes my current efforts with rollbar-go by adding the [discussed-about ](https://github.com/rollbar/rollbar-go/issues/45#issuecomment-453301866) support for another interface, beyond CauseStacker, to get pre-made stack traces from errors. It builds upon the previous PRs I submitted (#51 and #52).

Using these three PRs it's now a bit easier to continue adding support for other interfaces, for those who wish. The one downside to adding support for external interfaces that utilize custom structs (such as `stackTracer` from [pkg/errors](https://github.com/pkg/errors)) is the added dependencies. E.g. with this PR, rollbar-go now has a 3rd party dependency compared to before when it had zero.

A solution to 3rd party dependency cluttering could be to instead add a function property that the user can set, that will receive the error and that should return `[]runtime.Frame` or nil (in case it does not know how to extract the stack trace). If this function is set, it is called by `getOrBuildFrames` to retrieve the stack trace if possible. That way, rollbar-go would not need any more dependencies. However, obviously you lose out on the plug n' play aspect.

Nonetheless, this code is currently in use in production and works well in terms of propagating the pkg/errors stack traces to rollbar instead of generating new ones.

jessewgibbs: @fgblomqvist thanks for the PR.

@rokob can you take a look?
rokob: I'm not sure we want to take this dependency. I have to think about it. Let's rebase this on the current master now that the other changes are merged so we can see the unique set of changes here. I am leaning more towards your suggestion of having a configurable function to call to do the err -> stack extraction.
fgblomqvist: @rokob Rebased it. Afaik the travis failure is unrelated to this PR. Anyway, I agree with you. I don't think this PR is the optimal solution. I think parts of it are useful in terms of refactoring (e.g. the changes to the `getCause` function as well as the extraction of the `framesToSlice` function) but that could just be done in relation to a future PR anyway. I'm not sure if I'll have time the coming weeks to work on the better solution (a configurable function, as discussed) but it shouldn't be a big task. If I find time, I'll happily do it, unless someone else gets to it before me. So yeah, up to you what you want to do with this, whether you want to leave it open until a better solution comes along or just close it (or for some reason merge it). I take no offense no matter what :)

Patch:
diff --git a/client.go b/client.go
index 4bd3359..ddc1bc6 100644
--- a/client.go
+++ b/client.go
@@ -162,6 +162,14 @@ func (c *Client) SetTransform(transform func(map[string]interface{})) {
 	c.configuration.transform = transform
 }
 
+// SetStackTracer sets the stackTracer function which is called to extract the stack
+// trace from enhanced error types. Return nil if no trace information is available.
+// Return true if the error type can be handled and false otherwise.
+// This feature can be used to add support for custom error type stack trace extraction.
+func (c *Client) SetStackTracer(stackTracer func(err error) ([]runtime.Frame, bool)) {
+	c.configuration.stackTracer = stackTracer
+}
+
 // SetCheckIgnore sets the checkIgnore function which is called during the recovery
 // process of a panic that occurred inside a function wrapped by Wrap or WrapAndWait.
 // Return true if you wish to ignore this panic, false if you wish to
@@ -543,6 +551,7 @@ type configuration struct {
 	scrubFields  *regexp.Regexp
 	checkIgnore  func(string) bool
 	transform    func(map[string]interface{})
+	stackTracer  func(error) ([]runtime.Frame, bool)
 	person       Person
 	captureIp    captureIp
 }
diff --git a/errors/errors.go b/errors/errors.go
new file mode 100644
index 0000000..daddaa8
--- /dev/null
+++ b/errors/errors.go
@@ -0,0 +1,44 @@
+package errors
+
+import (
+	"runtime"
+
+	pkgerr "github.com/pkg/errors"
+)
+
+// StackTracer is able to extract stack traces from pkg/errors.
+func StackTracer(err error) ([]runtime.Frame, bool) {
+	type stackTracer interface {
+		StackTrace() pkgerr.StackTrace
+	}
+
+	switch x := err.(type) {
+	case stackTracer:
+		st := x.StackTrace()
+		pcs := make([]uintptr, len(st))
+		for i, pc := range st {
+			pcs[i] = uintptr(pc)
+		}
+		fr := runtime.CallersFrames(pcs)
+
+		return framesToSlice(fr), true
+	}
+
+	return nil, false
+}
+
+// framesToSlice extracts all the runtime.Frame from runtime.Frames.
+// This function has been copied from transform.go in rollbar-go.
+func framesToSlice(fr *runtime.Frames) []runtime.Frame {
+	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 frames
+}
diff --git a/errors/go.mod b/errors/go.mod
new file mode 100644
index 0000000..befc1a6
--- /dev/null
+++ b/errors/go.mod
@@ -0,0 +1,3 @@
+module github.com/rollbar/rollbar-go/errors
+
+require github.com/pkg/errors v0.8.1
diff --git a/errors/go.sum b/errors/go.sum
new file mode 100644
index 0000000..f29ab35
--- /dev/null
+++ b/errors/go.sum
@@ -0,0 +1,2 @@
+github.com/pkg/errors v0.8.1 h1:iURUrRGxPUNPdy5/HRSm+Yj6okJ6UtLINN0Q9M4+h3I=
+github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
diff --git a/go.sum b/go.sum
new file mode 100644
index 0000000..e69de29
diff --git a/rollbar.go b/rollbar.go
index 11970de..750cc16 100644
--- a/rollbar.go
+++ b/rollbar.go
@@ -126,6 +126,15 @@ func SetTransform(transform func(map[string]interface{})) {
 	std.SetTransform(transform)
 }
 
+// SetStackTracer sets the stackTracer function on the managed Client instance.
+// StackTracer is called to extract the stack trace from enhanced error types.
+// Return nil if no trace information is available. Return true if the error type
+// can be handled and false otherwise.
+// This feature can be used to add support for custom error type stack trace extraction.
+func SetStackTracer(stackTracer func(err error) ([]runtime.Frame, bool)) {
+	std.SetStackTracer(stackTracer)
+}
+
 // SetCheckIgnore sets the checkIgnore function on the managed Client instance.
 // CheckIgnore is called during the recovery process of a panic that
 // occurred inside a function wrapped by Wrap or WrapAndWait.
diff --git a/transforms.go b/transforms.go
index e15bdb6..c267984 100644
--- a/transforms.go
+++ b/transforms.go
@@ -211,7 +211,7 @@ func errorBody(configuration configuration, err error, skip int) (map[string]int
 	traceChain := []map[string]interface{}{}
 	fingerprint := ""
 	for {
-		stack := buildStack(getOrBuildFrames(err, parent, 1+skip))
+		stack := buildStack(getOrBuildFrames(err, parent, 1+skip, configuration.stackTracer))
 		traceChain = append(traceChain, buildTrace(err, stack))
 		if configuration.fingerprint {
 			fingerprint = fingerprint + stack.Fingerprint()
@@ -242,28 +242,58 @@ func buildTrace(err error, stack stack) map[string]interface{} {
 }
 
 func getCause(err error) error {
-	if cs, ok := err.(CauseStacker); ok {
+	type causer interface {
+		Cause() error
+	}
+
+	if cs, ok := err.(causer); ok {
 		return cs.Cause()
 	}
 	return nil
 }
 
-// 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 {
-		return cs.Stack()
-	} else if _, ok := parent.(CauseStacker); !ok {
-		return getCallersFrames(1 + skip)
+// getOrBuildFrames gets stack frames from errors that provide one of their own
+// otherwise, it builds a new stack trace. It returns the stack frames if the error
+// is of a compatible type. If the error is not, but the parent error is, it assumes
+// the parent error will be processed later and therefore returns nil.
+func getOrBuildFrames(err error, parent error, skip int,
+	tracer func(error) ([]runtime.Frame, bool)) []runtime.Frame {
+
+	switch x := err.(type) {
+	case CauseStacker:
+		return x.Stack()
+	default:
+		if tracer != nil {
+			if st, ok := tracer(err); ok && st != nil {
+				return st
+			}
+		}
 	}
 
-	return nil
+	switch parent.(type) {
+	case CauseStacker:
+		return nil
+	default:
+		if tracer != nil {
+			if _, ok := tracer(parent); ok {
+				return nil
+			}
+		}
+	}
+
+	return getCallersFrames(1 + skip)
 }
 
 func getCallersFrames(skip int) []runtime.Frame {
 	pc := make([]uintptr, 100)
 	runtime.Callers(2+skip, pc)
 	fr := runtime.CallersFrames(pc)
+
+	return framesToSlice(fr)
+}
+
+// framesToSlice extracts all the runtime.Frame from runtime.Frames.
+func framesToSlice(fr *runtime.Frames) []runtime.Frame {
 	frames := make([]runtime.Frame, 0)
 
 	for frame, more := fr.Next(); frame != (runtime.Frame{}); frame, more = fr.Next() {


Test patch:
diff --git a/errors/errors_test.go b/errors/errors_test.go
new file mode 100644
index 0000000..c612e02
--- /dev/null
+++ b/errors/errors_test.go
@@ -0,0 +1,49 @@
+package errors
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+
+	pkgerr "github.com/pkg/errors"
+)
+
+func TestStackTracerOfPkgErrorsWithoutParent(t *testing.T) {
+	err := pkgerr.New("")
+	frs, ok := StackTracer(err)
+	if !ok {
+		t.Errorf("got: unsupported type")
+	}
+
+	fr := frs[0]
+	if !strings.HasSuffix(fr.File, "rollbar-go/errors/errors_test.go") {
+		t.Errorf("got: %s", fr.File)
+	}
+	if fr.Function != "github.com/rollbar/rollbar-go/errors.TestStackTracerOfPkgErrorsWithoutParent" {
+		t.Errorf("got: %s", fr.Function)
+	}
+	if fr.Line != 12 {
+		t.Errorf("got: %d", fr.Line)
+	}
+}
+
+func TestStackTracerOfPkgErrorsWithParent(t *testing.T) {
+	cause := fmt.Errorf("cause")
+	effect := pkgerr.Wrap(cause, "effect")
+	effect2 := pkgerr.Wrap(effect, "effect2")
+	frs, ok := StackTracer(effect2)
+	if !ok {
+		t.Errorf("got: unsupported type")
+	}
+
+	fr := frs[0]
+	if !strings.HasSuffix(fr.File, "rollbar-go/errors/errors_test.go") {
+		t.Errorf("got: %s", fr.File)
+	}
+	if fr.Function != "github.com/rollbar/rollbar-go/errors.TestStackTracerOfPkgErrorsWithParent" {
+		t.Errorf("got: %s", fr.Function)
+	}
+	if fr.Line != 33 {
+		t.Errorf("got: %d", fr.Line)
+	}
+}
diff --git a/rollbar_test.go b/rollbar_test.go
index 6ddcfdd..722716e 100644
--- a/rollbar_test.go
+++ b/rollbar_test.go
@@ -416,7 +416,7 @@ func TestGetCauseOfCauseStacker(t *testing.T) {
 
 func TestGetOrBuildStackOfStdErrWithoutParent(t *testing.T) {
 	err := cs{fmt.Errorf(""), nil, getCallersFrames(0)}
-	if nil == getOrBuildFrames(err, nil, 0) {
+	if nil == getOrBuildFrames(err, nil, 0, nil) {
 		t.Error("should build stack if parent is not a CauseStacker")
 	}
 }
@@ -424,7 +424,7 @@ func TestGetOrBuildStackOfStdErrWithoutParent(t *testing.T) {
 func TestGetOrBuildStackOfStdErrWithParent(t *testing.T) {
 	cause := fmt.Errorf("cause")
 	effect := cs{fmt.Errorf("effect"), cause, getCallersFrames(0)}
-	if 0 != len(getOrBuildFrames(cause, effect, 0)) {
+	if 0 != len(getOrBuildFrames(cause, effect, 0, nil)) {
 		t.Error("should return empty stack of stadard error if parent is CauseStacker")
 	}
 }
@@ -435,7 +435,7 @@ func TestGetOrBuildStackOfCauseStackerWithoutParent(t *testing.T) {
 	if len(effect.Stack()) == 0 {
 		t.Fatal("stack should not be empty")
 	}
-	if effect.Stack()[0] != getOrBuildFrames(effect, nil, 0)[0] {
+	if effect.Stack()[0] != getOrBuildFrames(effect, nil, 0, nil)[0] {
 		t.Error("should use stack from effect")
 	}
 }
@@ -444,7 +444,7 @@ func TestGetOrBuildStackOfCauseStackerWithParent(t *testing.T) {
 	cause := fmt.Errorf("cause")
 	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] {
+	if effect2.Stack()[0] != getOrBuildFrames(effect2, effect, 0, nil)[0] {
 		t.Error("should use stack from effect2")
 	}
 }