-
Notifications
You must be signed in to change notification settings - Fork 327
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
SAMZA-2762: new cpu usage metric which counts child processes usage #1636
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
samza-core/src/main/java/org/apache/samza/container/host/OshiBasedStatisticsGetter.java
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/container/host/OshiBasedStatisticsGetter.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/container/host/OshiBasedStatisticsGetter.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/container/host/PosixCommandBasedStatisticsGetter.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/container/host/OshiBasedStatisticsGetter.java
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/container/host/ProcessCPUStatistics.java
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/container/host/OshiBasedStatisticsGetter.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/container/host/StatisticsMonitorImpl.java
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/container/host/SystemStatistics.java
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/container/host/SystemStatisticsGetter.java
Show resolved
Hide resolved
mynameborat
reviewed
Nov 4, 2022
samza-core/src/main/java/org/apache/samza/container/host/DefaultSystemStatisticsGetter.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/container/host/OshiBasedStatisticsGetter.java
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/container/host/ProcessCPUStatistics.java
Show resolved
Hide resolved
samza-core/src/main/java/org/apache/samza/container/host/PosixCommandBasedStatisticsGetter.java
Outdated
Show resolved
Hide resolved
samza-core/src/main/scala/org/apache/samza/container/SamzaContainerMetrics.scala
Show resolved
Hide resolved
Signed-off-by: Alan Zhang <shuai.xyz@gmail.com>
Signed-off-by: Alan Zhang <shuai.xyz@gmail.com>
Signed-off-by: Alan Zhang <shuai.xyz@gmail.com>
Signed-off-by: Alan Zhang <shuai.xyz@gmail.com>
Signed-off-by: Alan Zhang <shuai.xyz@gmail.com>
Looks good to me mostly. Can you close out the conversations that have been resolved and respond to the comments that are pending. Also, fix the latest checks so that I can merge them. |
Signed-off-by: Alan Zhang <shuai.xyz@gmail.com>
@mynameborat I have fixed the check issue(checksytle issue) and resolved all the comments. Can you please check? Thanks. |
ehoner
pushed a commit
to ehoner/samza
that referenced
this pull request
Apr 11, 2023
…pache#1636) Symptom We have observed that some use cases used quasar(TensorFlow framework) to do model inference and this framework spawn child processes(non-JVM) to run TensorFlow serving. These child processes were using high CPU usage(200%) however their CPU usage can't be captured by the existing CPU usage metric process-cpu-usage Cause The existing metric process-cpu-usage metric was designed for capturing the CPU usage for the JVM process only, it can't count the child processes(especially for non-JVM processes) usage. Changes Reply on oshi framwork to capture the CPU usage for the JVM process and all its child processes, and create a new metric to display the total CPU usage. The CPU usage percentage is calculated based on top of the logical CPU count on the system API Changes Added a new metric total-process-cpu-usage in SamzaContainerMetrics which is similar with how we provided physical-memory-mb metric
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Symptom
We have observed that some use cases used quasar(TensorFlow framework) to do model inference and this framework spawn child processes(non-JVM) to run TensorFlow serving. These child processes were using high CPU usage(200%) however their CPU usage can't be captured by the existing CPU usage metric
process-cpu-usage
Cause
The existing metric
process-cpu-usage
metric was designed for capturing the CPU usage for the JVM process only, it can't count the child processes(especially for non-JVM processes) usage.Changes
API Changes
total-process-cpu-usage
inSamzaContainerMetrics
which is similar with how we providedphysical-memory-mb
metricTests
samza-hello-samza
and verify the metric data points