-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
cockpit: Implement PCP metrics channel in the Python bridge #20049
base: main
Are you sure you want to change the base?
Conversation
ed025f6
to
11c3806
Compare
2821ebd
to
920768d
Compare
920768d
to
cd74f24
Compare
cd74f24
to
a20264b
Compare
a20264b
to
b4e0993
Compare
Tox is meh :( There is no pcp wheel so need to build it and the PyPi version is outdated 5.0 😞 |
2f49acb
to
e303bd2
Compare
ccb5c34
to
1515e07
Compare
1515e07
to
8c9c57b
Compare
877b872
to
34af402
Compare
e8f7a0e
to
99c2721
Compare
4ebf17d
to
31985ad
Compare
It is green now \o/ |
@@ -0,0 +1,670 @@ | |||
# This file is part of Cockpit. |
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.
Maybe rename the file to pcpmetrics.py?
I checked /var/lib/pcp/config/pmlogconf/tools/cockpit - that's the only remaining file in cockpit-pcp; all enabled metrics are already present in a multitude of other configs in /etc/pcp/pmlogconf , so we can just drop this. It's also strange and against our principles to change the system config with installing cockpit packages. So independently of this PR we can already drop that. Assuming that the cockpit-pcp binary and the manifest go away, we then just need to change our on-demand installation from |
See => #21020 |
Looking again at the fetchGroup API, this looks useful but it is so freaking broken, for some reason it returns no instances for the first metric.. (why!)
Maybe something for a follow up, it seems nice
|
31985ad
to
55322ca
Compare
@martinpitt @mvollmer an initial review would be appreciated, I am not super fond of the spaghetti soup, combining direct and archived metrics together kinda makes it messy This is also the reason we can't easily combine the internalmetrics and the PCP metrics, the archive metrics are too different. Plus we have no Sampler() class |
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.
Thanks @jelly , great work! I have a lot of little things, but honestly I'm not too worried about the code structure -- there is a lot of bit shifting, data type conversion, and plumbing, and I trust that all of this is necessary. But the overall structure is relatively easy to follow.
I'm really looking forward to landing this!
b9bee81
to
fdf20b7
Compare
The Python bridge still used the separate `cockpit-pcp` bridge for metrics gathering. For us to remove the full C bridge implementation the separate PCP bridge also has to be rewritten. This rewrite is a more or less Python copy of the C implementation using the Python PCP module. Even though the Python PCP module offers a "higher level" fetchGroup API but preliminary testing has found this has some issues with changing multi instance values and instances can not be omitted with a fetch group (but this can be done in our own code) Co-Authored-By: Allison Karlitskaya <[email protected]> Co-Authored-By: Tomas Matus <[email protected]>
fdf20b7
to
8d26edc
Compare
This seems to go green! @martinpitt I re-added the manifest as the JS code uses this to detect if PCP is available and if it's missing all PCP tests simply fail. A follow up PR will take care of that. |
Current ToDo's
exception
test/image-prepare -q debian-testing
it fails on pytest.Follow ups, I'd suggest tackling:
pcp python3-pcp
instead of cockpit-pcpcockpit-pcp
(release-blocker)