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
Open

Comments

@vadv
Copy link

@vadv 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

@peterbourgon peterbourgon commented Nov 14, 2019

A great point! :)

@TimSatke
Copy link

@TimSatke TimSatke 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

@peterbourgon peterbourgon commented Nov 25, 2019

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

@AshleyDumaine AshleyDumaine commented Feb 6, 2020

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

@peterbourgon peterbourgon commented Feb 6, 2020

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
…nter,Gauge} and possible panic
@prasadghagare
Copy link

@prasadghagare prasadghagare commented Jun 16, 2020

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

@TimSatke
Copy link

@TimSatke TimSatke 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
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.