fix(aggregate): clamp window duration to minimum of one second#1772
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7ef81589a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Binary Size Analysis (Agent Data Plane)Baseline: 7e8ab3a · Comparison: 2bad6b8 · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
This comment has been minimized.
This comment has been minimized.
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (35)Experiments configured
Bounds Checks: ✅ Passed (5)
ExplanationA change is flagged as a regression when |Δ mean %| > 5.00% in the regressing direction for its optimization goal AND SMP marks the experiment as a regression ( |
| | `aggregate_flush_open_windows` | Flush open windows on stop | | | ||
| | `aggregate_passthrough_idle_flush_timeout` | Passthrough buffer flush delay | | | ||
| | `aggregate_window_duration` | Aggregation window size | | | ||
| | `aggregate_window_duration_seconds` | Aggregation window size | | |
There was a problem hiding this comment.
Nit, whitespace is misaligned now.
Summary
This PR fixes a bug where sub-second window durations would cause the Aggregate transform to panic during sample insertion.
Since we take the window duration as an actual
Durationfield, we allow for sub-second aggregation windows.. but we also use whole seconds when calculating which bucket to drop a sample into, which means that if our bucket window is sub-second, our whole second math breaks down quickly, leading to division by zero and ultimately panics.In this PR, we've moved entirely to whole seconds for the window duration, and backed that up with switching to
NonZeroU64. This solves both the fractional second and "less than one" problem in one fell swoop.Change Type
How did you test this PR?
Existing tests: unit, integration, correctness, etc.
References
DADP-2