-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[systeminfo] Add CPU load channel, update dependencies #13292
Conversation
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -295,6 +296,14 @@ | |||
<config-description-ref uri="channel-type:systeminfo:highpriority_process"/> | |||
</channel-type> | |||
|
|||
<channel-type id="cpuLoad"> | |||
<item-type>Number:Dimensionless</item-type> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are setting Number:Dimensionless
but you are building a DecimalType state to update the channel state.
So Number
looks sufficient?
case CHANNEL_CPU_LOAD: | ||
DecimalType load = systeminfo.getSystemCpuLoad(); | ||
if (load != null) { | ||
load = new DecimalType(load.doubleValue() * 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using PercentType
which is the appropriate state for a percentage state value ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lolodomo I think this would be the right thing to do.
However, I did the same thing as for the existing usedHeapPercent channel, which was the last channel added and uses Number:Dimensionless with an underlying DecimalType.
But there are also a number of older channels that represent a percentage, with just Number as the type.
Converting them all to PercentType and Number:Dimensionless would be a breaking change. Adding yet another different combination doesn't feel right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lolodomo What is your view on this?
@@ -104,6 +104,7 @@ The binding introduces the following channels: | |||
|
|||
| Channel ID | Channel Description | Supported item type | Default priority | Advanced | | |||
|--------------------|------------------------------------------------------------------|---------------------|------------------|----------| | |||
| load | CPU Load in % | Number:Dimensionless| High | False | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my other comments, I am not sure that Number:Dimensionless
is more appropriate than Number
.
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I am not sure if the changed item type from Number to Number:Dimensionless for the channel type load_process is or not a breaking change. |
@lolodomo Thank you.
I think it is backward compatible, so nt a breaking change. I tested and was able to link a plain Number item to a Number:Dimensionless channel. |
* Add CPU load channel, update dependencies * use PercentType, correct process CPU load * Add and fix test Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
* Add CPU load channel, update dependencies * use PercentType, correct process CPU load * Add and fix test Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
* Add CPU load channel, update dependencies * use PercentType, correct process CPU load * Add and fix test Signed-off-by: Mark Herwege <mark.herwege@telenet.be> Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
* Add CPU load channel, update dependencies * use PercentType, correct process CPU load * Add and fix test Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
* Add CPU load channel, update dependencies * use PercentType, correct process CPU load * Add and fix test Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
* Add CPU load channel, update dependencies * use PercentType, correct process CPU load * Add and fix test Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Mark Herwege mark.herwege@telenet.be
The systeminfo binding currently provides average CPU load (expressed as average number of jobs queued) over 1, 5 and 15 minute intervals with respective channels. These measures are not available on Windows. There used to be a % CPU load channel, which was removed a while ago as the underlying oshi library changed.
It is still possible to get dynamic CPU load from current versions of the oshi library and have that available on all platforms (including Windows). That is implemented in this pull request.
This PR also updates the dependency to the latest version of the oshi libary (6.2.2). This version should also detect and support Apple M2 chips (not tested).