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
cleanup(scheduler): move metricRecorder to metrics package #115519
cleanup(scheduler): move metricRecorder to metrics package #115519
Conversation
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
4ab9d46
to
18ef9d6
Compare
|
||
// ObservePluginDurationAsync observes the plugin_execution_duration_seconds metric. | ||
// The metric will be flushed to Prometheus asynchronously. | ||
func (r *MetricAsyncRecorder) ObservePluginDurationAsync(extensionPoint, pluginName, status string, value float64) { |
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.
is it intentional to change the type of status from Status to string?
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.
Yes. We cannot use k8s.io/kubernetes/pkg/scheduler/framework
here due to the import cycle.
# When we import k8s.io/kubernetes/pkg/scheduler/framework here,
# and build the scheduler.
package k8s.io/kubernetes/pkg/scheduler/metrics
imports k8s.io/kubernetes/pkg/scheduler/framework
imports k8s.io/kubernetes/pkg/scheduler/framework/parallelize
imports k8s.io/kubernetes/pkg/scheduler/metrics: import cycle not allowed
/lgtm |
LGTM label has been added. Git tree hash: c3e6307bdb5f86d612868cd1ce7c3f537334d52e
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Huang-Wei, sanposhiho The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
"k8s.io/component-base/metrics" | ||
"time" | ||
|
||
k8smetrics "k8s.io/component-base/metrics" |
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.
/hold
can we use metrics directly here?
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.
Good catch. Fixed.
18ef9d6
to
aa7b176
Compare
What do you mean |
I'm not sure I understood correctly what you mean but I'd like to do (in the next PR) is:
|
Why the "same" MetricsAsyncRecorder object? -> Each |
Got the idea, I thought the
/lgtm |
LGTM label has been added. Git tree hash: a1086fa25a7c36e54d9981960677cd682a6f8b3c
|
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
#115083 wants to record
plugin_execution_duration_seconds
metric for PreEnqueue in the scheduling queue.Currently, all extension points record
plugin_execution_duration_seconds
viametricsRecorder
which records the metrics in a separate goroutine. Then, it's better to utilize this for PreEnqueue as well for better performance.The scheduling queue package ("k8s.io/kubernetes/pkg/scheduler/internal/queue") is outside of the framework package and the
metricsRecorder
in the frameworkImpl struct isn't callable from the scheduling queue.This PR just moves it to
pkg/scheduler/metrics/metric_recorder.go
with renaming it toMetricsAsyncRecorder
so that we can initializeMetricsAsyncRecorder
outside the framework and use the sameMetricsAsyncRecorder
object both in the framework and the scheduling queue (will be done in another PR).Which issue(s) this PR fixes:
Related to #115083
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: