Enable metrics for windows #1077
Conversation
richardpen
requested review from
samuelkarp,
aaithal,
petderek and
tehshwann
richardpen
force-pushed
the
metrics
branch
2 times, most recently
from
5776aed
to
dbb26b2
Compare
agent/stats/utils_windows.go
Outdated
| // dockerStatsToContainerStats returns a new object of the ContainerStats object from docker stats. | ||
| func dockerStatsToContainerStats(dockerStats *docker.Stats) (*ContainerStats, error) { | ||
| if numCores == uint64(0) { | ||
| seelog.Error("Invalid number of cpu cores") |
There was a problem hiding this comment.
Please add more context here: "Stats: invalid number of cpu cores in response"
agent/stats/utils_windows.go
Outdated
| func dockerStatsToContainerStats(dockerStats *docker.Stats) (*ContainerStats, error) { | ||
| if numCores == uint64(0) { | ||
| seelog.Error("Invalid number of cpu cores") | ||
| return nil, fmt.Errorf("Invalid number of cpu cores") |
There was a problem hiding this comment.
should start with a lower-case letter and conform to https://github.com/golang/go/wiki/CodeReviewComments#error-strings
agent/stats/utils_unix.go
Outdated
| // The length of PercpuUsage represents the number of cores in an instance. | ||
| if len(dockerStats.CPUStats.CPUUsage.PercpuUsage) == 0 { | ||
| seelog.Debug("Invalid container statistics reported, invalid stats payload from docker") | ||
| return nil, fmt.Errorf("Invalid container statistics reported") |
There was a problem hiding this comment.
please fix the error message here as well.
| ) | ||
|
|
||
| // dockerStatsToContainerStats returns a new object of the ContainerStats object from docker stats. | ||
| func dockerStatsToContainerStats(dockerStats *docker.Stats) (*ContainerStats, error) { |
There was a problem hiding this comment.
Can you add a unit test for this?
There was a problem hiding this comment.
it doesn't cover numCores block, right?
There was a problem hiding this comment.
yes, you're right, adding it.
richardpen
force-pushed
the
metrics
branch
2 times, most recently
from
3411046
to
abe50e7
Compare
|
|
||
| // dockerStatsToContainerStats returns a new object of the ContainerStats object from docker stats. | ||
| func dockerStatsToContainerStats(dockerStats *docker.Stats) (*ContainerStats, error) { | ||
| if numCores == uint64(0) { |
There was a problem hiding this comment.
theory it shouldn't happen, as it's acquired from runtime. But I don't know when will it happen.
There was a problem hiding this comment.
@sharanyad we should always remember that the runtime can send weird unexpected values to us (which has happened in the past). Since in this case, it'll lead to a panic, it's better to be protected
| return nil, fmt.Errorf("Invalid container statistics reported, no cpu core usage reported") | ||
| } | ||
|
|
||
| cpuUsage := dockerStats.CPUStats.CPUUsage.TotalUsage / numCores |
There was a problem hiding this comment.
Should we do a zero check for numCores here too?
| func dockerStatsToContainerStats(dockerStats *docker.Stats) (*ContainerStats, error) { | ||
| if numCores == uint64(0) { | ||
| seelog.Error("Invalid number of cpu cores acquired from the system") | ||
| return nil, fmt.Errorf("invalid number of cpu cores acquired from the system") |
There was a problem hiding this comment.
Can we still send memoryUsage and timestamp information since they don't require numCores ?
There was a problem hiding this comment.
I don't have a strong opinion here, but my suggestion is that we shouldn't send part of the metrics if something was wrong, not sending wrong metrics is better than sending wrong metrics I think.
agent/stats/utils_windows_test.go
Outdated
| @@ -0,0 +1,40 @@ | |||
| // +build windows,!integration | |||
| // Copyright 2014-2016 Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
| return nil, fmt.Errorf("invalid number of cpu cores acquired from the system") | ||
| } | ||
|
|
||
| cpuUsage := dockerStats.CPUStats.CPUUsage.TotalUsage / numCores |
There was a problem hiding this comment.
Can we verify that dividing by numCores is appropriate on Windows? From when I was playing around with it on a 4-core machine, docker stats showed 100% usage on Windows when using all 4 cores but showed 400% on Linux for the same workload.
There was a problem hiding this comment.
Does '100%' mean different things on each platform?
There was a problem hiding this comment.
@samuelkarp confirmed in that case, the metrics will be the same as docker stats, it will show 100%.
There was a problem hiding this comment.
So do we need to divide by numCores or should we remove that?
There was a problem hiding this comment.
we should keep it, also this code indicates the same.
There was a problem hiding this comment.
As discussed offline: I don't have an objection to merging this as long as we close out on the right math before we release anything.
richardpen
force-pushed
the
metrics
branch
2 times, most recently
from
3882502
to
317a3d8
Compare
Summary
Fix #608
Implementation details
Change the default configuration of metrics on windows to be enabled by default.
Testing
make release)go build -out amazon-ecs-agent.exe ./agent)make test) passgo test -timeout=25s ./agent/...) passmake run-integ-tests) pass.\scripts\run-integ-tests.ps1) passmake run-functional-tests) pass.\scripts\run-functional-tests.ps1) passNew tests cover the changes:
yes
Description for the changelog
Licensing
This contribution is under the terms of the Apache 2.0 License:
yes