feat: add --replay mode for JSONL event replay without eBPF#1010
Open
Molter73 wants to merge 4 commits into
Open
feat: add --replay mode for JSONL event replay without eBPF#1010Molter73 wants to merge 4 commits into
Molter73 wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesAdds Replay mode and task orchestration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as FactCli
participant Runtime as fact::run
participant Replay as replay::start
participant Tasks as JoinSet
participant Output as output::start
CLI->>Runtime: provide replay path
Runtime->>Replay: start JSONL reader
Replay->>Tasks: register replay task
Runtime->>Output: register output task
Replay-->>Runtime: send deserialized Event
Runtime->>Tasks: join_next()
Tasks-->>Runtime: completed task result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
6 tasks
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 @@
## main #1010 +/- ##
==========================================
- Coverage 33.22% 32.29% -0.93%
==========================================
Files 21 22 +1
Lines 2971 3090 +119
Branches 2971 3090 +119
==========================================
+ Hits 987 998 +11
- Misses 1981 2089 +108
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
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
Molter73
force-pushed
the
mauro/feat/replay-input
branch
from
0194aae to
896dab3
Compare
Molter73
force-pushed
the
mauro/feat/replay-input
branch
from
896dab3 to
b3dfb5a
Compare
Molter73
force-pushed
the
mauro/feat/replay-input
branch
3 times, most recently
from
835ac9f to
2b54c9a
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
2b54c9a to
49232f3
Compare
Collaborator
Author
|
There's a bit of cleanup around the metrics that will be addressed in #1118 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fact-ebpf/src/lib.rs (1)
108-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd JSON round-trip regression tests for the custom deserializers.
Replay consumes stdout-produced JSON, so test
serde_json::to_string→from_strforinode_key_tand everymonitored_tvariant, plus invalid/missing values. This protects the serialization contract from future drift.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 - 151, Add JSON round-trip regression tests covering inode_key_t and every monitored_t variant, using serde_json::to_string followed by from_str and asserting equality. Include invalid JSON and missing-field/value cases for the custom deserializers, covering the deserialize implementations for inode_key_t and monitored_t without changing their behavior.
🤖 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/lib.rs`:
- Around line 82-89: Update join_all_tasks to drain the JoinSet with repeated
join_next() calls, converting any JoinError into an anyhow::Error and
propagating task-level errors. Preserve successful completion as Ok(()) so the
shutdown call site can return failures instead of panicking.
---
Nitpick comments:
In `@fact-ebpf/src/lib.rs`:
- Around line 108-151: Add JSON round-trip regression tests covering inode_key_t
and every monitored_t variant, using serde_json::to_string followed by from_str
and asserting equality. Include invalid JSON and missing-field/value cases for
the custom deserializers, covering the deserialize implementations for
inode_key_t and monitored_t without changing their behavior.
🪄 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: 70765ba1-d5b9-4608-8f6c-0f3b470f21c4
📒 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/output/grpc.rsfact/src/output/mod.rsfact/src/output/otel.rsfact/src/output/stdout.rsfact/src/rate_limiter.rsfact/src/replay.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- CHANGELOG.md
- fact/src/event/process.rs
- fact/src/output/stdout.rs
- fact/src/replay.rs
- fact/src/config/mod.rs
- fact/src/config/reloader.rs
- fact/src/output/mod.rs
- fact/src/event/mod.rs
Also add `RUST_BACKTRACE=1` to the integration tests to get backtraces when fact errors out.
This prevents panicking in case a `JoinError` occurs.
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
2 participants
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.
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.
Additionally, in order to ensure all events are processed the shutdown sequence has changed from signalling all tasks to stop at once to stopping the input stage, then letting the shutdown propagate as downstream tasks find their receivers to have no more messages and being closed. To make this cleaner, sub-tasks like the periodic notifier for host_scanner and the different output components are now handled independently by their parent task.
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
--replay/FACT_REPLAY.--replaymode.