Fire-and-forget notifier calls in `ErrorReporter.notify`

[alexeyr-ci]

In #471, the error reporting notifiers are called synchronously in notify:

react_on_rails_pro/packages/node-renderer/src/shared/errorReporter.ts

Lines 35 to 44 in 78f78f5

	notifiers.forEach((notifier) => {
	try {
	notifier(msg, tracingContext);
	} catch (e) {
	log.error(
	// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
	`An error tracking notifier failed: ${(e as Error).message ?? e}\nStack:\n${(e as Error).stack}`,
	);
	}
	});

We could possibly improve performance a bit by scheduling the calls to happen later with async or setImmediate/setTimeout. Though it's only called on error, which should be rare, and the notifiers should be fast.

[alexeyr-ci]

Copying the relevant Slack discussion with @AbanoubGhadban here:

Alexey Romanov
Yesterday at 7:46 PM
setTimeout(fn, 0) and setImmediate(fn) for notifiers don't work with Sentry 8 correctly and fixing it would be possible but complicated, I think. And worse, other tracking services which rely on AsyncLocalContext (like BugSnag 8 ) may not be fixable at all.
(async () => { ... })() does, but doesn't give any benefit (something like await delay(0) inside to move the work to the next event loop iteration seems to break things again, which surprised me).

Abanoub Ghadban
Today at 11:11 AM
You mean, sentry can't get the correct error stack correctly in this case?

Alexey Romanov
Today at 11:18 AM
It can't get the correct transaction. I've changed tracing.test.ts to check Sentry reports within a tracing.trace (former tracing.withinTransaction) call are for the correct transaction

react_on_rails_pro/packages/node-renderer/tests/shared/tracing.test.ts

Line 49 in 78f78f5

expect(report.tags.transaction).toBe(spanName);

, and this assertion fails with setTimeout/setImmediate because report.tags.transaction becomes undefined.

Abanoub Ghadban
Today at 11:19 AM
OK, I need first to understand what trace does? Does it log to sentry the time taken by each render request and report errors if happen?
11:20
But seems we already catch all errors happen during the handling of the rendering request. So, actually, it won't report any errors.
11:21
Errors are reported manually in the catch block by calling errorReporter.message, aren't they? (edited)
11:22
Does tracing provide any additional functionality?

Alexey Romanov
Today at 11:25 AM
Tracing:
Reports request timing to Sentry, yes.
Not yet done, but it can include which user, etc. the request corresponds to.
It tells errorReporter.message/error calls which request they belong to.
11:25
The 3 point is the one which breaks with setTimeout. (edited)

Abanoub Ghadban
Today at 11:27 AM
OK, got it. Seems that the notifier is executed after the fn passed to the executor is finished.
11:29
I think you need to make errorReporter.message async and resolves after executing all callbacks functions.
11:29
Anyway, it's not a must because we already don't have heavy listeners now.
11:31
But anyway if you decided to make errorReporter.message async. You can execute await setResponse(errorResponseResult(exceptionMessage), res); before it.
11:31
Or you can

await Promise.all([
  errorReporter.message(exceptionMessage, context),
  setResponse(errorResponseResult(exceptionMessage), res),
]);

11:32
But anyway, it's not necessary because this situation is only happen if an error occurred which should be a rare situation

Alexey Romanov
Today at 11:34 AM
Seems that the notifier is executed after the fn passed to the executor is finished.
Not quite. errorReporter.message is executed within fn itself. If it calls the notifiers directly (right now) everything works.

But anyway if you decided to make errorReporter.message async.

If we await its result it's just overhead. We could do
(async () => { ... })()
in errorReporter.message but I don't think that's too useful either, as mentioned.
11:36

Anyway, it's not a must because we already don't have heavy listeners now.

Yes, and really Sentry.capture... returns as fast as possible and doesn't wait until the message is actually sent. I expect it's the same with other services.

Abanoub Ghadban
Today at 11:36 AM

If it calls the notifiers directly (right now) everything works.

Yes, I know that's the case now. I mean the problem happen when you try to execute notifiers asynchronously.
11:37
OK, as long as you handled possible errors, it's fine for me. Just keep it in mind if any client complained about slow down because of any reporting plugin (I don't think this will happen anyway).

Alexey Romanov
Today at 11:38 AM

(something like await delay(0) inside to move the work to the next event loop iteration seems to break things again, which surprised me)

Actually, I just had an idea about it, maybe my test isn't waiting enough (just as I needed to add Sentry.flush). But it can be done in a follow-up.
11:40

Just keep it in mind if any client complained about slow down because of any reporting plugin (I don't think this will happen anyway).

The current version of errorReporter.notify calls Sentry.capture... directly, so the change there should just be that it doesn't test if (this.honeybadger) and if (this.sentry) each time.

Abanoub Ghadban
Today at 11:41 AM
Anyway, just to be clear. I mean by awaiting notifiers is converting the notify function to

async function notify<T>(msg: T, tracingContext: TracingContext | undefined, notifiers: Notifier<T>[]) {
  log.error(`ErrorReporter notification: ${msg}`);
  const notifierPromises = notifiers.map((notifier) => new Promise((resolve, reject) => {
      try {
        notifier(msg, tracingContext);
      } catch (e) {
        log.error(
          // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
            `An error tracking notifier failed: ${(e as Error).message ?? e}\nStack:\n${(e as Error).stack}`,
          );
        }
        resolve(true);
      }),
  );
  await Promise.all(notifierPromises);
}

By awaiting them inside the fn, we will ensure that the notifier will be called inside the transaction. (edited)
11:42
And in the request handler, call them like this

await Promise.all([
  errorReporter.message(exceptionMessage, context),
  setResponse(errorResponseResult(exceptionMessage), res),
]);

11:44
You don't need to implement them now, just keep them in mind in case they needed.

Alexey Romanov
1 hour ago
OK, yes. This can work, but I think if we make it async we really don't want to await it within fn, just fire the notifiers and forget. The question is about testing it correctly 🙂