Only use the cause with a stack trace in GetOrNewStacktrace

[martinxsliu]

Following from https://github.com/getsentry/raven-go/pull/155, I realized that always passing in pkg/error's cause into GetOrNewStacktrace doesn't always work. This PR modifies the behaviour of this function so that it gives us the expected stack trace.

For context, the pkg/errors package allows you to create brand new errors using errors.New and errors.Errorf which has a stack trace and does not wrap anything. It also allows you to wrap other errors using errors.Wrap, Wrapf, and WithStack which returns a new error with a stack trace at the point Wrap etc was called, and has the original error as the cause. So if you are using pkg/error in you application, then when raven.CaptureError(err) is called, you end up in one of the following situations:

1. The original error that we're interested in was created by pkg/errors, and may have been wrapped any number of times up the call stack before being passed into CaptureError.
2. The original error was not created by pkg/errors, but was wrapped by it.
3. The original error was neither created nor wrapped by pkg/errors.

To provide the most context to the viewer of the error on Sentry, you would want to include the stack trace information as close to the original error as possible. For the scenarios above you want the stack trace at the point where:

1. The original error was created.
2. The original error was first wrapped by pkg/errors.
3. CaptureError is called (default behaviour without pkg/errors).

The issue that this PR addresses is that naively using errors.Cause(err) will return the original error which, in the case of (2), won't always contain the stack trace that we want. Instead I added a new private function causeWithStacktrace which finds the innermost error that does contain a stack trace. In the case (1) this returns the original error, in the case (2) this returns the first wrapped error, and in the case (3) this returns nil (since no layers contain a stack trace).

CaptureError and CaptureErrorAndWait now passes the error it receives into GetOrNewStacktrace which calls causeWithStacktrace. This PR also includes some go fmt changes and lint fixes.

[johngibb]

Any news on merging this? I'm running into the same situation—I'm calling errors.Wrap(...) on a bare error that came from a panic, and losing the stack trace since sentry only sends the stack for errors.Cause.

[nvartolomei]

There is a better way to do this. Sentry supports chained exceptions. Attached a screenshot of how this looks in Sentry UI.

The class for sending chained exceptions is already defined.

Instead of arguing whether we should capture a new stack trace, recover the last one or try to get a stack trace for the cause, why not send the whole chain?

func exceptions(err error, includePaths []string) (es Exceptions) {
    // build chained stacktrace,
    // as long as errors have an underlying cause
    for {
        // we're trying to recover stack traces added by pkg/errors,
        // it is meaningless at this point to create stack traces
        // since we don't care about CaptureError caller
        stackTrace := recoverPkgErrorsStacktrace(err, 3, includePaths)

        es.Values = append(es.Values, NewException(err, stackTrace))

        // reverse exceptions, this is the order sentry expects them
        // see https://docs.sentry.io/clientdev/interfaces/exception/
        for i := len(es.Values)/2 - 1; i >= 0; i-- {
            opp := len(es.Values) - 1 - i
            es.Values[i], es.Values[opp] = es.Values[opp], es.Values[i]
        }

        if withCause, ok := err.(causer); ok {
            err = withCause.Cause()
        } else {
            break
        }
    }

    // if newest error does not have a stack trace, we're creating one
    // it will be point to location where raven.Capture was called
    last := es.Values[len(es.Values)-1]
    if last.Stacktrace == nil {
        last.Stacktrace = NewStacktrace(3, 3, includePaths)
    }

    return
}

Where recoverPkgErrorsStacktrace is extracted out of GetOrNewStacktrace and is defined as

func recoverPkgErrorsStacktrace(err error, context int, appPackagePrefixes []string) *Stacktrace {
    stacktracer, errHasStacktrace := err.(interface {
        StackTrace() errors.StackTrace
    })
    if !errHasStacktrace {
        return nil
    }

    var frames []*StacktraceFrame
    for _, f := range stacktracer.StackTrace() {
        pc := uintptr(f) - 1
        fn := runtime.FuncForPC(pc)
        var file string
        var line int
        if fn != nil {
            file, line = fn.FileLine(pc)
        } else {
            file = "unknown"
        }
        frame := NewStacktraceFrame(pc, file, line, context, appPackagePrefixes)
        if frame != nil {
            frames = append([]*StacktraceFrame{frame}, frames...)
        }
    }
    return &Stacktrace{Frames: frames}
}

GetOrNewStacktrace is left for backwards compatibility as follows

func GetOrNewStacktrace(err error, skip int, context int, appPackagePrefixes []string) *Stacktrace {
    pkgErrorsStacktrace := recoverPkgErrorsStacktrace(err, context, appPackagePrefixes)

    if pkgErrorsStacktrace != nil {
        return pkgErrorsStacktrace
    } else {
        return NewStacktrace(skip+1, context, appPackagePrefixes)
    }
}

and new CaptureError + CaptureErrorAndWait should be updated in similar manner

func (client *Client) CaptureError(err error, tags map[string]string, interfaces ...Interface) string {
    if client == nil {
        return ""
    }

    if err == nil {
        return ""
    }

    if client.shouldExcludeErr(err.Error()) {
        return ""
    }

    packet := NewPacketWithExtra(
        err.Error(),
        extractExtra(err),
        append(
            append(interfaces, client.context.interfaces()...),
            exceptions(err, client.includePaths),
        )...,
    )
    eventID, _ := client.Capture(packet, tags)

    return eventID
}