doc: initial cut at support tiers for diag tools #21870
Conversation
| tools. Each of the tools and APIs has been put them into one of | ||
| the following tiers. | ||
|
|
||
| * Tier 1 - Mmust always be working for all |
|
We had a discussion at the last add-on API meeting about whether the tiers of support should have something to do with whether N-API supports the APIs that that particular diagnostic tool needs. For example, IMO, if CPU profiling is of tier-1 important to Node.js and the ecosystem, does it make sense that such diagnostics tools must use V8 APIs. Instead, should the criteria for tiers include whether N-API APIs are capable of supporting tools in that domain? |
| the runtime itself. | ||
|
|
||
| The Node.js project has assessed the tools and the APIs which support those | ||
| tools. Each of the tools and APIs has been put them into one of |
There was a problem hiding this comment.
Is them unnecessary here?
|
|
||
| * If the tool fits into a key cateogry as listed below. | ||
| * Whether the tool is actively used by the community. | ||
| * The Availability of alternatives. |
There was a problem hiding this comment.
Availability -> availability?
| collaborative decision between Diagnostics WG and Release WG. Some of the | ||
| criteria considered might be: | ||
|
|
||
| * If the tool fits into a key cateogry as listed below. |
| * The availability of reliable test suite that can be integrated into our CI. | ||
| * The availability of maintainer or community collaborator who will help | ||
| resolve issues when there are CI failures. | ||
| * If the tool is maintained the Node.js foundation GitHub organization. |
There was a problem hiding this comment.
maintained -> maintained by?
|
|
||
| The current categories of tools/APIs that fall under these Tiers are: | ||
|
|
||
| * FDDC (F) - First failure data capture, Easy to consume initial diagnostic |
There was a problem hiding this comment.
FDDC -> FFDC?
Easy -> easy?
| | Memory | node-heapdump | No | No | 2 | | ||
| | Memory | V8 heap profiler | No | Yes | 1 | | ||
| | Memory | V8 sampling heap profiler | No | Yes | 1 | | ||
| | AsyncFlow | Async Hooks(API) | ? | Yes | 1 | |
There was a problem hiding this comment.
Space before (API) here and below?
| | Debugger | V8 Debug protocol(API) | No | Yes | 1 | | ||
| | Debugger | Command line Debug Client | ? | Yes | 1 | | ||
| | Debugger | Chrome Dev tools | ? | No | 3 | | ||
| | Debugger | Chakracore - time-travel | No | data source only | to early | |
There was a problem hiding this comment.
data -> Data (for consistency)?
There was a problem hiding this comment.
Not sure, but was to early intended to be Too early?
| | Debugger | Chakracore - time-travel | No | data source only | to early | | ||
| | Tracing | trace_events(API) | No | Yes | 1 | | ||
| | Tracing | DTrace | No | Partial | 3 | | ||
| | Tracing | LTTng | No | Removed ? | N/A | |
There was a problem hiding this comment.
Unneeded space in Removed ??
| | Tracing | DTrace | No | Partial | 3 | | ||
| | Tracing | LTTng | No | Removed ? | N/A | | ||
| | Tracing | ETW | No | Partial | 3 | | ||
| | Profiling | V8 cpu profiler | No | Yes | 1 | |
There was a problem hiding this comment.
Should we include --prof as well? Is that included in V8 CPU profiler?
There was a problem hiding this comment.
I think that is the V8 cpu profiler ?
| | Tracing | ETW | No | Partial | 3 | | ||
| | Profiling | V8 cpu profiler | No | Yes | 1 | | ||
| | Profiling | Linux perf | No | Partial | ? | | ||
| | Profiling | Windows xperf | No | ? | ? | |
| | Profiling | Linux perf | No | Partial | ? | | ||
| | Profiling | Windows xperf | No | ? | ? | | ||
| | Profiling | Ox | No | No | to early | | ||
| | Profiling | node-clinic | No | No | to early | |
| | Tracing | DTrace | No | Partial | 3 | | ||
| | Tracing | LTTng | No | Removed ? | N/A | | ||
| | Tracing | ETW | No | Partial | 3 | | ||
| | Profiling | V8 cpu profiler | No | Yes | 1 | |
There was a problem hiding this comment.
Should we include --prof as well? Is that included in V8 CPU profiler?
| in this tier it must have a good test suite and that test suite and a job | ||
| must exist in the Node.js CI so that it can be run as part of the release | ||
| process. No commit to master should break this tool/API if the next | ||
| release is within 1 month. |
There was a problem hiding this comment.
We need to defined what does it mean to "work". In some cases it's a true-false decision (llnode), but in others it is a matter of accuracy (Linux perf in 8.x for example). I assume you mean the latter, but I think it should be written down.
There was a problem hiding this comment.
Since we'll require a test suite on Node.js CI for Tier 1 tools, maybe we could define "work" as "successfully running a CI for the tool's test suite"?
There was a problem hiding this comment.
agreed, updated to try to clarify that.
|
| | Profiling | V8 cpu profiler | No | Yes | 1 | | ||
| | Profiling | Linux perf | No | Partial | ? | | ||
| | Profiling | Windows xperf | No | ? | ? | | ||
| | Profiling | Ox | No | No | to early | |
There was a problem hiding this comment.
I would classify 0x being "Tier 4" for now.
There was a problem hiding this comment.
Ok will make that the target, but I'd like to handle tool promotion from unclassified to specified tiers with separate PRs.
| | Profiling | Ox | No | No | to early | | ||
| | Profiling | node-clinic | No | No | to early | | ||
| | F/P/T | appmetrics | No | No | ? | | ||
| | M/T | eBPF tracing tool | No | No | ? | |
There was a problem hiding this comment.
Can we convert this list to links to the various tools?
There was a problem hiding this comment.
Yes, but I'd prefer to do that in a separate PR. I will commit to doing that once this PR lands.
| in this tier it must have a good test suite and that test suite and a job | ||
| must exist in the Node.js CI so that it can be run as part of the release | ||
| process. No commit to master should break this tool/API if the next | ||
| release is within 1 month. |
There was a problem hiding this comment.
I think you can remove the condition. Current releases usually happen at least once a month.
There was a problem hiding this comment.
Better: keep the condition but change it to "if the next major release is within 1 month"
|
It doesn't have to be in this PR but I would like to have a link pointing to the documentation or repo for each tool. |
Strongly agree.
I would like to rate it Tier 1 as well, but there's only a handful of people working on the project right now and it essentially breaks at every V8 upgrade. Besides, some optimizations on V8 are making it harder to fix those breakages, and llnode use a best-effort approach to diagnose the application, which makes its result not as reliable as we'd expect from a Tier 1 tool. We started some discussion on this topic at nodejs/diagnostics#202.
We could classify Linux perf and other system profilers (DTrace, eBPF, ETW) which provide similar features as Tier 1, but what should we do if V8 revamps its compiler again in the future without adding support for those tools (like what happenend with Turbofan)? I think this is an important question for all tools that might break with V8 updates.
I think we should, even though |
| | Debugger | Chrome Dev tools | ? | No | 3 | | ||
| | Debugger | Chakracore - time-travel | No | data source only | to early | | ||
| | Tracing | trace_events(API) | No | Yes | 1 | | ||
| | Tracing | DTrace | No | Partial | 3 | |
There was a problem hiding this comment.
- DTrace is also used as a profiling tool and is widely used to generate FlameGraphs
- Systemtap should also be included since its situation is similar to DTrace (for tracing): we have a file specifying Systemtap tracepoints (https://github.com/nodejs/node/blob/master/src/node.stp). I would add it as
Tracing | Systemtap | No | Partial | ?
|
Thanks for all the feedback so far. I'll take a cut at incorporating it on my travels to SF for Node Summit this weekend. |
|
A new one, but seems worth to add? |
|
I am -1 on rating llnode tier 1, at least for the moment. My reasoning was described in nodejs/diagnostics#202 (comment) . llnode is just a plugin of lldb (correction: it can also be an addon linked to lldb now that the JS API landed). Unlike V8 or chromium, we don't have effective contact with the lldb project and it does not even support core file debugging that well, which is what llnode primarily used for (the situation may have been improved since that post but I don't feel a significant difference from day-to-day use of lldb). We should try to improve the situation, yes, but when talking about support and expectations, we should not make this a hard dependency of Node.js's release process. |
| * Tier 1 - Mmust always be working for all | ||
| Current and LTS Node.js releases. A release will not be shipped if the test | ||
| suite for the tool/API is not green. To be considered for inclusion | ||
| in this tier it must have a good test suite and that test suite and a job |
There was a problem hiding this comment.
I suggest we include entries similar to those required by citgm:
The maintainers of the module remain responsive when there are problems
The module must be actively used by the community
The module must be heavily depended on
Given that tier-one tools block our commits and releases.
|
Given that this document is basically a list of things officially supported by Node.js, i.e. we’re picking winners here (which we have very much tried to avoid so far), I think there should be a clear policy for tool authors to have their tools entered into this list before we merge this? |
|
@joyeecheung it was listed as tier 1 only as a longer term target, not what it will be. But happy to have it as something lower as an long term target as well. |
|
Back in the office this week, so will try to do an update tomorrow. @addaleax everything is starting at level "Unclassified", but I do get your point that we need to be careful about making"winners". On the other hand there are some cases were we need some additional support to even have one tool that is good enough for the ecosystem. I'd be happy to drop out some of the newer ones, but I think the approach was to make the "Unclassified" the largest set to show, if nothing else, that the project was aware of them and that they were not going to get into the higher tiers. |
|
@mhdawson I’m not worried about the particular amount of packages on this list, but I’d really like to see a policy that can guide package maintainers into having their own tools accepted into this document. |
|
@addaleax, thanks for the clarification. I'll think about that and try to incorporate some more info on that front when I update tomorrow. |
|
Failed to get to this today and I'm away Friday/Monday will be a priority for next week. |
|
@joyeecheung - I understand your reasoning - But that does not necessarily affect its criticality to the postmortem debugging. Please note, we don't have an alternative! It may be true that we don't have too many dump debugging scenarios - Node's prominent problem symptom seems to be My dealings with crash scenarios (largely nodejs/help repo) primarily had users in the development phase, so it was easy to play around with instrumented code and do trial and error - much more easier than dump debugging. When large scale production systems crash, (there may not be an identical test / staging system, the issue may be intermittent or even one-off) I fear we won't have the leeway for experimenting like that, and will have to resort to extract everything from the single core file we have, and |
|
@gireeshpunathil I feel like this is getting a little off-topic, since in this PR llnode is |
| criteria considered might be: | ||
|
|
||
| * If the tool fits into a key cateogry as listed below. | ||
| * Whether the tool is actively used by the community. |
There was a problem hiding this comment.
How do you define "the community"? Is this...
- Open source developers?
- The Node.js core collaborators?
- Developers deploying to FaaS infrastructure?
- Massive enterprise deployments?
We tend to use "the community" as a catch-all where I think we may be more well served by saying "the ecosystem" – but even then, we need to be explicit when we're codifying expectations around what that means.
There was a problem hiding this comment.
changed to the ecosystem. I don't think its any one of the above at the exclusion of the others. It is that its actively used by enough of the overall ecosystem which includes all of the categories you listed.
|
Believe I have updated for the comments so far. Please take another look: @mcollina @bnb @Trott @Alhadis @targos @mmarchini @joyeecheung @vsemozhetbyt @jasnell @BridgeAR |
| suite for the tool/API is not green. To be considered for inclusion | ||
| in this tier it must have a good test suite and that test suite and a job | ||
| must exist in the Node.js CI so that it can be run as part of the release | ||
| process. No commit to master should break this tool/API if the next |
There was a problem hiding this comment.
Maybe we should be more specific about what No commit to master should break this tool/API means? We probably will not run the tests of these tools for every PR to master, so this will be hard to enforce as-is. We may either:
- Run the tests of these tools with master every day/week and revert any recent commit that breaks the tests
- Or, acknowledge that we may have commits on master that break the tools without being noticed (because we may not run the tooling tests for those PRs), and limit this to
no commit to the current and LTS release branches should break..., which means we will run the tooling tests in the release process and exclude commits that break the tests (optionally, run the tests when someone suspects a PR may break the tools, i.g. similar to how CITGM works). This would be more practical.
There was a problem hiding this comment.
I'm ok with 2, given you running for every commit is problematic. In addition to the release process we should still aim to run them nightly if possible so that we get earlier warning if possible.
|
@nodejs/build-infra PTAL. There is something weird going on with the CI |
Looks like the directory needs a cleaning, maybe? HEAD detached at 0c60ab9a55
Untracked files:
(use "git add <file>..." to include in what will be committed)
tools/doc/node_modules/.bin/
tools/doc/node_modules/bail/
tools/doc/node_modules/boolbase/
tools/doc/node_modules/camelcase/
tools/doc/node_modules/ccount/
tools/doc/node_modules/character-entities-html4/
tools/doc/node_modules/character-entities-legacy/
tools/doc/node_modules/character-entities/
tools/doc/node_modules/character-reference-invalid/
tools/doc/node_modules/collapse-white-space/
tools/doc/node_modules/comma-separated-tokens/
tools/doc/node_modules/css-selector-parser/
tools/doc/node_modules/debug/
tools/doc/node_modules/define-properties/
tools/doc/node_modules/detab/
tools/doc/node_modules/extend/
tools/doc/node_modules/foreach/
tools/doc/node_modules/function-bind/
tools/doc/node_modules/has/
tools/doc/node_modules/hast-to-hyperscript/
tools/doc/node_modules/hast-util-from-parse5/
tools/doc/node_modules/hast-util-is-element/
tools/doc/node_modules/hast-util-parse-selector/
tools/doc/node_modules/hast-util-raw/
tools/doc/node_modules/hast-util-sanitize/
tools/doc/node_modules/hast-util-to-html/
tools/doc/node_modules/hast-util-to-parse5/
tools/doc/node_modules/hast-util-whitespace/
tools/doc/node_modules/hastscript/
tools/doc/node_modules/html-void-elements/
tools/doc/node_modules/inherits/
tools/doc/node_modules/is-alphabetical/
tools/doc/node_modules/is-alphanumerical/
tools/doc/node_modules/is-buffer/
tools/doc/node_modules/is-decimal/
tools/doc/node_modules/is-hexadecimal/
tools/doc/node_modules/is-nan/
tools/doc/node_modules/is-plain-obj/
tools/doc/node_modules/is-whitespace-character/
tools/doc/node_modules/is-word-character/
tools/doc/node_modules/kebab-case/
tools/doc/node_modules/lodash.iteratee/
tools/doc/node_modules/longest-streak/
tools/doc/node_modules/mapz/
tools/doc/node_modules/markdown-escapes/
tools/doc/node_modules/markdown-table/
tools/doc/node_modules/mdast-util-definitions/
tools/doc/node_modules/mdast-util-to-hast/
tools/doc/node_modules/mdurl/
tools/doc/node_modules/ms/
tools/doc/node_modules/nth-check/
tools/doc/node_modules/object-assign/
tools/doc/node_modules/object-keys/
tools/doc/node_modules/once/
tools/doc/node_modules/parse-entities/
tools/doc/node_modules/parse5/
tools/doc/node_modules/property-information/
tools/doc/node_modules/rehype-raw/
tools/doc/node_modules/rehype-stringify/
tools/doc/node_modules/remark-html/
tools/doc/node_modules/remark-parse/
tools/doc/node_modules/remark-rehype/
tools/doc/node_modules/remark-stringify/
tools/doc/node_modules/remark/
tools/doc/node_modules/repeat-string/
tools/doc/node_modules/replace-ext/
tools/doc/node_modules/space-separated-tokens/
tools/doc/node_modules/state-toggle/
tools/doc/node_modules/stringify-entities/
tools/doc/node_modules/to-vfile/
tools/doc/node_modules/trim-lines/
tools/doc/node_modules/trim-trailing-lines/
tools/doc/node_modules/trim/
tools/doc/node_modules/trough/
tools/doc/node_modules/unherit/
tools/doc/node_modules/unified/
tools/doc/node_modules/unist-builder/
tools/doc/node_modules/unist-util-find/
tools/doc/node_modules/unist-util-generated/
tools/doc/node_modules/unist-util-is/
tools/doc/node_modules/unist-util-position/
tools/doc/node_modules/unist-util-remove-position/
tools/doc/node_modules/unist-util-select/
tools/doc/node_modules/unist-util-stringify-position/
tools/doc/node_modules/unist-util-visit-parents/
tools/doc/node_modules/unist-util-visit/
tools/doc/node_modules/vfile-location/
tools/doc/node_modules/vfile-message/
tools/doc/node_modules/vfile/
tools/doc/node_modules/web-namespaces/
tools/doc/node_modules/wrappy/
tools/doc/node_modules/x-is-array/
tools/doc/node_modules/x-is-string/
tools/doc/node_modules/xtend/
tools/doc/node_modules/zwitch/
nothing added to commit but untracked files present (use "git add" to track)
+ git rev-parse HEAD
0c60ab9a55d2b51698ba65ff6c342f434fa33980
+ git rev-parse origin/master
5442c28b651a79c2269bf2b931e81cd553171656
+ '[' -n origin/master ']'
+ git rebase --committer-date-is-author-date origin/master
First, rewinding head to replay your work on top of it...
Applying: doc: initial cut at support tiers for diag tools
Applying: squash: partially address first set of comments
Applying: squash: address more comments
Applying: squash: address additional comments.
Applying: squash: address additional comments
Applying: squash: address rest of comments
Applying: squash: address comments
Applying: squash: fix linting issues from comments
Applying: squash: address comments
+ '[' -n 0c60ab9a55d2b51698ba65ff6c342f434fa33980 ']'
+ check_sha1=0c60ab9a55d2b51698ba65ff6c342f434fa33980
++ git rev-parse HEAD
+ head_sha1=43bdd12e130db75061672ae2ec6557e5503f430b
+ '[' 43bdd12e130db75061672ae2ec6557e5503f430b '!=' 0c60ab9a55d2b51698ba65ff6c342f434fa33980 ']'
+ exit 1 |
|
More recent run shows this: gmake: *** [Makefile:1147: lint-js-ci] Error 1
+ cat test-eslint.tap
+ grep -v '^ok\|^TAP version 13\|^1\.\.'
+ sed '/^/\s*$/d'
sed: 1: "/^/\s*$/d": invalid command code \ |
addaleax
removed
the
author ready
|
CI one more time: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/703/ |
|
Rebased, trying ci again: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/706/ |
|
CI green going to land. |
|
Landed in c241f4a |
PR-URL: #21870 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matheus Marchini <matheus@sthima.com>
PR-URL: #21870 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matheus Marchini <matheus@sthima.com>
PR-URL: #21870 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matheus Marchini <matheus@sthima.com>
PR-URL: #21870 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matheus Marchini <matheus@sthima.com>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesDiscussion on support tiers for diagnostic tools started at the Diagnostic summit earlier in the year. This specific text was discussed in the latest Diagnostics WG meeting and then tweaked based on the discussion. The goal would be to move tools to the target tier overtime as we manage to add the required testing.
@nodejs/diagnostics, @nodejs/release, @nodejs/tsc as this requires broad agreement on the approach/direction.