From 35e2dd62c25dbe57854a1ec15ffd2f411d39a096 Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Tue, 23 Sep 2025 18:29:02 +0200 Subject: [PATCH 01/20] Remove Skip fromTestRecordMethodsInputConcurrentSafe --- sdk/log/record_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/sdk/log/record_test.go b/sdk/log/record_test.go index c6ffbd16b6a..b0933b34da0 100644 --- a/sdk/log/record_test.go +++ b/sdk/log/record_test.go @@ -927,10 +927,6 @@ func TestTruncate(t *testing.T) { } func TestRecordMethodsInputConcurrentSafe(t *testing.T) { - if race() { - t.Skip("TODO: Fix bug https://github.com/open-telemetry/opentelemetry-go/issues/7364.") - } - nestedSlice := log.Slice("nested_slice", log.SliceValue(log.StringValue("nested_inner1"), log.StringValue("nested_inner2")), log.StringValue("nested_outer"), From b70173baaf1bc78fcb3ee7194f7b7c28aeeb8d9a Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Tue, 23 Sep 2025 19:18:33 +0200 Subject: [PATCH 02/20] Fix AddAttributes, SetAttributes, SetBody to not mutate the input --- sdk/log/record.go | 269 ++++++++++++++++++++++++++++++++--------- sdk/log/record_test.go | 2 +- 2 files changed, 216 insertions(+), 55 deletions(-) diff --git a/sdk/log/record.go b/sdk/log/record.go index 9dfd69b645b..b8bb15c284b 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -216,42 +216,52 @@ func (r *Record) AddAttributes(attrs ...log.KeyValue) { rIndex := r.attrIndex() defer putIndex(rIndex) - // Unique attrs that need to be added to r. This uses the same underlying - // array as attrs. - // - // Note, do not iterate attrs twice by just calling dedup(attrs) here. - unique := attrs[:0] - // Used to find duplicates within attrs itself. The index value is the - // index of the element in unique. - uIndex := getIndex() - defer putIndex(uIndex) - - // Deduplicate attrs within the scope of all existing attributes. + // Check if deduplication is actually needed. + needsDedup := false + seen := make(map[string]bool) for _, a := range attrs { - // Last-value-wins for any duplicates in attrs. - idx, found := uIndex[a.Key] - if found { - r.addDropped(1) - unique[idx] = a - continue + if seen[a.Key] || rIndex[a.Key] != 0 { + needsDedup = true + break } + seen[a.Key] = true + } - idx, found = rIndex[a.Key] - if found { - // New attrs overwrite any existing with the same key. - r.addDropped(1) - if idx < 0 { - r.front[-(idx + 1)] = a + if needsDedup { + // Create a new slice to avoid modifying the original. + unique := make([]log.KeyValue, 0, len(attrs)) + // Used to find duplicates within attrs itself. + // The index value is the index of the element in unique. + uIndex := getIndex() + defer putIndex(uIndex) + + // Deduplicate attrs within the scope of all existing attributes. + for _, a := range attrs { + // Last-value-wins for any duplicates in attrs. + idx, found := uIndex[a.Key] + if found { + r.addDropped(1) + unique[idx] = a + continue + } + + idx, found = rIndex[a.Key] + if found { + // New attrs overwrite any existing with the same key. + r.addDropped(1) + if idx < 0 { + r.front[-(idx + 1)] = a + } else { + r.back[idx] = a + } } else { - r.back[idx] = a + // Unique attribute. + unique = append(unique, a) + uIndex[a.Key] = len(unique) - 1 } - } else { - // Unique attribute. - unique = append(unique, a) - uIndex[a.Key] = len(unique) - 1 } + attrs = unique } - attrs = unique } if r.attributeCountLimit > 0 && n+len(attrs) > r.attributeCountLimit { @@ -294,12 +304,12 @@ func (r *Record) addAttrs(attrs []log.KeyValue) { var i int for i = 0; i < len(attrs) && r.nFront < len(r.front); i++ { a := attrs[i] - r.front[r.nFront] = r.applyAttrLimits(a) + r.front[r.nFront] = r.applyAttrLimitsAndDedup(a) r.nFront++ } for j, a := range attrs[i:] { - attrs[i+j] = r.applyAttrLimits(a) + attrs[i+j] = r.applyAttrLimitsAndDedup(a) } r.back = slices.Grow(r.back, len(attrs[i:])) r.back = append(r.back, attrs[i:]...) @@ -321,13 +331,13 @@ func (r *Record) SetAttributes(attrs ...log.KeyValue) { var i int for i = 0; i < len(attrs) && r.nFront < len(r.front); i++ { a := attrs[i] - r.front[r.nFront] = r.applyAttrLimits(a) + r.front[r.nFront] = r.applyAttrLimitsAndDedup(a) r.nFront++ } r.back = slices.Clone(attrs[i:]) for i, a := range r.back { - r.back[i] = r.applyAttrLimits(a) + r.back[i] = r.applyAttrLimitsAndDedup(a) } } @@ -342,10 +352,30 @@ func head(kvs []log.KeyValue, n int) (out []log.KeyValue, dropped int) { // dedup deduplicates kvs front-to-back with the last value saved. func dedup(kvs []log.KeyValue) (unique []log.KeyValue, dropped int) { + if len(kvs) <= 1 { + return kvs, 0 // No deduplication needed + } + + // Check if deduplication is actually needed by looking for duplicate keys + seen := make(map[string]bool) + hasDuplicates := false + for _, kv := range kvs { + if seen[kv.Key] { + hasDuplicates = true + break + } + seen[kv.Key] = true + } + + if !hasDuplicates { + return kvs, 0 // No deduplication needed. + } + + // Deduplication is needed, create a new slice to avoid modifying the original index := getIndex() defer putIndex(index) - unique = kvs[:0] // Use the same underlying array as kvs. + unique = make([]log.KeyValue, 0, len(kvs)) for _, a := range kvs { idx, found := index[a.Key] if found { @@ -421,12 +451,12 @@ func (r *Record) Clone() Record { return res } -func (r *Record) applyAttrLimits(attr log.KeyValue) log.KeyValue { - attr.Value = r.applyValueLimits(attr.Value) +func (r *Record) applyAttrLimitsAndDedup(attr log.KeyValue) log.KeyValue { + attr.Value = r.applyValueLimitsAndDedup(attr.Value) return attr } -func (r *Record) applyValueLimits(val log.Value) log.Value { +func (r *Record) applyValueLimitsAndDedup(val log.Value) log.Value { switch val.Kind() { case log.KindString: s := val.AsString() @@ -435,45 +465,176 @@ func (r *Record) applyValueLimits(val log.Value) log.Value { } case log.KindSlice: sl := val.AsSlice() - for i := range sl { - sl[i] = r.applyValueLimits(sl[i]) + + // First check if any limits need to be applied. + needsChange := slices.ContainsFunc(sl, r.needsValueLimitsOrDedup) + + if needsChange { + // Create a new slice to avoid modifying the original + newSl := make([]log.Value, len(sl)) + for i, item := range sl { + newSl[i] = r.applyValueLimitsAndDedup(item) + } + val = log.SliceValue(newSl...) } - val = log.SliceValue(sl...) + case log.KindMap: kvs := val.AsMap() + var newKvs []log.KeyValue + var dropped int + if !r.allowDupKeys { - // Deduplicate then truncate. Do not do at the same time to avoid - // wasted truncation operations. - var dropped int - kvs, dropped = dedup(kvs) + // Deduplicate then truncate. + // Do not do at the same time to avoid wasted truncation operations. + newKvs, dropped = dedup(kvs) r.addDropped(dropped) + } else { + newKvs = kvs } - for i := range kvs { - kvs[i] = r.applyAttrLimits(kvs[i]) + + // Check if any attribute limits need to be applied. + needsChange := false + if dropped > 0 { + needsChange = true // Already changed by dedup + } else { + for _, kv := range newKvs { + if r.needsValueLimitsOrDedup(kv.Value) { + needsChange = true + break + } + } + } + + if needsChange { + // Only create new slice if changes are needed. + if dropped == 0 { + // Make a copy to avoid modifying the original. + newKvs = make([]log.KeyValue, len(kvs)) + copy(newKvs, kvs) + } + + for i := range newKvs { + newKvs[i] = r.applyAttrLimitsAndDedup(newKvs[i]) + } + val = log.MapValue(newKvs...) } - val = log.MapValue(kvs...) } return val } +// needsValueLimitsOrDedup checks if a value would be modified by applyValueLimitsAndDedup. +func (r *Record) needsValueLimitsOrDedup(val log.Value) bool { + switch val.Kind() { + case log.KindString: + return len(val.AsString()) > r.attributeValueLengthLimit + case log.KindSlice: + if slices.ContainsFunc(val.AsSlice(), r.needsValueLimitsOrDedup) { + return true + } + case log.KindMap: + kvs := val.AsMap() + if !r.allowDupKeys && len(kvs) > 1 { + // Check for duplicates + seen := make(map[string]bool) + for _, kv := range kvs { + if seen[kv.Key] { + return true + } + seen[kv.Key] = true + } + } + for _, kv := range kvs { + if r.needsValueLimitsOrDedup(kv.Value) { + return true + } + } + } + return false +} + func (r *Record) dedupeBodyCollections(val log.Value) log.Value { switch val.Kind() { case log.KindSlice: sl := val.AsSlice() - for i := range sl { - sl[i] = r.dedupeBodyCollections(sl[i]) + + // Check if any nested values need deduplication. + needsChange := false + for _, item := range sl { + if r.needsBodyDedup(item) { + needsChange = true + break + } } - val = log.SliceValue(sl...) + + if needsChange { + // Create a new slice to avoid modifying the original. + newSl := make([]log.Value, len(sl)) + for i, item := range sl { + newSl[i] = r.dedupeBodyCollections(item) + } + val = log.SliceValue(newSl...) + } + case log.KindMap: - kvs, _ := dedup(val.AsMap()) - for i := range kvs { - kvs[i].Value = r.dedupeBodyCollections(kvs[i].Value) + kvs := val.AsMap() + newKvs, dropped := dedup(kvs) + + // Check if any nested values need deduplication. + needsValueChange := false + for _, kv := range newKvs { + if r.needsBodyDedup(kv.Value) { + needsValueChange = true + break + } + } + + if dropped > 0 || needsValueChange { + // Only create new value if changes are needed. + if dropped == 0 { + // Make a copy to avoid modifying the original. + newKvs = make([]log.KeyValue, len(kvs)) + copy(newKvs, kvs) + } + + for i := range newKvs { + newKvs[i].Value = r.dedupeBodyCollections(newKvs[i].Value) + } + val = log.MapValue(newKvs...) } - val = log.MapValue(kvs...) } return val } +// needsBodyDedup checks if a value would be modified by dedupeBodyCollections. +func (r *Record) needsBodyDedup(val log.Value) bool { + switch val.Kind() { + case log.KindSlice: + for _, item := range val.AsSlice() { + if r.needsBodyDedup(item) { + return true + } + } + case log.KindMap: + kvs := val.AsMap() + if len(kvs) > 1 { + // Check for duplicates. + seen := make(map[string]bool) + for _, kv := range kvs { + if seen[kv.Key] { + return true + } + seen[kv.Key] = true + } + } + for _, kv := range kvs { + if r.needsBodyDedup(kv.Value) { + return true + } + } + } + return false +} + // truncate returns a truncated version of s such that it contains less than // the limit number of characters. Truncation is applied by returning the limit // number of valid characters contained in s. diff --git a/sdk/log/record_test.go b/sdk/log/record_test.go index b0933b34da0..8a652a31f9c 100644 --- a/sdk/log/record_test.go +++ b/sdk/log/record_test.go @@ -986,7 +986,7 @@ func TestRecordMethodsInputConcurrentSafe(t *testing.T) { gotBody := r.Body() wantBody := log.MapValue( log.String("nested_key1", "duplicate"), - log.Map("nested_map", log.String("nested_inner_key", "nested_inn")), + log.Map("nested_map", log.String("nested_inner_key", "nested_inner_value")), ) if !gotBody.Equal(wantBody) { t.Errorf("Body does not match.\ngot:\n%v\nwant:\n%v", gotBody, wantBody) From 77e28ee18f960e99645ced40a52df6d1da44358a Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Tue, 23 Sep 2025 19:18:56 +0200 Subject: [PATCH 03/20] add missing dot --- sdk/log/record.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/log/record.go b/sdk/log/record.go index b8bb15c284b..d6555c303e3 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -534,7 +534,7 @@ func (r *Record) needsValueLimitsOrDedup(val log.Value) bool { case log.KindMap: kvs := val.AsMap() if !r.allowDupKeys && len(kvs) > 1 { - // Check for duplicates + // Check for duplicates. seen := make(map[string]bool) for _, kv := range kvs { if seen[kv.Key] { From 66f2a617bd095e88912b18f3dfc461cc4cf239a8 Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Tue, 23 Sep 2025 19:26:10 +0200 Subject: [PATCH 04/20] Use sync.Pool for seen maps --- sdk/log/record.go | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/sdk/log/record.go b/sdk/log/record.go index d6555c303e3..e25f489b98e 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -41,6 +41,20 @@ func putIndex(index map[string]int) { indexPool.Put(index) } +// seenPool is a pool of boolean maps used for duplicate checking. +var seenPool = sync.Pool{ + New: func() any { return make(map[string]bool) }, +} + +func getSeen() map[string]bool { + return seenPool.Get().(map[string]bool) +} + +func putSeen(seen map[string]bool) { + clear(seen) + seenPool.Put(seen) +} + // Record is a log record emitted by the Logger. // A log record with non-empty event name is interpreted as an event record. // @@ -218,7 +232,8 @@ func (r *Record) AddAttributes(attrs ...log.KeyValue) { // Check if deduplication is actually needed. needsDedup := false - seen := make(map[string]bool) + seen := getSeen() + defer putSeen(seen) for _, a := range attrs { if seen[a.Key] || rIndex[a.Key] != 0 { needsDedup = true @@ -357,7 +372,8 @@ func dedup(kvs []log.KeyValue) (unique []log.KeyValue, dropped int) { } // Check if deduplication is actually needed by looking for duplicate keys - seen := make(map[string]bool) + seen := getSeen() + defer putSeen(seen) hasDuplicates := false for _, kv := range kvs { if seen[kv.Key] { @@ -535,7 +551,8 @@ func (r *Record) needsValueLimitsOrDedup(val log.Value) bool { kvs := val.AsMap() if !r.allowDupKeys && len(kvs) > 1 { // Check for duplicates. - seen := make(map[string]bool) + seen := getSeen() + defer putSeen(seen) for _, kv := range kvs { if seen[kv.Key] { return true @@ -618,7 +635,8 @@ func (r *Record) needsBodyDedup(val log.Value) bool { kvs := val.AsMap() if len(kvs) > 1 { // Check for duplicates. - seen := make(map[string]bool) + seen := getSeen() + defer putSeen(seen) for _, kv := range kvs { if seen[kv.Key] { return true From 97ddd0714d55c4b3a43fdadf3994f580a0f0d871 Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Tue, 23 Sep 2025 19:30:35 +0200 Subject: [PATCH 05/20] remove unused race function --- sdk/log/race_off_test.go | 11 ----------- sdk/log/race_on_test.go | 11 ----------- 2 files changed, 22 deletions(-) delete mode 100644 sdk/log/race_off_test.go delete mode 100644 sdk/log/race_on_test.go diff --git a/sdk/log/race_off_test.go b/sdk/log/race_off_test.go deleted file mode 100644 index 6479bf9c54c..00000000000 --- a/sdk/log/race_off_test.go +++ /dev/null @@ -1,11 +0,0 @@ -// Copyright The OpenTelemetry Authors -// SPDX-License-Identifier: Apache-2.0 - -//go:build !race -// +build !race - -package log - -func race() bool { - return false -} diff --git a/sdk/log/race_on_test.go b/sdk/log/race_on_test.go deleted file mode 100644 index c726aa8a5cd..00000000000 --- a/sdk/log/race_on_test.go +++ /dev/null @@ -1,11 +0,0 @@ -// Copyright The OpenTelemetry Authors -// SPDX-License-Identifier: Apache-2.0 - -//go:build race -// +build race - -package log - -func race() bool { - return true -} From 33db4994dc642194ac5c699e93a235504ec2a125 Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Tue, 23 Sep 2025 19:33:17 +0200 Subject: [PATCH 06/20] Add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a17cf4dce5..98c515b18d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Return partial OTLP export errors to the caller in `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp`. (#7372) - Return partial OTLP export errors to the caller in `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc`. (#7372) - Return partial OTLP export errors to the caller in `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp`. (#7372) +- Fix `AddAttributes`, `SetAttributes`, `SetBody` on `Record` in `go.opentelemetry.io/otel/sdk/log` to not mutate input. (#7403) ### Removed From 40412d1a2b58185aa14b7c8291d890bbe8f0da36 Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Tue, 23 Sep 2025 19:34:44 +0200 Subject: [PATCH 07/20] add dots --- sdk/log/record.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sdk/log/record.go b/sdk/log/record.go index e25f489b98e..8bf8ef1b86e 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -368,10 +368,10 @@ func head(kvs []log.KeyValue, n int) (out []log.KeyValue, dropped int) { // dedup deduplicates kvs front-to-back with the last value saved. func dedup(kvs []log.KeyValue) (unique []log.KeyValue, dropped int) { if len(kvs) <= 1 { - return kvs, 0 // No deduplication needed + return kvs, 0 // No deduplication needed. } - // Check if deduplication is actually needed by looking for duplicate keys + // Check if deduplication is actually needed by looking for duplicate keys. seen := getSeen() defer putSeen(seen) hasDuplicates := false @@ -387,7 +387,7 @@ func dedup(kvs []log.KeyValue) (unique []log.KeyValue, dropped int) { return kvs, 0 // No deduplication needed. } - // Deduplication is needed, create a new slice to avoid modifying the original + // Deduplication is needed, create a new slice to avoid modifying the original. index := getIndex() defer putIndex(index) From defde16fa7cc8e9d18e6c102f1f558aa1e6e1c56 Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Tue, 23 Sep 2025 19:36:12 +0200 Subject: [PATCH 08/20] add missing dot --- sdk/log/record.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/log/record.go b/sdk/log/record.go index 8bf8ef1b86e..dd4822ab209 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -486,7 +486,7 @@ func (r *Record) applyValueLimitsAndDedup(val log.Value) log.Value { needsChange := slices.ContainsFunc(sl, r.needsValueLimitsOrDedup) if needsChange { - // Create a new slice to avoid modifying the original + // Create a new slice to avoid modifying the original. newSl := make([]log.Value, len(sl)) for i, item := range sl { newSl[i] = r.applyValueLimitsAndDedup(item) @@ -511,7 +511,7 @@ func (r *Record) applyValueLimitsAndDedup(val log.Value) log.Value { // Check if any attribute limits need to be applied. needsChange := false if dropped > 0 { - needsChange = true // Already changed by dedup + needsChange = true // Already changed by dedup. } else { for _, kv := range newKvs { if r.needsValueLimitsOrDedup(kv.Value) { From 25cd8f8603d6ea776910be7e08761d0a3b2d9683 Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Tue, 23 Sep 2025 19:56:25 +0200 Subject: [PATCH 09/20] limits should only take effect when the value is not negative --- sdk/log/record.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/log/record.go b/sdk/log/record.go index dd4822ab209..dbe748419e0 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -476,7 +476,7 @@ func (r *Record) applyValueLimitsAndDedup(val log.Value) log.Value { switch val.Kind() { case log.KindString: s := val.AsString() - if len(s) > r.attributeValueLengthLimit { + if r.attributeValueLengthLimit >= 0 && len(s) > r.attributeValueLengthLimit { val = log.StringValue(truncate(r.attributeValueLengthLimit, s)) } case log.KindSlice: @@ -542,7 +542,7 @@ func (r *Record) applyValueLimitsAndDedup(val log.Value) log.Value { func (r *Record) needsValueLimitsOrDedup(val log.Value) bool { switch val.Kind() { case log.KindString: - return len(val.AsString()) > r.attributeValueLengthLimit + return r.attributeValueLengthLimit >= 0 && len(val.AsString()) > r.attributeValueLengthLimit case log.KindSlice: if slices.ContainsFunc(val.AsSlice(), r.needsValueLimitsOrDedup) { return true From 9976991e375a6c496723228953749517f69d47d0 Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Tue, 23 Sep 2025 20:12:33 +0200 Subject: [PATCH 10/20] Faster pool resource return --- sdk/log/record.go | 55 +++++++++++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 21 deletions(-) diff --git a/sdk/log/record.go b/sdk/log/record.go index dbe748419e0..307b9800551 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -372,16 +372,17 @@ func dedup(kvs []log.KeyValue) (unique []log.KeyValue, dropped int) { } // Check if deduplication is actually needed by looking for duplicate keys. - seen := getSeen() - defer putSeen(seen) - hasDuplicates := false - for _, kv := range kvs { - if seen[kv.Key] { - hasDuplicates = true - break + hasDuplicates := func() bool { + seen := getSeen() + defer putSeen(seen) + for _, kv := range kvs { + if seen[kv.Key] { + return true + } + seen[kv.Key] = true } - seen[kv.Key] = true - } + return false + }() if !hasDuplicates { return kvs, 0 // No deduplication needed. @@ -551,13 +552,19 @@ func (r *Record) needsValueLimitsOrDedup(val log.Value) bool { kvs := val.AsMap() if !r.allowDupKeys && len(kvs) > 1 { // Check for duplicates. - seen := getSeen() - defer putSeen(seen) - for _, kv := range kvs { - if seen[kv.Key] { - return true + hasDuplicates := func() bool { + seen := getSeen() + defer putSeen(seen) + for _, kv := range kvs { + if seen[kv.Key] { + return true + } + seen[kv.Key] = true } - seen[kv.Key] = true + return false + }() + if hasDuplicates { + return true } } for _, kv := range kvs { @@ -635,13 +642,19 @@ func (r *Record) needsBodyDedup(val log.Value) bool { kvs := val.AsMap() if len(kvs) > 1 { // Check for duplicates. - seen := getSeen() - defer putSeen(seen) - for _, kv := range kvs { - if seen[kv.Key] { - return true + hasDuplicates := func() bool { + seen := getSeen() + defer putSeen(seen) + for _, kv := range kvs { + if seen[kv.Key] { + return true + } + seen[kv.Key] = true } - seen[kv.Key] = true + return false + }() + if hasDuplicates { + return true } } for _, kv := range kvs { From bd9150f956e6507ee7cb4a51a26d4c865ebdf3c3 Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Tue, 23 Sep 2025 20:17:41 +0200 Subject: [PATCH 11/20] Faster pool resource return cont --- sdk/log/record.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/sdk/log/record.go b/sdk/log/record.go index 307b9800551..54c7f69f31a 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -231,16 +231,17 @@ func (r *Record) AddAttributes(attrs ...log.KeyValue) { defer putIndex(rIndex) // Check if deduplication is actually needed. - needsDedup := false - seen := getSeen() - defer putSeen(seen) - for _, a := range attrs { - if seen[a.Key] || rIndex[a.Key] != 0 { - needsDedup = true - break + needsDedup := func() bool { + seen := getSeen() + defer putSeen(seen) + for _, a := range attrs { + if seen[a.Key] || rIndex[a.Key] != 0 { + return true + } + seen[a.Key] = true } - seen[a.Key] = true - } + return false + }() if needsDedup { // Create a new slice to avoid modifying the original. From efa0d94d1301144a79b0f6028f632ce1a939059a Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Tue, 23 Sep 2025 21:47:36 +0200 Subject: [PATCH 12/20] Add missing test cases --- sdk/log/record_test.go | 420 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 418 insertions(+), 2 deletions(-) diff --git a/sdk/log/record_test.go b/sdk/log/record_test.go index 8a652a31f9c..b997cbf9d6a 100644 --- a/sdk/log/record_test.go +++ b/sdk/log/record_test.go @@ -66,7 +66,7 @@ func TestRecordBody(t *testing.T) { want log.Value }{ { - name: "Bool", + name: "boolean value", body: log.BoolValue(true), want: log.BoolValue(true), }, @@ -98,7 +98,7 @@ func TestRecordBody(t *testing.T) { ), }, { - name: "nestedMap", + name: "nested map", body: log.MapValue( log.Map("key", log.Int64("key", 1), @@ -123,6 +123,268 @@ func TestRecordBody(t *testing.T) { log.Int64("1", 3), ), }, + { + name: "slice with nested deduplication", + body: log.SliceValue( + log.MapValue(log.String("key", "value1"), log.String("key", "value2")), + log.StringValue("normal"), + log.SliceValue( + log.MapValue(log.String("nested", "val1"), log.String("nested", "val2")), + ), + ), + want: log.SliceValue( + log.MapValue(log.String("key", "value2")), + log.StringValue("normal"), + log.SliceValue( + log.MapValue(log.String("nested", "val2")), + ), + ), + }, + { + name: "empty slice", + body: log.SliceValue(), + want: log.SliceValue(), + }, + { + name: "empty map", + body: log.MapValue(), + want: log.MapValue(), + }, + { + name: "single key map", + body: log.MapValue(log.String("single", "value")), + want: log.MapValue(log.String("single", "value")), + }, + { + name: "slice with no deduplication needed", + body: log.SliceValue( + log.StringValue("value1"), + log.StringValue("value2"), + log.MapValue(log.String("unique1", "val1")), + log.MapValue(log.String("unique2", "val2")), + ), + want: log.SliceValue( + log.StringValue("value1"), + log.StringValue("value2"), + log.MapValue(log.String("unique1", "val1")), + log.MapValue(log.String("unique2", "val2")), + ), + }, + { + name: "deeply nested slice and map structure", + body: log.SliceValue( + log.MapValue( + log.String("outer", "value"), + log.Slice("inner_slice", + log.MapValue(log.String("deep", "value1"), log.String("deep", "value2")), + ), + ), + ), + want: log.SliceValue( + log.MapValue( + log.String("outer", "value"), + log.Slice("inner_slice", + log.MapValue(log.String("deep", "value2")), + ), + ), + ), + }, + { + name: "slice with duplicates allowed", + allowDuplicates: true, + body: log.SliceValue( + log.MapValue(log.String("key", "value1"), log.String("key", "value2")), + ), + want: log.SliceValue( + log.MapValue(log.String("key", "value1"), log.String("key", "value2")), + ), + }, + { + name: "string value", + body: log.StringValue("test"), + want: log.StringValue("test"), + }, + { + name: "boolean value without deduplication", + body: log.BoolValue(true), + want: log.BoolValue(true), + }, + { + name: "integer value", + body: log.Int64Value(42), + want: log.Int64Value(42), + }, + { + name: "float value", + body: log.Float64Value(3.14), + want: log.Float64Value(3.14), + }, + { + name: "bytes value", + body: log.BytesValue([]byte("test")), + want: log.BytesValue([]byte("test")), + }, + { + name: "empty slice", + body: log.SliceValue(), + want: log.SliceValue(), + }, + { + name: "slice without nested deduplication", + body: log.SliceValue(log.StringValue("test"), log.BoolValue(true)), + want: log.SliceValue(log.StringValue("test"), log.BoolValue(true)), + }, + { + name: "slice with nested deduplication needed", + body: log.SliceValue(log.MapValue(log.String("key", "value1"), log.String("key", "value2"))), + want: log.SliceValue(log.MapValue(log.String("key", "value2"))), + }, + { + name: "empty map", + body: log.MapValue(), + want: log.MapValue(), + }, + { + name: "single key map", + body: log.MapValue(log.String("key", "value")), + want: log.MapValue(log.String("key", "value")), + }, + { + name: "map with duplicate keys", + body: log.MapValue(log.String("key", "value1"), log.String("key", "value2")), + want: log.MapValue(log.String("key", "value2")), + }, + { + name: "map without duplicates", + body: log.MapValue(log.String("key1", "value1"), log.String("key2", "value2")), + want: log.MapValue(log.String("key1", "value1"), log.String("key2", "value2")), + }, + { + name: "map with nested slice deduplication", + body: log.MapValue( + log.Slice("slice", log.MapValue(log.String("nested", "val1"), log.String("nested", "val2"))), + ), + want: log.MapValue( + log.Slice("slice", log.MapValue(log.String("nested", "val2"))), + ), + }, + { + name: "deeply nested structure with deduplication", + body: log.SliceValue( + log.MapValue( + log.Map("nested", + log.String("key", "value1"), + log.String("key", "value2"), + ), + ), + ), + want: log.SliceValue( + log.MapValue( + log.Map("nested", + log.String("key", "value2"), + ), + ), + ), + }, + { + name: "deeply nested structure without deduplication", + body: log.SliceValue( + log.MapValue( + log.Map("nested", + log.String("key1", "value1"), + log.String("key2", "value2"), + ), + ), + ), + want: log.SliceValue( + log.MapValue( + log.Map("nested", + log.String("key1", "value1"), + log.String("key2", "value2"), + ), + ), + ), + }, + { + name: "string value for collection deduplication", + body: log.StringValue("test"), + want: log.StringValue("test"), + }, + { + name: "boolean value for collection deduplication", + body: log.BoolValue(true), + want: log.BoolValue(true), + }, + { + name: "empty slice for collection deduplication", + body: log.SliceValue(), + want: log.SliceValue(), + }, + { + name: "slice without nested deduplication for collection testing", + body: log.SliceValue(log.StringValue("test"), log.BoolValue(true)), + want: log.SliceValue(log.StringValue("test"), log.BoolValue(true)), + }, + { + name: "slice with nested map requiring deduplication", + body: log.SliceValue( + log.MapValue(log.String("key", "value1"), log.String("key", "value2")), + log.StringValue("normal"), + ), + want: log.SliceValue( + log.MapValue(log.String("key", "value2")), + log.StringValue("normal"), + ), + }, + { + name: "deeply nested slice with map deduplication", + body: log.SliceValue( + log.SliceValue( + log.MapValue(log.String("deep", "value1"), log.String("deep", "value2")), + ), + ), + want: log.SliceValue( + log.SliceValue( + log.MapValue(log.String("deep", "value2")), + ), + ), + }, + { + name: "empty map for collection deduplication", + body: log.MapValue(), + want: log.MapValue(), + }, + { + name: "map with nested slice containing duplicates", + body: log.MapValue( + log.String("outer", "value"), + log.Slice("nested_slice", + log.MapValue(log.String("inner", "val1"), log.String("inner", "val2")), + ), + ), + want: log.MapValue( + log.String("outer", "value"), + log.Slice("nested_slice", + log.MapValue(log.String("inner", "val2")), + ), + ), + }, + { + name: "map with key duplication and nested value deduplication", + body: log.MapValue( + log.String("key1", "value1"), + log.String("key1", "value2"), // key dedup + log.Slice("slice", + log.MapValue(log.String("nested", "val1"), log.String("nested", "val2")), // nested value dedup + ), + ), + want: log.MapValue( + log.String("key1", "value2"), + log.Slice("slice", + log.MapValue(log.String("nested", "val2")), + ), + ), + }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { @@ -536,6 +798,46 @@ func TestRecordAttrDeduplication(t *testing.T) { return out }(), }, + { + name: "AttributeWithDuplicateKeys", + attrs: []log.KeyValue{ + log.String("duplicate", "first"), + log.String("unique", "value"), + log.String("duplicate", "second"), + }, + want: []log.KeyValue{ + log.String("duplicate", "second"), + log.String("unique", "value"), + }, + }, + { + name: "ManyDuplicateKeys", + attrs: []log.KeyValue{ + log.String("key", "value1"), + log.String("key", "value2"), + log.String("key", "value3"), + log.String("key", "value4"), + log.String("key", "value5"), + }, + want: []log.KeyValue{ + log.String("key", "value5"), + }, + }, + { + name: "InterleavedDuplicates", + attrs: []log.KeyValue{ + log.String("a", "a1"), + log.String("b", "b1"), + log.String("a", "a2"), + log.String("c", "c1"), + log.String("b", "b2"), + }, + want: []log.KeyValue{ + log.String("a", "a2"), + log.String("b", "b2"), + log.String("c", "c1"), + }, + }, } for _, tc := range testcases { @@ -657,6 +959,84 @@ func TestApplyAttrLimitsDeduplication(t *testing.T) { ), wantDroppedAttrs: 10, }, + { + name: "EmptyMap", + input: log.MapValue(), + want: log.MapValue(), + wantDroppedAttrs: 0, + }, + { + name: "SingleKeyMap", + input: log.MapValue(log.String("key1", "value1")), + want: log.MapValue(log.String("key1", "value1")), + wantDroppedAttrs: 0, + }, + { + name: "EmptySlice", + input: log.SliceValue(), + want: log.SliceValue(), + wantDroppedAttrs: 0, + }, + { + name: "SliceWithNestedDedup", + input: log.SliceValue( + log.MapValue(log.String("key", "value1"), log.String("key", "value2")), + log.StringValue("normal"), + ), + want: log.SliceValue( + log.MapValue(log.String("key", "value2")), + log.StringValue("normal"), + ), + wantDroppedAttrs: 1, + }, + { + name: "NestedSliceInMap", + input: log.MapValue( + log.Slice("slice_key", + log.MapValue(log.String("nested", "value1"), log.String("nested", "value2")), + ), + ), + want: log.MapValue( + log.Slice("slice_key", + log.MapValue(log.String("nested", "value2")), + ), + ), + wantDroppedAttrs: 1, + }, + { + name: "DeeplyNestedStructure", + input: log.MapValue( + log.Map("level1", + log.Map("level2", + log.Slice("level3", + log.MapValue(log.String("deep", "value1"), log.String("deep", "value2")), + ), + ), + ), + ), + want: log.MapValue( + log.Map("level1", + log.Map("level2", + log.Slice("level3", + log.MapValue(log.String("deep", "value2")), + ), + ), + ), + ), + wantDroppedAttrs: 1, + }, + { + name: "NestedMapWithoutDuplicateKeys", + input: log.SliceValue((log.MapValue( + log.String("key1", "value1"), + log.String("key2", "value2"), + ))), + want: log.SliceValue(log.MapValue( + log.String("key1", "value1"), + log.String("key2", "value2"), + )), + wantDroppedAttrs: 0, + }, } for _, tc := range testcases { @@ -770,6 +1150,42 @@ func TestApplyAttrLimitsTruncation(t *testing.T) { log.Map("7", log.String("a", "")), ), }, + { + name: "LongStringTruncated", + limit: 5, + input: log.StringValue("This is a very long string that should be truncated"), + want: log.StringValue("This "), + }, + { + name: "LongBytesNotTruncated", + limit: 5, + input: log.BytesValue([]byte("This is a very long byte array")), + want: log.BytesValue([]byte("This is a very long byte array")), + }, + { + name: "TruncationInNestedMap", + limit: 3, + input: log.MapValue( + log.String("short", "ok"), + log.String("long", "toolong"), + ), + want: log.MapValue( + log.String("short", "ok"), + log.String("long", "too"), + ), + }, + { + name: "TruncationInNestedSlice", + limit: 4, + input: log.SliceValue( + log.StringValue("good"), + log.StringValue("toolong"), + ), + want: log.SliceValue( + log.StringValue("good"), + log.StringValue("tool"), + ), + }, } for _, tc := range testcases { From 27bdafeaa3d586cba933fbe6edb0be285a88ccbd Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Thu, 25 Sep 2025 19:06:19 +0200 Subject: [PATCH 13/20] addAttrs to applyAttrLimitsAndDedup after slices.Grow --- sdk/log/record.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/sdk/log/record.go b/sdk/log/record.go index 54c7f69f31a..4f866d101f9 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -324,11 +324,13 @@ func (r *Record) addAttrs(attrs []log.KeyValue) { r.nFront++ } - for j, a := range attrs[i:] { - attrs[i+j] = r.applyAttrLimitsAndDedup(a) - } + // Make a copy to avoid modifying the original. + j := len(r.back) r.back = slices.Grow(r.back, len(attrs[i:])) r.back = append(r.back, attrs[i:]...) + for i, a := range r.back[j:] { + r.back[i+j] = r.applyAttrLimitsAndDedup(a) + } } // SetAttributes sets (and overrides) attributes to the log record. From d76cdaf3bcadec89b4ebd3d20fb3462f6cb896fe Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Thu, 25 Sep 2025 19:53:21 +0200 Subject: [PATCH 14/20] add TestRecordAddAttributesDoesNotMutateInput --- sdk/log/record_test.go | 52 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/sdk/log/record_test.go b/sdk/log/record_test.go index b997cbf9d6a..6f363527df0 100644 --- a/sdk/log/record_test.go +++ b/sdk/log/record_test.go @@ -1342,6 +1342,58 @@ func TestTruncate(t *testing.T) { } } +func TestRecordAddAttributesDoesNotMutateInput(t *testing.T) { + attrs := []log.KeyValue{ + log.String("attr1", "very long value that will be truncated"), + log.String("attr2", "another very long value that will be truncated"), + log.String("attr3", "yet another very long value that will be truncated"), + log.String("attr4", "more very long value that will be truncated"), + log.String("attr5", "extra very long value that will be truncated"), + log.String("attr6", "additional very long value that will be truncated"), + log.String("attr7", "more additional very long value that will be truncated"), + } + + originalValues := make([]string, len(attrs)) + for i, kv := range attrs { + originalValues[i] = kv.Value.AsString() + } + + r := &Record{ + attributeValueLengthLimit: 20, // Short limit to trigger truncation. + attributeCountLimit: -1, // No count limit. + allowDupKeys: false, + } + + r.AddAttributes(attrs...) + + // Verify that the original shared slice was not mutated + for i, kv := range attrs { + if kv.Value.AsString() != originalValues[i] { + t.Errorf("Input slice was mutated! Attribute %d: original=%q, current=%q", + i, originalValues[i], kv.Value.AsString()) + } + } + + // Verify that the record has the truncated values + var gotAttrs []log.KeyValue + r.WalkAttributes(func(kv log.KeyValue) bool { + gotAttrs = append(gotAttrs, kv) + return true + }) + wantAttr := []log.KeyValue{ + log.String("attr1", "very long value that"), + log.String("attr2", "another very long va"), + log.String("attr3", "yet another very lon"), + log.String("attr4", "more very long value"), + log.String("attr5", "extra very long valu"), + log.String("attr6", "additional very long"), + log.String("attr7", "more additional very"), + } + if !slices.EqualFunc(gotAttrs, wantAttr, func(a, b log.KeyValue) bool { return a.Equal(b) }) { + t.Errorf("Attributes do not match.\ngot:\n%v\nwant:\n%v", printKVs(gotAttrs), printKVs(wantAttr)) + } +} + func TestRecordMethodsInputConcurrentSafe(t *testing.T) { nestedSlice := log.Slice("nested_slice", log.SliceValue(log.StringValue("nested_inner1"), log.StringValue("nested_inner2")), From 4341fd404c450066df0e107d8cf585996108cbe9 Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Thu, 25 Sep 2025 19:59:08 +0200 Subject: [PATCH 15/20] more iterative code to ease readability and debugging --- sdk/log/record.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/sdk/log/record.go b/sdk/log/record.go index 4f866d101f9..5fdecee5ad1 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -487,7 +487,13 @@ func (r *Record) applyValueLimitsAndDedup(val log.Value) log.Value { sl := val.AsSlice() // First check if any limits need to be applied. - needsChange := slices.ContainsFunc(sl, r.needsValueLimitsOrDedup) + needsChange := false + for _, v := range sl { + if r.needsValueLimitsOrDedup(v) { + needsChange = true + break + } + } if needsChange { // Create a new slice to avoid modifying the original. @@ -548,8 +554,10 @@ func (r *Record) needsValueLimitsOrDedup(val log.Value) bool { case log.KindString: return r.attributeValueLengthLimit >= 0 && len(val.AsString()) > r.attributeValueLengthLimit case log.KindSlice: - if slices.ContainsFunc(val.AsSlice(), r.needsValueLimitsOrDedup) { - return true + for _, v := range val.AsSlice() { + if r.needsValueLimitsOrDedup(v) { + return true + } } case log.KindMap: kvs := val.AsMap() From e4604acb0206fe490174bfe11414a37f9d77e15b Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Thu, 25 Sep 2025 20:09:19 +0200 Subject: [PATCH 16/20] use map[string]struct{} for seenPool to reduce memory usage --- sdk/log/record.go | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/sdk/log/record.go b/sdk/log/record.go index 5fdecee5ad1..ef604b579b8 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -41,16 +41,16 @@ func putIndex(index map[string]int) { indexPool.Put(index) } -// seenPool is a pool of boolean maps used for duplicate checking. +// seenPool is a pool of maps used for duplicate checking. var seenPool = sync.Pool{ - New: func() any { return make(map[string]bool) }, + New: func() any { return make(map[string]struct{}) }, } -func getSeen() map[string]bool { - return seenPool.Get().(map[string]bool) +func getSeen() map[string]struct{} { + return seenPool.Get().(map[string]struct{}) } -func putSeen(seen map[string]bool) { +func putSeen(seen map[string]struct{}) { clear(seen) seenPool.Put(seen) } @@ -235,10 +235,10 @@ func (r *Record) AddAttributes(attrs ...log.KeyValue) { seen := getSeen() defer putSeen(seen) for _, a := range attrs { - if seen[a.Key] || rIndex[a.Key] != 0 { + if _, ok := seen[a.Key]; ok || rIndex[a.Key] != 0 { return true } - seen[a.Key] = true + seen[a.Key] = struct{}{} } return false }() @@ -379,10 +379,10 @@ func dedup(kvs []log.KeyValue) (unique []log.KeyValue, dropped int) { seen := getSeen() defer putSeen(seen) for _, kv := range kvs { - if seen[kv.Key] { + if _, ok := seen[kv.Key]; ok { return true } - seen[kv.Key] = true + seen[kv.Key] = struct{}{} } return false }() @@ -567,10 +567,10 @@ func (r *Record) needsValueLimitsOrDedup(val log.Value) bool { seen := getSeen() defer putSeen(seen) for _, kv := range kvs { - if seen[kv.Key] { + if _, ok := seen[kv.Key]; ok { return true } - seen[kv.Key] = true + seen[kv.Key] = struct{}{} } return false }() @@ -657,10 +657,10 @@ func (r *Record) needsBodyDedup(val log.Value) bool { seen := getSeen() defer putSeen(seen) for _, kv := range kvs { - if seen[kv.Key] { + if _, ok := seen[kv.Key]; ok { return true } - seen[kv.Key] = true + seen[kv.Key] = struct{}{} } return false }() From 3b673706ebfc093d94618678ef224694fbd5ae74 Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Wed, 1 Oct 2025 10:49:38 +0200 Subject: [PATCH 17/20] dedup func with single iteration --- sdk/log/record.go | 53 +++++++++++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/sdk/log/record.go b/sdk/log/record.go index ef604b579b8..8880b1124a2 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -27,6 +27,24 @@ var logAttrDropped = sync.OnceFunc(func() { global.Warn("limit reached: dropping log Record attributes") }) +// uniquePool is a pool of unique key-values used during de-duplication. +var uniquePool = sync.Pool{ + New: func() any { return new([]log.KeyValue) }, +} + +func getUnique() *[]log.KeyValue { + return uniquePool.Get().(*[]log.KeyValue) +} + +func putUnique(v *[]log.KeyValue) { + // To reduce peak allocation, return only smaller buffers to the pool. + const maxBufferSize = 128 + if cap(*v) <= maxBufferSize { + *v = (*v)[:0] + uniquePool.Put(v) + } +} + // indexPool is a pool of index maps used for de-duplication. var indexPool = sync.Pool{ New: func() any { return make(map[string]int) }, @@ -374,38 +392,27 @@ func dedup(kvs []log.KeyValue) (unique []log.KeyValue, dropped int) { return kvs, 0 // No deduplication needed. } - // Check if deduplication is actually needed by looking for duplicate keys. - hasDuplicates := func() bool { - seen := getSeen() - defer putSeen(seen) - for _, kv := range kvs { - if _, ok := seen[kv.Key]; ok { - return true - } - seen[kv.Key] = struct{}{} - } - return false - }() - - if !hasDuplicates { - return kvs, 0 // No deduplication needed. - } - - // Deduplication is needed, create a new slice to avoid modifying the original. index := getIndex() defer putIndex(index) - - unique = make([]log.KeyValue, 0, len(kvs)) + u := getUnique() + defer putUnique(u) for _, a := range kvs { idx, found := index[a.Key] if found { dropped++ - unique[idx] = a + (*u)[idx] = a } else { - unique = append(unique, a) - index[a.Key] = len(unique) - 1 + *u = append(*u, a) + index[a.Key] = len(*u) - 1 } } + + if dropped == 0 { + return kvs, 0 + } + + unique = make([]log.KeyValue, len(*u)) + copy(unique, *u) return unique, dropped } From 90843865cf2780fcebe9bb4e3cf0b677b37f35a4 Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Wed, 1 Oct 2025 10:56:14 +0200 Subject: [PATCH 18/20] improve comments --- sdk/log/record.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sdk/log/record.go b/sdk/log/record.go index 8880b1124a2..150f1758f34 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -27,7 +27,7 @@ var logAttrDropped = sync.OnceFunc(func() { global.Warn("limit reached: dropping log Record attributes") }) -// uniquePool is a pool of unique key-values used during de-duplication. +// uniquePool is a pool of unique attributes used for attributes de-duplication. var uniquePool = sync.Pool{ New: func() any { return new([]log.KeyValue) }, } @@ -45,7 +45,7 @@ func putUnique(v *[]log.KeyValue) { } } -// indexPool is a pool of index maps used for de-duplication. +// indexPool is a pool of index maps used for attributes de-duplication. var indexPool = sync.Pool{ New: func() any { return make(map[string]int) }, } @@ -59,7 +59,7 @@ func putIndex(index map[string]int) { indexPool.Put(index) } -// seenPool is a pool of maps used for duplicate checking. +// seenPool is a pool of seen keys used for maps de-duplication. var seenPool = sync.Pool{ New: func() any { return make(map[string]struct{}) }, } From d199ebe2ae4271dd4399844944af8b4bb6948b9f Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Wed, 1 Oct 2025 11:00:46 +0200 Subject: [PATCH 19/20] improve comments and pool --- sdk/log/record.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sdk/log/record.go b/sdk/log/record.go index 150f1758f34..87fedb6feaf 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -37,9 +37,9 @@ func getUnique() *[]log.KeyValue { } func putUnique(v *[]log.KeyValue) { - // To reduce peak allocation, return only smaller buffers to the pool. - const maxBufferSize = 128 - if cap(*v) <= maxBufferSize { + // To reduce peak allocation. + const maxUniqueSize = 1028 + if cap(*v) <= maxUniqueSize { *v = (*v)[:0] uniquePool.Put(v) } From 60be936b109c91b40d64c8c6e3f7f22893dff9b5 Mon Sep 17 00:00:00 2001 From: Robert Pajak Date: Wed, 1 Oct 2025 11:42:14 +0200 Subject: [PATCH 20/20] AddAttributes func with single iteration --- sdk/log/record.go | 79 ++++++++++++++++++++++------------------------- 1 file changed, 37 insertions(+), 42 deletions(-) diff --git a/sdk/log/record.go b/sdk/log/record.go index 87fedb6feaf..5b830b7ea8f 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -244,57 +244,52 @@ func (r *Record) AddAttributes(attrs ...log.KeyValue) { } if !r.allowDupKeys { + // Use a slice from the pool to avoid modifying the original. + // Note, do not iterate attrs twice by just calling dedup(attrs) here. + unique := getUnique() + defer putUnique(unique) + // Used to find duplicates between attrs and existing attributes in r. rIndex := r.attrIndex() defer putIndex(rIndex) - // Check if deduplication is actually needed. - needsDedup := func() bool { - seen := getSeen() - defer putSeen(seen) - for _, a := range attrs { - if _, ok := seen[a.Key]; ok || rIndex[a.Key] != 0 { - return true - } - seen[a.Key] = struct{}{} + // Used to find duplicates within attrs itself. + // The index value is the index of the element in unique. + uIndex := getIndex() + defer putIndex(uIndex) + + dropped := 0 + + // Deduplicate attrs within the scope of all existing attributes. + for _, a := range attrs { + // Last-value-wins for any duplicates in attrs. + idx, found := uIndex[a.Key] + if found { + dropped++ + (*unique)[idx] = a + continue } - return false - }() - - if needsDedup { - // Create a new slice to avoid modifying the original. - unique := make([]log.KeyValue, 0, len(attrs)) - // Used to find duplicates within attrs itself. - // The index value is the index of the element in unique. - uIndex := getIndex() - defer putIndex(uIndex) - - // Deduplicate attrs within the scope of all existing attributes. - for _, a := range attrs { - // Last-value-wins for any duplicates in attrs. - idx, found := uIndex[a.Key] - if found { - r.addDropped(1) - unique[idx] = a - continue - } - idx, found = rIndex[a.Key] - if found { - // New attrs overwrite any existing with the same key. - r.addDropped(1) - if idx < 0 { - r.front[-(idx + 1)] = a - } else { - r.back[idx] = a - } + idx, found = rIndex[a.Key] + if found { + // New attrs overwrite any existing with the same key. + dropped++ + if idx < 0 { + r.front[-(idx + 1)] = a } else { - // Unique attribute. - unique = append(unique, a) - uIndex[a.Key] = len(unique) - 1 + r.back[idx] = a } + } else { + // Unique attribute. + (*unique) = append(*unique, a) + uIndex[a.Key] = len(*unique) - 1 } - attrs = unique + } + + if dropped > 0 { + attrs = make([]log.KeyValue, len(*unique)) + copy(attrs, *unique) + r.addDropped(dropped) } }