Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using prometheus collector MustRegister in New{Counter,Gauge} and possible panic #937

Open
vadv opened this issue Nov 14, 2019 · 7 comments

Comments

@vadv
Copy link

vadv commented Nov 14, 2019

Thanks for the wonderful product!
I think it would be fair to point the limitations of calling New in the documentation, since calling New again calls panic.

@peterbourgon
Copy link
Member

A great point! :)

@tsatke
Copy link

tsatke commented Nov 22, 2019

Is this a bug or a documentation issue? If it is a bug, I would like to have a look at it.

@peterbourgon
Copy link
Member

Hmm, it could be either. The easiest fix would be to update the docs to mention it can panic, but a better fix would probably be a new constructor with took an e.g. prometheus.Registerer into which the metrics would be registered, and refactoring the existing constructor (with updated doc comment) to call the new constructor with prometheus.DefaultRegisterer (or whatever it's called).

@AshleyDumaine
Copy link

I also ran into this issue (the day this was filed interestingly) and implemented a Delete function for each of the metric types that calls Unregister: AshleyDumaine@5af0353
Would this be an acceptable solution?

@peterbourgon
Copy link
Member

I don't think Delete is the answer — metrics generally should be singletons in your component graph. If we need something more it would probably be to take an explicit Registerer.

prasadghagare added a commit to prasadghagare/kit that referenced this issue Jun 16, 2020
@prasadghagare
Copy link

Hi @TimSatke , are you still looking at this?
If not I would like to have a go at it.

@tsatke
Copy link

tsatke commented Jun 16, 2020

I didn’t look at it, since I didn’t consider any answer a clear „go“, so yes, take it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants