New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a "factory" to make promauto work for custom registries #713
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is 100% what we need!
Thank you! I am voting to merge this (I suggested minor thing) and we are happy to move to this in Thanos 🚀
Thanks for this work ❤️
Signed-off-by: beorn7 <beorn@grafana.com>
Also, update the package documentation. The concerns about the global registry aren't really valid anymore because promauto now also works with custom registries. The musings about the http.DefaultMux are more a digression and shouldn't be in a doc comment. Signed-off-by: beorn7 <beorn@grafana.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! I think "factory" sounds OK. Thanks for this 👍
This constructor was simply forgotten. Signed-off-by: beorn7 <beorn@grafana.com>
85c9767
to
1c2884b
Compare
This is now ready for the final review. As a byproduct, I realized that Context for the above: Untyped metrics cannot really happen in direct instrumentation, that's why |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing, thanks!
LGTM 👍
// NewConstSummary (and their respective Must… versions). NewConstMetric is used | ||
// for all metric types with just a float64 as their value: Counter, Gauge, and | ||
// a special “type” called Untyped. Use the latter if you are not sure if the | ||
// mirrored metric is a Counter or a Gauge. Creation of the Metric instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious. Why in this case:
mirrored metric is a Counter or a Gauge.
We cannot just use Gauge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you couldn't/shouldn't use rate
on a gauge. It's really a difference if something is a Gauge for sure or if you don't know if it is a Gauge or a Counter.
Untyped
is really mostly for mirroring some 3rd party metrics where you don't know, or the kind of metrics even changes between different scenarios (happens typically for hardware metrics exposed in generic ways).
// (prometheus.DefaultRegisterer). This allows very compact code, avoiding any | ||
// references to the registry altogether, but all the constructors in this | ||
// package will panic if the registration fails. | ||
// Package promauto provides alternative constructors for the fundamental |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
// local or the global registry, and if you want to handle the error or risk a | ||
// panic. With the constructors in the promauto package, registration is | ||
// automatic, and if it fails, it will always panic. Furthermore, the | ||
// constructors will often be called in the var section of a file, which means |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be really not recommended. I would remove the word often
.. (:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of people do use the global registry, and for them it's normal to create metrics in the var
section. Even important players in the Prometheus community recommend that. We have to leave it open here what way to pick. If I remove the "often", it will sounds like you would always create metrics in the var
section.
// whoever wants to use it, will do so explicitly, with an opportunity to read | ||
// this warning. | ||
// | ||
// Enjoy promauto responsibly! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha, thanks!
r prometheus.Registerer | ||
} | ||
|
||
// With creates a Factory using the provided Registerer for registration of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if registerer is nil? (We often do that for tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh actually it is just not registered... Awesome! ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's also documented in the Factory
doc comment.
Besides the scary naming (happy to hear better ideas than the dreaded "factory"), the whole package documentation needs to updated, should we go down this path. (We don't need to talk about how promauto only works with the global registry anymore.)
@bwplotka I think this comes close to your request for a
promauto
extension, but it is done in a way that reads more nicely than an additionalRegisterer
parameter in each constructor function.