[FEAT#170] LLM wiring — MeasuredProvider 통합 + LLM_POLICY (chain/cheapest/latency/hybrid) - #342
Merged
Merged
Conversation
BuildProvider 를 backward-compat wrapper 로 유지하고 새 BuildProviderWithOptions 추가. provider 후보는 inner→outer 순으로 다음 decorator 로 wrap: 1. RealProvider (gemini / openai / anthropic) 2. RetryProvider — rate_limit/network 백오프 재시도 (#215) 3. MeasuredProvider — per-call latency EMA + Prometheus 메트릭 + Stats (#170) candidates 가 *MeasuredProvider 이므로 정책 (LatencyWeighted / Hybrid) 의 type assertion 이 hit — 동적 metric 기반 정렬 가능. registry 가 nil 이면 collector 등록 skip, in-memory EMA/Stats 만 유지 (테스트 / metrics 비활성 환경). 정책 선택 (LLM_POLICY, default chain): - chain (또는 fixed): FixedOrder(fallbackOrder...) — gemini → openai → anthropic 정적 순서 - cheapest : CheapestFirst(caps) — CostInputPer1M 오름차순 - latency : LatencyWeighted(caps) — EMA latency 오름차순 (이력 없으면 baseline) - hybrid : Hybrid(caps, weights) — cost + latency + failure_rate 가중 합산 · LLM_HYBRID_WEIGHTS="cost,latency,failure_rate" 환경변수로 override (default 1.0,1.0,0.5) · 음수 / 형식 오류 시 default 로 fallback + warn 로그 - unknown : warn + FixedOrder 로 fallback (운영 안전망) 테스트 11건 추가 (test/pkg/llm/wiring/policy_test.go): - 정책 default / chain alias / cheapest / latency / hybrid 각 선택 검증 - hybrid valid 가중치 통과 - hybrid invalid (non-float / 개수 오류 / 음수) → default fallback + warn - unknown LLM_POLICY → FixedOrder fallback + warn - PrometheusRegistry 전달 시 idempotent re-wiring 검증 (collector 중복 등록 보호) - nil registry 시 metrics_registered=false 로 로그
…ICY 문서화 (이슈 #170) cmd/issuetracker/main.go: - 기존 BuildProvider(log) → BuildProviderWithOptions(log, Options{PrometheusRegistry}) - 동일 metrics.NewRegistry() 인스턴스 공유 — /metrics endpoint 로 issuetracker_llm_* 메트릭 노출 - llmgen / refiner 가 공유하는 동일 provider 가 측정됨 .env.example: - LLM_POLICY (default chain) — chain / cheapest / latency / hybrid 4가지 옵션 설명 - LLM_HYBRID_WEIGHTS (default 1.0,1.0,0.5) — hybrid 정책 가중치
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (4)
📝 WalkthroughWalkthroughThis PR extends LLM provider wiring to support environment-driven policy selection (chain/cheapest/latency/hybrid) with optional Prometheus metrics integration. It adds configuration options, refactors the builder to support dynamic policies, and integrates measured providers for metrics-based routing decisions. ChangesPolicy-Driven LLM Routing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/llm/wiring/wiring.go (1)
103-174: ⚡ Quick winConsider extracting candidate-building logic to improve function length.
BuildProviderWithOptionsis 71 lines, exceeding the 50-line guideline. The candidate-building loop (lines 120-154) could be extracted to a helper function likebuildCandidateProviders(...)to improve modularity and readability.♻️ Suggested refactor
Extract the candidate-building loop to a separate function:
// buildCandidateProviders constructs provider candidates from fallbackOrder with MeasuredProvider wrapping. func buildCandidateProviders( log *logger.Logger, cfg *config.LLMConfig, primaryName string, measuredFactory *llm.MeasuredFactory, ) ([]llm.Provider, []string) { candidates := make([]llm.Provider, 0, len(fallbackOrder)) activeNames := make([]string, 0, len(fallbackOrder)) for _, name := range fallbackOrder { apiKey := lookupProviderAPIKey(name) if apiKey == "" && name == primaryName { apiKey = cfg.APIKey } if apiKey == "" { log.WithField("provider", name).Debug("skipping provider — no API key configured") continue } model := "" if name == primaryName { model = cfg.Model } p, perr := llm.New(llm.Config{ Provider: name, APIKey: apiKey, Model: model, Timeout: cfg.Timeout, }) if perr != nil { log.WithError(perr).WithField("provider", name).Warn("failed to construct LLM provider, skipping") continue } retryWrapped := llm.NewRetryProvider(p, llm.RetryProviderOptions{}) measured := measuredFactory.Wrap(retryWrapped) candidates = append(candidates, measured) activeNames = append(activeNames, name) } return candidates, activeNames }Then simplify
BuildProviderWithOptions:func BuildProviderWithOptions(log *logger.Logger, opts Options) llm.Provider { cfg, err := config.LoadLLM() if err != nil { log.WithError(err).Warn("failed to load LLM config, llm provider disabled") return nil } if !cfg.Enabled { log.Info("LLM provider disabled (LLM_ENABLED=false)") return nil } primaryName := normalizePrimary(cfg.Provider) measuredFactory := llm.NewMeasuredFactory(opts.PrometheusRegistry, metricsLabelPrefix) - candidates := make([]llm.Provider, 0, len(fallbackOrder)) - activeNames := make([]string, 0, len(fallbackOrder)) - for _, name := range fallbackOrder { - apiKey := lookupProviderAPIKey(name) - // LLM_API_KEY fallback 은 primary (LLM_PROVIDER) 에만 적용 — backward compat. - if apiKey == "" && name == primaryName { - apiKey = cfg.APIKey - } - if apiKey == "" { - log.WithField("provider", name).Debug("skipping provider — no API key configured") - continue - } - model := "" - if name == primaryName { - model = cfg.Model - } - p, perr := llm.New(llm.Config{ - Provider: name, - APIKey: apiKey, - Model: model, - Timeout: cfg.Timeout, - }) - if perr != nil { - log.WithError(perr).WithField("provider", name).Warn("failed to construct LLM provider, skipping") - continue - } - // RetryProvider — rate_limit / network 발생 시 chain fallback 전에 같은 provider 에서 - // backoff 재시도 (이슈 `#215`). default 정책 (RateLimit 5회/10s + Network 3회/1s). - retryWrapped := llm.NewRetryProvider(p, llm.RetryProviderOptions{}) - // MeasuredProvider — per-call latency EMA + Prometheus 메트릭 (이슈 `#170`). - // outer wrap 으로 retries 포함한 end-to-end 시간을 기록 — 정책이 보는 latency 는 실 사용자 경험. - measured := measuredFactory.Wrap(retryWrapped) - candidates = append(candidates, measured) - activeNames = append(activeNames, name) - } + candidates, activeNames := buildCandidateProviders(log, cfg, primaryName, measuredFactory) if len(candidates) == 0 { log.Warn("LLM provider disabled — no API keys configured (set GEMINI_API_KEY / OPENAI_API_KEY / ANTHROPIC_API_KEY)") return nil } pol, polName := selectPolicy(log) composed := chain.NewWithPolicy(pol, candidates, chain.WithPolicyLogger(log)) log.WithFields(map[string]interface{}{ "chain": activeNames, "first_in_chain": activeNames[0], "configured_primary": primaryName, "policy": polName, "timeout": cfg.Timeout.String(), "metrics_registered": opts.PrometheusRegistry != nil, }).Info("LLM provider chain enabled") return composed }As per coding guidelines: "Go 함수는 최대 50줄"
🤖 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 `@pkg/llm/wiring/wiring.go` around lines 103 - 174, BuildProviderWithOptions is over the 50-line guideline; extract the candidate-building loop into a helper function (e.g., buildCandidateProviders) that takes (log *logger.Logger, cfg *config.LLMConfig, primaryName string, measuredFactory *llm.MeasuredFactory) and returns ([]llm.Provider, []string). Move the logic that iterates fallbackOrder, calls lookupProviderAPIKey (with the primary-key fallback), constructs providers via llm.New, wraps with llm.NewRetryProvider and measuredFactory.Wrap, and collects candidates and activeNames into that helper; then call it from BuildProviderWithOptions and use its returned candidates/activeNames in the rest of the function (preserving the same logging and error handling).
🤖 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 `@pkg/llm/wiring/wiring.go`:
- Around line 103-174: BuildProviderWithOptions is over the 50-line guideline;
extract the candidate-building loop into a helper function (e.g.,
buildCandidateProviders) that takes (log *logger.Logger, cfg *config.LLMConfig,
primaryName string, measuredFactory *llm.MeasuredFactory) and returns
([]llm.Provider, []string). Move the logic that iterates fallbackOrder, calls
lookupProviderAPIKey (with the primary-key fallback), constructs providers via
llm.New, wraps with llm.NewRetryProvider and measuredFactory.Wrap, and collects
candidates and activeNames into that helper; then call it from
BuildProviderWithOptions and use its returned candidates/activeNames in the rest
of the function (preserving the same logging and error handling).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb1cec96-3a73-4f48-bd21-490353decc16
📒 Files selected for processing (4)
.env.examplecmd/issuetracker/main.gopkg/llm/wiring/wiring.gotest/pkg/llm/wiring/policy_test.go
There was a problem hiding this comment.
Pull request overview
이 PR은 LLM provider 체인 구성(wiring)에서 MeasuredProvider를 실제로 적용하고, LLM_POLICY/LLM_HYBRID_WEIGHTS 환경변수로 라우팅 정책을 선택할 수 있도록 연결(wiring)하는 변경입니다. 또한 cmd/issuetracker에서 Prometheus registry를 LLM wiring에 전달해 /metrics로 LLM 메트릭을 노출할 수 있게 합니다.
Changes:
BuildProviderWithOptions도입 및 후보 provider를RetryProvider→MeasuredProvider로 wrap하도록 wiring 변경LLM_POLICY(chain/cheapest/latency/hybrid) +LLM_HYBRID_WEIGHTS파싱 로직 추가- main에서 metrics registry를 wiring에 전달,
.env.example및 정책 선택 테스트 추가
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
pkg/llm/wiring/wiring.go |
옵션 기반 provider 빌드 + MeasuredProvider wrapping + 정책/가중치 env 선택 로직 추가 |
cmd/issuetracker/main.go |
LLM wiring에 Prometheus registry 전달하도록 변경 |
test/pkg/llm/wiring/policy_test.go |
LLM_POLICY/LLM_HYBRID_WEIGHTS에 따른 정책 선택 및 registry wiring 테스트 추가 |
.env.example |
LLM_POLICY 및 LLM_HYBRID_WEIGHTS 사용 예시/설명 추가 |
There was a problem hiding this comment.
Code Review
This pull request introduces dynamic LLM routing policies, including 'cheapest', 'latency', and 'hybrid' options, alongside the default static chain. It also integrates Prometheus metrics to track provider latency and failure rates, which are used for dynamic routing decisions. The provider wiring was refactored to support these new capabilities while maintaining backward compatibility. Feedback was provided regarding the need to validate that hybrid policy weights are finite numbers to avoid calculation errors.
… test / 주석 갱신 Copilot 피드백 4건 반영: 1. wiring.go: first_in_chain → first_in_candidates (Copilot #1) - 동적 정책 (cheapest/latency/hybrid) 은 매 호출마다 policy.Select 가 정렬을 변경하므로 "정책 적용 전 후보 목록의 첫 항목" 의미로 명명 정합화 — 운영 디버깅 시 오해 방지 - 실제 첫 시도 provider 는 chain Generate 시점 로그에서 확인 2. wiring.go: loadHybridWeights 에러 메시지 형식 통일 (Copilot #2) - 기존: "LLM_HYBRID_WEIGHTS expects ..." (대문자 시작) - 변경: "invalid LLM_HYBRID_WEIGHTS: ..." (소문자 + invalid 접두) - 다른 env 파싱 에러 패턴 (선례 PR #340) 과 일관 — 로그 필터링 / 테스트 안정성 ↑ 3. wiring_test.go: clearLLMEnv 에 LLM_POLICY / LLM_HYBRID_WEIGHTS 추가 (Copilot #3) - 로컬 운영자 .env 가 set 된 상태에서 hermetic 보장 - 기존 test 의 first_in_chain → first_in_candidates 리네임 동기화 4. cmd/issuetracker/main.go: stale 주석 갱신 (Copilot #4) - 기존: "FixedOrder('gemini') 정책으로 Gemini 단일 provider" - 변경: 4가지 정책 (chain/cheapest/latency/hybrid) 선택 가능 + LLM_POLICY 명시
gemini Medium 반영: - math.IsNaN(f) || math.IsInf(f, 0) 검사 추가 - Hybrid 정책의 normalize 계산이 NaN 전파로 깨지는 것 방지 - 음수 거부보다 먼저 검사 — -Inf 도 IsInf(_, 0) 에 포함 테스트 추가: NaN / Inf / -Inf 3 케이스 모두 invalid 로 거부됨 검증.
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 #170
배경
사전 검증 결과 — 재료는 다 있고 조립만 남은 상태:
구현 완료 (이전 PR):
pkg/llm/measured.go—MeasuredProvider(per-call latency EMA + Prometheus 메트릭 + Stats)pkg/llm/policy/hybrid.go— Hybrid (cost + latency + failure_rate 가중 합산, MeasuredProvider type assertion)pkg/llm/policy/latency.go— LatencyWeighted (EMA 기반 동적 정렬)pkg/llm/policy/cheapest.go— CheapestFirst (Capabilities cost 기반)pkg/llm/capabilities.go— 정적 baseline (cost / latency)누락 (본 PR scope):
pkg/llm/wiring/wiring.go가 candidates 를MeasuredProvider로 wrap 하지 않음 → policy 의 type assertion 실패 → metric 기반 정렬 불가cmd/issuetracker/main.go가 Prometheus registry 를 전달하지 않음구현 내용
pkg/llm/wiring/wiring.goBuildProvider(log)→ backward-compat wrapper 로 유지 (기존 test 변경 없음)BuildProviderWithOptions(log, Options)신설 —Options.PrometheusRegistry수용*MeasuredProvider이므로 policy 의c.(*llm.MeasuredProvider)가 hitMeasuredFactory단일 인스턴스 공유 — 모든 provider 가 동일 collector set 사용 (중복 등록 panic 회피)정책 선택 (
LLM_POLICY환경변수)chain(default) /fixedcheapestlatencyhybridLLM_HYBRID_WEIGHTS(hybrid 정책 전용)cost,latency,failure_rate(예:1.0,1.0,0.5)DefaultHybridWeights()(1.0, 1.0, 0.5)cmd/issuetracker/main.goBuildProvider(log)→BuildProviderWithOptions(log, Options{PrometheusRegistry: metricsRegistry})metrics.NewRegistry()인스턴스 공유 — /metrics endpoint 로issuetracker_llm_*메트릭 노출.env.exampleLLM_POLICY(default chain) — 4가지 옵션 설명LLM_HYBRID_WEIGHTS(default 1.0,1.0,0.5)테스트 (test/pkg/llm/wiring/policy_test.go) — 11건
CI / 머지 게이트 점검
go build ./...통과go test -race -count=1 ./test/...전부 okgo vet ./...깨끗변경 영향 범위 + 위험도
MeasuredFactory의 idempotent registration 으로 collector 중복 panic 방지롤백 계획
LLM_POLICY=chain(또는 unset) — 정책 변경만 되돌리기git revert— wiring 단일 패키지 변경🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
LLM_POLICY) and hybrid weight configuration (LLM_HYBRID_WEIGHTS).Tests