[Bug] Fix external metric fetching for GPU optimizer autoscaling and replace dead scale-down annotation in samples

[yaojiejia]

Pull Request Description

This PR fixes two bugs that make samples/autoscaling/optimizer-kpa.yaml (and the heterogeneous GPU samples that copy it) not work. If you apply these samples today, the PodAutoscaler starts up but fails to read its metric on every cycle, so it never scales anything.

Bug 1: the annotation does nothing. The samples set kpa.autoscaling.aibrix.ai/scale-down-delay, but the controller no longer reads keys with the kpa. prefix. It only reads autoscaling.aibrix.ai/scale-down-cooldown-window. The old key is silently ignored, so scale-down always waits the default 5 minutes. This PR renames the key to the correct one in all 14 files that had it, keeping each file's original value.

Bug 2: the metric can never be fetched. These samples read vllm:deployment_replicas from the GPU optimizer. This worked when the samples were written, but the metrics fetcher refactor (#1487) changed how external metrics are fetched, and two things broke:

• The fetcher now only accepts metrics listed in a central registry, and this metric is not in it, so every fetch fails right away.
• The fetcher ignores the path field and always calls /metrics, but the GPU optimizer only serves /metrics/{namespace}/{deployment}.

The fix:

• Add a new method, FetchRawMetric, that fetches a metric straight from the given URL without checking the registry. External metrics like the optimizer's output are not engine metrics, so the registry should not apply to them.
• Make the optimizer fetch build its URL from the protocolType, endpoint, and path fields in the spec, so path is actually used.
• Update the six sample files to use metricSourceType: external instead of the deprecated domain.

Testing: a new unit test covers the new method (fetching an unregistered metric from an optimizer-style path, a metric missing from the response, and a wrong path). All existing unit tests for pkg/metrics and pkg/controller/podautoscaler pass, along with go build, go vet, and gofmt.

Related Issues

Resolves: #2615

Important: Before submitting, please complete the description above and review the checklist below.

---

Contribution Guidelines (Expand for Details)

We appreciate your contribution to aibrix! To ensure a smooth review process and maintain high code quality, please adhere to the following guidelines:

Pull Request Title Format

Your PR title should start with one of these prefixes to indicate the nature of the change:

• [Bug]: Corrections to existing functionality
• [CI]: Changes to build process or CI pipeline
• [Docs]: Updates or additions to documentation
• [API]: Modifications to aibrix's API or interface
• [CLI]: Changes or additions to the Command Line Interface
• [Misc]: For changes not covered above (use sparingly)

Note: For changes spanning multiple categories, use multiple prefixes in order of importance.

Submission Checklist

• PR title includes appropriate prefix(es)
• Changes are clearly explained in the PR description
• New and existing tests pass successfully
• Code adheres to project style and best practices
• Documentation updated to reflect changes (if applicable)
• Thorough testing completed, no regressions introduced

By submitting this PR, you confirm that you've read these guidelines and your changes align with the project's contribution standards.

[gemini-code-assist]

Code Review

This pull request renames the scale-down delay annotation to autoscaling.aibrix.ai/scale-down-cooldown-window and updates the metric source type from domain to external across various YAML configurations. Additionally, it introduces a FetchRawMetric method in the metrics engine fetcher to retrieve raw Prometheus metrics directly from configured endpoints, bypassing the central registry. Feedback on these changes highlights two improvement opportunities in FetchRawMetric: replacing time.After with time.NewTimer to prevent potential memory leaks upon context cancellation, and checking ctx.Err() after a failed fetch to fail fast when the context is cancelled.

[gemini-code-assist]

Using time.After in a select statement inside a loop can cause a temporary memory leak if the context is cancelled before the timer fires, as the underlying timer is not stopped and remains in memory until it expires. It is highly recommended to use time.NewTimer instead and ensure it is stopped when the select block exits.

			timer := time.NewTimer(delay)
			select {
			case <-ctx.Done():
				timer.Stop()
				return nil, ctx.Err()
			case <-timer.C:
			}

[googs1025]

plz check this to avoid memory leak

[yaojiejia]

plz check this to avoid memory leak

done

[gemini-code-assist]

If fetchAllMetricsFromURL fails because the context was cancelled or timed out, continuing the loop will log a misleading retry message and wait unnecessarily. Checking ctx.Err() immediately after the error allows the function to fail fast and return the context error without redundant logging or retries.

		allMetrics, err := ef.fetchAllMetricsFromURL(ctx, url)
		if err != nil {
			if ctx.Err() != nil {
				return nil, ctx.Err()
			}
			klog.V(4).InfoS("Failed to fetch metrics from URL",
				"attempt", attempt+1, "identifier", identifier, "url", url, "error", err)
			continue
		}

[Copilot]

Pull request overview

This PR fixes optimizer-based autoscaling samples by (1) updating a deprecated/ignored scale-down annotation to the currently supported key and (2) restoring external metric fetching from the GPU optimizer by honoring the configured path and bypassing the central engine-metric registry for optimizer-provided metrics.

Changes:

• Replace kpa.autoscaling.aibrix.ai/scale-down-delay with autoscaling.aibrix.ai/scale-down-cooldown-window across affected sample/config YAMLs.
• Add EngineMetricsFetcher.FetchRawMetric to fetch a raw Prometheus metric from an explicit URL (no central registry lookup).
• Update the GPU optimizer external-metrics fetch path to build protocol://endpoint/<path> and use FetchRawMetric; add unit tests for the new behavior.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
samples/volcano-engine/autoscaler.yaml	Updates scale-down annotation key to the supported one.
samples/heterogeneous/deepseek-coder-7b-v100-podautoscaler.yaml	Updates annotation key; switches metricSourceType to external.
samples/heterogeneous/deepseek-coder-7b-l20-podautoscaler.yaml	Updates annotation key; switches metricSourceType to external.
samples/deepseek-r1/deepseek-r1-autoscaling.yaml	Updates scale-down annotation key to the supported one.
samples/autoscaling/optimizer-kpa.yaml	Updates annotation key; switches metricSourceType to external.
samples/autoscaling/kpa.yaml	Updates scale-down annotation key to the supported one.
samples/autoscaling/external-metrics-kpa.yaml	Updates scale-down annotation key to the supported one.
pkg/metrics/engine_fetcher.go	Adds FetchRawMetric for unregistered/external Prometheus metric fetching from an explicit URL.
pkg/metrics/engine_fetcher_test.go	Adds unit tests covering FetchRawMetric success and failure modes.
pkg/controller/podautoscaler/metrics/fetcher.go	Builds optimizer URL from protocolType, endpoint, path and uses FetchRawMetric.
development/tutorials/distributed/fleet-autoscaling.yaml	Updates scale-down annotation key to the supported one.
development/app/config/templates/podautoscaler/podautoscaler_kpa.yaml	Updates scale-down annotation key to the supported one.
development/app/config/simulator/patch_podautoscaler_a100.yaml	Updates annotation key; switches metricSourceType to external.
development/app/config/heterogeneous/simulator_a40/patch_podautoscaler_a40.yaml	Updates annotation key; switches metricSourceType to external.
config/samples/autoscaling_v1alpha1_mock_llama.yaml	Updates scale-down annotation key to the supported one.
benchmarks/scenarios/autoscaling/deepseek-llm-7b-chat/optimizer-kpa.yaml	Updates annotation key; switches metricSourceType to external.
benchmarks/scenarios/autoscaling/deepseek-llm-7b-chat/kpa.yaml	Updates scale-down annotation key to the supported one.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[googs1025]

Overall LGTM. plz check the code agent review comment

[varungup90]

Here are a few findings and structural observations from reviewing the new FetchRawMetric path and the GPU optimizer integration:

1. Robustness & Error Handling

• Non-retried parse errors in FetchRawMetric (pkg/metrics/engine_fetcher.go): Unlike FetchTypedMetric, when GetCounterGaugeValue returns an error, FetchRawMetric returns immediately rather than logging and retrying with backoff. Transient parse issues or partially written scraped endpoints will prematurely fail the entire retry loop.
• Loss of root cause error context: When all retry attempts fail, the final error message loses the root cause details (which are only logged at klog.V(4)). Preserving or wrapping the underlying error would make misconfigured endpoints much easier to diagnose from status/logs.
• Unguarded family.Metric[0] indexing: There is no check when multiple metric instances/label combinations are present in a single returned family. It silently defaults to the first metric, which could pick the wrong series if an endpoint returns more than one.

---

2. Metric & Protocol Support

• Missing Histogram support in FetchRawMetric: FetchRawMetric hardcodes GetCounterGaugeValue (handling strictly Counter and Gauge types), silently dropping the Histogram parsing path supported by parseMetricInstance in the registry-driven flow.
• Manual URL construction in fetchFromGPUOptimizer (pkg/controller/podautoscaler/metrics/fetcher.go): Hand-rolling the URL string logic introduces minor edge cases (e.g., strings.TrimLeft won't handle trailing slashes on Endpoint). Switching to net/url or url.JoinPath would make path concatenation more robust. Additionally, the protocol-default fallback logic appears dead due to existing webhook validations.

---

3. Maintainability

• Retry loop duplication: The attempt / backoff / select retry machinery is now copy-pasted in 3 places across engine_fetcher.go (FetchTypedMetric, FetchRawMetric, and FetchAllTypedMetrics). Extracting a shared retry helper would clean up the duplication and prevent edge-case drift between fetchers._