alnzng commented on code in PR #1636:
URL: https://github.com/apache/samza/pull/1636#discussion_r1014528695
##########
samza-core/src/main/scala/org/apache/samza/container/SamzaContainerMetrics.scala:
##########
@@ -49,6 +49,7 @@ class SamzaContainerMetrics(
val executorWorkFactor = newGauge("executor-work-factor", 1.0)
val physicalMemoryMb = newGauge("physical-memory-mb", 0.0F)
val physicalMemoryUtilization = newGauge("physical-memory-utilization", 0.0F)
+ val totalProcessCpuUsage = newGauge("total-process-cpu-usage", 0.0)
Review Comment:
Prefer to create a new metric(e.g. total-process-cpu-usage) to represent the
cpu usage for the Samza container’s JVM process and all its child/descendant
processes because of
- I view it as a non-backward compatible change if let the existing
“process-cpu-usage” metric to include child/descendant processes especially for
non-JVM processes.
- Samza official doc clearly states that the `process-cpu-usage` returns
the “recent cpu usage” for the Java Virtual Machine process:
https://samza.apache.org/learn/documentation/1.6.0/operations/monitoring.html
- Having a new metric could be consistent with how Samza exposes
physical-memory-mb . This memory related metric which includes child process is
available for Samza container only, the AM doesn’t have it.
- Implementation/Maintainance complexity. If keep using the existing
`process-cpu-usage` we will have to deprecate the JVMMetrics implementation and
add a new hook into AM to expose the metric.
Good point on the doc, let me update the document.
##########
samza-core/src/main/scala/org/apache/samza/container/SamzaContainerMetrics.scala:
##########
@@ -49,6 +49,7 @@ class SamzaContainerMetrics(
val executorWorkFactor = newGauge("executor-work-factor", 1.0)
val physicalMemoryMb = newGauge("physical-memory-mb", 0.0F)
val physicalMemoryUtilization = newGauge("physical-memory-utilization", 0.0F)
+ val totalProcessCpuUsage = newGauge("total-process-cpu-usage", 0.0)
Review Comment:
Prefer to create a new metric(e.g. total-process-cpu-usage) to represent the
cpu usage for the Samza container’s JVM process and all its child/descendant
processes because of
- I view it as a non-backward compatible change if let the existing
“process-cpu-usage” metric to include child/descendant processes especially for
non-JVM processes.
- Samza official doc clearly states that the `process-cpu-usage` returns
the “recent cpu usage” for the Java Virtual Machine process:
https://samza.apache.org/learn/documentation/1.6.0/operations/monitoring.html
- Having a new metric could be consistent with how Samza exposes
`physical-memory-mb`. This memory related metric which includes child process
is available for Samza container only, the AM doesn’t have it.
- Implementation/Maintainance complexity. If keep using the existing
`process-cpu-usage` we will have to deprecate the JVMMetrics implementation and
add a new hook into AM to expose the metric.
Good point on the doc, let me update the document.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@samza.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
