feat: Add placement status metrics by britaniar · Pull Request #1077 · Azure/fleet

britaniar · 2025-03-12T22:52:06Z

Description of your changes

Fixes #

I have: added a metric to emit crp condition status' along with a test.

Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

zhiying-lin

Should we emit the last condition if the crp is not completed? since your metrics type is gauge. WE only need to know the current status.

pkg/controllers/clusterresourceplacement/controller.go

ryanzhang-oss · 2025-03-18T23:04:49Z

please see https://github.com/Azure/fleet/pull/1083/files#diff-fe7cf80f71bd1a1911f8703979980085428e3e1c81175394c3ea3f7e7dc350e6 to add tests

pkg/controllers/clusterresourceplacement/controller.go

pkg/utils/controller/metrics/metrics.go

pkg/controllers/clusterresourceplacement/controller.go

pkg/controllers/clusterresourceplacement/controller_integration_test.go

pkg/controllers/clusterresourceplacement/controller.go

pkg/controllers/clusterresourceplacement/controller_integration_test.go

pkg/controllers/clusterresourceplacement/controller_test.go

kaito-pr-agent · 2025-04-03T16:26:45Z

Title

(Describe updated until commit `d3e408d`)

feat: Emit detailed metrics for workload placements

Description

Added new metrics to track the completion status and last timestamp of workload placements.
Updated test cases to validate the newly added metrics.
Refactored metric emission logic into separate functions for better readability and maintainability.
Added support for emitting metrics for different generations of CRP specs.
Enhanced test coverage to include scenarios with different CRP states and conditions.

Changes walkthrough 📝

Relevant files

Enhancement

controller.go `Add metric emission and deletion logic` pkg/controllers/clusterresourceplacement/controller.go Added new functions `emitPlacementStatusMetric` and `checkPlacementCompleteMetric` Updated metric deletion logic in `handleDelete` and `handleUpdate` Refactored metric emission in `handleUpdate`	+34/-3
metrics.go `Add new Prometheus metrics` pkg/utils/controller/metrics/metrics.go Added new Prometheus metrics `FleetPlacementCompleteLastTimeStampSeconds` and `FleetPlacementStatusLastTimeStampSeconds` Updated metric registration in `init`	+13/-5
metrics.go `Add utility package for metrics comparison` test/utils/metrics/metrics.go Created new package `metrics` with utility functions for comparing Prometheus metrics	+30/-0

Tests

controller_integration_test.go `Add integration tests for new metrics` pkg/controllers/clusterresourceplacement/controller_integration_test.go Added new test cases for different CRP states and conditions Updated existing test cases to validate new metrics Created helper functions `createAvailableClusterResourceBinding`, `updateClusterResourceBindingWithReportDiff`, and `checkPlacementStatusMetric`	+1310/-8

Need help?
Type /help how to ... in the comments thread for any questions about PR-Agent usage.
Check out the documentation for more information.

kaito-pr-agent · 2025-04-03T16:26:57Z

PR Reviewer Guide 🔍

(Review updated until commit `d3e408d`)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 500 - Partially compliant Compliant requirements: Emit metrics for CRP status changes Ensure metrics are emitted correctly based on CRP conditions Non-compliant requirements: None Requires further human verification: None
Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Potential Metric Emission Issue The code adds new metrics but lacks proper initialization and registration. Ensure these metrics are properly registered in the init function. klog.ErrorS(controller.NewUnexpectedBehaviorError(err), "We have encountered a fatal error that can't be retried, requeue after a day") return ctrl.Result{}, nil // ignore this unexpected error } startTime := time.Now() klog.V(2).InfoS("ClusterResourcePlacement reconciliation starts", "clusterResourcePlacement", name) defer func() { latency := time.Since(startTime).Milliseconds() klog.V(2).InfoS("ClusterResourcePlacement reconciliation ends", "clusterResourcePlacement", name, "latency", latency) }() crp := fleetv1beta1.ClusterResourcePlacement{} if err := r.Client.Get(ctx, types.NamespacedName{Name: name}, &crp); err != nil { if apierrors.IsNotFound(err) { klog.V(4).InfoS("Ignoring NotFound clusterResourcePlacement", "clusterResourcePlacement", name) return ctrl.Result{}, nil } klog.ErrorS(err, "Failed to get clusterResourcePlacement", "clusterResourcePlacement", name) return ctrl.Result{}, controller.NewAPIServerError(true, err) Missing Metric Initialization The new metrics are declared but not initialized. Ensure they are properly initialized in the init function. // of active workers per controller. FleetActiveWorkers = prometheus.NewGaugeVec(prometheus.GaugeOpts{ Name: "fleet_workload_active_workers", Help: "Number of currently used workers per controller", }, []string{"controller"}) // FleetPlacementCompleteLastTimeStampSeconds is a prometheus metric which keeps track if the placement is complete. FleetPlacementCompleteLastTimeStampSeconds = prometheus.NewGaugeVec(prometheus.GaugeOpts{ Name: "fleet_workload_placement_complete_last_timestamp_seconds", Help: "Timestamp in seconds of the current completion status of crp. The 'isCompleted' label indicates whether CRP completion is true or false.", }, []string{"name", "isCompleted"}) // FleetPlacementStatusLastTimeStampSeconds is a prometheus metric which keeps track of the last placement status. FleetPlacementStatusLastTimeStampSeconds = prometheus.NewGaugeVec(prometheus.GaugeOpts{ Name: "fleet_workload_placement_status_last_timestamp_seconds", Help: "Timestamp in seconds of the last current placement status condition of crp.", }, []string{"name", "generation", "conditionType", "status"})

kaito-pr-agent · 2025-04-03T17:55:30Z

Persistent review updated to latest commit 8a89ce8

kaito-pr-agent · 2025-04-03T18:27:24Z

Persistent review updated to latest commit 0200684

kaito-pr-agent · 2025-04-03T18:36:18Z

Persistent review updated to latest commit c6a8490

pkg/controllers/clusterresourceplacement/controller.go

pkg/controllers/clusterresourceplacement/controller_integration_test.go

kaito-pr-agent · 2025-04-04T16:28:04Z

Persistent review updated to latest commit 222867e

jwtty

LGTM

pkg/controllers/clusterresourceplacement/controller.go

pkg/utils/controller/metrics/metrics.go

pkg/controllers/clusterresourceplacement/controller.go

kaito-pr-agent · 2025-04-09T02:33:28Z

Persistent review updated to latest commit c74a770

pkg/controllers/clusterresourceplacement/controller_integration_test.go

kaito-pr-agent · 2025-04-09T21:08:52Z

Persistent review updated to latest commit 4ed96c6

kaito-pr-agent · 2025-04-09T21:23:45Z

Persistent review updated to latest commit d446d00

pkg/controllers/clusterresourceplacement/controller.go

pkg/controllers/clusterresourceplacement/controller_integration_test.go

kaito-pr-agent · 2025-04-10T19:20:47Z

Persistent review updated to latest commit fef4bb6

kaito-pr-agent · 2025-04-10T22:52:43Z

Persistent review updated to latest commit 07ffce0

kaito-pr-agent · 2025-04-10T23:34:37Z

Persistent review updated to latest commit 1cec5f4

kaito-pr-agent · 2025-04-10T23:43:48Z

Persistent review updated to latest commit 9bbdf16

pkg/controllers/clusterresourceplacement/controller_integration_test.go

pkg/utils/test_util.go

zhiying-lin · 2025-04-11T06:59:38Z

pkg/controllers/clusterresourceplacement/controller.go

+			metrics.FleetPlacementStatusLastTimeStampSeconds.WithLabelValues(crp.Name, strconv.FormatInt(crp.Generation, 10), string(condType.ClusterResourcePlacementConditionType()), status).SetToCurrentTime()
+			return
+		}
+	}


There are two options:

two metrics, like the current one, completedMetrics + incompletedPlacementStatus metrics

one metrics, PlacementStatus metrics to record the last condition of CPR. isCompleted = ReportDiff == true || available condition true

Personally i prefer option 2

I personally prefer option 1. The completed metrics makes it a lot easier to query whether the CRP is completed or not for dashboards. If in the future, not sure if we will, we add more final conditions (like Available and ReportDiff) then we will have to continue changing the alert to include these cases. The second metric was just to provide more information in the alert, while the first helps with dashboard and easier to filter out through the logs.

The current status metric never has a "good" status. How are we going to use this metric? I feel this metric is not independent: we always need to use it along with the completed metric, we always need to verify if crp is still incomplete first and then refer to this metric to see why it's still incomplete.

agree with Wantong, they seem coupled, not sure how to join them but with different timestamp.

Talked to Ryan. We will couple them.

zhiying-lin · 2025-04-11T07:02:02Z

pkg/controllers/clusterresourceplacement/controller_integration_test.go

+			gotCRP = retrieveAndValidateClusterResourcePlacement(testCRPName, wantCRP)
+
+			By("Ensure placement complete metric was emitted with isCompleted True")
+			checkPlacementCompleteMetric(customRegistry, testCRPName, true, 2)


we also need to validate the crp status metrics

For a complete crp, the test is very flaky on figuring out what statuses will be emitted depending on how fast the conditions update since we don't have all the controllers working properly in the integration test. Originally, we wanted the placementstatus to record the last status for incomplete crp to see where they stopped to include in the alert.

i see, could you please add a comment there to clarify why we don't validate the status metrics here.

pkg/controllers/clusterresourceplacement/controller_integration_test.go

pkg/utils/test_util.go

kaito-pr-agent · 2025-04-11T22:22:13Z

Persistent review updated to latest commit 43250ec

kaito-pr-agent · 2025-04-11T22:38:50Z

Persistent review updated to latest commit d3e408d

jwtty · 2025-04-14T01:07:10Z

test/utils/metrics/metrics.go

+			if a.Name == nil || b.Name == nil {
+				return a.Name == nil
+			}
+			return *a.Name < *b.Name // Sort by label


We could use a.GetName() < b.GetName() to avoid nil comparison. I did in #1107

is this function a dup?

Yea, I just don't know whether this PR or #1107 will merge first. The latter one can always reuse this util.

jwtty · 2025-04-14T01:10:39Z

pkg/controllers/clusterresourceplacement/controller.go

+			metrics.FleetPlacementStatusLastTimeStampSeconds.WithLabelValues(crp.Name, strconv.FormatInt(crp.Generation, 10), string(condType.ClusterResourcePlacementConditionType()), status).SetToCurrentTime()
+			return
+		}
+	}


The current status metric never has a "good" status. How are we going to use this metric? I feel this metric is not independent: we always need to use it along with the completed metric, we always need to verify if crp is still incomplete first and then refer to this metric to see why it's still incomplete.

jwtty · 2025-04-14T01:51:56Z

pkg/controllers/clusterresourceplacement/controller.go

 			klog.ErrorS(updateErr, "Failed to update the status", "clusterResourcePlacement", crpKObj)
 			return ctrl.Result{}, controller.NewUpdateIgnoreConflictError(updateErr)
 		}
+		metrics.FleetPlacementStatusLastTimeStampSeconds.DeletePartialMatch(prometheus.Labels{"name": crp.Name})


Here you can call emitPlacementStatusMetric too.

jwtty · 2025-04-14T01:56:43Z

pkg/controllers/clusterresourceplacement/controller.go

 			r.Recorder.Event(crp, corev1.EventTypeNormal, i.EventReasonForTrue(), i.EventMessageForTrue())
 		}
 	}
+	emitPlacementStatusMetric(crp)


I think it's better to call this metrics emit function in a defer function because we have so many returns in handleUpdate. We may easily miss cases if we call this function ad-hoc.

jwtty · 2025-04-14T02:02:09Z

pkg/controllers/clusterresourceplacement/controller_integration_test.go

 			retrieveAndValidateClusterResourcePlacement(testCRPName, wantCRP)
+
+			By("Ensure placement complete metric was emitted with isCompleted False")
+			wantCompleteMetrics := []*prometheusclientmodel.Metric{


This incomplete metric can be refactored into a function. There's code duplication.

jwtty · 2025-04-14T02:10:05Z

pkg/controllers/clusterresourceplacement/controller_integration_test.go

+			}
+			Expect(k8sClient.Status().Update(ctx, gotPolicySnapshot)).Should(Succeed(), "Failed to update the policy snapshot status")
+
+			By("By creating clusterResourceBinding on member-1")


Suggested change

By("By creating clusterResourceBinding on member-1")

By("Create clusterResourceBinding on member-1")

I actually prefer using "Creating", but I see none of the other code use "ing".

jwtty · 2025-04-14T02:10:22Z

pkg/controllers/clusterresourceplacement/controller_integration_test.go

+			By("By creating clusterResourceBinding on member-1")
+			member1Binding = createOverriddenClusterResourceBinding(member1Name, gotPolicySnapshot, gotResourceSnapshot)
+
+			By("By validating the CRP status")


Suggested change

By("By validating the CRP status")

By("Validate the CRP status")

jwtty · 2025-04-14T02:17:44Z

pkg/controllers/clusterresourceplacement/controller_integration_test.go

+			}
+			checkPlacementStatusMetric(customRegistry, wantStatusMetrics)
+
+			By("By creating a synchronized clusterResourceBinding on member-2")


Suggested change

By("By creating a synchronized clusterResourceBinding on member-2")

By("Create a synchronized clusterResourceBinding on member-2")

jwtty · 2025-04-14T02:21:58Z

pkg/controllers/clusterresourceplacement/controller_integration_test.go

+			Expect(k8sClient.Update(ctx, crp)).Should(Succeed(), "Failed to update crp")
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: testCRPName}, crp)).Should(BeNil(), "Get() clusterResourcePlacement mismatch")
+
+			By("By validating the CRP status with new spec")


Suggested change

By("By validating the CRP status with new spec")

By("Validate the CRP status with new spec")

jwtty · 2025-04-14T02:40:39Z

pkg/controllers/clusterresourceplacement/controller_integration_test.go

+				Spec: crp.Spec,
+				Status: placementv1beta1.ClusterResourcePlacementStatus{
+					ObservedResourceIndex: "0",
+					Conditions: []metav1.Condition{


I think this piece of code can also be refactored.

ryanzhang-oss · 2025-04-14T21:57:13Z

test/utils/metrics/metrics.go

+			if a.Name == nil || b.Name == nil {
+				return a.Name == nil
+			}
+			return *a.Name < *b.Name // Sort by label


is this function a dup?

ryanzhang-oss · 2025-04-14T22:00:12Z

pkg/controllers/clusterresourceplacement/controller.go

+	metrics.FleetPlacementCompleteLastTimeStampSeconds.DeletePartialMatch(prometheus.Labels{"name": crp.Name})
+	metrics.FleetPlacementStatusLastTimeStampSeconds.DeletePartialMatch(prometheus.Labels{"name": crp.Name})


I wonder why move the delete earlier instead of replacing them at line 105?

I asked to move these earlier. If the pod stops when finalizer is removed but before metrics are removed, they would be left never deleted.

michaelawyu · 2025-04-15T15:42:19Z

Hi Britania! I am closing this PR as part of the CNCF repo migration process; please consider moving (re-creating) this PR in the new repo once the sync PR is merged. If there's any question/concern, please let me know. Thanks 🙏

britaniar force-pushed the addEdgeMetrics branch from f0ded74 to fc22495 Compare March 12, 2025 23:02

britaniar marked this pull request as ready for review March 12, 2025 23:28

britaniar force-pushed the addEdgeMetrics branch 4 times, most recently from c25f067 to 42093bd Compare March 13, 2025 21:03

zhiying-lin reviewed Mar 14, 2025

View reviewed changes

pkg/controllers/clusterresourceplacement/controller.go Outdated Show resolved Hide resolved

pkg/controllers/clusterresourceplacement/controller.go Outdated Show resolved Hide resolved

britaniar force-pushed the addEdgeMetrics branch 3 times, most recently from d7683e2 to 6f85e45 Compare March 29, 2025 00:32

britaniar commented Mar 29, 2025

View reviewed changes

pkg/controllers/clusterresourceplacement/controller.go Outdated Show resolved Hide resolved

jwtty reviewed Apr 2, 2025

View reviewed changes

britaniar added 2 commits April 3, 2025 09:18

add placement status metrics

623f26d

revert controller_test.go

8486603

britaniar force-pushed the addEdgeMetrics branch from 6f85e45 to 8486603 Compare April 3, 2025 16:26

address comments

8a89ce8

try to reduce flakiness

0200684

only check for complete metric for last scenario

c6a8490

jwtty reviewed Apr 4, 2025

View reviewed changes

britaniar added 3 commits April 4, 2025 09:15

address comments

ed646ef

update test cases

a6026c9

revert changes

222867e

jwtty previously approved these changes Apr 7, 2025

View reviewed changes

zhiying-lin reviewed Apr 8, 2025

View reviewed changes

jwtty reviewed Apr 8, 2025

View reviewed changes

pkg/controllers/clusterresourceplacement/controller.go Show resolved Hide resolved

add report diff test and address comments

c74a770

britaniar dismissed jwtty’s stale review via c74a770 April 9, 2025 02:32

zhiying-lin reviewed Apr 9, 2025

View reviewed changes

use cmp.Diff to compare metric and expectedMetric labels

4ed96c6

fix lint errors

d446d00

zhiying-lin reviewed Apr 10, 2025

View reviewed changes

update check function & tests

fef4bb6

move metric comparison to common test utils

07ffce0

fix test

1cec5f4

fix test

9bbdf16

zhiying-lin reviewed Apr 11, 2025

View reviewed changes

zhiying-lin mentioned this pull request Apr 11, 2025

feat: add updaterun metrics and update Progressing condition #1107

Closed

1 task

address comments

d3e408d

britaniar force-pushed the addEdgeMetrics branch from 43250ec to d3e408d Compare April 11, 2025 22:38

jwtty reviewed Apr 14, 2025

View reviewed changes

ryanzhang-oss reviewed Apr 14, 2025

View reviewed changes

michaelawyu closed this Apr 15, 2025

	By("By creating clusterResourceBinding on member-1")
	By("Create clusterResourceBinding on member-1")

	By("By validating the CRP status")
	By("Validate the CRP status")

	By("By creating a synchronized clusterResourceBinding on member-2")
	By("Create a synchronized clusterResourceBinding on member-2")

	By("By validating the CRP status with new spec")
	By("Validate the CRP status with new spec")

		metrics.FleetPlacementCompleteLastTimeStampSeconds.DeletePartialMatch(prometheus.Labels{"name": crp.Name})
		metrics.FleetPlacementStatusLastTimeStampSeconds.DeletePartialMatch(prometheus.Labels{"name": crp.Name})

Comments

Conversation

britaniar commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of your changes

How has this code been tested

Special notes for your reviewer

Uh oh!

zhiying-lin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanzhang-oss commented Mar 18, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaito-pr-agent bot commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Title

(Describe updated until commit d3e408d)

Description

Changes walkthrough 📝

Uh oh!

kaito-pr-agent bot commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit d3e408d)

Uh oh!

kaito-pr-agent bot commented Apr 3, 2025

Uh oh!

kaito-pr-agent bot commented Apr 3, 2025

Uh oh!

kaito-pr-agent bot commented Apr 3, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaito-pr-agent bot commented Apr 4, 2025

Uh oh!

jwtty left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaito-pr-agent bot commented Apr 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaito-pr-agent bot commented Apr 9, 2025

Uh oh!

kaito-pr-agent bot commented Apr 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaito-pr-agent bot commented Apr 10, 2025

Uh oh!

kaito-pr-agent bot commented Apr 10, 2025

Uh oh!

britaniar commented Mar 12, 2025 •

edited

Loading

kaito-pr-agent bot commented Apr 3, 2025 •

edited

Loading

(Describe updated until commit `d3e408d`)

kaito-pr-agent bot commented Apr 3, 2025 •

edited

Loading

(Review updated until commit `d3e408d`)

zhiying-lin Apr 11, 2025 •

edited

Loading

britaniar Apr 11, 2025 •

edited

Loading