From 68d92494907ae1d8a87b2e29cc9cc2a2709ca3a9 Mon Sep 17 00:00:00 2001 From: Mike Spreitzer Date: Fri, 29 Apr 2022 17:24:27 -0400 Subject: [PATCH 1/5] Add wrapper for TimingHistogram Do not bother wrapping WeightedHistogram because it is not used in k/k. --- hack/verify-prometheus-imports.sh | 1 + .../component-base/metrics/histogram.go | 1 + .../component-base/metrics/histogram_test.go | 40 +- .../k8s.io/component-base/metrics/metric.go | 25 +- .../src/k8s.io/component-base/metrics/opts.go | 49 +++ .../metrics/timing_histogram.go | 268 +++++++++++++ .../metrics/timing_histogram_test.go | 370 ++++++++++++++++++ .../k8s.io/component-base/metrics/wrappers.go | 73 ++++ vendor/modules.txt | 1 + 9 files changed, 812 insertions(+), 16 deletions(-) create mode 100644 staging/src/k8s.io/component-base/metrics/timing_histogram.go create mode 100644 staging/src/k8s.io/component-base/metrics/timing_histogram_test.go diff --git a/hack/verify-prometheus-imports.sh b/hack/verify-prometheus-imports.sh index d7c689fe72f..3572a3d905f 100755 --- a/hack/verify-prometheus-imports.sh +++ b/hack/verify-prometheus-imports.sh @@ -66,6 +66,7 @@ allowed_prometheus_importers=( ./staging/src/k8s.io/component-base/metrics/testutil/metrics_test.go ./staging/src/k8s.io/component-base/metrics/testutil/promlint.go ./staging/src/k8s.io/component-base/metrics/testutil/testutil.go + ./staging/src/k8s.io/component-base/metrics/timing_histogram_test.go ./staging/src/k8s.io/component-base/metrics/value.go ./staging/src/k8s.io/component-base/metrics/wrappers.go ./test/e2e/apimachinery/flowcontrol.go diff --git a/staging/src/k8s.io/component-base/metrics/histogram.go b/staging/src/k8s.io/component-base/metrics/histogram.go index e93c7a4b3ff..45c5da97217 100644 --- a/staging/src/k8s.io/component-base/metrics/histogram.go +++ b/staging/src/k8s.io/component-base/metrics/histogram.go @@ -18,6 +18,7 @@ package metrics import ( "context" + "github.com/blang/semver/v4" "github.com/prometheus/client_golang/prometheus" ) diff --git a/staging/src/k8s.io/component-base/metrics/histogram_test.go b/staging/src/k8s.io/component-base/metrics/histogram_test.go index e563a395ac8..e13cc18281b 100644 --- a/staging/src/k8s.io/component-base/metrics/histogram_test.go +++ b/staging/src/k8s.io/component-base/metrics/histogram_test.go @@ -87,6 +87,19 @@ func TestHistogram(t *testing.T) { }) c := NewHistogram(test.HistogramOpts) registry.MustRegister(c) + cm := c.ObserverMetric.(prometheus.Metric) + + metricChan := make(chan prometheus.Metric, 2) + c.Collect(metricChan) + close(metricChan) + m1 := <-metricChan + if m1 != cm { + t.Error("Unexpected metric", m1, cm) + } + m2, ok := <-metricChan + if ok { + t.Error("Unexpected second metric", m2) + } ms, err := registry.Gather() assert.Equalf(t, test.expectedMetricCount, len(ms), "Got %v metrics, Want: %v metrics", len(ms), test.expectedMetricCount) @@ -179,7 +192,24 @@ func TestHistogramVec(t *testing.T) { }) c := NewHistogramVec(test.HistogramOpts, test.labels) registry.MustRegister(c) - c.WithLabelValues("1", "2").Observe(1.0) + ov12 := c.WithLabelValues("1", "2") + cm1 := ov12.(prometheus.Metric) + ov12.Observe(1.0) + + if test.expectedMetricCount > 0 { + metricChan := make(chan prometheus.Metric, 2) + c.Collect(metricChan) + close(metricChan) + m1 := <-metricChan + if m1 != cm1 { + t.Error("Unexpected metric", m1, cm1) + } + m2, ok := <-metricChan + if ok { + t.Error("Unexpected second metric", m2) + } + } + ms, err := registry.Gather() assert.Equalf(t, test.expectedMetricCount, len(ms), "Got %v metrics, Want: %v metrics", len(ms), test.expectedMetricCount) assert.Nil(t, err, "Gather failed %v", err) @@ -218,12 +248,12 @@ func TestHistogramWithLabelValueAllowList(t *testing.T) { var tests = []struct { desc string labelValues [][]string - expectMetricValues map[string]int + expectMetricValues map[string]uint64 }{ { desc: "Test no unexpected input", labelValues: [][]string{{"allowed", "b1"}, {"allowed", "b2"}}, - expectMetricValues: map[string]int{ + expectMetricValues: map[string]uint64{ "allowed b1": 1.0, "allowed b2": 1.0, }, @@ -231,7 +261,7 @@ func TestHistogramWithLabelValueAllowList(t *testing.T) { { desc: "Test unexpected input", labelValues: [][]string{{"allowed", "b1"}, {"not_allowed", "b1"}}, - expectMetricValues: map[string]int{ + expectMetricValues: map[string]uint64{ "allowed b1": 1.0, "unexpected b1": 1.0, }, @@ -274,7 +304,7 @@ func TestHistogramWithLabelValueAllowList(t *testing.T) { labelValuePair := aValue + " " + bValue expectedValue, ok := test.expectMetricValues[labelValuePair] assert.True(t, ok, "Got unexpected label values, lable_a is %v, label_b is %v", aValue, bValue) - actualValue := int(m.GetHistogram().GetSampleCount()) + actualValue := m.GetHistogram().GetSampleCount() assert.Equalf(t, expectedValue, actualValue, "Got %v, wanted %v as the count while setting label_a to %v and label b to %v", actualValue, expectedValue, aValue, bValue) } } diff --git a/staging/src/k8s.io/component-base/metrics/metric.go b/staging/src/k8s.io/component-base/metrics/metric.go index c72aecfc6b4..e57e0b383d1 100644 --- a/staging/src/k8s.io/component-base/metrics/metric.go +++ b/staging/src/k8s.io/component-base/metrics/metric.go @@ -22,6 +22,7 @@ import ( "github.com/blang/semver/v4" "github.com/prometheus/client_golang/prometheus" dto "github.com/prometheus/client_model/go" + promext "k8s.io/component-base/metrics/prometheusextension" "k8s.io/klog/v2" ) @@ -203,6 +204,7 @@ func (c *selfCollector) Collect(ch chan<- prometheus.Metric) { // no-op vecs for convenience var noopCounterVec = &prometheus.CounterVec{} var noopHistogramVec = &prometheus.HistogramVec{} +var noopTimingHistogramVec = &promext.TimingHistogramVec{} var noopGaugeVec = &prometheus.GaugeVec{} var noopObserverVec = &noopObserverVector{} @@ -211,17 +213,18 @@ var noop = &noopMetric{} type noopMetric struct{} -func (noopMetric) Inc() {} -func (noopMetric) Add(float64) {} -func (noopMetric) Dec() {} -func (noopMetric) Set(float64) {} -func (noopMetric) Sub(float64) {} -func (noopMetric) Observe(float64) {} -func (noopMetric) SetToCurrentTime() {} -func (noopMetric) Desc() *prometheus.Desc { return nil } -func (noopMetric) Write(*dto.Metric) error { return nil } -func (noopMetric) Describe(chan<- *prometheus.Desc) {} -func (noopMetric) Collect(chan<- prometheus.Metric) {} +func (noopMetric) Inc() {} +func (noopMetric) Add(float64) {} +func (noopMetric) Dec() {} +func (noopMetric) Set(float64) {} +func (noopMetric) Sub(float64) {} +func (noopMetric) Observe(float64) {} +func (noopMetric) ObserveWithWeight(float64, uint64) {} +func (noopMetric) SetToCurrentTime() {} +func (noopMetric) Desc() *prometheus.Desc { return nil } +func (noopMetric) Write(*dto.Metric) error { return nil } +func (noopMetric) Describe(chan<- *prometheus.Desc) {} +func (noopMetric) Collect(chan<- prometheus.Metric) {} type noopObserverVector struct{} diff --git a/staging/src/k8s.io/component-base/metrics/opts.go b/staging/src/k8s.io/component-base/metrics/opts.go index 04203b74e0a..9d359d6acb6 100644 --- a/staging/src/k8s.io/component-base/metrics/opts.go +++ b/staging/src/k8s.io/component-base/metrics/opts.go @@ -24,6 +24,7 @@ import ( "github.com/prometheus/client_golang/prometheus" "k8s.io/apimachinery/pkg/util/sets" + promext "k8s.io/component-base/metrics/prometheusextension" ) var ( @@ -189,6 +190,54 @@ func (o *HistogramOpts) toPromHistogramOpts() prometheus.HistogramOpts { } } +// TimingHistogramOpts bundles the options for creating a TimingHistogram metric. It is +// mandatory to set Name to a non-empty string. All other fields are optional +// 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 +} + +// Modify help description on the metric description. +func (o *TimingHistogramOpts) markDeprecated() { + o.deprecateOnce.Do(func() { + o.Help = fmt.Sprintf("(Deprecated since %v) %v", o.DeprecatedVersion, o.Help) + }) +} + +// annotateStabilityLevel annotates help description on the metric description with the stability level +// of the metric +func (o *TimingHistogramOpts) annotateStabilityLevel() { + o.annotateOnce.Do(func() { + o.Help = fmt.Sprintf("[%v] %v", o.StabilityLevel, o.Help) + }) +} + +// convenience function to allow easy transformation to the prometheus +// counterpart. This will do more once we have a proper label abstraction +func (o *TimingHistogramOpts) toPromHistogramOpts() promext.TimingHistogramOpts { + return promext.TimingHistogramOpts{ + Namespace: o.Namespace, + Subsystem: o.Subsystem, + Name: o.Name, + Help: o.Help, + ConstLabels: o.ConstLabels, + Buckets: o.Buckets, + InitialValue: o.InitialValue, + } +} + // SummaryOpts bundles the options for creating a Summary metric. It is // mandatory to set Name to a non-empty string. While all other fields are // optional and can safely be left at their zero value, it is recommended to set diff --git a/staging/src/k8s.io/component-base/metrics/timing_histogram.go b/staging/src/k8s.io/component-base/metrics/timing_histogram.go new file mode 100644 index 00000000000..1472b541dab --- /dev/null +++ b/staging/src/k8s.io/component-base/metrics/timing_histogram.go @@ -0,0 +1,268 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package metrics + +import ( + "context" + "time" + + "github.com/blang/semver/v4" + promext "k8s.io/component-base/metrics/prometheusextension" +) + +// TimingHistogram is our internal representation for our wrapping struct around timing +// histograms. It implements both kubeCollector and GaugeMetric +type TimingHistogram struct { + GaugeMetric + *TimingHistogramOpts + nowFunc func() time.Time + lazyMetric + selfCollector +} + +// NewTimingHistogram returns an object which is TimingHistogram-like. However, nothing +// will be measured until the histogram is registered somewhere. +func NewTimingHistogram(opts *TimingHistogramOpts) *TimingHistogram { + return NewTestableTimingHistogram(time.Now, opts) +} + +// NewTestableTimingHistogram adds injection of the clock +func NewTestableTimingHistogram(nowFunc func() time.Time, opts *TimingHistogramOpts) *TimingHistogram { + opts.StabilityLevel.setDefaults() + + h := &TimingHistogram{ + TimingHistogramOpts: opts, + nowFunc: nowFunc, + lazyMetric: lazyMetric{}, + } + h.setPrometheusHistogram(noopMetric{}) + h.lazyInit(h, BuildFQName(opts.Namespace, opts.Subsystem, opts.Name)) + return h +} + +// setPrometheusHistogram sets the underlying KubeGauge object, i.e. the thing that does the measurement. +func (h *TimingHistogram) setPrometheusHistogram(histogram promext.TimingHistogram) { + h.GaugeMetric = histogram + h.initSelfCollection(histogram) +} + +// DeprecatedVersion returns a pointer to the Version or nil +func (h *TimingHistogram) DeprecatedVersion() *semver.Version { + return parseSemver(h.TimingHistogramOpts.DeprecatedVersion) +} + +// initializeMetric invokes the actual prometheus.Histogram object instantiation +// and stores a reference to it +func (h *TimingHistogram) initializeMetric() { + h.TimingHistogramOpts.annotateStabilityLevel() + // this actually creates the underlying prometheus gauge. + histogram, err := promext.NewTestableTimingHistogram(h.nowFunc, h.TimingHistogramOpts.toPromHistogramOpts()) + if err != nil { + panic(err) // handle as for regular histograms + } + h.setPrometheusHistogram(histogram) +} + +// initializeDeprecatedMetric invokes the actual prometheus.Histogram object instantiation +// but modifies the Help description prior to object instantiation. +func (h *TimingHistogram) initializeDeprecatedMetric() { + h.TimingHistogramOpts.markDeprecated() + h.initializeMetric() +} + +// WithContext allows the normal TimingHistogram metric to pass in context. The context is no-op now. +func (h *TimingHistogram) WithContext(ctx context.Context) GaugeMetric { + return h.GaugeMetric +} + +// timingHistogramVec is the internal representation of our wrapping struct around prometheus +// TimingHistogramVecs. +type timingHistogramVec struct { + *promext.TimingHistogramVec + *TimingHistogramOpts + nowFunc func() time.Time + lazyMetric + originalLabels []string +} + +// NewTimingHistogramVec returns an object which satisfies kubeCollector and +// wraps the promext.timingHistogramVec object. Note well the way that +// behavior depends on registration and whether this is hidden. +func NewTimingHistogramVec(opts *TimingHistogramOpts, labels []string) PreContextAndRegisterableGaugeMetricVec { + return NewTestableTimingHistogramVec(time.Now, opts, labels) +} + +// NewTestableTimingHistogramVec adds injection of the clock. +func NewTestableTimingHistogramVec(nowFunc func() time.Time, opts *TimingHistogramOpts, labels []string) PreContextAndRegisterableGaugeMetricVec { + 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, + TimingHistogramOpts: opts, + nowFunc: nowFunc, + originalLabels: labels, + lazyMetric: lazyMetric{}, + } + v.lazyInit(v, fqName) + return v +} + +// DeprecatedVersion returns a pointer to the Version or nil +func (v *timingHistogramVec) DeprecatedVersion() *semver.Version { + return parseSemver(v.TimingHistogramOpts.DeprecatedVersion) +} + +func (v *timingHistogramVec) initializeMetric() { + v.TimingHistogramOpts.annotateStabilityLevel() + v.TimingHistogramVec = promext.NewTestableTimingHistogramVec(v.nowFunc, v.TimingHistogramOpts.toPromHistogramOpts(), v.originalLabels...) +} + +func (v *timingHistogramVec) initializeDeprecatedMetric() { + v.TimingHistogramOpts.markDeprecated() + v.initializeMetric() +} + +func (v *timingHistogramVec) Set(value float64, labelValues ...string) { + gm, _ := v.WithLabelValues(labelValues...) + gm.Set(value) +} + +func (v *timingHistogramVec) Inc(labelValues ...string) { + gm, _ := v.WithLabelValues(labelValues...) + gm.Inc() +} + +func (v *timingHistogramVec) Dec(labelValues ...string) { + gm, _ := v.WithLabelValues(labelValues...) + gm.Dec() +} + +func (v *timingHistogramVec) Add(delta float64, labelValues ...string) { + gm, _ := v.WithLabelValues(labelValues...) + gm.Add(delta) +} +func (v *timingHistogramVec) SetToCurrentTime(labelValues ...string) { + gm, _ := v.WithLabelValues(labelValues...) + gm.SetToCurrentTime() +} + +func (v *timingHistogramVec) SetForLabels(value float64, labels map[string]string) { + gm, _ := v.With(labels) + gm.Set(value) +} + +func (v *timingHistogramVec) IncForLabels(labels map[string]string) { + gm, _ := v.With(labels) + gm.Inc() +} + +func (v *timingHistogramVec) DecForLabels(labels map[string]string) { + gm, _ := v.With(labels) + gm.Dec() +} + +func (v *timingHistogramVec) AddForLabels(delta float64, labels map[string]string) { + gm, _ := v.With(labels) + gm.Add(delta) +} +func (v *timingHistogramVec) SetToCurrentTimeForLabels(labels map[string]string) { + gm, _ := v.With(labels) + gm.SetToCurrentTime() +} + +// WithLabelValues, if called after this vector has been +// registered in at least one registry and this vector is not +// hidden, will return a GaugeMetric that is NOT a noop along +// with nil error. If called on a hidden vector then it will +// return a noop and a nil error. Otherwise it returns a noop +// and an error that passes ErrIsNotReady. +func (v *timingHistogramVec) WithLabelValues(lvs ...string) (GaugeMetric, error) { + if v.IsHidden() { + return noop, nil + } + if !v.IsCreated() { + return noop, errNotReady + } + if v.LabelValueAllowLists != nil { + v.LabelValueAllowLists.ConstrainToAllowedList(v.originalLabels, lvs) + } + return v.TimingHistogramVec.WithLabelValues(lvs...).(GaugeMetric), nil +} + +// With, if called after this vector has been +// registered in at least one registry and this vector is not +// hidden, will return a GaugeMetric that is NOT a noop along +// with nil error. If called on a hidden vector then it will +// return a noop and a nil error. Otherwise it returns a noop +// and an error that passes ErrIsNotReady. +func (v *timingHistogramVec) With(labels map[string]string) (GaugeMetric, error) { + if v.IsHidden() { + return noop, nil + } + if !v.IsCreated() { + return noop, errNotReady + } + if v.LabelValueAllowLists != nil { + v.LabelValueAllowLists.ConstrainLabelMap(labels) + } + return v.TimingHistogramVec.With(labels).(GaugeMetric), nil +} + +// Delete deletes the metric where the variable labels are the same as those +// passed in as labels. It returns true if a metric was deleted. +// +// It is not an error if the number and names of the Labels are inconsistent +// with those of the VariableLabels in Desc. However, such inconsistent Labels +// can never match an actual metric, so the method will always return false in +// that case. +func (v *timingHistogramVec) Delete(labels map[string]string) bool { + if !v.IsCreated() { + return false // since we haven't created the metric, we haven't deleted a metric with the passed in values + } + return v.TimingHistogramVec.Delete(labels) +} + +// Reset deletes all metrics in this vector. +func (v *timingHistogramVec) Reset() { + if !v.IsCreated() { + return + } + + v.TimingHistogramVec.Reset() +} + +// WithContext returns wrapped timingHistogramVec with context +func (v *timingHistogramVec) WithContext(ctx context.Context) GaugeMetricVec { + return &TimingHistogramVecWithContext{ + ctx: ctx, + timingHistogramVec: v, + } +} + +// TimingHistogramVecWithContext is the wrapper of timingHistogramVec with context. +// Currently the context is ignored. +type TimingHistogramVecWithContext struct { + *timingHistogramVec + ctx context.Context +} diff --git a/staging/src/k8s.io/component-base/metrics/timing_histogram_test.go b/staging/src/k8s.io/component-base/metrics/timing_histogram_test.go new file mode 100644 index 00000000000..43b5a1489ac --- /dev/null +++ b/staging/src/k8s.io/component-base/metrics/timing_histogram_test.go @@ -0,0 +1,370 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package metrics + +import ( + "testing" + "time" + + "github.com/blang/semver/v4" + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/assert" + + apimachineryversion "k8s.io/apimachinery/pkg/version" + testclock "k8s.io/utils/clock/testing" +) + +func TestTimingHistogram(t *testing.T) { + v115 := semver.MustParse("1.15.0") + var tests = []struct { + desc string + *TimingHistogramOpts + registryVersion *semver.Version + expectedMetricCount int + expectedHelp string + }{ + { + desc: "Test non deprecated", + TimingHistogramOpts: &TimingHistogramOpts{ + Namespace: "namespace", + Name: "metric_test_name", + Subsystem: "subsystem", + Help: "histogram help message", + Buckets: DefBuckets, + InitialValue: 13, + }, + registryVersion: &v115, + expectedMetricCount: 1, + expectedHelp: "EXPERIMENTAL: [ALPHA] histogram help message", + }, + { + desc: "Test deprecated", + TimingHistogramOpts: &TimingHistogramOpts{ + Namespace: "namespace", + Name: "metric_test_name", + Subsystem: "subsystem", + Help: "histogram help message", + DeprecatedVersion: "1.15.0", + Buckets: DefBuckets, + InitialValue: 3, + }, + registryVersion: &v115, + expectedMetricCount: 1, + expectedHelp: "EXPERIMENTAL: [ALPHA] (Deprecated since 1.15.0) histogram help message", + }, + { + desc: "Test hidden", + TimingHistogramOpts: &TimingHistogramOpts{ + Namespace: "namespace", + Name: "metric_test_name", + Subsystem: "subsystem", + Help: "histogram help message", + DeprecatedVersion: "1.14.0", + Buckets: DefBuckets, + InitialValue: 5, + }, + registryVersion: &v115, + expectedMetricCount: 0, + expectedHelp: "EXPERIMENTAL: histogram help message", + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + registry := newKubeRegistry(apimachineryversion.Info{ + Major: "1", + Minor: "15", + GitVersion: "v1.15.0-alpha-1.12345", + }) + t0 := time.Now() + clk := testclock.NewFakePassiveClock(t0) + c := NewTestableTimingHistogram(clk.Now, test.TimingHistogramOpts) + registry.MustRegister(c) + + metricChan := make(chan prometheus.Metric) + go func() { + c.Collect(metricChan) + close(metricChan) + }() + m1 := <-metricChan + gm1, ok := m1.(GaugeMetric) + if !ok || gm1 != c.GaugeMetric { + t.Error("Unexpected metric", m1, c.GaugeMetric) + } + m2, ok := <-metricChan + if ok { + t.Error("Unexpected second metric", m2) + } + + ms, err := registry.Gather() + assert.Equalf(t, test.expectedMetricCount, len(ms), "Got %v metrics, Want: %v metrics", len(ms), test.expectedMetricCount) + assert.Nil(t, err, "Gather failed %v", err) + + for _, metric := range ms { + assert.Equalf(t, test.expectedHelp, metric.GetHelp(), "Got %s as help message, want %s", metric.GetHelp(), test.expectedHelp) + } + + // let's exercise the metric and check that it still works + v0 := test.TimingHistogramOpts.InitialValue + dt1 := time.Nanosecond + t1 := t0.Add(dt1) + clk.SetTime(t1) + var v1 float64 = 10 + c.Set(v1) + dt2 := time.Hour + t2 := t1.Add(dt2) + clk.SetTime(t2) + var v2 float64 = 1e6 + c.Add(v2 - v1) + dt3 := time.Microsecond + t3 := t2.Add(dt3) + clk.SetTime(t3) + c.Set(0) + expectedCount := uint64(dt1 + dt2 + dt3) + expectedSum := float64(dt1)*v0 + float64(dt2)*v1 + float64(dt3)*v2 + ms, err = registry.Gather() + assert.Nil(t, err, "Gather failed %v", err) + + for _, mf := range ms { + t.Logf("Considering metric family %s", mf.GetName()) + for _, m := range mf.GetMetric() { + assert.Equalf(t, expectedCount, m.GetHistogram().GetSampleCount(), "Got %v, want %v as the sample count of metric %s", m.GetHistogram().GetSampleCount(), expectedCount, m.String()) + assert.Equalf(t, expectedSum, m.GetHistogram().GetSampleSum(), "Got %v, want %v as the sample sum of metric %s", m.GetHistogram().GetSampleSum(), expectedSum, m.String()) + } + } + }) + } +} + +func TestTimingHistogramVec(t *testing.T) { + v115 := semver.MustParse("1.15.0") + var tests = []struct { + desc string + *TimingHistogramOpts + labels []string + registryVersion *semver.Version + expectedMetricCount int + expectedHelp string + }{ + { + desc: "Test non deprecated", + TimingHistogramOpts: &TimingHistogramOpts{ + Namespace: "namespace", + Name: "metric_test_name", + Subsystem: "subsystem", + Help: "histogram help message", + Buckets: DefBuckets, + InitialValue: 5, + }, + labels: []string{"label_a", "label_b"}, + registryVersion: &v115, + expectedMetricCount: 1, + expectedHelp: "EXPERIMENTAL: [ALPHA] histogram help message", + }, + { + desc: "Test deprecated", + TimingHistogramOpts: &TimingHistogramOpts{ + Namespace: "namespace", + Name: "metric_test_name", + Subsystem: "subsystem", + Help: "histogram help message", + DeprecatedVersion: "1.15.0", + Buckets: DefBuckets, + InitialValue: 13, + }, + labels: []string{"label_a", "label_b"}, + registryVersion: &v115, + expectedMetricCount: 1, + expectedHelp: "EXPERIMENTAL: [ALPHA] (Deprecated since 1.15.0) histogram help message", + }, + { + desc: "Test hidden", + TimingHistogramOpts: &TimingHistogramOpts{ + Namespace: "namespace", + Name: "metric_test_name", + Subsystem: "subsystem", + Help: "histogram help message", + DeprecatedVersion: "1.14.0", + Buckets: DefBuckets, + InitialValue: 42, + }, + labels: []string{"label_a", "label_b"}, + registryVersion: &v115, + expectedMetricCount: 0, + expectedHelp: "EXPERIMENTAL: histogram help message", + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + registry := newKubeRegistry(apimachineryversion.Info{ + Major: "1", + Minor: "15", + GitVersion: "v1.15.0-alpha-1.12345", + }) + t0 := time.Now() + clk := testclock.NewFakePassiveClock(t0) + c := NewTestableTimingHistogramVec(clk.Now, test.TimingHistogramOpts, test.labels) + registry.MustRegister(c) + var v0 float64 = 3 + cm1, err := c.WithLabelValues("1", "2") + if err != nil { + t.Error(err) + } + cm1.Set(v0) + + if test.expectedMetricCount > 0 { + metricChan := make(chan prometheus.Metric, 2) + c.Collect(metricChan) + close(metricChan) + m1 := <-metricChan + if m1 != cm1.(prometheus.Metric) { + t.Error("Unexpected metric", m1, cm1) + } + m2, ok := <-metricChan + if ok { + t.Error("Unexpected second metric", m2) + } + } + + ms, err := registry.Gather() + assert.Equalf(t, test.expectedMetricCount, len(ms), "Got %v metrics, Want: %v metrics", len(ms), test.expectedMetricCount) + assert.Nil(t, err, "Gather failed %v", err) + for _, metric := range ms { + if metric.GetHelp() != test.expectedHelp { + assert.Equalf(t, test.expectedHelp, metric.GetHelp(), "Got %s as help message, want %s", metric.GetHelp(), test.expectedHelp) + } + } + + // let's exercise the metric and verify it still works + c.Set(v0, "1", "3") + c.Set(v0, "2", "3") + dt1 := time.Nanosecond + t1 := t0.Add(dt1) + clk.SetTime(t1) + c.Add(5.0, "1", "2") + c.Add(5.0, "1", "3") + c.Add(5.0, "2", "3") + ms, err = registry.Gather() + assert.Nil(t, err, "Gather failed %v", err) + + for _, mf := range ms { + t.Logf("Considering metric family %s", mf.String()) + assert.Equalf(t, 3, len(mf.GetMetric()), "Got %v metrics, wanted 3 as the count for family %#+v", len(mf.GetMetric()), mf) + for _, m := range mf.GetMetric() { + expectedCount := uint64(dt1) + expectedSum := float64(dt1) * v0 + assert.Equalf(t, expectedCount, m.GetHistogram().GetSampleCount(), "Got %v, expected histogram sample count to equal %d for metric %s", m.GetHistogram().GetSampleCount(), expectedCount, m.String()) + assert.Equalf(t, expectedSum, m.GetHistogram().GetSampleSum(), "Got %v, expected histogram sample sum to equal %v for metric %s", m.GetHistogram().GetSampleSum(), expectedSum, m.String()) + } + } + }) + } +} + +func TestTimingHistogramWithLabelValueAllowList(t *testing.T) { + labelAllowValues := map[string]string{ + "namespace_subsystem_metric_allowlist_test,label_a": "allowed", + } + labels := []string{"label_a", "label_b"} + opts := &TimingHistogramOpts{ + Namespace: "namespace", + Name: "metric_allowlist_test", + Subsystem: "subsystem", + InitialValue: 7, + } + var tests = []struct { + desc string + labelValues [][]string + expectMetricValues map[string]uint64 + }{ + { + desc: "Test no unexpected input", + labelValues: [][]string{{"allowed", "b1"}, {"allowed", "b2"}}, + expectMetricValues: map[string]uint64{ + "allowed b1": 1.0, + "allowed b2": 1.0, + }, + }, + { + desc: "Test unexpected input", + labelValues: [][]string{{"allowed", "b1"}, {"not_allowed", "b1"}}, + expectMetricValues: map[string]uint64{ + "allowed b1": 1.0, + "unexpected b1": 1.0, + }, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + SetLabelAllowListFromCLI(labelAllowValues) + registry := newKubeRegistry(apimachineryversion.Info{ + Major: "1", + Minor: "15", + GitVersion: "v1.15.0-alpha-1.12345", + }) + t0 := time.Now() + clk := testclock.NewFakePassiveClock(t0) + c := NewTestableTimingHistogramVec(clk.Now, opts, labels) + registry.MustRegister(c) + var v0 float64 = 13 + for _, lv := range test.labelValues { + c.Set(v0, lv...) + } + + dt1 := 3 * time.Hour + t1 := t0.Add(dt1) + clk.SetTime(t1) + + for _, lv := range test.labelValues { + c.Add(1.0, lv...) + } + mfs, err := registry.Gather() + assert.Nil(t, err, "Gather failed %v", err) + + for _, mf := range mfs { + if *mf.Name != BuildFQName(opts.Namespace, opts.Subsystem, opts.Name) { + continue + } + mfMetric := mf.GetMetric() + t.Logf("Consider metric family %s", mf.GetName()) + + for _, m := range mfMetric { + var aValue, bValue string + for _, l := range m.Label { + if *l.Name == "label_a" { + aValue = *l.Value + } + if *l.Name == "label_b" { + bValue = *l.Value + } + } + labelValuePair := aValue + " " + bValue + expectedCount, ok := test.expectMetricValues[labelValuePair] + assert.True(t, ok, "Got unexpected label values, lable_a is %v, label_b is %v", aValue, bValue) + expectedSum := float64(dt1) * v0 * float64(expectedCount) + expectedCount *= uint64(dt1) + actualCount := m.GetHistogram().GetSampleCount() + actualSum := m.GetHistogram().GetSampleSum() + assert.Equalf(t, expectedCount, actualCount, "Got %v, wanted %v as the count while setting label_a to %v and label b to %v", actualCount, expectedCount, aValue, bValue) + assert.Equalf(t, expectedSum, actualSum, "Got %v, wanted %v as the sum while setting label_a to %v and label b to %v", actualSum, expectedSum, aValue, bValue) + } + } + }) + } +} diff --git a/staging/src/k8s.io/component-base/metrics/wrappers.go b/staging/src/k8s.io/component-base/metrics/wrappers.go index 6ae8a458acb..4fb5e4d87af 100644 --- a/staging/src/k8s.io/component-base/metrics/wrappers.go +++ b/staging/src/k8s.io/component-base/metrics/wrappers.go @@ -17,6 +17,9 @@ limitations under the License. package metrics import ( + "context" + "errors" + "github.com/prometheus/client_golang/prometheus" dto "github.com/prometheus/client_model/go" ) @@ -65,6 +68,70 @@ type GaugeMetric interface { SetToCurrentTime() } +// GaugeMetricVec is a collection of Gauges that differ only in label values. +// This is really just one Metric. +// It might be better called GaugeVecMetric, but that pattern of name is already +// taken by the other pattern --- which is treacherous. The treachery is that +// WithLabelValues can return an object that is permanently broken (i.e., a noop). +type GaugeMetricVec interface { + Set(value float64, labelValues ...string) + Inc(labelValues ...string) + Dec(labelValues ...string) + Add(delta float64, labelValues ...string) + SetToCurrentTime(labelValues ...string) + + SetForLabels(value float64, labels map[string]string) + IncForLabels(labels map[string]string) + DecForLabels(labels map[string]string) + AddForLabels(delta float64, labels map[string]string) + SetToCurrentTimeForLabels(labels map[string]string) + + // WithLabelValues, if called after this vector has been + // registered in at least one registry and this vector is not + // hidden, will return a GaugeMetric that is NOT a noop along + // with nil error. If called on a hidden vector then it will + // return a noop and a nil error. Otherwise it returns a noop + // and an error that passes ErrIsNotReady. + WithLabelValues(labelValues ...string) (GaugeMetric, error) + + // With, if called after this vector has been + // registered in at least one registry and this vector is not + // hidden, will return a GaugeMetric that is NOT a noop along + // with nil error. If called on a hidden vector then it will + // return a noop and a nil error. Otherwise it returns a noop + // and an error that passes ErrIsNotReady. + With(labels map[string]string) (GaugeMetric, error) + + // Delete asserts that the vec should have no member for the given label set. + // The returned bool indicates whether there was a change. + // The return will certainly be `false` if the given label set has the wrong + // set of label names. + Delete(map[string]string) bool + + // Reset removes all the members + Reset() +} + +// PreContextGaugeMetricVec is something that can construct a GaugeMetricVec +// that uses a given Context. +type PreContextGaugeMetricVec interface { + // WithContext creates a GaugeMetricVec that uses the given Context + WithContext(ctx context.Context) GaugeMetricVec +} + +// RegisterableGaugeMetricVec is the intersection of Registerable and GaugeMetricVec +type RegisterableGaugeMetricVec interface { + Registerable + GaugeMetricVec +} + +// PreContextAndRegisterableGaugeMetricVec is the intersection of +// PreContextGaugeMetricVec and RegisterableGaugeMetricVec +type PreContextAndRegisterableGaugeMetricVec interface { + PreContextGaugeMetricVec + RegisterableGaugeMetricVec +} + // ObserverMetric captures individual observations. type ObserverMetric interface { Observe(float64) @@ -93,3 +160,9 @@ type GaugeFunc interface { Metric Collector } + +func ErrIsNotReady(err error) bool { + return err == errNotReady +} + +var errNotReady = errors.New("metric vec is not registered yet") diff --git a/vendor/modules.txt b/vendor/modules.txt index f3860c0c9ff..b8d202bf04b 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -1954,6 +1954,7 @@ k8s.io/component-base/metrics/prometheus/ratelimiter k8s.io/component-base/metrics/prometheus/restclient k8s.io/component-base/metrics/prometheus/version k8s.io/component-base/metrics/prometheus/workqueue +k8s.io/component-base/metrics/prometheusextension k8s.io/component-base/metrics/testutil k8s.io/component-base/term k8s.io/component-base/traces From b2c0c22e8f4b17079f37e9eeefa48464a5ac2539 Mon Sep 17 00:00:00 2001 From: Mike Spreitzer Date: Fri, 6 May 2022 04:07:51 -0400 Subject: [PATCH 2/5] A bit more tidying up --- .../k8s.io/component-base/metrics/timing_histogram.go | 8 ++++---- staging/src/k8s.io/component-base/metrics/wrappers.go | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) 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 1472b541dab..ab05cc03a31 100644 --- a/staging/src/k8s.io/component-base/metrics/timing_histogram.go +++ b/staging/src/k8s.io/component-base/metrics/timing_histogram.go @@ -196,13 +196,13 @@ func (v *timingHistogramVec) SetToCurrentTimeForLabels(labels map[string]string) // hidden, will return a GaugeMetric that is NOT a noop along // with nil error. If called on a hidden vector then it will // return a noop and a nil error. Otherwise it returns a noop -// and an error that passes ErrIsNotReady. +// and an error that passes ErrIsNotRegistered. func (v *timingHistogramVec) WithLabelValues(lvs ...string) (GaugeMetric, error) { if v.IsHidden() { return noop, nil } if !v.IsCreated() { - return noop, errNotReady + return noop, errNotRegistered } if v.LabelValueAllowLists != nil { v.LabelValueAllowLists.ConstrainToAllowedList(v.originalLabels, lvs) @@ -215,13 +215,13 @@ func (v *timingHistogramVec) WithLabelValues(lvs ...string) (GaugeMetric, error) // hidden, will return a GaugeMetric that is NOT a noop along // with nil error. If called on a hidden vector then it will // return a noop and a nil error. Otherwise it returns a noop -// and an error that passes ErrIsNotReady. +// and an error that passes ErrIsNotRegistered. func (v *timingHistogramVec) With(labels map[string]string) (GaugeMetric, error) { if v.IsHidden() { return noop, nil } if !v.IsCreated() { - return noop, errNotReady + return noop, errNotRegistered } if v.LabelValueAllowLists != nil { v.LabelValueAllowLists.ConstrainLabelMap(labels) diff --git a/staging/src/k8s.io/component-base/metrics/wrappers.go b/staging/src/k8s.io/component-base/metrics/wrappers.go index 4fb5e4d87af..840a18c48e1 100644 --- a/staging/src/k8s.io/component-base/metrics/wrappers.go +++ b/staging/src/k8s.io/component-base/metrics/wrappers.go @@ -91,7 +91,7 @@ type GaugeMetricVec interface { // hidden, will return a GaugeMetric that is NOT a noop along // with nil error. If called on a hidden vector then it will // return a noop and a nil error. Otherwise it returns a noop - // and an error that passes ErrIsNotReady. + // and an error that passes ErrIsNotRegistered. WithLabelValues(labelValues ...string) (GaugeMetric, error) // With, if called after this vector has been @@ -99,7 +99,7 @@ type GaugeMetricVec interface { // hidden, will return a GaugeMetric that is NOT a noop along // with nil error. If called on a hidden vector then it will // return a noop and a nil error. Otherwise it returns a noop - // and an error that passes ErrIsNotReady. + // and an error that passes ErrIsNotRegistered. With(labels map[string]string) (GaugeMetric, error) // Delete asserts that the vec should have no member for the given label set. @@ -161,8 +161,8 @@ type GaugeFunc interface { Collector } -func ErrIsNotReady(err error) bool { - return err == errNotReady +func ErrIsNotRegistered(err error) bool { + return err == errNotRegistered } -var errNotReady = errors.New("metric vec is not registered yet") +var errNotRegistered = errors.New("metric vec is not registered yet") From 4df968ae1ce2889d5c708865ec5ec42a7b69596c Mon Sep 17 00:00:00 2001 From: Mike Spreitzer Date: Mon, 9 May 2022 22:52:33 -0400 Subject: [PATCH 3/5] Make more ordinary and add benchmarks of wrapped timing histograms --- .../metrics/timing_histogram.go | 113 ++++++++---------- .../metrics/timing_histogram_test.go | 88 ++++++++++++-- .../k8s.io/component-base/metrics/wrappers.go | 56 +++++---- 3 files changed, 161 insertions(+), 96 deletions(-) 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 ab05cc03a31..6d790248810 100644 --- a/staging/src/k8s.io/component-base/metrics/timing_histogram.go +++ b/staging/src/k8s.io/component-base/metrics/timing_histogram.go @@ -143,61 +143,16 @@ func (v *timingHistogramVec) initializeDeprecatedMetric() { v.initializeMetric() } -func (v *timingHistogramVec) Set(value float64, labelValues ...string) { - gm, _ := v.WithLabelValues(labelValues...) - gm.Set(value) -} - -func (v *timingHistogramVec) Inc(labelValues ...string) { - gm, _ := v.WithLabelValues(labelValues...) - gm.Inc() -} - -func (v *timingHistogramVec) Dec(labelValues ...string) { - gm, _ := v.WithLabelValues(labelValues...) - gm.Dec() -} - -func (v *timingHistogramVec) Add(delta float64, labelValues ...string) { - gm, _ := v.WithLabelValues(labelValues...) - gm.Add(delta) -} -func (v *timingHistogramVec) SetToCurrentTime(labelValues ...string) { - gm, _ := v.WithLabelValues(labelValues...) - gm.SetToCurrentTime() -} - -func (v *timingHistogramVec) SetForLabels(value float64, labels map[string]string) { - gm, _ := v.With(labels) - gm.Set(value) -} - -func (v *timingHistogramVec) IncForLabels(labels map[string]string) { - gm, _ := v.With(labels) - gm.Inc() -} - -func (v *timingHistogramVec) DecForLabels(labels map[string]string) { - gm, _ := v.With(labels) - gm.Dec() -} - -func (v *timingHistogramVec) AddForLabels(delta float64, labels map[string]string) { - gm, _ := v.With(labels) - gm.Add(delta) -} -func (v *timingHistogramVec) SetToCurrentTimeForLabels(labels map[string]string) { - gm, _ := v.With(labels) - gm.SetToCurrentTime() -} - -// WithLabelValues, if called after this vector has been -// registered in at least one registry and this vector is not -// hidden, will return a GaugeMetric that is NOT a noop along -// with nil error. If called on a hidden vector then it will -// return a noop and a nil error. Otherwise it returns a noop -// and an error that passes ErrIsNotRegistered. -func (v *timingHistogramVec) WithLabelValues(lvs ...string) (GaugeMetric, error) { +// WithLabelValuesChecked, if called on a hidden vector, +// will return a noop gauge and a nil error. +// If called before this vector has been registered in +// at least one registry, will return a noop gauge and +// an error that passes ErrIsNotRegistered. +// If called with a syntactic problem in the labels, will +// return a noop gauge and an error about the labels. +// If none of the above apply, this method will return +// the appropriate vector member and a nil error. +func (v *timingHistogramVec) WithLabelValuesChecked(lvs ...string) (GaugeMetric, error) { if v.IsHidden() { return noop, nil } @@ -207,16 +162,33 @@ func (v *timingHistogramVec) WithLabelValues(lvs ...string) (GaugeMetric, error) if v.LabelValueAllowLists != nil { v.LabelValueAllowLists.ConstrainToAllowedList(v.originalLabels, lvs) } - return v.TimingHistogramVec.WithLabelValues(lvs...).(GaugeMetric), nil + ops, err := v.TimingHistogramVec.GetMetricWithLabelValues(lvs...) + return ops.(GaugeMetric), err } -// With, if called after this vector has been -// registered in at least one registry and this vector is not -// hidden, will return a GaugeMetric that is NOT a noop along -// with nil error. If called on a hidden vector then it will -// return a noop and a nil error. Otherwise it returns a noop -// and an error that passes ErrIsNotRegistered. -func (v *timingHistogramVec) With(labels map[string]string) (GaugeMetric, error) { +// WithLabelValues calls WithLabelValuesChecked +// and handles errors as follows. +// An error that passes ErrIsNotRegistered is ignored +// and the noop gauge is returned; +// all other errors cause a panic. +func (v *timingHistogramVec) WithLabelValues(lvs ...string) GaugeMetric { + ans, err := v.WithLabelValuesChecked(lvs...) + if err == nil || ErrIsNotRegistered(err) { + return ans + } + panic(err) +} + +// WithChecked, if called on a hidden vector, +// will return a noop gauge and a nil error. +// If called before this vector has been registered in +// at least one registry, will return a noop gauge and +// an error that passes ErrIsNotRegistered. +// If called with a syntactic problem in the labels, will +// return a noop gauge and an error about the labels. +// If none of the above apply, this method will return +// the appropriate vector member and a nil error. +func (v *timingHistogramVec) WithChecked(labels map[string]string) (GaugeMetric, error) { if v.IsHidden() { return noop, nil } @@ -226,7 +198,20 @@ func (v *timingHistogramVec) With(labels map[string]string) (GaugeMetric, error) if v.LabelValueAllowLists != nil { v.LabelValueAllowLists.ConstrainLabelMap(labels) } - return v.TimingHistogramVec.With(labels).(GaugeMetric), nil + ops, err := v.TimingHistogramVec.GetMetricWith(labels) + return ops.(GaugeMetric), err +} + +// With calls WithChecked and handles errors as follows. +// An error that passes ErrIsNotRegistered is ignored +// and the noop gauge is returned; +// all other errors cause a panic. +func (v *timingHistogramVec) With(labels map[string]string) GaugeMetric { + ans, err := v.WithChecked(labels) + if err == nil || ErrIsNotRegistered(err) { + return ans + } + panic(err) } // Delete deletes the metric where the variable labels are the same as those diff --git a/staging/src/k8s.io/component-base/metrics/timing_histogram_test.go b/staging/src/k8s.io/component-base/metrics/timing_histogram_test.go index 43b5a1489ac..b397ca44e34 100644 --- a/staging/src/k8s.io/component-base/metrics/timing_histogram_test.go +++ b/staging/src/k8s.io/component-base/metrics/timing_histogram_test.go @@ -221,7 +221,7 @@ func TestTimingHistogramVec(t *testing.T) { c := NewTestableTimingHistogramVec(clk.Now, test.TimingHistogramOpts, test.labels) registry.MustRegister(c) var v0 float64 = 3 - cm1, err := c.WithLabelValues("1", "2") + cm1, err := c.WithLabelValuesChecked("1", "2") if err != nil { t.Error(err) } @@ -251,14 +251,14 @@ func TestTimingHistogramVec(t *testing.T) { } // let's exercise the metric and verify it still works - c.Set(v0, "1", "3") - c.Set(v0, "2", "3") + c.WithLabelValues("1", "3").Set(v0) + c.WithLabelValues("2", "3").Set(v0) dt1 := time.Nanosecond t1 := t0.Add(dt1) clk.SetTime(t1) - c.Add(5.0, "1", "2") - c.Add(5.0, "1", "3") - c.Add(5.0, "2", "3") + c.WithLabelValues("1", "2").Add(5.0) + c.WithLabelValues("1", "3").Add(5.0) + c.WithLabelValues("2", "3").Add(5.0) ms, err = registry.Gather() assert.Nil(t, err, "Gather failed %v", err) @@ -324,7 +324,7 @@ func TestTimingHistogramWithLabelValueAllowList(t *testing.T) { registry.MustRegister(c) var v0 float64 = 13 for _, lv := range test.labelValues { - c.Set(v0, lv...) + c.WithLabelValues(lv...).Set(v0) } dt1 := 3 * time.Hour @@ -332,7 +332,7 @@ func TestTimingHistogramWithLabelValueAllowList(t *testing.T) { clk.SetTime(t1) for _, lv := range test.labelValues { - c.Add(1.0, lv...) + c.WithLabelValues(lv...).Add(1.0) } mfs, err := registry.Gather() assert.Nil(t, err, "Gather failed %v", err) @@ -368,3 +368,75 @@ func TestTimingHistogramWithLabelValueAllowList(t *testing.T) { }) } } + +func BenchmarkTimingHistogram(b *testing.B) { + b.StopTimer() + now := time.Now() + th := NewTestableTimingHistogram(func() time.Time { return now }, &TimingHistogramOpts{ + Namespace: "testns", + Subsystem: "testsubsys", + Name: "testhist", + Help: "Me", + Buckets: []float64{1, 2, 4, 8, 16}, + InitialValue: 3, + }) + registry := NewKubeRegistry() + registry.MustRegister(th) + var x int + b.StartTimer() + for i := 0; i < b.N; i++ { + now = now.Add(time.Duration(31-x) * time.Microsecond) + th.Set(float64(x)) + x = (x + i) % 23 + } +} + +func BenchmarkTimingHistogramVecEltCached(b *testing.B) { + b.StopTimer() + now := time.Now() + hv := NewTestableTimingHistogramVec(func() time.Time { return now }, &TimingHistogramOpts{ + Namespace: "testns", + Subsystem: "testsubsys", + Name: "testhist", + Help: "Me", + Buckets: []float64{1, 2, 4, 8, 16}, + InitialValue: 3, + }, + []string{"label1", "label2"}) + registry := NewKubeRegistry() + registry.MustRegister(hv) + th, err := hv.WithLabelValuesChecked("v1", "v2") + if err != nil { + b.Error(err) + } + var x int + b.StartTimer() + for i := 0; i < b.N; i++ { + now = now.Add(time.Duration(31-x) * time.Microsecond) + th.Set(float64(x)) + x = (x + i) % 23 + } +} + +func BenchmarkTimingHistogramVecEltFetched(b *testing.B) { + b.StopTimer() + now := time.Now() + hv := NewTestableTimingHistogramVec(func() time.Time { return now }, &TimingHistogramOpts{ + Namespace: "testns", + Subsystem: "testsubsys", + Name: "testhist", + Help: "Me", + Buckets: []float64{1, 2, 4, 8, 16}, + InitialValue: 3, + }, + []string{"label1", "label2"}) + registry := NewKubeRegistry() + registry.MustRegister(hv) + var x int + b.StartTimer() + for i := 0; i < b.N; i++ { + now = now.Add(time.Duration(31-x) * time.Microsecond) + hv.WithLabelValues("v1", "v2").Set(float64(x)) + x = (x + i) % 60 + } +} diff --git a/staging/src/k8s.io/component-base/metrics/wrappers.go b/staging/src/k8s.io/component-base/metrics/wrappers.go index 840a18c48e1..9ca4c8cb217 100644 --- a/staging/src/k8s.io/component-base/metrics/wrappers.go +++ b/staging/src/k8s.io/component-base/metrics/wrappers.go @@ -74,33 +74,41 @@ type GaugeMetric interface { // taken by the other pattern --- which is treacherous. The treachery is that // WithLabelValues can return an object that is permanently broken (i.e., a noop). type GaugeMetricVec interface { - Set(value float64, labelValues ...string) - Inc(labelValues ...string) - Dec(labelValues ...string) - Add(delta float64, labelValues ...string) - SetToCurrentTime(labelValues ...string) - SetForLabels(value float64, labels map[string]string) - IncForLabels(labels map[string]string) - DecForLabels(labels map[string]string) - AddForLabels(delta float64, labels map[string]string) - SetToCurrentTimeForLabels(labels map[string]string) + // WithLabelValuesChecked, if called on a hidden vector, + // will return a noop gauge and a nil error. + // If called before this vector has been registered in + // at least one registry, will return a noop gauge and + // an error that passes ErrIsNotRegistered. + // If called with a syntactic problem in the labels, will + // return a noop gauge and an error about the labels. + // If none of the above apply, this method will return + // the appropriate vector member and a nil error. + WithLabelValuesChecked(labelValues ...string) (GaugeMetric, error) - // WithLabelValues, if called after this vector has been - // registered in at least one registry and this vector is not - // hidden, will return a GaugeMetric that is NOT a noop along - // with nil error. If called on a hidden vector then it will - // return a noop and a nil error. Otherwise it returns a noop - // and an error that passes ErrIsNotRegistered. - WithLabelValues(labelValues ...string) (GaugeMetric, error) + // WithLabelValues calls WithLabelValuesChecked + // and handles errors as follows. + // An error that passes ErrIsNotRegistered is ignored + // and the noop gauge is returned; + // all other errors cause a panic. + WithLabelValues(labelValues ...string) GaugeMetric - // With, if called after this vector has been - // registered in at least one registry and this vector is not - // hidden, will return a GaugeMetric that is NOT a noop along - // with nil error. If called on a hidden vector then it will - // return a noop and a nil error. Otherwise it returns a noop - // and an error that passes ErrIsNotRegistered. - With(labels map[string]string) (GaugeMetric, error) + // WithChecked, if called on a hidden vector, + // will return a noop gauge and a nil error. + // If called before this vector has been registered in + // at least one registry, will return a noop gauge and + // an error that passes ErrIsNotRegistered. + // If called with a syntactic problem in the labels, will + // return a noop gauge and an error about the labels. + // If none of the above apply, this method will return + // the appropriate vector member and a nil error. + WithChecked(labels map[string]string) (GaugeMetric, error) + + // With calls WithChecked and handles errors as follows. + // An error that passes ErrIsNotRegistered is ignored + // and the noop gauge is returned; + // all other errors cause a panic. + With(labels map[string]string) GaugeMetric // Delete asserts that the vec should have no member for the given label set. // The returned bool indicates whether there was a change. From c87cd36d3e683dd270fb5a68655363058b2b4a49 Mon Sep 17 00:00:00 2001 From: Mike Spreitzer Date: Wed, 11 May 2022 09:04:17 -0400 Subject: [PATCH 4/5] Finish making Gauge and TimingHistogram implement the same interface --- .../k8s.io/component-base/metrics/counter.go | 17 +++-- .../k8s.io/component-base/metrics/gauge.go | 69 +++++++++++++++---- .../component-base/metrics/histogram.go | 12 +++- .../k8s.io/component-base/metrics/summary.go | 14 ++-- .../metrics/timing_histogram.go | 50 ++++++++------ .../k8s.io/component-base/metrics/wrappers.go | 39 ++++------- 6 files changed, 128 insertions(+), 73 deletions(-) diff --git a/staging/src/k8s.io/component-base/metrics/counter.go b/staging/src/k8s.io/component-base/metrics/counter.go index 7342dc37d1e..78c211a0ede 100644 --- a/staging/src/k8s.io/component-base/metrics/counter.go +++ b/staging/src/k8s.io/component-base/metrics/counter.go @@ -18,6 +18,7 @@ package metrics import ( "context" + "github.com/blang/semver/v4" "github.com/prometheus/client_golang/prometheus" dto "github.com/prometheus/client_model/go" @@ -106,9 +107,14 @@ type CounterVec struct { originalLabels []string } -// NewCounterVec returns an object which satisfies the kubeCollector and CounterVecMetric interfaces. +var _ kubeCollector = &CounterVec{} + +// TODO: make this true: var _ CounterVecMetric = &CounterVec{} + +// NewCounterVec returns an object which satisfies the kubeCollector and (almost) CounterVecMetric interfaces. // However, the object returned will not measure anything unless the collector is first -// registered, since the metric is lazily instantiated. +// registered, since the metric is lazily instantiated, and only members extracted after +// registration will actually measure anything. func NewCounterVec(opts *CounterOpts, labels []string) *CounterVec { opts.StabilityLevel.setDefaults() @@ -149,13 +155,16 @@ func (v *CounterVec) initializeDeprecatedMetric() { v.initializeMetric() } -// Default Prometheus behavior actually results in the creation of a new metric -// if a metric with the unique label values is not found in the underlying stored metricMap. +// Default Prometheus Vec behavior is that member extraction results in creation of a new element +// if one with the unique label values is not found in the underlying stored metricMap. // This means that if this function is called but the underlying metric is not registered // (which means it will never be exposed externally nor consumed), the metric will exist in memory // for perpetuity (i.e. throughout application lifecycle). // // For reference: https://github.com/prometheus/client_golang/blob/v0.9.2/prometheus/counter.go#L179-L197 +// +// In contrast, the Vec behavior in this package is that member extraction before registration +// returns a permanent noop object. // WithLabelValues returns the Counter for the given slice of label // values (same order as the VariableLabels in Desc). If that combination of diff --git a/staging/src/k8s.io/component-base/metrics/gauge.go b/staging/src/k8s.io/component-base/metrics/gauge.go index 168221ecdd5..dd2df34ac62 100644 --- a/staging/src/k8s.io/component-base/metrics/gauge.go +++ b/staging/src/k8s.io/component-base/metrics/gauge.go @@ -18,6 +18,7 @@ package metrics import ( "context" + "github.com/blang/semver/v4" "github.com/prometheus/client_golang/prometheus" @@ -33,7 +34,11 @@ type Gauge struct { selfCollector } -// NewGauge returns an object which satisfies the kubeCollector and KubeGauge interfaces. +var _ GaugeMetric = &Gauge{} +var _ Registerable = &Gauge{} +var _ kubeCollector = &Gauge{} + +// NewGauge returns an object which satisfies the kubeCollector, Registerable, and Gauge interfaces. // However, the object returned will not measure anything unless the collector is first // registered, since the metric is lazily instantiated. func NewGauge(opts *GaugeOpts) *Gauge { @@ -88,9 +93,14 @@ type GaugeVec struct { originalLabels []string } -// NewGaugeVec returns an object which satisfies the kubeCollector and KubeGaugeVec interfaces. +var _ GaugeVecMetric = &GaugeVec{} +var _ Registerable = &GaugeVec{} +var _ kubeCollector = &GaugeVec{} + +// NewGaugeVec returns an object which satisfies the kubeCollector, Registerable, and GaugeVecMetric interfaces. // However, the object returned will not measure anything unless the collector is first -// registered, since the metric is lazily instantiated. +// registered, since the metric is lazily instantiated, and only members extracted after +// registration will actually measure anything. func NewGaugeVec(opts *GaugeOpts, labels []string) *GaugeVec { opts.StabilityLevel.setDefaults() @@ -130,26 +140,55 @@ func (v *GaugeVec) initializeDeprecatedMetric() { v.initializeMetric() } -// Default Prometheus behavior actually results in the creation of a new metric -// if a metric with the unique label values is not found in the underlying stored metricMap. +func (v *GaugeVec) WithLabelValuesChecked(lvs ...string) (GaugeMetric, error) { + if v.IsHidden() { + return noop, nil + } + if !v.IsCreated() { + return noop, errNotRegistered // return no-op gauge + } + if v.LabelValueAllowLists != nil { + v.LabelValueAllowLists.ConstrainToAllowedList(v.originalLabels, lvs) + } + elt, err := v.GaugeVec.GetMetricWithLabelValues(lvs...) + return elt, err +} + +// Default Prometheus Vec behavior is that member extraction results in creation of a new element +// if one with the unique label values is not found in the underlying stored metricMap. // This means that if this function is called but the underlying metric is not registered // (which means it will never be exposed externally nor consumed), the metric will exist in memory // for perpetuity (i.e. throughout application lifecycle). // // For reference: https://github.com/prometheus/client_golang/blob/v0.9.2/prometheus/gauge.go#L190-L208 +// +// In contrast, the Vec behavior in this package is that member extraction before registration +// returns a permanent noop object. // WithLabelValues returns the GaugeMetric for the given slice of label // values (same order as the VariableLabels in Desc). If that combination of // label values is accessed for the first time, a new GaugeMetric is created IFF the gaugeVec // has been registered to a metrics registry. func (v *GaugeVec) WithLabelValues(lvs ...string) GaugeMetric { + ans, err := v.WithLabelValuesChecked(lvs...) + if err == nil || ErrIsNotRegistered(err) { + return ans + } + panic(err) +} + +func (v *GaugeVec) WithChecked(labels map[string]string) (GaugeMetric, error) { + if v.IsHidden() { + return noop, nil + } if !v.IsCreated() { - return noop // return no-op gauge + return noop, errNotRegistered // return no-op gauge } if v.LabelValueAllowLists != nil { - v.LabelValueAllowLists.ConstrainToAllowedList(v.originalLabels, lvs) + v.LabelValueAllowLists.ConstrainLabelMap(labels) } - return v.GaugeVec.WithLabelValues(lvs...) + elt, err := v.GaugeVec.GetMetricWith(labels) + return elt, err } // With returns the GaugeMetric for the given Labels map (the label names @@ -157,13 +196,11 @@ func (v *GaugeVec) WithLabelValues(lvs ...string) GaugeMetric { // accessed for the first time, a new GaugeMetric is created IFF the gaugeVec has // been registered to a metrics registry. func (v *GaugeVec) With(labels map[string]string) GaugeMetric { - if !v.IsCreated() { - return noop // return no-op gauge + ans, err := v.WithChecked(labels) + if err == nil || ErrIsNotRegistered(err) { + return ans } - if v.LabelValueAllowLists != nil { - v.LabelValueAllowLists.ConstrainLabelMap(labels) - } - return v.GaugeVec.With(labels) + panic(err) } // Delete deletes the metric where the variable labels are the same as those @@ -219,6 +256,10 @@ func (v *GaugeVec) WithContext(ctx context.Context) *GaugeVecWithContext { } } +func (v *GaugeVec) InterfaceWithContext(ctx context.Context) GaugeVecMetric { + return v.WithContext(ctx) +} + // GaugeVecWithContext is the wrapper of GaugeVec with context. type GaugeVecWithContext struct { *GaugeVec diff --git a/staging/src/k8s.io/component-base/metrics/histogram.go b/staging/src/k8s.io/component-base/metrics/histogram.go index 45c5da97217..838f09e17fd 100644 --- a/staging/src/k8s.io/component-base/metrics/histogram.go +++ b/staging/src/k8s.io/component-base/metrics/histogram.go @@ -101,7 +101,10 @@ type HistogramVec struct { // NewHistogramVec returns an object which satisfies kubeCollector and wraps the // prometheus.HistogramVec object. However, the object returned will not measure -// anything unless the collector is first registered, since the metric is lazily instantiated. +// anything unless the collector is first registered, since the metric is lazily instantiated, +// and only members extracted after +// registration will actually measure anything. + func NewHistogramVec(opts *HistogramOpts, labels []string) *HistogramVec { opts.StabilityLevel.setDefaults() @@ -137,13 +140,16 @@ func (v *HistogramVec) initializeDeprecatedMetric() { v.initializeMetric() } -// Default Prometheus behavior actually results in the creation of a new metric -// if a metric with the unique label values is not found in the underlying stored metricMap. +// Default Prometheus Vec behavior is that member extraction results in creation of a new element +// if one with the unique label values is not found in the underlying stored metricMap. // This means that if this function is called but the underlying metric is not registered // (which means it will never be exposed externally nor consumed), the metric will exist in memory // for perpetuity (i.e. throughout application lifecycle). // // For reference: https://github.com/prometheus/client_golang/blob/v0.9.2/prometheus/histogram.go#L460-L470 +// +// In contrast, the Vec behavior in this package is that member extraction before registration +// returns a permanent noop object. // WithLabelValues returns the ObserverMetric for the given slice of label // values (same order as the VariableLabels in Desc). If that combination of diff --git a/staging/src/k8s.io/component-base/metrics/summary.go b/staging/src/k8s.io/component-base/metrics/summary.go index fb1108f9245..c7621b986a4 100644 --- a/staging/src/k8s.io/component-base/metrics/summary.go +++ b/staging/src/k8s.io/component-base/metrics/summary.go @@ -18,6 +18,7 @@ package metrics import ( "context" + "github.com/blang/semver/v4" "github.com/prometheus/client_golang/prometheus" ) @@ -93,7 +94,9 @@ type SummaryVec struct { // NewSummaryVec returns an object which satisfies kubeCollector and wraps the // prometheus.SummaryVec object. However, the object returned will not measure -// anything unless the collector is first registered, since the metric is lazily instantiated. +// anything unless the collector is first registered, since the metric is lazily instantiated, +// and only members extracted after +// registration will actually measure anything. // // DEPRECATED: as per the metrics overhaul KEP func NewSummaryVec(opts *SummaryOpts, labels []string) *SummaryVec { @@ -130,13 +133,16 @@ func (v *SummaryVec) initializeDeprecatedMetric() { v.initializeMetric() } -// Default Prometheus behavior actually results in the creation of a new metric -// if a metric with the unique label values is not found in the underlying stored metricMap. +// Default Prometheus Vec behavior is that member extraction results in creation of a new element +// if one with the unique label values is not found in the underlying stored metricMap. // This means that if this function is called but the underlying metric is not registered // (which means it will never be exposed externally nor consumed), the metric will exist in memory // for perpetuity (i.e. throughout application lifecycle). // -// For reference: https://github.com/prometheus/client_golang/blob/v0.9.2/prometheus/summary.go#L485-L495 +// For reference: https://github.com/prometheus/client_golang/blob/v0.9.2/prometheus/histogram.go#L460-L470 +// +// In contrast, the Vec behavior in this package is that member extraction before registration +// returns a permanent noop object. // WithLabelValues returns the ObserverMetric for the given slice of label // values (same order as the VariableLabels in Desc). If that combination of 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 6d790248810..8e54850185b 100644 --- a/staging/src/k8s.io/component-base/metrics/timing_histogram.go +++ b/staging/src/k8s.io/component-base/metrics/timing_histogram.go @@ -34,6 +34,10 @@ type TimingHistogram struct { selfCollector } +var _ GaugeMetric = &TimingHistogram{} +var _ Registerable = &TimingHistogram{} +var _ kubeCollector = &TimingHistogram{} + // NewTimingHistogram returns an object which is TimingHistogram-like. However, nothing // will be measured until the histogram is registered somewhere. func NewTimingHistogram(opts *TimingHistogramOpts) *TimingHistogram { @@ -89,9 +93,9 @@ func (h *TimingHistogram) WithContext(ctx context.Context) GaugeMetric { return h.GaugeMetric } -// timingHistogramVec is the internal representation of our wrapping struct around prometheus +// TimingHistogramVec is the internal representation of our wrapping struct around prometheus // TimingHistogramVecs. -type timingHistogramVec struct { +type TimingHistogramVec struct { *promext.TimingHistogramVec *TimingHistogramOpts nowFunc func() time.Time @@ -99,15 +103,19 @@ type timingHistogramVec struct { originalLabels []string } -// NewTimingHistogramVec returns an object which satisfies kubeCollector and -// wraps the promext.timingHistogramVec object. Note well the way that +var _ GaugeVecMetric = &TimingHistogramVec{} +var _ Registerable = &TimingHistogramVec{} +var _ kubeCollector = &TimingHistogramVec{} + +// NewTimingHistogramVec returns an object which satisfies the kubeCollector, Registerable, and GaugeVecMetric interfaces +// and wraps an underlying promext.TimingHistogramVec object. Note well the way that // behavior depends on registration and whether this is hidden. -func NewTimingHistogramVec(opts *TimingHistogramOpts, labels []string) PreContextAndRegisterableGaugeMetricVec { +func NewTimingHistogramVec(opts *TimingHistogramOpts, labels []string) *TimingHistogramVec { return NewTestableTimingHistogramVec(time.Now, opts, labels) } // NewTestableTimingHistogramVec adds injection of the clock. -func NewTestableTimingHistogramVec(nowFunc func() time.Time, opts *TimingHistogramOpts, labels []string) PreContextAndRegisterableGaugeMetricVec { +func NewTestableTimingHistogramVec(nowFunc func() time.Time, opts *TimingHistogramOpts, labels []string) *TimingHistogramVec { opts.StabilityLevel.setDefaults() fqName := BuildFQName(opts.Namespace, opts.Subsystem, opts.Name) @@ -117,7 +125,7 @@ func NewTestableTimingHistogramVec(nowFunc func() time.Time, opts *TimingHistogr } allowListLock.RUnlock() - v := &timingHistogramVec{ + v := &TimingHistogramVec{ TimingHistogramVec: noopTimingHistogramVec, TimingHistogramOpts: opts, nowFunc: nowFunc, @@ -129,16 +137,16 @@ func NewTestableTimingHistogramVec(nowFunc func() time.Time, opts *TimingHistogr } // DeprecatedVersion returns a pointer to the Version or nil -func (v *timingHistogramVec) DeprecatedVersion() *semver.Version { +func (v *TimingHistogramVec) DeprecatedVersion() *semver.Version { return parseSemver(v.TimingHistogramOpts.DeprecatedVersion) } -func (v *timingHistogramVec) initializeMetric() { +func (v *TimingHistogramVec) initializeMetric() { v.TimingHistogramOpts.annotateStabilityLevel() v.TimingHistogramVec = promext.NewTestableTimingHistogramVec(v.nowFunc, v.TimingHistogramOpts.toPromHistogramOpts(), v.originalLabels...) } -func (v *timingHistogramVec) initializeDeprecatedMetric() { +func (v *TimingHistogramVec) initializeDeprecatedMetric() { v.TimingHistogramOpts.markDeprecated() v.initializeMetric() } @@ -152,7 +160,7 @@ func (v *timingHistogramVec) initializeDeprecatedMetric() { // return a noop gauge and an error about the labels. // If none of the above apply, this method will return // the appropriate vector member and a nil error. -func (v *timingHistogramVec) WithLabelValuesChecked(lvs ...string) (GaugeMetric, error) { +func (v *TimingHistogramVec) WithLabelValuesChecked(lvs ...string) (GaugeMetric, error) { if v.IsHidden() { return noop, nil } @@ -171,7 +179,7 @@ func (v *timingHistogramVec) WithLabelValuesChecked(lvs ...string) (GaugeMetric, // An error that passes ErrIsNotRegistered is ignored // and the noop gauge is returned; // all other errors cause a panic. -func (v *timingHistogramVec) WithLabelValues(lvs ...string) GaugeMetric { +func (v *TimingHistogramVec) WithLabelValues(lvs ...string) GaugeMetric { ans, err := v.WithLabelValuesChecked(lvs...) if err == nil || ErrIsNotRegistered(err) { return ans @@ -188,7 +196,7 @@ func (v *timingHistogramVec) WithLabelValues(lvs ...string) GaugeMetric { // return a noop gauge and an error about the labels. // If none of the above apply, this method will return // the appropriate vector member and a nil error. -func (v *timingHistogramVec) WithChecked(labels map[string]string) (GaugeMetric, error) { +func (v *TimingHistogramVec) WithChecked(labels map[string]string) (GaugeMetric, error) { if v.IsHidden() { return noop, nil } @@ -206,7 +214,7 @@ func (v *timingHistogramVec) WithChecked(labels map[string]string) (GaugeMetric, // An error that passes ErrIsNotRegistered is ignored // and the noop gauge is returned; // all other errors cause a panic. -func (v *timingHistogramVec) With(labels map[string]string) GaugeMetric { +func (v *TimingHistogramVec) With(labels map[string]string) GaugeMetric { ans, err := v.WithChecked(labels) if err == nil || ErrIsNotRegistered(err) { return ans @@ -221,7 +229,7 @@ func (v *timingHistogramVec) With(labels map[string]string) GaugeMetric { // with those of the VariableLabels in Desc. However, such inconsistent Labels // can never match an actual metric, so the method will always return false in // that case. -func (v *timingHistogramVec) Delete(labels map[string]string) bool { +func (v *TimingHistogramVec) Delete(labels map[string]string) bool { if !v.IsCreated() { return false // since we haven't created the metric, we haven't deleted a metric with the passed in values } @@ -229,7 +237,7 @@ func (v *timingHistogramVec) Delete(labels map[string]string) bool { } // Reset deletes all metrics in this vector. -func (v *timingHistogramVec) Reset() { +func (v *TimingHistogramVec) Reset() { if !v.IsCreated() { return } @@ -237,17 +245,17 @@ func (v *timingHistogramVec) Reset() { v.TimingHistogramVec.Reset() } -// WithContext returns wrapped timingHistogramVec with context -func (v *timingHistogramVec) WithContext(ctx context.Context) GaugeMetricVec { +// WithContext returns wrapped TimingHistogramVec with context +func (v *TimingHistogramVec) InterfaceWithContext(ctx context.Context) GaugeVecMetric { return &TimingHistogramVecWithContext{ ctx: ctx, - timingHistogramVec: v, + TimingHistogramVec: v, } } -// TimingHistogramVecWithContext is the wrapper of timingHistogramVec with context. +// TimingHistogramVecWithContext is the wrapper of TimingHistogramVec with context. // Currently the context is ignored. type TimingHistogramVecWithContext struct { - *timingHistogramVec + *TimingHistogramVec ctx context.Context } diff --git a/staging/src/k8s.io/component-base/metrics/wrappers.go b/staging/src/k8s.io/component-base/metrics/wrappers.go index 9ca4c8cb217..509d005b690 100644 --- a/staging/src/k8s.io/component-base/metrics/wrappers.go +++ b/staging/src/k8s.io/component-base/metrics/wrappers.go @@ -17,7 +17,6 @@ limitations under the License. package metrics import ( - "context" "errors" "github.com/prometheus/client_golang/prometheus" @@ -68,12 +67,18 @@ type GaugeMetric interface { SetToCurrentTime() } -// GaugeMetricVec is a collection of Gauges that differ only in label values. -// This is really just one Metric. -// It might be better called GaugeVecMetric, but that pattern of name is already -// taken by the other pattern --- which is treacherous. The treachery is that -// WithLabelValues can return an object that is permanently broken (i.e., a noop). -type GaugeMetricVec interface { +// GaugeVecMetric is a collection of Gauges that differ only in label values. +type GaugeVecMetric interface { + // Default Prometheus Vec behavior is that member extraction results in creation of a new element + // if one with the unique label values is not found in the underlying stored metricMap. + // This means that if this function is called but the underlying metric is not registered + // (which means it will never be exposed externally nor consumed), the metric would exist in memory + // for perpetuity (i.e. throughout application lifecycle). + // + // For reference: https://github.com/prometheus/client_golang/blob/v0.9.2/prometheus/gauge.go#L190-L208 + // + // In contrast, the Vec behavior in this package is that member extraction before registration + // returns a permanent noop object. // WithLabelValuesChecked, if called on a hidden vector, // will return a noop gauge and a nil error. @@ -120,26 +125,6 @@ type GaugeMetricVec interface { Reset() } -// PreContextGaugeMetricVec is something that can construct a GaugeMetricVec -// that uses a given Context. -type PreContextGaugeMetricVec interface { - // WithContext creates a GaugeMetricVec that uses the given Context - WithContext(ctx context.Context) GaugeMetricVec -} - -// RegisterableGaugeMetricVec is the intersection of Registerable and GaugeMetricVec -type RegisterableGaugeMetricVec interface { - Registerable - GaugeMetricVec -} - -// PreContextAndRegisterableGaugeMetricVec is the intersection of -// PreContextGaugeMetricVec and RegisterableGaugeMetricVec -type PreContextAndRegisterableGaugeMetricVec interface { - PreContextGaugeMetricVec - RegisterableGaugeMetricVec -} - // ObserverMetric captures individual observations. type ObserverMetric interface { Observe(float64) From d23254b7ea1bef8982aeef5039803b6987f4c144 Mon Sep 17 00:00:00 2001 From: Mike Spreitzer Date: Thu, 12 May 2022 10:27:58 -0400 Subject: [PATCH 5/5] More revisions after review --- .../k8s.io/component-base/metrics/gauge.go | 12 +++--- .../metrics/timing_histogram.go | 40 +++++++++++-------- .../metrics/timing_histogram_test.go | 4 +- .../k8s.io/component-base/metrics/wrappers.go | 12 +++--- 4 files changed, 37 insertions(+), 31 deletions(-) diff --git a/staging/src/k8s.io/component-base/metrics/gauge.go b/staging/src/k8s.io/component-base/metrics/gauge.go index dd2df34ac62..04041bab652 100644 --- a/staging/src/k8s.io/component-base/metrics/gauge.go +++ b/staging/src/k8s.io/component-base/metrics/gauge.go @@ -141,10 +141,10 @@ func (v *GaugeVec) initializeDeprecatedMetric() { } func (v *GaugeVec) WithLabelValuesChecked(lvs ...string) (GaugeMetric, error) { - if v.IsHidden() { - return noop, nil - } if !v.IsCreated() { + if v.IsHidden() { + return noop, nil + } return noop, errNotRegistered // return no-op gauge } if v.LabelValueAllowLists != nil { @@ -178,10 +178,10 @@ func (v *GaugeVec) WithLabelValues(lvs ...string) GaugeMetric { } func (v *GaugeVec) WithChecked(labels map[string]string) (GaugeMetric, error) { - if v.IsHidden() { - return noop, nil - } if !v.IsCreated() { + if v.IsHidden() { + return noop, nil + } return noop, errNotRegistered // return no-op gauge } if v.LabelValueAllowLists != nil { 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 8e54850185b..c015a04ea9e 100644 --- a/staging/src/k8s.io/component-base/metrics/timing_histogram.go +++ b/staging/src/k8s.io/component-base/metrics/timing_histogram.go @@ -24,10 +24,16 @@ import ( promext "k8s.io/component-base/metrics/prometheusextension" ) -// TimingHistogram is our internal representation for our wrapping struct around timing -// histograms. It implements both kubeCollector and GaugeMetric -type TimingHistogram struct { +// PrometheusTimingHistogram is the abstraction of the underlying histogram +// that we want to promote from the wrapper. +type PrometheusTimingHistogram interface { GaugeMetric +} + +// TimingHistogram is our internal representation for our wrapping struct around +// timing histograms. It implements both kubeCollector and GaugeMetric +type TimingHistogram struct { + PrometheusTimingHistogram *TimingHistogramOpts nowFunc func() time.Time lazyMetric @@ -60,7 +66,7 @@ func NewTestableTimingHistogram(nowFunc func() time.Time, opts *TimingHistogramO // setPrometheusHistogram sets the underlying KubeGauge object, i.e. the thing that does the measurement. func (h *TimingHistogram) setPrometheusHistogram(histogram promext.TimingHistogram) { - h.GaugeMetric = histogram + h.PrometheusTimingHistogram = histogram h.initSelfCollection(histogram) } @@ -90,7 +96,7 @@ func (h *TimingHistogram) initializeDeprecatedMetric() { // WithContext allows the normal TimingHistogram metric to pass in context. The context is no-op now. func (h *TimingHistogram) WithContext(ctx context.Context) GaugeMetric { - return h.GaugeMetric + return h.PrometheusTimingHistogram } // TimingHistogramVec is the internal representation of our wrapping struct around prometheus @@ -151,20 +157,20 @@ func (v *TimingHistogramVec) initializeDeprecatedMetric() { v.initializeMetric() } -// WithLabelValuesChecked, if called on a hidden vector, -// will return a noop gauge and a nil error. -// If called before this vector has been registered in +// WithLabelValuesChecked, if called before this vector has been registered in // at least one registry, will return a noop gauge and // an error that passes ErrIsNotRegistered. +// If called on a hidden vector, +// will return a noop gauge and a nil error. // If called with a syntactic problem in the labels, will // return a noop gauge and an error about the labels. // If none of the above apply, this method will return // the appropriate vector member and a nil error. func (v *TimingHistogramVec) WithLabelValuesChecked(lvs ...string) (GaugeMetric, error) { - if v.IsHidden() { - return noop, nil - } if !v.IsCreated() { + if v.IsHidden() { + return noop, nil + } return noop, errNotRegistered } if v.LabelValueAllowLists != nil { @@ -187,20 +193,20 @@ func (v *TimingHistogramVec) WithLabelValues(lvs ...string) GaugeMetric { panic(err) } -// WithChecked, if called on a hidden vector, -// will return a noop gauge and a nil error. -// If called before this vector has been registered in +// WithChecked, if called before this vector has been registered in // at least one registry, will return a noop gauge and // an error that passes ErrIsNotRegistered. +// If called on a hidden vector, +// will return a noop gauge and a nil error. // If called with a syntactic problem in the labels, will // return a noop gauge and an error about the labels. // If none of the above apply, this method will return // the appropriate vector member and a nil error. func (v *TimingHistogramVec) WithChecked(labels map[string]string) (GaugeMetric, error) { - if v.IsHidden() { - return noop, nil - } if !v.IsCreated() { + if v.IsHidden() { + return noop, nil + } return noop, errNotRegistered } if v.LabelValueAllowLists != nil { diff --git a/staging/src/k8s.io/component-base/metrics/timing_histogram_test.go b/staging/src/k8s.io/component-base/metrics/timing_histogram_test.go index b397ca44e34..60703d9cb8b 100644 --- a/staging/src/k8s.io/component-base/metrics/timing_histogram_test.go +++ b/staging/src/k8s.io/component-base/metrics/timing_histogram_test.go @@ -102,8 +102,8 @@ func TestTimingHistogram(t *testing.T) { }() m1 := <-metricChan gm1, ok := m1.(GaugeMetric) - if !ok || gm1 != c.GaugeMetric { - t.Error("Unexpected metric", m1, c.GaugeMetric) + if !ok || gm1 != c.PrometheusTimingHistogram { + t.Error("Unexpected metric", m1, c.PrometheusTimingHistogram) } m2, ok := <-metricChan if ok { diff --git a/staging/src/k8s.io/component-base/metrics/wrappers.go b/staging/src/k8s.io/component-base/metrics/wrappers.go index 509d005b690..06a8eec50cf 100644 --- a/staging/src/k8s.io/component-base/metrics/wrappers.go +++ b/staging/src/k8s.io/component-base/metrics/wrappers.go @@ -80,11 +80,11 @@ type GaugeVecMetric interface { // In contrast, the Vec behavior in this package is that member extraction before registration // returns a permanent noop object. - // WithLabelValuesChecked, if called on a hidden vector, - // will return a noop gauge and a nil error. - // If called before this vector has been registered in + // WithLabelValuesChecked, if called before this vector has been registered in // at least one registry, will return a noop gauge and // an error that passes ErrIsNotRegistered. + // If called on a hidden vector, + // will return a noop gauge and a nil error. // If called with a syntactic problem in the labels, will // return a noop gauge and an error about the labels. // If none of the above apply, this method will return @@ -98,11 +98,11 @@ type GaugeVecMetric interface { // all other errors cause a panic. WithLabelValues(labelValues ...string) GaugeMetric - // WithChecked, if called on a hidden vector, - // will return a noop gauge and a nil error. - // If called before this vector has been registered in + // WithChecked, if called before this vector has been registered in // at least one registry, will return a noop gauge and // an error that passes ErrIsNotRegistered. + // If called on a hidden vector, + // will return a noop gauge and a nil error. // If called with a syntactic problem in the labels, will // return a noop gauge and an error about the labels. // If none of the above apply, this method will return