Repository navigation
fix(nodes): calculate analytics over available retained history - #494
Merged
Merged
Conversation
Owner
Author
Review — CS-Coordinator PR#494 — head e1fd75dVerdict: REQUEST CHANGES. One medium finding, and the rest is optional or informational. The fix addresses a real defect. Rates were divided by the requested 30 or 365 days even when only a few days are held in memory. The approach is sound:
Findings
Before mergeAfter fix 1, run on staging for a few hours and check that |
…late observations (#485) Routine eviction moved coverage.from to the latest observation of each evicted tx, so a re-heard old tx (up to ~2 days late in production) made availability and packets/day jump on every eviction tick. Packet membership is by FirstSeen, so the edge is now the evicted tx's FirstSeen (+1 ns). Late observations of evicted txs are counted in coverage.late_evicted_observations as a known, bounded gap and never move from. Unparseable FirstSeen and capped/failed/loading paths stay conservative. Also add a canonical-UTC (YYYY-MM-DDTHH:MM:SSZ) fast path to parseNodeAnalyticsTime with time.Parse fallback, guarded by a parity test. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
dborup
marked this pull request as ready for review
October 16, 2026 12:34
This was referenced Oct 16, 2026
Closed
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
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.
Summary
Relates to #485.
Calculate node availability and packets/day over the available retained-history interval rather than diluting a short in-memory history across the entire requested 30/365 days. Full analytics and the lightweight summary share the same coverage snapshot and calculation.
coveragemetadata while preserving existing response fields.FirstSeenvalues are not evidence of historical coverage.coverage.fromto the evicted transmission'sFirstSeen(the eviction boundary), never to a late re-hearing. Late observations of evicted transmissions are counted incoverage.late_evicted_observations(known, bounded gap).Regression and review evidence
The first regression failed on unchanged production: 48 hours containing 48 packets were reported as 1.6 packets/day for a requested month rather than 24.
Deterministic tests cover full/summary parity, complete ranges, UTC and non-UTC clocks, exact boundaries, weighted partial hours, empty/unknown history, startup/background transitions, real retained-window loading, old re-heard transmissions, caps, eviction and SQLite count/cursor/scan failures.
Independent review found and resolved three edge cases before publication:
The final independent review is APPROVE with no remaining findings on tree
93e69468baabe9e750ab295df9c946548e759aa3. The reviewer's unchanged handoff reproduction and all coverage regressions pass with-race(6.051 s).Verification
-race; vet, gofmt and diff checks are clean.GitHub/Linux CI is separate evidence and will be reported after the new PR run.
Performance and limits
The constant-sized coverage snapshot is O(1) (~133 ns/op, 64 B/op, 2 allocs). Evidence is reduced during existing load/ingest loops; there is no added per-request SQL or complete-store history scan. Endpoint aggregation remains O(packets indexed to the requested node).
Round 2 adds a canonical-UTC (
YYYY-MM-DDTHH:MM:SSZ) fast path toparseNodeAnalyticsTime(fallback totime.Parse, parity-tested). Per timestamp: ~29.6 ns (fast) vs ~42.4 ns (time.Parse), 0 allocs both (BenchmarkParseNodeAnalyticsTime485).Endpoint benchmark, same machine and flags, 1 s, 3 runs, medians (Linux amd64, 4 CPUs; run-to-run spread about 2-3%):
The end-to-end gain is within noise (about 1-2%). Profiling shows timestamp parsing is now ~4.5% of the summary request; the dominant cost is per-packet JSON decoding in the accumulator, which this PR does not touch. The earlier round-1 +12-14% (about 2.3 ms at 30K) therefore largely remains and is not claimed as recovered. It is the price of correct, validated UTC handling and should be weighed at merge review. This is a correctness change, not a performance optimization.
Coverage is deliberately conservative. A capped, failed or unverifiably incomplete startup discards uncertified historical intervals and uses subsequent supported evidence instead of guessing. The standard E2E fixture exposes an existing loader limitation: seeded negative transmission IDs are counted as eligible but omitted by its legacy cursor. The new metadata therefore reports partial/unverified history and can show zero historical rates on that fixture. This PR does not hide that mismatch, change node-membership semantics, or silently alter the loader's ID behavior. Repairing that loader limitation is separate work.
Status: draft, auto-merge disabled. Round 2 review fixes are pushed; readiness, merge and staging validation (check that
coverage.fromfollows the oldest in-memory packet across eviction ticks) are left to the maintainers.Attribution
Reference adapted to the fork's full/summary API and loader lifecycle: