PR feedback

* Move OTLP implementation to separate file
* Simplify map key sorting

Author: dricross

metric/distribution/exph/exph.go

@@ -4,8 +4,10 @@
 package exph
 
 import (
+	"cmp"
 	"fmt"
 	"log"
+	"maps"
 	"math"
 	"slices"
 
@@ -77,12 +79,9 @@ func (d *ExpHistogramDistribution) ValuesAndCounts() ([]float64, []float64) {
 	counts := []float64{}
 
 	// iterate through positive buckets in descending order
-	posOffsetIndicies := make([]int, 0, len(d.positiveBuckets))
-	for offsetIndex := range d.positiveBuckets {
-		posOffsetIndicies = append(posOffsetIndicies, offsetIndex)
-	}
-	slices.Sort(posOffsetIndicies)
-	slices.Reverse(posOffsetIndicies)
+	posOffsetIndicies := slices.SortedFunc(maps.Keys(d.positiveBuckets), func(a, b int) int {
+		return cmp.Compare(b, a)
+	})
 	for _, offsetIndex := range posOffsetIndicies {
 		counter := d.positiveBuckets[offsetIndex]
 		bucketBegin := LowerBoundary(offsetIndex, int(d.scale))
@@ -98,11 +97,7 @@ func (d *ExpHistogramDistribution) ValuesAndCounts() ([]float64, []float64) {
 	}
 
 	// iterate through negative buckets in ascending order
-	negOffsetIndicies := make([]int, 0, len(d.negativeBuckets))
-	for offsetIndex := range d.negativeBuckets {
-		negOffsetIndicies = append(negOffsetIndicies, offsetIndex)
-	}
-	slices.Sort(negOffsetIndicies)
+	negOffsetIndicies := slices.Sorted(maps.Keys(d.negativeBuckets))
 	for _, offsetIndex := range negOffsetIndicies {
 		counter := d.negativeBuckets[offsetIndex]
 		bucketBegin := LowerBoundary(offsetIndex, int(d.scale))
@@ -228,62 +223,6 @@ func (d *ExpHistogramDistribution) ConvertFromOtel(dp pmetric.ExponentialHistogr
 }
 
 func (d *ExpHistogramDistribution) Resize(_ int) []*ExpHistogramDistribution {
-	// for now, do not split data points into separate PMD requests
+	// TODO: split data points into separate PMD requests if the number of buckets exceeds the API limit
 	return []*ExpHistogramDistribution{d}
 }
-
-// MapToIndexScale0 computes a bucket index at scale 0.
-func MapToIndexScale0(value float64) int {
-	// Note: Frexp() rounds submnormal values to the smallest normal
-	// value and returns an exponent corresponding to fractions in the
-	// range [0.5, 1), whereas an exponent for the range [1, 2), so
-	// subtract 1 from the exponent immediately.
-	frac, exp := math.Frexp(value)
-	exp--
-
-	if frac == 0.5 && value > 0 {
-		// Special case for positive powers of two: they fall into the bucket
-		// numbered one less.
-		exp--
-	}
-	return exp
-}
-
-// MapToIndexNegativeScale computes a bucket index for scales <= 0.
-func MapToIndexNegativeScale(value float64, scale int) int {
-	return MapToIndexScale0(value) >> -scale
-}
-
-// MapToIndex for any scale
-//
-// Values near a boundary could be mapped into the incorrect bucket due to float point calculation inaccuracy.
-func MapToIndex(value float64, scale int) int {
-	// Special case for power-of-two values.
-	if frac, exp := math.Frexp(value); frac == 0.5 {
-		return ((exp - 1) << scale) - 1
-	}
-	scaleFactor := math.Ldexp(math.Log2E, scale)
-	// The use of math.Log() to calculate the bucket index is not guaranteed to be exactly correct near powers of two.
-	return int(math.Floor(math.Log(math.Abs(value)) * scaleFactor))
-}
-
-// LowerBoundaryNegativeScale computes the lower boundary for index
-// with scales <= 0.
-func LowerBoundaryNegativeScale(index int, scale int) float64 {
-	return math.Ldexp(1, index<<-scale)
-}
-
-func LowerBoundary(index, scale int) float64 {
-	if scale <= 0 {
-		return LowerBoundaryNegativeScale(index, scale)
-	}
-	return LowerBoundaryPositiveScale(index, scale)
-}
-
-// LowerBoundary computes the bucket boundary for positive scales.
-//
-// The returned value may be inaccurate due to accumulated floating point calculation errors
-func LowerBoundaryPositiveScale(index, scale int) float64 {
-	inverseFactor := math.Ldexp(math.Ln2, -scale)
-	return math.Exp(float64(index) * inverseFactor)
-}

metric/distribution/exph/exph_test.go

@@ -638,207 +638,6 @@ func TestAddDistribution(t *testing.T) {
 
 }
 
-func TestMapToIndexPositiveScale(t *testing.T) {
-	tests := []struct {
-		name     string
-		scale    int
-		values   []float64
-		expected []int
-	}{
-		{
-			name:     "positive value inside bucket",
-			scale:    1,
-			values:   []float64{1.3, 1.5, 2.2, 3.9, 4.2, 6.0},
-			expected: []int{0, 1, 2, 3, 4, 5},
-		},
-		{
-			// for positive values, histogram buckets use upper-inclusive boundaries
-			// this is only reliable on boundaries that are powers of 2
-			name:     "positive value is on boundary",
-			scale:    1,
-			values:   []float64{2.0, 4.0, 8.0},
-			expected: []int{1, 3, 5},
-		},
-		{
-			name:     "negative value inside bucket",
-			scale:    1,
-			values:   []float64{-1.3, -1.5, -2.2, -3.9, -4.2, -6.0},
-			expected: []int{0, 1, 2, 3, 4, 5},
-		},
-		{
-			// for negative values, histogram buckets use lower-inclusive boundaries
-			// this is only reliable on boundaries that are powers of 2
-			name:     "negative value is on boundary",
-			scale:    1,
-			values:   []float64{-1.0, -2.0, -4.0},
-			expected: []int{0, 2, 4},
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			for i, value := range tt.values {
-				assert.Equal(t, tt.expected[i], MapToIndex(value, tt.scale), "expected value %f to map to index %d with scale %d", value, tt.expected[i], tt.scale)
-			}
-		})
-	}
-
-}
-
-func TestMapToIndexScale0(t *testing.T) {
-	tests := []struct {
-		name     string
-		values   []float64
-		expected []int
-	}{
-		{
-			name:     "positive value inside bucket",
-			values:   []float64{1.5, 3.0, 6, 12, 18},
-			expected: []int{0, 1, 2, 3, 4},
-		},
-		{
-			// for negative values, histogram buckets use lower-inclusive boundaries
-			name:     "positive value is on boundary",
-			values:   []float64{2, 4, 8, 16.0, 32},
-			expected: []int{0, 1, 2, 3, 4},
-		},
-		{
-			name:     "negative value inside bucket",
-			values:   []float64{-1.5, -3.0, -6, -12, -18},
-			expected: []int{0, 1, 2, 3, 4},
-		},
-		{
-			// for negative values, histogram buckets use lower-inclusive boundaries
-			name:     "negative value is on boundary",
-			values:   []float64{-2, -4, -8, -16, -32},
-			expected: []int{1, 2, 3, 4, 5},
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			for i, value := range tt.values {
-				assert.Equal(t, tt.expected[i], MapToIndexNegativeScale(value, 0), "expected value %f to map to index %d with scale 0", value, tt.expected[i])
-			}
-		})
-	}
-}
-
-func TestMapToIndexNegativeScale(t *testing.T) {
-	tests := []struct {
-		name     string
-		scale    int
-		values   []float64
-		expected []int
-	}{
-		{
-			name:     "positive value inside bucket",
-			scale:    -1,
-			values:   []float64{1.5, 5.0, 32, 80, 500, 2000},
-			expected: []int{0, 1, 2, 3, 4, 5},
-		},
-		{
-			// for positive values, histogram buckets use upper-inclusive boundaries
-			name:     "positive value is on boundary",
-			scale:    -1,
-			values:   []float64{4, 16, 64, 256, 1024},
-			expected: []int{0, 1, 2, 3, 4, 5},
-		},
-		{
-			name:     "negative value inside bucket",
-			scale:    -1,
-			values:   []float64{-1.5, -5.0, -32, -80, -500, -2000},
-			expected: []int{0, 1, 2, 3, 4, 5},
-		},
-		{
-			// for negative values, histogram buckets use lower-inclusive boundaries
-			name:     "negative value is on boundary",
-			scale:    -1,
-			values:   []float64{-1, -4, -16, -64, -256, -1024},
-			expected: []int{0, 1, 2, 3, 4, 5, 6},
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			for i, value := range tt.values {
-				assert.Equal(t, tt.expected[i], MapToIndexNegativeScale(value, tt.scale), "expected value %f to map to index %d with scale %d", value, tt.expected[i], tt.scale)
-			}
-		})
-	}
-}
-
-func TestLowerBoundary(t *testing.T) {
-	// scale = 1, base = 2^(1/2) or sqrt(2) = 1.41421
-	assert.InDelta(t, 1.41421, LowerBoundary(1, 1), 0.01) // 2^(1/2)
-	assert.InDelta(t, 2.0, LowerBoundary(2, 1), 0.01)     // 2^(2/2)
-	assert.InDelta(t, 2.82842, LowerBoundary(3, 1), 0.01) // 2^(3/2)
-	assert.InDelta(t, 4.0, LowerBoundary(4, 1), 0.01)     // 2^(4/2)
-	assert.InDelta(t, 8.0, LowerBoundary(6, 1), 0.01)     // 2^(6/2)
-	assert.InDelta(t, 16.0, LowerBoundary(8, 1), 0.01)    // 2^(8/2)
-
-	// scale = 2, base = 2^(1/4) = 1.18921
-	assert.InDelta(t, 1.18921, LowerBoundary(1, 2), 0.01) // 2^(1/4)
-	assert.InDelta(t, 1.41421, LowerBoundary(2, 2), 0.01) // 2^(2/4)
-	assert.InDelta(t, 1.68180, LowerBoundary(3, 2), 0.01) // 2^(3/4)
-	assert.InDelta(t, 2.0, LowerBoundary(4, 2), 0.01)     // 2^(4/4)
-	assert.InDelta(t, 2.82842, LowerBoundary(6, 2), 0.01) // 2^(6/8)
-	assert.InDelta(t, 4.0, LowerBoundary(8, 2), 0.01)     // 2^(8/8)
-
-	// scale = 0, base = 2
-	assert.Equal(t, 1.0, LowerBoundary(0, 0)) // 2^0
-	assert.Equal(t, 2.0, LowerBoundary(1, 0)) // 2^1
-	assert.Equal(t, 4.0, LowerBoundary(2, 0)) // 2^2
-	assert.Equal(t, 8.0, LowerBoundary(3, 0)) // 2^3
-
-	assert.Equal(t, 1.0, LowerBoundary(0, -1))  // 4^0
-	assert.Equal(t, 4.0, LowerBoundary(1, -1))  // 4^1
-	assert.Equal(t, 16.0, LowerBoundary(2, -1)) // 4^2
-	assert.Equal(t, 64.0, LowerBoundary(3, -1)) // 4^3
-
-	// scale = -2, base = 2^(2^2) = 2^4 = 16
-	assert.Equal(t, 1.0, LowerBoundary(0, -2))    // 16^0
-	assert.Equal(t, 16.0, LowerBoundary(1, -2))   // 16^1
-	assert.Equal(t, 256.0, LowerBoundary(2, -2))  // 16^2
-	assert.Equal(t, 4096.0, LowerBoundary(3, -2)) // 16^3
-
-	assert.Equal(t, 1.0, LowerBoundary(0, -1))  // 4^0
-	assert.Equal(t, 4.0, LowerBoundary(1, -1))  // 2^(2^1)^1 = 4^1 = 2^2 = 4^1
-	assert.Equal(t, 16.0, LowerBoundary(2, -1)) // (2^2^1)^2 = 4^2 = 2^4 = 4^2
-	assert.Equal(t, 64.0, LowerBoundary(3, -1)) // (2^2^1)^3 = 4^3 = 2^6 = 4^3
-
-	// scale = -2, base = 2^(2^2) = 2^4 = 16
-	assert.Equal(t, 1.0, LowerBoundary(0, -2))    // (2^(2^2))^0 = 16^0 = 1
-	assert.Equal(t, 16.0, LowerBoundary(1, -2))   // (2^(2^2))^1 = 2^4 = 16^1
-	assert.Equal(t, 256.0, LowerBoundary(2, -2))  // (2^(2^2))^2 = 2^8 = 16^2
-	assert.Equal(t, 4096.0, LowerBoundary(3, -2)) // (2^(2^2))^3 = 2^12 = 16^3
-}
-
-func BenchmarkLowerBoundary(b *testing.B) {
-	b.Run("positive scale", func(b *testing.B) {
-		for i := 0; i < b.N; i++ {
-			LowerBoundary(10, 1)
-		}
-	})
-
-	b.Run("scale 0", func(b *testing.B) {
-		for i := 0; i < b.N; i++ {
-			LowerBoundary(10, 0)
-		}
-	})
-
-	b.Run("negative scale", func(b *testing.B) {
-		for i := 0; i < b.N; i++ {
-			LowerBoundary(10, -1)
-		}
-	})
-}
-
-func BenchmarkLowerBoundaryNegativeScale(b *testing.B) {
-	b.Run("reference", func(b *testing.B) {
-		for i := 0; i < b.N; i++ {
-			LowerBoundaryNegativeScale(10, -9)
-		}
-	})
-}
-
 func BenchmarkConvertFromOtel(b *testing.B) {
 
 	ts := time.Date(2025, time.March, 31, 22, 6, 30, 0, time.UTC)

metric/distribution/exph/mapping.go

@@ -0,0 +1,64 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: MIT
+
+// The following functions are originally sourced from OpenTelemetry's reference implementation. See
+// https://opentelemetry.io/docs/specs/otel/metrics/data-model/#producer-expectations
+package exph
+
+import "math"
+
+// MapToIndexScale0 computes a bucket index at scale 0.
+func MapToIndexScale0(value float64) int {
+	// Note: Frexp() rounds submnormal values to the smallest normal
+	// value and returns an exponent corresponding to fractions in the
+	// range [0.5, 1), whereas an exponent for the range [1, 2), so
+	// subtract 1 from the exponent immediately.
+	frac, exp := math.Frexp(value)
+	exp--
+
+	if frac == 0.5 && value > 0 {
+		// Special case for positive powers of two: they fall into the bucket
+		// numbered one less.
+		exp--
+	}
+	return exp
+}
+
+// MapToIndexNegativeScale computes a bucket index for scales <= 0.
+func MapToIndexNegativeScale(value float64, scale int) int {
+	return MapToIndexScale0(value) >> -scale
+}
+
+// MapToIndex for any scale
+//
+// Values near a boundary could be mapped into the incorrect bucket due to float point calculation inaccuracy.
+func MapToIndex(value float64, scale int) int {
+	// Special case for power-of-two values.
+	if frac, exp := math.Frexp(value); frac == 0.5 {
+		return ((exp - 1) << scale) - 1
+	}
+	scaleFactor := math.Ldexp(math.Log2E, scale)
+	// The use of math.Log() to calculate the bucket index is not guaranteed to be exactly correct near powers of two.
+	return int(math.Floor(math.Log(math.Abs(value)) * scaleFactor))
+}
+
+// LowerBoundaryNegativeScale computes the lower boundary for index
+// with scales <= 0.
+func LowerBoundaryNegativeScale(index int, scale int) float64 {
+	return math.Ldexp(1, index<<-scale)
+}
+
+func LowerBoundary(index, scale int) float64 {
+	if scale <= 0 {
+		return LowerBoundaryNegativeScale(index, scale)
+	}
+	return LowerBoundaryPositiveScale(index, scale)
+}
+
+// LowerBoundary computes the bucket boundary for positive scales.
+//
+// The returned value may be inaccurate due to accumulated floating point calculation errors
+func LowerBoundaryPositiveScale(index, scale int) float64 {
+	inverseFactor := math.Ldexp(math.Ln2, -scale)
+	return math.Exp(float64(index) * inverseFactor)
+}