From e1e022f54841f897c4afb3ef00b9b13780543e63 Mon Sep 17 00:00:00 2001 From: Mike Spreitzer Date: Thu, 10 Sep 2020 01:17:01 -0400 Subject: [PATCH 1/7] Start drafting SamplingHistogram This covers SamplingHistogram but not SamplingHistogramVec (because the vec support of prometheus is not public). --- .../sampling-histogram.go | 120 ++++++++++++++++++ .../sampling-histogram_test.go | 120 ++++++++++++++++++ 2 files changed, 240 insertions(+) create mode 100644 staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram.go create mode 100644 staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram_test.go diff --git a/staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram.go b/staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram.go new file mode 100644 index 000000000000..ce5b3e8459ca --- /dev/null +++ b/staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram.go @@ -0,0 +1,120 @@ +/* +Copyright 2020 Mike Spreitzer. + +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 prometheus_extension + +import ( + "sync" + "time" + + "github.com/prometheus/client_golang/prometheus" + dto "github.com/prometheus/client_model/go" + "k8s.io/utils/clock" +) + +// A SamplingHistogram samples the values of a float64 variable at a +// configured rate. The samples contribute to a Histogram. +type SamplingHistogram interface { + prometheus.Metric + prometheus.Collector + + // Sets the variable to the given value. + Set(float64) +} + +type SamplingHistogramOpts struct { + prometheus.HistogramOpts + + // The initial value of the variable + InitialValue float64 + + // The variable is sampled once every this often + SamplingPeriod time.Duration +} + +func NewSamplingHistogram(opts SamplingHistogramOpts) SamplingHistogram { + return NewTestableSamplingHistogram(clock.RealClock{}, opts) +} + +func NewTestableSamplingHistogram(clock clock.Clock, opts SamplingHistogramOpts) SamplingHistogram { + desc := prometheus.NewDesc( + prometheus.BuildFQName(opts.Namespace, opts.Subsystem, opts.Name), + opts.Help, + nil, + opts.ConstLabels, + ) + return newSamplingHistogram(clock, desc, opts) +} + +func newSamplingHistogram(clock clock.Clock, desc *prometheus.Desc, opts SamplingHistogramOpts, labelValues ...string) SamplingHistogram { + return &samplingHistogram{ + samplingPeriod: opts.SamplingPeriod, + histogram: prometheus.NewHistogram(opts.HistogramOpts), + clock: clock, + lastSampleIndex: clock.Now().UnixNano() / int64(opts.SamplingPeriod), + value: opts.InitialValue, + } +} + +type samplingHistogram struct { + samplingPeriod time.Duration + histogram prometheus.Histogram + clock clock.Clock + lock sync.Mutex + + // identifies the last sampling period completed + lastSampleIndex int64 + value float64 +} + +var _ SamplingHistogram = &samplingHistogram{} + +func (sh *samplingHistogram) Set(newValue float64) { + sh.Update(func(float64) float64 { return newValue }) +} + +func (sh *samplingHistogram) Update(updateFn func(float64) float64) { + oldValue, numSamples := func() (float64, int64) { + sh.lock.Lock() + defer sh.lock.Unlock() + newSampleIndex := sh.clock.Now().UnixNano() / int64(sh.samplingPeriod) + deltaIndex := newSampleIndex - sh.lastSampleIndex + sh.lastSampleIndex = newSampleIndex + oldValue := sh.value + sh.value = updateFn(sh.value) + return oldValue, deltaIndex + }() + for i := int64(0); i < numSamples; i++ { + sh.histogram.Observe(oldValue) + } +} + +func (sh *samplingHistogram) Desc() *prometheus.Desc { + return sh.histogram.Desc() +} + +func (sh *samplingHistogram) Write(dest *dto.Metric) error { + return sh.histogram.Write(dest) +} + +func (sh *samplingHistogram) Describe(ch chan<- *prometheus.Desc) { + sh.histogram.Describe(ch) +} + +func (sh *samplingHistogram) Collect(ch chan<- prometheus.Metric) { + sh.Update(func(value float64) float64 { return value }) + sh.histogram.Collect(ch) +} diff --git a/staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram_test.go b/staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram_test.go new file mode 100644 index 000000000000..76150225c77c --- /dev/null +++ b/staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram_test.go @@ -0,0 +1,120 @@ +/* +Copyright 2020 Mike Spreitzer. + +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 prometheus_extension + +import ( + "testing" + "time" + + "github.com/prometheus/client_golang/prometheus" + dto "github.com/prometheus/client_model/go" + testclock "k8s.io/utils/clock/testing" +) + +func TestSamplingHistogram(t *testing.T) { + clk := testclock.NewFakeClock(time.Unix(time.Now().Unix(), 999999990)) + sh := NewTestableSamplingHistogram(clk, SamplingHistogramOpts{ + HistogramOpts: prometheus.HistogramOpts{ + Namespace: "test", + Subsystem: "func", + Name: "one", + Help: "a helpful string", + ConstLabels: map[string]string{"l1": "v1", "l2": "v2"}, + Buckets: []float64{0, 1, 2}, + }, + InitialValue: 1, + SamplingPeriod: time.Second, + }) + expectHistogram(t, "After creation", sh, 0, 0, 0, 0) + sh.Set(2) + expectHistogram(t, "After initial Set", sh, 0, 0, 0, 0) + clk.Step(9 * time.Nanosecond) + expectHistogram(t, "Just before the end of the first sampling period", sh, 0, 0, 0, 0) + clk.Step(1 * time.Nanosecond) + expectHistogram(t, "At the end of the first sampling period", sh, 0, 0, 1, 1) + clk.Step(1 * time.Nanosecond) + expectHistogram(t, "Barely into second sampling period", sh, 0, 0, 1, 1) + sh.Set(-0.5) + sh.Set(0.5) + clk.Step(999999998 * time.Nanosecond) + expectHistogram(t, "Just before the end of second sampling period", sh, 0, 0, 1, 1) + clk.Step(1 * time.Nanosecond) + expectHistogram(t, "At the end of second sampling period", sh, 0, 1, 2, 2) +} + +func expectHistogram(t *testing.T, when string, sh SamplingHistogram, buckets ...uint64) { + metrics := make(chan prometheus.Metric, 2) + sh.Collect(metrics) + var dtom dto.Metric + select { + case metric := <-metrics: + metric.Write(&dtom) + default: + t.Errorf("%s, zero Metrics collected", when) + } + select { + case metric := <-metrics: + t.Errorf("%s, collected more than one Metric; second Metric = %#+v", when, metric) + default: + } + missingLabels := map[string]string{"l1": "v1", "l2": "v2"} + for _, lp := range dtom.Label { + if lp == nil || lp.Name == nil || lp.Value == nil { + continue + } + if val, ok := missingLabels[*(lp.Name)]; ok { + if val != *(lp.Value) { + t.Errorf("%s, found label named %q with value %q instead of %q", when, *(lp.Name), *(lp.Value), val) + } + } else { + t.Errorf("%s, got unexpected label name %q", when, *(lp.Name)) + } + delete(missingLabels, *(lp.Name)) + } + if len(missingLabels) > 0 { + t.Errorf("%s, missed labels %#+v", when, missingLabels) + } + if dtom.Histogram == nil { + t.Errorf("%s, Collect returned a Metric without a Histogram: %#+v", when, dtom) + } + mh := dtom.Histogram + if len(mh.Bucket) != len(buckets)-1 { + t.Errorf("%s, expected %d buckets but got %d: %#+v", when, len(buckets)-1, len(mh.Bucket), mh.Bucket) + } + if mh.SampleCount == nil { + t.Errorf("%s, got Histogram with nil SampleCount", when) + } + if *(mh.SampleCount) != buckets[len(mh.Bucket)] { + t.Errorf("%s, SampleCount=%d but expected %d", when, *(mh.SampleCount), buckets[len(mh.Bucket)]) + } + for i, mhBucket := range mh.Bucket { + if mhBucket == nil { + t.Errorf("%s, bucket %d was nil", when, i) + } + if mhBucket.UpperBound == nil || mhBucket.CumulativeCount == nil { + t.Errorf("%s, bucket %d had nil bound or count", when, i) + } + ub := float64(i) + if ub != *(mhBucket.UpperBound) { + t.Errorf("%s, bucket %d's upper bound was %v", when, i, *(mhBucket.UpperBound)) + } + expectedCount := buckets[i] + if expectedCount != *(mhBucket.CumulativeCount) { + t.Errorf("%s, bucket %d's count was %d rather than %d", when, i, mhBucket.CumulativeCount, expectedCount) + } + } +} From 93ef341029310a5b3bf743a051cef3ada5fdd88e Mon Sep 17 00:00:00 2001 From: Mike Spreitzer Date: Thu, 10 Sep 2020 02:24:13 -0400 Subject: [PATCH 2/7] Added wrapper for SamplingHistogram --- .../src/k8s.io/component-base/metrics/opts.go | 52 +++++++++++++ .../sampling-histogram.go | 11 ++- .../sampling-histogram_test.go | 2 +- .../metrics/sampling-histogram.go | 75 +++++++++++++++++++ .../k8s.io/component-base/metrics/wrappers.go | 9 ++- 5 files changed, 144 insertions(+), 5 deletions(-) create mode 100644 staging/src/k8s.io/component-base/metrics/sampling-histogram.go diff --git a/staging/src/k8s.io/component-base/metrics/opts.go b/staging/src/k8s.io/component-base/metrics/opts.go index 906050c7f78a..f61f8d0d9479 100644 --- a/staging/src/k8s.io/component-base/metrics/opts.go +++ b/staging/src/k8s.io/component-base/metrics/opts.go @@ -22,6 +22,7 @@ import ( "time" "github.com/prometheus/client_golang/prometheus" + promext "k8s.io/component-base/metrics/prometheus_extension" ) // KubeOpts is superset struct for prometheus.Opts. The prometheus Opts structure @@ -180,6 +181,57 @@ func (o *HistogramOpts) toPromHistogramOpts() prometheus.HistogramOpts { } } +// SamplingHistogramOpts bundles the options for creating a SamplingHistogram 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 SamplingHistogramOpts struct { + Namespace string + Subsystem string + Name string + Help string + ConstLabels map[string]string + Buckets []float64 + InitialValue float64 + SamplingPeriod time.Duration + DeprecatedVersion string + deprecateOnce sync.Once + annotateOnce sync.Once + StabilityLevel StabilityLevel +} + +// Modify help description on the metric description. +func (o *SamplingHistogramOpts) 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 *SamplingHistogramOpts) 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 *SamplingHistogramOpts) toPromSamplingHistogramOpts() promext.SamplingHistogramOpts { + return promext.SamplingHistogramOpts{ + HistogramOpts: prometheus.HistogramOpts{ + Namespace: o.Namespace, + Subsystem: o.Subsystem, + Name: o.Name, + Help: o.Help, + ConstLabels: o.ConstLabels, + Buckets: o.Buckets, + }, + InitialValue: o.InitialValue, + SamplingPeriod: o.SamplingPeriod, + } +} + // 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/prometheus_extension/sampling-histogram.go b/staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram.go index ce5b3e8459ca..518547b10e02 100644 --- a/staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram.go +++ b/staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram.go @@ -31,8 +31,11 @@ type SamplingHistogram interface { prometheus.Metric prometheus.Collector - // Sets the variable to the given value. + // Set the variable to the given value. Set(float64) + + // Add the given change to the variable + Add(float64) } type SamplingHistogramOpts struct { @@ -86,6 +89,10 @@ func (sh *samplingHistogram) Set(newValue float64) { sh.Update(func(float64) float64 { return newValue }) } +func (sh *samplingHistogram) Add(delta float64) { + sh.Update(func(oldValue float64) float64 { return oldValue + delta }) +} + func (sh *samplingHistogram) Update(updateFn func(float64) float64) { oldValue, numSamples := func() (float64, int64) { sh.lock.Lock() @@ -115,6 +122,6 @@ func (sh *samplingHistogram) Describe(ch chan<- *prometheus.Desc) { } func (sh *samplingHistogram) Collect(ch chan<- prometheus.Metric) { - sh.Update(func(value float64) float64 { return value }) + sh.Add(0) sh.histogram.Collect(ch) } diff --git a/staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram_test.go b/staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram_test.go index 76150225c77c..71e25d0466e1 100644 --- a/staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram_test.go +++ b/staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram_test.go @@ -49,7 +49,7 @@ func TestSamplingHistogram(t *testing.T) { clk.Step(1 * time.Nanosecond) expectHistogram(t, "Barely into second sampling period", sh, 0, 0, 1, 1) sh.Set(-0.5) - sh.Set(0.5) + sh.Add(1) clk.Step(999999998 * time.Nanosecond) expectHistogram(t, "Just before the end of second sampling period", sh, 0, 0, 1, 1) clk.Step(1 * time.Nanosecond) diff --git a/staging/src/k8s.io/component-base/metrics/sampling-histogram.go b/staging/src/k8s.io/component-base/metrics/sampling-histogram.go new file mode 100644 index 000000000000..42f16590e89d --- /dev/null +++ b/staging/src/k8s.io/component-base/metrics/sampling-histogram.go @@ -0,0 +1,75 @@ +/* +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 ( + "github.com/blang/semver" + + promext "k8s.io/component-base/metrics/prometheus_extension" +) + +// SamplingHistogram is our internal representation for our wrapping struct around prometheus +// sampling histograms. SamplingHistogram implements both kubeCollector and SimpleGaugeMetric +type SamplingHistogram struct { + SimpleGaugeMetric + *SamplingHistogramOpts + lazyMetric + selfCollector +} + +var _ kubeCollector = &SamplingHistogram{} +var _ SimpleGaugeMetric = &SamplingHistogram{} + +// NewSamplingHistogram returns an object which is SamplingHistogram-like. However, nothing +// will be measured until the histogram is registered somewhere. +func NewSamplingHistogram(opts *SamplingHistogramOpts) *SamplingHistogram { + opts.StabilityLevel.setDefaults() + + h := &SamplingHistogram{ + SamplingHistogramOpts: opts, + lazyMetric: lazyMetric{}, + } + h.setPrometheusSamplingHistogram(noopMetric{}) + h.lazyInit(h, BuildFQName(opts.Namespace, opts.Subsystem, opts.Name)) + return h +} + +// setPrometheusSamplingHistogram sets the underlying KubeGauge object, i.e. the thing that does the measurement. +func (h *SamplingHistogram) setPrometheusSamplingHistogram(histogram promext.SamplingHistogram) { + h.SimpleGaugeMetric = histogram + h.initSelfCollection(histogram) +} + +// DeprecatedVersion returns a pointer to the Version or nil +func (h *SamplingHistogram) DeprecatedVersion() *semver.Version { + return parseSemver(h.SamplingHistogramOpts.DeprecatedVersion) +} + +// initializeMetric invokes the actual prometheus.SamplingHistogram object instantiation +// and stores a reference to it +func (h *SamplingHistogram) initializeMetric() { + h.SamplingHistogramOpts.annotateStabilityLevel() + // this actually creates the underlying prometheus gauge. + h.setPrometheusSamplingHistogram(promext.NewSamplingHistogram(h.SamplingHistogramOpts.toPromSamplingHistogramOpts())) +} + +// initializeDeprecatedMetric invokes the actual prometheus.SamplingHistogram object instantiation +// but modifies the Help description prior to object instantiation. +func (h *SamplingHistogram) initializeDeprecatedMetric() { + h.SamplingHistogramOpts.markDeprecated() + h.initializeMetric() +} diff --git a/staging/src/k8s.io/component-base/metrics/wrappers.go b/staging/src/k8s.io/component-base/metrics/wrappers.go index 6ae8a458acb7..00d12515642c 100644 --- a/staging/src/k8s.io/component-base/metrics/wrappers.go +++ b/staging/src/k8s.io/component-base/metrics/wrappers.go @@ -55,12 +55,17 @@ type CounterVecMetric interface { With(prometheus.Labels) CounterMetric } +// SimpleGaugeMetric is an interface which defines a small subset of the interface provided by prometheus.Gauge +type SimpleGaugeMetric interface { + Set(float64) + Add(float64) +} + // GaugeMetric is an interface which defines a subset of the interface provided by prometheus.Gauge type GaugeMetric interface { - Set(float64) + SimpleGaugeMetric Inc() Dec() - Add(float64) Write(out *dto.Metric) error SetToCurrentTime() } From c33889941fea720df11ea5ca5ecc5697702d9e27 Mon Sep 17 00:00:00 2001 From: Mike Spreitzer Date: Thu, 10 Sep 2020 03:10:05 -0400 Subject: [PATCH 3/7] update bazel and vendor/modules.txt --- .../src/k8s.io/component-base/metrics/BUILD | 3 ++ .../metrics/prometheus_extension/BUILD | 39 +++++++++++++++++++ vendor/modules.txt | 1 + 3 files changed, 43 insertions(+) create mode 100644 staging/src/k8s.io/component-base/metrics/prometheus_extension/BUILD diff --git a/staging/src/k8s.io/component-base/metrics/BUILD b/staging/src/k8s.io/component-base/metrics/BUILD index b75763c4c1bd..31055f227b52 100644 --- a/staging/src/k8s.io/component-base/metrics/BUILD +++ b/staging/src/k8s.io/component-base/metrics/BUILD @@ -15,6 +15,7 @@ go_library( "opts.go", "processstarttime.go", "registry.go", + "sampling-histogram.go", "summary.go", "value.go", "version.go", @@ -26,6 +27,7 @@ go_library( visibility = ["//visibility:public"], deps = [ "//staging/src/k8s.io/apimachinery/pkg/version:go_default_library", + "//staging/src/k8s.io/component-base/metrics/prometheus_extension:go_default_library", "//staging/src/k8s.io/component-base/version:go_default_library", "//vendor/github.com/blang/semver:go_default_library", "//vendor/github.com/prometheus/client_golang/prometheus:go_default_library", @@ -79,6 +81,7 @@ filegroup( "//staging/src/k8s.io/component-base/metrics/prometheus/restclient:all-srcs", "//staging/src/k8s.io/component-base/metrics/prometheus/version:all-srcs", "//staging/src/k8s.io/component-base/metrics/prometheus/workqueue:all-srcs", + "//staging/src/k8s.io/component-base/metrics/prometheus_extension:all-srcs", "//staging/src/k8s.io/component-base/metrics/testutil:all-srcs", ], tags = ["automanaged"], diff --git a/staging/src/k8s.io/component-base/metrics/prometheus_extension/BUILD b/staging/src/k8s.io/component-base/metrics/prometheus_extension/BUILD new file mode 100644 index 000000000000..b07ce054c3c2 --- /dev/null +++ b/staging/src/k8s.io/component-base/metrics/prometheus_extension/BUILD @@ -0,0 +1,39 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["sampling-histogram.go"], + importmap = "k8s.io/kubernetes/vendor/k8s.io/component-base/metrics/prometheus_extension", + importpath = "k8s.io/component-base/metrics/prometheus_extension", + visibility = ["//visibility:public"], + deps = [ + "//vendor/github.com/prometheus/client_golang/prometheus:go_default_library", + "//vendor/github.com/prometheus/client_model/go:go_default_library", + "//vendor/k8s.io/utils/clock:go_default_library", + ], +) + +go_test( + name = "go_default_test", + srcs = ["sampling-histogram_test.go"], + embed = [":go_default_library"], + deps = [ + "//vendor/github.com/prometheus/client_golang/prometheus:go_default_library", + "//vendor/github.com/prometheus/client_model/go:go_default_library", + "//vendor/k8s.io/utils/clock/testing:go_default_library", + ], +) + +filegroup( + name = "package-srcs", + srcs = glob(["**"]), + tags = ["automanaged"], + visibility = ["//visibility:private"], +) + +filegroup( + name = "all-srcs", + srcs = [":package-srcs"], + tags = ["automanaged"], + visibility = ["//visibility:public"], +) diff --git a/vendor/modules.txt b/vendor/modules.txt index ac972aa186a3..ed4527e0fc99 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -2171,6 +2171,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/prometheus_extension k8s.io/component-base/metrics/testutil k8s.io/component-base/term k8s.io/component-base/version From 32c82c10c0998c2ce0cb26d71ac7ef940cb23594 Mon Sep 17 00:00:00 2001 From: Mike Spreitzer Date: Fri, 11 Sep 2020 02:14:03 -0400 Subject: [PATCH 4/7] Fixed lint, constrained SamplingPeriod > 0 --- .../src/k8s.io/component-base/metrics/BUILD | 4 ++-- .../src/k8s.io/component-base/metrics/opts.go | 2 +- .../BUILD | 4 ++-- .../sampling-histogram.go | 24 +++++++++++++------ .../sampling-histogram_test.go | 7 ++++-- .../metrics/sampling-histogram.go | 2 +- vendor/modules.txt | 2 +- 7 files changed, 29 insertions(+), 16 deletions(-) rename staging/src/k8s.io/component-base/metrics/{prometheus_extension => prometheusextension}/BUILD (91%) rename staging/src/k8s.io/component-base/metrics/{prometheus_extension => prometheusextension}/sampling-histogram.go (79%) rename staging/src/k8s.io/component-base/metrics/{prometheus_extension => prometheusextension}/sampling-histogram_test.go (95%) diff --git a/staging/src/k8s.io/component-base/metrics/BUILD b/staging/src/k8s.io/component-base/metrics/BUILD index 31055f227b52..27522aae83f8 100644 --- a/staging/src/k8s.io/component-base/metrics/BUILD +++ b/staging/src/k8s.io/component-base/metrics/BUILD @@ -27,7 +27,7 @@ go_library( visibility = ["//visibility:public"], deps = [ "//staging/src/k8s.io/apimachinery/pkg/version:go_default_library", - "//staging/src/k8s.io/component-base/metrics/prometheus_extension:go_default_library", + "//staging/src/k8s.io/component-base/metrics/prometheusextension:go_default_library", "//staging/src/k8s.io/component-base/version:go_default_library", "//vendor/github.com/blang/semver:go_default_library", "//vendor/github.com/prometheus/client_golang/prometheus:go_default_library", @@ -81,7 +81,7 @@ filegroup( "//staging/src/k8s.io/component-base/metrics/prometheus/restclient:all-srcs", "//staging/src/k8s.io/component-base/metrics/prometheus/version:all-srcs", "//staging/src/k8s.io/component-base/metrics/prometheus/workqueue:all-srcs", - "//staging/src/k8s.io/component-base/metrics/prometheus_extension:all-srcs", + "//staging/src/k8s.io/component-base/metrics/prometheusextension:all-srcs", "//staging/src/k8s.io/component-base/metrics/testutil:all-srcs", ], tags = ["automanaged"], diff --git a/staging/src/k8s.io/component-base/metrics/opts.go b/staging/src/k8s.io/component-base/metrics/opts.go index f61f8d0d9479..f77c4788f869 100644 --- a/staging/src/k8s.io/component-base/metrics/opts.go +++ b/staging/src/k8s.io/component-base/metrics/opts.go @@ -22,7 +22,7 @@ import ( "time" "github.com/prometheus/client_golang/prometheus" - promext "k8s.io/component-base/metrics/prometheus_extension" + promext "k8s.io/component-base/metrics/prometheusextension" ) // KubeOpts is superset struct for prometheus.Opts. The prometheus Opts structure diff --git a/staging/src/k8s.io/component-base/metrics/prometheus_extension/BUILD b/staging/src/k8s.io/component-base/metrics/prometheusextension/BUILD similarity index 91% rename from staging/src/k8s.io/component-base/metrics/prometheus_extension/BUILD rename to staging/src/k8s.io/component-base/metrics/prometheusextension/BUILD index b07ce054c3c2..7d515560cf32 100644 --- a/staging/src/k8s.io/component-base/metrics/prometheus_extension/BUILD +++ b/staging/src/k8s.io/component-base/metrics/prometheusextension/BUILD @@ -3,8 +3,8 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", srcs = ["sampling-histogram.go"], - importmap = "k8s.io/kubernetes/vendor/k8s.io/component-base/metrics/prometheus_extension", - importpath = "k8s.io/component-base/metrics/prometheus_extension", + importmap = "k8s.io/kubernetes/vendor/k8s.io/component-base/metrics/prometheusextension", + importpath = "k8s.io/component-base/metrics/prometheusextension", visibility = ["//visibility:public"], deps = [ "//vendor/github.com/prometheus/client_golang/prometheus:go_default_library", diff --git a/staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram.go b/staging/src/k8s.io/component-base/metrics/prometheusextension/sampling-histogram.go similarity index 79% rename from staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram.go rename to staging/src/k8s.io/component-base/metrics/prometheusextension/sampling-histogram.go index 518547b10e02..5d590ac88472 100644 --- a/staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram.go +++ b/staging/src/k8s.io/component-base/metrics/prometheusextension/sampling-histogram.go @@ -14,9 +14,10 @@ See the License for the specific language governing permissions and limitations under the License. */ -package prometheus_extension +package prometheusextension import ( + "fmt" "sync" "time" @@ -38,21 +39,27 @@ type SamplingHistogram interface { Add(float64) } +// SamplingHistogramOpts bundles the options for creating a +// SamplingHistogram metric. This builds on the options for creating +// a Histogram metric. type SamplingHistogramOpts struct { prometheus.HistogramOpts - // The initial value of the variable + // The initial value of the variable. InitialValue float64 - // The variable is sampled once every this often + // The variable is sampled once every this often. + // Must be set to a positive value. SamplingPeriod time.Duration } -func NewSamplingHistogram(opts SamplingHistogramOpts) SamplingHistogram { +// NewSamplingHistogram creates a new SamplingHistogram +func NewSamplingHistogram(opts SamplingHistogramOpts) (SamplingHistogram, error) { return NewTestableSamplingHistogram(clock.RealClock{}, opts) } -func NewTestableSamplingHistogram(clock clock.Clock, opts SamplingHistogramOpts) SamplingHistogram { +// NewTestableSamplingHistogram creates a SamplingHistogram that uses a mockable clock +func NewTestableSamplingHistogram(clock clock.Clock, opts SamplingHistogramOpts) (SamplingHistogram, error) { desc := prometheus.NewDesc( prometheus.BuildFQName(opts.Namespace, opts.Subsystem, opts.Name), opts.Help, @@ -62,14 +69,17 @@ func NewTestableSamplingHistogram(clock clock.Clock, opts SamplingHistogramOpts) return newSamplingHistogram(clock, desc, opts) } -func newSamplingHistogram(clock clock.Clock, desc *prometheus.Desc, opts SamplingHistogramOpts, labelValues ...string) SamplingHistogram { +func newSamplingHistogram(clock clock.Clock, desc *prometheus.Desc, opts SamplingHistogramOpts, labelValues ...string) (SamplingHistogram, error) { + if opts.SamplingPeriod <= 0 { + return nil, fmt.Errorf("the given sampling period was %v but must be positive", opts.SamplingPeriod) + } return &samplingHistogram{ samplingPeriod: opts.SamplingPeriod, histogram: prometheus.NewHistogram(opts.HistogramOpts), clock: clock, lastSampleIndex: clock.Now().UnixNano() / int64(opts.SamplingPeriod), value: opts.InitialValue, - } + }, nil } type samplingHistogram struct { diff --git a/staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram_test.go b/staging/src/k8s.io/component-base/metrics/prometheusextension/sampling-histogram_test.go similarity index 95% rename from staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram_test.go rename to staging/src/k8s.io/component-base/metrics/prometheusextension/sampling-histogram_test.go index 71e25d0466e1..c27765f0b15a 100644 --- a/staging/src/k8s.io/component-base/metrics/prometheus_extension/sampling-histogram_test.go +++ b/staging/src/k8s.io/component-base/metrics/prometheusextension/sampling-histogram_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package prometheus_extension +package prometheusextension import ( "testing" @@ -27,7 +27,7 @@ import ( func TestSamplingHistogram(t *testing.T) { clk := testclock.NewFakeClock(time.Unix(time.Now().Unix(), 999999990)) - sh := NewTestableSamplingHistogram(clk, SamplingHistogramOpts{ + sh, err := NewTestableSamplingHistogram(clk, SamplingHistogramOpts{ HistogramOpts: prometheus.HistogramOpts{ Namespace: "test", Subsystem: "func", @@ -39,6 +39,9 @@ func TestSamplingHistogram(t *testing.T) { InitialValue: 1, SamplingPeriod: time.Second, }) + if sh == nil || err != nil { + t.Errorf("Creation failed; err=%s", err.Error()) + } expectHistogram(t, "After creation", sh, 0, 0, 0, 0) sh.Set(2) expectHistogram(t, "After initial Set", sh, 0, 0, 0, 0) diff --git a/staging/src/k8s.io/component-base/metrics/sampling-histogram.go b/staging/src/k8s.io/component-base/metrics/sampling-histogram.go index 42f16590e89d..1492838a4460 100644 --- a/staging/src/k8s.io/component-base/metrics/sampling-histogram.go +++ b/staging/src/k8s.io/component-base/metrics/sampling-histogram.go @@ -19,7 +19,7 @@ package metrics import ( "github.com/blang/semver" - promext "k8s.io/component-base/metrics/prometheus_extension" + promext "k8s.io/component-base/metrics/prometheusextension" ) // SamplingHistogram is our internal representation for our wrapping struct around prometheus diff --git a/vendor/modules.txt b/vendor/modules.txt index ed4527e0fc99..1bfdf41d2441 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -2171,7 +2171,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/prometheus_extension +k8s.io/component-base/metrics/prometheusextension k8s.io/component-base/metrics/testutil k8s.io/component-base/term k8s.io/component-base/version From 64943ed47eef5430bd14f9d70bf3401a5cd43fac Mon Sep 17 00:00:00 2001 From: Mike Spreitzer Date: Fri, 11 Sep 2020 03:56:29 -0400 Subject: [PATCH 5/7] fix wrapper --- .../src/k8s.io/component-base/metrics/sampling-histogram.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/staging/src/k8s.io/component-base/metrics/sampling-histogram.go b/staging/src/k8s.io/component-base/metrics/sampling-histogram.go index 1492838a4460..e699eec61cb6 100644 --- a/staging/src/k8s.io/component-base/metrics/sampling-histogram.go +++ b/staging/src/k8s.io/component-base/metrics/sampling-histogram.go @@ -64,7 +64,8 @@ func (h *SamplingHistogram) DeprecatedVersion() *semver.Version { func (h *SamplingHistogram) initializeMetric() { h.SamplingHistogramOpts.annotateStabilityLevel() // this actually creates the underlying prometheus gauge. - h.setPrometheusSamplingHistogram(promext.NewSamplingHistogram(h.SamplingHistogramOpts.toPromSamplingHistogramOpts())) + sh, _ := promext.NewSamplingHistogram(h.SamplingHistogramOpts.toPromSamplingHistogramOpts()) + h.setPrometheusSamplingHistogram(sh) } // initializeDeprecatedMetric invokes the actual prometheus.SamplingHistogram object instantiation From 0e0bcd9c4f3fa1c7166f65cb1f084cfa6a9d1185 Mon Sep 17 00:00:00 2001 From: Mike Spreitzer Date: Fri, 11 Sep 2020 22:03:55 -0400 Subject: [PATCH 6/7] Add hack to get k8s.io/utils/clock/testing in vendor/ As per https://github.com/kubernetes/kubernetes/pull/84205/#issuecomment-660362689 --- build/tools.go | 1 + vendor/k8s.io/utils/clock/BUILD | 5 +- vendor/k8s.io/utils/clock/testing/BUILD | 24 ++ .../k8s.io/utils/clock/testing/fake_clock.go | 274 ++++++++++++++++++ vendor/modules.txt | 1 + 5 files changed, 304 insertions(+), 1 deletion(-) create mode 100644 vendor/k8s.io/utils/clock/testing/BUILD create mode 100644 vendor/k8s.io/utils/clock/testing/fake_clock.go diff --git a/build/tools.go b/build/tools.go index 33958d7bfba6..71799c1d04e1 100644 --- a/build/tools.go +++ b/build/tools.go @@ -29,4 +29,5 @@ import ( _ "k8s.io/gengo/examples/import-boss/generators" _ "k8s.io/gengo/examples/set-gen/generators" _ "k8s.io/kube-openapi/cmd/openapi-gen" + _ "k8s.io/utils/clock/testing" ) diff --git a/vendor/k8s.io/utils/clock/BUILD b/vendor/k8s.io/utils/clock/BUILD index 1b2e304c37f3..aa2c1799dc19 100644 --- a/vendor/k8s.io/utils/clock/BUILD +++ b/vendor/k8s.io/utils/clock/BUILD @@ -17,7 +17,10 @@ filegroup( filegroup( name = "all-srcs", - srcs = [":package-srcs"], + srcs = [ + ":package-srcs", + "//vendor/k8s.io/utils/clock/testing:all-srcs", + ], tags = ["automanaged"], visibility = ["//visibility:public"], ) diff --git a/vendor/k8s.io/utils/clock/testing/BUILD b/vendor/k8s.io/utils/clock/testing/BUILD new file mode 100644 index 000000000000..6a254d8010ed --- /dev/null +++ b/vendor/k8s.io/utils/clock/testing/BUILD @@ -0,0 +1,24 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library") + +go_library( + name = "go_default_library", + srcs = ["fake_clock.go"], + importmap = "k8s.io/kubernetes/vendor/k8s.io/utils/clock/testing", + importpath = "k8s.io/utils/clock/testing", + visibility = ["//visibility:public"], + deps = ["//vendor/k8s.io/utils/clock:go_default_library"], +) + +filegroup( + name = "package-srcs", + srcs = glob(["**"]), + tags = ["automanaged"], + visibility = ["//visibility:private"], +) + +filegroup( + name = "all-srcs", + srcs = [":package-srcs"], + tags = ["automanaged"], + visibility = ["//visibility:public"], +) diff --git a/vendor/k8s.io/utils/clock/testing/fake_clock.go b/vendor/k8s.io/utils/clock/testing/fake_clock.go new file mode 100644 index 000000000000..2fc34aed6db5 --- /dev/null +++ b/vendor/k8s.io/utils/clock/testing/fake_clock.go @@ -0,0 +1,274 @@ +/* +Copyright 2014 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 testing + +import ( + "sync" + "time" + + "k8s.io/utils/clock" +) + +var ( + _ = clock.Clock(&FakeClock{}) + _ = clock.Clock(&IntervalClock{}) +) + +// FakeClock implements clock.Clock, but returns an arbitrary time. +type FakeClock struct { + lock sync.RWMutex + time time.Time + + // waiters are waiting for the fake time to pass their specified time + waiters []*fakeClockWaiter +} + +type fakeClockWaiter struct { + targetTime time.Time + stepInterval time.Duration + skipIfBlocked bool + destChan chan time.Time + fired bool +} + +// NewFakeClock constructs a fake clock set to the provided time. +func NewFakeClock(t time.Time) *FakeClock { + return &FakeClock{ + time: t, + } +} + +// Now returns f's time. +func (f *FakeClock) Now() time.Time { + f.lock.RLock() + defer f.lock.RUnlock() + return f.time +} + +// Since returns time since the time in f. +func (f *FakeClock) Since(ts time.Time) time.Duration { + f.lock.RLock() + defer f.lock.RUnlock() + return f.time.Sub(ts) +} + +// After is the fake version of time.After(d). +func (f *FakeClock) After(d time.Duration) <-chan time.Time { + f.lock.Lock() + defer f.lock.Unlock() + stopTime := f.time.Add(d) + ch := make(chan time.Time, 1) // Don't block! + f.waiters = append(f.waiters, &fakeClockWaiter{ + targetTime: stopTime, + destChan: ch, + }) + return ch +} + +// NewTimer constructs a fake timer, akin to time.NewTimer(d). +func (f *FakeClock) NewTimer(d time.Duration) clock.Timer { + f.lock.Lock() + defer f.lock.Unlock() + stopTime := f.time.Add(d) + ch := make(chan time.Time, 1) // Don't block! + timer := &fakeTimer{ + fakeClock: f, + waiter: fakeClockWaiter{ + targetTime: stopTime, + destChan: ch, + }, + } + f.waiters = append(f.waiters, &timer.waiter) + return timer +} + +// Tick constructs a fake ticker, akin to time.Tick +func (f *FakeClock) Tick(d time.Duration) <-chan time.Time { + if d <= 0 { + return nil + } + f.lock.Lock() + defer f.lock.Unlock() + tickTime := f.time.Add(d) + ch := make(chan time.Time, 1) // hold one tick + f.waiters = append(f.waiters, &fakeClockWaiter{ + targetTime: tickTime, + stepInterval: d, + skipIfBlocked: true, + destChan: ch, + }) + + return ch +} + +// Step moves the clock by Duration and notifies anyone that's called After, +// Tick, or NewTimer. +func (f *FakeClock) Step(d time.Duration) { + f.lock.Lock() + defer f.lock.Unlock() + f.setTimeLocked(f.time.Add(d)) +} + +// SetTime sets the time. +func (f *FakeClock) SetTime(t time.Time) { + f.lock.Lock() + defer f.lock.Unlock() + f.setTimeLocked(t) +} + +// Actually changes the time and checks any waiters. f must be write-locked. +func (f *FakeClock) setTimeLocked(t time.Time) { + f.time = t + newWaiters := make([]*fakeClockWaiter, 0, len(f.waiters)) + for i := range f.waiters { + w := f.waiters[i] + if !w.targetTime.After(t) { + + if w.skipIfBlocked { + select { + case w.destChan <- t: + w.fired = true + default: + } + } else { + w.destChan <- t + w.fired = true + } + + if w.stepInterval > 0 { + for !w.targetTime.After(t) { + w.targetTime = w.targetTime.Add(w.stepInterval) + } + newWaiters = append(newWaiters, w) + } + + } else { + newWaiters = append(newWaiters, f.waiters[i]) + } + } + f.waiters = newWaiters +} + +// HasWaiters returns true if After has been called on f but not yet satisfied (so you can +// write race-free tests). +func (f *FakeClock) HasWaiters() bool { + f.lock.RLock() + defer f.lock.RUnlock() + return len(f.waiters) > 0 +} + +// Sleep is akin to time.Sleep +func (f *FakeClock) Sleep(d time.Duration) { + f.Step(d) +} + +// IntervalClock implements clock.Clock, but each invocation of Now steps the clock forward the specified duration +type IntervalClock struct { + Time time.Time + Duration time.Duration +} + +// Now returns i's time. +func (i *IntervalClock) Now() time.Time { + i.Time = i.Time.Add(i.Duration) + return i.Time +} + +// Since returns time since the time in i. +func (i *IntervalClock) Since(ts time.Time) time.Duration { + return i.Time.Sub(ts) +} + +// After is unimplemented, will panic. +// TODO: make interval clock use FakeClock so this can be implemented. +func (*IntervalClock) After(d time.Duration) <-chan time.Time { + panic("IntervalClock doesn't implement After") +} + +// NewTimer is unimplemented, will panic. +// TODO: make interval clock use FakeClock so this can be implemented. +func (*IntervalClock) NewTimer(d time.Duration) clock.Timer { + panic("IntervalClock doesn't implement NewTimer") +} + +// Tick is unimplemented, will panic. +// TODO: make interval clock use FakeClock so this can be implemented. +func (*IntervalClock) Tick(d time.Duration) <-chan time.Time { + panic("IntervalClock doesn't implement Tick") +} + +// Sleep is unimplemented, will panic. +func (*IntervalClock) Sleep(d time.Duration) { + panic("IntervalClock doesn't implement Sleep") +} + +var _ = clock.Timer(&fakeTimer{}) + +// fakeTimer implements clock.Timer based on a FakeClock. +type fakeTimer struct { + fakeClock *FakeClock + waiter fakeClockWaiter +} + +// C returns the channel that notifies when this timer has fired. +func (f *fakeTimer) C() <-chan time.Time { + return f.waiter.destChan +} + +// Stop stops the timer and returns true if the timer has not yet fired, or false otherwise. +func (f *fakeTimer) Stop() bool { + f.fakeClock.lock.Lock() + defer f.fakeClock.lock.Unlock() + + newWaiters := make([]*fakeClockWaiter, 0, len(f.fakeClock.waiters)) + for i := range f.fakeClock.waiters { + w := f.fakeClock.waiters[i] + if w != &f.waiter { + newWaiters = append(newWaiters, w) + } + } + + f.fakeClock.waiters = newWaiters + + return !f.waiter.fired +} + +// Reset resets the timer to the fake clock's "now" + d. It returns true if the timer has not yet +// fired, or false otherwise. +func (f *fakeTimer) Reset(d time.Duration) bool { + f.fakeClock.lock.Lock() + defer f.fakeClock.lock.Unlock() + + active := !f.waiter.fired + + f.waiter.fired = false + f.waiter.targetTime = f.fakeClock.time.Add(d) + + var isWaiting bool + for i := range f.fakeClock.waiters { + w := f.fakeClock.waiters[i] + if w == &f.waiter { + isWaiting = true + break + } + } + if !isWaiting { + f.fakeClock.waiters = append(f.fakeClock.waiters, &f.waiter) + } + + return active +} diff --git a/vendor/modules.txt b/vendor/modules.txt index 1bfdf41d2441..22c2f47f3416 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -2465,6 +2465,7 @@ k8s.io/system-validators/validators # k8s.io/utils => k8s.io/utils v0.0.0-20200729134348-d5654de09c73 k8s.io/utils/buffer k8s.io/utils/clock +k8s.io/utils/clock/testing k8s.io/utils/exec k8s.io/utils/exec/testing k8s.io/utils/inotify From bbb0cf5d31116cb383079e24dbd05c1a110c67e3 Mon Sep 17 00:00:00 2001 From: Mike Spreitzer Date: Fri, 11 Sep 2020 23:16:39 -0400 Subject: [PATCH 7/7] Add control flow to silence static checker Before this change, the staticcheck test in the pull-kubernetes-verify job complained about some possible nil pointer dereferences, but those would never happen because `t.Errorf` aborts the test. Added control flow (that will never be reached, if you understand `t.Errorf`) to make it obvious that the nil dereferences will never happen. --- .../metrics/prometheusextension/sampling-histogram_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/component-base/metrics/prometheusextension/sampling-histogram_test.go b/staging/src/k8s.io/component-base/metrics/prometheusextension/sampling-histogram_test.go index c27765f0b15a..01aacada968e 100644 --- a/staging/src/k8s.io/component-base/metrics/prometheusextension/sampling-histogram_test.go +++ b/staging/src/k8s.io/component-base/metrics/prometheusextension/sampling-histogram_test.go @@ -93,6 +93,7 @@ func expectHistogram(t *testing.T, when string, sh SamplingHistogram, buckets .. } if dtom.Histogram == nil { t.Errorf("%s, Collect returned a Metric without a Histogram: %#+v", when, dtom) + return } mh := dtom.Histogram if len(mh.Bucket) != len(buckets)-1 { @@ -100,16 +101,17 @@ func expectHistogram(t *testing.T, when string, sh SamplingHistogram, buckets .. } if mh.SampleCount == nil { t.Errorf("%s, got Histogram with nil SampleCount", when) - } - if *(mh.SampleCount) != buckets[len(mh.Bucket)] { + } else if *(mh.SampleCount) != buckets[len(mh.Bucket)] { t.Errorf("%s, SampleCount=%d but expected %d", when, *(mh.SampleCount), buckets[len(mh.Bucket)]) } for i, mhBucket := range mh.Bucket { if mhBucket == nil { t.Errorf("%s, bucket %d was nil", when, i) + continue } if mhBucket.UpperBound == nil || mhBucket.CumulativeCount == nil { t.Errorf("%s, bucket %d had nil bound or count", when, i) + continue } ub := float64(i) if ub != *(mhBucket.UpperBound) {