Use AllMetrics for enable & disable lists instead of ignoreWhitelist #2910
Conversation
|
Hi @eero-t. Thanks for your PR. I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
container/factory.go
Outdated
| func (ms *MetricSet) Set(value string) error { | ||
| *ms = MetricSet{} | ||
| if value == "" { | ||
| return nil | ||
| } | ||
| for _, metric := range strings.Split(value, ",") { | ||
| if AllMetrics.Has(MetricKind(metric)) { | ||
| (*ms).Add(MetricKind(metric)) | ||
| } else { | ||
| return fmt.Errorf("unsupported metric %q specified", metric) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Can you add a comment to MetricSet and state that the struct is not thread safe? And make sure that the struct is not accessed from multiple goroutines in cAdvisor?
There was a problem hiding this comment.
I added couple of commits clarifying & documenting MericSet thread-safety (removing unused Append() method, changing Add() method to private one and documenting Set() method usage).
|
/ok-to-test |
Fixes also -disable_metrics 'process' metric name in running.md, and updates list of '-disable_metrics' option metrics. Metrics names are now listed in sorted order. Note: I've followed the existing documentation convention of giving cAdvisor option names with two dashes (--enable_metrics) although current cAdvisor uses only one dash (-enable_metrics).
Use of AllMetrics instead of ignoreWhitelist, adds CpuUsageMetrics
("cpu") & AppMetrics ("app") to metrics that can be disabled/enabled
from command line. Whether they are enabled is already checked by
prometheus export code.
That also makes metricSetValue type redundant, as it can now be
replaced with MetricSet use. Removing it requires moving String() &
Set() methods used by flag package for option handling and validating
given metrics options, to factory.go.
Inlining container.AllMetrics.Difference() call makes the code intent more explicit, and is about same amount of code.
Both Add() and Set() methods modify the existing mapping. As Add() method is used only by Set() and Difference(), and in latter it's done thread-safely, make add private method and document that Set() is not thread-safe. Set() is used only by flag package through Value interface, which is documented to be called only once, i.e. it's safe.
Add app & cpu metrics to relevant places.
eero-t
force-pushed
the
remove-ignorewhitelist
branch
from
ee0b8e7
to
9ed2b33
Compare
|
Changes:
|
Use of AllMetrics instead of ignoreWhitelist, adds CpuUsageMetrics ("cpu") & AppMetrics ("app"), which are already checked by prometheus export code, to metrics that can be disabled/enabled from command line.
That also makes metricSetValue type redundant, as it can now be replaced with MetricSet use. Removing it requires moving String() & Set() methods, used for options texts and validating given metrics options, to factory.go.
toIncludedMetrics() one-line wrapper is also removed, as inlining container.AllMetrics.Difference() call makes the code intent more explicit, and is about same amount of code.