From e028d7230fe28b06d4142d756b760b2fcd0944a5 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Tue, 4 Jun 2024 19:42:31 +0200 Subject: [PATCH 01/26] Initial setup native histograms --- modules/generator/overrides_test.go | 4 + modules/generator/registry/interface.go | 2 + .../generator/registry/native_histogram.go | 218 ++++++++++++++++++ .../registry/native_histogram_test.go | 3 + modules/generator/registry/overrides.go | 1 + modules/generator/registry/registry.go | 11 +- modules/generator/registry/registry_test.go | 4 + modules/generator/registry/test.go | 14 ++ modules/overrides/config.go | 17 +- modules/overrides/config_legacy.go | 2 + modules/overrides/interface.go | 1 + modules/overrides/runtime_config_overrides.go | 4 + 12 files changed, 271 insertions(+), 10 deletions(-) create mode 100644 modules/generator/registry/native_histogram.go create mode 100644 modules/generator/registry/native_histogram_test.go diff --git a/modules/generator/overrides_test.go b/modules/generator/overrides_test.go index 165424b6638..ce000fe9786 100644 --- a/modules/generator/overrides_test.go +++ b/modules/generator/overrides_test.go @@ -56,6 +56,10 @@ func (m *mockOverrides) MetricsGeneratorDisableCollection(string) bool { return false } +func (m *mockOverrides) MetricsGeneratorGenerateNativeHistograms(string) bool { + return false +} + func (m *mockOverrides) MetricsGenerationTraceIDLabelName(userID string) string { return "" } diff --git a/modules/generator/registry/interface.go b/modules/generator/registry/interface.go index 463c0fc2be0..01dfdd214aa 100644 --- a/modules/generator/registry/interface.go +++ b/modules/generator/registry/interface.go @@ -17,6 +17,8 @@ type Counter interface { // Histogram // https://prometheus.io/docs/concepts/metric_types/#histogram type Histogram interface { + metric + // ObserveWithExemplar observes a datapoint with the given values. traceID will be added as exemplar. ObserveWithExemplar(labelValueCombo *LabelValueCombo, value float64, traceID string, multiplier float64) } diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go new file mode 100644 index 00000000000..c8d9c29c868 --- /dev/null +++ b/modules/generator/registry/native_histogram.go @@ -0,0 +1,218 @@ +package registry + +import ( + "sync" + "time" + + "github.com/prometheus/client_golang/prometheus" + dto "github.com/prometheus/client_model/go" + promhistogram "github.com/prometheus/prometheus/model/histogram" + "github.com/prometheus/prometheus/model/labels" + "github.com/prometheus/prometheus/storage" + "go.uber.org/atomic" +) + +type nativeHistogram struct { + metricName string + + // TODO we can also switch to a HistrogramVec and let prometheus handle the labels. This would remove the series map + // and all locking around it. + // Downside: you need to list labels at creation time while our interfaces only pass labels at observe time, this + // will requires a bigger refactor, maybe something for a second pass? + // Might break processors that have variable amount of labels... + //promHistogram prometheus.HistogramVec + + seriesMtx sync.Mutex + series map[uint64]*nativeHistogramSeries + + onAddSerie func(count uint32) bool + onRemoveSerie func(count uint32) + + traceIDLabelName string +} + +type nativeHistogramSeries struct { + // labels should not be modified after creation + labels LabelPair + promHistogram prometheus.Histogram + lastUpdated *atomic.Int64 +} + +var ( + _ Histogram = (*nativeHistogram)(nil) + _ metric = (*nativeHistogram)(nil) +) + +func newNativeHistogram(name string, onAddSeries func(uint32) bool, onRemoveSeries func(count uint32), traceIDLabelName string) *nativeHistogram { + if onAddSeries == nil { + onAddSeries = func(uint32) bool { + return true + } + } + if onRemoveSeries == nil { + onRemoveSeries = func(uint32) {} + } + + if traceIDLabelName == "" { + traceIDLabelName = "traceID" + } + + return &nativeHistogram{ + metricName: name, + series: make(map[uint64]*nativeHistogramSeries), + onAddSerie: onAddSeries, + onRemoveSerie: onRemoveSeries, + traceIDLabelName: traceIDLabelName, + } +} + +func (h *nativeHistogram) ObserveWithExemplar(labelValueCombo *LabelValueCombo, value float64, traceID string, multiplier float64) { + hash := labelValueCombo.getHash() + + h.seriesMtx.Lock() + defer h.seriesMtx.Unlock() + + s, ok := h.series[hash] + if ok { + h.updateSeries(s, value, traceID, multiplier) + return + } + + if !h.onAddSerie(h.activeSeriesPerHistogramSerie()) { + return + } + + newSeries := h.newSeries(labelValueCombo, value, traceID, multiplier) + s, ok = h.series[hash] + if ok { + h.updateSeries(s, value, traceID, multiplier) + return + } + h.series[hash] = newSeries +} + +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(), + // TODO support help text + //Help: "", + // TODO we can set these to also emit a classic histogram + // Buckets: nil, + // TODO check if these values are sensible and break them out + NativeHistogramBucketFactor: 1.1, + NativeHistogramMaxBucketNumber: 100, + NativeHistogramMinResetDuration: 15 * time.Minute, + }), + lastUpdated: atomic.NewInt64(0), + } + + h.updateSeries(newSeries, value, traceID, multiplier) + + return newSeries +} + +func (h *nativeHistogram) updateSeries(s *nativeHistogramSeries, value float64, traceID string, multiplier float64) { + s.promHistogram.(prometheus.ExemplarObserver).ObserveWithExemplar( + value*multiplier, + map[string]string{h.traceIDLabelName: traceID}, + ) + s.lastUpdated.Store(time.Now().UnixMilli()) +} + +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) { + h.seriesMtx.Lock() + defer h.seriesMtx.Unlock() + + labelsCount := 0 + if h.series[0] != nil { + labelsCount = len(h.series[0].labels.names) + } + lbls := make(labels.Labels, 1+len(externalLabels)+labelsCount) + lb := labels.NewBuilder(lbls) + + 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]) + } + + // Append native histogram + encodedMetric := &dto.Metric{} + + // Encode to protobuf representation + err := s.promHistogram.Write(encodedMetric) + if err != nil { + return activeSeries, err + } + encodedHistogram := encodedMetric.GetHistogram() + + // Decode to Prometheus representation + h := promhistogram.Histogram{ + Schema: encodedHistogram.GetSchema(), + Count: encodedHistogram.GetSampleCount(), + Sum: encodedHistogram.GetSampleSum(), + ZeroThreshold: encodedHistogram.GetZeroThreshold(), + ZeroCount: encodedHistogram.GetZeroCount(), + } + if len(encodedHistogram.PositiveSpan) > 0 { + h.PositiveSpans = make([]promhistogram.Span, len(encodedHistogram.PositiveSpan)) + for i, span := range encodedHistogram.PositiveSpan { + h.PositiveSpans[i] = promhistogram.Span{ + Offset: span.GetOffset(), + Length: span.GetLength(), + } + } + } + h.PositiveBuckets = encodedHistogram.PositiveDelta + if len(encodedHistogram.NegativeSpan) > 0 { + h.NegativeSpans = make([]promhistogram.Span, len(encodedHistogram.NegativeSpan)) + for i, span := range encodedHistogram.NegativeSpan { + h.NegativeSpans[i] = promhistogram.Span{ + Offset: span.GetOffset(), + Length: span.GetLength(), + } + } + } + h.NegativeBuckets = encodedHistogram.NegativeDelta + + // TODO update activeSeries + + _, err = appender.AppendHistogram(0, lb.Labels(), timeMs, &h, nil) + if err != nil { + return activeSeries, err + } + } + + return +} + +func (h *nativeHistogram) removeStaleSeries(staleTimeMs int64) { + h.seriesMtx.Lock() + defer h.seriesMtx.Unlock() + for hash, s := range h.series { + if s.lastUpdated.Load() < staleTimeMs { + delete(h.series, hash) + h.onRemoveSerie(h.activeSeriesPerHistogramSerie()) + } + } +} + +func (h *nativeHistogram) activeSeriesPerHistogramSerie() uint32 { + // TODO can we estimate this? + return 1 +} diff --git a/modules/generator/registry/native_histogram_test.go b/modules/generator/registry/native_histogram_test.go new file mode 100644 index 00000000000..06e04a7134d --- /dev/null +++ b/modules/generator/registry/native_histogram_test.go @@ -0,0 +1,3 @@ +package registry + +// TODO 😶 diff --git a/modules/generator/registry/overrides.go b/modules/generator/registry/overrides.go index c308637be53..39482808e4d 100644 --- a/modules/generator/registry/overrides.go +++ b/modules/generator/registry/overrides.go @@ -10,6 +10,7 @@ type Overrides interface { MetricsGeneratorMaxActiveSeries(userID string) uint32 MetricsGeneratorCollectionInterval(userID string) time.Duration MetricsGeneratorDisableCollection(userID string) bool + MetricsGeneratorGenerateNativeHistograms(userID string) bool MetricsGenerationTraceIDLabelName(userID string) string } diff --git a/modules/generator/registry/registry.go b/modules/generator/registry/registry.go index be9f295c14c..cd2d24cf762 100644 --- a/modules/generator/registry/registry.go +++ b/modules/generator/registry/registry.go @@ -147,8 +147,15 @@ func (r *ManagedRegistry) NewCounter(name string) Counter { return c } -func (r *ManagedRegistry) NewHistogram(name string, buckets []float64) Histogram { - h := newHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, r.overrides.MetricsGenerationTraceIDLabelName(r.tenant)) +func (r *ManagedRegistry) NewHistogram(name string, buckets []float64) (h Histogram) { + traceIDLabelName := r.overrides.MetricsGenerationTraceIDLabelName(r.tenant) + + if !r.overrides.MetricsGeneratorGenerateNativeHistograms(r.tenant) { + h = newHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName) + } else { + h = newNativeHistogram(name, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName) + } + r.registerMetric(h) return h } diff --git a/modules/generator/registry/registry_test.go b/modules/generator/registry/registry_test.go index caf945851c6..db74034b5c5 100644 --- a/modules/generator/registry/registry_test.go +++ b/modules/generator/registry/registry_test.go @@ -316,6 +316,10 @@ func (m *mockOverrides) MetricsGeneratorDisableCollection(string) bool { return m.disableCollection } +func (m *mockOverrides) MetricsGeneratorGenerateNativeHistograms(userID string) bool { + return false +} + func (m *mockOverrides) MetricsGenerationTraceIDLabelName(string) string { return "" } diff --git a/modules/generator/registry/test.go b/modules/generator/registry/test.go index 0a19a794147..9edf81a9518 100644 --- a/modules/generator/registry/test.go +++ b/modules/generator/registry/test.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/prometheus/prometheus/model/labels" + "github.com/prometheus/prometheus/storage" ) // TestRegistry is a simple implementation of Registry intended for tests. It is not concurrent-safe. @@ -147,6 +148,7 @@ type testHistogram struct { } var _ Histogram = (*testHistogram)(nil) +var _ metric = (*testHistogram)(nil) func (t *testHistogram) ObserveWithExemplar(labelValueCombo *LabelValueCombo, value float64, _ string, multiplier float64) { lbls := make(labels.Labels, len(labelValueCombo.labels.names)) @@ -166,8 +168,20 @@ func (t *testHistogram) ObserveWithExemplar(labelValueCombo *LabelValueCombo, va t.registry.addToMetric(t.nameBucket, withLe(lbls, math.Inf(1)), 1*multiplier) } +func (t *testHistogram) name() string { + panic("implement me") +} + func withLe(lbls labels.Labels, le float64) labels.Labels { lb := labels.NewBuilder(lbls) lb.Set(labels.BucketLabel, formatFloat(le)) return lb.Labels() } + +func (t *testHistogram) collectMetrics(_ storage.Appender, _ int64, _ map[string]string) (activeSeries int, err error) { + panic("implement me") +} + +func (t *testHistogram) removeStaleSeries(int64) { + panic("implement me") +} diff --git a/modules/overrides/config.go b/modules/overrides/config.go index e5390895242..18ed85cef4c 100644 --- a/modules/overrides/config.go +++ b/modules/overrides/config.go @@ -127,16 +127,17 @@ func (h *RemoteWriteHeaders) toStringStringMap() map[string]string { } type MetricsGeneratorOverrides struct { - RingSize int `yaml:"ring_size,omitempty" json:"ring_size,omitempty"` - Processors listtomap.ListToMap `yaml:"processors,omitempty" json:"processors,omitempty"` - MaxActiveSeries uint32 `yaml:"max_active_series,omitempty" json:"max_active_series,omitempty"` - CollectionInterval time.Duration `yaml:"collection_interval,omitempty" json:"collection_interval,omitempty"` - DisableCollection bool `yaml:"disable_collection,omitempty" json:"disable_collection,omitempty"` - TraceIDLabelName string `yaml:"trace_id_label_name,omitempty" json:"trace_id_label_name,omitempty"` - RemoteWriteHeaders RemoteWriteHeaders `yaml:"remote_write_headers,omitempty" json:"remote_write_headers,omitempty"` + RingSize int `yaml:"ring_size,omitempty" json:"ring_size,omitempty"` + Processors listtomap.ListToMap `yaml:"processors,omitempty" json:"processors,omitempty"` + MaxActiveSeries uint32 `yaml:"max_active_series,omitempty" json:"max_active_series,omitempty"` + CollectionInterval time.Duration `yaml:"collection_interval,omitempty" json:"collection_interval,omitempty"` + DisableCollection bool `yaml:"disable_collection,omitempty" json:"disable_collection,omitempty"` + GenerateNativeHistograms bool `yaml:"generate_native_histograms" json:"generate_native_histograms,omitempty"` + TraceIDLabelName string `yaml:"trace_id_label_name,omitempty" json:"trace_id_label_name,omitempty"` - Forwarder ForwarderOverrides `yaml:"forwarder,omitempty" json:"forwarder,omitempty"` + RemoteWriteHeaders RemoteWriteHeaders `yaml:"remote_write_headers,omitempty" json:"remote_write_headers,omitempty"` + Forwarder ForwarderOverrides `yaml:"forwarder,omitempty" json:"forwarder,omitempty"` Processor ProcessorOverrides `yaml:"processor,omitempty" json:"processor,omitempty"` IngestionSlack time.Duration `yaml:"ingestion_time_range_slack" json:"ingestion_time_range_slack"` } diff --git a/modules/overrides/config_legacy.go b/modules/overrides/config_legacy.go index 9d33c0faf93..c659b8333de 100644 --- a/modules/overrides/config_legacy.go +++ b/modules/overrides/config_legacy.go @@ -28,6 +28,7 @@ func (c *Overrides) toLegacy() LegacyOverrides { MetricsGeneratorMaxActiveSeries: c.MetricsGenerator.MaxActiveSeries, MetricsGeneratorCollectionInterval: c.MetricsGenerator.CollectionInterval, MetricsGeneratorDisableCollection: c.MetricsGenerator.DisableCollection, + MetricsGeneratorGenerateNativeHistograms: c.MetricsGenerator.GenerateNativeHistograms, MetricsGeneratorTraceIDLabelName: c.MetricsGenerator.TraceIDLabelName, MetricsGeneratorRemoteWriteHeaders: c.MetricsGenerator.RemoteWriteHeaders, MetricsGeneratorForwarderQueueSize: c.MetricsGenerator.Forwarder.QueueSize, @@ -89,6 +90,7 @@ type LegacyOverrides struct { MetricsGeneratorMaxActiveSeries uint32 `yaml:"metrics_generator_max_active_series" json:"metrics_generator_max_active_series"` MetricsGeneratorCollectionInterval time.Duration `yaml:"metrics_generator_collection_interval" json:"metrics_generator_collection_interval"` MetricsGeneratorDisableCollection bool `yaml:"metrics_generator_disable_collection" json:"metrics_generator_disable_collection"` + MetricsGeneratorGenerateNativeHistograms bool `yaml:"metrics_generator_generate_native_histograms" json:"metrics_generator_generate_native_histograms"` MetricsGeneratorTraceIDLabelName string `yaml:"metrics_generator_trace_id_label_name" json:"metrics_generator_trace_id_label_name"` MetricsGeneratorForwarderQueueSize int `yaml:"metrics_generator_forwarder_queue_size" json:"metrics_generator_forwarder_queue_size"` MetricsGeneratorForwarderWorkers int `yaml:"metrics_generator_forwarder_workers" json:"metrics_generator_forwarder_workers"` diff --git a/modules/overrides/interface.go b/modules/overrides/interface.go index be32432859d..4d77026a8c7 100644 --- a/modules/overrides/interface.go +++ b/modules/overrides/interface.go @@ -46,6 +46,7 @@ type Interface interface { MetricsGeneratorMaxActiveSeries(userID string) uint32 MetricsGeneratorCollectionInterval(userID string) time.Duration MetricsGeneratorDisableCollection(userID string) bool + MetricsGeneratorGenerateNativeHistograms(userID string) bool MetricsGenerationTraceIDLabelName(userID string) string MetricsGeneratorRemoteWriteHeaders(userID string) map[string]string MetricsGeneratorForwarderQueueSize(userID string) int diff --git a/modules/overrides/runtime_config_overrides.go b/modules/overrides/runtime_config_overrides.go index 052f97795b6..5df119e8a46 100644 --- a/modules/overrides/runtime_config_overrides.go +++ b/modules/overrides/runtime_config_overrides.go @@ -396,6 +396,10 @@ func (o *runtimeConfigOverridesManager) MetricsGeneratorDisableCollection(userID return o.getOverridesForUser(userID).MetricsGenerator.DisableCollection } +func (o *runtimeConfigOverridesManager) MetricsGeneratorGenerateNativeHistograms(userID string) bool { + return o.getOverridesForUser(userID).MetricsGenerator.GenerateNativeHistograms +} + // MetricsGenerationTraceIDLabelName is the label name used for the trace ID in metrics. // "TraceID" is used if no value is provided. func (o *runtimeConfigOverridesManager) MetricsGenerationTraceIDLabelName(userID string) string { From 37a0ce51efb8c83c88d7295764859398b4e1aad8 Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Wed, 5 Jun 2024 21:24:50 +0200 Subject: [PATCH 02/26] Bump prometheus dependencies, map exemplars over --- .../docker-compose/local/docker-compose.yaml | 1 + example/docker-compose/shared/tempo.yaml | 1 + .../generator/registry/native_histogram.go | 42 +++++++++++++++- .../registry/native_histogram_test.go | 48 +++++++++++++++++++ modules/generator/registry/registry.go | 1 + 5 files changed, 91 insertions(+), 2 deletions(-) diff --git a/example/docker-compose/local/docker-compose.yaml b/example/docker-compose/local/docker-compose.yaml index a213684991d..8a6512ab916 100644 --- a/example/docker-compose/local/docker-compose.yaml +++ b/example/docker-compose/local/docker-compose.yaml @@ -42,6 +42,7 @@ services: - --config.file=/etc/prometheus.yaml - --web.enable-remote-write-receiver - --enable-feature=exemplar-storage + - --enable-feature=native-histograms volumes: - ../shared/prometheus.yaml:/etc/prometheus.yaml ports: diff --git a/example/docker-compose/shared/tempo.yaml b/example/docker-compose/shared/tempo.yaml index 11697c9bf48..f21f99659d7 100644 --- a/example/docker-compose/shared/tempo.yaml +++ b/example/docker-compose/shared/tempo.yaml @@ -57,3 +57,4 @@ overrides: defaults: metrics_generator: processors: [service-graphs, span-metrics, local-blocks] # enables metrics generator + generate_native_histograms: true diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go index c8d9c29c868..f964f6540ee 100644 --- a/modules/generator/registry/native_histogram.go +++ b/modules/generator/registry/native_histogram.go @@ -6,10 +6,12 @@ import ( "github.com/prometheus/client_golang/prometheus" dto "github.com/prometheus/client_model/go" + "github.com/prometheus/prometheus/model/exemplar" promhistogram "github.com/prometheus/prometheus/model/histogram" "github.com/prometheus/prometheus/model/labels" "github.com/prometheus/prometheus/storage" "go.uber.org/atomic" + "google.golang.org/protobuf/types/known/timestamppb" ) type nativeHistogram struct { @@ -98,9 +100,9 @@ func (h *nativeHistogram) newSeries(labelValueCombo *LabelValueCombo, value floa promHistogram: prometheus.NewHistogram(prometheus.HistogramOpts{ Name: h.name(), // TODO support help text - //Help: "", + Help: "Native histogram for metric " + h.name(), // TODO we can set these to also emit a classic histogram - // Buckets: nil, + Buckets: nil, // TODO check if these values are sensible and break them out NativeHistogramBucketFactor: 1.1, NativeHistogramMaxBucketNumber: 100, @@ -196,6 +198,24 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 if err != nil { return activeSeries, err } + + if len(encodedHistogram.Exemplars) > 0 { + for _, encodedExemplar := range encodedHistogram.Exemplars { + + e := exemplar.Exemplar{ + Labels: convertLabelPairToLabels(encodedExemplar.Label), + Value: encodedExemplar.GetValue(), + Ts: convertTimestampToMs(encodedExemplar.GetTimestamp()), + HasTs: true, + } + + _, err = appender.AppendExemplar(0, lb.Labels(), e) + if err != nil { + return activeSeries, err + } + } + } + } return @@ -216,3 +236,21 @@ func (h *nativeHistogram) activeSeriesPerHistogramSerie() uint32 { // TODO can we estimate this? return 1 } + +func convertLabelPairToLabels(lbps []*dto.LabelPair) labels.Labels { + lbs := make([]labels.Label, len(lbps)) + for i, lbp := range lbps { + lbs[i] = labels.Label{ + Name: lbp.GetName(), + Value: lbp.GetValue(), + } + } + return lbs +} + +func convertTimestampToMs(ts *timestamppb.Timestamp) int64 { + if ts == nil { + return 0 + } + return ts.GetSeconds()*1000 + int64(ts.GetNanos()/1_000_000) +} diff --git a/modules/generator/registry/native_histogram_test.go b/modules/generator/registry/native_histogram_test.go index 06e04a7134d..efec7d8bd14 100644 --- a/modules/generator/registry/native_histogram_test.go +++ b/modules/generator/registry/native_histogram_test.go @@ -1,3 +1,51 @@ package registry +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + // TODO 😶 + +func Test_native_histogram(t *testing.T) { + var seriesAdded int + onAdd := func(count uint32) bool { + seriesAdded++ + return true + } + + h := newNativeHistogram("my_histogram", onAdd, nil, "trace_id") + + h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-1"}), 1.0, "trace-1", 1.0) + h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-2"}), 1.5, "trace-2", 1.0) + + assert.Equal(t, 2, seriesAdded) + + collectionTimeMs := time.Now().UnixMilli() + expectedSamples := []sample{} + expectedExemplars := []exemplarSample{} + collectMetricAndAssert(t, h, collectionTimeMs, nil, 10, expectedSamples, expectedExemplars) + + h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-2"}), 2.5, "trace-2.2", 1.0) + h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-3"}), 3.0, "trace-3", 1.0) + + assert.Equal(t, 3, seriesAdded) + + collectionTimeMs = time.Now().UnixMilli() + expectedSamples = []sample{} + expectedExemplars = []exemplarSample{} + collectMetricAndAssert(t, h, collectionTimeMs, nil, 15, expectedSamples, expectedExemplars) + + h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-2"}), 2.5, "trace-2.2", 20.0) + h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-3"}), 3.0, "trace-3", 13.5) + h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-3"}), 1.0, "trace-3", 7.5) + + assert.Equal(t, 3, seriesAdded) + + collectionTimeMs = time.Now().UnixMilli() + expectedSamples = []sample{} + expectedExemplars = []exemplarSample{} + collectMetricAndAssert(t, h, collectionTimeMs, nil, 15, expectedSamples, expectedExemplars) +} diff --git a/modules/generator/registry/registry.go b/modules/generator/registry/registry.go index cd2d24cf762..e5eb3eb6050 100644 --- a/modules/generator/registry/registry.go +++ b/modules/generator/registry/registry.go @@ -153,6 +153,7 @@ func (r *ManagedRegistry) NewHistogram(name string, buckets []float64) (h Histog if !r.overrides.MetricsGeneratorGenerateNativeHistograms(r.tenant) { h = newHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName) } else { + level.Warn(r.logger).Log("msg", "creating native histogram!", "metric", name) h = newNativeHistogram(name, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName) } From 4424d58a0f8c3cd5956ff294c423780db76b669a Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Thu, 6 Jun 2024 15:59:13 +0000 Subject: [PATCH 03/26] Add GenerateNativeHistograms from legacy --- modules/overrides/config_legacy.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/modules/overrides/config_legacy.go b/modules/overrides/config_legacy.go index c659b8333de..9b1f9fed6ed 100644 --- a/modules/overrides/config_legacy.go +++ b/modules/overrides/config_legacy.go @@ -159,14 +159,15 @@ func (l *LegacyOverrides) toNewLimits() Overrides { CompactionWindow: l.CompactionWindow, }, MetricsGenerator: MetricsGeneratorOverrides{ - RingSize: l.MetricsGeneratorRingSize, - Processors: l.MetricsGeneratorProcessors, - MaxActiveSeries: l.MetricsGeneratorMaxActiveSeries, - CollectionInterval: l.MetricsGeneratorCollectionInterval, - DisableCollection: l.MetricsGeneratorDisableCollection, - TraceIDLabelName: l.MetricsGeneratorTraceIDLabelName, - IngestionSlack: l.MetricsGeneratorIngestionSlack, - RemoteWriteHeaders: l.MetricsGeneratorRemoteWriteHeaders, + RingSize: l.MetricsGeneratorRingSize, + Processors: l.MetricsGeneratorProcessors, + MaxActiveSeries: l.MetricsGeneratorMaxActiveSeries, + CollectionInterval: l.MetricsGeneratorCollectionInterval, + DisableCollection: l.MetricsGeneratorDisableCollection, + TraceIDLabelName: l.MetricsGeneratorTraceIDLabelName, + IngestionSlack: l.MetricsGeneratorIngestionSlack, + RemoteWriteHeaders: l.MetricsGeneratorRemoteWriteHeaders, + GenerateNativeHistograms: l.MetricsGeneratorGenerateNativeHistograms, Forwarder: ForwarderOverrides{ QueueSize: l.MetricsGeneratorForwarderQueueSize, Workers: l.MetricsGeneratorForwarderWorkers, From bc01e93a183b2f8d7962dda2d160f40c575a87e9 Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Thu, 6 Jun 2024 16:17:11 +0000 Subject: [PATCH 04/26] Add test coverage for legacy overrides --- modules/overrides/config_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/modules/overrides/config_test.go b/modules/overrides/config_test.go index db8aa7f1f37..71cad3c1f0c 100644 --- a/modules/overrides/config_test.go +++ b/modules/overrides/config_test.go @@ -139,6 +139,7 @@ metrics_generator_processor_local_blocks_max_block_bytes: 10 metrics_generator_processor_local_blocks_flush_check_period: 11s metrics_generator_processor_local_blocks_trace_idle_period: 12s metrics_generator_processor_local_blocks_complete_block_timeout: 13s +metrics_generator_generate_native_histograms: true block_retention: 14s max_bytes_per_tag_values_query: 15 max_blocks_per_tag_values_query: 16 @@ -183,6 +184,7 @@ defaults: forwarder: queue_size: 6 workers: 7 + generate_native_histograms: true processor: service_graphs: histogram_buckets: From 5492faded0b70b7b8bd41a3a2591ec79ead88853 Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Fri, 7 Jun 2024 14:22:17 +0000 Subject: [PATCH 05/26] Plumb overrides into remote write config generation and test --- modules/generator/instance_test.go | 4 ++ modules/generator/overrides.go | 1 + modules/generator/registry/appender_test.go | 9 ++++ .../generator/registry/native_histogram.go | 39 ++++++++--------- .../registry/native_histogram_test.go | 28 +++++++++--- modules/generator/registry/registry.go | 2 +- modules/generator/registry/test.go | 6 ++- modules/generator/storage/config_util.go | 6 ++- modules/generator/storage/config_util_test.go | 23 ++++++++-- modules/generator/storage/instance.go | 43 +++++++++++-------- modules/generator/storage/instance_test.go | 9 +++- modules/generator/storage/overrides.go | 1 + 12 files changed, 116 insertions(+), 55 deletions(-) diff --git a/modules/generator/instance_test.go b/modules/generator/instance_test.go index 3f6d7a9de65..6536f975856 100644 --- a/modules/generator/instance_test.go +++ b/modules/generator/instance_test.go @@ -397,3 +397,7 @@ func (n noopAppender) Rollback() error { return nil } func (n noopAppender) UpdateMetadata(prometheus_storage.SeriesRef, labels.Labels, metadata.Metadata) (prometheus_storage.SeriesRef, error) { return 0, nil } + +func (n noopAppender) AppendCTZeroSample(ref prometheus_storage.SeriesRef, l labels.Labels, t, ct int64) (prometheus_storage.SeriesRef, error) { + return 0, nil +} diff --git a/modules/generator/overrides.go b/modules/generator/overrides.go index 377f1a26cbc..15a57a7639b 100644 --- a/modules/generator/overrides.go +++ b/modules/generator/overrides.go @@ -15,6 +15,7 @@ type metricsGeneratorOverrides interface { registry.Overrides storage.Overrides + MetricsGeneratorGenerateNativeHistograms(userID string) bool MetricsGeneratorIngestionSlack(userID string) time.Duration MetricsGeneratorProcessors(userID string) map[string]struct{} MetricsGeneratorProcessorServiceGraphsHistogramBuckets(userID string) []float64 diff --git a/modules/generator/registry/appender_test.go b/modules/generator/registry/appender_test.go index cc01b23d260..09faddd8d80 100644 --- a/modules/generator/registry/appender_test.go +++ b/modules/generator/registry/appender_test.go @@ -10,6 +10,7 @@ import ( "github.com/prometheus/prometheus/model/labels" "github.com/prometheus/prometheus/model/metadata" "github.com/prometheus/prometheus/storage" + prometheus_storage "github.com/prometheus/prometheus/storage" ) type noopAppender struct{} @@ -41,6 +42,10 @@ func (n noopAppender) UpdateMetadata(storage.SeriesRef, labels.Labels, metadata. return 0, nil } +func (n noopAppender) AppendCTZeroSample(ref prometheus_storage.SeriesRef, l labels.Labels, t, ct int64) (prometheus_storage.SeriesRef, error) { + return 0, nil +} + type capturingAppender struct { samples []sample exemplars []exemplarSample @@ -118,3 +123,7 @@ func (c *capturingAppender) Rollback() error { func (c *capturingAppender) UpdateMetadata(storage.SeriesRef, labels.Labels, metadata.Metadata) (storage.SeriesRef, error) { return 0, nil } + +func (c *capturingAppender) AppendCTZeroSample(ref prometheus_storage.SeriesRef, l labels.Labels, t, ct int64) (prometheus_storage.SeriesRef, error) { + return 0, nil +} diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go index f964f6540ee..0f4bc259f6e 100644 --- a/modules/generator/registry/native_histogram.go +++ b/modules/generator/registry/native_histogram.go @@ -22,7 +22,7 @@ type nativeHistogram struct { // Downside: you need to list labels at creation time while our interfaces only pass labels at observe time, this // will requires a bigger refactor, maybe something for a second pass? // Might break processors that have variable amount of labels... - //promHistogram prometheus.HistogramVec + // promHistogram prometheus.HistogramVec seriesMtx sync.Mutex series map[uint64]*nativeHistogramSeries @@ -30,6 +30,8 @@ type nativeHistogram struct { onAddSerie func(count uint32) bool onRemoveSerie func(count uint32) + buckets []float64 + traceIDLabelName string } @@ -45,7 +47,7 @@ var ( _ metric = (*nativeHistogram)(nil) ) -func newNativeHistogram(name string, onAddSeries func(uint32) bool, onRemoveSeries func(count uint32), traceIDLabelName string) *nativeHistogram { +func newNativeHistogram(name string, buckets []float64, onAddSeries func(uint32) bool, onRemoveSeries func(count uint32), traceIDLabelName string) *nativeHistogram { if onAddSeries == nil { onAddSeries = func(uint32) bool { return true @@ -65,6 +67,7 @@ func newNativeHistogram(name string, onAddSeries func(uint32) bool, onRemoveSeri onAddSerie: onAddSeries, onRemoveSerie: onRemoveSeries, traceIDLabelName: traceIDLabelName, + buckets: buckets, } } @@ -85,11 +88,6 @@ func (h *nativeHistogram) ObserveWithExemplar(labelValueCombo *LabelValueCombo, } newSeries := h.newSeries(labelValueCombo, value, traceID, multiplier) - s, ok = h.series[hash] - if ok { - h.updateSeries(s, value, traceID, multiplier) - return - } h.series[hash] = newSeries } @@ -98,11 +96,9 @@ func (h *nativeHistogram) newSeries(labelValueCombo *LabelValueCombo, value floa // TODO move these labels in HistogramOpts.ConstLabels? labels: labelValueCombo.getLabelPair(), promHistogram: prometheus.NewHistogram(prometheus.HistogramOpts{ - Name: h.name(), - // TODO support help text - Help: "Native histogram for metric " + h.name(), - // TODO we can set these to also emit a classic histogram - Buckets: nil, + Name: h.name(), + Help: "Native histogram for metric " + h.name(), + Buckets: h.buckets, // TODO check if these values are sensible and break them out NativeHistogramBucketFactor: 1.1, NativeHistogramMaxBucketNumber: 100, @@ -138,6 +134,7 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 } lbls := make(labels.Labels, 1+len(externalLabels)+labelsCount) lb := labels.NewBuilder(lbls) + activeSeries = len(h.series) lb.Set(labels.MetricName, h.metricName) @@ -164,7 +161,7 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 encodedHistogram := encodedMetric.GetHistogram() // Decode to Prometheus representation - h := promhistogram.Histogram{ + hist := promhistogram.Histogram{ Schema: encodedHistogram.GetSchema(), Count: encodedHistogram.GetSampleCount(), Sum: encodedHistogram.GetSampleSum(), @@ -172,29 +169,27 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 ZeroCount: encodedHistogram.GetZeroCount(), } if len(encodedHistogram.PositiveSpan) > 0 { - h.PositiveSpans = make([]promhistogram.Span, len(encodedHistogram.PositiveSpan)) + hist.PositiveSpans = make([]promhistogram.Span, len(encodedHistogram.PositiveSpan)) for i, span := range encodedHistogram.PositiveSpan { - h.PositiveSpans[i] = promhistogram.Span{ + hist.PositiveSpans[i] = promhistogram.Span{ Offset: span.GetOffset(), Length: span.GetLength(), } } } - h.PositiveBuckets = encodedHistogram.PositiveDelta + hist.PositiveBuckets = encodedHistogram.PositiveDelta if len(encodedHistogram.NegativeSpan) > 0 { - h.NegativeSpans = make([]promhistogram.Span, len(encodedHistogram.NegativeSpan)) + hist.NegativeSpans = make([]promhistogram.Span, len(encodedHistogram.NegativeSpan)) for i, span := range encodedHistogram.NegativeSpan { - h.NegativeSpans[i] = promhistogram.Span{ + hist.NegativeSpans[i] = promhistogram.Span{ Offset: span.GetOffset(), Length: span.GetLength(), } } } - h.NegativeBuckets = encodedHistogram.NegativeDelta - - // TODO update activeSeries + hist.NegativeBuckets = encodedHistogram.NegativeDelta - _, err = appender.AppendHistogram(0, lb.Labels(), timeMs, &h, nil) + _, err = appender.AppendHistogram(0, lb.Labels(), timeMs, &hist, nil) if err != nil { return activeSeries, err } diff --git a/modules/generator/registry/native_histogram_test.go b/modules/generator/registry/native_histogram_test.go index efec7d8bd14..12674952eb2 100644 --- a/modules/generator/registry/native_histogram_test.go +++ b/modules/generator/registry/native_histogram_test.go @@ -7,16 +7,14 @@ import ( "github.com/stretchr/testify/assert" ) -// TODO 😶 - func Test_native_histogram(t *testing.T) { var seriesAdded int onAdd := func(count uint32) bool { - seriesAdded++ + seriesAdded += int(count) return true } - h := newNativeHistogram("my_histogram", onAdd, nil, "trace_id") + h := newNativeHistogram("my_histogram", nil, onAdd, nil, "trace_id") h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-1"}), 1.0, "trace-1", 1.0) h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-2"}), 1.5, "trace-2", 1.0) @@ -26,7 +24,7 @@ func Test_native_histogram(t *testing.T) { collectionTimeMs := time.Now().UnixMilli() expectedSamples := []sample{} expectedExemplars := []exemplarSample{} - collectMetricAndAssert(t, h, collectionTimeMs, nil, 10, expectedSamples, expectedExemplars) + collectMetricAndAssert(t, h, collectionTimeMs, nil, 2, expectedSamples, expectedExemplars) h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-2"}), 2.5, "trace-2.2", 1.0) h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-3"}), 3.0, "trace-3", 1.0) @@ -36,7 +34,7 @@ func Test_native_histogram(t *testing.T) { collectionTimeMs = time.Now().UnixMilli() expectedSamples = []sample{} expectedExemplars = []exemplarSample{} - collectMetricAndAssert(t, h, collectionTimeMs, nil, 15, expectedSamples, expectedExemplars) + collectMetricAndAssert(t, h, collectionTimeMs, nil, 3, expectedSamples, expectedExemplars) h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-2"}), 2.5, "trace-2.2", 20.0) h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-3"}), 3.0, "trace-3", 13.5) @@ -47,5 +45,21 @@ func Test_native_histogram(t *testing.T) { collectionTimeMs = time.Now().UnixMilli() expectedSamples = []sample{} expectedExemplars = []exemplarSample{} - collectMetricAndAssert(t, h, collectionTimeMs, nil, 15, expectedSamples, expectedExemplars) + collectMetricAndAssert(t, h, collectionTimeMs, nil, 3, expectedSamples, expectedExemplars) +} + +// Duplicate labels should not grow the series count. +func Test_ObserveWithExemplar_duplicate(t *testing.T) { + var seriesAdded int + onAdd := func(count uint32) bool { + seriesAdded += int(count) + return true + } + h := newNativeHistogram("my_histogram", []float64{0.1, 0.2}, onAdd, nil, "trace_id") + + lv := newLabelValueCombo([]string{"label"}, []string{"value-1"}) + + h.ObserveWithExemplar(lv, 1.0, "trace-1", 1.0) + h.ObserveWithExemplar(lv, 1.1, "trace-1", 1.0) + assert.Equal(t, 1, seriesAdded) } diff --git a/modules/generator/registry/registry.go b/modules/generator/registry/registry.go index e5eb3eb6050..538aa8dd2b2 100644 --- a/modules/generator/registry/registry.go +++ b/modules/generator/registry/registry.go @@ -154,7 +154,7 @@ func (r *ManagedRegistry) NewHistogram(name string, buckets []float64) (h Histog h = newHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName) } else { level.Warn(r.logger).Log("msg", "creating native histogram!", "metric", name) - h = newNativeHistogram(name, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName) + h = newNativeHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName) } r.registerMetric(h) diff --git a/modules/generator/registry/test.go b/modules/generator/registry/test.go index 9edf81a9518..0489595122b 100644 --- a/modules/generator/registry/test.go +++ b/modules/generator/registry/test.go @@ -147,8 +147,10 @@ type testHistogram struct { registry *TestRegistry } -var _ Histogram = (*testHistogram)(nil) -var _ metric = (*testHistogram)(nil) +var ( + _ Histogram = (*testHistogram)(nil) + _ metric = (*testHistogram)(nil) +) func (t *testHistogram) ObserveWithExemplar(labelValueCombo *LabelValueCombo, value float64, _ string, multiplier float64) { lbls := make(labels.Labels, len(labelValueCombo.labels.names)) diff --git a/modules/generator/storage/config_util.go b/modules/generator/storage/config_util.go index 0aacbf1f6ef..b277305cb62 100644 --- a/modules/generator/storage/config_util.go +++ b/modules/generator/storage/config_util.go @@ -13,7 +13,7 @@ import ( // generateTenantRemoteWriteConfigs creates a copy of the remote write configurations with the // X-Scope-OrgID header present for the given tenant, unless Tempo is run in single tenant mode or instructed not to add X-Scope-OrgID header. -func generateTenantRemoteWriteConfigs(originalCfgs []prometheus_config.RemoteWriteConfig, tenant string, headers map[string]string, addOrgIDHeader bool, logger log.Logger) []*prometheus_config.RemoteWriteConfig { +func generateTenantRemoteWriteConfigs(originalCfgs []prometheus_config.RemoteWriteConfig, tenant string, headers map[string]string, addOrgIDHeader bool, logger log.Logger, sendNativeHistograms bool) []*prometheus_config.RemoteWriteConfig { var cloneCfgs []*prometheus_config.RemoteWriteConfig for _, originalCfg := range originalCfgs { @@ -42,6 +42,10 @@ func generateTenantRemoteWriteConfigs(originalCfgs []prometheus_config.RemoteWri cloneCfg.Headers[k] = v } + cloneCfg.SendNativeHistograms = sendNativeHistograms + // TODO: enable exemplars + // cloneCfg.SendExemplars = sendExemplars + cloneCfgs = append(cloneCfgs, cloneCfg) } diff --git a/modules/generator/storage/config_util_test.go b/modules/generator/storage/config_util_test.go index ac88751e3fb..3fbea7bc281 100644 --- a/modules/generator/storage/config_util_test.go +++ b/modules/generator/storage/config_util_test.go @@ -32,7 +32,7 @@ func Test_generateTenantRemoteWriteConfigs(t *testing.T) { addOrgIDHeader := true - result := generateTenantRemoteWriteConfigs(original, "my-tenant", nil, addOrgIDHeader, logger) + result := generateTenantRemoteWriteConfigs(original, "my-tenant", nil, addOrgIDHeader, logger, false) assert.Equal(t, original[0].URL, result[0].URL) assert.Equal(t, map[string]string{}, original[0].Headers, "Original headers have been modified") @@ -61,7 +61,7 @@ func Test_generateTenantRemoteWriteConfigs_singleTenant(t *testing.T) { addOrgIDHeader := true - result := generateTenantRemoteWriteConfigs(original, util.FakeTenantID, nil, addOrgIDHeader, logger) + result := generateTenantRemoteWriteConfigs(original, util.FakeTenantID, nil, addOrgIDHeader, logger, false) assert.Equal(t, original[0].URL, result[0].URL) @@ -95,7 +95,7 @@ func Test_generateTenantRemoteWriteConfigs_addOrgIDHeader(t *testing.T) { addOrgIDHeader := false - result := generateTenantRemoteWriteConfigs(original, "my-tenant", nil, addOrgIDHeader, logger) + result := generateTenantRemoteWriteConfigs(original, "my-tenant", nil, addOrgIDHeader, logger, false) assert.Equal(t, original[0].URL, result[0].URL) assert.Empty(t, original[0].Headers, "X-Scope-OrgID header is not added") @@ -104,6 +104,23 @@ func Test_generateTenantRemoteWriteConfigs_addOrgIDHeader(t *testing.T) { assert.Equal(t, map[string]string{"foo": "bar", "x-scope-orgid": "fake-tenant"}, result[1].Headers, "Original headers not modified") } +func Test_generateTenantRemoteWriteConfigs_sendNativeHistograms(t *testing.T) { + logger := log.NewLogfmtLogger(log.NewSyncWriter(os.Stdout)) + + original := []prometheus_config.RemoteWriteConfig{ + { + URL: &prometheus_common_config.URL{URL: urlMustParse("http://prometheus-1/api/prom/push")}, + Headers: map[string]string{}, + }, + } + + result := generateTenantRemoteWriteConfigs(original, "my-tenant", nil, false, logger, true) + assert.Equal(t, true, result[0].SendNativeHistograms, "SendNativeHistograms should be true") + + result = generateTenantRemoteWriteConfigs(original, "my-tenant", nil, false, logger, false) + assert.Equal(t, false, result[0].SendNativeHistograms, "SendNativeHistograms should be true") +} + func Test_copyMap(t *testing.T) { original := map[string]string{ "k1": "v1", diff --git a/modules/generator/storage/instance.go b/modules/generator/storage/instance.go index 42426076eb2..129090449a9 100644 --- a/modules/generator/storage/instance.go +++ b/modules/generator/storage/instance.go @@ -21,10 +21,10 @@ import ( tsdb_errors "github.com/prometheus/prometheus/tsdb/errors" ) -var metricStorageHeadersUpdateFailed = promauto.NewCounterVec(prometheus.CounterOpts{ +var metricStorageRemoteWriteUpdateFailed = promauto.NewCounterVec(prometheus.CounterOpts{ Namespace: "tempo", - Name: "metrics_generator_storage_headers_update_failed_total", - Help: "The total number of times updating the remote write headers failed", + Name: "metrics_generator_storage_remote_write_update_failed_total", + Help: "The total number of times updating the remote write configueration failed", }, []string{"tenant"}) type Storage interface { @@ -40,10 +40,14 @@ type storageImpl struct { remote *remote.Storage storage storage.Storage - tenantID string - currentHeaders map[string]string - overrides Overrides - closeCh chan struct{} + tenantID string + + // Cached from the overrides + currentHeaders map[string]string + sendNativeHistograms bool + + overrides Overrides + closeCh chan struct{} logger log.Logger } @@ -81,8 +85,10 @@ func New(cfg *Config, o Overrides, tenant string, reg prometheus.Registerer, log remoteStorage := remote.NewStorage(log.With(logger, "component", "remote"), reg, startTimeCallback, walDir, cfg.RemoteWriteFlushDeadline, &noopScrapeManager{}) headers := o.MetricsGeneratorRemoteWriteHeaders(tenant) + sendNativeHistograms := o.MetricsGeneratorGenerateNativeHistograms(tenant) + remoteStorageConfig := &prometheus_config.Config{ - RemoteWriteConfigs: generateTenantRemoteWriteConfigs(cfg.RemoteWrite, tenant, headers, cfg.RemoteWriteAddOrgIDHeader, logger), + RemoteWriteConfigs: generateTenantRemoteWriteConfigs(cfg.RemoteWrite, tenant, headers, cfg.RemoteWriteAddOrgIDHeader, logger, sendNativeHistograms), } err = remoteStorage.ApplyConfig(remoteStorageConfig) @@ -102,10 +108,11 @@ func New(cfg *Config, o Overrides, tenant string, reg prometheus.Registerer, log remote: remoteStorage, storage: storage.NewFanout(logger, wal, remoteStorage), - tenantID: tenant, - currentHeaders: headers, - overrides: o, - closeCh: make(chan struct{}), + tenantID: tenant, + currentHeaders: headers, + sendNativeHistograms: sendNativeHistograms, + overrides: o, + closeCh: make(chan struct{}), logger: logger, } @@ -141,15 +148,17 @@ func (s *storageImpl) watchOverrides() { select { case <-t.C: newHeaders := s.overrides.MetricsGeneratorRemoteWriteHeaders(s.tenantID) - if !headersEqual(s.currentHeaders, newHeaders) { - level.Info(s.logger).Log("msg", "updating remote write headers") + newSendNativeHistograms := s.overrides.MetricsGeneratorGenerateNativeHistograms(s.tenantID) + if !headersEqual(s.currentHeaders, newHeaders) || s.sendNativeHistograms != newSendNativeHistograms { + level.Info(s.logger).Log("msg", "updating remote write configuration") s.currentHeaders = newHeaders + s.sendNativeHistograms = newSendNativeHistograms err := s.remote.ApplyConfig(&prometheus_config.Config{ - RemoteWriteConfigs: generateTenantRemoteWriteConfigs(s.cfg.RemoteWrite, s.tenantID, newHeaders, s.cfg.RemoteWriteAddOrgIDHeader, s.logger), + RemoteWriteConfigs: generateTenantRemoteWriteConfigs(s.cfg.RemoteWrite, s.tenantID, newHeaders, s.cfg.RemoteWriteAddOrgIDHeader, s.logger, newSendNativeHistograms), }) if err != nil { - metricStorageHeadersUpdateFailed.WithLabelValues(s.tenantID).Inc() - level.Error(s.logger).Log("msg", "Failed to update remote write headers. Remote write will continue with old headers", "err", err) + metricStorageRemoteWriteUpdateFailed.WithLabelValues(s.tenantID).Inc() + level.Error(s.logger).Log("msg", "Failed to update remote write configuration. Remote write will continue with configuration", "err", err) } } case <-s.closeCh: diff --git a/modules/generator/storage/instance_test.go b/modules/generator/storage/instance_test.go index f3c8bce762a..eae0ac257a6 100644 --- a/modules/generator/storage/instance_test.go +++ b/modules/generator/storage/instance_test.go @@ -203,7 +203,7 @@ func TestInstance_remoteWriteHeaders(t *testing.T) { headers := map[string]string{user.OrgIDHeaderName: "my-other-tenant"} - instance, err := New(&cfg, &mockOverrides{headers}, "test-tenant", &noopRegisterer{}, logger) + instance, err := New(&cfg, &mockOverrides{headers, false}, "test-tenant", &noopRegisterer{}, logger) require.NoError(t, err) // Refuse requests - the WAL should buffer data until requests succeed @@ -361,13 +361,18 @@ func waitUntil(timeout time.Duration, f func() bool) error { var _ Overrides = (*mockOverrides)(nil) type mockOverrides struct { - headers map[string]string + headers map[string]string + nativeHistograms bool } func (m *mockOverrides) MetricsGeneratorRemoteWriteHeaders(string) map[string]string { return m.headers } +func (m *mockOverrides) MetricsGeneratorGenerateNativeHistograms(string) bool { + return m.nativeHistograms +} + var _ prometheus.Registerer = (*noopRegisterer)(nil) type noopRegisterer struct{} diff --git a/modules/generator/storage/overrides.go b/modules/generator/storage/overrides.go index 8d91ef05aa4..645152183fa 100644 --- a/modules/generator/storage/overrides.go +++ b/modules/generator/storage/overrides.go @@ -2,4 +2,5 @@ package storage type Overrides interface { MetricsGeneratorRemoteWriteHeaders(userID string) map[string]string + MetricsGeneratorGenerateNativeHistograms(userID string) bool } From 1bed650452779abf1da634151e29a2f1c77d2111 Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Tue, 18 Jun 2024 20:32:59 +0000 Subject: [PATCH 06/26] Lint for unused vars and duplicate imports --- modules/generator/instance_test.go | 2 +- modules/generator/registry/appender_test.go | 5 ++--- modules/generator/registry/registry_test.go | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/modules/generator/instance_test.go b/modules/generator/instance_test.go index 6536f975856..9e0af7bd289 100644 --- a/modules/generator/instance_test.go +++ b/modules/generator/instance_test.go @@ -398,6 +398,6 @@ func (n noopAppender) UpdateMetadata(prometheus_storage.SeriesRef, labels.Labels return 0, nil } -func (n noopAppender) AppendCTZeroSample(ref prometheus_storage.SeriesRef, l labels.Labels, t, ct int64) (prometheus_storage.SeriesRef, error) { +func (n noopAppender) AppendCTZeroSample(_ prometheus_storage.SeriesRef, _ labels.Labels, _, _ int64) (prometheus_storage.SeriesRef, error) { return 0, nil } diff --git a/modules/generator/registry/appender_test.go b/modules/generator/registry/appender_test.go index 09faddd8d80..bdce640d67b 100644 --- a/modules/generator/registry/appender_test.go +++ b/modules/generator/registry/appender_test.go @@ -10,7 +10,6 @@ import ( "github.com/prometheus/prometheus/model/labels" "github.com/prometheus/prometheus/model/metadata" "github.com/prometheus/prometheus/storage" - prometheus_storage "github.com/prometheus/prometheus/storage" ) type noopAppender struct{} @@ -42,7 +41,7 @@ func (n noopAppender) UpdateMetadata(storage.SeriesRef, labels.Labels, metadata. return 0, nil } -func (n noopAppender) AppendCTZeroSample(ref prometheus_storage.SeriesRef, l labels.Labels, t, ct int64) (prometheus_storage.SeriesRef, error) { +func (n noopAppender) AppendCTZeroSample(_ storage.SeriesRef, _ labels.Labels, _, _ int64) (storage.SeriesRef, error) { return 0, nil } @@ -124,6 +123,6 @@ func (c *capturingAppender) UpdateMetadata(storage.SeriesRef, labels.Labels, met return 0, nil } -func (c *capturingAppender) AppendCTZeroSample(ref prometheus_storage.SeriesRef, l labels.Labels, t, ct int64) (prometheus_storage.SeriesRef, error) { +func (c *capturingAppender) AppendCTZeroSample(_ storage.SeriesRef, _ labels.Labels, _, _ int64) (storage.SeriesRef, error) { return 0, nil } diff --git a/modules/generator/registry/registry_test.go b/modules/generator/registry/registry_test.go index db74034b5c5..4c7bc1094be 100644 --- a/modules/generator/registry/registry_test.go +++ b/modules/generator/registry/registry_test.go @@ -316,7 +316,7 @@ func (m *mockOverrides) MetricsGeneratorDisableCollection(string) bool { return m.disableCollection } -func (m *mockOverrides) MetricsGeneratorGenerateNativeHistograms(userID string) bool { +func (m *mockOverrides) MetricsGeneratorGenerateNativeHistograms(_ string) bool { return false } From 87305efcd06d9cfc5acf05cd8994db9f98a8de1b Mon Sep 17 00:00:00 2001 From: Koenraad Verheyden Date: Wed, 19 Jun 2024 20:05:23 +0200 Subject: [PATCH 07/26] Map 'classic' histograms out of prom.Histogram --- .../generator/registry/native_histogram.go | 71 +++++++++++- .../registry/native_histogram_test.go | 105 ++++++++++++++++-- modules/generator/registry/registry.go | 1 + 3 files changed, 163 insertions(+), 14 deletions(-) diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go index 0f4bc259f6e..42ff839724e 100644 --- a/modules/generator/registry/native_histogram.go +++ b/modules/generator/registry/native_histogram.go @@ -134,7 +134,8 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 } lbls := make(labels.Labels, 1+len(externalLabels)+labelsCount) lb := labels.NewBuilder(lbls) - activeSeries = len(h.series) + //activeSeries = len(h.series) + activeSeries = 0 lb.Set(labels.MetricName, h.metricName) @@ -145,12 +146,12 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 for _, s := range h.series { - // set series-specific labels + // Set series-specific labels for i, name := range s.labels.names { lb.Set(name, s.labels.values[i]) } - // Append native histogram + // Extract histogram encodedMetric := &dto.Metric{} // Encode to protobuf representation @@ -160,6 +161,65 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 } encodedHistogram := encodedMetric.GetHistogram() + // *** Classic histogram + + // sum + lb.Set(labels.MetricName, h.metricName+"_sum") + _, err = appender.Append(0, lb.Labels(), timeMs, encodedHistogram.GetSampleSum()) + if err != nil { + return activeSeries, err + } + activeSeries += 1 + + // count + lb.Set(labels.MetricName, h.metricName+"_count") + _, err = appender.Append(0, lb.Labels(), timeMs, encodedHistogram.GetSampleCountFloat()) + if err != nil { + return activeSeries, err + } + activeSeries += 1 + + // bucket + lb.Set(labels.MetricName, h.metricName+"_bucket") + + for _, bucket := range encodedHistogram.Bucket { + // add "le" label + lb.Set(labels.BucketLabel, formatFloat(bucket.GetUpperBound())) + + ref, err := appender.Append(0, lb.Labels(), timeMs, bucket.GetCumulativeCountFloat()) + if err != nil { + return activeSeries, err + } + activeSeries += 1 + + ex := bucket.Exemplar + if ex != nil { + // TODO are we appending the same exemplar twice? + _, err = appender.AppendExemplar(ref, lb.Labels(), exemplar.Exemplar{ + Labels: convertLabelPairToLabels(ex.GetLabel()), + Value: ex.GetValue(), + Ts: timeMs, + }) + if err != nil { + return activeSeries, err + } + } + } + + // Add +Inf bucket + lb.Set(labels.BucketLabel, "+Inf") + + _, err = appender.Append(0, lb.Labels(), timeMs, encodedHistogram.GetSampleSum()) + if err != nil { + return activeSeries, err + } + activeSeries += 1 + + // drop "le" label again + lb.Del(labels.BucketLabel) + + // *** Native histogram + // Decode to Prometheus representation hist := promhistogram.Histogram{ Schema: encodedHistogram.GetSchema(), @@ -189,21 +249,24 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 } hist.NegativeBuckets = encodedHistogram.NegativeDelta + lb.Set(labels.MetricName, h.metricName) _, err = appender.AppendHistogram(0, lb.Labels(), timeMs, &hist, nil) if err != nil { return activeSeries, err } + // TODO impact on active series from appending a histogram? + activeSeries += 0 if len(encodedHistogram.Exemplars) > 0 { for _, encodedExemplar := range encodedHistogram.Exemplars { + // TODO are we appending the same exemplar twice? e := exemplar.Exemplar{ Labels: convertLabelPairToLabels(encodedExemplar.Label), Value: encodedExemplar.GetValue(), Ts: convertTimestampToMs(encodedExemplar.GetTimestamp()), HasTs: true, } - _, err = appender.AppendExemplar(0, lb.Labels(), e) if err != nil { return activeSeries, err diff --git a/modules/generator/registry/native_histogram_test.go b/modules/generator/registry/native_histogram_test.go index 12674952eb2..b7cc1aac4e0 100644 --- a/modules/generator/registry/native_histogram_test.go +++ b/modules/generator/registry/native_histogram_test.go @@ -4,17 +4,21 @@ import ( "testing" "time" + "github.com/prometheus/prometheus/model/exemplar" + "github.com/prometheus/prometheus/model/labels" "github.com/stretchr/testify/assert" ) func Test_native_histogram(t *testing.T) { + // Note this test does not verify the correctness of the native histograms + var seriesAdded int onAdd := func(count uint32) bool { seriesAdded += int(count) return true } - h := newNativeHistogram("my_histogram", nil, onAdd, nil, "trace_id") + h := newNativeHistogram("my_histogram", []float64{1, 2}, onAdd, nil, "trace_id") h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-1"}), 1.0, "trace-1", 1.0) h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-2"}), 1.5, "trace-2", 1.0) @@ -22,9 +26,31 @@ func Test_native_histogram(t *testing.T) { assert.Equal(t, 2, seriesAdded) collectionTimeMs := time.Now().UnixMilli() - expectedSamples := []sample{} - expectedExemplars := []exemplarSample{} - collectMetricAndAssert(t, h, collectionTimeMs, nil, 2, expectedSamples, expectedExemplars) + expectedSamples := []sample{ + newSample(map[string]string{"__name__": "my_histogram_count", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "+Inf"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_count", "label": "value-2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_sum", "label": "value-2"}, collectionTimeMs, 1.5), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "1"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "+Inf"}, collectionTimeMs, 1), + } + expectedExemplars := []exemplarSample{ + newExemplar(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "1"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-1"}), + Value: 1.0, + Ts: collectionTimeMs, + }), + newExemplar(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "2"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-2"}), + Value: 1.5, + Ts: collectionTimeMs, + }), + } + collectMetricAndAssert(t, h, collectionTimeMs, nil, 10, expectedSamples, expectedExemplars) h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-2"}), 2.5, "trace-2.2", 1.0) h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-3"}), 3.0, "trace-3", 1.0) @@ -32,9 +58,36 @@ func Test_native_histogram(t *testing.T) { assert.Equal(t, 3, seriesAdded) collectionTimeMs = time.Now().UnixMilli() - expectedSamples = []sample{} - expectedExemplars = []exemplarSample{} - collectMetricAndAssert(t, h, collectionTimeMs, nil, 3, expectedSamples, expectedExemplars) + expectedSamples = []sample{ + newSample(map[string]string{"__name__": "my_histogram_count", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "+Inf"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_count", "label": "value-2"}, collectionTimeMs, 2), + newSample(map[string]string{"__name__": "my_histogram_sum", "label": "value-2"}, collectionTimeMs, 4.0), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "1"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "+Inf"}, collectionTimeMs, 2), + newSample(map[string]string{"__name__": "my_histogram_count", "label": "value-3"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_sum", "label": "value-3"}, collectionTimeMs, 3), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "1"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "2"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "+Inf"}, collectionTimeMs, 1), + } + expectedExemplars = []exemplarSample{ + newExemplar(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "+Inf"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-2.2"}), + Value: 2.5, + Ts: collectionTimeMs, + }), + newExemplar(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "+Inf"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-3"}), + Value: 3.0, + Ts: collectionTimeMs, + }), + } + collectMetricAndAssert(t, h, collectionTimeMs, nil, 15, expectedSamples, expectedExemplars) h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-2"}), 2.5, "trace-2.2", 20.0) h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-3"}), 3.0, "trace-3", 13.5) @@ -43,9 +96,41 @@ func Test_native_histogram(t *testing.T) { assert.Equal(t, 3, seriesAdded) collectionTimeMs = time.Now().UnixMilli() - expectedSamples = []sample{} - expectedExemplars = []exemplarSample{} - collectMetricAndAssert(t, h, collectionTimeMs, nil, 3, expectedSamples, expectedExemplars) + expectedSamples = []sample{ + newSample(map[string]string{"__name__": "my_histogram_count", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "+Inf"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_count", "label": "value-2"}, collectionTimeMs, 22), + newSample(map[string]string{"__name__": "my_histogram_sum", "label": "value-2"}, collectionTimeMs, 54.0), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "1"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "+Inf"}, collectionTimeMs, 22), + newSample(map[string]string{"__name__": "my_histogram_count", "label": "value-3"}, collectionTimeMs, 22.0), + newSample(map[string]string{"__name__": "my_histogram_sum", "label": "value-3"}, collectionTimeMs, 51.0), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "1"}, collectionTimeMs, 7.5), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "2"}, collectionTimeMs, 7.5), + newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "+Inf"}, collectionTimeMs, 22.0), + } + expectedExemplars = []exemplarSample{ + newExemplar(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "+Inf"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-2.2"}), + Value: 2.5, + Ts: collectionTimeMs, + }), + newExemplar(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "1"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-3"}), + Value: 1.0, + Ts: collectionTimeMs, + }), + newExemplar(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "+Inf"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-3"}), + Value: 3.0, + Ts: collectionTimeMs, + }), + } + collectMetricAndAssert(t, h, collectionTimeMs, nil, 15, expectedSamples, expectedExemplars) } // Duplicate labels should not grow the series count. diff --git a/modules/generator/registry/registry.go b/modules/generator/registry/registry.go index 538aa8dd2b2..75e52362599 100644 --- a/modules/generator/registry/registry.go +++ b/modules/generator/registry/registry.go @@ -150,6 +150,7 @@ func (r *ManagedRegistry) NewCounter(name string) Counter { func (r *ManagedRegistry) NewHistogram(name string, buckets []float64) (h Histogram) { traceIDLabelName := r.overrides.MetricsGenerationTraceIDLabelName(r.tenant) + // Temporary switch: use the old implementation when native histograms are disabled, eventually the new implementation can handle all cases if !r.overrides.MetricsGeneratorGenerateNativeHistograms(r.tenant) { h = newHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName) } else { From 9ee1f6aaf01accc0d0cd14191daf4188a39a78da Mon Sep 17 00:00:00 2001 From: yuna Date: Thu, 20 Jun 2024 18:39:46 +0200 Subject: [PATCH 08/26] More tweaking to get classic histograms working, not there yet though :( --- modules/generator/registry/counter_test.go | 9 ++++ .../generator/registry/native_histogram.go | 46 +++++++++++++------ 2 files changed, 42 insertions(+), 13 deletions(-) diff --git a/modules/generator/registry/counter_test.go b/modules/generator/registry/counter_test.go index cc0a9e39f98..d4ce706e52d 100644 --- a/modules/generator/registry/counter_test.go +++ b/modules/generator/registry/counter_test.go @@ -1,6 +1,7 @@ package registry import ( + "fmt" "math/rand" "sync" "testing" @@ -270,5 +271,13 @@ func collectMetricAndAssert(t *testing.T, m metric, collectionTimeMs int64, exte assert.False(t, appender.isCommitted) assert.False(t, appender.isRolledback) assert.ElementsMatch(t, expectedSamples, appender.samples) + fmt.Println("Expected samples:") + for _, expectedSample := range expectedSamples { + fmt.Println(" - ", expectedSample.l, expectedSample.v) + } + fmt.Println("Appender samples:") + for _, sample := range appender.samples { + fmt.Println(" - ", sample.l, sample.v) + } assert.ElementsMatch(t, expectedExemplars, appender.exemplars) } diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go index 42ff839724e..17cf9fcd84e 100644 --- a/modules/generator/registry/native_histogram.go +++ b/modules/generator/registry/native_histogram.go @@ -1,6 +1,7 @@ package registry import ( + "math" "sync" "time" @@ -113,10 +114,12 @@ func (h *nativeHistogram) newSeries(labelValueCombo *LabelValueCombo, value floa } func (h *nativeHistogram) updateSeries(s *nativeHistogramSeries, value float64, traceID string, multiplier float64) { - s.promHistogram.(prometheus.ExemplarObserver).ObserveWithExemplar( - value*multiplier, - map[string]string{h.traceIDLabelName: traceID}, - ) + for i := 0.0; i < multiplier; i++ { + s.promHistogram.(prometheus.ExemplarObserver).ObserveWithExemplar( + value, + map[string]string{h.traceIDLabelName: traceID}, + ) + } s.lastUpdated.Store(time.Now().UnixMilli()) } @@ -134,7 +137,6 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 } lbls := make(labels.Labels, 1+len(externalLabels)+labelsCount) lb := labels.NewBuilder(lbls) - //activeSeries = len(h.series) activeSeries = 0 lb.Set(labels.MetricName, h.metricName) @@ -173,7 +175,7 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 // count lb.Set(labels.MetricName, h.metricName+"_count") - _, err = appender.Append(0, lb.Labels(), timeMs, encodedHistogram.GetSampleCountFloat()) + _, err = appender.Append(0, lb.Labels(), timeMs, getIfGreaterThenZeroOr(encodedHistogram.GetSampleCountFloat(), encodedHistogram.GetSampleCount())) if err != nil { return activeSeries, err } @@ -182,11 +184,19 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 // bucket 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. + infBucketWasAdded := false + for _, bucket := range encodedHistogram.Bucket { // add "le" label lb.Set(labels.BucketLabel, formatFloat(bucket.GetUpperBound())) - ref, err := appender.Append(0, lb.Labels(), timeMs, bucket.GetCumulativeCountFloat()) + if bucket.GetUpperBound() == math.Inf(1) { + infBucketWasAdded = true + } + + ref, err := appender.Append(0, lb.Labels(), timeMs, getIfGreaterThenZeroOr(bucket.GetCumulativeCountFloat(), bucket.GetCumulativeCount())) if err != nil { return activeSeries, err } @@ -206,14 +216,16 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 } } - // Add +Inf bucket - lb.Set(labels.BucketLabel, "+Inf") + if !infBucketWasAdded { + // Add +Inf bucket + lb.Set(labels.BucketLabel, "+Inf") - _, err = appender.Append(0, lb.Labels(), timeMs, encodedHistogram.GetSampleSum()) - if err != nil { - return activeSeries, err + _, err = appender.Append(0, lb.Labels(), timeMs, getIfGreaterThenZeroOr(encodedHistogram.GetSampleCountFloat(), encodedHistogram.GetSampleCount())) + if err != nil { + return activeSeries, err + } + activeSeries += 1 } - activeSeries += 1 // drop "le" label again lb.Del(labels.BucketLabel) @@ -312,3 +324,11 @@ func convertTimestampToMs(ts *timestamppb.Timestamp) int64 { } return ts.GetSeconds()*1000 + int64(ts.GetNanos()/1_000_000) } + +// getIfGreaterThenZeroOr returns v1 is if it's greater than zero, otherwise it returns v2. +func getIfGreaterThenZeroOr(v1 float64, v2 uint64) float64 { + if v1 > 0 { + return v1 + } + return float64(v2) +} From e42038dd99885db2ab50041f705d87226c2c55ce Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Thu, 20 Jun 2024 17:44:57 +0000 Subject: [PATCH 09/26] Lint increment --- modules/generator/registry/native_histogram.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go index 17cf9fcd84e..d60e794ccc2 100644 --- a/modules/generator/registry/native_histogram.go +++ b/modules/generator/registry/native_histogram.go @@ -171,7 +171,7 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 if err != nil { return activeSeries, err } - activeSeries += 1 + activeSeries++ // count lb.Set(labels.MetricName, h.metricName+"_count") @@ -179,7 +179,7 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 if err != nil { return activeSeries, err } - activeSeries += 1 + activeSeries++ // bucket lb.Set(labels.MetricName, h.metricName+"_bucket") @@ -200,7 +200,7 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 if err != nil { return activeSeries, err } - activeSeries += 1 + activeSeries++ ex := bucket.Exemplar if ex != nil { From d6687fc645360c4dcef02b18fe3ba41e7b59085a Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Thu, 20 Jun 2024 17:44:57 +0000 Subject: [PATCH 10/26] Lint increment --- modules/generator/registry/native_histogram.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go index d60e794ccc2..ce56cb2bff1 100644 --- a/modules/generator/registry/native_histogram.go +++ b/modules/generator/registry/native_histogram.go @@ -224,7 +224,7 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 if err != nil { return activeSeries, err } - activeSeries += 1 + activeSeries++ } // drop "le" label again From 6cf37c94825698c7d6f007da49b95581a920ed72 Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Fri, 21 Jun 2024 20:11:10 +0000 Subject: [PATCH 11/26] Refactor native histogram tests --- .../registry/native_histogram_test.go | 580 ++++++++++++++---- 1 file changed, 459 insertions(+), 121 deletions(-) diff --git a/modules/generator/registry/native_histogram_test.go b/modules/generator/registry/native_histogram_test.go index b7cc1aac4e0..550df5b94e4 100644 --- a/modules/generator/registry/native_histogram_test.go +++ b/modules/generator/registry/native_histogram_test.go @@ -1,150 +1,488 @@ package registry import ( + "sort" "testing" "time" "github.com/prometheus/prometheus/model/exemplar" "github.com/prometheus/prometheus/model/labels" + "github.com/prometheus/prometheus/storage" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func Test_native_histogram(t *testing.T) { - // Note this test does not verify the correctness of the native histograms - +// Duplicate labels should not grow the series count. +func Test_ObserveWithExemplar_duplicate(t *testing.T) { var seriesAdded int onAdd := func(count uint32) bool { seriesAdded += int(count) return true } + h := newNativeHistogram("my_histogram", []float64{0.1, 0.2}, onAdd, nil, "trace_id") - h := newNativeHistogram("my_histogram", []float64{1, 2}, onAdd, nil, "trace_id") - - h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-1"}), 1.0, "trace-1", 1.0) - h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-2"}), 1.5, "trace-2", 1.0) + lv := newLabelValueCombo([]string{"label"}, []string{"value-1"}) - assert.Equal(t, 2, seriesAdded) + h.ObserveWithExemplar(lv, 1.0, "trace-1", 1.0) + h.ObserveWithExemplar(lv, 1.1, "trace-1", 1.0) + assert.Equal(t, 1, seriesAdded) +} - collectionTimeMs := time.Now().UnixMilli() - expectedSamples := []sample{ - newSample(map[string]string{"__name__": "my_histogram_count", "label": "value-1"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "1"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "2"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "+Inf"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_count", "label": "value-2"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_sum", "label": "value-2"}, collectionTimeMs, 1.5), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "1"}, collectionTimeMs, 0), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "2"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "+Inf"}, collectionTimeMs, 1), - } - expectedExemplars := []exemplarSample{ - newExemplar(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "1"}, exemplar.Exemplar{ - Labels: labels.FromMap(map[string]string{"trace_id": "trace-1"}), - Value: 1.0, - Ts: collectionTimeMs, - }), - newExemplar(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "2"}, exemplar.Exemplar{ - Labels: labels.FromMap(map[string]string{"trace_id": "trace-2"}), - Value: 1.5, - Ts: collectionTimeMs, - }), +func Test_Histograms(t *testing.T) { + // A single observations has a label value combo, a value, and a multiplier. + type observations []struct { + labelValueCombo *LabelValueCombo + value float64 + multiplier float64 + traceID string } - collectMetricAndAssert(t, h, collectionTimeMs, nil, 10, expectedSamples, expectedExemplars) - - h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-2"}), 2.5, "trace-2.2", 1.0) - h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-3"}), 3.0, "trace-3", 1.0) - - assert.Equal(t, 3, seriesAdded) - - collectionTimeMs = time.Now().UnixMilli() - expectedSamples = []sample{ - newSample(map[string]string{"__name__": "my_histogram_count", "label": "value-1"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "1"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "2"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "+Inf"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_count", "label": "value-2"}, collectionTimeMs, 2), - newSample(map[string]string{"__name__": "my_histogram_sum", "label": "value-2"}, collectionTimeMs, 4.0), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "1"}, collectionTimeMs, 0), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "2"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "+Inf"}, collectionTimeMs, 2), - newSample(map[string]string{"__name__": "my_histogram_count", "label": "value-3"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_sum", "label": "value-3"}, collectionTimeMs, 3), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "1"}, collectionTimeMs, 0), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "2"}, collectionTimeMs, 0), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "+Inf"}, collectionTimeMs, 1), + + // A single collection has a few observations, and some expectations. This + // allows the test to track the accumulation of observations over a series of + // collections. + type collections []struct { + observations observations + expectedSamples []sample + expectedExemplars []exemplarSample + expectedSeriesCount int } - expectedExemplars = []exemplarSample{ - newExemplar(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "+Inf"}, exemplar.Exemplar{ - Labels: labels.FromMap(map[string]string{"trace_id": "trace-2.2"}), - Value: 2.5, - Ts: collectionTimeMs, - }), - newExemplar(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "+Inf"}, exemplar.Exemplar{ - Labels: labels.FromMap(map[string]string{"trace_id": "trace-3"}), - Value: 3.0, - Ts: collectionTimeMs, - }), + + collectionTimeMs := time.Now().UnixMilli() + + cases := []struct { + name string + buckets []float64 + collections collections + }{ + { + name: "single collection single observation", + buckets: []float64{1, 2}, + collections: collections{ + { + observations: observations{ + { + labelValueCombo: newLabelValueCombo([]string{"label"}, []string{"value-1"}), + value: 1.0, + multiplier: 1.0, + traceID: "trace-1", + }, + }, + expectedSeriesCount: 5, + expectedSamples: []sample{ + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "+Inf"}, collectionTimeMs, 1), + }, + expectedExemplars: []exemplarSample{ + newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "1"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-1"}), + Value: 1.0, + Ts: collectionTimeMs, + }), + }, + }, + }, + }, + { + name: "single collection double observation", + buckets: []float64{1, 2}, + collections: collections{ + { + observations: observations{ + { + labelValueCombo: newLabelValueCombo([]string{"label"}, []string{"value-1"}), + value: 1.0, + multiplier: 1.0, + traceID: "trace-1", + }, + { + labelValueCombo: newLabelValueCombo([]string{"label"}, []string{"value-2"}), + value: 1.5, + multiplier: 1.0, + traceID: "trace-2", + }, + }, + expectedSeriesCount: 10, + expectedSamples: []sample{ + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "+Inf"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-2"}, collectionTimeMs, 1.5), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "1"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "+Inf"}, collectionTimeMs, 1), + }, + expectedExemplars: []exemplarSample{ + newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "1"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-1"}), + Value: 1.0, + Ts: collectionTimeMs, + }), + newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "2"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-2"}), + Value: 1.5, + Ts: collectionTimeMs, + }), + }, + }, + }, + }, + { + name: "double collection double observation", + buckets: []float64{1, 2}, + collections: collections{ + { + observations: observations{ + { + labelValueCombo: newLabelValueCombo([]string{"label"}, []string{"value-1"}), + value: 1.0, + multiplier: 1.0, + traceID: "trace-1", + }, + { + labelValueCombo: newLabelValueCombo([]string{"label"}, []string{"value-2"}), + value: 1.5, + multiplier: 1.0, + traceID: "trace-2", + }, + }, + expectedSeriesCount: 10, + expectedSamples: []sample{ + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "+Inf"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-2"}, collectionTimeMs, 1.5), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "1"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "+Inf"}, collectionTimeMs, 1), + }, + expectedExemplars: []exemplarSample{ + newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "1"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-1"}), + Value: 1.0, + Ts: collectionTimeMs, + }), + newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "2"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-2"}), + Value: 1.5, + Ts: collectionTimeMs, + }), + }, + }, + { + observations: observations{ + { + labelValueCombo: newLabelValueCombo([]string{"label"}, []string{"value-2"}), + value: 2.5, + multiplier: 1.0, + traceID: "trace-2", + }, + { + labelValueCombo: newLabelValueCombo([]string{"label"}, []string{"value-3"}), + value: 3.0, + multiplier: 1.0, + traceID: "trace-3", + }, + }, + expectedSeriesCount: 15, + expectedSamples: []sample{ + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "+Inf"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-2"}, collectionTimeMs, 2), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-2"}, collectionTimeMs, 4), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "1"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "+Inf"}, collectionTimeMs, 2), + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-3"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-3"}, collectionTimeMs, 3), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-3", "le": "1"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-3", "le": "2"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-3", "le": "+Inf"}, collectionTimeMs, 1), + }, + expectedExemplars: []exemplarSample{ + newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "+Inf"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-2"}), + Value: 2.5, + Ts: collectionTimeMs, + }), + newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-3", "le": "+Inf"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-3"}), + Value: 3, + Ts: collectionTimeMs, + }), + }, + }, + }, + }, + { + name: "many observations with some multipliers", + buckets: []float64{1, 2}, + collections: collections{ + { + observations: observations{ + { + labelValueCombo: newLabelValueCombo([]string{"label"}, []string{"value-1"}), + value: 1.0, + multiplier: 1.0, + traceID: "trace-1", + }, + { + labelValueCombo: newLabelValueCombo([]string{"label"}, []string{"value-2"}), + value: 1.5, + multiplier: 1.0, + traceID: "trace-2", + }, + }, + expectedSeriesCount: 10, + expectedSamples: []sample{ + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "+Inf"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-2"}, collectionTimeMs, 1.5), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "1"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "+Inf"}, collectionTimeMs, 1), + }, + expectedExemplars: []exemplarSample{ + newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "1"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-1"}), + Value: 1.0, + Ts: collectionTimeMs, + }), + newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "2"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-2"}), + Value: 1.5, + Ts: collectionTimeMs, + }), + }, + }, + { + observations: observations{ + { + labelValueCombo: newLabelValueCombo([]string{"label"}, []string{"value-2"}), + value: 2.5, + multiplier: 1.0, + traceID: "trace-2", + }, + { + labelValueCombo: newLabelValueCombo([]string{"label"}, []string{"value-3"}), + value: 3.0, + multiplier: 1.0, + traceID: "trace-3", + }, + }, + expectedSeriesCount: 15, + expectedSamples: []sample{ + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "+Inf"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-2"}, collectionTimeMs, 2), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-2"}, collectionTimeMs, 4), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "1"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "+Inf"}, collectionTimeMs, 2), + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-3"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-3"}, collectionTimeMs, 3), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-3", "le": "1"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-3", "le": "2"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-3", "le": "+Inf"}, collectionTimeMs, 1), + }, + expectedExemplars: []exemplarSample{ + newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "+Inf"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-2"}), + Value: 2.5, + Ts: collectionTimeMs, + }), + newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-3", "le": "+Inf"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-3"}), + Value: 3, + Ts: collectionTimeMs, + }), + }, + }, + { + observations: observations{ + { + labelValueCombo: newLabelValueCombo([]string{"label"}, []string{"value-2"}), + value: 2.5, + multiplier: 20.0, + traceID: "trace-2.2", + }, + { + labelValueCombo: newLabelValueCombo([]string{"label"}, []string{"value-3"}), + value: 3.0, + multiplier: 13.5, + traceID: "trace-3", + }, + { + labelValueCombo: newLabelValueCombo([]string{"label"}, []string{"value-3"}), + value: 1.0, + multiplier: 7.5, + traceID: "trace-3.3", + }, + }, + expectedSeriesCount: 15, + expectedSamples: []sample{ + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "1"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "+Inf"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-2"}, collectionTimeMs, 22), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-2"}, collectionTimeMs, 54.0), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "1"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "2"}, collectionTimeMs, 1), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "+Inf"}, collectionTimeMs, 22), + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-3"}, collectionTimeMs, 22), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-3"}, collectionTimeMs, 51), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-3", "le": "1"}, collectionTimeMs, 7.5), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-3", "le": "2"}, collectionTimeMs, 7.5), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-3", "le": "+Inf"}, collectionTimeMs, 22.0), + }, + expectedExemplars: []exemplarSample{ + newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "+Inf"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-2.2"}), + Value: 2.5, + Ts: collectionTimeMs, + }), + newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-3", "le": "+Inf"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-3"}), + Value: 3, + Ts: collectionTimeMs, + }), + newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-3", "le": "1"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-3.3"}), + Value: 1, + Ts: collectionTimeMs, + }), + }, + }, + }, + }, } - collectMetricAndAssert(t, h, collectionTimeMs, nil, 15, expectedSamples, expectedExemplars) - - h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-2"}), 2.5, "trace-2.2", 20.0) - h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-3"}), 3.0, "trace-3", 13.5) - h.ObserveWithExemplar(newLabelValueCombo([]string{"label"}, []string{"value-3"}), 1.0, "trace-3", 7.5) - - assert.Equal(t, 3, seriesAdded) - - collectionTimeMs = time.Now().UnixMilli() - expectedSamples = []sample{ - newSample(map[string]string{"__name__": "my_histogram_count", "label": "value-1"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "1"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "2"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-1", "le": "+Inf"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_count", "label": "value-2"}, collectionTimeMs, 22), - newSample(map[string]string{"__name__": "my_histogram_sum", "label": "value-2"}, collectionTimeMs, 54.0), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "1"}, collectionTimeMs, 0), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "2"}, collectionTimeMs, 1), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "+Inf"}, collectionTimeMs, 22), - newSample(map[string]string{"__name__": "my_histogram_count", "label": "value-3"}, collectionTimeMs, 22.0), - newSample(map[string]string{"__name__": "my_histogram_sum", "label": "value-3"}, collectionTimeMs, 51.0), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "1"}, collectionTimeMs, 7.5), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "2"}, collectionTimeMs, 7.5), - newSample(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "+Inf"}, collectionTimeMs, 22.0), + + // Tests a single histogram implementation. + testHistogram := func(t *testing.T, h Histogram, collections collections) { + for _, c := range collections { + appender := &capturingAppender{} + for _, obs := range c.observations { + h.ObserveWithExemplar(obs.labelValueCombo, obs.value, obs.traceID, obs.multiplier) + } + + collectMetricsAndAssertSeries(t, h, collectionTimeMs, c.expectedSeriesCount, appender) + if len(c.expectedSamples) > 0 { + assertAppenderSamples(t, appender, c.expectedSamples) + } + if len(c.expectedExemplars) > 0 { + assertAppenderExemplars(t, appender, c.expectedExemplars) + } + } } - expectedExemplars = []exemplarSample{ - newExemplar(map[string]string{"__name__": "my_histogram_bucket", "label": "value-2", "le": "+Inf"}, exemplar.Exemplar{ - Labels: labels.FromMap(map[string]string{"trace_id": "trace-2.2"}), - Value: 2.5, - Ts: collectionTimeMs, - }), - newExemplar(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "1"}, exemplar.Exemplar{ - Labels: labels.FromMap(map[string]string{"trace_id": "trace-3"}), - Value: 1.0, - Ts: collectionTimeMs, - }), - newExemplar(map[string]string{"__name__": "my_histogram_bucket", "label": "value-3", "le": "+Inf"}, exemplar.Exemplar{ - Labels: labels.FromMap(map[string]string{"trace_id": "trace-3"}), - Value: 3.0, - Ts: collectionTimeMs, - }), + + // Tests both classic and native histograms. + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Run("classic", func(t *testing.T) { + var seriesAdded int + onAdd := func(count uint32) bool { + seriesAdded += int(count) + return true + } + h := newHistogram("test_histogram", tc.buckets, onAdd, nil, "trace_id") + testHistogram(t, h, tc.collections) + }) + t.Run("native", func(t *testing.T) { + var seriesAdded int + onAdd := func(count uint32) bool { + seriesAdded += int(count) + return true + } + h := newNativeHistogram("test_histogram", tc.buckets, onAdd, nil, "trace_id") + testHistogram(t, h, tc.collections) + }) + }) } - collectMetricAndAssert(t, h, collectionTimeMs, nil, 15, expectedSamples, expectedExemplars) } -// Duplicate labels should not grow the series count. -func Test_ObserveWithExemplar_duplicate(t *testing.T) { - var seriesAdded int - onAdd := func(count uint32) bool { - seriesAdded += int(count) - return true - } - h := newNativeHistogram("my_histogram", []float64{0.1, 0.2}, onAdd, nil, "trace_id") +func collectMetricsAndAssertSeries(t *testing.T, m metric, collectionTimeMs int64, expectedSeries int, appender storage.Appender) { + activeSeries, err := m.collectMetrics(appender, collectionTimeMs, nil) + require.NoError(t, err) + require.Equal(t, expectedSeries, activeSeries) +} - lv := newLabelValueCombo([]string{"label"}, []string{"value-1"}) +func assertAppenderSamples(t *testing.T, appender *capturingAppender, expectedSamples []sample) { + t.Run("Samples", func(t *testing.T) { + require.Len(t, appender.samples, len(expectedSamples)) + // for i, expected := range expectedSamples { + // assert.Equal(t, expected, appender.samples[i]) + // } - h.ObserveWithExemplar(lv, 1.0, "trace-1", 1.0) - h.ObserveWithExemplar(lv, 1.1, "trace-1", 1.0) - assert.Equal(t, 1, seriesAdded) + sort.Slice(expectedSamples[:], func(i, j int) bool { + return expectedSamples[i].l.String() < expectedSamples[j].l.String() + }) + // t.Log("Expected samples:") + // for _, expectedSample := range expectedSamples { + // t.Log(" - ", expectedSample.l, expectedSample.v, expectedSample.t) + // } + + sort.Slice(appender.samples[:], func(i, j int) bool { + return appender.samples[i].l.String() < appender.samples[j].l.String() + }) + + // t.Log("Appender samples:") + // for _, sample := range appender.samples { + // t.Log(" - ", sample.l, sample.v, sample.t) + // } + + matched := assert.ElementsMatchf(t, expectedSamples, appender.samples, "samples mismatch") + if !matched { + for i, expected := range expectedSamples { + if expected.l.String() != appender.samples[i].l.String() || expected.v != appender.samples[i].v || expected.t != appender.samples[i].t { + t.Log("Mismatch at index", i) + t.Log("Expected: ", expected.l, expected.v, expected.t) + t.Log("Actual: ", appender.samples[i].l, appender.samples[i].v, appender.samples[i].t) + } + } + } + }) +} + +func assertAppenderExemplars(t *testing.T, appender *capturingAppender, expected []exemplarSample) { + t.Run("Exemplars", func(t *testing.T) { + require.Len(t, appender.exemplars, len(expected)) + sort.Slice(expected[:], func(i, j int) bool { + return expected[i].l.String() < expected[j].l.String() + }) + + sort.Slice(appender.exemplars[:], func(i, j int) bool { + return appender.exemplars[i].l.String() < appender.exemplars[j].l.String() + }) + + matched := assert.ElementsMatchf(t, expected, appender.exemplars, "exemplars mismatch") + if !matched { + for i, expected := range expected { + if expected.l.String() != appender.exemplars[i].l.String() || !expected.e.Equals(appender.exemplars[i].e) { + t.Log("Mismatch at index", i) + t.Log("Expected: ", expected.l, expected.e) + t.Log("Actual: ", appender.exemplars[i].l, appender.exemplars[i].e) + } + } + } + }) } From 2bd0d25576b5aea7427371a42c7736a03359f397 Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Fri, 21 Jun 2024 20:13:22 +0000 Subject: [PATCH 12/26] Track and reset the buckets for which exemplars have been recorded --- .../generator/registry/native_histogram.go | 60 ++++++++++--------- 1 file changed, 31 insertions(+), 29 deletions(-) diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go index ce56cb2bff1..15b7037025e 100644 --- a/modules/generator/registry/native_histogram.go +++ b/modules/generator/registry/native_histogram.go @@ -41,6 +41,7 @@ type nativeHistogramSeries struct { labels LabelPair promHistogram prometheus.Histogram lastUpdated *atomic.Int64 + histogram *dto.Histogram } var ( @@ -157,17 +158,17 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 encodedMetric := &dto.Metric{} // Encode to protobuf representation - err := s.promHistogram.Write(encodedMetric) + err = s.promHistogram.Write(encodedMetric) if err != nil { return activeSeries, err } - encodedHistogram := encodedMetric.GetHistogram() + s.histogram = encodedMetric.GetHistogram() // *** Classic histogram // sum lb.Set(labels.MetricName, h.metricName+"_sum") - _, err = appender.Append(0, lb.Labels(), timeMs, encodedHistogram.GetSampleSum()) + _, err = appender.Append(0, lb.Labels(), timeMs, s.histogram.GetSampleSum()) if err != nil { return activeSeries, err } @@ -175,7 +176,7 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 // count lb.Set(labels.MetricName, h.metricName+"_count") - _, err = appender.Append(0, lb.Labels(), timeMs, getIfGreaterThenZeroOr(encodedHistogram.GetSampleCountFloat(), encodedHistogram.GetSampleCount())) + _, err = appender.Append(0, lb.Labels(), timeMs, getIfGreaterThenZeroOr(s.histogram.GetSampleCountFloat(), s.histogram.GetSampleCount())) if err != nil { return activeSeries, err } @@ -188,7 +189,7 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 // for that bucket or not. To avoid adding it twice, keep track of it with this boolean. infBucketWasAdded := false - for _, bucket := range encodedHistogram.Bucket { + for _, bucket := range s.histogram.Bucket { // add "le" label lb.Set(labels.BucketLabel, formatFloat(bucket.GetUpperBound())) @@ -196,23 +197,23 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 infBucketWasAdded = true } - ref, err := appender.Append(0, lb.Labels(), timeMs, getIfGreaterThenZeroOr(bucket.GetCumulativeCountFloat(), bucket.GetCumulativeCount())) - if err != nil { - return activeSeries, err + ref, appendErr := appender.Append(0, lb.Labels(), timeMs, getIfGreaterThenZeroOr(bucket.GetCumulativeCountFloat(), bucket.GetCumulativeCount())) + if appendErr != nil { + return activeSeries, appendErr } activeSeries++ - ex := bucket.Exemplar - if ex != nil { + 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(ex.GetLabel()), - Value: ex.GetValue(), + Labels: convertLabelPairToLabels(bucket.Exemplar.GetLabel()), + Value: bucket.Exemplar.GetValue(), Ts: timeMs, }) if err != nil { return activeSeries, err } + bucket.Exemplar.Reset() } } @@ -220,7 +221,7 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 // Add +Inf bucket lb.Set(labels.BucketLabel, "+Inf") - _, err = appender.Append(0, lb.Labels(), timeMs, getIfGreaterThenZeroOr(encodedHistogram.GetSampleCountFloat(), encodedHistogram.GetSampleCount())) + _, err = appender.Append(0, lb.Labels(), timeMs, getIfGreaterThenZeroOr(s.histogram.GetSampleCountFloat(), s.histogram.GetSampleCount())) if err != nil { return activeSeries, err } @@ -234,43 +235,44 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 // Decode to Prometheus representation hist := promhistogram.Histogram{ - Schema: encodedHistogram.GetSchema(), - Count: encodedHistogram.GetSampleCount(), - Sum: encodedHistogram.GetSampleSum(), - ZeroThreshold: encodedHistogram.GetZeroThreshold(), - ZeroCount: encodedHistogram.GetZeroCount(), + Schema: s.histogram.GetSchema(), + Count: s.histogram.GetSampleCount(), + Sum: s.histogram.GetSampleSum(), + ZeroThreshold: s.histogram.GetZeroThreshold(), + ZeroCount: s.histogram.GetZeroCount(), } - if len(encodedHistogram.PositiveSpan) > 0 { - hist.PositiveSpans = make([]promhistogram.Span, len(encodedHistogram.PositiveSpan)) - for i, span := range encodedHistogram.PositiveSpan { + if len(s.histogram.PositiveSpan) > 0 { + hist.PositiveSpans = make([]promhistogram.Span, len(s.histogram.PositiveSpan)) + for i, span := range s.histogram.PositiveSpan { hist.PositiveSpans[i] = promhistogram.Span{ Offset: span.GetOffset(), Length: span.GetLength(), } } } - hist.PositiveBuckets = encodedHistogram.PositiveDelta - if len(encodedHistogram.NegativeSpan) > 0 { - hist.NegativeSpans = make([]promhistogram.Span, len(encodedHistogram.NegativeSpan)) - for i, span := range encodedHistogram.NegativeSpan { + hist.PositiveBuckets = s.histogram.PositiveDelta + if len(s.histogram.NegativeSpan) > 0 { + hist.NegativeSpans = make([]promhistogram.Span, len(s.histogram.NegativeSpan)) + for i, span := range s.histogram.NegativeSpan { hist.NegativeSpans[i] = promhistogram.Span{ Offset: span.GetOffset(), Length: span.GetLength(), } } } - hist.NegativeBuckets = encodedHistogram.NegativeDelta + hist.NegativeBuckets = s.histogram.NegativeDelta lb.Set(labels.MetricName, h.metricName) _, err = appender.AppendHistogram(0, lb.Labels(), timeMs, &hist, nil) if err != nil { return activeSeries, err } - // TODO impact on active series from appending a histogram? + + // TODO: impact on active series from appending a histogram? activeSeries += 0 - if len(encodedHistogram.Exemplars) > 0 { - for _, encodedExemplar := range encodedHistogram.Exemplars { + if len(s.histogram.Exemplars) > 0 { + for _, encodedExemplar := range s.histogram.Exemplars { // TODO are we appending the same exemplar twice? e := exemplar.Exemplar{ From 69f96df2016d928ae91b633c329672a13965d0be Mon Sep 17 00:00:00 2001 From: yuna Date: Mon, 24 Jun 2024 19:51:21 +0200 Subject: [PATCH 13/26] Split multiplier test into integer and floating point --- .../registry/native_histogram_test.go | 56 ++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/modules/generator/registry/native_histogram_test.go b/modules/generator/registry/native_histogram_test.go index 550df5b94e4..5ed06119269 100644 --- a/modules/generator/registry/native_histogram_test.go +++ b/modules/generator/registry/native_histogram_test.go @@ -53,6 +53,8 @@ func Test_Histograms(t *testing.T) { name string buckets []float64 collections collections + // native histogram does not support all features yet + skipNativeHistogram bool }{ { name: "single collection single observation", @@ -226,8 +228,56 @@ func Test_Histograms(t *testing.T) { }, }, { - name: "many observations with some multipliers", + name: "integer multiplier", buckets: []float64{1, 2}, + collections: collections{ + { + observations: observations{ + { + labelValueCombo: newLabelValueCombo([]string{"label"}, []string{"value-1"}), + value: 1.5, + multiplier: 20.0, + traceID: "trace-1", + }, + { + labelValueCombo: newLabelValueCombo([]string{"label"}, []string{"value-2"}), + value: 3.0, + multiplier: 13, + traceID: "trace-2", + }, + }, + expectedSeriesCount: 15, + expectedSamples: []sample{ + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-1"}, collectionTimeMs, 20), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-1"}, collectionTimeMs, 20*1.5), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "1"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "2"}, collectionTimeMs, 20), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "+Inf"}, collectionTimeMs, 20), + newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-2"}, collectionTimeMs, 13), + newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-2"}, collectionTimeMs, 13*3), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "1"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "2"}, collectionTimeMs, 0), + newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "+Inf"}, collectionTimeMs, 13), + }, + expectedExemplars: []exemplarSample{ + newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "+Inf"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-1"}), + Value: 1.5, + Ts: collectionTimeMs, + }), + newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "+Inf"}, exemplar.Exemplar{ + Labels: labels.FromMap(map[string]string{"trace_id": "trace-2"}), + Value: 3, + Ts: collectionTimeMs, + }), + }, + }, + }, + }, + { + name: "many observations with floating point multiplier", + skipNativeHistogram: true, + buckets: []float64{1, 2}, collections: collections{ { observations: observations{ @@ -408,6 +458,10 @@ func Test_Histograms(t *testing.T) { testHistogram(t, h, tc.collections) }) t.Run("native", func(t *testing.T) { + if tc.skipNativeHistogram { + t.SkipNow() + } + var seriesAdded int onAdd := func(count uint32) bool { seriesAdded += int(count) From 21a00be7e3d5bc1cdfcb20013a95369295ac16cc Mon Sep 17 00:00:00 2001 From: yuna Date: Mon, 24 Jun 2024 22:00:26 +0200 Subject: [PATCH 14/26] Fix expectedSeriesCount and examplars in test --- modules/generator/registry/native_histogram_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/generator/registry/native_histogram_test.go b/modules/generator/registry/native_histogram_test.go index 5ed06119269..34b033f1730 100644 --- a/modules/generator/registry/native_histogram_test.go +++ b/modules/generator/registry/native_histogram_test.go @@ -246,7 +246,7 @@ func Test_Histograms(t *testing.T) { traceID: "trace-2", }, }, - expectedSeriesCount: 15, + expectedSeriesCount: 10, expectedSamples: []sample{ newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-1"}, collectionTimeMs, 20), newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-1"}, collectionTimeMs, 20*1.5), @@ -260,7 +260,7 @@ func Test_Histograms(t *testing.T) { newSample(map[string]string{"__name__": "test_histogram_bucket", "label": "value-2", "le": "+Inf"}, collectionTimeMs, 13), }, expectedExemplars: []exemplarSample{ - newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "+Inf"}, exemplar.Exemplar{ + newExemplar(map[string]string{"__name__": "test_histogram_bucket", "label": "value-1", "le": "2"}, exemplar.Exemplar{ Labels: labels.FromMap(map[string]string{"trace_id": "trace-1"}), Value: 1.5, Ts: collectionTimeMs, From 3ff3eb85abb298739b44b42ab444632ab815b996 Mon Sep 17 00:00:00 2001 From: yuna Date: Mon, 24 Jun 2024 22:03:33 +0200 Subject: [PATCH 15/26] Drop expectedSeriesCount and just len of expectedSamples instead --- .../generator/registry/native_histogram_test.go | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/modules/generator/registry/native_histogram_test.go b/modules/generator/registry/native_histogram_test.go index 34b033f1730..fe4bf590332 100644 --- a/modules/generator/registry/native_histogram_test.go +++ b/modules/generator/registry/native_histogram_test.go @@ -41,10 +41,9 @@ func Test_Histograms(t *testing.T) { // allows the test to track the accumulation of observations over a series of // collections. type collections []struct { - observations observations - expectedSamples []sample - expectedExemplars []exemplarSample - expectedSeriesCount int + observations observations + expectedSamples []sample + expectedExemplars []exemplarSample } collectionTimeMs := time.Now().UnixMilli() @@ -69,7 +68,6 @@ func Test_Histograms(t *testing.T) { traceID: "trace-1", }, }, - expectedSeriesCount: 5, expectedSamples: []sample{ newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-1"}, collectionTimeMs, 1), newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), @@ -106,7 +104,6 @@ func Test_Histograms(t *testing.T) { traceID: "trace-2", }, }, - expectedSeriesCount: 10, expectedSamples: []sample{ newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-1"}, collectionTimeMs, 1), newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), @@ -153,7 +150,6 @@ func Test_Histograms(t *testing.T) { traceID: "trace-2", }, }, - expectedSeriesCount: 10, expectedSamples: []sample{ newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-1"}, collectionTimeMs, 1), newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), @@ -194,7 +190,6 @@ func Test_Histograms(t *testing.T) { traceID: "trace-3", }, }, - expectedSeriesCount: 15, expectedSamples: []sample{ newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-1"}, collectionTimeMs, 1), newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), @@ -246,7 +241,6 @@ func Test_Histograms(t *testing.T) { traceID: "trace-2", }, }, - expectedSeriesCount: 10, expectedSamples: []sample{ newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-1"}, collectionTimeMs, 20), newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-1"}, collectionTimeMs, 20*1.5), @@ -294,7 +288,6 @@ func Test_Histograms(t *testing.T) { traceID: "trace-2", }, }, - expectedSeriesCount: 10, expectedSamples: []sample{ newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-1"}, collectionTimeMs, 1), newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), @@ -335,7 +328,6 @@ func Test_Histograms(t *testing.T) { traceID: "trace-3", }, }, - expectedSeriesCount: 15, expectedSamples: []sample{ newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-1"}, collectionTimeMs, 1), newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), @@ -387,7 +379,6 @@ func Test_Histograms(t *testing.T) { traceID: "trace-3.3", }, }, - expectedSeriesCount: 15, expectedSamples: []sample{ newSample(map[string]string{"__name__": "test_histogram_count", "label": "value-1"}, collectionTimeMs, 1), newSample(map[string]string{"__name__": "test_histogram_sum", "label": "value-1"}, collectionTimeMs, 1), @@ -435,7 +426,7 @@ func Test_Histograms(t *testing.T) { h.ObserveWithExemplar(obs.labelValueCombo, obs.value, obs.traceID, obs.multiplier) } - collectMetricsAndAssertSeries(t, h, collectionTimeMs, c.expectedSeriesCount, appender) + collectMetricsAndAssertSeries(t, h, collectionTimeMs, len(c.expectedSamples), appender) if len(c.expectedSamples) > 0 { assertAppenderSamples(t, appender, c.expectedSamples) } From 501a054650621f0f8dc7ba7c85e05f92ae828b07 Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Thu, 27 Jun 2024 20:11:43 +0000 Subject: [PATCH 16/26] Reduce log spam --- modules/generator/registry/registry.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/generator/registry/registry.go b/modules/generator/registry/registry.go index 75e52362599..9682be7be6d 100644 --- a/modules/generator/registry/registry.go +++ b/modules/generator/registry/registry.go @@ -154,7 +154,7 @@ func (r *ManagedRegistry) NewHistogram(name string, buckets []float64) (h Histog if !r.overrides.MetricsGeneratorGenerateNativeHistograms(r.tenant) { h = newHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName) } else { - level.Warn(r.logger).Log("msg", "creating native histogram!", "metric", name) + level.Debug(r.logger).Log("msg", "creating native histogram!", "metric", name) h = newNativeHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName) } From 52bd702ff4f874553c985e772901a2375053ec5e Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Thu, 27 Jun 2024 20:16:07 +0000 Subject: [PATCH 17/26] Apply existing interface constraint to Gauge and Counter --- modules/generator/registry/counter.go | 3 +++ modules/generator/registry/gauge.go | 5 ++++- modules/generator/registry/histogram.go | 2 ++ modules/generator/registry/interface.go | 4 ++++ 4 files changed, 13 insertions(+), 1 deletion(-) diff --git a/modules/generator/registry/counter.go b/modules/generator/registry/counter.go index 9e102ce2f2e..cdd0ae7f5f2 100644 --- a/modules/generator/registry/counter.go +++ b/modules/generator/registry/counter.go @@ -9,7 +9,10 @@ import ( "go.uber.org/atomic" ) +var _ metric = (*counter)(nil) + type counter struct { + metric metricName string // seriesMtx is used to sync modifications to the map, not to the data in series diff --git a/modules/generator/registry/gauge.go b/modules/generator/registry/gauge.go index 65181133f0e..b5c65cb5e59 100644 --- a/modules/generator/registry/gauge.go +++ b/modules/generator/registry/gauge.go @@ -9,9 +9,12 @@ import ( "go.uber.org/atomic" ) -// this is mostly copied from counter +var _ metric = (*gauge)(nil) +// this is mostly copied from counter type gauge struct { + metric + metricName string // seriesMtx is used to sync modifications to the map, not to the data in series diff --git a/modules/generator/registry/histogram.go b/modules/generator/registry/histogram.go index c8baf69547a..c698d591d06 100644 --- a/modules/generator/registry/histogram.go +++ b/modules/generator/registry/histogram.go @@ -14,6 +14,8 @@ import ( "go.uber.org/atomic" ) +var _ metric = (*histogram)(nil) + type histogram struct { metricName string nameCount string diff --git a/modules/generator/registry/interface.go b/modules/generator/registry/interface.go index 01dfdd214aa..67eca391827 100644 --- a/modules/generator/registry/interface.go +++ b/modules/generator/registry/interface.go @@ -11,6 +11,8 @@ type Registry interface { // Counter // https://prometheus.io/docs/concepts/metric_types/#counter type Counter interface { + metric + Inc(labelValueCombo *LabelValueCombo, value float64) } @@ -27,6 +29,8 @@ type Histogram interface { // https://prometheus.io/docs/concepts/metric_types/#gauge // https://pkg.go.dev/github.com/prometheus/client_golang/prometheus#Gauge type Gauge interface { + metric + // Set sets the Gauge to an arbitrary value. Set(labelValueCombo *LabelValueCombo, value float64) Inc(labelValueCombo *LabelValueCombo, value float64) From 77859e1b7ea0c8177317e58fef7ca75e4bafdd55 Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Mon, 1 Jul 2024 16:24:07 +0000 Subject: [PATCH 18/26] Update test instances for metric interface --- modules/generator/registry/test.go | 38 ++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/modules/generator/registry/test.go b/modules/generator/registry/test.go index 0489595122b..fe82b5f5fb2 100644 --- a/modules/generator/registry/test.go +++ b/modules/generator/registry/test.go @@ -26,14 +26,14 @@ func NewTestRegistry() *TestRegistry { func (t *TestRegistry) NewCounter(name string) Counter { return &testCounter{ - name: name, + n: name, registry: t, } } func (t *TestRegistry) NewGauge(name string) Gauge { return &testGauge{ - name: name, + n: name, registry: t, } } @@ -84,7 +84,7 @@ func (t *TestRegistry) String() string { } type testCounter struct { - name string + n string registry *TestRegistry } @@ -101,11 +101,23 @@ func (t *testCounter) Inc(labelValueCombo *LabelValueCombo, value float64) { } sort.Sort(lbls) - t.registry.addToMetric(t.name, lbls, value) + t.registry.addToMetric(t.n, lbls, value) +} + +func (t *testCounter) name() string { + return t.n +} + +func (t *testCounter) collectMetrics(appender storage.Appender, timeMs int64, externalLabels map[string]string) (activeSeries int, err error) { + return 0, nil +} + +func (t *testCounter) removeStaleSeries(int64) { + panic("implement me") } type testGauge struct { - name string + n string registry *TestRegistry } @@ -122,7 +134,7 @@ func (t *testGauge) Inc(labelValueCombo *LabelValueCombo, value float64) { } sort.Sort(lbls) - t.registry.addToMetric(t.name, lbls, value) + t.registry.addToMetric(t.n, lbls, value) } func (t *testGauge) Set(labelValueCombo *LabelValueCombo, value float64) { @@ -132,13 +144,25 @@ func (t *testGauge) Set(labelValueCombo *LabelValueCombo, value float64) { } sort.Sort(lbls) - t.registry.setMetric(t.name, lbls, value) + t.registry.setMetric(t.n, lbls, value) } func (t *testGauge) SetForTargetInfo(labelValueCombo *LabelValueCombo, value float64) { t.Set(labelValueCombo, value) } +func (t *testGauge) name() string { + return t.n +} + +func (t *testGauge) collectMetrics(appender storage.Appender, timeMs int64, externalLabels map[string]string) (activeSeries int, err error) { + return 0, nil +} + +func (t *testGauge) removeStaleSeries(int64) { + panic("implement me") +} + type testHistogram struct { nameSum string nameCount string From 43608a9afb02b19bd485c210658047d0acb18280 Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Mon, 1 Jul 2024 16:24:24 +0000 Subject: [PATCH 19/26] Use int64 in place of atomic since always under lock --- modules/generator/registry/native_histogram.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go index 15b7037025e..0e2343cb6c5 100644 --- a/modules/generator/registry/native_histogram.go +++ b/modules/generator/registry/native_histogram.go @@ -11,7 +11,6 @@ import ( promhistogram "github.com/prometheus/prometheus/model/histogram" "github.com/prometheus/prometheus/model/labels" "github.com/prometheus/prometheus/storage" - "go.uber.org/atomic" "google.golang.org/protobuf/types/known/timestamppb" ) @@ -40,7 +39,7 @@ type nativeHistogramSeries struct { // labels should not be modified after creation labels LabelPair promHistogram prometheus.Histogram - lastUpdated *atomic.Int64 + lastUpdated int64 histogram *dto.Histogram } @@ -106,7 +105,7 @@ func (h *nativeHistogram) newSeries(labelValueCombo *LabelValueCombo, value floa NativeHistogramMaxBucketNumber: 100, NativeHistogramMinResetDuration: 15 * time.Minute, }), - lastUpdated: atomic.NewInt64(0), + lastUpdated: 0, } h.updateSeries(newSeries, value, traceID, multiplier) @@ -121,7 +120,7 @@ func (h *nativeHistogram) updateSeries(s *nativeHistogramSeries, value float64, map[string]string{h.traceIDLabelName: traceID}, ) } - s.lastUpdated.Store(time.Now().UnixMilli()) + s.lastUpdated = time.Now().UnixMilli() } func (h *nativeHistogram) name() string { @@ -297,7 +296,7 @@ func (h *nativeHistogram) removeStaleSeries(staleTimeMs int64) { h.seriesMtx.Lock() defer h.seriesMtx.Unlock() for hash, s := range h.series { - if s.lastUpdated.Load() < staleTimeMs { + if s.lastUpdated < staleTimeMs { delete(h.series, hash) h.onRemoveSerie(h.activeSeriesPerHistogramSerie()) } From 25fc5e4edc0d9541d7519d92cb428b30dc5080c2 Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Mon, 1 Jul 2024 16:39:47 +0000 Subject: [PATCH 20/26] Lint for interface updates --- modules/generator/registry/counter.go | 1 + modules/generator/registry/gauge.go | 1 + modules/generator/registry/test.go | 6 +++--- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/modules/generator/registry/counter.go b/modules/generator/registry/counter.go index cdd0ae7f5f2..bea9152a41d 100644 --- a/modules/generator/registry/counter.go +++ b/modules/generator/registry/counter.go @@ -12,6 +12,7 @@ import ( var _ metric = (*counter)(nil) type counter struct { + //nolint unused metric metricName string diff --git a/modules/generator/registry/gauge.go b/modules/generator/registry/gauge.go index b5c65cb5e59..3a8ec22d86b 100644 --- a/modules/generator/registry/gauge.go +++ b/modules/generator/registry/gauge.go @@ -13,6 +13,7 @@ var _ metric = (*gauge)(nil) // this is mostly copied from counter type gauge struct { + //nolint unused metric metricName string diff --git a/modules/generator/registry/test.go b/modules/generator/registry/test.go index fe82b5f5fb2..7fd1b2f482f 100644 --- a/modules/generator/registry/test.go +++ b/modules/generator/registry/test.go @@ -108,8 +108,8 @@ func (t *testCounter) name() string { return t.n } -func (t *testCounter) collectMetrics(appender storage.Appender, timeMs int64, externalLabels map[string]string) (activeSeries int, err error) { - return 0, nil +func (t *testCounter) collectMetrics(_ storage.Appender, _ int64, _ map[string]string) (activeSeries int, err error) { + return } func (t *testCounter) removeStaleSeries(int64) { @@ -155,7 +155,7 @@ func (t *testGauge) name() string { return t.n } -func (t *testGauge) collectMetrics(appender storage.Appender, timeMs int64, externalLabels map[string]string) (activeSeries int, err error) { +func (t *testGauge) collectMetrics(_ storage.Appender, _ int64, _ map[string]string) (activeSeries int, err error) { return 0, nil } From 73f49a062b9906d5757d3e379d1d4023359b25e4 Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Tue, 2 Jul 2024 18:15:46 +0000 Subject: [PATCH 21/26] Drop series[0] check --- modules/generator/registry/native_histogram.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go index 0e2343cb6c5..b608fa7ddae 100644 --- a/modules/generator/registry/native_histogram.go +++ b/modules/generator/registry/native_histogram.go @@ -131,11 +131,7 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 h.seriesMtx.Lock() defer h.seriesMtx.Unlock() - labelsCount := 0 - if h.series[0] != nil { - labelsCount = len(h.series[0].labels.names) - } - lbls := make(labels.Labels, 1+len(externalLabels)+labelsCount) + lbls := make(labels.Labels, 1+len(externalLabels)) lb := labels.NewBuilder(lbls) activeSeries = 0 From f7dcdab647bd7d640bd6b83dc034e6bf87237fb2 Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Wed, 3 Jul 2024 15:01:11 +0000 Subject: [PATCH 22/26] Convert override from bool to string and test values --- cmd/tempo/app/overrides_validation.go | 7 + cmd/tempo/app/overrides_validation_test.go | 29 ++++ modules/generator/overrides.go | 2 +- modules/generator/overrides_test.go | 6 +- .../generator/registry/native_histogram.go | 153 ++++++++++-------- .../registry/native_histogram_test.go | 4 +- modules/generator/registry/overrides.go | 2 +- modules/generator/registry/registry.go | 13 +- modules/generator/registry/registry_test.go | 57 ++++++- modules/generator/storage/instance.go | 13 +- modules/generator/storage/instance_test.go | 7 +- modules/generator/storage/overrides.go | 2 +- modules/overrides/config.go | 14 +- modules/overrides/config_legacy.go | 2 +- modules/overrides/interface.go | 2 +- modules/overrides/runtime_config_overrides.go | 2 +- 16 files changed, 220 insertions(+), 95 deletions(-) diff --git a/cmd/tempo/app/overrides_validation.go b/cmd/tempo/app/overrides_validation.go index 170b8c4f562..6c53fb773c2 100644 --- a/cmd/tempo/app/overrides_validation.go +++ b/cmd/tempo/app/overrides_validation.go @@ -33,6 +33,13 @@ func (r *runtimeConfigValidator) Validate(config *overrides.Overrides) error { } } + if config.MetricsGenerator.GenerateNativeHistograms != "classic" && + config.MetricsGenerator.GenerateNativeHistograms != "native" && + config.MetricsGenerator.GenerateNativeHistograms != "both" && + config.MetricsGenerator.GenerateNativeHistograms != "" { + return fmt.Errorf("metrics_generator.generate_native_histograms \"%s\" is not a valid value, valid values: classic, native, both", config.MetricsGenerator.GenerateNativeHistograms) + } + return nil } diff --git a/cmd/tempo/app/overrides_validation_test.go b/cmd/tempo/app/overrides_validation_test.go index 1fa8739669e..f6745084d25 100644 --- a/cmd/tempo/app/overrides_validation_test.go +++ b/cmd/tempo/app/overrides_validation_test.go @@ -51,6 +51,35 @@ func Test_runtimeOverridesValidator(t *testing.T) { }, overrides: overrides.Overrides{Ingestion: overrides.IngestionOverrides{TenantShardSize: 3}}, }, + { + name: "metrics_generator.generate_native_histograms invalid", + cfg: Config{}, + overrides: overrides.Overrides{MetricsGenerator: overrides.MetricsGeneratorOverrides{ + GenerateNativeHistograms: "invalid", + }}, + expErr: "metrics_generator.generate_native_histograms \"invalid\" is not a valid value, valid values: classic, native, both", + }, + { + name: "metrics_generator.generate_native_histograms classic", + cfg: Config{}, + overrides: overrides.Overrides{MetricsGenerator: overrides.MetricsGeneratorOverrides{ + GenerateNativeHistograms: "classic", + }}, + }, + { + name: "metrics_generator.generate_native_histograms native", + cfg: Config{}, + overrides: overrides.Overrides{MetricsGenerator: overrides.MetricsGeneratorOverrides{ + GenerateNativeHistograms: "native", + }}, + }, + { + name: "metrics_generator.generate_native_histograms both", + cfg: Config{}, + overrides: overrides.Overrides{MetricsGenerator: overrides.MetricsGeneratorOverrides{ + GenerateNativeHistograms: "both", + }}, + }, } for _, tc := range testCases { diff --git a/modules/generator/overrides.go b/modules/generator/overrides.go index 15a57a7639b..cad9312f820 100644 --- a/modules/generator/overrides.go +++ b/modules/generator/overrides.go @@ -15,7 +15,7 @@ type metricsGeneratorOverrides interface { registry.Overrides storage.Overrides - MetricsGeneratorGenerateNativeHistograms(userID string) bool + MetricsGeneratorGenerateNativeHistograms(userID string) string MetricsGeneratorIngestionSlack(userID string) time.Duration MetricsGeneratorProcessors(userID string) map[string]struct{} MetricsGeneratorProcessorServiceGraphsHistogramBuckets(userID string) []float64 diff --git a/modules/generator/overrides_test.go b/modules/generator/overrides_test.go index ce000fe9786..b9074cf6754 100644 --- a/modules/generator/overrides_test.go +++ b/modules/generator/overrides_test.go @@ -56,11 +56,11 @@ func (m *mockOverrides) MetricsGeneratorDisableCollection(string) bool { return false } -func (m *mockOverrides) MetricsGeneratorGenerateNativeHistograms(string) bool { - return false +func (m *mockOverrides) MetricsGeneratorGenerateNativeHistograms(string) string { + return "" } -func (m *mockOverrides) MetricsGenerationTraceIDLabelName(userID string) string { +func (m *mockOverrides) MetricsGenerationTraceIDLabelName(string) string { return "" } diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go index b608fa7ddae..9588400d9d6 100644 --- a/modules/generator/registry/native_histogram.go +++ b/modules/generator/registry/native_histogram.go @@ -5,6 +5,7 @@ import ( "sync" "time" + "github.com/grafana/tempo/modules/overrides" "github.com/prometheus/client_golang/prometheus" dto "github.com/prometheus/client_model/go" "github.com/prometheus/prometheus/model/exemplar" @@ -33,6 +34,9 @@ type nativeHistogram struct { buckets []float64 traceIDLabelName string + + // "native" or "both", to include classic histograms. + mode string } type nativeHistogramSeries struct { @@ -48,7 +52,7 @@ var ( _ metric = (*nativeHistogram)(nil) ) -func newNativeHistogram(name string, buckets []float64, onAddSeries func(uint32) bool, onRemoveSeries func(count uint32), traceIDLabelName string) *nativeHistogram { +func newNativeHistogram(name string, buckets []float64, onAddSeries func(uint32) bool, onRemoveSeries func(count uint32), traceIDLabelName string, f func() string) *nativeHistogram { if onAddSeries == nil { onAddSeries = func(uint32) bool { return true @@ -69,6 +73,8 @@ func newNativeHistogram(name string, buckets []float64, onAddSeries func(uint32) onRemoveSerie: onRemoveSeries, traceIDLabelName: traceIDLabelName, buckets: buckets, + // FIXME: the mode never gets updated after here, so we can't downgrade from "both" to "native" + mode: f(), } } @@ -157,77 +163,21 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 if err != nil { return activeSeries, err } - s.histogram = encodedMetric.GetHistogram() - - // *** Classic histogram - - // sum - lb.Set(labels.MetricName, h.metricName+"_sum") - _, err = appender.Append(0, lb.Labels(), 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())) - if err != nil { - return activeSeries, err - } - activeSeries++ - - // bucket - 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. - infBucketWasAdded := false - - for _, bucket := range s.histogram.Bucket { - // add "le" label - 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())) - if appendErr != nil { - return activeSeries, appendErr - } - 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(), - Ts: timeMs, - }) - if err != nil { - return activeSeries, err - } - bucket.Exemplar.Reset() - } - } - if !infBucketWasAdded { - // Add +Inf bucket - lb.Set(labels.BucketLabel, "+Inf") + // 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() - _, err = appender.Append(0, lb.Labels(), timeMs, getIfGreaterThenZeroOr(s.histogram.GetSampleCountFloat(), s.histogram.GetSampleCount())) - if err != nil { - return activeSeries, err + // If we are in "both" mode, also emit classic histograms + if h.mode == string(overrides.HistogramMethodBoth) { + classicSeries, classicErr := h.classicHistograms(appender, lb, timeMs, s) + activeSeries += classicSeries + if classicErr != nil { + return activeSeries, classicErr } - activeSeries++ } - // drop "le" label again - lb.Del(labels.BucketLabel) - - // *** Native histogram - // Decode to Prometheus representation hist := promhistogram.Histogram{ Schema: s.histogram.GetSchema(), @@ -304,6 +254,75 @@ func (h *nativeHistogram) activeSeriesPerHistogramSerie() uint32 { return 1 } +func (h *nativeHistogram) classicHistograms(appender storage.Appender, lb *labels.Builder, timeMs int64, s *nativeHistogramSeries) (activeSeries int, err error) { + // sum + lb.Set(labels.MetricName, h.metricName+"_sum") + _, err = appender.Append(0, lb.Labels(), 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())) + if err != nil { + return activeSeries, err + } + activeSeries++ + + // bucket + 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. + infBucketWasAdded := false + + for _, bucket := range s.histogram.Bucket { + // add "le" label + 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())) + if appendErr != nil { + return activeSeries, appendErr + } + 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(), + Ts: timeMs, + }) + if err != nil { + return activeSeries, err + } + bucket.Exemplar.Reset() + } + } + + if !infBucketWasAdded { + // Add +Inf bucket + lb.Set(labels.BucketLabel, "+Inf") + + _, err = appender.Append(0, lb.Labels(), timeMs, getIfGreaterThenZeroOr(s.histogram.GetSampleCountFloat(), s.histogram.GetSampleCount())) + if err != nil { + return activeSeries, err + } + activeSeries++ + } + + // drop "le" label again + lb.Del(labels.BucketLabel) + + return +} + func convertLabelPairToLabels(lbps []*dto.LabelPair) labels.Labels { lbs := make([]labels.Label, len(lbps)) for i, lbp := range lbps { diff --git a/modules/generator/registry/native_histogram_test.go b/modules/generator/registry/native_histogram_test.go index fe4bf590332..9173c9aef63 100644 --- a/modules/generator/registry/native_histogram_test.go +++ b/modules/generator/registry/native_histogram_test.go @@ -19,7 +19,7 @@ func Test_ObserveWithExemplar_duplicate(t *testing.T) { seriesAdded += int(count) return true } - h := newNativeHistogram("my_histogram", []float64{0.1, 0.2}, onAdd, nil, "trace_id") + h := newNativeHistogram("my_histogram", []float64{0.1, 0.2}, onAdd, nil, "trace_id", "both") lv := newLabelValueCombo([]string{"label"}, []string{"value-1"}) @@ -458,7 +458,7 @@ func Test_Histograms(t *testing.T) { seriesAdded += int(count) return true } - h := newNativeHistogram("test_histogram", tc.buckets, onAdd, nil, "trace_id") + h := newNativeHistogram("test_histogram", tc.buckets, onAdd, nil, "trace_id", "both") testHistogram(t, h, tc.collections) }) }) diff --git a/modules/generator/registry/overrides.go b/modules/generator/registry/overrides.go index 39482808e4d..445c1705c86 100644 --- a/modules/generator/registry/overrides.go +++ b/modules/generator/registry/overrides.go @@ -10,7 +10,7 @@ type Overrides interface { MetricsGeneratorMaxActiveSeries(userID string) uint32 MetricsGeneratorCollectionInterval(userID string) time.Duration MetricsGeneratorDisableCollection(userID string) bool - MetricsGeneratorGenerateNativeHistograms(userID string) bool + MetricsGeneratorGenerateNativeHistograms(userID string) string MetricsGenerationTraceIDLabelName(userID string) string } diff --git a/modules/generator/registry/registry.go b/modules/generator/registry/registry.go index 9682be7be6d..32be25c9e07 100644 --- a/modules/generator/registry/registry.go +++ b/modules/generator/registry/registry.go @@ -14,6 +14,7 @@ import ( "github.com/prometheus/prometheus/storage" "go.uber.org/atomic" + "github.com/grafana/tempo/modules/overrides" tempo_log "github.com/grafana/tempo/pkg/util/log" ) @@ -150,12 +151,14 @@ func (r *ManagedRegistry) NewCounter(name string) Counter { func (r *ManagedRegistry) NewHistogram(name string, buckets []float64) (h Histogram) { traceIDLabelName := r.overrides.MetricsGenerationTraceIDLabelName(r.tenant) - // Temporary switch: use the old implementation when native histograms are disabled, eventually the new implementation can handle all cases - if !r.overrides.MetricsGeneratorGenerateNativeHistograms(r.tenant) { - h = newHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName) + histograms := r.overrides.MetricsGeneratorGenerateNativeHistograms(r.tenant) + + // Temporary switch: use the old implementation when native histograms are + // disabled, eventually the new implementation can handle all cases + if overrides.HasNativeHistograms(histograms) { + h = newNativeHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName, histograms) } else { - level.Debug(r.logger).Log("msg", "creating native histogram!", "metric", name) - h = newNativeHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName) + h = newHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName) } r.registerMetric(h) diff --git a/modules/generator/registry/registry_test.go b/modules/generator/registry/registry_test.go index 4c7bc1094be..47e661da176 100644 --- a/modules/generator/registry/registry_test.go +++ b/modules/generator/registry/registry_test.go @@ -255,6 +255,54 @@ func TestValidLabelValueCombo(t *testing.T) { }) } +func TestHistogramOverridesConfig(t *testing.T) { + cases := []struct { + name string + nativeHistogramMode string + typeOfHistogram interface{} + }{ + { + "empty", + "", + &histogram{}, + }, + { + "bad", + "invalid", + &histogram{}, + }, + { + "classic", + "classic", + &histogram{}, + }, + { + "native", + "native", + &nativeHistogram{}, + }, + { + "both", + "both", + &nativeHistogram{}, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + appender := &capturingAppender{} + overrides := &mockOverrides{ + generateNativeHistograms: c.nativeHistogramMode, + } + registry := New(&Config{}, overrides, "test", appender, log.NewNopLogger()) + defer registry.Close() + + tt := registry.NewHistogram("histogram", []float64{1.0, 2.0}) + require.IsType(t, c.typeOfHistogram, tt) + }) + } +} + func collectRegistryMetricsAndAssert(t *testing.T, r *ManagedRegistry, appender *capturingAppender, expectedSamples []sample) { collectionTimeMs := time.Now().UnixMilli() r.collectMetrics(context.Background()) @@ -298,8 +346,9 @@ func collectRegistryMetricsAndAssert(t *testing.T, r *ManagedRegistry, appender } type mockOverrides struct { - maxActiveSeries uint32 - disableCollection bool + maxActiveSeries uint32 + disableCollection bool + generateNativeHistograms string } var _ Overrides = (*mockOverrides)(nil) @@ -316,8 +365,8 @@ func (m *mockOverrides) MetricsGeneratorDisableCollection(string) bool { return m.disableCollection } -func (m *mockOverrides) MetricsGeneratorGenerateNativeHistograms(_ string) bool { - return false +func (m *mockOverrides) MetricsGeneratorGenerateNativeHistograms(_ string) string { + return m.generateNativeHistograms } func (m *mockOverrides) MetricsGenerationTraceIDLabelName(string) string { diff --git a/modules/generator/storage/instance.go b/modules/generator/storage/instance.go index 129090449a9..25932698b64 100644 --- a/modules/generator/storage/instance.go +++ b/modules/generator/storage/instance.go @@ -10,6 +10,7 @@ import ( "github.com/go-kit/log" "github.com/go-kit/log/level" + "github.com/grafana/tempo/modules/overrides" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promauto" "github.com/prometheus/common/model" @@ -85,7 +86,8 @@ func New(cfg *Config, o Overrides, tenant string, reg prometheus.Registerer, log remoteStorage := remote.NewStorage(log.With(logger, "component", "remote"), reg, startTimeCallback, walDir, cfg.RemoteWriteFlushDeadline, &noopScrapeManager{}) headers := o.MetricsGeneratorRemoteWriteHeaders(tenant) - sendNativeHistograms := o.MetricsGeneratorGenerateNativeHistograms(tenant) + generateNativeHistograms := o.MetricsGeneratorGenerateNativeHistograms(tenant) + sendNativeHistograms := overrides.HasNativeHistograms(generateNativeHistograms) remoteStorageConfig := &prometheus_config.Config{ RemoteWriteConfigs: generateTenantRemoteWriteConfigs(cfg.RemoteWrite, tenant, headers, cfg.RemoteWriteAddOrgIDHeader, logger, sendNativeHistograms), @@ -111,8 +113,9 @@ func New(cfg *Config, o Overrides, tenant string, reg prometheus.Registerer, log tenantID: tenant, currentHeaders: headers, sendNativeHistograms: sendNativeHistograms, - overrides: o, - closeCh: make(chan struct{}), + + overrides: o, + closeCh: make(chan struct{}), logger: logger, } @@ -148,7 +151,9 @@ func (s *storageImpl) watchOverrides() { select { case <-t.C: newHeaders := s.overrides.MetricsGeneratorRemoteWriteHeaders(s.tenantID) - newSendNativeHistograms := s.overrides.MetricsGeneratorGenerateNativeHistograms(s.tenantID) + newGenerateNativeHistograms := s.overrides.MetricsGeneratorGenerateNativeHistograms(s.tenantID) + newSendNativeHistograms := overrides.HasNativeHistograms(newGenerateNativeHistograms) + if !headersEqual(s.currentHeaders, newHeaders) || s.sendNativeHistograms != newSendNativeHistograms { level.Info(s.logger).Log("msg", "updating remote write configuration") s.currentHeaders = newHeaders diff --git a/modules/generator/storage/instance_test.go b/modules/generator/storage/instance_test.go index eae0ac257a6..9705ef7ccb5 100644 --- a/modules/generator/storage/instance_test.go +++ b/modules/generator/storage/instance_test.go @@ -13,6 +13,7 @@ import ( "github.com/go-kit/log" "github.com/grafana/dskit/user" + "github.com/grafana/tempo/modules/overrides" "github.com/prometheus/client_golang/prometheus" prometheus_common_config "github.com/prometheus/common/config" "github.com/prometheus/common/model" @@ -203,7 +204,7 @@ func TestInstance_remoteWriteHeaders(t *testing.T) { headers := map[string]string{user.OrgIDHeaderName: "my-other-tenant"} - instance, err := New(&cfg, &mockOverrides{headers, false}, "test-tenant", &noopRegisterer{}, logger) + instance, err := New(&cfg, &mockOverrides{headers, string(overrides.HistogramMethodClassic)}, "test-tenant", &noopRegisterer{}, logger) require.NoError(t, err) // Refuse requests - the WAL should buffer data until requests succeed @@ -362,14 +363,14 @@ var _ Overrides = (*mockOverrides)(nil) type mockOverrides struct { headers map[string]string - nativeHistograms bool + nativeHistograms string } func (m *mockOverrides) MetricsGeneratorRemoteWriteHeaders(string) map[string]string { return m.headers } -func (m *mockOverrides) MetricsGeneratorGenerateNativeHistograms(string) bool { +func (m *mockOverrides) MetricsGeneratorGenerateNativeHistograms(string) string { return m.nativeHistograms } diff --git a/modules/generator/storage/overrides.go b/modules/generator/storage/overrides.go index 645152183fa..6c9a5f307b7 100644 --- a/modules/generator/storage/overrides.go +++ b/modules/generator/storage/overrides.go @@ -2,5 +2,5 @@ package storage type Overrides interface { MetricsGeneratorRemoteWriteHeaders(userID string) map[string]string - MetricsGeneratorGenerateNativeHistograms(userID string) bool + MetricsGeneratorGenerateNativeHistograms(userID string) string } diff --git a/modules/overrides/config.go b/modules/overrides/config.go index 18ed85cef4c..beb4853a7f9 100644 --- a/modules/overrides/config.go +++ b/modules/overrides/config.go @@ -51,6 +51,14 @@ const ( MetricsGeneratorDryRunEnabled = "metrics_generator_dry_run_enabled" ) +type HistogramMethod string + +const ( + HistogramMethodClassic HistogramMethod = "classic" + HistogramMethodNative HistogramMethod = "native" + HistogramMethodBoth HistogramMethod = "both" +) + var metricLimitsDesc = prometheus.NewDesc( "tempo_limits_defaults", "Default resource limits", @@ -132,7 +140,7 @@ type MetricsGeneratorOverrides struct { MaxActiveSeries uint32 `yaml:"max_active_series,omitempty" json:"max_active_series,omitempty"` CollectionInterval time.Duration `yaml:"collection_interval,omitempty" json:"collection_interval,omitempty"` DisableCollection bool `yaml:"disable_collection,omitempty" json:"disable_collection,omitempty"` - GenerateNativeHistograms bool `yaml:"generate_native_histograms" json:"generate_native_histograms,omitempty"` + GenerateNativeHistograms string `yaml:"generate_native_histograms" json:"generate_native_histograms,omitempty"` TraceIDLabelName string `yaml:"trace_id_label_name,omitempty" json:"trace_id_label_name,omitempty"` RemoteWriteHeaders RemoteWriteHeaders `yaml:"remote_write_headers,omitempty" json:"remote_write_headers,omitempty"` @@ -278,3 +286,7 @@ func (c *Config) Collect(ch chan<- prometheus.Metric) { ch <- prometheus.MustNewConstMetric(metricLimitsDesc, prometheus.GaugeValue, float64(c.Defaults.Compaction.BlockRetention), MetricBlockRetention) ch <- prometheus.MustNewConstMetric(metricLimitsDesc, prometheus.GaugeValue, float64(c.Defaults.MetricsGenerator.MaxActiveSeries), MetricMetricsGeneratorMaxActiveSeries) } + +func HasNativeHistograms(s string) bool { + return s == string(HistogramMethodNative) || s == string(HistogramMethodBoth) +} diff --git a/modules/overrides/config_legacy.go b/modules/overrides/config_legacy.go index 9b1f9fed6ed..3e12340101a 100644 --- a/modules/overrides/config_legacy.go +++ b/modules/overrides/config_legacy.go @@ -90,7 +90,7 @@ type LegacyOverrides struct { MetricsGeneratorMaxActiveSeries uint32 `yaml:"metrics_generator_max_active_series" json:"metrics_generator_max_active_series"` MetricsGeneratorCollectionInterval time.Duration `yaml:"metrics_generator_collection_interval" json:"metrics_generator_collection_interval"` MetricsGeneratorDisableCollection bool `yaml:"metrics_generator_disable_collection" json:"metrics_generator_disable_collection"` - MetricsGeneratorGenerateNativeHistograms bool `yaml:"metrics_generator_generate_native_histograms" json:"metrics_generator_generate_native_histograms"` + MetricsGeneratorGenerateNativeHistograms string `yaml:"metrics_generator_generate_native_histograms" json:"metrics_generator_generate_native_histograms"` MetricsGeneratorTraceIDLabelName string `yaml:"metrics_generator_trace_id_label_name" json:"metrics_generator_trace_id_label_name"` MetricsGeneratorForwarderQueueSize int `yaml:"metrics_generator_forwarder_queue_size" json:"metrics_generator_forwarder_queue_size"` MetricsGeneratorForwarderWorkers int `yaml:"metrics_generator_forwarder_workers" json:"metrics_generator_forwarder_workers"` diff --git a/modules/overrides/interface.go b/modules/overrides/interface.go index 4d77026a8c7..f3d6af0349c 100644 --- a/modules/overrides/interface.go +++ b/modules/overrides/interface.go @@ -46,7 +46,7 @@ type Interface interface { MetricsGeneratorMaxActiveSeries(userID string) uint32 MetricsGeneratorCollectionInterval(userID string) time.Duration MetricsGeneratorDisableCollection(userID string) bool - MetricsGeneratorGenerateNativeHistograms(userID string) bool + MetricsGeneratorGenerateNativeHistograms(userID string) string MetricsGenerationTraceIDLabelName(userID string) string MetricsGeneratorRemoteWriteHeaders(userID string) map[string]string MetricsGeneratorForwarderQueueSize(userID string) int diff --git a/modules/overrides/runtime_config_overrides.go b/modules/overrides/runtime_config_overrides.go index 5df119e8a46..5402253d22e 100644 --- a/modules/overrides/runtime_config_overrides.go +++ b/modules/overrides/runtime_config_overrides.go @@ -396,7 +396,7 @@ func (o *runtimeConfigOverridesManager) MetricsGeneratorDisableCollection(userID return o.getOverridesForUser(userID).MetricsGenerator.DisableCollection } -func (o *runtimeConfigOverridesManager) MetricsGeneratorGenerateNativeHistograms(userID string) bool { +func (o *runtimeConfigOverridesManager) MetricsGeneratorGenerateNativeHistograms(userID string) string { return o.getOverridesForUser(userID).MetricsGenerator.GenerateNativeHistograms } From d1847bcbe016fb2733827fb2d5fd8fec40dd8f25 Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Wed, 3 Jul 2024 16:11:03 +0000 Subject: [PATCH 23/26] Push mode func into native histograms implementation for update handling --- .../generator/registry/native_histogram.go | 112 +++++++++++------- .../registry/native_histogram_test.go | 18 ++- modules/generator/registry/registry.go | 6 +- modules/overrides/config.go | 4 + 4 files changed, 96 insertions(+), 44 deletions(-) diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go index 9588400d9d6..4ffab25b1f4 100644 --- a/modules/generator/registry/native_histogram.go +++ b/modules/generator/registry/native_histogram.go @@ -35,8 +35,10 @@ type nativeHistogram struct { traceIDLabelName string - // "native" or "both", to include classic histograms. - mode string + // Returns "native" or "both", to include classic histograms. We wrap the + // ovverrides with this func so that we can update the implementation after + // the nativeHistogram has been created. + modeFunc func() string } type nativeHistogramSeries struct { @@ -52,7 +54,7 @@ var ( _ metric = (*nativeHistogram)(nil) ) -func newNativeHistogram(name string, buckets []float64, onAddSeries func(uint32) bool, onRemoveSeries func(count uint32), traceIDLabelName string, f func() string) *nativeHistogram { +func newNativeHistogram(name string, buckets []float64, onAddSeries func(uint32) bool, onRemoveSeries func(count uint32), traceIDLabelName string, histogramsModeFunc func() string) *nativeHistogram { if onAddSeries == nil { onAddSeries = func(uint32) bool { return true @@ -62,6 +64,12 @@ func newNativeHistogram(name string, buckets []float64, onAddSeries func(uint32) onRemoveSeries = func(uint32) {} } + if histogramsModeFunc == nil { + histogramsModeFunc = func() string { + return "native" + } + } + if traceIDLabelName == "" { traceIDLabelName = "traceID" } @@ -73,8 +81,7 @@ func newNativeHistogram(name string, buckets []float64, onAddSeries func(uint32) onRemoveSerie: onRemoveSeries, traceIDLabelName: traceIDLabelName, buckets: buckets, - // FIXME: the mode never gets updated after here, so we can't downgrade from "both" to "native" - mode: f(), + modeFunc: histogramsModeFunc, } } @@ -164,53 +171,37 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 return activeSeries, err } - // Store the encoded histogram here so we can keep track of 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" mode, also emit classic histograms - if h.mode == string(overrides.HistogramMethodBoth) { + // NOTE: A subtle point here is that the new histogram implementation is + // chosen in the registry. This means that if the user overrides change + // from "both" to "classic", we will still be using the new histograms + // implementation. This is because the registry is not recreated when the + // overrides change. This is a tradeoff to avoid recreating the registry + // for every change in the overrides, but it does mean that even if user + // changes to "classic" histograms, we will still be using the new + // histograms implementation and want to avoid generating native + // histograms. + + // If we are in "both" or "classic" mode, also emit classic histograms. + if overrides.HasClassicHistograms(h.modeFunc()) { classicSeries, classicErr := h.classicHistograms(appender, lb, timeMs, s) - activeSeries += classicSeries if classicErr != nil { return activeSeries, classicErr } + activeSeries += classicSeries } - // Decode to Prometheus representation - hist := promhistogram.Histogram{ - Schema: s.histogram.GetSchema(), - Count: s.histogram.GetSampleCount(), - Sum: s.histogram.GetSampleSum(), - ZeroThreshold: s.histogram.GetZeroThreshold(), - ZeroCount: s.histogram.GetZeroCount(), - } - if len(s.histogram.PositiveSpan) > 0 { - hist.PositiveSpans = make([]promhistogram.Span, len(s.histogram.PositiveSpan)) - for i, span := range s.histogram.PositiveSpan { - hist.PositiveSpans[i] = promhistogram.Span{ - Offset: span.GetOffset(), - Length: span.GetLength(), - } + // If we are in "both" or "native" mode, also emit native histograms. + if overrides.HasNativeHistograms(h.modeFunc()) { + nativeSeries, nativeErr := h.nativeHistograms(appender, lb, timeMs, s) + if nativeErr != nil { + return nativeSeries, nativeErr } - } - hist.PositiveBuckets = s.histogram.PositiveDelta - if len(s.histogram.NegativeSpan) > 0 { - hist.NegativeSpans = make([]promhistogram.Span, len(s.histogram.NegativeSpan)) - for i, span := range s.histogram.NegativeSpan { - hist.NegativeSpans[i] = promhistogram.Span{ - Offset: span.GetOffset(), - Length: span.GetLength(), - } - } - } - hist.NegativeBuckets = s.histogram.NegativeDelta - - lb.Set(labels.MetricName, h.metricName) - _, err = appender.AppendHistogram(0, lb.Labels(), timeMs, &hist, nil) - if err != nil { - return activeSeries, err + activeSeries += nativeSeries } // TODO: impact on active series from appending a histogram? @@ -254,6 +245,45 @@ func (h *nativeHistogram) activeSeriesPerHistogramSerie() uint32 { return 1 } +func (h *nativeHistogram) nativeHistograms(appender storage.Appender, lb *labels.Builder, timeMs int64, s *nativeHistogramSeries) (activeSeries int, err error) { + // Decode to Prometheus representation + hist := promhistogram.Histogram{ + Schema: s.histogram.GetSchema(), + Count: s.histogram.GetSampleCount(), + Sum: s.histogram.GetSampleSum(), + ZeroThreshold: s.histogram.GetZeroThreshold(), + ZeroCount: s.histogram.GetZeroCount(), + } + if len(s.histogram.PositiveSpan) > 0 { + hist.PositiveSpans = make([]promhistogram.Span, len(s.histogram.PositiveSpan)) + for i, span := range s.histogram.PositiveSpan { + hist.PositiveSpans[i] = promhistogram.Span{ + Offset: span.GetOffset(), + Length: span.GetLength(), + } + } + } + hist.PositiveBuckets = s.histogram.PositiveDelta + if len(s.histogram.NegativeSpan) > 0 { + hist.NegativeSpans = make([]promhistogram.Span, len(s.histogram.NegativeSpan)) + for i, span := range s.histogram.NegativeSpan { + hist.NegativeSpans[i] = promhistogram.Span{ + Offset: span.GetOffset(), + Length: span.GetLength(), + } + } + } + hist.NegativeBuckets = s.histogram.NegativeDelta + + lb.Set(labels.MetricName, h.metricName) + _, err = appender.AppendHistogram(0, lb.Labels(), timeMs, &hist, nil) + if err != nil { + return activeSeries, err + } + + return +} + func (h *nativeHistogram) classicHistograms(appender storage.Appender, lb *labels.Builder, timeMs int64, s *nativeHistogramSeries) (activeSeries int, err error) { // sum lb.Set(labels.MetricName, h.metricName+"_sum") diff --git a/modules/generator/registry/native_histogram_test.go b/modules/generator/registry/native_histogram_test.go index 9173c9aef63..7ceab9c82e7 100644 --- a/modules/generator/registry/native_histogram_test.go +++ b/modules/generator/registry/native_histogram_test.go @@ -19,7 +19,12 @@ func Test_ObserveWithExemplar_duplicate(t *testing.T) { seriesAdded += int(count) return true } - h := newNativeHistogram("my_histogram", []float64{0.1, 0.2}, onAdd, nil, "trace_id", "both") + + f := func() string { + return "both" + } + + h := newNativeHistogram("my_histogram", []float64{0.1, 0.2}, onAdd, nil, "trace_id", f) lv := newLabelValueCombo([]string{"label"}, []string{"value-1"}) @@ -54,6 +59,7 @@ func Test_Histograms(t *testing.T) { collections collections // native histogram does not support all features yet skipNativeHistogram bool + modeFunc func() string }{ { name: "single collection single observation", @@ -453,12 +459,20 @@ func Test_Histograms(t *testing.T) { t.SkipNow() } + modeFunc := tc.modeFunc + if modeFunc == nil { + modeFunc = func() string { + return "both" + } + } + var seriesAdded int onAdd := func(count uint32) bool { seriesAdded += int(count) return true } - h := newNativeHistogram("test_histogram", tc.buckets, onAdd, nil, "trace_id", "both") + + h := newNativeHistogram("test_histogram", tc.buckets, onAdd, nil, "trace_id", modeFunc) testHistogram(t, h, tc.collections) }) }) diff --git a/modules/generator/registry/registry.go b/modules/generator/registry/registry.go index 32be25c9e07..4087eb665c8 100644 --- a/modules/generator/registry/registry.go +++ b/modules/generator/registry/registry.go @@ -153,10 +153,14 @@ func (r *ManagedRegistry) NewHistogram(name string, buckets []float64) (h Histog histograms := r.overrides.MetricsGeneratorGenerateNativeHistograms(r.tenant) + histogramsModeFunc := func() string { + return r.overrides.MetricsGeneratorGenerateNativeHistograms(r.tenant) + } + // Temporary switch: use the old implementation when native histograms are // disabled, eventually the new implementation can handle all cases if overrides.HasNativeHistograms(histograms) { - h = newNativeHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName, histograms) + h = newNativeHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName, histogramsModeFunc) } else { h = newHistogram(name, buckets, r.onAddMetricSeries, r.onRemoveMetricSeries, traceIDLabelName) } diff --git a/modules/overrides/config.go b/modules/overrides/config.go index beb4853a7f9..0717e1722b9 100644 --- a/modules/overrides/config.go +++ b/modules/overrides/config.go @@ -290,3 +290,7 @@ func (c *Config) Collect(ch chan<- prometheus.Metric) { func HasNativeHistograms(s string) bool { return s == string(HistogramMethodNative) || s == string(HistogramMethodBoth) } + +func HasClassicHistograms(s string) bool { + return s == string(HistogramMethodClassic) || s == string(HistogramMethodBoth) +} From b81dc77e86ef2e5d6c46c67cd4de3a7f26a24efb Mon Sep 17 00:00:00 2001 From: Zach Leslie Date: Wed, 3 Jul 2024 16:56:29 +0000 Subject: [PATCH 24/26] Drop unused variable --- modules/generator/registry/native_histogram.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/modules/generator/registry/native_histogram.go b/modules/generator/registry/native_histogram.go index 4ffab25b1f4..a5b155d433f 100644 --- a/modules/generator/registry/native_histogram.go +++ b/modules/generator/registry/native_histogram.go @@ -197,11 +197,10 @@ func (h *nativeHistogram) collectMetrics(appender storage.Appender, timeMs int64 // If we are in "both" or "native" mode, also emit native histograms. if overrides.HasNativeHistograms(h.modeFunc()) { - nativeSeries, nativeErr := h.nativeHistograms(appender, lb, timeMs, s) + nativeErr := h.nativeHistograms(appender, lb, timeMs, s) if nativeErr != nil { - return nativeSeries, nativeErr + return activeSeries, nativeErr } - activeSeries += nativeSeries } // TODO: impact on active series from appending a histogram? @@ -245,7 +244,7 @@ func (h *nativeHistogram) activeSeriesPerHistogramSerie() uint32 { return 1 } -func (h *nativeHistogram) nativeHistograms(appender storage.Appender, lb *labels.Builder, timeMs int64, s *nativeHistogramSeries) (activeSeries int, err error) { +func (h *nativeHistogram) nativeHistograms(appender storage.Appender, lb *labels.Builder, timeMs int64, s *nativeHistogramSeries) (err error) { // Decode to Prometheus representation hist := promhistogram.Histogram{ Schema: s.histogram.GetSchema(), @@ -278,7 +277,7 @@ func (h *nativeHistogram) nativeHistograms(appender storage.Appender, lb *labels lb.Set(labels.MetricName, h.metricName) _, err = appender.AppendHistogram(0, lb.Labels(), timeMs, &hist, nil) if err != nil { - return activeSeries, err + return err } return From 89363e6c0584c7a9dbd96327df7f0d8cc9f0963e Mon Sep 17 00:00:00 2001 From: yuna Date: Wed, 10 Jul 2024 18:29:43 +0200 Subject: [PATCH 25/26] Set --enable-feature=native-histograms on all prometheus docker-compose setups --- example/docker-compose/agent/docker-compose.yaml | 1 + example/docker-compose/cross-cluster/docker-compose.yaml | 1 + example/docker-compose/debug/docker-compose.yaml | 1 + example/docker-compose/distributed/docker-compose.yaml | 1 + .../otel-collector-multitenant/docker-compose.yaml | 1 + example/docker-compose/otel-collector/docker-compose.yaml | 1 + .../docker-compose/scalable-single-binary/docker-compose.yaml | 1 + 7 files changed, 7 insertions(+) diff --git a/example/docker-compose/agent/docker-compose.yaml b/example/docker-compose/agent/docker-compose.yaml index 856a38c9212..b84a3c0b624 100644 --- a/example/docker-compose/agent/docker-compose.yaml +++ b/example/docker-compose/agent/docker-compose.yaml @@ -51,6 +51,7 @@ services: - --config.file=/etc/prometheus.yaml - --web.enable-remote-write-receiver - --enable-feature=exemplar-storage + - --enable-feature=native-histograms volumes: - ../shared/prometheus.yaml:/etc/prometheus.yaml ports: diff --git a/example/docker-compose/cross-cluster/docker-compose.yaml b/example/docker-compose/cross-cluster/docker-compose.yaml index c272148b423..ea9d210807b 100644 --- a/example/docker-compose/cross-cluster/docker-compose.yaml +++ b/example/docker-compose/cross-cluster/docker-compose.yaml @@ -164,6 +164,7 @@ services: - --config.file=/etc/prometheus.yaml - --web.enable-remote-write-receiver - --enable-feature=exemplar-storage + - --enable-feature=native-histograms volumes: - ./prometheus.yaml:/etc/prometheus.yaml ports: diff --git a/example/docker-compose/debug/docker-compose.yaml b/example/docker-compose/debug/docker-compose.yaml index b61fe154cd0..b6d7a60397a 100644 --- a/example/docker-compose/debug/docker-compose.yaml +++ b/example/docker-compose/debug/docker-compose.yaml @@ -44,6 +44,7 @@ services: - --config.file=/etc/prometheus.yaml - --web.enable-remote-write-receiver - --enable-feature=exemplar-storage + - --enable-feature=native-histograms volumes: - ../shared/prometheus.yaml:/etc/prometheus.yaml ports: diff --git a/example/docker-compose/distributed/docker-compose.yaml b/example/docker-compose/distributed/docker-compose.yaml index 0dbdf33542b..abf51e32abe 100644 --- a/example/docker-compose/distributed/docker-compose.yaml +++ b/example/docker-compose/distributed/docker-compose.yaml @@ -140,6 +140,7 @@ services: - --config.file=/etc/prometheus.yaml - --web.enable-remote-write-receiver - --enable-feature=exemplar-storage + - --enable-feature=native-histograms volumes: - ./prometheus.yaml:/etc/prometheus.yaml ports: diff --git a/example/docker-compose/otel-collector-multitenant/docker-compose.yaml b/example/docker-compose/otel-collector-multitenant/docker-compose.yaml index 9b1acf8ee70..ed6cf59e4fe 100644 --- a/example/docker-compose/otel-collector-multitenant/docker-compose.yaml +++ b/example/docker-compose/otel-collector-multitenant/docker-compose.yaml @@ -49,6 +49,7 @@ services: - --config.file=/etc/prometheus.yaml - --web.enable-remote-write-receiver - --enable-feature=exemplar-storage + - --enable-feature=native-histograms volumes: - ../shared/prometheus.yaml:/etc/prometheus.yaml ports: diff --git a/example/docker-compose/otel-collector/docker-compose.yaml b/example/docker-compose/otel-collector/docker-compose.yaml index a84d7b3988c..c9c3fbedac5 100644 --- a/example/docker-compose/otel-collector/docker-compose.yaml +++ b/example/docker-compose/otel-collector/docker-compose.yaml @@ -49,6 +49,7 @@ services: - --config.file=/etc/prometheus.yaml - --web.enable-remote-write-receiver - --enable-feature=exemplar-storage + - --enable-feature=native-histograms volumes: - ../shared/prometheus.yaml:/etc/prometheus.yaml ports: diff --git a/example/docker-compose/scalable-single-binary/docker-compose.yaml b/example/docker-compose/scalable-single-binary/docker-compose.yaml index 9ac9f765d45..2cb2bc2106c 100644 --- a/example/docker-compose/scalable-single-binary/docker-compose.yaml +++ b/example/docker-compose/scalable-single-binary/docker-compose.yaml @@ -55,6 +55,7 @@ services: - --config.file=/etc/prometheus.yaml - --web.enable-remote-write-receiver - --enable-feature=exemplar-storage + - --enable-feature=native-histograms volumes: - ./prometheus.yaml:/etc/prometheus.yaml From f19e1b7b2b6fb09539ac59bb0a816e429dadc24f Mon Sep 17 00:00:00 2001 From: yuna Date: Wed, 10 Jul 2024 18:38:55 +0200 Subject: [PATCH 26/26] Update generate_native_histograms setting --- example/docker-compose/shared/tempo.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/example/docker-compose/shared/tempo.yaml b/example/docker-compose/shared/tempo.yaml index f21f99659d7..b1ab5ce7607 100644 --- a/example/docker-compose/shared/tempo.yaml +++ b/example/docker-compose/shared/tempo.yaml @@ -57,4 +57,4 @@ overrides: defaults: metrics_generator: processors: [service-graphs, span-metrics, local-blocks] # enables metrics generator - generate_native_histograms: true + generate_native_histograms: both