Bump node-clinic to tier 3

[BridgeAR]

Right now node-clinic is not yet in any diagnostic support tier. However, recently we ran into issues with 10.10 that could have been detected early on if the clinic test suite would be run on the Node.js infrastructure.

The tool fulfills becomes more and more popular at the moment and bumping it to 3 would likely be the right thing at the moment. That way we are able to guarantee a better stability level overall.

I opened the issue here to discuss it first in the working group before opening a PR to change the tier right away. If there are any questions or if there is any further information necessary, please ask.

@mcollina please also weight in.

[mcollina]

Recently we had a case where the generation of flamegraphs was broken in both Node.js 10.10.0 (all the times) and 8.12.0 (in certain cases). I think we are not testing enough those parts of our runtime.

[mike-kaufman]

Discussed on Diagnostics WG call on 2018-09-19. here's a summary:

• There's some concern here that clinic is too broad and touches too many things, whereas smaller, more targeted tools would be better scoped here. Is there some existing flame graph tool that could be added to the matrix that would be more targeted and prevent clinic's flame graphs from breaking?
• is it appropriate to test this via citgm?
• can we add tests at an API level to ensure that the flame graphs APIs are working correctly? i.e., the tests aren't validating clinic itself, but rather are testing APIs clinic depends on. API tests would have a better chance of moving up to higher tiers (e.g., blocking releases)

Can we get some details on the exact APIs being used by clinic?

[mmarchini]

@mcollina could you give a little more context on what broke and why? This would help us understand how to keep it from breaking in the future.

[richardlau]

is it appropriate to test this via citgm?

FYI there's a PR to add it to the default citgm lookup: nodejs/citgm#603

[mcollina]

The following commit not being backported broke Node 8.12.0 (I did a brittle workaround on 0x side): nodejs/node#19285 nodejs/node#19260 nodejs/node#22825.

At more or less the same time, flamegraphs broke on Node 10.10 because of nodejs/node#22786 and it was fixed in nodejs/node#22790.

It seems that whole area of Node core is not tested nor validated before a release.

[joyeecheung]

Do the tests of clinic pass on all the Node.js release lines that it supports right now?
The majority of --prof-process comes from V8 and the breakage may come from V8 as well. https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/buildTimeTrend CITGM has been very red for a long time and I am concerned that adding something that deeply depends on V8 (hence the fix may have to come from V8) would make the situation worse.

[mhdawson]

Would a test for the tick profile processor have caught the problem? Just wondering if we could add a smaller test to cover this case?

[ofrobots]

This case needs a test that runs as part of the CI so that V8's integration builds pick it up too. That way we can avoid V8 breaking this functionality in the first place. I would imagine that Clinic could be part of CitGM, but not the CI.

[richardlau]

Would a test for the tick profile processor have caught the problem? Just wondering if we could add a smaller test to cover this case?

Our existing tick processor tests don't catch nodejs/node#22825, so we at least have a coverage gap: nodejs/node#22825 (comment)

[mcollina]

Filling that coverage gap would solve the immediate problem, yes.

So, we can talk about bumping --prof-process up in the Tier hierarchy instead (T3).

[joyeecheung]

Bumping --prof-process to T3 first sounds more reasonable to me.

[mhdawson]

+1 to bump up --prof-process

[cjihrig]

+1 to bumping up --prof-process

[Fishrock123]

Please bump --prof-process. It's shipped with node and should obviously not ship broken.

[mmarchini]

I can include --prof-process in nodejs/node#22915