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.

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

node-report: meld into core #22712

Merged
merged 3 commits into from Jan 18, 2019

Conversation

@gireeshpunathil
Copy link
Member

gireeshpunathil commented Sep 5, 2018

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

This PR adds the the process.report tool.

  1. When enabled, node-report significantly helps root cause various types of problems, including support issues sent to the various repos of the Node.js organization

  2. The requirement of explicitly adding the dependency to node-report in users' applications often represents a blocker to adoption and thus cancels 1) benefits

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)

Changes at a glance:

  • opt-in feature for auto-generation of report through --report-events
  • APIs exposed through util module. APIs are enabled by default.
  • opt-out at configure time through --without-node-report
  • only minimal refactoring to natively merge the module
  • tests covering all the APIs and report generating events
  • a new page for node-report in the document section

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

additional /cc

@rnchamberlain : original author of this component
@richardlau : previous work on merging this into core
@mhdawson : orignal requirement raiser through nodejs/node-report#110
@nodejs/diagnostics : in relation to nodejs/TSC#586
@jasnell @rvagg @joyeecheung : in relation to their specific requirements on interfaces /defaults (#19661)

@gireeshpunathil gireeshpunathil requested review from BridgeAR and bnoordhuis Sep 5, 2018


```js
const util = require('util');
const report_str = util.getReport();

This comment has been minimized.

@mcollina

mcollina Sep 5, 2018

Member

I would prefer this to go into @nodejs/report instead as there is some consensus in that direction.
Otherwise, I would go for util.report.generate() or something similar.

This comment has been minimized.

@jasnell

jasnell Sep 5, 2018

Member

Another possibility would be util.generateDiagnosticReport(). I'm not convinced that a new standalone module (even scoped) is necessary for this.

This comment has been minimized.

@mcollina

mcollina Sep 5, 2018

Member

My overall concern is that this adds 3 different methods to util. Moreover, the docs for this is separate. Even if we do not want a separate method, just having a util.report namespace would be more user friendly.

(I would also avoid to load this until util.report  is accessed).

This comment has been minimized.

@jasnell

jasnell Sep 5, 2018

Member

I'd be good with util.report with the methods:

  • util.report.getDiagnosticReport([error])
  • util.report.generateDiagnosticReport([filename][, error])
  • util.report.setDiagnosticReportOptions(options)

This comment has been minimized.

@joyeecheung

joyeecheung Oct 4, 2018

Member

@jasnell Can we use some abstractions (classes?) instead of going with aMethodWith20Letters?

This comment has been minimized.

@vipinmenon

vipinmenon Oct 6, 2018

Contributor

node report is with util.report now and the documentation is updated accordingly.

Show resolved Hide resolved AUTHORS Outdated
Show resolved Hide resolved doc/api/node_report.md Outdated
Show resolved Hide resolved doc/api/node_report.md Outdated
Show resolved Hide resolved doc/api/node_report.md Outdated
Show resolved Hide resolved doc/api/node_report.md Outdated
Show resolved Hide resolved doc/api/node_report.md Outdated
Show resolved Hide resolved doc/api/util.md Outdated
Show resolved Hide resolved doc/api/util.md Outdated
Show resolved Hide resolved doc/api/util.md Outdated
Show resolved Hide resolved doc/api/util.md Outdated
Show resolved Hide resolved doc/api/util.md Outdated
Show resolved Hide resolved src/node_config.cc Outdated
Show resolved Hide resolved src/node_options.cc Outdated
Show resolved Hide resolved src/node_options.cc Outdated
Show resolved Hide resolved src/node_report_module.cc Outdated
Show resolved Hide resolved src/node_util.cc Outdated
Show resolved Hide resolved test/node-report/helper.js Outdated
@jasnell
Copy link
Member

jasnell left a comment

Good start. Needs a bit of work. Left some initial comments.

@richardlau
Copy link
Member

richardlau left a comment

Thanks @gireeshpunathil! Did a cursory glance over and left some comments. Will follow up with a proper technical review.

Docs should note that this feature is experimental (at least initially). I also have a preference for calling the report diagnostics report (or something similar) instead of node-report in core as I find the node- prefix redundant.

Show resolved Hide resolved doc/api/node_report.md Outdated
Show resolved Hide resolved doc/api/util.md Outdated
Show resolved Hide resolved src/node_options.cc Outdated
Show resolved Hide resolved test/node-report/helper.js Outdated
Show resolved Hide resolved node.gyp Outdated
Show resolved Hide resolved doc/api/util.md Outdated
Show resolved Hide resolved doc/api/util.md Outdated
Show resolved Hide resolved doc/api/util.md Outdated
Show resolved Hide resolved doc/api/util.md Outdated
Show resolved Hide resolved src/node_report.h Outdated

@richardlau richardlau referenced this pull request Sep 5, 2018

Closed

[WIP] add node-report to core #19661

0 of 4 tasks complete
@misterdjules

This comment has been minimized.

Copy link
Contributor

misterdjules commented Sep 5, 2018

@gireeshpunathil

Make node-report part of core runtime, to satisfy its tier1 status
on diagnostic tooling

What part of the tier1 status requires something to be part of Node.js' core?

Show resolved Hide resolved doc/api/node_report.md Outdated
Show resolved Hide resolved doc/api/node_report.md Outdated
Show resolved Hide resolved doc/api/node_report.md Outdated
Show resolved Hide resolved src/node_report.h Outdated
Show resolved Hide resolved src/node_report.cc Outdated
Show resolved Hide resolved src/node_report_module.cc Outdated
Show resolved Hide resolved src/node_report_module.cc Outdated
Show resolved Hide resolved src/node_report_module.cc Outdated
Show resolved Hide resolved src/node_report_module.cc Outdated
Show resolved Hide resolved src/node_report_utils.cc Outdated

vipinmenon and others added some commits Sep 5, 2018

doc: add node-report documentation
a separate section added for node-report at top level
main documentation added to doc/api/report.md
API documentation added to doc/api/process.md

PR-URL: #22712
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <Michael_Dawson@ca.ibm.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
test: add node-report tests
One test per each API, so that additional tests in future are modular.
test/common/report.js contain common functions  that tests leverage.

PR-URL: #22712
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <Michael_Dawson@ca.ibm.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

@gireeshpunathil gireeshpunathil force-pushed the gireeshpunathil:nrmerge branch from 489183c to 55e0ad9 Jan 18, 2019

@gireeshpunathil gireeshpunathil merged commit 55e0ad9 into nodejs:master Jan 18, 2019

@gireeshpunathil

This comment has been minimized.

Copy link
Member Author

gireeshpunathil commented Jan 18, 2019

thanks to all who participated, contributed and reviewed; it has been a pleasure working on this.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 19, 2019

For releasers: This was landed in 01cd219...55e0ad9

We also may want to add a note to the entry for the first commit in the changelog, because the commit title tells very little about the change itself.

@gireeshpunathil

This comment has been minimized.

Copy link
Member Author

gireeshpunathil commented Jan 19, 2019

@addaleax - thnx, can I know where? (to the bottom of 4f67973 ?) and what? (general description about the report, or the changed files, or something else?)

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 19, 2019

@gireeshpunathil I don’t know where – I think here is fine? And basically I’m just looking for a very short description of the commit that would fit into our one-line changelog, to explain the commit message title, e.g. (This commit adds the the process.report tool)?

@gireeshpunathil

This comment has been minimized.

Copy link
Member Author

gireeshpunathil commented on 4f67973 Jan 19, 2019

for posterity: This commit adds the the process.report tool

@gireeshpunathil

This comment has been minimized.

Copy link
Member Author

gireeshpunathil commented Jan 19, 2019

added the one liner to the head or the PR description, as well as to the bottom of the main commit.

@addaleax addaleax added the report label Jan 22, 2019

targos added a commit that referenced this pull request Jan 24, 2019

src: merge into core
Make node-report part of core runtime because:

1. When enabled, node-report significantly helps root cause various
types of problems, including support issues sent to the various repos
of the Node.js organization.

2. The requirement of explicitly adding the dependency to node-report
in user applications often represents a blocker to adoption.

Major deviation from the module version of the node-report is that the
report is generated in JSON format, as opposed to human readable text.

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)

Co-authored-by: Bidisha Pyne <bidipyne@in.ibm.com>
Co-authored-by: Howard Hellyer <hhellyer@uk.ibm.com>
Co-authored-by: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Co-authored-by: Julian Alimin <dmastag@yahoo.com>
Co-authored-by: Lakshmi Swetha Gopireddy <lakshmigopireddy@in.ibm.com>
Co-authored-by: Manusaporn Treerungroj <m.treerungroj@gmail.com>
Co-authored-by: Michael Dawson <michael_dawson@ca.ibm.com>
Co-authored-by: Richard Chamberlain <richard_chamberlain@uk.ibm.com>
Co-authored-by: Richard Lau <riclau@uk.ibm.com>
Co-authored-by: Sam Roberts <vieuxtech@gmail.com>
Co-authored-by: Vipin Menon <vipinmv1@in.ibm.com>

PR-URL: #22712
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <Michael_Dawson@ca.ibm.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

targos added a commit that referenced this pull request Jan 24, 2019

doc: add node-report documentation
a separate section added for node-report at top level
main documentation added to doc/api/report.md
API documentation added to doc/api/process.md

PR-URL: #22712
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <Michael_Dawson@ca.ibm.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

targos added a commit that referenced this pull request Jan 24, 2019

test: add node-report tests
One test per each API, so that additional tests in future are modular.
test/common/report.js contain common functions  that tests leverage.

PR-URL: #22712
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <Michael_Dawson@ca.ibm.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

MylesBorins added a commit that referenced this pull request Jan 24, 2019

2019-01-224, Version 11.8.0 (Current)
Notable Changes:

* events:
  * For unhandled `error` events with an argument that is not an
    `Error` object, the resulting exeption will have more information
    about the argument.
    #25621
* child_process:
  * When the `maxBuffer` option is passed, `stdout` and `stderr` will
    be truncated rather than unavailable in case of an error.
    #24951
* policy:
  * Experimental support for module integrity checks through a manifest
    file is implemented now.
    #23834
* n-api:
  * The `napi_threadsafe_function` feature is now stable.
    #25556
* report:
  * An experimental diagnostic API for capturing process state is
    available as `process.report` and through command line flags.
    #22712
* tls:
  * `tls.connect()` takes a `timeout` option analogous to the
    `net.connect()` one.
    #25517
* worker:
  * `process.umask()` is available as a read-only function inside Worker
    threads now.
    #25526
  * An `execArgv` option that supports a subset of Node.js command line
    options is supported now.
    #25467

PR-URL: Coming Soon

@MylesBorins MylesBorins referenced this pull request Jan 24, 2019

Merged

v11.8.0 proposal #25687

MylesBorins added a commit that referenced this pull request Jan 24, 2019

2019-01-224, Version 11.8.0 (Current)
Notable Changes:

* events:
  * For unhandled `error` events with an argument that is not an
    `Error` object, the resulting exeption will have more information
    about the argument.
    #25621
* child_process:
  * When the `maxBuffer` option is passed, `stdout` and `stderr` will
    be truncated rather than unavailable in case of an error.
    #24951
* policy:
  * Experimental support for module integrity checks through a manifest
    file is implemented now.
    #23834
* n-api:
  * The `napi_threadsafe_function` feature is now stable.
    #25556
* report:
  * An experimental diagnostic API for capturing process state is
    available as `process.report` and through command line flags.
    #22712
* tls:
  * `tls.connect()` takes a `timeout` option analogous to the
    `net.connect()` one.
    #25517
* worker:
  * `process.umask()` is available as a read-only function inside Worker
    threads now.
    #25526
  * An `execArgv` option that supports a subset of Node.js command line
    options is supported now.
    #25467

PR-URL: #25687

MylesBorins added a commit that referenced this pull request Jan 24, 2019

2019-01-24, Version 11.8.0 (Current)
Notable Changes:

* events:
  * For unhandled `error` events with an argument that is not an
    `Error` object, the resulting exeption will have more information
    about the argument.
    #25621
* child_process:
  * When the `maxBuffer` option is passed, `stdout` and `stderr` will
    be truncated rather than unavailable in case of an error.
    #24951
* policy:
  * Experimental support for module integrity checks through a manifest
    file is implemented now.
    #23834
* n-api:
  * The `napi_threadsafe_function` feature is now stable.
    #25556
* report:
  * An experimental diagnostic API for capturing process state is
    available as `process.report` and through command line flags.
    #22712
* tls:
  * `tls.connect()` takes a `timeout` option analogous to the
    `net.connect()` one.
    #25517
* worker:
  * `process.umask()` is available as a read-only function inside Worker
    threads now.
    #25526
  * An `execArgv` option that supports a subset of Node.js command line
    options is supported now.
    #25467

PR-URL: #25687

MylesBorins added a commit that referenced this pull request Jan 24, 2019

2019-01-24, Version 11.8.0 (Current)
Notable Changes:

* events:
  * For unhandled `error` events with an argument that is not an
    `Error` object, the resulting exeption will have more information
    about the argument.
    #25621
* child_process:
  * When the `maxBuffer` option is passed, `stdout` and `stderr` will
    be truncated rather than unavailable in case of an error.
    #24951
* policy:
  * Experimental support for module integrity checks through a manifest
    file is implemented now.
    #23834
* n-api:
  * The `napi_threadsafe_function` feature is now stable.
    #25556
* report:
  * An experimental diagnostic API for capturing process state is
    available as `process.report` and through command line flags.
    #22712
* tls:
  * `tls.connect()` takes a `timeout` option analogous to the
    `net.connect()` one.
    #25517
* worker:
  * `process.umask()` is available as a read-only function inside Worker
    threads now.
    #25526
  * An `execArgv` option that supports a subset of Node.js command line
    options is supported now.
    #25467

PR-URL: #25687

MylesBorins added a commit that referenced this pull request Jan 25, 2019

2019-01-24, Version 11.8.0 (Current)
Notable Changes:

* events:
  * For unhandled `error` events with an argument that is not an
    `Error` object, the resulting exeption will have more information
    about the argument.
    #25621
* child_process:
  * When the `maxBuffer` option is passed, `stdout` and `stderr` will
    be truncated rather than unavailable in case of an error.
    #24951
* policy:
  * Experimental support for module integrity checks through a manifest
    file is implemented now.
    #23834
* n-api:
  * The `napi_threadsafe_function` feature is now stable.
    #25556
* report:
  * An experimental diagnostic API for capturing process state is
    available as `process.report` and through command line flags.
    #22712
* tls:
  * `tls.connect()` takes a `timeout` option analogous to the
    `net.connect()` one.
    #25517
* worker:
  * `process.umask()` is available as a read-only function inside Worker
    threads now.
    #25526
  * An `execArgv` option that supports a subset of Node.js command line
    options is supported now.
    #25467

PR-URL: #25687

@addaleax addaleax referenced this pull request Feb 4, 2019

Open

report: catch internal segfaults #25915

2 of 4 tasks complete

BethGriggs added a commit that referenced this pull request Feb 12, 2019

lib: rearm pre-existing signal event registrations
process.on('somesignal', ...) semantics expect the process to catch the
signal and invoke the associated handler. `setupSignalHandlers` perform
the additional task of preparing the libuv signal handler and associate
it with the event handler. It is possible that by the time this is
setup there could be pre-existing registrations that pre-date this setup
in the boot sequence.

So rearm pre-existing signal event registrations to get upto speed.

Ref: #22712 (comment)

PR-URL: #24651
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment