From 9b47674bc5cb6cd9bde01fcaa2a35624634b0399 Mon Sep 17 00:00:00 2001
From: Tyler Yahn <MrAlias@users.noreply.github.com>
Date: Thu, 17 Aug 2023 15:06:46 -0700
Subject: [PATCH] Make getBin and scaleChange methods of expoHistogramDataPoint
 (#4451)

Both functions receive parameters from an expoHistogramDataPoint and are
only ever used by other methods of an expoHistogramDataPoint. Make the
functions methods of expoHistogramDataPoint so the parameter arguments
can be dropped and the functions are scoped to the type they are used
for.

Co-authored-by: Aaron Clawson <3766680+MadVikingGod@users.noreply.github.com>
---
 .../aggregate/exponential_histogram.go        | 24 +++++++++----------
 .../aggregate/exponential_histogram_test.go   | 19 ++++++++-------
 2 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/sdk/metric/internal/aggregate/exponential_histogram.go b/sdk/metric/internal/aggregate/exponential_histogram.go
index 07d9fd6e22c..e9ba04eea5e 100644
--- a/sdk/metric/internal/aggregate/exponential_histogram.go
+++ b/sdk/metric/internal/aggregate/exponential_histogram.go
@@ -102,7 +102,7 @@ func (p *expoHistogramDataPoint[N]) record(v N) {
 		return
 	}
 
-	bin := getBin(absV, p.scale)
+	bin := p.getBin(absV)
 
 	bucket := &p.posBuckets
 	if v < 0 {
@@ -111,7 +111,7 @@ func (p *expoHistogramDataPoint[N]) record(v N) {
 
 	// If the new bin would make the counts larger than maxScale, we need to
 	// downscale current measurements.
-	if scaleDelta := scaleChange(bin, bucket.startBin, len(bucket.counts), p.maxSize); scaleDelta > 0 {
+	if scaleDelta := p.scaleChange(bin, bucket.startBin, len(bucket.counts)); scaleDelta > 0 {
 		if p.scale-scaleDelta < expoMinScale {
 			// With a scale of -10 there is only two buckets for the whole range of float64 values.
 			// This can only happen if there is a max size of 1.
@@ -123,17 +123,16 @@ func (p *expoHistogramDataPoint[N]) record(v N) {
 		p.posBuckets.downscale(scaleDelta)
 		p.negBuckets.downscale(scaleDelta)
 
-		bin = getBin(absV, p.scale)
+		bin = p.getBin(absV)
 	}
 
 	bucket.record(bin)
 }
 
-// getBin returns the bin of the bucket that the value v should be recorded
-// into at the given scale.
-func getBin(v float64, scale int) int {
+// getBin returns the bin v should be recorded into.
+func (p *expoHistogramDataPoint[N]) getBin(v float64) int {
 	frac, exp := math.Frexp(v)
-	if scale <= 0 {
+	if p.scale <= 0 {
 		// Because of the choice of fraction is always 1 power of two higher than we want.
 		correction := 1
 		if frac == .5 {
@@ -141,9 +140,9 @@ func getBin(v float64, scale int) int {
 			// will be one higher than we want.
 			correction = 2
 		}
-		return (exp - correction) >> (-scale)
+		return (exp - correction) >> (-p.scale)
 	}
-	return exp<<scale + int(math.Log(frac)*scaleFactors[scale]) - 1
+	return exp<<p.scale + int(math.Log(frac)*scaleFactors[p.scale]) - 1
 }
 
 // scaleFactors are constants used in calculating the logarithm index. They are
@@ -172,8 +171,9 @@ var scaleFactors = [21]float64{
 	math.Ldexp(math.Log2E, 20),
 }
 
-// scaleChange returns the magnitude of the scale change needed to fit bin in the bucket.
-func scaleChange(bin, startBin, length, maxSize int) int {
+// scaleChange returns the magnitude of the scale change needed to fit bin in
+// the bucket. If no scale change is needed 0 is returned.
+func (p *expoHistogramDataPoint[N]) scaleChange(bin, startBin, length int) int {
 	if length == 0 {
 		// No need to rescale if there are no buckets.
 		return 0
@@ -187,7 +187,7 @@ func scaleChange(bin, startBin, length, maxSize int) int {
 	}
 
 	count := 0
-	for high-low >= maxSize {
+	for high-low >= p.maxSize {
 		low = low >> 1
 		high = high >> 1
 		count++
diff --git a/sdk/metric/internal/aggregate/exponential_histogram_test.go b/sdk/metric/internal/aggregate/exponential_histogram_test.go
index 4209ffd651d..040370d4729 100644
--- a/sdk/metric/internal/aggregate/exponential_histogram_test.go
+++ b/sdk/metric/internal/aggregate/exponential_histogram_test.go
@@ -655,7 +655,8 @@ func TestScaleChange(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := scaleChange(tt.args.bin, tt.args.startBin, tt.args.length, tt.args.maxSize)
+			p := newExpoHistogramDataPoint[float64](tt.args.maxSize, 20, false, false)
+			got := p.scaleChange(tt.args.bin, tt.args.startBin, tt.args.length)
 			if got != tt.want {
 				t.Errorf("scaleChange() = %v, want %v", got, tt.want)
 			}
@@ -927,15 +928,15 @@ func FuzzGetBin(f *testing.F) {
 			t.Skip("skipping test for zero")
 		}
 
-		// GetBin is only used with a range of -10 to 20.
-		scale = (scale%31+31)%31 - 10
-
-		got := getBin(v, scale)
-		if v <= lowerBound(got, scale) {
-			t.Errorf("v=%x scale =%d had bin %d, but was below lower bound %x", v, scale, got, lowerBound(got, scale))
+		p := newExpoHistogramDataPoint[float64](4, 20, false, false)
+		// scale range is -10 to 20.
+		p.scale = (scale%31+31)%31 - 10
+		got := p.getBin(v)
+		if v <= lowerBound(got, p.scale) {
+			t.Errorf("v=%x scale =%d had bin %d, but was below lower bound %x", v, p.scale, got, lowerBound(got, p.scale))
 		}
-		if v > lowerBound(got+1, scale) {
-			t.Errorf("v=%x scale =%d had bin %d, but was above upper bound %x", v, scale, got, lowerBound(got+1, scale))
+		if v > lowerBound(got+1, p.scale) {
+			t.Errorf("v=%x scale =%d had bin %d, but was above upper bound %x", v, p.scale, got, lowerBound(got+1, p.scale))
 		}
 	})
 }