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 <stephen.day@docker.com>

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
+	newMetric   func(labelValues ...string) Metric
-	hashAdd   func(h uint64, s string) uint64 // replace hash function for testing collision handling
+	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{},
+		children:    map[uint64][]metricWithLabelValues{},
-		desc:      desc,
+		desc:        desc,
-		newMetric: newMetric,
+		newMetric:   newMetric,
-		hashAdd:   hashAdd,
+		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()
+	return m.getOrCreateMetric(h, lvs), nil
-	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
 }
 
 // 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()
+	return m.getOrCreateMetric(h, labels), nil
-	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
 }
 
 // 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
+	return m.deleteByHash(h, labels)
-	for _, k := range m.desc.variableLabels {
-		lvs = append(lvs, labels[k])
-	}
-
-	return m.deleteByHash(h, lvs...)
 }
 
 // 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 {
+	i := m.findMetric(metrics, values)
-		delete(m.children, h)
-	}
-
-	i := findMetric(lvs, metrics)
 	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 {
+// getOrCreateMetric retrieves the metric by hash and label value or creates it
-	metric, ok := m.getMetric(hash, labelValues...)
+// 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
+		lvs := m.copyLabelValues(lvs)
-		// down this code path.
+		metric = m.newMetric(lvs...)
-		copiedLabelValues := append(make([]string, 0, len(labelValues)), labelValues...)
+		m.children[hash] = append(m.children[hash], metricWithLabelValues{values: lvs, metric: metric})
-		metric = m.newMetric(copiedLabelValues...)
-		m.children[hash] = append(m.children[hash], metricLabelValue{values: copiedLabelValues, 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) {
+func (m *MetricVec) selectMetric(metrics []metricWithLabelValues, lvs interface{}) (Metric, bool) {
-	switch len(metrics) {
+	i := m.findMetric(metrics, lvs)
-	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)
 
 	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 {
+func (m *MetricVec) findMetric(metrics []metricWithLabelValues, lvs interface{}) int {
-next:
 	for i, metric := range metrics {
-		if len(metric.values) != len(lvs) {
+		if m.matchLabels(metric.values, lvs) {
-			continue
+			return i
 		}
-
-		for j, v := range metric.values {
-			if v != lvs[j] {
-				continue next
-			}
-		}
-		// falling out of the loop here means we have a match!
-
-		return i
 	}
 
 	return len(metrics)
 }
+
+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
+			}
+		}
+
+		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")
+	}
+}
+
+// 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
+}

prometheus/vec_test.go

@@ -16,11 +16,23 @@ package prometheus
 import (
 	"fmt"
 	"testing"
+
+	dto "github.com/prometheus/client_model/go"
 )
 
 func TestDelete(t *testing.T) {
 	vec := newUntypedMetricVec("test", "helpless", []string{"l1", "l2"})
+	testDelete(t, vec)
+}
 
+func TestDeleteWithCollisions(t *testing.T) {
+	vec := newUntypedMetricVec("test", "helpless", []string{"l1", "l2"})
+	vec.hashAdd = func(h uint64, s string) uint64 { return 1 }
+	vec.hashAddByte = func(h uint64, b byte) uint64 { return 1 }
+	testDelete(t, vec)
+}
+
+func testDelete(t *testing.T, vec *MetricVec) {
 	if got, want := vec.Delete(Labels{"l1": "v1", "l2": "v2"}), false; got != want {
 		t.Errorf("got %v, want %v", got, want)
 	}
@@ -58,6 +70,7 @@ func TestDeleteLabelValues(t *testing.T) {
 func TestDeleteLabelValuesWithCollisions(t *testing.T) {
 	vec := newUntypedMetricVec("test", "helpless", []string{"l1", "l2"})
 	vec.hashAdd = func(h uint64, s string) uint64 { return 1 }
+	vec.hashAddByte = func(h uint64, b byte) uint64 { return 1 }
 	testDeleteLabelValues(t, vec)
 }
 
@@ -67,14 +80,19 @@ func testDeleteLabelValues(t *testing.T, vec *MetricVec) {
 	}
 
 	vec.With(Labels{"l1": "v1", "l2": "v2"}).(Untyped).Set(42)
+	vec.With(Labels{"l1": "v1", "l2": "v3"}).(Untyped).Set(42) // add junk data for collision
 	if got, want := vec.DeleteLabelValues("v1", "v2"), true; got != want {
 		t.Errorf("got %v, want %v", got, want)
 	}
 	if got, want := vec.DeleteLabelValues("v1", "v2"), false; got != want {
 		t.Errorf("got %v, want %v", got, want)
 	}
+	if got, want := vec.DeleteLabelValues("v1", "v3"), true; got != want {
+		t.Errorf("got %v, want %v", got, want)
+	}
 
 	vec.With(Labels{"l1": "v1", "l2": "v2"}).(Untyped).Set(42)
+	// delete out of order
 	if got, want := vec.DeleteLabelValues("v2", "v1"), false; got != want {
 		t.Errorf("got %v, want %v", got, want)
 	}
@@ -83,6 +101,74 @@ func testDeleteLabelValues(t *testing.T, vec *MetricVec) {
 	}
 }
 
+func TestMetricVec(t *testing.T) {
+	vec := newUntypedMetricVec("test", "helpless", []string{"l1", "l2"})
+	testMetricVec(t, vec)
+}
+
+func TestMetricVecWithCollisions(t *testing.T) {
+	vec := newUntypedMetricVec("test", "helpless", []string{"l1", "l2"})
+	vec.hashAdd = func(h uint64, s string) uint64 { return 1 }
+	vec.hashAddByte = func(h uint64, b byte) uint64 { return 1 }
+	testMetricVec(t, vec)
+}
+
+func testMetricVec(t *testing.T, vec *MetricVec) {
+	vec.Reset() // actually test Reset now!
+
+	var pair [2]string
+	// keep track of metrics
+	expected := map[[2]string]int{}
+
+	for i := 0; i < 1000; i++ {
+		pair[0], pair[1] = fmt.Sprint(i%4), fmt.Sprint(i%5) // varying combinations multiples
+		expected[pair]++
+		vec.WithLabelValues(pair[0], pair[1]).(Untyped).Inc()
+
+		expected[[2]string{"v1", "v2"}]++
+		vec.WithLabelValues("v1", "v2").(Untyped).Inc()
+	}
+
+	var total int
+	for _, metrics := range vec.children {
+		for _, metric := range metrics {
+			total++
+			copy(pair[:], metric.values)
+
+			// is there a better way to access the value of a metric?
+			var metricOut dto.Metric
+			metric.metric.Write(&metricOut)
+			actual := *metricOut.Untyped.Value
+
+			var actualPair [2]string
+			for i, label := range metricOut.Label {
+				actualPair[i] = *label.Value
+			}
+
+			// test output pair against metric.values to ensure we've selected
+			// the right one. We check this to ensure the below check means
+			// anything at all.
+			if actualPair != pair {
+				t.Fatalf("unexpected pair association in metric map: %v != %v", actualPair, pair)
+			}
+
+			if actual != float64(expected[pair]) {
+				t.Fatalf("incorrect counter value for %v: %v != %v", pair, actual, expected[pair])
+			}
+		}
+	}
+
+	if total != len(expected) {
+		t.Fatalf("unexpected number of metrics: %v != %v", total, len(expected))
+	}
+
+	vec.Reset()
+
+	if len(vec.children) > 0 {
+		t.Fatalf("reset failed")
+	}
+}
+
 func newUntypedMetricVec(name, help string, labels []string) *MetricVec {
 	desc := NewDesc("test", "helpless", labels, nil)
 	vec := newMetricVec(desc, func(lvs ...string) Metric {