Merge pull request #85845 from RainbowMango/pr_enable_hidden_custom_collector

Provide a mechanism to enable hidden metrics in stable collector

staging/src/k8s.io/component-base/metrics/collector.go

@@ -37,14 +37,21 @@ type StableCollector interface {
 
 	// Create will initialize all Desc and it intends to be called by registry.
 	Create(version *semver.Version, self StableCollector) bool
+
+	// ClearState will clear all the states marked by Create.
+	ClearState()
+
+	// HiddenMetrics tells the list of hidden metrics with fqName.
+	HiddenMetrics() []string
 }
 
 // BaseStableCollector which implements almost all of the methods defined by StableCollector
 // is a convenient assistant for custom collectors.
 // It is recommend that inherit BaseStableCollector when implementing custom collectors.
 type BaseStableCollector struct {
-	descriptors []*Desc // stores all Desc collected from DescribeWithStability().
+	descriptors map[string]*Desc // stores all descriptors by pair<fqName, Desc>, these are collected from DescribeWithStability().
-	registrable []*Desc // stores registrable Desc(not be hidden), is a subset of descriptors.
+	registrable map[string]*Desc // stores registrable descriptors by pair<fqName, Desc>, is a subset of descriptors.
+	hidden      map[string]*Desc // stores hidden descriptors by pair<fqName, Desc>, is a subset of descriptors.
 	self        StableCollector
 }
 
@@ -88,7 +95,19 @@ func (bsc *BaseStableCollector) Collect(ch chan<- prometheus.Metric) {
 }
 
 func (bsc *BaseStableCollector) add(d *Desc) {
-	bsc.descriptors = append(bsc.descriptors, d)
+	if len(d.fqName) == 0 {
+		panic("nameless metrics will be not allowed")
+	}
+
+	if bsc.descriptors == nil {
+		bsc.descriptors = make(map[string]*Desc)
+	}
+
+	if _, exist := bsc.descriptors[d.fqName]; exist {
+		panic(fmt.Sprintf("duplicate metrics (%s) will be not allowed", d.fqName))
+	}
+
+	bsc.descriptors[d.fqName] = d
 }
 
 // Init intends to be called by registry.
@@ -108,6 +127,22 @@ func (bsc *BaseStableCollector) init(self StableCollector) {
 	}
 }
 
+func (bsc *BaseStableCollector) trackRegistrableDescriptor(d *Desc) {
+	if bsc.registrable == nil {
+		bsc.registrable = make(map[string]*Desc)
+	}
+
+	bsc.registrable[d.fqName] = d
+}
+
+func (bsc *BaseStableCollector) trackHiddenDescriptor(d *Desc) {
+	if bsc.hidden == nil {
+		bsc.hidden = make(map[string]*Desc)
+	}
+
+	bsc.hidden[d.fqName] = d
+}
+
 // Create intends to be called by registry.
 // Create will return true as long as there is one or more metrics not be hidden.
 // Otherwise return false, that means the whole collector will be ignored by registry.
@@ -117,9 +152,9 @@ func (bsc *BaseStableCollector) Create(version *semver.Version, self StableColle
 	for _, d := range bsc.descriptors {
 		d.create(version)
 		if d.IsHidden() {
-			// do nothing for hidden metrics
+			bsc.trackHiddenDescriptor(d)
 		} else {
-			bsc.registrable = append(bsc.registrable, d)
+			bsc.trackRegistrableDescriptor(d)
 		}
 	}
 
@@ -130,5 +165,25 @@ func (bsc *BaseStableCollector) Create(version *semver.Version, self StableColle
 	return false
 }
 
+// ClearState will clear all the states marked by Create.
+// It intends to be used for re-register a hidden metric.
+func (bsc *BaseStableCollector) ClearState() {
+	for _, d := range bsc.descriptors {
+		d.ClearState()
+	}
+
+	bsc.descriptors = nil
+	bsc.registrable = nil
+	bsc.hidden = nil
+}
+
+// HiddenMetrics tells the list of hidden metrics with fqName.
+func (bsc *BaseStableCollector) HiddenMetrics() (fqNames []string) {
+	for i := range bsc.hidden {
+		fqNames = append(fqNames, bsc.hidden[i].fqName)
+	}
+	return
+}
+
 // Check if our BaseStableCollector implements necessary interface
 var _ StableCollector = &BaseStableCollector{}

staging/src/k8s.io/component-base/metrics/collector_test.go

@@ -17,10 +17,13 @@ limitations under the License.
 package metrics
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 
 	"github.com/prometheus/client_golang/prometheus/testutil"
+	"github.com/stretchr/testify/assert"
+
 	apimachineryversion "k8s.io/apimachinery/pkg/version"
 )
 
@@ -92,3 +95,42 @@ func TestBaseCustomCollector(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+func TestInvalidCustomCollector(t *testing.T) {
+	var currentVersion = apimachineryversion.Info{
+		Major:      "1",
+		Minor:      "17",
+		GitVersion: "v1.17.0-alpha-1.12345",
+	}
+	var namelessDesc = NewDesc("", "this is a nameless metric", nil, nil, ALPHA, "")
+	var duplicatedDescA = NewDesc("test_duplicated_metric", "this is a duplicated metric A", nil, nil, ALPHA, "")
+	var duplicatedDescB = NewDesc("test_duplicated_metric", "this is a duplicated metric B", nil, nil, ALPHA, "")
+
+	var tests = []struct {
+		name        string
+		descriptors []*Desc
+		panicStr    string
+	}{
+		{
+			name:        "nameless metric will be not allowed",
+			descriptors: []*Desc{namelessDesc},
+			panicStr:    "nameless metrics will be not allowed",
+		},
+		{
+			name:        "duplicated metric will be not allowed",
+			descriptors: []*Desc{duplicatedDescA, duplicatedDescB},
+			panicStr:    fmt.Sprintf("duplicate metrics (%s) will be not allowed", duplicatedDescA.fqName),
+		},
+	}
+
+	for _, test := range tests {
+		tc := test
+		t.Run(tc.name, func(t *testing.T) {
+			registry := newKubeRegistry(currentVersion)
+			customCollector := newTestCustomCollector(tc.descriptors...)
+			assert.Panics(t, func() {
+				registry.CustomMustRegister(customCollector)
+			}, tc.panicStr)
+		})
+	}
+}

staging/src/k8s.io/component-base/metrics/registry.go

@@ -83,6 +83,7 @@ func SetShowHidden() {
 		// re-register collectors that has been hidden in phase of last registry.
 		for _, r := range registries {
 			r.enableHiddenCollectors()
+			r.enableHiddenStableCollectors()
 		}
 	})
 }
@@ -120,7 +121,7 @@ type KubeRegistry interface {
 	CustomMustRegister(cs ...StableCollector)
 	Register(Registerable) error
 	MustRegister(...Registerable)
-	Unregister(Registerable) bool
+	Unregister(collector Collector) bool
 	Gather() ([]*dto.MetricFamily, error)
 }
 
@@ -131,7 +132,9 @@ type kubeRegistry struct {
 	PromRegistry
 	version              semver.Version
 	hiddenCollectors     map[string]Registerable // stores all collectors that has been hidden
+	stableCollectors     []StableCollector       // stores all stable collector
 	hiddenCollectorsLock sync.RWMutex
+	stableCollectorsLock sync.RWMutex
 }
 
 // Register registers a new Collector to be included in metrics
@@ -166,10 +169,11 @@ func (kr *kubeRegistry) MustRegister(cs ...Registerable) {
 
 // CustomRegister registers a new custom collector.
 func (kr *kubeRegistry) CustomRegister(c StableCollector) error {
+	kr.trackStableCollectors(c)
+
 	if c.Create(&kr.version, c) {
 		return kr.PromRegistry.Register(c)
 	}
-
 	return nil
 }
 
@@ -177,6 +181,8 @@ func (kr *kubeRegistry) CustomRegister(c StableCollector) error {
 // StableCollectors and panics upon the first registration that causes an
 // error.
 func (kr *kubeRegistry) CustomMustRegister(cs ...StableCollector) {
+	kr.trackStableCollectors(cs...)
+
 	collectors := make([]prometheus.Collector, 0, len(cs))
 	for _, c := range cs {
 		if c.Create(&kr.version, c) {
@@ -211,7 +217,7 @@ func (kr *kubeRegistry) RawMustRegister(cs ...prometheus.Collector) {
 // returns whether a Collector was unregistered. Note that an unchecked
 // Collector cannot be unregistered (as its Describe method does not
 // yield any descriptor).
-func (kr *kubeRegistry) Unregister(collector Registerable) bool {
+func (kr *kubeRegistry) Unregister(collector Collector) bool {
 	return kr.PromRegistry.Unregister(collector)
 }
 
@@ -234,16 +240,54 @@ func (kr *kubeRegistry) trackHiddenCollector(c Registerable) {
 	kr.hiddenCollectors[c.FQName()] = c
 }
 
+// trackStableCollectors stores all custom collectors.
+func (kr *kubeRegistry) trackStableCollectors(cs ...StableCollector) {
+	kr.stableCollectorsLock.Lock()
+	defer kr.stableCollectorsLock.Unlock()
+
+	kr.stableCollectors = append(kr.stableCollectors, cs...)
+}
+
 // enableHiddenCollectors will re-register all of the hidden collectors.
 func (kr *kubeRegistry) enableHiddenCollectors() {
+	if len(kr.hiddenCollectors) == 0 {
+		return
+	}
+
 	kr.hiddenCollectorsLock.Lock()
-	defer kr.hiddenCollectorsLock.Unlock()
+	cs := make([]Registerable, 0, len(kr.hiddenCollectors))
 
 	for _, c := range kr.hiddenCollectors {
 		c.ClearState()
-		kr.MustRegister(c)
+		cs = append(cs, c)
 	}
+
 	kr.hiddenCollectors = nil
+	kr.hiddenCollectorsLock.Unlock()
+	kr.MustRegister(cs...)
+}
+
+// enableHiddenStableCollectors will re-register the stable collectors if there is one or more hidden metrics in it.
+// Since we can not register a metrics twice, so we have to unregister first then register again.
+func (kr *kubeRegistry) enableHiddenStableCollectors() {
+	if len(kr.stableCollectors) == 0 {
+		return
+	}
+
+	kr.stableCollectorsLock.Lock()
+
+	cs := make([]StableCollector, 0, len(kr.stableCollectors))
+	for _, c := range kr.stableCollectors {
+		if len(c.HiddenMetrics()) > 0 {
+			kr.Unregister(c) // unregister must happens before clear state, otherwise no metrics would be unregister
+			c.ClearState()
+			cs = append(cs, c)
+		}
+	}
+
+	kr.stableCollectors = nil
+	kr.stableCollectorsLock.Unlock()
+	kr.CustomMustRegister(cs...)
 }
 
 func newKubeRegistry(v apimachineryversion.Info) *kubeRegistry {

staging/src/k8s.io/component-base/metrics/registry_test.go

@@ -391,3 +391,89 @@ func TestEnableHiddenMetrics(t *testing.T) {
 		})
 	}
 }
+
+func TestEnableHiddenStableCollector(t *testing.T) {
+	var currentVersion = apimachineryversion.Info{
+		Major:      "1",
+		Minor:      "17",
+		GitVersion: "v1.17.0-alpha-1.12345",
+	}
+	var normal = NewDesc("test_enable_hidden_custom_metric_normal", "this is a normal metric", []string{"name"}, nil, STABLE, "")
+	var hiddenA = NewDesc("test_enable_hidden_custom_metric_hidden_a", "this is the hidden metric A", []string{"name"}, nil, STABLE, "1.16.0")
+	var hiddenB = NewDesc("test_enable_hidden_custom_metric_hidden_b", "this is the hidden metric B", []string{"name"}, nil, STABLE, "1.16.0")
+
+	var tests = []struct {
+		name                      string
+		descriptors               []*Desc
+		metricNames               []string
+		expectMetricsBeforeEnable string
+		expectMetricsAfterEnable  string
+	}{
+		{
+			name:        "all hidden",
+			descriptors: []*Desc{hiddenA, hiddenB},
+			metricNames: []string{"test_enable_hidden_custom_metric_hidden_a",
+				"test_enable_hidden_custom_metric_hidden_b"},
+			expectMetricsBeforeEnable: "",
+			expectMetricsAfterEnable: `
+        		# HELP test_enable_hidden_custom_metric_hidden_a [STABLE] (Deprecated since 1.16.0) this is the hidden metric A
+        		# TYPE test_enable_hidden_custom_metric_hidden_a gauge
+        		test_enable_hidden_custom_metric_hidden_a{name="value"} 1
+        		# HELP test_enable_hidden_custom_metric_hidden_b [STABLE] (Deprecated since 1.16.0) this is the hidden metric B
+        		# TYPE test_enable_hidden_custom_metric_hidden_b gauge
+        		test_enable_hidden_custom_metric_hidden_b{name="value"} 1
+			`,
+		},
+		{
+			name:        "partial hidden",
+			descriptors: []*Desc{normal, hiddenA, hiddenB},
+			metricNames: []string{"test_enable_hidden_custom_metric_normal",
+				"test_enable_hidden_custom_metric_hidden_a",
+				"test_enable_hidden_custom_metric_hidden_b"},
+			expectMetricsBeforeEnable: `
+        		# HELP test_enable_hidden_custom_metric_normal [STABLE] this is a normal metric
+        		# TYPE test_enable_hidden_custom_metric_normal gauge
+        		test_enable_hidden_custom_metric_normal{name="value"} 1
+			`,
+			expectMetricsAfterEnable: `
+        		# HELP test_enable_hidden_custom_metric_normal [STABLE] this is a normal metric
+        		# TYPE test_enable_hidden_custom_metric_normal gauge
+        		test_enable_hidden_custom_metric_normal{name="value"} 1
+        		# HELP test_enable_hidden_custom_metric_hidden_a [STABLE] (Deprecated since 1.16.0) this is the hidden metric A
+        		# TYPE test_enable_hidden_custom_metric_hidden_a gauge
+        		test_enable_hidden_custom_metric_hidden_a{name="value"} 1
+        		# HELP test_enable_hidden_custom_metric_hidden_b [STABLE] (Deprecated since 1.16.0) this is the hidden metric B
+        		# TYPE test_enable_hidden_custom_metric_hidden_b gauge
+        		test_enable_hidden_custom_metric_hidden_b{name="value"} 1
+			`,
+		},
+	}
+
+	for _, test := range tests {
+		tc := test
+		t.Run(tc.name, func(t *testing.T) {
+			registry := newKubeRegistry(currentVersion)
+			customCollector := newTestCustomCollector(tc.descriptors...)
+			registry.CustomMustRegister(customCollector)
+
+			if err := testutil.GatherAndCompare(registry, strings.NewReader(tc.expectMetricsBeforeEnable), tc.metricNames...); err != nil {
+				t.Fatalf("before enable test failed: %v", err)
+			}
+
+			SetShowHidden()
+			defer func() {
+				showHiddenOnce = *new(sync.Once)
+				showHidden.Store(false)
+			}()
+
+			if err := testutil.GatherAndCompare(registry, strings.NewReader(tc.expectMetricsAfterEnable), tc.metricNames...); err != nil {
+				t.Fatalf("after enable test failed: %v", err)
+			}
+
+			// refresh descriptors so as to share with cases.
+			for _, d := range tc.descriptors {
+				d.ClearState()
+			}
+		})
+	}
+}