Add a "factory" to make promauto work for custom registries #713
Conversation
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>
This constructor was simply forgotten. Signed-off-by: beorn7 <beorn@grafana.com>
beorn7
force-pushed
the
beorn7/promauto
branch
from
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 |
| // 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.
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.
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 |
| // 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.
That should be really not recommended. I would remove the word often .. (:
There was a problem hiding this comment.
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! |
| r prometheus.Registerer | ||
| } | ||
|
|
||
| // With creates a Factory using the provided Registerer for registration of the |
There was a problem hiding this comment.
What if registerer is nil? (We often do that for tests)
There was a problem hiding this comment.
Oh actually it is just not registered... Awesome! ❤️
There was a problem hiding this comment.
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
promautoextension, but it is done in a way that reads more nicely than an additionalRegistererparameter in each constructor function.