Merge pull request #456 from HeavyWombat/refactor/send-metrics

Refactor metrics observe calls to report metrics as soon as possible

pkg/controller/buildrun/buildrun_controller.go

@@ -337,6 +337,16 @@ func (r *ReconcileBuildRun) Reconcile(request reconcile.Request) (reconcile.Resu
 				// risk is that when the controller is now restarted before the field is set, another TaskRun will be created
 				ctxlog.Error(ctx, err, "Failed to update BuildRun status is ignored", namespace, request.Namespace, name, request.Name)
 			}
+
+			// Increase BuildRun count in metrics
+			buildmetrics.BuildRunCountInc(buildRun.Status.BuildSpec.StrategyRef.Name)
+
+			// Report buildrun ramp-up duration (time between buildrun creation and taskrun creation)
+			buildmetrics.BuildRunRampUpDurationObserve(
+				buildRun.Status.BuildSpec.StrategyRef.Name,
+				buildRun.Namespace,
+				generatedTaskRun.CreationTimestamp.Time.Sub(buildRun.CreationTimestamp.Time),
+			)
 		} else {
 			return reconcile.Result{}, err
 		}
@@ -383,35 +393,29 @@ func (r *ReconcileBuildRun) Reconcile(request reconcile.Request) (reconcile.Resu
 			}
 
 			buildRun.Status.LatestTaskRunRef = &lastTaskRun.Name
-			buildRun.Status.StartTime = lastTaskRun.Status.StartTime
+
+			if buildRun.Status.StartTime == nil && lastTaskRun.Status.StartTime != nil {
+				buildRun.Status.StartTime = lastTaskRun.Status.StartTime
+
+				// Report the buildrun established duration (time between the creation of the buildrun and the start of the buildrun)
+				buildmetrics.BuildRunEstablishObserve(
+					buildRun.Status.BuildSpec.StrategyRef.Name,
+					buildRun.Namespace,
+					buildRun.Status.StartTime.Time.Sub(buildRun.CreationTimestamp.Time),
+				)
+			}
+
 			if lastTaskRun.Status.CompletionTime != nil && buildRun.Status.CompletionTime == nil {
 				buildRun.Status.CompletionTime = lastTaskRun.Status.CompletionTime
 
 				if buildRun.Status.BuildSpec.StrategyRef != nil {
-					// Increase BuildRun count in metrics
-					buildmetrics.BuildRunCountInc(buildRun.Status.BuildSpec.StrategyRef.Name)
-
-					// buildrun established duration (time between the creation of the buildrun and the start of the buildrun)
-					buildmetrics.BuildRunEstablishObserve(
-						buildRun.Status.BuildSpec.StrategyRef.Name,
-						buildRun.Namespace,
-						buildRun.Status.StartTime.Time.Sub(buildRun.CreationTimestamp.Time),
-					)
-
 					// buildrun completion duration (total time between the creation of the buildrun and the buildrun completion)
 					buildmetrics.BuildRunCompletionObserve(
 						buildRun.Status.BuildSpec.StrategyRef.Name,
 						buildRun.Namespace,
 						buildRun.Status.CompletionTime.Time.Sub(buildRun.CreationTimestamp.Time),
 					)
 
-					// buildrun ramp-up duration (time between buildrun creation and taskrun creation)
-					buildmetrics.BuildRunRampUpDurationObserve(
-						buildRun.Status.BuildSpec.StrategyRef.Name,
-						buildRun.Namespace,
-						lastTaskRun.CreationTimestamp.Time.Sub(buildRun.CreationTimestamp.Time),
-					)
-
 					// Look for the pod created by the taskrun
 					var pod = &corev1.Pod{}
 					if err := r.client.Get(ctx, types.NamespacedName{Namespace: request.Namespace, Name: lastTaskRun.Status.PodName}, pod); err == nil {

pkg/controller/buildrun/buildrun_controller_test.go

@@ -211,7 +211,9 @@ var _ = Describe("Reconcile BuildRun", func() {
 			It("deletes a generated service account when the task run ends", func() {
 
 				// setup a buildrun to use a generated service account
+				buildSample = ctl.DefaultBuild(buildName, "foobar-strategy", build.ClusterBuildStrategyKind)
 				buildRunSample = ctl.BuildRunWithSAGenerate(buildRunSample.Name, buildName)
+				buildRunSample.Status.BuildSpec = &buildSample.Spec
 				buildRunSample.Labels = make(map[string]string)
 				buildRunSample.Labels[build.LabelBuild] = buildName
 

pkg/metrics/metrics_test.go

@@ -9,12 +9,11 @@ import (
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
-	io_prometheus_client "github.com/prometheus/client_model/go"
-
-	crmetrics "sigs.k8s.io/controller-runtime/pkg/metrics"
+	. "github.com/shipwright-io/build/pkg/metrics"
 
+	io_prometheus_client "github.com/prometheus/client_model/go"
 	"github.com/shipwright-io/build/pkg/config"
-	. "github.com/shipwright-io/build/pkg/metrics"
+	crmetrics "sigs.k8s.io/controller-runtime/pkg/metrics"
 )
 
 var _ = Describe("Custom Metrics", func() {
@@ -55,7 +54,7 @@ var _ = Describe("Custom Metrics", func() {
 				"build_builds_registered_total",
 			}
 
-			knownHistrogramMetrics = []string{
+			knownHistogramMetrics = []string{
 				"build_buildrun_establish_duration_seconds",
 				"build_buildrun_completion_duration_seconds",
 				"build_buildrun_rampup_duration_seconds",
@@ -64,17 +63,17 @@ var _ = Describe("Custom Metrics", func() {
 			}
 		)
 
-		// initialise the counter metrics result map with empty maps
+		// initialize the counter metrics result map with empty maps
 		for _, name := range knownCounterMetrics {
 			counterMetrics[name] = map[string]float64{}
 		}
 
-		// initialise the histrogram metrics result map with empty maps
-		for _, name := range knownHistrogramMetrics {
+		// initialize the histogram metrics result map with empty maps
+		for _, name := range knownHistogramMetrics {
 			histogramMetrics[name] = map[buildRunLabels]float64{}
 		}
 
-		// initialise prometheus (second init should be no-op)
+		// initialize prometheus (second init should be no-op)
 		config := config.NewDefaultConfig()
 		config.Prometheus.HistogramEnabledLabels = []string{"buildstrategy", "namespace"}
 		InitPrometheus(config)

test/catalog.go

@@ -480,6 +480,11 @@ func (c *Catalog) DefaultTaskRunWithStatus(trName string, buildRunName string, n
 					},
 				},
 			},
+			TaskRunStatusFields: v1beta1.TaskRunStatusFields{
+				StartTime: &metav1.Time{
+					Time: time.Now(),
+				},
+			},
 		},
 	}
 }
@@ -538,6 +543,11 @@ func (c *Catalog) DefaultTaskRunWithFalseStatus(trName string, buildRunName stri
 					},
 				},
 			},
+			TaskRunStatusFields: v1beta1.TaskRunStatusFields{
+				StartTime: &metav1.Time{
+					Time: time.Now(),
+				},
+			},
 		},
 	}
 }
@@ -622,6 +632,7 @@ func (c *Catalog) DefaultBuildWithFalseRegistered(buildName string, strategyName
 
 // DefaultBuildRun returns a minimal BuildRun object
 func (c *Catalog) DefaultBuildRun(buildRunName string, buildName string) *build.BuildRun {
+	var defaultBuild = c.DefaultBuild(buildName, "foobar-strategy", build.ClusterBuildStrategyKind)
 	return &build.BuildRun{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: buildRunName,
@@ -631,6 +642,9 @@ func (c *Catalog) DefaultBuildRun(buildRunName string, buildName string) *build.
 				Name: buildName,
 			},
 		},
+		Status: build.BuildRunStatus{
+			BuildSpec: &defaultBuild.Spec,
+		},
 	}
 }
 
@@ -704,7 +718,6 @@ func (c *Catalog) BuildRunWithExistingOwnerReferences(buildRunName string, build
 // BuildRunWithFakeNamespace returns a BuildRun object with
 // a namespace that does not exist
 func (c *Catalog) BuildRunWithFakeNamespace(buildRunName string, buildName string) *build.BuildRun {
-
 	return &build.BuildRun{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      buildRunName,