From 434a8ed85d5c61523c49d9ebb3fc49379ddef998 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Wed, 17 Aug 2016 14:01:11 +0200 Subject: [PATCH 1/2] Bring back zero-alloc label-value access for metric vecs Also, fix mutex copy-by-value bug. --- prometheus/counter.go | 2 +- prometheus/gauge.go | 2 +- prometheus/histogram.go | 2 +- prometheus/summary.go | 2 +- prometheus/untyped.go | 2 +- prometheus/vec.go | 202 +++++++++++++++++++++++----------------- prometheus/vec_test.go | 4 +- 7 files changed, 122 insertions(+), 94 deletions(-) diff --git a/prometheus/counter.go b/prometheus/counter.go index 7463858..ee37949 100644 --- a/prometheus/counter.go +++ b/prometheus/counter.go @@ -82,7 +82,7 @@ func (c *counter) Add(v float64) { // CounterVec embeds MetricVec. See there for a full list of methods with // detailed documentation. type CounterVec struct { - MetricVec + *MetricVec } // NewCounterVec creates a new CounterVec based on the provided CounterOpts and diff --git a/prometheus/gauge.go b/prometheus/gauge.go index af3037d..8b70e51 100644 --- a/prometheus/gauge.go +++ b/prometheus/gauge.go @@ -58,7 +58,7 @@ func NewGauge(opts GaugeOpts) Gauge { // (e.g. number of operations queued, partitioned by user and operation // type). Create instances with NewGaugeVec. type GaugeVec struct { - MetricVec + *MetricVec } // NewGaugeVec creates a new GaugeVec based on the provided GaugeOpts and diff --git a/prometheus/histogram.go b/prometheus/histogram.go index 3d550ba..9719e8f 100644 --- a/prometheus/histogram.go +++ b/prometheus/histogram.go @@ -287,7 +287,7 @@ func (h *histogram) Write(out *dto.Metric) error { // (e.g. HTTP request latencies, partitioned by status code and method). Create // instances with NewHistogramVec. type HistogramVec struct { - MetricVec + *MetricVec } // NewHistogramVec creates a new HistogramVec based on the provided HistogramOpts and diff --git a/prometheus/summary.go b/prometheus/summary.go index 2d6739e..bce05bf 100644 --- a/prometheus/summary.go +++ b/prometheus/summary.go @@ -390,7 +390,7 @@ func (s quantSort) Less(i, j int) bool { // (e.g. HTTP request latencies, partitioned by status code and method). Create // instances with NewSummaryVec. type SummaryVec struct { - MetricVec + *MetricVec } // NewSummaryVec creates a new SummaryVec based on the provided SummaryOpts and diff --git a/prometheus/untyped.go b/prometheus/untyped.go index b1baf07..5faf7e6 100644 --- a/prometheus/untyped.go +++ b/prometheus/untyped.go @@ -56,7 +56,7 @@ func NewUntyped(opts UntypedOpts) Untyped { // labels. This is used if you want to count the same thing partitioned by // various dimensions. Create instances with NewUntypedVec. type UntypedVec struct { - MetricVec + *MetricVec } // NewUntypedVec creates a new UntypedVec based on the provided UntypedOpts and diff --git a/prometheus/vec.go b/prometheus/vec.go index 28384c3..7f3eef9 100644 --- a/prometheus/vec.go +++ b/prometheus/vec.go @@ -37,8 +37,8 @@ type MetricVec struct { // 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{ +func newMetricVec(desc *Desc, newMetric func(lvs ...string) Metric) *MetricVec { + return &MetricVec{ children: map[uint64][]metricWithLabelValues{}, desc: desc, newMetric: newMetric, @@ -102,7 +102,7 @@ func (m *MetricVec) GetMetricWithLabelValues(lvs ...string) (Metric, error) { return nil, err } - return m.getOrCreateMetric(h, lvs), nil + return m.getOrCreateMetricWithLabelValues(h, lvs), nil } // GetMetricWith returns the Metric for the given Labels map (the label names @@ -123,7 +123,7 @@ func (m *MetricVec) GetMetricWith(labels Labels) (Metric, error) { return nil, err } - return m.getOrCreateMetric(h, labels), nil + return m.getOrCreateMetricWithLabels(h, labels), nil } // WithLabelValues works as GetMetricWithLabelValues, but panics if an error @@ -171,7 +171,7 @@ func (m *MetricVec) DeleteLabelValues(lvs ...string) bool { if err != nil { return false } - return m.deleteByHash(h, lvs) + return m.deleteByHashWithLabelValues(h, lvs) } // Delete deletes the metric where the variable labels are the same as those @@ -193,21 +193,40 @@ func (m *MetricVec) Delete(labels Labels) bool { return false } - return m.deleteByHash(h, labels) + return m.deleteByHashWithLabels(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, lvs interface{}) bool { +// deleteByHashWithLabelValues 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) deleteByHashWithLabelValues(h uint64, lvs []string) bool { metrics, ok := m.children[h] if !ok { return false } - i := m.findMetric(metrics, lvs) + i := m.findMetricWithLabelValues(metrics, lvs) + 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 +} + +// deleteByHashWithLabels 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) deleteByHashWithLabels(h uint64, labels Labels) bool { + metrics, ok := m.children[h] + if !ok { + return false + } + i := m.findMetricWithLabels(metrics, labels) if i >= len(metrics) { return false } @@ -258,15 +277,13 @@ func (m *MetricVec) hashLabels(labels Labels) (uint64, error) { return h, nil } -// 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. +// getOrCreateMetricWithLabelValues retrieves the metric by hash and label value +// or creates it and returns the new one. // // This function holds the mutex. -func (m *MetricVec) getOrCreateMetric(hash uint64, lvs interface{}) Metric { +func (m *MetricVec) getOrCreateMetricWithLabelValues(hash uint64, lvs []string) Metric { m.mtx.RLock() - metric, ok := m.getMetric(hash, lvs) + metric, ok := m.getMetricWithLabelValues(hash, lvs) m.mtx.RUnlock() if ok { return metric @@ -274,103 +291,114 @@ func (m *MetricVec) getOrCreateMetric(hash uint64, lvs interface{}) Metric { m.mtx.Lock() defer m.mtx.Unlock() - metric, ok = m.getMetric(hash, lvs) + metric, ok = m.getMetricWithLabelValues(hash, lvs) if !ok { - lvs := m.copyLabelValues(lvs) + // Copy to avoid allocation in case wo don't go down this code path. + copiedLVs := make([]string, len(lvs)) + copy(copiedLVs, lvs) + metric = m.newMetric(copiedLVs...) + m.children[hash] = append(m.children[hash], metricWithLabelValues{values: copiedLVs, metric: metric}) + } + return metric +} + +// getOrCreateMetricWithLabelValues retrieves the metric by hash and label value +// or creates it and returns the new one. +// +// This function holds the mutex. +func (m *MetricVec) getOrCreateMetricWithLabels(hash uint64, labels Labels) Metric { + m.mtx.RLock() + metric, ok := m.getMetricWithLabels(hash, labels) + m.mtx.RUnlock() + if ok { + return metric + } + + m.mtx.Lock() + defer m.mtx.Unlock() + metric, ok = m.getMetricWithLabels(hash, labels) + if !ok { + lvs := m.extractLabelValues(labels) 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. -// -// lvs must be of type Labels or []string. -func (m *MetricVec) getMetric(h uint64, lvs interface{}) (Metric, bool) { +// getMetricWithLabelValues gets a metric while handling possible collisions in +// the hash space. Must be called while holding read mutex. +func (m *MetricVec) getMetricWithLabelValues(h uint64, lvs []string) (Metric, bool) { metrics, ok := m.children[h] if ok { - return m.selectMetric(metrics, lvs) + if i := m.findMetricWithLabelValues(metrics, lvs); i < len(metrics) { + return metrics[i].metric, true + } } - 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 +// getMetricWithLabels gets a metric while handling possible collisions in +// the hash space. Must be called while holding read mutex. +func (m *MetricVec) getMetricWithLabels(h uint64, labels Labels) (Metric, bool) { + metrics, ok := m.children[h] + if ok { + if i := m.findMetricWithLabels(metrics, labels); 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 { +// findMetricWithLabelValues returns the index of the matching metric or +// len(metrics) if not found. +func (m *MetricVec) findMetricWithLabelValues(metrics []metricWithLabelValues, lvs []string) int { for i, metric := range metrics { - if m.matchLabels(metric.values, lvs) { + if m.matchLabelValues(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 +// findMetricWithLabels returns the index of the matching metric or len(metrics) +// if not found. +func (m *MetricVec) findMetricWithLabels(metrics []metricWithLabelValues, labels Labels) int { + for i, metric := range metrics { + if m.matchLabels(metric.values, labels) { + return i } - - 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") } + 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)) +func (m *MetricVec) matchLabelValues(values []string, lvs []string) bool { + if len(values) != len(lvs) { + return false } + for i, v := range values { + if v != lvs[i] { + return false + } + } + return true +} +func (m *MetricVec) matchLabels(values []string, labels Labels) bool { + if len(labels) != len(values) { + return false + } + for i, k := range m.desc.variableLabels { + if values[i] != labels[k] { + return false + } + } + return true +} + +func (m *MetricVec) extractLabelValues(labels Labels) []string { + labelValues := make([]string, len(labels)) + for i, k := range m.desc.variableLabels { + labelValues[i] = labels[k] + } return labelValues } diff --git a/prometheus/vec_test.go b/prometheus/vec_test.go index 6673c02..445a6b3 100644 --- a/prometheus/vec_test.go +++ b/prometheus/vec_test.go @@ -225,7 +225,7 @@ func TestCounterVecEndToEndWithCollision(t *testing.T) { t.Errorf("got label value %q, want %q", got, want) } if got, want := m.GetCounter().GetValue(), 1.; got != want { - t.Errorf("got value %d, want %d", got, want) + t.Errorf("got value %f, want %f", got, want) } m.Reset() if err := vec.WithLabelValues("!0IC=VloaY").Write(m); err != nil { @@ -235,7 +235,7 @@ func TestCounterVecEndToEndWithCollision(t *testing.T) { t.Errorf("got label value %q, want %q", got, want) } if got, want := m.GetCounter().GetValue(), 2.; got != want { - t.Errorf("got value %d, want %d", got, want) + t.Errorf("got value %f, want %f", got, want) } } From 9604506fffff794f333a7b2d820d18d43c4a5a46 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Wed, 17 Aug 2016 16:08:12 +0200 Subject: [PATCH 2/2] Cut v0.8.0 --- CHANGELOG.md | 23 +++++++++++++++++++++++ VERSION | 2 +- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c662f9..330788a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,26 @@ +## 0.8.0 / 2016-08-17 +* [CHANGE] Registry is doing more consistency checks. This might break + existing setups that used to export inconsistent metrics. +* [CHANGE] Pushing to Pushgateway moved to package `push` and changed to allow + arbitrary grouping. +* [CHANGE] Removed `SelfCollector`. +* [CHANGE] Removed `PanicOnCollectError` and `EnableCollectChecks` methods. +* [CHANGE] Moved packages to the prometheus/common repo: `text`, `model`, + `extraction`. +* [CHANGE] Deprecated a number of functions. +* [FEATURE] Allow custom registries. Added `Registerer` and `Gatherer` + interfaces. +* [FEATURE] Separated HTTP exposition, allowing custom HTTP handlers (package + `promhttp`) and enabling the creation of other exposition mechanisms. +* [FEATURE] `MustRegister` is variadic now, allowing registration of many + collectors in one call. +* [FEATURE] Added HTTP API v1 package. +* [ENHANCEMENT] Numerous documentation improvements. +* [ENHANCEMENT] Improved metric sorting. +* [ENHANCEMENT] Inlined fnv64a hashing for improved performance. +* [ENHANCEMENT] Several test improvements. +* [BUGFIX] Handle collisions in MetricVec. + ## 0.7.0 / 2015-07-27 * [CHANGE] Rename ExporterLabelPrefix to ExportedLabelPrefix. * [BUGFIX] Closed gaps in metric consistency check. diff --git a/VERSION b/VERSION index faef31a..a3df0a6 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.7.0 +0.8.0