Promote job metrics #113010

soltysh · 2022-10-12T14:10:12Z

What type of PR is this?

/kind cleanup
/sig apps

What this PR does / why we need it:

This should've been promoted along with the new cronjob controller, as part of kubernetes/enhancements#2214

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Promote  job-related metrics to stable to follow IndexedJobs GA, the following metrics had their name updated to match metrics API guidelines:
- job_sync_total -> job_syncs_total
- job_finished_total -> jobs_finished_total

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/issues/2214

soltysh · 2022-10-12T14:10:46Z

/triage accepted
/priority backlog

k8s-triage-robot · 2022-10-12T15:01:32Z

This PR may require stable metrics review.

Stable metrics are guaranteed to not change. Please review the documentation for the requirements and lifecycle of stable metrics and ensure that your metrics meet these guidelines.

soltysh · 2022-10-12T16:07:21Z

/assign @dgrisonnet
for metrics part

alculquicondor · 2022-10-12T17:20:42Z

pkg/controller/job/metrics/metrics.go

@@ -37,7 +37,7 @@ var (
 			Subsystem:      JobControllerSubsystem,
 			Name:           "job_sync_duration_seconds",


Let's add a new metric without the job_ prefix and deprecate this one. Same for others below.

while I agree that the job prefix feels a bit redundant here since it is implied by the subsystem, metrics such as job_controller_finished_total would not be very explicit. So, I am fine with keeping this prefix on some of the metrics that need to be explicitly scoped.

dgrisonnet · 2022-10-14T09:18:02Z

pkg/controller/job/metrics/metrics.go

@@ -51,7 +51,7 @@ var (
 			Subsystem:      JobControllerSubsystem,
 			Name:           "job_sync_total",


Counters generally have the object they are counting in the plural form to denote that it accumulated multiple data, so I would recommend renaming this metric to syncs_total.

dgrisonnet · 2022-10-14T09:24:37Z

pkg/controller/job/metrics/metrics.go

@@ -37,7 +37,7 @@ var (
 			Subsystem:      JobControllerSubsystem,
 			Name:           "job_sync_duration_seconds",


while I agree that the job prefix feels a bit redundant here since it is implied by the subsystem, metrics such as job_controller_finished_total would not be very explicit. So, I am fine with keeping this prefix on some of the metrics that need to be explicitly scoped.

alculquicondor · 2022-10-14T13:43:13Z

pkg/controller/job/metrics/metrics.go

@@ -64,7 +64,7 @@ var (
 			Subsystem:      JobControllerSubsystem,
 			Name:           "job_finished_total",
 			Help:           "The number of finished job",
-			StabilityLevel: metrics.ALPHA,
+			StabilityLevel: metrics.STABLE,


following @dgrisonnet's advice, this should be jobs_finished_total?

alculquicondor · 2022-10-14T13:43:51Z

pkg/controller/job/metrics/metrics.go

-			Name:      "job_pods_finished_total",
-			Help:      "The number of finished Pods that are fully tracked",
+			Subsystem:      JobControllerSubsystem,
+			Name:           "job_pods_finished_total",


Would job_controller_pods_finished_total be enough? or shall we keep the jobs_ prefix @dgrisonnet

alculquicondor · 2022-11-04T13:40:30Z

pkg/controller/job/metrics/metrics.go

@@ -37,7 +37,7 @@ var (
 			Subsystem:      JobControllerSubsystem,
 			Name:           "job_sync_duration_seconds",
 			Help:           "The time it took to sync a job",
-			StabilityLevel: metrics.ALPHA,
+			StabilityLevel: metrics.STABLE,
 			Buckets:        metrics.ExponentialBuckets(0.001, 2, 15),


note to self:

At 100 qps, creating 500 pods should take 5s. The max bucket is 16, so these buckets should give us enough signal.

alculquicondor

/lgtm

alculquicondor · 2022-11-04T13:44:20Z

pkg/controller/job/metrics/metrics.go

+			Name:           "jobs_pods_finished_total",
+			Help:           "The number of finished Pods that are fully tracked",


nit:

Suggested change

Name: "jobs_pods_finished_total",

Help: "The number of finished Pods that are fully tracked",

Name: "pods_finished_total",

Help: "The number of finished Pods that are fully tracked in Job statuses",

/hold cancel

oops, I meant
/lgtm cancel

prefixing with job is also fine with me since it adds a bit of context, but we can also remove it, both make sense in my opinion

In that case, we can leave it as is to avoid disturbing existing users of this metric.

sounds good to me, I would just change to singular pod since it is used as a way to scope where the pods are coming from rather than as a quantitative object

you mean singular job? We are counting pods.

job_pods_finished_total?

ah yes, I totally meant singular job

dgrisonnet · 2022-11-04T14:23:42Z

pkg/controller/job/metrics/metrics.go

-			Name:           "job_finished_total",
-			Help:           "The number of finished job",
-			StabilityLevel: metrics.ALPHA,
+			Name:           "jobs_finished_total",


To be consistent with the terminology used in the status API (https://github.com/kubernetes/api/blob/v0.25.3/batch/v1/types.go#L326-L401), would it make sense to rename this to jobs_completed_total?

No, because it includes both Complete and Failed jobs.

oh ok, thank you for clarifying :)

dgrisonnet · 2022-11-04T14:29:25Z

pkg/controller/job/metrics/metrics.go

+			Name:           "jobs_pods_finished_total",
+			Help:           "The number of finished Pods that are fully tracked",


prefixing with job is also fine with me since it adds a bit of context, but we can also remove it, both make sense in my opinion

alculquicondor

/lgtm

dgrisonnet · 2022-11-08T10:02:18Z

/lgtm
/approve

k8s-ci-robot · 2022-11-08T10:02:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrisonnet, soltysh

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/controller/job/OWNERS~~ [soltysh]
~~test/instrumentation/testdata/OWNERS~~ [dgrisonnet]

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 alculquicondor Oct 12, 2022

soltysh force-pushed the promote_job_metrics branch from d2402e3 to 7bc6d30 Compare October 12, 2022 14:10

k8s-ci-robot requested review from alculquicondor and mortent October 12, 2022 14:11

soltysh force-pushed the promote_job_metrics branch from 7bc6d30 to 693f47b Compare October 12, 2022 14:45

k8s-ci-robot assigned dgrisonnet Oct 12, 2022

alculquicondor reviewed Oct 12, 2022

View reviewed changes

dgrisonnet reviewed Oct 14, 2022

View reviewed changes

alculquicondor reviewed Oct 14, 2022

View reviewed changes

soltysh force-pushed the promote_job_metrics branch from 693f47b to 5d721b5 Compare November 4, 2022 12:50

alculquicondor reviewed Nov 4, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2022

alculquicondor reviewed Nov 4, 2022

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2022

dgrisonnet reviewed Nov 4, 2022

View reviewed changes

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

Promote job-related metrics to stable

39d9981

soltysh force-pushed the promote_job_metrics branch from 5d721b5 to 39d9981 Compare November 7, 2022 18:30

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

alculquicondor reviewed Nov 7, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2022

k8s-ci-robot merged commit aef9a37 into kubernetes:master Nov 8, 2022

k8s-ci-robot added this to the v1.26 milestone Nov 8, 2022

soltysh deleted the promote_job_metrics branch November 8, 2022 11:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promote job metrics #113010

Promote job metrics #113010

soltysh commented Oct 12, 2022 •

edited

soltysh commented Oct 12, 2022

k8s-triage-robot commented Oct 12, 2022

soltysh commented Oct 12, 2022

alculquicondor Oct 12, 2022

dgrisonnet Oct 14, 2022 •

edited

dgrisonnet Oct 14, 2022

dgrisonnet Oct 14, 2022 •

edited

alculquicondor Oct 14, 2022

alculquicondor Oct 14, 2022

alculquicondor Nov 4, 2022

alculquicondor left a comment

alculquicondor Nov 4, 2022

alculquicondor Nov 4, 2022

dgrisonnet Nov 4, 2022

alculquicondor Nov 4, 2022

dgrisonnet Nov 4, 2022

alculquicondor Nov 4, 2022

dgrisonnet Nov 4, 2022

dgrisonnet Nov 4, 2022

alculquicondor Nov 4, 2022

dgrisonnet Nov 4, 2022

dgrisonnet Nov 4, 2022

alculquicondor left a comment

dgrisonnet commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

		@@ -37,7 +37,7 @@ var (
		Subsystem: JobControllerSubsystem,
		Name: "job_sync_duration_seconds",

		@@ -51,7 +51,7 @@ var (
		Subsystem: JobControllerSubsystem,
		Name: "job_sync_total",

		Name: "jobs_pods_finished_total",
		Help: "The number of finished Pods that are fully tracked",

Promote job metrics #113010

Promote job metrics #113010

Conversation

soltysh commented Oct 12, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

soltysh commented Oct 12, 2022

k8s-triage-robot commented Oct 12, 2022

soltysh commented Oct 12, 2022

Choose a reason for hiding this comment

dgrisonnet Oct 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgrisonnet Oct 14, 2022 • 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

alculquicondor left a 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

alculquicondor left a comment

Choose a reason for hiding this comment

dgrisonnet commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

soltysh commented Oct 12, 2022 •

edited

dgrisonnet Oct 14, 2022 •

edited

dgrisonnet Oct 14, 2022 •

edited