fix(saturation): log actual analyzer mode instead of hardcoded value#1334
fix(saturation): log actual analyzer mode instead of hardcoded value#1334Goutham-Annem wants to merge 2 commits into
Conversation
The 'Optimization completed successfully' log entry always reported mode=saturation-only, even when the queueing-model or V2 saturation analyzer path was actually selected. Extract a small helper that maps the resolved analyzerName to its log label, and use it for the completion log instead of the hardcoded string. Fixes llm-d#962 Signed-off-by: Goutham Annem <Goutham-Annem@users.noreply.github.com>
ev-shindin
left a comment
ev-shindin
left a comment
There was a problem hiding this comment.
LGTM, there are sime comments to address.
Address ev-shindin review suggestions on PR llm-d#1334: - Fold the modeLabelForAnalyzer helper into optimize's existing switch so the mode label is set in the same case that selects the optimize path, eliminating a second switch that had to stay in lockstep. - Rename log labels: "saturation" → "saturation-v2" and "saturation-only" → "saturation-v1" for unambiguous mode identification. - Update mode_label_test.go to mirror the new inline switch logic and expected label values. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| t.Run(tt.name, func(t *testing.T) { | ||
| // Reproduce the same switch logic as optimize() so the expected labels | ||
| // are verified without needing a full engine instance. | ||
| var mode string |
There was a problem hiding this comment.
Non-vlocking: The test body re-implements the optimize() switch rather than invoking it, so it can't catch the regression its doc comment promises. Rename "saturation-v2" → "saturation-v3" in engine.go and forget the test, and it still passes (its private copy and wantMode both still say "saturation-v2"). Extract a helper in engine.go and call it from both sides — this also removes the duplicated "saturation-vN" literals and the style asymmetry (queueing uses a constant, V1/V2 use literals):
func analyzerModeLabel(analyzerName string) string {
switch analyzerName {
case interfaces.QueueingModelAnalyzerName: return interfaces.QueueingModelAnalyzerName
case interfaces.SaturationAnalyzerName: return "saturation-v2"
default: return "saturation-v1"
}
}| // - Queueing model: QueueingModelAnalyzer → AnalyzerResult → Optimizer.Optimize → Enforcer bridge | ||
| // V1 will be deprecated once V2 is fully validated. | ||
| // Queueing model is activated by presence of wva-queueing-model-config ConfigMap. | ||
| var mode string |
There was a problem hiding this comment.
Nit: The mode label is assigned inline across the three switch arms, with one arm using the interfaces.QueueingModelAnalyzerName constant and the other two using raw "saturation-vN" literals. Replacing the inline assignments with mode := analyzerModeLabel(analyzerName) (above) keeps the path-selection switch and puts all three labels in one place, making the asymmetry intentional and the test meaningful in one move.
2 participants
Fixes #962
Proposed Changes
🐛 Fix bug
The "Optimization completed successfully" log entry in
internal/engines/saturation/engine.goalways hardcodedmode=saturation-only, regardless of whether the queueing-model, V2 saturation, or V1 (legacy saturation-only) analyzer path actually ran.Extracted
modeLabelForAnalyzer(analyzerName string) string, which maps the resolvedanalyzerNameto the same label used by theswitchthat selects the optimize path, and used it for the completion log instead of the literal string.Pre-review Checklist
Release Note
Docs
N/A — internal log field, no user-facing documentation impact.
How tested
TestModeLabelForAnalyzer(table-driven,internal/engines/saturation/mode_label_test.go) covering all three modes plus the empty/unset and unrecognized fallback cases.go test ./internal/engines/saturation/...passes (61.4% coverage maintained).make testrun locally: only pre-existing, unrelated failure ininternal/engines/analyzers/throughput(a flaky degenerate-input case initl_model_test.go), confirmed present onmainwithout this change too.gofmt/go vetclean.