prometheus.Unregister does not actually unregister the collector in a meaningful way

[haspeterd]

client_golang/prometheus/registry.go : func (r *registry) Unregister(c Collector) bool

There's a comment at the end of this function:

    // dimHashesByName is left untouched as those must be consistent
    // throughout the lifetime of a program.

What this means is that if you unregister a collector it will remove it from the map of collectors - so you can register it again. But the description sticks around, so unless you register it with the same help, MustRegister will panic when you re-register.

This comes up mostly in the context of unit testing. I'd love to just have a defer unregister in every test case, but that gives the impression of working without actually allowing you to cleanly re-register a collector of the same name in a different test case. The workaround seems to be to just refactor my code to mock out the prometheus registration, but it would be nice if I didn't have to do that.

Perhaps if the library provided mock implementations of the registry, rather than relying on the static var declaration in registry.go? As it is, no test code can safely use prometheus.Register() without either wrapping it in boilerplate or making sure that all descriptions are always the same.

Failing that, the comment for Unregister() should be much clearer about the fact that some traces of the old collector will remain.

[beorn7]

Unregister is not there for test purposes. The unregistering happens in the only meaningful way possible. The title of this issue is misleading. The client library invests a whole lot of effort to prevent inconsistent metric exposition.

What you want, is a separate registry, so you can use a separate registry for each test case. That's covered in https://github.com/prometheus/client_golang/issues/46 .

I'll declare this a dup of https://github.com/prometheus/client_golang/issues/46 . Please follow up if I misunderstood you.

[haspeterd]

Totally fine that unregister doesn't do what I think it should do; the problem then becomes that the function documentation is misleading/incomplete. The fact that unregister does not remove all traces of the collector is a significant part of its contract that isn't visible without digging into the implementation. Adding a simple comment to this effect would be beneficial.

46 would definitely meet my use case; in the meantime it's fairly straightforward to wrap the registry calls in a facade that's swappable for unit tests.

[haspeterd]

Opened https://github.com/prometheus/client_golang/issues/205 to fix the documentation.

[xaionaro]

The unregistering happens in the only meaningful way possible.

Could you elaborate on this, please? What if I want to remove a CounterVec and re-add it with more labels?

[beorn7]

That's not possible by design. The idea is that one and the same binary, during its lifetime, never exposes metrics that are inconsistent with each other.

[xaionaro]

@beorn7 : Sorry, I just do not follow why not to clean-up dimHashesByName. What is the negative impact here? The positive impact is clear: for example it will be possible to re-register a metric with another set of labels.

[beorn7]

The positive impact is clear: for example it will be possible to re-register a metric with another set of labels.

That's not a positive impact, it's the actual problem. When dealing with non-trivial PromQL, it's very confusing if labels on a metric suddenly appear or disappear. We cannot prevent that from happening globally, but at least instrumentation libraries can guarantee it for the pre-defined collectors over the lifetime of a binary. See https://prometheus.io/docs/instrumenting/writing_clientlibs/#labels