fix: wire the metrics option live — Prometheus module becomes the implementation (LAB-517) - #75
fix: wire the metrics option live — Prometheus module becomes the implementation (LAB-517)#7527Bslash6 wants to merge 9 commits into
Conversation
…lementation (LAB-517)
createCache({ metrics }) was accepted by the types and every intent preset,
then silently ignored: CacheImpl never read it and the entire Prometheus
module was dead code. Silent config no-ops are trust bugs.
- CacheImpl now threads metrics through get/set/delete/wrap: hits (l1/l2),
misses, per-attempt operation counters, error counters, duration
histograms, L1 entry/memory gauges, and the circuit-breaker state gauge.
- metrics accepts boolean | MetricsConfig (prefix, defaultLabels, registry,
onError). The registry option was previously accepted-and-ignored — now
honored; metrics are reused via getSingleMetric so multiple caches sharing
a registry no longer kill initialization with duplicate-registration.
- Live L1/L2 hit and miss counters feed the SaaS X-CacheKit-L1-* telemetry
headers: the CachekitIO metricsProvider is auto-wired from them (an
explicit user-supplied provider still wins).
- If metrics is enabled without the optional prom-client peer dependency,
the SDK reports once through the library logger and degrades to no-ops —
loud, never silent.
- New pluggable logger hook (setLogger): all ad-hoc console.error sites
(invalidation channel, background refresh, Redis error events, metrics
fallback) now route through it; default remains console.error.
- CacheMetrics/NoopMetrics/createMetrics/MetricsCollector/MetricsConfig and
setLogger/CachekitLogger exported from the package index.
Co-authored-by: multica-agent <github@multica.ai>
WalkthroughThis PR updates native binding loading, adds in-process and distributed-lock stampede protection, expands metrics configuration and telemetry, introduces reusable Prometheus metrics, and provides pluggable internal error logging. ChangesNative binding loading
Cache protection and observability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant CacheImpl
participant LockableBackend
participant MetricsCollector
Caller->>CacheImpl: wrap key
CacheImpl->>LockableBackend: check and coordinate cold miss
LockableBackend-->>CacheImpl: cached value or lock result
CacheImpl->>MetricsCollector: record hit, miss, or failure
CacheImpl-->>Caller: return value
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
27Bslash6
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
Expert-panel review found exists() was the only public op left uninstrumented: L1 hits went uncounted and an L2 backend error degraded to a silent false with no errors_total trace — invisible in the very metrics this PR wires live. Root cause: the instrument()-inside / execute()-outside pairing was copy-pasted per method, so exists() simply drifted out of the set. Extract it into a single run() helper that every op routes through, making the omission structurally impossible. Co-authored-by: multica-agent <github@multica.ai>
…LAB-519) A cold L1+L2 miss previously executed the wrapped function once per concurrent caller — N cold callers meant N upstream calls, N L2 GETs (N billed misses under metered-misses), and N L2 writes. In-process (always on): concurrent wrap() calls for one cache key share a single in-flight promise covering the L2 read, compute, and write. The L2 read must be inside the flight — the GET-miss is the billed event, so deduping only the compute would still bill N misses. Cross-process (opt-in, stampede.distributedLock): wires the previously caller-less LockableBackend capability around the miss path, mirroring cachekit-py's acquire_lock flow — acquire, double-check L2, compute, write, release. Contested waiters retry the lock on an interval rather than polling get() (a poll GET against a still-cold key is itself a billed miss), then fall through to computing after lockWaitMs: the lease is best-effort mitigation, never a correctness gate. Lock calls stay outside the reliability executor so lock failures neither stack retry latency nor open the circuit breaker for data operations. Deliberately no general admission-control cap beyond L1's maxConcurrentRefreshes: on Node's single-threaded event loop concurrent misses don't compete for threads, single-flight collapses the per-key herd, and distinct-key floods are bounded by backend timeouts plus the circuit breaker. Decision recorded on StampedeConfig. Co-authored-by: multica-agent <github@multica.ai>
Port LAB-519 single-flight + distributed-lock cold path onto the cache-core.ts extraction from LAB-595 (#78): CacheImpl moved to the shared core, so the single-flight map, stampede config validation, and resolveMiss/resolveUnderLock/computeAndStore land there (now shared with the Workers entrypoint). Backend construction stays in the runtimes — CacheRuntime.resolveBackend grows an optional stampede param so the Node runtime can select cachekitioWithLocking for apiKey configs; the Workers runtime is deliberately untouched (lock-capable backends remain an explicit construction there). StampedeConfig export moves to exports-common.ts (platform-neutral surface).
…lab-517-wire-metrics Stacks LAB-517 on LAB-519 per the LAB-761 merge plan (metrics observe the cache, so they land on top; when #77 merges this PR's diff collapses to metrics-only). Ports the metrics wiring onto the cache-core.ts extraction from LAB-595: - Instrumentation (run/execute/instrument, hit/miss/L1-stats recording) moves into the shared CacheImpl in cache-core.ts, composing with the LAB-519 single-flight cold path — followers share one recorded miss. - cache-core imports metrics/prometheus.js TYPES only (prom-client is Node-only and dynamic import still enters esbuild's resolve graph); the collector is injected via a new optional CacheRuntime.createMetrics hook, with an inline no-op fallback, so Workers degrade to no-ops the same way a missing prom-client peer does on Node. - resolveBackend gains an l1Telemetry getter param so the Node runtime auto-wires the SaaS X-CacheKit-L1-* metricsProvider from live counters, composed with the LAB-519 lockable-wrapper selection. - setLogger/CachekitLogger move to exports-common.ts: logger.js is in the shared graph (background-refresh, invalidation) and Workers apps need the same error sink control. Prometheus exports stay Node-only. - intents metrics?: boolean | MetricsConfig widening ports to intents-core.ts (type-only import, workers-safe).
|
Un-dirtied per the LAB-761 merge plan ("#75 rebases on #77"): this branch now stacks on Port notes (main's LAB-595 refactor moved
Verified locally: type-check, lint, format, full vitest (588 passed — metrics + single-flight suites together), Workers test lane (54 passed), and |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cachekit/src/metrics/prometheus.ts (1)
148-214: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
defaultLabelsneed to be added to each metric’s label set.
record*(),startTimer()andupdate*()all passthis.defaultLabelsinto prom-client, but the metrics are initialised with fixedlabelNamesthat don’t include those keys. Any non-emptydefaultLabelswill make these calls throw at runtime, so no metrics get recorded. If per-instance labels are intended, includeObject.keys(this.defaultLabels)in each metric’slabelNames; if not, move them to registry-level default labels.🤖 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 `@packages/cachekit/src/metrics/prometheus.ts` around lines 148 - 214, Update the metric initialization around getOrCreate and each metric definition to include Object.keys(this.defaultLabels) in every labelNames array, preserving each metric’s existing labels. Ensure defaultLabels keys are supported by operationsCounter, hitsCounter, missesCounter, errorsCounter, durationHistogram, l1EntriesGauge, l1MemoryGauge, and circuitBreakerGauge so record*, startTimer, and update* calls remain valid.packages/cachekit/src/cache-core.ts (1)
363-413: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThread
skipL1through the cold path
wrap()already skips the SWR read, butresolveMiss()/resolveUnderLock()still callgetEntry(), andcomputeAndStore()repopulates L1 unconditionally. Passoptions.skipL1into the cold-path read and suppress the L1 write-back as well, otherwise callers can still see or repopulate L1 after opting out.🤖 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 `@packages/cachekit/src/cache-core.ts` around lines 363 - 413, Thread the skipL1 option through resolveMiss() and resolveUnderLock() into getEntry(), so cold-path reads bypass L1 when requested. Update computeAndStore() and the getEntry() L1 population path to suppress write-back under skipL1, while preserving existing behavior when the option is false.
🤖 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 `@packages/cachekit/src/cache-core.ts`:
- Around line 283-320: Guard the metrics error-handling callback used by the
MetricsCollector so exceptions from onError cannot escape or create unhandled
rejections. Update the metrics initialization or configuration around the
recordHit, recordMiss, recordFailure, publishL1Stats, and execute methods to
wrap this.errorHandler invocation in a safe try/catch, preserving best-effort
metric behavior.
---
Outside diff comments:
In `@packages/cachekit/src/cache-core.ts`:
- Around line 363-413: Thread the skipL1 option through resolveMiss() and
resolveUnderLock() into getEntry(), so cold-path reads bypass L1 when requested.
Update computeAndStore() and the getEntry() L1 population path to suppress
write-back under skipL1, while preserving existing behavior when the option is
false.
In `@packages/cachekit/src/metrics/prometheus.ts`:
- Around line 148-214: Update the metric initialization around getOrCreate and
each metric definition to include Object.keys(this.defaultLabels) in every
labelNames array, preserving each metric’s existing labels. Ensure defaultLabels
keys are supported by operationsCounter, hitsCounter, missesCounter,
errorsCounter, durationHistogram, l1EntriesGauge, l1MemoryGauge, and
circuitBreakerGauge so record*, startTimer, and update* calls remain valid.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 283a9dd4-0474-4e5a-a927-8a3e0804b5fd
📒 Files selected for processing (20)
packages/cachekit-core-ts/index.jspackages/cachekit/README.mdpackages/cachekit/src/backends/cachekitio.test.tspackages/cachekit/src/backends/redis.tspackages/cachekit/src/cache-core.tspackages/cachekit/src/cache.metrics.test.tspackages/cachekit/src/cache.single-flight.test.tspackages/cachekit/src/cache.tspackages/cachekit/src/cache/background-refresh.tspackages/cachekit/src/constants.tspackages/cachekit/src/exports-common.tspackages/cachekit/src/index.test.tspackages/cachekit/src/index.tspackages/cachekit/src/intents-core.tspackages/cachekit/src/invalidation/redis-channel.tspackages/cachekit/src/logger.test.tspackages/cachekit/src/logger.tspackages/cachekit/src/metrics/prometheus.test.tspackages/cachekit/src/metrics/prometheus.tspackages/cachekit/src/types/cache.ts
A user-supplied onError that throws escaped handleError() and rejected the collector's promise, turning the cache layer's fire-and-forget void this.metrics.*() calls into unhandled rejections. Both call sites (handleError and the initialize failure path) now invoke the handler through a guarded helper that reports the handler's own failure via the library logger — metrics stay best-effort, the never-reject invariant holds. CodeRabbit-Resolved: cache-core.ts:320:Guard the metrics error han Co-authored-by: multica-agent <github@multica.ai>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Auto-rebase sweep found this PR conflicted against |
|
Auto-rebase attempted a merge of
|
|
Auto-rebase: conflicts in |
|
Auto-rebase: still conflicted against The new conflict is structural, not textual: |
|
Auto-rebase attempted: merging
Left for a human to reconcile. Merge aborted, branch untouched. |
|
Conflicts with |
Conflicts resolved:
- cache-core.ts: keep both new fields (metrics/telemetry + swrRequiresWaitUntil);
re-seat recordHit('l1') inside the restructured SWR-hit return (LAB-751) and
add it to the new no-waitUntil plain L1 read — both paths are L1 hits.
- intents-core.ts: main's IntentBackendOptions shape + this branch's
metrics: boolean | MetricsConfig union and doc comment (Production/Secure;
IO merged clean with the union intact).
- README.md: keep both new sections (Stampede Protection + Backends).
This comment has been minimized.
This comment has been minimized.
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
- cache.metrics.test.ts: drop the wholesale 'as unknown as' reshape of
getMetricsAsJSON() — use prom-client's declared types end to end,
narrowing only the undocumented per-sample metricName field that
prom-client emits at runtime for histogram sub-series but omits from
MetricValue. The circuit-breaker gauge check needs no cast at all.
- cachekitio.test.ts: hoist the fixture credential into FAKE_API_KEY
('ck_test_fake-not-a-secret', detect-secrets allowlisted) so the value
is unmistakably fake; an env-sourced key would add a hidden test
dependency for zero security gain, so deliberately not process.env.
- background-refresh.ts: route the waitUntil-registration failure log
(arrived from main, LAB-750 era) through logError like every other
internal error site — keeps this PR's setLogger invariant true.
Kody-Resolved: cachekitio.test.ts:380:Hardcoded-looking API key
Kody-Resolved: cache.metrics.test.ts:61:Double type assertion
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
@kody start-review |
# Conflicts: # .secrets.baseline
eaf0f9e
|
Auto-rebased onto main: only conflict was the `generated_at` timestamp in `.secrets.baseline` (cosmetic scan metadata, secret entries auto-merged clean); Cargo.lock/Cargo.toml and test fixtures auto-merged with no marker conflicts. Lint + format-check + type-check pass. CI will re-run. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cachekit/src/cache-core.ts (2)
526-538: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRecord L2 outcomes for
exists().When L1 misses, the backend result bypasses both
recordHit('l2')andrecordMiss(). L2-only existence checks therefore under-report hit/miss counters and SaaS L1 telemetry.Proposed fix
- return this.run('exists', false, () => this.backend.exists(key)); + return this.run('exists', false, async () => { + const exists = await this.backend.exists(key); + if (exists) this.recordHit('l2'); + else this.recordMiss(); + return exists; + });🤖 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 `@packages/cachekit/src/cache-core.ts` around lines 526 - 538, Update exists() so that after the L1 lookup misses, the backend existence result records recordHit('l2') when true and recordMiss() when false before returning it. Preserve the existing L1 hit handling and use the existing telemetry methods rather than adding new counters.
493-501: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftProtect L2 from stale refresh writes before relying on marker expiry.
L1 version tokens do not protect the unconditional L2 write, so overlapping or slower refreshes can overwrite newer data and later repopulate stale L1 values.
packages/cachekit/src/cache-core.ts#L493-L501: replace the unconditional refresh write with a conditional/versioned write or serialise it with explicit writes.packages/cachekit/src/cache-core.ts#L799-L812: ensurecomputeAndStoreuses that protected write path.packages/cachekit/src/constants.ts#L152-L161: revise the “benign duplicate” guarantee or retain markers until refresh completion.🤖 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 `@packages/cachekit/src/cache-core.ts` around lines 493 - 501, Protect L2 from stale refresh writes by replacing the unconditional backend.set path near cache-core.ts:493-501 with a conditional/versioned write or serialization with explicit writes, while preserving L1 version-token behavior. Update computeAndStore near cache-core.ts:799-812 to use the protected write path, and revise the benign-duplicate guarantee in constants.ts:152-161 or retain markers through refresh completion so stale refreshes cannot repopulate L1.
🤖 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 `@packages/cachekit/src/metrics/prometheus.ts`:
- Around line 240-258: Make the shared logError invocation non-throwing so
user-provided logger failures cannot escape metrics error handling or violate
the collector’s no-rejection guarantee; update the error-reporting path around
invokeErrorHandler and handleError in
packages/cachekit/src/metrics/prometheus.ts (lines 240-258). In
packages/cachekit/src/cache/background-refresh.ts (lines 87-116), ensure refresh
cleanup, including cancelRefresh, still executes when error reporting fails, and
verify async paths produce no unhandled promise rejections.
---
Outside diff comments:
In `@packages/cachekit/src/cache-core.ts`:
- Around line 526-538: Update exists() so that after the L1 lookup misses, the
backend existence result records recordHit('l2') when true and recordMiss() when
false before returning it. Preserve the existing L1 hit handling and use the
existing telemetry methods rather than adding new counters.
- Around line 493-501: Protect L2 from stale refresh writes by replacing the
unconditional backend.set path near cache-core.ts:493-501 with a
conditional/versioned write or serialization with explicit writes, while
preserving L1 version-token behavior. Update computeAndStore near
cache-core.ts:799-812 to use the protected write path, and revise the
benign-duplicate guarantee in constants.ts:152-161 or retain markers through
refresh completion so stale refreshes cannot repopulate L1.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e9926f5a-f3b6-475e-ab7e-d89d40ab898a
📒 Files selected for processing (11)
.secrets.baselinepackages/cachekit/README.mdpackages/cachekit/src/backends/cachekitio.test.tspackages/cachekit/src/cache-core.tspackages/cachekit/src/cache.metrics.test.tspackages/cachekit/src/cache/background-refresh.tspackages/cachekit/src/constants.tspackages/cachekit/src/index.tspackages/cachekit/src/intents-core.tspackages/cachekit/src/metrics/prometheus.test.tspackages/cachekit/src/metrics/prometheus.ts
| private invokeErrorHandler(err: Error): boolean { | ||
| if (!this.errorHandler) return false; | ||
| try { | ||
| this.errorHandler(err); | ||
| } catch (handlerError) { | ||
| logError('[cachekit] metrics onError handler threw:', handlerError); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Handle errors from async metric operations. | ||
| * m5 Fix: Proper error handling instead of silent failures. | ||
| */ | ||
| private handleError(error: unknown, context: string): void { | ||
| const err = error instanceof Error ? error : new Error(String(error)); | ||
|
|
||
| if (this.errorHandler) { | ||
| this.errorHandler(err); | ||
| } else { | ||
| // eslint-disable-next-line no-console | ||
| console.error(`[cachekit] Metrics error (${context}):`, err.message); | ||
| if (!this.invokeErrorHandler(err)) { | ||
| logError(`[cachekit] Metrics error (${context}):`, err.message); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the shared logger non-throwing.
logError directly calls the active, user-pluggable logger. If that logger throws, metrics methods reject despite their fire-and-forget contract; background refreshes reject and skip cancelRefresh, leaving the L1 refresh marker until expiry. Guard logger invocation centrally.
packages/cachekit/src/metrics/prometheus.ts#L240-L258: preserve the collector’s no-rejection guarantee when error reporting itself fails.packages/cachekit/src/cache/background-refresh.ts#L87-L116: ensure refresh cleanup still runs when reporting an error fails.
As per path instructions, “Verify async error handling — no unhandled promise rejections.”
📍 Affects 2 files
packages/cachekit/src/metrics/prometheus.ts#L240-L258(this comment)packages/cachekit/src/cache/background-refresh.ts#L87-L116
🤖 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 `@packages/cachekit/src/metrics/prometheus.ts` around lines 240 - 258, Make the
shared logError invocation non-throwing so user-provided logger failures cannot
escape metrics error handling or violate the collector’s no-rejection guarantee;
update the error-reporting path around invokeErrorHandler and handleError in
packages/cachekit/src/metrics/prometheus.ts (lines 240-258). In
packages/cachekit/src/cache/background-refresh.ts (lines 87-116), ensure refresh
cleanup, including cancelRefresh, still executes when error reporting fails, and
verify async paths produce no unhandled promise rejections.
Source: Path instructions
1 participant
Closes LAB-517.
Problem
createCache({ metrics })was accepted by the types and defaulted totrueby every production intent preset — then silently ignored.CacheImplnever read it, and the entire Prometheus module (src/metrics/prometheus.ts) was dead code: unexported, referenced only by its own tests. A user who wired Prometheus per the types got no metrics and no error. Found by the LAB-275 cross-SDK audit; this is the LAB-388 pattern as a runtime surface.What changed
The dead module is now the live implementation (per the issue's AC — wire it, don't reject it):
CacheImplthreads metrics throughget/set/delete/wrap:cachekit_hits_total{layer=l1|l2}— including thewrap()SWR fast pathcachekit_misses_totalcachekit_operations_total{operation,status}— recorded per backend attempt (inside retry), the honest reading of an operations countercachekit_errors_total{error_type}cachekit_operation_duration_seconds{operation}cachekit_l1_entries/cachekit_l1_memory_bytesgauges (updated on every L1 mutation)cachekit_circuit_breaker_stategauge (published after each reliability-stack execution)metricsnow acceptsboolean | MetricsConfig. Two latent bugs in the module fixed on the way:MetricsConfig.registrywas accepted-and-ignored (the same bug class this ticket is about) — now honored.getSingleMetric.CacheImpl), and the SaaSX-CacheKit-L1-*telemetrymetricsProvideris auto-wired from them for CachekitIO config backends. An explicit user-supplied provider still wins.metrics: truewithout the optionalprom-clientpeer dep reports once through the library logger with an actionable message ("install prom-client or set metrics: false") and degrades to no-ops. Decision per the AC's "throw or warn loudly": warn loudly, because intent presets defaultmetrics: trueand throwing would break every existing preset user who never opted in.setLogger): all ad-hocconsole.errorsites (cache.ts,background-refresh.ts,redis.ts,redis-channel.ts, metrics fallback) route through it. Default remainsconsole.error— zero behavior change unless opted in.CacheMetrics,NoopMetrics,createMetrics,MetricsCollector,MetricsConfig,setLogger,CachekitLoggerfrom the package index.Non-goals (per issue)
OpenTelemetry tracing; Python's
enable_prometheus_metrics()DX helper.Testing
cache.metrics.test.tsdrives real prom-client throughCacheImplend-to-end (no mocks on the metrics path) — hits/misses/ops/errors/durations/gauges, wrap() SWR path, shared-registry reuse, custom prefix, disabled-by-default;cachekitio.test.tscovers telemetry header auto-wire + user override + L1-disabled;logger.test.tscovers the hook incl. a live background-refresh failure.Docs pass (mandatory gate)
packages/cachekit/README.mdObservability section: full metric list (was missing gauges), missing-peer-dep behavior,setLogger, SaaS telemetry auto-wire.metricsrow + Observability section — PR follows in cachekit-io/docs.Summary by CodeRabbit
metricsoption) plus CachekitIO live L1 telemetry headers.setLogger, with public metrics exports on Node.