Opt-in hooks and signal by default by rnchamberlain · Pull Request #28 · nodejs/node-report

rnchamberlain · 2016-11-23T16:12:02Z

This changes the default action on require('nodereport') to opt-in to the exception and fatal-error hooks, and to enable the signal handler. So 'node -r nodereport application.js' will get all the nodereport functionality without requiring application code changes.

For users programming to the API who don't want the invasive hooks and signal handler, a alternative api-only entry point, require('nodereport/api') is provided. This does not enable the hooks and signal unless and until .setEvents() is called.

Addresses issue #25

This changes the default action on require('nodereport') to opt-in to the exception and fatal-error hooks, and to enable the signal handler. So 'node -r nodereport application.js' will get all the functionality without any code changes. For users programming to the API, a separate api-only entry point, require('nodereport/api') is provided. This does not enable the hooks and signal unless and until .setEvents() is called. Also includes a change to the behaviour on uncaught exception: default behaviour is now the same as the node default, ie no SIGILL/core dump

richardlau

Some nits. Test fails for linux-x64 with Node.js v6.9.1.

Also documentation will need updating. Probably would be a good idea to add tests that validate reports are generated in the default cases (bonus for also checking reports are not generated for the default require('nodereport/api') case).

richardlau · 2016-11-23T18:28:25Z

@@ -0,0 +1,34 @@
+// Main module entry point for nodereport
+
+var api = require('./api');


const instead of var?

richardlau · 2016-11-23T18:28:59Z

+var api = require('./api');
+
+// Process NODEREPORT_EVENTS env var
+var options = process.env.NODEREPORT_EVENTS;


const here as well.

richardlau · 2016-11-23T18:51:03Z

 {
  "name": "nodereport",
-  "main": "nodereport.node",
+  "main": "./main",


./ is unnecessary here.

Line now removed (on Sam's suggestion to use index.js instead of main.js)

richardlau · 2016-11-23T18:59:38Z

-              'Process should exit with expected signal ');
+  child.on('exit', (code) => {
+    tap.plan(3);
+    tap.equal(code, 1, 'Process should exit with expected return code');


Just tried running your branch on Node.js v6.9.1 linux-x64 and got this:

-bash-4.2$ npm test > nodereport@1.0.6 test /home/users/riclau/sandbox/github/nodereport > tap test/test*.js test/test-api.js ...................................... 7/7 test/test-exception.js ................................ 6/7 not ok Process should exit with expected return code +++ found --- wanted -1 +0 compare: === at: line: 26 column: 9 file: test/test-exception.js type: ChildProcess function: child.on stack: | ChildProcess.child.on (test/test-exception.js:26:9) source: | tap.equal(code, 1, 'Process should exit with expected return code'); test/test-fatal-error.js .............................. 7/7 test/test-signal.js ................................... 8/8 total ............................................... 28/29 28 passing (2s) 1 failing npm ERR! Test failed. See above for more details. -bash-4.2$

I have reverted the change to the behaviour on uncaught exception, I think that needs a separate PR. Test now passing for me.

rnchamberlain · 2016-11-24T09:54:53Z

Ah, behavior on uncaught exception (when hooked) on v6.9.1 is different to v7.0.0. Return code is 0 instead of 1 - hence the test failure - and the exception message and stack trace on the console is missing. Investigating.

Re the tests, currently we are testing for example: require('nodereport').setEvents('fatalerror')which is no longer as important a use case as
1) require('nodereport') or
2) require('nodereport/api').setEvents('fatalerror')
We should probably test both of those cases, at least.

Yes docs will need updating.

Fishrock123 · 2016-11-28T20:10:29Z

I'll try to get to this soon. :D

sam-github · 2016-11-29T16:13:27Z

 *
 !src/**
 !binding.gyp
+!main.js


index.js is traditional, and doesn't require config in package.json

done, using index.js now

sam-github · 2016-11-29T16:14:43Z

+  api.setEvents('exception+fatalerror+signal+apicall');
+}
+
+exports.triggerReport = function() {


why not exports.triggerReport = api.triggerReport;?

sam-github · 2016-11-29T16:15:00Z

+};
+exports.setVerbose = function() {
+  return api.setVerbose.apply(null, arguments);
+};


^ no newline at end of file

Simplify exports in index.js Revert change to default behaviour on uncaught exception Update documentation Update tests to reflect default change (setEvents() not needed)

rnchamberlain · 2016-12-07T13:45:30Z

I have reverted the change to the default behavior on uncaught exception - currently it goes to the v8 abort/sigill - will fix later in a separate PR.

Docs updated to advertise node -r nodereport app.js

For the tests, I have changed them to test the new default behavior - i.e. setEvents() calls are no longer needed. I will add tests specifically for the require('nodereport/api') use case in a later PR.

Fishrock123 · 2016-12-12T19:26:55Z

Sounds good to me. Doesn't merge tho so I didn't try it.

rnchamberlain · 2016-12-14T10:46:47Z

As there's been some churn on my branch I'll close this and raise a new PR.

richardlau suggested changes Nov 23, 2016

View reviewed changes

sam-github approved these changes Nov 29, 2016

View reviewed changes

rnchamberlain added 3 commits December 6, 2016 13:38

Merge branch 'master' into exception-default

62d6151

Use index.js for main entry point instead of main.js

15a4740

Simplify exports in index.js Revert change to default behaviour on uncaught exception Update documentation Update tests to reflect default change (setEvents() not needed)

Fix var -> const nits in index.js

eacc529

rnchamberlain closed this Dec 14, 2016

rnchamberlain deleted the exception-default branch December 14, 2016 10:47

rnchamberlain mentioned this pull request Dec 14, 2016

Opt-in hooks and signal by default #33

Closed

		@@ -0,0 +1,34 @@
		// Main module entry point for nodereport

		var api = require('./api');

Conversation

rnchamberlain commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardlau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnchamberlain commented Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fishrock123 commented Nov 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnchamberlain commented Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fishrock123 commented Dec 12, 2016

Uh oh!

rnchamberlain commented Dec 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rnchamberlain commented Nov 23, 2016 •

edited

Loading

rnchamberlain commented Nov 24, 2016 •

edited

Loading

rnchamberlain commented Dec 7, 2016 •

edited

Loading