feat(libsy): count classifier fail-open fallbacks on /metrics - #205
feat(libsy): count classifier fail-open fallbacks on /metrics#205elyasmnvidian wants to merge 1 commit into
Conversation
52c8d5b to
2becceb
Compare
WalkthroughChangesThe judge classifier now categorizes fail-open errors, logs them, and records a metric tagged by model and reason. Integration tests cover failure modes and confirm valid verdicts are not counted. Classifier fail-open telemetry
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/libsy/tests/observability.rs (1)
858-863: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: no case covers
timeoutor the catch-all reasons.
client_error_reasonmapsTimeout→"timeout"and falls back to"client_error"/"call_error"; neither label is exercised. AJudgeOutcome::CallError(timeout_error)case would close the boundary most likely to be hit in production.🤖 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/libsy/tests/observability.rs` around lines 858 - 863, Extend the observability test cases using the JudgeOutcome enum to cover client_error_reason’s Timeout mapping and fallback labels. Add a JudgeOutcome::CallError case that produces a timeout error and assert the expected "timeout" reason, while also exercising the catch-all client-error/call-error fallback as appropriate.crates/libsy/src/observability.rs (1)
221-232: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the fail-open counter handle
meter().u64_counter(...).build()runs on every fail-open, adding avoidable overhead on a hot outage path. ALazyLock<Counter<u64>>(or similar cached handle) would keep recording cheap; this test setup installs the global meter provider once beforeinitialize_metrics(), so a process-wide handle should fit.🤖 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/libsy/src/observability.rs` around lines 221 - 232, Cache the counter handle used by record_classifier_fail_open in a process-wide LazyLock (or equivalent), initializing it with meter().u64_counter("switchyard.classifier_fail_open").build() once. Update record_classifier_fail_open to reuse the cached handle while preserving the existing value and judge_model/reason labels.
🤖 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.
Nitpick comments:
In `@crates/libsy/src/observability.rs`:
- Around line 221-232: Cache the counter handle used by
record_classifier_fail_open in a process-wide LazyLock (or equivalent),
initializing it with
meter().u64_counter("switchyard.classifier_fail_open").build() once. Update
record_classifier_fail_open to reuse the cached handle while preserving the
existing value and judge_model/reason labels.
In `@crates/libsy/tests/observability.rs`:
- Around line 858-863: Extend the observability test cases using the
JudgeOutcome enum to cover client_error_reason’s Timeout mapping and fallback
labels. Add a JudgeOutcome::CallError case that produces a timeout error and
assert the expected "timeout" reason, while also exercising the catch-all
client-error/call-error fallback as appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: daa96b57-cce4-48f5-a503-07972298ded6
📒 Files selected for processing (3)
crates/libsy/src/algorithms/util/llm_judge.rscrates/libsy/src/observability.rscrates/libsy/tests/observability.rs
7f02755 to
1e763a9
Compare
Signed-off-by: Elyas Mehtabuddin <emehtabuddin@nvidia.com>
1e763a9 to
c6f0a31
Compare
Problem
The judge model behind a classifier route returns an error:
What happens
The request still succeeds — the route falls open to the capable tier and serves — but
/metricsshows no sign that it happened:$ curl -s localhost:4000/metrics | grep classifier_fail_openNothing prints; only a warn log records it. A dashboard cannot tell a healthy hour from one where every request is routing without a verdict, quietly degraded to the fallback tier.
The fix
A judge is an optimization, not a dependency, so when it fails the classifier treats the result as "no verdict" and routes on the fallback tier rather than failing the caller. That fallback was invisible to monitoring. A
switchyard.classifier_fail_opencounter now increments on each fail-open, labeled with a boundedreasonand thejudge_model, so a judge outage shows up on/metrics.Evidence
Same 500, after the fix — the request still returns 200 from the capable tier, and the fallback is counted:
reasonis one of a bounded set —timeout,transport,upstream_4xx,upstream_5xx,invalid_response,parse_error,client_error— read only from the error kind and HTTP status. A valid verdict is a real routing decision, not a fail-open, and is not counted. This is an observability change, not a performance change, so there is no benchmark.How tested
crates/libsy/tests/observability.rs, driving a realLlmTaskClassifier:classifier_fail_open_is_counted_by_reason— the judge fails seven ways (timeout, transport, HTTP 4xx/5xx, unparseable reply, a streamed reply that fails to decode part way, and an uncategorized client error); asserts each falls open to the capable target, still serves, and increments the counter once with the matching reason. The streamed case reads the judge stream to completion, so all three fail-open points are covered.valid_verdict_is_not_counted_as_a_fail_open— a valid verdict whose low solve-probability routes to the same capable tier a fail-open would, so the counter — not the routed tier — is what proves nothing was counted.Notes for reviewers
x-switchyard-fallbackheader is a natural follow-up; it needs a small trait bound so the generic fall-through router can carry the reason onto its decision for the server to stamp, so it is kept out to keep the counter self-contained.