-
Notifications
You must be signed in to change notification settings - Fork 78
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
Use MemAvailable when it's available on Linux 3.14+ kernel #71
Use MemAvailable when it's available on Linux 3.14+ kernel #71
Conversation
a18c1f1
to
8c7ccf2
Compare
sigar_linux_common.go
Outdated
self.ActualFree = self.Free + kern | ||
} | ||
self.Used = self.Total - self.ActualFree | ||
self.ActualUsed = self.Used |
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.
Leave actual memory usage
like this for now, equal with the memory usage.
d25d817
to
9cc7566
Compare
sigar_linux_common.go
Outdated
kern := buffers + cached | ||
self.ActualFree = self.Free + kern | ||
self.ActualUsed = self.Used - kern | ||
if self.ActualFree > 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.
You should be able to unit test this pretty easily by simulating the contents of the /proc/meminfo
file.
With the current data structure it might not be possible to detect the difference between MemAvailable: 0 kB
and MemAvailable
not present. You don't want to fallback to the secondary calculation when MemAvailable: 0 kB
so you might need to refactor.
if self.ActualFree > 0.0 {
doesn't look right?
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.
That's a good point. I would need to refactor the code to be able to distinguish between MemAvailable: 0kB
and MemAvailable
not present.
3b1eb28
to
61712b4
Compare
meminfoContents := ` | ||
MemTotal: 500184 kB | ||
MemFree: 31360 kB | ||
MemAvailable: 414168 kB |
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.
Could you please add a test case for when MemAvailable: 0 kB
.
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.
Sure.
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.
Done
189cd1c
to
9b218a1
Compare
We need a CHANGELOG entry too. |
88d2e9e
to
34de5a6
Compare
@andrewkroh Ready for the final review. |
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.
LGTM. Just need to move the changelog entry.
CHANGELOG.md
Outdated
@@ -33,6 +33,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). | |||
privileges in a process token. | |||
- Added utility code for interfacing with linux NETLINK_INET_DIAG. #60 | |||
- Added `ProcEnv` for getting a process's environment variables. #61 | |||
- Read `MemAvailable` value for kernel 3.14+ |
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.
This should be up in the "unreleased" section.
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.
Done, thanks for noticing it.
57112e3
to
c071f65
Compare
Update deps to include the latest changes from the elastic/gosigar#71 which includes a change to use `MemAvailable` on Linux. This closes #4202.
Use MemAvailable when it's available on Linux 3.14+ kernel, otherwise calculate it as free memory plus the caches and buffers.
(Fixes elastic/beats#4202)