diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d525e89fd..2a5ba3cc60 100644 --- 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 index b6ca734c5d..1e14bff697 100644 --- 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) {