*.: Add a way to ensure all created metrics are properly registered in Prometheus Registry.

[bwplotka]

We have lot's of issue recently when we add metric, instrument and it's not actually registered 🤦‍♂️ e.g #2100

That's enough. I am looking on a mandatory way to test if ALL metrics are actually registered when you start Thanos binary.

Some ideas:
A) Mandatory unit test for all Metrics structs with Registerer that checks what was registered.

• More automated?
  B) Grep for all metrics creations and check e2e tests if /metrics shows them (kind of primitive)
  C) New** methods for Prometheus client that registers metrics in custom registery
  D) Wrap prometheus client library with something that allows checking if New* method that creates metrics has the corresponding metrics in Registered. (weird)

Any hints on how to do it, @brian-brazil @beorn7 @brancz ? I think C makes sense the most + add item to CONTRIBUTING.md

[bwplotka]

@beorn7 is there an idea to improve prom client for such metric constructor? Which allows passing Registerer? Why would you not register that's the question :/ I guess this is for ability to register in multiple registeres...

[brian-brazil]

https://www.robustperception.io/when-should-you-unit-test-instrumentation and https://www.robustperception.io/how-to-unit-test-prometheus-instrumentation have my general thoughts.

Promauto with a global registry does have its advantages.

[bwplotka]

Yes, but we need something for the custom registry. The global registry is anti-pattern as we tried to explain during out talk with @kakkoyun http://are-you-testing-your-observability.now.sh/

[bwplotka]

Maybe we should just expand promauto package (: That would be nice

[beorn7]

The global registry is anti-pattern as we tried to explain during out talk

This will always be a point of contention. I wouldn't take for granted that all will share your opinion, even after watching your talk.

Maybe we should just expand promauto package

The "elegance" of the global registry is that you can just say promauto.NewCounter with the normal arguments. With a custom registry, you need to pass it in as an additional parameter. That doubles the (already high) number of constructor functions. And then you need to add another set with Must... because the auto-register functions would return an error and you probably want the panicking versions, too.

Or you go down the approach that OpenTelemetry took and add factory methods like NewCounter to the registry. Those method would return collectors already registered with the registry whose method you have called. I considered this approach multiple times but always ran into some reason not to use it. Things I remember right now:

• It would be a quite large shift from the current pattern.
• It would be "yet another way of doing the same thing".
• Since registration can fail, the return value of those methods needed to include an error. And/or you create MustNewCounter style methods, which would panic instead.
• One of the reasons why people use custom registries is that they want to register the same collector with different libraries, or they want to have the option of not registering at all or register later. (Those needs are certainly rare, but very easy and common cases can be handled with a global registry. I'm concerned if we provide three different ways (for easy cases, intermediate cases, and rare special cases) instead of just two.)
• The registry would not have a simple interface with only Register and Gather as methods, but there would be a very fat interface (NewCounter, MustNewCounter, NewGauge, MustNewGauge, NewCounterVec, etc. etc.).

I'm sure there are more reasons.

Coming back to your original problem:

• It's easy to write glue code on your own for the suggested factory functions that create a metric and register it immediately. If it works out well, we can consider them as additions to promauto or in a separate package.
• Have you looked at https://godoc.org/github.com/prometheus/client_golang/prometheus/testutil#GatherAndCompare ?
• You could write a little helper that registers all Collectors in a struct via reflection. (I have considered that quite often, and it might be a good idea as a tool in client_golang. Again, I would be interested to see it in use first.)

[bwplotka]

Awesome, thanks for this ❤️

This will always be a point of contention. I wouldn't take for granted that all will share your opinion, even after watching your talk.

Totally, but at least I can share my opinion, right? (: And I don't expect anyone to change their mind after our talk. It was touching many things (: And we need to work on persuasion skills first... (:

.With a custom registry, you need to pass it in as an additional parameter. That doubles the (already high) number of constructor functions.

Hm, I think I like it despite the arguments...

OK, so it looks we have plenty of options here:

A) Mandatory unit test for all Metrics structs with Registerer that checks what was registered.
B) our wrapper for: New** methods for Prometheus client that registers metrics in the custom registry
C) our wrapper for: register with New** methods (so your factory idea)
D) You could write a little helper that registers all Collectors in a struct via reflection. (I have considered that quite often, and it might be a good idea as a tool in client_golang. Again, I would be interested to see it in use first.)

Hm.. could give example for D @beorn7 ?

I love the idea of us exploring one of those and propose this to https://github.com/prometheus/client_golang once we proved it makes sense.

[beorn7]

Hm.. could give example for D @beorn7 ?

prometheus/client_golang#712

[bwplotka]

❤️ Thanks! I commented, on your PR.. TL;DR I think still some promauto.NewCounter(reg, opts...) is the most tempting for me. (:

[beorn7]

prometheus/client_golang#713 is my idea of going down the promauto path.

[bwplotka]

I am super happy with: prometheus/client_golang#713

This would be my choice.

Any other opinion @SuperQ @GiedriusS @kakkoyun @yeya24 ?

[kakkoyun]

I also really like the factory idea in prometheus/client_golang#713.
I don't know how intensively reflection used in client_golang, but I'd love to avoid that., for this one.

So thanks @beorn7 ❤️ Please feel free to reach out, if you need a hand on this.

[bwplotka]

AC:

• Update prometheus client go dependency
• Move all metric creation to newly created promauto.With