From cebad0da66ea8251667a6990320da01e17cee1a7 Mon Sep 17 00:00:00 2001 From: Han Kang Date: Thu, 25 Apr 2019 14:49:03 -0700 Subject: [PATCH] make method names more succinct, improve documentation for posterity --- pkg/util/metrics/counter.go | 32 +++++++++------- pkg/util/metrics/counter_test.go | 6 +-- pkg/util/metrics/metric.go | 63 ++++++++++++++++---------------- pkg/util/metrics/opts.go | 6 +-- pkg/util/metrics/registry.go | 4 +- 5 files changed, 58 insertions(+), 53 deletions(-) diff --git a/pkg/util/metrics/counter.go b/pkg/util/metrics/counter.go index 588fd061bf1..29d87742a3f 100644 --- a/pkg/util/metrics/counter.go +++ b/pkg/util/metrics/counter.go @@ -53,13 +53,13 @@ func (c *kubeCounter) setPrometheusCounter(counter prometheus.Counter) { c.initSelfCollection(counter) } -// GetDeprecatedVersion returns a pointer to the Version or nil -func (c *kubeCounter) GetDeprecatedVersion() *semver.Version { +// DeprecatedVersion returns a pointer to the Version or nil +func (c *kubeCounter) DeprecatedVersion() *semver.Version { return c.CounterOpts.DeprecatedVersion } // initializeMetric invocation creates the actual underlying Counter. Until this method is called -// our underlying counter is a no-op. +// the underlying counter is a no-op. func (c *kubeCounter) initializeMetric() { c.CounterOpts.annotateStabilityLevel() // this actually creates the underlying prometheus counter. @@ -67,13 +67,13 @@ func (c *kubeCounter) initializeMetric() { } // initializeDeprecatedMetric invocation creates the actual (but deprecated) Counter. Until this method -// is called our underlying counter is a no-op. +// is called the underlying counter is a no-op. func (c *kubeCounter) initializeDeprecatedMetric() { c.CounterOpts.markDeprecated() c.initializeMetric() } -// kubeCounterVec is our internal representation of our wrapping struct around prometheus +// kubeCounterVec is the internal representation of our wrapping struct around prometheus // counterVecs. kubeCounterVec implements both KubeCollector and KubeCounterVec. type kubeCounterVec struct { *prometheus.CounterVec @@ -96,30 +96,34 @@ func NewCounterVec(opts CounterOpts, labels []string) *kubeCounterVec { return cv } -// GetDeprecatedVersion returns a pointer to the Version or nil -func (v *kubeCounterVec) GetDeprecatedVersion() *semver.Version { +// DeprecatedVersion returns a pointer to the Version or nil +func (v *kubeCounterVec) DeprecatedVersion() *semver.Version { return v.CounterOpts.DeprecatedVersion } // initializeMetric invocation creates the actual underlying CounterVec. Until this method is called -// our underlying counterVec is a no-op. +// the underlying counterVec is a no-op. func (v *kubeCounterVec) initializeMetric() { v.CounterVec = prometheus.NewCounterVec(v.CounterOpts.toPromCounterOpts(), v.originalLabels) } -// initializeMetric invocation creates the actual (but deprecated) CounterVec. Until this method is called -// our underlying counterVec is a no-op. +// initializeDeprecatedMetric invocation creates the actual (but deprecated) CounterVec. Until this method is called +// the underlying counterVec is a no-op. func (v *kubeCounterVec) initializeDeprecatedMetric() { v.CounterOpts.markDeprecated() 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. This -// is undesirable for us, since we want a way to turn OFF metrics which end up turning into memory -// leaks. +// if a metric 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/master/prometheus/counter.go#L148-L177 +// For reference: https://github.com/prometheus/client_golang/blob/v0.9.2/prometheus/counter.go#L179-L197 +// +// This method returns a no-op metric if the metric is not actually created/registered, avoiding that +// memory leak. func (v *kubeCounterVec) WithLabelValues(lvs ...string) KubeCounter { if !v.IsCreated() { return noop // return no-op counter diff --git a/pkg/util/metrics/counter_test.go b/pkg/util/metrics/counter_test.go index 82609e23db8..62b4944c27b 100644 --- a/pkg/util/metrics/counter_test.go +++ b/pkg/util/metrics/counter_test.go @@ -101,7 +101,7 @@ func TestCounter(t *testing.T) { } } - // let's increment the counter N number of times and verify that the metric retains the count correctly + // increment the counter N number of times and verify that the metric retains the count correctly numberOfTimesToIncrement := 3 for i := 0; i < numberOfTimesToIncrement; i++ { c.Inc() @@ -188,7 +188,7 @@ func TestCounterVec(t *testing.T) { if err != nil { t.Fatalf("Gather failed %v", err) } - // we no-opt here when we don't have any metric families (i.e. when the metric is hidden) + // this no-opts here when there are no metric families (i.e. when the metric is hidden) for _, mf := range mfs { if len(mf.GetMetric()) != 1 { t.Errorf("Got %v metrics, wanted 1 as the count", len(mf.GetMetric())) @@ -206,7 +206,7 @@ func TestCounterVec(t *testing.T) { t.Fatalf("Gather failed %v", err) } - // we no-opt here when we don't have any metric families (i.e. when the metric is hidden) + // this no-opts here when there are no metric families (i.e. when the metric is hidden) for _, mf := range mfs { if len(mf.GetMetric()) != 3 { t.Errorf("Got %v metrics, wanted 3 as the count", len(mf.GetMetric())) diff --git a/pkg/util/metrics/metric.go b/pkg/util/metrics/metric.go index cc21e1a33af..d63bf9e408a 100644 --- a/pkg/util/metrics/metric.go +++ b/pkg/util/metrics/metric.go @@ -24,39 +24,41 @@ import ( "sync" ) -/** - * This extends the prometheus.Collector interface so that we can customize the metric - * registration process. Specifically, we defer metric initialization until ActuallyCreate - * is called, which then delegates to the underlying metric's initializeMetric or - * initializeDeprecatedMetric method call depending on whether the metric is deprecated or not. +/* +This extends the prometheus.Collector interface to allow customization of the metric +registration process. Defer metric initialization until Create() is called, which then +delegates to the underlying metric's initializeMetric or initializeDeprecatedMetric +method call depending on whether the metric is deprecated or not. */ type KubeCollector interface { Collector LazyMetric - GetDeprecatedVersion() *semver.Version + DeprecatedVersion() *semver.Version // Each collector metric should provide an initialization function // for both deprecated and non-deprecated variants of a metric. This - // is necessary since we are now deferring metric instantiation + // is necessary since metric instantiation will be deferred // until the metric is actually registered somewhere. initializeMetric() initializeDeprecatedMetric() } -// LazyMetric defines our registration functionality. We expect LazyMetric -// objects to lazily instantiate metrics (i.e defer metric instantiation until when -// ActuallyCreate is explicitly called). +/* +LazyMetric defines our registration functionality. LazyMetric objects are expected +to lazily instantiate metrics (i.e defer metric instantiation until when +the Create() function is explicitly called). + */ type LazyMetric interface { - ActuallyCreate(*semver.Version) bool + Create(*semver.Version) bool IsCreated() bool IsHidden() bool IsDeprecated() bool } /* - * lazyMetric implements LazyMetric. A lazy metric is lazy because it waits until metric - * registration time before instantiation. Add it as an anonymous field to a struct that - * implements KubeCollector to get deferred registration behavior. You must call lazyInit - * with the KubeCollector itself as an argument. +lazyMetric implements LazyMetric. A lazy metric is lazy because it waits until metric +registration time before instantiation. Add it as an anonymous field to a struct that +implements KubeCollector to get deferred registration behavior. You must call lazyInit +with the KubeCollector itself as an argument. */ type lazyMetric struct { isDeprecated bool @@ -78,11 +80,11 @@ func (r *lazyMetric) lazyInit(self KubeCollector) { r.self = self } -// determineDeprecationStatus figures out whether our lazy metric should be deprecated or not. It takes -// a Version argument which should be the version of the binary in which this code is currently being -// executed. +// determineDeprecationStatus figures out whether the lazy metric should be deprecated or not. +// This method takes a Version argument which should be the version of the binary in which +// this code is currently being executed. func (r *lazyMetric) determineDeprecationStatus(version semver.Version) { - selfVersion := r.self.GetDeprecatedVersion() + selfVersion := r.self.DeprecatedVersion() if selfVersion == nil { return } @@ -105,13 +107,12 @@ func (r *lazyMetric) IsDeprecated() bool { return r.isDeprecated } -// Defer initialization of metric until we know if we actually need to -// register the thing. This wrapper just allows us to consolidate the -// syncOnce logic in a single spot and toggle the flag, since this -// behavior will be consistent across metrics. -// -// This no-opts and returns true if metric is already created. -func (r *lazyMetric) ActuallyCreate(version *semver.Version) bool { +// Create forces the initialization of metric which has been deferred until +// the point at which this method is invoked. This method will determine whether +// the metric is deprecated or hidden, no-opting if the metric should be considered +// hidden. Furthermore, this function no-opts and returns true if metric is already +// created. +func (r *lazyMetric) Create(version *semver.Version) bool { if version != nil { r.determineDeprecationStatus(*version) } @@ -130,11 +131,11 @@ func (r *lazyMetric) ActuallyCreate(version *semver.Version) bool { return r.IsCreated() } -/** - * This code is directly lifted from the prometheus codebase. It's a convenience struct which - * allows you satisfy the Collector interface automatically if you already satisfy the Metric interface. - * - * For reference: https://github.com/prometheus/client_golang/blob/65d3a96fbaa7c8c9535d7133d6d98cd50eed4db8/prometheus/collector.go#L98-L120 +/* +This code is directly lifted from the prometheus codebase. It's a convenience struct which +allows you satisfy the Collector interface automatically if you already satisfy the Metric interface. + +For reference: https://github.com/prometheus/client_golang/blob/v0.9.2/prometheus/collector.go#L98-L120 */ type selfCollector struct { metric prometheus.Metric diff --git a/pkg/util/metrics/opts.go b/pkg/util/metrics/opts.go index 985c323c6d4..ad1d9980634 100644 --- a/pkg/util/metrics/opts.go +++ b/pkg/util/metrics/opts.go @@ -23,9 +23,9 @@ import ( "sync" ) -// KubeOpts is superset struct for prometheus.Opts. We choose not to embed -// the prometheus Opts structure here because that would change struct initialization -// in the manner to which people are currently accustomed. +// KubeOpts is superset struct for prometheus.Opts. The prometheus Opts structure +// is purposefully not embedded here because that would change struct initialization +// in the manner which people are currently accustomed. // // Name must be set to a non-empty string. DeprecatedVersion is defined only // if the metric for which this options applies is, in fact, deprecated. diff --git a/pkg/util/metrics/registry.go b/pkg/util/metrics/registry.go index 97a7ee3e8fb..57afeb149c7 100644 --- a/pkg/util/metrics/registry.go +++ b/pkg/util/metrics/registry.go @@ -45,7 +45,7 @@ func MustRegister(cs ...KubeCollector) { } func (kr *KubeRegistry) Register(c KubeCollector) error { - if c.ActuallyCreate(&kr.version) { + if c.Create(&kr.version) { return kr.PromRegistry.Register(c) } return nil @@ -54,7 +54,7 @@ func (kr *KubeRegistry) Register(c KubeCollector) error { func (kr *KubeRegistry) MustRegister(cs ...KubeCollector) { metrics := make([]prometheus.Collector, 0, len(cs)) for _, c := range cs { - if c.ActuallyCreate(&kr.version) { + if c.Create(&kr.version) { metrics = append(metrics, c) } }