Merge pull request #213 from claytono/conflicting-metrics-test

Add checking for conflicting metrics

exporter.go

@@ -57,6 +57,19 @@ func (u uncheckedCollector) Collect(c chan<- prometheus.Metric) {
 	u.c.Collect(c)
 }
 
+type metricType int
+
+const (
+	CounterMetricType metricType = iota
+	GaugeMetricType
+	SummaryMetricType
+	HistogramMetricType
+)
+
+type metricChecker interface {
+	metricConflicts(string, metricType) bool
+}
+
 func getLabelNames(labels prometheus.Labels) []string {
 	names := make([]string, 0, len(labels))
 	for labelName := range labels {
@@ -96,12 +109,15 @@ func NewCounterContainer() *CounterContainer {
 	}
 }
 
-func (c *CounterContainer) Get(metricName string, labels prometheus.Labels, help string) (prometheus.Counter, error) {
+func (c *CounterContainer) Get(metricName string, labels prometheus.Labels, mc metricChecker, help string) (prometheus.Counter, error) {
 	labelNames := getLabelNames(labels)
 	mapKey := getContainerMapKey(metricName, labelNames)
 
 	counterVec, ok := c.Elements[mapKey]
 	if !ok {
+		if mc.metricConflicts(metricName, CounterMetricType) {
+			return nil, fmt.Errorf("metric with name %s is already registered", metricName)
+		}
 		metricsCount.WithLabelValues("counter").Inc()
 		counterVec = prometheus.NewCounterVec(prometheus.CounterOpts{
 			Name: metricName,
@@ -134,12 +150,15 @@ func NewGaugeContainer() *GaugeContainer {
 	}
 }
 
-func (c *GaugeContainer) Get(metricName string, labels prometheus.Labels, help string) (prometheus.Gauge, error) {
+func (c *GaugeContainer) Get(metricName string, labels prometheus.Labels, mc metricChecker, help string) (prometheus.Gauge, error) {
 	labelNames := getLabelNames(labels)
 	mapKey := getContainerMapKey(metricName, labelNames)
 
 	gaugeVec, ok := c.Elements[mapKey]
 	if !ok {
+		if mc.metricConflicts(metricName, GaugeMetricType) {
+			return nil, fmt.Errorf("metric with name %s is already registered", metricName)
+		}
 		metricsCount.WithLabelValues("gauge").Inc()
 		gaugeVec = prometheus.NewGaugeVec(prometheus.GaugeOpts{
 			Name: metricName,
@@ -174,12 +193,21 @@ func NewSummaryContainer(mapper *mapper.MetricMapper) *SummaryContainer {
 	}
 }
 
-func (c *SummaryContainer) Get(metricName string, labels prometheus.Labels, help string, mapping *mapper.MetricMapping) (prometheus.Observer, error) {
+func (c *SummaryContainer) Get(metricName string, labels prometheus.Labels, mc metricChecker, help string, mapping *mapper.MetricMapping) (prometheus.Observer, error) {
 	labelNames := getLabelNames(labels)
 	mapKey := getContainerMapKey(metricName, labelNames)
 
 	summaryVec, ok := c.Elements[mapKey]
 	if !ok {
+		if mc.metricConflicts(metricName, SummaryMetricType) {
+			return nil, fmt.Errorf("metric with name %s is already registered", metricName)
+		}
+		if mc.metricConflicts(metricName+"_sum", SummaryMetricType) {
+			return nil, fmt.Errorf("metric with name %s is already registered", metricName)
+		}
+		if mc.metricConflicts(metricName+"_count", SummaryMetricType) {
+			return nil, fmt.Errorf("metric with name %s is already registered", metricName)
+		}
 		metricsCount.WithLabelValues("summary").Inc()
 		quantiles := c.mapper.Defaults.Quantiles
 		if mapping != nil && mapping.Quantiles != nil && len(mapping.Quantiles) > 0 {
@@ -228,12 +256,21 @@ func NewHistogramContainer(mapper *mapper.MetricMapper) *HistogramContainer {
 	}
 }
 
-func (c *HistogramContainer) Get(metricName string, labels prometheus.Labels, help string, mapping *mapper.MetricMapping) (prometheus.Observer, error) {
+func (c *HistogramContainer) Get(metricName string, labels prometheus.Labels, mc metricChecker, help string, mapping *mapper.MetricMapping) (prometheus.Observer, error) {
 	labelNames := getLabelNames(labels)
 	mapKey := getContainerMapKey(metricName, labelNames)
 
 	histogramVec, ok := c.Elements[mapKey]
 	if !ok {
+		if mc.metricConflicts(metricName+"_sum", HistogramMetricType) {
+			return nil, fmt.Errorf("metric with name %s is already registered", metricName)
+		}
+		if mc.metricConflicts(metricName+"_count", HistogramMetricType) {
+			return nil, fmt.Errorf("metric with name %s is already registered", metricName)
+		}
+		if mc.metricConflicts(metricName+"_bucket", HistogramMetricType) {
+			return nil, fmt.Errorf("metric with name %s is already registered", metricName)
+		}
 		metricsCount.WithLabelValues("histogram").Inc()
 		buckets := c.mapper.Defaults.Buckets
 		if mapping != nil && mapping.Buckets != nil && len(mapping.Buckets) > 0 {
@@ -309,6 +346,7 @@ type LabelValues struct {
 	lastRegisteredAt time.Time
 	labels           prometheus.Labels
 	ttl              time.Duration
+	metricType       metricType
 }
 
 type Exporter struct {
@@ -424,11 +462,12 @@ func (b *Exporter) handleEvent(event Event) {
 		counter, err := b.Counters.Get(
 			metricName,
 			prometheusLabels,
+			b,
 			help,
 		)
 		if err == nil {
 			counter.Add(event.Value())
-			b.saveLabelValues(metricName, prometheusLabels, mapping.Ttl)
+			b.saveLabelValues(metricName, CounterMetricType, prometheusLabels, mapping.Ttl)
 			eventStats.WithLabelValues("counter").Inc()
 		} else {
 			log.Debugf(regErrF, metricName, err)
@@ -439,6 +478,7 @@ func (b *Exporter) handleEvent(event Event) {
 		gauge, err := b.Gauges.Get(
 			metricName,
 			prometheusLabels,
+			b,
 			help,
 		)
 
@@ -448,7 +488,7 @@ func (b *Exporter) handleEvent(event Event) {
 			} else {
 				gauge.Set(event.Value())
 			}
-			b.saveLabelValues(metricName, prometheusLabels, mapping.Ttl)
+			b.saveLabelValues(metricName, GaugeMetricType, prometheusLabels, mapping.Ttl)
 			eventStats.WithLabelValues("gauge").Inc()
 		} else {
 			log.Debugf(regErrF, metricName, err)
@@ -469,12 +509,13 @@ func (b *Exporter) handleEvent(event Event) {
 			histogram, err := b.Histograms.Get(
 				metricName,
 				prometheusLabels,
+				b,
 				help,
 				mapping,
 			)
 			if err == nil {
 				histogram.Observe(event.Value() / 1000) // prometheus presumes seconds, statsd millisecond
-				b.saveLabelValues(metricName, prometheusLabels, mapping.Ttl)
+				b.saveLabelValues(metricName, HistogramMetricType, prometheusLabels, mapping.Ttl)
 				eventStats.WithLabelValues("timer").Inc()
 			} else {
 				log.Debugf(regErrF, metricName, err)
@@ -485,12 +526,13 @@ func (b *Exporter) handleEvent(event Event) {
 			summary, err := b.Summaries.Get(
 				metricName,
 				prometheusLabels,
+				b,
 				help,
 				mapping,
 			)
 			if err == nil {
 				summary.Observe(event.Value() / 1000) // prometheus presumes seconds, statsd millisecond
-				b.saveLabelValues(metricName, prometheusLabels, mapping.Ttl)
+				b.saveLabelValues(metricName, SummaryMetricType, prometheusLabels, mapping.Ttl)
 				eventStats.WithLabelValues("timer").Inc()
 			} else {
 				log.Debugf(regErrF, metricName, err)
@@ -528,7 +570,7 @@ func (b *Exporter) removeStaleMetrics() {
 }
 
 // saveLabelValues stores label values set to labelValues and update lastRegisteredAt time and ttl value
-func (b *Exporter) saveLabelValues(metricName string, labels prometheus.Labels, ttl time.Duration) {
+func (b *Exporter) saveLabelValues(metricName string, metricType metricType, labels prometheus.Labels, ttl time.Duration) {
 	metric, hasMetric := b.labelValues[metricName]
 	if !hasMetric {
 		metric = make(map[uint64]*LabelValues)
@@ -538,8 +580,9 @@ func (b *Exporter) saveLabelValues(metricName string, labels prometheus.Labels,
 	metricLabelValues, ok := metric[hash]
 	if !ok {
 		metricLabelValues = &LabelValues{
-			labels: labels,
+			labels:     labels,
-			ttl:    ttl,
+			ttl:        ttl,
+			metricType: metricType,
 		}
 		b.labelValues[metricName][hash] = metricLabelValues
 	}
@@ -549,6 +592,29 @@ func (b *Exporter) saveLabelValues(metricName string, labels prometheus.Labels,
 	metricLabelValues.ttl = ttl
 }
 
+func (b *Exporter) metricConflicts(metricName string, metricType metricType) bool {
+	metric, hasMetric := b.labelValues[metricName]
+	if !hasMetric {
+		// No metric with this name exists
+		return false
+	}
+
+	// The metric does exist.  All metrics in the hash should be of the same
+	// type, so we pick check the first one we find in the hash to check the
+	// type.
+	for _, lvs := range metric {
+		if lvs.metricType == metricType {
+			// We've found a copy of this metric with this type, but different
+			// labels, so it's safe to create a new one.
+			return false
+		}
+	}
+
+	// The metric exists, but it's of a different type than we're trying to
+	// create.
+	return true
+}
+
 func NewExporter(mapper *mapper.MetricMapper) *Exporter {
 	return &Exporter{
 		Counters:    NewCounterContainer(),

exporter_test.go

@@ -157,6 +157,213 @@ mappings:
 	}
 }
 
+// TestConflictingMetrics validates that the exporter will not register metrics
+// of different types that have overlapping names.
+func TestConflictingMetrics(t *testing.T) {
+	scenarios := []struct {
+		name     string
+		expected []float64
+		in       Events
+	}{
+		{
+			name:     "counter vs gauge",
+			expected: []float64{1},
+			in: Events{
+				&CounterEvent{
+					metricName: "cvg_test",
+					value:      1,
+				},
+				&GaugeEvent{
+					metricName: "cvg_test",
+					value:      2,
+				},
+			},
+		},
+		{
+			name:     "counter vs gauge with different labels",
+			expected: []float64{1, 2},
+			in: Events{
+				&CounterEvent{
+					metricName: "cvgl_test",
+					value:      1,
+					labels:     map[string]string{"tag": "1"},
+				},
+				&CounterEvent{
+					metricName: "cvgl_test",
+					value:      2,
+					labels:     map[string]string{"tag": "2"},
+				},
+				&GaugeEvent{
+					metricName: "cvgl_test",
+					value:      3,
+					labels:     map[string]string{"tag": "1"},
+				},
+			},
+		},
+		{
+			name:     "counter vs gauge with same labels",
+			expected: []float64{3},
+			in: Events{
+				&CounterEvent{
+					metricName: "cvgsl_test",
+					value:      1,
+					labels:     map[string]string{"tag": "1"},
+				},
+				&CounterEvent{
+					metricName: "cvgsl_test",
+					value:      2,
+					labels:     map[string]string{"tag": "1"},
+				},
+				&GaugeEvent{
+					metricName: "cvgsl_test",
+					value:      3,
+					labels:     map[string]string{"tag": "1"},
+				},
+			},
+		},
+		{
+			name:     "gauge vs counter",
+			expected: []float64{2},
+			in: Events{
+				&GaugeEvent{
+					metricName: "gvc_test",
+					value:      2,
+				},
+				&CounterEvent{
+					metricName: "gvc_test",
+					value:      1,
+				},
+			},
+		},
+		{
+			name:     "counter vs histogram sum",
+			expected: []float64{1},
+			in: Events{
+				&CounterEvent{
+					metricName: "histogram_test1_sum",
+					value:      1,
+				},
+				&TimerEvent{
+					metricName: "histogram.test1",
+					value:      2,
+				},
+			},
+		},
+		{
+			name:     "counter vs histogram count",
+			expected: []float64{1},
+			in: Events{
+				&CounterEvent{
+					metricName: "histogram_test2_count",
+					value:      1,
+				},
+				&TimerEvent{
+					metricName: "histogram.test2",
+					value:      2,
+				},
+			},
+		},
+		{
+			name:     "counter vs histogram bucket",
+			expected: []float64{1},
+			in: Events{
+				&CounterEvent{
+					metricName: "histogram_test3_bucket",
+					value:      1,
+				},
+				&TimerEvent{
+					metricName: "histogram.test3",
+					value:      2,
+				},
+			},
+		},
+		{
+			name:     "counter vs summary quantile",
+			expected: []float64{1},
+			in: Events{
+				&CounterEvent{
+					metricName: "cvsq_test",
+					value:      1,
+				},
+				&TimerEvent{
+					metricName: "cvsq_test",
+					value:      2,
+				},
+			},
+		},
+		{
+			name:     "counter vs summary count",
+			expected: []float64{1},
+			in: Events{
+				&CounterEvent{
+					metricName: "cvsc_count",
+					value:      1,
+				},
+				&TimerEvent{
+					metricName: "cvsc",
+					value:      2,
+				},
+			},
+		},
+		{
+			name:     "counter vs summary sum",
+			expected: []float64{1},
+			in: Events{
+				&CounterEvent{
+					metricName: "cvss_sum",
+					value:      1,
+				},
+				&TimerEvent{
+					metricName: "cvss",
+					value:      2,
+				},
+			},
+		},
+	}
+
+	config := `
+mappings:
+- match: histogram.*
+  timer_type: histogram
+  name: "histogram_${1}"
+`
+	for _, s := range scenarios {
+		t.Run(s.name, func(t *testing.T) {
+			testMapper := &mapper.MetricMapper{}
+			err := testMapper.InitFromYAMLString(config)
+			if err != nil {
+				t.Fatalf("Config load error: %s %s", config, err)
+			}
+
+			events := make(chan Events)
+			go func() {
+				events <- s.in
+				close(events)
+			}()
+			ex := NewExporter(testMapper)
+			ex.Listen(events)
+
+			metrics, err := prometheus.DefaultGatherer.Gather()
+			if err != nil {
+				t.Fatalf("Cannot gather from DefaultGatherer: %v", err)
+			}
+
+			for i, e := range s.expected {
+				mn := s.in[i].MetricName()
+				m := getFloat64(metrics, mn, s.in[i].Labels())
+
+				if m == nil {
+					t.Fatalf("Could not find time series with metric name '%v'", mn)
+				}
+
+				if *m != e {
+					t.Fatalf("Expected to get %v, but got %v instead", e, *m)
+				}
+			}
+		})
+	}
+}
+
 // TestEmptyStringMetric validates when a metric name ends up
 // being the empty string after applying the match replacements
 // tha we don't panic the Exporter Listener.