migrate pid file to core #77037

pgayvallet · 2020-09-09T12:07:19Z

Summary

Checklist

Unit or functional tests were updated or added to match the most common scenarios

pgayvallet · 2020-09-09T12:11:40Z

src/core/server/environment/write_pid_file.ts

+  process.on('unhandledRejection', function (reason) {
+    logger.warn(`Detected an unhandled Promise rejection.\n${reason}`);
+  });


This was in the legacy file, so I kept it for now, but I honestly have no idea why this was here.

This seems already handled by

kibana/src/setup_node_env/exit_on_warning.js

Lines 39 to 45 in bf04235

// 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);

@spalger Am I safe to remove this from this file?

I wonder if this is here so that warnings are included in the standard kibana log, rather than only written to the console, once the logger is setup. That said, both of these listeners do seem to be setup. I'd ask @watson just to be sure though.

(sorry for the late handoff, was out last week)

I must have missed the original unhandledRejection listener from src/legacy/server/pid/index.js when I implemented the one in src/setup_node_env/exit_on_warning.js.

The intention with the src/setup_node_env/exit_on_warning.js script is to exit on warnings during development and CI only. This way we catch these warnings as early as possible. It's all too easy to overlook them in the logs during development/CI, and so they slip into production builds. In production we normally run with the --no-warnings flag, so none of the listeners in src/setup_node_env/exit_on_warning.js are added in production.

So in production, the listener in this file is still active (though output depends on log-level). Whether or not we want this in production is a good question that I don't know the answer to. It's worth noting that Node.js it self will output the following to STDERR in case there's no unhandledRejection listener:

(node:88082) UnhandledPromiseRejectionWarning: undefined
(node:88082) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:88082) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

So adding the listener in this file means that we'll suppress that default behaviour even if log-level is set below warning.

Side note: Starting in Node.js 15, the process will crash by default if an unhandledRejection happens, but we'll not be affected by this until we upgrade to Node.js 16 as we'll skip 15.

Thanks for the answer. To avoid changing the behavior that was present in legacy, I will keep this now.

…-pid-file

elasticmachine · 2020-09-09T17:35:24Z

Pinging @elastic/kibana-platform (Team:Platform)

mshustov · 2020-09-10T12:43:43Z

src/core/server/environment/write_pid_file.ts

+
+  await writeFile(path, pid);
+
+  logger.info(`wrote pid file to ${path}`, { path, pid });


It used to have debug level

mshustov · 2020-09-10T12:51:55Z

src/core/server/environment/write_pid_file.test.ts

+    expect(writeFile).toHaveBeenCalledWith('/pid-file', pid);
+
+    expect(process.once).toHaveBeenCalledTimes(2);
+    expect(process.once).toHaveBeenCalledWith('exit', expect.any(Function));


I guess it's not worth the effort to test process.exit & SIGINT, so that is ok

jbudz

LGTM.

On the non-issue side - do you know where this lives in the setup lifecycle? I've been a little iffy on the (legacy) current implementation - similar to the logger, this feels like a bootstrap/setup_node_env type task that has the most utility when setup ASAP before the platform starts. e.g. the longer it takes for this file to write the less it can be used rely on process monitoring.

I haven't seen any issues opened because of it, so np. Feel free to ignore and we can revisit if issues do come up.

pgayvallet · 2020-09-11T06:15:17Z

@jbudz

do you know where this lives in the setup lifecycle?

The environment service is the first one to be setup during the server's setup phase.

kibana/src/core/server/server.ts

Lines 107 to 110 in a19484a

    
           public async setup() { 
        
             this.log.debug('setting up server'); 
        
             const environmentSetup = await this.environment.setup();

it's still in the server initialization, not in Root, or in setup_node_env, but moving it sooner would block us from properly logging messages using the platform logger, and, more importantly, to read the config file using the config service (at least for now, we are currently working on moving the config/logging service to packages to answer such needs, but it may take some time). So unless there is an explicit request to do so now, I'd say handling this in a follow-up would be preferable. I can open an issue to track it if you want.

…-pid-file

jbudz · 2020-09-14T16:10:29Z

Yeah no problem, I'm okay with delaying on the issue too. I'm half curious to see if we ever get a production type issue filed for it. It doesn't seem to be impacting anyone so far.

…-pid-file

src/core/server/environment/environment_service.ts

…-pid-file

watson

Changes regarding unhandledRejection handler LGTM 👍

kibanamachine · 2020-09-15T12:29:39Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 7bd512d

Build metrics

distributable file count

id	value	diff	baseline
default	45553	+1	45552
oss	27230	+1	27229
total		+2

History

💚 Build #74390 succeeded 47c5d5b
💚 Build #73919 succeeded eb0950c
💚 Build #73368 succeeded ce9d900
💔 Build #73349 failed 6914c15

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

* migrate pid file to core * use correct log level + add comment * move `unhandledRejection` to service's setup * update comment

migrate pid file to core

6914c15

pgayvallet added Feature:Legacy Removal Issues related to removing legacy Kibana Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0 release_note:skip Skip the PR/issue when compiling release notes labels Sep 9, 2020

pgayvallet commented Sep 9, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into kbn-77019-migrate…

ce9d900

…-pid-file

pgayvallet marked this pull request as ready for review September 9, 2020 17:35

pgayvallet requested review from a team as code owners September 9, 2020 17:35

mshustov approved these changes Sep 10, 2020

View reviewed changes

jbudz approved these changes Sep 10, 2020

View reviewed changes

pgayvallet added 2 commits September 11, 2020 08:15

Merge remote-tracking branch 'upstream/master' into kbn-77019-migrate…

fec23e4

…-pid-file

use correct log level + add comment

eb0950c

pgayvallet added 2 commits September 15, 2020 10:33

Merge remote-tracking branch 'upstream/master' into kbn-77019-migrate…

72720df

…-pid-file

move unhandledRejection to service's setup

47c5d5b

watson reviewed Sep 15, 2020

View reviewed changes

src/core/server/environment/environment_service.ts Outdated Show resolved Hide resolved

pgayvallet added 2 commits September 15, 2020 12:13

update comment

25ecc6a

Merge remote-tracking branch 'upstream/master' into kbn-77019-migrate…

7bd512d

…-pid-file

watson approved these changes Sep 15, 2020

View reviewed changes

pgayvallet merged commit cdea019 into elastic:master Sep 15, 2020

pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Sep 15, 2020

migrate pid file to core (elastic#77037)

602e08a

* migrate pid file to core * use correct log level + add comment * move `unhandledRejection` to service's setup * update comment

pgayvallet mentioned this pull request Sep 15, 2020

[7.x] migrate pid file to core (#77037) #77473

Merged

pgayvallet added a commit that referenced this pull request Sep 15, 2020

migrate pid file to core (#77037) (#77473)

ba0f06c

* migrate pid file to core * use correct log level + add comment * move `unhandledRejection` to service's setup * update comment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate pid file to core #77037

migrate pid file to core #77037

pgayvallet commented Sep 9, 2020 •

edited

Loading

pgayvallet Sep 9, 2020

spalger Sep 14, 2020

watson Sep 15, 2020

pgayvallet Sep 15, 2020

elasticmachine commented Sep 9, 2020

mshustov Sep 10, 2020

mshustov Sep 10, 2020

jbudz left a comment

pgayvallet commented Sep 11, 2020 •

edited

Loading

jbudz commented Sep 14, 2020

watson left a comment

kibanamachine commented Sep 15, 2020

	// 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);


		await writeFile(path, pid);

		logger.info(`wrote pid file to ${path}`, { path, pid });

migrate pid file to core #77037

migrate pid file to core #77037

Conversation

pgayvallet commented Sep 9, 2020 • edited Loading

Summary

Checklist

pgayvallet Sep 9, 2020

Choose a reason for hiding this comment

spalger Sep 14, 2020

Choose a reason for hiding this comment

watson Sep 15, 2020

Choose a reason for hiding this comment

pgayvallet Sep 15, 2020

Choose a reason for hiding this comment

elasticmachine commented Sep 9, 2020

mshustov Sep 10, 2020

Choose a reason for hiding this comment

mshustov Sep 10, 2020

Choose a reason for hiding this comment

jbudz left a comment

Choose a reason for hiding this comment

pgayvallet commented Sep 11, 2020 • edited Loading

jbudz commented Sep 14, 2020

watson left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 15, 2020

💚 Build Succeeded

Build metrics

distributable file count

History

pgayvallet commented Sep 9, 2020 •

edited

Loading

pgayvallet commented Sep 11, 2020 •

edited

Loading