Repository navigation
Add L2 cache storage observability - #4393
Closed
larry-dalmeida wants to merge 27 commits into
Closed
larry-dalmeida wants to merge 27 commits into
larry-dalmeida wants to merge 27 commits into
Conversation
Implements fine-grained observability for cache filter results by HTTP
status code class via ScopedPrometheusRegisterer.
New metrics:
- skipper_cache_result_total{result, code_class} counter
- skipper_cache_lru_bytes{route} gauge
Cardinality bounded: 3 result types × 4 code_class values = 12 series max.
Backward compatible: existing flat-key counters still emitted.
LWSRE-214
Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Add comprehensive tests for new labeled cache metrics across all code_class values and cache result types (hit/miss/stale). Document new skipper_cache_result_total and skipper_cache_lru_bytes metrics in operation guide. LWSRE-214 Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
…2_get_error
Request()'s Vary-sentinel check ("vary:"+baseKey) was routed through the
regular, metrics-instrumented Storage.Get(). That lookup runs on every
single request, regardless of whether a real cache decision is being
made, so it was counting as a cache outcome on every request -
inflating l1_hit/l2_hit/l2_miss/l2_get_error (plain and labeled) with
internal plumbing noise rather than real caching behavior.
Add Storage.GetSilent(), implemented by both LRUStorage (delegates to
Get - it records no metrics of its own) and L2Storage (shares Get's
logic via a new internal get(recordMetrics bool), skipping only the
outcome-metric calls; L1 write-through warming still runs on a silent
hit, since that's a caching side effect of the read, not an outcome
metric). filter.go's sentinel lookup now calls GetSilent instead of Get.
LWSRE-214
Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
coalesce() had zero timing instrumentation despite being the thundering- herd protection point for cold misses. Measure from right after the singleflight DoChan call to when the result channel yields - near-zero for the leader (it computes the result itself), the leader's real fetch time for followers waiting on it. Same MeasureSince/customHistogramM mechanism as the existing cache.reval_wait_duration/cache.reval_duration. LWSRE-214 Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
L2Storage only had pass/fail counters (l2_hit, l2_miss, l2_get_error, l2_set_fallback) - no timing. Measure the underlying Valkey/Redis Get/SetWithExpire round-trip directly, on every call including internal lookups like the Vary-sentinel check, via the existing MeasureSince/ customHistogramM mechanism (same convention as cache.reval_wait_duration/cache.reval_duration/cache.coalesce_wait_duration). Verified locally against a real Valkey instance: -enable-l2-cache alone is not sufficient to exercise this - selectL2CacheClient only runs inside skipper.go's `if o.EnableSwarm` block, so -enable-swarm is also required or L2Storage is never constructed and cache() silently falls back to L1-only. Noted this in the local verification setup's README. LWSRE-214 Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
cache.storage_error was a single unlabeled counter incremented from 9
generic call sites in filter.go, none of which know whether f.storage is
LRUStorage (L1) or L2Storage (L2) - backend was unknowable there. 4 of
those 9 (all on the Delete path) could never fire: LRUStorage.Delete
always returns nil, and L2Storage.Delete only ever returns nil to its
caller too (L1 delete always runs after a best-effort L2 delete).
Added IncStorageError(backend, op) -> skipper_cache_storage_error_total
{backend, op}, called from the storage implementations themselves where
backend is actually known: LRUStorage.Set/Get (encode/decode failure),
L2Storage.Set/Get (encode/decode failure), L2Storage.Delete (the real L2
failure, previously only visible as the generic unlabeled counter).
IncStorageError also increments the pre-existing flat cache.storage_error
counter and the labeled skipper_cache_result_total{result="storage_error"}
metric from this one call site, so existing consumers of either see zero
behavior change - this only adds backend/op attribution going forward.
Removed filter.go's 9 generic increments now that the storage layer
self-reports (keeping both would double-count). Updated the 7 existing
tests that asserted on those generic increments via a backend-opaque
test double (failingSetStorage/failingDeleteStorage) - a failure from
code that isn't a real LRUStorage/L2Storage genuinely can't be
attributed, so they now assert no metric fires, only that the request
is still served and nothing panics. Added real attribution tests for the
LRUStorage/L2Storage failure paths.
LWSRE-214
Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
coalesce() clones ctx.Request() (a server-received request, which carries RequestURI and a relative URL) and passes it straight to f.fetch (http.Client.Do). The client unconditionally rejects any request with RequestURI set, so the leader's fetch inside the singleflight closure fails on every single cold miss. coalesce() then returns without calling ctx.Serve(), silently falling through to skipper's normal backend-proxy pipeline instead - which resolves the backend correctly on its own, so client-visible correctness is unaffected and the bug has been invisible. The real effect: singleflight cold-miss coalescing has never actually worked. Concurrent requests to the same cold key each independently fall through to the normal path instead of being deduplicated to one origin request - origin-request amplification under concurrent load. Verified locally: 10 concurrent requests to a fresh cold key produced 10 origin hits before this fix, 1 after. Fix mirrors the already-correct pattern a few hundred lines down in this same file, in doRevalidate's direct-dispatch branch: resolve ctx.BackendUrl(), set req.URL.Scheme/Host, clear req.RequestURI. Fixing only that regressed TestProxy_L1Cache_VaryStar_NeverCached: storeDecision() (coalesce()'s leader path only calls this one, not Response()'s separate storage path) was missing the Vary: * never- cache check Response() already has, and had simply never been exercised before now - coalesce()'s fetch always crashed first. Added the same check to storeDecision(), the one choke point both callers share. Added a regression test using a real httptest backend + proxytest, not a stub: every existing Coalesce* test in this file stubs f.fetch directly, which bypasses net/http's client-side RequestURI validation entirely and cannot catch this class of bug. Confirmed the new test fails (10 backend calls, not 1) with the fix reverted. Pre-existing since the cache() filter's original commit (zalando#3991), through zalando#4033, zalando#4315, zalando#4344 - not introduced by any of those. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com> Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Share validated request preparation between cold-miss coalescing and background revalidation. Preserve filtered direct requests and outgoing Host, replay original requests for RFC and unresolved backends, and report invalid backend URLs before pipeline fallback. Use singleflight join barriers for deterministic concurrent tests, cover real-client dispatch and dynamic routing, and prohibit sharing Vary-star responses. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Buffer independent nonstoreable follower responses before closing origin streams. Preserve response metadata and count body-read failures as bypass errors without serving incomplete payloads. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com> Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Reproduce the cache-first Host and path contract violation through a real proxy, and verify that Host/prefix rewrites before cache preserve cold and warm behavior. Document the required ordering without changing the executor or external deployment routes. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com> Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Demonstrate that a response filter after cache is skipped on direct cold, warm, and stale paths. Verify moving it before cache applies client Cache-Control without mutating stored origin directives. Runtime and deployment sources unchanged. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com> Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Serve eligible stale responses on network and body-read errors, timeouts, and HTTP 500/502/503/504. Preserve their original storage timestamps through failed foreground and background refreshes, without extending the SIE window. Keep normal negative caching when no eligible stale response exists. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com> Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Require matching Vary dimensions before sharing cacheable cold responses, including when Authorization is in the cache key. Route mismatched cacheable followers through their variant singleflight key. Share variant entry and sentinel persistence across cold fetches, Response, and background refresh; retain the sentinel through SIE. Cover concurrent and warm Accept-Language requests through a real proxy, with and without Authorization keys. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com> Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Scale the full-cycle sequence with virtual time: seed a Vary response, restart the process, optionally wipe volatile L2, then age beyond freshness plus SWR. Retained L2 serves stale with original timestamps and 27s TTL for 3s fresh plus 24s SIE; losing both tiers correctly yields origin500/MISS. No runtime TTL changes. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com> Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Recheck the exact variant silently inside revalidation singleflight. Skip only fresh entries stored since queue admission, preserving retries after failed refreshes without retaining an in-flight guard. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com> Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Reject standalone 304 storage in the shared admission helper so RFC loopback Response cannot replace a stored representation with an empty conditional response before the refresh worker merges it. Keep validators and variant keys unchanged. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com> Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Cover both omitted Content-Length and selected representation length for en/de variants in RFC loopback and force direct/loopback modes. Both valid forms reproduce the baseline RFC empty-body overwrite. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com> Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Wire TCP dial, response-header, TLS handshake, and expect-continue backend options to the cache client transport. Preserve dial-only semantics instead of using http.Client.Timeout. Match proxy defaults for zero header/expect-continue settings. Reproduce the ignored header deadline and verify timeout SIE retention, unchanged timestamps, and healthy recovery using a real delayed origin. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com> Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Add skipper_cache_lru_entries{route} and skipper_cache_lru_capacity_bytes gauges. ShardedByteLRU.Usage returns bytes and entry count together; Bytes delegates to it.
Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Count hit, stale, and miss outcomes per route ID and record the time from process start to the first cache hit. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
- Route revalidation outcomes (started/success/failed/dropped) - Origin fetch duration by route and status class - Cache-Control directive tracking from origin responses These metrics instrument cache operations across routes and origin interactions. Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Signed-off-by: Larry D Almeida <hello@larrydalmeida.com>
Signed-off-by: larry-dalmeida <hello@larrydalmeida.com>
larry-dalmeida
added a commit
that referenced
this pull request
Oct 29, 2026
Integrate request-serving provenance and purpose-scoped L2 metrics from #4392 and #4393 while retaining the consolidated cache correctness fixes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Larry D Almeida <20266078+larry-dalmeida@users.noreply.github.com>
Collaborator
Author
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
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.
Overview
Implementation of L2 cache storage observability for Skipper, providing physical storage operation metrics (GET/SET/DELETE) separate from cache lookup outcomes.
Changes
GetSilentto exclude them from legacy lookup countersTesting
make lintpassesmake shortcheckincomplete (Docker daemon instability)Implementation Details
Files Modified
filters/cache/storage.go: Added l2Purpose type/helpers, updated GetSilent docsfilters/cache/l2_storage.go: Added operation/error recording, replaced GET/SET/DELETE timingfilters/cache/filter.go: Wrapped all 15 storage call sites with purpose, converted 3 reads to GetSilentmetrics/metrics.go: Added CacheL2Observer interfacemetrics/prometheus.go: Added two CounterVec collectors + observer methodsmetrics/all_kind.go: Added observer forwardingmetrics/metricstest/metricsmock.go: Added mock methodsdocs/operation/operation.md: Added operational documentationRelated
This is an orthogonal change to request-tier attribution; a separate PR would add per-request L1/L2 tier outcomes for accurate dashboard ratios.