Fix bug where an empty exemplar was added to counters (#5395)

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

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,42 @@ 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 will only include the +Inf bucket if there is an exemplar for it
+	// https://github.com/prometheus/client_golang/blob/d038ab96c0c7b9cd217a39072febd610bcdf1fd8/prometheus/metric.go#L189
+	// we need to handle the case where it is present, or where it is missing.
+	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
 }
 

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:       []float64{0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10},
+									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,