Re: [pve-devel] [PATCH pve-manager 2/3] Fix #5708: Add CPU raw counters

[Daniel Kral <d.kral@proxmox.com>]

On 9/30/24 08:17, Sascha Westermann wrote:
> If the data is also to be processed by external metric servers, I think
> the integration in `PVE::ProcFSTools::read_proc_stat()` makes sense. The
> term `cpustat` would no longer conflict in this case, as the content
> would be virtually the same. I would create a new patch series for this,
> but when looking at `read_proc_stat` a few questions arose for me:
> 
>> sub read_proc_stat {
>>      my $res = { user => 0, nice => 0, system => 0, idle => 0 , iowait => 0, irq => 0, softirq => 0, steal => 0, guest => 0, guest_nice => 0, sum => 0};
> 
> In order to remain consistent with the structure, the same fields per
> CPU would also need to be initialized with 0. However, this is only
> done when I find an entry of the form cpu<num>, which implicitly gives
> me values - so the initialization would not do anything. In this
> context, I wonder whether there are any situations where no values can
> be set? I would actually say that this is not the case and that the
> initialization can be removed.

Those fields seem to be initialized to zero here, as they are summed 
later for `$res->{total}` [0]. If the `/proc/stat` file couldn't be read 
or for some reason no `cpu` was captured, those would be initialized to 
`undef`. As far as I'm aware, an addition with a `undef` value would 
convert it to `0` in numerical contexts, so removing it shouldn't be a 
problem, but I could also be missing something here.

---

At least for me, I think you wouldn't need to initialize the `cpu<num>` 
fields with zero, but just make sure that any capture group that uses a 
prefixed `?` could become `undef` [1], so that case needs to be handled, 
just like `$res->{guest}` and `$res->{guest_nice}` is.

> 
> Side note: `sum` is not used, it probably means `total`, right?

As far as I'm aware, I couldn't find a usage for the `sum` property 
anywhere the `read_proc_stat` is used and could be deleted.

> 
>>     if (my $fh = IO::File->new ("/proc/stat", "r")) {
>> 	while (defined (my $line = <$fh>)) {
>> 	    if ($line =~ m|^cpu\s+(\d+)\s+(\d+)\s+(\d+)\s+(\d+)\s+(\d+)\s+(\d+)\s+(\d+)\s+(\d+)(?:\s+(\d+)\s+(\d+))?|) {
>> 		$res->{user} = $1 - ($9 // 0);
>> 		$res->{nice} = $2 - ($10 // 0);
>> 		$res->{system} = $3;
> 
> `user` contains `guest` and `nice` contains `guest_nice`, so I
> understand that the values are subtracted. Wouldn't it be better to use
> the original values here? Especially when these are passed out as raw
> values via API, it would certainly be helpful if they correspond 1:1
> with the documentation from /proc/stat.

I can only point you in the direction of the original author's commit, 
it seems like it was inspired by other monitoring tools, but I haven't 
checked the latter two references in the commit message [2].

I get your point, but there might already be many (external) users, 
which depend on these values as they are calculated here (thinking of 
InfluxDB/Grafana dashboards, API users, ...). So if it doesn't hurt your 
use case too badly (and others that would use your new data points 
too!), I would adopt that for the `cpu<num>` datapoints as well.

> 
> The values are output as a string in the JSON output. Is there anything
> against casting them to int?

Good catch! It would be great if any numerical value is also treated 
like that. Be aware that Perl doesn't really have an explicit way to 
'cast' variables as types are handled by context. The builtin `int` 
function only makes sure that we only get the integer part of an 
expression [3]. So be aware, but if it doesn't break something (or 
change probable existing expectations for API users), go ahead.

> 
>>     my $diff = ($ctime - $last_proc_stat->{ctime}) * $clock_ticks * $cpucount;
>>
>>     if ($diff > 1000) { # don't update too often
> 
> I don't understand the condition. `$ctime - $last_proc_stat->{ctime}`
> corresponds to the elapsed time in seconds as a float, `clock_ticks`
> would normally be 100. So that would mean that on a system with one CPU
> core an update may only take place every 10 seconds, but with 8 cores
> every 1.25 seconds? Is that an error? Is it even necessary to suppress
> updates if $diff > 0?

Unfortunately, as some other things here, these changes predate our git 
repository and I couldn't tell you exactly why it is that way.

As far as I can say, as this is retrieved from the WebGUI's node status 
page around every second, this seems like it should "buffer" changes to 
these two data points, so that there are no spikes for the cpu% and 
wait%. But I don't quite get why it is multiplied with `$clock_ticks` 
and `$cpu_count` either.

If you really want to change this, I would do that in another patch 
(series) and mark it as an "RFC" (Read for Comments), so it gets a 
little more discussion if that change conflicts with anyone.

---

Hope these remarks clear up some problems. Looking forward to your patches!

Cheers,
Daniel

[0] https://git.proxmox.com/?p=pve-common.git;a=commitdiff;h=5224b31b
[1] https://perldoc.perl.org/perlvar#$%3Cdigits%3E-($1,-$2,-...)
[2] https://git.proxmox.com/?p=pve-common.git;a=commitdiff;h=c140206b
[3] https://perldoc.perl.org/functions/int


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel