Merge pull request #143 from adambkaplan/fix-build-metrics-count

Bug 1891362: Remove the openshift_build_result_total metric
openshift · Nov 12, 2020 · 77e8248 · 77e8248
2 parents e1af3a5 + 1b1d0af
commit 77e8248
Show file tree

Hide file tree

Showing 4 changed files with 2 additions and 222 deletions.
diff --git a/pkg/build/buildutil/util.go b/pkg/build/buildutil/util.go
@@ -68,10 +68,6 @@ func IsTerminalPhase(phase buildv1.BuildPhase) bool {
 	return true
 }
 
-func IsBuildSuccessful(phase buildv1.BuildPhase) bool {
-	return phase == buildv1.BuildPhaseComplete
-}
-
 // BuildConfigSelector returns a label Selector which can be used to find all
 // builds for a BuildConfig.
 func BuildConfigSelector(name string) labels.Selector {

diff --git a/pkg/build/controller/build/build_controller.go b/pkg/build/controller/build/build_controller.go
@@ -1554,9 +1554,7 @@ func (bc *BuildController) updateBuild(build *buildv1.Build, update *buildUpdate
 }
 
 func (bc *BuildController) handleBuildCompletion(build *buildv1.Build) {
-	metrics.IncrementBuildFinished(build)
 	bcName := sharedbuildutil.ConfigNameForBuild(build)
-	// empty bcName means that user submitted a raw Build object, with no associated BuildConfig
 	if len(strings.TrimSpace(bcName)) != 0 {
 		bc.enqueueBuildConfig(build.Namespace, bcName)
 		if err := common.HandleBuildPruning(bcName, build.Namespace, bc.buildLister, bc.buildConfigLister, bc.buildDeleter); err != nil {

diff --git a/pkg/build/metrics/prometheus/metrics.go b/pkg/build/metrics/prometheus/metrics.go
@@ -1,29 +1,25 @@
 package prometheus
 
 import (
-	"strings"
 	"sync"
 
 	"github.com/blang/semver"
 	"github.com/prometheus/client_golang/prometheus"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	kselector "k8s.io/apimachinery/pkg/labels"
-	"k8s.io/component-base/metrics"
 	"k8s.io/component-base/metrics/legacyregistry"
 	"k8s.io/klog/v2"
 
 	buildv1 "github.com/openshift/api/build/v1"
 	buildlister "github.com/openshift/client-go/build/listers/build/v1"
-	"github.com/openshift/openshift-controller-manager/pkg/build/buildutil"
 )
 
 const (
 	separator        = "_"
 	buildSubsystem   = "openshift_build"
 	buildCount       = "total"
 	buildCountQuery  = buildSubsystem + separator + buildCount
-	buildResultQuery = buildSubsystem + separator + "result" + separator + buildCount
 	activeBuild      = "active_time_seconds"
 	activeBuildQuery = buildSubsystem + separator + activeBuild
 )
@@ -41,11 +37,6 @@ var (
 		[]string{"namespace", "name", "phase", "reason", "strategy"},
 		nil,
 	)
-	buildResultCounter = metrics.NewCounterVec(&metrics.CounterOpts{
-		Name:           buildResultQuery,
-		Help:           "Counts the total number of finished builds across all namespaces by result and strategy",
-		StabilityLevel: metrics.ALPHA,
-	}, []string{"result", "strategy"})
 	bc             = buildCollector{}
 	cancelledPhase = string(buildv1.BuildPhaseCancelled)
 	completePhase  = string(buildv1.BuildPhaseComplete)
@@ -68,7 +59,7 @@ type buildCollector struct {
 func IntializeMetricsCollector(buildLister buildlister.BuildLister) {
 	if !bc.IsCreated() {
 		bc.lister = buildLister
-		legacyregistry.MustRegister(&bc, buildResultCounter)
+		legacyregistry.MustRegister(&bc)
 	}
 	klog.V(4).Info("build metrics registered with prometheus")
 }
@@ -133,23 +124,6 @@ func (bc *buildCollector) FQName() string {
 	return buildSubsystem
 }
 
-// IncrementBuildFinished increments the openshift_build_result_count metric based on the terminal
-// state of the build.
-func IncrementBuildFinished(build *buildv1.Build) {
-	if build == nil {
-		return
-	}
-	resultLabel := "failed"
-	if buildutil.IsBuildSuccessful(build.Status.Phase) {
-		resultLabel = "success"
-	}
-	buildResultCounter.
-		WithLabelValues(
-			resultLabel,
-			strings.ToLower(strategyType(build.Spec.Strategy)),
-		).Inc()
-}
-
 func addCountGauge(ch chan<- prometheus.Metric, desc *prometheus.Desc, phase, reason, strategy string, v float64) {
 	lv := []string{phase, reason, strategy}
 	ch <- prometheus.MustNewConstMetric(desc, prometheus.GaugeValue, v, lv...)

diff --git a/pkg/build/metrics/prometheus/metrics_test.go b/pkg/build/metrics/prometheus/metrics_test.go
@@ -40,7 +40,7 @@ func (f *fakeResponseWriter) WriteHeader(statusCode int) {
 	f.statusCode = statusCode
 }
 
-func TestLegacyMetrics(t *testing.T) {
+func TestMetrics(t *testing.T) {
 	// went per line vs. a block of expected test in case assumptions on ordering are subverted, as well as defer on
 	// getting newlines right
 	expectedResponse := []string{
@@ -142,191 +142,3 @@ func TestLegacyMetrics(t *testing.T) {
 		}
 	}
 }
-
-func TestBuildFinsishedCounter(t *testing.T) {
-	builds := []buildv1.Build{
-		{
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: "test-1",
-				Name:      "build-raw-docker-1",
-			},
-			Spec: buildv1.BuildSpec{
-				CommonSpec: buildv1.CommonSpec{
-					Strategy: buildv1.BuildStrategy{
-						Type:           buildv1.DockerBuildStrategyType,
-						DockerStrategy: &buildv1.DockerBuildStrategy{},
-					},
-				},
-			},
-			Status: buildv1.BuildStatus{
-				Phase: buildv1.BuildPhaseComplete,
-			},
-		},
-		{
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: "test-1",
-				Name:      "build-docker-1",
-				Annotations: map[string]string{
-					buildv1.BuildConfigAnnotation: "build-docker",
-				},
-			},
-			Spec: buildv1.BuildSpec{
-				CommonSpec: buildv1.CommonSpec{
-					Strategy: buildv1.BuildStrategy{
-						Type:           buildv1.DockerBuildStrategyType,
-						DockerStrategy: &buildv1.DockerBuildStrategy{},
-					},
-				},
-			},
-			Status: buildv1.BuildStatus{
-				Phase: buildv1.BuildPhaseComplete,
-			},
-		},
-		{
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: "test-1",
-				Name:      "build-docker-2",
-				Annotations: map[string]string{
-					buildv1.BuildConfigAnnotation: "build-docker",
-				},
-			},
-			Spec: buildv1.BuildSpec{
-				CommonSpec: buildv1.CommonSpec{
-					Strategy: buildv1.BuildStrategy{
-						Type:           buildv1.DockerBuildStrategyType,
-						DockerStrategy: &buildv1.DockerBuildStrategy{},
-					},
-				},
-			},
-			Status: buildv1.BuildStatus{
-				Phase: buildv1.BuildPhaseComplete,
-			},
-		},
-		{
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: "test-1",
-				Name:      "build-docker-3",
-				Annotations: map[string]string{
-					buildv1.BuildConfigAnnotation: "build-docker",
-				},
-			},
-			Spec: buildv1.BuildSpec{
-				CommonSpec: buildv1.CommonSpec{
-					Strategy: buildv1.BuildStrategy{
-						Type:           buildv1.DockerBuildStrategyType,
-						DockerStrategy: &buildv1.DockerBuildStrategy{},
-					},
-				},
-			},
-			Status: buildv1.BuildStatus{
-				Phase: buildv1.BuildPhaseFailed,
-			},
-		},
-		{
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: "test-1",
-				Name:      "build-docker-4",
-				Annotations: map[string]string{
-					buildv1.BuildConfigAnnotation: "build-docker",
-				},
-			},
-			Spec: buildv1.BuildSpec{
-				CommonSpec: buildv1.CommonSpec{
-					Strategy: buildv1.BuildStrategy{
-						Type:           buildv1.DockerBuildStrategyType,
-						DockerStrategy: &buildv1.DockerBuildStrategy{},
-					},
-				},
-			},
-			Status: buildv1.BuildStatus{
-				Phase: buildv1.BuildPhaseError,
-			},
-		},
-		{
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: "test-2",
-				Name:      "build-docker-1",
-				Annotations: map[string]string{
-					buildv1.BuildConfigAnnotation: "build-docker",
-				},
-			},
-			Spec: buildv1.BuildSpec{
-				CommonSpec: buildv1.CommonSpec{
-					Strategy: buildv1.BuildStrategy{
-						Type:           buildv1.DockerBuildStrategyType,
-						DockerStrategy: &buildv1.DockerBuildStrategy{},
-					},
-				},
-			},
-			Status: buildv1.BuildStatus{
-				Phase: buildv1.BuildPhaseComplete,
-			},
-		},
-		{
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: "test-2",
-				Name:      "build-src-1",
-				Annotations: map[string]string{
-					buildv1.BuildConfigAnnotation: "build-src",
-				},
-			},
-			Spec: buildv1.BuildSpec{
-				CommonSpec: buildv1.CommonSpec{
-					Strategy: buildv1.BuildStrategy{
-						Type:           buildv1.SourceBuildStrategyType,
-						SourceStrategy: &buildv1.SourceBuildStrategy{},
-					},
-				},
-			},
-			Status: buildv1.BuildStatus{
-				Phase: buildv1.BuildPhaseComplete,
-			},
-		},
-		{
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: "test-2",
-				Name:      "build-src-2",
-				Annotations: map[string]string{
-					buildv1.BuildConfigAnnotation: "build-src",
-				},
-			},
-			Spec: buildv1.BuildSpec{
-				CommonSpec: buildv1.CommonSpec{
-					Strategy: buildv1.BuildStrategy{
-						Type:           buildv1.SourceBuildStrategyType,
-						SourceStrategy: &buildv1.SourceBuildStrategy{},
-					},
-				},
-			},
-			Status: buildv1.BuildStatus{
-				Phase: buildv1.BuildPhaseCancelled,
-			},
-		},
-	}
-	expectedResponse := []string{
-		"# HELP openshift_build_result_total [ALPHA] Counts the total number of finished builds across all namespaces by result and strategy",
-		"# TYPE openshift_build_result_total counter",
-		"openshift_build_result_total{result=\"failed\",strategy=\"docker\"} 2",
-		"openshift_build_result_total{result=\"failed\",strategy=\"source\"} 1",
-		"openshift_build_result_total{result=\"success\",strategy=\"docker\"} 4",
-		"openshift_build_result_total{result=\"success\",strategy=\"source\"} 1",
-	}
-
-	legacyregistry.MustRegister(buildResultCounter)
-
-	for _, build := range builds {
-		IncrementBuildFinished(&build)
-	}
-
-	h := promhttp.HandlerFor(legacyregistry.DefaultGatherer, promhttp.HandlerOpts{ErrorHandling: promhttp.PanicOnError})
-	rw := &fakeResponseWriter{header: http.Header{}}
-	h.ServeHTTP(rw, &http.Request{})
-
-	respStr := rw.String()
-
-	for _, s := range expectedResponse {
-		if !strings.Contains(respStr, s) {
-			t.Errorf("expected string %s did not appear in %s", s, respStr)
-		}
-	}
-}