should unregister metrics from prometheus registry

[qinguoan]

open close, open (error occurred), because register metrics to promethues registry twice.

[krasi-georgiev]

sorry I don't quite understand the issue. If you want someone to look into an issue it should include a lot more details.

[simonpasquier]

IIUC @qinguoan complains about this:

r := prometheus.NewRegistry()
db, _ := tsdb.Open("/tmp", nil, r, nil)
db.Close()
db, _ = tsdb.Open("/tmp", nil, r, nil) // <- this will panic

The "obvious" solution is to pass a nil registry for subsequent calls of tsdb.Open(). At least, the documentation should be improved to warn about it. Not sure if we want to change the API or Unregister the metrics on Close().

[krasi-georgiev]

thanks for clarifying I have put it in my tasks queue and will check if we can do some check inside the Open func

[juliusv]

The "obvious" solution is to pass a nil registry for subsequent calls of tsdb.Open().

But then the registry will keep the metrics of the now-closed TSDB instead of the new TSDB's metrics.

I don't think we have a precedent for unregistering metrics in Close(), but I also don't see why it would be a bad idea.

[brian-brazil]

The Go client will prevent those metrics being re-registered.

[juliusv]

@brian-brazil I thought it would work as long as the help, type, labels, etc. do not change. From https://godoc.org/github.com/prometheus/client_golang/prometheus#Registerer:

    [...]
    // Note that even after unregistering, it will not be possible to
    // register a new Collector that is inconsistent with the unregistered
    // Collector, e.g. a Collector collecting metrics with the same name but
    // a different help string. The rationale here is that the same registry
    // instance must only collect consistent metrics throughout its
    // lifetime.
    Unregister(Collector) bool

[brian-brazil]

@qinguoan Can you explain why you are opening multiple TSDBs?

[krasi-georgiev]

@juliusv shall we call this a bug so I can look into it, or wait a bit longer to see if @qinguoan will post his use case?

[juliusv]

Whether it's a bug or request for code usage enhancement is up to anyone's interpretation, but just unregistering metrics upon close seems like a decent idea.

[gouthamve]

Yes, even I think it makes sense to unregister. Closing a db and opening the same one again will also error.

[brian-brazil]

That would remove the metrics from /metrics though, and metrics should be present until the process dies. This could result in missing these on the last scrape.

On Fri 31 Aug 2018, 11:26 Goutham Veeramachaneni, notifications@github.com wrote:

Yes, even I think it makes sense to unregister. Closing a db and opening the same one again will also error.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/prometheus/tsdb/issues/367#issuecomment-417607752, or mute the thread https://github.com/notifications/unsubscribe-auth/AGyTdqyi-1NZiZ6uQL5N023uJPYbxUAhks5uWQEsgaJpZM4V6Io_ .

[krasi-georgiev]

I looked into the metrics implementation and these are spread across different structs in tsdb so unregistering all metrics at db.close() would requite some refactoring.

@qinguoan do you still need this and can you give some details on why do you need to open a new db using the same Registry?

[krasi-georgiev]

looks stale, unless we have some good use case for this it is not worth such a big refactoring.