From 6c7d13b94930eb64693a861dea881d157b2826c5 Mon Sep 17 00:00:00 2001 From: David Ashpole Date: Mon, 15 Apr 2024 13:05:43 +0000 Subject: [PATCH] fix bug where an empty exemplar was added to counters --- CHANGELOG.md | 5 ++ bridges/prometheus/producer.go | 42 +++++++++++----- bridges/prometheus/producer_test.go | 77 ++++++++++++++++++++++++++++- 3 files changed, 110 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e0994e6bea..3ef0d841922 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,11 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Add the new `go.opentelemetry.io/contrib/instrgen` package to provide auto-generated source code instrumentation. (#3068, #3108) - `NewSDK` in `go.opentelemetry.io/contrib/config` now returns a configured SDK with a valid `MeterProvider`. (#4804) +### Fixed + +- Fix bug where an empty exemplar was added to counters in `go.opentelemetry.io/contrib/bridges/prometheus`. (#5395) +- Fix bug where the last histogram bucket was missing in `go.opentelemetry.io/contrib/bridges/prometheus`. (#5395) + ## [1.25.0/0.50.0/0.19.0/0.5.0/0.0.1] - 2024-04-05 ### Added diff --git a/bridges/prometheus/producer.go b/bridges/prometheus/producer.go index f98048ca98f..6a892352383 100644 --- a/bridges/prometheus/producer.go +++ b/bridges/prometheus/producer.go @@ -7,6 +7,7 @@ import ( "context" "errors" "fmt" + "math" "strings" "time" @@ -150,7 +151,9 @@ func convertCounter(metrics []*dto.Metric, now time.Time) metricdata.Sum[float64 StartTime: processStartTime, Time: now, Value: m.GetCounter().GetValue(), - Exemplars: []metricdata.Exemplar[float64]{convertExemplar(m.GetCounter().GetExemplar())}, + } + if ex := m.GetCounter().GetExemplar(); ex != nil { + dp.Exemplars = []metricdata.Exemplar[float64]{convertExemplar(ex)} } createdTs := m.GetCounter().GetCreatedTimestamp() if createdTs.IsValid() { @@ -251,7 +254,7 @@ func convertHistogram(metrics []*dto.Metric, now time.Time) metricdata.Histogram Temporality: metricdata.CumulativeTemporality, } for i, m := range metrics { - bounds, bucketCounts, exemplars := convertBuckets(m.GetHistogram().GetBucket()) + bounds, bucketCounts, exemplars := convertBuckets(m.GetHistogram().GetBucket(), m.GetHistogram().GetSampleCount()) dp := metricdata.HistogramDataPoint[float64]{ Attributes: convertLabels(m.GetLabel()), StartTime: processStartTime, @@ -274,26 +277,41 @@ func convertHistogram(metrics []*dto.Metric, now time.Time) metricdata.Histogram return otelHistogram } -func convertBuckets(buckets []*dto.Bucket) ([]float64, []uint64, []metricdata.Exemplar[float64]) { +func convertBuckets(buckets []*dto.Bucket, sampleCount uint64) ([]float64, []uint64, []metricdata.Exemplar[float64]) { if len(buckets) == 0 { // This should never happen return nil, nil, nil } - bounds := make([]float64, len(buckets)-1) - bucketCounts := make([]uint64, len(buckets)) + // buckets may, or may not include the +Inf bucket, so we need to handle + // both cases. + hasInf := math.IsInf(buckets[len(buckets)-1].GetUpperBound(), +1) + var bounds []float64 + var bucketCounts []uint64 + if hasInf { + bounds = make([]float64, len(buckets)-1) + bucketCounts = make([]uint64, len(buckets)) + } else { + bounds = make([]float64, len(buckets)) + bucketCounts = make([]uint64, len(buckets)+1) + } exemplars := make([]metricdata.Exemplar[float64], 0) for i, bucket := range buckets { - // The last bound is the +Inf bound, which is implied in OTel, but is - // explicit in Prometheus. Skip the last boundary, and assume it is the - // +Inf bound. - if i < len(bounds) { - bounds[i] = bucket.GetUpperBound() + // The last bound may be the +Inf bucket, which is implied in OTel, but + // is explicit in Prometheus. Skip the last boundary if it is the +Inf + // bound. + if bound := bucket.GetUpperBound(); !math.IsInf(bound, +1) { + bounds[i] = bound } bucketCounts[i] = bucket.GetCumulativeCount() - if bucket.GetExemplar() != nil { - exemplars = append(exemplars, convertExemplar(bucket.GetExemplar())) + if ex := bucket.GetExemplar(); ex != nil { + exemplars = append(exemplars, convertExemplar(ex)) } } + if !hasInf { + // The Inf bucket was missing, so set the last bucket counts to the + // overall count + bucketCounts[len(bucketCounts)-1] = sampleCount + } return bounds, bucketCounts, exemplars } diff --git a/bridges/prometheus/producer_test.go b/bridges/prometheus/producer_test.go index e5afec45e24..a529c957838 100644 --- a/bridges/prometheus/producer_test.go +++ b/bridges/prometheus/producer_test.go @@ -69,6 +69,41 @@ func TestProduce(t *testing.T) { }, { name: "counter", + testFn: func(reg *prometheus.Registry) { + metric := prometheus.NewCounter(prometheus.CounterOpts{ + Name: "test_counter_metric", + Help: "A counter metric for testing", + ConstLabels: prometheus.Labels(map[string]string{ + "foo": "bar", + }), + }) + reg.MustRegister(metric) + metric.Add(245.3) + }, + expected: []metricdata.ScopeMetrics{{ + Scope: instrumentation.Scope{ + Name: scopeName, + }, + Metrics: []metricdata.Metrics{ + { + Name: "test_counter_metric", + Description: "A counter metric for testing", + Data: metricdata.Sum[float64]{ + Temporality: metricdata.CumulativeTemporality, + IsMonotonic: true, + DataPoints: []metricdata.DataPoint[float64]{ + { + Attributes: attribute.NewSet(attribute.String("foo", "bar")), + Value: 245.3, + }, + }, + }, + }, + }, + }}, + }, + { + name: "counter with exemplar", testFn: func(reg *prometheus.Registry) { metric := prometheus.NewCounter(prometheus.CounterOpts{ Name: "test_counter_metric", @@ -167,6 +202,44 @@ func TestProduce(t *testing.T) { }), }) reg.MustRegister(metric) + metric.Observe(578.3) + }, + expected: []metricdata.ScopeMetrics{{ + Scope: instrumentation.Scope{ + Name: scopeName, + }, + Metrics: []metricdata.Metrics{ + { + Name: "test_histogram_metric", + Description: "A histogram metric for testing", + Data: metricdata.Histogram[float64]{ + Temporality: metricdata.CumulativeTemporality, + DataPoints: []metricdata.HistogramDataPoint[float64]{ + { + Count: 1, + Sum: 578.3, + Bounds: prometheus.DefBuckets, + BucketCounts: []uint64{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}, + Attributes: attribute.NewSet(attribute.String("foo", "bar")), + Exemplars: []metricdata.Exemplar[float64]{}, + }, + }, + }, + }, + }, + }}, + }, + { + name: "histogram with exemplar", + testFn: func(reg *prometheus.Registry) { + metric := prometheus.NewHistogram(prometheus.HistogramOpts{ + Name: "test_histogram_metric_with_exemplar", + Help: "A histogram metric for testing", + ConstLabels: prometheus.Labels(map[string]string{ + "foo": "bar", + }), + }) + reg.MustRegister(metric) metric.(prometheus.ExemplarObserver).ObserveWithExemplar( 578.3, prometheus.Labels{ "trace_id": traceIDStr, @@ -181,7 +254,7 @@ func TestProduce(t *testing.T) { }, Metrics: []metricdata.Metrics{ { - Name: "test_histogram_metric", + Name: "test_histogram_metric_with_exemplar", Description: "A histogram metric for testing", Data: metricdata.Histogram[float64]{ Temporality: metricdata.CumulativeTemporality, @@ -189,7 +262,7 @@ func TestProduce(t *testing.T) { { Count: 1, Sum: 578.3, - Bounds: []float64{0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10}, + Bounds: prometheus.DefBuckets, BucketCounts: []uint64{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}, Attributes: attribute.NewSet(attribute.String("foo", "bar")), Exemplars: []metricdata.Exemplar[float64]{