Change scheduler metrics to conform metrics guidelines #72332

danielqsj · 2018-12-26T09:02:35Z

What type of PR is this?

/sig instrumentation

What this PR does / why we need it:

As part of kubernetes metrics overhaul, change scheduler metrics to conform Kubernetes metrics instrumentation guidelines.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

This patch does not remove the existing metrics but mark them as deprecated.
We need 2 releases for users to convert monitoring configuration.

Does this PR introduce a user-facing change?:

Change scheduler metrics to conform metrics guidelines.
The following metrics are deprecated, and will be removed in a future release:
* `e2e_scheduling_latency_microseconds`
* `scheduling_algorithm_latency_microseconds`
* `scheduling_algorithm_predicate_evaluation`
* `scheduling_algorithm_priority_evaluation`
* `scheduling_algorithm_preemption_evaluation`
* `binding_latency_microseconds`
Please convert to the following metrics:
* `e2e_scheduling_latency_seconds`
* `scheduling_algorithm_latency_seconds`
* `scheduling_algorithm_predicate_evaluation_seconds`
* `scheduling_algorithm_priority_evaluation_seconds`
* `scheduling_algorithm_preemption_evaluation_seconds`
* `binding_latency_seconds`

danielqsj · 2018-12-26T09:02:56Z

/kind feature
/assign @brancz

Huang-Wei · 2018-12-26T18:31:40Z

pkg/scheduler/core/generic_scheduler.go

@@ -182,14 +182,16 @@ func (g *genericScheduler) Schedule(pod *v1.Pod, nodeLister algorithm.NodeLister
 			FailedPredicates: failedPredicateMap,
 		}
 	}
-	metrics.SchedulingAlgorithmPredicateEvaluationDuration.Observe(metrics.SinceInMicroseconds(startPredicateEvalTime))
+	metrics.SchedulingAlgorithmPredicateEvaluationDuration.Observe(metrics.SinceInSeconds(startPredicateEvalTime))


Are you saying replacing "microseconds" with "seconds" is a common pattern? If yes, can you show that where it's documented?

Correct.
From the KEP kubernetes-metrics-overhaul, Kubernetes metrics should follow Prometheus best practices.
The seconds is the suggested base unit for time type metrics. And seconds already been widely used in other Kubernetes metrics.
So we should change these metrics to let them more consistent with others.

Huang-Wei

A nit: please add explicit "DEPRECATED" notice (maybe in release-note section) to notify users that original "microseconds" metrics will be deprecated, so that we can remove them in future releases.

Otherwise
/lgtm

Huang-Wei · 2018-12-28T23:03:20Z

/retest

Huang-Wei · 2018-12-28T23:03:37Z

/priority important-longterm

danielqsj · 2018-12-29T01:55:23Z

@Huang-Wei thanks, release-note is updated.

ravisantoshgudimetla · 2018-12-30T16:40:47Z

pkg/scheduler/metrics/metrics.go

 		prometheus.HistogramOpts{
 			Subsystem: SchedulerSubsystem,
 			Name:      "e2e_scheduling_latency_microseconds",
-			Help:      "E2e scheduling latency (scheduling algorithm + binding)",
+			Help:      "E2e scheduling latency in microseconds (scheduling algorithm + binding)",


I think adding deprecated in help might be useful.

brancz · 2019-01-07T15:08:15Z

Agreed with @ravisantoshgudimetla could you start each of the deprecated metric's descriptions with "(deprecated)"? Thanks! Otherwise this lgtm!

danielqsj · 2019-01-08T05:09:38Z

@brancz @Huang-Wei
PR rebased and add Deprecated in metrics description. PTAL

brancz · 2019-01-10T17:08:28Z

/lgtm
/approve

k8s-ci-robot · 2019-01-10T17:10:27Z

@brancz: GitHub didn't allow me to assign the following users: sig-scheduling-maintainers.

Note that only kubernetes members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign sig-scheduling-maintainers

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.

brancz · 2019-01-10T17:12:36Z

cc @kubernetes/sig-scheduling-pr-reviews

danielqsj · 2019-01-14T03:23:11Z

@ravisantoshgudimetla can you help review this? Thanks
/assign @ravisantoshgudimetla

ravisantoshgudimetla

Thank you @danielqsj for working on this PR.

/lgtm

k8s-ci-robot · 2019-01-15T03:57:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, danielqsj, ravisantoshgudimetla

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:

~~pkg/scheduler/OWNERS~~ [ravisantoshgudimetla]

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

k8s-ci-robot assigned brancz Dec 26, 2018

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Dec 26, 2018

k8s-ci-robot requested review from ravisantoshgudimetla and resouer December 26, 2018 09:03

danielqsj mentioned this pull request Dec 26, 2018

Change existing metrics to conform metrics guidelines #72333

Closed

9 tasks

Huang-Wei reviewed Dec 26, 2018

View reviewed changes

Huang-Wei reviewed Dec 28, 2018

View reviewed changes

k8s-ci-robot assigned Huang-Wei Dec 28, 2018

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

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 28, 2018

ravisantoshgudimetla reviewed Dec 30, 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 Jan 5, 2019

danielqsj added 2 commits January 8, 2019 11:16

Change scheduler metrics to conform guideline

5b42a84

Mark deprecated in related scheduler metrics

0f516f7

danielqsj force-pushed the ks branch from 8e322e6 to 0f516f7 Compare January 8, 2019 05:08

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 8, 2019

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

danielqsj mentioned this pull request Jan 8, 2019

Mark deprecated in related workqueue metrics #72679

Merged

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 10, 2019

k8s-ci-robot assigned ravisantoshgudimetla Jan 14, 2019

ravisantoshgudimetla approved these changes Jan 15, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2019

k8s-ci-robot merged commit b91cbf7 into kubernetes:master Jan 15, 2019

brancz mentioned this pull request Aug 7, 2019

Kubernetes Metrics Overhaul kubernetes/enhancements#1206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change scheduler metrics to conform metrics guidelines #72332

Change scheduler metrics to conform metrics guidelines #72332

danielqsj commented Dec 26, 2018 •

edited

Loading

danielqsj commented Dec 26, 2018

Huang-Wei Dec 26, 2018

danielqsj Dec 27, 2018

Huang-Wei left a comment

Huang-Wei commented Dec 28, 2018

Huang-Wei commented Dec 28, 2018

danielqsj commented Dec 29, 2018

ravisantoshgudimetla Dec 30, 2018

brancz commented Jan 7, 2019

danielqsj commented Jan 8, 2019 •

edited

Loading

brancz commented Jan 10, 2019

k8s-ci-robot commented Jan 10, 2019

brancz commented Jan 10, 2019

danielqsj commented Jan 14, 2019

ravisantoshgudimetla left a comment

k8s-ci-robot commented Jan 15, 2019

Change scheduler metrics to conform metrics guidelines #72332

Change scheduler metrics to conform metrics guidelines #72332

Conversation

danielqsj commented Dec 26, 2018 • edited Loading

danielqsj commented Dec 26, 2018

Huang-Wei Dec 26, 2018

Choose a reason for hiding this comment

danielqsj Dec 27, 2018

Choose a reason for hiding this comment

Huang-Wei left a comment

Choose a reason for hiding this comment

Huang-Wei commented Dec 28, 2018

Huang-Wei commented Dec 28, 2018

danielqsj commented Dec 29, 2018

ravisantoshgudimetla Dec 30, 2018

Choose a reason for hiding this comment

brancz commented Jan 7, 2019

danielqsj commented Jan 8, 2019 • edited Loading

brancz commented Jan 10, 2019

k8s-ci-robot commented Jan 10, 2019

brancz commented Jan 10, 2019

danielqsj commented Jan 14, 2019

ravisantoshgudimetla left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 15, 2019

danielqsj commented Dec 26, 2018 •

edited

Loading

danielqsj commented Jan 8, 2019 •

edited

Loading