Fixed panic while collecting Prometheus metrics (#4483)

`HistogramData.buckets` must be immutable after `HistogramData.Metric()` has been called, so take an extra copy. Signed-off-by: Bryan Boreham <[email protected]> Co-authored-by: Marco Pracucci <[email protected]> Co-authored-by: Peter Štibraný <[email protected]>
cortexproject · Sep 17, 2021 · 83d15b5 · 83d15b5
1 parent 0911a59
commit 83d15b5
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 6 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -53,6 +53,7 @@
 * [BUGFIX] Memberlist: forward only changes, not entire original message. #4419
 * [BUGFIX] Memberlist: don't accept old tombstones as incoming change, and don't forward such messages to other gossip members. #4420
 * [BUGFIX] Querier: fixed panic when querying exemplars and using `-distributor.shard-by-all-labels=false`. #4473
+* [BUGFIX] Compactor: fixed panic while collecting Prometheus metrics. #4483
 
 ## 1.10.0 / 2021-08-03
 

diff --git a/pkg/util/metrics_helper.go b/pkg/util/metrics_helper.go
@@ -454,14 +454,17 @@ func (s *SummaryData) Metric(desc *prometheus.Desc, labelValues ...string) prome
 	return prometheus.MustNewConstSummary(desc, s.sampleCount, s.sampleSum, s.quantiles, labelValues...)
 }
 
-// HistogramData keeps data required to build histogram Metric
+// HistogramData keeps data required to build histogram Metric.
 type HistogramData struct {
 	sampleCount uint64
 	sampleSum   float64
 	buckets     map[float64]uint64
 }
 
-// Adds histogram from gathered metrics to this histogram data.
+// AddHistogram adds histogram from gathered metrics to this histogram data.
+// Do not call this function after Metric() has been invoked, because histogram created by Metric
+// is using the buckets map (doesn't make a copy), and it's not allowed to change the buckets
+// after they've been passed to a prometheus.Metric.
 func (d *HistogramData) AddHistogram(histo *dto.Histogram) {
 	d.sampleCount += histo.GetSampleCount()
 	d.sampleSum += histo.GetSampleSum()
@@ -477,7 +480,10 @@ func (d *HistogramData) AddHistogram(histo *dto.Histogram) {
 	}
 }
 
-// Merged another histogram data into this one.
+// AddHistogramData merges another histogram data into this one.
+// Do not call this function after Metric() has been invoked, because histogram created by Metric
+// is using the buckets map (doesn't make a copy), and it's not allowed to change the buckets
+// after they've been passed to a prometheus.Metric.
 func (d *HistogramData) AddHistogramData(histo HistogramData) {
 	d.sampleCount += histo.sampleCount
 	d.sampleSum += histo.sampleSum
@@ -492,12 +498,22 @@ func (d *HistogramData) AddHistogramData(histo HistogramData) {
 	}
 }
 
-// Return prometheus metric from this histogram data.
+// Metric returns prometheus metric from this histogram data.
+//
+// Note that returned metric shares bucket with this HistogramData, so avoid
+// doing more modifications to this HistogramData after calling Metric.
 func (d *HistogramData) Metric(desc *prometheus.Desc, labelValues ...string) prometheus.Metric {
 	return prometheus.MustNewConstHistogram(desc, d.sampleCount, d.sampleSum, d.buckets, labelValues...)
 }
 
-// Creates new histogram data collector.
+// Copy returns a copy of this histogram data.
+func (d *HistogramData) Copy() *HistogramData {
+	cp := &HistogramData{}
+	cp.AddHistogramData(*d)
+	return cp
+}
+
+// NewHistogramDataCollector creates new histogram data collector.
 func NewHistogramDataCollector(desc *prometheus.Desc) *HistogramDataCollector {
 	return &HistogramDataCollector{
 		desc: desc,
@@ -522,7 +538,9 @@ func (h *HistogramDataCollector) Collect(out chan<- prometheus.Metric) {
 	h.dataMu.RLock()
 	defer h.dataMu.RUnlock()
 
-	out <- h.data.Metric(h.desc)
+	// We must create a copy of the HistogramData data structure before calling Metric()
+	// to honor its contract.
+	out <- h.data.Copy().Metric(h.desc)
 }
 
 func (h *HistogramDataCollector) Add(hd HistogramData) {