Skip to content
This repository has been archived by the owner on Jun 18, 2021. It is now read-only.

Add node-report to node core #103

Closed
jasnell opened this issue Oct 9, 2017 · 16 comments
Closed

Add node-report to node core #103

jasnell opened this issue Oct 9, 2017 · 16 comments

Comments

@jasnell
Copy link
Member

jasnell commented Oct 9, 2017

I would like to propose that the node-report module be added as a new core module, with the ability to trigger it via command line flag as opposed to preload module, e.g. node --node-report.

This would require a few steps:

  • Parts of the code would need to be reworked to not depend on NaN
  • Tests would need to be refactored to not depend on tap
  • We would need a strategy for continuing to support the standalone native module for older Node.js versions (much like we do with readable-streams.

Ping @mhdawson

@rnchamberlain
Copy link
Contributor

A couple of thoughts:

  • Would we need to add support for any more platforms?
  • Would the user configuration via env vars still be ok? Might be nice if some of that was via the command line

@sam-github
Copy link
Contributor

more thoughts:

  • cli config is easier for interactive users, but env config is easier for cloud and production environments (where sometimes CLI config is close to impossible)
  • there might need to be some dual maintenance so node-report is still available on LTS lines, though backporting a node integration to LTS lines would help with that

@gibfahn
Copy link
Member

gibfahn commented Oct 10, 2017

cli config is easier for interactive users, but env config is easier for cloud and production environments (where sometimes CLI config is close to impossible)

Doesn't NODE_OPTIONS fix that?

@sam-github
Copy link
Contributor

Oh, yes, I guess it does. Handy feature, that one.

@cjihrig
Copy link

cjihrig commented Oct 10, 2017

Just out of curiosity, why do we need/want to do this? Is there something missing by having it outside of core?

@rnchamberlain
Copy link
Contributor

There has been quite a bit of debate already on inside vs outside core, and on the whole the "keep core small" argument seems to win, eg:
nodejs/post-mortem#44
https://developer.ibm.com/node/2017/04/20/keeping-node-js-core-small/

On the other hand, the node and v8 APIs that we can use to trigger diagnostics and to obtain useful information when things go wrong are quite limited, and IMO we can do a better job if we are inside core. eg see:
nodejs/node#15335

@mhdawson
Copy link
Member

@cjihrig I believe there is value in having key diagnostic tools bundled in, particularly when they require compilation.

In the case of node-report once you have a problem, if it is there it is easy to ask ops to grab the reports. If its not already there it requires installing additional code to production, this may need a compiler that is not already installed and may also require additional organization review/legal ok. I understand this could be done beforehand but I believe that there will be enough cases were this has not been done that bundling will make life easier for end users.

@rnchamberlain ideally it should have support for all of the platforms supported by Node.js, are we missing some ?

@jasnell
Copy link
Member Author

jasnell commented Oct 11, 2017

+1 to "there is value in having key diagnostic tools bundled in, particularly when they require compilation"

I have been hearing consistently from customers that they want better primitives and tools included with Node.js by default that would help them diagnose issues.

@rnchamberlain
Copy link
Contributor

@mhdawson re platforms, we currently run the CI on these: debian8-64 fedora22 fedora23 osx1010 ppcle-ubuntu1404 ubuntu1204-64 ubuntu1404-64 ubuntu1604-64 rhel72-s390x
ppcbe-ubuntu1404 smartos16-64 smartos15-64 smartos14-64 win10 win2012r2

From the list here, I think we are just missing FreeBSD
https://github.com/nodejs/node/blob/master/BUILDING.md#building-nodejs-on-supported-platforms

@sam-github
Copy link
Contributor

I'm a big fan of small core, but I've also been on too many calls where customers have problems, but have not prepared to solve them by deploying troubleshooting tools. It can be pretty epic getting compiled node modules onto production machines when the Ops policies are "no compilers on production". It means finding an architecturally similar machine, installing node development tools, building, then sometimes scping through multiple levels. "Set this env var, restart, and send us the report" is a much better story.

@gibfahn
Copy link
Member

gibfahn commented Oct 11, 2017

+1 to "there is value in having key diagnostic tools bundled in, particularly when they require compilation"

Wouldn't prebuilt binaries (e.g. with node-pre-gyp) solve that problem?

@sam-github
Copy link
Contributor

node-pre-gyp is good for home desktop windows users, it allows installation without needing compiler tool chains.

But it substitutes the vs install problem with another problem: it requires arbitrary outbound network access to a CDN, something which is often firewalled out in corportate environments. Even environments that have specifically opened an outbound firewall hole to npmjs.org or have an npm registry proxy won't work with pre-gyp. The appmetrics approach is better: it includes the pre-built artifacts in the npm installable bundle.

@gibfahn
Copy link
Member

gibfahn commented Oct 12, 2017

The appmetrics approach is better: it includes the pre-built artifacts in the npm installable bundle.

Ahh, I thought node-pre-gyp did this (or had an option to). This is what I was suggesting.

@Fishrock123
Copy link
Contributor

Perhaps we can build in the C++ bits into some diagnostics API and then use that to produce reports from a more external module ala node-report?

@cjihrig
Copy link

cjihrig commented Oct 16, 2017

Big +1 to @Fishrock123's suggestion.

gireeshpunathil added a commit to gireeshpunathil/node that referenced this issue Sep 4, 2018
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
gireeshpunathil added a commit to gireeshpunathil/node that referenced this issue Sep 4, 2018
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
gireeshpunathil added a commit to gireeshpunathil/node that referenced this issue Sep 5, 2018
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
gireeshpunathil added a commit to gireeshpunathil/node that referenced this issue Sep 5, 2018
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
@gireeshpunathil
Copy link
Member

fyi - nodejs/node#22712 is merged now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants