Skip to content

Add cgroup cpu/mem/disk usage metrics#16472

Merged
suneet-s merged 15 commits intoapache:masterfrom
ac9817:add-more-cgroup-metrics
May 29, 2024
Merged

Add cgroup cpu/mem/disk usage metrics#16472
suneet-s merged 15 commits intoapache:masterfrom
ac9817:add-more-cgroup-metrics

Conversation

@ac9817
Copy link
Copy Markdown
Contributor

@ac9817 ac9817 commented May 20, 2024 •
edited
Loading

Description

This PRs expands the existing metrics reported by the Cgroup monitors. Specifically adds the usage for cpu, memory and disk.

Release note

Introduces new usage metrics for cpu and memory cgroups

Key changed/added classes in this PR
  • CgroupCpuMonitor.java
  • CgroupMemoryMonitor.java
  • CgroupDiskMonitor.java

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

arunramani
arunramani reviewed May 20, 2024
View reviewed changes
Comment thread processing/src/main/java/org/apache/druid/java/util/metrics/CgroupCpuMonitor.java Outdated
arunramani
arunramani reviewed May 20, 2024
View reviewed changes
Comment thread processing/src/main/java/org/apache/druid/java/util/metrics/cgroups/CgroupUtil.java Outdated
arunramani
arunramani reviewed May 20, 2024
View reviewed changes
Comment thread processing/src/main/java/org/apache/druid/java/util/metrics/cgroups/CgroupUtil.java Outdated
arunramani
arunramani reviewed May 20, 2024
View reviewed changes
Comment thread processing/src/main/java/org/apache/druid/java/util/metrics/cgroups/CgroupUtil.java Outdated
arunramani
arunramani reviewed May 20, 2024
View reviewed changes
Comment thread processing/src/main/java/org/apache/druid/java/util/metrics/cgroups/Cpu.java Outdated
arunramani
arunramani reviewed May 20, 2024
View reviewed changes
Comment thread docs/operations/metrics.md Outdated
@arunramani
Copy link
Copy Markdown
Contributor

arunramani commented May 20, 2024

Added a few comments but overall, the change looks good to me. Were you able to validate that you were getting the right values for these? One way would be to test your assumptions in Docker and see if your numbers match with what Docker sees.

suneet-s
suneet-s reviewed May 20, 2024
View reviewed changes
Comment thread docs/operations/metrics.md Outdated
@ac9817
Copy link
Copy Markdown
Contributor Author

ac9817 commented May 20, 2024

Were you able to validate that you were getting the right values for these? One way would be to test your assumptions in Docker and see if your numbers match with what Docker sees.

Yeah tested in docker to see the formats match. But for cpu usage percentage, where do you want me to confirm from ?

@arunramani
Copy link
Copy Markdown
Contributor

arunramani commented May 20, 2024

Were you able to validate that you were getting the right values for these? One way would be to test your assumptions in Docker and see if your numbers match with what Docker sees.

Yeah tested in docker to see the formats match. But for cpu usage percentage, where do you want me to confirm from ?

The Docker desktop UI displays CPU usage for a container, but I'm not 100% sure if even Docker does it right. But if it does, it'll give you additional confidence.

@ac9817
Copy link
Copy Markdown
Contributor Author

ac9817 commented May 21, 2024

The Docker desktop UI displays CPU usage for a container, but I'm not 100% sure if even Docker does it right. But if it does, it'll give you additional confidence.

I have tested with Datadog, even though the %s aren't exactly matching up, the spikes are matching. So I think it is safe to assume working fine.

gianm
gianm reviewed May 21, 2024
View reviewed changes
Comment thread docs/operations/metrics.md Outdated
@ac9817 ac9817 force-pushed the add-more-cgroup-metrics branch from 47cca7f to ba6fea9 Compare May 28, 2024 06:30
@ac9817 ac9817 requested review from gianm and suneet-s May 28, 2024 06:30
github-advanced-security[bot]
github-advanced-security AI found potential problems May 28, 2024
View reviewed changes
Comment thread processing/src/test/java/org/apache/druid/java/util/metrics/CgroupDiskMonitorTest.java Fixed
Comment thread processing/src/test/java/org/apache/druid/java/util/metrics/cgroups/DiskTest.java Fixed
Comment thread processing/src/main/java/org/apache/druid/java/util/metrics/CgroupCpuMonitor.java Fixed
Comment thread processing/src/main/java/org/apache/druid/java/util/metrics/CgroupCpuMonitor.java Fixed
Adithya Chakilam added 2 commits May 28, 2024 02:28
@ac9817 ac9817 changed the title Add cgroup cpu/mem usage metrics Add cgroup cpu/mem/disk usage metrics May 28, 2024
Adithya Chakilam added 2 commits May 28, 2024 18:19
github-advanced-security[bot]
github-advanced-security AI found potential problems May 29, 2024
View reviewed changes
Comment thread processing/src/main/java/org/apache/druid/java/util/metrics/CgroupCpuMonitor.java Fixed
Comment thread processing/src/main/java/org/apache/druid/java/util/metrics/CgroupCpuMonitor.java Fixed
github-advanced-security[bot]
github-advanced-security AI found potential problems May 29, 2024
View reviewed changes
Comment thread processing/src/main/java/org/apache/druid/java/util/metrics/CgroupCpuMonitor.java
this.cgroupDiscoverer = cgroupDiscoverer;
this.dimensions = dimensions;
try {
Process p = new ProcessBuilder("getconf", "CLK_TCK").start();

Check warning

Code scanning / CodeQL

Executing a command with a relative path

Command with a relative path 'getconf' is executed.
suneet-s
suneet-s approved these changes May 29, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@suneet-s suneet-s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @adithyachakilam !

@suneet-s suneet-s merged commit a9044ac into apache:master May 29, 2024
@kfaraz kfaraz added this to the 31.0.0 milestone Oct 4, 2024
@kfaraz kfaraz mentioned this pull request Oct 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@suneet-s suneet-s suneet-s approved these changes

@gianm gianm Awaiting requested review from gianm

+1 more reviewer

@arunramani arunramani arunramani left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area - Documentation Area - Metrics/Event Emitting

Projects

None yet

Milestone

31.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@ac9817 @arunramani @gianm @suneet-s @github-advanced-security @kfaraz