feat: add --replay mode for JSONL event replay without eBPF#1010
feat: add --replay mode for JSONL event replay without eBPF#1010Molter73 wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughAdds a ChangesReplay mode and task orchestration refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Molter73
force-pushed
the
mauro/feat/replay-input
branch
2 times, most recently
from
b4c135f to
68ec0c4
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## mauro/tests/rate-limit #1010 +/- ##
==========================================================
- Coverage 33.22% 32.26% -0.96%
==========================================================
Files 21 22 +1
Lines 2971 3093 +122
Branches 2971 3093 +122
==========================================================
+ Hits 987 998 +11
- Misses 1981 2092 +111
Partials 3 3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fact/src/event/mod.rs (1)
73-80: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
hostnamewhen replaying events.#[serde(skip_deserializing)]drops it to""onEventdeserialization, andfact/src/replay.rsforwards that value unchanged intofact_api::FileActivity. Replayed output loses the recorded host identity; add a deserialization default or repopulate it in the replay path if that’s not intended.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fact/src/event/mod.rs` around lines 73 - 80, The Event deserialization currently drops hostname to an empty value, which then gets forwarded unchanged by the replay path. Update the Event type’s serde handling so hostname is preserved or restored during deserialization, and verify the replay logic in replay.rs / FileActivity construction uses the recorded hostname instead of the blank default.
🧹 Nitpick comments (2)
fact-ebpf/src/lib.rs (1)
108-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider round-trip tests for the new Deserialize impls.
No tests exercise serialize→deserialize round trips for
inode_key_t/monitored_t, matching Codecov's coverage gap on this file. A couple of small unit tests would catch drift between the manualSerialize/Deserializeimpls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fact-ebpf/src/lib.rs` around lines 108 - 194, Add small unit tests for the new manual serde implementations in inode_key_t and monitored_t to cover serialize-to-deserialize round trips. Use the existing Serialize/Deserialize impls for inode_key_t and monitored_t to verify that a value serialized to JSON (or equivalent serde format) deserializes back to the same value, including at least one case for each monitored_t variant and a representative inode_key_t value. Place the tests near the serde impls in lib.rs so they clearly exercise these symbols and prevent drift between the custom serialization and deserialization logic.fact/src/replay.rs (1)
13-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo tests for the replay reader.
Matches Codecov's coverage gap for this file. A test spawning
start()against a small temp JSONL (including a malformed line) would validate the parse/skip/forward behavior cheaply.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fact/src/replay.rs` around lines 13 - 61, Add coverage for replay::start by introducing a test that spawns it with a temporary JSONL file and a watch receiver, then verifies the reader forwards valid Event entries, skips malformed JSON lines, and continues after deserialize failures. Use the start function and its line-by-line parsing path in replay.rs so the test exercises the existing parse/skip/forward behavior end to end.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fact/src/config/mod.rs`:
- Line 44: The new replay field in the configuration schema is missing dedicated
test coverage. Update fact/src/config/tests.rs to add cases that parse a YAML
replay: <path> value, exercise the replay() accessor, and verify update() merges
a Some(path) value correctly; use the existing Config and update() test patterns
as the reference point. Also extend the fully specified config fixtures that
already include replay: None so they cover the new field consistently, but make
sure there is at least one explicit test for parsing and merging replay.
In `@fact/src/event/process.rs`:
- Around line 41-48: The Process.username field is only marked
skip_deserializing, so replayed events keep the default empty value and never
get repopulated. Update the Process struct’s serde handling to mirror the
Event.hostname fix by adding a default provider (or equivalent reassignment
path) for username in fact/src/event/process.rs, and ensure the
replay/deserialization flow restores a usable username instead of leaving it
blank.
In `@fact/src/replay.rs`:
- Around line 13-61: The replay task in start() is being treated as a global
shutdown signal when it reaches EOF, which can stop downstream consumers before
their bounded queues are drained. Update the replay completion path so finishing
the file does not flip the shared running flag in run(); instead, keep replay
completion separate from shutdown or add an explicit drain/flush step for
RateLimiter and Output before exit. Use the start() task and the
run()/running.changed() shutdown flow to locate the fix.
---
Outside diff comments:
In `@fact/src/event/mod.rs`:
- Around line 73-80: The Event deserialization currently drops hostname to an
empty value, which then gets forwarded unchanged by the replay path. Update the
Event type’s serde handling so hostname is preserved or restored during
deserialization, and verify the replay logic in replay.rs / FileActivity
construction uses the recorded hostname instead of the blank default.
---
Nitpick comments:
In `@fact-ebpf/src/lib.rs`:
- Around line 108-194: Add small unit tests for the new manual serde
implementations in inode_key_t and monitored_t to cover serialize-to-deserialize
round trips. Use the existing Serialize/Deserialize impls for inode_key_t and
monitored_t to verify that a value serialized to JSON (or equivalent serde
format) deserializes back to the same value, including at least one case for
each monitored_t variant and a representative inode_key_t value. Place the tests
near the serde impls in lib.rs so they clearly exercise these symbols and
prevent drift between the custom serialization and deserialization logic.
In `@fact/src/replay.rs`:
- Around line 13-61: Add coverage for replay::start by introducing a test that
spawns it with a temporary JSONL file and a watch receiver, then verifies the
reader forwards valid Event entries, skips malformed JSON lines, and continues
after deserialize failures. Use the start function and its line-by-line parsing
path in replay.rs so the test exercises the existing parse/skip/forward behavior
end to end.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 9570c942-4422-4544-b4e3-40dde97bdb37
📒 Files selected for processing (14)
CHANGELOG.mdfact-ebpf/src/lib.rsfact/src/bpf/mod.rsfact/src/config/mod.rsfact/src/config/reloader.rsfact/src/config/tests.rsfact/src/event/mod.rsfact/src/event/process.rsfact/src/host_scanner.rsfact/src/lib.rsfact/src/metrics/exporter.rsfact/src/output/mod.rsfact/src/rate_limiter.rsfact/src/replay.rs
| #[derive(Debug, Clone, Default, Serialize, Deserialize)] | ||
| pub struct Process { | ||
| comm: String, | ||
| args: Vec<String>, | ||
| exe_path: PathBuf, | ||
| container_id: Option<String>, | ||
| uid: u32, | ||
| #[serde(skip_deserializing)] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Same skip-deserializing gap as Event.hostname — username is always blank on replay.
Same root cause as the Event.hostname field flagged in fact/src/event/mod.rs: this field will deserialize to "" for every replayed event with no reassignment happening. Worth applying the same fix (e.g., a default = "..." fn) if a live-username lookup is available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fact/src/event/process.rs` around lines 41 - 48, The Process.username field
is only marked skip_deserializing, so replayed events keep the default empty
value and never get repopulated. Update the Process struct’s serde handling to
mirror the Event.hostname fix by adding a default provider (or equivalent
reassignment path) for username in fact/src/event/process.rs, and ensure the
replay/deserialization flow restores a usable username instead of leaving it
blank.
There was a problem hiding this comment.
@CodeRabbit, when replaying events, the hostname and username are really not that important when replaying, for the time being we can keep them empty. In the future, if we want to change this behavior we can change the types to String and parse them directly from the JSON line.
There was a problem hiding this comment.
@Molter73, understood — that makes sense for the replay use case. Thanks for the clarification, I won't flag this further for this PR.
✏️ Learnings added
Learnt from: Molter73
Repo: stackrox/fact PR: 1010
File: fact/src/event/process.rs:41-48
Timestamp: 2026-07-14T10:33:28.625Z
Learning: In `fact/src/event/process.rs` and `fact/src/event/mod.rs` (fact repo, Rust), `Process.username` and `Event.hostname` are intentionally left empty (`skip_deserializing`) when replaying events from a JSONL file via the `--replay` CLI mode, since these fields are not important for replay/profiling purposes. If this needs to change in the future, the maintainer (Molter73) noted the types could be changed to `String` and parsed directly from the JSON line instead of relying on live host lookups.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Molter73
force-pushed
the
mauro/feat/replay-input
branch
2 times, most recently
from
c0267d4 to
0194aae
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fact/src/lib.rs (1)
89-148: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDrain the outer
JoinSeton normal shutdown (fact/src/lib.rs:137-146)
run()sendsrunning = falseand returns immediately on Ctrl-C/SIGTERM whenis_replayis false, so droppingtask_setaborts still-running pipeline tasks before they can flush buffered events. Drain the set after signaling shutdown on the normal path too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fact/src/lib.rs` around lines 89 - 148, Update run() to drain the outer task_set after running.send(false) on normal shutdown, including Ctrl-C and SIGTERM when is_replay is false, awaiting each task and propagating errors through flatten_task_result. Preserve the existing replay draining behavior while ensuring task_set is not dropped until all pipeline tasks have finished flushing.
🧹 Nitpick comments (1)
fact-ebpf/src/lib.rs (1)
108-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused deserialization tests.
Please cover stdout→replay round trips, missing
inode/devfields, unknown map keys, allmonitored_tvariants, and rejection of unknown variants. These implementations are directly exercised byserde_json::from_value::<Event>in replay mode, but the supplied coverage report shows this file lacks coverage.Also applies to: 176-193
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fact-ebpf/src/lib.rs` around lines 108 - 150, Add focused serde deserialization tests around inode_key_t’s Deserialize implementation and the Event replay deserialization path: verify stdout-to-replay round trips, missing inode and dev errors, ignored unknown map keys, every monitored_t variant, and rejection of unknown variants. Use serde_json::from_value::<Event> for replay-mode coverage and keep the tests scoped to these behaviors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@fact/src/lib.rs`:
- Around line 89-148: Update run() to drain the outer task_set after
running.send(false) on normal shutdown, including Ctrl-C and SIGTERM when
is_replay is false, awaiting each task and propagating errors through
flatten_task_result. Preserve the existing replay draining behavior while
ensuring task_set is not dropped until all pipeline tasks have finished
flushing.
---
Nitpick comments:
In `@fact-ebpf/src/lib.rs`:
- Around line 108-150: Add focused serde deserialization tests around
inode_key_t’s Deserialize implementation and the Event replay deserialization
path: verify stdout-to-replay round trips, missing inode and dev errors, ignored
unknown map keys, every monitored_t variant, and rejection of unknown variants.
Use serde_json::from_value::<Event> for replay-mode coverage and keep the tests
scoped to these behaviors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: cddb4ea2-ff9b-46e9-b1a1-a2ee710806be
📒 Files selected for processing (17)
CHANGELOG.mdfact-ebpf/src/lib.rsfact/src/bpf/mod.rsfact/src/config/mod.rsfact/src/config/reloader.rsfact/src/config/tests.rsfact/src/event/mod.rsfact/src/event/process.rsfact/src/host_scanner.rsfact/src/lib.rsfact/src/metrics/exporter.rsfact/src/metrics/kernel_metrics.rsfact/src/metrics/mod.rsfact/src/output/mod.rsfact/src/output/stdout.rsfact/src/rate_limiter.rsfact/src/replay.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- CHANGELOG.md
- fact/src/replay.rs
- fact/src/config/tests.rs
- fact/src/rate_limiter.rs
- fact/src/host_scanner.rs
- fact/src/event/process.rs
- fact/src/event/mod.rs
- fact/src/config/mod.rs
- fact/src/output/mod.rs
Add a few missing tests for the `rate_limit` configuration value.
Molter73
force-pushed
the
mauro/feat/replay-input
branch
from
0194aae to
896dab3
Compare
Add a --replay <file> CLI flag that reads pre-recorded events from a JSONL file instead of loading eBPF programs. This allows profiling the entire userspace pipeline with tools like valgrind DHAT that don't support BPF syscalls. In replay mode, the BPF loader and HostScanner are bypassed entirely. Events flow directly from the JSONL reader through the RateLimiter and Output stages. Changes: - Add Deserialize impls for inode_key_t and monitored_t (fact-ebpf) - Add Deserialize derives to Event, FileData, Process and related types - Make kernel metrics optional in Exporter (None in replay mode) - Migrate task handles to a shared JoinSet for cleaner lifecycle mgmt - Add replay module with async JSONL file reader - Add --replay flag to CLI and config Assisted-by: `agent-mode` <noreply@opencode.ai>
Molter73
force-pushed
the
mauro/feat/replay-input
branch
from
896dab3 to
b3dfb5a
Compare
2 participants
Description
Add a --replay CLI flag that reads pre-recorded events from a JSONL file instead of loading eBPF programs. This allows profiling the entire userspace pipeline with tools like valgrind DHAT that don't support BPF syscalls.
In replay mode, the BPF loader and HostScanner are bypassed entirely. Events flow directly from the JSONL reader through the RateLimiter and Output stages.
Changes:
Assisted-by: claude-opus-4.6 noreply@opencode.ai
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
Run fact in stdout mode redirecting output to a file, then used that file to replay events on a mock server.
Summary by CodeRabbit
--replaymode to replay events from a JSONL file (skips live BPF/host scanning).