Add support for pkg/errors stack trace extraction

[fgblomqvist]

This PR concludes my current efforts with rollbar-go by adding the discussed-about 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) 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 :)

[hazcod]

Hi @fgblomqvist are there still pans for merging this? I'm a potential rollbar user, but I really need stacktrace extraction for debugging.

[fgblomqvist]

@hazcod I believe the decision that was (essentially) made was to not merge this, but rather come up with a different solution where the user of rollbar-go can write a hook function that does the extraction any way they like it. The reason is to not take on a 3rd-party dependency (while pkg/errors is pretty much universally used, not everyone uses it). We have used this PR in production for several months without a problem, so in case you want to utilize it until a proper solution has been created and merged, it should be safe to do so. I'm not a maintainer of this package so can't make any merging-decisions.

[mback2k]

@rokob Could you please give us an update on your thoughts about this? I would really like to see this or an alternative solution merged to be able to make use of heroku/rollrus#24.

[mback2k]

@fgblomqvist I just posted a sub-PR with updates to this PR with my interpretation of the alternative approach discussed here.

[fgblomqvist]

@rokob the PR has been updated to reflect the changes that @mback2k did. It is now more modular, according to the idea I originally had but does still include a fully functional pkg/errors module that can be used if one wants to. What do you think?

[rokob]

I think this looks good, pkg/errors is pretty much universal at this point. cc: @brianr

[mback2k]

I hope this PR makes it into master before #64. @rokob @brianr Would you mind taking a look?

[waltjones]

I can take a look when I review the other PR. @rokob has moved on to other projects.

[bvisness]

So we now have a half-interface and half-function approach to unwrapping and stack traces. Errors still have to implement Cause() or Unwrap() (or Just Work, as in pkg/errors because it has Cause()), but stack traces require you to call SetStackTracer.

It seems to me like there are several ways of doing error chaining:

• Using an interface, looking for Cause() error
• Using the standard Unwrap() error
• Using a function like SetErrorUnwrapper(error) (error, bool), analogous to SetStackTracker

And multiple ways of doing stack traces:

• Using an interface, looking for Stack() []runtime.Frame
• Using a function like SetStackTracer

The function options are the most general and can work with any error type, including those from third-party packages. But existing users of this library are used to using CauseStacker and making sure their custom errors implement that interface. From a migration perspective, the function approach requires extra configuration in each application's Rollbar client, but the interface approach allows all users of the custom error type to get tracing and unwrapping behavior "for free".

So, which of these do we want to support? All of them? And which of them should potentially be supported but deprecated now that Go provides a standard way of unwrapping errors?

To simplify this situation, I think you could have the implementation always use the function approach, but provide default functions that check the interfaces. For example:

type Stacker interface {
    Stack() []runtime.Frame
}

type UnwrapperFunc func(error) error
type TracerFunc func(error) ([]runtime.Frame, bool)

// The client uses the following functions by default:

func DefaultUnwrapperFunc(err error) error {
    type causer interface {
        Cause() error
    }

    if c, ok := err.(causer); ok {
        return c.Cause()
    }
    if unwrapped := xerrors.Unwrap(err); unwrapped != nil {
        return unwrapped
    }
    return nil
}

func DefaultTracerFunc(err error) ([]runtime.Frame, bool) {
    if s, ok := err.(Stacker); ok {
        return s.Stack(), true
    }

    return nil, false
}

// The defaults can be overridden with the following:

func SetErrorUnwrapper(unwrapper UnwrapperFunc) {
    // set it
}

func SetStackTracer(stackTracer TracerFunc) {
    // set it
}

// transforms.go can now just use the unwrapper and stackTracer functions
// to keep the logic simpler.

We could also deprecate CauseStacker to encourage people to use Unwrap() instead for error types they can control.

I'd like @waltjones's, @fgblomqvist's, and @mback2k's thoughts on this.

[bvisness]

I'm happy to modify my work in #64 to accomplish whatever we decide is best.

[waltjones]

@bvisness For any of the Rollbar SDKs, we try to minimize dependencies, enable the widest range of language and framework versions, and preserve back compatibility in the SDK.

Go is unique in that the language environment is still evolving quickly, full error functionality isn't built in, and the errors package is still at 0.x, with features both being added and removed from one version to the next.

Here are the things #53 does that are a good model for this situation:

• The pkg/errors dependency is optional.
• The previous behavior remains the default, no code changes required.
• Other solutions are enabled if they provide the same interface, not just the author's use case.

As a bonus, errors/errors.go is a good template for anyone who needs to customize their own solution.

I like the idea behind your work supporting errors.Unwrap. The above points are what would make it the most useful for everyone.

[bvisness]

The pkg/errors being discussed here is not the standard library errors package I was referring to. The new errors.Unwrap method is part of Go 1.13 and is thus subject to their compatibility guarantees, whereas pkg/errors is a separate thing on GitHub that is at version 0.x and not part of the standard library.

I believe my proposal maintains backwards compatibility, but we can move further discussion over to #64 to avoid cluttering this.