[extension/sumologicextension] Replace go-ps with gopsutil library#33403
[extension/sumologicextension] Replace go-ps with gopsutil library#33403harshshahsumo wants to merge 8 commits into
Conversation
github-actions
Bot
requested review from
aboguszewski-sumo,
kasia-kujawa,
mat-rumian,
rnishtala-sumo,
sumo-drosiek and
swiatekm
|
Hello @harshshahsumo, thanks for your contribution! Please sign the CLA when you get a chance 👍 |
| e, _ := v.Name() // Ignore error | ||
| e = strings.ToLower(e) |
There was a problem hiding this comment.
Why do we ignore the error? What are possible errors?
There was a problem hiding this comment.
Looks like, at least on some platforms, calling Name() can result in a syscall, so I guess the error is platform specific. IMO we should propagate these errors to the caller.
|
This is great news, as it will reduce final collector binary size due to gopsutil being in use by so many other components. Thank you. |
sumo-drosiek
left a comment
sumo-drosiek
left a comment
There was a problem hiding this comment.
LGTM, but you need to run make lint in component directory
|
|
||
| for _, v := range p { | ||
| e := strings.ToLower(v.Executable()) | ||
| e, err := v.Name() |
There was a problem hiding this comment.
Why are we using the process name now, when in the past we were using the executable? I'd prefer if we replace the library without any semantic changes first.
There was a problem hiding this comment.
From documentation it looks like this is for the same underlying value, or at least, the closest available (from what I can tell).
go-ps doc reference for Executable():
Executable name running this process.
gopsutil doc reference for Name():
Name returns name of the process.
@harshshahsumo Can you confirm that the same value is being returned here?
There was a problem hiding this comment.
I thought we could use https://pkg.go.dev/github.com/shirou/gopsutil/v3@v3.24.5/process#Process.Exe. It returns a full path to the executable, but it's easy to extract just the binary name. I tried to look through the implementation of https://pkg.go.dev/github.com/shirou/gopsutil/v3@v3.24.5/process#Process.Name, and it was complex enough that I'm not confident it will always give the same result.
There was a problem hiding this comment.
Thanks for investigating! Sounds good to me.
I'll add a label to this PR to make sure the CI/CD actions run on Windows to make sure any path logic works properly there as well 👍
crobert-1
added
Run Windows
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
echlebek
left a comment
echlebek
left a comment
There was a problem hiding this comment.
@crobert-1 Anything more to do here before this can be merged? Stale bot is coming to reap 😆
From my understanding, the main reason this is blocked is @swiatekm's open request. Once that has been updated and conflicts are resolved I believe it will be able to move forward. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
7 participants
Description:
Replacing the old go-ps library to the existing widely used gopsutil library
Link to tracking Issue:
Link to the issue
Testing:
ran the go make test
Documentation:
No documentation required as its only library change