feat(observability): cache hit-rate counter on /api/health deep probe#536
Conversation
…lth deep probe Completes runbook item 3.2 (observability/alerts/rollback). The answer-SLO snapshot from #513 already exposes the hybrid_rpc_errors and degraded/source-only rates; the one remaining silent-degradation counter — cache hit-rate — is now instrumented in-process at the retrieval hot path and surfaced on the deep probe. - New src/lib/observability/cache-metrics.ts: cumulative process-start hit/miss counters (Prometheus-style; a scraper derives a windowed rate from deltas). - getCachedSearch records exactly one hit/miss per cache-enabled lookup (skipCache / TTL-or-size-0 records neither). - Health route exposes a `cache` block for any authorized deep probe (in-process, so it works in demo mode too); like `slo`, it never flips 200/503 liveness. - Refresh docs/observability-slos.md: the /api/health extension is now shipped (both `slo` and `cache`); only alert-channel wiring + host metrics remain. Tests: cache-metrics unit test + a health-route deep-probe case; full vitest suite green (1682 passed, 1 skipped). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change adds cumulative retrieval-cache metrics, records two-layer cache outcomes, exposes cache data through authorized deep health probes, keeps readiness responses cache-free, extends health-route tests, and documents the SLO and cache counter behavior. ChangesCache Observability
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Probe
participant healthResponse
participant CacheMetrics
participant Supabase
Probe->>healthResponse: send authorized deep probe
healthResponse->>CacheMetrics: request cacheMetricsSnapshot()
healthResponse->>Supabase: run existing deep health/SLO checks
CacheMetrics-->>healthResponse: cache counters
Supabase-->>healthResponse: SLO result or unavailable
healthResponse-->>Probe: return cache and optional slo blocks
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4c858434d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@docs/observability-slos.md`:
- Around line 200-205: Update the observability documentation to state that
Supabase health-probe failures reported or thrown by probeSupabaseHealth()
return 503 and affect readiness, while answerSloSnapshot() failures only omit
the slo block without changing readiness. Replace the claim that neither block
flips liveness with wording that distinguishes readiness from liveness and
accurately describes the deep-probe failure behavior.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c6229807-1513-4e25-bc93-13a8eacaa536
📒 Files selected for processing (6)
docs/observability-slos.mdsrc/app/api/health/route.tssrc/lib/observability/cache-metrics.tssrc/lib/rag-cache.tstests/cache-metrics.test.tstests/health-route.test.ts
…nter Addresses Codex P2 on #536. The counter was incremented inside getCachedSearch, which only sees the process-local cache — a local miss followed by a shared (rag_response_cache) hit was recorded as a miss, so a cold process or a multi-instance deployment with a warm shared cache would show a deflated hit-rate and could trip a false degradation alert. Move the recording to the two-layer cache orchestration in searchChunksWithTelemetry, recording exactly once with full knowledge of both layers via a pure classifySearchCacheOutcome(): a request served by either the local or the shared cache is a hit; a miss is counted only when a layer was consulted and missed; disabled/skipped lookups (shared returns null) record neither. getCachedSearch reverts to its original form. Tests: classifySearchCacheOutcome unit tests covering the local-miss/shared-hit regression, plus the disabled and both-miss cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ee80270d2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@copilot resolve the merge conflicts in this pull request |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Cannot run autofix: This PR has merge conflicts. Please resolve the conflicts with the base branch and try again. Alternatively, use |
|
@copilot resolve the merge conflicts in this pull request |
|
@copilot resolve the merge conflicts in this pull request |
Head branch was pushed to by a user without write access
Resolved. The only conflict was in Resolution: added |
The merge conflicts were already resolved in commit
The merge conflicts were already resolved in commit |
Head branch was pushed to by a user without write access
…format ready route Builds on the bot's main-integration (cache metrics in the shared healthResponse helper). Adds defense-in-depth so the operator-gated cache counter cannot leak: it is exposed only to a genuinely token-authorized deep probe (not merely `includeCache`), so a future route passing `allowUnauthenticatedDeep` without `includeCache:false` still cannot expose it. Adds a /api/health/ready guard test asserting no slo/cache leak even to a token-bearing caller, plus a doc note. Also reflows the ready route to satisfy prettier (the bot's one-liner exceeded print width and would have failed format:check). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…back-8d9b79' into claude/observability-alerts-rollback-8d9b79
…disabled; clarify liveness doc Addresses two PR #536 review findings: - Codex P2 (rag.ts): getSharedCachedSearch only short-circuits on skipCache/TTL, not on RAG_SEARCH_CACHE_SIZE, so a size-0 deployment (local cache off, default TTL) recorded disabled lookups as misses — a false 0% hit-rate. Gate the counter on a shared isSearchCacheEnabled() predicate (skipCache / TTL / size), threaded through classifySearchCacheOutcome(cacheEnabled, …). Added regression tests for the disabled-but-shared-ran case. - CodeRabbit (docs): the "neither block flips liveness" wording implied a Supabase probe failure stays 200. Corrected to distinguish counter *values* (never flip liveness) from probeSupabaseHealth() failures (set checks.supabase = error → 503) and answerSloSnapshot() failures (omit slo only). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/observability/cache-metrics.ts (1)
48-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one production implementation of
classifySearchCacheOutcome.The supplied graph context also shows an identical implementation at
src/lib/rag.ts:56-65. If that is a separate local function, remove it and import this helper so the tested implementation cannot diverge from the retrieval path.🤖 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 `@src/lib/observability/cache-metrics.ts` around lines 48 - 65, Remove the duplicate local classifySearchCacheOutcome implementation in the retrieval path of rag.ts and import the shared helper from cache-metrics.ts. Ensure all existing callers use the imported function while preserving the current hit, miss, and skip behavior.src/lib/rag-cache.ts (1)
182-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnsure the new cache gate is truly single-sourced.
The supplied graph context shows matching
isSearchCacheEnabledandgetCachedSearchimplementations insrc/lib/rag.ts:186-225. If those are real duplicates rather than stale references, update the production path to import these helpers; otherwise cache gating can drift from the observability logic.🤖 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 `@src/lib/rag-cache.ts` around lines 182 - 195, Ensure cache gating has one production implementation by locating the duplicate isSearchCacheEnabled and getCachedSearch symbols in src/lib/rag.ts and updating that path to import and use the helpers from rag-cache.ts. Remove or bypass the duplicate implementations while preserving existing cache lookup behavior and observability semantics.
🤖 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 `@src/lib/observability/cache-metrics.ts`:
- Around line 48-65: Remove the duplicate local classifySearchCacheOutcome
implementation in the retrieval path of rag.ts and import the shared helper from
cache-metrics.ts. Ensure all existing callers use the imported function while
preserving the current hit, miss, and skip behavior.
In `@src/lib/rag-cache.ts`:
- Around line 182-195: Ensure cache gating has one production implementation by
locating the duplicate isSearchCacheEnabled and getCachedSearch symbols in
src/lib/rag.ts and updating that path to import and use the helpers from
rag-cache.ts. Remove or bypass the duplicate implementations while preserving
existing cache lookup behavior and observability semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e689d4cd-3d79-4b28-b1b9-2468c53fdcca
📒 Files selected for processing (8)
docs/observability-slos.mdsrc/app/api/health/ready/route.tssrc/lib/health-response.tssrc/lib/observability/cache-metrics.tssrc/lib/rag-cache.tssrc/lib/rag.tstests/cache-metrics.test.tstests/health-route.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/app/api/health/ready/route.ts
- src/lib/health-response.ts
- tests/cache-metrics.test.ts
- src/lib/rag.ts
- docs/observability-slos.md
- tests/health-route.test.ts
|
Re the two 🧹 nitpicks (single-source 🤖 Addressed by Claude Code |
What & why
Completes runbook item 3.2 (observability / alerts / rollback). The answer-SLO snapshot from #513 already exposes the
hybrid_rpc_errorsand degraded/source-only rates onGET /api/health?deep=1. The one remaining silent-degradation counter fromdocs/observability-slos.md— cache hit-rate — is added here, instrumented in-process at the retrieval hot path exactly as that module's follow-up note specified.Changes
src/lib/observability/cache-metrics.ts(new): cumulative process-start hit/miss counters (recordCacheLookup/cacheMetricsSnapshot). Prometheus-style — a scraper derives a windowed hit-rate from the delta between two polls.src/lib/rag-cache.ts:getCachedSearchrecords exactly one hit/miss per cache-enabled lookup (extractedlookupCachedSearchso all miss branches funnel through one record site;skipCache/ TTL-or-size-0 records neither). No change to what the function returns — retrieval behavior is untouched.src/app/api/health/route.ts: exposes acacheblock for any authorized deep probe (in-process, so it works in demo mode too). Likeslo, it is reliability telemetry only and never flips 200/503 liveness — a bad rate must not trip the container HEALTHCHECK into a restart it can't fix.docs/observability-slos.md: §4 now documents the shippedslo+cacheblocks (was stale — still listed the whole extension as a gap); §5 keeps the remaining operator work (alert-channel wiring + host metrics).Example (
GET /api/health?deep=1, authorized)Clinical governance preflight
Observability-only. No change to ingestion, answer generation, retrieval/ranking, source rendering, document access, privacy, or clinical output —
getCachedSearchreturns identical results; the only new behavior is a counter side-effect and a gated telemetry field on/api/health.Verification
vitestsuite green: 1682 passed, 1 skipped.tsc --noEmit, ESLint (--max-warnings 0), and Prettier--checkall clean on touched files.tests/cache-metrics.test.ts(counter unit test) + a health-route deep-probe case asserting thecacheblock.🤖 Generated with Claude Code