Add optimization loop performance metrics#981
Open
jia-gao wants to merge 4 commits intollm-d:mainfrom
Open
Conversation
Add two Prometheus metrics to track optimization loop performance: - wva_optimization_duration_seconds: histogram tracking duration of each optimization cycle, labeled by status (success/error) - wva_models_processed_total: counter tracking total models processed Implementation: - internal/constants/metrics.go: add metric name and label constants - internal/metrics/metrics.go: register histogram and counter, add ObserveOptimizationDuration and IncrModelsProcessed helper methods - internal/engines/saturation/engine.go: instrument optimize() with deferred timing observation and model count tracking - internal/metrics/optimization_metrics_test.go: unit tests verifying histogram observation, counter increment, and nil safety Closes llm-d#914
Contributor
Author
|
Note on the If a |
Collaborator
|
/ok-to-test |
Contributor
|
🚀 Kind E2E (full) triggered by |
Contributor
|
🚀 OpenShift E2E — approve and run ( |
ev-shindin
requested changes
Apr 5, 2026
Collaborator
ev-shindin
left a comment
ev-shindin
left a comment
There was a problem hiding this comment.
Please address review comments
Changes based on review by @ev-shindin: 1. Use named return value (retErr) instead of manual optimizeErr assignments — idiomatic Go, captures all return paths automatically 2. Add controller_instance label to optimization duration histogram for HA deployment compatibility 3. Replace MetricsEmitter methods with package-level functions (ObserveOptimizationDuration, SetModelsProcessed) — avoids adding a second emitter instance on the Engine struct 4. Change models-processed from counter to gauge — directly shows models in last cycle, more useful for dashboards and alerting 5. Move modelsProcessed assignment to right after modelGroups is computed — accurately reflects models entering the pipeline regardless of apply outcome 6. Remove "partial" from status label documentation — not currently emitted, can be added when a partial completion path exists
Contributor
Author
|
/ok-to-test |
ev-shindin
approved these changes
Apr 13, 2026
Collaborator
ev-shindin
left a comment
ev-shindin
left a comment
There was a problem hiding this comment.
Good progress. Please address review comments.
ev-shindin
requested changes
Apr 13, 2026
Collaborator
ev-shindin
left a comment
ev-shindin
left a comment
There was a problem hiding this comment.
Please address review comments
- Rename wva_models_processed_total to wva_models_processed (gauge, not counter — _total suffix violates Prometheus convention). - Convert modelsProcessedGauge to GaugeVec with conditional controller_instance label so HA deployments don't collide. - Always set modelsProcessedGauge in defer so the value reflects the current cycle even when optimize() returns early with no active VAs. - Bump test file copyright to 2026.
Contributor
Author
thanks for the comments! fixed them |
Collaborator
|
/lgtm please fix linter issues |
github-actions
bot
added
the
lgtm
applySaturationDecisions logs and handles all errors inline, never returning a non-nil error. Remove the error return and simplify the caller in optimize(). Fixes the unparam lint failure. Signed-off-by: Jiazhou Gao <gjz140103@gmail.com>
jia-gao
force-pushed
the
feat/optimization-loop-metrics-914
branch
from
bbdb5f9 to
7529833
Compare
Contributor
Author
|
/ok-to-test |
1 similar comment
Collaborator
|
/ok-to-test |
Contributor
|
🚀 Kind E2E (full) triggered by |
Contributor
|
🚀 OpenShift E2E — approve and run ( |
Contributor
GPU Pre-flight Check ✅GPUs are available for e2e-openshift tests. Proceeding with deployment.
|
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
2 participants
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.
Summary
Add timing and throughput metrics for the optimization loop. Closes #914.
New Metrics
wva_optimization_duration_secondsstatuswva_models_processed_totalHistogram buckets:
{0.01, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10}secondsStatus labels:
success,errorImplementation
internal/constants/metrics.go— New metric name and label constantsinternal/metrics/metrics.go— Register histogram + counter inInitMetrics(), addObserveOptimizationDuration()andIncrModelsProcessed()methods onMetricsEmitterinternal/engines/saturation/engine.go— Instrumentoptimize()with a deferred timing observation that captures duration and status (success/error), and tracks models processed per cycleinternal/metrics/optimization_metrics_test.go— Unit testsTests
Acceptance Criteria (from issue)