perf(metrics): per-worker handle cache for every per-request emit path - #925
Conversation
Replace the per-emit macro path (per-label String allocations, Key build, KeyHasher run, and a sharded-registry probe on every increment) and the two shared RwLock series caches with one thread-local handle cache keyed by instance, site, and label values. Under thread-per-core, a per-worker cache is the natural shape: the steady-state emit is one FNV hash over a reused key buffer plus one map probe, with no shared state touched beyond the series' own value atomics. The shared-lock alternative was measured recovering almost nothing (+0.5% throughput on the c=128 saturation grid) because the lock word bounces between workers on every emit; the thread-local variant recovered +4.4% on the same grid (AISIX-Cloud#1259 item 3b spike). Semantics are unchanged: same series names, label sets, and values. - Request/usage series bundles keep per-field lazy registration, so proxy-only paths still never mint aisix_llm_* series - Eviction (safety-valve cap of 1024 entries per map per worker) drops only our handle; re-registering resumes the same registry series - A label value containing the key separator byte falls back to the uncached emit instead of risking key aliasing - Cache keys carry a per-instance id, so parallel tests with multiple recorders cannot cross-serve handles
Follow-ups from the first A/B profile and the adversarial review: - in-flight bookkeeping: replace the Mutex<HashMap<(String, String)>> (two String allocations plus SipHash per request edge) with a linear scan over a bounded slot vector - no allocation on the steady state - pre-format the per-instance cache-key prefix once instead of running fmt::write on every emit; hand-roll the status-code digits for the same reason - add exposition tests pinning the deployment counter, budget gauge, and legacy request families against key/label drift: two label sets each, asserting distinct fully-labelled series (review findings: all three families previously had no multi-label-set coverage, so a key builder dropping a label would alias series and ship green)
📝 WalkthroughWalkthroughChangesThe metrics module replaces shared lock-based caches with bounded thread-local worker caches. Metric handles initialize lazily, cache keys use instance-scoped prefixes, unsafe separator values use an uncached path, and all metric emission paths use cached helpers. Tests cover eviction, reuse, isolation, labels, concurrency, and legacy behavior. Metrics cache migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Introduces per-worker metric-handle caching to reduce allocation, hashing, and lock contention on metric emission paths.
Changes:
- Adds thread-local, bounded handle caches with reusable keys.
- Replaces shared request/usage caches and optimizes in-flight tracking.
- Expands cache correctness and exposition tests.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // `metrics::counter!`-family macros rebuild a `Key` (one owned `String` per | ||
| // label), hash it, and probe the recorder's sharded registry on EVERY emit. | ||
| // `metrics::Counter`/`Gauge`/`Histogram` are `Arc`-backed handles wired | ||
| // straight to the series' storage, so registering once per label set and | ||
| // reusing the handle removes all of that from the steady-state path. |
There was a problem hiding this comment.
Fair catch — narrowed. The title and description now say per-REQUEST emit paths, and 6e8c434 documents on sync_config_status why it deliberately stays on the macro path: it runs once per scrape rather than per request, and its zeroing discipline works on churning label sets (hash_info, per-kind rejected/partial/stale) — exactly the shape a first-seen handle cache handles worst. Routing it through the cache would add complexity for zero hot-path benefit.
![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 (1)
crates/aisix-obs/src/metrics.rs (1)
3365-3376: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd coverage for the new in-flight slot scan.
The in-flight bookkeeping changed from a map to a linear scan over
Vec<(String, String, i64)>. Two behaviours of that scan have no test:
- The zero clamp at line 1115 and line 1118. A decrement without a matching increment must leave the gauge at 0, not at -1.
- The slot predicate
e == endpoint && p == inbound_protocol. If a regression compares onlyendpoint, two protocols share one slot. Every current test uses a single pair, so the regression would pass.
in_flight_gauge_returns_to_zerocovers only the balanced single-pair case.As per coding guidelines: "Define verifiable success criteria, write regression tests for bugs and invalid inputs, and verify each implementation step."
💚 Proposed tests
#[test] fn in_flight_gauge_clamps_an_unmatched_decrement_at_zero() { let m = Metrics::new(false); m.decrement_proxy_in_flight("/v1/chat/completions", "openai"); let rendered = m.render(); let line = rendered .lines() .find(|l| l.starts_with(M_PROXY_IN_FLIGHT)) .expect("in-flight gauge must render"); assert!(line.ends_with(" 0"), "unmatched decrement must clamp: {line}"); } #[test] fn in_flight_slots_stay_distinct_per_endpoint_and_protocol() { let m = Metrics::new(false); m.increment_proxy_in_flight("/v1/chat/completions", "openai"); m.increment_proxy_in_flight("/v1/chat/completions", "anthropic"); m.increment_proxy_in_flight("/v1/messages", "anthropic"); m.decrement_proxy_in_flight("/v1/chat/completions", "anthropic"); let rendered = m.render(); let value = |endpoint: &str, protocol: &str| { rendered .lines() .find(|l| { l.starts_with(M_PROXY_IN_FLIGHT) && l.contains(&format!("endpoint=\"{endpoint}\"")) && l.contains(&format!("inbound_protocol=\"{protocol}\"")) }) .and_then(|l| l.rsplit(' ').next()) .map(str::to_owned) }; assert_eq!(value("/v1/chat/completions", "openai").as_deref(), Some("1")); assert_eq!( value("/v1/chat/completions", "anthropic").as_deref(), Some("0") ); assert_eq!(value("/v1/messages", "anthropic").as_deref(), Some("1")); }🤖 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 `@crates/aisix-obs/src/metrics.rs` around lines 3365 - 3376, Extend the metrics tests near in_flight_gauge_returns_to_zero with coverage for unmatched decrements clamping at zero and for distinct in-flight slots keyed by both endpoint and inbound protocol. Add cases that decrement without incrementing, and that use multiple endpoint/protocol combinations before decrementing one pair; assert rendered labels and values confirm only the matching slot changes.Source: Coding guidelines
🤖 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 `@crates/aisix-obs/src/metrics.rs`:
- Around line 1096-1142: Update increment_proxy_in_flight,
decrement_proxy_in_flight, and in_flight_delta so the updated proxy in-flight
value is published through set_proxy_in_flight_gauge while the proxy_in_flight
lock remains held. Avoid returning the value for a later gauge update outside
the critical section, and preserve the existing clamping and shared gauge
behavior.
---
Outside diff comments:
In `@crates/aisix-obs/src/metrics.rs`:
- Around line 3365-3376: Extend the metrics tests near
in_flight_gauge_returns_to_zero with coverage for unmatched decrements clamping
at zero and for distinct in-flight slots keyed by both endpoint and inbound
protocol. Add cases that decrement without incrementing, and that use multiple
endpoint/protocol combinations before decrementing one pair; assert rendered
labels and values confirm only the matching slot changes.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 18bd9119-1f9e-48ec-8c29-9f7f812d51f4
📒 Files selected for processing (1)
crates/aisix-obs/src/metrics.rs
The independent PR audit demonstrated by mutation that two of the three aliasing-guard tests could not catch a single dropped key label: their label-set pairs differed in several dimensions at once, so removing one label still produced distinct cache keys and the suite stayed green. Add per-family vary-one-label tests: a base label set emitted twice plus one variant per label differing only in that label, asserting one rendered series per label set. Covered families: the request and usage series bundles, the legacy request pair (counter and duration keys separately), budget gauges, LLM time-to-first-token, the SLO latency histograms, auth decisions, guardrail latency, rate-limit rejections, tokens-by-client, consumed tokens, and the in-flight gauge. The three mutations that previously survived are now caught (verified by re-running them). Also corrects the two test comments that overclaimed drift protection and documents the bounded-endpoint precondition on in_flight_delta (audit LOW-3).
Independent audit dispositionA cold independent audit (no shared context with the author session) reviewed this PR across correctness, reliability, security, sensitive-info leakage, breaking changes, and test coverage, including mutation testing of the new guards. Findings and dispositions:
No HIGH findings. The audit also verified: all 27 converted emit sites have complete key/register label parity, exposition label tuples are structurally identical to base across all 39 metric families, the RefCell borrow discipline has no re-entry path, FNV's lack of DoS resistance is fenced by operator-bounded label values plus the per-map cap, no public API was removed, and the 1,108-test claim (now 1,121 with the new guards) reproduces including single-threaded runs. |
- Publish the in-flight gauge while the slot lock is held: with the emit outside the critical section, two concurrent edges on one pair could publish out of order and strand a stale value on an endpoint that then goes idle. No path takes this lock from inside the worker cache, so emitting under it cannot deadlock. - Add the two missing in-flight tests: an unmatched decrement clamps the gauge at zero, and the slot predicate matches on BOTH endpoint and protocol (a single-field regression would corrupt values while the series-count guard stayed green). - Document why sync_config_status stays on the macro path: it runs per scrape, not per request, and its zeroing discipline works on churning label sets - the shape a first-seen handle cache handles worst.
membphis
changed the title
|
Re the outside-diff-range review note (in-flight slot-scan coverage): both proposed tests are taken in 6e8c434 — in_flight_gauge_clamps_an_unmatched_decrement_at_zero pins the zero clamp, and in_flight_slots_stay_distinct_per_endpoint_and_protocol pins the two-field slot predicate with a value-level assertion (the existing series-count guard alone would stay green under a single-field predicate regression). Now 1,123 tests passing. |
What
Replaces the per-emit metrics-macro path — per-label
Stringallocations, aKeybuild, a KeyHasher run, and a sharded-registry probe on every increment — and the two shared-RwLockseries caches with one thread-local handle cache keyed by(instance, site, label values)over a reused key buffer (FNV-1a). Also slims the in-flight gauge bookkeeping (aMutex<HashMap<(String, String)>>that allocated two keyStrings and ran SipHash per request edge) to a linear scan over a bounded slot vector.Series names, label sets, and values are unchanged. The exposition is asserted byte-equivalent by tests; the benchmark legs also diffed
/metricssnapshots (67 series, zero diff).Why thread-local
The gateway runs thread-per-core, so per-worker duplication of a bounded handle set is the natural shape. The spike for AISIX-Cloud#1259 item 3b measured the alternatives on the same rig, same binary, same session (c=128 saturation, 4 valid windows each, fail=0):
RwLockThe global-lock variant recovers almost nothing: the lock word bounces between workers on every emit, and replacing the registry's hash+probe with another shared hash+probe is a wash. Removing the shared state is the whole win. This mirrors how the metrics ecosystem already leans:
metrics' own handles are designed for registration reuse, a mainstream Rust gateway ships a handle cache in front of the same recorder, and the rust-prometheus local-metrics API / large Rust infrastructure projects pre-resolve handles off the hot path entirely.Measured result (same rig, anchored before/after, c=128, 4 windows/leg, fail=0)
Round 2 (front/back anchor drift +0.9%):
Round 1 (before the in-flight/key-build follow-up commit) measured -3.13us on the same basis. Memory is flat (idle RSS and HWM unchanged). Spike prediction for this mechanism was 5-6us; the remainder is histogram sample storage, deliberately split out to #924 (different mechanism, and its largest share is allocator arena contention that the planned allocator spike would double-claim).
Behavior notes
aisix_llm_*series.Tests
1,123 passing across aisix-obs/proxy/server, including new pins: eviction-continues-counting, instance isolation, separator fallback, cross-thread accumulation, and vary-one-label key-drift guards for every cached family (request/usage bundles, the legacy pair's counter and duration keys separately, budget, TTFT, SLO latency, auth, guardrail latency, rate-limit rejections, tokens-by-client, consumed tokens, in-flight, deployment).
The independent audit for this PR demonstrated by mutation that the first round of aliasing-guard tests could not catch a single dropped key label (their label-set pairs differed in several dimensions at once); the vary-one-label suite replaces that claim with one variant per label differing in exactly that label, and the three previously-surviving mutations are now caught (re-verified). Full audit disposition in the PR comments.
Summary by CodeRabbit
Performance
Reliability
Observability