-
Notifications
You must be signed in to change notification settings - Fork 665
[Bug] Fix external metric fetching for GPU optimizer autoscaling and replace dead scale-down annotation in samples #2616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
5c91d92
a9ef25c
93718d5
45a47ac
0230459
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -156,6 +156,49 @@ func (ef *EngineMetricsFetcher) FetchTypedMetric(ctx context.Context, endpoint, | |
| metricName, identifier, ef.config.MaxRetries+1) | ||
| } | ||
|
|
||
| // FetchRawMetric fetches a metric by its raw Prometheus name from an explicit metrics URL, | ||
| // bypassing the central metric registry. External sources such as the GPU optimizer expose | ||
| // caller-defined metrics on caller-defined paths, so neither the registry's metric | ||
| // definitions nor its per-engine paths apply to them. | ||
| func (ef *EngineMetricsFetcher) FetchRawMetric(ctx context.Context, url, identifier, rawMetricName string) (MetricValue, error) { | ||
| for attempt := 0; attempt <= ef.config.MaxRetries; attempt++ { | ||
| if attempt > 0 { | ||
| delay := ef.calculateBackoffDelay(attempt) | ||
| klog.V(4).InfoS("Retrying raw metric fetch", | ||
| "attempt", attempt, "delay", delay, "identifier", identifier, "metric", rawMetricName) | ||
|
|
||
| select { | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| case <-time.After(delay): | ||
| } | ||
| } | ||
|
|
||
| allMetrics, err := ef.fetchAllMetricsFromURL(ctx, url) | ||
| if err != nil { | ||
| klog.V(4).InfoS("Failed to fetch metrics from URL", | ||
| "attempt", attempt+1, "identifier", identifier, "url", url, "error", err) | ||
| continue | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If 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
} |
||
|
|
||
| family, exists := allMetrics[rawMetricName] | ||
| if !exists || len(family.Metric) == 0 { | ||
| klog.V(4).InfoS("Raw metric not found in response", | ||
| "attempt", attempt+1, "identifier", identifier, "metric", rawMetricName) | ||
| continue | ||
| } | ||
|
|
||
| metricValue, err := GetCounterGaugeValue(family.Metric[0], family.GetType()) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse raw metric %s from %s: %w", rawMetricName, identifier, err) | ||
| } | ||
| return metricValue, nil | ||
| } | ||
|
|
||
| return nil, fmt.Errorf("failed to fetch raw metric %s from %s after %d attempts", | ||
| rawMetricName, identifier, ef.config.MaxRetries+1) | ||
| } | ||
|
|
||
| // FetchAllTypedMetrics fetches all available typed metrics from an engine endpoint | ||
| func (ef *EngineMetricsFetcher) FetchAllTypedMetrics(ctx context.Context, endpoint, engineType, identifier string, requestedMetrics []string) (*EngineMetricsResult, error) { | ||
| result := &EngineMetricsResult{ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
time.Afterin aselectstatement 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 usetime.NewTimerinstead and ensure it is stopped when the select block exits.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plz check this to avoid memory leak
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done