[WIP] add node-report to core #19661

richardlau · 2018-03-28T17:57:29Z

This is very early proof-of-concept attempt to add node-report to core, on the basis that it is easier to critique something that exists. It's nowhere near ready to land. I'm not sure how much time I'll get to work on this over Easter, so am submitting what I currently have for comments.

I've started on the basis that we keep node-report as a module so that it can be used with older versions of Node.js, and as such aimed to keep modifications to node-report itself to a minimum (commits prefixed node-report: don't really belong here and ultimately should be upstreamed if this is the direction we want to take).

Currently what I have builds on Linux and sets up node-report to trigger on fatal errors and signals (SIGUSR2). Tested in the repl:

-bash-4.2$ ./node
> process.kill(process.pid, "SIGUSR2");

Writing Node.js report to file: node-report.20180328.135221.96153.001.txt
Node.js report completed
true
>

Obviously needs tests (I have an idea on how to adapt the existing node-report tests by mocking enough of the tap module's interface with core's assert to use with https://github.com/nodejs/node-report/blob/master/test/common.js).

Refs: #18760
Refs: nodejs/node-report#103

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

jasnell · 2018-03-29T10:29:05Z

Adding node-report to core is definitely the right direction but I do believe that it still ought to be an opt-in to use. (the example in the post above suggests that it's potentially always enabled)

joyeecheung · 2018-03-30T16:53:49Z

I agree with @jasnell , for now we should enable it only with -r node-report or another command line flag, which also makes this feature experimental.

richardlau · 2018-04-12T18:44:24Z

for now we should enable it only with -r node-report or another command line flag, which also makes this feature experimental.

ack. I'm leaning towards something like --report-* options (e.g., --report-events fatalerror,signal -- n.b., node-report uses + separators for events, but for a CLI option I think , separated is more common).

Currently working on updating the update script to pull node-report from GitHub instead of npm because we don't publish node-report's tests to npm.

richardlau · 2018-04-12T18:45:26Z

tools/update-node-report.sh

+cd node-report-tmp
+npm init --yes
+
+npm install --global-style --no-bin-links --production --no-package-lock --ignore-scripts node-report@latest


This needs to change to pull from GitHub because we don't publish node-report's tests to npm.

rvagg · 2018-04-12T22:28:01Z

@joyeecheung re your comment in #19903, would enabling this in our tests mean simply adding a cmdline flag to all executions of node in test.py? Is there much overhead in simply enabling node-report? It'd be a shame to extend our already long test run time by much more.

joyeecheung · 2018-04-13T04:01:04Z

@rvagg I meant adding the Flags: comment in flaky tests that crash with fatal errors (I assume we will provide a CLi option to enable node-report) It would be helpful to get the report for those tests if the immediate output to stderr does not provide enough context.

joyeecheung · 2018-04-13T04:05:18Z

@rvagg Although if we are just setting handlers I guess that would not bring too much overhead...except when someone submit a CI that simply fails, then we will be flooded by reports if it is enabled on all tests

gireeshpunathil · 2018-08-14T03:53:21Z

@richardlau - just wanted to know how is this progressing, and if there are any challenges. please let me know if I can be of some help, including fixing any current issues or working on any pending items, thanks!

richardlau · 2018-08-14T18:14:29Z

@gireeshpunathil Thanks for the prompt. I've pushed the current state of my workspace -- This is still very much WIP and the latest commit contains some temporary debugging output (which will make non-related tests that test stdout/stderr to fail as they won't be expecting the extra output).

When I started this PR I had the belief that we could take the source code from https://github.com/nodejs/node-report and drop it into core with minimal tweaking, with a view to keeping the codebases in sync. I'm slowly coming round to believing this is not practical and that we would be better served severing the direct link between the module (https://github.com/nodejs/node-report) and the code that ends up in core based on the hacks I've had to do so far.

For example, if you run the added test/parallel/test-node-report-signal.js you will get a node-report written but it will be missing bits of information that the node-report module obtains from the process object (as the process object is not fully set up at the point node-report initializes). While it's likely the node-report initialization could be addressed (maybe via internalBinding) it would actually be more straightforward for node-report to get the information (i.e. the Node.js and dependency versions) directly since it's in core rather than via the process object, which of course means the tweaks are looking less minimal.

Things like nodejs/node-report#113 are also more easily solved if node-report is in core rather than the stand-alone module since the code in core would only have to deal with the version of Node.js it is in whereas the module has to support all LTS versions (for this reason the standalone module uses nan, but the code in core should be rewritten to not require this dependency).

gireeshpunathil · 2018-08-15T06:39:38Z

@richardlau - thanks for the explanation. let me set my basics right: reviewing your first wave of changes, I believe the design is to retain https://github.com/nodejs/node-report as upstream for bleeding edge development and https://www.npmjs.com/package/node-report as module for external consumption so embed the capability ofnode-report into core with minimal or zero changes as possible (w.r.t upstream). Is this right?

Given that node-report was born in the node.js project family itself and its maintainers part of wider node.js organization, can't we meld this into core with further development happening within core itself as opposed to upstream? Also retire the module from npm registry because we have it in-built in the runtime? If so, we can remove many peripheral entities (index.js, package.json etc.) and integrate this like any other built-in module (such as fs, tcp etc.). What do you think? Who are the relevant stake holders that can take a call on this? thanks.

richardlau · 2018-08-16T05:25:55Z

thanks for the explanation. let me set my basics right: reviewing your first wave of changes, I believe the design is to retain https://github.com/nodejs/node-report as upstream for bleeding edge development and https://www.npmjs.com/package/node-report as module for external consumption so embed the capability of node-report into core with minimal or zero changes as possible (w.r.t upstream). Is this right?

Yes, that was my original thought. The idea was that we could then slowly transition over to the code in core once integrated (we would still have LTS releases that would need the module unless of course we backport).

Given that node-report was born in the node.js project family itself and its maintainers part of wider node.js organization, can't we meld this into core with further development happening within core itself as opposed to upstream? Also retire the module from npm registry because we have it in-built in the runtime? If so, we can remove many peripheral entities (index.js, package.json etc.) and integrate this like any other built-in module (such as fs, tcp etc.). What do you think?

Yes, this is what I meant by "severing the direct link between the module (https://github.com/nodejs/node-report) and the code that ends up in core". By "retire the module from npm registry" we would just not update it (or update it infrequently) -- there are other modules currently depending on the node-report module (https://www.npmjs.com/package/node-report?activeTab=dependents).

Who are the relevant stake holders that can take a call on this? thanks.

"The node-report project falls under the governance of the post-mortem working group". Note though that there is a proposal to fold the Post-mortem WG into the Diagnostics WG (nodejs/TSC#586).

gireeshpunathil · 2018-08-16T05:55:09Z

thanks @richardlau - this makes perfect sense. Yes, I see your proposal on
nodejs/TSC#586 now.

So anticipating that post-mortem will fold into diagnostics and diagnostics will endorse the intent of this PR, the most appropriate direction seems to be to port node-report natively into core.

I will take a look at your last set of changes and see where are we.

gireeshpunathil · 2018-08-16T14:23:43Z

deps/node-report/binding.gyp

+ },
+ ],
+}
+


do we still need this? can't we accommodate the procedures in node.gyp?

No, it's not needed. It's not actually used in the core build (see changes to node.gyp).

This was pulled in by the tools/update-node-report.sh script -- The original intention was to periodically update the source in core from the module and the script was added first to do so. Other than running dmn no other clean up is done in the script (no reason not to but then if we're breaking the direct link between the module and the code in core the update script is unnecessary).

gireeshpunathil · 2018-08-16T14:25:32Z

deps/node-report/index.js

+exports.setSignal = api.setSignal;
+exports.setFileName = api.setFileName;
+exports.setDirectory = api.setDirectory;
+exports.setVerbose = api.setVerbose;


will these be exported here, as opposed to util module as suggested by other reviewers?

No, this file is not used in core. At the moment no JS api is implemented in core.

gireeshpunathil · 2018-08-16T14:26:51Z

deps/node-report/node_modules/nan/LICENSE.md

+
+The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.


can we eliminate the need for nan altogether and work directly with v8? following other Node.js absrtactions?

Yes. In fact if we're breaking the link between the module and the code in core there is no reason to use nan and can work directly with V8.

Long term we might look at using n-api.

gireeshpunathil · 2018-08-16T14:28:15Z

deps/node-report/package.json

+ "devDependencies": {
+ "tap": "^8.0.0"
+ }
+}


could we elevate the version number to node_version.h and eliminate the need for this file?

In my opinion once this is in core and there is no longer a link to the module node-report doesn't require a version because it is no longer a dependency (i.e. it versions with the version of Node.js that is released).

gireeshpunathil · 2018-08-16T14:31:19Z

src/node.cc

+ report_events.replace(c, 1, "+");
+ c = report_events.find_first_of(",", c + 1);
+ }
+ fprintf(stderr, "filtered events %s \n", report_events.c_str());


can we have a default value for --report-events ? i.e., in case if user specifies this flag but no values, how about picking the most relevant trace events and feed those into report_events filed?

gireeshpunathil · 2018-08-16T14:32:15Z

tools/getnodereportversion.py

+ raise Exception('Could not find pattern matching %s' % regex)
+
+if __name__ == '__main__':
+ print(get_version())


as suggested earlier, if we are porting natively, can we eliminate this altogether, and record the version statically?

In my opinion once this is in core and there is no longer a link to the module node-report doesn't require a version because it is no longer a dependency (i.e. it versions with the version of Node.js that is released).

gireeshpunathil · 2018-08-16T14:34:40Z

deps/node-report/README.md

+reports can be triggered on unhandled exceptions, fatal errors, signals
+and calls to a JavaScript API.
+
+Supports Node.js versions 4, 6 and 8 on AIX, Linux, MacOS, SmartOS and Windows.


probably change this to reflect in which versions it is available in core and in which versions to use external package?

We'd drop this readme (but we do need to add some sort of documentation).

gireeshpunathil · 2018-08-16T14:35:03Z

deps/node-report/README.md

+## Usage
+
+```bash
+npm install node-report


irrelevant now?

We'd drop this readme (but we do need to add some sort of documentation).

gireeshpunathil · 2018-08-16T14:35:32Z

deps/node-report/README.md

+
+```bash
+npm install node-report
+node -r node-report app.js


-r -> --report_events ?

We'd drop this readme (but we do need to add some sort of documentation).

gireeshpunathil · 2018-08-16T14:37:21Z

deps/node-report/README.md

+application.
+
+```js
+var nodereport = require('node-report');


if we are launching with --report-events do we still need the require?
if the JS interface is going to util package, what the require() will return?

We'd drop this readme (but we do need to add some sort of documentation).

gireeshpunathil · 2018-08-16T14:46:56Z

Left some comments. Predominantly it is all pointing at the level of fusing we wanted and how much we want to baseline on the upstream. For example:

eliminate /deps/node-report and bring the source into node/src, test into node/test, eliminate binding.gyp and bring build script into node.gyp etc. - through restructuring, no further dependency with nodejs/node-repot, further work happens here.
retain /deps/node-report' but remove nan` dependency, and retain everything else - partial restructuring: porting is easy, maintenance may be tough
retain everything, but bring minimal changes for building and running it in core - current approach?

richardlau · 2018-08-16T16:55:22Z

@gireeshpunathil I'm leaning towards eliminating deps/node-report altogether and implementing node-report directly as a file(s) in src. In which case the dependency on nan should be removed.

Using node-report in core would be different from using node-report as a module, so the documentation would have to be rewritten.

Activating node-report should be via command line parameters (i.e. --report-events) so that, at least to start with, it is opt-in. Command line parameters have the advantage that they can be whitelisted for use in the NODE_OPTIONS environment variable. I have no firm idea what a JS api (suggested elsewhere to be on util) might look like in core.

Make node-report part of core runtime, to satisfy its tier1 status on diagnostic tooling. No new functionalities have been added, changes that are required for melding it as a built-in capability has been affected on the module version of node-report (https://github.com/nodejs/node-report) Refs: nodejs#19661 Refs: nodejs#18760 Refs: nodejs/node-report#103

richardlau · 2018-09-05T17:24:22Z

Closing in favor of #22712.

richardlau added the wip Issues and PRs that are still a work in progress. label Mar 28, 2018

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Mar 28, 2018

richardlau mentioned this pull request Mar 29, 2018

Add prebuilt binaries nodejs/node-report#109

Closed

richardlau commented Apr 12, 2018

View reviewed changes

mhdawson mentioned this pull request Apr 12, 2018

Moving node-report into core nodejs/node-report#110

Closed

joyeecheung mentioned this pull request Apr 12, 2018

CI failures with no output/stack trace #19903

Closed

joyeecheung mentioned this pull request May 31, 2018

Agenda for Diagnostics session at collaborator summit openjs-foundation/summit#71

Closed

richardlau added 6 commits August 14, 2018 13:34

tools: add node-report update script

71b475e

deps: add node-report@2.2.1

5b0ec0c

node-report: expose initialize function

8c3290a

build: add node-report

dacd0d9

test: add node-report tests

9b5fb8a

squash: commit test tweaks

c54ebfd

richardlau force-pushed the node-report branch from 967b81e to c54ebfd Compare August 14, 2018 17:50

gireeshpunathil reviewed Aug 16, 2018

View reviewed changes

gireeshpunathil mentioned this pull request Sep 5, 2018

node-report: meld into core #22712

Merged

4 tasks

richardlau closed this Sep 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] add node-report to core #19661

[WIP] add node-report to core #19661

richardlau commented Mar 28, 2018

jasnell commented Mar 29, 2018

joyeecheung commented Mar 30, 2018

richardlau commented Apr 12, 2018

richardlau Apr 12, 2018

rvagg commented Apr 12, 2018

joyeecheung commented Apr 13, 2018

joyeecheung commented Apr 13, 2018

gireeshpunathil commented Aug 14, 2018

richardlau commented Aug 14, 2018

gireeshpunathil commented Aug 15, 2018

richardlau commented Aug 16, 2018

gireeshpunathil commented Aug 16, 2018

gireeshpunathil Aug 16, 2018

richardlau Aug 16, 2018

gireeshpunathil Aug 16, 2018

richardlau Aug 16, 2018

gireeshpunathil Aug 16, 2018

richardlau Aug 16, 2018

gireeshpunathil Aug 16, 2018

richardlau Aug 16, 2018

gireeshpunathil Aug 16, 2018

gireeshpunathil Aug 16, 2018

richardlau Aug 16, 2018

gireeshpunathil Aug 16, 2018

richardlau Aug 16, 2018

gireeshpunathil Aug 16, 2018

richardlau Aug 16, 2018

gireeshpunathil Aug 16, 2018

richardlau Aug 16, 2018

gireeshpunathil Aug 16, 2018

richardlau Aug 16, 2018

gireeshpunathil commented Aug 16, 2018

richardlau commented Aug 16, 2018

richardlau commented Sep 5, 2018


		The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

		THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.

[WIP] add node-report to core #19661

[WIP] add node-report to core #19661

Conversation

richardlau commented Mar 28, 2018

Checklist

jasnell commented Mar 29, 2018

joyeecheung commented Mar 30, 2018

richardlau commented Apr 12, 2018

Choose a reason for hiding this comment

rvagg commented Apr 12, 2018

joyeecheung commented Apr 13, 2018

joyeecheung commented Apr 13, 2018

gireeshpunathil commented Aug 14, 2018

richardlau commented Aug 14, 2018

gireeshpunathil commented Aug 15, 2018

richardlau commented Aug 16, 2018

gireeshpunathil commented Aug 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gireeshpunathil commented Aug 16, 2018

richardlau commented Aug 16, 2018

richardlau commented Sep 5, 2018