feat(server): per-reason prompt-cache reject metrics and APC trace log#810
Merged
Conversation
Prompt-cache observability previously counted inserts, hits, and a single reject class (`rejects_oversized` in prompt_cache/metrics.rs, `prompt_cache_insert_rejects` in batch/observability.rs), with no visibility into why a donate-back or adopt attempt was declined.
Add a fixed seven-reason breakdown (`oversized`, `disabled`, `prefix_too_short`, `mode_mismatch`, `empty_set`, `layout_constraints`, `block_boundary_floor`) grounded in the real decline sites read from `donate_finished_sequence_cache` and `try_adopt_cached_prefix` in batch/scheduler.rs, plus a `last_reject` snapshot (reason, seq_id, context_len, timestamp) behind a `Mutex`. The shared `PromptCacheRejectReason`/`PromptCacheRejectCounters` types live in prompt_cache/metrics.rs and are reused by both `AtomicPromptCacheMetrics` (store-level, the three reasons the store itself can detect) and `BatchObservability` (scheduler-level, the full set including adopt-only declines the store never sees).
The store's `insert`/`insert_snapshot` now classify all three `InsertError` variants through the new `PromptCacheMetrics::record_reject` hook (previously only `OversizedEntry` reached any hook). The scheduler records a reason at every real decline site in both functions, including the previously untracked empty-set and pre-detach prefix-too-short short-circuits.
`/v1/cache/stats` (routes/cache.rs) gains `reject_*` counters and `last_reject_*` fields sourced from the batch observability snapshot, independent of whether the store is enabled. The Prometheus exposition (routes/metrics.rs) gains `mlxcel_prompt_cache_reject_total{reason="..."}`, matching the existing `mlxcel_prompt_cache_evictions_total` labeling pattern.
`MLXCEL_APC_TRACE=1` enables `tracing::info!` one-liners at store, adopt, and reject events with sequence id, matched/context length, and reason, greppable via `apc_event=`. The flag is read once through a `OnceLock` (`apc_trace_enabled()` in batch/observability.rs) so the hot path pays a single relaxed load, not a per-event env lookup.
The optional load-time dry-run cacheability self-check from the issue is skipped; it needs model-specific cacheability knowledge that does not exist as a callable check today, so it is not a cheap addition. Left as a follow-up.
Tests: reason counters increment on forced store declines (prompt_cache/store_tests.rs), the scheduler-facing reason/last-reject bookkeeping (batch/observability.rs, batch/scheduler_prompt_cache_tests.rs), and the extended `/v1/cache/stats` response shape (routes/cache.rs, routes/cache_tests.rs, prompt_cache/apc_integration_tests.rs).
inureyes
added
type:enhancement
…etry Add the MLXCEL_APC_TRACE row to the "Logging, profiling, and capture variables" table in docs/environment-variables.md, addressing the pr-reviewer MEDIUM on PR #810. States precisely that only the literal value `1` enables it (an exact string match, unlike the presence-based switches around it), that it is read once via OnceLock effectively at first use, and that it emits one tracing info line per prompt-cache store, adopt, and reject event with seq id, lengths, and reason. Also addresses two reviewer LOW comments with one-line code comments, no behavior change: - src/server/batch/scheduler.rs: note in try_adopt_cached_prefix that a sequence-slot allocation failure during snapshot restore is intentionally not recorded as a prompt-cache reject, since it is scheduler capacity exhaustion rather than a cache-specific decline. - src/server/prompt_cache/metrics.rs: note on the PromptCacheMetrics::record_reject default method that the production adapter (BatchMetricsCacheAdapter) leaves this hook at its no-op default, so /v1/cache/stats and Prometheus reject-reason exposition is fed solely by the scheduler's BatchObservability path, not by this store-level hook. Validation: - cargo fmt --all -- --check (clean; doc and comment-only changes, no build needed) Refs #774
Member
Author
PR Finalization CompleteSummary
Pushed as |
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.
Summary
Prompt-cache observability previously counted inserts, hits, and a single reject class with no visibility into why a donate-back or adopt attempt was declined. This adds a fixed per-reason reject breakdown, a last-reject diagnostic snapshot, and an opt-in trace log, surfaced through both
/v1/cache/statsand the Prometheus exposition.What changed
src/server/prompt_cache/metrics.rs: newPromptCacheRejectReasonenum (7 fixed reasons:Oversized,Disabled,PrefixTooShort,ModeMismatch,EmptySet,LayoutConstraints,BlockBoundaryFloor),PromptCacheLastRejectsnapshot struct, andPromptCacheRejectCounters(atomic counters plus aMutex-guarded last-reject slot), shared by the store-level and scheduler-level metrics.PromptCacheMetricsgains arecord_rejecthook;AtomicPromptCacheMetricsembeds areject_reasons: PromptCacheRejectCountersfield.src/server/prompt_cache/store.rs:insert/insert_snapshotnow callrecord_rejectfor all threeInsertErrorvariants (Disabled,PrefixTooShort,OversizedEntry) instead of onlyOversizedEntry.src/server/batch/observability.rs:BatchObservabilitygainsprompt_cache_reject_reasons: PromptCacheRejectCounters,record_prompt_cache_reject(reason, seq_id, context_len), the correspondingObservabilitySnapshotfields (7 per-reason counts plusprompt_cache_last_reject), andapc_trace_enabled()(env read once viaOnceLock).src/server/batch/scheduler.rs: every real decline site intry_adopt_cached_prefixanddonate_finished_sequence_cachenow classifies and records a reason (snapshot-restore failure,require_whole_entrypartial-match decline, paged clone decline, backend mismatch, empty detached/kv sets, dense truncate failure, paged block-floor and trim failures, finaladopt/adopt_pagedfailure, pre-detach prefix-too-short screens, and the store-levelInsertErroron both insert paths).MLXCEL_APC_TRACE=1-gatedtracing::info!one-liners added at store, adopt, and reject events.src/server/routes/cache.rs:/v1/cache/statsgainsreject_oversized,reject_disabled,reject_prefix_too_short,reject_mode_mismatch,reject_empty_set,reject_layout_constraints,reject_block_boundary_floor, andlast_reject_*fields, sourced from the batch observability snapshot (populated even when the store is disabled, matching the existing paged-pool field convention).src/server/routes/metrics.rs: Prometheus gainsmlxcel_prompt_cache_reject_total{reason="..."}across the 7 reasons, next to the existingmlxcel_prompt_cache_evictions_totalfamily.src/server/batch/mod.rs: re-exports the newPromptCacheLastRejectSnapshottype.src/server/prompt_cache/store_tests.rs,src/server/batch/observability.rs,src/server/batch/scheduler_prompt_cache_tests.rs,src/server/routes/cache_tests.rs,src/server/prompt_cache/apc_integration_tests.rs(updated call sites and new coverage for the reason breakdown, last-reject snapshot, and the extended/v1/cache/statsshape).The optional load-time dry-run cacheability self-check mentioned in the issue is skipped: it would need model-specific cacheability knowledge that has no existing callable hook, so it is not a cheap addition. A follow-up issue is suggested if that stretch goal is still wanted.
Test plan
cargo test --release --features cuda --lib -- server::prompt_cache server::batch::scheduler_prompt_cache_tests server::batch::observability server::routes::cache(195 passed, 0 failed)Closes #774