feat(inputs.procstat): Add child level tag #16105

tguenneguez · 2024-10-29T18:23:25Z

Summary

Add tag for children process to be able to have information by level

Checklist

No AI generated code was used in this PR

Related issues

resolves #16104

DStrand1

Thanks for the PR! I have one code question, but also does this close issue #16104? If so could you update the PR description to close the issue? Thanks!

DStrand1 · 2024-10-29T21:12:31Z

plugins/inputs/procstat/filter.go

@@ -202,6 +202,7 @@ func (f *Filter) ApplyFilter() ([]processGroup, error) {
 					tags[k] = v
 				}
 				tags["parent_pid"] = strconv.FormatInt(int64(p.Pid), 10)
+				tags["child_level"] = strconv.FormatInt(int64(depth), 10)


I think this should be a configuration option since it is a new tag

I agree as it will also likely produce high cardinality. How about adding this as a field instead?

I think the cardinality is higher on the parent_pid field so this change is less impactful than one might think.

In any case, we cannot add a new tag unconditionally...

Okay, I understand your point of view.
What option would you suggest to add this tag?
Is there a naming rule?
It also means that for each plugin each time new tags are added, an option is needed. Is this likely to create plugins with long configurations?
We could put:
tag_child_level.
If not defined, the tag is not added.
If tag_child_level=toto then the level is added in the tag "toto"

I think this should be a configuration option since it is a new tag
@DStrand1 Have you a name recommanded for activate new tag ?

[...] It also means that for each plugin each time new tags are added, an option is needed. Is this likely to create plugins with long configurations? [...]

That's why we prefer "lists" of what to add instead of boolean options. This way we have one option with many potential values. For this plugin there actually already is the tag_with option, so why don't you just add the child-level there?
Check the code for how this option is used to conditionally add tags...

Thanks.
I tryed, but I have 2 errors that I doesn't understand ...

tguenneguez · 2024-10-30T13:07:32Z

Thanks for the PR! I have one code question, but also does this close issue #16104?

Yes it's the same. The issue is in the PR description.

What do you understand exactly by :

If so could you update the PR description to close the issue? Thanks!

srebhan · 2024-12-09T11:58:14Z

@tguenneguez the error means that the sample.conf file does not match what is in the README.md.

You need to add the option documentation to sample.conf and then run make docs to embed that sample.conf file into the README.md. This is done to ensure that the config options documented in the README do match the ones in the example config produced by Telegraf...

…into AddChildInfo

tguenneguez · 2024-12-09T19:01:58Z

@tguenneguez the error means that the sample.conf file does not match what is in the README.md.

You need to add the option documentation to sample.conf and then run make docs to embed that sample.conf file into the README.md. This is done to ensure that the config options documented in the README do match the ones in the example config produced by Telegraf...

Thank you very much, I don't know how you found the cause, if you can tell me I will be more independent next time ;-)

…into AddChildInfo

tguenneguez · 2024-12-10T19:39:05Z

I understand my error ;-)
tag_with is for procstat metric and not for procstat_lookup metrics
Also, the level is not internal information for process, but additionnal information of grouping procstat_lookup...

So I thing it will be great to have possibility to generate "procstat_lookup" for different level...

telegraf-tiger · 2024-12-11T17:21:56Z

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 increases the Telegraf binary size by 1.79 % for linux amd64 (new size: 273.6 MB, nightly size 268.8 MB)

📦 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 the update. I do have some comments. The most important one is to only add the child_level tag if the option is actually set. The code adds it unconditionally still.

srebhan · 2024-12-13T10:11:31Z

plugins/inputs/procstat/README.md

+  ##   cmdline     -- full commandline
+  ##   pid         -- ID of the process
+  ##   ppid        -- ID of the process' parent
+  ##   status      -- state of the process
+  ##   user        -- username owning the process


Why are you changing the indentation level here?

srebhan · 2024-12-13T10:11:56Z

plugins/inputs/procstat/README.md

+  ## tag only available for procstat_lookup
+  ##   level       -- level of the process filtering


Could you please indent the option similar to the ones above?

srebhan · 2024-12-13T10:12:40Z

plugins/inputs/procstat/sample.conf

-  ##   cmdline   -- full commandline
-  ##   pid       -- ID of the process
-  ##   ppid      -- ID of the process' parent
-  ##   status    -- state of the process
-  ##   user      -- username owning the process
+  ##   cmdline     -- full commandline
+  ##   pid         -- ID of the process
+  ##   ppid        -- ID of the process' parent
+  ##   status      -- state of the process
+  ##   user        -- username owning the process


See my comments in README.md...

srebhan · 2024-12-13T10:15:13Z

plugins/inputs/procstat/filter.go

@@ -201,10 +200,11 @@ func (f *Filter) ApplyFilter() ([]processGroup, error) {
 				for k, v := range group.tags {
 					tags[k] = v
 				}
-				tags["parent_pid"] = strconv.FormatInt(int64(p.Pid), 10)


Why are you removing this here?

srebhan · 2024-12-13T10:16:12Z

plugins/inputs/procstat/filter.go

@@ -180,7 +179,7 @@ func (f *Filter) ApplyFilter() ([]processGroup, error) {

 			matched = append(matched, p)
 		}
-		result = append(result, processGroup{processes: matched, tags: g.tags})
+		result = append(result, processGroup{processes: matched, tags: g.tags, level: 0})


level will be 0 without explicitly setting it. Please revert to the previous code.

srebhan · 2024-12-13T10:17:56Z

plugins/inputs/procstat/procstat.go

+		tags := map[string]interface{}{
+			"pid_count":   count,
+			"running":     len(running),
+			"result_code": 0,
+		}
 		// Add lookup statistics-metric
 		acc.AddFields(
 			"procstat_lookup",
-			map[string]interface{}{
-				"pid_count":   count,
-				"running":     len(running),
-				"result_code": 0,
-			},
+			tags,


Please revert as this has nothing to do with your feature I think.

srebhan · 2024-12-13T10:18:53Z

plugins/inputs/procstat/procstat.go

@@ -494,7 +513,7 @@ func (p *Procstat) findSupervisorUnits() ([]PidsTags, error) {
 		if err != nil {
 			return nil, fmt.Errorf("getting children for %d failed: %w", pid, err)
 		}
-		tags := map[string]string{"pattern": p.Pattern, "parent_pid": p.Pattern}
+		tags := map[string]string{"pattern": p.Pattern, "parent_pid": p.Pattern, "child_level": p.Pattern}


You need to conditionally add the child_level here!

srebhan · 2024-12-13T10:19:16Z

plugins/parsers/grok/README.md

@@ -103,6 +103,15 @@ Debug](https://grokdebug.herokuapp.com) application quite useful!
  ##   %{COMBINED_LOG_FORMAT} (access logs + referrer & agent)
  grok_patterns = ["%{COMBINED_LOG_FORMAT}"]

+  ## This is a list of patterns to count the given log file(s) for.


Please move changes to the GROK parser to a separate PR!

feat(inputs.procstat): Add child level tag

6c3c47d

telegraf-tiger bot added area/procstat feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Oct 29, 2024

DStrand1 reviewed Oct 29, 2024

View reviewed changes

DStrand1 self-assigned this Oct 29, 2024

tguenneguez and others added 2 commits December 5, 2024 19:19

Add level information as field and add avaibility to add as tag

bb65f20

Merge branch 'influxdata:master' into AddChildInfo

6e4220d

tguenneguez added 2 commits December 9, 2024 19:58

Update sample.conf doc

20b36fc

Merge branch 'AddChildInfo' of https://github.com/tguenneguez/telegraf …

94a780a

…into AddChildInfo

tguenneguez and others added 4 commits December 9, 2024 20:02

Merge branch 'influxdata:master' into AddChildInfo

f89948c

Corrige syntaxe

5fc2efd

Merge branch 'AddChildInfo' of https://github.com/tguenneguez/telegraf …

ad4f627

…into AddChildInfo

Add tag level for procstat_lookup

ebffcd7

tguenneguez and others added 5 commits December 10, 2024 20:58

Merge branch 'influxdata:master' into AddChildInfo

d9f3e53

Corrige syntaxe

276dd19

Remove arguments not used

3073e64

Corrige le bug de label

b8e6ea5

Merge branch 'influxdata:master' into AddChildInfo

cbf1c24

srebhan requested changes Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(inputs.procstat): Add child level tag #16105

feat(inputs.procstat): Add child level tag #16105

tguenneguez commented Oct 29, 2024 •

edited by srebhan

Loading

DStrand1 left a comment

DStrand1 Oct 29, 2024

srebhan Oct 30, 2024

tguenneguez Oct 30, 2024

srebhan Oct 31, 2024

tguenneguez Oct 31, 2024

tguenneguez Nov 13, 2024

tguenneguez Nov 19, 2024

srebhan Dec 4, 2024

tguenneguez Dec 5, 2024

tguenneguez commented Oct 30, 2024

srebhan commented Dec 9, 2024

tguenneguez commented Dec 9, 2024 •

edited

Loading

tguenneguez commented Dec 10, 2024

telegraf-tiger bot commented Dec 11, 2024

Artifact URLs

srebhan left a comment

srebhan Dec 13, 2024

srebhan Dec 13, 2024

srebhan Dec 13, 2024

srebhan Dec 13, 2024

srebhan Dec 13, 2024

srebhan Dec 13, 2024

srebhan Dec 13, 2024

srebhan Dec 13, 2024

		## tag only available for procstat_lookup
		## level -- level of the process filtering

feat(inputs.procstat): Add child level tag #16105

Are you sure you want to change the base?

feat(inputs.procstat): Add child level tag #16105

Conversation

tguenneguez commented Oct 29, 2024 • edited by srebhan Loading

Summary

Checklist

Related issues

DStrand1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tguenneguez commented Oct 30, 2024

srebhan commented Dec 9, 2024

tguenneguez commented Dec 9, 2024 • edited Loading

tguenneguez commented Dec 10, 2024

telegraf-tiger bot commented Dec 11, 2024

Artifact URLs

srebhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tguenneguez commented Oct 29, 2024 •

edited by srebhan

Loading

tguenneguez commented Dec 9, 2024 •

edited

Loading