Add a nginxprocess package for working with NGINX processes
#956
Conversation
|
✅ All required contributors have signed the F5 CLA for this PR. Thank you! |
|
I have hereby read the F5 CLA and agree to its terms |
ryepup
force-pushed
the
export-pid-features
branch
from
22a996f to
39ba7e4
Compare
|
I'm not sure why the pipeline is failing; |
ryepup
force-pushed
the
export-pid-features
branch
from
39ba7e4 to
eed2042
Compare
ryepup
force-pushed
the
export-pid-features
branch
from
eed2042 to
f27ef37
Compare
ryepup
force-pushed
the
export-pid-features
branch
4 times, most recently
from
2a306bf to
fe5c421
Compare
|
ryepup
force-pushed
the
export-pid-features
branch
2 times, most recently
from
690b7a5 to
04c36a0
Compare
| } | ||
| } | ||
|
|
||
| func BenchmarkList(b *testing.B) { |
There was a problem hiding this comment.
Not sure if we should keep these benchmarks as they are dependent on reading processes running on your machine. Results would vary between machines. It could cause our benchmark comparisons to fail in our pipeline https://github.com/nginx/agent/blob/v3/.github/workflows/ci.yml#L225
There was a problem hiding this comment.
This benchmark was useful for development to compare with the existing implementation and verify how expensive additional gopsutil calls were. I can see how it might mess up CI due to randomness of the runners.
Which would you prefer:
- delete it
- put a
b.Skipfat the top that devs can comment out if they're working around here - put a
if os.Getenv("CI") != "" { b.Skipf(...) }at the top
There was a problem hiding this comment.
It they were useful for you during development then maybe we should keep them and put a b.Skipf in. Maybe just add a comment as well to the function to say why they are skipped.
Based on @dhurley feedback in nginx#956 (comment)
ryepup
force-pushed
the
export-pid-features
branch
from
04c36a0 to
8344487
Compare
github-actions
bot
added
the
documentation
Based on @dhurley feedback in nginx#956 (comment)
ryepup
force-pushed
the
export-pid-features
branch
from
8344487 to
988212a
Compare
This package is intended for use by any software that wants to identify NGINX processes. It's mostly a refactor of the existing `process.ProcessOperator` with a few differences and some features wanted by NGINXaaS. - minimizes `gopsutil` calls, many of which end up shelling out to `ps`. `nginxprocess.List` filters out non-NGINX processes early. `process.ProcessOperator` gathered details for all processes, and then non-NGINX processes were filtered out later by `instance.NginxProcessParser`. `nginxprocess.List` also fetches less process information, keeping only what the agent codebase actually used. A quick benchmark on my laptop (~750 processes, 12 of which are NGINX) showed a drastic speed improvement: | Benchmark | Iterations | Time (ns/op) | Memory (B/op) | Allocations (allocs/op) | | ----------- | ---------- | ------------ | ------------- | ----------------------- | | old | 1 | 26832273195 | 18282768 | 133775 | | new | 9 | 114558263 | 3439672 | 12836 | - uses a functional options pattern to optionally fetch process status, which is only needed in a few places and saves some more shelling out to `ps`. This could be expanded in the future to support other options while retaining backwards compatibility. - includes some error testing funcs to let callers handle different errors without needing to know how those errors are implemented. This helps trap `gopsutil` implementation details in `package nginxprocess` and makes future `gopsutil` upgrades easier. - respects context cancellation a little faster
Swap internal packages to use the shared `nginxprocess` package for details about NGINX processes. This is mostly switching `model.Process` for `nginxprocess.Process`, updating tests, and using features of `nginxprocess. There is one non-trivial change: `model.Process.Running` has been dropped, there is no equivalent in `nginxprocess`. `Running` was almost always true, because it was calculated immediately after a successful call to `NewProcessWithContext`. In order for it to be false, a process would have to end after `NewProcessWithContext`, but before `IsRunningWithContext`, which is an incredibly tiny time window. There is also no guarantee that the process is _still_ running by the time callers check `Running`, so the field was a little misleading. The system now relies solely on errors from `NewProcessWithContext` to identify terminated processes. Worst case we catch a dead process on the next tick of `HealthWatcherService`
Existing names were really similar to imported package names or other variables.
Based on @dhurley feedback in nginx#956 (comment)
ryepup
force-pushed
the
export-pid-features
branch
from
988212a to
9bc5a98
Compare
|
Did a rebase, looks like there's some test flake around the otel collector? |
Proposed changes
Add a new
nginxprocesspackage to export useful utilities around working with NGINX processes, and update internals to use that package. NGINXaaS for Azure has some proprietary code that also wants to use these utilities.This comes with a significant speed improvement by filtering for NGINX processes earlier and fetching less data from the OS. See individual commits for more details.
Checklist
Before creating a PR, run through this checklist and mark each as complete.
CONTRIBUTINGdocumentmake install-toolsand have attached any dependency changes to this pull requestREADME.md)