From e57a28af2fb454bdd708cff506f65a09b6715f04 Mon Sep 17 00:00:00 2001 From: Christian Schlotter Date: Fri, 18 Aug 2023 15:45:54 +0200 Subject: [PATCH] feat(custommetrics) always extract the metric headers but only write them when we have metrics --- internal/store/builder.go | 68 +----------------------- pkg/metrics_store/metrics_writer.go | 16 +++--- pkg/metrics_store/metrics_writer_test.go | 32 +++++++++++ 3 files changed, 39 insertions(+), 77 deletions(-) diff --git a/internal/store/builder.go b/internal/store/builder.go index 31e9f2e450..691fcd9326 100644 --- a/internal/store/builder.go +++ b/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( diff --git a/pkg/metrics_store/metrics_writer.go b/pkg/metrics_store/metrics_writer.go index 63ac811ae9..ce86cb88ee 100644 --- a/pkg/metrics_store/metrics_writer.go +++ b/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 { diff --git a/pkg/metrics_store/metrics_writer_test.go b/pkg/metrics_store/metrics_writer_test.go index b13c160555..73bf164765 100644 --- a/pkg/metrics_store/metrics_writer_test.go +++ b/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, "") + } +}