Skip to content

Commit

Permalink
feat: Report chain of errors when available (#185)
Browse files Browse the repository at this point in the history 
Instead of reporting only the top parent of an error chain, report all errors that are part of the chain.
  • Loading branch information
@rhcarvalho
rhcarvalho authored Apr 16, 2020
1 parent 7bf3e16 commit 6aa4e2b
Show file tree
Hide file tree
Showing 2 changed files with 195 additions and 73 deletions.
59 changes: 41 additions & 18 deletions client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,15 @@ import (
"time"
)

// maxErrorDepth is the maximum number of errors reported in a chain of errors.
// This protects the SDK from an arbitrarily long chain of wrapped errors.
//
// An additional consideration is that arguably reporting a long chain of errors
// is of little use when debugging production errors with Sentry. The Sentry UI
// is not optimized for long chains either. The top-level error together with a
// stack trace is often the most useful information.
const maxErrorDepth = 10

// usageError is used to report to Sentry an SDK usage error.
//
// It is not exported because it is never returned by any function or method in
Expand Down Expand Up @@ -306,34 +315,48 @@ func (client *Client) eventFromMessage(message string, level Level) *Event {
}

func (client *Client) eventFromException(exception error, level Level) *Event {
if exception == nil {
exception = usageError{fmt.Errorf("%s called with nil error", callerFunctionName())}
err := exception
if err == nil {
err = usageError{fmt.Errorf("%s called with nil error", callerFunctionName())}
}

stacktrace := ExtractStacktrace(exception)
event := &Event{Level: level}

if stacktrace == nil {
stacktrace = NewStacktrace()
for i := 0; i < maxErrorDepth && err != nil; i++ {
event.Exception = append(event.Exception, Exception{
Value: err.Error(),
Type: reflect.TypeOf(err).String(),
Stacktrace: ExtractStacktrace(err),
})
if previous, ok := err.(interface{ Cause() error }); ok {
err = previous.Cause()
} else {
err = nil
}
}

cause := exception
// Handle wrapped errors for github.com/pingcap/errors and github.com/pkg/errors
if ex, ok := exception.(interface{ Cause() error }); ok {
if c := ex.Cause(); c != nil {
cause = c
}
// Add a trace of the current stack to the most recent error in a chain if
// it doesn't have a stack trace yet.
// We only add to the most recent error to avoid duplication and because the
// current stack is most likely unrelated to errors deeper in the chain.
if event.Exception[0].Stacktrace == nil {
event.Exception[0].Stacktrace = NewStacktrace()
}

event := NewEvent()
event.Level = level
event.Exception = []Exception{{
Value: cause.Error(),
Type: reflect.TypeOf(cause).String(),
Stacktrace: stacktrace,
}}
// event.Exception should be sorted such that the most recent error is last.
reverse(event.Exception)

return event
}

// reverse reverses the slice a in place.
func reverse(a []Exception) {
for i := len(a)/2 - 1; i >= 0; i-- {
opp := len(a) - 1 - i
a[i], a[opp] = a[opp], a[i]
}
}

func (client *Client) processEvent(event *Event, hint *EventHint, scope EventModifier) *EventID {
options := client.Options()

Expand Down
209 changes: 154 additions & 55 deletions client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ package sentry
import (
"errors"
"testing"
"time"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
pkgErrors "github.com/pkg/errors"
)

Expand Down Expand Up @@ -59,54 +59,12 @@ func TestCaptureMessageShouldSucceedWithoutNilScope(t *testing.T) {
assertEqual(t, transport.lastEvent.Message, "foo")
}

func TestCaptureExceptionShouldSendEventWithProvidedError(t *testing.T) {
client, scope, transport := setupClientTest()
client.CaptureException(errors.New("custom error"), nil, scope)
assertEqual(t, transport.lastEvent.Exception[0].Type, "*errors.errorString")
assertEqual(t, transport.lastEvent.Exception[0].Value, "custom error")
}

func TestCaptureExceptionShouldNotFailWhenPassedNil(t *testing.T) {
client, scope, transport := setupClientTest()
client.CaptureException(nil, nil, scope)
want := &Event{
Level: "error",
Platform: "go",
Exception: []Exception{
{
Type: "sentry.usageError",
Value: "CaptureException called with nil error",
},
},
}
opts := cmp.Options{
cmpopts.IgnoreFields(Event{}, "EventID", "Sdk", "ServerName", "Timestamp"),
cmpopts.IgnoreTypes(&Stacktrace{}),
cmpopts.EquateEmpty(),
}
if diff := cmp.Diff(want, transport.lastEvent, opts); diff != "" {
t.Errorf("event mismatch (-want +got):\n%s", diff)
}
if transport.lastEvent.Exception[0].Stacktrace == nil {
t.Errorf("missing stacktrace")
}
}

type customErr struct{}

func (e *customErr) Error() string {
return "wat"
}

func TestCaptureExceptionShouldExtractCorrectTypeAndValueForWrappedErrors(t *testing.T) {
client, scope, transport := setupClientTest()
cause := &customErr{}
err := pkgErrors.WithStack(cause)
client.CaptureException(err, nil, scope)
assertEqual(t, transport.lastEvent.Exception[0].Type, "*sentry.customErr")
assertEqual(t, transport.lastEvent.Exception[0].Value, "wat")
}

type customErrWithCause struct{ cause error }

func (e *customErrWithCause) Error() string {
Expand All @@ -117,20 +75,161 @@ func (e *customErrWithCause) Cause() error {
return e.cause
}

func TestCaptureExceptionShouldNotUseCauseIfCauseIsNil(t *testing.T) {
client, scope, transport := setupClientTest()
err := &customErrWithCause{cause: nil}
client.CaptureException(err, nil, scope)
assertEqual(t, transport.lastEvent.Exception[0].Type, "*sentry.customErrWithCause")
assertEqual(t, transport.lastEvent.Exception[0].Value, "err")
type captureExceptionTestGroup struct {
name string
tests []captureExceptionTest
}

func TestCaptureExceptionShouldUseCauseIfCauseIsNotNil(t *testing.T) {
client, scope, transport := setupClientTest()
err := &customErrWithCause{cause: &customErr{}}
client.CaptureException(err, nil, scope)
assertEqual(t, transport.lastEvent.Exception[0].Type, "*sentry.customErr")
assertEqual(t, transport.lastEvent.Exception[0].Value, "wat")
type captureExceptionTest struct {
name string
err error
want []Exception
}

func TestCaptureException(t *testing.T) {
basicTests := []captureExceptionTest{
{
name: "NilError",
err: nil,
want: []Exception{
{
Type: "sentry.usageError",
Value: "CaptureException called with nil error",
Stacktrace: &Stacktrace{Frames: []Frame{}},
},
},
},
{
name: "SimpleError",
err: errors.New("custom error"),
want: []Exception{
{
Type: "*errors.errorString",
Value: "custom error",
Stacktrace: &Stacktrace{Frames: []Frame{}},
},
},
},
}

errorChainTests := []captureExceptionTest{
{
name: "MostRecentErrorHasStack",
err: pkgErrors.WithStack(&customErr{}),
want: []Exception{
{
Type: "*sentry.customErr",
Value: "wat",
// No Stacktrace, because we can't tell where the error came
// from and because we have a stack trace in the most recent
// error in the chain.
},
{
Type: "*errors.withStack",
Value: "wat",
Stacktrace: &Stacktrace{Frames: []Frame{}},
},
},
},
{
name: "ChainWithNilCause",
err: &customErrWithCause{cause: nil},
want: []Exception{
{
Type: "*sentry.customErrWithCause",
Value: "err",
Stacktrace: &Stacktrace{Frames: []Frame{}},
},
},
},
{
name: "ChainWithoutStacktrace",
err: &customErrWithCause{cause: &customErr{}},
want: []Exception{
{
Type: "*sentry.customErr",
Value: "wat",
},
{
Type: "*sentry.customErrWithCause",
Value: "err",
Stacktrace: &Stacktrace{Frames: []Frame{}},
},
},
},
}

tests := []captureExceptionTestGroup{
{
name: "Basic",
tests: basicTests,
},
{
name: "ErrorChain",
tests: errorChainTests,
},
}

for _, grp := range tests {
for _, tt := range grp.tests {
tt := tt
t.Run(grp.name+"/"+tt.name, func(t *testing.T) {
client, _, transport := setupClientTest()
client.CaptureException(tt.err, nil, nil)
if transport.lastEvent == nil {
t.Fatal("missing event")
}
got := transport.lastEvent.Exception
if diff := cmp.Diff(tt.want, got); diff != "" {
t.Errorf("Event mismatch (-want +got):\n%s", diff)
}
})
}
}
}

func TestCaptureEvent(t *testing.T) {
client, _, transport := setupClientTest()

eventID := EventID("0123456789abcdef")
timestamp := time.Now().UTC()
serverName := "testServer"

client.CaptureEvent(&Event{
EventID: eventID,
Timestamp: timestamp,
ServerName: serverName,
}, nil, nil)

if transport.lastEvent == nil {
t.Fatal("missing event")
}
want := &Event{
EventID: eventID,
Timestamp: timestamp,
ServerName: serverName,
Level: LevelInfo,
Platform: "go",
Sdk: SdkInfo{
Name: "sentry.go",
Version: Version,
Integrations: []string{},
Packages: []SdkPackage{
{
// FIXME: name format doesn't follow spec in
// https://docs.sentry.io/development/sdk-dev/event-payloads/sdk/
Name: "sentry-go",
Version: Version,
},
// TODO: perhaps the list of packages is incomplete or there
// should not be any package at all.
},
},
}
got := transport.lastEvent
if diff := cmp.Diff(want, got); diff != "" {
t.Errorf("Event mismatch (-want +got):\n%s", diff)
}
}

func TestCaptureEventShouldSendEventWithProvidedError(t *testing.T) {
Expand Down

0 comments on commit 6aa4e2b

Please sign in to comment.