Improve node OnUncaughtException integration

Problem Statement

The current integration configuration for the node OnUncaughtExceptionOptions exception can be a bit confusing, especially with recently necessary changes regarding the default behavior.

Current behavior:

Following the node.js default, we exit the process on uncaught exceptions
For e.g. next.js, we need the capability to opt out of this behavior - however, we only want to opt out of it if any other handler has been registered.
Then there is also the option of providing an onFatalError handler to conditionally define if the process should exit or not

Solution Brainstorm

We could add a new option to (eventually) replace all existing options, e.g. mode.

A possible API could be:

type OnUncaughtExceptionMode = 'exit' | 'continue' | (firstError: Error, secondError?: Error) => boolean;

// default, matches node.js
new OnUncaughtException({
  onException: 'exit'
});

new OnUncaughtException({
  onException: 'continue'
});

new OnUncaughtException({
  onException: (error) => {
    // do something
    // return `true` to exit the process, any other return value will keep it running
    return true;
  }
});

With the behaviour:

exit: Always exit the process on uncaught exceptions. The default.
continue: Do not exit the process. Could be set by the next.js integration.
a function: Leave this to the user. Can eventually replace onFatalError

@lobsterkatie

So I think the following exception handler would kind of cover all relevant cases for now:

const onExceptionHandler = (error: Error): void => {
  const { onException } = this._options;
  const hub = getCurrentHub();

  if (hub.getIntegration(OnUncaughtException)) {
    hub.withScope((scope: Scope) => {
      scope.setLevel('fatal');
      hub.captureException(error, {
        originalException: error,
        data: { mechanism: { handled: false, type: 'onuncaughtexception' } },
      });
    });
  }

  if (onException === 'exit') {
    logAndExitProcess(error);
    return;
  }

  if (onException === 'warn') {
   logger.warn(`onUncaughtException integration encountered an exception: ${error}`);
    return;
  }

  if (typeof onException === 'function') {
    const result = onException(error);

    if (result !== undefined) {
      logAndExitProcess(result);
    }
    return;
  }

  // else: 'continue'
};

My idea would be to, for now, allow to opt-in to that behavior by setting onException on the integration. If that's set, use the new logic, else, the "old" one.

Then we can think about deprecating the "old" functionality, and flipping defaults for v8. The trickiest migration/thing is the onFatalError stuff, not sure if we want to/need to keep support for that (without any changes) in the future.

Thoughts, @lobsterkatie ?

We could then remove (or deprecate) the exitEvenIfOtherHandlersAreRegistered and instead set onException: 'continue' in the nextjs integration.

I agree that, by default, it shouldn't call exit. We just updated the library and took a day to understand why our servers were killed 😅.

@lforst

Can this be closed now @lforst?

Yes! Thanks for reminding me.

Navigation Menu

Repository navigation

Improve node `OnUncaughtException` integration #6146

Problem Statement

Solution Brainstorm

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Improve node OnUncaughtException integration #6146

Description

Problem Statement

Solution Brainstorm

Activity

mydea commented on Nov 9, 2022

Nikoms commented on Dec 13, 2022

AbhiPrasad commented on Apr 24, 2024

lforst commented on Apr 25, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

Improve node `OnUncaughtException` integration #6146