-
Notifications
You must be signed in to change notification settings - Fork 618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add noiseDelta to telemetry test #1955
Conversation
|
||
params.MetricName = aws.String("MemoryUtilization") | ||
metrics, err = VerifyMetrics(cwclient, params, false) | ||
metrics, err = VerifyMetrics(cwclient, params, false, 0.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this one zero instead of statsNoiseDelta?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
golang won't let me overload a function and I didn't want to use variadic args (...).
The existing VerifyMetrics
signature uses the idleCluster
bool:
func VerifyMetrics(cwclient *cloudwatch.CloudWatch, params *cloudwatch.GetMetricStatisticsInput, idleCluster bool)
So now if idleCluster = true
, we need a corresponding noiseDelta float. There are no checks for idleCluster = false
so the 0.0 is effectively a nil placeholder wherever we have false
. If there's a better way of doing this, I'm open to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use statsNoiseDelta as a constant within the function? I don't think we gain any flexibility by passing it around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests failing, apart from that looks good. 🐑
This PR is created against the master branch, not dev...two secrets tests failed... |
Updated to point to dev. |
After re-running tests, checks are all green. See commit status (https://github.com/fierlion/amazon-ecs-agent/branches), not status above. |
Summary
The docker stats reported to CloudWatch have little fluctuations. Where we expect absolute 0.0% CPU usage now, the stats actually fluctuate. See testing done below >>
This unifies the delta that we use to account for noise across the entire telemetry test.
Implementation details
added a noise constant (5.0). Updated VerifyMetrics to take this same constant, so we can use it everywhere, and change it only at the const.
Testing
In my testing, I saw idle cloudwatch metrics for CPU reach anywhere between 0.0 and 2.3% max when I ran the telemetry task. This means that we will see failures if we query cloudwatch and expect an absolute 0.0% average for an idleCluster.
Description for the changelog
Add functional test fix
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.