[FEAT#144] LLM 비용/성능 기반 routing 정책 + 동적 chain 합성 - #167
Merged
Merged
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR implements dynamic LLM provider routing based on capabilities (cost, latency, context window), policies, and task hints. It introduces a policy layer that selects from multiple candidate providers per request, a metrics wrapper for tracking provider performance, capability metadata structures, and multiple routing strategies (cost-based, latency-based, hybrid scoring, fixed order). Changes
Sequence DiagramsequenceDiagram
participant Client
participant PolicyProvider
participant Policy
participant MeasuredProvider as Candidate<br/>(MeasuredProvider)
participant Chain as Internal<br/>Chain.Provider
Client->>PolicyProvider: Generate(ctx, req)
activate PolicyProvider
PolicyProvider->>PolicyProvider: Validate policy & candidates
PolicyProvider->>Policy: Select(ctx, req, candidates)
activate Policy
Policy->>MeasuredProvider: Get capabilities/stats<br/>for scoring
Note over Policy: Compute scores based on<br/>cost, latency, failure rate
Policy-->>PolicyProvider: Ordered candidates
deactivate Policy
PolicyProvider->>Chain: Generate with<br/>ordered handlers
activate Chain
Chain->>MeasuredProvider: Generate (1st candidate)
activate MeasuredProvider
MeasuredProvider->>MeasuredProvider: Measure latency<br/>Update stats
MeasuredProvider-->>Chain: Response | Error
deactivate MeasuredProvider
alt Success
Chain-->>PolicyProvider: Response
else Delegatable Error
Chain->>MeasuredProvider: Generate (2nd candidate)
MeasuredProvider-->>Chain: Response
Chain-->>PolicyProvider: Response
end
deactivate Chain
PolicyProvider-->>Client: Response | Error
deactivate PolicyProvider
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 0/1 reviews remaining, refill in 47 minutes and 14 seconds.Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces a dynamic LLM routing framework featuring performance monitoring, multiple selection policies (cost, latency, and hybrid), and a prompt management system. Feedback addresses a potential panic in Prometheus metric registration, ensures atomic consistency in provider statistics through improved synchronization, and suggests utilizing defined sentinel errors for better error handling.
…144 Phase 1.A/B) - Capabilities 구조체: CostInputPer1M / CostOutputPer1M / ContextWindow / AvgLatencyMs - CapabilitiesProvider 인터페이스 — Static (본 PR) + 향후 Refreshable (주기 동적 갱신) 가 같은 인터페이스 구현 - StaticCapabilitiesProvider: 2026-04 기준 hardcode pricing (openai / anthropic / gemini) - Request.TaskHint 필드 + 표준 hint 상수 (summary / reasoning / json / large_context) — routing policy hook
…#144 Phase 1.C) - llm.MeasuredProvider: 다른 Provider 를 wrap, 호출당 latency + 성공·실패 기록 - in-memory Stats — atomic counters + EMA latency (alpha 0.2) — routing policy 가 직접 read - Prometheus metric (registry 주입 시): - llm_provider_latency_seconds{provider,status} (histogram, 0.1s ~ 60s buckets) - llm_provider_call_total{provider,status} (counter) - registry nil 이면 in-memory Stats 만 동작 (Prometheus 등록 skip — backward compatible)
- pkg/llm/policy.Policy: Select(ctx, req, candidates) ([]Provider, error) - 정렬된 슬라이스 반환 — chain 합성 (Phase 3) 과 자연스럽게 어울림 - capabilityFor 헬퍼 — caps lookup 실패 시 zero Capabilities (graceful fallback) - CheapestFirst: (CostInputPer1M, CostOutputPer1M) asc 정렬 — 동률 시 출력 단가 보조 키
- MeasuredProvider 의 EMA latency 가 있으면 우선 사용, 없으면 Capabilities.AvgLatencyMs fallback - default 는 deterministic ascending 정렬 (lower latency 우선) - WithStochastic(true) 옵션: 1/(latency+ε) 가중치로 확률 무작위 선택 — 한 provider lock-in 회피 - WithRand 옵션: 결정적 테스트용 *rand.Rand 주입
- HybridWeights{Cost, Latency, FailureRate} — 운영자가 시그널 비중 조정
- DefaultHybridWeights: Cost=1.0, Latency=1.0, FailureRate=0.5 (안정성 보조)
- 각 시그널 후보 내 max 로 normalize 후 가중 합산 — 절대값 mismatch 회피
- 모든 가중치 0 이면 입력 순서 보존 (graceful no-op)
- score asc 정렬 — 낮을수록 (저렴 + 빠름 + 안정) 우선
…se 3) - chain.NewWithPolicy(policy, candidates, opts) — 매 호출마다 policy.Select 가 순서 결정 - 정렬된 ordered 슬라이스를 chain.Provider 의 handlers 로 위임 — 위임/cancel 정책 동일 (코드 중복 회피) - llm.Provider 인터페이스 컴파일 체크 (var _) - 호출자 코드는 chain.Provider 와 동일 — 투명한 wrapper
- prompt.Load(name): scripts/prompts/<name>.txt 또는 .md 자동 시도 - 환경변수 ISSUETRACKER_PROMPTS_DIR 로 디렉토리 override 가능 - path traversal 방지: name 에 path separator 포함 시 ErrNotFound - scripts/prompts/.gitkeep 으로 디렉토리 placeholder 등록 (실제 prompts 는 호출자 추가)
- test/pkg/llm/capabilities_test.go: StaticCapabilitiesProvider lookup + custom table - test/pkg/llm/measured_test.go: latency EMA / failure rate / Prometheus metric 등록 검증 - test/pkg/llm/policy/policy_test.go: CheapestFirst / LatencyWeighted (정적+EMA+stochastic) / Hybrid - test/pkg/llm/chain/policy_test.go: PolicyProvider 위임 / fallback / nil / canceled ctx - test/pkg/llm/prompt/prompt_test.go: txt/md 우선순위 / not found / path traversal 거부
- gemini #3165045920 (HIGH): NewMeasuredProvider 동일 registry+labelPrefix 다회 호출 시 collector 중복 등록으로 panic — 다중 LLM provider 환경에서 process crash 가능 - MeasuredFactory: registry 당 1회 collector 생성·등록, factory.Wrap(inner) 으로 여러 provider 공유 - collector 가 (provider, status) label 로 구분하므로 단일 인스턴스가 모든 wrapped provider metric 처리 - 테스트 호출 갱신 (NewMeasuredProvider → NewMeasuredFactory.Wrap)
- gemini #3165045925/#3165045932: atomic counter 두 값 (Calls/Failures) 독립 read 시 race 가능 — Failures > Calls 인 순간 FailureRate > 1.0 노출 - atomic.Uint64 제거, 모든 필드 단일 sync.RWMutex 보호 - Calls() / Failures() 메소드로 read API 통일 (이전 atomic 직접 노출에서 캡슐화) - record() 도 단일 lock 안에서 모든 필드 갱신 — partial state read 차단 - 테스트 갱신 (Calls.Load() → Calls() 호출)
- gemini #3165045945: sentinel 정의됐지만 wrap 에 미사용 — errors.Is 매칭 의도 불명확한 dead code - 본 PR scope 인 인프라 layer 에 호출처 0건이라 export 의도 없음 — 단순 제거가 깔끔 - 향후 wrap 의도가 생기면 그 PR 에서 sentinel 재도입 + wrap 동시 구현 (의도 명확)
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
test/pkg/llm/prompt/prompt_test.go (1)
14-63: ⚡ Quick winConvert these
Loadtests into a table-driven suite.Current cases are good, but setup/assertion duplication is high. A single table-driven test with subtests will
reduce repetition and make new scenarios cheaper to add.♻️ Refactor sketch
-func TestLoad_TxtFile(t *testing.T) { ... } -func TestLoad_MdFile(t *testing.T) { ... } -func TestLoad_PrefersTxtOverMd(t *testing.T) { ... } -func TestLoad_NotFound(t *testing.T) { ... } -func TestLoad_RejectsPathSeparator(t *testing.T) { ... } -func TestLoad_EmptyName(t *testing.T) { ... } +func TestLoad(t *testing.T) { + testCases := []struct { + name string + prompt string + prepare func(t *testing.T, dir string) + wantBody string + wantErr error + checkErr func(t *testing.T, err error) + }{ + // txt, md, prefer txt, not found, path separator, empty name ... + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + t.Setenv(prompt.EnvPromptsDir, dir) + if tc.prepare != nil { + tc.prepare(t, dir) + } + + got, err := prompt.Load(tc.prompt) + if tc.checkErr != nil { + tc.checkErr(t, err) + } else { + assert.ErrorIs(t, err, tc.wantErr) + } + assert.Equal(t, tc.wantBody, got) + }) + } +}As per coding guidelines: "
**/*_test.go: Write table-driven tests for unit tests" and
"**/*_test.go: Use table-driven tests in Go for testing multiple scenarios."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/pkg/llm/prompt/prompt_test.go` around lines 14 - 63, Replace the six separate tests (TestLoad_TxtFile, TestLoad_MdFile, TestLoad_PrefersTxtOverMd, TestLoad_NotFound, TestLoad_RejectsPathSeparator, TestLoad_EmptyName) with a single table-driven test that iterates over cases describing name, files to create (filename->content), expected body or expected error, and any env setup; for each case run t.Run(subname, func(t *testing.T){ ... }) which sets prompt.EnvPromptsDir to t.TempDir(), writes the specified files, calls prompt.Load(name), and asserts either the returned body equals the expected string or the error matches expected (using assert.ErrorIs for prompt.ErrNotFound where appropriate and errors.Is to distinguish traversal vs not-found); keep the special expectations for preferring .txt over .md and for rejecting path separators as separate table entries so all scenarios reuse the same setup/assertion logic around prompt.Load and prompt.EnvPromptsDir.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/llm/capabilities.go`:
- Around line 71-73: The Get method on StaticCapabilitiesProvider can panic when
called on a nil receiver; update StaticCapabilitiesProvider.Get to check if s ==
nil and immediately return (Capabilities{}, false) before accessing s.table,
keeping the rest of the lookup logic (using capKey{Provider: provider, Model:
model}) unchanged so callers receive a safe false result for nil providers.
In `@pkg/llm/chain/policy.go`:
- Around line 36-41: NewWithPolicy currently assigns the caller's candidates
slice header to PolicyProvider (pp := &PolicyProvider{policy: p, candidates:
candidates}), which can lead to external mutation/races; change NewWithPolicy to
make a defensive copy of the candidates slice (allocate a new slice of
appropriate length/capacity and copy the elements) and assign that copy to
pp.candidates before applying opts so the PolicyProvider holds its own
goroutine-safe slice independent of caller mutations.
- Around line 55-56: The current nil check on the Policy interface (p.policy)
only detects a nil interface, not a typed nil underlying pointer; update the
guard used before calling Select to detect typed nils as well: either change the
field to a concrete implementation type if a specific implementation is
required, or add a robust runtime check before use (e.g., test if p.policy ==
nil || (reflect.ValueOf(p.policy).Kind() == reflect.Ptr &&
reflect.ValueOf(p.policy).IsNil()) or more generally use
reflect.ValueOf(p.policy).IsNil() for nil-able kinds) and return the
ErrCodeBadRequest llm.Error when the check indicates a nil underlying value;
apply the same check at both the p.policy nil-check site and at the Select
invocation site to avoid calling Select on a typed-nil receiver.
In `@pkg/llm/measured.go`:
- Around line 112-127: The code currently calls registry.MustRegister(...) in
NewMeasuredFactory which will panic on duplicate registration; change to use
registry.Register for both f.latencyHist and f.callCounter, check the returned
error, and if it's a prometheus.AlreadyRegisteredError extract the
ExistingCollector and assign it back to f.latencyHist/f.callCounter (type-assert
to *prometheus.HistogramVec / *prometheus.CounterVec respectively); for any
other error return or propagate it instead of panicking so the factory becomes
idempotent and avoids production panics.
In `@pkg/llm/policy/fixed.go`:
- Around line 35-36: NewFixedOrder currently stores the caller-owned slice
directly, making FixedOrder.mutable and unsafe; change NewFixedOrder to
defensively copy the names slice (e.g., create a new []string with length
len(names) and copy or use append([]string(nil), names...)) and store that copy
in the FixedOrder struct so the policy remains immutable and goroutine-safe;
update construction in NewFixedOrder and ensure FixedOrder.names is treated as
read-only thereafter.
In `@pkg/llm/policy/hybrid.go`:
- Around line 62-67: Replace the current check that treats LatencyMs()==0 as
unmeasured with a call-count check: use mp.Stats().Calls() > 0 to decide if
measured latency is available and, when true, set latencies[i] =
mp.Stats().LatencyMs(), otherwise fall back to caps.AvgLatencyMs; keep
failures[i] = mp.Stats().FailureRate() as-is. This change should be applied
where latencies, failures, mp, caps and i are used (the block that currently
checks LatencyMs()).
In `@pkg/llm/policy/latency.go`:
- Around line 79-85: The code currently treats observed latency 0ms as “no data”
by checking observed := mp.Stats().LatencyMs(); observed > 0; change the logic
in the measured-provider branch to check call count instead: use
mp.Stats().Calls() > 0 to decide if the measured value should be used, and still
return the measured latency value (observed) even when it equals 0; keep the
fallback to capabilityFor(p.caps, provider, req) and float64(caps.AvgLatencyMs)
unchanged.
- Around line 23-27: The injected *rand.Rand is accessed concurrently in
LatencyWeighted.Select (and in shuffleByInverseWeight) when stochastic is true,
causing a data race; add a sync.RWMutex (or sync.Mutex) field to the
LatencyWeighted struct, update WithRand to set the rng as now-protected, and
wrap all usages of p.rng.Float64()/p.rng.* with the mutex (RLock/RUnlock for
reads, Lock/Unlock if any mutation) so the custom RNG is safe for concurrent
Select calls; alternatively, if p.rng is nil, use the global thread-safe rand
package functions to avoid locking.
In `@pkg/llm/policy/policy.go`:
- Around line 46-54: The helper capabilityFor currently hides lookup misses by
returning an empty llm.Capabilities for both "not found" and actual empty
capabilities; change capabilityFor(caps llm.CapabilitiesProvider, p
llm.Provider, req llm.Request) to return (llm.Capabilities, bool) so the bool
indicates whether Get returned ok, and update all callers (policy scoring code /
functions that call capabilityFor in this package) to explicitly handle the
false case (either by resolving the provider's configured default model before
scoring or skipping/penalizing unknown providers) instead of treating an unknown
as a zero-cost capability.
---
Nitpick comments:
In `@test/pkg/llm/prompt/prompt_test.go`:
- Around line 14-63: Replace the six separate tests (TestLoad_TxtFile,
TestLoad_MdFile, TestLoad_PrefersTxtOverMd, TestLoad_NotFound,
TestLoad_RejectsPathSeparator, TestLoad_EmptyName) with a single table-driven
test that iterates over cases describing name, files to create
(filename->content), expected body or expected error, and any env setup; for
each case run t.Run(subname, func(t *testing.T){ ... }) which sets
prompt.EnvPromptsDir to t.TempDir(), writes the specified files, calls
prompt.Load(name), and asserts either the returned body equals the expected
string or the error matches expected (using assert.ErrorIs for
prompt.ErrNotFound where appropriate and errors.Is to distinguish traversal vs
not-found); keep the special expectations for preferring .txt over .md and for
rejecting path separators as separate table entries so all scenarios reuse the
same setup/assertion logic around prompt.Load and prompt.EnvPromptsDir.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 295d592f-48e3-431d-ac79-65f7101a602c
📒 Files selected for processing (16)
pkg/llm/capabilities.gopkg/llm/chain/policy.gopkg/llm/llm.gopkg/llm/measured.gopkg/llm/policy/cheapest.gopkg/llm/policy/fixed.gopkg/llm/policy/hybrid.gopkg/llm/policy/latency.gopkg/llm/policy/policy.gopkg/llm/prompt/prompt.goscripts/prompts/.gitkeeptest/pkg/llm/capabilities_test.gotest/pkg/llm/chain/policy_test.gotest/pkg/llm/measured_test.gotest/pkg/llm/policy/policy_test.gotest/pkg/llm/prompt/prompt_test.go
- pkg/llm/policy/fixed.go: NewFixedOrder(names...) — 지정 순서로 매칭, 미매칭 candidate 는 필터링 - 단일 provider 운영 / 무료 한도 내 제한 / A/B 강제 핀 등에 사용 - 기존 정책 (CheapestFirst / LatencyWeighted / Hybrid) 과 동일 Policy 인터페이스 구현 - 4 케이스 단위 테스트: 단일 핀 / 명시 순서 / 미매칭 필터 / 빈 names no-op
Closed
5 tasks
- capabilities.go: StaticCapabilitiesProvider.Get nil receiver guard 추가 - chain/policy.go: NewWithPolicy candidates defensive copy - measured.go: MustRegister → Register + AlreadyRegisteredError 처리로 idempotent factory (panic 회피) - policy/fixed.go: NewFixedOrder names defensive copy - policy/hybrid.go, policy/latency.go: 측정 여부 판정을 LatencyMs()>0 → Calls()>0 으로 변경 (sub-ms valid 측정값 보존) - policy/latency.go: 주입 *rand.Rand 를 sync.Mutex 로 보호 (concurrent-unsafe 해소) - 테스트: MeasuredFactory idempotent + LatencyWeighted stochastic concurrent race 검증
This was referenced Apr 30, 2026
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
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.
연관 이슈
Closes #144 (선행: #165 Prometheus client 도입 — 머지 완료)
배경
기존 `pkg/llm/chain` 의 정적 순차 fallback 을 넘어, 비용 / 성능 / 작업 특성 / 동적 metric 을 입력으로 매 호출마다 적합한 provider 를 선택하는 routing 정책 layer 를 도입합니다.
호출자 코드는 변화 없음 — `chain.NewWithPolicy(policy, candidates)` 가 `llm.Provider` 인터페이스를 그대로 노출.
구현 내용 (8 commits)
Phase 1 — 인프라
Capabilities + CapabilitiesProvider + Request.TaskHint (`e4490d8`)
MeasuredProvider + Prometheus metric (`820e600`)
Phase 2 — 정책
Policy interface + CheapestFirst (`7afaedc`)
LatencyWeighted (`65a2457`)
Hybrid (`fa8c1ff`)
Phase 3 — chain 합성
Phase 4 — 프롬프트 + 테스트
pkg/llm/prompt (`0dba1b9`)
단위 테스트 (`ed7e390`)
CI / 머지 게이트 점검
변경 영향 범위 + 위험도
롤백 계획
후속 이슈 후보
Summary by CodeRabbit