Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch @hapi/podium errors #84575

Merged
merged 3 commits into from
Dec 3, 2020
Merged

Catch @hapi/podium errors #84575

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aab7e9a
Catch @hapi/podium errors
watson Nov 30, 2020
82350ed
Merge branch 'master' into catch-podium-errors
kibanamachine Dec 2, 2020
88de16e
Merge branch 'master' into catch-podium-errors
kibanamachine Dec 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions packages/kbn-legacy-logging/src/legacy_logging_server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,18 @@ export class LegacyLoggingServer {
public log({ level, context, message, error, timestamp, meta = {} }: LogRecord) {
const { tags = [], ...metadata } = meta;

this.events.emit('log', {
data: getDataToLog(error, metadata, message),
tags: [getLegacyLogLevel(level), ...context.split('.'), ...tags],
timestamp: timestamp.getTime(),
});
this.events
.emit('log', {
data: getDataToLog(error, metadata, message),
tags: [getLegacyLogLevel(level), ...context.split('.'), ...tags],
timestamp: timestamp.getTime(),
})
// @ts-expect-error @hapi/podium emit is actually an async function
.catch((err) => {
// eslint-disable-next-line no-console
console.error('An unexpected error occurred while writing to the log:', err.stack);
process.exit(1);
Comment on lines +129 to +130
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@mshustov mshustov Dec 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@mshustov mshustov Dec 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@watson Yes, I think so

});
}

public stop() {
Expand Down