Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Create a public registry interface and separate out HTTP exposition
General context and approch =========================== This is the first part of the long awaited wider refurbishment of `client_golang/prometheus/...`. After a lot of struggling, I decided to not go for one breaking big-bang, but cut things into smaller steps after all, mostly to keep the changes manageable and easy to review. I'm aiming for having the invasive breaking changes concentrated in as few steps as possible (ideally one). Some steps will not be breaking at all, but typically there will be breaking changes that only affect quite special cases so that 95+% of users will not be affected. This first step is an example for that, see details below. What's happening in this commit? ================================ This step is about finally creating an exported registry interface. This could not be done by simply export the existing internal implementation because the interface would be _way_ too fat. This commit introduces a very lean `Registry` interface. Most of the existing functionality that is not part of that interface is provided by helper functions, not by methods (e.g. `MustRegisterWith`). The functions that act on the default registry are retained (with very few exceptions) so that most use cases won't see a change. The default registry is kept in the public variable `DefaultRegistry`. This follows the example of the http package in the standard library (cf. `http.DefaultServeMux`, `http.DefaultClient`) with the same implications. Another important part in making the registry lean is the extraction of the HTTP exposition, which also allows for customization of the HTTP exposition. The following issues are fixed by this commit (some solved "on the fly" now that I was touching the code anyway and it would have been stupid to port the bugs): #46 #100 #170 #205 What future changes does this commit enable? ============================================ The following items are not yet implemented, but this commit opens the possibility of implementing these independently. - The separation of the HTTP exposition allows the implementation of other exposition methods based on the Registry interface, as known from other Prometheus client libraries, e.g. sending the metrics to Graphite. Cf. #197 - The public `Registry` interface allows the implementation of convenience tools for testing metrics collection. Those tools can inspect the collected MetricFamily protobufs and compare them to expectation. Also, tests can use their own testing instance of a registry. Cf. #58 Notable non-goals of this commit ================================ Non-goals that will be tackled later ------------------------------------ The following two issues are quite closely connected to the changes in this commit but the line has been drawn deliberately to address them in later steps of the refurbishment: - `InstrumentHandler` has many known problems. The plan is to create a saner way to conveniently intrument HTTP handlers and remove the old `InstrumentHandler` altogether. To keep breakage low for now, even the default handler to expose metrics is still using the old `InstrumentHandler`. Cf. #200 - There is work underway to make the whole handling of metric descriptors (`Desc`) more intuitive and transparent for the user (including an ability for less strict checking, cf. #47). That's quite invasive from the perspective of the internal code, namely the registry. I deliberately kept those changes out of this commit. Non-goals that _might_ be tackled later --------------------------------------- There is a strong and understandable urge to divide the `prometheus` package into a number of sub-packages (like `registry`, `collectors`, `http`, `metrics`, …). However, to not run into a multitude of circular import chains, this would need to break every single existing usage of the library. (As just one example, if the ubiquitious `prometheus.MustRegister` (with more than 2,000 uses on GitHub alone) is kept in the `prometheus` package, but the other registry concerns go into a new `registry` package, then the `prometheus` package would import the `registry` package (to call the actual register method), while at the same time the `registry` package needs to import the `prometheus` package to access `Collector`, `Metric`, `Desc` and more. If we moved `MustRegister` into the `registry` package, thousands of code lines would have to be fixed (which would be easy if the world was a mono repo, but it is not).) The main problem is really the top-level functions like `MustRegister`, `Handler`, `Push`, …, which effectively pull everything into one package. Those functions are however very convenient for the easy and very frequent use-cases. This problem has to be revisited later. For now, I'm trying to keep the amount of exported names in the package as low as possible (e.g. I unexported expvarCollector in this commit because the NewExpvarCollector constructor is enough to export, similar to the other exporters). Non-goals that won't be tackled anytime soon -------------------------------------------- Something that I have played with a lot is "streaming collection", i.e. allow an implementation of the `Registry` interface that collects metrics incrementally and serves them while doing so. As it has turned out, this has many many issues and makes the `Registry` interface very clunky. Eventually, I made the call that it is unlikely we will really implement streaming collection, and making the interface more clunky for something that might not even happen is really a big no-no. Note that the `Registry` interface only creates the in-memory representation of the metric family protobufs in one go. The serializaton onto the wire can still be handled in a streaming fashion. What are the breaking changes? ============================== - Signature of functions pushing to Pushgateway has changed to allow arbitrary grouping (which was planned for a long time anyway, and now that I worked on it, I finally did it, cf. #100). - `SetMetricFamilyInjectionHook` is gone. A registry with a MetricFamily injection hook has to be created now with `NewRegistryWithInjectionHook`. - `PanicOnCollectError` is gone. This behavior can now be configured when creating a custom HTTP handler. - `EnableCollectChecks` is gone. A registry with those checks can now be created with `NewPedanticRegistry` (it is only ever used to test custom Collectors).
- Loading branch information