From 393af897fab5f9a613dae0ec7f9c84b1cef6e131 Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Mon, 28 Oct 2024 18:18:03 +0000 Subject: [PATCH 1/6] Include native histograms in benchmark --- modules/generator/generator_test.go | 2 +- .../generator/registry/native_histogram.go | 50 ++++++++++--------- modules/generator/registry/registry.go | 2 +- 3 files changed, 29 insertions(+), 25 deletions(-) diff --git a/modules/generator/generator_test.go b/modules/generator/generator_test.go index dcfafaca163..381ea0f325e 100644 --- a/modules/generator/generator_test.go +++ b/modules/generator/generator_test.go @@ -225,7 +225,7 @@ func BenchmarkCollect(b *testing.B) { spanMetricsDimensions: []string{"k8s.cluster.name", "k8s.namespace.name"}, spanMetricsEnableTargetInfo: true, spanMetricsTargetInfoExcludedDimensions: []string{"excluded}"}, - // nativeHistograms: overrides.HistogramMethodBoth, + nativeHistograms: overrides.HistogramMethodBoth, } ) diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go index 95596f6f315..0d9db548af4 100644 --- a/modules/generator/registry/native_histogram.go +++ b/modules/generator/registry/native_histogram.go @@ -39,11 +39,14 @@ type nativeHistogram struct { // generate. A diff in the configured value on the processors will cause a // reload of the process, and a new instance of the histogram to be created. histogramOverride HistogramMode + + externalLabels map[string]string } type nativeHistogramSeries struct { // labels should not be modified after creation - labels LabelPair + lb *labels.Builder + labels labels.Labels promHistogram prometheus.Histogram lastUpdated int64 histogram *dto.Histogram @@ -68,7 +71,7 @@ var ( _ metric = (*nativeHistogram)(nil) ) -func newNativeHistogram(name string, buckets []float64, onAddSeries func(uint32) bool, onRemoveSeries func(count uint32), traceIDLabelName string, histogramOverride HistogramMode) *nativeHistogram { +func newNativeHistogram(name string, buckets []float64, onAddSeries func(uint32) bool, onRemoveSeries func(count uint32), traceIDLabelName string, histogramOverride HistogramMode, externalLabels map[string]string) *nativeHistogram { if onAddSeries == nil { onAddSeries = func(uint32) bool { return true @@ -90,6 +93,7 @@ func newNativeHistogram(name string, buckets []float64, onAddSeries func(uint32) traceIDLabelName: traceIDLabelName, buckets: buckets, histogramOverride: histogramOverride, + externalLabels: externalLabels, } } @@ -116,7 +120,6 @@ func (h *nativeHistogram) ObserveWithExemplar(labelValueCombo *LabelValueCombo, func (h *nativeHistogram) newSeries(labelValueCombo *LabelValueCombo, value float64, traceID string, multiplier float64) *nativeHistogramSeries { newSeries := &nativeHistogramSeries{ // TODO move these labels in HistogramOpts.ConstLabels? - labels: labelValueCombo.getLabelPair(), promHistogram: prometheus.NewHistogram(prometheus.HistogramOpts{ Name: h.name(), Help: "Native histogram for metric " + h.name(), @@ -132,6 +135,18 @@ func (h *nativeHistogram) newSeries(labelValueCombo *LabelValueCombo, value floa h.updateSeries(newSeries, value, traceID, multiplier) + lbls := labelValueCombo.getLabelPair() + lb := labels.NewBuilder(make(labels.Labels, 1+len(lbls.names))) + + lb.Set(labels.MetricName, h.metricName) + + // set external labels + for name, value := range h.externalLabels { + lb.Set(name, value) + } + newSeries.labels = lb.Labels() + newSeries.lb = lb + return newSeries } @@ -149,27 +164,17 @@ func (h *nativeHistogram) name() string { return h.metricName } -func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64, externalLabels map[string]string) (activeSeries int, err error) { +func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64, _ map[string]string) (activeSeries int, err error) { h.seriesMtx.Lock() defer h.seriesMtx.Unlock() - lbls := make(labels.Labels, 1+len(externalLabels)) - lb := labels.NewBuilder(lbls) activeSeries = 0 - lb.Set(labels.MetricName, h.metricName) - - // set external labels - for name, value := range externalLabels { - lb.Set(name, value) - } - for _, s := range h.series { - // Set series-specific labels - for i, name := range s.labels.names { - lb.Set(name, s.labels.values[i]) - } + // for i, name := range s.labels.names { + // lb.Set(name, s.labels.values[i]) + // } // Extract histogram encodedMetric := &dto.Metric{} @@ -187,7 +192,7 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 // If we are in "both" or "classic" mode, also emit classic histograms. if hasClassicHistograms(h.histogramOverride) { - classicSeries, classicErr := h.classicHistograms(appender, lb, timeMs, s) + classicSeries, classicErr := h.classicHistograms(appender, s.lb, timeMs, s) if classicErr != nil { return activeSeries, classicErr } @@ -196,7 +201,7 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 // If we are in "both" or "native" mode, also emit native histograms. if hasNativeHistograms(h.histogramOverride) { - nativeErr := h.nativeHistograms(appender, lb, timeMs, s) + nativeErr := h.nativeHistograms(appender, s.labels, timeMs, s) if nativeErr != nil { return activeSeries, nativeErr } @@ -215,7 +220,7 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 Ts: convertTimestampToMs(encodedExemplar.GetTimestamp()), HasTs: true, } - _, err = appender.AppendExemplar(0, lb.Labels(), e) + _, err = appender.AppendExemplar(0, s.labels, e) if err != nil { return activeSeries, err } @@ -243,7 +248,7 @@ func (h *nativeHistogram) activeSeriesPerHistogramSerie() uint32 { return 1 } -func (h *nativeHistogram) nativeHistograms(appender storage.Appender, lb *labels.Builder, timeMs int64, s *nativeHistogramSeries) (err error) { +func (h *nativeHistogram) nativeHistograms(appender storage.Appender, lbls labels.Labels, timeMs int64, s *nativeHistogramSeries) (err error) { // Decode to Prometheus representation hist := promhistogram.Histogram{ Schema: s.histogram.GetSchema(), @@ -273,8 +278,7 @@ func (h *nativeHistogram) nativeHistograms(appender storage.Appender, lb *labels } hist.NegativeBuckets = s.histogram.NegativeDelta - lb.Set(labels.MetricName, h.metricName) - _, err = appender.AppendHistogram(0, lb.Labels(), timeMs, &hist, nil) + _, err = appender.AppendHistogram(0, lbls, timeMs, &hist, nil) if err != nil { return err } diff --git a/modules/generator/registry/registry.go b/modules/generator/registry/registry.go index 5b96f586b25..d7fca905032 100644 --- a/modules/generator/registry/registry.go +++ b/modules/generator/registry/registry.go @@ -156,7 +156,7 @@ func (r *ManagedRegistry) NewHistogram(name string, buckets []float64, histogram // are disabled, eventually the new implementation can handle all cases if hasNativeHistograms(histogramOverride) { - h = newNativeHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName, histogramOverride) + h = newNativeHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName, histogramOverride, r.externalLabels) } else { h = newHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName, r.externalLabels) } From 784ddd2b0f3fd940c3b7a743e865ece39cf040f3 Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Mon, 28 Oct 2024 19:26:10 +0000 Subject: [PATCH 2/6] Fix tests --- .../generator/registry/native_histogram.go | 24 +++++++++---------- .../registry/native_histogram_test.go | 4 ++-- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go index 0d9db548af4..714a23027af 100644 --- a/modules/generator/registry/native_histogram.go +++ b/modules/generator/registry/native_histogram.go @@ -113,8 +113,7 @@ func (h *nativeHistogram) ObserveWithExemplar(labelValueCombo *LabelValueCombo, return } - newSeries := h.newSeries(labelValueCombo, value, traceID, multiplier) - h.series[hash] = newSeries + h.series[hash] = h.newSeries(labelValueCombo, value, traceID, multiplier) } func (h *nativeHistogram) newSeries(labelValueCombo *LabelValueCombo, value float64, traceID string, multiplier float64) *nativeHistogramSeries { @@ -138,12 +137,17 @@ func (h *nativeHistogram) newSeries(labelValueCombo *LabelValueCombo, value floa lbls := labelValueCombo.getLabelPair() lb := labels.NewBuilder(make(labels.Labels, 1+len(lbls.names))) - lb.Set(labels.MetricName, h.metricName) - + // set series labels + for i, name := range lbls.names { + lb.Set(name, lbls.values[i]) + } // set external labels for name, value := range h.externalLabels { lb.Set(name, value) } + + lb.Set(labels.MetricName, h.metricName) + newSeries.labels = lb.Labels() newSeries.lb = lb @@ -171,11 +175,6 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 activeSeries = 0 for _, s := range h.series { - // Set series-specific labels - // for i, name := range s.labels.names { - // lb.Set(name, s.labels.values[i]) - // } - // Extract histogram encodedMetric := &dto.Metric{} @@ -185,9 +184,10 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 return activeSeries, err } - // NOTE: Store the encoded histogram here so we can keep track of the exemplars - // that have been sent. The value is updated here, but the pointers remain - // the same, and so Reset() call below can be used to clear the exemplars. + // NOTE: Store the encoded histogram here so we can keep track of the + // exemplars that have been sent. The value is updated here, but the + // pointers remain the same, and so Reset() call below can be used to clear + // the exemplars. s.histogram = encodedMetric.GetHistogram() // If we are in "both" or "classic" mode, also emit classic histograms. diff --git a/modules/generator/registry/native_histogram_test.go b/modules/generator/registry/native_histogram_test.go index 91ad19e34e2..504c6d4d44e 100644 --- a/modules/generator/registry/native_histogram_test.go +++ b/modules/generator/registry/native_histogram_test.go @@ -20,7 +20,7 @@ func Test_ObserveWithExemplar_duplicate(t *testing.T) { return true } - h := newNativeHistogram("my_histogram", []float64{0.1, 0.2}, onAdd, nil, "trace_id", HistogramModeBoth) + h := newNativeHistogram("my_histogram", []float64{0.1, 0.2}, onAdd, nil, "trace_id", HistogramModeBoth, nil) lv := newLabelValueCombo([]string{"label"}, []string{"value-1"}) @@ -463,7 +463,7 @@ func Test_Histograms(t *testing.T) { } onAdd := func(uint32) bool { return true } - h := newNativeHistogram("test_histogram", tc.buckets, onAdd, nil, "trace_id", HistogramModeBoth) + h := newNativeHistogram("test_histogram", tc.buckets, onAdd, nil, "trace_id", HistogramModeBoth, nil) testHistogram(t, h, tc.collections) }) }) From f367ceadb5d9c679ae13cadef8085833fef5123a Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Tue, 29 Oct 2024 15:39:18 +0000 Subject: [PATCH 3/6] Reduce calls to Labels() --- .../generator/registry/native_histogram.go | 33 +++++++++++++++---- 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go index 714a23027af..3fd42ad326d 100644 --- a/modules/generator/registry/native_histogram.go +++ b/modules/generator/registry/native_histogram.go @@ -1,6 +1,7 @@ package registry import ( + "fmt" "math" "sync" "time" @@ -41,6 +42,11 @@ type nativeHistogram struct { histogramOverride HistogramMode externalLabels map[string]string + + // classic + nameCount string + nameSum string + nameBucket string } type nativeHistogramSeries struct { @@ -56,6 +62,11 @@ type nativeHistogramSeries struct { // This avoids Prometheus throwing away the first value in the series, // due to the transition from null -> x. firstSeries *atomic.Bool + + // classic + countLabels labels.Labels + sumLabels labels.Labels + // bucketLabels []labels.Labels } func (hs *nativeHistogramSeries) isNew() bool { @@ -94,6 +105,11 @@ func newNativeHistogram(name string, buckets []float64, onAddSeries func(uint32) buckets: buckets, histogramOverride: histogramOverride, externalLabels: externalLabels, + + // classic + nameCount: fmt.Sprintf("%s_count", name), + nameSum: fmt.Sprintf("%s_sum", name), + nameBucket: fmt.Sprintf("%s_bucket", name), } } @@ -151,6 +167,14 @@ func (h *nativeHistogram) newSeries(labelValueCombo *LabelValueCombo, value floa newSeries.labels = lb.Labels() newSeries.lb = lb + // _count + lb.Set(labels.MetricName, h.nameCount) + newSeries.countLabels = lb.Labels() + + // _sum + lb.Set(labels.MetricName, h.nameSum) + newSeries.sumLabels = lb.Labels() + return newSeries } @@ -288,9 +312,8 @@ func (h *nativeHistogram) nativeHistograms(appender storage.Appender, lbls label func (h *nativeHistogram) classicHistograms(appender storage.Appender, lb *labels.Builder, timeMs int64, s *nativeHistogramSeries) (activeSeries int, err error) { if s.isNew() { - lb.Set(labels.MetricName, h.metricName+"_count") endOfLastMinuteMs := getEndOfLastMinuteMs(timeMs) - _, err = appender.Append(0, lb.Labels(), endOfLastMinuteMs, 0) + _, err = appender.Append(0, s.countLabels, endOfLastMinuteMs, 0) if err != nil { return activeSeries, err } @@ -298,16 +321,14 @@ func (h *nativeHistogram) classicHistograms(appender storage.Appender, lb *label } // sum - lb.Set(labels.MetricName, h.metricName+"_sum") - _, err = appender.Append(0, lb.Labels(), timeMs, s.histogram.GetSampleSum()) + _, err = appender.Append(0, s.sumLabels, timeMs, s.histogram.GetSampleSum()) if err != nil { return activeSeries, err } activeSeries++ // count - lb.Set(labels.MetricName, h.metricName+"_count") - _, err = appender.Append(0, lb.Labels(), timeMs, getIfGreaterThenZeroOr(s.histogram.GetSampleCountFloat(), s.histogram.GetSampleCount())) + _, err = appender.Append(0, s.countLabels, timeMs, getIfGreaterThenZeroOr(s.histogram.GetSampleCountFloat(), s.histogram.GetSampleCount())) if err != nil { return activeSeries, err } From 1cb964399993c7f8283a1b2e830030215c5aee96 Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Thu, 31 Oct 2024 14:26:16 +0000 Subject: [PATCH 4/6] Clean up todos --- modules/generator/registry/native_histogram.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go index 3fd42ad326d..3e3db660cd3 100644 --- a/modules/generator/registry/native_histogram.go +++ b/modules/generator/registry/native_histogram.go @@ -134,12 +134,10 @@ func (h *nativeHistogram) ObserveWithExemplar(labelValueCombo *LabelValueCombo, func (h *nativeHistogram) newSeries(labelValueCombo *LabelValueCombo, value float64, traceID string, multiplier float64) *nativeHistogramSeries { newSeries := &nativeHistogramSeries{ - // TODO move these labels in HistogramOpts.ConstLabels? promHistogram: prometheus.NewHistogram(prometheus.HistogramOpts{ - Name: h.name(), - Help: "Native histogram for metric " + h.name(), - Buckets: h.buckets, - // TODO check if these values are sensible and break them out + Name: h.name(), + Help: "Native histogram for metric " + h.name(), + Buckets: h.buckets, NativeHistogramBucketFactor: 1.1, NativeHistogramMaxBucketNumber: 100, NativeHistogramMinResetDuration: 15 * time.Minute, @@ -356,7 +354,6 @@ func (h *nativeHistogram) classicHistograms(appender storage.Appender, lb *label activeSeries++ if bucket.Exemplar != nil && len(bucket.Exemplar.Label) > 0 { - // TODO are we appending the same exemplar twice? _, err = appender.AppendExemplar(ref, lb.Labels(), exemplar.Exemplar{ Labels: convertLabelPairToLabels(bucket.Exemplar.GetLabel()), Value: bucket.Exemplar.GetValue(), From c6b71532603a34ac87070fdfc822445f062dd95d Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Thu, 31 Oct 2024 15:23:48 +0000 Subject: [PATCH 5/6] Reduce function signature on classicHistograms --- modules/generator/registry/native_histogram.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go index 3e3db660cd3..9ea5f761ae8 100644 --- a/modules/generator/registry/native_histogram.go +++ b/modules/generator/registry/native_histogram.go @@ -214,7 +214,7 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 // If we are in "both" or "classic" mode, also emit classic histograms. if hasClassicHistograms(h.histogramOverride) { - classicSeries, classicErr := h.classicHistograms(appender, s.lb, timeMs, s) + classicSeries, classicErr := h.classicHistograms(appender, timeMs, s) if classicErr != nil { return activeSeries, classicErr } @@ -308,7 +308,7 @@ func (h *nativeHistogram) nativeHistograms(appender storage.Appender, lbls label return } -func (h *nativeHistogram) classicHistograms(appender storage.Appender, lb *labels.Builder, timeMs int64, s *nativeHistogramSeries) (activeSeries int, err error) { +func (h *nativeHistogram) classicHistograms(appender storage.Appender, timeMs int64, s *nativeHistogramSeries) (activeSeries int, err error) { if s.isNew() { endOfLastMinuteMs := getEndOfLastMinuteMs(timeMs) _, err = appender.Append(0, s.countLabels, endOfLastMinuteMs, 0) @@ -333,7 +333,7 @@ func (h *nativeHistogram) classicHistograms(appender storage.Appender, lb *label activeSeries++ // bucket - lb.Set(labels.MetricName, h.metricName+"_bucket") + s.lb.Set(labels.MetricName, h.metricName+"_bucket") // the Prometheus histogram will sometimes add the +Inf bucket, it depends on whether there is an exemplar // for that bucket or not. To avoid adding it twice, keep track of it with this boolean. @@ -341,20 +341,20 @@ func (h *nativeHistogram) classicHistograms(appender storage.Appender, lb *label for _, bucket := range s.histogram.Bucket { // add "le" label - lb.Set(labels.BucketLabel, formatFloat(bucket.GetUpperBound())) + s.lb.Set(labels.BucketLabel, formatFloat(bucket.GetUpperBound())) if bucket.GetUpperBound() == math.Inf(1) { infBucketWasAdded = true } - ref, appendErr := appender.Append(0, lb.Labels(), timeMs, getIfGreaterThenZeroOr(bucket.GetCumulativeCountFloat(), bucket.GetCumulativeCount())) + ref, appendErr := appender.Append(0, s.lb.Labels(), timeMs, getIfGreaterThenZeroOr(bucket.GetCumulativeCountFloat(), bucket.GetCumulativeCount())) if appendErr != nil { return activeSeries, appendErr } activeSeries++ if bucket.Exemplar != nil && len(bucket.Exemplar.Label) > 0 { - _, err = appender.AppendExemplar(ref, lb.Labels(), exemplar.Exemplar{ + _, err = appender.AppendExemplar(ref, s.lb.Labels(), exemplar.Exemplar{ Labels: convertLabelPairToLabels(bucket.Exemplar.GetLabel()), Value: bucket.Exemplar.GetValue(), Ts: timeMs, @@ -368,9 +368,9 @@ func (h *nativeHistogram) classicHistograms(appender storage.Appender, lb *label if !infBucketWasAdded { // Add +Inf bucket - lb.Set(labels.BucketLabel, "+Inf") + s.lb.Set(labels.BucketLabel, "+Inf") - _, err = appender.Append(0, lb.Labels(), timeMs, getIfGreaterThenZeroOr(s.histogram.GetSampleCountFloat(), s.histogram.GetSampleCount())) + _, err = appender.Append(0, s.lb.Labels(), timeMs, getIfGreaterThenZeroOr(s.histogram.GetSampleCountFloat(), s.histogram.GetSampleCount())) if err != nil { return activeSeries, err } @@ -378,7 +378,7 @@ func (h *nativeHistogram) classicHistograms(appender storage.Appender, lb *label } // drop "le" label again - lb.Del(labels.BucketLabel) + s.lb.Del(labels.BucketLabel) return } From 69f5ae1ba2edbdc496fd554d7f80a31080a4ac11 Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Thu, 31 Oct 2024 16:57:37 +0000 Subject: [PATCH 6/6] Add additional TODOs --- modules/generator/registry/counter.go | 3 ++- modules/generator/registry/gauge.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/modules/generator/registry/counter.go b/modules/generator/registry/counter.go index 8f7f5af1a09..907a9e57b1f 100644 --- a/modules/generator/registry/counter.go +++ b/modules/generator/registry/counter.go @@ -137,6 +137,7 @@ func (c *counter) collectMetrics(appender storage.Appender, timeMs int64, extern // add metric name baseLabels = append(baseLabels, labels.Label{Name: labels.MetricName, Value: c.metricName}) + // TODO: avoid allocation on each collection lb := labels.NewBuilder(baseLabels) for _, s := range c.series { @@ -166,7 +167,7 @@ func (c *counter) collectMetrics(appender storage.Appender, timeMs int64, extern return } - // TODO support exemplars + // TODO: support exemplars } return diff --git a/modules/generator/registry/gauge.go b/modules/generator/registry/gauge.go index 3a8ec22d86b..e0ca82c882a 100644 --- a/modules/generator/registry/gauge.go +++ b/modules/generator/registry/gauge.go @@ -149,6 +149,7 @@ func (g *gauge) collectMetrics(appender storage.Appender, timeMs int64, external baseLabels = append(baseLabels, labels.Label{Name: name, Value: value}) } + // TODO: avoid allocation on each collection lb := labels.NewBuilder(baseLabels) for _, s := range g.series { @@ -166,7 +167,7 @@ func (g *gauge) collectMetrics(appender storage.Appender, timeMs int64, external if err != nil { return } - // TODO support exemplars + // TODO: support exemplars } return