feat: track child processes #168
Conversation
not-matthias
force-pushed
the
cod-1744-child-processes-are-not-visible-in-the-flamegraphs
branch
from
1f6367f to
0ecb974
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds child process tracking functionality to the CodSpeed runner by introducing a new exectrack crate that uses eBPF to monitor process creation, execution, and termination events. The changes also refactor shared BPF utilities into a new codspeed-bpf crate to avoid code duplication between memtrack and exectrack.
Key changes:
- New
codspeed-bpfcrate provides shared BPF utilities including process tracking, ring buffer polling, and probe attachment macros - New
exectrackcrate tracks process hierarchies using eBPF tracepoints (sched_process_fork,sched_process_exec,sched_process_exit) - Runner integration wraps benchmark commands with
codspeed-exectrackto capture process execution trees
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
crates/codspeed-bpf/* |
New shared library providing common BPF functionality (process tracking, polling, macros, build utilities) |
crates/exectrack/* |
New crate for tracking process execution hierarchies via eBPF |
crates/runner-shared/src/artifacts/process_hierarchy.rs |
Data structures for representing process hierarchies |
crates/memtrack/* |
Refactored to use shared utilities from codspeed-bpf |
src/run/runner/wall_time/executor.rs |
Integrates exectrack by wrapping commands with codspeed-exectrack |
.github/workflows/ci.yml |
Updates test exclusions for new BPF crates |
Cargo.toml |
Adds new workspace members |
Comments suppressed due to low confidence (1)
crates/codspeed-bpf/src/poller.rs:24
- The documentation mentions type parameters T and M, but the doc comment formatting could be improved. The "Type Parameters" section should use standard rustdoc format with
* \T`instead of* T` to properly render the type parameter as code.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Return a dummy thread handle | ||
| let thread_handle = std::thread::spawn(|| {}); |
There was a problem hiding this comment.
A dummy thread handle is created and returned but serves no meaningful purpose. The return type should be simplified to just return the ProcessHierarchy, or the thread_handle parameter should be removed from all test functions since it's never used meaningfully (all tests just call join() on it immediately).
| echo "CODSPEED_MEMTRACK_BINARY=$(which codspeed-memtrack)" >> $GITHUB_ENV | ||
| - run: cargo test --all --exclude memtrack | ||
| - run: cargo test --all --exclude memtrack exectrack |
There was a problem hiding this comment.
The CI workflow excludes both 'memtrack' and 'exectrack' from tests, but the syntax is incorrect. The command should be --exclude memtrack --exclude exectrack (with two separate --exclude flags) instead of --exclude memtrack exectrack.
| - run: cargo test --all --exclude memtrack exectrack | |
| - run: cargo test --all --exclude memtrack --exclude exectrack |
| #[cfg(not(feature = "ebpf"))] | ||
| fn main() {} |
There was a problem hiding this comment.
The conditional compilation for main() is placed after the main implementation rather than before. For clarity, the #[cfg(not(feature = "ebpf"))] empty implementation should be defined first, followed by the #[cfg(feature = "ebpf")] version (or the cfg attribute should be on the first main function).
| let (_env_file, _script_file, cmd_builder) = | ||
| WallTimeExecutor::walltime_bench_cmd(config, run_data)?; | ||
|
|
||
| let cmd_builder = Self::wrap_with_exectrack(cmd_builder, &run_data.profile_folder)?; |
There was a problem hiding this comment.
The wrap_with_exectrack function is called unconditionally, which means the runner will always try to execute the codspeed-exectrack binary. This will fail if the binary is not installed or not in PATH. There should be a configuration flag to enable/disable process tracking, similar to how enable_perf is used for the perf runner, or the code should handle the case where exectrack is not available.
| @@ -0,0 +1 @@ | |||
| CLAUDE.md No newline at end of file | |||
There was a problem hiding this comment.
The AGENTS.md file contains only "CLAUDE.md" as its content. This appears to be a reference or pointer file, but its purpose is unclear. If this is meant to redirect to CLAUDE.md, it should either contain proper markdown content explaining the reference or use a more standard approach like a symlink.
| EventType::Exit => { | ||
| // Update process metadata with exit info | ||
| if let Some(metadata) = self.hierarchy.processes.get_mut(&pid) { | ||
| metadata.exit_code = Some(event.tid as i32); // Exit code in tid field |
There was a problem hiding this comment.
The comment "Exit code in tid field" is misleading. The exit code should be extracted from the event properly, not simply cast from the tid field. This appears to be a workaround and should be documented more clearly about why the exit code is stored in the tid field, or the event structure should be designed to have a proper exit_code field.
| metadata.exit_code = Some(event.tid as i32); // Exit code in tid field | |
| // Use event.exit_code() to extract the exit code. | |
| // If the exit code is currently stored in the tid field, event.exit_code() should encapsulate this logic. | |
| // TODO: Update Event struct to have a proper exit_code field if possible. | |
| metadata.exit_code = Some(event.exit_code()); |
No description provided.