From 71cefcea15a2e5c691d183d3ce409bf3063ebd8c Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 26 Jan 2022 11:28:18 -0800 Subject: [PATCH 01/10] Replace recordingSpan attributes implementation Instead of an LRU strategy for cap-ing span attributes, comply with the specification and drop last added. This means the attributesmap type can be replaced with a simple map. --- sdk/trace/attributesmap.go | 91 ---------------------------- sdk/trace/attributesmap_test.go | 103 -------------------------------- sdk/trace/benchmark_test.go | 23 +++++++ sdk/trace/span.go | 74 +++++++++++++++-------- sdk/trace/trace_test.go | 82 +++++++++++++++++-------- sdk/trace/tracer.go | 4 +- 6 files changed, 132 insertions(+), 245 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() - } -} diff --git a/sdk/trace/benchmark_test.go b/sdk/trace/benchmark_test.go index 335deaf4fc3..3398a457cb4 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" @@ -35,6 +36,28 @@ func BenchmarkStartEndSpan(b *testing.B) { }) } +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 BenchmarkSpanWithAttributes_4(b *testing.B) { traceBenchmark(b, "Benchmark Start With 4 Attributes", func(b *testing.B, t trace.Tracer) { ctx := context.Background() diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 41a68b58551..ba770f5424d 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -137,9 +137,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. 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 map[attribute.Key]attribute.Value + maxAttributes int + droppedAttributes int // events are stored in FIFO queue capped by configured limit. events *evictedQueue @@ -209,7 +214,32 @@ func (s *recordingSpan) SetAttributes(attributes ...attribute.KeyValue) { if !s.IsRecording() { return } - s.copyToCappedAttributes(attributes...) + + 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() { + continue + } + + if _, exists := s.attributes[a.Key]; exists { + // Setting an attribute with the same key as an existing attribute + // SHOULD overwrite the existing attribute's value. + s.attributes[a.Key] = a.Value + } else { + if len(s.attributes) >= s.maxAttributes { + // For each unique attribute key, addition of which would + // result in exceeding the limit, the SDK MUST discard that + // key/value pair. + s.droppedAttributes++ + } else { + s.attributes[a.Key] = a.Value + } + } + } } // End ends the span. This method does nothing if the span is already ended or @@ -398,14 +428,22 @@ func (s *recordingSpan) EndTime() time.Time { return s.endTime } +// attributesLocked returns the attributes s has assuming s.mu.Lock is held by +// the caller. The order of the returned slice is not guaranteed to be +// the same for multiple calls. +func (s *recordingSpan) attributesLocked() []attribute.KeyValue { + a := make([]attribute.KeyValue, 0, len(s.attributes)) + for k, v := range s.attributes { + a = append(a, attribute.KeyValue{Key: k, Value: v}) + } + return a +} + // Attributes returns the attributes of this span. 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() + return s.attributesLocked() } // Links returns the links of this span. @@ -474,7 +512,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 +562,9 @@ 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 { + sd.attributes = s.attributesLocked() + sd.droppedAttributeCount = s.droppedAttributes } if len(s.events.queue) > 0 { sd.events = s.interfaceArrayToEventArray() @@ -555,18 +593,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..635413de667 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -393,7 +393,7 @@ func TestSetSpanAttributesOnStart(t *testing.T) { span := startSpan(tp, "StartSpanAttribute", trace.WithAttributes(attribute.String("key1", "value1")), - trace.WithAttributes(attribute.String("key2", "value2")), + trace.WithAttributes(attribute.String("key1", "value2")), ) got, err := endSpan(te, span) if err != nil { @@ -408,8 +408,7 @@ func TestSetSpanAttributesOnStart(t *testing.T) { parent: sc.WithRemote(true), name: "span0", attributes: []attribute.KeyValue{ - attribute.String("key1", "value1"), - attribute.String("key2", "value2"), + attribute.String("key1", "value2"), }, spanKind: trace.SpanKindInternal, instrumentationLibrary: instrumentation.Library{Name: "StartSpanAttribute"}, @@ -470,39 +469,70 @@ 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]) +} - 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) { diff --git a/sdk/trace/tracer.go b/sdk/trace/tracer.go index 1177c729a8f..2ac6b29562e 100644 --- a/sdk/trace/tracer.go +++ b/sdk/trace/tracer.go @@ -18,6 +18,7 @@ import ( "context" "time" + "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/sdk/instrumentation" "go.opentelemetry.io/otel/trace" ) @@ -127,7 +128,8 @@ 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), + attributes: make(map[attribute.Key]attribute.Value), + maxAttributes: tr.provider.spanLimits.AttributeCountLimit, events: newEvictedQueue(tr.provider.spanLimits.EventCountLimit), links: newEvictedQueue(tr.provider.spanLimits.LinkCountLimit), tracer: tr, From 5e83e4a11fc3727149b565b0d905010ffc46a09d Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Fri, 28 Jan 2022 10:18:51 -0800 Subject: [PATCH 02/10] Update otlptracegrpc client test Do not depend on attributes being returned in a consistent order. --- .../otlptrace/otlptracegrpc/client_test.go | 33 ++++++++++--------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/exporters/otlp/otlptrace/otlptracegrpc/client_test.go b/exporters/otlp/otlptrace/otlptracegrpc/client_test.go index 9a02ca227b6..b21992617dd 100644 --- a/exporters/otlp/otlptrace/otlptracegrpc/client_test.go +++ b/exporters/otlp/otlptrace/otlptracegrpc/client_test.go @@ -17,6 +17,7 @@ import ( "context" "fmt" "net" + "sort" "strings" "testing" "time" @@ -297,34 +298,34 @@ func TestNew_withMultipleAttributeTypes(t *testing.T) { expected := []*commonpb.KeyValue{ { - Key: "Int", + Key: "Bool", Value: &commonpb.AnyValue{ - Value: &commonpb.AnyValue_IntValue{ - IntValue: 1, + Value: &commonpb.AnyValue_BoolValue{ + BoolValue: true, }, }, }, { - Key: "Int64", + Key: "Float64", Value: &commonpb.AnyValue{ - Value: &commonpb.AnyValue_IntValue{ - IntValue: 3, + Value: &commonpb.AnyValue_DoubleValue{ + DoubleValue: 2.22, }, }, }, { - Key: "Float64", + Key: "Int", Value: &commonpb.AnyValue{ - Value: &commonpb.AnyValue_DoubleValue{ - DoubleValue: 2.22, + Value: &commonpb.AnyValue_IntValue{ + IntValue: 1, }, }, }, { - Key: "Bool", + Key: "Int64", Value: &commonpb.AnyValue{ - Value: &commonpb.AnyValue_BoolValue{ - BoolValue: true, + Value: &commonpb.AnyValue_IntValue{ + IntValue: 3, }, }, }, @@ -339,10 +340,10 @@ func TestNew_withMultipleAttributeTypes(t *testing.T) { } // Verify attributes - if !assert.Len(t, rss[0].Attributes, len(expected)) { - t.Fatalf("attributes count: got %d, want %d\n", len(rss[0].Attributes), len(expected)) - } - for i, actual := range rss[0].Attributes { + attr := rss[0].Attributes + require.Len(t, attr, len(expected)) + sort.Slice(attr, func(i, j int) bool { return attr[i].Key < attr[j].Key }) + for i, actual := range attr { if a, ok := actual.Value.Value.(*commonpb.AnyValue_DoubleValue); ok { e, ok := expected[i].Value.Value.(*commonpb.AnyValue_DoubleValue) if !ok { From 4e1055dd5995c4079e16337b854808a46732f35a Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Fri, 28 Jan 2022 10:39:57 -0800 Subject: [PATCH 03/10] Add changes to changelog --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb91daf7486..60c686ff1de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Changed - Jaeger exporter takes into additional 70 bytes overhead into consideration when sending UDP packets (#2489, #2512) +- The attributes returned from the `ReadOnlySpan` and `ReadWriteSpan` in `go.opentelemetry.io/otel/sdk/trace` are unordered. + Multiple calls to retrieve these attributes will return a slice of them that is not guaranteed to be in the same order. + If these attributes need to be consistently ordered, the `sort` package can be used. + Given these attributes will have unique keys `sort.Slice(attr, func(i, j int) bool { return attr[i].Key < attr[j].Key })` can be used to sort `attr` stably. (#2555) ### Deprecated @@ -30,6 +34,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. (#2555) ### Removed From 0de495f8db0effbb94622213bda494c8b4a0663d Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Fri, 28 Jan 2022 10:46:21 -0800 Subject: [PATCH 04/10] Update sdk/trace span docs on attr instability --- sdk/trace/span.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 0923318ae11..f080d766839 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 attribute is not guaranteed to be stable. Attributes() []attribute.KeyValue // Links returns all the links the span has to other spans. Links() []Link @@ -210,6 +211,10 @@ 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 @@ -429,8 +434,7 @@ func (s *recordingSpan) EndTime() time.Time { } // attributesLocked returns the attributes s has assuming s.mu.Lock is held by -// the caller. The order of the returned slice is not guaranteed to be -// the same for multiple calls. +// the caller. The order of the returned slice is not guaranteed to be stable. func (s *recordingSpan) attributesLocked() []attribute.KeyValue { a := make([]attribute.KeyValue, 0, len(s.attributes)) for k, v := range s.attributes { @@ -440,6 +444,8 @@ func (s *recordingSpan) attributesLocked() []attribute.KeyValue { } // 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() From 1452d8c412fc770e5e4333adab486f22e1a2a32a Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Fri, 28 Jan 2022 14:06:12 -0800 Subject: [PATCH 05/10] Update sdk/trace/span.go Co-authored-by: Anthony Mirabella --- sdk/trace/span.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index f080d766839..1740634c545 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -54,7 +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 attribute is not guaranteed to be stable. + // 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 From dd5aaff051daa3cbf351afe646023ba8835b932d Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Mon, 31 Jan 2022 08:34:10 -0800 Subject: [PATCH 06/10] Count invalid attrs as dropped --- sdk/trace/span.go | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 1740634c545..f990eb1faf5 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -227,6 +227,7 @@ func (s *recordingSpan) SetAttributes(attributes ...attribute.KeyValue) { // 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 } From c3f08f4d55eade2f2bac01a90372a46dc812e804 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Mon, 31 Jan 2022 08:44:28 -0800 Subject: [PATCH 07/10] Update trace tests for dropped count --- sdk/trace/trace_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sdk/trace/trace_test.go b/sdk/trace/trace_test.go index 635413de667..91d1247ea21 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -495,6 +495,7 @@ func TestSetSpanAttributesOverLimit(t *testing.T) { } assert.Contains(t, got.attributes, attrs[1]) assert.Contains(t, got.attributes, attrs[2]) + assert.Equal(t, got.droppedAttributeCount, 1) } func TestSpanAttributeCapacityDropOrder(t *testing.T) { @@ -560,7 +561,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 != "" { From 0639ef90085cd78f0d64e8eaaa43eb83dbacddc8 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Mon, 31 Jan 2022 13:05:49 -0800 Subject: [PATCH 08/10] Remove maxAttributes field from recordingSpan Avoid the duplicate reference to the value held by the parent TracerProvider and just refer to that directly when needed. --- sdk/trace/span.go | 12 ++++++------ sdk/trace/tracer.go | 1 - 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index f990eb1faf5..b9837f4cedd 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -139,12 +139,12 @@ type recordingSpan struct { spanContext trace.SpanContext // attributes is a collection of user provided key/values. The collection - // is constrained by a configurable maximum. 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. + // 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 map[attribute.Key]attribute.Value - maxAttributes int droppedAttributes int // events are stored in FIFO queue capped by configured limit. @@ -236,7 +236,7 @@ func (s *recordingSpan) SetAttributes(attributes ...attribute.KeyValue) { // SHOULD overwrite the existing attribute's value. s.attributes[a.Key] = a.Value } else { - if len(s.attributes) >= s.maxAttributes { + if len(s.attributes) >= s.tracer.provider.spanLimits.AttributeCountLimit { // For each unique attribute key, addition of which would // result in exceeding the limit, the SDK MUST discard that // key/value pair. diff --git a/sdk/trace/tracer.go b/sdk/trace/tracer.go index 2ac6b29562e..824c55fa760 100644 --- a/sdk/trace/tracer.go +++ b/sdk/trace/tracer.go @@ -129,7 +129,6 @@ func (tr *tracer) newRecordingSpan(psc, sc trace.SpanContext, name string, sr Sa name: name, startTime: startTime, attributes: make(map[attribute.Key]attribute.Value), - maxAttributes: tr.provider.spanLimits.AttributeCountLimit, events: newEvictedQueue(tr.provider.spanLimits.EventCountLimit), links: newEvictedQueue(tr.provider.spanLimits.LinkCountLimit), tracer: tr, From b1b4ce4125c6b1f9f5b2bde990954152b41d8690 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 1 Feb 2022 13:08:28 -0800 Subject: [PATCH 09/10] Lazy allocate recordingSpan attributes map Only allocate if attributes are recorded. --- sdk/trace/span.go | 8 ++++++++ sdk/trace/tracer.go | 2 -- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index b9837f4cedd..7e7dc8ae597 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -223,6 +223,10 @@ func (s *recordingSpan) SetAttributes(attributes ...attribute.KeyValue) { s.mu.Lock() defer s.mu.Unlock() + if s.attributes == nil { + s.attributes = make(map[attribute.Key]attribute.Value) + } + 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 @@ -437,6 +441,10 @@ func (s *recordingSpan) EndTime() time.Time { // attributesLocked returns the attributes s has assuming s.mu.Lock is held by // the caller. The order of the returned slice is not guaranteed to be stable. func (s *recordingSpan) attributesLocked() []attribute.KeyValue { + if s.attributes == nil { + return nil + } + a := make([]attribute.KeyValue, 0, len(s.attributes)) for k, v := range s.attributes { a = append(a, attribute.KeyValue{Key: k, Value: v}) diff --git a/sdk/trace/tracer.go b/sdk/trace/tracer.go index 824c55fa760..c12e0f8787b 100644 --- a/sdk/trace/tracer.go +++ b/sdk/trace/tracer.go @@ -18,7 +18,6 @@ import ( "context" "time" - "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/sdk/instrumentation" "go.opentelemetry.io/otel/trace" ) @@ -128,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: make(map[attribute.Key]attribute.Value), events: newEvictedQueue(tr.provider.spanLimits.EventCountLimit), links: newEvictedQueue(tr.provider.spanLimits.LinkCountLimit), tracer: tr, From a7d714ec9350617d6f97d4008ebf92317cfff235 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Wed, 2 Feb 2022 08:33:16 -0800 Subject: [PATCH 10/10] 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 91d1247ea21..82ba8a08227 100644 --- a/sdk/trace/trace_test.go +++ b/sdk/trace/trace_test.go @@ -1883,3 +1883,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()) +}