Change the inclusivity of exponential histogram bounds (#2982)

* Use lower-inclusive boundaries

* make exponent and logarithm more symmetric

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Co-authored-by: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com>

CHANGELOG.md

@@ -13,9 +13,12 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Support Go 1.19.
   Include compatibility testing and document support. (#3077)
 
-### Fixed
+### Changed
 
 - Fix misidentification of OpenTelemetry `SpanKind` in OpenTracing bridge (`go.opentelemetry.io/otel/bridge/opentracing`).  (#3096)
+- The exponential histogram mapping functions have been updated with
+  exact upper-inclusive boundary support following the [corresponding
+  specification change](https://github.com/open-telemetry/opentelemetry-specification/pull/2633). (#2982)
 
 ## [1.9.0/0.0.3] - 2022-08-01
 

sdk/metric/aggregator/exponential/benchmark_test.go

@@ -0,0 +1,67 @@
+// Copyright The OpenTelemetry Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//       http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package exponential
+
+import (
+	"fmt"
+	"math/rand"
+	"testing"
+
+	"go.opentelemetry.io/otel/sdk/metric/aggregator/exponential/mapping"
+	"go.opentelemetry.io/otel/sdk/metric/aggregator/exponential/mapping/exponent"
+	"go.opentelemetry.io/otel/sdk/metric/aggregator/exponential/mapping/logarithm"
+)
+
+func benchmarkMapping(b *testing.B, name string, mapper mapping.Mapping) {
+	b.Run(fmt.Sprintf("mapping_%s", name), func(b *testing.B) {
+		src := rand.New(rand.NewSource(54979))
+		for i := 0; i < b.N; i++ {
+			_ = mapper.MapToIndex(1 + src.Float64())
+		}
+	})
+}
+
+func benchmarkBoundary(b *testing.B, name string, mapper mapping.Mapping) {
+	b.Run(fmt.Sprintf("boundary_%s", name), func(b *testing.B) {
+		src := rand.New(rand.NewSource(54979))
+		for i := 0; i < b.N; i++ {
+			_, _ = mapper.LowerBoundary(int32(src.Int63()))
+		}
+	})
+}
+
+// An earlier draft of this benchmark included a lookup-table based
+// implementation:
+// https://github.com/open-telemetry/opentelemetry-go-contrib/pull/1353
+// That mapping function uses O(2^scale) extra space and falls
+// somewhere between the exponent and logarithm methods compared here.
+// In the test, lookuptable was 40% faster than logarithm, which did
+// not justify the significant extra complexity.
+
+// Benchmarks the MapToIndex function.
+func BenchmarkMapping(b *testing.B) {
+	em, _ := exponent.NewMapping(-1)
+	lm, _ := logarithm.NewMapping(1)
+	benchmarkMapping(b, "exponent", em)
+	benchmarkMapping(b, "logarithm", lm)
+}
+
+// Benchmarks the LowerBoundary function.
+func BenchmarkReverseMapping(b *testing.B) {
+	em, _ := exponent.NewMapping(-1)
+	lm, _ := logarithm.NewMapping(1)
+	benchmarkBoundary(b, "exponent", em)
+	benchmarkBoundary(b, "logarithm", lm)
+}

sdk/metric/aggregator/exponential/mapping/exponent/exponent.go

@@ -19,6 +19,7 @@ import (
 	"math"
 
 	"go.opentelemetry.io/otel/sdk/metric/aggregator/exponential/mapping"
+	"go.opentelemetry.io/otel/sdk/metric/aggregator/exponential/mapping/internal"
 )
 
 const (
@@ -63,52 +64,61 @@ func NewMapping(scale int32) (mapping.Mapping, error) {
 	return &prebuiltMappings[scale-MinScale], nil
 }
 
+// minNormalLowerBoundaryIndex is the largest index such that
+// base**index is <= MinValue.  A histogram bucket with this index
+// covers the range (base**index, base**(index+1)], including
+// MinValue.
+func (e *exponentMapping) minNormalLowerBoundaryIndex() int32 {
+	idx := int32(internal.MinNormalExponent) >> e.shift
+	if e.shift < 2 {
+		// For scales -1 and 0 the minimum value 2**-1022
+		// is a power-of-two multiple, meaning it belongs
+		// to the index one less.
+		idx--
+	}
+	return idx
+}
+
+// maxNormalLowerBoundaryIndex is the index such that base**index
+// equals the largest representable boundary.  A histogram bucket with this
+// index covers the range (0x1p+1024/base, 0x1p+1024], which includes
+// MaxValue; note that this bucket is incomplete, since the upper
+// boundary cannot be represented.  One greater than this index
+// corresponds with the bucket containing values > 0x1p1024.
+func (e *exponentMapping) maxNormalLowerBoundaryIndex() int32 {
+	return int32(internal.MaxNormalExponent) >> e.shift
+}
+
 // MapToIndex implements mapping.Mapping.
 func (e *exponentMapping) MapToIndex(value float64) int32 {
 	// Note: we can assume not a 0, Inf, or NaN; positive sign bit.
+	if value < internal.MinValue {
+		return e.minNormalLowerBoundaryIndex()
+	}
 
-	// Note: bit-shifting does the right thing for negative
-	// exponents, e.g., -1 >> 1 == -1.
-	return getBase2(value) >> e.shift
-}
+	// Extract the raw exponent.
+	rawExp := internal.GetNormalBase2(value)
 
-func (e *exponentMapping) minIndex() int32 {
-	return int32(MinNormalExponent) >> e.shift
-}
+	// In case the value is an exact power of two, compute a
+	// correction of -1:
+	correction := int32((internal.GetSignificand(value) - 1) >> internal.SignificandWidth)
 
-func (e *exponentMapping) maxIndex() int32 {
-	return int32(MaxNormalExponent) >> e.shift
+	// Note: bit-shifting does the right thing for negative
+	// exponents, e.g., -1 >> 1 == -1.
+	return (rawExp + correction) >> e.shift
 }
 
 // LowerBoundary implements mapping.Mapping.
 func (e *exponentMapping) LowerBoundary(index int32) (float64, error) {
-	if min := e.minIndex(); index < min {
+	if min := e.minNormalLowerBoundaryIndex(); index < min {
 		return 0, mapping.ErrUnderflow
 	}
 
-	if max := e.maxIndex(); index > max {
+	if max := e.maxNormalLowerBoundaryIndex(); index > max {
 		return 0, mapping.ErrOverflow
 	}
 
-	unbiased := int64(index << e.shift)
-
-	// Note: although the mapping function rounds subnormal values
-	// up to the smallest normal value, there are still buckets
-	// that may be filled that start at subnormal values.  The
-	// following code handles this correctly.  It's equivalent to and
-	// faster than math.Ldexp(1, int(unbiased)).
-	if unbiased < int64(MinNormalExponent) {
-		subnormal := uint64(1 << SignificandWidth)
-		for unbiased < int64(MinNormalExponent) {
-			unbiased++
-			subnormal >>= 1
-		}
-		return math.Float64frombits(subnormal), nil
-	}
-	exponent := unbiased + ExponentBias
-
-	bits := uint64(exponent << SignificandWidth)
-	return math.Float64frombits(bits), nil
+	return math.Ldexp(1, int(index<<e.shift)), nil
 }
 
 // Scale implements mapping.Mapping.