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
• @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:

sentry-javascript/packages/node/src/integrations/onuncaughtexception.ts

Line 57 in d716bd9

let onFatalError: onFatalErrorHandlerType = logAndExitProcess;

sentry-javascript/packages/node/src/handlers.ts

Lines 430 to 453 in 7956bd8

	export function logAndExitProcess(error: Error): void {
	console.error(error && error.stack ? error.stack : error);
	const client = getCurrentHub().getClient<NodeClient>();
	if (client === undefined) {
	logger.warn('No NodeClient was defined, we are exiting the process now.');
	global.process.exit(1);
	return;
	}
	const options = client.getOptions();
	const timeout =
	(options && options.shutdownTimeout && options.shutdownTimeout > 0 && options.shutdownTimeout) ||
	DEFAULT_SHUTDOWN_TIMEOUT;
	forget(
	client.close(timeout).then((result: boolean) => {
	if (!result) {
	logger.warn('We reached the timeout for emptying the request buffer, still exiting now!');
	}
	global.process.exit(1);
	}),
	);
	}

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: #1661 (comment)

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

[kamilogorek]

@OliverJAsh @JamesHagerman updated the docs - getsentry/sentry-docs#2643

@JCMais every fatal error is already marked as such with level: fatal tag -

sentry-javascript/packages/node/src/integrations/onuncaughtexception.ts

Line 80 in c864ff6

scope.setLevel(Severity.Fatal);

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!