From 0acc1bc5f4a8f1bdaa6932bc715fb5e21933b5e7 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 1 Feb 2022 15:49:33 -0800 Subject: [PATCH 01/11] 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. --- sdk/trace/benchmark_test.go | 23 +++++++ sdk/trace/span.go | 123 +++++++++++++++++++++++++++++------- sdk/trace/trace_test.go | 80 ++++++++++++++++------- sdk/trace/tracer.go | 1 - 4 files changed, 178 insertions(+), 49 deletions(-) diff --git a/sdk/trace/benchmark_test.go b/sdk/trace/benchmark_test.go index 335deaf4fc3..81a06e2f8f9 100644 --- a/sdk/trace/benchmark_test.go +++ b/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() diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 0111c475895..ed06c63b008 100644 --- a/sdk/trace/span.go +++ b/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 @@ -137,9 +138,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 @@ -205,11 +211,91 @@ 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() + + max := s.tracer.provider.spanLimits.AttributeCountLimit + if len(s.attributes)+len(attributes) > max { + exists := make(map[attribute.Key]int, len(s.attributes)) + for i, a := range s.attributes { + if idx, ok := exists[a.Key]; ok { + s.attributes[idx] = a + + copy(s.attributes[i:], s.attributes[i+1:]) + s.attributes[len(s.attributes)-1] = attribute.KeyValue{} + s.attributes = s.attributes[:len(s.attributes)-1] + } else { + exists[a.Key] = i + } + } + + // Perform all updates before dropping. + for _, a := range attributes { + if idx, ok := exists[a.Key]; ok { + s.attributes[idx] = a + continue + } + + if !a.Valid() { + s.droppedAttributes++ + continue + } + + if len(s.attributes) >= max { + s.droppedAttributes++ + } else { + s.attributes = append(s.attributes, a) + exists[a.Key] = len(s.attributes) - 1 + } + } + return + } + + 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.droppedAttributes++ + continue + } + s.attributes = append(s.attributes, a) + } + + if len(s.attributes) > max { + s.pruneAttrsLocked() + } +} + +func (s *recordingSpan) pruneAttrsLocked() { + exists := make(map[attribute.Key]int, len(s.attributes)) + for i, a := range s.attributes { + if idx, ok := exists[a.Key]; ok { + // Setting an attribute with the same key as an existing attribute + // SHOULD overwrite the existing attribute's value. + s.attributes[idx] = a + + copy(s.attributes[i:], s.attributes[i+1:]) + s.attributes[len(s.attributes)-1] = attribute.KeyValue{} + s.attributes = s.attributes[:len(s.attributes)-1] + } else { + exists[a.Key] = i + } + } + + max := s.tracer.provider.spanLimits.AttributeCountLimit + if len(s.attributes) > max { + s.droppedAttributes += len(s.attributes) - max + s.attributes = s.attributes[:max] + } } // End ends the span. This method does nothing if the span is already ended or @@ -399,13 +485,13 @@ 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{} - } - return s.attributes.toKeyValue() + s.pruneAttrsLocked() + return s.attributes } // Links returns the links of this span. @@ -474,7 +560,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 @@ -524,9 +610,10 @@ 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.pruneAttrsLocked() + sd.attributes = s.attributes + sd.droppedAttributeCount = s.droppedAttributes } if len(s.events.queue) > 0 { sd.events = s.interfaceArrayToEventArray() @@ -555,18 +642,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 diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index be27ed62041..8e2fbea9550 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -470,39 +470,71 @@ func TestSamplerAttributesLocalChildSpan(t *testing.T) { } func TestSetSpanAttributesOverLimit(t *testing.T) { - te := NewTestExporter() - tp := NewTracerProvider(WithSpanLimits(SpanLimits{AttributeCountLimit: 2}), WithSyncer(te), WithResource(resource.Empty())) + // The tracing specification states: + // + // Setting an attribute with the same key as an existing attribute + // SHOULD overwrite the existing attribute's value. - span := startSpan(tp, "SpanAttributesOverLimit") - span.SetAttributes( + te := NewTestExporter() + tp := NewTracerProvider( + WithSpanLimits(SpanLimits{AttributeCountLimit: 2}), + WithSyncer(te), + WithResource(resource.Empty()), + ) + attrs := []attribute.KeyValue{ attribute.Bool("key1", true), attribute.String("key2", "value2"), attribute.Bool("key1", false), // Replace key1. - attribute.Int64("key4", 4), // Remove key2 and add key4 - ) + attribute.Int64("key4", 4), // Dropped + } + + span := startSpan(tp, "SpanAttributesOverLimit") + span.SetAttributes(attrs...) got, err := endSpan(te, span) if err != nil { t.Fatal(err) } + assert.Contains(t, got.attributes, attrs[1]) + assert.Contains(t, got.attributes, attrs[2]) + assert.Equal(t, 1, got.droppedAttributeCount) +} - 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), - }, - spanKind: trace.SpanKindInternal, - droppedAttributeCount: 1, - instrumentationLibrary: instrumentation.Library{Name: "SpanAttributesOverLimit"}, - } - if diff := cmpDiff(got, want); diff != "" { - t.Errorf("SetSpanAttributesOverLimit: -got +want %s", diff) +func TestSpanAttributeCapacityDropOrder(t *testing.T) { + // 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. + + const ( + instName = "TestSpanAttributeCapacityDropOrder" + spanName = "test span" + ) + + te := NewTestExporter() + tp := NewTracerProvider( + WithSyncer(te), + WithSpanLimits(SpanLimits{AttributeCountLimit: 1}), + ) + attrs := []attribute.KeyValue{ + attribute.String("key1", "value1"), + attribute.String("key2", "value2"), } + + _, span := tp.Tracer(instName).Start(context.Background(), spanName) + span.SetAttributes(attrs[0]) + // Should be dropped based on limits. + span.SetAttributes(attrs[1]) + // Should "update" the first attribute, and drop the second. + span.SetAttributes(attrs...) + span.End() + + got, ok := te.GetSpan(spanName) + require.Truef(t, ok, "span %s not exported", spanName) + assert.Equal(t, attrs[:1], got.attributes) + assert.Equal(t, 2, got.droppedAttributeCount) } func TestSetSpanAttributesWithInvalidKey(t *testing.T) { @@ -530,7 +562,7 @@ func TestSetSpanAttributesWithInvalidKey(t *testing.T) { attribute.Bool("key1", false), }, spanKind: trace.SpanKindInternal, - droppedAttributeCount: 0, + droppedAttributeCount: 1, instrumentationLibrary: instrumentation.Library{Name: "SpanToSetInvalidKeyOrValue"}, } if diff := cmpDiff(got, want); diff != "" { diff --git a/sdk/trace/tracer.go b/sdk/trace/tracer.go index 1177c729a8f..c12e0f8787b 100644 --- a/sdk/trace/tracer.go +++ b/sdk/trace/tracer.go @@ -127,7 +127,6 @@ func (tr *tracer) newRecordingSpan(psc, sc trace.SpanContext, name string, sr Sa 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, From cb4d0e342d6d6c29df2152cd7ab28b5c7f352804 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 2 Feb 2022 12:23:24 -0800 Subject: [PATCH 02/11] Remove attributesmap files --- sdk/trace/attributesmap.go | 91 ---------------------------- sdk/trace/attributesmap_test.go | 103 -------------------------------- 2 files changed, 194 deletions(-) delete mode 100644 sdk/trace/attributesmap.go delete mode 100644 sdk/trace/attributesmap_test.go diff --git a/sdk/trace/attributesmap.go b/sdk/trace/attributesmap.go deleted file mode 100644 index b891c8178b7..00000000000 --- a/sdk/trace/attributesmap.go +++ /dev/null @@ -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) - } -} diff --git a/sdk/trace/attributesmap_test.go b/sdk/trace/attributesmap_test.go deleted file mode 100644 index 90a201e2195..00000000000 --- a/sdk/trace/attributesmap_test.go +++ /dev/null @@ -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() - } -} From 6952bc523a03458a7595bd053848029c29114649 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 3 Feb 2022 08:56:56 -0800 Subject: [PATCH 03/11] Refine addition algorithm Unify duplicated code. Fix deduplication algorithm. Fix droppedAttributes to always be returned, even if the span has no attributes. --- sdk/trace/span.go | 132 ++++++++++++++++++++++++++-------------------- 1 file changed, 74 insertions(+), 58 deletions(-) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index e80921c4cbc..89e9ff0a337 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -212,79 +212,65 @@ func (s *recordingSpan) SetAttributes(attributes ...attribute.KeyValue) { s.mu.Lock() defer s.mu.Unlock() - max := s.tracer.provider.spanLimits.AttributeCountLimit - if len(s.attributes)+len(attributes) > max { - exists := make(map[attribute.Key]int, len(s.attributes)) - for i, a := range s.attributes { - if idx, ok := exists[a.Key]; ok { - s.attributes[idx] = a - - copy(s.attributes[i:], s.attributes[i+1:]) - s.attributes[len(s.attributes)-1] = attribute.KeyValue{} - s.attributes = s.attributes[:len(s.attributes)-1] - } else { - exists[a.Key] = i - } - } - - // Perform all updates before dropping. - for _, a := range attributes { - if idx, ok := exists[a.Key]; ok { - s.attributes[idx] = a - continue - } - - if !a.Valid() { - s.droppedAttributes++ - continue - } - - if len(s.attributes) >= max { - s.droppedAttributes++ - } else { - s.attributes = append(s.attributes, a) - exists[a.Key] = len(s.attributes) - 1 - } - } + // 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 { - // 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() { + // Drop all invalid attributes. s.droppedAttributes++ continue } s.attributes = append(s.attributes, a) } - - if len(s.attributes) > max { - s.pruneAttrsLocked() - } } -func (s *recordingSpan) pruneAttrsLocked() { - exists := make(map[attribute.Key]int, len(s.attributes)) - for i, a := range s.attributes { +// 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. + 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 { - // Setting an attribute with the same key as an existing attribute - // SHOULD overwrite the existing attribute's value. + // Perform all updates before dropping, even when at capacity. s.attributes[idx] = a + continue + } - copy(s.attributes[i:], s.attributes[i+1:]) - s.attributes[len(s.attributes)-1] = attribute.KeyValue{} - s.attributes = s.attributes[:len(s.attributes)-1] + 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 { - exists[a.Key] = i + s.attributes = append(s.attributes, a) + exists[a.Key] = len(s.attributes) - 1 } } - - max := s.tracer.provider.spanLimits.AttributeCountLimit - if len(s.attributes) > max { - s.droppedAttributes += len(s.attributes) - max - s.attributes = s.attributes[:max] - } } // End ends the span. This method does nothing if the span is already ended or @@ -479,10 +465,40 @@ func (s *recordingSpan) EndTime() time.Time { func (s *recordingSpan) Attributes() []attribute.KeyValue { s.mu.Lock() defer s.mu.Unlock() - s.pruneAttrsLocked() + 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() { + 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 + } + } + // 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. func (s *recordingSpan) Links() []Link { s.mu.Lock() @@ -600,10 +616,10 @@ func (s *recordingSpan) snapshot() ReadOnlySpan { sd.childSpanCount = s.childSpanCount if len(s.attributes) > 0 { - s.pruneAttrsLocked() + s.dedupeAttrs() sd.attributes = s.attributes - sd.droppedAttributeCount = s.droppedAttributes } + sd.droppedAttributeCount = s.droppedAttributes if len(s.events.queue) > 0 { sd.events = s.interfaceArrayToEventArray() sd.droppedEventCount = s.events.droppedCount From 44ff00b006a15c3f10dbfdd7f6d8b557495cbfcd Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 3 Feb 2022 09:18:20 -0800 Subject: [PATCH 04/11] Unify span SetAttributes tests --- sdk/trace/trace_test.go | 283 ++++++++++++++++++++++++---------------- 1 file changed, 168 insertions(+), 115 deletions(-) diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 8e2fbea9550..914a577f796 100644 --- a/sdk/trace/trace_test.go +++ b/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,104 +441,185 @@ func TestSamplerAttributesLocalChildSpan(t *testing.T) { assert.Equal(t, []attribute.KeyValue{attribute.Int("callCount", 1)}, gotSpan1.Attributes()) } -func TestSetSpanAttributesOverLimit(t *testing.T) { - // The tracing specification states: - // - // Setting an attribute with the same key as an existing attribute - // SHOULD overwrite the existing attribute's value. - - te := NewTestExporter() - tp := NewTracerProvider( - WithSpanLimits(SpanLimits{AttributeCountLimit: 2}), - WithSyncer(te), - WithResource(resource.Empty()), - ) - attrs := []attribute.KeyValue{ - 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), // Dropped + attribute.String("key3", "value3"), + attribute.String("key4", "value4"), + attribute.String("key1", "value5"), + attribute.String("key2", "value6"), + attribute.String("key3", "value7"), } + invalid := attribute.KeyValue{} - span := startSpan(tp, "SpanAttributesOverLimit") - span.SetAttributes(attrs...) - got, err := endSpan(te, span) - if err != nil { - t.Fatal(err) - } - assert.Contains(t, got.attributes, attrs[1]) - assert.Contains(t, got.attributes, attrs[2]) - assert.Equal(t, 1, got.droppedAttributeCount) -} + tests := []struct { + name string + input [][]attribute.KeyValue + wantAttrs []attribute.KeyValue + wantDropped int + }{ + { + name: "array", + input: [][]attribute.KeyValue{attrs[:3]}, + wantAttrs: attrs[:3], + }, + { + 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], + }, -func TestSpanAttributeCapacityDropOrder(t *testing.T) { - // 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. + // 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. - const ( - instName = "TestSpanAttributeCapacityDropOrder" - spanName = "test span" - ) + { + name: "drop_last_added", + input: [][]attribute.KeyValue{attrs[:3], attrs[3:4], attrs[3:4]}, + wantAttrs: attrs[:3], + wantDropped: 2, + }, - te := NewTestExporter() - tp := NewTracerProvider( - WithSyncer(te), - WithSpanLimits(SpanLimits{AttributeCountLimit: 1}), - ) - attrs := []attribute.KeyValue{ - attribute.String("key1", "value1"), - attribute.String("key2", "value2"), - } + // 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. - _, span := tp.Tracer(instName).Start(context.Background(), spanName) - span.SetAttributes(attrs[0]) - // Should be dropped based on limits. - span.SetAttributes(attrs[1]) - // Should "update" the first attribute, and drop the second. - span.SetAttributes(attrs...) - span.End() + { + 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], + []attribute.KeyValue{attrs[2], attrs[2], attrs[2]}, + }, + wantAttrs: attrs[:3], + }, + { + name: "deduplicate/added_at_cappacity", + input: [][]attribute.KeyValue{ + attrs[:3], + []attribute.KeyValue{attrs[2], attrs[2], attrs[2]}, + }, + wantAttrs: attrs[:3], + }, + { + name: "invalid", + input: [][]attribute.KeyValue{ + []attribute.KeyValue{invalid}, + }, + wantDropped: 1, + }, + { + name: "invalid_with_valid", + input: [][]attribute.KeyValue{ + []attribute.KeyValue{invalid, attrs[0]}, + }, + wantAttrs: attrs[:1], + wantDropped: 1, + }, + { + name: "invalid_over_capacity", + input: [][]attribute.KeyValue{ + []attribute.KeyValue{invalid, invalid, invalid, invalid, attrs[0]}, + }, + wantAttrs: attrs[:1], + wantDropped: 4, + }, + { + name: "valid:invalid/under_capacity", + input: [][]attribute.KeyValue{ + attrs[:1], + []attribute.KeyValue{invalid}, + }, + wantAttrs: attrs[:1], + wantDropped: 1, + }, + { + name: "valid:invalid/over_capacity", + input: [][]attribute.KeyValue{ + attrs[:1], + []attribute.KeyValue{invalid, invalid, invalid, invalid}, + }, + wantAttrs: attrs[:1], + wantDropped: 4, + }, + { + name: "valid_at_capacity:invalid", + input: [][]attribute.KeyValue{ + attrs[:3], + []attribute.KeyValue{invalid, invalid, invalid, invalid}, + }, + wantAttrs: attrs[:3], + wantDropped: 4, + }, + } - got, ok := te.GetSpan(spanName) - require.Truef(t, ok, "span %s not exported", spanName) - assert.Equal(t, attrs[:1], got.attributes) - assert.Equal(t, 2, got.droppedAttributeCount) -} + const ( + capacity = 3 + instName = "TestSpanAttributeCapacity" + spanName = "test span" + ) -func TestSetSpanAttributesWithInvalidKey(t *testing.T) { - te := NewTestExporter() - tp := NewTracerProvider(WithSpanLimits(SpanLimits{}), WithSyncer(te), WithResource(resource.Empty())) + 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() - span := startSpan(tp, "SpanToSetInvalidKeyOrValue") - span.SetAttributes( - attribute.Bool("", true), - attribute.Bool("key1", false), - ) - got, err := endSpan(te, span) - if err != nil { - t.Fatal(err) - } + got, ok := te.GetSpan(spanName) + require.Truef(t, ok, "span %s not exported", spanName) - want := &snapshot{ - spanContext: trace.NewSpanContext(trace.SpanContextConfig{ - TraceID: tid, - TraceFlags: 0x1, - }), - parent: sc.WithRemote(true), - name: "span0", - attributes: []attribute.KeyValue{ - attribute.Bool("key1", false), - }, - spanKind: trace.SpanKindInternal, - droppedAttributeCount: 1, - instrumentationLibrary: instrumentation.Library{Name: "SpanToSetInvalidKeyOrValue"}, - } - if diff := cmpDiff(got, want); diff != "" { - t.Errorf("SetSpanAttributesWithInvalidKey: -got +want %s", diff) + assert.ElementsMatch(t, test.wantAttrs, got.Attributes(), "exected attributes") + assert.Equal(t, test.wantDropped, got.DroppedAttributes(), "dropped attributes") + }) } } From 8797c26e93876908a75e3da9e2ca7fcb65a077fb Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 3 Feb 2022 09:20:08 -0800 Subject: [PATCH 05/11] Doc fix to attr drop order in changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb91daf7486..e61150dc26c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Change the `otlpmetric.Client` interface's `UploadMetrics` method to accept a single `ResourceMetrics` instead of a slice of them. (#2491) - Specify explicit buckets in Prometheus example. (#2493) - W3C baggage will now decode urlescaped values. (#2529) +- 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 From 3c609fa3c64993a5b53bd4cb9e6e57d14f6af183 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 3 Feb 2022 09:32:12 -0800 Subject: [PATCH 06/11] Test span and snapshot attrs --- sdk/trace/trace_test.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 914a577f796..dfdd501bfa3 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -614,11 +614,19 @@ func TestSpanSetAttributes(t *testing.T) { } span.End() - got, ok := te.GetSpan(spanName) + 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) - assert.ElementsMatch(t, test.wantAttrs, got.Attributes(), "exected attributes") - assert.Equal(t, test.wantDropped, got.DroppedAttributes(), "dropped attributes") + // 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") }) } } From 326b64628db574b406efd8b2516e85f261003c3b Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 3 Feb 2022 09:34:25 -0800 Subject: [PATCH 07/11] fix lint --- sdk/trace/trace_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index dfdd501bfa3..d44e3ce37d6 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -531,7 +531,7 @@ func TestSpanSetAttributes(t *testing.T) { name: "deduplicate/added", input: [][]attribute.KeyValue{ attrs[:2], - []attribute.KeyValue{attrs[2], attrs[2], attrs[2]}, + {attrs[2], attrs[2], attrs[2]}, }, wantAttrs: attrs[:3], }, @@ -539,21 +539,21 @@ func TestSpanSetAttributes(t *testing.T) { name: "deduplicate/added_at_cappacity", input: [][]attribute.KeyValue{ attrs[:3], - []attribute.KeyValue{attrs[2], attrs[2], attrs[2]}, + {attrs[2], attrs[2], attrs[2]}, }, wantAttrs: attrs[:3], }, { name: "invalid", input: [][]attribute.KeyValue{ - []attribute.KeyValue{invalid}, + {invalid}, }, wantDropped: 1, }, { name: "invalid_with_valid", input: [][]attribute.KeyValue{ - []attribute.KeyValue{invalid, attrs[0]}, + {invalid, attrs[0]}, }, wantAttrs: attrs[:1], wantDropped: 1, @@ -561,7 +561,7 @@ func TestSpanSetAttributes(t *testing.T) { { name: "invalid_over_capacity", input: [][]attribute.KeyValue{ - []attribute.KeyValue{invalid, invalid, invalid, invalid, attrs[0]}, + {invalid, invalid, invalid, invalid, attrs[0]}, }, wantAttrs: attrs[:1], wantDropped: 4, @@ -570,7 +570,7 @@ func TestSpanSetAttributes(t *testing.T) { name: "valid:invalid/under_capacity", input: [][]attribute.KeyValue{ attrs[:1], - []attribute.KeyValue{invalid}, + {invalid}, }, wantAttrs: attrs[:1], wantDropped: 1, @@ -579,7 +579,7 @@ func TestSpanSetAttributes(t *testing.T) { name: "valid:invalid/over_capacity", input: [][]attribute.KeyValue{ attrs[:1], - []attribute.KeyValue{invalid, invalid, invalid, invalid}, + {invalid, invalid, invalid, invalid}, }, wantAttrs: attrs[:1], wantDropped: 4, @@ -588,7 +588,7 @@ func TestSpanSetAttributes(t *testing.T) { name: "valid_at_capacity:invalid", input: [][]attribute.KeyValue{ attrs[:3], - []attribute.KeyValue{invalid, invalid, invalid, invalid}, + {invalid, invalid, invalid, invalid}, }, wantAttrs: attrs[:3], wantDropped: 4, From b8cff41a89e22278d9ea317a1e64a8660b2deab2 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 3 Feb 2022 14:06:41 -0800 Subject: [PATCH 08/11] Add tests for recordingSpan method defaults --- sdk/trace/trace_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index d44e3ce37d6..5ba0015584a 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -1945,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()) +} From c242f7907cb809073e7feba48ddb512eb9714a36 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 3 Feb 2022 14:15:01 -0800 Subject: [PATCH 09/11] Comment why pre-allocation is not done --- sdk/trace/span.go | 5 +++++ sdk/trace/tracer.go | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 89e9ff0a337..779cde691dd 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -244,6 +244,9 @@ func (s *recordingSpan) SetAttributes(attributes ...attribute.KeyValue) { 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) @@ -473,6 +476,8 @@ func (s *recordingSpan) Attributes() []attribute.KeyValue { // // 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) } diff --git a/sdk/trace/tracer.go b/sdk/trace/tracer.go index ee4ed410fbf..ab7b06167fb 100644 --- a/sdk/trace/tracer.go +++ b/sdk/trace/tracer.go @@ -122,6 +122,13 @@ func (tr *tracer) newRecordingSpan(psc, sc trace.SpanContext, name string, sr Sa } s := &recordingSpan{ + // Do not pre-allocate the span slice here to be the max capacity. + // Doing so will allocate memory that is not going to be use if the + // user never adds attributes or does not add attributes to up to the + // capacity. The default Go compiler has been tested to handle + // dynamically allocating needed space when attributes are added in a + // more performant way than we can predetermine here. + parent: psc, spanContext: sc, spanKind: trace.ValidateSpanKind(config.SpanKind()), From ad7106057900959971009969598b85d8b7ef4558 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Thu, 3 Feb 2022 15:20:23 -0800 Subject: [PATCH 10/11] Correct grammar in recordingSpan allocation comment --- sdk/trace/tracer.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/sdk/trace/tracer.go b/sdk/trace/tracer.go index ab7b06167fb..08fb0fd5d21 100644 --- a/sdk/trace/tracer.go +++ b/sdk/trace/tracer.go @@ -122,12 +122,13 @@ func (tr *tracer) newRecordingSpan(psc, sc trace.SpanContext, name string, sr Sa } s := &recordingSpan{ - // Do not pre-allocate the span slice here to be the max capacity. - // Doing so will allocate memory that is not going to be use if the - // user never adds attributes or does not add attributes to up to the - // capacity. The default Go compiler has been tested to handle - // dynamically allocating needed space when attributes are added in a - // more performant way than we can predetermine here. + // 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 little to now added + // attributes. parent: psc, spanContext: sc, From 3a8e6043e522f5c713879172e0706a7f9bc98fce Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Fri, 4 Feb 2022 15:45:11 -0800 Subject: [PATCH 11/11] Update sdk/trace/tracer.go Co-authored-by: Anthony Mirabella --- sdk/trace/tracer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/trace/tracer.go b/sdk/trace/tracer.go index 08fb0fd5d21..5b8ab43be3d 100644 --- a/sdk/trace/tracer.go +++ b/sdk/trace/tracer.go @@ -127,7 +127,7 @@ func (tr *tracer) newRecordingSpan(psc, sc trace.SpanContext, name string, sr Sa // 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 little to now added + // especially for the common use case of few to no added // attributes. parent: psc,