Skip to content

Commit

Permalink
Utilize the new slices package in sdk/metric (#4982)
Browse files Browse the repository at this point in the history
  • Loading branch information
MrAlias authored Feb 27, 2024
1 parent 561714a commit b302227
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 24 deletions.
5 changes: 2 additions & 3 deletions sdk/metric/aggregation.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package metric // import "go.opentelemetry.io/otel/sdk/metric"
import (
"errors"
"fmt"
"slices"
)

// errAgg is wrapped by misconfigured aggregations.
Expand Down Expand Up @@ -141,10 +142,8 @@ func (h AggregationExplicitBucketHistogram) err() error {

// copy returns a deep copy of h.
func (h AggregationExplicitBucketHistogram) copy() Aggregation {
b := make([]float64, len(h.Boundaries))
copy(b, h.Boundaries)
return AggregationExplicitBucketHistogram{
Boundaries: b,
Boundaries: slices.Clone(h.Boundaries),
NoMinMax: h.NoMinMax,
}
}
Expand Down
4 changes: 2 additions & 2 deletions sdk/metric/exemplar.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package metric // import "go.opentelemetry.io/otel/sdk/metric"
import (
"os"
"runtime"
"slices"

"go.opentelemetry.io/otel/sdk/metric/internal/exemplar"
"go.opentelemetry.io/otel/sdk/metric/internal/x"
Expand All @@ -40,8 +41,7 @@ func reservoirFunc[N int64 | float64](agg Aggregation) func() exemplar.Reservoir
// use AlignedHistogramBucketExemplarReservoir.
a, ok := agg.(AggregationExplicitBucketHistogram)
if ok && len(a.Boundaries) > 0 {
cp := make([]float64, len(a.Boundaries))
copy(cp, a.Boundaries)
cp := slices.Clone(a.Boundaries)
return func() exemplar.Reservoir[N] {
bounds := cp
return exemplar.Histogram[N](bounds)
Expand Down
28 changes: 12 additions & 16 deletions sdk/metric/internal/aggregate/histogram.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package aggregate // import "go.opentelemetry.io/otel/sdk/metric/internal/aggreg

import (
"context"
"slices"
"sort"
"sync"
"time"
Expand Down Expand Up @@ -68,9 +69,8 @@ func newHistValues[N int64 | float64](bounds []float64, noSum bool, limit int, r
// passed boundaries is ultimately this type's responsibility. Make a copy
// here so we can always guarantee this. Or, in the case of failure, have
// complete control over the fix.
b := make([]float64, len(bounds))
copy(b, bounds)
sort.Float64s(b)
b := slices.Clone(bounds)
slices.Sort(b)
return &histValues[N]{
noSum: noSum,
bounds: b,
Expand Down Expand Up @@ -150,8 +150,7 @@ func (s *histogram[N]) delta(dest *metricdata.Aggregation) int {
defer s.valuesMu.Unlock()

// Do not allow modification of our copy of bounds.
bounds := make([]float64, len(s.bounds))
copy(bounds, s.bounds)
bounds := slices.Clone(s.bounds)

n := len(s.values)
hDPts := reset(h.DataPoints, n, n)
Expand Down Expand Up @@ -201,28 +200,25 @@ func (s *histogram[N]) cumulative(dest *metricdata.Aggregation) int {
defer s.valuesMu.Unlock()

// Do not allow modification of our copy of bounds.
bounds := make([]float64, len(s.bounds))
copy(bounds, s.bounds)
bounds := slices.Clone(s.bounds)

n := len(s.values)
hDPts := reset(h.DataPoints, n, n)

var i int
for a, b := range s.values {
// The HistogramDataPoint field values returned need to be copies of
// the buckets value as we will keep updating them.
//
// TODO (#3047): Making copies for bounds and counts incurs a large
// memory allocation footprint. Alternatives should be explored.
counts := make([]uint64, len(b.counts))
copy(counts, b.counts)

hDPts[i].Attributes = a
hDPts[i].StartTime = s.start
hDPts[i].Time = t
hDPts[i].Count = b.count
hDPts[i].Bounds = bounds
hDPts[i].BucketCounts = counts

// The HistogramDataPoint field values returned need to be copies of
// the buckets value as we will keep updating them.
//
// TODO (#3047): Making copies for bounds and counts incurs a large
// memory allocation footprint. Alternatives should be explored.
hDPts[i].BucketCounts = slices.Clone(b.counts)

if !s.noSum {
hDPts[i].Sum = b.total
Expand Down
3 changes: 2 additions & 1 deletion sdk/metric/internal/exemplar/hist.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package exemplar // import "go.opentelemetry.io/otel/sdk/metric/internal/exempla

import (
"context"
"slices"
"sort"
"time"

Expand All @@ -28,7 +29,7 @@ import (
//
// The passed bounds will be sorted by this function.
func Histogram[N int64 | float64](bounds []float64) Reservoir[N] {
sort.Float64s(bounds)
slices.Sort(bounds)
return &histRes[N]{
bounds: bounds,
storage: newStorage[N](len(bounds) + 1),
Expand Down
4 changes: 2 additions & 2 deletions sdk/metric/internal/exemplar/rand_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ package exemplar
import (
"context"
"math"
"sort"
"slices"
"testing"

"github.com/stretchr/testify/assert"
Expand All @@ -43,7 +43,7 @@ func TestFixedSizeSamplingCorrectness(t *testing.T) {
data[i] = (-1.0 / intensity) * math.Log(random())
}
// Sort to test position bias.
sort.Float64s(data)
slices.Sort(data)

r := FixedSize[float64](sampleSize)
for _, value := range data {
Expand Down

0 comments on commit b302227

Please sign in to comment.