siimon / prom-client

Prometheus client for node.js
Apache License 2.0
3.15k stars 376 forks source link

Hot Module Reload problem with prom-client #196

Open cabrinoob opened 6 years ago

cabrinoob commented 6 years ago

Hi, I'am using your library and I love it, but I found this little problem (not really a big one, but ...).

I'am using your library in a NestJS project. This framework proposes to use HMR with webpack. I have a provider class for prometheus-things which have this contructor :

constructor(){
        this.counter = new Counter({
            name: 'fred_total_method_calls',
            help: 'Example of a counter',
            labelNames: ['method','path']
        });

        this.gauge = new Gauge({
            name: 'fred_method_response_time',
            help: 'Example of a gauge',
            labelNames: ['method','path']
        });
    }

When I'am running my project in my develop environment, and when I modify something somewhere in the project I have this error :

[HMR] Updated modules:
[HMR]  - ./src/app.controller.ts
[HMR]  - ./src/app.module.ts
[HMR]  - ./src/main.hmr.ts
[HMR]  - ./src/app.service.ts
[HMR] Update applied.
[Nest] 8624   - 2018-5-31 15:22:03   [ExceptionHandler] A metric with the name fred_total_method_calls has already been registered.

When the Hot Module Reload event is handled, webpack regenerate the changing code but existing metrics are still there.

I have found a workarround :

constructor(){
       register.clear();
        this.counter = new Counter({
            name: 'fred_total_method_calls',
            help: 'Example of a counter',
            labelNames: ['method','path']
        });
...

I call register.clear() in my constructor and it works in HMR mode. But I don't like this solution because this line is only here for my comfort allowing me to use HMR in my development phase. It has no direct buisness intrest.

So, do you have a better workarround for this problem?

Thank you

jeremija commented 6 years ago

I have a similar problem when using express-prom-bundle and running my supertest tests via mocha --watch for the second time:

Error: A metric with the name up has already been registered.
    at Registry.registerMetric (node_modules/prom-client/lib/registry.js:71:10)
    at Gauge.config.registers.forEach.registryInstance (node_modules/prom-client/lib/gauge.js:72:21)
    at Array.forEach (<anonymous>)
    at new Gauge (node_modules/prom-client/lib/gauge.js:71:20)
    at Object.up (node_modules/express-prom-bundle/src/index.js:72:17)
zbjornson commented 6 years ago

I haven't used Webpack's HMR, but it looks like you might want to put that register.clear() bit in module.hot.accept, as shown here: https://webpack.js.org/guides/hot-module-replacement/#gotchas.

On principle I don't think there's a nice way for this to "just work." The registry (a singleton, if using the global reg) and metrics are both meant to be created once; HMR is creating the registry once but the metrics more than once.

SimenB commented 6 years ago

One thing we could have is like a Counter.createOrReuse which takes all the same options as the constructor and returns the old instance if it exists in the registry. Then throw if one with the sane name but different help text or labels is there already.

Or just make that the default in the constructor

waisbrot commented 5 years ago

I have a similar problem: a test framework that bypasses Node's module-caching in some cases, so I have to wrap prom-client with my own module that creates meters idempotently.

Is this something that would be considered if I wrote it as a PR? I was thinking just what @SimenB describes (I'd have called it Counter.create but don't have a strong opinion there).

dfairaizl commented 5 years ago

We just ran into this too, is there a preferred way to ensure counters and other metrics are only created once?

slim-hmidi commented 5 years ago

I don't know if it is the same problem as you but I got this error when I run the tests:

Boot failed Error: A metric with the name nodejs_gc_runs_total has already been registered.
    at Registry.registerMetric (/home/user/project/node_modules/prom-client/lib/registry.js:79:10)
    at Counter.config.registers.forEach.registryInstance (/home/user/project/node_modules/prom-client/lib/counter.js:80:21)
    at Array.forEach (<anonymous>)
    at new Counter (/home/user/project/node_modules/prom-client/lib/counter.js:79:20)
    at module.exports (/home/user/project/node_modules/prometheus-gc-stats/index.js:33:19)
    at module.exports.app (/home/user/project/node_modules/kaliida/index.js:39:24)
    at module.exports.app (/home/user/project/node_modules/rf-init/src/boot/routeKaliida.js:6:3)
    at iterator (/home/user/project/node_modules/rf-init/src/index.js:125:37)
    at pReduce (/home/user/project/node_modules/p-each-series/index.js:4:73)
    at Promise.all.then.value (/home/user/project/node_modules/p-reduce/index.js:16:10)

and the script to run tests in package.json:

{
  scripts: {
      "test": "mocha --recursive --exit"
  }
}

I modify the test script by:

{
  scripts: {
      "test": "NODE_ENV=test mocha --recursive --exit"
  }
}

Then the error was fixed. I hope that it will help you if you faced the same problem with test.

cadorn commented 5 years ago

I call client.register.removeSingleMetric('<name>') before I register metrics which seems to solve the issue.

adambisek commented 4 years ago

This works (you have to call clear on every HMR reload on SSR):

import promBundle from 'express-prom-bundle'
import prom from 'prom-client'
import { RequestHandler } from 'express'

const applyPrometheusMiddleware = (): RequestHandler => {
  prom.register.clear() // to avoid breaking SSR HMR
  return promBundle({ includeMethod: true, includePath: true })
}

export default applyPrometheusMiddleware

in express initialization:

const server = express()
server
  .disable('x-powered-by')
  .use(applyPrometheusMiddleware()) // don't put this middleware into global variable, or you'll break SSR HMR!
theliuk commented 4 years ago

I'm having the same issue with tap test framework.

client.register.removeSingleMetric('')

client.register.clear()

does the job.

harry-gocity commented 1 year ago

FWIW, we found that register.clear() looked like it fixed things locally. But in our production build (Next.js) clear() was clearing all metrics collected by collectDefaultMetrics too. I assume not visible locally due to HMR calling collectDefaultMetrics often enough. Switching to removeSingleMetric() was the solution for us.

MagnusSafty commented 2 months ago

The confustion for me here turned out to be that if you don't specify a register when creating a new metric, it will register to the default register.

const register = new Registry();

const exampleGauge = new Gauge({
  name: 'test_gauge',
  help: 'Example of a gauge',
  labelNames: ['test_label'],
}); 

register.registerMetric(exampleGauge);

The above would fail on the second request, the following fixed my issue

const register = new Registry();

const exampleGauge = new Gauge({
  name: 'test_gauge',
  help: 'Example of a gauge',
  labelNames: ['test_label'],
  registers: [register],
});