From 069b088ad7bb70f50bc22b581005205b869a16a0 Mon Sep 17 00:00:00 2001 From: RainbowMango Date: Wed, 27 Nov 2019 17:39:49 +0800 Subject: [PATCH 1/3] The descs in a stable collector should be tracked by a map instead of slice. --- .../component-base/metrics/collector.go | 39 ++++++++++++++--- .../component-base/metrics/collector_test.go | 42 +++++++++++++++++++ 2 files changed, 76 insertions(+), 5 deletions(-) diff --git a/staging/src/k8s.io/component-base/metrics/collector.go b/staging/src/k8s.io/component-base/metrics/collector.go index 49394b784dd..9604ab18fa7 100644 --- a/staging/src/k8s.io/component-base/metrics/collector.go +++ b/staging/src/k8s.io/component-base/metrics/collector.go @@ -43,8 +43,9 @@ type StableCollector interface { // 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(). - registrable []*Desc // stores registrable Desc(not be hidden), is a subset of descriptors. + descriptors map[string]*Desc // stores all descriptors by pair, these are collected from DescribeWithStability(). + registrable map[string]*Desc // stores registrable descriptors by pair, is a subset of descriptors. + hidden map[string]*Desc // stores hidden descriptors by pair, is a subset of descriptors. self StableCollector } @@ -88,7 +89,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 +121,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 +146,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) } } diff --git a/staging/src/k8s.io/component-base/metrics/collector_test.go b/staging/src/k8s.io/component-base/metrics/collector_test.go index db6c77c44a5..03f33067b04 100644 --- a/staging/src/k8s.io/component-base/metrics/collector_test.go +++ b/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) + }) + } +} From ea7a230af63e60a96705c6382b83443390abdbbb Mon Sep 17 00:00:00 2001 From: RainbowMango Date: Tue, 3 Dec 2019 17:12:07 +0800 Subject: [PATCH 2/3] All stable collector should be tracked in registry. --- .../k8s.io/component-base/metrics/collector.go | 11 +++++++++++ .../src/k8s.io/component-base/metrics/registry.go | 15 ++++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/staging/src/k8s.io/component-base/metrics/collector.go b/staging/src/k8s.io/component-base/metrics/collector.go index 9604ab18fa7..382ce5ce77d 100644 --- a/staging/src/k8s.io/component-base/metrics/collector.go +++ b/staging/src/k8s.io/component-base/metrics/collector.go @@ -37,6 +37,9 @@ type StableCollector interface { // Create will initialize all Desc and it intends to be called by registry. Create(version *semver.Version, self StableCollector) bool + + // HiddenMetrics tells the list of hidden metrics with fqName. + HiddenMetrics() []string } // BaseStableCollector which implements almost all of the methods defined by StableCollector @@ -159,5 +162,13 @@ func (bsc *BaseStableCollector) Create(version *semver.Version, self StableColle return false } +// 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{} diff --git a/staging/src/k8s.io/component-base/metrics/registry.go b/staging/src/k8s.io/component-base/metrics/registry.go index a2fd8a06cdc..4265b6b6663 100644 --- a/staging/src/k8s.io/component-base/metrics/registry.go +++ b/staging/src/k8s.io/component-base/metrics/registry.go @@ -131,7 +131,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 +168,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 +180,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) { @@ -234,6 +239,14 @@ 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() { kr.hiddenCollectorsLock.Lock() From f2f0435e7187945239d1a58e659f2a2417a71db5 Mon Sep 17 00:00:00 2001 From: RainbowMango Date: Tue, 3 Dec 2019 17:58:15 +0800 Subject: [PATCH 3/3] Enable hidden custom collectors when calling SetShowHidden(). --- .../component-base/metrics/collector.go | 15 ++++ .../k8s.io/component-base/metrics/registry.go | 39 ++++++++- .../component-base/metrics/registry_test.go | 86 +++++++++++++++++++ 3 files changed, 136 insertions(+), 4 deletions(-) diff --git a/staging/src/k8s.io/component-base/metrics/collector.go b/staging/src/k8s.io/component-base/metrics/collector.go index 382ce5ce77d..1f7ecb3de49 100644 --- a/staging/src/k8s.io/component-base/metrics/collector.go +++ b/staging/src/k8s.io/component-base/metrics/collector.go @@ -38,6 +38,9 @@ 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 } @@ -162,6 +165,18 @@ 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 { diff --git a/staging/src/k8s.io/component-base/metrics/registry.go b/staging/src/k8s.io/component-base/metrics/registry.go index 4265b6b6663..3ff4826e542 100644 --- a/staging/src/k8s.io/component-base/metrics/registry.go +++ b/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) } @@ -216,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) } @@ -249,14 +250,44 @@ func (kr *kubeRegistry) trackStableCollectors(cs ...StableCollector) { // 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 { diff --git a/staging/src/k8s.io/component-base/metrics/registry_test.go b/staging/src/k8s.io/component-base/metrics/registry_test.go index 9058b295bde..4dc225d4eb3 100644 --- a/staging/src/k8s.io/component-base/metrics/registry_test.go +++ b/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() + } + }) + } +}