fix: wire the metrics option live — Prometheus module becomes the implementation (LAB-517)#75
Open
27Bslash6 wants to merge 5 commits into
Open
fix: wire the metrics option live — Prometheus module becomes the implementation (LAB-517)#7527Bslash6 wants to merge 5 commits into
27Bslash6 wants to merge 5 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>
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (20)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
27Bslash6
changed the title
Contributor
Author
|
@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).
Contributor
Author
|
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 |
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
1 participant
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.
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.