diff --git a/client.go b/client.go index 119c92572..93ede2319 100644 --- a/client.go +++ b/client.go @@ -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 @@ -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() diff --git a/client_test.go b/client_test.go index ccff29f5c..276e0c460 100644 --- a/client_test.go +++ b/client_test.go @@ -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" ) @@ -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 { @@ -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) {