Repository navigation
feat(metrics): add bounded counters for listen WS handshake, API 429, and direct LLM quota (#21461) - #23520
feat(metrics): add bounded counters for listen WS handshake, API 429, and direct LLM quota (#21461)#23520ducanh3005 wants to merge 3 commits into
Conversation
… and direct LLM quota (BasedHardware#21461)
Payout & Verification Info (
|
Git-on-my-level
left a comment
There was a problem hiding this comment.
Verdict: the counters themselves are well designed — bounded label sets, startup zero-initialization, real tests — and one mechanical CI blocker (Hygiene line-count ratchet) needs clearing before merge.
Findings:
- backend/utils/metrics.py — all three counters pre-create their full label cross-product and the record_* helpers collapse unknown values to other/custom, so there is no cardinality-growth path. Maintainability note: _API_RATE_LIMIT_POLICY_FAMILIES hardcodes the family list instead of deriving it from RATE_POLICIES in utils/rate_limit_config.py — a new policy family added there will silently land in policy_family="custom". Consider deriving the set from RATE_POLICIES or adding a parity test.
- backend/utils/other/endpoints.py — placement is right on both paths: the redis_policy increment sits after the RATE_LIMIT_SHADOW early-return (shadow traffic is not counted as throttling), and rate_limit_custom increments only on the actual 429 raise. Keys/uids are never used as labels (family prefix only) — no identifiers leak into metrics.
- backend/routers/transcribe.py — increments inside the existing except RuntimeError handlers before return; no behavior change beyond the metric.
- backend/utils/llm/byok_errors.py — _classify_rate_limit_or_quota_error mirrors the existing classifier without changing classify_byok_llm_error, and records platform as well as byok sources. Minor: any 429 whose message merely contains "quota" (e.g. "quota resets at midnight") is classified reason="quota" — fine for alerting, just be aware the quota/rate_limit split is heuristic.
- backend/tests/unit/test_listen_quota_handshake_metrics.py — good coverage: zero-init children, label collapse, both WS surfaces, both rate-limit scopes, platform/byok LLM paths; the endpoints.cached mutation is cleaned up in a finally block.
Blocking (Hygiene): the product-file-line-count ratchet fails because backend/utils/metrics.py grew 1523 -> 1594. Either add a line like:
Line-Count-Exception: backend/utils/metrics.py | 1523 -> 1594 |
to the PR body, or move the new counters into their own module if you prefer a split.
Not your fault (Backend unit suite): all 6 shards failed on 5 pre-existing pyright errors in utils/retrieval/tools/omi_tools.py, utils/retrieval/tools/result_bounds.py, and utils/wrapped/generate_2025.py — the same errors failed main shortly before your CI ran, and main has since gone green. Rebasing onto current main (or re-running after a rebase) should clear it.
Performance: negligible — roughly a hundred pre-created series, increments only on failure paths. Blast radius: backend metrics/alerting only; a wrong label merely misroutes an alert.
USER IMPACT: none — internal observability change; no user-visible behavior differs.
What I'd still want as a reviewer: the policy-family parity guard (or derivation from RATE_POLICIES), and a green Backend unit suite on the rebased head. Beyond that, the new metric names and label contracts are what #21461's alerting rules will key on, so a maintainer should sign off on those names before merge.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
|
Thanks for the review @Git-on-my-level! Addressed all points in
|
Closes #21461
Failure-Class: none
Line-Count-Exception: backend/utils/metrics.py | 1523 -> 1592 | Central Prometheus registry owns bounded WS handshake, API 429, and direct LLM quota counters (#21461).
Summary
Adds bounded, startup zero-initialized Prometheus counters to cover the three alerting gaps after pausing raw-line Loki log rules (#21461):
omi_listen_ws_handshake_failures_total{surface,reason}(backend/utils/metrics.py,backend/routers/transcribe.py):surface(mobile,web, fallbackother) andreason(accept_runtime_error, fallbackother) labels, startup zero-initialized.websocket.accept()RuntimeErrorhandlers in_listen(surface='mobile') andweb_listen_handler(surface='web').omi_api_rate_limit_throttled_total{scope,policy_family}(backend/utils/metrics.py,backend/utils/other/endpoints.py):scope(redis_policy,ip_window, fallbackother) andpolicy_family(derived directly fromRATE_POLICIESinutils/rate_limit_config.pyplus fallbackcustom), startup zero-initialized across all families.rate_limit_custom(scope='ip_window') and live (non-shadow)_enforce_rate_limit(scope='redis_policy').omi_direct_llm_rate_limit_errors_total{provider,source,reason}(backend/utils/metrics.py,backend/utils/llm/byok_errors.py):provider(openai,anthropic,gemini,openrouter,groq,other),source(platform,byok,other), andreason(quota,rate_limit), startup zero-initialized across all 6 providers.handle_llm_errorvia_classify_rate_limit_or_quota_errorwhile keepingclassify_byok_llm_errorbehavior unchanged.Verification
pytest backend/tests/unit/test_listen_quota_handshake_metrics.py -v(6/6 passing, includingRATE_POLICIESfamily parity check)