Skip to content

Commit fe9bab5

Browse files
MrAliasMrAlias
authored
Use Distinct instead of Set for map keys (open-telemetry#5027)
1 parent da2949b commit fe9bab5

12 files changed

+196
-143
lines changed

‎CHANGELOG.md

+4
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
88

99
## [Unreleased]
1010

11+
### Fixed
12+
13+
- Clarify the documentation about equivalence guarantees for the `Set` and `Distinct` types in `go.opentelemetry.io/otel/attribute`. (#5027)
14+
1115
### Removed
1216

1317
- Drop support for [Go 1.20]. (#4967)

‎attribute/set.go

+14-5
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,24 @@ type (
1515
// immutable set of attributes, with an internal cache for storing
1616
// attribute encodings.
1717
//
18-
// This type supports the Equivalent method of comparison using values of
19-
// type Distinct.
18+
// This type will remain comparable for backwards compatibility. The
19+
// equivalence of Sets across versions is not guaranteed to be stable.
20+
// Prior versions may find two Sets to be equal or not when compared
21+
// directly (i.e. ==), but subsequent versions may not. Users should use
22+
// the Equals method to ensure stable equivalence checking.
23+
//
24+
// Users should also use the Distinct returned from Equivalent as a map key
25+
// instead of a Set directly. In addition to that type providing guarantees
26+
// on stable equivalence, it may also provide performance improvements.
2027
Set struct {
2128
equivalent Distinct
2229
}
2330

24-
// Distinct wraps a variable-size array of KeyValue, constructed with keys
25-
// in sorted order. This can be used as a map key or for equality checking
26-
// between Sets.
31+
// Distinct is a unique identifier of a Set.
32+
//
33+
// Distinct is designed to be ensures equivalence stability: comparisons
34+
// will return the save value across versions. For this reason, Distinct
35+
// should always be used as a map key instead of a Set.
2736
Distinct struct {
2837
iface interface{}
2938
}

‎attribute/value_test.go

+34-7
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ func TestValue(t *testing.T) {
9595
}
9696
}
9797

98-
func TestSetComparability(t *testing.T) {
98+
func TestEquivalence(t *testing.T) {
9999
pairs := [][2]attribute.KeyValue{
100100
{
101101
attribute.Bool("Bool", true),
@@ -139,12 +139,39 @@ func TestSetComparability(t *testing.T) {
139139
},
140140
}
141141

142-
for _, p := range pairs {
143-
s0, s1 := attribute.NewSet(p[0]), attribute.NewSet(p[1])
144-
m := map[attribute.Set]struct{}{s0: {}}
145-
_, ok := m[s1]
146-
assert.Truef(t, ok, "%s not comparable", p[0].Value.Type())
147-
}
142+
t.Run("Distinct", func(t *testing.T) {
143+
for _, p := range pairs {
144+
s0, s1 := attribute.NewSet(p[0]), attribute.NewSet(p[1])
145+
m := map[attribute.Distinct]struct{}{s0.Equivalent(): {}}
146+
_, ok := m[s1.Equivalent()]
147+
assert.Truef(t, ok, "Distinct comparison of %s type: not equivalent", p[0].Value.Type())
148+
assert.Truef(
149+
t,
150+
ok,
151+
"Distinct comparison of %s type: not equivalent: %s != %s",
152+
p[0].Value.Type(),
153+
s0.Encoded(attribute.DefaultEncoder()),
154+
s1.Encoded(attribute.DefaultEncoder()),
155+
)
156+
}
157+
})
158+
159+
t.Run("Set", func(t *testing.T) {
160+
// Maintain backwards compatibility.
161+
for _, p := range pairs {
162+
s0, s1 := attribute.NewSet(p[0]), attribute.NewSet(p[1])
163+
m := map[attribute.Set]struct{}{s0: {}}
164+
_, ok := m[s1]
165+
assert.Truef(
166+
t,
167+
ok,
168+
"Set comparison of %s type: not equivalent: %s != %s",
169+
p[0].Value.Type(),
170+
s0.Encoded(attribute.DefaultEncoder()),
171+
s1.Encoded(attribute.DefaultEncoder()),
172+
)
173+
}
174+
})
148175
}
149176

150177
func TestAsSlice(t *testing.T) {

‎sdk/metric/internal/aggregate/exponential_histogram.go

+39-37
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,8 @@ const (
3030

3131
// expoHistogramDataPoint is a single data point in an exponential histogram.
3232
type expoHistogramDataPoint[N int64 | float64] struct {
33-
res exemplar.Reservoir[N]
33+
attrs attribute.Set
34+
res exemplar.Reservoir[N]
3435

3536
count uint64
3637
min N
@@ -48,7 +49,7 @@ type expoHistogramDataPoint[N int64 | float64] struct {
4849
zeroCount uint64
4950
}
5051

51-
func newExpoHistogramDataPoint[N int64 | float64](maxSize, maxScale int, noMinMax, noSum bool) *expoHistogramDataPoint[N] {
52+
func newExpoHistogramDataPoint[N int64 | float64](attrs attribute.Set, maxSize, maxScale int, noMinMax, noSum bool) *expoHistogramDataPoint[N] {
5253
f := math.MaxFloat64
5354
max := N(f) // if N is int64, max will overflow to -9223372036854775808
5455
min := N(-f)
@@ -57,6 +58,7 @@ func newExpoHistogramDataPoint[N int64 | float64](maxSize, maxScale int, noMinMa
5758
min = N(minInt64)
5859
}
5960
return &expoHistogramDataPoint[N]{
61+
attrs: attrs,
6062
min: max,
6163
max: min,
6264
maxSize: maxSize,
@@ -289,7 +291,7 @@ func newExponentialHistogram[N int64 | float64](maxSize, maxScale int32, noMinMa
289291

290292
newRes: r,
291293
limit: newLimiter[*expoHistogramDataPoint[N]](limit),
292-
values: make(map[attribute.Set]*expoHistogramDataPoint[N]),
294+
values: make(map[attribute.Distinct]*expoHistogramDataPoint[N]),
293295

294296
start: now(),
295297
}
@@ -305,7 +307,7 @@ type expoHistogram[N int64 | float64] struct {
305307

306308
newRes func() exemplar.Reservoir[N]
307309
limit limiter[*expoHistogramDataPoint[N]]
308-
values map[attribute.Set]*expoHistogramDataPoint[N]
310+
values map[attribute.Distinct]*expoHistogramDataPoint[N]
309311
valuesMu sync.Mutex
310312

311313
start time.Time
@@ -323,12 +325,12 @@ func (e *expoHistogram[N]) measure(ctx context.Context, value N, fltrAttr attrib
323325
defer e.valuesMu.Unlock()
324326

325327
attr := e.limit.Attributes(fltrAttr, e.values)
326-
v, ok := e.values[attr]
328+
v, ok := e.values[attr.Equivalent()]
327329
if !ok {
328-
v = newExpoHistogramDataPoint[N](e.maxSize, e.maxScale, e.noMinMax, e.noSum)
330+
v = newExpoHistogramDataPoint[N](attr, e.maxSize, e.maxScale, e.noMinMax, e.noSum)
329331
v.res = e.newRes()
330332

331-
e.values[attr] = v
333+
e.values[attr.Equivalent()] = v
332334
}
333335
v.record(value)
334336
v.res.Offer(ctx, t, value, droppedAttr)
@@ -349,32 +351,32 @@ func (e *expoHistogram[N]) delta(dest *metricdata.Aggregation) int {
349351
hDPts := reset(h.DataPoints, n, n)
350352

351353
var i int
352-
for a, b := range e.values {
353-
hDPts[i].Attributes = a
354+
for _, val := range e.values {
355+
hDPts[i].Attributes = val.attrs
354356
hDPts[i].StartTime = e.start
355357
hDPts[i].Time = t
356-
hDPts[i].Count = b.count
357-
hDPts[i].Scale = int32(b.scale)
358-
hDPts[i].ZeroCount = b.zeroCount
358+
hDPts[i].Count = val.count
359+
hDPts[i].Scale = int32(val.scale)
360+
hDPts[i].ZeroCount = val.zeroCount
359361
hDPts[i].ZeroThreshold = 0.0
360362

361-
hDPts[i].PositiveBucket.Offset = int32(b.posBuckets.startBin)
362-
hDPts[i].PositiveBucket.Counts = reset(hDPts[i].PositiveBucket.Counts, len(b.posBuckets.counts), len(b.posBuckets.counts))
363-
copy(hDPts[i].PositiveBucket.Counts, b.posBuckets.counts)
363+
hDPts[i].PositiveBucket.Offset = int32(val.posBuckets.startBin)
364+
hDPts[i].PositiveBucket.Counts = reset(hDPts[i].PositiveBucket.Counts, len(val.posBuckets.counts), len(val.posBuckets.counts))
365+
copy(hDPts[i].PositiveBucket.Counts, val.posBuckets.counts)
364366

365-
hDPts[i].NegativeBucket.Offset = int32(b.negBuckets.startBin)
366-
hDPts[i].NegativeBucket.Counts = reset(hDPts[i].NegativeBucket.Counts, len(b.negBuckets.counts), len(b.negBuckets.counts))
367-
copy(hDPts[i].NegativeBucket.Counts, b.negBuckets.counts)
367+
hDPts[i].NegativeBucket.Offset = int32(val.negBuckets.startBin)
368+
hDPts[i].NegativeBucket.Counts = reset(hDPts[i].NegativeBucket.Counts, len(val.negBuckets.counts), len(val.negBuckets.counts))
369+
copy(hDPts[i].NegativeBucket.Counts, val.negBuckets.counts)
368370

369371
if !e.noSum {
370-
hDPts[i].Sum = b.sum
372+
hDPts[i].Sum = val.sum
371373
}
372374
if !e.noMinMax {
373-
hDPts[i].Min = metricdata.NewExtrema(b.min)
374-
hDPts[i].Max = metricdata.NewExtrema(b.max)
375+
hDPts[i].Min = metricdata.NewExtrema(val.min)
376+
hDPts[i].Max = metricdata.NewExtrema(val.max)
375377
}
376378

377-
b.res.Collect(&hDPts[i].Exemplars)
379+
val.res.Collect(&hDPts[i].Exemplars)
378380

379381
i++
380382
}
@@ -402,32 +404,32 @@ func (e *expoHistogram[N]) cumulative(dest *metricdata.Aggregation) int {
402404
hDPts := reset(h.DataPoints, n, n)
403405

404406
var i int
405-
for a, b := range e.values {
406-
hDPts[i].Attributes = a
407+
for _, val := range e.values {
408+
hDPts[i].Attributes = val.attrs
407409
hDPts[i].StartTime = e.start
408410
hDPts[i].Time = t
409-
hDPts[i].Count = b.count
410-
hDPts[i].Scale = int32(b.scale)
411-
hDPts[i].ZeroCount = b.zeroCount
411+
hDPts[i].Count = val.count
412+
hDPts[i].Scale = int32(val.scale)
413+
hDPts[i].ZeroCount = val.zeroCount
412414
hDPts[i].ZeroThreshold = 0.0
413415

414-
hDPts[i].PositiveBucket.Offset = int32(b.posBuckets.startBin)
415-
hDPts[i].PositiveBucket.Counts = reset(hDPts[i].PositiveBucket.Counts, len(b.posBuckets.counts), len(b.posBuckets.counts))
416-
copy(hDPts[i].PositiveBucket.Counts, b.posBuckets.counts)
416+
hDPts[i].PositiveBucket.Offset = int32(val.posBuckets.startBin)
417+
hDPts[i].PositiveBucket.Counts = reset(hDPts[i].PositiveBucket.Counts, len(val.posBuckets.counts), len(val.posBuckets.counts))
418+
copy(hDPts[i].PositiveBucket.Counts, val.posBuckets.counts)
417419

418-
hDPts[i].NegativeBucket.Offset = int32(b.negBuckets.startBin)
419-
hDPts[i].NegativeBucket.Counts = reset(hDPts[i].NegativeBucket.Counts, len(b.negBuckets.counts), len(b.negBuckets.counts))
420-
copy(hDPts[i].NegativeBucket.Counts, b.negBuckets.counts)
420+
hDPts[i].NegativeBucket.Offset = int32(val.negBuckets.startBin)
421+
hDPts[i].NegativeBucket.Counts = reset(hDPts[i].NegativeBucket.Counts, len(val.negBuckets.counts), len(val.negBuckets.counts))
422+
copy(hDPts[i].NegativeBucket.Counts, val.negBuckets.counts)
421423

422424
if !e.noSum {
423-
hDPts[i].Sum = b.sum
425+
hDPts[i].Sum = val.sum
424426
}
425427
if !e.noMinMax {
426-
hDPts[i].Min = metricdata.NewExtrema(b.min)
427-
hDPts[i].Max = metricdata.NewExtrema(b.max)
428+
hDPts[i].Min = metricdata.NewExtrema(val.min)
429+
hDPts[i].Max = metricdata.NewExtrema(val.max)
428430
}
429431

430-
b.res.Collect(&hDPts[i].Exemplars)
432+
val.res.Collect(&hDPts[i].Exemplars)
431433

432434
i++
433435
// TODO (#3006): This will use an unbounded amount of memory if there

‎sdk/metric/internal/aggregate/exponential_histogram_test.go

+13-12
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ func testExpoHistogramDataPointRecord[N int64 | float64](t *testing.T) {
130130
restore := withHandler(t)
131131
defer restore()
132132

133-
dp := newExpoHistogramDataPoint[N](tt.maxSize, 20, false, false)
133+
dp := newExpoHistogramDataPoint[N](alice, tt.maxSize, 20, false, false)
134134
for _, v := range tt.values {
135135
dp.record(v)
136136
dp.record(-v)
@@ -176,7 +176,7 @@ func testExpoHistogramMinMaxSumInt64(t *testing.T) {
176176
for _, v := range tt.values {
177177
h.measure(context.Background(), v, alice, nil)
178178
}
179-
dp := h.values[alice]
179+
dp := h.values[alice.Equivalent()]
180180

181181
assert.Equal(t, tt.expected.max, dp.max)
182182
assert.Equal(t, tt.expected.min, dp.min)
@@ -218,7 +218,7 @@ func testExpoHistogramMinMaxSumFloat64(t *testing.T) {
218218
for _, v := range tt.values {
219219
h.measure(context.Background(), v, alice, nil)
220220
}
221-
dp := h.values[alice]
221+
dp := h.values[alice.Equivalent()]
222222

223223
assert.Equal(t, tt.expected.max, dp.max)
224224
assert.Equal(t, tt.expected.min, dp.min)
@@ -305,7 +305,7 @@ func testExpoHistogramDataPointRecordFloat64(t *testing.T) {
305305
restore := withHandler(t)
306306
defer restore()
307307

308-
dp := newExpoHistogramDataPoint[float64](tt.maxSize, 20, false, false)
308+
dp := newExpoHistogramDataPoint[float64](alice, tt.maxSize, 20, false, false)
309309
for _, v := range tt.values {
310310
dp.record(v)
311311
dp.record(-v)
@@ -322,21 +322,21 @@ func TestExponentialHistogramDataPointRecordLimits(t *testing.T) {
322322
// These bins are calculated from the following formula:
323323
// floor( log2( value) * 2^20 ) using an arbitrary precision calculator.
324324

325-
fdp := newExpoHistogramDataPoint[float64](4, 20, false, false)
325+
fdp := newExpoHistogramDataPoint[float64](alice, 4, 20, false, false)
326326
fdp.record(math.MaxFloat64)
327327

328328
if fdp.posBuckets.startBin != 1073741823 {
329329
t.Errorf("Expected startBin to be 1073741823, got %d", fdp.posBuckets.startBin)
330330
}
331331

332-
fdp = newExpoHistogramDataPoint[float64](4, 20, false, false)
332+
fdp = newExpoHistogramDataPoint[float64](alice, 4, 20, false, false)
333333
fdp.record(math.SmallestNonzeroFloat64)
334334

335335
if fdp.posBuckets.startBin != -1126170625 {
336336
t.Errorf("Expected startBin to be -1126170625, got %d", fdp.posBuckets.startBin)
337337
}
338338

339-
idp := newExpoHistogramDataPoint[int64](4, 20, false, false)
339+
idp := newExpoHistogramDataPoint[int64](alice, 4, 20, false, false)
340340
idp.record(math.MaxInt64)
341341

342342
if idp.posBuckets.startBin != 66060287 {
@@ -641,7 +641,7 @@ func TestScaleChange(t *testing.T) {
641641
}
642642
for _, tt := range tests {
643643
t.Run(tt.name, func(t *testing.T) {
644-
p := newExpoHistogramDataPoint[float64](tt.args.maxSize, 20, false, false)
644+
p := newExpoHistogramDataPoint[float64](alice, tt.args.maxSize, 20, false, false)
645645
got := p.scaleChange(tt.args.bin, tt.args.startBin, tt.args.length)
646646
if got != tt.want {
647647
t.Errorf("scaleChange() = %v, want %v", got, tt.want)
@@ -652,7 +652,7 @@ func TestScaleChange(t *testing.T) {
652652

653653
func BenchmarkPrepend(b *testing.B) {
654654
for i := 0; i < b.N; i++ {
655-
agg := newExpoHistogramDataPoint[float64](1024, 20, false, false)
655+
agg := newExpoHistogramDataPoint[float64](alice, 1024, 20, false, false)
656656
n := math.MaxFloat64
657657
for j := 0; j < 1024; j++ {
658658
agg.record(n)
@@ -663,7 +663,7 @@ func BenchmarkPrepend(b *testing.B) {
663663

664664
func BenchmarkAppend(b *testing.B) {
665665
for i := 0; i < b.N; i++ {
666-
agg := newExpoHistogramDataPoint[float64](1024, 20, false, false)
666+
agg := newExpoHistogramDataPoint[float64](alice, 1024, 20, false, false)
667667
n := smallestNonZeroNormalFloat64
668668
for j := 0; j < 1024; j++ {
669669
agg.record(n)
@@ -704,6 +704,7 @@ func BenchmarkExponentialHistogram(b *testing.B) {
704704

705705
func TestSubNormal(t *testing.T) {
706706
want := &expoHistogramDataPoint[float64]{
707+
attrs: alice,
707708
maxSize: 4,
708709
count: 3,
709710
min: math.SmallestNonzeroFloat64,
@@ -717,7 +718,7 @@ func TestSubNormal(t *testing.T) {
717718
},
718719
}
719720

720-
ehdp := newExpoHistogramDataPoint[float64](4, 20, false, false)
721+
ehdp := newExpoHistogramDataPoint[float64](alice, 4, 20, false, false)
721722
ehdp.record(math.SmallestNonzeroFloat64)
722723
ehdp.record(math.SmallestNonzeroFloat64)
723724
ehdp.record(math.SmallestNonzeroFloat64)
@@ -1060,7 +1061,7 @@ func FuzzGetBin(f *testing.F) {
10601061
t.Skip("skipping test for zero")
10611062
}
10621063

1063-
p := newExpoHistogramDataPoint[float64](4, 20, false, false)
1064+
p := newExpoHistogramDataPoint[float64](alice, 4, 20, false, false)
10641065
// scale range is -10 to 20.
10651066
p.scale = (scale%31+31)%31 - 10
10661067
got := p.getBin(v)

0 commit comments

Comments
 (0)