feat(capture): make tick duration configurable #1727
Draft
Conversation
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
Add configurable sampling frequency to CaptureManager via tick_duration_ms parameter. Previously hardcoded to 1000ms (1Hz), now supports arbitrary intervals (e.g., 100ms for 10Hz, 10ms for 100Hz). Changes: - Add CaptureManagerBuilder for ergonomic configuration - Add SignalWatchers struct to group required signal watchers - Add tick_duration_ms parameter to all constructors - Export DEFAULT_TICK_DURATION_MS constant (1000ms) - Standardize tick_duration_ms type to u64 everywhere Breaking changes: - Constructor signatures changed to use SignalWatchers - new_with_format, new_jsonl, new_parquet, new_multi all have new signatures Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update the main lading binary to use SignalWatchers struct and the new constructor signature with tick_duration_ms parameter. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update the fuzz_capture_harness binary to use SignalWatchers struct and the new constructor signature with tick_duration_ms parameter. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
blt
reviewed
Jan 26, 2026
Collaborator
blt
left a comment
blt
left a comment
There was a problem hiding this comment.
Cool, some thoughts on how we thread the needle here but I like where this is headed.
Comment on lines
+30
to
+33
| /// Default duration of a single `Accumulator` tick in milliseconds. | ||
| /// This constant is kept for backwards compatibility in tests. | ||
| #[cfg(test)] | ||
| pub(crate) const DEFAULT_TICK_DURATION_MS: u128 = 1_000; |
Collaborator
There was a problem hiding this comment.
The tests need to vary their tick rate to be valid, else we'll only assert that the default tick is square to what we want. We can drop this constant and add a variable tick parameter to the tests.
Comment on lines
+34
to
+36
| /// Default duration of a single `Accumulator` tick in milliseconds. | ||
| /// Can be overridden via [`CaptureManagerBuilder::tick_duration_ms`]. | ||
| pub const DEFAULT_TICK_DURATION_MS: u64 = 1_000; |
Collaborator
There was a problem hiding this comment.
Since we're moving this to a configurable item I'd prefer to see us drop these constants and rely solely on the config -> struct field path. That makes converting the tests to have variable tick time more tractable as well.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What does this PR do?
Add configurable sampling frequency to
CaptureManagerviatick_duration_msparameter. Previously hardcoded to 1000ms (1Hz), now supports arbitrary intervals (e.g., 100ms for 10Hz, 10ms for 100Hz).Changes:
CaptureManagerBuilderfor ergonomic configurationSignalWatchersstruct to group required signal watcherstick_duration_msparameter to all constructorsDEFAULT_TICK_DURATION_MSconstant (1000ms)tick_duration_mstype tou64everywhereMotivation
Being able to capture signal at higher frequency than 1Hz.
Additional Notes
Breaking changes: