report: improve the utility of Sentry reports #34

knz · 2020-05-21T22:47:17Z

The recent update to the new Sentry SDK has decreased the utility
compared to previously, where it wasn't even great to start with.

This patch fixes it. (screenshots below)

The most salient improvements:

The bold Sentry issue title now reports the error location
(file/line/function) if available, instead of the constant
string "reported error"
The remainder of the Sentry issue title now lists some more
details about the error type and the first safe detail string
if available, instead of an empty string.
The stack traces are not any more duplicated between Sentry
"Exception" objects and the "Additional data" payloads.
The Message field is now properly populated, like it was prior to
the SDK upgrade.

The improvement can be observed by comparing the two following screenshots side-by-side.

"Before" screenshot:

"After" screenshot:

This change is

yuzefovich

Thanks for working on this! I have one question and several nits.

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @knz and @tbg)

report/report.go, line 135 at r1 (raw file):

	// typesBuf will become the payload for the "error types" Additional
	// data field. It explains the go types of the layers in the error

nit: maybe s/go/Go/g.

report/report.go, line 152 at r1 (raw file):

	var firstDetailLine string

	for i := len(details) - 1; i >= 0; i-- {

Why are we iterating in the reverse order? Maybe if we iterate in the forward order, then we won't need to reverse the exceptions below?

report/report.go, line 155 at r1 (raw file):

		// Collect the type name for this layer of error wrapping, towards
		// the "error types" additional data field.
		fullTypename := details[i].OriginalTypeName

nit: s/Typename/TypeName/g to be inline with OriginalTypeName.

report/report.go, line 159 at r1 (raw file):

		fm := "*"
		if fullTypename != mark.FamilyName {
			// tn can be different from the family when an error type has

nit: s/tn/fullTypename/g.

report/report.go, line 241 at r1 (raw file):

			//
			// Note: we only print the details if no stack trace was found
			// that that level. This is because stack trace annotations also

nit: s/that that/at that/g.

report/report.go, line 294 at r1 (raw file):

	// If there is no exception payload, synthetize one.
	if len(event.Exception) == 0 {
		// We know we don't have a strack trace to extra line/function

nit: s/strack/stack/g

The recent update to the new Sentry SDK has *decreased* the utility compared to previously, where it wasn't even great to start with. This patch fixes it. The most salient improvements: - The bold Sentry issue title now reports the error location (file/line/function) if available, instead of the constant string "`reported error`" - The remainder of the Sentry report title now lists some more details about the error type and the first safe detail string if available, instead of an empty string. - The stack traces are not any more duplicated between Sentry "Exception" objects and the "Additional data" payloads. - The Message field is now properly populated, like it was prior to the SDK upgrade.

knz

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @tbg and @yuzefovich)

report/report.go, line 135 at r1 (raw file):