Cluster event listeners are attached even if not using clusters

[SimenB]

Because of https://github.com/siimon/prom-client/blob/426941fbc22433fff932d8f9fb09ebba4818f514/index.js#L24, https://github.com/siimon/prom-client/blob/426941fbc22433fff932d8f9fb09ebba4818f514/lib/cluster.js#L131-L170 is always ran

This results in a memory leak if require('prom-client'); is done multiples times and the require.cache is blown away, as might happen in tests.

I get the following after rerunning my tests, which blow away the require cache in order to mock a dep. (node:80194) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 message listeners added. Use emitter.setMaxListeners() to increase limit

[SimenB]

/cc @zbjornson.

Quick fix is to do const process = require('process'); at the top, but I don't like that.

Preferably you would do something like

const promClient = require('prom-client');

promClient.activateClusterSupport();

Some sort of opt-in is needed, IMO.

[zbjornson]

Since it's very unusual to clear the require cache, especially globally and not for one specific module that you own, I didn't think this was a problem when I added it. :-/

This would now be a breaking change, but we could require a specific function call like setupClusterMetrics() to init the event listener.

Your process comment would work; jest doesn't reload that one specifically.

If you just want to avoid that message about event listener leaks while running tests, there's some jest configuration that can be changed (or switch to something other than jest that doesn't break node's singleton module principle).

Will keep thinking but I didn't come up with much previously. This is supposed to be the right way to do singletons.

[SimenB]

I was able to solve my particular problem by rearranging import order, which allows me to stop calling jest.resetModules(); which is good. 🙂 I still think we should do something here, though.

This is supposed to be the right way to do singletons.

That's fine if I actually imported the cluster code. I don't though, for me the cluster listener is pure overhead. I don't run node in clusters, I don't need prom-client to listen or emit events for it.

I think just requiring a module shouldn't set up listeners on global objects unless that's the module's sole purpose. If I did require('prom-client-cluster') or require('prom-client/cluster') I think it's perfectly fine to attach the listeners. But not for a general purpose thing such as prom-client.

Not sure what the best solution is, though.

[zbjornson]

I think just requiring a module shouldn't set up listeners on global objects unless that's the module's sole purpose.

Makes total sense. It's pretty negligible overhead, but on premise I agree with you. Unfortunately changing that will be semver major since the current behavior has already been released 😞.

Some API ideas:

1. require('prom-client/cluster') as you proposed
2. new AggregatorRegistry() implicitly adds the listener
3. var ar = new AggregatorRegistry(); ar.init() explicitly inits the listeners (method name TBD)

Note that in cases 2 and 3, we should take steps to only add the listeners once. Jest will still break this, but we can make it safe for a user to instantiate multiple AggregatorRegistrys or call ar.init() multiple times.

[zbjornson]

Hm, actually... we could do option 2 without it being a breaking change... I'll work on a PR for that this week.

[SimenB]

Perfect, thanks!

[SimenB]

@zbjornson have you had a chance to look into it?

[jdmarshall]

This implementation assumes that only the first AggregatorRegistry call should listen, which means most of the tests that are currently green are likely testing the wrong instance of AggregatorRegistry.

This is also a curious limitation for run-time, considering AggregatorRegistry's contentType is determined in the constructor. This means you cannot produce multiple metrics routes for different metrics formats. Which will likely be a problem for expanding OpenMetrics support.