siimon / prom-client

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

A metric with the name xxx has already been registered #334

Open shaharsol opened 4 years ago

shaharsol commented 4 years ago

I believe the intention here https://github.com/siimon/prom-client/blob/c63689bbbb6a0395ce9d806b0cc17cf5d380f4a8/lib/registry.js#L83 was to use something like !_.isEqual so consecutive creation of the same metric won't fail (behave like a getOrCreate).

trboltom commented 4 years ago

@shaharsol I have tried to create PR for this, but prom-client does not have underscore library and I dont believe that we want to add it. We could use stringify() and compare strings, but I dont really like that solution and not sure it will pass the CR.

zbjornson commented 4 years ago

I'm not sure that I understand the problem/scenario. Metric names must be unique for Prometheus to work, and registering two different metric functions with the same name is an error. Can you explain please?

trboltom commented 4 years ago

@zbjornson You are correct that registering two different metrics with the same name has to end with an error. We wanted to do this change in order to suppress this error in case you try to register the same metric twice.

This can happen for example when you are adding new collectors in the constructor of some service and then inject this service without making it a singleton. This will result in constructor of this service being called multiple times during one request, which will end up in try to register one metric multiple times.

This is an error/mistake on the developers side and so it's probably not good enough reason to implement this. I thought that it could be a good idea to ignore second attempt and just dont do any action. However as I am writing this comment it has become clear to me that we probably really dont want to implement it in this way. Its probably better to keep this check there as is and just let developer know that something is not alright.

If you agree I will close that PR, but this issue is not mine and so someone else will have to close it if all agree

zbjornson commented 4 years ago

Thanks for the explanation @trboltom! @shaharsol is that your situation as well?

shaharsol commented 4 years ago

In mongodb when I run db.users.insert, and there's no users collection, then it creates it, otherwise it uses the existing collection. I don't need to check everytime if the collection exists and creates it if it doesn't. Was simply expecting the same behaviour from prom-client. It is obvious that if I try to create the same metric a 2nd time with the same params, then I want to get that same existing metric back.

juriwiens commented 4 years ago

What about adding an optional config like throwIfMetricExists, that is true by default? It would allow developers to explicitly disable the check if they know what they are doing.

zbjornson commented 4 years ago

Thanks for the clear explanation @trboltom! Based on that and the other comments in this thread, it sounds like we should keep the current behavior. I think that encourages clean code logic also (e.g. ensure your services are singletons).

Note that this is easy to handle upstream:

if (!registry.getSingleMetric("metric_name"))
  new Gauge(...)

In mongodb when I run db.users.insert, and there's no users collection, then it creates it, otherwise it uses the existing collection.

(Side note: this design decision frustrates me. I've made typos before in collection names and this behavior makes the error hard to find.)

ScutRukawa commented 3 years ago

@trboltom hi , could you konw why prom-client don't surport one metrics with different tags . For example . cpu{tag1=1,tag2=2,tag3=3} , cpu{tag1=1,tag2=2} ,my app will generate metrics like this.

trboltom commented 3 years ago

@ScutRukawa hi, unfortunately I don't remember this issue anymore, but please take a look at this https://prometheus.io/docs/instrumenting/writing_clientlibs/ I believe it's this part

Client libraries MUST NOT under any circumstances allow users to have different label names for the same metric for Gauge/Counter/Summary/Histogram or any other Collector offered by the library.

Most prometheus clients do not support missing or different labels. And even when they do, you are not guaranteed that everything will be working correctly. I would try to prevent this at any cost.

I don't know anything about your application, bur maybe if you can explain a bit your situation, we might be able to help you. However I am not sure if this issue is best place to ask such questions :)

glensc commented 10 months ago

IMHO the problem is that new Gauge forces to use some "global registry":

This can be overridden by providing your own registry instance:

const registry = new Registry();
const gauge = new Gauge({
// ...
        registers: [registry],
...
zhirschtritt commented 10 months ago

@glensc Agreed ^^, https://github.com/siimon/prom-client/issues/265 offers a similar solution which seems reasonable.