Catch @hapi/podium errors

[watson]

If an async error occurred within the call to this.events.emit, the error would be very hard to trace. In my case I would not get any meaningful stack trace and it took me many days to track it down to this location. Adding this catch solves that issue. I think exiting as I do here is ok, as the alternative (not having the catch) would resolve in an unhandled rejection which has the same effect. But please let me know if it can be done better 😃

[pgayvallet]

Now that this is identified, maybe we would like to just log the error and not exit the process? This seems a bit overkill for an error occurring inside logging? @restrry wdyt?

[mshustov]

This seems a bit overkill for an error occurring inside logging?

Maybe it's an overkill, but it's easy to miss that log message in stderr, so debugging a Kibana crash without logs might be a nightmare. The current PR replicates the current behavior, and it doesn't seem we've ever had any complaints about it. CC @joshdover

[watson]

This commit has been in my working directory for about a week, so some of the details about the original behaviour is unfortunately a little bit fuzzy, but I just tried to recreate my original issue which I was trying to debug when discovering this, and found that under normal circumstances, we do get a regular unhandled rejection crash which fairly easily can be traced to the right location. However, my special circumstance was that the server was shut down prior to the unhandled rejection event being fired, and so it was never logged.

I'm fine with either logging an continuing, re-throwing, or using process.exit.

[joshdover]

Maybe it's an overkill, but it's easy to miss that log message in stderr, so debugging a Kibana crash without logs might be a nightmare.

I'm probably missing something, but how does crashing the process make debugging this easier for the user?

That said, does the current audit logging implementation go through this mechanism? If so, we may want a crash here if we're unable to write audit log events. Even if not, I also lean towards just leaving this as the current behavior until we remove legacy logging altogether in 8.0 and make the behavior change then.

[watson]

how does crashing the process make debugging this easier for the user?

I think it's just meant to make it easier for us as developers 😅 The end-user not so much.

Today the process crashes on unhandled rejections as the process is considered to be in an unknown state and that it's unsafe to continue:

kibana/src/setup_node_env/exit_on_warning.js

Lines 62 to 72 in 5420177

	// While the above warning listener would also be called on
	// unhandledRejection warnings, we can give a better error message if we
	// handle them separately:
	process.on('unhandledRejection', function (reason) {
	console.error('Unhandled Promise rejection detected:');
	console.error();
	console.error(reason);
	console.error();
	console.error('Terminating process...');
	process.exit(1);
	});

does the current audit logging implementation go through this mechanism?

I'm actually not sure. @thomheymann Do you know if audit logging uses the same logger class as the rest of Kibana?

[mshustov]

I'm probably missing something, but how does crashing the process make debugging this easier for the user?

Kibana could stop logging messages at any time due to an error in logging system. Should it crash later (for example, due to a lack of disk space), users aren't be able to diagnosis a problem without logs.

That said, does the current audit logging implementation go through this mechanism? If so, we may want a crash here if we're unable to write audit log events.

Yes, it does.

Even if not, I also lean towards just leaving this as the current behavior until we remove legacy logging altogether in 8.0 and make the behavior change then.

Ideally, we should initiate a graceful shutdown for such cases, but right now the core doesn't provide such functionality.

[watson]

@restrry So does that mean, that until such a time when we have a graceful shutdown mechanism, that the current approach in this PR is ok to merge?

[mshustov]

@watson Yes, I think so

[watson]

@elasticmachine merge upstream

[watson]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 88de16e

Metrics [docs]

✅ unchanged

History

• 💚 Build #91541 succeeded 82350ed
• 💚 Build #90854 succeeded aab7e9a

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream