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

Send error details to Sentry #1166

Merged
merged 2 commits into from
Mar 19, 2024
Merged

Send error details to Sentry #1166

merged 2 commits into from
Mar 19, 2024

Conversation

daniellemaxwell
Copy link
Collaborator

@daniellemaxwell daniellemaxwell commented Mar 18, 2024

Scope of changes

Fixes Object captured as exception Sentry error.

Type of change

  • bug fix
  • new feature
  • documentation
  • other (describe)

Acceptance criteria

Describe how reviewers can test this change to be sure that it works correctly. Add a checklist if possible

Author checklist

  • I have manually tested the change and/or added automation in the form of unit tests or integration tests
  • I have updated the dependencies list
  • I have recompiled and included new protocol buffers to reflect changes I made
  • I have added new test fixtures as needed to support added tests
  • Check this box if a reviewer can merge this pull request after approval (leave it unchecked if you want to do it yourself)
  • I have moved the associated Shortcut story to "Ready for Review"

Reviewer(s) checklist

  • Any new user-facing content that has been added for this PR has been QA'ed to ensure correct grammar, spelling, and understandability.

@@ -99,7 +99,7 @@ const StartPage: React.FC = () => {
}

// catch this error in sentry
Sentry.captureException(err);
Sentry.captureException(err?.original);
Copy link
Collaborator Author

@daniellemaxwell daniellemaxwell Mar 18, 2024

Choose a reason for hiding this comment

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

After reading this comment, it seems that Sentry didn't think err is an error object: getsentry/sentry-react-native#391 (comment)

err?.original returns an object inside the error object with more details, so this could potentially resolve the issue.

I should note that if a user enters incorrect log in details, they see an error message explaining that the username or password is invalid. This change will not affect that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When it comes to the ?. syntax, what will be passed into captureException if the original property is not on err? Probably we would just want to pass in err in that case, right?

I may just be unfamiliar with ?. but how does this code differ?

try {
    Sentry.captureException(err);
} catch {
     Sentry.captureException(err?.original);
}

or is it more like:

if (err?.original) {
    Sentry.captureException(err?.original);
} else {
    Sentry.captureException(err);
}

Or is it that we can always guarantee that there is an err.original on the error object?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

err returns an object with the error, error code, error description, and another object (original) that contains the error and error description.
Screenshot 2024-03-19 at 2 50 25 PM

?. will check if err is undefined before attempting to read what's inside the original object. It was added to prevent an undefined error from being returned.

If original isn't included, then err wouldn't be passed into the exception. I didn't include an else because we currently pass err into Sentry.captureException and get an error message. I thought possibly passing in this object directly could be helpful. The error didn't trigger in development mode for me to confirm if this is truly the solution.

From what I can see, err.original will be included in the err object. However, this is coming from Auth0 so I can't quite guarantee this won't ever change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I see now that this is already in a try/catch block for Auth0 specific handling. Thank you for clarifying this!

Copy link
Collaborator

@bbengfort bbengfort left a comment

Choose a reason for hiding this comment

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

Thanks for making this fix!

@daniellemaxwell daniellemaxwell merged commit 54fa187 into main Mar 19, 2024
15 checks passed
@daniellemaxwell daniellemaxwell deleted the sc-24411 branch March 19, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants