Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Mar 13, 2020

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 13, 2020
@addaleax addaleax added the report Issues and PRs related to process.report. label Mar 13, 2020
src/node_report.h Outdated Show resolved Hide resolved
doc/api/cli.md Outdated Show resolved Hide resolved
@addaleax
Copy link
Member

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
Copy link
Contributor Author

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.

src/node_report.h Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

doc/api/process.md Outdated Show resolved Hide resolved
test/common/report.js Outdated Show resolved Hide resolved
src/node_options.cc Outdated Show resolved Hide resolved
doc/api/process.md Outdated Show resolved Hide resolved
@richardlau
Copy link
Member

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).

@sam-github sam-github marked this pull request as ready for review March 16, 2020 18:12
@sam-github sam-github force-pushed the report-compact branch 2 times, most recently from 1e69aff to e57c14f Compare March 16, 2020 18:38
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.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor Author

Landed in 8cbbc70

@sam-github sam-github closed this Mar 17, 2020
sam-github added a commit that referenced this pull request Mar 17, 2020
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.

PR-URL: #32254
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 19, 2020
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.

PR-URL: #32254
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 19, 2020
@sam-github sam-github deleted the report-compact branch March 20, 2020 21:37
MylesBorins pushed a commit that referenced this pull request Mar 24, 2020
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.

PR-URL: #32254
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 22, 2020
@sam-github
Copy link
Contributor Author

@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
Copy link
Member

targos commented Apr 22, 2020

@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 added a commit to sam-github/node that referenced this pull request Apr 22, 2020
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.

PR-URL: nodejs#32254
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@sam-github
Copy link
Contributor Author

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
Copy link
Member

targos commented Apr 22, 2020

@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
Copy link
Contributor Author

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
Copy link
Member

targos commented Apr 22, 2020

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.

targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
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.

PR-URL: nodejs#32254
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
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.

PR-URL: #32254
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. report Issues and PRs related to process.report. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants