Use runtime.Frames instead of rollbar.Stack in CauseStacker interface

[rollbar]

Use runtime.Frames instead of rollbar.Stack in CauseStacker interface

[rokob]

The purpose of the CauseStacker interface is primarily for the Cause part rather than the Stack part as it is the only way to chain errors. I see what you mean about losing the error class name, this can be alleviated by making an error wrapper which returns the original error as the Cause as we would then surface both the original error and the wrapper in the UI as they would become part of a chain.

I think the easiest thing to do here is actually to support multiple interfaces, it isn't that hard and would provide some flexibility.

If you have a type that implements part of the interface, i.e. just the Stack part, with a different Stack type than rollbar.Stack like runtime.Frames, then making a wrapper around that to turn runtime.Frames into rollbar.Stack should be straight forward and would only require the package that is actually reporting to Rollbar, which already has the dependency, to know about the rollbar package types.

Let me see what I can put together to support multiple options for providing your own stack. I have an example with a wrapper type that I will clean up and put in the repo too which shows how to get the original error class name along with the stack.

[jessewgibbs]

@dofelt thanks for the suggestion. Our primary maintainer for the Go SDK will be out for a couple of weeks, but when he gets back I'll ask him to take a look at this suggestion.

[dolfelt]

@jessewgibbs Thanks! I'm more than happy to help out if you think it's something that would benefit the SDK. For now I've just created an intermediate struct that implements the CauseStacker interface to avoid the dependency.

[fgblomqvist]

Any update on this?

[jessewgibbs]

@fgblomqvist this is still in our backlog. We're glad to accept PRs if someone out there wants to implement it :)

[fgblomqvist]

@jessewgibbs just wrote some code to work my way around it, which naturally felt less than optimal.
I'd happily do the change, I'll take a look tonight to see if I feel like I know what's going on (but no promises) :)

Update: Took a look at it and did some work. Doesn't seem to bad. I'll post an update next week (won't have much time to work on it for the rest of this week).

[fgblomqvist]

Until this gets fixed/changed, most likely every error generated by an application will have to be wrapped/aliased in order to satisfy CauseStacker (due to the rollbar.Stack type). But even once it's been changed (to the proposed runtime.Frames), most errors will still have to be wrapped/aliased since I've personally not seen many/if any errors that has a "Stack()" method that returns precisely that type. The issue I have with wrapping/aliasing is that the error class name gets lost in the process (on top of the fact that I have to do it to begin with). While it may not be the most important information, I do think it is still nice (and tbh it is shown in Rollbar for a reason).

Now, I get the purpose of checking if the error already has a stack (to simplify in some cases), and that this has to be done by checking for some interface compliance, but I guess I don't really see the point of defining an own interface for this (since it, like I mentioned, will most likely help very few if any people right out-of-box). This leads me to wonder if it's better to just utilize an existing interface that at least a few thousand (if not significantly more) applications already rely on. The most popular error package I know of is pkg/errors and it has a stackTracer interface (that while not exported, "it is considered a part of its stable public interface."):

type Frame uintptr
type StackTrace []Frame

type stackTracer interface {
        StackTrace() errors.StackTrace
}

Then, people can wrap/alias their other error types (that have stack traces) to satisfy this one instead. Alternatively, rollbar-go can go the direction of supporting multiple interfaces? E.g. on top of it's own Stack (if that is still considered necessary), it could support errors.stackTracer, and maybe more in the future.

Yet another solution is to (perhaps drop Stack and) allow passing of a stack trace separately, similar to how stvp/roll used to do it/is doing it.

Would love some input on these thoughts from @jessewgibbs and/or @dolfelt :)

[fgblomqvist]

Did not think about having the wrapper report the original error as the final cause, smart :)

Other than that, sounds good. Not sure if it's a part of your plan, but maybe keep the cause and the stack interface separate (similar to how pkg/errors handles it)? Since they fulfill separate purposes. E.g. one "causer" interfacer and one "stacker/stacktracer" interface (or rather multiple, like we just talked about). I think a lot of error types satisfy a "causer" interface automatically since it seems universal for that to be "Cause() error". But yeah then the user could fiddle to satisfy one of the "stacker" interfaces (if not already satisfied).

[fgblomqvist]

@rokob Just to clarify: with what you said, what's the status of this issue? I'm still willing to continue/do the PR for swapping Stack for runtime.Frames, but just wanna make sure you don't have in mind to keep CauseStacker as-is and just allow other interfaces.

If Stack is still going away (or rather, I'm keeping it internally in my fork, with the sole purpose of building a JSON serializable struct from a runtime.Frames at the end of the flow). I do find runtime.Frames a bit awkward to work with since it's a more dynamic/linked-list-similar structure compared to a normal array/slice, but I still think it's better than exporting Stack. That being said, maybe it's better/easier to just go with some alternative, such as []runtime.Frame or perhaps just a []uintptr and then call runtime.CallersFrames on it before we transform it to a JSON serializable struct?

[fgblomqvist]

This issue can be closed, I believe.