Merge pull request #221 from stevvooe/benchmark-with-labels-metric-vec

metricvec: handle hash collision for labeled metrics

prometheus/counter.go

@@ -96,19 +96,15 @@ func NewCounterVec(opts CounterOpts, labelNames []string) *CounterVec {
 		opts.ConstLabels,
 	)
 	return &CounterVec{
-		MetricVec: MetricVec{
+		MetricVec: newMetricVec(desc, func(lvs ...string) Metric {
-			children: map[uint64]Metric{},
+			result := &counter{value: value{
-			desc:     desc,
+				desc:       desc,
-			newMetric: func(lvs ...string) Metric {
+				valType:    CounterValue,
-				result := &counter{value: value{
+				labelPairs: makeLabelPairs(desc, lvs),
-					desc:       desc,
+			}}
-					valType:    CounterValue,
+			result.init(result) // Init self-collection.
-					labelPairs: makeLabelPairs(desc, lvs),
+			return result
-				}}
+		}),
-				result.init(result) // Init self-collection.
-				return result
-			},
-		},
 	}
 }
 

prometheus/gauge.go

@@ -72,13 +72,9 @@ func NewGaugeVec(opts GaugeOpts, labelNames []string) *GaugeVec {
 		opts.ConstLabels,
 	)
 	return &GaugeVec{
-		MetricVec: MetricVec{
+		MetricVec: newMetricVec(desc, func(lvs ...string) Metric {
-			children: map[uint64]Metric{},
+			return newValue(desc, GaugeValue, 0, lvs...)
-			desc:     desc,
+		}),
-			newMetric: func(lvs ...string) Metric {
-				return newValue(desc, GaugeValue, 0, lvs...)
-			},
-		},
 	}
 }
 

prometheus/histogram.go

@@ -301,13 +301,9 @@ func NewHistogramVec(opts HistogramOpts, labelNames []string) *HistogramVec {
 		opts.ConstLabels,
 	)
 	return &HistogramVec{
-		MetricVec: MetricVec{
+		MetricVec: newMetricVec(desc, func(lvs ...string) Metric {
-			children: map[uint64]Metric{},
+			return newHistogram(desc, opts, lvs...)
-			desc:     desc,
+		}),
-			newMetric: func(lvs ...string) Metric {
-				return newHistogram(desc, opts, lvs...)
-			},
-		},
 	}
 }
 

prometheus/summary.go

@@ -404,13 +404,9 @@ func NewSummaryVec(opts SummaryOpts, labelNames []string) *SummaryVec {
 		opts.ConstLabels,
 	)
 	return &SummaryVec{
-		MetricVec: MetricVec{
+		MetricVec: newMetricVec(desc, func(lvs ...string) Metric {
-			children: map[uint64]Metric{},
+			return newSummary(desc, opts, lvs...)
-			desc:     desc,
+		}),
-			newMetric: func(lvs ...string) Metric {
-				return newSummary(desc, opts, lvs...)
-			},
-		},
 	}
 }
 

prometheus/untyped.go

@@ -70,13 +70,9 @@ func NewUntypedVec(opts UntypedOpts, labelNames []string) *UntypedVec {
 		opts.ConstLabels,
 	)
 	return &UntypedVec{
-		MetricVec: MetricVec{
+		MetricVec: newMetricVec(desc, func(lvs ...string) Metric {
-			children: map[uint64]Metric{},
+			return newValue(desc, UntypedValue, 0, lvs...)
-			desc:     desc,
+		}),
-			newMetric: func(lvs ...string) Metric {
-				return newValue(desc, UntypedValue, 0, lvs...)
-			},
-		},
 	}
 }
 

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,10 +27,31 @@ import (
 // provided in this package.
 type MetricVec struct {
 	mtx      sync.RWMutex // Protects the children.
-	children map[uint64]Metric
+	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
+	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][]metricWithLabelValues{},
+		desc:        desc,
+		newMetric:   newMetric,
+		hashAdd:     hashAdd,
+		hashAddByte: hashAddByte,
+	}
+}
+
+// metricWithLabelValues provides the metric and its label values for
+// disambiguation on hash collision.
+type metricWithLabelValues struct {
+	values []string
+	metric Metric
 }
 
 // Describe implements Collector. The length of the returned slice
@@ -42,8 +65,10 @@ func (m *MetricVec) Collect(ch chan<- Metric) {
 	m.mtx.RLock()
 	defer m.mtx.RUnlock()
 
-	for _, metric := range m.children {
+	for _, metrics := range m.children {
-		ch <- metric
+		for _, metric := range metrics {
+			ch <- metric.metric
+		}
 	}
 }
 
@@ -77,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.children[h]
-	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
@@ -107,20 +123,7 @@ func (m *MetricVec) GetMetricWith(labels Labels) (Metric, error) {
 		return nil, err
 	}
 
-	m.mtx.RLock()
+	return m.getOrCreateMetric(h, labels), nil
-	metric, ok := m.children[h]
-	m.mtx.RUnlock()
-	if ok {
-		return metric, nil
-	}
-
-	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
@@ -168,11 +171,7 @@ func (m *MetricVec) DeleteLabelValues(lvs ...string) bool {
 	if err != nil {
 		return false
 	}
-	if _, ok := m.children[h]; !ok {
+	return m.deleteByHash(h, lvs)
-		return false
-	}
-	delete(m.children, h)
-	return true
 }
 
 // Delete deletes the metric where the variable labels are the same as those
@@ -193,10 +192,31 @@ func (m *MetricVec) Delete(labels Labels) bool {
 	if err != nil {
 		return false
 	}
-	if _, ok := m.children[h]; !ok {
+
+	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.
+//
+// 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
 	}
-	delete(m.children, h)
+
+	i := m.findMetric(metrics, values)
+	if i >= len(metrics) {
+		return false
+	}
+
+	if len(metrics) > 1 {
+		m.children[h] = append(metrics[:i], metrics[i+1:]...)
+	} else {
+		delete(m.children, h)
+	}
 	return true
 }
 
@@ -216,7 +236,8 @@ func (m *MetricVec) hashLabelValues(vals []string) (uint64, error) {
 	}
 	h := hashNew()
 	for _, val := range vals {
-		h = hashAdd(h, val)
+		h = m.hashAdd(h, val)
+		h = m.hashAddByte(h, model.SeparatorByte)
 	}
 	return h, nil
 }
@@ -231,19 +252,125 @@ func (m *MetricVec) hashLabels(labels Labels) (uint64, error) {
 		if !ok {
 			return 0, fmt.Errorf("label name %q missing in label map", label)
 		}
-		h = hashAdd(h, val)
+		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.children[hash]
+// 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] = metric
 	}
 	return metric
 }
+
+// getMetric while handling possible collisions in the hash space. Must be
+// called while holding read mutex.
+//
+// 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(metrics, lvs)
+	}
+
+	return nil, false
+}
+
+func (m *MetricVec) selectMetric(metrics []metricWithLabelValues, lvs interface{}) (Metric, bool) {
+	i := m.findMetric(metrics, lvs)
+
+	if i < len(metrics) {
+		return metrics[i].metric, true
+	}
+
+	return nil, false
+}
+
+// findMetric returns the index of the matching metric or len(metrics) if not
+// found.
+func (m *MetricVec) findMetric(metrics []metricWithLabelValues, lvs interface{}) int {
+	for i, metric := range metrics {
+		if m.matchLabels(metric.values, lvs) {
+			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

@@ -14,19 +14,25 @@
 package prometheus
 
 import (
+	"fmt"
 	"testing"
+
+	dto "github.com/prometheus/client_model/go"
 )
 
 func TestDelete(t *testing.T) {
-	desc := NewDesc("test", "helpless", []string{"l1", "l2"}, nil)
+	vec := newUntypedMetricVec("test", "helpless", []string{"l1", "l2"})
-	vec := MetricVec{
+	testDelete(t, vec)
-		children: map[uint64]Metric{},
+}
-		desc:     desc,
-		newMetric: func(lvs ...string) Metric {
-			return newValue(desc, UntypedValue, 0, lvs...)
-		},
-	}
 
+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)
 	}
@@ -57,28 +63,36 @@ func TestDelete(t *testing.T) {
 }
 
 func TestDeleteLabelValues(t *testing.T) {
-	desc := NewDesc("test", "helpless", []string{"l1", "l2"}, nil)
+	vec := newUntypedMetricVec("test", "helpless", []string{"l1", "l2"})
-	vec := MetricVec{
+	testDeleteLabelValues(t, vec)
-		children: map[uint64]Metric{},
+}
-		desc:     desc,
-		newMetric: func(lvs ...string) Metric {
-			return newValue(desc, UntypedValue, 0, lvs...)
-		},
-	}
 
+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)
+}
+
+func testDeleteLabelValues(t *testing.T, vec *MetricVec) {
 	if got, want := vec.DeleteLabelValues("v1", "v2"), false; got != want {
 		t.Errorf("got %v, want %v", got, want)
 	}
 
 	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)
 	}
@@ -86,3 +100,145 @@ func TestDeleteLabelValues(t *testing.T) {
 		t.Errorf("got %v, want %v", got, want)
 	}
 }
+
+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 {
+		return newValue(desc, UntypedValue, 0, lvs...)
+	})
+	return &vec
+}
+
+func BenchmarkMetricVecWithLabelValuesBasic(B *testing.B) {
+	benchmarkMetricVecWithLabelValues(B, map[string][]string{
+		"l1": []string{"onevalue"},
+		"l2": []string{"twovalue"},
+	})
+}
+
+func BenchmarkMetricVecWithLabelValues2Keys10ValueCardinality(B *testing.B) {
+	benchmarkMetricVecWithLabelValuesCardinality(B, 2, 10)
+}
+
+func BenchmarkMetricVecWithLabelValues4Keys10ValueCardinality(B *testing.B) {
+	benchmarkMetricVecWithLabelValuesCardinality(B, 4, 10)
+}
+
+func BenchmarkMetricVecWithLabelValues2Keys100ValueCardinality(B *testing.B) {
+	benchmarkMetricVecWithLabelValuesCardinality(B, 2, 100)
+}
+
+func BenchmarkMetricVecWithLabelValues10Keys100ValueCardinality(B *testing.B) {
+	benchmarkMetricVecWithLabelValuesCardinality(B, 10, 100)
+}
+
+func BenchmarkMetricVecWithLabelValues10Keys1000ValueCardinality(B *testing.B) {
+	benchmarkMetricVecWithLabelValuesCardinality(B, 10, 1000)
+}
+
+func benchmarkMetricVecWithLabelValuesCardinality(B *testing.B, nkeys, nvalues int) {
+	labels := map[string][]string{}
+
+	for i := 0; i < nkeys; i++ {
+		var (
+			k  = fmt.Sprintf("key-%v", i)
+			vs = make([]string, 0, nvalues)
+		)
+		for j := 0; j < nvalues; j++ {
+			vs = append(vs, fmt.Sprintf("value-%v", j))
+		}
+		labels[k] = vs
+	}
+
+	benchmarkMetricVecWithLabelValues(B, labels)
+}
+
+func benchmarkMetricVecWithLabelValues(B *testing.B, labels map[string][]string) {
+	var keys []string
+	for k := range labels { // map order dependent, who cares though
+		keys = append(keys, k)
+	}
+
+	values := make([]string, len(labels)) // value cache for permutations
+	vec := newUntypedMetricVec("test", "helpless", keys)
+
+	B.ReportAllocs()
+	B.ResetTimer()
+	for i := 0; i < B.N; i++ {
+		// varies input across provide map entries based on key size.
+		for j, k := range keys {
+			candidates := labels[k]
+			values[j] = candidates[i%len(candidates)]
+		}
+
+		vec.WithLabelValues(values...)
+	}
+}