Sentry-javascript: Documentation suggestion: warn about using `onFatalError`

[OliverJAsh]

• [ ] Review the documentation: https://docs.sentry.io/
• [ ] Search for existing issues: https://github.com/getsentry/sentry-javascript/issues
• [ ] Use the latest release: https://github.com/getsentry/sentry-javascript/releases
• [ ] Provide a link to the affected event from your Sentry account

Package + Version

• [ ] @sentry/browser
• [x] @sentry/node
• [ ] raven-js
• [ ] raven-node _(raven for node)_
• [ ] other:

Version:

5.15.5

Description

For the longest time I've been passing an onFatalError option where I do something along the lines of what Raven used to do:

https://github.com/getsentry/raven-node/blob/56a588e3deab4494c6f41e14ed906f3d40511781/lib/client.js#L86-L89

    this.onFatalError = this.defaultOnFatalError = function (err, sendErr, eventId) {
      console.error(err && err.stack ? err.stack : err);
      process.exit(1);
    };

(For context, the only reason I did this was just because I wanted to prefix the logged error message with "fatal error", to differentiate them from non-fatal errors we choose to log.)

However, just today I noticed that uncaught exceptions were not being reported to Sentry. After some debugging it seemed to be because of my custom onFatalError. In the latest version of @sentry/node, the default onFatalError only kills the process when all events have finished sending:

https://github.com/getsentry/sentry-javascript/blob/d716bd96952cdd7a86662246b8b0f80761fe1275/packages/node/src/integrations/onuncaughtexception.ts#L57

https://github.com/getsentry/sentry-javascript/blob/7956bd84fac447005682192a04e29aaa95172554/packages/node/src/handlers.ts#L430-L453

By passing a custom onFatalError I was unintentionally overriding this behaviour, thus the event never reached Sentry because the process was killed before the event was sent.

For this reason, I think the docs for onFatalError should carry a big warning—that by customising this functionality, you lose this important facility. WDYT?

[OliverJAsh]

I'm also curious if there's any way to achieve the use case I outlined above without breaking error reporting.

(For context, the only reason I did this was just because I wanted to prefix the logged error message with "fatal error", to differentiate them from non-fatal errors we choose to log.)

Idea: what if the onFatalError option (or perhaps a new option) was only called once the fatal error had finished being sent, so it's safe to exit the process?

[JCMais]

Just hit this same issue, use case is adding more contextual logging to the fatal error.

[JamesHagerman]

I just hit a similar but related issue that would have been much easier to understand with more documentation.

This older issue spells out the intent better then the docs: https://github.com/getsentry/sentry-javascript/issues/1661#issuecomment-718200921

Perhaps the overlap here is more evidence of how documentation can be improved here?

[kamilogorek]

@OliverJAsh @JamesHagerman updated the docs - https://github.com/getsentry/sentry-docs/pull/2643

@JCMais every fatal error is already marked as such with level: fatal tag - https://github.com/getsentry/sentry-javascript/blob/c864ff63d3515be4efa3b1399ef9cafe89497661/packages/node/src/integrations/onuncaughtexception.ts#L80
This will let you modify the log message with ease if you need to:

Sentry.init({
  beforeSend(event) {
    if (event.level === 'fatal') {
      // modify the event
    }
    return event;
  }
});

or do the above with event processors instead.

Feel free to let me know if it's not sufficient and I'll happily reopen this issue. Cheers!