From 13b53c93e92ba361a3f5d3a3d211fcb47b37f171 Mon Sep 17 00:00:00 2001 From: warnar boekkooi Date: Wed, 2 Oct 2024 07:48:08 +0200 Subject: [PATCH 1/7] trace: SetAttributes only when attributes are provided MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit benchstat: ``` goos: linux goarch: amd64 pkg: go.opentelemetry.io/otel/sdk/trace cpu: 11th Gen Intel(R) Core(TM) i5-11400H @ 2.70GHz │ new.txt │ new1.txt │ │ sec/op │ sec/op vs base │ TraceStart/with_a_simple_span-12 451.0n ± 5% 375.0n ± 1% -16.85% (p=0.000 n=10) TraceStart/with_several_links-12 595.8n ± 3% 501.7n ± 1% -15.80% (p=0.000 n=10) TraceStart/with_attributes-12 644.5n ± 10% 569.5n ± 4% -11.63% (p=0.000 n=10) geomean 557.4n 474.9n -14.79% │ new.txt │ new1.txt │ │ B/op │ B/op vs base │ TraceStart/with_a_simple_span-12 496.0 ± 0% 496.0 ± 0% ~ (p=1.000 n=10) ¹ TraceStart/with_several_links-12 672.0 ± 0% 672.0 ± 0% ~ (p=1.000 n=10) ¹ TraceStart/with_attributes-12 752.0 ± 0% 752.0 ± 0% ~ (p=1.000 n=10) ¹ geomean 630.5 630.5 +0.00% ¹ all samples are equal │ new.txt │ new1.txt │ │ allocs/op │ allocs/op vs base │ TraceStart/with_a_simple_span-12 2.000 ± 0% 2.000 ± 0% ~ (p=1.000 n=10) ¹ TraceStart/with_several_links-12 3.000 ± 0% 3.000 ± 0% ~ (p=1.000 n=10) ¹ TraceStart/with_attributes-12 4.000 ± 0% 4.000 ± 0% ~ (p=1.000 n=10) ¹ geomean 2.884 2.884 +0.00% ¹ all samples are equal ``` --- 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 4945f508303..e71cf41a726 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -210,7 +210,7 @@ func (s *recordingSpan) SetStatus(code codes.Code, description string) { // attributes the span is configured to have, the last added attributes will // be dropped. func (s *recordingSpan) SetAttributes(attributes ...attribute.KeyValue) { - if !s.IsRecording() { + if !s.IsRecording() || len(attributes) == 0 { return } From ef84c7bacc64232fd4397f686a43fbc521e0607c Mon Sep 17 00:00:00 2001 From: warnar boekkooi Date: Wed, 2 Oct 2024 09:06:06 +0200 Subject: [PATCH 2/7] fix: grow s.attributes only when needed Grow increases the slice's capacity, if necessary, to guarantee space for another n elements. This change now ensures that `n` is the amount of elements to grow and not the capacity amount. --- sdk/trace/span.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index e71cf41a726..1790f997c60 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -233,7 +233,7 @@ func (s *recordingSpan) SetAttributes(attributes ...attribute.KeyValue) { // Otherwise, add without deduplication. When attributes are read they // will be deduplicated, optimizing the operation. - s.attributes = slices.Grow(s.attributes, len(s.attributes)+len(attributes)) + s.attributes = slices.Grow(s.attributes, len(attributes)) for _, a := range attributes { if !a.Valid() { // Drop all invalid attributes. @@ -280,13 +280,17 @@ func (s *recordingSpan) addOverCapAttrs(limit int, attrs []attribute.KeyValue) { // 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) + exists := make(map[attribute.Key]int, len(s.attributes)) s.dedupeAttrsFromRecord(&exists) // Now that s.attributes is deduplicated, adding unique attributes up to // the capacity of s will not over allocate s.attributes. - sum := len(attrs) + len(s.attributes) - s.attributes = slices.Grow(s.attributes, min(sum, limit)) + + // max size = limit + maxCap := min(len(attrs)+len(s.attributes), limit) + if cap(s.attributes) < maxCap { + s.attributes = slices.Grow(s.attributes, maxCap-cap(s.attributes)) + } for _, a := range attrs { if !a.Valid() { // Drop all invalid attributes. From 3737aecd50aa0abb47ebabcc2d3cc2fae66c7718 Mon Sep 17 00:00:00 2001 From: warnar boekkooi Date: Wed, 2 Oct 2024 09:25:14 +0200 Subject: [PATCH 3/7] fix: add missing truncateAttr when updating an attribute --- sdk/trace/span.go | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 1790f997c60..2a993c12436 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -300,6 +300,7 @@ func (s *recordingSpan) addOverCapAttrs(limit int, attrs []attribute.KeyValue) { if idx, ok := exists[a.Key]; ok { // Perform all updates before dropping, even when at capacity. + a = truncateAttr(s.tracer.provider.spanLimits.AttributeValueLengthLimit, a) s.attributes[idx] = a continue } From 48156c1c1236c4d0cb002b58634bf6c159e9af1d Mon Sep 17 00:00:00 2001 From: warnar boekkooi Date: Wed, 2 Oct 2024 10:54:32 +0200 Subject: [PATCH 4/7] chore: add changelog entry --- CHANGELOG.md | 1 + sdk/trace/span.go | 12 ++++++------ 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 02d31643d69..9890893fe6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - `Logger.Enabled` in `go.opentelemetry.io/otel/log` now accepts a newly introduced `EnabledParameters` type instead of `Record`. (#5791) - `FilterProcessor.Enabled` in `go.opentelemetry.io/otel/sdk/log/internal/x` now accepts `EnabledParameters` instead of `Record`. (#5791) - The `Record` type in `go.opentelemetry.io/otel/log` is no longer comparable. (#5847) +- Performance improvements for `recordingSpan` `SetAttributes`. (#5864) ### Deprecated diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 2a993c12436..7715ff97bc1 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -281,7 +281,7 @@ func (s *recordingSpan) addOverCapAttrs(limit int, attrs []attribute.KeyValue) { // 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, len(s.attributes)) - s.dedupeAttrsFromRecord(&exists) + s.dedupeAttrsFromRecord(exists) // Now that s.attributes is deduplicated, adding unique attributes up to // the capacity of s will not over allocate s.attributes. @@ -584,23 +584,23 @@ func (s *recordingSpan) Attributes() []attribute.KeyValue { 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) + exists := make(map[attribute.Key]int, len(s.attributes)) + 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) { +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 { + if idx, ok := record[a.Key]; ok { unique[idx] = a } else { unique = append(unique, a) - (*record)[a.Key] = len(unique) - 1 + record[a.Key] = len(unique) - 1 } } // s.attributes have element types of attribute.KeyValue. These types are From f6fd877d5057750543deebe07e5066c6d38d82ff Mon Sep 17 00:00:00 2001 From: warnar boekkooi Date: Fri, 4 Oct 2024 07:56:46 +0200 Subject: [PATCH 5/7] chore: avoid doing multiple locks on SetAttributes --- sdk/trace/span.go | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 7715ff97bc1..25391ec5a8d 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -174,6 +174,16 @@ func (s *recordingSpan) IsRecording() bool { s.mu.Lock() defer s.mu.Unlock() + return s.isRecording() +} + +// isRecording returns if this span is being recorded. If this span has ended +// this will return false. +// This is done without acquiring a lock. +func (s *recordingSpan) isRecording() bool { + if s == nil { + return false + } return s.endTime.IsZero() } @@ -182,11 +192,15 @@ func (s *recordingSpan) IsRecording() bool { // included in the set status when the code is for an error. If this span is // not being recorded than this method does nothing. func (s *recordingSpan) SetStatus(code codes.Code, description string) { - if !s.IsRecording() { + if s == nil { return } + s.mu.Lock() defer s.mu.Unlock() + if !s.isRecording() { + return + } if s.status.Code > code { return } @@ -210,12 +224,15 @@ func (s *recordingSpan) SetStatus(code codes.Code, description string) { // attributes the span is configured to have, the last added attributes will // be dropped. func (s *recordingSpan) SetAttributes(attributes ...attribute.KeyValue) { - if !s.IsRecording() || len(attributes) == 0 { + if s == nil { return } s.mu.Lock() defer s.mu.Unlock() + if !s.isRecording() || len(attributes) == 0 { + return + } limit := s.tracer.provider.spanLimits.AttributeCountLimit if limit == 0 { @@ -523,12 +540,15 @@ func (s *recordingSpan) addEvent(name string, o ...trace.EventOption) { // SetName sets the name of this span. If this span is not being recorded than // this method does nothing. func (s *recordingSpan) SetName(name string) { - if !s.IsRecording() { + if s == nil { return } s.mu.Lock() defer s.mu.Unlock() + if !s.isRecording() { + return + } s.name = name } @@ -760,12 +780,16 @@ func (s *recordingSpan) snapshot() ReadOnlySpan { } func (s *recordingSpan) addChild() { - if !s.IsRecording() { + if s == nil { return } + s.mu.Lock() + defer s.mu.Unlock() + if !s.isRecording() { + return + } s.childSpanCount++ - s.mu.Unlock() } func (*recordingSpan) private() {} From 6185ce39b2ea1e4a273652d9a1691392de49bfa0 Mon Sep 17 00:00:00 2001 From: Warnar Boekkooi Date: Fri, 4 Oct 2024 10:20:45 +0200 Subject: [PATCH 6/7] Update CHANGELOG.md Co-authored-by: Damien Mathieu <42@dmathieu.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9890893fe6f..a78f78b14cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - `Logger.Enabled` in `go.opentelemetry.io/otel/log` now accepts a newly introduced `EnabledParameters` type instead of `Record`. (#5791) - `FilterProcessor.Enabled` in `go.opentelemetry.io/otel/sdk/log/internal/x` now accepts `EnabledParameters` instead of `Record`. (#5791) - The `Record` type in `go.opentelemetry.io/otel/log` is no longer comparable. (#5847) -- Performance improvements for `recordingSpan` `SetAttributes`. (#5864) +- Performance improvements for the trace SDK `SetAttributes` method in `Span`. (#5864) ### Deprecated From 6bc49cf47897ee6877f38fbd82346d80241a15d3 Mon Sep 17 00:00:00 2001 From: warnar boekkooi Date: Fri, 4 Oct 2024 14:48:18 +0200 Subject: [PATCH 7/7] chore: check attributes count before locking --- sdk/trace/span.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 25391ec5a8d..fc8d10d33b7 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -224,13 +224,13 @@ func (s *recordingSpan) SetStatus(code codes.Code, description string) { // attributes the span is configured to have, the last added attributes will // be dropped. func (s *recordingSpan) SetAttributes(attributes ...attribute.KeyValue) { - if s == nil { + if s == nil || len(attributes) == 0 { return } s.mu.Lock() defer s.mu.Unlock() - if !s.isRecording() || len(attributes) == 0 { + if !s.isRecording() { return }