Merge pull request #1921 from dgrisonnet/simplify-metricswriter

Remove duplicated MetricsWriter implementation

Author: k8s-ci-robot

internal/store/builder.go

@@ -234,24 +234,20 @@ func (b *Builder) WithAllowLabels(labels map[string][]string) error {
 // Build initializes and registers all enabled stores.
 // It returns metrics writers which can be used to write out
 // metrics from the stores.
-func (b *Builder) Build() []metricsstore.MetricsWriter {
+func (b *Builder) Build() metricsstore.MetricsWriterList {
 	if b.familyGeneratorFilter == nil {
 		panic("familyGeneratorFilter should not be nil")
 	}
 
-	var metricsWriters []metricsstore.MetricsWriter
+	var metricsWriters metricsstore.MetricsWriterList
 	var activeStoreNames []string
 
 	for _, c := range b.enabledResources {
 		constructor, ok := availableStores[c]
 		if ok {
 			stores := cacheStoresToMetricStores(constructor(b))
 			activeStoreNames = append(activeStoreNames, c)
-			if len(stores) == 1 {
-				metricsWriters = append(metricsWriters, stores[0])
-			} else {
-				metricsWriters = append(metricsWriters, metricsstore.NewMultiStoreMetricsWriter(stores))
-			}
+			metricsWriters = append(metricsWriters, metricsstore.NewMetricsWriter(stores...))
 		}
 	}
 

pkg/builder/builder.go

@@ -19,8 +19,6 @@ package builder
 import (
 	"context"
 
-	generator "k8s.io/kube-state-metrics/v2/pkg/metric_generator"
-
 	"github.com/prometheus/client_golang/prometheus"
 	vpaclientset "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/clientset/versioned"
 	clientset "k8s.io/client-go/kubernetes"
@@ -29,6 +27,7 @@ import (
 	internalstore "k8s.io/kube-state-metrics/v2/internal/store"
 	ksmtypes "k8s.io/kube-state-metrics/v2/pkg/builder/types"
 	"k8s.io/kube-state-metrics/v2/pkg/customresource"
+	generator "k8s.io/kube-state-metrics/v2/pkg/metric_generator"
 	metricsstore "k8s.io/kube-state-metrics/v2/pkg/metrics_store"
 	"k8s.io/kube-state-metrics/v2/pkg/options"
 )
@@ -135,7 +134,7 @@ func (b *Builder) WithCustomResourceStoreFactories(fs ...customresource.Registry
 
 // Build initializes and registers all enabled stores.
 // Returns metric writers.
-func (b *Builder) Build() []metricsstore.MetricsWriter {
+func (b *Builder) Build() metricsstore.MetricsWriterList {
 	return b.internal.Build()
 }
 

pkg/builder/types/interfaces.go

@@ -51,7 +51,7 @@ type BuilderInterface interface {
 	DefaultGenerateStoresFunc() BuildStoresFunc
 	DefaultGenerateCustomResourceStoresFunc() BuildCustomResourceStoresFunc
 	WithCustomResourceStoreFactories(fs ...customresource.RegistryFactory)
-	Build() []metricsstore.MetricsWriter
+	Build() metricsstore.MetricsWriterList
 	BuildStores() [][]cache.Store
 }
 

pkg/metrics_store/metrics_store.go

@@ -17,7 +17,6 @@ limitations under the License.
 package metricsstore
 
 import (
-	"io"
 	"sync"
 
 	"k8s.io/apimachinery/pkg/api/meta"
@@ -144,18 +143,3 @@ func (s *MetricsStore) Replace(list []interface{}, _ string) error {
 func (s *MetricsStore) Resync() error {
 	return nil
 }
-
-// WriteAll writes all metrics of the store into the given writer, zipped with the
-// help text of each metric family.
-func (s *MetricsStore) WriteAll(w io.Writer) {
-	s.mutex.RLock()
-	defer s.mutex.RUnlock()
-
-	for i, help := range s.headers {
-		w.Write([]byte(help))
-		w.Write([]byte{'\n'})
-		for _, metricFamilies := range s.metrics {
-			w.Write(metricFamilies[i])
-		}
-	}
-}

pkg/metrics_store/metrics_store_test.go

@@ -81,7 +81,11 @@ func TestObjectsSameNameDifferentNamespaces(t *testing.T) {
 	}
 
 	w := strings.Builder{}
-	ms.WriteAll(&w)
+	mw := NewMetricsWriter(ms)
+	err := mw.WriteAll(&w)
+	if err != nil {
+		t.Fatalf("failed to write metrics: %v", err)
+	}
 	m := w.String()
 
 	for _, id := range serviceIDS {

pkg/metrics_store/metrics_writer.go

@@ -16,28 +16,28 @@ limitations under the License.
 
 package metricsstore
 
-import "io"
+import (
+	"fmt"
+	"io"
+)
 
-// MetricsWriter is the interface that wraps the WriteAll method.
-// WriteAll writes out bytes to the underlying writer.
-type MetricsWriter interface {
-	WriteAll(w io.Writer)
-}
+// MetricsWriterList represent a list of MetricsWriter
+type MetricsWriterList []*MetricsWriter
 
-// MultiStoreMetricsWriter is a struct that holds multiple MetricsStore(s) and
+// MetricsWriter is a struct that holds multiple MetricsStore(s) and
 // implements the MetricsWriter interface.
 // It should be used with stores which have the same metric headers.
 //
-// MultiStoreMetricsWriter writes out metrics from the underlying stores so that
+// MetricsWriter writes out metrics from the underlying stores so that
 // metrics with the same name coming from different stores end up grouped together.
 // It also ensures that the metric headers are only written out once.
-type MultiStoreMetricsWriter struct {
+type MetricsWriter struct {
 	stores []*MetricsStore
 }
 
-// NewMultiStoreMetricsWriter creates a new MultiStoreMetricsWriter.
-func NewMultiStoreMetricsWriter(stores []*MetricsStore) MetricsWriter {
-	return &MultiStoreMetricsWriter{
+// NewMetricsWriter creates a new MetricsWriter.
+func NewMetricsWriter(stores ...*MetricsStore) *MetricsWriter {
+	return &MetricsWriter{
 		stores: stores,
 	}
 }
@@ -46,9 +46,9 @@ func NewMultiStoreMetricsWriter(stores []*MetricsStore) MetricsWriter {
 //
 // WriteAll writes metrics so that the ones with the same name
 // are grouped together when written out.
-func (m MultiStoreMetricsWriter) WriteAll(w io.Writer) {
+func (m MetricsWriter) WriteAll(w io.Writer) error {
 	if len(m.stores) == 0 {
-		return
+		return nil
 	}
 
 	for _, s := range m.stores {
@@ -59,12 +59,19 @@ func (m MultiStoreMetricsWriter) WriteAll(w io.Writer) {
 	}
 
 	for i, help := range m.stores[0].headers {
-		w.Write([]byte(help))
-		w.Write([]byte{'\n'})
+		_, err := w.Write([]byte(help + "\n"))
+		if err != nil {
+			return fmt.Errorf("failed to write help text: %v", err)
+		}
+
 		for _, s := range m.stores {
 			for _, metricFamilies := range s.metrics {
-				w.Write(metricFamilies[i])
+				_, err := w.Write(metricFamilies[i])
+				if err != nil {
+					return fmt.Errorf("failed to write metrics family: %v", err)
+				}
 			}
 		}
 	}
+	return nil
 }

pkg/metrics_store/metrics_writer_test.go

@@ -83,9 +83,12 @@ func TestWriteAllWithSingleStore(t *testing.T) {
 		}
 	}
 
-	multiNsWriter := metricsstore.NewMultiStoreMetricsWriter([]*metricsstore.MetricsStore{store})
+	multiNsWriter := metricsstore.NewMetricsWriter(store)
 	w := strings.Builder{}
-	multiNsWriter.WriteAll(&w)
+	err := multiNsWriter.WriteAll(&w)
+	if err != nil {
+		t.Fatalf("failed to write metrics: %v", err)
+	}
 	result := w.String()
 
 	resultLines := strings.Split(strings.TrimRight(result, "\n"), "\n")
@@ -192,9 +195,12 @@ func TestWriteAllWithMultipleStores(t *testing.T) {
 		}
 	}
 
-	multiNsWriter := metricsstore.NewMultiStoreMetricsWriter([]*metricsstore.MetricsStore{s1, s2})
+	multiNsWriter := metricsstore.NewMetricsWriter(s1, s2)
 	w := strings.Builder{}
-	multiNsWriter.WriteAll(&w)
+	err := multiNsWriter.WriteAll(&w)
+	if err != nil {
+		t.Fatalf("failed to write metrics: %v", err)
+	}
 	result := w.String()
 
 	resultLines := strings.Split(strings.TrimRight(result, "\n"), "\n")

pkg/metricshandler/metrics_handler.go

@@ -51,7 +51,7 @@ type MetricsHandler struct {
 
 	// mtx protects metricsWriters, curShard, and curTotalShards
 	mtx            *sync.RWMutex
-	metricsWriters []metricsstore.MetricsWriter
+	metricsWriters metricsstore.MetricsWriterList
 	curShard       int32
 	curTotalShards int
 }
@@ -200,7 +200,10 @@ func (m *MetricsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 
 	for _, w := range m.metricsWriters {
-		w.WriteAll(writer)
+		err := w.WriteAll(writer)
+		if err != nil {
+			klog.ErrorS(err, "Failed to write metrics")
+		}
 	}
 
 	// In case we gzipped the response, we have to close the writer.

tests/lib/lib_test.go

@@ -55,7 +55,11 @@ func TestAsLibrary(t *testing.T) {
 	time.Sleep(time.Second)
 
 	w := strings.Builder{}
-	c.WriteAll(&w)
+	mw := metricsstore.NewMetricsWriter(c)
+	err = mw.WriteAll(&w)
+	if err != nil {
+		t.Fatalf("failed to write metrics: %v", err)
+	}
 	m := w.String()
 
 	if !strings.Contains(m, service.ObjectMeta.Name) {