Fixing scheduling latency metrics #64316

krzysied · 2018-05-25T14:18:53Z

What this PR does / why we need it:
Allows to measure and to display scheduling latency metrics during tests. Provides new functionality of resetting scheduler latency metrics.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #63493

Special notes for your reviewer:
E2eSchedulingLatency, SchedulingAlgorithmLatency, BindingLatency are now available
as subtypes of OperationLatency.

Release note:

NONE

krzysied · 2018-05-25T14:23:01Z

/retest

krzysied · 2018-05-25T14:25:17Z

/assign shyamjvs

shyamjvs · 2018-05-25T15:41:30Z

cmd/kube-scheduler/app/server.go

@@ -225,7 +227,16 @@ func buildHandlerChain(handler http.Handler, authn authenticator.Request, authz
 func newMetricsHandler(config *componentconfig.KubeSchedulerConfiguration) http.Handler {
 	pathRecorderMux := mux.NewPathRecorderMux("kube-scheduler")
 	configz.InstallHandler(pathRecorderMux)
-	pathRecorderMux.Handle("/metrics", prometheus.Handler())
+	//metrics.Register()


Could you remove this commented code?

shyamjvs · 2018-05-25T15:45:25Z

cmd/kube-scheduler/app/server.go

@@ -243,7 +254,15 @@ func newHealthzHandler(config *componentconfig.KubeSchedulerConfiguration, separ
 	healthz.InstallHandler(pathRecorderMux)
 	if !separateMetrics {
 		configz.InstallHandler(pathRecorderMux)
-		pathRecorderMux.Handle("/metrics", prometheus.Handler())
+		defaultMetricsHandler := prometheus.Handler().ServeHTTP


Instead of duplicating the same code as above, could you move this part into a separate function?
Like here - https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/server/routes/metrics.go#L44

shyamjvs · 2018-05-25T15:46:28Z

pkg/scheduler/metrics/metrics.go

@@ -25,23 +25,26 @@ import (

 const schedulerSubsystem = "scheduler"

+const BindingType = "binding"


Please put these 3 values within a single const block.

shyamjvs · 2018-05-25T15:49:34Z

pkg/scheduler/metrics/metrics.go

+const SchedulingAlgorithmType ="scheduling_algorithm"
+const E2eSchedulingType = "e2e_scheduling"
+
+type resettableCollector interface {


I don't think this should be called a resettableCollector (because it's just a collector, and not all of them are resettable). Can't you directly just use a prometheus.Collector?

shyamjvs · 2018-05-25T15:52:39Z

pkg/scheduler/metrics/metrics.go

-	)
-	SchedulingAlgorithmLatency = prometheus.NewHistogram(
-		prometheus.HistogramOpts{
+	OperationLatency = prometheus.NewSummaryVec(


Could you name this variable as SchedulingLatency instead? That'll make it more clearer.

shyamjvs · 2018-05-25T15:53:45Z

pkg/scheduler/metrics/metrics.go

-			Name:      "scheduling_algorithm_latency_microseconds",
-			Help:      "Scheduling algorithm latency",
-			Buckets:   prometheus.ExponentialBuckets(1000, 2, 15),
+			Name: "operation_latency_microseconds",


Could you rename this to scheduling_latencies_summary instead? (to keep up with the naming convention used for apiserver)

shyamjvs · 2018-05-25T15:57:20Z

pkg/scheduler/metrics/metrics.go

-			Help:      "Scheduling algorithm latency",
-			Buckets:   prometheus.ExponentialBuckets(1000, 2, 15),
+			Name: "operation_latency_microseconds",
+			Help: "operation latency",


I'd suggest writing something more descriptive in the Help string. Like "Scheduling latency in microseconds split by sub-parts of the scheduling operation".

shyamjvs · 2018-05-25T15:58:56Z

pkg/scheduler/metrics/metrics.go

+			Name: "operation_latency_microseconds",
+			Help: "operation latency",
+			// Make the sliding window of 5h.
+			// TODO: The value for this should be based on our SLI definition (medium term).


Can you change:
our -> some
medium -> long

shyamjvs · 2018-05-25T15:59:24Z

pkg/scheduler/metrics/metrics.go

 		},
+		[]string{"operationType"},


Just operation is enough.

shyamjvs · 2018-05-25T16:04:42Z

test/e2e/framework/metrics_util.go

-		switch sample.Metric[model.MetricNameLabel] {
-		case "scheduler_scheduling_algorithm_latency_microseconds":
+		switch sample.Metric["operationType"] {
+		case "scheduling_algorithm":


Please replace these hard-coded strings with their corresponding const vars you defined above.

shyamjvs · 2018-05-25T16:06:31Z

test/e2e/framework/metrics_util.go

@@ -520,6 +524,53 @@ func VerifySchedulerLatency(c clientset.Interface) (*SchedulingLatency, error) {
 	return latency, nil
 }

+func ResetSchedulerLatency(c clientset.Interface) error {


Rename this to ResetSchedulerLatencyMetrics?

shyamjvs · 2018-05-28T10:28:20Z

pkg/scheduler/metrics/metrics.go

-const schedulerSubsystem = "scheduler"
+const (
+	SchedulerSubsystem = "scheduler"
+	SchedulingLatencyName = "scheduling_latencies_summary"


I don't think it's too useful to have this const. Maybe just put the string directly inside the metric definition (like other metrics)?

Why do we want to have operation types as a consts and and metric name as a string? Shouldn't we have coherent style - all strings or all consts?

Yes, you have a valid point there.

So ideally we should make all hard-coded strings in files as global constants (so we only define the 'real' string once and later on only have references to it). However, moving too many things to global consts makes the file larger and harder to read. So the rule I'm usually following is to make a string const only if it's referenced enough times (3-4) in the codebase, so it's worth it. But this is subjective :)

shyamjvs · 2018-05-28T10:29:38Z

pkg/scheduler/metrics/metrics.go

+	SchedulerSubsystem = "scheduler"
+	SchedulingLatencyName = "scheduling_latencies_summary"
+
+	OperationLabel = "opertion"


Also can we rename this variable to OperationType and change the below ones to simply Binding, SchedulingAlgorithm, etc? Will make the meaning more clearer imo.

shyamjvs · 2018-05-28T10:52:59Z

test/e2e/framework/metrics_util.go

@@ -130,6 +131,8 @@ func (m *MetricsForE2E) SummaryKind() string {
 	return "MetricsForE2E"
 }

+var SchedulingLatencyMetricName = model.LabelValue(schedulerMetric.SchedulerSubsystem + "_" + schedulerMetric.SchedulingLatencyName)


Maybe it'll be cleaner to have sth similar to apiserver like this:

var InterestingSchedulerMetrics = []string{ "scheduler_whatever" }

The problem is that scheduler latency metric has a special handling (other metrics will have different structure/labels). Always at some point you will need to verify if name == SCHEDULER_METRIC_NAME. That's why I prefer having variable to having value in array.

Yes. However, isn't it the same for apiserver metrics too?

I don't think so. The apiserver e2e metric are group of metrics with the some structure, so none of those metric requires special handling.
The apiserver latency metrics are more similar to scheduler latency metric. However in this case, metric names are compared to explicit strings.

shyamjvs · 2018-05-28T10:58:23Z

test/e2e/framework/metrics_util.go

@@ -521,6 +528,53 @@ func VerifySchedulerLatency(c clientset.Interface) (*SchedulingMetrics, error) {
 	return latency, nil
 }

+func ResetSchedulerLatencyMetrics(c clientset.Interface) error {


I see some overlap of this function with getSchedulingLatency(). Can we somehow unify them?

shyamjvs · 2018-05-28T10:59:21Z

test/e2e/scalability/density.go

@@ -442,6 +440,7 @@ var _ = SIGDescribe("Density", func() {

 		uuid = string(utiluuid.NewUUID())

+		framework.ExpectNoError(framework.ResetSchedulerLatencyMetrics(c))


Actually, let's name this function as ResetSchedulerMetrics (as we may expand it in future to reset more metrics than just latency).

wojtek-t · 2018-05-30T09:23:32Z

/approve

shyamjvs · 2018-05-30T09:31:45Z

/lgtm

k8s-ci-robot · 2018-05-30T09:31:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: krzysied, shyamjvs, wojtek-t

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:

~~cmd/kube-scheduler/OWNERS~~ [wojtek-t]
~~pkg/scheduler/OWNERS~~ [wojtek-t]
~~test/OWNERS~~ [shyamjvs,wojtek-t]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

shyamjvs · 2018-05-30T13:01:09Z

/kind cleanup
/priority important-soon

shyamjvs · 2018-05-30T13:03:42Z

These metrics are important for our tests. @jberkus could you please approve for milestone?

wojtek-t · 2018-05-30T14:38:42Z

Approving as scalability sig lead

k8s-github-robot · 2018-05-30T14:39:17Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@krzysied @shyamjvs @wojtek-t

Pull Request Labels

sig/scalability sig/scheduling: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/cleanup: Adding tests, refactoring, fixing old bugs.

Help

k8s-github-robot · 2018-05-30T15:42:14Z

Automatic merge from submit-queue (batch tested with PRs 63328, 64316, 64444, 64449, 64453). If you want to cherry-pick this change to another branch, please follow the instructions here.

bsalamat · 2018-05-30T18:52:28Z

pkg/scheduler/metrics/metrics.go

-const schedulerSubsystem = "scheduler"
+const (
+	// SchedulerSubsystem - subsystem name used by scheduler
+	SchedulerSubsystem = "scheduler"


Renaming metrics is a breaking change. It is not backward compatible and is going to break monitoring systems that rely on these metrics. This PR should be reverted.

Yeah, that's a valid concern and also the option I was initially suggesting in #63493 (comment). I've cc'd there sig-scheduling and you - following lazy consensus since there weren't any objections :)

So, what do you think about re-adding the old metric alongside the new one (instead of replacing it)?

Wasn't this metric broken anyway?

I don't think so.. IIUC we were just not capturing the values properly in our test framework (which expects summary instead of histogram).

If we're going to add "aliases" for metrics, I think we should make more of a push toward prometheus best practices. The various histograms in this file and their relationships are a bit confusing.

I agree with @misterikkit. Some of these names were not following Prometheus best practices. The new ones introduces in this PR are not following those best practices either. We should definitely keep the old ones for backward compatibility and maybe add new aliases based on the best practices.

bsalamat · 2018-06-01T21:29:07Z

@krzysied Did you have a chance to revert the changes to the metrics names?

shyamjvs · 2018-06-04T08:55:42Z

@bsalamat @misterikkit Does the following sgty:

Re-introduce the old metrics (which are histogram-type)
Keep the new metrics (which are summary-type)

And to clarify, this is not a question of "aliases" - the new metrics aren't just a name change, they're introducing summary-type metrics (like mentioned above). And the reason is we want to have accurate percentiles of scheduling latencies for measuring the performance better (some context here - #63493 (comment))

And wrt naming the new metrics, sure we can change the name.. that's not a problem. Could you point us to what best practices you're referring?

shyamjvs · 2018-06-04T08:58:06Z

@krzysied can make the changes based on what we agree above.

misterikkit · 2018-06-04T19:34:24Z

the new metrics aren't just a name change, they're introducing summary-type metrics

Ahh, I failed to notice the distinction. I guess the sliding window is a more useful view of performance over the last N minutes, but is that better for observing health in a live cluster?

And wrt naming the new metrics, sure we can change the name.. that's not a problem. Could you point us to what best practices you're referring?

https://prometheus.io/docs/practices/naming/

krzysied · 2018-06-06T16:13:41Z

I've created the PR #64838 that re-introduces old metrics and changes new one to satisfy prometheus best practice.

@bsalamat @misterikkit Could you please take a look and give an option as to whether this change looks good to you?

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Adding summary metric for scheduling latency **What this PR does / why we need it**: Re-introduces histogram metrics for the backward compatibility. Changes SchedulingLatency metric to satisfy prometheus best practice. ref #64316 **Release note**: ```release-note NONE ```

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 25, 2018

k8s-ci-robot requested review from gmarek and ncdc May 25, 2018 14:19

k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label May 25, 2018

k8s-ci-robot assigned shyamjvs May 25, 2018

shyamjvs reviewed May 25, 2018

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2018

krzysied force-pushed the scheduling_latency_metric branch 2 times, most recently from 6162545 to fe35b2a Compare May 28, 2018 10:01

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 28, 2018

krzysied force-pushed the scheduling_latency_metric branch from fe35b2a to 8f31bb6 Compare May 28, 2018 10:06

shyamjvs reviewed May 28, 2018

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2018

shyamjvs added this to the v1.11 milestone May 30, 2018

k8s-github-robot added the milestone/incomplete-labels label May 30, 2018

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels May 30, 2018

k8s-github-robot added milestone/needs-approval and removed milestone/incomplete-labels labels May 30, 2018

wojtek-t added status/approved-for-milestone sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. labels May 30, 2018

k8s-github-robot removed the milestone/needs-approval label May 30, 2018

k8s-github-robot merged commit a2d8636 into kubernetes:master May 30, 2018

bsalamat reviewed May 30, 2018

View reviewed changes

krzysied mentioned this pull request Jun 6, 2018

Adding summary metric for scheduling latency #64838

Merged

shyamjvs mentioned this pull request Jun 21, 2018

Split scheduler latency metric to fine-grained steps #65306

Merged

		@@ -25,23 +25,26 @@ import (

		const schedulerSubsystem = "scheduler"

		const BindingType = "binding"

		@@ -442,6 +440,7 @@ var _ = SIGDescribe("Density", func() {

		uuid = string(utiluuid.NewUUID())

		framework.ExpectNoError(framework.ResetSchedulerLatencyMetrics(c))

Fixing scheduling latency metrics #64316

Fixing scheduling latency metrics #64316

Conversation

krzysied commented May 25, 2018

krzysied commented May 25, 2018

krzysied commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented May 30, 2018

shyamjvs commented May 30, 2018

k8s-ci-robot commented May 30, 2018

shyamjvs commented May 30, 2018

shyamjvs commented May 30, 2018

wojtek-t commented May 30, 2018

k8s-github-robot commented May 30, 2018

k8s-github-robot commented May 30, 2018

bsalamat May 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsalamat commented Jun 1, 2018

shyamjvs commented Jun 4, 2018

shyamjvs commented Jun 4, 2018

misterikkit commented Jun 4, 2018

krzysied commented Jun 6, 2018

bsalamat May 30, 2018 •

edited