[collectd 6] cpu plugin: Align metrics with OpenTelemetry recommendations. #4216
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
octo
force-pushed
the
6/cpu
branch
2 times, most recently
from
000d934
to
0d3380e
Compare
|
Solaris builds show quite a few warnings (not completely overlapping):
And the other of them fails to: |
Thanks for pointing this out. Fixed. |
eero-t
requested changes
Jan 19, 2024
There was a problem hiding this comment.
Bug in config_keys[] needs to be fixed.
Also, does CPU plugin need to handle run-time CPU hot-plugging, or why states is 1D dynamically resized [cpu*states] array [1], instead init-allocated 2D [cpu][state] array?
[1] I'd rather avoid things like cpu_num = u->states_num / STATE_MAX and active_index = (cpu * STATE_MAX) + STATE_ACTIVE if hot-plug support is not needed, as they complicate code significantly.
|
Thanks for the review @eero-t. I think I have addressed all your points. |
eero-t
reviewed
Jan 22, 2024
eero-t
approved these changes
Jan 22, 2024
* Add metric description and unit. * Update label names (e.g. "cpu" → "system.cpu.logical_number") * Divide rates by number of CPUs, so that the sum of all rates equals to 1. (Previously the sum of all rates was equal to the number of logical CPUs) * Remove the "cpu=total" label when aggregating CPUs.
* The options `ReportUsage`, `ReportUtilization`, and `ReportNumCpu` control
which metrics are emitted on a high level. Other options no longer influence
*what* is being collected. This also allows to report usage and utilization
metrics simultaneously.
* The documentation has been updated to reflect that the plugin no longer
emits a percentage, but a ratio for utilization metrics.
This one is done in a second aggregation loop, because we require a CPU-level aggregate rate to be available to properly scale the counter.
…o` and `usage_count`.
…ount`. The functionality is tested in the test cases for `usage_ratio` and `usage_count` and there is no need to test these separately. The opposite: the rest of the CPU plugin only uses `usage_ratio` and `usage_count`, so testing the global variants leaks abstraction.
This way the use of the field is much easier to understand when reading the code.
This will just work transparently.
Co-authored-by: Eero Tamminen <eero.t.tamminen@intel.com>
Also sort `config_keys`.
Co-authored-by: Eero Tamminen <eero.t.tamminen@intel.com>
Fixed. Thanks @eero-t! |
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.
cpu=totallabel when aggregating CPUs.usage_tand unit tests ensure that it functions as expected.ChangeLog: CPU plugin: The metric schema has been aligned with OpenTelemetry semantic conventions.