feat(inputs.procstat): Allow Multi filter

[tguenneguez]

Summary

Add new option "multi_criterias" to be able to search process with multiples criterias.
For exemple :

• user and exe
• exe and pattern

It's possible with somethink like :

metricpass = "tags.process_name == "httpd""
But in this case, telegraf collect lot of information to put them in garbage can

Checklist

• No AI generated code was used in this PR

Related issues

#6174

[telegraf-tiger]

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_arm64.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz

[srebhan]

@tguenneguez thanks for looking into this and thanks for your contribution. I think using statefull PID finders (i.e. internal variables) is not a good solution because this has proven to get complicated over time. Instead I would propose to add one or multiple filter section(s) with the following form

type Filter struct {
    Exec string
    ...
    Recursive bool
}

and then change the PID finders to process those filters. For pgrep this would mostly relate to combining the command-line arguments. For native this would mean you get the process list once and then filter each process according to the filter settings.

So this is a larger restructuring of the plugin. My proposal is to implement this for pgrep-only first and check if that works as I think the combination of command-line args should be easier. Duplicate code where necessary. Afterwards we can, in a second PR, also do it for the native finder...

What do you think?

[tguenneguez]

Good morning,

I understand your thinking, but I see 2 objections:

1. This new good generates a compatibility break.
2. It will not be possible to set actions for certain filters (tags, metricpass, etc.)

On the other hand, with pgrep, we do not deport the process search to the arguments provided so the filtering is carried out by pgrep directly.
In "native" mode, we actually list all the processes and then look at those that match the criteria, which can be cumbersome on a machine with a lot of processes, but that's roughly what "pgrep" does when the we look at the sources.

Finally, if we go in this direction, the "exec" filtering is not fair.
For linux with pgrep, it filters on the process name while natively, the filtering is on the binary.
For this point, I was thinking of making another PR by adding "name" filtering. This would allow in "native" mode to search on the process name (/proc/(pid)/stat). In "pgrep" mode, this would be equivalent to "exec" for compatibility.

Good reflection
Thomas

[srebhan]

@tguenneguez thanks for your thoughts! Regarding the naming etc, this should really be an independent PR!

Regarding your concerns/objections:

This new good generates a compatibility break.

The idea is to keep the old way around and add this feature without breaking the existing output if you use the old way of configuration. I'm emphasizing this as changing current code-paths has the risk of unexpected side-effects. If for example now people specified multiple criteria, only one of those "wins" and is used, all others are silently ignored. If you now change that, people will suddenly get different results.

Furthermore, with the current "flat" configuration it is kind of unclear what the emitted "aggregation metrics" really aggregate...

It will not be possible to set actions for certain filters (tags, metricpass, etc.)

Well that's also not possible today. :-) You can set tags per plugin-instance which is what most people want. You can still extend this per emitted metric using processors (e.g. the lookup processor).

[tguenneguez]

Hello @srebhan

The idea is to keep the old way around and add this feature without breaking the existing output if you use the old way of configuration. I'm emphasizing this as changing current code-paths has the risk of unexpected side-effects. If for example now people specified multiple criteria, only one of those "wins" and is used, all others are silently ignored. If you now change that, people will suddenly get different results.
That's wy a had the "multi_criterias" option.
false (default), the first filter criterias is used.
true, all filter criterias are used.
With your method, the user will understant that without filter, only the first is used, if

Furthermore, with the current "flat" configuration it is kind of unclear what the emitted "aggregation metrics" really aggregate...
It's simple for the agregation, it's for each per plugin-instance

Other think, about using pgrep first. I think this is not good for some case :
Windows, you will install pgrep.
Linux, pgrep search in the 4096 characters and with some process (ie java), you will have more than 4096 char of args and the search doesn't work.
I think methode pgrep will be implement first and stay available for compatibilite, but I think it's not good idea.
In my use case I don't use pgrep methode ;-)

On the other hand, your proposal requires (it seems to me a very big development) that I could not take care of...
When we see that the initial request dates from Jul 26, 2019, it would be interesting to propose something ?

[srebhan]

I didn't mean to suggest pgrep first. But no matter if you use pgrep or the native method there are two ways to so multi-criterion filtering:

1. Iterate over the criteria. Get the set of PIDs for the criterion and the intersect it with the set of remaining PIDs that matched the previous criteria. For example if you want to get all processes of user jo with the pattern myapp.* you can first get all processes for user jo and then all processes with pattern myapp.* and then determine the PIDs which are available in both lists.
2. Combine the criteria and match each process against this combined criterion. In pgrep this is simply done by combining the command-line options, for the native finder this would mean get the process-list, iterate over the processes and for each process iterate over the criteria until one doesn't match.

The second approach is more consistent and less expensive and would be my preferred one, i.e. the one I suggest.

[tguenneguez]

Good morning,
I understand your logic and I am absolutely aware of it. But if you look at the logic of searching for processes whether in the pgrep source code or in the promitives of github.com/shirou/gopsutil/v3/process on "linux" ;-)
pgrep:
use of the procps_pids_get system call which lists all the PIDs
https://gitlab.com/procps-ng/procps/-/blob/master/src/pgrep.c?ref_type=heads#L744
native:
use of Processes which lists all PIDs
https://pkg.go.dev/github.com/shirou/gopsutil/v3/process#Processes
Secondly, the two solutions perform filtering

If you look at the code is developed in this direction.
The only difference between your approach and the current approach is to only make one call to retrieve the list of PIDs regardless of the number of simple or multiple filters made in a second step.
But that will pose another problem, the list of PIDs varies very quickly and by the time you get to the last filter criterion, the processes have changed and the filtering no longer works.
I think we need to limit as much as possible the moment when we retrieve the list of PIDs and the moment when we do the filtering.

Thanks
Thomas

[srebhan]

@tguenneguez I drafted a PoC of my idea in #14948. What do you think?

[cschug]

Typo? s/processus/processes/

[cschug]

Typo? s/processus/processes/

[powersj]

I'm going to close this PR while we continue to work through Sven's in #14948. We need to come to an agreement on the expected behavior and desire for these filters over there.