Update CPU handler #651
Update CPU handler #651
Conversation
github-actions
bot
requested review from
breedx-splk,
jack-berg and
kittylyst
| private volatile double machineMinuteAverage = 0; | ||
| private static final int POLLING_INTERVAL_S = 1; | ||
| private int count = 0; | ||
| private double machineSum = 0; |
There was a problem hiding this comment.
Does JFR guarantee that events are always dispatched on the same thread? If not, then this should probably be volatile as well.
There was a problem hiding this comment.
Good question. The assumption is that the events are fired onto a SingleThreadExecutor but it is in theory possible that some could use a different model, although I would expect that the extra complexity would very much outweigh any possible throughput gain.
There was a problem hiding this comment.
I made it volatile and added synchronization
| } | ||
|
|
||
| @Override | ||
| public void accept(RecordedEvent ev) { | ||
| if (count % 60 == 0) { |
There was a problem hiding this comment.
I think this 60 literal also needs to take into consideration the polling interval, right? So it needs to be computed.
Like if the interval changes to 2s, then the 60 would need to be changed to 30.
There was a problem hiding this comment.
We have a hardcoded METRIC_NAME_MACHINE_MINUTE and the metric defined in spec is, per, https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/runtime-environment-metrics.md#jvm-metrics specifically a 1-minute metric. So, I think this is OK - it isn't supposed to represent an average over a general interval.
There was a problem hiding this comment.
I removed the unnecessary calculation. Now it always uses 60 second intervals.
There was a problem hiding this comment.
I think process.runtime.jvm.system.cpu.load_1m is intended to be a "load average", e.g. https://en.wikipedia.org/wiki/Load_(computing)#Unix-style_load_calculation and https://docs.oracle.com/javase/8/docs/api/java/lang/management/OperatingSystemMXBean.html#getSystemLoadAverage--, instead of an "average cpu utilization"
There was a problem hiding this comment.
ohh I see. I think that part of the semantic conventions was written with JMX in mind because JFR does not gather that information. A new event emitting that information will have to be created for this metric. Closing the PR.
| private volatile double machineMinuteAverage = 0; | ||
| private static final int POLLING_INTERVAL_S = 1; | ||
| private int count = 0; | ||
| private double machineSum = 0; |
There was a problem hiding this comment.
Good question. The assumption is that the events are fired onto a SingleThreadExecutor but it is in theory possible that some could use a different model, although I would expect that the extra complexity would very much outweigh any possible throughput gain.
| } | ||
|
|
||
| @Override | ||
| public void accept(RecordedEvent ev) { | ||
| if (count % 60 == 0) { |
There was a problem hiding this comment.
We have a hardcoded METRIC_NAME_MACHINE_MINUTE and the metric defined in spec is, per, https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/runtime-environment-metrics.md#jvm-metrics specifically a 1-minute metric. So, I think this is OK - it isn't supposed to represent an average over a general interval.
...reaming/src/test/java/io/opentelemetry/contrib/jfr/metrics/JfrOverallCPULoadHandlerTest.java
Outdated
Show resolved
Hide resolved
...g/src/main/java/io/opentelemetry/contrib/jfr/metrics/internal/cpu/OverallCPULoadHandler.java
Outdated
Show resolved
Hide resolved
| machineSum += machine; | ||
| machineMinuteQueue.add(machine); | ||
| // Compute new result | ||
| machineMinuteAverage = machineSum / machineMinuteQueue.size(); |
There was a problem hiding this comment.
Nice.
Would be good to somehow breakout the pieces of code involved in this so they can be unit tested.
There was a problem hiding this comment.
I added a commit that extracts the moving average code and adds a test for it. But I'm not sure it's an ideal solution because the method is now public.
|
Closed because |
Description:
This PR updates the existing CPU event handler to also implement the metric
process.runtime.jvm.system.cpu.load_1m.This was a part of a larger PR here.
jfr-streaming/src/main/java/io/opentelemetry/contrib/jfr/metrics/internal/Constants.javahas been kept the same as it was in the larger PR because it is used in all of the smaller constituent PRs. This will hopefully also avoid some conflicts as the PRs are sequentially merged. As a result, there are some constants that are defined, but not used in this PR.