cgroup monitors: Add mem/disk/cpu usage metrics for V2 #16905
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for (int i = 1; i < parts.length; i++) { | ||
| String[] keyValue = parts[i].split("="); | ||
| if (keyValue.length == 2) { | ||
| stats.put(keyValue[0], Long.parseLong(keyValue[1])); |
Check notice
Code scanning / CodeQL
Missing catch of NumberFormatException Note
There was a problem hiding this comment.
Similar comment as the Cpu Monitor - if we use Longs.parseLong we will need to handle nulls when emitting the metrics
There was a problem hiding this comment.
There shouldn't be any nulls, it should either be a valid long value that should be obtained here or a exception thrown, catched at line 113. Am I missing anything ?
There was a problem hiding this comment.
I think I got confused here because it looks like if there is an issue with parsing metrics for any disk, no disk metrics are reported and that is perhaps what hte code scanning bot is complaining about.
| public class CgroupV2CpuMonitorTest | ||
| { | ||
| @Rule | ||
| public ExpectedException expectedException = ExpectedException.none(); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
| public class CgroupV2DiskMonitorTest | ||
| { | ||
| @Rule | ||
| public ExpectedException expectedException = ExpectedException.none(); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
| public class CgroupV2MemoryMonitorTest | ||
| { | ||
| @Rule | ||
| public ExpectedException expectedException = ExpectedException.none(); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note test
|
The changes looks reasonable to me. My one ask would be to add some javadoc comments around what proc file is being parsed and what the expected format is supposed to be. |
|
Added the comments for io.stat and cpu.stat, memory is same as v1. Also, test cases implies the format of the files |
processing/src/main/java/org/apache/druid/java/util/metrics/CgroupV2CpuMonitor.java
Show resolved
Hide resolved
| private final long userUsec; | ||
| private final long systemUsec; | ||
|
|
||
| public Snapshot(long usageUsec, long userUsec, long systemUsec) |
There was a problem hiding this comment.
Should the constructor accept @Nullable Long objects instead so that we can distinguish between parse exceptions and 0s when reading the file?
There was a problem hiding this comment.
We basically skip emitting metrics on parse exceptions and would not end up creating this Snapshot object at all.
| for (int i = 1; i < parts.length; i++) { | ||
| String[] keyValue = parts[i].split("="); | ||
| if (keyValue.length == 2) { | ||
| stats.put(keyValue[0], Long.parseLong(keyValue[1])); |
There was a problem hiding this comment.
Similar comment as the Cpu Monitor - if we use Longs.parseLong we will need to handle nulls when emitting the metrics
processing/src/main/java/org/apache/druid/java/util/metrics/cgroups/Cpu.java
Show resolved
Hide resolved
Description
This PR add the support for cgroup monitors for v2. For now, only the usage of cpu/disk/memory are being supported.
Release note
New monitors for cgroups v2 are now available to track usage of cpu/disk/memory.
Key changed/added classes in this PR
CgroupV2CpuMonitorCgroupV2DiskMonitorCgroupV2MemoryMonitorThis PR has: