Adding system metrics support #97

Edwinhr716 · 2024-04-11T22:35:47Z

What type of PR is this?

/kind feature

What this PR does / why we need it

Which issue(s) this PR fixes

Fixes #87

Special notes for your reviewer

Does this PR introduce a user-facing change?

adding recreate_group_times, rolling_update_duration metrics, and replica_ready_status_duration

k8s-ci-robot · 2024-04-11T22:35:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Edwinhr716
Once this PR has been reviewed and has the lgtm label, please assign ahg-g for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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 · 2024-04-11T22:35:56Z

Hi @Edwinhr716. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

kerthcet · 2024-04-12T02:29:09Z

/ok-to-test

…es, and functionality of the latter

ahg-g · 2024-04-16T16:37:44Z

is this ready for review? asking because it is still marked as WIP

Edwinhr716 · 2024-04-16T17:09:48Z

not yet, currently working on adding a metric for latency for replicas to be scheduled and become ready, and also plan on adding unit tests

Edwinhr716 · 2024-04-17T17:43:00Z

Done with most of it, PTAL. Only missing unit test for ReadyReplicaStatus but want to make sure the implementation looks good first

liurupeng · 2024-04-17T23:23:36Z

pkg/metrics/metrics.go

+ }, []string{"leadername"},
+ )
+
+ replicaReadyStatusDuration = prometheus.NewHistogramVec(


add a comment here about what is the metrics record, like "the latency for a pod group to be ready after creation"

but overall, I start to think about whether it's necessary to have this metrics for now.

the help attribute of the object describes what it records, but I can add a comment for it as well

I don't think we need such a metric. The time it takes for the pod to become ready is not a characteristic of the lws controller.

liurupeng · 2024-04-17T23:25:04Z

pkg/metrics/metrics.go

+
+func ReplicaReadyStatus(readyPods []corev1.Pod, startTime metav1.Time) {
+ for _, pod := range readyPods {
+ readyTime := podutils.PodReadyConditionLastTransitionTime(pod).Time


when the leader pod is ready, its worker statefulset may not be ready right? the latency here calculates the leader pods' latency to be ready

we may need to pass the worker sts and leader pod as a pair and make sure that latency=worker_sts_ready-leader_pod_created, wdyt

The LeaderPod is only added into the list that is passed if the worker sts is ready, that is checked here

I guess I'm a bit confused with what the replica latency measures, I thought it was from when the lws object was created/edited to when the replica is ready. So is it from when the leader statefulset is created to when the worker sts is ready?

liurupeng · 2024-04-17T23:27:07Z

pkg/controllers/leaderworkerset_controller.go

@@ -569,3 +573,12 @@ func templateUpdated(sts *appsv1.StatefulSet, lws *leaderworkerset.LeaderWorkerS
 func replicasUpdated(sts *appsv1.StatefulSet, lws *leaderworkerset.LeaderWorkerSet) bool {
 return *sts.Spec.Replicas != *lws.Spec.Replicas
 }
+
+func getLastTransitionTime(conditionType string, lws *leaderworkerset.LeaderWorkerSet) metav1.Time {


maybe move this to a util file?

Was thinking about that as well, can create an lws utils file similar to the one that exists for pods, and move this function, templateUpdated and replicasUpdated to it in a different PR.

liurupeng · 2024-04-17T23:29:04Z

pkg/controllers/leaderworkerset_controller.go

@@ -389,6 +390,9 @@ func (r *LeaderWorkerSetReconciler) updateConditions(ctx context.Context, lws *l
 updateCondition := setCondition(lws, condition)
 // if condition changed, record events
 if updateCondition {
+ if updatedCount == int(*lws.Spec.Replicas) {
+ metrics.ReplicaReadyStatus(readyLeaderPods, getLastTransitionTime(string(leaderworkerset.LeaderWorkerSetProgressing), lws))


the start time here is when the lws just progress, there might be a possibility that the leader pod is not scheduled.

Asked a similar question in other comment: what is the replica latency actually measuring? I thought it was from lws creation to when the replica is ready

liurupeng · 2024-04-17T23:30:05Z

pkg/metrics/metrics.go

+ }, []string{"hash"},
+ )
+
+ recreateGroupTimes = prometheus.NewCounterVec(


this is not called anywhere else right?

Yeah, adding it to register it, but waiting on #106 to implement it.

ahg-g

I don't know if we really need to add our own metrics at this stage, I would focus on making sure that we are getting the metrics that controller-runtime publishes by default; see https://book.kubebuilder.io/reference/metrics-reference

Those metrics cover a lot of what we need already for monitoring the health of the operator, like how long a reconcile call takes, and I think the first PR should focus on that only and it should be included in the 0.2.0 release.

ahg-g · 2024-04-18T06:32:13Z

pkg/metrics/metrics.go

+ }, []string{"leadername"},
+ )
+
+ replicaReadyStatusDuration = prometheus.NewHistogramVec(


I don't think we need such a metric. The time it takes for the pod to become ready is not a characteristic of the lws controller.

kerthcet · 2024-04-18T09:06:24Z

+1 to add these metrics later since not part of the 0.2.0.

liurupeng · 2024-04-19T17:33:09Z

/hold
As discussed with Edwin, hold the PR for now, thanks @Edwinhr716

k8s-ci-robot · 2024-04-19T17:37:07Z

PR needs rebase.

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.

liurupeng · 2024-05-22T18:40:56Z

close for now, @Edwinhr716

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Apr 11, 2024

k8s-ci-robot requested review from kerthcet and liurupeng April 11, 2024 22:35

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

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 11, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 11, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 12, 2024

added registeration of rolling_update_duration and recreate_group_tim…

d7f8907

…es, and functionality of the latter

added ReplicaReadyStatus metric, added unit tests

c901648

Edwinhr716 force-pushed the add-metrics branch from 2300d6f to c901648 Compare April 16, 2024 22:55

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 16, 2024

fixed where the readystatus is reported

59a987a

Edwinhr716 changed the title ~~[WIP] Adding system metrics support~~ Adding system metrics support Apr 17, 2024

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 17, 2024

liurupeng requested a review from ahg-g April 17, 2024 22:59

liurupeng reviewed Apr 17, 2024

View reviewed changes

ahg-g reviewed Apr 18, 2024

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 19, 2024

liurupeng closed this May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding system metrics support #97

Adding system metrics support #97

Edwinhr716 commented Apr 11, 2024 •

edited

k8s-ci-robot commented Apr 11, 2024

k8s-ci-robot commented Apr 11, 2024

kerthcet commented Apr 12, 2024

ahg-g commented Apr 16, 2024

Edwinhr716 commented Apr 16, 2024 •

edited

Edwinhr716 commented Apr 17, 2024

liurupeng Apr 17, 2024

liurupeng Apr 17, 2024

Edwinhr716 Apr 17, 2024

ahg-g Apr 18, 2024

liurupeng Apr 17, 2024

liurupeng Apr 17, 2024

Edwinhr716 Apr 17, 2024

liurupeng Apr 17, 2024

Edwinhr716 Apr 17, 2024

liurupeng Apr 17, 2024

Edwinhr716 Apr 17, 2024

liurupeng Apr 17, 2024

Edwinhr716 Apr 17, 2024

ahg-g left a comment

ahg-g Apr 18, 2024

kerthcet commented Apr 18, 2024

liurupeng commented Apr 19, 2024

k8s-ci-robot commented Apr 19, 2024

liurupeng commented May 22, 2024

Adding system metrics support #97

Adding system metrics support #97

Conversation

Edwinhr716 commented Apr 11, 2024 • edited

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

Does this PR introduce a user-facing change?

k8s-ci-robot commented Apr 11, 2024

k8s-ci-robot commented Apr 11, 2024

kerthcet commented Apr 12, 2024

ahg-g commented Apr 16, 2024

Edwinhr716 commented Apr 16, 2024 • edited

Edwinhr716 commented Apr 17, 2024

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

ahg-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerthcet commented Apr 18, 2024

liurupeng commented Apr 19, 2024

k8s-ci-robot commented Apr 19, 2024

liurupeng commented May 22, 2024

Edwinhr716 commented Apr 11, 2024 •

edited

Edwinhr716 commented Apr 16, 2024 •

edited