Repository navigation
fix: Use monotonic clocks for migration latency and FDv2 stream-init timing - #539
tanderson-ld wants to merge 1 commit into
Conversation
…timing Wall-clock steps (DST transitions via naive datetime.now(), NTP corrections, VM/container time resync) could report wildly wrong or negative migration-operation latencies, and could inflate or zero out the FDv2 stream-init durations reported in diagnostic events. Migration latency (sync and async) is now measured with a monotonic clock and delivered to the tracker as a timedelta, unchanged in type. FDv2 stream-init duration is measured monotonically from the real connection moment (the SSE Start action) rather than a wall-clock projection of the next attempt; the wall clock is kept only for the reported timestamp. The projected next-attempt baseline is removed, and the diagnostic accumulator now clamps durationMillis to a non-negative integer once for every caller. Teardown paths (clean close and unexpected event errors) clear the attempt baseline so a later reconnect failure cannot report a duration spanning the backoff sleep. SDK-3307
|
AI review comment: tracking a known, deliberately-deferred diagnostics gap introduced alongside this change, so it isn't lost. FDv2 failed stream-init records are undercounted during a sustained outageWhat. The FDv2 streaming synchronizers record a failed stream-init only when the connection attempt's start is observable. The initial connect's start is observable (stamped at Spec angle. The DIAG diagnostic-events spec calls for one stream-init entry per connection attempt, so this is a conformance gap, not merely reduced telemetry — and it undercounts precisely when failed-init data is most useful. Regression vs. pre-existing. This gap is introduced by this PR's move to observed- Scope / severity. Diagnostics-only. No impact on flag evaluation, data correctness, connection/retry behavior, or the FDv1-fallback latch. Medium as a spec-MUST gap; Low as pure observability. Options for a future fix.
Not addressed in this PR by design. |
|
Need to address pending issue related to undercounting. |
Summary
Replaces wall-clock time with monotonic time in the two places this SDK still measures a duration from one, found by the clock-monotonicity re-audit. Implements SDK-3307 (re-audit findings #39/#40/#41).
A wall-clock step (DST via naive
datetime.now(), NTP correction, VM/container time resync) previously could:migrations/migrator.py,async_migrator.py):datetime.now()is naive local time, so a DST transition reported ±1h (or negative) latency to LaunchDarkly, plus any NTP/manual step. Finding added config option for events_uri #39 (HIGH).datasourcev2/streaming.py,async_streaming.py): the duration was a wall-clock difference hidden behind anelapsed if elapsed >= 0 else 0clamp, and the next attempt's baseline was a projected future wall time (time() + next_retry_delay) that produced short/negative durations even without a clock step. Findings Send the toggle default value with flag request events #40 / coerce user keys to strings #41 (MED).Changes
Impl::util.monotonic_seconds()is the single monotonic seam (added in the sibling SDK-3105 work; reused here).monotonic_seconds()and delivered toOpTracker.latencyas atimedelta— identical type and, under a stable clock, identical value; a backward step can no longer produce a negative latency.Startaction) instead of a projection; the wall clock is kept only for the reportedtimestamp. The projectednext_attempt_startfield is removed fromStreamErrorDecision(andclassify_stream_errorno longer takesnext_retry_delay), and the teardown paths (clean close, unexpected event error) clear the attempt baseline so a later reconnect failure can't record a duration spanning the backoff sleep._DiagnosticAccumulator.record_stream_initclampsdurationMillisto a non-negative integer once, centrally, for every caller (the per-site clamp is removed).Deliberately unchanged (wall clock is correct):
DataSourceErrorInfo.time, eventcreationDate, diagnosticdataSinceDate, the big-segments cross-process staleness stamp.Behavioral notes
Not a breaking change — every modified component is
Impl-internal, and behavior under a stable clock is unchanged. Note for test suites that time-travel: these measurements no longer followtime.time();freezegundoes not patchtime.monotonic()by default.Known limitation tracked separately
There is a known FDv2 diagnostic-count gap (failed stream-init records undercounted during a sustained outage) that is not addressed here — see the tracking comment on this PR. It is diagnostics-only, affects no flag evaluation, data correctness, or retry behavior, and will be handled on its own.
Testing
Full suite passes (1775 passed, 371 DB-skipped). New tests: migration latency measured monotonically (sync + async, via a module-patched clock); FDv2 stream-init duration measured from the
Startaction rather than run-start or a projection; a clean-close-mid-init regression test proving no stale baseline survives into a later reconnect; and the diagnostic accumulator's non-negative clamp. mypy, isort, and pycodestyle clean.Same change-class as ruby-server-sdk#459 and the merged python PR #537.
Note
Overview
Switches duration measurements from wall clock to monotonic time so DST, NTP, and manual clock steps no longer corrupt telemetry.
Migration operations (sync and async executors) now time latency with
monotonic_seconds()and report atimedeltatoOpTracker.latencyinstead of naivedatetime.now()deltas.FDv2 streaming diagnostics record stream-init duration from the monotonic baseline stamped on each SSE
Start(wall clock is still used only for the diagnostic timestamp). The old projectednext_attempt_startpath is removed fromclassify_stream_error/StreamErrorDecision; error and clean-close paths clear the attempt baseline so reconnect/backoff cannot inflate the next measurement._DiagnosticAccumulator.record_stream_initclamps negative durations in one place.New tests cover monotonic migration latency, stream-init timing regressions, stale-baseline cleanup, and the diagnostic clamp.
Reviewed by Cursor Bugbot for commit 4cef86b. Bugbot is set up for automated code reviews on this repo. Configure here.