Deprecate Sortable (#4734)

* Deprecate Sortable * Remove redundant checks * Move code to a non-deprecated func * Apply suggestions from code review Co-authored-by: Tyler Yahn <[email protected]> * Mention individual deprecated function * Update attribute/set.go Co-authored-by: Robert Pająk <[email protected]> * Add BenchmarkNewSet --------- Co-authored-by: Tyler Yahn <[email protected]> Co-authored-by: Robert Pająk <[email protected]>
open-telemetry · Mar 12, 2024 · 81bbab3 · 81bbab3
1 parent b1e9956
commit 81bbab3
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 63 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -16,6 +16,12 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 - Drop support for [Go 1.20]. (#4967)
 
+### Deprecated
+
+- Deprecate `go.opentelemetry.io/otel/attribute.Sortable` type. (#4734)
+- Deprecate `go.opentelemetry.io/otel/attribute.NewSetWithSortable` function. (#4734)
+- Deprecate `go.opentelemetry.io/otel/attribute.NewSetWithSortableFiltered` function. (#4734)
+
 ## [1.24.0/0.46.0/0.0.1-alpha] 2024-02-23
 
 This release is the last to support [Go 1.20].

diff --git a/attribute/set.go b/attribute/set.go
@@ -4,10 +4,11 @@
 package attribute // import "go.opentelemetry.io/otel/attribute"
 
 import (
+	"cmp"
 	"encoding/json"
 	"reflect"
+	"slices"
 	"sort"
-	"sync"
 )
 
 type (
@@ -37,10 +38,11 @@ type (
 		iface interface{}
 	}
 
-	// Sortable implements sort.Interface, used for sorting KeyValue. This is
-	// an exported type to support a memory optimization. A pointer to one of
-	// these is needed for the call to sort.Stable(), which the caller may
-	// provide in order to avoid an allocation. See NewSetWithSortable().
+	// Sortable implements sort.Interface, used for sorting KeyValue.
+	//
+	// Deprecated: This type is no longer used. It was added as a performance
+	// optimization for Go < 1.21 that is no longer needed (Go < 1.21 is no
+	// longer supported by the module).
 	Sortable []KeyValue
 )
 
@@ -54,12 +56,6 @@ var (
 			iface: [0]KeyValue{},
 		},
 	}
-
-	// sortables is a pool of Sortables used to create Sets with a user does
-	// not provide one.
-	sortables = sync.Pool{
-		New: func() interface{} { return new(Sortable) },
-	}
 )
 
 // EmptySet returns a reference to a Set with no elements.
@@ -185,26 +181,18 @@ func empty() Set {
 // Except for empty sets, this method adds an additional allocation compared
 // with calls that include a Sortable.
 func NewSet(kvs ...KeyValue) Set {
-	// Check for empty set.
-	if len(kvs) == 0 {
-		return empty()
-	}
-	srt := sortables.Get().(*Sortable)
-	s, _ := NewSetWithSortableFiltered(kvs, srt, nil)
-	sortables.Put(srt)
+	s, _ := NewSetWithFiltered(kvs, nil)
 	return s
 }
 
 // NewSetWithSortable returns a new Set. See the documentation for
 // NewSetWithSortableFiltered for more details.
 //
 // This call includes a Sortable option as a memory optimization.
-func NewSetWithSortable(kvs []KeyValue, tmp *Sortable) Set {
-	// Check for empty set.
-	if len(kvs) == 0 {
-		return empty()
-	}
-	s, _ := NewSetWithSortableFiltered(kvs, tmp, nil)
+//
+// Deprecated: Use [NewSet] instead.
+func NewSetWithSortable(kvs []KeyValue, _ *Sortable) Set {
+	s, _ := NewSetWithFiltered(kvs, nil)
 	return s
 }
 
@@ -218,48 +206,12 @@ func NewSetWithFiltered(kvs []KeyValue, filter Filter) (Set, []KeyValue) {
 	if len(kvs) == 0 {
 		return empty(), nil
 	}
-	srt := sortables.Get().(*Sortable)
-	s, filtered := NewSetWithSortableFiltered(kvs, srt, filter)
-	sortables.Put(srt)
-	return s, filtered
-}
-
-// NewSetWithSortableFiltered returns a new Set.
-//
-// Duplicate keys are eliminated by taking the last value.  This
-// re-orders the input slice so that unique last-values are contiguous
-// at the end of the slice.
-//
-// This ensures the following:
-//
-// - Last-value-wins semantics
-// - Caller sees the reordering, but doesn't lose values
-// - Repeated call preserve last-value wins.
-//
-// Note that methods are defined on Set, although this returns Set. Callers
-// can avoid memory allocations by:
-//
-// - allocating a Sortable for use as a temporary in this method
-// - allocating a Set for storing the return value of this constructor.
-//
-// The result maintains a cache of encoded attributes, by attribute.EncoderID.
-// This value should not be copied after its first use.
-//
-// The second []KeyValue return value is a list of attributes that were
-// excluded by the Filter (if non-nil).
-func NewSetWithSortableFiltered(kvs []KeyValue, tmp *Sortable, filter Filter) (Set, []KeyValue) {
-	// Check for empty set.
-	if len(kvs) == 0 {
-		return empty(), nil
-	}
-
-	*tmp = kvs
 
 	// Stable sort so the following de-duplication can implement
 	// last-value-wins semantics.
-	sort.Stable(tmp)
-
-	*tmp = nil
+	slices.SortStableFunc(kvs, func(a, b KeyValue) int {
+		return cmp.Compare(a.Key, b.Key)
+	})
 
 	position := len(kvs) - 1
 	offset := position - 1
@@ -287,6 +239,35 @@ func NewSetWithSortableFiltered(kvs []KeyValue, tmp *Sortable, filter Filter) (S
 	return Set{equivalent: computeDistinct(kvs)}, nil
 }
 
+// NewSetWithSortableFiltered returns a new Set.
+//
+// Duplicate keys are eliminated by taking the last value.  This
+// re-orders the input slice so that unique last-values are contiguous
+// at the end of the slice.
+//
+// This ensures the following:
+//
+// - Last-value-wins semantics
+// - Caller sees the reordering, but doesn't lose values
+// - Repeated call preserve last-value wins.
+//
+// Note that methods are defined on Set, although this returns Set. Callers
+// can avoid memory allocations by:
+//
+// - allocating a Sortable for use as a temporary in this method
+// - allocating a Set for storing the return value of this constructor.
+//
+// The result maintains a cache of encoded attributes, by attribute.EncoderID.
+// This value should not be copied after its first use.
+//
+// The second []KeyValue return value is a list of attributes that were
+// excluded by the Filter (if non-nil).
+//
+// Deprecated: Use [NewSetWithFiltered] instead.
+func NewSetWithSortableFiltered(kvs []KeyValue, _ *Sortable, filter Filter) (Set, []KeyValue) {
+	return NewSetWithFiltered(kvs, filter)
+}
+
 // filteredToFront filters slice in-place using keep function. All KeyValues that need to
 // be removed are moved to the front. All KeyValues that need to be kept are
 // moved (in-order) to the back. The index for the first KeyValue to be kept is

diff --git a/attribute/set_test.go b/attribute/set_test.go
@@ -356,3 +356,22 @@ func BenchmarkFiltering(b *testing.B) {
 	b.Run("Filtered", benchFn(func(kv attribute.KeyValue) bool { return kv.Key == "A" }))
 	b.Run("AllDropped", benchFn(func(attribute.KeyValue) bool { return false }))
 }
+
+var sinkSet attribute.Set
+
+func BenchmarkNewSet(b *testing.B) {
+	attrs := []attribute.KeyValue{
+		attribute.String("B1", "2"),
+		attribute.String("C2", "5"),
+		attribute.String("B3", "2"),
+		attribute.String("C4", "1"),
+		attribute.String("A5", "4"),
+		attribute.String("C6", "3"),
+		attribute.String("A7", "1"),
+	}
+	b.ReportAllocs()
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		sinkSet = attribute.NewSet(attrs...)
+	}
+}