Skip to content

Commit

Permalink
fix: filter SES from Sentry stack trace frames (#8584)
Browse files Browse the repository at this point in the history
## **Description**

SES is shown at the top of the stack trace, but we don't have a lockdown
(repairIntrinsics) option to hide it yet

this simply removes the SES frame from the Sentry error event stack
trace

we identify this with ease by the `filename`, rather than the verbose
`context_line`

here's an example of a SES Sentry error event stack trace frame that we
remove:

```
    {
      "colno":25,
      "context_line":"        error=  construct(FERAL_ERROR, rest, new.target);",
      "filename":"app:///ses.cjs",
      "function":"Error",
      "in_app":true,
      "lineno":7575,
      "platform":"javascript",
      "post_context":[
        "Array"
      ],
      "pre_context":[
        "Array"
      ]
    },
```

- https://docs.sentry.io/platforms/javascript/configuration/filtering
- https://github.com/endojs/endo/blob/master/packages/ses/docs/reference.md#options-quick-reference

## **Related issues**

Fixes: #8586

## **Manual testing steps**

- create personal Sentry account
- run Sentry wizard to add/update `sentry.properties` files
- update `app/util/sentry/utils.js` `Sentry.init({dsn: xxx})` with your
own

+ for local testing, enable SES in debug-mode
([here](https://github.com/MetaMask/metamask-mobile/blob/main/patches/react-native%2B0.71.15.patch#L94))
+ test a handled `Sentry.captureException('test')`
+ test a `Sentry.nativeCrash()`
+ test an unhandled `throw new Error('test')`

- observe error on sentry.io
- `Error(ses)` no longer seen
- `app:///ses.cjs` frame no longer present in stack trace
- `app:///ses.cjs` still present in breadcrumbs

## **Screenshots/Recordings**

### **Before**

<img width="303" alt="Screenshot 2024-02-14 at 9 07 15 pm"
src="https://github.com/MetaMask/metamask-mobile/assets/1881059/6044a3fc-c13e-41bb-a606-901e3328b8f4">

<img width="563" alt="Screenshot 2024-02-14 at 9 08 27 pm"
src="https://github.com/MetaMask/metamask-mobile/assets/1881059/865632c8-90b3-41bf-813b-671432cc5339">

### **After**

<img width="330" alt="Screenshot 2024-02-14 at 9 07 37 pm"
src="https://github.com/MetaMask/metamask-mobile/assets/1881059/98aab38f-ab00-435b-a5cc-f19060c307d2">

<img width="462" alt="Screenshot 2024-02-14 at 9 08 53 pm"
src="https://github.com/MetaMask/metamask-mobile/assets/1881059/82e5031b-aaf4-481e-a44d-aa02757ccceb">

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [x] In case it's not yet "ready for review", I've set it to "draft".
- [x] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
  • Loading branch information
leotm authored Feb 26, 2024
1 parent 0656d30 commit 6841a91
Showing 1 changed file with 17 additions and 0 deletions.
17 changes: 17 additions & 0 deletions app/util/sentry/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,25 @@ function removeDeviceName(report) {
report.contexts.device.name = null;
}

/**
* Removes SES from the Sentry error event stack trace.
* By default, SES is shown as the top level frame, which can obscure errors.
* We filter it out by identifying the SES stack trace frame simply by 'filename',
* since the 'context_line' is rather verbose.
* @param {*} report - the error event
*/
function removeSES(report) {
const stacktraceFrames = report.exception.values[0].stacktrace.frames;
const filteredFrames = stacktraceFrames.filter(
(frame) => frame.filename !== 'app:///ses.cjs',
);
report.exception.values[0].stacktrace.frames = filteredFrames;
}

function rewriteReport(report) {
try {
// filter out SES from error stack trace
removeSES(report);
// simplify certain complex error messages (e.g. Ethjs)
simplifyErrorMessages(report);
// remove urls from error message
Expand Down

0 comments on commit 6841a91

Please sign in to comment.