Metrics: Prevent duplicates in MultiRegistry (#85880)

pkg/infra/metrics/service.go

@@ -2,9 +2,10 @@ package metrics
 
 import (
 	"context"
-	"errors"
+	"fmt"
 	"regexp"
 	"sort"
+	"strings"
 
 	"github.com/prometheus/client_golang/prometheus"
 	dto "github.com/prometheus/client_model/go"
@@ -108,7 +109,7 @@ func (g *addPrefixWrapper) Gather() ([]*dto.MetricFamily, error) {
 			*m.Name = "grafana_" + *m.Name
 			// since we are modifying the name, we need to check for duplicates in the gatherer
 			if _, exists := names[*m.Name]; exists {
-				return nil, errors.New("duplicate metric name: " + *m.Name)
+				return nil, fmt.Errorf("duplicate metric name: %s", *m.Name)
 			}
 		}
 		// keep track of names to detect duplicates
@@ -140,11 +141,20 @@ func (r *multiRegistry) Gather() (mfs []*dto.MetricFamily, err error) {
 
 		for i := 0; i < len(mf); i++ {
 			m := mf[i]
+			_, exists := names[*m.Name]
 			// prevent duplicate metric names
-			if _, exists := names[*m.Name]; !exists {
-				names[*m.Name] = struct{}{}
-				mfs = append(mfs, m)
+			if exists {
+				// we can skip go_ metrics without returning an error
+				// because they are known to be duplicates in both
+				// the k8s and prometheus gatherers.
+				if strings.HasPrefix(*m.Name, "go_") {
+					continue
+				}
+				errs = append(errs, fmt.Errorf("duplicate metric name: %s", *m.Name))
+				continue
 			}
+			names[*m.Name] = struct{}{}
+			mfs = append(mfs, m)
 		}
 	}
 

pkg/infra/metrics/service_test.go

@@ -7,7 +7,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestK8sGathererWrapper_Gather(t *testing.T) {
+func TestGathererPrefixWrapper_Gather(t *testing.T) {
 	orig := &mockGatherer{}
 	g := newAddPrefixWrapper(orig)
 
@@ -48,6 +48,88 @@ func TestK8sGathererWrapper_Gather(t *testing.T) {
 	})
 }
 
+func TestMultiRegistry_Gather(t *testing.T) {
+	one := &mockGatherer{}
+	two := &mockGatherer{}
+	g := newMultiRegistry(one, two)
+
+	t.Run("should merge and sort metrics", func(t *testing.T) {
+		one.GatherFunc = func() ([]*dto.MetricFamily, error) {
+			return []*dto.MetricFamily{
+				{Name: strptr("b")},
+				{Name: strptr("a")},
+			}, nil
+		}
+
+		two.GatherFunc = func() ([]*dto.MetricFamily, error) {
+			return []*dto.MetricFamily{
+				{Name: strptr("d")},
+				{Name: strptr("c")},
+			}, nil
+		}
+
+		expectedMF := []*dto.MetricFamily{
+			{Name: strptr("a")},
+			{Name: strptr("b")},
+			{Name: strptr("c")},
+			{Name: strptr("d")},
+		}
+
+		mf, err := g.Gather()
+		require.NoError(t, err)
+		require.Equal(t, expectedMF, mf)
+	})
+
+	t.Run("duplicate metrics result in an error", func(t *testing.T) {
+		one.GatherFunc = func() ([]*dto.MetricFamily, error) {
+			return []*dto.MetricFamily{
+				{Name: strptr("b")},
+				{Name: strptr("a")},
+			}, nil
+		}
+
+		two.GatherFunc = func() ([]*dto.MetricFamily, error) {
+			return []*dto.MetricFamily{
+				{Name: strptr("d")},
+				{Name: strptr("c")},
+				{Name: strptr("a")},
+			}, nil
+		}
+		_, err := g.Gather()
+		require.Error(t, err)
+	})
+
+	t.Run("duplicate go_ prefixed metrics do not result in an error", func(t *testing.T) {
+		one.GatherFunc = func() ([]*dto.MetricFamily, error) {
+			return []*dto.MetricFamily{
+				{Name: strptr("b")},
+				{Name: strptr("a")},
+				{Name: strptr("go_a")},
+			}, nil
+		}
+
+		two.GatherFunc = func() ([]*dto.MetricFamily, error) {
+			return []*dto.MetricFamily{
+				{Name: strptr("d")},
+				{Name: strptr("c")},
+				{Name: strptr("go_a")},
+			}, nil
+		}
+
+		expectedMF := []*dto.MetricFamily{
+			{Name: strptr("a")},
+			{Name: strptr("b")},
+			{Name: strptr("c")},
+			{Name: strptr("d")},
+			{Name: strptr("go_a")},
+		}
+
+		mf, err := g.Gather()
+		require.NoError(t, err)
+		require.Equal(t, expectedMF, mf)
+	})
+}
+
 type mockGatherer struct {
 	GatherFunc func() ([]*dto.MetricFamily, error)
 }