Support pkg/errors stack traces

[jasonkeene]

Goal

When calling bugsnag.Notify with an error that was created from github.com/pkg/errors package the library currently generates a new stack trace which is mostly useless. I want the library to use the stack trace that is already a part of the error. This matches previous behavior with ErrorWithCallers and ErrorWithStackFrames.

Design

Callers() []uintptr is not defined on github.com/pkg/errors that have stack traces. However, a very similar method StackTrace() errors.StackTrace exists which is basically the callers. It is easy to convert between the two.

Changeset

Added

ErrorWithStackTrace is exposed and part of the errors package to communicate that stack traces from errors created by github.com/pkg/errors are supported.

Changed

Added ErrorWithStackTrace as a case to the type switch for setting stack trace data.

Tests

Added a test for this behavior. I also had to skip a number of tests that were failing on the latest golang version. These were all related to stack text format. I assume this has changed in the past few Go versions.

Review

For the submitter, initial self-review:

• Commented on code changes inline explain the reasoning behind the approach
• Reviewed the test cases added for completeness and possible points for discussion
• A changelog entry was added for the goal of this pull request
• Check the scope of the changeset - is everything in the diff required for the pull request?
• This pull request is ready for:
  • Initial review of the intended approach, not yet feature complete
  • Structural review of the classes, functions, and properties modified
  • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

• Consistency across platforms for structures or concepts added or modified
• Consistency between the changeset and the goal stated above
• Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
• Usage friction - is the proposed change in usage cumbersome or complicated?
• Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
• Concurrency concerns - if components are accessed asynchronously, what issues will arise
• Thoroughness of added tests and any missing edge cases
• Idiomatic use of the language

[mattdyoung]

Hi @jasonkeene

Thanks for the PR! We've been discussing this one and it's something we've been considering supporting in bugsnag-go, but haven't yet been able to schedule the implementation work for.

We'd need to consider this carefully before merging, in particular working out how we'd want to support the package without pulling it in as a dependency for all users.

We'll definitely consider this for a future release, but for the time being I'd suggest forking the library and using that until we have the bandwidth to cover those other considerations and pull this into the library.

[jasonkeene]

@mattdyoung Sounds good. We will fork for the time being.

You might want to have a conversation with the pkg/errors folks about adding a Callers() []uintptr method and then it wouldn't require taking a dependency. One way or another pkg/errors is an extremely popular error library for Go that should be supported.

[kinbiko]

Hi @mattdyoung!

FYI: here's an approach would solve the issue of avoiding new imports.

This approach does have other downsides though, e.g. performance (might not care about that -- probably negligible compared to the network I/O of sending the payload. Profiling/benchmarking required.) and an implicit dependency on what the format of pkg/errors stacktraces look like (can work around that by programming defensively, falling back on the current behaviour).

[mattdyoung]

Hi @kinbiko!

How's it going? Thanks for the suggested approach :-)

[kattrali]

Supported in v1.6.0. Thanks!