Add metrics for admission attempts count and duration #233

alculquicondor · 2022-04-27T18:43:32Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add metrics for admission attempts count and duration, needed to track that the scheduler is doing work.

Add role and rolebinding for prometheus to be able to list the services in the kueue-system namespace.

Which issue(s) this PR fixes:

Part of #199 (4)

Special notes for your reviewer:

This doesn't add the prometheus ServiceMonitor, Role or RoleBinding by default, as users might have their own monitoring system, or they might not want to setup any prometheus at all.

In a follow up, I'll document how users can setup prometheus.

k8s-ci-robot · 2022-04-27T18:43:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor

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:

~~OWNERS~~ [alculquicondor]

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

alculquicondor · 2022-04-27T19:12:10Z

/assign @kerthcet @ahg-g

k8s-ci-robot · 2022-04-27T19:12:12Z

@alculquicondor: GitHub didn't allow me to assign the following users: kerthcet.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @kerthcet @ahg-g

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.

alculquicondor · 2022-04-27T19:12:22Z

/hold
to prevent premature merge

Add role and rolebinding for prometheus to be able to list the services in the kueue-system namespace. Change-Id: I77cf51536ebf53ece9a4ba2d8457dbc3e71d1e8d

ahg-g · 2022-04-28T19:15:27Z

config/default/manager_auth_proxy_patch.yaml

@@ -20,6 +20,3 @@ spec:
        - containerPort: 8443
          protocol: TCP
          name: https
-      - name: manager


why did you decide to remove this? :)

It initially had flags related to metrics, but they are all in the component config now. There is no point of keeping it in the patch.

ahg-g · 2022-04-28T19:18:17Z

pkg/metrics/metrics.go

+		prometheus.CounterOpts{
+			Subsystem: subsystemName,
+			Name:      "admission_attempts_total",
+			Help:      "Number of attempts to admit pods, by result. `success` means that at least one workload was admitted, `inadmissible` means that no workload was admitted.",


s/pods/workloads

"at least one"

hmm, I don't think this will be easy to understand, we need to clarify that each attempt could admit more than one workload.

Clarified.

unless we should increase the counter with the number of workloads?

Although that's not very useful if you want to measure the throughput of the scheduler.

sg, we should have a separate gauge metric tracking the number of workloads broken down by pending vs admitted and by ClusterQueue

ahg-g · 2022-04-28T19:19:59Z

pkg/metrics/metrics.go

+		prometheus.HistogramOpts{
+			Subsystem: subsystemName,
+			Name:      "admission_attempt_duration_seconds",
+			Help:      "Latency of an admission attempt",


Suggested change

Help: "Latency of an admission attempt",

Help: "Latency of an admission attempt broken down by result",

Change-Id: If6f34c08cc4f2d222815ecb257bd37ef5b4ae969

ahg-g · 2022-04-28T21:07:39Z

/label tide/merge-method-squash

ahg-g · 2022-04-28T21:07:48Z

/lgtm

ahg-g · 2022-04-29T01:46:12Z

/hold cancel

nayihz · 2022-05-16T14:58:56Z

config/prometheus/role.yaml

+kind: Role
+metadata:
+  name: prometheus-k8s
+  namespace: system


Why do we create Role in system namespace? What is system namespace for?

well, this is where kustomize works, refer to

kueue/config/default/kustomization.yaml

Lines 4 to 9 in a4703d3

# Value of this field is prepended to the

# names of all resources, e.g. a deployment named

# "wordpress" becomes "alices-wordpress".

# Note that it should also match with the prefix (text before '-') of the namespace

# field above.

namePrefix: kueue-

Got it. Thanks

…s#233) * Add metrics for admission attempts count and duration Add role and rolebinding for prometheus to be able to list the services in the kueue-system namespace. Change-Id: I77cf51536ebf53ece9a4ba2d8457dbc3e71d1e8d * Improve help messages Change-Id: If6f34c08cc4f2d222815ecb257bd37ef5b4ae969

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 27, 2022

k8s-ci-robot requested review from ArangoGutierrez and denkensk April 27, 2022 18:43

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 27, 2022

alculquicondor force-pushed the admission_metrics branch from f519b44 to 3173820 Compare April 27, 2022 18:44

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 27, 2022

alculquicondor force-pushed the admission_metrics branch 5 times, most recently from 98ba201 to dbc015e Compare April 27, 2022 19:11

k8s-ci-robot assigned ahg-g Apr 27, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 27, 2022

alculquicondor force-pushed the admission_metrics branch from dbc015e to f0d01ea Compare April 27, 2022 19:13

Add metrics for admission attempts count and duration

cce3cdf

Add role and rolebinding for prometheus to be able to list the services in the kueue-system namespace. Change-Id: I77cf51536ebf53ece9a4ba2d8457dbc3e71d1e8d

alculquicondor force-pushed the admission_metrics branch from f0d01ea to cce3cdf Compare April 27, 2022 19:14

ahg-g reviewed Apr 28, 2022

View reviewed changes

Improve help messages

f8f611c

Change-Id: If6f34c08cc4f2d222815ecb257bd37ef5b4ae969

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 28, 2022

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 29, 2022

k8s-ci-robot merged commit 7cb7d69 into kubernetes-sigs:main Apr 29, 2022

nayihz reviewed May 16, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metrics for admission attempts count and duration #233

Add metrics for admission attempts count and duration #233

alculquicondor commented Apr 27, 2022 •

edited

Loading

k8s-ci-robot commented Apr 27, 2022

alculquicondor commented Apr 27, 2022

k8s-ci-robot commented Apr 27, 2022

alculquicondor commented Apr 27, 2022

ahg-g Apr 28, 2022

alculquicondor Apr 28, 2022

ahg-g Apr 28, 2022

alculquicondor Apr 28, 2022

ahg-g Apr 28, 2022 •

edited

Loading

ahg-g Apr 28, 2022

alculquicondor Apr 28, 2022

ahg-g commented Apr 28, 2022

ahg-g commented Apr 28, 2022

ahg-g commented Apr 29, 2022

nayihz May 16, 2022

kerthcet May 16, 2022

nayihz May 17, 2022

	Help: "Latency of an admission attempt",
	Help: "Latency of an admission attempt broken down by result",

	# Value of this field is prepended to the
	# names of all resources, e.g. a deployment named
	# "wordpress" becomes "alices-wordpress".
	# Note that it should also match with the prefix (text before '-') of the namespace
	# field above.
	namePrefix: kueue-

Add metrics for admission attempts count and duration #233

Add metrics for admission attempts count and duration #233

Conversation

alculquicondor commented Apr 27, 2022 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

k8s-ci-robot commented Apr 27, 2022

alculquicondor commented Apr 27, 2022

k8s-ci-robot commented Apr 27, 2022

alculquicondor commented Apr 27, 2022

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

ahg-g Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahg-g commented Apr 28, 2022

ahg-g commented Apr 28, 2022

ahg-g commented Apr 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Apr 27, 2022 •

edited

Loading

ahg-g Apr 28, 2022 •

edited

Loading