fix!: host-metrics system.cpu.utilization calculation fix
#1741
Conversation
pichlermarc
added
bug
Codecov Report
@@ Coverage Diff @@
## main #1741 +/- ##
==========================================
- Coverage 91.44% 91.05% -0.40%
==========================================
Files 143 133 -10
Lines 7273 6560 -713
Branches 1458 1308 -150
==========================================
- Hits 6651 5973 -678
+ Misses 622 587 -35
|
…a/opentelemetry-js-contrib into dluna/1718-fix-cpu-utilization
| * The first time will return 0 for % masurements since there is not enough | ||
| * data to calculate it |
There was a problem hiding this comment.
Wouldn't it be possible to load cpus at startup and calculate the load from then? It would miss the first few milliseconds, but would avoid having a 0 measurement on each program restart.
There was a problem hiding this comment.
it would make testing harder but it is possible. I'll give it a try
There was a problem hiding this comment.
Luckily the test is setup in a way that is not affected by this data preload. The changes are pushed :)
| ensureValue(metric, { state: 'user', cpu: '0' }, 30247.935978659552); | ||
| ensureValue(metric, { state: 'system', cpu: '0' }, 21071.23374458153); | ||
| ensureValue(metric, { state: 'idle', cpu: '0' }, 124998.56618872957); | ||
| ensureValue(metric, { state: 'user', cpu: '0' }, 0.7); |
There was a problem hiding this comment.
Is the value rounded or is this just a coincidence that they're all 1 significant figure?
There was a problem hiding this comment.
It's not a coincidence. The data is mocked with packages/opentelemetry-host-metrics/test/mocks/cpu.json.
david-luna
changed the title
system.cpu.utilization calculation fixsystem.cpu.utilization calculation fix
|
@legendecas @dyladan could you give another look please? |
| @@ -27,6 +27,11 @@ export enum METRIC_NAMES { | |||
| PROCESS_MEMORY_USAGE = 'process.memory.usage', | |||
| } | |||
|
|
|||
| export enum METRIC_ATTRIBUTES { | |||
There was a problem hiding this comment.
I've added this enum to properly report the attributes with their full name as set on the semantic conventions
|
Thank you for working on this. |
|
Thanks @legendecas :) I'll prepare another PR for |
…metry#1741) * tests: tune the fake clock for testing * tests: tune the values to expect * chore: lint autofix * chore: improve os.cpus mock * chore: revert implementation for process CPU usage * chore: rename var * chore: avoid having useless data upon 1st collection * chore: remove comments * chore: change system.cpu attribute names to match semantic conventions * chore: update test * chore: lint fix
Which problem is this PR solving?
this PR is fixing the calculation of
system.cpu.utilizationmetrics which is currently returning values that are out of the range [0,1] specified in semantic conventions. See #1718Short description of the changes
the calculation now is made based on the previous values returned from the
os.cpusNodeJS API and the time it was called. AlsoDate.nowstatic function is used to avoid unnecessary object creation (date objects) on each measurement.