Allow white- and blacklisting metrics to be exposed #488
Conversation
k8s-ci-robot
added
cncf-cla: yes
CHANGELOG.md
Outdated
| @@ -1,6 +1,7 @@ | |||
| ## Unreleased | |||
|
|
|||
| * [CHANGE] `kube_job_status_start_time` and `kube_job_status_completion_time` metric types changed from counter to gauge. | |||
| * [FEATURE] Allow white- and blacklisting metrics to be exposed. | |||
There was a problem hiding this comment.
s/white- and blacklisting/white- and black-listing
pkg/options/types.go
Outdated
| @@ -25,6 +25,43 @@ import ( | |||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |||
| ) | |||
|
|
|||
| type MetricSet map[string]struct{} | |||
|
|
|||
| func (c *MetricSet) String() string { | |||
There was a problem hiding this comment.
s/c /ms. ms stands for MetricSet
pkg/options/types.go
Outdated
| type MetricSet map[string]struct{} | ||
|
|
||
| func (c *MetricSet) String() string { | ||
| s := *c |
pkg/options/types.go
Outdated
| return strings.Join(ss, ",") | ||
| } | ||
|
|
||
| func (c *MetricSet) Set(value string) error { |
pkg/options/types.go
Outdated
| } | ||
|
|
||
| func (c *MetricSet) Set(value string) error { | ||
| s := *c |
pkg/options/types.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (s MetricSet) asSlice() []string { |
There was a problem hiding this comment.
s/s /ms
Actually s is fine to me, but i think we should be consistent. :)
pkg/options/types.go
Outdated
| return metrics | ||
| } | ||
|
|
||
| func (s MetricSet) IsEmpty() bool { |
pkg/options/types.go
Outdated
| return len(s.asSlice()) == 0 | ||
| } | ||
|
|
||
| func (s *MetricSet) Type() string { |
main.go
Outdated
| blacklistEnabled := !blacklist.IsEmpty() | ||
| filterEnabled := whitelistEnabled || blacklistEnabled | ||
|
|
||
| return GathererFunc(func() ([]*dto.MetricFamily, error) { |
There was a problem hiding this comment.
I suggest we make this function a package-level function thus we can add some unit tests for it. We need to determine the relationship between whitelist and blacklist:
- Does we permit users specify both whitelist and blacklist
- Does kube-state-metrics should only prefer whitelist and ignore blacklist even blacklist is not empty.
Currently, the implementation is kube-state-metrics prefer whitelist metrics even blacklist is not empty which i think is not the right logic. :)
The right way is to filter the collected metrics on whitelist and blacklist and then merge the filtered metrics.
brancz
force-pushed
the
limit
branch
2 times, most recently
from
64bad71 to
6d04729
Compare
pkg/metrics/metrics.go
Outdated
| blacklistEnabled := !blacklist.IsEmpty() | ||
| filterEnabled := whitelistEnabled || blacklistEnabled | ||
|
|
||
| return GathererFunc(func() ([]*dto.MetricFamily, error) { |
There was a problem hiding this comment.
I suggest we split the GathererFunc into a inter-package level function, just like FilteredGatherer And add some ut for this.
There was a problem hiding this comment.
@brancz Once you have time, could you please update this PR? :)
There was a problem hiding this comment.
Can you explain what you meant me to do with GathererFunc? It's just so a function satisfies the prometheus.Gatherer interface.
pkg/metrics/metrics.go
Outdated
| continue | ||
| } | ||
|
|
||
| _, onBlacklist := blacklist[name] |
There was a problem hiding this comment.
Why do you check whether whitelistEnabled is true, but you don't check blacklistEnabled here.
There was a problem hiding this comment.
We have to opt for one strategy here. Both the whitelist and blacklist are explicit, so which one is the right one, the one where we explicitly said we definitely want this, or the one where we said we definitely don't want this. As whitelists are usually pretty hand picked I figured that would be the stronger of the two.
There was a problem hiding this comment.
I'm going to make sure to document he precedence behavior.
There was a problem hiding this comment.
I changed blacklist and whitelist to be mutually exclusive. It really doesn't make any sense to have both here.
|
This is ready for a re-review @andyxning @dohnto. |
| @@ -69,6 +73,8 @@ func (o *Options) AddFlags() { | |||
| o.flags.StringVar(&o.TelemetryHost, "telemetry-host", "0.0.0.0", `Host to expose kube-state-metrics self metrics on.`) | |||
| o.flags.Var(&o.Collectors, "collectors", fmt.Sprintf("Comma-separated list of collectors to be enabled. Defaults to %q", &DefaultCollectors)) | |||
| o.flags.Var(&o.Namespaces, "namespace", fmt.Sprintf("Comma-separated list of namespaces to be enabled. Defaults to %q", &DefaultNamespaces)) | |||
| o.flags.Var(&o.MetricWhitelist, "metric-whitelist", "Comma-separated list of metrics to be exposed. The whitelist and blacklist are mutually exclusive.") | |||
There was a problem hiding this comment.
We should state this clearly. !
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyxning, brancz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
In large Kubernetes clusters single metrics can have a significant effect on the total number of metrics exposed by a single scrape. This PR allows white or blacklisting metrics to be exposed.
@andyxning