feat(inputs.procstat): Obtain process information through supervisor

[chenbt-hz]

• Updated associated README.md.
• Wrote appropriate unit tests.
• Pull request title or commits are in conventional commit format

resolves #13416

[telegraf-tiger]

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

[srebhan]

Thanks @chenbt-hz for the nice feature! I have some comments in the code. Please take a look and let me know once you addressed them.

[chenbt-hz]

Hello, I have modified PR according to the suggestion, but I still have a few questions.
May I ask if you @srebhan have time to help review again or answer my questions? Thank you!

The following are the main contents of my modifications this time:

1. Fix unit test for the 'supervisor'_ Unit [] string`
2. Fix the bug of procstat_ Lookup that tags ["supervisor unit"] is incorrect
3. Adjust the code one by one according to the modification suggestions.

[srebhan]

The code looks good, but I think you should also implement the ChildPattern function for the native finder...

[telegraf-tiger]

Hello! I am closing this issue due to inactivity. I hope you were able to resolve your problem, if not please try posting this question in our Community Slack or Community Forums or provide additional details in this issue and reqeust that it be re-opened. Thank you!

[chenbt-hz]

Hello, I noticed that this pull request has been closed.
@srebhan Would you be able to take a moment to review the current code and see if it can be merged? Thank you very much for your time and assistance!

[srebhan]

Thanks for the update and sorry for missing out on your update @chenbt-hz!

I do have some more small comments. Nothing too big. Can you please additionally check the unit-tests which should be adapted as we now also get childs from the native-finder...

[chenbt-hz]

Thanks for the update and sorry for missing out on your update @chenbt-hz!

I do have some more small comments. Nothing too big. Can you please additionally check the unit-tests which should be adapted as we now also get childs from the native-finder...

I added the TestChildPattern function test. May I ask if that's what you're talking about?

[srebhan]

Sure, if you look at the test-go-linux CI test at the end of the PR it says

=== FAIL: plugins/inputs/procstat TestChildPattern (0.02s)
    native_finder_test.go:50: 
                Error Trace:    /go/src/github.com/influxdata/telegraf/plugins/inputs/procstat/native_finder_test.go:50
                Error:          Not equal: 
                                expected: []procstat.PID{145942}
                                actual  : []procstat.PID(nil)
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1,4 +1,2 @@
                                -([]procstat.PID) (len=1) {
                                - (procstat.PID) 145942
                                -}
                                +([]procstat.PID) <nil>
                                 
                Test:           TestChildPattern

DONE 6550 tests, 224 skipped, 1 failure in 432.569s

I guess it is because before the latest commits you returned nil for the native child finders and now you return an actual PID. That's what the unit-test TestChildPattern is complaining about. Therefore, I think you need to adapt the TestChildPattern test to reflect that the native finder for children is implemented now.

[powersj]

@chenbt-hz - Were you able to look at Sven's comment about updating a test case now that you have implemented the child pattern? Thanks

[chenbt-hz]

@chenbt-hz - Were you able to look at Sven's comment about updating a test case now that you have implemented the child pattern? Thanks

I'm sorry for my late reply. I may need your @srebhan @powersj help to solve this problem, as my free account cannot be tested in circleci until 11/01/2023.

The current issue is that it is not possible to access the sub process PID number under the current testing process in circleci.
I used the vscode plugin during local testing on Mac, the testing process was usr/local/go/bin/go test timeout 30s - run ^ TestChildPattern $github. com/influxdata/delete/plugins/inputs/procstat , so I was able to pass the test normally.

However, I found that gotestsum testing was used in cricleci, so I tried to modify the code as follows, but still couldn't obtain the correct pid。

func TestChildPattern(t *testing.T) {
	if runtime.GOOS == "linux" || runtime.GOOS == "darwin" {
		cmd := exec.Command("/bin/bash", "-c", "echo \"TestChildPattern\" && sleep 10")
		....
		childpids, err := f.ChildPattern("TestChildPattern")
		...
    }
}

or

func TestChildPattern(t *testing.T) {
	if runtime.GOOS == "linux" || runtime.GOOS == "darwin" {
		cmd := exec.Command("/bin/bash", "-c", " sleep 10")
		....
		childpids, err := f.ChildPattern("gotestsum")
		...
  }
}

[powersj]

Hi,

Should the pattern be the sleep or echo command?

[chenbt-hz]

Hi,

Should the pattern be the sleep or echo command?

I have tried using the sleep or echo command but still couldn't obtain it. I'm not sure if there are unknown differences between CircleCI's environment and my local Mac environment that caused this anomaly.

[telegraf-tiger]

Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.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]

Approving this due to formal reasons and without the neutral view required... ;-)

[powersj]

Thank you both!

[hackery]

This creates a breaking change for the systemd_unit part of the plugin, renaming it to systemd_units. Not updated in latest README for the plugin.

Raised by @tjnome in #14438