Please expose `wrappingCollector` publicly

[joewreschnig]

trafficstars

We have a number of custom collectors that wrap other collectors - e.g. some cached value is periodically refreshed, where we want to collect metrics about the last refresh time, and also metrics about the value itself.

Currently for the outer collector to add a label or metric to the inner collector, we either need to reimplement wrappingCollector ourselves, or we need to also apply the labels to the outer collector when it's registered using e.g. WrapRegistererWith, in turn leading to some ugly patterns like

	{
		r := prometheus.WrapRegistererWith(labels, prometheus.DefaultRegisterer)
		r.MustRegister(c)
		defer r.Unregister(c)
	}

repeated for every collector with different labels.

With a public wrappingCollector we could avoid this by either wrapping it at the layer between the outer and inner collector.

[bwplotka]

Hi, thanks for proposing this.

It sounds what you try to achieve is similar to our attempt here: https://github.com/prometheus/client_golang/blob/cache/prometheus/cache/cache.go

Do you mind providing us with more details? It might be useful to upstream that special collector if this will be useful for others!

Currently for the outer collector to add a label or metric to the inner collector, we either need to reimplement wrappingCollector ourselves, or we need to also apply the labels to the outer collector when it's registered using e.g. WrapRegistererWith, in turn leading to some ugly patterns like. With a public wrappingCollector we could avoid this by either wrapping it at the layer between the outer and inner collector.

Does it mean you want to change the labels passed to WrapRegistererWith dynamically?

[joewreschnig]

Sorry, I think I wasn't clear about presenting my use case and it got confused with what I'm asking for. I don't want to cache metrics, and I don't want to change labels over time. I'm just using a cache (of some application-specific objects, not metrics) as an example of where we have nested prometheus.Collectors and I would like to use wrapped metrics.

tl;dr: If wrappingCollector (or wrapDesc / wrappingMetric) was exposed publicly we could write our custom collectors in a clearer and more flexible way.

Detailed use case information

We have a general-purpose "cache some value and refresh it every X interval" type, Cache[T]. This is a prometheus.Collector, it reports metrics like update_timestamp_seconds and errors_total. But it's also caching something, like an Image or a GitRepository. These are themselves Collectors, e.g. Image might report height and width, GitRepository repo_size_bytes, repo_branches, etc. Cache's Collect method checks whether the cached value is a collector, and if so, also reports its metrics. The cached value can't be registered directly because nothing has a direct reference to it other than the Cache, and because it's changing in parallel with other work.

The problem arises when a program has multiple Cache values; the update_timestamp_seconds metric will conflict. If it's both a Cache[Image] and a Cache[GitRepository] at the same time we can work around that by providing some Labels to NewCache. But we still can't have two Cache[Image]s at the same time because the image metrics will conflict. Without something like wrappingCollector or wrapDesc and wrappedMetric, Cache has no way to apply its labels to the subcollection.

As Desc is entirely private, implementing our own wrappingCollector equivalent quickly explodes how much of the client we need to reimplement. Otherwise the prometheus package only exposes wrapping at the registry level, so then we either have to push all wrapping parameters down the stack all the way to the scope of registration lifetime, or we need to invert control and have Cache self-register/unregister which leads to a more complex Cache implementation (e.g. a manual finalizer).

If we did have a public WrappingCollector, we could instead compose c completely ahead of time, even in a completely different scope, and the registration process doesn't have to care whether it's wrapped or not:

	c := prometheus.Wrap(cache, labels)
	// ...
	prometheus.MustRegister(c)
	defer prometheus.Unregister(c)

Or, again independently of registration details, Cache's Collect method could wrap Descs and Metrics individually when forwarding from the subcollector. (Probably we would have Cache use the first approach but we have some other subcollectors which might use the second.)

[SuperQ]

I have another possible use case for this.

We have a process worker pool controller. We'd like to use collectors.NewProcessCollector() to create a collector for each worker.

But that function needs to be wrapped with additional worker_poolandworker_id` labels.

So we'd end up with something like this:

for w := range workers {
  c := prometheus.Wrap(
      collectors.NewProcessCollector(...),
      prometheus.Labels{"worker_id", w.ID},
  )
  prometheus.MustRegister(c)
}

Not sure if this is valid, but that's what we're in need of.

[SuperQ]

Nevermind, this turned out to be easier to do than I thought.

prometheus.WrapRegistererWith(
  prometheus.Labels{"worker_id": workerID},
  prometheus.DefaultRegisterer,
).MustRegister(collectors.NewProcessCollector(...))

[stale[bot]]

Hello 👋 Looks like there was no activity on this issue for the last 3 months. Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗 If there will be no activity in the next 4 weeks, this issue will be closed (we can always reopen an issue if we need!).

[joewreschnig]

I'm still interested in this.

We could probably improve our user-facing API a little after #1103, which would make it easier for the cache to maintain its own Registry doing all the wrapping. But implementation-wise wrapping the Collector directly still seems like the better option to me - it's less intrusive in the structs in question and lighter than having lots of full Registrys around.

[stale[bot]]

Closing for now as promised, let us know if you need this to be reopened! 🤗