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 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 { 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 bf0c41c1112..dea73046570 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 @@ -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 map[attribute.Key]attribute.Value + droppedAttributes int // events are stored in FIFO queue capped by configured limit. events evictedQueue @@ -194,11 +200,45 @@ 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 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 + if !a.Valid() { + s.droppedAttributes++ + 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.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. + s.droppedAttributes++ + } else { + s.attributes[a.Key] = a.Value + } + } + } } // End ends the span. This method does nothing if the span is already ended or @@ -387,14 +427,27 @@ 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 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}) + } + return a +} + // 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() + return s.attributesLocked() } // Links returns the links of this span. @@ -463,7 +516,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,9 +566,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() @@ -544,18 +597,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..82ba8a08227 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,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, got.droppedAttributeCount, 1) +} - 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 +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 != "" { @@ -1852,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()) +} diff --git a/sdk/trace/tracer.go b/sdk/trace/tracer.go index b63b4196516..ee4ed410fbf 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,