src,cli: support compact (one-line) JSON reports

[sam-github]

Multi-line JSON is more human readable, but harder for log aggregators
to consume, they usually require a log message per line, particularly
for JSON. Compact output will be consumable by aggregators such as EFK
(Elastic Search-Fluentd-Kibana), LogDNA, DataDog, etc.

I'll address these in follow-up PRs

I've left this as draft, even though it passes test locally, has docs, etc., because in the process of doing this I've become convinced that JSON output should default to a compact single-line representation, easily consumable by tools, and should *also* default to stdout (or perhaps stderr) as the report destination. The _default_ is important, because reports have a problem in that under some conditions, the command line settings aren't reachable, and you get default behaviour whether you want it or not.

Now, that in itself is actually solvable, at the price of global variables, which we've avoided because some uses Node.js can't access them, so we are trying to isolate all CLI configuration into v8 reachable values, which makes sense, except that the diagnostic report can't always access them, so is possibly not such a good candidate for that limitation.

If the report isn't going to be able to respect the command line flags, specifically, the (new) --report-compact and the pre-existing --report-filename=stdout, then its not possible to integrate into modern logging systems. Pretty much everyone (I couldn't find a counter-example, but probably one exists), has gone 12-factor app as a matter of principle, or is using docker and got forced into logging to stdout. As it is, its not possible to force the diagnostic report to go to stdout always, so the file it forced to disk is going to just get discarded with the container.

The current defaults seem targetted at local development: human readable, written to the CWD. I'm not sure there are good deployment practices that suggest writing log files to the CWD, or even having a CWD that is writable by the user ID node is running.

Given the experimental nature of node reports, on the path to stabilizing them, maybe its time to remove some of the configurability wrt to output, and also provide a one-shot "hook everything" CLI/env option, a user running in deployment wants a diag report on all the abnormal exit paths!

@mcollina (a known one-line json fan) @rmg @richardlau @boneskull @nodejs/diagnostics, thoughts?

Note, humanizing single-line JSON is easy:

cat compact.json | npx pino-pretty
cat compact.json | jq .

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

[addaleax]

Multi-line JSON is more human readable, but harder for log aggregators
to consume, they usually require a log message per line, particularly
for JSON.

That’s true, but at the same time we don’t generally generate more than a single report per process – I personally think of node-report more as a lightweight core dump rather than something that should be logged regularly and appended as multiple lines into a single file.

[sam-github]

lightweight core dump

Yes, specifically, so light weight that it can be represented as JSON and fed into the log aggregation system (which is not quite possible, even with this PR, because --report-filename=stdout isn't respected in all situations).

rather than something that should be logged regularly

Hitting something with SIGUSR2 occaisonally to keep track of the trend is not unreasonable, and doing it when you are begining to suspect an instance of misbehaviour, but not sure enough to terminate it yet, is pretty much the SIGUSR2 use-case.

and appended as multiple lines into a single file.

I hope I didn't imply that I think anything should be written to a file, I would prefer that it write (at least by default) only to stdout or stderr.

[mcollina]

lgtm

[richardlau]

I've left this as draft, even though it passes test locally, has docs, etc., because in the process of doing this I've become convinced that JSON output should default to a compact single-line representation, easily consumable by tools, and should also default to stdout (or perhaps stderr) as the report destination. The default is important, because reports have a problem in that under some conditions, the command line settings aren't reachable, and you get default behaviour whether you want it or not.

I think the "some conditions" are fatal errors where we don't have an env pointer. In that particuar case I'd not be opposed to having defaults to stderr as Node.js/V8 already writes some output to stderr in that case (the VM is basically about to terminate abruptly).

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/29874/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/29889/

[sam-github]

Landed in 8cbbc70

[sam-github]

@targos will there be any blockers to this landing in v12.x?

I notice that #32497 and #32618 got marked blocked, shall I backport all the report-related PRs to v12.x-staging?

[targos]

@sam-github the only blocker is to wait for a semver-minor release. I'll ping you when I prepare the next one if it needs to be backported

[sam-github]

I just checked, this conflicts on cherry pick, as do the others.

I could cherry-pick them one at a time and fix conflicts, but that might cause them to land out of order wrt. the other commits.

I'd really like this to go out in the next minor, though.

https://github.com/sam-github/node/tree/report-v12.x was the first, then picking 2fa74e3 conflicted....

I'll pick this up again after the patch of v12.x, but any suggestions on the order to make backports would be appreciated. Perhaps easiest would be to pull in sam-github@1d8ea18 for this PR, then ping me for every subsequent commit that fails to land, and I can fix them in order?

[targos]

@sam-github do you know what is the reason for the conflicts? maybe they will disappear after I cherry-pick other minor changes that landed before this one?

[sam-github]

We will see. Lines near lines I changed were also changed. Its hard to trace back through the history. I'll wait to hear from you, just pls do ping me, and I will fix any problems you find as quickly as I can.

I do notice that #32497 and #32618 are bug fixes, but they aren't landing in the 12.x bug-fix release. I guess its because they conflict if the semver-minors that preceeded them on master aren't present?

[targos]

I guess its because they conflict if the semver-minors that preceeded them on master aren't present?

Honestly, I don't know. I reviewed more than 300 commits to prepare v12.16.3 and backported almost 200 of them.
Maybe they conflict because of semver-major changes that landed in v13.0.0 but I didn't have enough time to check each commit individually. I think there are enough commits for v12.16.3 and I'll check the rest in order when I do 12.17.0.