EventLoop leak

[justinmchase]

This appears to be a leak:

https://github.com/siimon/prom-client/blob/master/lib/metrics/eventLoopLag.js#L47-L50

You're not exposing a way to call histogram.disable() and so there is no way to turn it off and it will permanently hang open the process.

Ideally collectDefaultMetrics would return something that I could call stop on at a later time.

const register = new Registry()
const collect = collectDefaultMetrics({ register: this.register })

export function stop() {
  collect.stop()
}

I got this info from why-is-node-running module:

# ELDHISTOGRAM
node:internal/async_hooks:201                                                   
node:internal/perf/event_loop_delay:81                                          
node:internal/perf/event_loop_delay:78                                          
/var/app/example/node_modules/prom-client/lib/metrics/eventLoopLag.js:47 - const histogram = perf_hooks.monitorEventLoopDelay({
/var/app/example/node_modules/prom-client/lib/defaultMetrics.js:43       - metric(config.register, config);

[SimenB]

collectDefaultMetrics should deffo return a function that tears down whatever it set up. Wanna send a PR?

[justinmchase]

@SimenB I'll give it a go.

[zbjornson]

Asked about the potential for an upstream unref() method: https://github.com/nodejs/node/issues/42132

[justinmchase]

That would be much better, I can't think of a time you'd want this to keep it open.

[SimenB]

yeah, that makes sense!

[zbjornson]

@justinmchase can you provide a full repro for this please? Neither of these are keeping Node.js (16.13.1) alive for me (see also https://github.com/nodejs/node/issues/42132#issuecomment-1053935605):

const pc = require("."); // in the prom-client directory
const register = new pc.Registry()
const collect = pc.collectDefaultMetrics({ register })

const perf_hooks = require('perf_hooks');
const histogram = perf_hooks.monitorEventLoopDelay({
	resolution: 10,
});
histogram.enable();

[justinmchase]

Unfortunately I have a fairly complex setup of course and when I run why-is-node-running I have a number of things showing up in the list and this was just one of them, so I reported it here.

So perhaps that library is mis-reporting unreffed things? But one of the things its reporting is actually a real reference... Great.

[zbjornson]

It looks like a false-positive. why-is-node-running reports everything that is reported through async_hooks, except for a few things that it specifically excludes: https://github.com/mafintosh/why-is-node-running/blob/24fb4c878753390a05d00959e6173d0d3c31fddd/index.js#L8-L11. Performance hooks are new enough that they just might not have been excluded yet. Want to open an issue with why-is-node-running and see if they agree?

[justinmchase]

Will do, thank you for the diligence!