fabxc
commented
8 years ago Cool, I'm very much in favor of this.
Performance/allocation penalty of the slice interface is not overly relevant as registration and collection happens relatively rare. (Exception would be high-frequency scrape scenarios.)
Haven't measured it, but I am relatively certain that channels are more expensive anyway.
Only "power users" get in touch with the Collector interface, which limits the damage of the unusual API.
Not sure about that. It's very common in exporters and can also be in regular code, where it's the easier/cleaner option than always keeping a regular variable in sync with an instrumentation gauge or counter.
func (c *C) Collect(m chan<- prometheus.Metric) {
m <- c.constMetric
c.subCollector.Collect(m)
c.anotherSubCollector.Collect(m)
}I always found this relatively hard to read/understand without proper context of the client library. Realistically, most devs don't want instrumentation be an understanding overhead.
The pain point here, especially for concurrent collections, is that the user has to care about bottom-up propagation. May I suggest an alternative interface:
type Sampler interface {
Sample(...Metric)
SampleFrom(...Collector)
}
type Registrator interface {
Register(...*Desc)
RegisterFrom(...Collector)
}
type Collector interface {
Describe(Registrator)
Collect(Sampler)
}We are perfectly fine by the functionality being implemented using channels. We just want to keep this as an implementation detail. So passing in a receiving object as an argument still seems fine to me and keeps things top-down. Naming is a bit tricky of course as we definitely have a verb overlap across various concerns.
Sequential collection:
func (c *C) Collect(s Sampler) {
s.Sample(c.constMetric1, c.constMetric2)
s.SampleFrom(c.subCollector, c.anotherSubCollector)
}Concurrent collection:
func (c *C) Collect(s Sampler) {
var wg sync.WaitGroup
wg.Add(len(c.colls))
for _, coll := range c.colls {
go func(coll prometheus.Collector) {
s.SampleFrom(coll)
// equivalent: coll.Collect(s)
wg.Done()
}(coll)
}
wg.Wait()
}As concurrent collection may be common in exporters, we can actually make that a bit more convenient to users:
type Sampler interface {
Sample(...Metric)
SampleFrom(...Collector)
SampleAsyncFrom(...Collector)
}Concurrent collection:
func (c *C) Collect(s Sampler) {
s.SampleAsyncFrom(c.colls...)
}The async case of couse exposes no control for the degree of concurrency. However, I hardly expect this to ever be relevant and it can still be solved via:
func (c *C) Collect(s Sampler) {
s.SampleAsyncFrom(c.colls[:10]...)
s.SampleAsyncFrom(c.colls[10:]...)
}Still, naming is tricky, we could also go with passing anonymous functions. There we would want to stick with a single function though and the *From methods would have to be dropped.
beorn7
grobie
brian-brazil
bboreham
jacksontj
This is the current
Collectorinterface:The main reason for having channels there was that back in 2013, channels were cool, so I was primarily looking for neat ways to make use of them rather than avoiding them in APIs. While the current interface has some merits, the pattern used here is very unconventional, and now that breaking changes will happen anyway, we have the chance of changing the interface. In the following, I'll list pros and cons to come to an informed decision.
The new interface would look like:
Reasons for changing the interface
Reasons for keeping the interface as is
Appendix: Code examples
Sequential collection with old interface
Sequential collection with new interface
or more compact (but arguably also more confusing):
Concurrent collection with old interface
Concurrent collection with new interface