From 67f3128cb9b3cb9c6a9ddbd07ff106dd65b0e4e8 Mon Sep 17 00:00:00 2001 From: yongruilin Date: Tue, 15 Oct 2024 10:26:46 -0700 Subject: [PATCH] refactor: Defer metrics label value allow list initialization Defer the initialization of label value allow lists until the first invocation of `WithLabelValues` or `With`. This fixes the issue that metrics initialized before flag applied which results in label value allow list is not honored. --- .../k8s.io/component-base/metrics/counter.go | 24 ++++- .../k8s.io/component-base/metrics/gauge.go | 23 ++++- .../component-base/metrics/histogram.go | 23 ++++- .../src/k8s.io/component-base/metrics/opts.go | 98 ++++++++++--------- .../k8s.io/component-base/metrics/summary.go | 23 ++++- .../metrics/timing_histogram.go | 23 ++++- 6 files changed, 142 insertions(+), 72 deletions(-) diff --git a/staging/src/k8s.io/component-base/metrics/counter.go b/staging/src/k8s.io/component-base/metrics/counter.go index 5664a68a900..875b0312a04 100644 --- a/staging/src/k8s.io/component-base/metrics/counter.go +++ b/staging/src/k8s.io/component-base/metrics/counter.go @@ -119,11 +119,6 @@ func NewCounterVec(opts *CounterOpts, labels []string) *CounterVec { opts.StabilityLevel.setDefaults() fqName := BuildFQName(opts.Namespace, opts.Subsystem, opts.Name) - allowListLock.RLock() - if allowList, ok := labelValueAllowLists[fqName]; ok { - opts.LabelValueAllowLists = allowList - } - allowListLock.RUnlock() cv := &CounterVec{ CounterVec: noopCounterVec, @@ -176,7 +171,17 @@ func (v *CounterVec) WithLabelValues(lvs ...string) CounterMetric { } if v.LabelValueAllowLists != nil { v.LabelValueAllowLists.ConstrainToAllowedList(v.originalLabels, lvs) + } else { + v.initializeLabelAllowListsOnce.Do(func() { + allowListLock.RLock() + if allowList, ok := labelValueAllowLists[v.FQName()]; ok { + v.LabelValueAllowLists = allowList + allowList.ConstrainToAllowedList(v.originalLabels, lvs) + } + allowListLock.RUnlock() + }) } + return v.CounterVec.WithLabelValues(lvs...) } @@ -190,6 +195,15 @@ func (v *CounterVec) With(labels map[string]string) CounterMetric { } if v.LabelValueAllowLists != nil { v.LabelValueAllowLists.ConstrainLabelMap(labels) + } else { + v.initializeLabelAllowListsOnce.Do(func() { + allowListLock.RLock() + if allowList, ok := labelValueAllowLists[v.FQName()]; ok { + v.LabelValueAllowLists = allowList + allowList.ConstrainLabelMap(labels) + } + allowListLock.RUnlock() + }) } return v.CounterVec.With(labels) } diff --git a/staging/src/k8s.io/component-base/metrics/gauge.go b/staging/src/k8s.io/component-base/metrics/gauge.go index 89631115acb..ac33897f708 100644 --- a/staging/src/k8s.io/component-base/metrics/gauge.go +++ b/staging/src/k8s.io/component-base/metrics/gauge.go @@ -105,11 +105,6 @@ func NewGaugeVec(opts *GaugeOpts, labels []string) *GaugeVec { opts.StabilityLevel.setDefaults() fqName := BuildFQName(opts.Namespace, opts.Subsystem, opts.Name) - allowListLock.RLock() - if allowList, ok := labelValueAllowLists[fqName]; ok { - opts.LabelValueAllowLists = allowList - } - allowListLock.RUnlock() cv := &GaugeVec{ GaugeVec: noopGaugeVec, @@ -149,6 +144,15 @@ func (v *GaugeVec) WithLabelValuesChecked(lvs ...string) (GaugeMetric, error) { } if v.LabelValueAllowLists != nil { v.LabelValueAllowLists.ConstrainToAllowedList(v.originalLabels, lvs) + } else { + v.initializeLabelAllowListsOnce.Do(func() { + allowListLock.RLock() + if allowList, ok := labelValueAllowLists[v.FQName()]; ok { + v.LabelValueAllowLists = allowList + allowList.ConstrainToAllowedList(v.originalLabels, lvs) + } + allowListLock.RUnlock() + }) } elt, err := v.GaugeVec.GetMetricWithLabelValues(lvs...) return elt, err @@ -186,6 +190,15 @@ func (v *GaugeVec) WithChecked(labels map[string]string) (GaugeMetric, error) { } if v.LabelValueAllowLists != nil { v.LabelValueAllowLists.ConstrainLabelMap(labels) + } else { + v.initializeLabelAllowListsOnce.Do(func() { + allowListLock.RLock() + if allowList, ok := labelValueAllowLists[v.FQName()]; ok { + v.LabelValueAllowLists = allowList + allowList.ConstrainLabelMap(labels) + } + allowListLock.RUnlock() + }) } elt, err := v.GaugeVec.GetMetricWith(labels) return elt, err diff --git a/staging/src/k8s.io/component-base/metrics/histogram.go b/staging/src/k8s.io/component-base/metrics/histogram.go index e6884f35c6f..2ffbe3192bc 100644 --- a/staging/src/k8s.io/component-base/metrics/histogram.go +++ b/staging/src/k8s.io/component-base/metrics/histogram.go @@ -96,11 +96,6 @@ func NewHistogramVec(opts *HistogramOpts, labels []string) *HistogramVec { opts.StabilityLevel.setDefaults() fqName := BuildFQName(opts.Namespace, opts.Subsystem, opts.Name) - allowListLock.RLock() - if allowList, ok := labelValueAllowLists[fqName]; ok { - opts.LabelValueAllowLists = allowList - } - allowListLock.RUnlock() v := &HistogramVec{ HistogramVec: noopHistogramVec, @@ -148,6 +143,15 @@ func (v *HistogramVec) WithLabelValues(lvs ...string) ObserverMetric { } if v.LabelValueAllowLists != nil { v.LabelValueAllowLists.ConstrainToAllowedList(v.originalLabels, lvs) + } else { + v.initializeLabelAllowListsOnce.Do(func() { + allowListLock.RLock() + if allowList, ok := labelValueAllowLists[v.FQName()]; ok { + v.LabelValueAllowLists = allowList + allowList.ConstrainToAllowedList(v.originalLabels, lvs) + } + allowListLock.RUnlock() + }) } return v.HistogramVec.WithLabelValues(lvs...) } @@ -162,6 +166,15 @@ func (v *HistogramVec) With(labels map[string]string) ObserverMetric { } if v.LabelValueAllowLists != nil { v.LabelValueAllowLists.ConstrainLabelMap(labels) + } else { + v.initializeLabelAllowListsOnce.Do(func() { + allowListLock.RLock() + if allowList, ok := labelValueAllowLists[v.FQName()]; ok { + v.LabelValueAllowLists = allowList + allowList.ConstrainLabelMap(labels) + } + allowListLock.RUnlock() + }) } return v.HistogramVec.With(labels) } diff --git a/staging/src/k8s.io/component-base/metrics/opts.go b/staging/src/k8s.io/component-base/metrics/opts.go index 59994e3db9d..596851cfd9e 100644 --- a/staging/src/k8s.io/component-base/metrics/opts.go +++ b/staging/src/k8s.io/component-base/metrics/opts.go @@ -44,16 +44,17 @@ var ( // Name must be set to a non-empty string. DeprecatedVersion is defined only // if the metric for which this options applies is, in fact, deprecated. type KubeOpts struct { - Namespace string - Subsystem string - Name string - Help string - ConstLabels map[string]string - DeprecatedVersion string - deprecateOnce sync.Once - annotateOnce sync.Once - StabilityLevel StabilityLevel - LabelValueAllowLists *MetricLabelAllowList + Namespace string + Subsystem string + Name string + Help string + ConstLabels map[string]string + DeprecatedVersion string + deprecateOnce sync.Once + annotateOnce sync.Once + StabilityLevel StabilityLevel + initializeLabelAllowListsOnce sync.Once + LabelValueAllowLists *MetricLabelAllowList } // BuildFQName joins the given three name components by "_". Empty name @@ -160,17 +161,18 @@ func (o *GaugeOpts) toPromGaugeOpts() prometheus.GaugeOpts { // and can safely be left at their zero value, although it is strongly // encouraged to set a Help string. type HistogramOpts struct { - Namespace string - Subsystem string - Name string - Help string - ConstLabels map[string]string - Buckets []float64 - DeprecatedVersion string - deprecateOnce sync.Once - annotateOnce sync.Once - StabilityLevel StabilityLevel - LabelValueAllowLists *MetricLabelAllowList + Namespace string + Subsystem string + Name string + Help string + ConstLabels map[string]string + Buckets []float64 + DeprecatedVersion string + deprecateOnce sync.Once + annotateOnce sync.Once + StabilityLevel StabilityLevel + initializeLabelAllowListsOnce sync.Once + LabelValueAllowLists *MetricLabelAllowList } // Modify help description on the metric description. @@ -206,18 +208,19 @@ func (o *HistogramOpts) toPromHistogramOpts() prometheus.HistogramOpts { // and can safely be left at their zero value, although it is strongly // encouraged to set a Help string. type TimingHistogramOpts struct { - Namespace string - Subsystem string - Name string - Help string - ConstLabels map[string]string - Buckets []float64 - InitialValue float64 - DeprecatedVersion string - deprecateOnce sync.Once - annotateOnce sync.Once - StabilityLevel StabilityLevel - LabelValueAllowLists *MetricLabelAllowList + Namespace string + Subsystem string + Name string + Help string + ConstLabels map[string]string + Buckets []float64 + InitialValue float64 + DeprecatedVersion string + deprecateOnce sync.Once + annotateOnce sync.Once + StabilityLevel StabilityLevel + initializeLabelAllowListsOnce sync.Once + LabelValueAllowLists *MetricLabelAllowList } // Modify help description on the metric description. @@ -255,20 +258,21 @@ func (o *TimingHistogramOpts) toPromHistogramOpts() promext.TimingHistogramOpts // a help string and to explicitly set the Objectives field to the desired value // as the default value will change in the upcoming v0.10 of the library. type SummaryOpts struct { - Namespace string - Subsystem string - Name string - Help string - ConstLabels map[string]string - Objectives map[float64]float64 - MaxAge time.Duration - AgeBuckets uint32 - BufCap uint32 - DeprecatedVersion string - deprecateOnce sync.Once - annotateOnce sync.Once - StabilityLevel StabilityLevel - LabelValueAllowLists *MetricLabelAllowList + Namespace string + Subsystem string + Name string + Help string + ConstLabels map[string]string + Objectives map[float64]float64 + MaxAge time.Duration + AgeBuckets uint32 + BufCap uint32 + DeprecatedVersion string + deprecateOnce sync.Once + annotateOnce sync.Once + StabilityLevel StabilityLevel + initializeLabelAllowListsOnce sync.Once + LabelValueAllowLists *MetricLabelAllowList } // Modify help description on the metric description. diff --git a/staging/src/k8s.io/component-base/metrics/summary.go b/staging/src/k8s.io/component-base/metrics/summary.go index d40421645af..57b60338372 100644 --- a/staging/src/k8s.io/component-base/metrics/summary.go +++ b/staging/src/k8s.io/component-base/metrics/summary.go @@ -109,11 +109,6 @@ func NewSummaryVec(opts *SummaryOpts, labels []string) *SummaryVec { opts.StabilityLevel.setDefaults() fqName := BuildFQName(opts.Namespace, opts.Subsystem, opts.Name) - allowListLock.RLock() - if allowList, ok := labelValueAllowLists[fqName]; ok { - opts.LabelValueAllowLists = allowList - } - allowListLock.RUnlock() v := &SummaryVec{ SummaryOpts: opts, @@ -160,6 +155,15 @@ func (v *SummaryVec) WithLabelValues(lvs ...string) ObserverMetric { } if v.LabelValueAllowLists != nil { v.LabelValueAllowLists.ConstrainToAllowedList(v.originalLabels, lvs) + } else { + v.initializeLabelAllowListsOnce.Do(func() { + allowListLock.RLock() + if allowList, ok := labelValueAllowLists[v.FQName()]; ok { + v.LabelValueAllowLists = allowList + allowList.ConstrainToAllowedList(v.originalLabels, lvs) + } + allowListLock.RUnlock() + }) } return v.SummaryVec.WithLabelValues(lvs...) } @@ -174,6 +178,15 @@ func (v *SummaryVec) With(labels map[string]string) ObserverMetric { } if v.LabelValueAllowLists != nil { v.LabelValueAllowLists.ConstrainLabelMap(labels) + } else { + v.initializeLabelAllowListsOnce.Do(func() { + allowListLock.RLock() + if allowList, ok := labelValueAllowLists[v.FQName()]; ok { + v.LabelValueAllowLists = allowList + allowList.ConstrainLabelMap(labels) + } + allowListLock.RUnlock() + }) } return v.SummaryVec.With(labels) } diff --git a/staging/src/k8s.io/component-base/metrics/timing_histogram.go b/staging/src/k8s.io/component-base/metrics/timing_histogram.go index a0f0b253c7d..08751ecbb2b 100644 --- a/staging/src/k8s.io/component-base/metrics/timing_histogram.go +++ b/staging/src/k8s.io/component-base/metrics/timing_histogram.go @@ -125,11 +125,6 @@ func NewTestableTimingHistogramVec(nowFunc func() time.Time, opts *TimingHistogr opts.StabilityLevel.setDefaults() fqName := BuildFQName(opts.Namespace, opts.Subsystem, opts.Name) - allowListLock.RLock() - if allowList, ok := labelValueAllowLists[fqName]; ok { - opts.LabelValueAllowLists = allowList - } - allowListLock.RUnlock() v := &TimingHistogramVec{ TimingHistogramVec: noopTimingHistogramVec, @@ -175,6 +170,15 @@ func (v *TimingHistogramVec) WithLabelValuesChecked(lvs ...string) (GaugeMetric, } if v.LabelValueAllowLists != nil { v.LabelValueAllowLists.ConstrainToAllowedList(v.originalLabels, lvs) + } else { + v.initializeLabelAllowListsOnce.Do(func() { + allowListLock.RLock() + if allowList, ok := labelValueAllowLists[v.FQName()]; ok { + v.LabelValueAllowLists = allowList + allowList.ConstrainToAllowedList(v.originalLabels, lvs) + } + allowListLock.RUnlock() + }) } ops, err := v.TimingHistogramVec.GetMetricWithLabelValues(lvs...) if err != nil { @@ -214,6 +218,15 @@ func (v *TimingHistogramVec) WithChecked(labels map[string]string) (GaugeMetric, } if v.LabelValueAllowLists != nil { v.LabelValueAllowLists.ConstrainLabelMap(labels) + } else { + v.initializeLabelAllowListsOnce.Do(func() { + allowListLock.RLock() + if allowList, ok := labelValueAllowLists[v.FQName()]; ok { + v.LabelValueAllowLists = allowList + allowList.ConstrainLabelMap(labels) + } + allowListLock.RUnlock() + }) } ops, err := v.TimingHistogramVec.GetMetricWith(labels) return ops.(GaugeMetric), err