allow ignoreMetrics in new manager #1159
Conversation
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
|
cc @vishh |
|
LGTM |
| @@ -184,6 +184,10 @@ func New(memoryCache *memory.InMemoryCache, sysfs sysfs.SysFs, maxHousekeepingIn | |||
| inHostNamespace = true | |||
| } | |||
|
|
|||
| if ignoreMetricsSet == nil { | |||
| ignoreMetricsSet = ignoreMetrics.MetricSet | |||
There was a problem hiding this comment.
I'd prefer that this was explicitly passed in from the main module rather than implicitly set here.
There was a problem hiding this comment.
Good idea - so move the flag defined in this file to cadvisor.go in the project root and pass it down from there?
sjenning
force-pushed
the
manager-accept-ignoremetrics
branch
from
776e615
to
5156f5d
Compare
|
@timstclair updated |
| if err == nil { | ||
| t.Fatalf("Expected nil manager to return error") | ||
| } | ||
| } | ||
|
|
||
| func TestTcpMetricsAreDisabledByDefault(t *testing.T) { |
There was a problem hiding this comment.
nit: Can we keep this test around?
|
Just one nit. Otherwise LGTM |
| func main() { | ||
| defer glog.Flush() | ||
| // Tcp metrics are ignored by default. | ||
| flag.Var(&ignoreMetrics, "disable_metrics", "comma-separated list of metrics to be disabled. Options are `disk`, `network`, `tcp`. Note: tcp is disabled by default due to high CPU usage.") | ||
| flag.Set("disable_metrics", "tcp") |
There was a problem hiding this comment.
Rather than calling flag.Set, just add TCP to the default metrics on line 58
sjenning
force-pushed
the
manager-accept-ignoremetrics
branch
from
5156f5d
to
574820f
Compare
|
@vishh @timothysc updated |
| func main() { | ||
| defer glog.Flush() | ||
| // Tcp metrics are ignored by default. |
There was a problem hiding this comment.
(nit) move the comment too.
sjenning
force-pushed
the
manager-accept-ignoremetrics
branch
from
574820f
to
00934be
Compare
|
Thanks. BTW - I'm @timstclair not @timothysc - there are 2 of us :) |
|
LGTM. |
sjenning
force-pushed
the
manager-accept-ignoremetrics
branch
from
00934be
to
8aa6164
Compare
|
@timstclair undetected name collision, my bad :) updated |
|
LGTM |
allow ignoreMetrics in new manager
kubelet would like to be able to set the ignore metrics but currently this is only supported when invoking cadvisor from the command line.
This allows the caller of manger.New() to pass in a set of metrics to ignore, falling back to the defaults if none are provided.
cc @ncdc @derekwaynecarr