From 30c3e315740c786b91a167211697b2fb7db8ae41 Mon Sep 17 00:00:00 2001 From: Pedro Tanaka Date: Fri, 26 Aug 2022 11:21:02 +0200 Subject: [PATCH 1/6] Improving isolation of metric conflict test Signed-off-by: Pedro Tanaka --- pkg/exporter/exporter_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/exporter/exporter_test.go b/pkg/exporter/exporter_test.go index ef7836f..d4b4ff7 100644 --- a/pkg/exporter/exporter_test.go +++ b/pkg/exporter/exporter_test.go @@ -520,10 +520,11 @@ mappings: events <- s.in close(events) }() - ex := NewExporter(prometheus.DefaultRegisterer, testMapper, log.NewNopLogger(), eventsActions, eventsUnmapped, errorEventStats, eventStats, conflictingEventStats, metricsCount) + registerer := prometheus.NewRegistry() + ex := NewExporter(registerer, testMapper, log.NewNopLogger(), eventsActions, eventsUnmapped, errorEventStats, eventStats, conflictingEventStats, metricsCount) ex.Listen(events) - metrics, err := prometheus.DefaultGatherer.Gather() + metrics, err := registerer.Gather() if err != nil { t.Fatalf("Cannot gather from DefaultGatherer: %v", err) } From 7afa060ce4329cf1771009f5fb115cecf7b63270 Mon Sep 17 00:00:00 2001 From: Pedro Tanaka Date: Fri, 26 Aug 2022 11:29:31 +0200 Subject: [PATCH 2/6] Fixing clash problem when counter clashes with previous registered histogram Signed-off-by: Pedro Tanaka --- pkg/registry/registry.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/pkg/registry/registry.go b/pkg/registry/registry.go index e6d6bec..f91797c 100644 --- a/pkg/registry/registry.go +++ b/pkg/registry/registry.go @@ -19,6 +19,7 @@ import ( "hash" "hash/fnv" "sort" + "strings" "time" "github.com/prometheus/client_golang/prometheus" @@ -165,6 +166,15 @@ func (r *Registry) GetCounter(metricName string, labels prometheus.Labels, help return nil, fmt.Errorf("metric with name %s is already registered", metricName) } + histogramSuffixes := []string{"_bucket", "_count", "_sum"} + for _, suffix := range histogramSuffixes { + if strings.HasSuffix(metricName, suffix) { + if r.MetricConflicts(strings.TrimSuffix(metricName, suffix), metrics.CounterMetricType) { + return nil, fmt.Errorf("metric with name %s is already registered", metricName) + } + } + } + var counterVec *prometheus.CounterVec if vh == nil { metricsCount.WithLabelValues("counter").Inc() From c41fa101f5f7ca1adf639678020005b6a0254ac8 Mon Sep 17 00:00:00 2001 From: Pedro Tanaka Date: Fri, 26 Aug 2022 11:53:27 +0200 Subject: [PATCH 3/6] Bring back test cases after rebase problem Signed-off-by: Pedro Tanaka --- pkg/exporter/exporter_test.go | 42 +++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/pkg/exporter/exporter_test.go b/pkg/exporter/exporter_test.go index d4b4ff7..e741301 100644 --- a/pkg/exporter/exporter_test.go +++ b/pkg/exporter/exporter_test.go @@ -401,6 +401,48 @@ func TestConflictingMetrics(t *testing.T) { }, }, }, + { + name: "histogram vs counter", + expected: []float64{2}, + in: event.Events{ + &event.ObserverEvent{ + OMetricName: "histogram_test1", + OValue: 2, + }, + &event.CounterEvent{ + CMetricName: "histogram_test1_count", + CValue: 1, + }, + }, + }, + { + name: "histogram vs counter with sum prefix", + expected: []float64{2}, + in: event.Events{ + &event.ObserverEvent{ + OMetricName: "histogram_test1", + OValue: 2, + }, + &event.CounterEvent{ + CMetricName: "histogram_test1_sum", + CValue: 1, + }, + }, + }, + { + name: "histogram vs counter with bucket suffix", + expected: []float64{2}, + in: event.Events{ + &event.ObserverEvent{ + OMetricName: "histogram_test1", + OValue: 2, + }, + &event.CounterEvent{ + CMetricName: "histogram_test1_bucket", + CValue: 1, + }, + }, + }, { name: "counter vs histogram", expected: []float64{1}, From 31b05ef232c25cb099086553364abca989c839e0 Mon Sep 17 00:00:00 2001 From: Pedro Tanaka Date: Fri, 26 Aug 2022 11:58:41 +0200 Subject: [PATCH 4/6] Fixing typo Signed-off-by: Pedro Tanaka --- pkg/exporter/exporter_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/exporter/exporter_test.go b/pkg/exporter/exporter_test.go index e741301..7be782f 100644 --- a/pkg/exporter/exporter_test.go +++ b/pkg/exporter/exporter_test.go @@ -402,7 +402,7 @@ func TestConflictingMetrics(t *testing.T) { }, }, { - name: "histogram vs counter", + name: "histogram vs counter with count suffix", expected: []float64{2}, in: event.Events{ &event.ObserverEvent{ @@ -416,7 +416,7 @@ func TestConflictingMetrics(t *testing.T) { }, }, { - name: "histogram vs counter with sum prefix", + name: "histogram vs counter with sum suffix", expected: []float64{2}, in: event.Events{ &event.ObserverEvent{ From 83cec219c880e0be9fb66effb50dd07731817974 Mon Sep 17 00:00:00 2001 From: Pedro Tanaka Date: Mon, 29 Aug 2022 08:51:05 +0200 Subject: [PATCH 5/6] Checking conflict for gauges as well and refactoring into function Signed-off-by: Pedro Tanaka --- pkg/exporter/exporter_test.go | 34 +++++++++++++++++++++++++++++++--- pkg/registry/registry.go | 29 ++++++++++++++++++++--------- 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/pkg/exporter/exporter_test.go b/pkg/exporter/exporter_test.go index 7be782f..6e1744c 100644 --- a/pkg/exporter/exporter_test.go +++ b/pkg/exporter/exporter_test.go @@ -443,6 +443,34 @@ func TestConflictingMetrics(t *testing.T) { }, }, }, + { + name: "histogram vs gauge with sum suffix", + expected: []float64{2}, + in: event.Events{ + &event.ObserverEvent{ + OMetricName: "histogram_test1", + OValue: 2, + }, + &event.GaugeEvent{ + GMetricName: "histogram_test1_sum", + GValue: 1, + }, + }, + }, + { + name: "histogram vs gauge with count suffix", + expected: []float64{2}, + in: event.Events{ + &event.ObserverEvent{ + OMetricName: "histogram_test1", + OValue: 2, + }, + &event.GaugeEvent{ + GMetricName: "histogram_test1_count", + GValue: 1, + }, + }, + }, { name: "counter vs histogram", expected: []float64{1}, @@ -562,11 +590,11 @@ mappings: events <- s.in close(events) }() - registerer := prometheus.NewRegistry() - ex := NewExporter(registerer, testMapper, log.NewNopLogger(), eventsActions, eventsUnmapped, errorEventStats, eventStats, conflictingEventStats, metricsCount) + reg := prometheus.NewRegistry() + ex := NewExporter(reg, testMapper, log.NewNopLogger(), eventsActions, eventsUnmapped, errorEventStats, eventStats, conflictingEventStats, metricsCount) ex.Listen(events) - metrics, err := registerer.Gather() + metrics, err := reg.Gather() if err != nil { t.Fatalf("Cannot gather from DefaultGatherer: %v", err) } diff --git a/pkg/registry/registry.go b/pkg/registry/registry.go index f91797c..0938ddb 100644 --- a/pkg/registry/registry.go +++ b/pkg/registry/registry.go @@ -166,13 +166,9 @@ func (r *Registry) GetCounter(metricName string, labels prometheus.Labels, help return nil, fmt.Errorf("metric with name %s is already registered", metricName) } - histogramSuffixes := []string{"_bucket", "_count", "_sum"} - for _, suffix := range histogramSuffixes { - if strings.HasSuffix(metricName, suffix) { - if r.MetricConflicts(strings.TrimSuffix(metricName, suffix), metrics.CounterMetricType) { - return nil, fmt.Errorf("metric with name %s is already registered", metricName) - } - } + err := r.checkHistogramNameCollision(metricName) + if err != nil { + return nil, err } var counterVec *prometheus.CounterVec @@ -191,7 +187,6 @@ func (r *Registry) GetCounter(metricName string, labels prometheus.Labels, help } var counter prometheus.Counter - var err error if counter, err = counterVec.GetMetricWith(labels); err != nil { return nil, err } @@ -200,6 +195,18 @@ func (r *Registry) GetCounter(metricName string, labels prometheus.Labels, help return counter, nil } +func (r *Registry) checkHistogramNameCollision(metricName string) error { + histogramSuffixes := []string{"_bucket", "_count", "_sum"} + for _, suffix := range histogramSuffixes { + if strings.HasSuffix(metricName, suffix) { + if r.MetricConflicts(strings.TrimSuffix(metricName, suffix), metrics.CounterMetricType) { + return fmt.Errorf("metric with name %s is already registered", metricName) + } + } + } + return nil +} + func (r *Registry) GetGauge(metricName string, labels prometheus.Labels, help string, mapping *mapper.MetricMapping, metricsCount *prometheus.GaugeVec) (prometheus.Gauge, error) { hash, labelNames := r.HashLabels(labels) vh, mh := r.Get(metricName, hash, metrics.GaugeMetricType) @@ -211,6 +218,11 @@ func (r *Registry) GetGauge(metricName string, labels prometheus.Labels, help st return nil, fmt.Errorf("metrics.Metric with name %s is already registered", metricName) } + err := r.checkHistogramNameCollision(metricName) + if err != nil { + return nil, fmt.Errorf("metrics.Metric with name %s conflicts with previously registered histogram", metricName) + } + var gaugeVec *prometheus.GaugeVec if vh == nil { metricsCount.WithLabelValues("gauge").Inc() @@ -227,7 +239,6 @@ func (r *Registry) GetGauge(metricName string, labels prometheus.Labels, help st } var gauge prometheus.Gauge - var err error if gauge, err = gaugeVec.GetMetricWith(labels); err != nil { return nil, err } From da06fabcb67c41be39d69e735911be6bfc2233f8 Mon Sep 17 00:00:00 2001 From: Pedro Tanaka Date: Mon, 29 Aug 2022 08:53:50 +0200 Subject: [PATCH 6/6] Use same error message Signed-off-by: Pedro Tanaka --- pkg/registry/registry.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/registry/registry.go b/pkg/registry/registry.go index 0938ddb..87d39a1 100644 --- a/pkg/registry/registry.go +++ b/pkg/registry/registry.go @@ -220,7 +220,7 @@ func (r *Registry) GetGauge(metricName string, labels prometheus.Labels, help st err := r.checkHistogramNameCollision(metricName) if err != nil { - return nil, fmt.Errorf("metrics.Metric with name %s conflicts with previously registered histogram", metricName) + return nil, fmt.Errorf("metrics.Metric with name %s is already registered", metricName) } var gaugeVec *prometheus.GaugeVec