[Instrumentation.Process] Added the metrics for CpuTime and CpuUtilization. #625
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #625 +/- ##
==========================================
+ Coverage 77.76% 77.86% +0.10%
==========================================
Files 170 170
Lines 5163 5201 +38
==========================================
+ Hits 4015 4050 +35
- Misses 1148 1151 +3
|
utpilla
added
the
comp:instrumentation.process
|
|
||
| private class InstrumentsValues | ||
| { | ||
| private const int TicksPerSecond = 10 * 7; |
There was a problem hiding this comment.
This doesn't look correct to me.
https://docs.microsoft.com/en-us/dotnet/api/system.timespan.tickspersecond?view=net-6.0
| private double? cpuTime; | ||
| private double? cpuUtlization; | ||
|
|
||
| private DateTime lastCollectionTimestamp = DateTime.Now; |
There was a problem hiding this comment.
Use DateTime.UtcNow instead of DateTime.Now. It is much faster.
There was a problem hiding this comment.
In addition, these are not monotonic, I think for duration we should use something that measures elapsed time rather than wall clock.
| var currentCpuTime = this.currentProcess.TotalProcessorTime.Ticks * TicksPerSecond; | ||
| var deltaInCpuTime = currentCpuTime - this.lastCollectionCpuTime; | ||
|
|
||
| this.lastCollectionTimestamp = DateTime.Now; |
| (this.memoryUsage, this.virtualMemoryUsage, this.cpuTime, this.cpuUtlization) = this.UpdateValues(); | ||
| } | ||
|
|
||
| var elapsedTime = (DateTime.Now - this.lastCollectionTimestamp).Ticks * TicksPerSecond; |
There was a problem hiding this comment.
To convert ticks to seconds, you need to divide the ticks by the TicksPerSecond.
There was a problem hiding this comment.
right, this is embarrassing, thanks for pointing it out.
| $"process.cpu.time", | ||
| () => values.GetCpuTime(), | ||
| unit: "s", | ||
| description: "The amount of time that the current process has actively used a CPU to perform computations."); |
There was a problem hiding this comment.
would be good to specify the measurement time unit in the description
| $"process.cpu.utilization", | ||
| () => values.GetCpuUtilization(), | ||
| unit: "s", | ||
| description: "Difference in process.cpu.time since the last collection of observerble instruments from the MetricReader, divided by the elapsed time multiply by the number of CPUs available to the current process."); |
There was a problem hiding this comment.
CPU utilization percentage since last collection would be better?
The unit should be a percentage here?
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Changes
Working towards: #447
Background: #335
Following OTel spec.
For the
process.cpu.timemetric, Process.TotalProcessorTime was used to get the value.For the
process.cpu.utilizationmetric, it was defined as the "difference in process.cpu.time since the last measurement, divided by the elapsed time and number of CPUs available to the process" in the spec.In this PR, the last measurement was defined as the last time
GetCpuUtilization()callback function was invoked.At the collection phase of the metricReader of the Exporter,
GetCpuUtilization()will be triggered.The
lastCollectionTimestampandlastCollectionCpuTimewill be updated before the current CPU utilization result was returned.The unit defined in the spec for the above two mentioning CPU metrics are "s" (second). In this PR, the values were being calculated by converting TimeSpan.Ticks
to seconds for more granularity.
Please note that currently, broken down by different states , i.e.
user,system, orwaitfor CPU related metrics is not implemented in this PR because of the following reasons.waitstate from the existing Process releated APIs from .net.The plan is to release an alpha version of the
OpenTelemetry.Instrumentation.Processpackage and collect customer feedback. And based on the feedback, we decide whether the division by states feature should be implemented for CPU-related metrics or not.