munin-monitoring / munin

Main repository for munin master / node / plugins
http://munin-monitoring.org
Other
1.98k stars 473 forks source link

proc does not support process names with a dot #777

Closed doublehp closed 6 years ago

doublehp commented 7 years ago

munin-update.log :

2017/01/12 19:25:37 [WARNING] Service proc_threads on server:4949 returned no data for label zmaudit
2017/01/12 19:25:37 [WARNING] Service proc_threads on server:4949 returned no data for label zmtelemetry
2017/01/12 19:25:37 [WARNING] Service proc_threads on server:4949 returned no data for label zmwatch
2017/01/12 19:25:37 [WARNING] Service proc_threads on server:4949 returned no data for label zmdc
2017/01/12 19:25:37 [WARNING] Service proc_threads on server:4949 returned no data for label zmfilter

./proc config :

multigraph proc_threads
ssh.value 2
sshfs.value 20
jsvc_unifi.value 19
mongod.value 19
perl.value 0
zma.value 5
zmc.value 5
zmdc.pl.value 1
zmfilter.pl.value 1
zmaudit.pl.value 1
zmwatch.pl.value 1
zmtelemetry.pl.value 1

Conf file for plugin:

[proc]
user root
# name in parenthesis in /proc/123/stat
env.procname ssh|sshfs|jsvc|mongod|perl|zma|zmc|zmdc.pl|zmfilter.pl|zmaudit.pl|zmwatch.pl|zmtelemetry.pl
env.procargs ||unifi||||||||

Not my fault if the process name contains a dot. If i remove the dot and extension, proc can not find process at all cause regexp will fail.

doublehp commented 7 years ago

Here is my fix for this issue:

# diff /usr/share/munin/plugins/proc proc
323c323
<         STATLINE: foreach my $line(`grep -h \\\($procname[$i]\\\) /proc/[0-9]*/stat`) {
---
>         STATLINE: foreach my $line(`grep -h \\\($procname[$i]\\\) /proc/[0-9]*/stat 2>/dev/null`) {
533c533,534
<         $procuniq[$i] = "$procname[$i]";
---
> #        $procuniq[$i] = "$procname[$i]";
>       ($procuniq[$i] = $procname[$i]) =~ tr[.][_];
doublehp commented 7 years ago

Update: see https://github.com/munin-monitoring/munin/issues/830 for details

# diff /usr/share/munin/plugins/proc proc
323c323
<         STATLINE: foreach my $line(`grep -h \\\($procname[$i]\\\) /proc/[0-9]*/stat`) {
---
>         STATLINE: foreach my $line(`grep -h \\\($procname[$i]\\\) /proc/[0-9]*/stat 2>/dev/null`) {
533c533,535
<         $procuniq[$i] = "$procname[$i]";
---
> #        $procuniq[$i] = "$procname[$i]";
> #     ($procuniq[$i] = $procname[$i]) =~ tr[.][_];
>       ($procuniq[$i] = $procname[$i]) =~ tr/.A-Z-/_a-z_/;
536c538,539
<             $procuniq[$i] = $procuniq[$i]."_".$procargs[$i];
---
> #            $procuniq[$i] = $procuniq[$i]."_".$procargs[$i];
>             ($procuniq[$i] = $procuniq[$i]."_".$procargs[$i]) =~ tr/.A-Z-/_a-z_/;
540c543,544
<             $procuniq[$i] = $procuniq[$i]."___".$procuser[$i];
---
> #            $procuniq[$i] = $procuniq[$i]."___".$procuser[$i];
>             ($procuniq[$i] = $procuniq[$i]."___".$procuser[$i]) =~ tr/.A-Z-/_a-z_/;

I know that caps can be an issue in category names; can it be an issue in a multigraph name ?

Not sure it's sane, or if there may be other potential bugs I am not aware about, or if the cap conversion is really required ...

sumpfralle commented 7 years ago

How about using the clean_fieldname function, that is part of munin? This would solve unexpected border cases, as well.

Of course, it would be perfect, if you would create a pull request instead of embedding patches :) (otherwise I will manually transfer your patch and hope for the best)

Thank you!

doublehp commented 7 years ago

Only 24h per day; yes clean_fieldname would be better, never used it; it was already time consuming to isolate the line where problem lays and check it was the correct place to fix things; never learnt to create pulls, maybe an other day. Also, I am very bad at perl, and may propose subefficient solutions; offering a diff here will let you (or other merger guy) fix details, while this would probably more complex with pull requests. Especially, a bug tracker is a place to discuss about optimal solution; I don't feel good enough coder to submit pull requests.

sumpfralle commented 7 years ago

I understand. If no one else (with a more perl-friedly brain) jumps in, I will take a look at it.

sumpfralle commented 6 years ago

This was fixed in 4edef49ae.

doublehp commented 6 years ago

Thanks.

At last ... some one fixes the bugs. Happy to see source evolving.

Can't wait for the update.

doublehp commented 6 years ago

sumpfralle I have reworked my plugin ; and I don't see any update in debian; so, before creating a new bug, I report my update here:

# diff /usr/share/munin/plugins/proc /etc/munin/plugins/proc
323c323
<         STATLINE: foreach my $line(`grep -h \\\($procname[$i]\\\) /proc/[0-9]*/stat`) {
---
>         STATLINE: foreach my $line(`grep -h \\\($procname[$i]\\\) /proc/[0-9]*/stat 2>/dev/null`) {
533c533,538
<         $procuniq[$i] = "$procname[$i]";
---
> #        $procuniq[$i] = "$procname[$i]";
> #     ($procuniq[$i] = $procname[$i]) =~ tr[.][_];
>       ($procuniq[$i] = $procname[$i]) =~ s/[ .-]/_/g;
> #     $procargs[$i] =~ tr/ .-/_/s;
>       # s/[\s.-]+/_/g; or tr/ .-/_/s;
> # genio: regex version: s/[ .-]/_/g   that will replace all individual things with an underscore.   s/[ .-]+/_/g  will squash them if there are more than one together and replace with one underscore.   and the literal space can be replaced with \s in either to represent all whitespace characters
536a542
>           $procuniq[$i] =~ s/[ .-]/_/g;

this works much better; it could yet be improoved for procuser ...

And I probably don't catch yet all possible bad chars

sumpfralle commented 6 years ago

I don't see any update in debian

The process is as follows:

  1. release v2.0.35
  2. update Debian pacakge

Don't hold your breath! :)

Regarding your changeset proposal: clean_fieldname is really the proper and most simple way to solve this kind of problems.

I took a closer look at the current state of the plugin in stable-2.0. The above fix for master cannot be easily backported - thus I fixed the most relevant issue in stable-2.0:

doublehp commented 6 years ago

Looks good for me.