runtime/metrics: Delete metrics on object delete

Delete the object metrics when the object is deleted. This ensures
that stale metrics about a deleted object is no longer exported.

As a result, the `ConditionDelete` is no longer needed. Another reason
to not have `ConditionDelete` is that a condition can only be one of
True, False or Unknown.

This introduces new delete methods in the low level metrics Recorder. In
the high level controller metrics, the existing Record*() methods are
updated to check if the given object is deleted, and call record or
delete based on that. This helps make this change transparent. The user
of this API don't need to update any code for the metrics to get deleted
when the associated object is deleted.

Signed-off-by: Sunny <[email protected]>

runtime/controller/metrics.go

@@ -75,6 +75,10 @@ func (m Metrics) RecordDuration(ctx context.Context, obj conditions.Getter, star
 			logr.FromContextOrDiscard(ctx).Error(err, "unable to get object reference to record duration")
 			return
 		}
+		if !obj.GetDeletionTimestamp().IsZero() {
+			m.MetricsRecorder.DeleteDuration(*ref)
+			return
+		}
 		m.MetricsRecorder.RecordDuration(*ref, startTime)
 	}
 }
@@ -87,22 +91,38 @@ func (m Metrics) RecordSuspend(ctx context.Context, obj conditions.Getter, suspe
 			logr.FromContextOrDiscard(ctx).Error(err, "unable to get object reference to record suspend")
 			return
 		}
+		if !obj.GetDeletionTimestamp().IsZero() {
+			m.MetricsRecorder.DeleteSuspend(*ref)
+			return
+		}
 		m.MetricsRecorder.RecordSuspend(*ref, suspend)
 	}
 }
 
 // RecordReadiness records the meta.ReadyCondition status for the given obj.
 func (m Metrics) RecordReadiness(ctx context.Context, obj conditions.Getter) {
+	if !obj.GetDeletionTimestamp().IsZero() {
+		m.DeleteCondition(ctx, obj, meta.ReadyCondition)
+		return
+	}
 	m.RecordCondition(ctx, obj, meta.ReadyCondition)
 }
 
 // RecordReconciling records the meta.ReconcilingCondition status for the given obj.
 func (m Metrics) RecordReconciling(ctx context.Context, obj conditions.Getter) {
+	if !obj.GetDeletionTimestamp().IsZero() {
+		m.DeleteCondition(ctx, obj, meta.ReconcilingCondition)
+		return
+	}
 	m.RecordCondition(ctx, obj, meta.ReconcilingCondition)
 }
 
 // RecordStalled records the meta.StalledCondition status for the given obj.
 func (m Metrics) RecordStalled(ctx context.Context, obj conditions.Getter) {
+	if !obj.GetDeletionTimestamp().IsZero() {
+		m.DeleteCondition(ctx, obj, meta.StalledCondition)
+		return
+	}
 	m.RecordCondition(ctx, obj, meta.StalledCondition)
 }
 
@@ -120,5 +140,19 @@ func (m Metrics) RecordCondition(ctx context.Context, obj conditions.Getter, con
 	if rc == nil {
 		rc = conditions.UnknownCondition(conditionType, "", "")
 	}
-	m.MetricsRecorder.RecordCondition(*ref, *rc, !obj.GetDeletionTimestamp().IsZero())
+	m.MetricsRecorder.RecordCondition(*ref, *rc)
+}
+
+// DeleteCondition deletes the condition metrics of the given conditionType for
+// the given object.
+func (m Metrics) DeleteCondition(ctx context.Context, obj conditions.Getter, conditionType string) {
+	if m.MetricsRecorder == nil {
+		return
+	}
+	ref, err := reference.GetReference(m.Scheme, obj)
+	if err != nil {
+		logr.FromContextOrDiscard(ctx).Error(err, "unable to get object reference to delete condition metric")
+		return
+	}
+	m.MetricsRecorder.DeleteCondition(*ref, conditionType)
 }

runtime/metrics/recorder.go

@@ -25,10 +25,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-const (
-	ConditionDeleted = "Deleted"
-)
-
 // Recorder is a struct for recording GitOps Toolkit metrics for a controller.
 //
 // Use NewRecorder to initialise it with properly configured metric names.
@@ -77,19 +73,20 @@ func (r *Recorder) Collectors() []prometheus.Collector {
 }
 
 // RecordCondition records the condition as given for the ref.
-func (r *Recorder) RecordCondition(ref corev1.ObjectReference, condition metav1.Condition, deleted bool) {
-	for _, status := range []string{string(metav1.ConditionTrue), string(metav1.ConditionFalse), string(metav1.ConditionUnknown), ConditionDeleted} {
+func (r *Recorder) RecordCondition(ref corev1.ObjectReference, condition metav1.Condition) {
+	for _, status := range []metav1.ConditionStatus{metav1.ConditionTrue, metav1.ConditionFalse, metav1.ConditionUnknown} {
 		var value float64
-		if deleted {
-			if status == ConditionDeleted {
-				value = 1
-			}
-		} else {
-			if status == string(condition.Status) {
-				value = 1
-			}
+		if status == condition.Status {
+			value = 1
 		}
-		r.conditionGauge.WithLabelValues(ref.Kind, ref.Name, ref.Namespace, condition.Type, status).Set(value)
+		r.conditionGauge.WithLabelValues(ref.Kind, ref.Name, ref.Namespace, condition.Type, string(status)).Set(value)
+	}
+}
+
+// DeleteCondition deletes the condition metrics for the ref.
+func (r *Recorder) DeleteCondition(ref corev1.ObjectReference, conditionType string) {
+	for _, status := range []metav1.ConditionStatus{metav1.ConditionTrue, metav1.ConditionFalse, metav1.ConditionUnknown} {
+		r.conditionGauge.DeleteLabelValues(ref.Kind, ref.Name, ref.Namespace, conditionType, string(status))
 	}
 }
 
@@ -102,7 +99,17 @@ func (r *Recorder) RecordSuspend(ref corev1.ObjectReference, suspend bool) {
 	r.suspendGauge.WithLabelValues(ref.Kind, ref.Name, ref.Namespace).Set(value)
 }
 
+// DeleteSuspend deletes the suspend metric for the ref.
+func (r *Recorder) DeleteSuspend(ref corev1.ObjectReference) {
+	r.suspendGauge.DeleteLabelValues(ref.Kind, ref.Name, ref.Namespace)
+}
+
 // RecordDuration records the duration since start for the given ref.
 func (r *Recorder) RecordDuration(ref corev1.ObjectReference, start time.Time) {
 	r.durationHistogram.WithLabelValues(ref.Kind, ref.Name, ref.Namespace).Observe(time.Since(start).Seconds())
 }
+
+// DeleteDuration deletes the duration metric for the ref.
+func (r *Recorder) DeleteDuration(ref corev1.ObjectReference) {
+	r.durationHistogram.DeleteLabelValues(ref.Kind, ref.Name, ref.Namespace)
+}

runtime/metrics/recorder_test.go

@@ -44,15 +44,13 @@ func TestRecorder_RecordCondition(t *testing.T) {
 		Status: metav1.ConditionTrue,
 	}
 
-	rec.RecordCondition(ref, cond, false)
+	rec.RecordCondition(ref, cond)
 
 	metricFamilies, err := reg.Gather()
-	if err != nil {
-		require.NoError(t, err)
-	}
+	require.NoError(t, err)
 
 	require.Equal(t, len(metricFamilies), 1)
-	require.Equal(t, len(metricFamilies[0].Metric), 4)
+	require.Equal(t, len(metricFamilies[0].Metric), 3)
 
 	var conditionTrueValue float64
 	for _, m := range metricFamilies[0].Metric {
@@ -69,6 +67,13 @@ func TestRecorder_RecordCondition(t *testing.T) {
 	}
 
 	require.Equal(t, conditionTrueValue, float64(1))
+
+	// Delete metrics.
+	rec.DeleteCondition(ref, cond.Type)
+
+	metricFamilies, err = reg.Gather()
+	require.NoError(t, err)
+	require.Equal(t, len(metricFamilies), 0)
 }
 
 func TestRecorder_RecordDuration(t *testing.T) {
@@ -86,9 +91,7 @@ func TestRecorder_RecordDuration(t *testing.T) {
 	rec.RecordDuration(ref, reconcileStart)
 
 	metricFamilies, err := reg.Gather()
-	if err != nil {
-		require.NoError(t, err)
-	}
+	require.NoError(t, err)
 
 	require.Equal(t, len(metricFamilies), 1)
 	require.Equal(t, len(metricFamilies[0].Metric), 1)
@@ -110,4 +113,53 @@ func TestRecorder_RecordDuration(t *testing.T) {
 			t.Errorf("expected namespace label to be %s, got %s", ref.Namespace, *pair.Value)
 		}
 	}
+
+	// Delete metrics.
+	rec.DeleteDuration(ref)
+
+	metricFamilies, err = reg.Gather()
+	require.NoError(t, err)
+	require.Equal(t, len(metricFamilies), 0)
+}
+
+func TestRecorder_RecordSuspend(t *testing.T) {
+	rec := NewRecorder()
+	reg := prometheus.NewRegistry()
+	reg.MustRegister(rec.suspendGauge)
+
+	ref := corev1.ObjectReference{
+		Kind:      "GitRepository",
+		Namespace: "default",
+		Name:      "test",
+	}
+
+	rec.RecordSuspend(ref, true)
+
+	metricFamilies, err := reg.Gather()
+	require.NoError(t, err)
+
+	require.Equal(t, len(metricFamilies), 1)
+	require.Equal(t, len(metricFamilies[0].Metric), 1)
+
+	value := *metricFamilies[0].Metric[0].GetGauge().Value
+	require.EqualValues(t, value, 1, "expected gauge value")
+
+	for _, pair := range metricFamilies[0].Metric[0].GetLabel() {
+		if *pair.Name == "kind" {
+			require.Equal(t, *pair.Value, ref.Kind, "unexpected kind")
+		}
+		if *pair.Name == "name" {
+			require.Equal(t, *pair.Value, ref.Name, "unexpected name")
+		}
+		if *pair.Name == "namespace" {
+			require.Equal(t, *pair.Value, ref.Namespace, "unexpected namespace")
+		}
+	}
+
+	// Delete metrics.
+	rec.DeleteSuspend(ref)
+
+	metricFamilies, err = reg.Gather()
+	require.NoError(t, err)
+	require.Equal(t, len(metricFamilies), 0)
 }