feat(observability): add opt-in verbose/DEBUG routing, failover, and discovery logging - #943
feat(observability): add opt-in verbose/DEBUG routing, failover, and discovery logging#943seonghobae wants to merge 3 commits into
Conversation
…discovery logging Every orchestrator/free pool-exhaustion incident this session (contextual- orchestrator#922, .github#1437) had to be root-caused purely from external CI evidence, because this gateway's core routing/failover/discovery decisions were completely invisible: orchestrator.py had zero logging statements anywhere, and even the four files that did call logging.getLogger(__name__) had no root logger configuration anywhere in the codebase, so .debug()/.info() calls were unconditionally dropped. Bridges TaskOrchestrator._append_audit_event (already secret-free, PII-encrypted) to the standard logging module at DEBUG, with an added redact_value pass as defense-in-depth. Adds a stage-by-stage failover_candidates_resolved funnel event (ranked count, ZDR/capability/tag exclusions, circuit-breaker fallback) and a candidate_pool_exhausted event (tried agent ids, final classified error) -- the exact evidence needed to distinguish "the pool is thin because of ZDR filtering" from "because of an open circuit breaker" from "because nvidia_nim and nvidia_nim_sub returned different catalogs" instead of only seeing an opaque final error. Circuit- breaker open/close transitions are logged. model_discovery.py logs per-provider credential-skip/success/failure and the shared Models.dev fetch's retry/recovery/exhaustion (the exact mechanism ADR 0041 hardened earlier without making it observable). Visibility is opt-in: --verbose/-v on the CLI, or CONTEXTUAL_ORCHESTRATOR_LOG_LEVEL for the CI sidecar (no .github changes needed). Fixed one real regression the full suite caught: a tool-fallback test asserted the audit stream contained only tool_fallback_decision events, which no longer holds now that failover_candidates_resolved is also recorded -- filtered to the relevant event type instead of loosening the new event. 20 new tests (secrets-never-leak assertions throughout, following test_telemetry.py's established caplog pattern). Full suite: 2820 passed, 1 skipped, 1 pre-existing unrelated failure (fast_mlsirm native module unavailable in this sandbox, confirmed identical with/without this change). 100% docstring coverage.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
|
This is not caused by this PR's diff. Re-running it now would just fail again identically (no verdict exists yet), so I'm not spending the one re-run on it; I'll keep this PR watched until the scheduler's retry lands a verdict and this goes green on its own. Generated by Claude Code |
… field python-logger-credential-disclosure flags any logger call mentioning "credential" as a potential secret leak. The flagged log line only ever logs source.credential_name (the KV registry key identifier, e.g. "OPENAI_API_KEY") -- never the resolved api_key value, which never reaches any log call in this module.
|
Fixed in 2d7977c: Reproduced the exact finding locally against this PR's head ( Generated by Claude Code |
…ndent-account contract test_bootstrap_selector_treats_nim_primary_and_sub_as_one_outage_domain asserted the pre-#941 behavior (select_bootstrap_discovered_agents collapsing nvidia_nim/nvidia_nim_sub into one provider family). 5224a4c (part of #941) deliberately removed that grouping -- documented in docs/product-technical-gap-baseline.md and ADR 0032 as superseded, since each credential's API key may be entitled to a different model catalog -- but left this one test asserting the old behavior, making it fail unconditionally on main. Verified this fails identically on a clean origin/main checkout with none of this PR's changes present. No production code changed; the test now asserts the documented, already-shipped independent-account contract.
|
Found and ported a fix for a pre-existing Root cause: commit Updated the test (no production code changed) to assert the current, documented, already-shipped independent-account contract: with This is filed here per the CI-red rule (base-branch failure ported into this PR rather than left blocking it), not scope creep on the logging feature — happy to also land it as its own PR against Generated by Claude Code |
|
Closing as redundant within this three-PR logging cluster (#942/#943/#946, all opened concurrently for the same feature) — keeping three competing full implementations open isn't productive.
To be clear about what this PR does that #946 currently doesn't: the Per triage, only #946 is staying open across this cluster. Thanks for the root-cause framing ( Generated by Claude Code |
|
Model taxonomy reminder: the requested grouping axis is model group ( |
Summary
orchestrator/freepool exhaustion has repeatedly surfaced as an opaque final error with no visibility into which candidates were tried, why they were excluded, or which provider actually failed. This PR makes the already-secret-free internal decision evidence in the routing/failover/discovery path visible in live process output, opt-in via--verbose/-vorCONTEXTUAL_ORCHESTRATOR_LOG_LEVEL=DEBUG.No root logger configuration previously existed anywhere in the repository (no
basicConfig,StreamHandler,setLevel,addHandler), so every existing.debug()/.info()call acrossserver.py,telemetry.py,video_jobs.py, andopenrouter_uptime.pywas silently unreachable. This closes that gap by adding_configure_logging()in__main__.py(env-var and--verbose-driven, idempotent, package-scoped) and new call sites at the actual decision points:TaskOrchestrator._append_audit_eventnow also emits its already-PII-protected event detail throughloggingat DEBUG (one extraredact_valuepass as defense-in-depth).TaskOrchestrator._failover_candidatesemits afailover_candidates_resolvedevent with a stage-by-stage exclusion funnel (disabled / ZDR-filtered / non-chat / tag-mismatch / circuit-open-fallback counts) — the exact evidence needed to tell whether a thinorchestrator/freepool is caused by ZDR-only mode, a capability/tag mismatch, an open circuit breaker, or genuinely few ranked candidates.TaskOrchestrator._invokeemitscandidate_pool_exhaustedwhen every candidate in a role's pool fails, naming the candidates tried and the final error kind._circuit_open,_record_failure,_record_success) logscircuit_breaker_opened(WARNING),circuit_breaker_auto_closed(INFO), andcircuit_breaker_closed(INFO) on actual state transitions only.model_discovery.py'sdiscover_provider_models/discover_all_modelslog per-provider skip/failure/completion (provider_discovery_skipped,provider_discovery_failed,provider_discovery_completed,discovery_run_completed), and the shared Models.dev retry loop logs each retry/exhaustion/recovery.This branch was rebased onto current
mainafter PR #941 (codex/all-key-discovery-latency-routing) landed its own independent, simpler--verbose/logging.basicConfig(level=logging.DEBUG)wiring and its own DEBUG-levelmodel_discovery.pylog lines in the same functions. The two were reconciled rather than layered:_configure_logging()(env-var precedence, idempotent handler attach, package-scoped) replaces the barelogging.basicConfigcalls at both--verbosecall sites, and the richer INFO/WARNING-level discovery events (withcredential_name,free_count, structured reason codes) replace #941's DEBUG-only duplicates of the same events, since the whole point of this PR is that this evidence needs to be visible by default in a--verboserun, not require chasing DEBUG output to find it.Explicitly not a fix for
orchestrator/freeexhaustion itself — this is the observability layer needed to empirically diagnose why it happens, since guessing at (or assuming) the cause — including whetherNVIDIA_NIM_API_KEYandNVIDIA_NIM_API_KEY_SUBactually expose identical model catalogs — is exactly what this closes the door on.Developer experience
_configure_logging()incontextual_orchestrator/__main__.py:--verbose/-von both the main CLI anddiscover-modelssubcommand, orCONTEXTUAL_ORCHESTRATOR_LOG_LEVEL=<LEVEL>; explicit flag wins if both are set; idempotent (no duplicate handlers across repeated calls).docs/architecture.mddocuments the event names to watch for and how to enable them.docs/library_research.mdrecords the Ponytail decision (stdliblogging, no new dependency; OTel/ADR 0122 stays scoped to per-request span correlation, not this pool-lifecycle evidence).CHANGELOG.d/verbose-debug-logging.mdfragment added per this repo's convention.User experience
No behavior change to routing, failover, or discovery outcomes — this is additive, opt-in diagnostic output only. Default (no flag, no env var) behavior is byte-for-byte unchanged.
Test plan
tests/test_verbose_debug_logging.py(new): audit-event→logging bridge (DEBUG marker, secret redaction viaredact_value, no-log-below-threshold),_failover_candidates' exclusion funnel (normal and circuit-open-fallback cases),candidate_pool_exhaustedon full failure, circuit-breaker open/close/auto-close log transitions, andmodel_discovery.py's skip/success/failure/retry/recovery logging.tests/test_verbose_logging_cli.py(new):_configure_logging's own contract (noop when unset, env sets level+handler, explicit wins over env, idempotent, invalid level raisesValueError) plus--verboseintegration on both CLI paths.tests/test_tool_execution_fallback.py: fixed a pre-existing exact-list assertion overlist_recent_audit_events()that broke oncefailover_candidates_resolved(noactionkey) also appears in the same audit stream — narrowed to filter byevent_typeinstead of loosening the new event.tests/test_model_discovery.py: updated one assertion (account=/model_count=→provider=/discovered_count=) to match the reconciled log format from merging with fix(discovery): keep credential catalogs independent #941.python -m pytest tests -q(2820+ tests; the only unrelated failure is a pre-existingModuleNotFoundError: No module named 'fast_mlsirm'intests/test_psychometric_routing.py, an optional native dependency not installed in this sandbox).interrogate(project-wide,fail-under = 80perpyproject.toml): 100% on the changed files.git diff --stat origin/main...HEADscoped to exactly the 10 intended files (branch was rebased from stale, already-squash-merged fix(routing): classify primary provider transport failures explicitly #922 history onto freshmain).Generated by Claude Code