Replace recordingSpan attributes implementation with slice of attributes (#2576)

* Replace recordingSpan attributes implementation

Instead of an LRU strategy for cap-ing span attributes, comply with the
specification and drop last added. To do this, the attributesmap is
replaced with a slice of attributes.

* Remove attributesmap files

* Refine addition algorithm

Unify duplicated code.

Fix deduplication algorithm.

Fix droppedAttributes to always be returned, even if the span has no
attributes.

* Unify span SetAttributes tests

* Doc fix to attr drop order in changelog

* Test span and snapshot attrs

* fix lint

* Add tests for recordingSpan method defaults

* Comment why pre-allocation is not done

* Correct grammar in recordingSpan allocation comment

* Update sdk/trace/tracer.go

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

CHANGELOG.md

@@ -31,6 +31,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Specify explicit buckets in Prometheus example. (#2493)
 - W3C baggage will now decode urlescaped values. (#2529)
 - Baggage members are now only validated once, when calling `NewMember` and not also when adding it to the baggage itself. (#2522)
+- The order attributes are dropped from spans in the `go.opentelemetry.io/otel/sdk/trace` package when capacity is reached is fixed to be in compliance with the OpenTelemetry specification.
+  Instead of dropping the least-recently-used attribute, the last added attribute is dropped.
+  This drop order still only applies to attributes with unique keys not already contained in the span.
+  If an attribute is added with a key already contained in the span, that attribute is updated to the new value being added. (#2576)
 
 ### Removed
 

sdk/trace/attributesmap.go

@@ -1,91 +0,0 @@
-// Copyright The OpenTelemetry Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package trace // import "go.opentelemetry.io/otel/sdk/trace"
-
-import (
-	"container/list"
-
-	"go.opentelemetry.io/otel/attribute"
-)
-
-// attributesMap is a capped map of attributes, holding the most recent attributes.
-// Eviction is done via a LRU method, the oldest entry is removed to create room for a new entry.
-// Updates are allowed and they refresh the usage of the key.
-//
-// This is based from https://github.com/hashicorp/golang-lru/blob/master/simplelru/lru.go
-// With a subset of the its operations and specific for holding attribute.KeyValue
-type attributesMap struct {
-	attributes   map[attribute.Key]*list.Element
-	evictList    *list.List
-	droppedCount int
-	capacity     int
-}
-
-func newAttributesMap(capacity int) *attributesMap {
-	lm := &attributesMap{
-		attributes: make(map[attribute.Key]*list.Element),
-		evictList:  list.New(),
-		capacity:   capacity,
-	}
-	return lm
-}
-
-func (am *attributesMap) add(kv attribute.KeyValue) {
-	// Check for existing item
-	if ent, ok := am.attributes[kv.Key]; ok {
-		am.evictList.MoveToFront(ent)
-		ent.Value = &kv
-		return
-	}
-
-	// Add new item
-	entry := am.evictList.PushFront(&kv)
-	am.attributes[kv.Key] = entry
-
-	// Verify size not exceeded
-	if am.evictList.Len() > am.capacity {
-		am.removeOldest()
-		am.droppedCount++
-	}
-}
-
-// toKeyValue copies the attributesMap into a slice of attribute.KeyValue and
-// returns it. If the map is empty, a nil is returned.
-// TODO: Is it more efficient to return a pointer to the slice?
-func (am *attributesMap) toKeyValue() []attribute.KeyValue {
-	len := am.evictList.Len()
-	if len == 0 {
-		return nil
-	}
-
-	attributes := make([]attribute.KeyValue, 0, len)
-	for ent := am.evictList.Back(); ent != nil; ent = ent.Prev() {
-		if value, ok := ent.Value.(*attribute.KeyValue); ok {
-			attributes = append(attributes, *value)
-		}
-	}
-
-	return attributes
-}
-
-// removeOldest removes the oldest item from the cache.
-func (am *attributesMap) removeOldest() {
-	ent := am.evictList.Back()
-	if ent != nil {
-		am.evictList.Remove(ent)
-		kv := ent.Value.(*attribute.KeyValue)
-		delete(am.attributes, kv.Key)
-	}
-}

sdk/trace/attributesmap_test.go

@@ -1,103 +0,0 @@
-// Copyright The OpenTelemetry Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package trace
-
-import (
-	"fmt"
-	"testing"
-
-	"go.opentelemetry.io/otel/attribute"
-)
-
-const testKeyFmt = "test-key-%d"
-
-func TestAttributesMap(t *testing.T) {
-	wantCapacity := 128
-	attrMap := newAttributesMap(wantCapacity)
-
-	for i := 0; i < 256; i++ {
-		attrMap.add(attribute.Int(fmt.Sprintf(testKeyFmt, i), i))
-	}
-	if attrMap.capacity != wantCapacity {
-		t.Errorf("attrMap.capacity: got '%d'; want '%d'", attrMap.capacity, wantCapacity)
-	}
-
-	if attrMap.droppedCount != wantCapacity {
-		t.Errorf("attrMap.droppedCount: got '%d'; want '%d'", attrMap.droppedCount, wantCapacity)
-	}
-
-	for i := 0; i < wantCapacity; i++ {
-		key := attribute.Key(fmt.Sprintf(testKeyFmt, i))
-		_, ok := attrMap.attributes[key]
-		if ok {
-			t.Errorf("key %q should be dropped", testKeyFmt)
-		}
-	}
-	for i := wantCapacity; i < 256; i++ {
-		key := attribute.Key(fmt.Sprintf(testKeyFmt, i))
-		_, ok := attrMap.attributes[key]
-		if !ok {
-			t.Errorf("key %q should not be dropped", key)
-		}
-	}
-}
-
-func TestAttributesMapGetOldestRemoveOldest(t *testing.T) {
-	attrMap := newAttributesMap(128)
-
-	for i := 0; i < 128; i++ {
-		attrMap.add(attribute.Int(fmt.Sprintf(testKeyFmt, i), i))
-	}
-
-	attrMap.removeOldest()
-	attrMap.removeOldest()
-	attrMap.removeOldest()
-
-	for i := 0; i < 3; i++ {
-		key := attribute.Key(fmt.Sprintf(testKeyFmt, i))
-		_, ok := attrMap.attributes[key]
-		if ok {
-			t.Errorf("key %q should be removed", key)
-		}
-	}
-}
-
-func TestAttributesMapToKeyValue(t *testing.T) {
-	attrMap := newAttributesMap(128)
-
-	for i := 0; i < 128; i++ {
-		attrMap.add(attribute.Int(fmt.Sprintf(testKeyFmt, i), i))
-	}
-
-	kv := attrMap.toKeyValue()
-
-	gotAttrLen := len(kv)
-	wantAttrLen := 128
-	if gotAttrLen != wantAttrLen {
-		t.Errorf("len(attrMap.attributes): got '%d'; want '%d'", gotAttrLen, wantAttrLen)
-	}
-}
-
-func BenchmarkAttributesMapToKeyValue(b *testing.B) {
-	attrMap := newAttributesMap(128)
-
-	for i := 0; i < 128; i++ {
-		attrMap.add(attribute.Int(fmt.Sprintf(testKeyFmt, i), i))
-	}
-
-	for n := 0; n < b.N; n++ {
-		attrMap.toKeyValue()
-	}
-}

sdk/trace/benchmark_test.go

@@ -16,6 +16,7 @@ package trace_test
 
 import (
 	"context"
+	"fmt"
 	"testing"
 	"time"
 
@@ -24,6 +25,28 @@ import (
 	"go.opentelemetry.io/otel/trace"
 )
 
+func BenchmarkSpanSetAttributesOverCapacity(b *testing.B) {
+	tp := sdktrace.NewTracerProvider(
+		sdktrace.WithSpanLimits(sdktrace.SpanLimits{AttributeCountLimit: 1}),
+	)
+	tracer := tp.Tracer("BenchmarkSpanSetAttributesOverCapacity")
+	ctx := context.Background()
+	attrs := make([]attribute.KeyValue, 128)
+	for i := range attrs {
+		key := fmt.Sprintf("key-%d", i)
+		attrs[i] = attribute.Bool(key, true)
+	}
+
+	b.ReportAllocs()
+	b.ResetTimer()
+
+	for i := 0; i < b.N; i++ {
+		_, span := tracer.Start(ctx, "/foo")
+		span.SetAttributes(attrs...)
+		span.End()
+	}
+}
+
 func BenchmarkStartEndSpan(b *testing.B) {
 	traceBenchmark(b, "Benchmark StartEndSpan", func(b *testing.B, t trace.Tracer) {
 		ctx := context.Background()

sdk/trace/span.go

@@ -54,6 +54,7 @@ type ReadOnlySpan interface {
 	// the span has not ended.
 	EndTime() time.Time
 	// Attributes returns the defining attributes of the span.
+	// The order of the returned attributes is not guaranteed to be stable across invocations.
 	Attributes() []attribute.KeyValue
 	// Links returns all the links the span has to other spans.
 	Links() []Link
@@ -129,9 +130,14 @@ type recordingSpan struct {
 	// spanContext holds the SpanContext of this span.
 	spanContext trace.SpanContext
 
-	// attributes are capped at configured limit. When the capacity is reached
-	// an oldest entry is removed to create room for a new entry.
-	attributes *attributesMap
+	// attributes is a collection of user provided key/values. The collection
+	// is constrained by a configurable maximum held by the parent
+	// TracerProvider. When additional attributes are added after this maximum
+	// is reached these attributes the user is attempting to add are dropped.
+	// This dropped number of attributes is tracked and reported in the
+	// ReadOnlySpan exported when the span ends.
+	attributes        []attribute.KeyValue
+	droppedAttributes int
 
 	// events are stored in FIFO queue capped by configured limit.
 	events evictedQueue
@@ -194,11 +200,80 @@ func (s *recordingSpan) SetStatus(code codes.Code, description string) {
 // will be overwritten with the value contained in attributes.
 //
 // If this span is not being recorded than this method does nothing.
+//
+// If adding attributes to the span would exceed the maximum amount of
+// attributes the span is configured to have, the last added attributes will
+// be dropped.
 func (s *recordingSpan) SetAttributes(attributes ...attribute.KeyValue) {
 	if !s.IsRecording() {
 		return
 	}
-	s.copyToCappedAttributes(attributes...)
+
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	// If adding these attributes could exceed the capacity of s perform a
+	// de-duplication and truncation while adding to avoid over allocation.
+	if len(s.attributes)+len(attributes) > s.tracer.provider.spanLimits.AttributeCountLimit {
+		s.addOverCapAttrs(attributes)
+		return
+	}
+
+	// Otherwise, add without deduplication. When attributes are read they
+	// will be deduplicated, optimizing the operation.
+	for _, a := range attributes {
+		if !a.Valid() {
+			// Drop all invalid attributes.
+			s.droppedAttributes++
+			continue
+		}
+		s.attributes = append(s.attributes, a)
+	}
+}
+
+// addOverCapAttrs adds the attributes attrs to the span s while
+// de-duplicating the attributes of s and attrs and dropping attributes that
+// exceed the capacity of s.
+//
+// This method assumes s.mu.Lock is held by the caller.
+//
+// This method should only be called when there is a possibility that adding
+// attrs to s will exceed the capacity of s. Otherwise, attrs should be added
+// to s without checking for duplicates and all retrieval methods of the
+// attributes for s will de-duplicate as needed.
+func (s *recordingSpan) addOverCapAttrs(attrs []attribute.KeyValue) {
+	// In order to not allocate more capacity to s.attributes than needed,
+	// prune and truncate this addition of attributes while adding.
+
+	// Do not set a capacity when creating this map. Benchmark testing has
+	// showed this to only add unused memory allocations in general use.
+	exists := make(map[attribute.Key]int)
+	s.dedupeAttrsFromRecord(&exists)
+
+	// Now that s.attributes is deduplicated, adding unique attributes up to
+	// the capacity of s will not over allocate s.attributes.
+	for _, a := range attrs {
+		if !a.Valid() {
+			// Drop all invalid attributes.
+			s.droppedAttributes++
+			continue
+		}
+
+		if idx, ok := exists[a.Key]; ok {
+			// Perform all updates before dropping, even when at capacity.
+			s.attributes[idx] = a
+			continue
+		}
+
+		if len(s.attributes) >= s.tracer.provider.spanLimits.AttributeCountLimit {
+			// Do not just drop all of the remaining attributes, make sure
+			// updates are checked and performed.
+			s.droppedAttributes++
+		} else {
+			s.attributes = append(s.attributes, a)
+			exists[a.Key] = len(s.attributes) - 1
+		}
+	}
 }
 
 // End ends the span. This method does nothing if the span is already ended or
@@ -388,13 +463,45 @@ func (s *recordingSpan) EndTime() time.Time {
 }
 
 // Attributes returns the attributes of this span.
+//
+// The order of the returned attributes is not guaranteed to be stable.
 func (s *recordingSpan) Attributes() []attribute.KeyValue {
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	if s.attributes.evictList.Len() == 0 {
-		return []attribute.KeyValue{}
+	s.dedupeAttrs()
+	return s.attributes
+}
+
+// dedupeAttrs deduplicates the attributes of s to fit capacity.
+//
+// This method assumes s.mu.Lock is held by the caller.
+func (s *recordingSpan) dedupeAttrs() {
+	// Do not set a capacity when creating this map. Benchmark testing has
+	// showed this to only add unused memory allocations in general use.
+	exists := make(map[attribute.Key]int)
+	s.dedupeAttrsFromRecord(&exists)
+}
+
+// dedupeAttrsFromRecord deduplicates the attributes of s to fit capacity
+// using record as the record of unique attribute keys to their index.
+//
+// This method assumes s.mu.Lock is held by the caller.
+func (s *recordingSpan) dedupeAttrsFromRecord(record *map[attribute.Key]int) {
+	// Use the fact that slices share the same backing array.
+	unique := s.attributes[:0]
+	for _, a := range s.attributes {
+		if idx, ok := (*record)[a.Key]; ok {
+			unique[idx] = a
+		} else {
+			unique = append(unique, a)
+			(*record)[a.Key] = len(unique) - 1
+		}
 	}
-	return s.attributes.toKeyValue()
+	// s.attributes have element types of attribute.KeyValue. These types are
+	// not pointers and they themselves do not contain pointer fields,
+	// therefore the duplicate values do not need to be zeroed for them to be
+	// garbage collected.
+	s.attributes = unique
 }
 
 // Links returns the links of this span.
@@ -463,7 +570,7 @@ func (s *recordingSpan) addLink(link trace.Link) {
 func (s *recordingSpan) DroppedAttributes() int {
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	return s.attributes.droppedCount
+	return s.droppedAttributes
 }
 
 // DroppedLinks returns the number of links dropped by the span due to limits
@@ -513,10 +620,11 @@ func (s *recordingSpan) snapshot() ReadOnlySpan {
 	sd.status = s.status
 	sd.childSpanCount = s.childSpanCount
 
-	if s.attributes.evictList.Len() > 0 {
-		sd.attributes = s.attributes.toKeyValue()
-		sd.droppedAttributeCount = s.attributes.droppedCount
+	if len(s.attributes) > 0 {
+		s.dedupeAttrs()
+		sd.attributes = s.attributes
 	}
+	sd.droppedAttributeCount = s.droppedAttributes
 	if len(s.events.queue) > 0 {
 		sd.events = s.interfaceArrayToEventArray()
 		sd.droppedEventCount = s.events.droppedCount
@@ -544,18 +652,6 @@ func (s *recordingSpan) interfaceArrayToEventArray() []Event {
 	return eventArr
 }
 
-func (s *recordingSpan) copyToCappedAttributes(attributes ...attribute.KeyValue) {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-	for _, a := range attributes {
-		// Ensure attributes conform to the specification:
-		// https://github.com/open-telemetry/opentelemetry-specification/blob/v1.0.1/specification/common/common.md#attributes
-		if a.Valid() {
-			s.attributes.add(a)
-		}
-	}
-}
-
 func (s *recordingSpan) addChild() {
 	if !s.IsRecording() {
 		return

sdk/trace/trace_test.go

@@ -419,34 +419,6 @@ func TestSetSpanAttributesOnStart(t *testing.T) {
 	}
 }
 
-func TestSetSpanAttributes(t *testing.T) {
-	te := NewTestExporter()
-	tp := NewTracerProvider(WithSyncer(te), WithResource(resource.Empty()))
-	span := startSpan(tp, "SpanAttribute")
-	span.SetAttributes(attribute.String("key1", "value1"))
-	got, err := endSpan(te, span)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	want := &snapshot{
-		spanContext: trace.NewSpanContext(trace.SpanContextConfig{
-			TraceID:    tid,
-			TraceFlags: 0x1,
-		}),
-		parent: sc.WithRemote(true),
-		name:   "span0",
-		attributes: []attribute.KeyValue{
-			attribute.String("key1", "value1"),
-		},
-		spanKind:               trace.SpanKindInternal,
-		instrumentationLibrary: instrumentation.Library{Name: "SpanAttribute"},
-	}
-	if diff := cmpDiff(got, want); diff != "" {
-		t.Errorf("SetSpanAttributes: -got +want %s", diff)
-	}
-}
-
 func TestSamplerAttributesLocalChildSpan(t *testing.T) {
 	sampler := &testSampler{prefix: "span", t: t}
 	te := NewTestExporter()
@@ -469,72 +441,193 @@ func TestSamplerAttributesLocalChildSpan(t *testing.T) {
 	assert.Equal(t, []attribute.KeyValue{attribute.Int("callCount", 1)}, gotSpan1.Attributes())
 }
 
-func TestSetSpanAttributesOverLimit(t *testing.T) {
-	te := NewTestExporter()
-	tp := NewTracerProvider(WithSpanLimits(SpanLimits{AttributeCountLimit: 2}), WithSyncer(te), WithResource(resource.Empty()))
-
-	span := startSpan(tp, "SpanAttributesOverLimit")
-	span.SetAttributes(
-		attribute.Bool("key1", true),
+func TestSpanSetAttributes(t *testing.T) {
+	attrs := [...]attribute.KeyValue{
+		attribute.String("key1", "value1"),
 		attribute.String("key2", "value2"),
-		attribute.Bool("key1", false), // Replace key1.
-		attribute.Int64("key4", 4),    // Remove key2 and add key4
-	)
-	got, err := endSpan(te, span)
-	if err != nil {
-		t.Fatal(err)
+		attribute.String("key3", "value3"),
+		attribute.String("key4", "value4"),
+		attribute.String("key1", "value5"),
+		attribute.String("key2", "value6"),
+		attribute.String("key3", "value7"),
 	}
+	invalid := attribute.KeyValue{}
 
-	want := &snapshot{
-		spanContext: trace.NewSpanContext(trace.SpanContextConfig{
-			TraceID:    tid,
-			TraceFlags: 0x1,
-		}),
-		parent: sc.WithRemote(true),
-		name:   "span0",
-		attributes: []attribute.KeyValue{
-			attribute.Bool("key1", false),
-			attribute.Int64("key4", 4),
+	tests := []struct {
+		name        string
+		input       [][]attribute.KeyValue
+		wantAttrs   []attribute.KeyValue
+		wantDropped int
+	}{
+		{
+			name:      "array",
+			input:     [][]attribute.KeyValue{attrs[:3]},
+			wantAttrs: attrs[:3],
 		},
-		spanKind:               trace.SpanKindInternal,
-		droppedAttributeCount:  1,
-		instrumentationLibrary: instrumentation.Library{Name: "SpanAttributesOverLimit"},
-	}
-	if diff := cmpDiff(got, want); diff != "" {
-		t.Errorf("SetSpanAttributesOverLimit: -got +want %s", diff)
-	}
-}
-
-func TestSetSpanAttributesWithInvalidKey(t *testing.T) {
-	te := NewTestExporter()
-	tp := NewTracerProvider(WithSpanLimits(SpanLimits{}), WithSyncer(te), WithResource(resource.Empty()))
-
-	span := startSpan(tp, "SpanToSetInvalidKeyOrValue")
-	span.SetAttributes(
-		attribute.Bool("", true),
-		attribute.Bool("key1", false),
-	)
-	got, err := endSpan(te, span)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	want := &snapshot{
-		spanContext: trace.NewSpanContext(trace.SpanContextConfig{
-			TraceID:    tid,
-			TraceFlags: 0x1,
-		}),
-		parent: sc.WithRemote(true),
-		name:   "span0",
-		attributes: []attribute.KeyValue{
-			attribute.Bool("key1", false),
+		{
+			name:      "single_value:array",
+			input:     [][]attribute.KeyValue{attrs[:1], attrs[1:3]},
+			wantAttrs: attrs[:3],
+		},
+		{
+			name:      "array:single_value",
+			input:     [][]attribute.KeyValue{attrs[:2], attrs[2:3]},
+			wantAttrs: attrs[:3],
+		},
+		{
+			name:      "single_values",
+			input:     [][]attribute.KeyValue{attrs[:1], attrs[1:2], attrs[2:3]},
+			wantAttrs: attrs[:3],
+		},
+
+		// The tracing specification states:
+		//
+		//   For each unique attribute key, addition of which would result in
+		//   exceeding the limit, SDK MUST discard that key/value pair
+		//
+		// Therefore, adding attributes after the capacity is reached should
+		// result in those attributes being dropped.
+
+		{
+			name:        "drop_last_added",
+			input:       [][]attribute.KeyValue{attrs[:3], attrs[3:4], attrs[3:4]},
+			wantAttrs:   attrs[:3],
+			wantDropped: 2,
+		},
+
+		// The tracing specification states:
+		//
+		//   Setting an attribute with the same key as an existing attribute
+		//   SHOULD overwrite the existing attribute's value.
+		//
+		// Therefore, attributes are updated regardless of capacity state.
+
+		{
+			name:      "single_value_update",
+			input:     [][]attribute.KeyValue{attrs[:1], attrs[:3]},
+			wantAttrs: attrs[:3],
+		},
+		{
+			name:      "all_update",
+			input:     [][]attribute.KeyValue{attrs[:3], attrs[4:7]},
+			wantAttrs: attrs[4:7],
+		},
+		{
+			name:      "all_update/multi",
+			input:     [][]attribute.KeyValue{attrs[:3], attrs[4:7], attrs[:3]},
+			wantAttrs: attrs[:3],
+		},
+		{
+			name:      "deduplicate/under_capacity",
+			input:     [][]attribute.KeyValue{attrs[:1], attrs[:1], attrs[:1]},
+			wantAttrs: attrs[:1],
+		},
+		{
+			name:      "deduplicate/over_capacity",
+			input:     [][]attribute.KeyValue{attrs[:1], attrs[:1], attrs[:1], attrs[:3]},
+			wantAttrs: attrs[:3],
+		},
+		{
+			name: "deduplicate/added",
+			input: [][]attribute.KeyValue{
+				attrs[:2],
+				{attrs[2], attrs[2], attrs[2]},
+			},
+			wantAttrs: attrs[:3],
+		},
+		{
+			name: "deduplicate/added_at_cappacity",
+			input: [][]attribute.KeyValue{
+				attrs[:3],
+				{attrs[2], attrs[2], attrs[2]},
+			},
+			wantAttrs: attrs[:3],
+		},
+		{
+			name: "invalid",
+			input: [][]attribute.KeyValue{
+				{invalid},
+			},
+			wantDropped: 1,
+		},
+		{
+			name: "invalid_with_valid",
+			input: [][]attribute.KeyValue{
+				{invalid, attrs[0]},
+			},
+			wantAttrs:   attrs[:1],
+			wantDropped: 1,
+		},
+		{
+			name: "invalid_over_capacity",
+			input: [][]attribute.KeyValue{
+				{invalid, invalid, invalid, invalid, attrs[0]},
+			},
+			wantAttrs:   attrs[:1],
+			wantDropped: 4,
+		},
+		{
+			name: "valid:invalid/under_capacity",
+			input: [][]attribute.KeyValue{
+				attrs[:1],
+				{invalid},
+			},
+			wantAttrs:   attrs[:1],
+			wantDropped: 1,
+		},
+		{
+			name: "valid:invalid/over_capacity",
+			input: [][]attribute.KeyValue{
+				attrs[:1],
+				{invalid, invalid, invalid, invalid},
+			},
+			wantAttrs:   attrs[:1],
+			wantDropped: 4,
+		},
+		{
+			name: "valid_at_capacity:invalid",
+			input: [][]attribute.KeyValue{
+				attrs[:3],
+				{invalid, invalid, invalid, invalid},
+			},
+			wantAttrs:   attrs[:3],
+			wantDropped: 4,
 		},
-		spanKind:               trace.SpanKindInternal,
-		droppedAttributeCount:  0,
-		instrumentationLibrary: instrumentation.Library{Name: "SpanToSetInvalidKeyOrValue"},
 	}
-	if diff := cmpDiff(got, want); diff != "" {
-		t.Errorf("SetSpanAttributesWithInvalidKey: -got +want %s", diff)
+
+	const (
+		capacity = 3
+		instName = "TestSpanAttributeCapacity"
+		spanName = "test span"
+	)
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			te := NewTestExporter()
+			tp := NewTracerProvider(
+				WithSyncer(te),
+				WithSpanLimits(SpanLimits{AttributeCountLimit: capacity}),
+			)
+			_, span := tp.Tracer(instName).Start(context.Background(), spanName)
+			for _, a := range test.input {
+				span.SetAttributes(a...)
+			}
+			span.End()
+
+			require.Implements(t, (*ReadOnlySpan)(nil), span)
+			roSpan := span.(ReadOnlySpan)
+
+			// Ensure the span itself is valid.
+			assert.ElementsMatch(t, test.wantAttrs, roSpan.Attributes(), "exected attributes")
+			assert.Equal(t, test.wantDropped, roSpan.DroppedAttributes(), "dropped attributes")
+
+			snap, ok := te.GetSpan(spanName)
+			require.Truef(t, ok, "span %s not exported", spanName)
+
+			// Ensure the exported span snapshot is valid.
+			assert.ElementsMatch(t, test.wantAttrs, snap.Attributes(), "exected attributes")
+			assert.Equal(t, test.wantDropped, snap.DroppedAttributes(), "dropped attributes")
+		})
 	}
 }
 
@@ -1852,3 +1945,11 @@ func TestWithIDGenerator(t *testing.T) {
 		require.NoError(t, err)
 	}
 }
+
+func TestEmptyRecordingSpanAttributes(t *testing.T) {
+	assert.Nil(t, (&recordingSpan{}).Attributes())
+}
+
+func TestEmptyRecordingSpanDroppedAttributes(t *testing.T) {
+	assert.Equal(t, 0, (&recordingSpan{}).DroppedAttributes())
+}

sdk/trace/tracer.go

@@ -122,12 +122,19 @@ func (tr *tracer) newRecordingSpan(psc, sc trace.SpanContext, name string, sr Sa
 	}
 
 	s := &recordingSpan{
+		// Do not pre-allocate the attributes slice here! Doing so will
+		// allocate memory that is likely never going to be used, or if used,
+		// will be over-sized. The default Go compiler has been tested to
+		// dynamically allocate needed space very well. Benchmarking has shown
+		// it to be more performant than what we can predetermine here,
+		// especially for the common use case of few to no added
+		// attributes.
+
 		parent:      psc,
 		spanContext: sc,
 		spanKind:    trace.ValidateSpanKind(config.SpanKind()),
 		name:        name,
 		startTime:   startTime,
-		attributes:  newAttributesMap(tr.provider.spanLimits.AttributeCountLimit),
 		events:      newEvictedQueue(tr.provider.spanLimits.EventCountLimit),
 		links:       newEvictedQueue(tr.provider.spanLimits.LinkCountLimit),
 		tracer:      tr,