search

weaveworks / common

Libraries used in multiple Weave projects
Other
129 stars 92 forks source link

Restarting a server leads to metrics re-registration issues #285

Closed thampiotr closed 1 year ago

thampiotr commented 1 year ago

Problem:

When I run a common/server/server.go, then shut it down and start a new one (e.g. with a different port) in the same metrics namespace, I get a panic due to re-registration of metrics.

I tried working around this by keeping track of the metrics and explicitly Unregister-ing them, but this will still lead to errors such as:

An error has occurred while serving metrics:

1 error(s) occurred:
* collected metric "my_metric_name" { label:<name:"test_label" value:"test_value" > gauge:<value:0 > } was collected before with the same name and label values

This is because even after unregistering, it is not possible to register a new Collector that is inconsistent with the unregistered collector (as per prometheus.Registerer docs).

Proposed solution:

Instead of using reg.MustRegister(<collector>), the server should use reg.Register. When the AlreadyRegisteredError is returned by reg.Register, it will contain the previously registered Collector. The server should use that previously registered Collector instead.

Discussion

I’m curious to hear what are your thoughts on this approach? Are there any alternatives that you would recommend? Would you be open to a contribution?

bboreham commented 1 year ago

I think, under your proposal, if this library was used in a program which happened to also use the same metric name for something else, things could get confusing.

How about splitting server.New() into a function which registers the metrics and a function which creates the server? And leave the existing API creating a singleton which lasts the life of the program.

thampiotr commented 1 year ago

I think, under your proposal, if this library was used in a program which happened to also use the same metric name for something else, things could get confusing.

Thanks @bboreham, that's a very good point!

How about splitting server.New() into a function which registers the metrics and a function which creates the server? And leave the existing API creating a singleton which lasts the life of the program.

I've drafted this solution to see how it looks here: https://github.com/weaveworks/common/pull/289. It looks good to me - let me know if you have any early feedback. I'll add some tests before marking it as ready for review.

thampiotr commented 1 year ago

I've added tests and the change is now ready for review: https://github.com/weaveworks/common/pull/289