Embed Ingester metrics into struct

[jtlisi]

Relates To: #1512

This PR embeds the metrics from the ingester into the ingester struct. This means they will only be initialized if an ingester struct is initialized.

I avoided embedding a prometheus Registerer into the config and passing it because we do not currently use non-default registerers anywhere in cortex. Down the line we may want to consider this approach.

Signed-off-by: Jacob Lisi jacob.t.lisi@gmail.com

[beorn7]

I think this change only addresses a small part of the problems. It's essentially only moving the registration into the constructor, but still registers everything with the global registry. That's certainly an improvement, but you could have it much simpler (by not using promauto metrics but the normal metrics from the prometheus package, and then explicitly register them with prometheus.Register in the constructor). The problem now is that you will get a panic if you call ingester.New twice (e.g. in a test), with no way of intercepting it.

There are many ways to do it, but I see a certain convergence onto something like in prometheus/tsdb. Check out https://github.com/prometheus/tsdb/blob/master/db.go#L426 and take it from there. They are also using a metrics struct there. Note how you can pass in nil as the prometheus.Registerer, which will just not register anything.

[jtlisi]

@beorn7 Thanks for the feedback, I updated this PR to reflect your suggestions.

[beorn7]

I'll have a detailed look ASAP (but perhaps not tonight… 😴 ).

[csmarchbanks]

I like the pattern, but what is the scope you want to remove metrics for? It looks like you only removed the metrics from ingester.go, but there are lots of other metrics being registered by promauto or init in the ingester package.

[beorn7]

I think this is just meant as an incremental step to try things out, with the goal of converting all metrics in the package to the same pattern eventually.

[csmarchbanks]

This incremental step looks good. If you want to add more metrics from the ingester package to the struct I am happy to review again though!

[bboreham]

seems legit