Merge pull request #2154 from chrischdi/pr-custom-metrics-missing-header

fix: custommetrics: always extract the headers but only write it when we have metrics

Author: k8s-ci-robot

internal/store/builder.go

@@ -38,10 +38,6 @@ import (
 	policyv1 "k8s.io/api/policy/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	storagev1 "k8s.io/api/storage/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/client-go/discovery"
 	clientset "k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/klog/v2"
@@ -544,10 +540,7 @@ func (b *Builder) buildCustomResourceStores(resourceName string,
 	metricFamilies = generator.FilterFamilyGenerators(b.familyGeneratorFilter, metricFamilies)
 	composedMetricGenFuncs := generator.ComposeMetricGenFuncs(metricFamilies)
 
-	var familyHeaders []string
-	if b.hasResources(resourceName, expectedType) {
-		familyHeaders = generator.ExtractMetricFamilyHeaders(metricFamilies)
-	}
+	familyHeaders := generator.ExtractMetricFamilyHeaders(metricFamilies)
 
 	gvr := util.GVRFromType(resourceName, expectedType)
 	var gvrString string
@@ -590,65 +583,6 @@ func (b *Builder) buildCustomResourceStores(resourceName string,
 	return stores
 }
 
-func (b *Builder) hasResources(resourceName string, expectedType interface{}) bool {
-	gvr := util.GVRFromType(resourceName, expectedType)
-	if gvr == nil {
-		return true
-	}
-	discoveryClient, err := util.CreateDiscoveryClient(b.utilOptions.Apiserver, b.utilOptions.Kubeconfig)
-	if err != nil {
-		klog.ErrorS(err, "Failed to create discovery client")
-		return false
-	}
-	g := gvr.Group
-	v := gvr.Version
-	r := gvr.Resource
-	isCRDInstalled, err := discovery.IsResourceEnabled(discoveryClient, schema.GroupVersionResource{
-		Group:    g,
-		Version:  v,
-		Resource: r,
-	})
-	if err != nil {
-		klog.ErrorS(err, "Failed to check if CRD is enabled", "group", g, "version", v, "resource", r)
-		return false
-	}
-	if !isCRDInstalled {
-		klog.InfoS("CRD is not installed", "group", g, "version", v, "resource", r)
-		return false
-	}
-	// Wait for the resource to come up.
-	timer := time.NewTimer(ResourceDiscoveryTimeout)
-	ticker := time.NewTicker(ResourceDiscoveryInterval)
-	dynamicClient, err := util.CreateDynamicClient(b.utilOptions.Apiserver, b.utilOptions.Kubeconfig)
-	if err != nil {
-		klog.ErrorS(err, "Failed to create dynamic client")
-		return false
-	}
-	var list *unstructured.UnstructuredList
-	for range ticker.C {
-		select {
-		case <-timer.C:
-			klog.InfoS("No CRs found for GVR", "group", g, "version", v, "resource", r)
-			return false
-		default:
-			list, err = dynamicClient.Resource(schema.GroupVersionResource{
-				Group:    g,
-				Version:  v,
-				Resource: r,
-			}).List(b.ctx, metav1.ListOptions{})
-			if err != nil {
-				klog.ErrorS(err, "Failed to list objects", "group", g, "version", v, "resource", r)
-				return false
-			}
-		}
-		if len(list.Items) > 0 {
-			break
-		}
-	}
-
-	return true
-}
-
 // startReflector starts a Kubernetes client-go reflector with the given
 // listWatcher and registers it with the given store.
 func (b *Builder) startReflector(

pkg/metrics_store/metrics_writer.go

@@ -58,20 +58,16 @@ func (m MetricsWriter) WriteAll(w io.Writer) error {
 		}(s)
 	}
 
-	// If the first store has no headers, but has metrics, we need to write out
-	// an empty header to ensure that the metrics are written out correctly.
-	if m.stores[0].headers == nil && m.stores[0].metrics != nil {
-		m.stores[0].headers = []string{""}
-	}
 	for i, help := range m.stores[0].headers {
 		if help != "" && help != "\n" {
 			help += "\n"
 		}
-		// TODO: This writes out the help text for each metric family, before checking if the metrics for it exist,
-		// TODO: which is not ideal, and furthermore, diverges from the OpenMetrics standard.
-		_, err := w.Write([]byte(help))
-		if err != nil {
-			return fmt.Errorf("failed to write help text: %v", err)
+
+		if len(m.stores[0].metrics) > 0 {
+			_, err := w.Write([]byte(help))
+			if err != nil {
+				return fmt.Errorf("failed to write help text: %v", err)
+			}
 		}
 
 		for _, s := range m.stores {

pkg/metrics_store/metrics_writer_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package metricsstore_test
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 
@@ -231,3 +232,34 @@ func TestWriteAllWithMultipleStores(t *testing.T) {
 		}
 	}
 }
+
+// TestWriteAllWithEmptyStores checks that nothing is printed if no metrics exist for metric families.
+func TestWriteAllWithEmptyStores(t *testing.T) {
+	genFunc := func(obj interface{}) []metric.FamilyInterface {
+		mf1 := metric.Family{
+			Name:    "kube_service_info_1",
+			Metrics: []*metric.Metric{},
+		}
+
+		mf2 := metric.Family{
+			Name:    "kube_service_info_2",
+			Metrics: []*metric.Metric{},
+		}
+
+		return []metric.FamilyInterface{&mf1, &mf2}
+	}
+	store := metricsstore.NewMetricsStore([]string{"Info 1 about services", "Info 2 about services"}, genFunc)
+
+	multiNsWriter := metricsstore.NewMetricsWriter(store)
+	w := strings.Builder{}
+	err := multiNsWriter.WriteAll(&w)
+	if err != nil {
+		t.Fatalf("failed to write metrics: %v", err)
+	}
+	result := w.String()
+	fmt.Println(result)
+
+	if result != "" {
+		t.Fatalf("Unexpected output, got %q, want %q", result, "")
+	}
+}