Skip to content

Commit

Permalink
Make getBin and scaleChange methods of expoHistogramDataPoint (#4451)
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
MrAlias and MadVikingGod committed Aug 17, 2023
1 parent 9d9b71f commit 9b47674
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 21 deletions.
24 changes: 12 additions & 12 deletions sdk/metric/internal/aggregate/exponential_histogram.go
Expand Up @@ -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 {
Expand All @@ -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.
Expand All @@ -123,27 +123,26 @@ 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 {
// If v is an exact power of two the frac will be .5 and the exp
// 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
Expand Down Expand Up @@ -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
Expand All @@ -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++
Expand Down
19 changes: 10 additions & 9 deletions sdk/metric/internal/aggregate/exponential_histogram_test.go
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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))
}
})
}
Expand Down

0 comments on commit 9b47674

Please sign in to comment.