Add HandlerWithAllowlist which can control exposed metrics #1327
Conversation
There was a problem hiding this comment.
Thanks!
Normally this could be done on registration, so you only register what you want.
I guess you want to register all and filter afterwards in one place. This has consequences, one would check in the code that certain metric is instrumented but it's actually ignored near the handler.
Still, I think it's fine, however there is no need to add complexity to handler AND add new gatherer. We could simply add new gatherer similar to what you have here. Anyone can then wrap their registry with it.
prometheus/promhttp/http.go
Outdated
| @@ -381,6 +393,35 @@ type HandlerOpts struct { | |||
| ProcessStartTime time.Time | |||
| } | |||
|
|
|||
| type FilteredDefaultGatherer struct { | |||
There was a problem hiding this comment.
I would just add FilteredGatherer that wraps registry and pass through only matching metrics. Matchers can be done with something like https://github.com/prometheus/client_golang/blob/main/prometheus/collectors/go_collector_latest.go#L93 - regexp.
This allows us to allow list those metrics you wanted to hardcode here, outside of FilteredGatherer - on caller side (:
CatherineF-dev
force-pushed
the
allowlist
branch
from
90037a6
to
19e5daa
Compare
CatherineF-dev
force-pushed
the
allowlist
branch
from
19e5daa
to
bf4494e
Compare
There was a problem hiding this comment.
Is it still needed? I noticed the CFP/feature request was closed.
Nevertheless it feels useful, just we might want to discuss an exact API. Thanks! 🤗
| @@ -90,6 +93,13 @@ func HandlerFor(reg prometheus.Gatherer, opts HandlerOpts) http.Handler { | |||
| return HandlerForTransactional(prometheus.ToTransactionalGatherer(reg), opts) | |||
| } | |||
|
|
|||
| // HandlerWithAllowlist remove metrics whose name can't match with metricNameMatcher | |||
| func HandlerWithAllowlist(metricNameMatcher *regexp.Regexp) http.Handler { | |||
There was a problem hiding this comment.
This is nice, but we might want to discuss exact API. e.g. do we want only metric names or also labels? What would be flexible enough?
We think it might be useful to tackle that with OpenMetrics changes and have same API for per scrape or per handler options 🤔
After adding HandlerWithAllowlist, cilium or other components can exclude high cardinality metrics instead of disabling metrics collection in a running cluster.
context: cilium/cilium#27471
I notice this pr introduces a new dependency (dto "github.com/prometheus/client_model/go"), maybe need to move HandlerWithAllowlist to another file.