Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the structure of sentry reports #94

Merged
merged 3 commits into from
Mar 9, 2022

Conversation

knz
Copy link
Contributor

@knz knz commented Mar 4, 2022

Informs cockroachdb/cockroach#76571.

This PR includes a redacted copy of the verbose error printout in the Message field of Sentry reports.

This, arguably, was desirable from the beginning, but was impossible when the errors library was first designed, because at the time we did not have a reliable redaction library. Now that we have one, we can use it.

The main visible improvement resulting from this PR is the verbose (and tree-preserving) printout of the error hidden behind barriers (e.g. those included behind assertion failures). However, this change will also now include more details about other situations, such as:

  • known Go runtime errors (e.g. context canceled, os.PathError, etc)
  • custom error types that have a safe formatter function even though they don't report safe strings via the SafeDetails interface
  • the tree structure of secondary errors

(and more generally leverages the full power of the recently redesigned errors.FormatError code)

For example, given the starting point Go code:

myErr := errutil.Newf("Hello %s %d", "world", redact.Safe(123))
myErr = errutil.Wrapf(myErr, "some prefix %s", "unseen")
myErr = errutil.NewAssertionErrorWithWrappedErrf(myErr, "assert %s %s", redact.Safe("safe"), "unsafe")

Example, before:
image

Example, after:
image


This change is Reviewable

This commit extracts more safe details for the error hidden behind a
barrier. It achieves this using the new `SafeFormatError()` API via
the `redact` package.

As a side effect, this guarantees that a sentry report for a
barrier (including, but not limited to, assertion failures constructed
with errors.NewAssertionErrorWithWrappedErrf) now always spells out
the type of the error behind the barrier. It also includes any
safe-for-reporting strings that the hidden error object would report
when printed out via `%+v`.

Note that this patch is a band-aid: this is really adding more
complexity to the `SafeDetails()` API, which is arguably somewhat
obsolete now that the `errors` and `redact` packages collaborate to
extract safe strings. However, there is some marginal utility
remaining, in the particular case of an error object transported over
the wire where the target server doesn't know how to decode the error
behind the barrier; in this case, the approach taken here ensures that
a modicum of reportable structure is still included.
Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks! :lgtm:

Reviewed 11 of 11 files at r1, 15 of 15 files at r2, 13 of 13 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @knz)


barriers/barriers.go, line 135 at r3 (raw file):

}

// barrierErr is the "old" type nane of barrierErr.  We use a new

nit: s/barrierErr is/barrierError is/ and s/nane/name/.


report/report_test.go, line 47 at r2 (raw file):

// 	myErr := errutil.Newf("Hello %s %d", "world", redact.Safe(123))
// 	myErr = errutil.Wrapf(myErr, "some prefix %s", "unseen")
// 	// myErr := goErr.New("hai")

nit: this line looks like a possible leftover.

@knz knz force-pushed the 20220304-barrier-details branch from 9b0a953 to 80eef64 Compare March 9, 2022 10:27
Copy link
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yuzefovich)


barriers/barriers.go, line 135 at r3 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: s/barrierErr is/barrierError is/ and s/nane/name/.

Nice catch. fixed.

This commit changes the composition of Sentry reports to include a
redacted verbose printout of the error at the beginning of the main
Message field.

This ensures that error objects with more complex printouts than just
safe detail strings get spelled out more clearly in the sentry
reports.

One example of this is that the tree structure of errors included
behind barriers (e.g. that included behind
errors.NewAssertionErrorWithWrappedErrf) is preserved in the sentry
printout.
@knz knz force-pushed the 20220304-barrier-details branch from 80eef64 to feb9d32 Compare March 9, 2022 10:28
This changes the implementation of barriers to use a redactable string
as message payload. This makes it possible to include more details
(all the safe parts of the message payload) when printing out
the barrier cause.

In order to ensure safety when interacting with previous versions of
the library without this logic, we must be careful to wrap/escape the
message string received when decoding a network error. To force this
additional wrapping, we rename the error type so that it can get a
separate decode function.
@knz knz merged commit 676eef5 into cockroachdb:master Mar 9, 2022
@knz knz deleted the 20220304-barrier-details branch March 9, 2022 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants