Initial drop of nodereport project for nodejs repo by rnchamberlain · Pull Request #2 · nodejs/node-report

rnchamberlain · 2016-08-25T15:56:15Z

Implementation of native NPM module for diagnostic report triggered on exception and fatal error events, signals and JavaScript API. Supports Node v4 and v6, Linux, Mac and Windows. Includes testcases and demo samples.

Fishrock123 · 2016-08-29T16:00:25Z

I'm getting this on OS X 10.10.5 and Node.js v4/v6:

> node-gyp rebuild

  CXX(target) Release/obj.target/nodereport/src/node_report.o
../src/node_report.cc:675:76: warning: format specifies type 'long' but the argument has type '__darwin_suseconds_t' (aka 'int') [-Wformat]
    fprintf(fp,"\n  User mode CPU: %ld.%06ld secs", stats.ru_utime.tv_sec, stats.ru_utime.tv_usec);
                                       ~~~~~                               ^~~~~~~~~~~~~~~~~~~~~~
                                       %06d
../src/node_report.cc:676:78: warning: format specifies type 'long' but the argument has type '__darwin_suseconds_t' (aka 'int') [-Wformat]
    fprintf(fp,"\n  Kernel mode CPU: %ld.%06ld secs", stats.ru_stime.tv_sec, stats.ru_stime.tv_usec);
                                         ~~~~~                               ^~~~~~~~~~~~~~~~~~~~~~
                                         %06d
../src/node_report.cc:684:17: error: use of undeclared identifier 'RUSAGE_THREAD'
  if (getrusage(RUSAGE_THREAD, &stats) == 0) {
                ^
../src/node_report.cc:685:76: warning: format specifies type 'long' but the argument has type '__darwin_suseconds_t' (aka 'int') [-Wformat]
    fprintf(fp,"\n  User mode CPU: %ld.%06ld secs", stats.ru_utime.tv_sec, stats.ru_utime.tv_usec);
                                       ~~~~~                               ^~~~~~~~~~~~~~~~~~~~~~
                                       %06d
../src/node_report.cc:686:78: warning: format specifies type 'long' but the argument has type '__darwin_suseconds_t' (aka 'int') [-Wformat]
    fprintf(fp,"\n  Kernel mode CPU: %ld.%06ld secs", stats.ru_stime.tv_sec, stats.ru_stime.tv_usec);
                                         ~~~~~                               ^~~~~~~~~~~~~~~~~~~~~~
                                         %06d
4 warnings and 1 error generated.
make: *** [Release/obj.target/nodereport/src/node_report.o] Error 1

Also when applying this patch:

warning: squelched 20 whitespace errors
warning: 25 lines add whitespace errors.

mhdawson · 2016-08-29T19:35:35Z

As an FYI Richard is on holiday this week , so response will be delayed.

mhdawson · 2016-08-31T20:46:37Z

@bnoordhuis, believe you looked at PR to core, so you make be able to review this as the key issues will be the same, just as an npm versus part of core.

bnoordhuis · 2016-09-01T11:15:07Z

+      "conditions": [
+        ["OS=='linux'", {
+          "defines": [ "_GNU_SOURCE" ],
+          "cflags": [ "-g -O2 -std=c++11", ],


These should be split, not as a single string.

mhdawson · 2016-09-28T14:51:26Z

LGTM with a few nits.

rnchamberlain · 2016-10-03T14:03:57Z

@mhdawson thanks for the review, I have moved the blocks of code that print version information and stack traces in node_report.cc into separate functions, and fixed the minor nits. Also added the scripts entry into package.json so uses can run "npm test", and fixed some CRLF line endings in the test code.

I had not considered line-mode options (options are currently supported via env vars and/or JS API calls). A typical node command line is:

node <node and v8 options> application.js <application options>

We could allow nodereport options to be mixed in with either the node/v8 or the application options. Suitable unique naming would be need, e.g something like "nodereport_events=exception+signal". Application options are available via process.argv, though I am not sure how to access that from C++, otherwise I should be able to get the command line from the OS . Anyway, something for a future enhancement.

Fishrock123 · 2016-10-04T16:10:51Z

Got it to build on OS X, is NodeReport.20161004.120508.2622.001.txt just meant to be read manually? Seems reasonably understandable to me.

Fishrock123 · 2016-10-04T16:21:45Z

Should this be merged so that it can be worked on more collaboratively?

rnchamberlain · 2016-10-04T16:39:07Z

@Fishrock123 yes absolutely, the report is intended to be read manually, using any text editor/notepad/email etc, and should be readily understandable to node developers, admins, testers etc

Yes this is ready to go and for others to contribute. @mhdawson - not sure how this repo is set up, who has powers to merge this? thanks

Fishrock123 · 2016-10-04T16:45:33Z

I think we should probably just give you perms to merge it. cc/ @nodejs/post-mortem

misterdjules · 2016-10-04T17:09:22Z

This repo has @mhdawson and the @nodejs/post-mortem team as collaborators. @rnchamberlain isn't (to my surprise) a member of the @nodejs/post-mortem team, even though he's clearly active in that space, so I added him to that team.

Once @rnchamberlain accepts the invite, he'll have the credentials to push commits to this repo.

sam-github · 2016-10-04T19:08:28Z

+
+    var nodereport = require('nodereport');
+
+By default, this will trigger a NodeReport to be written to the current


I don't think this is a good idea. It means that there is no way to opt-out of auto-handling of unhandledException, which would be useful for someone who wanted to only use the manual .triggerReport(). I think it would make more sense for a require of nodereport to make no changes to global process state until it was attached. Syntax could be require('nodereport')(), or require('nodereport').attach(), or any other variant.

also, I would want to control independently whether nodereport attached to SIGUSR2, or even configure the signal.

Hi @sam-github, you can override the defaults and configure which events you want via the API, so
require("nodereport").setEvents("fatalerror+signal");
will switch off the exception handler, likewise
require("nodereport").setEvents("exception+fatalerror");
will switch off the signal handler.

require("nodereport").setSignal("SIGQUIT");
will configure SIGQUIT for the trigger signal instead of SIGUSR2, and we could add more choices for that (though I just found a bug in that code, fix on it's way).

Perhaps an optional options arg with key events as an array of potential triggers and key sigusr2 as a boolean to enable/disable the signal would be a nice approach.

Such an arg would be reasonably extensible for other triggers too.

Also allows for clear defaults to be set early in code.

Yes, I think that's the way to go, extra parameter(s) on the module Initialize(). There is a limitation here in the v8 APIs, in that isolate->SetFatalErrorHandler(), isolate->SetCaptureStackTraceForUncaughtExceptions() and isolate->SetAbortOnUncaughtExceptionCallback() do not have 'unset' counterparts, and they don't support chaining of the callbacks. So if the nodereport module sets those at startup, a subsequent setEvents() call to the module can only switch off what we do in our callback, the underlying change to v8/node behaviour remains in place. Using env vars gets around that in the current implementation, as they are available at module init, but they are a bit nasty. So Sam's require('nodereport')(...), suggestion, with 'at start-up' equivalents of the setEvents(), setSignal(), setDirectory etc APIs would be a better solution.

sam-github · 2016-10-04T19:11:23Z

+Configuration is available via these environment variables and/or Javascript
+API calls:
+
+    export NODEREPORT_EVENTS=exception+fatalerror+signal


that its possible to configure nodereport via env vars is great, that its necessary to do so is I think too much focus on policy instead of mechanism

I'd expect the .setEvents() API to be the usual way to configure it, with the env vars as an alternative maybe for admins when changing the code is not convenient. I'll update the readme to reflect that.

Re the defaults, the design was for all the events to work 'out of the box' i.e. with just require("nodereport"). We could take a more cautious approach as you suggest, and just have
.triggerReport() available until .setEvents(list of selected events) is called.

Fishrock123 · 2016-10-04T19:19:26Z

I don't think we should pull from core eslint (it's a legacy mess). Just use standard.

mhdawson · 2016-10-05T13:49:41Z

I believe I have given everybody in the post-mortem team write access to this repo. Was just waiting for 1 more L.G.T.M from one of the other reviewers before landing.

joshgav · 2016-10-06T18:02:10Z

+}
+NAN_METHOD(SetCoreDump) {
+  Nan::Utf8String parameter(info[0]);
+  nodereport_events = ProcessNodeReportCoreSwitch(*parameter);


I believe this is intended: s/nodereport_events/nodereport_core/

@joshgav good spot, and same error in SetSignal method below, fix in progress

Fishrock123 · 2016-10-11T20:14:24Z

Status? I think I'd actually like to start using this.

rnchamberlain · 2016-10-12T15:43:33Z

Fixed as discussed with @sam-github and @joshgav. So require('nodereport') by default now just enables the .triggerReport() functionality. No changes to the Node/v8 hooks and signals until the requirer explicitly calls .setEvents("exception+signal+apicall+etc").

Status: as per @mhdawson above, just another LGTM then we land. Meantime available to try out now with
npm install rnchamberlain/nodereport-1

sam-github · 2016-10-13T14:52:08Z

@bnoordhuis LGTY?

@rnchamberlain API LGTM. If you want to make it easy to enable all capabilities, you could implement a usage like:

require('nodereport')();

So a side-effectless require is possible, but all side-effects can be opted in easily, and the "module call" above would check for env vars

You could also extend above with an options object rather than method calls for configuration:

require('nodereport')({
  events: 'signal',
  signal: 'SIGHUP',
  });

EDIT: just suggestions, which can be taken up in a subsequent PR if you are interested, they don't have to happen now.

rnchamberlain · 2016-10-13T15:44:12Z

@sam-github I had a look at both of your suggested syntax options, but couldn't immediately work out how to code them on the C++ side! There is a mention of a two-argument form of the Init() function here: https://nodejs.org/api/addons.html, which has a module object passed in as well as the exports object? Anyway yes, potential future enhancement - thanks.

bnoordhuis · 2016-10-13T16:10:30Z

+
+# Build directories
+/build/
+/node_modules/


This file uses Windows line endings for some reason.

bnoordhuis · 2016-10-13T16:11:37Z

+    api.js - NodeReport triggered by Javascript API call
+    exception.js - NodeReport triggered by unhandled exception
+    fatalerror.js - NodeReport triggered by fatal error on Javascript heap out of memory
+    loop.js - looping application, NodeReport triggered using kill -USR2 <pid>


There is some trailing whitespace in this file.

bnoordhuis · 2016-10-13T16:15:03Z

+setTimeout(function() {
+  console.log('loop.js: timeout expired, exiting.');
+  process.exit(0); 
+}, 60000);


bnoordhuis · 2016-10-13T16:20:31Z

+
+  if (info[0]->IsString()) {
+    // Filename parameter supplied
+    Nan::Utf8String filename_parameter(info[0]->ToString());


You don't have to ToString() the argument to Nan::Utf8String.

bnoordhuis · 2016-10-13T16:21:25Z

+    if (filename_parameter.length() < NR_MAXNAME) {
+      snprintf(filename, sizeof(filename), "%s", *filename_parameter);
+    } else {
+      Nan::ThrowSyntaxError("nodereport: filename parameter is too long");


SyntaxError is kind of weird to throw for a bad argument.

bnoordhuis · 2016-10-13T16:54:26Z

+    break;
+  case kFatalError:
+#if NODE_VERSION_AT_LEAST(6, 0, 0)
+    if (!strncmp(location,"MarkCompactCollector", sizeof("MarkCompactCollector") - 1)) {


bnoordhuis · 2016-10-13T16:55:49Z

+  // Walk the frames printing symbolic information if available
+  for (int i = 0; i < numberOfFrames; i++) {
+    DWORD64 dwOffset64 = 0;
+    DWORD64 dwAddress = (DWORD64)(frames[i]);


C++-style casts, please!

bnoordhuis · 2016-10-13T16:56:04Z

+    if (SymFromAddr(hProcess, dwAddress, &dwOffset64, pSymbol)) {
+        DWORD dwOffset = 0;
+        IMAGEHLP_LINE64 line;
+        line.SizeOfStruct = sizeof(IMAGEHLP_LINE64);


sizeof(line)?

https://msdn.microsoft.com/en-us/library/windows/desktop/ms680190%28v=vs.85%29.aspx

Right, but that doesn't preclude sizeof(line), does it?

Either sizeof(type name) or sizeof(variable name) work OK I believe, the MSDN examples uses sizeof(type name). Is sizeof(variable name) preferred?

It is in the node core code base so you may want to follow that here. The reasoning is that sizeof(var) won't break when the type of var changes.

bnoordhuis · 2016-10-13T16:57:25Z

+  lpvEnv = GetEnvironmentStrings();
+  if (lpvEnv != NULL) {
+    // Variable strings are separated by NULL byte, and the block is terminated by a NULL byte.
+    lpszVariable = (LPTSTR) lpvEnv;


bnoordhuis · 2016-10-13T16:59:08Z

+#endif  // !__GLIBC_PREREQ(2, 17)
+#else
+#define secure_getenv getenv
+#endif  // defined(__GLIBC__)


Potential downside is that a .node compiled against glibc 2.17 won't work with e.g. glibc 2.15. That is, you won't be able to simply copy the shared object around.

I wonder if that's a common thing to do, copy binaries around from an npm install. Someone is bound to do it I suppose, and they will get an unresolved symbol at load time. Under consideration!

I think I have fixed everything else, see commits below, and thanks for the review.

Its a nice to have to be able to build on one system, and run on another slightly different one. It fails often enough that its usually necessary to build on a system of identical os/etc level and architecture as the deployment/production system.

OK, glibc 2.17 is fairly recent: https://sourceware.org/glibc/wiki/Glibc%20Timeline. I'll do it the node.cc way and have a local implementation of secure_getenv().

rnchamberlain · 2016-10-19T16:41:11Z

Looking back we have an LGTM and an API LGTM on this, I'll merge shortly unless anyone objects. Thanks.

Initial drop of nodereport project for nodejs repo

a582ea9

bnoordhuis reviewed Sep 1, 2016
View reviewed changes

Separate functions for report sections, plus fix minor nits

af73614

Fix missing mutex initialization (OSX aborts on signal)

bd8b06b

sam-github reviewed Oct 4, 2016

View reviewed changes

Fix .js files to use 2 space indentation

0f2093b

joshgav reviewed Oct 6, 2016

View reviewed changes

Fix API to allow live changes to trigger signal

56e9ee1

rnchamberlain added 2 commits October 12, 2016 15:49

Trigger events other than API are off by default

efb729e

Missing ifdef, no signal support on Windows

2b5e0a0

bnoordhuis reviewed Oct 13, 2016

View reviewed changes

rnchamberlain added 3 commits October 14, 2016 16:08

Improve signal functions, fix K&Risms, whitespace etc

4365862

Fix for __func__ not available on VS 2013

a46a004

Add local secure_getenv() and fix sizeof call

7cd3f76

rnchamberlain merged commit 1ba381f into nodejs:master Oct 21, 2016

rnchamberlain deleted the initial-drop branch October 21, 2016 09:22

rnchamberlain mentioned this pull request Nov 16, 2016

Improve README.md #23

Closed


		var nodereport = require('nodereport');

		By default, this will trigger a NodeReport to be written to the current

Conversation

rnchamberlain commented Aug 25, 2016

Uh oh!

Fishrock123 commented Aug 29, 2016

Uh oh!

mhdawson commented Aug 29, 2016

Uh oh!

mhdawson commented Aug 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Sep 28, 2016

Uh oh!

rnchamberlain commented Oct 3, 2016

Uh oh!

Fishrock123 commented Oct 4, 2016

Uh oh!

Fishrock123 commented Oct 4, 2016

Uh oh!

rnchamberlain commented Oct 4, 2016

Uh oh!

Fishrock123 commented Oct 4, 2016

Uh oh!

misterdjules commented Oct 4, 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!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Oct 5, 2016

Uh oh!

joshgav Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Oct 11, 2016

Uh oh!

rnchamberlain commented Oct 12, 2016

Uh oh!

sam-github commented Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnchamberlain commented Oct 13, 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!

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 Oct 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Fishrock123 commented Oct 4, 2016 •

edited

Loading

joshgav Oct 6, 2016 •

edited

Loading

sam-github commented Oct 13, 2016 •

edited

Loading

rnchamberlain Oct 17, 2016 •

edited

Loading