Remove channel buffers for callback queue in process monitor #36441
Conversation
Signed-off-by: grantseltzer <[email protected]>
grantseltzer
added
changelog/no-changelog
team/dynamic-instrumentation
github-actions
bot
added
component/system-probe
short review
Uncompressed package size comparisonComparison with ancestor Diff per package
Decision✅ Passed |
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
|
| } | ||
| } | ||
|
|
||
| // initNetlinkProcessEventMonitor initialize the netlink socket filter for process event monitor. | ||
| func (pm *ProcessMonitor) initNetlinkProcessEventMonitor() error { | ||
| pm.netlinkDoneChannel = make(chan struct{}) | ||
| pm.netlinkErrorsChannel = make(chan error, 10) | ||
| pm.netlinkEventsChannel = make(chan netlink.ProcEvent, processMonitorEventQueueSize) | ||
| pm.netlinkEventsChannel = make(chan netlink.ProcEvent) |
There was a problem hiding this comment.
I only traced pendingCallbacksQueueSize, you can probably get away with just updating that one if you run into CI issues with any changes here
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 889b2b4 ❌ Experiments with missing or malformed dataThis is a critical error. No usable optimization goal data was produced by the listed experiments. This may be a result of misconfiguration. Ping #single-machine-performance and we can help out. Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | docker_containers_memory | memory utilization | +0.33 | [+0.28, +0.38] | 1 | Logs |
| ➖ | file_tree | memory utilization | +0.31 | [+0.17, +0.45] | 1 | Logs |
| ➖ | tcp_syslog_to_blackhole | ingress throughput | +0.20 | [+0.13, +0.28] | 1 | Logs |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.14 | [+0.04, +0.23] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.12 | [+0.06, +0.18] | 1 | Logs bounds checks dashboard |
| ➖ | ddot_logs | memory utilization | +0.09 | [-0.02, +0.20] | 1 | Logs |
| ➖ | ddot_metrics | memory utilization | +0.08 | [-0.04, +0.20] | 1 | Logs |
| ➖ | uds_dogstatsd_20mb_12k_contexts_20_senders | memory utilization | +0.06 | [+0.03, +0.10] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency_linear_load | egress throughput | +0.04 | [-0.18, +0.26] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency | egress throughput | +0.00 | [-0.86, +0.87] | 1 | Logs |
| ➖ | file_to_blackhole_100ms_latency | egress throughput | +0.00 | [-0.84, +0.84] | 1 | Logs |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | -0.00 | [-0.03, +0.02] | 1 | Logs |
| ➖ | file_to_blackhole_300ms_latency | egress throughput | -0.01 | [-0.85, +0.84] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api | ingress throughput | -0.01 | [-0.30, +0.29] | 1 | Logs |
| ➖ | file_to_blackhole_500ms_latency | egress throughput | -0.01 | [-0.87, +0.86] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency_http2 | egress throughput | -0.02 | [-0.86, +0.82] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency | egress throughput | -0.03 | [-0.87, +0.82] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency_http1 | egress throughput | -0.03 | [-0.88, +0.82] | 1 | Logs |
| ➖ | otlp_ingest_metrics | memory utilization | -0.10 | [-0.25, +0.04] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api_cpu | % cpu utilization | -0.35 | [-1.23, +0.54] | 1 | Logs |
| ➖ | quality_gate_logs | % cpu utilization | -0.48 | [-3.26, +2.30] | 1 | Logs bounds checks dashboard |
| ➖ | otlp_ingest_logs | memory utilization | -0.59 | [-0.74, -0.43] | 1 | Logs |
| ➖ | docker_containers_cpu | % cpu utilization | -2.88 | [-6.62, +0.86] | 1 | Logs |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | links |
|---|---|---|---|---|
| ✅ | docker_containers_cpu | simple_check_run | 10/10 | |
| ✅ | docker_containers_memory | memory_usage | 10/10 | |
| ✅ | docker_containers_memory | simple_check_run | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http1 | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http1 | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http2 | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_0ms_latency_http2 | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_1000ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_1000ms_latency_linear_load | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_100ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_100ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_300ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_300ms_latency | memory_usage | 10/10 | |
| ✅ | file_to_blackhole_500ms_latency | lost_bytes | 10/10 | |
| ✅ | file_to_blackhole_500ms_latency | memory_usage | 10/10 | |
| ✅ | quality_gate_idle | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_logs | lost_bytes | 10/10 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check lost_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
There was a problem hiding this comment.
Why do you use process monitor? you should be using process-event-data-streams
A couple of months ago I wrote your team in slack about migrating to PEDS, as we know the process-monitor is unstable on one hand, and it cannot be used by multiple modules (usm + dynamic instrumentation) at the same time
your team promised to make the migration
The reason for the buffering came from process-churn that caused the entire code to be slow and blocked
The plan remains to use PEDS, but it will take some rearchitecting of code. As far as I understand this affects PEDS as well though. Does USM operate alright with dropped callbacks? |
You shouldn't be using process monitor, but PEDS directly (unlike USM today), so the code you've changed shouldn't impact peds |
|
Closing in favor of #36775 |
Motivation
When starting to deploy the dynamic instrumentation module I observed hard to track down behavior where services that we want to instrument were not being picked up by the process monitor. Through manual investigation @tylfin and I concluded that a potential cause was the buffered channels used in the process monitor.
When
SubscribeExec()gets called, the callback that's passed gets add to a collection of callbacks. Then, every time an exec occurs, the collection of callbacks is iterated over and each one is passed to a worker pool where individual go routines will call the callback on the exec'd process' pid. However, on startup the process monitor iterates over the systems running process tree and treats each running process as if it were an exec, passing the callbacks for each on the channel. Depending on how many subscribers, this can overwhelm the channels buffer, filling it up.When the channel buffer is filled, current logic just drops the callback from being sent on it instead of waiting for the buffer to be emptied. I'm not sure why this was done this way. Regardless, every time a new subscriber is added to the process monitor, the need to increase the buffer size will come again. Therefore I don't see a reason to keep this unbuffered and let the workers complete all callbacks.
Describe how you validated your changes
Deploying custom image to staging cluster to test manually. Need input from other teams that rely on process monitor to ensure it works as expected for them.
Possible Drawbacks / Trade-offs
Additional Notes