From 923ff5879772d004965eb274b08318b529a1fcea Mon Sep 17 00:00:00 2001 From: Pierre Souchay Date: Tue, 29 Dec 2020 15:48:17 +0100 Subject: [PATCH] Use the same Help for metrics with and without labels When metrics are pre-registered with Help and without labels, when issuing metrics with labels, the help was filled with the key_name. But prometheus client enforces help to be consistent accross several elements, so, it can break at Runtime if a metric has been declared without its labels and later issued this metric with labels. In order to fix this, we store the help declared at startup for each key, so, when declaring a metric with some labels, we try to re-used pre-declared help for the metric. It should fix the issue that appeared with Consul 1.9.x and fix https://github.com/hashicorp/consul/issues/9471 --- prometheus/prometheus.go | 68 +++++++++++++++++++----------- prometheus/prometheus_test.go | 78 ++++++++++++++++++++++++++++++++--- 2 files changed, 116 insertions(+), 30 deletions(-) diff --git a/prometheus/prometheus.go b/prometheus/prometheus.go index db67a4d..1dcf530 100644 --- a/prometheus/prometheus.go +++ b/prometheus/prometheus.go @@ -58,13 +58,14 @@ type PrometheusSink struct { summaries sync.Map counters sync.Map expiration time.Duration + help map[string]string } // GaugeDefinition can be provided to PrometheusOpts to declare a constant gauge that is not deleted on expiry. type GaugeDefinition struct { - Name []string + Name []string ConstLabels []metrics.Label - Help string + Help string } type gauge struct { @@ -76,9 +77,9 @@ type gauge struct { // SummaryDefinition can be provided to PrometheusOpts to declare a constant summary that is not deleted on expiry. type SummaryDefinition struct { - Name []string + Name []string ConstLabels []metrics.Label - Help string + Help string } type summary struct { @@ -89,9 +90,9 @@ type summary struct { // CounterDefinition can be provided to PrometheusOpts to declare a constant counter that is not deleted on expiry. type CounterDefinition struct { - Name []string + Name []string ConstLabels []metrics.Label - Help string + Help string } type counter struct { @@ -112,11 +113,12 @@ func NewPrometheusSinkFrom(opts PrometheusOpts) (*PrometheusSink, error) { summaries: sync.Map{}, counters: sync.Map{}, expiration: opts.Expiration, + help: make(map[string]string), } - initGauges(&sink.gauges, opts.GaugeDefinitions) - initSummaries(&sink.summaries, opts.SummaryDefinitions) - initCounters(&sink.counters, opts.CounterDefinitions) + initGauges(&sink.gauges, opts.GaugeDefinitions, sink.help) + initSummaries(&sink.summaries, opts.SummaryDefinitions, sink.help) + initCounters(&sink.counters, opts.CounterDefinitions, sink.help) reg := opts.Registerer if reg == nil { @@ -191,22 +193,24 @@ func (p *PrometheusSink) Collect(c chan<- prometheus.Metric) { }) } -func initGauges(m *sync.Map, gauges []GaugeDefinition) { +func initGauges(m *sync.Map, gauges []GaugeDefinition, help map[string]string) { for _, g := range gauges { key, hash := flattenKey(g.Name, g.ConstLabels) + help[fmt.Sprintf("gauge.%s", key)] = g.Help pG := prometheus.NewGauge(prometheus.GaugeOpts{ Name: key, Help: g.Help, ConstLabels: prometheusLabels(g.ConstLabels), }) - m.Store(hash, &gauge{ Gauge: pG }) + m.Store(hash, &gauge{Gauge: pG}) } return } -func initSummaries(m *sync.Map, summaries []SummaryDefinition) { +func initSummaries(m *sync.Map, summaries []SummaryDefinition, help map[string]string) { for _, s := range summaries { key, hash := flattenKey(s.Name, s.ConstLabels) + help[fmt.Sprintf("summary.%s", key)] = s.Help pS := prometheus.NewSummary(prometheus.SummaryOpts{ Name: key, Help: s.Help, @@ -214,20 +218,21 @@ func initSummaries(m *sync.Map, summaries []SummaryDefinition) { ConstLabels: prometheusLabels(s.ConstLabels), Objectives: map[float64]float64{0.5: 0.05, 0.9: 0.01, 0.99: 0.001}, }) - m.Store(hash, &summary{ Summary: pS }) + m.Store(hash, &summary{Summary: pS}) } return } -func initCounters(m *sync.Map, counters []CounterDefinition) { +func initCounters(m *sync.Map, counters []CounterDefinition, help map[string]string) { for _, c := range counters { key, hash := flattenKey(c.Name, c.ConstLabels) + help[fmt.Sprintf("counter.%s", key)] = c.Help pC := prometheus.NewCounter(prometheus.CounterOpts{ Name: key, Help: c.Help, ConstLabels: prometheusLabels(c.ConstLabels), }) - m.Store(hash, &counter{ Counter: pC }) + m.Store(hash, &counter{Counter: pC}) } return } @@ -274,16 +279,21 @@ func (p *PrometheusSink) SetGaugeWithLabels(parts []string, val float32, labels localGauge.updatedAt = time.Now() p.gauges.Store(hash, &localGauge) - // The gauge does not exist, create the gauge and allow it to be deleted + // The gauge does not exist, create the gauge and allow it to be deleted } else { + help := key + existingHelp, ok := p.help[fmt.Sprintf("gauge.%s", key)] + if ok { + help = existingHelp + } g := prometheus.NewGauge(prometheus.GaugeOpts{ Name: key, - Help: key, + Help: help, ConstLabels: prometheusLabels(labels), }) g.Set(float64(val)) pg = &gauge{ - Gauge: g, + Gauge: g, updatedAt: time.Now(), canDelete: true, } @@ -306,18 +316,23 @@ func (p *PrometheusSink) AddSampleWithLabels(parts []string, val float32, labels localSummary.updatedAt = time.Now() p.summaries.Store(hash, &localSummary) - // The summary does not exist, create the Summary and allow it to be deleted + // The summary does not exist, create the Summary and allow it to be deleted } else { + help := key + existingHelp, ok := p.help[fmt.Sprintf("summary.%s", key)] + if ok { + help = existingHelp + } s := prometheus.NewSummary(prometheus.SummaryOpts{ Name: key, - Help: key, + Help: help, MaxAge: 10 * time.Second, ConstLabels: prometheusLabels(labels), Objectives: map[float64]float64{0.5: 0.05, 0.9: 0.01, 0.99: 0.001}, }) s.Observe(float64(val)) ps = &summary{ - Summary: s, + Summary: s, updatedAt: time.Now(), canDelete: true, } @@ -346,16 +361,21 @@ func (p *PrometheusSink) IncrCounterWithLabels(parts []string, val float32, labe localCounter.updatedAt = time.Now() p.counters.Store(hash, &localCounter) - // The counter does not exist yet, create it and allow it to be deleted + // The counter does not exist yet, create it and allow it to be deleted } else { + help := key + existingHelp, ok := p.help[fmt.Sprintf("counter.%s", key)] + if ok { + help = existingHelp + } c := prometheus.NewCounter(prometheus.CounterOpts{ Name: key, - Help: key, + Help: help, ConstLabels: prometheusLabels(labels), }) c.Add(float64(val)) pc = &counter{ - Counter: c, + Counter: c, updatedAt: time.Now(), canDelete: true, } diff --git a/prometheus/prometheus_test.go b/prometheus/prometheus_test.go index b359ecb..d0e223b 100644 --- a/prometheus/prometheus_test.go +++ b/prometheus/prometheus_test.go @@ -8,6 +8,7 @@ import ( "net/http/httptest" "net/url" "reflect" + "strings" "testing" "time" @@ -56,16 +57,16 @@ func TestNewPrometheusSink(t *testing.T) { func TestDefinitions(t *testing.T) { gaugeDef := GaugeDefinition{ - Name: []string{"my", "test", "gauge"}, - Help: "A gauge for testing? How helpful!", + Name: []string{"my", "test", "gauge"}, + Help: "A gauge for testing? How helpful!", } summaryDef := SummaryDefinition{ - Name: []string{"my", "test", "summary"}, - Help: "A summary for testing? How helpful!", + Name: []string{"my", "test", "summary"}, + Help: "A summary for testing? How helpful!", } counterDef := CounterDefinition{ - Name: []string{"my", "test", "summary"}, - Help: "A counter for testing? How helpful!", + Name: []string{"my", "test", "summary"}, + Help: "A counter for testing? How helpful!", } // PrometheusSink config w/ definitions for each metric type @@ -79,6 +80,7 @@ func TestDefinitions(t *testing.T) { if err != nil { t.Fatalf("err = #{err}, want nil") } + defer prometheus.Unregister(sink) // We can't just len(x) where x is a sync.Map, so we range over the single item and assert the name in our metric // definition matches the key we have for the map entry. Should fail if any metrics exist that aren't defined, or if @@ -168,3 +170,67 @@ func TestSetGauge(t *testing.T) { t.Fatal(response) } } + +func TestDefinitionsWithLabels(t *testing.T) { + gaugeDef := GaugeDefinition{ + Name: []string{"my", "test", "gauge"}, + Help: "A gauge for testing? How helpful!", + } + summaryDef := SummaryDefinition{ + Name: []string{"my", "test", "summary"}, + Help: "A summary for testing? How helpful!", + } + counterDef := CounterDefinition{ + Name: []string{"my", "test", "summary"}, + Help: "A counter for testing? How helpful!", + } + + // PrometheusSink config w/ definitions for each metric type + cfg := PrometheusOpts{ + Expiration: 5 * time.Second, + GaugeDefinitions: append([]GaugeDefinition{}, gaugeDef), + SummaryDefinitions: append([]SummaryDefinition{}, summaryDef), + CounterDefinitions: append([]CounterDefinition{}, counterDef), + } + sink, err := NewPrometheusSinkFrom(cfg) + if err != nil { + t.Fatalf("err =%#v, want nil", err) + } + defer prometheus.Unregister(sink) + if len(sink.help) != 3 { + t.Fatalf("Expected len(sink.help) to be 3, was %d: %#v", len(sink.help), sink.help) + } + + sink.SetGaugeWithLabels(gaugeDef.Name, 42.0, []metrics.Label{ + {Name: "version", Value: "some info"}, + }) + sink.gauges.Range(func(key, value interface{}) bool { + localGauge := *value.(*gauge) + if !strings.Contains(localGauge.Desc().String(), gaugeDef.Help) { + t.Fatalf("expected gauge to include correct help=%s, but was %s", gaugeDef.Help, localGauge.Desc().String()) + } + return true + }) + + sink.AddSampleWithLabels(summaryDef.Name, 42.0, []metrics.Label{ + {Name: "version", Value: "some info"}, + }) + sink.summaries.Range(func(key, value interface{}) bool { + metric := *value.(*summary) + if !strings.Contains(metric.Desc().String(), summaryDef.Help) { + t.Fatalf("expected gauge to include correct help=%s, but was %s", summaryDef.Help, metric.Desc().String()) + } + return true + }) + + sink.IncrCounterWithLabels(counterDef.Name, 42.0, []metrics.Label{ + {Name: "version", Value: "some info"}, + }) + sink.counters.Range(func(key, value interface{}) bool { + metric := *value.(*counter) + if !strings.Contains(metric.Desc().String(), counterDef.Help) { + t.Fatalf("expected gauge to include correct help=%s, but was %s", counterDef.Help, metric.Desc().String()) + } + return true + }) +}