[RFC] Remove quartz/clock from silence and nflog, and drop the dependency#5376
[RFC] Remove quartz/clock from silence and nflog, and drop the dependency#5376ultrotter wants to merge 2 commits into
Conversation
This is only used by tests, and we can instead use testing/synctest to get stable mocked time and time.Sleep for instant time advancement in testing/synctest. Add synctest to TestSilencesSnapshot as it needs stable times even if it doesn't advance them Signed-off-by: Guido Trotter <guido@hudson-trading.com>
Signed-off-by: Guido Trotter <guido@hudson-trading.com>
📝 WalkthroughWalkthroughRemoves the quartz clock dependency from silence and nflog code, updates benchmarks and tests to use real wall-clock time, and adds a ChangesQuartz clock removal and synctest migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Maintenance
participant Ticker as time.Ticker
participant Log as nflog.Log
participant Silences
Maintenance->>Ticker: time.NewTicker(interval)
Ticker-->>Maintenance: tick
Maintenance->>Silences: run maintenance
Maintenance->>Log: time.Since(start) for completion log
Related Issues: None specified. Related PRs: None specified. Suggested labels: testing, refactor, dependencies Suggested reviewers: None specified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
silence/silence_bench_test.go (1)
140-140: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueInconsistent expiry-window margins after switching to real time.
benchmarkMutes/benchmarkMutesParallelhad theirEndsAtwindows bumped from 1 minute to 24 hours when moving off the mock clock (Lines 95, 107, 511), presumably to avoid silences expiring mid-run under real wall-clock time.BenchmarkMutesIncremental,benchmarkQuery,benchmarkQueryParallel, andbenchmarkQueryWithConcurrentAddsfollow the identical pattern (computenowonce, then loop withb.Loop()/b.RunParallel()for the benchtime duration) but were left with only a 1-hour window (Lines 163, 173, 216, 227, 282, 347, 426, 461). With real time now driving expiry, a sufficiently long-benchtime/-countrun (e.g., CI stability runs) could let these silences actually expire mid-benchmark, silently skewingQState(SilenceStateActive)results.Consider extending these windows to match the 24-hour margin applied elsewhere for consistency and to remove this residual flakiness risk.
Also applies to: 163-163, 173-173, 216-216, 227-227, 261-261, 282-282, 328-328, 347-347, 407-407, 426-426, 461-461
🤖 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 `@silence/silence_bench_test.go` at line 140, Extend the silence expiry windows in the benchmark helpers that use a single cached time value, since `BenchmarkMutesIncremental`, `benchmarkQuery`, `benchmarkQueryParallel`, and `benchmarkQueryWithConcurrentAdds` still use a 1-hour `EndsAt` margin while the real-time benchmarks elsewhere were widened to 24 hours. Update the `EndsAt` calculations in those benchmark setup blocks to match the 24-hour buffer used by `benchmarkMutes`/`benchmarkMutesParallel`, keeping the existing `now := time.Now()` pattern intact so `QState(SilenceStateActive)` remains stable across long benchmark runs.
🤖 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.
Nitpick comments:
In `@silence/silence_bench_test.go`:
- Line 140: Extend the silence expiry windows in the benchmark helpers that use
a single cached time value, since `BenchmarkMutesIncremental`, `benchmarkQuery`,
`benchmarkQueryParallel`, and `benchmarkQueryWithConcurrentAdds` still use a
1-hour `EndsAt` margin while the real-time benchmarks elsewhere were widened to
24 hours. Update the `EndsAt` calculations in those benchmark setup blocks to
match the 24-hour buffer used by `benchmarkMutes`/`benchmarkMutesParallel`,
keeping the existing `now := time.Now()` pattern intact so
`QState(SilenceStateActive)` remains stable across long benchmark runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0fd80bec-27c6-4118-ac50-c6645df8d5ae
📒 Files selected for processing (4)
silence/silence.gosilence/silence_bench_test.gosilence/silence_test.gotest/util.go
This is only used by tests, and we can instead use testing/synctest to get stable mocked time and time.Sleep for instant time advancement in testing/synctest.
Add synctest to TestSilencesSnapshot as it needs stable times even if it doesn't advance them
Pull Request Checklist
Please check all the applicable boxes.
Which user-facing changes does this PR introduce?