lib: create diagnostics_channel module by Qard · Pull Request #34895 · nodejs/node

Qard · 2020-08-23T20:13:02Z

I've continued the effort started in nodejs/diagnostics#180 to create an API focused on high-performance reporting of arbitrary diagnostics data via named channels. The intent is to layer some more complex concepts on top of this event stream to produce things like metrics or spans for tracing. This is drawing on some ideas from a previous experiment of mine to build a generic data pipeline abstracted by more structural constructs here: https://github.com/Qard/universal-monitoring-bus/blob/master/example.js

This is still somewhat a work-in-progress, but is usable in its current state and should already perform fairly well. It is carefully optimized to avoid lookups and to pull expensive behaviours as much as possible out of the publish path and put the expense exclusively on subscription and channel creation. There is some duplication in the API as it is currently as I want to present this to the community and gather feedback on what parts should stay, what parts should go, and also if there are any things that should be added or changed.

Also, the reason for making this a core module and not just something on npm is that the intent is largely to use this to gather diagnostics data from within Node.js core itself, which requires that it exist within Node.js. It is designed as a public API so that some other userland libraries may also make use of it to report diagnostic data. The hope is for an API such as this to largely eliminate the need for APM vendors to patch Node.js core, the module loader and much of the external module ecosystem, which is somewhat fragile and has a significant performance impact.

Please keep in mind:

APM vendors very much need a solution to eliminate the need for extensive patching
This is very much an experiment and is likely to evolve significantly before considering actually landing it
I understand there is likely to be pushback to this for various reasons. I'll be working to alleviate any concerns that are shared here. If that requires significant changes, so be it.

cc @nodejs/diagnostics

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

bmeck · 2020-08-23T21:31:38Z

It would be good to give example reasons on when/why to use these APIs rather than what these APIs do.

lib/diagnostics_channel.js

Qard · 2020-08-23T21:39:51Z

Yep, planning on expanding docs further. I just put together the current docs today as a quick intro to how it works. I'll continue to build out the docs further as API details are solidified more and we build a clearer idea of the higher level constructs to layer over top of it. I plan to discuss this at the next diagnostics meeting to figure out if this is a sufficient code to the higher-level APIs we're thinking of.

lib/diagnostics_channel.js

mcollina

Have you researched approaches based on a trie, e.g. https://www.npmjs.com/package/qlobber?

doc/api/diagnostics_channel.md

lib/diagnostics_channel.js

doc/api/diagnostics_channel.md

lib/diagnostics_channel.js

Qard · 2020-08-24T06:29:01Z

No, I have not really looked into possible trie uses. I figured more complicated matching could just be implemented on top of the shouldListen function. I expect the majority of cases would be that one subscribing to events would listen to specific channels and that more complicated matching would be somewhat of a niche need--I'm not even 100% certain it's needed.

mcollina · 2020-08-24T07:30:02Z

The advantage of a trie data structure is to avoid the memory leak this implementation has.

doc/api/diagnostics_channel.md

github-actions · 2020-10-29T17:20:18Z

Commit Queue failed

- Loading data for nodejs/node/pull/34895
✔  Done loading data for nodejs/node/pull/34895
----------------------------------- PR info ------------------------------------
Title      lib: create diagnostics_channel module (#34895)
Author     Stephen Belanger  (@Qard)
Branch     Qard:diagnostics-channel -> nodejs:master
Labels     author ready, build, diag-agenda, experimental, lib / src, notable-change, semver-minor, tsc-agenda
Commits    2
 - lib: create diagnostics_channel module
 - http: report request start and end with diagnostics_channel
Committers 1
 - Stephen Belanger 
PR-URL: https://github.com/nodejs/node/pull/34895
Reviewed-By: Bryan English 
Reviewed-By: Gerhard Stöbich 
Reviewed-By: Vladimir de Turckheim 
Reviewed-By: Rich Trott 
Reviewed-By: Gabriel Schulhof 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/34895
Reviewed-By: Bryan English 
Reviewed-By: Gerhard Stöbich 
Reviewed-By: Vladimir de Turckheim 
Reviewed-By: Rich Trott 
Reviewed-By: Gabriel Schulhof 
--------------------------------------------------------------------------------
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-10-08T21:33:28Z: https://ci.nodejs.org/job/node-test-pull-request/33479/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - lib: create diagnostics_channel module
   ⚠  - http: report request start and end with diagnostics_channel
- Querying data for job/node-test-pull-request/33479/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Sun, 23 Aug 2020 20:13:02 GMT
   ✔  Approvals: 5
   ✔  - Bryan English (@bengl): https://github.com/nodejs/node/pull/34895#pullrequestreview-505212451
   ✔  - Gerhard Stöbich (@Flarna): https://github.com/nodejs/node/pull/34895#pullrequestreview-504258780
   ✔  - Vladimir de Turckheim (@vdeturckheim): https://github.com/nodejs/node/pull/34895#pullrequestreview-505186641
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/34895#pullrequestreview-519770538
   ✔  - Gabriel Schulhof (@gabrielschulhof) (TSC): https://github.com/nodejs/node/pull/34895#pullrequestreview-519770808
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

Commit Queue action: https://github.com/nodejs/node/actions/runs/336348757

mhdawson

LGTM

nodejs-github-bot · 2020-10-30T17:24:10Z

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

Qard · 2020-10-30T18:32:35Z

I'm getting tons of flaky tests from completely unrelated parts of the test suite. Can't seem to get that CI to pass. 😩

MylesBorins · 2020-10-30T18:54:24Z

@Qard I have some PRs to mark tests flaky. One just landed the other should land in the next 30 ish. I'll rebase and kick off tests again once it lands if you like

Qard · 2020-10-30T18:55:47Z

I can do it, just ping me here. :)

nodejs-github-bot · 2020-10-30T23:17:05Z

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

github-actions · 2020-10-31T00:47:14Z

Commit Queue failed

- Loading data for nodejs/node/pull/34895
✔  Done loading data for nodejs/node/pull/34895
----------------------------------- PR info ------------------------------------
Title      lib: create diagnostics_channel module (#34895)
Author     Stephen Belanger  (@Qard)
Branch     Qard:diagnostics-channel -> nodejs:master
Labels     author ready, build, commit-queue-failed, diag-agenda, experimental, lib / src, notable-change, semver-minor
Commits    2
 - lib: create diagnostics_channel module
 - http: report request start and end with diagnostics_channel
Committers 1
 - Stephen Belanger 
PR-URL: https://github.com/nodejs/node/pull/34895
Reviewed-By: Bryan English 
Reviewed-By: Gerhard Stöbich 
Reviewed-By: Vladimir de Turckheim 
Reviewed-By: Rich Trott 
Reviewed-By: Gabriel Schulhof 
Reviewed-By: Michael Dawson 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/34895
Reviewed-By: Bryan English 
Reviewed-By: Gerhard Stöbich 
Reviewed-By: Vladimir de Turckheim 
Reviewed-By: Rich Trott 
Reviewed-By: Gabriel Schulhof 
Reviewed-By: Michael Dawson 
--------------------------------------------------------------------------------
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2020-10-30T23:17:05Z: https://ci.nodejs.org/job/node-test-pull-request/33982/
- Querying data for job/node-test-pull-request/33982/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Sun, 23 Aug 2020 20:13:02 GMT
   ✔  Approvals: 6
   ✔  - Bryan English (@bengl): https://github.com/nodejs/node/pull/34895#pullrequestreview-505212451
   ✔  - Gerhard Stöbich (@Flarna): https://github.com/nodejs/node/pull/34895#pullrequestreview-521096246
   ✔  - Vladimir de Turckheim (@vdeturckheim): https://github.com/nodejs/node/pull/34895#pullrequestreview-505186641
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/34895#pullrequestreview-519770538
   ✔  - Gabriel Schulhof (@gabrielschulhof) (TSC): https://github.com/nodejs/node/pull/34895#pullrequestreview-519770808
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/34895#pullrequestreview-519910783
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

Commit Queue action: https://github.com/nodejs/node/actions/runs/338585202

nodejs-github-bot · 2020-10-31T01:13:22Z

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

nodejs-github-bot · 2020-10-31T14:52:30Z

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

nodejs-github-bot · 2020-10-31T17:27:07Z

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

github-actions · 2020-10-31T19:51:15Z

Commit Queue failed

- Loading data for nodejs/node/pull/34895
✔  Done loading data for nodejs/node/pull/34895
----------------------------------- PR info ------------------------------------
Title      lib: create diagnostics_channel module (#34895)
Author     Stephen Belanger  (@Qard)
Branch     Qard:diagnostics-channel -> nodejs:master
Labels     author ready, build, diag-agenda, experimental, lib / src, notable-change, semver-minor
Commits    2
 - lib: create diagnostics_channel module
 - http: report request start and end with diagnostics_channel
Committers 1
 - Stephen Belanger 
PR-URL: https://github.com/nodejs/node/pull/34895
Reviewed-By: Bryan English 
Reviewed-By: Gerhard Stöbich 
Reviewed-By: Vladimir de Turckheim 
Reviewed-By: Rich Trott 
Reviewed-By: Gabriel Schulhof 
Reviewed-By: Michael Dawson 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/34895
Reviewed-By: Bryan English 
Reviewed-By: Gerhard Stöbich 
Reviewed-By: Vladimir de Turckheim 
Reviewed-By: Rich Trott 
Reviewed-By: Gabriel Schulhof 
Reviewed-By: Michael Dawson 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - lib: create diagnostics_channel module
   ⚠  - http: report request start and end with diagnostics_channel
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-10-31T17:27:07Z: https://ci.nodejs.org/job/node-test-pull-request/33999/
- Querying data for job/node-test-pull-request/33999/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Sun, 23 Aug 2020 20:13:02 GMT
   ✔  Approvals: 6
   ✔  - Bryan English (@bengl): https://github.com/nodejs/node/pull/34895#pullrequestreview-505212451
   ✔  - Gerhard Stöbich (@Flarna): https://github.com/nodejs/node/pull/34895#pullrequestreview-521096246
   ✔  - Vladimir de Turckheim (@vdeturckheim): https://github.com/nodejs/node/pull/34895#pullrequestreview-505186641
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/34895#pullrequestreview-519770538
   ✔  - Gabriel Schulhof (@gabrielschulhof) (TSC): https://github.com/nodejs/node/pull/34895#pullrequestreview-519770808
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/34895#pullrequestreview-519910783
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

Commit Queue action: https://github.com/nodejs/node/actions/runs/339451618

github-actions · 2020-10-31T21:24:16Z

Landed in 5dd344a...7231b51

Qard added lib / src Issues and PRs related to general changes in the lib or src directory. diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. labels Aug 23, 2020

Qard requested review from mcollina and mmarchini August 23, 2020 20:13

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Aug 23, 2020

Qard mentioned this pull request Aug 23, 2020

Diagnostics Channel - Next steps nodejs/diagnostics#180

Closed

8 tasks

Qard requested a review from Flarna August 23, 2020 20:16

Qard force-pushed the diagnostics-channel branch 4 times, most recently from 6e2b771 to c9c3787 Compare August 23, 2020 21:28

bmeck reviewed Aug 23, 2020

View reviewed changes