feat(metrics)!: classify cache misses by bounded reason - #130
Closed
lan17 wants to merge 5 commits into
Closed
Conversation
Add a reason label to the miss metric so watermark-fenced misses are separable from cold keys. Decoders and DialCacheRedisClient.read() now return a discriminated RedisReadOutcome instead of payload-or-null, with four bounded read-miss reasons (not_found, frame_unsupported, watermark_unreadable, watermark_invalidated) plus a metrics-level deserialization_failed for serializer.load failures. Local layers always emit not_found. A runtime outcome guard routes malformed client results through the existing fail-open cache_read error path so the label vocabulary stays bounded. BREAKING CHANGE: DialCacheRedisClient.read() and the exported decodeRedisFrame/decodeTrackedRedisFrame return RedisReadOutcome instead of RedisCachePayload | null, and dialcache_miss_counter gains a reason label, so an old-schema collector in the same Prometheus registry now fails adapter construction.
- Migrate all four benchmark fake clients to RedisReadOutcome shapes; the legacy payload/null returns tripped the new outcome guard, failing three scenarios on their own asserts and hanging the dark-fill one - Single-source the guard's reason set via satisfies Record<RedisReadMissReason, true> so a future reason cannot drift out of it, and pin that a client-supplied deserialization_failed is rejected as malformed - Pin frame-before-watermark precedence with competing-bad-state decoder assertions and a real-Redis mirror - Delegate FakeRedis read classification to the exported decoders on a copied frame instead of hand-mirroring the ladder - Fix the stale shadow-spec sentence still defining a clean miss as a null read; scope deserialization_failed to the remote layer; note that only the first fenced read per invalidation window labels watermark_invalidated; qualify not_found's wrong-type doc as tracked-MGET-only
Collapse the decoder singletons, the guard's exhaustive flags object, and its derived Set into a single REDIS_READ_MISS_OUTCOMES table: one frozen outcome per bounded reason, returned by the decoders and now also the normal form the core collapses every client miss onto. The read guard (renamed normalizeRedisReadOutcome) reads each client-owned property exactly once, recaptures hit payloads, and returns canonical singletons for misses, so accessor-backed or otherwise unstable client objects can never flip answers between validation and metric emission. Rejections now carry a bounded shape fingerprint naming the malformed class - legacy payload-or-null, non-object value, unknown status, unbounded or core-owned reason, payload-less hit - so a stale-client migration storm names its own cause. Also: unify the two identical started-read interfaces as StartedRead<T>; pin the accessor-flip behavior in both directions plus the toString prototype-chain forgery; add the missing negative type pin that deserialization_failed is not assignable to RedisReadMissReason.
The decoders and the read normalizer alias one frozen outcome per miss reason, so a mutable entry would let a single consumer corrupt every later miss and its bounded metric label process-wide; pin identity, frozenness, and mutation rejection. Also narrow not_found's doc to the tracked MGET value member - a wrong-type watermark member reads as nil and classifies watermark_unreadable, as the README and integration tests already state.
…ication Conflicts were the adjacent bounded-vocabulary blocks in the adapter tests (MISS_REASONS vs COMPRESSION_OUTCOMES / ERROR_KINDS with the new compression kind) - resolved by keeping both. Compression operates on serializer output inside the frame payload, one layer below the read outcome classification, so the two features compose without semantic changes; a rejected compression envelope surfaces as deserialization_failed alongside its fallback_raw/read_over_limit compression outcome, now cross-referenced in the miss-reasons table.
This was referenced Aug 23, 2026
lan17
commented
Aug 29, 2026
OwnerAuthor
Closing this stale, conflicting implementation in favor of the protocol-current reconciliation spec in #145. A fresh implementation PR will follow from current main; this branch should not be rebased mechanically. |
lan17 added a commit
that referenced
this pull request
Sep 3, 2026
## Summary Implements #145 (replaces the stale attempt in #130). Every DialCache miss event now says **why** it missed. Until now, a miss storm during a future-buffer invalidation window was indistinguishable from a cold cache or an eviction problem: the miss counter only told you *that* a layer missed, never whether the value was truly absent or a stored frame was being rejected by an invalidation watermark. This PR adds one required, bounded `reason` to the existing miss event — no new metric instrument, no new Redis command, no extra round trip, and no change to serving, refill, invalidation, stale-recovery, or shadow behavior. ## What changes in your metrics `miss` events now carry `reason` alongside the existing `cacheNamespace` / `useCase` / `keyType` / `layer` labels, on every layer (`request_local`, `local`, `remote`, `remote_shadow`): | `reason` | Fires when | Typical cause | | --- | --- | --- | | `value_absent` | The layer had no retrievable value at all: never populated, physically expired or evicted, Redis `nil`, or a tracked-`MGET` wrong-type member (which Redis reports as `nil`). Request-local and process-local misses are always this. | Cold keys, physical TTL expiry, eviction pressure. | | `expired` | A complete, supported frame with a valid, non-future timestamp was present, but its logical age against the reader's clock reached the effective remote TTL `F`. Covers `age >= M` misses and `F <= age < M` frames retained as stale-on-error candidates, on both caller and shadow reads. | Ordinary TTL churn on hot keys; the routine steady-state miss reason whenever `staleOnErrorMaxAgeSec` is configured. | | `watermark_fenced` | A complete, supported tracked frame with a positive safe-integer timestamp was rejected because its `createdAtMs` was at or below a valid observed invalidation watermark. Decided before deserialization. | Reads inside an active `futureBufferMs` invalidation window; writer/invalidator clock skew. | | `unclassified` | The miss is real but attributable to none of the above: unrecognized custom-adapter results (including `null`), short/unsupported frames, zero-stamped tracked frames, malformed watermark metadata paired with a present frame, future or invalid timestamps, and deserialization failures. | Un-migrated custom adapters; protocol-edge states. | The operational win: during an invalidation window you can now separate fence churn from genuine absence at a glance; routine TTL churn (including the stale-on-error `F..M` window, where every read of a retained frame is a miss) is `expired` rather than noise in the other buckets; and because expiry no longer lands there, a sudden `unclassified` plateau genuinely points at protocol-edge states (skewed clocks, corrupt frames, deserialization failures, legacy adapters) instead of hiding inside the aggregate. **Deviation from #145:** the issue specified a three-value union and named `unclassified` as the safe default for stale-on-error retained frames "unless a separately justified reason is added". Review found that with `staleOnErrorMaxAgeSec` configured every read of a retained `F..M` frame is a miss, so `unclassified` would have been the routine steady-state reason for exactly the use cases that enable stale-on-error, and the plateau signal above would not hold. `expired` is that separately justified reason; the union is four values and the cardinality note below is 4x accordingly. **Prometheus** — `dialcache_miss_counter` keeps its name and gains `reason` as a fifth label: ```promql # Fence churn by use case during an invalidation window sum by (use_case) (rate(dialcache_miss_counter{layer="remote", reason="watermark_fenced"}[5m])) # Pre-existing total-miss and hit-ratio queries: aggregate reason away sum by (cache_namespace, use_case, key_type, layer) (rate(dialcache_miss_counter[5m])) ``` **Datadog** — `dialcache.miss.count` keeps its name and gains the same `reason` tag, e.g. `sum:dialcache.miss.count{reason:watermark_fenced} by {use_case}`. Expect up to 4× the miss-series cardinality (four bounded reasons; only `remote` and `remote_shadow` tuples can carry every reason, since request-local and local misses are always `value_absent`). ## Migration - **Prometheus registries:** the label set changed, so registering against a registry that already holds a 4-label `dialcache_miss_counter` (e.g. a not-yet-upgraded sidecar library) fails loudly at construction — upgrade producers together per registry. Nothing silently mislabels. - **Mixed-fleet rollouts:** old processes emit reason-less series while new ones carry `reason`. Total-miss and miss/request-ratio queries must `sum by (...)` the shared labels (example above); reason-aware dashboards should group by `reason` explicitly. - **Custom metrics adapters:** `DialCacheMetricsAdapter.miss` now receives `MissMetricLabels` (extends the unchanged `CacheMetricLabels` with required `reason: CacheMissReason`). Adapters whose `miss` parameter is typed as the broader `CacheMetricLabels` compile unchanged and may ignore the field; exact label snapshots, exhaustive `Record`s over reasons, and adapters that reject or forward unknown fields must add it. - **Custom Redis adapters:** `RedisReadResult` is now `DecodedRedisFrame | RedisReadMiss { kind: "miss", reason, observedWatermarkMs? }`. `null`, `RedisWatermarkMiss`, `decodeRedisFrame`, and `decodeTrackedRedisFrame` are removed; use `decodeRedisReadResult` (untracked) / `decodeTrackedRedisReadResult` (tracked) and the exported `isRedisReadMiss` guard. An un-migrated adapter still fails open at runtime (`null` or any unrecognized result is an `unclassified` miss with a normal refill) but loses reason precision and fenced-refill suppression until it returns the typed miss. ## How classification works Bundled decoders return one miss shape, `RedisReadMiss { kind: "miss", reason, observedWatermarkMs? }`, attaching `observedWatermarkMs` only when the same atomic tracked snapshot carried a valid numeric watermark. Cause and fence are deliberately independent: Redis `nil` is decisive evidence of absence, so an absent value reports `value_absent` while still carrying the observed watermark that lets PR 143's two-sample admission skip a known-fenced refill — only a complete supported frame actually rejected by the watermark reports `watermark_fenced`. Core treats the adapter boundary as untrusted and normalizes every result once, at one choke point: `kind: "miss"` is the only discriminator; invalid or missing reasons become `unclassified`, a `watermark_fenced` claim without a valid tracked fence is demoted to `unclassified`, invalid fences (`NaN`, negatives, non-integers, untracked keys) are dropped, and `null`, `undefined`, or any other unrecognized value is an `unclassified` miss with a normal refill. Objects without `kind: "miss"` are frame candidates and go through the existing timestamp and serializer validation. Decoded hits keep the existing `{ payload, createdAtMs }` shape. Core-side rejections never acquire a refill fence: logical-age rejections emit `expired`, future timestamps and deserialization failures emit `unclassified`; conditional refill suppression remains exactly PR 143's adapter-level watermark-miss path, byte-compatible with `main` (differential-verified across the decode input matrix). ## Breaking change `DialCacheMetricsAdapter.miss` now receives `MissMetricLabels`, and first-party miss metrics require a `reason` label/tag. `RedisReadResult` is now `DecodedRedisFrame | RedisReadMiss`; `null`, `RedisWatermarkMiss`, `decodeRedisFrame`, and `decodeTrackedRedisFrame` are removed, and bundled Redis adapters return `RedisReadMiss` for every semantic miss. Under the pre-1.0 release policy, this is a minor release. ## Validation - Node.js 22.22.0: `corepack pnpm check` - typecheck - 656 unit tests with coverage thresholds - ESM/CJS build and declaration generation - packed-package type and runtime checks - Node.js 22.22.0: `corepack pnpm test:integration` - 143 passed - 2 expected local GLIDE Cluster skips because announced container IPs are not host-routable - Review follow-up (`fix(metrics): preserve existing refill and decoder behavior`) removed the fence-carry scope creep and restored decoder/refill parity with `main`, differential-verified across the decode input matrix BREAKING CHANGE: DialCacheMetricsAdapter.miss now receives MissMetricLabels and first-party miss metrics require a reason label/tag; RedisReadResult is now DecodedRedisFrame | RedisReadMiss ({ kind: "miss", reason, observedWatermarkMs? }); null, RedisWatermarkMiss, decodeRedisFrame, and decodeTrackedRedisFrame are removed, and bundled Redis adapters return RedisReadMiss for every semantic miss.
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 freeto 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
Cache misses were indistinguishable in metrics: the watermark fence decision in
decodeTrackedRedisFramecollapsed "key absent", "frame unsupported", "watermark missing/malformed", and "fenced by watermark" into onenull. Operators could not separate invalidation churn from cold keys.The miss metric now carries a bounded
reasonlabel (mirroring the existingdisabled{reason}precedent):not_found— key absent/expired (all layers; local layers always use this)frame_unsupported— short frame or unsupported frame versionwatermark_unreadable— tracked read with missing/malformed/non-finite watermarkwatermark_invalidated— tracked frame fenced (createdAt <= watermark)deserialization_failed— payload read butserializer.loadthrewsum by (use_case) (rate(dialcache_miss_counter{reason="watermark_invalidated"}[5m]))now measures invalidation churn directly, including the repeated stale-frame transfer cost during future-buffer windows.frame_unsupported/watermark_unreadableshould be ~zero in steady state and make good anomaly alerts.Design
DialCacheRedisClient.read()return a discriminatedRedisReadOutcome({status:"hit",payload}|{status:"miss",reason}). The decoder remains the single source of truth for classification; bundled adapters needed zero code changes.RedisCacherejects malformed client results (e.g. a stale client still returningpayload | null) into the existing fail-opencache_readerror path, so the label vocabulary cannot go unbounded.remote_shadow; shadow fill behavior is unchanged — a fenced dark read stays fill-eligible because the tracked write script re-fences server-side (pinned by a new test).CacheGetResulttypes unchanged: misses are recorded where discovered, no caller re-records.Breaking changes
DialCacheRedisClient.read()and exporteddecodeRedisFrame/decodeTrackedRedisFramereturnRedisReadOutcomeinstead ofRedisCachePayload | null. Custom clients delegating to the exported decoders need zero logic changes; plain-JS stragglers fail loudly open (typed error →cache_read→ fallback), never mislabel.dialcache_miss_countergains areasonlabel — an old-schema collector in the same Prometheus registry now fails adapter construction (validateExistingCollectors).sum without (reason)totals are unchanged. Datadog gains an additive tag (wire-compatible).miss(labels: CacheMetricLabels)still compile; widen toMissMetricLabelsto consume the reason.Verification
pnpm checkgreen: typecheck, 447 unit tests (coverage 97.5% lines / 94.7% branches, gates pass), build, packed-tarball consumer test (incl. exhaustiveMissReasonrecord and ESM/CJS decoder checks assertingwatermark_invalidated)pnpm test:integrationgreen: 113 tests against real Redis 6.2, Valkey 8, and clusterinvalidateRemote, malformed-outcome guard (5 shapes), fenced shadow-read fill parity, Prometheus/Datadog exhaustive-enum guards