Skip to content
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

exclude cache value from container memory utilization metric #582

Merged
merged 1 commit into from
Dec 7, 2016

Conversation

emilymcafee
Copy link

@emilymcafee emilymcafee commented Nov 2, 2016

Summary

Closes #280 by excluding cache usage from memory utilization stat.

Implementation details

Subtract MemoryStats.Stats.Cache from MemoryStats.Usage and use the result as the memory stat.

Testing

I have not been able to get the test suite to run successfully on master/dev or on the fork. I'm new to go, so guessing this is an issue with my local configuration 😅 A few things that I've been able to test successfully:

  • The unit test for the affected file.
  • Build an image from the fork + override the agent on a running container instance, confirm via Docker pseudo-files & CloudWatch that the actual usage metric is reported instead of the metric that includes the cache.

I'm happy to keep trying to get the suite to run if this is a prerequisite for a PR, but would really appreciate some pointers on how to get tests running in a fresh environment (any gotchas with versioning/dependencies?)

  • Builds (make release)
  • Unit tests (make short-test) pass
  • Integration tests (make test and make run-integ-tests) pass
  • Functional tests (make run-functional-tests) pass

New tests cover the changes: Yes

Description for the changelog

Fix memory metric to exclude cache value.

Licensing

This contribution is under the terms of the Apache 2.0 License: Yes

@theotherian
Copy link

Thanks for working on this! This issue has been causing our dev organization some headaches; looking forward to seeing it fixed!

@aaithal
Copy link
Contributor

aaithal commented Dec 7, 2016

@emilymcafee Thanks a lot for this PR. As there have been no concerns raised in #280 for this, I'm merging this as everything seems to be in order as far as tests are concerned.

@aaithal aaithal merged commit f2019ae into aws:dev Dec 7, 2016
@emilymcafee
Copy link
Author

\o/ thanks @aaithal !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants