tidaldb/.sdlc/features/pg1-instrumented-metrics/review.md

pg1-instrumented-metrics: Code Review

Summary

Implementation adds 5 new metric capabilities to tidalDB's existing metrics pipeline:

1. Percentile extraction (p50/p95/p99) from cumulative histograms
2. Per-signal-type write counters
3. Personalization staleness tracking
4. Feedback-loop latency measurement
5. /diagnostics JSON endpoint

Files Changed

File	Lines Added	Description
tidal/src/db/metrics/histogram.rs	~130	total_count(), percentile(), render_percentile_gauges() + 7 tests
tidal/src/db/metrics/mod.rs	~200	UserSignalTimestampMap, 4 new fields, render_diagnostics(), prometheus additions
tidal/src/db/signals.rs	~15	Per-type counter increment, user timestamp recording
tidal/src/db/query_ops.rs	~50	Staleness + feedback-loop in retrieve() and search()
tidal/src/db/http.rs	~20	/diagnostics route
tidal/tests/pg1_instrumented_metrics.rs	~190	4 integration tests

Review Checklist

Correctness

• Percentile interpolation uses standard linear interpolation within cumulative buckets
• Empty histogram returns None for percentile, empty string for gauges
• Per-type counter bounded at 256 entries (prevents memory growth from adversarial types)
• UserSignalTimestampMap bounded at 10,000 entries with sampling-based eviction
• Feedback-loop only records when signal was within 60s AND entity is in results
• Staleness correctly computes now - last_signal_ts in microseconds

Safety

• All new code behind #[cfg(feature = "metrics")] -- zero cost when disabled
• Verified: cargo check passes without metrics feature
• No new dependencies added
• Memory ordering: all new atomics use Relaxed (consistent with existing pattern)
• DashMap::iter() is used correctly (shard-level locking, not global)

Performance

• Per-signal-type counter: DashMap::get() is O(1) lock-free read in common case
• User timestamp recording: single DashMap::insert() per signal_with_context
• Staleness: single DashMap::get() per query (O(1))
• Feedback-loop: linear scan of result items (bounded by query limit, typically 20)
• Eviction: scans 64 entries max (O(1) amortized)

Test Coverage

• 7 unit tests for histogram percentile methods
• 4 integration tests covering all new metrics
• 1352 lib tests pass (7 more than baseline 1345)
• Integration tests verify Prometheus output and JSON diagnostics

Style

• Clippy clean in changed files (no new warnings)
• Doc comments on all public/pub(crate) methods
• Consistent with existing metrics patterns (AtomicU64, feature-gating, render_prometheus)

Issues Found

None. Implementation is clean and follows established patterns.

Verdict

APPROVE -- Implementation is correct, well-tested, and follows all existing conventions.