metricvec: refactor collision handling to handle equality

After increasing unit test coverage, it was found that the split
function call nature of metric matching wasn't working well in many
cases. By increasing test coverage, we've ensured that both the fast
path and fallback collision path are working appropriately.

With these changes, there is a further performance hit, but now the
results are ensured to be correct.

Signed-off-by: Stephen J Day <[email protected]>

Author: stevvooe

prometheus/vec.go

@@ -16,6 +16,8 @@ package prometheus
 import (
 	"fmt"
 	"sync"
+
+	"github.com/prometheus/common/model"
 )
 
 // MetricVec is a Collector to bundle metrics of the same name that
@@ -25,25 +27,29 @@ import (
 // provided in this package.
 type MetricVec struct {
 	mtx      sync.RWMutex // Protects the children.
-	children map[uint64][]metricLabelValue
+	children map[uint64][]metricWithLabelValues
 	desc     *Desc
 
-	newMetric func(labelValues ...string) Metric
-	hashAdd   func(h uint64, s string) uint64 // replace hash function for testing collision handling
+	newMetric   func(labelValues ...string) Metric
+	hashAdd     func(h uint64, s string) uint64 // replace hash function for testing collision handling
+	hashAddByte func(h uint64, b byte) uint64
 }
 
 // newMetricVec returns an initialized MetricVec. The concrete value is
 // returned for embedding into another struct.
 func newMetricVec(desc *Desc, newMetric func(lvs ...string) Metric) MetricVec {
 	return MetricVec{
-		children:  map[uint64][]metricLabelValue{},
-		desc:      desc,
-		newMetric: newMetric,
-		hashAdd:   hashAdd,
+		children:    map[uint64][]metricWithLabelValues{},
+		desc:        desc,
+		newMetric:   newMetric,
+		hashAdd:     hashAdd,
+		hashAddByte: hashAddByte,
 	}
 }
 
-type metricLabelValue struct {
+// metricWithLabelValues provides the metric and its label values for
+// disambiguation on hash collision.
+type metricWithLabelValues struct {
 	values []string
 	metric Metric
 }
@@ -96,16 +102,7 @@ func (m *MetricVec) GetMetricWithLabelValues(lvs ...string) (Metric, error) {
 		return nil, err
 	}
 
-	m.mtx.RLock()
-	metric, ok := m.getMetric(h, lvs...)
-	m.mtx.RUnlock()
-	if ok {
-		return metric, nil
-	}
-
-	m.mtx.Lock()
-	defer m.mtx.Unlock()
-	return m.getOrCreateMetric(h, lvs...), nil
+	return m.getOrCreateMetric(h, lvs), nil
 }
 
 // GetMetricWith returns the Metric for the given Labels map (the label names
@@ -126,21 +123,7 @@ func (m *MetricVec) GetMetricWith(labels Labels) (Metric, error) {
 		return nil, err
 	}
 
-	m.mtx.RLock()
-	metrics, ok := m.children[h]
-	if ok && len(metrics) == 1 {
-		m.mtx.RUnlock()
-		return metrics[0].metric, nil
-	}
-	m.mtx.RUnlock()
-
-	lvs := make([]string, len(labels))
-	for i, label := range m.desc.variableLabels {
-		lvs[i] = labels[label]
-	}
-	m.mtx.Lock()
-	defer m.mtx.Unlock()
-	return m.getOrCreateMetric(h, lvs...), nil
+	return m.getOrCreateMetric(h, labels), nil
 }
 
 // WithLabelValues works as GetMetricWithLabelValues, but panics if an error
@@ -188,7 +171,7 @@ func (m *MetricVec) DeleteLabelValues(lvs ...string) bool {
 	if err != nil {
 		return false
 	}
-	return m.deleteByHash(h, lvs...)
+	return m.deleteByHash(h, lvs)
 }
 
 // Delete deletes the metric where the variable labels are the same as those
@@ -210,28 +193,21 @@ func (m *MetricVec) Delete(labels Labels) bool {
 		return false
 	}
 
-	var lvs []string
-	for _, k := range m.desc.variableLabels {
-		lvs = append(lvs, labels[k])
-	}
-
-	return m.deleteByHash(h, lvs...)
+	return m.deleteByHash(h, labels)
 }
 
 // deleteByHash removes the metric from the hash bucket h. If there are
 // multiple matches in the bucket, use lvs to select a metric and remove only
 // that metric.
-func (m *MetricVec) deleteByHash(h uint64, lvs ...string) bool {
+//
+// lvs MUST be of type Labels or []string or this method will panic.
+func (m *MetricVec) deleteByHash(h uint64, values interface{}) bool {
 	metrics, ok := m.children[h]
 	if !ok {
 		return false
 	}
 
-	if len(metrics) < 2 {
-		delete(m.children, h)
-	}
-
-	i := findMetric(lvs, metrics)
+	i := m.findMetric(metrics, values)
 	if i >= len(metrics) {
 		return false
 	}
@@ -261,6 +237,7 @@ func (m *MetricVec) hashLabelValues(vals []string) (uint64, error) {
 	h := hashNew()
 	for _, val := range vals {
 		h = m.hashAdd(h, val)
+		h = m.hashAddByte(h, model.SeparatorByte)
 	}
 	return h, nil
 }
@@ -276,43 +253,51 @@ func (m *MetricVec) hashLabels(labels Labels) (uint64, error) {
 			return 0, fmt.Errorf("label name %q missing in label map", label)
 		}
 		h = m.hashAdd(h, val)
+		h = m.hashAddByte(h, model.SeparatorByte)
 	}
 	return h, nil
 }
 
-func (m *MetricVec) getOrCreateMetric(hash uint64, labelValues ...string) Metric {
-	metric, ok := m.getMetric(hash, labelValues...)
+// getOrCreateMetric retrieves the metric by hash and label value or creates it
+// and returns the new one.
+//
+// lvs MUST be of type Labels or []string or this method will panic.
+//
+// This function holds the mutex.
+func (m *MetricVec) getOrCreateMetric(hash uint64, lvs interface{}) Metric {
+	m.mtx.RLock()
+	metric, ok := m.getMetric(hash, lvs)
+	m.mtx.RUnlock()
+	if ok {
+		return metric
+	}
+
+	m.mtx.Lock()
+	defer m.mtx.Unlock()
+	metric, ok = m.getMetric(hash, lvs)
 	if !ok {
-		// Copy labelValues. Otherwise, they would be allocated even if we don't go
-		// down this code path.
-		copiedLabelValues := append(make([]string, 0, len(labelValues)), labelValues...)
-		metric = m.newMetric(copiedLabelValues...)
-		m.children[hash] = append(m.children[hash], metricLabelValue{values: copiedLabelValues, metric: metric})
+		lvs := m.copyLabelValues(lvs)
+		metric = m.newMetric(lvs...)
+		m.children[hash] = append(m.children[hash], metricWithLabelValues{values: lvs, metric: metric})
 	}
 	return metric
 }
 
 // getMetric while handling possible collisions in the hash space. Must be
 // called while holding read mutex.
-func (m *MetricVec) getMetric(h uint64, lvs ...string) (Metric, bool) {
+//
+// lvs must be of type Labels or []string.
+func (m *MetricVec) getMetric(h uint64, lvs interface{}) (Metric, bool) {
 	metrics, ok := m.children[h]
 	if ok {
-		return m.selectMetric(lvs, metrics)
+		return m.selectMetric(metrics, lvs)
 	}
 
 	return nil, false
 }
 
-func (m *MetricVec) selectMetric(lvs []string, metrics []metricLabelValue) (Metric, bool) {
-	switch len(metrics) {
-	case 0:
-		return nil, false
-	case 1:
-		// collisions are rare, this should be the fast path.
-		return metrics[0].metric, true
-	}
-
-	i := findMetric(lvs, metrics)
+func (m *MetricVec) selectMetric(metrics []metricWithLabelValues, lvs interface{}) (Metric, bool) {
+	i := m.findMetric(metrics, lvs)
 
 	if i < len(metrics) {
 		return metrics[i].metric, true
@@ -323,22 +308,69 @@ func (m *MetricVec) selectMetric(lvs []string, metrics []metricLabelValue) (Metr
 
 // findMetric returns the index of the matching metric or len(metrics) if not
 // found.
-func findMetric(lvs []string, metrics []metricLabelValue) int {
-next:
+func (m *MetricVec) findMetric(metrics []metricWithLabelValues, lvs interface{}) int {
 	for i, metric := range metrics {
-		if len(metric.values) != len(lvs) {
-			continue
+		if m.matchLabels(metric.values, lvs) {
+			return i
 		}
+	}
+
+	return len(metrics)
+}
 
-		for j, v := range metric.values {
-			if v != lvs[j] {
-				continue next
+func (m *MetricVec) matchLabels(values []string, lvs interface{}) bool {
+	switch lvs := lvs.(type) {
+	case []string:
+		if len(values) != len(lvs) {
+			return false
+		}
+
+		for i, v := range values {
+			if v != lvs[i] {
+				return false
+			}
+		}
+
+		return true
+	case Labels:
+		if len(lvs) != len(values) {
+			return false
+		}
+
+		for i, k := range m.desc.variableLabels {
+			if values[i] != lvs[k] {
+				return false
 			}
 		}
-		// falling out of the loop here means we have a match!
 
-		return i
+		return true
+	default:
+		// If we reach this condition, there is an unexpected type being used
+		// as a labels value. Either add branch here for the new type or fix
+		// the bug causing the type to be passed in.
+		panic("unsupported type")
 	}
+}
 
-	return len(metrics)
+// copyLabelValues copies the labels values into common string slice format to
+// use when allocating the metric and to keep track of hash collision
+// ambiguity.
+//
+// lvs must be of type Labels or []string or this method will panic.
+func (m *MetricVec) copyLabelValues(lvs interface{}) []string {
+	var labelValues []string
+	switch lvs := lvs.(type) {
+	case []string:
+		labelValues = make([]string, len(lvs))
+		copy(labelValues, lvs)
+	case Labels:
+		labelValues = make([]string, len(lvs))
+		for i, k := range m.desc.variableLabels {
+			labelValues[i] = lvs[k]
+		}
+	default:
+		panic(fmt.Sprintf("unsupported type for lvs: %#v", lvs))
+	}
+
+	return labelValues
 }