From 713e6eb604002f6afff0bcfd8edcd7a15685b1e4 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Mon, 17 Sep 2018 11:50:42 +0200 Subject: [PATCH 1/2] Let NewConst... functions detect invalid Desc The error of the invalid Desc is returned in that case. Signed-off-by: beorn7 --- prometheus/histogram.go | 5 ++++- prometheus/summary.go | 5 ++++- prometheus/value.go | 6 +++++- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/prometheus/histogram.go b/prometheus/histogram.go index a1868d2..7086b76 100644 --- a/prometheus/histogram.go +++ b/prometheus/histogram.go @@ -554,7 +554,7 @@ func (h *constHistogram) Write(out *dto.Metric) error { // bucket. // // NewConstHistogram returns an error if the length of labelValues is not -// consistent with the variable labels in Desc. +// consistent with the variable labels in Desc or if Desc is invalid. func NewConstHistogram( desc *Desc, count uint64, @@ -562,6 +562,9 @@ func NewConstHistogram( buckets map[float64]uint64, labelValues ...string, ) (Metric, error) { + if desc.err != nil { + return nil, desc.err + } if err := validateLabelValues(labelValues, len(desc.variableLabels)); err != nil { return nil, err } diff --git a/prometheus/summary.go b/prometheus/summary.go index 38ee98c..12ff034 100644 --- a/prometheus/summary.go +++ b/prometheus/summary.go @@ -586,7 +586,7 @@ func (s *constSummary) Write(out *dto.Metric) error { // map[float64]float64{0.5: 0.23, 0.99: 0.56} // // NewConstSummary returns an error if the length of labelValues is not -// consistent with the variable labels in Desc. +// consistent with the variable labels in Desc or if Desc is invalid. func NewConstSummary( desc *Desc, count uint64, @@ -594,6 +594,9 @@ func NewConstSummary( quantiles map[float64]float64, labelValues ...string, ) (Metric, error) { + if desc.err != nil { + return nil, desc.err + } if err := validateLabelValues(labelValues, len(desc.variableLabels)); err != nil { return nil, err } diff --git a/prometheus/value.go b/prometheus/value.go index 5e7d4b6..eb248f1 100644 --- a/prometheus/value.go +++ b/prometheus/value.go @@ -77,8 +77,12 @@ func (v *valueFunc) Write(out *dto.Metric) error { // operations. However, when implementing custom Collectors, it is useful as a // throw-away metric that is generated on the fly to send it to Prometheus in // the Collect method. NewConstMetric returns an error if the length of -// labelValues is not consistent with the variable labels in Desc. +// labelValues is not consistent with the variable labels in Desc or if Desc is +// invalid. func NewConstMetric(desc *Desc, valueType ValueType, value float64, labelValues ...string) (Metric, error) { + if desc.err != nil { + return nil, desc.err + } if err := validateLabelValues(labelValues, len(desc.variableLabels)); err != nil { return nil, err } From 663a9ad019ad4e1a9f121900aa6aa58ac1e0aa7a Mon Sep 17 00:00:00 2001 From: beorn7 Date: Mon, 17 Sep 2018 12:07:31 +0200 Subject: [PATCH 2/2] Make Help strings optional This is in line with https://prometheus.io/docs/instrumenting/writing_clientlibs/#metric-description-and-help Since the zero value of a string in Go is `""`, we cannot distinguish between a Help string not set and an empty Help string. Thus, we just make it formally optional here with an encouragement to set it in the doc comment. In v0.10, the Help string will probably become a "normal" argument of the constructor rather than a field in an Opts struct. Signed-off-by: beorn7 --- prometheus/desc.go | 6 +----- prometheus/examples_test.go | 31 ------------------------------- prometheus/histogram.go | 7 ++++--- prometheus/metric.go | 7 ++++--- prometheus/summary.go | 10 +++++----- 5 files changed, 14 insertions(+), 47 deletions(-) diff --git a/prometheus/desc.go b/prometheus/desc.go index e959162..7b8827f 100644 --- a/prometheus/desc.go +++ b/prometheus/desc.go @@ -67,7 +67,7 @@ type Desc struct { // NewDesc allocates and initializes a new Desc. Errors are recorded in the Desc // and will be reported on registration time. variableLabels and constLabels can -// be nil if no such labels should be set. fqName and help must not be empty. +// be nil if no such labels should be set. fqName must not be empty. // // variableLabels only contain the label names. Their label values are variable // and therefore not part of the Desc. (They are managed within the Metric.) @@ -80,10 +80,6 @@ func NewDesc(fqName, help string, variableLabels []string, constLabels Labels) * help: help, variableLabels: variableLabels, } - if help == "" { - d.err = errors.New("empty help string") - return d - } if !model.IsValidMetricName(model.LabelValue(fqName)) { d.err = fmt.Errorf("%q is not a valid metric name", fqName) return d diff --git a/prometheus/examples_test.go b/prometheus/examples_test.go index 775e03c..538a239 100644 --- a/prometheus/examples_test.go +++ b/prometheus/examples_test.go @@ -89,37 +89,6 @@ func ExampleGaugeFunc() { // GaugeFunc 'goroutines_count' registered. } -func ExampleCounter() { - pushCounter := prometheus.NewCounter(prometheus.CounterOpts{ - Name: "repository_pushes", // Note: No help string... - }) - err := prometheus.Register(pushCounter) // ... so this will return an error. - if err != nil { - fmt.Println("Push counter couldn't be registered, no counting will happen:", err) - return - } - - // Try it once more, this time with a help string. - pushCounter = prometheus.NewCounter(prometheus.CounterOpts{ - Name: "repository_pushes", - Help: "Number of pushes to external repository.", - }) - err = prometheus.Register(pushCounter) - if err != nil { - fmt.Println("Push counter couldn't be registered AGAIN, no counting will happen:", err) - return - } - - pushComplete := make(chan struct{}) - // TODO: Start a goroutine that performs repository pushes and reports - // each completion via the channel. - for range pushComplete { - pushCounter.Inc() - } - // Output: - // Push counter couldn't be registered, no counting will happen: descriptor Desc{fqName: "repository_pushes", help: "", constLabels: {}, variableLabels: []} is invalid: empty help string -} - func ExampleCounterVec() { httpReqs := prometheus.NewCounterVec( prometheus.CounterOpts{ diff --git a/prometheus/histogram.go b/prometheus/histogram.go index 7086b76..29dc8e3 100644 --- a/prometheus/histogram.go +++ b/prometheus/histogram.go @@ -110,8 +110,9 @@ func ExponentialBuckets(start, factor float64, count int) []float64 { } // HistogramOpts bundles the options for creating a Histogram metric. It is -// mandatory to set Name and Help to a non-empty string. All other fields are -// optional and can safely be left at their zero value. +// 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 HistogramOpts struct { // Namespace, Subsystem, and Name are components of the fully-qualified // name of the Histogram (created by joining these components with @@ -122,7 +123,7 @@ type HistogramOpts struct { Subsystem string Name string - // Help provides information about this Histogram. Mandatory! + // Help provides information about this Histogram. // // Metrics with the same fully-qualified name must have the same Help // string. diff --git a/prometheus/metric.go b/prometheus/metric.go index ae44621..55e6d86 100644 --- a/prometheus/metric.go +++ b/prometheus/metric.go @@ -59,8 +59,9 @@ type Metric interface { // implementation XXX has its own XXXOpts type, but in most cases, it is just be // an alias of this type (which might change when the requirement arises.) // -// It is mandatory to set Name and Help to a non-empty string. All other fields -// are optional and can safely be left at their zero value. +// 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 Opts struct { // Namespace, Subsystem, and Name are components of the fully-qualified // name of the Metric (created by joining these components with @@ -71,7 +72,7 @@ type Opts struct { Subsystem string Name string - // Help provides information about this metric. Mandatory! + // Help provides information about this metric. // // Metrics with the same fully-qualified name must have the same Help // string. diff --git a/prometheus/summary.go b/prometheus/summary.go index 12ff034..f7e92d8 100644 --- a/prometheus/summary.go +++ b/prometheus/summary.go @@ -81,10 +81,10 @@ const ( ) // SummaryOpts bundles the options for creating a Summary metric. It is -// mandatory to set Name and Help to a non-empty string. While all other fields -// are optional and can safely be left at their zero value, it is recommended to -// explicitly set the Objectives field to the desired value as the default value -// will change in the upcoming v0.10 of the library. +// 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 +// a help string and to explicitly set the Objectives field to the desired value +// as the default value will change in the upcoming v0.10 of the library. type SummaryOpts struct { // Namespace, Subsystem, and Name are components of the fully-qualified // name of the Summary (created by joining these components with @@ -95,7 +95,7 @@ type SummaryOpts struct { Subsystem string Name string - // Help provides information about this Summary. Mandatory! + // Help provides information about this Summary. // // Metrics with the same fully-qualified name must have the same Help // string.