Add support for chained errors #185

rhcarvalho · 2020-03-23T21:10:19Z

Instead of reporting only the top parent of an error chain, report all errors that are part of the chain.

Supports chains of errors using the Cause method (used by popular error packages predating Go 1.13).

Support for errors using the Unwrap method (introduced in Go 1.13) or a combination of Unwrap and Cause will come briefly in a follow up.

Fixes #176.

Example report of chained errors

rhcarvalho · 2020-04-15T11:41:24Z

client_test.go

-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")
-}


This got replaced by the test SimpleError below.

rhcarvalho · 2020-04-15T11:42:07Z

client_test.go

-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")
-	}
-}


This got replaced by the test NilError below.

rhcarvalho · 2020-04-15T11:42:47Z

client_test.go

-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")
-}


This got replaced by the test MostRecentErrorHasStack below.

rhcarvalho · 2020-04-15T11:43:29Z

client_test.go

-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")


This got replaced with the test ChainWithNilCause below.

rhcarvalho · 2020-04-15T11:44:23Z

client_test.go

-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")


This got replaced with the test ChainWithoutStacktrace below.

rhcarvalho · 2020-04-15T11:46:34Z

client_test.go

+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)
+	}


This is notably not a very good test, but a starting point to add tests around code that fills in event fields.

It is so that TestCaptureException can focus on the list of "exceptions" (errors) instead of dealing with stuff like SDK info, Platform, etc.

rhcarvalho · 2020-04-15T11:46:57Z

client_test.go

+			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.
+			},


This is something we need to review in the future.

bruno-garcia

Got a lot to contribute here.

bruno-garcia · 2020-04-15T15:13:14Z

client.go

 	return event
 }

+// reverse reverses the slice a in place.


Suggested change

// reverse reverses the slice a in place.

// reverse reverses the slice in place.

Not a typo, a is the name of the argument :)

I have 0 to contribute.
Maybe useless questions: Is there really no built-in generic array reversal function in Go? Oh, no generic? (I mean, it's an array, forget generics)
Also, what about renaming a -> exceptions or, e?
And finally:

Suggested change

// reverse reverses the slice a in place.

// reverse reverses the slice 'a' in place.

kamilogorek · 2020-04-16T11:05:51Z

client.go

 	return event
 }

+// reverse reverses the slice a in place.
+func reverse(a []Exception) {


Ref: https://github.com/golang/go/wiki/SliceTricks#reversing :D

kamilogorek · 2020-04-16T11:08:14Z

client_test.go

+
+	for _, grp := range tests {
+		for _, tt := range grp.tests {
+			tt := tt


Is it for closure scoping purpose?

Yes. Because tt is in the scope of the subtest, having a new variable for each test prevents running into https://github.com/golang/go/wiki/CommonMistakes and allows us to run tests in parallel if we wanted to.

The observed behavior in the current code base today would be the same without this line, but I like to have it to help whomever will maintain this later.

It is easy to imagine some contribution to make tests run in parallel (not a priority today because they are pretty fast anyway), the tests would be green and we would not realize that we're only running the last test case over and over...

feat: Report chain of errors when available

7348755

rhcarvalho force-pushed the chained-errors branch from 56815be to 7348755 Compare April 15, 2020 11:34

rhcarvalho commented Apr 15, 2020

View reviewed changes

rhcarvalho marked this pull request as ready for review April 15, 2020 11:47

rhcarvalho requested a review from kamilogorek April 15, 2020 11:47

bruno-garcia reviewed Apr 15, 2020

View reviewed changes

kamilogorek reviewed Apr 16, 2020

View reviewed changes

kamilogorek approved these changes Apr 16, 2020

View reviewed changes

rhcarvalho merged commit 6aa4e2b into getsentry:master Apr 16, 2020

rhcarvalho deleted the chained-errors branch April 16, 2020 12:08

gukz mentioned this pull request May 8, 2020

Fix event init in eventFromException #220

Merged

rhcarvalho mentioned this pull request Jun 3, 2020

feat: Extract correct type and value from top-most error #85

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for chained errors #185

Add support for chained errors #185

rhcarvalho commented Mar 23, 2020 •

edited

Loading

rhcarvalho Apr 15, 2020

rhcarvalho Apr 15, 2020

rhcarvalho Apr 15, 2020

rhcarvalho Apr 15, 2020

rhcarvalho Apr 15, 2020

rhcarvalho Apr 15, 2020

rhcarvalho Apr 15, 2020

bruno-garcia left a comment

bruno-garcia Apr 15, 2020

rhcarvalho Apr 15, 2020

bruno-garcia Apr 15, 2020 •

edited

Loading

kamilogorek Apr 16, 2020

kamilogorek Apr 16, 2020

rhcarvalho Apr 16, 2020

	// reverse reverses the slice a in place.
	// reverse reverses the slice in place.

	// reverse reverses the slice a in place.
	// reverse reverses the slice 'a' in place.

Add support for chained errors #185

Add support for chained errors #185

Conversation

rhcarvalho commented Mar 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-garcia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-garcia Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhcarvalho commented Mar 23, 2020 •

edited

Loading

bruno-garcia Apr 15, 2020 •

edited

Loading