Convert Process monitoring to GO (ebpf.plugin) - #23862
Draft
thiagoftsm wants to merge 6 commits into
Draft
thiagoftsm wants to merge 6 commits into
thiagoftsm wants to merge 6 commits into
Conversation
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
19 issues found across 21 files
Confidence score: 2/5
src/collectors/ebpf.plugin/ebpfgo.plugin/process_global.gochanges legacy process charts and dimensions, which can break existing dashboards, queries, and alerts for process status, process/thread activity, and task errors — restore the legacy charts and dimensions alongside the new ones.src/collectors/ebpf.plugin/ebpfgo.plugin/main.gocan let cachestat overwrite process ownership and clear the process availability flag when both collectors are enabled, making process metrics appear unavailable — enforce one owner for the shared-memory segment and preserve its availability state.src/collectors/ebpf.plugin/ebpfgo.plugin/libbpfloader/process_libbpf.ccan silently publish zero snapshots or skip release/fork hooks because setup errors are ignored and legacy program names do not match the lookup — propagate setup failures and use the actual program names.src/collectors/apps.plugin/apps_ebpf_shared_memory.candsrc/collectors/apps.plugin/apps_output.cuse inconsistent counter-reset and incremental-total handling, so restarts or shared-memory resets can suppress process updates or produce incorrect chart values — apply the established reset and accumulation semantics to process metrics.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/collectors/ebpf.plugin/ebpfgo.plugin/libbpfloader/process_libbpf.go">
<violation number="1" location="src/collectors/ebpf.plugin/ebpfgo.plugin/libbpfloader/process_libbpf.go:151">
P3: `SupportsCore` is an unused receiver method duplicating the package-level probe used by the loader. Remove this method and its non-libbpf counterpart to avoid carrying a redundant API.</violation>
</file>
<file name="src/collectors/ebpf.plugin/ebpfgo.plugin/main.go">
<violation number="1" location="src/collectors/ebpf.plugin/ebpfgo.plugin/main.go:193">
P1: When process per-PID collection is enabled alongside cachestat, cachestat still publishes the shared-memory segment even though process was elected owner. Its subsequent writes clear the process availability flag, so apps.plugin intermittently loses process charts. Gate cachestat publishing on the elected-owner state while continuing to update the shared store.</violation>
</file>
<file name="src/collectors/ebpf.plugin/ebpfgo.plugin/process_global.go">
<violation number="1" location="src/collectors/ebpf.plugin/ebpfgo.plugin/process_global.go:64">
P1: The Go collector removes the legacy `system.process_status` chart, so dashboards and alerts lose running-process and zombie counts. Restore `process_status` and publish `Forks-Clones` and `Exits-TaskClose` with its two dimensions.</violation>
<violation number="2" location="src/collectors/ebpf.plugin/ebpfgo.plugin/process_global.go:78">
P2: The new `system.task_error` chart replaces the legacy per-process/per-thread error dimensions with one aggregate `errors` dimension. Preserve the two legacy error dimensions so existing queries and alerts continue to receive their streams.</violation>
<violation number="3" location="src/collectors/ebpf.plugin/ebpfgo.plugin/process_global.go:126">
P1: The new `system.process_thread` chart collapses process and thread starts into one `calls` dimension. Existing consumers lose the process/thread breakdown and the legacy dimensions; publish separate `process` and `thread` dimensions instead.</violation>
</file>
<file name="src/collectors/ebpf.plugin/ebpfgo.plugin/process_shared_memory.go">
<violation number="1" location="src/collectors/ebpf.plugin/ebpfgo.plugin/process_shared_memory.go:54">
P2: The process path gates freshness and staleness on the raw BPF `app.Ct`, but every other module in this store replaces that value with a synthetic, boot-relative token laid on the counter movement (fd) because the CO-RE base/legacy objects write ct once at map-entry creation or never, and buffer/arena objects only advance it per observed event. With a frozen ct a PID is declared stale after processStaleCycles, loses its baseline (hasPrevious=false) and its rows publish zero deltas even when the process is active, until the next event that happens to advance ct; on a flavor fallback that means per-app process charts publish only zeros. Follow the ebpfFreshnessToken/ebpfModuleIdentity pattern used by fd/dcstat: stamp process rows with a synthetic store-wide token and derive staleness from counter movement instead of ct equality.</violation>
<violation number="2" location="src/collectors/ebpf.plugin/ebpfgo.plugin/process_shared_memory.go:56">
P2: After `miss` reaches `processStaleCycles`, this assignment keeps the PID in `processMiss`, so every later update returns it and invokes `PidIsAlive` again. Store the miss only below the threshold, resetting the debouncer after each stale candidate as the sibling stores do.</violation>
<violation number="3" location="src/collectors/ebpf.plugin/ebpfgo.plugin/process_shared_memory.go:131">
P3: `ProcessSnapshot` has no call site in the repository, so this new map-copy API is dead code and adds maintenance surface without affecting publication. Remove it until a consumer exists, or wire it into the intended consumer.</violation>
</file>
<file name="src/collectors/ebpf.plugin/ebpfgo.plugin/process_loader_other.go">
<violation number="1" location="src/collectors/ebpf.plugin/ebpfgo.plugin/process_loader_other.go:11">
P3: LoadProcessLegacy returns a bespoke fmt.Errorf while every sibling module stub (cachestat, dcstat, fd, socket, dns) returns libbpfloader.ErrDisabled, and the existing disabled-build tests (e.g. fd_runtime_disabled_test.go:48, dcstat_runtime_disabled_test.go:43) assert errors.Is(err, ErrDisabled). Return libbpfloader.ErrDisabled here for a consistent, testable disabled-build contract; the non-libbpf callers (main.go:161) only log the error, so the message is not needed.</violation>
<violation number="2" location="src/collectors/ebpf.plugin/ebpfgo.plugin/process_loader_other.go:16">
P3: processLoadFunction is never called anywhere — rg across src/ finds only its definition in this file and in process_loader_libbpf.go:13, and the real load path (tryLoadProcessPlan/LoadProcessLegacy) uses ProcessRuntimeOpenMode directly. This stub (and its libbpf counterpart) is dead code that mirrors a function the loader no longer uses; drop it from this file and the libbpf file unless a caller is wired up.</violation>
</file>
<file name="src/collectors/ebpf.plugin/ebpfgo.plugin/process_targets.go">
<violation number="1" location="src/collectors/ebpf.plugin/ebpfgo.plugin/process_targets.go:26">
P2: `resolveProcessTargets()` does no resolution: it unconditionally returns `defaultProcessTargets()` and `nil`, never performing the kernel-version split its own comment promises (`_do_fork` for < 5.9.16 vs `__do_sys_clone` otherwise). The `error` return and the whole function are therefore misleading. Either implement the kernel-version-based selection here or drop the stub and its comment so the code matches its actual behavior.</violation>
</file>
<file name="src/collectors/ebpf.plugin/ebpfgo.plugin/libbpfloader/loader.go">
<violation number="1" location="src/collectors/ebpf.plugin/ebpfgo.plugin/libbpfloader/loader.go:253">
P3: The new ProcessRuntime disabled-build stubs lack the test coverage that every sibling runtime has: cachestat_runtime_disabled_test.go, dcstat_runtime_disabled_test.go, dns_runtime_disabled_test.go, fd_runtime_disabled_test.go, and socket_runtime_disabled_test.go all verify the disabled stubs, but no process_runtime_disabled_test.go exists. Add one (mirroring e.g. cachestat_runtime_disabled_test.go with the !netdata_ebpf_libbpf tag) that asserts ProcessRuntimeOpenMode returns ErrDisabled and that Prepare/Load/Attach/UpdateController/Snapshot/SnapshotApps/DeletePids all return ErrDisabled.</violation>
</file>
<file name="src/collectors/ebpf.plugin/ebpfgo.plugin/process_loader_libbpf.go">
<violation number="1" location="src/collectors/ebpf.plugin/ebpfgo.plugin/process_loader_libbpf.go:49">
P2: When `btf path` points to a non-default directory, process CO-RE loading still uses libbpf’s default BTF path because this call never passes `cfg.BTFPath`; pass the configured vmlinux path into the process runtime.</violation>
</file>
<file name="src/collectors/ebpf.plugin/ebpfgo.plugin/libbpfloader/process_libbpf.c">
<violation number="1" location="src/collectors/ebpf.plugin/ebpfgo.plugin/libbpfloader/process_libbpf.c:103">
P2: When a CO-RE skeleton open fails, this branch leaks the partially opened skeleton before fallback continues. Destroy the selected skeleton, or close the opened object for legacy mode, before freeing `rt`.</violation>
<violation number="2" location="src/collectors/ebpf.plugin/ebpfgo.plugin/libbpfloader/process_libbpf.c:189">
P1: Legacy process objects use different program names than this lookup, so release-task and fork/clone hooks are skipped while attach still succeeds. Use the actual program names and select `_do_fork` versus `kernel_clone` for the target kernel; fail when a required hook cannot be attached.</violation>
<violation number="3" location="src/collectors/ebpf.plugin/ebpfgo.plugin/libbpfloader/process_libbpf.c:243">
P1: When event setup fails, both snapshot paths silently publish zero data because they ignore `process_setup_events()`'s error. Initialize the event transport during load or propagate the setup failure from both snapshot functions.</violation>
</file>
<file name="src/collectors/apps.plugin/apps_ebpf_shared_memory.c">
<violation number="1" location="src/collectors/apps.plugin/apps_ebpf_shared_memory.c:291">
P1: The new process accumulator omits the ct-regression reset that dcstat, fd, and cachestat all perform. After a Go plugin restart or SHM/map reset, `ct` regresses and `p->ebpf.process.ct <= p->ebpf_process_ct` stays true forever, permanently suppressing the process charts. Add the same reset used by the other modules: `if (p->ebpf.process.ct < p->ebpf_process_ct) p->ebpf_process_ct = 0;` before the gate.</violation>
</file>
<file name="src/collectors/ebpf.plugin/ebpfgo.plugin/process_plan.go">
<violation number="1" location="src/collectors/ebpf.plugin/ebpfgo.plugin/process_plan.go:8">
P3: `processDefaultBTFFile` and `processMaxPIDTableSize` are declared but never used. Remove the dead constants; if a max PID-table size is intentional, wire processMaxPIDTableSize into PidTableSize validation in applyCommonCollectorConfig.</violation>
</file>
<file name="src/collectors/apps.plugin/apps_output.c">
<violation number="1" location="src/collectors/apps.plugin/apps_output.c:440">
P2: The process charts are declared "incremental", but their source totals are zeroed every collection cycle in apps_ebpf_accumulate_process() and refilled with per-interval deltas. The established fd pattern keeps totals monotonic for incremental charts (zeroing would spike every point), while the per-cycle-zeroed dcstat module uses "absolute". Only one of these is correct: either keep the totals monotonic across cycles (as fd does) and keep "incremental", or keep the per-cycle zeroing (as dcstat does) and switch these charts to "absolute".</violation>
</file>
Architecture diagram
sequenceDiagram
participant Cfg as Config Files
participant Main as ebpf-go.plugin Main
participant Loader as Process Loader
participant Runtime as libbpf Runtime
participant Kernel as Kernel eBPF
participant Store as Shared Memory Store
participant SHM as POSIX SHM
participant Apps as apps.plugin
participant Netdata as Netdata Agent
Note over Cfg,Netdata: Process Monitoring via ebpf-go.plugin
Main->>Cfg: Read process.conf + ebpf.conf
Cfg-->>Main: Process config (enabled, apps, pid table)
Main->>Loader: LoadProcessLegacy(config)
Loader->>Loader: Check kernel version & reject list
Loader->>Runtime: Open object file (base/buffer/arena flavor)
alt libbpf build (netdata_ebpf_libbpf tag)
Runtime->>Runtime: Prepare (PID table size, maps per core)
Runtime->>Runtime: Load BPF programs
Runtime->>Runtime: Attach kprobes/tracepoints
Runtime->>Kernel: Attach to sched_process_exit/fork, release_task, kernel_clone
Runtime-->>Loader: Success
Loader-->>Main: ProcessLegacyHandle
else No libbpf build
Loader-->>Main: Error: requires libbpf tag
end
Main->>Main: Start process global collector goroutine
Main->>Main: Determine if SHM store is needed (apps or cgroups enabled)
loop Every update_every interval
Main->>Runtime: Snapshot (global counters)
Runtime->>Kernel: Read tbl_total_stats map
Kernel-->>Runtime: Aggregate exits, forks, clones, errors
Runtime-->>Main: Global snapshot
Note over Main: Compute deltas from previous cycle
Main->>Netdata: Publish system.process_thread, exit, task_close, task_error charts
alt Apps collection enabled
Main->>Runtime: SnapshotApps (per-PID data)
Runtime->>Kernel: Iterate tbl_pid_stats map
Kernel-->>Runtime: Per-PID process events
Runtime-->>Main: ProcessAppSnapshot list
Main->>Store: UpdateAppsProcess(snapshots)
Store->>Store: Track ct stagnation (stale PIDs)
alt Stale PIDs detected
Main->>Runtime: DeletePids(stale)
Runtime->>Kernel: Remove PID entries from BPF map
Main->>Store: RemoveProcessPIDs(dead)
end
alt This module owns SHM publishing
Main->>SHM: Open/create shared memory segment
Main->>Store: Publish to SHM with PROCESS flag
Store->>SHM: Write per-PID process rows
end
end
end
Note over Apps,Netdata: Consumer Side
Apps->>SHM: Read shared memory header (process flag set)
SHM-->>Apps: Per-PID process data + flag EBPFGO_SHM_FLAG_PROCESS
Apps->>Apps: Accumulate process per-target totals
Apps->>Netdata: Send ebpf_process_start, thread_start, task_exit, task_released, task_error charts
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if handle.AppsEnabled || handle.CgroupsEnabled { | ||
| cachestatStore = store | ||
| cachestatWillPublish = true | ||
| cachestatWillPublish = !processWillPublish |
Contributor
There was a problem hiding this comment.
P1: When process per-PID collection is enabled alongside cachestat, cachestat still publishes the shared-memory segment even though process was elected owner. Its subsequent writes clear the process availability flag, so apps.plugin intermittently loses process charts. Gate cachestat publishing on the elected-owner state while continuing to update the shared store.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/collectors/ebpf.plugin/ebpfgo.plugin/main.go, line 193:
<comment>When process per-PID collection is enabled alongside cachestat, cachestat still publishes the shared-memory segment even though process was elected owner. Its subsequent writes clear the process availability flag, so apps.plugin intermittently loses process charts. Gate cachestat publishing on the elected-owner state while continuing to update the shared store.</comment>
<file context>
@@ -159,7 +190,7 @@ func main() {
if handle.AppsEnabled || handle.CgroupsEnabled {
cachestatStore = store
- cachestatWillPublish = true
+ cachestatWillPublish = !processWillPublish
}
anyStarted = true
</file context>
| chart, dim string | ||
| value int64 | ||
| }{ | ||
| {"process_thread", "calls", p.Forks + p.Clones}, |
Contributor
There was a problem hiding this comment.
P1: The new system.process_thread chart collapses process and thread starts into one calls dimension. Existing consumers lose the process/thread breakdown and the legacy dimensions; publish separate process and thread dimensions instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/collectors/ebpf.plugin/ebpfgo.plugin/process_global.go, line 126:
<comment>The new `system.process_thread` chart collapses process and thread starts into one `calls` dimension. Existing consumers lose the process/thread breakdown and the legacy dimensions; publish separate `process` and `thread` dimensions instead.</comment>
<file context>
@@ -0,0 +1,241 @@
+ chart, dim string
+ value int64
+ }{
+ {"process_thread", "calls", p.Forks + p.Clones},
+ {"exit", "calls", p.Exits},
+ {"task_close", "calls", p.TaskClose},
</file context>
| algorithm: "incremental", | ||
| }, | ||
| { | ||
| id: "task_close", |
Contributor
There was a problem hiding this comment.
P1: The Go collector removes the legacy system.process_status chart, so dashboards and alerts lose running-process and zombie counts. Restore process_status and publish Forks-Clones and Exits-TaskClose with its two dimensions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/collectors/ebpf.plugin/ebpfgo.plugin/process_global.go, line 64:
<comment>The Go collector removes the legacy `system.process_status` chart, so dashboards and alerts lose running-process and zombie counts. Restore `process_status` and publish `Forks-Clones` and `Exits-TaskClose` with its two dimensions.</comment>
<file context>
@@ -0,0 +1,241 @@
+ algorithm: "incremental",
+ },
+ {
+ id: "task_close",
+ title: "Tasks released",
+ units: "calls/s",
</file context>
| uint32_t key = 0; | ||
| #ifdef PROCESS_HAS_CORE | ||
| if (rt->core && rt->flavor != PROCESS_BASE) { | ||
| if (!rt->rb && !rt->arena_state) process_setup_events(rt); |
Contributor
There was a problem hiding this comment.
P1: When event setup fails, both snapshot paths silently publish zero data because they ignore process_setup_events()'s error. Initialize the event transport during load or propagate the setup failure from both snapshot functions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/collectors/ebpf.plugin/ebpfgo.plugin/libbpfloader/process_libbpf.c, line 243:
<comment>When event setup fails, both snapshot paths silently publish zero data because they ignore `process_setup_events()`'s error. Initialize the event transport during load or propagate the setup failure from both snapshot functions.</comment>
<file context>
@@ -0,0 +1,333 @@
+ uint32_t key = 0;
+#ifdef PROCESS_HAS_CORE
+ if (rt->core && rt->flavor != PROCESS_BASE) {
+ if (!rt->rb && !rt->arena_state) process_setup_events(rt);
+ if (rt->rb) ring_buffer__poll(rt->rb, 0);
+ if (rt->arena_state) rt->arena_tail = nd_ebpf_arena_drain(rt->arena_state, rt->arena_tail, process_event_arena, rt);
</file context>
| } | ||
| } | ||
| #endif | ||
| const char *names[] = {"netdata_tracepoint_sched_process_exit", "netdata_tracepoint_sched_process_exec", "netdata_tracepoint_sched_process_fork", "netdata_release_task", "netdata_sys_clone"}; |
Contributor
There was a problem hiding this comment.
P1: Legacy process objects use different program names than this lookup, so release-task and fork/clone hooks are skipped while attach still succeeds. Use the actual program names and select _do_fork versus kernel_clone for the target kernel; fail when a required hook cannot be attached.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/collectors/ebpf.plugin/ebpfgo.plugin/libbpfloader/process_libbpf.c, line 189:
<comment>Legacy process objects use different program names than this lookup, so release-task and fork/clone hooks are skipped while attach still succeeds. Use the actual program names and select `_do_fork` versus `kernel_clone` for the target kernel; fail when a required hook cannot be attached.</comment>
<file context>
@@ -0,0 +1,333 @@
+ }
+ }
+#endif
+ const char *names[] = {"netdata_tracepoint_sched_process_exit", "netdata_tracepoint_sched_process_exec", "netdata_tracepoint_sched_process_fork", "netdata_release_task", "netdata_sys_clone"};
+ rt->links = callocz(5, sizeof(*rt->links));
+ if (!rt->links) return -1;
</file context>
| s.rebuildEntriesLocked() | ||
| } | ||
|
|
||
| // ProcessSnapshot is the public interface for retrieving process data. |
Contributor
There was a problem hiding this comment.
P3: ProcessSnapshot has no call site in the repository, so this new map-copy API is dead code and adds maintenance surface without affecting publication. Remove it until a consumer exists, or wire it into the intended consumer.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/collectors/ebpf.plugin/ebpfgo.plugin/process_shared_memory.go, line 131:
<comment>`ProcessSnapshot` has no call site in the repository, so this new map-copy API is dead code and adds maintenance surface without affecting publication. Remove it until a consumer exists, or wire it into the intended consumer.</comment>
<file context>
@@ -0,0 +1,147 @@
+ s.rebuildEntriesLocked()
+}
+
+// ProcessSnapshot is the public interface for retrieving process data.
+func (s *ebpfSharedMemoryStore) ProcessSnapshot() (map[uint32]netdataPublishProcess, map[uint32]ebpfModuleIdentity) {
+ s.mu.RLock()
</file context>
| return nil, fmt.Errorf("process collector requires libbpf build (netdata_ebpf_libbpf tag)") | ||
| } | ||
|
|
||
| func processLoadFunction(cfg ProcessLegacyConfig, plan LoadPlan) (*libbpfloader.ProcessRuntime, error) { |
Contributor
There was a problem hiding this comment.
P3: processLoadFunction is never called anywhere — rg across src/ finds only its definition in this file and in process_loader_libbpf.go:13, and the real load path (tryLoadProcessPlan/LoadProcessLegacy) uses ProcessRuntimeOpenMode directly. This stub (and its libbpf counterpart) is dead code that mirrors a function the loader no longer uses; drop it from this file and the libbpf file unless a caller is wired up.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/collectors/ebpf.plugin/ebpfgo.plugin/process_loader_other.go, line 16:
<comment>processLoadFunction is never called anywhere — rg across src/ finds only its definition in this file and in process_loader_libbpf.go:13, and the real load path (tryLoadProcessPlan/LoadProcessLegacy) uses ProcessRuntimeOpenMode directly. This stub (and its libbpf counterpart) is dead code that mirrors a function the loader no longer uses; drop it from this file and the libbpf file unless a caller is wired up.</comment>
<file context>
@@ -0,0 +1,18 @@
+ return nil, fmt.Errorf("process collector requires libbpf build (netdata_ebpf_libbpf tag)")
+}
+
+func processLoadFunction(cfg ProcessLegacyConfig, plan LoadPlan) (*libbpfloader.ProcessRuntime, error) {
+ return nil, fmt.Errorf("process collector requires libbpf build (netdata_ebpf_libbpf tag)")
+}
</file context>
| "github.com/netdata/netdata/src/collectors/ebpf.plugin/ebpfgo.plugin/libbpfloader" | ||
| ) | ||
|
|
||
| func LoadProcessLegacy(cfg ProcessLegacyConfig) (*ProcessLegacyHandle, error) { |
Contributor
There was a problem hiding this comment.
P3: LoadProcessLegacy returns a bespoke fmt.Errorf while every sibling module stub (cachestat, dcstat, fd, socket, dns) returns libbpfloader.ErrDisabled, and the existing disabled-build tests (e.g. fd_runtime_disabled_test.go:48, dcstat_runtime_disabled_test.go:43) assert errors.Is(err, ErrDisabled). Return libbpfloader.ErrDisabled here for a consistent, testable disabled-build contract; the non-libbpf callers (main.go:161) only log the error, so the message is not needed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/collectors/ebpf.plugin/ebpfgo.plugin/process_loader_other.go, line 11:
<comment>LoadProcessLegacy returns a bespoke fmt.Errorf while every sibling module stub (cachestat, dcstat, fd, socket, dns) returns libbpfloader.ErrDisabled, and the existing disabled-build tests (e.g. fd_runtime_disabled_test.go:48, dcstat_runtime_disabled_test.go:43) assert errors.Is(err, ErrDisabled). Return libbpfloader.ErrDisabled here for a consistent, testable disabled-build contract; the non-libbpf callers (main.go:161) only log the error, so the message is not needed.</comment>
<file context>
@@ -0,0 +1,18 @@
+ "github.com/netdata/netdata/src/collectors/ebpf.plugin/ebpfgo.plugin/libbpfloader"
+)
+
+func LoadProcessLegacy(cfg ProcessLegacyConfig) (*ProcessLegacyHandle, error) {
+ _ = cfg
+ return nil, fmt.Errorf("process collector requires libbpf build (netdata_ebpf_libbpf tag)")
</file context>
| // No-op in the disabled build because the runtime never acquired native resources. | ||
| } | ||
|
|
||
| func ProcessRuntimeOpenMode(path string, useCore bool) (*ProcessRuntime, error) { |
Contributor
There was a problem hiding this comment.
P3: The new ProcessRuntime disabled-build stubs lack the test coverage that every sibling runtime has: cachestat_runtime_disabled_test.go, dcstat_runtime_disabled_test.go, dns_runtime_disabled_test.go, fd_runtime_disabled_test.go, and socket_runtime_disabled_test.go all verify the disabled stubs, but no process_runtime_disabled_test.go exists. Add one (mirroring e.g. cachestat_runtime_disabled_test.go with the !netdata_ebpf_libbpf tag) that asserts ProcessRuntimeOpenMode returns ErrDisabled and that Prepare/Load/Attach/UpdateController/Snapshot/SnapshotApps/DeletePids all return ErrDisabled.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/collectors/ebpf.plugin/ebpfgo.plugin/libbpfloader/loader.go, line 253:
<comment>The new ProcessRuntime disabled-build stubs lack the test coverage that every sibling runtime has: cachestat_runtime_disabled_test.go, dcstat_runtime_disabled_test.go, dns_runtime_disabled_test.go, fd_runtime_disabled_test.go, and socket_runtime_disabled_test.go all verify the disabled stubs, but no process_runtime_disabled_test.go exists. Add one (mirroring e.g. cachestat_runtime_disabled_test.go with the !netdata_ebpf_libbpf tag) that asserts ProcessRuntimeOpenMode returns ErrDisabled and that Prepare/Load/Attach/UpdateController/Snapshot/SnapshotApps/DeletePids all return ErrDisabled.</comment>
<file context>
@@ -248,6 +250,40 @@ func (r *SocketRuntime) Close() {
// No-op in the disabled build because the runtime never acquired native resources.
}
+func ProcessRuntimeOpenMode(path string, useCore bool) (*ProcessRuntime, error) {
+ return newDisabledRuntime[ProcessRuntime](path, useCore)
+}
</file context>
| const processKernelMask uint32 = (1 << 12) - 1 | ||
| const processDefaultPIDTableSize uint32 = 32768 | ||
| const processMaxPIDTableSize uint32 = 32768 | ||
| const processDefaultBTFFile = "vmlinux" |
Contributor
There was a problem hiding this comment.
P3: processDefaultBTFFile and processMaxPIDTableSize are declared but never used. Remove the dead constants; if a max PID-table size is intentional, wire processMaxPIDTableSize into PidTableSize validation in applyCommonCollectorConfig.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/collectors/ebpf.plugin/ebpfgo.plugin/process_plan.go, line 8:
<comment>`processDefaultBTFFile` and `processMaxPIDTableSize` are declared but never used. Remove the dead constants; if a max PID-table size is intentional, wire processMaxPIDTableSize into PidTableSize validation in applyCommonCollectorConfig.</comment>
<file context>
@@ -0,0 +1,134 @@
+const processKernelMask uint32 = (1 << 12) - 1
+const processDefaultPIDTableSize uint32 = 32768
+const processMaxPIDTableSize uint32 = 32768
+const processDefaultBTFFile = "vmlinux"
+
+// processMaxBaseSelector is the highest SelectKernelName index for which a
</file context>
Contributor
There was a problem hiding this comment.
1 existing issue remains and 2 new issues found across 5 files (changes from recent commits).
Confidence score: 3/5
- In
src/collectors/ebpf.plugin/ebpfgo.plugin/process_loader_libbpf.go, the legacy retry may report success after attaching only tracepoints when the process probe names are missing, leaving process monitoring incomplete; require the expected legacy links or otherwise return failure. - In
src/collectors/ebpf.plugin/ebpfgo.plugin/process_loader_libbpf.go, a failed CO-RE attempt followed by a failed legacy retry can return the wrong diagnostic becauselegacyErris not preserved; retain it inlastErr. - In
src/collectors/ebpf.plugin/ebpfgo.plugin/process_plan_test.go, coverage does not verify the Apps-disabled/Cgroups-disabled case, so the base buffer flavor could regress unnoticed; add an assertion that the configured flavor is preserved.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/collectors/ebpf.plugin/ebpfgo.plugin/process_loader_libbpf.go">
<violation number="1" location="src/collectors/ebpf.plugin/ebpfgo.plugin/process_loader_libbpf.go:107">
P3: When both the CO-RE attempt and this legacy retry fail, preserve `legacyErr` in `lastErr`; otherwise callers receive the wrong failure and lose the useful legacy-path diagnostic.</violation>
</file>
<file name="src/collectors/ebpf.plugin/ebpfgo.plugin/process_plan_test.go">
<violation number="1" location="src/collectors/ebpf.plugin/ebpfgo.plugin/process_plan_test.go:10">
P3: The test only exercises the apps/cgroups override cases. Add a case with both AppsEnabled and CgroupsEnabled false that asserts the configured buffer flavor is preserved, so a regression that makes the base override unconditional would be caught.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
| if candidate.LoadMode == LoadCore { | ||
| legacy := candidate | ||
| legacy.LoadMode = LoadLegacy | ||
| if handle, legacyErr := tryLoadProcessPlan(cfg, legacy); legacyErr == nil { |
Contributor
There was a problem hiding this comment.
P3: When both the CO-RE attempt and this legacy retry fail, preserve legacyErr in lastErr; otherwise callers receive the wrong failure and lose the useful legacy-path diagnostic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/collectors/ebpf.plugin/ebpfgo.plugin/process_loader_libbpf.go, line 107:
<comment>When both the CO-RE attempt and this legacy retry fail, preserve `legacyErr` in `lastErr`; otherwise callers receive the wrong failure and lose the useful legacy-path diagnostic.</comment>
<file context>
@@ -97,6 +97,17 @@ func LoadProcessLegacy(cfg ProcessLegacyConfig) (*ProcessLegacyHandle, error) {
+ if candidate.LoadMode == LoadCore {
+ legacy := candidate
+ legacy.LoadMode = LoadLegacy
+ if handle, legacyErr := tryLoadProcessPlan(cfg, legacy); legacyErr == nil {
+ return handle, nil
+ }
</file context>
| apps bool | ||
| cgroups bool | ||
| }{ | ||
| "apps": {apps: true}, |
Contributor
There was a problem hiding this comment.
P3: The test only exercises the apps/cgroups override cases. Add a case with both AppsEnabled and CgroupsEnabled false that asserts the configured buffer flavor is preserved, so a regression that makes the base override unconditional would be caught.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/collectors/ebpf.plugin/ebpfgo.plugin/process_plan_test.go, line 10:
<comment>The test only exercises the apps/cgroups override cases. Add a case with both AppsEnabled and CgroupsEnabled false that asserts the configured buffer flavor is preserved, so a regression that makes the base override unconditional would be caught.</comment>
<file context>
@@ -0,0 +1,33 @@
+ apps bool
+ cgroups bool
+ }{
+ "apps": {apps: true},
+ "cgroups": {cgroups: true},
+ }
</file context>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test Plan
Additional Information
For users: How does this change affect me?
Summary by cubic
Moves process monitoring from the legacy C
ebpf.pluginto the Goebpf-go.plugin. Process eBPF data is now collected, charted, and published by the Go collector instead of the C one.apps.pluginreads per-PID process data from the Go plugin's shared memory and adds charts for process/thread starts, exits, task releases, and creation errors.netdata_ebpf_libbpftag; load-plan selection and shared-memory delta publishing are covered by unit tests.Written for commit 49d89cd. Summary will update on new commits.