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

Added sentry hint with the original errs #37

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ReallyLiri
Copy link

This is very helpful in BeforeSend, to have the actual original error structs to be able for example to filter out net.Error instances

@TheZeroSlave TheZeroSlave requested a review from grongor June 23, 2023 12:53
@TheZeroSlave
Copy link
Owner

TheZeroSlave commented Jun 23, 2023

@ReallyLiri hi, thnx for PR)
@grongor hi, could you help me with reviewing? )

Copy link
Contributor

@costela costela left a comment

Choose a reason for hiding this comment

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

Sorry for the drive-by review by a non-maintainer 😇
I just noticed the PR while working on #40.

Just added a coulpe of thoughts.

@@ -188,25 +189,32 @@ func (c *core) addExceptionsFromError(
err error,
) []sentry.Exception {
for i := 0; i < maxErrorDepth && err != nil; i++ {
wrappedErr := err
err = errors.Cause(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively, why not keep the select logic below, and just keep track of a - possibly nil - previousErr, initialized outside the loop?

this way we need mentally less "special-casing" of this specific lib.

Comment on lines +201 to +202
if strings.HasSuffix(exception.Type, "errors.fundamental") {
// err was created with `errors.New(msg)` - make `msg` the type,
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually see *errors.errString here (see https://goplay.tools/snippet/txizx5D1JSQ).

Is the comment maybe not up-to-date? Or the check?

Also, this type doesn't seem to have ever been renamed since its introduction 12 years ago, but it may still be a bit brittle to check directly against it. Maybe it would be enough to check for any error type inside of errors?

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.

4 participants