Add a new daemon for stats monitoring
ClosedPublic

Authored by davidedmundson on Mar 27 2020, 10:53 AM.

Details

Summary

ksysguardd, whilst good, has a few problems

The code is a bit archaic, it relies on a polling API, which is overhead
for infrequently changed values or where setting up a monitor has a big
overhead.

It also moves the problem of translations into the daemon, allowing for
better extensibility without requiring client side changes.

The daemon is based around a typical OO model. Plugins have lists of
objects, those objects have properties using common Qt patterns. A
property also has various metadata.

For full compatibility ksgrd is wrapped and the plan is to land with the
bridge, then slowly land patches that use the new API natively.

An nvidia plugin is also added to show the API being used in another
format.

This is all consumed by the new API posted in D28141

Test Plan

Unit test
Used with the new library to create a new suite of applet (upcoming patch)
Used in a ported ksysguard

Diff Detail

Repository
R106 KSysguard
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 25821
Build 25839: arc lint + arc unit
Restricted Application added a project: Plasma. · View Herald TranscriptMar 27 2020, 10:53 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidedmundson requested review of this revision.Mar 27 2020, 10:53 AM
plugins/global/nvidia/nvidiaplugin.json
2

To address this, I don't use the json at all, as it's itself a factory class which provides metadata.

But I couldnt' get rid of it without error.

zzag added a subscriber: zzag.Mar 27 2020, 11:08 AM
zzag added inline comments.
plugins/global/nvidia/nvidia.cpp
3–4

Typo?

Drop object library, it was too cool for some cmake versions

Register bus name after loading plugins

mart added a subscriber: mart.Apr 21 2020, 9:07 AM
mart added inline comments.
kstats/ksysguard_iface.xml
23 ↗(On Diff #78977)

since this is array in/array out, could consistence of naming be improved?
either
sensorsIds/sensorsInfos
or
sensorIds/sensorInfos

kstats/ksysguarddaemon.cpp
58 ↗(On Diff #78977)

needs to be always polling every half a second? could it be somewhat adjustable (maybe depending on ac/on battery?)

86 ↗(On Diff #78977)

if (

kstats/test/main.cpp
1 ↗(On Diff #78977)

copyright?

57 ↗(On Diff #78977)

what's the commandline option missing?

libkstats/AggregateSensor.cpp
199 ↗(On Diff #78977)

if this timer is needed for compression, shouldn't be the same timer always restarted?
if i call singleshot 10 times, iirc that slot will be invoked delayed 10 times

plugins/global/ksgrd/ksgrdiface.cpp
185 ↗(On Diff #78977)

if () {
}

378 ↗(On Diff #78977)

maybe mapping on a config file?

ivan added a subscriber: ivan.Apr 21 2020, 9:16 AM
ivan added inline comments.
kstats/autotests/main.cpp
45 ↗(On Diff #78977)

setMax?

kstats/client.cpp
38 ↗(On Diff #78977)
56 ↗(On Diff #78977)

Maybe:

if(auto sensor = m_daemon->findSensor(sensorPath)) {
    ...
}
ivan added inline comments.Apr 21 2020, 9:25 AM
kstats/client.cpp
65–66 ↗(On Diff #78977)

... = sensor->info();

79–80 ↗(On Diff #78977)

if (auto sensor = ...)

kstats/ksysguarddaemon.cpp
74 ↗(On Diff #78977)

[=] is evil

111–113 ↗(On Diff #78977)

clang-format

161 ↗(On Diff #78977)

I'm not going to be bold enough to propose

if (const auto variant = ...; variant.isValid()) { }
172–173 ↗(On Diff #78977)

const int, const int

ahiemstra added inline comments.
kstats/client.cpp
45 ↗(On Diff #78977)

This should probably use qAsConst.

kstats/ksysguarddaemon.cpp
76 ↗(On Diff #78977)

Should we do some error logging here? Might help future plugin development.

93 ↗(On Diff #78977)
const auto containers = provider->containers();
for(auto container : containers) {
111 ↗(On Diff #78977)

qAsConst(m_containers)

112 ↗(On Diff #78977)

Like above, objects() should be in a const variable. (Also for sensors on the line below).

201 ↗(On Diff #78977)

qAsConst(m_clients)

libkstats/AggregateSensor.cpp
199 ↗(On Diff #78977)

It's guarded with "m_dataChangeQueued" which only gets reset after the timer triggers.

libkstats/SensorContainer.cpp
64 ↗(On Diff #78977)

Uh, so shouldn't we fix this?

libkstats/SensorContainer.h
56 ↗(On Diff #78977)

Maybe move this to private with friend class SensorObject? Though I don't really see why this is internal only. Also probably should rename it to addObject?

davidedmundson marked 23 inline comments as done.

all the changes

davidedmundson marked an inline comment as done.Apr 24 2020, 11:09 AM
davidedmundson added inline comments.
kstats/ksysguarddaemon.cpp
58 ↗(On Diff #78977)

Indeed, the current code is every 2s. I ramped it up to expose any issues.

I would quite like it to be somewhat container specific.
i.e disk storage doesn't change much, CPU is more volatile.

I'll change it, and then maybe we can revisit this topic once some more things are ported.

libkstats/SensorContainer.cpp
64 ↗(On Diff #78977)

Seems like a plan :)

libkstats/SensorContainer.h
56 ↗(On Diff #78977)

It was just to explicitly enforce the memory management combined with the tree management.

I'll go with private, then we can change if we find we need to

plugins/global/ksgrd/ksgrdiface.cpp
378 ↗(On Diff #78977)

Plan (with some pending code) is to just move these in natively into a more readable plugin

davidedmundson marked an inline comment as done.

Clear up plugin loading

mart accepted this revision.May 4 2020, 12:14 PM
This revision is now accepted and ready to land.May 4 2020, 12:14 PM
ngraham accepted this revision.May 4 2020, 2:19 PM
ahiemstra accepted this revision.May 6 2020, 2:21 PM

Let's do this.

This revision was automatically updated to reflect the committed changes.