[receiver/dockerstats] Fix memory usage/percent calculation #21166
Conversation
| // See https://github.com/moby/moby/issues/40727 for the background. | ||
| func calculateMemUsageNoCache(memoryStats *dtypes.MemoryStats) uint64 { | ||
| // cgroup v1 | ||
| if v, isCgroup1 := memoryStats.Stats["total_inactive_file"]; isCgroup1 && v < memoryStats.Usage { |
There was a problem hiding this comment.
So is the logic that we know it's cgroup v1 if total_inactive_file exists? And if it doesn't then we know it's v2?
There was a problem hiding this comment.
yes total_inactive_file is not present with cgroups v2
| func calculateMemoryPercent(memoryStats *dtypes.MemoryStats) float64 { | ||
| if float64(memoryStats.Limit) == 0 { | ||
| return 0 | ||
| // calculateMemUsageNoCache calculate memory usage of the container. |
There was a problem hiding this comment.
Since you've taken this function from the Docker CLI code, which is under the Apache license, it will need a copyright notice similar to calculateCPUPercent().
There was a problem hiding this comment.
I have updated the notice that is on top of calculateCPUPercent which is copied from the same place to avoid having this notice multiple times, perhaps I can move all these three functions calculateCPUPercent, calculateMemUsageNoCache, and calculateMemoryPercent to a single file with one notice in the top WDYT?
There was a problem hiding this comment.
done in 6f3b680
| @@ -562,7 +562,7 @@ resourceMetrics: | |||
| sum: | |||
| aggregationTemporality: 2 | |||
| dataPoints: | |||
| - asInt: "352256" | |||
| - asInt: "425984" | |||
There was a problem hiding this comment.
The description of this metric is now a bit misleading, can you change that in metadata.yml?
It can probably just be "Total memory usage of the container".
There was a problem hiding this comment.
Perhaps replacing the total cache ?:
Memory usage of the container. This excludes the inactive file memory.
Or just mentioning that excludes "cache" like Docker CLI docs does in the note?
Memory usage of the container. This excludes the cache.
WDYT
There was a problem hiding this comment.
Either one works for me
There was a problem hiding this comment.
done in a756251
|
Hi @mx-psi - this has had codeowner approval for a couple of weeks now. It needs an approver or maintainer to look at it for it to get through. |
Woops, totally slipped through the cracks, thanks for the ping! It looks good to me so we can merge :) |
Description:
This PR implements the following proposals:
container.memory.usage.total: The proposal is to support both versions of cgroups and use the same method of the docker cli is using which represents better usage for some use-cases according to this discussion.container.memory.percent: Similar proposal to use the same method of the docker cli, based on the total usage previously calculated.Link to tracking Issue:
#21097
Testing:
I extended the current metrics tests.
Output from
docker statsof the added test:Documentation: