diff --git a/prometheus/vec.go b/prometheus/vec.go index 02a7455..ec847bd 100644 --- a/prometheus/vec.go +++ b/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 } - - 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 +} diff --git a/prometheus/vec_test.go b/prometheus/vec_test.go index 674abb9..fb26b67 100644 --- a/prometheus/vec_test.go +++ b/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 {