From b2b803cc607af13aa9466e5b0a6c9c549f9966bc Mon Sep 17 00:00:00 2001 From: Anton Manakin Date: Sun, 2 Jun 2024 21:16:18 +0300 Subject: [PATCH 1/6] sdk/log: use pointer receiver in applying attrs limits --- sdk/log/record.go | 4 ++-- sdk/log/record_test.go | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/sdk/log/record.go b/sdk/log/record.go index 9f8c63d2cd3..61416632a53 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -391,12 +391,12 @@ func (r *Record) Clone() Record { return res } -func (r Record) applyAttrLimits(attr log.KeyValue) log.KeyValue { +func (r *Record) applyAttrLimits(attr log.KeyValue) log.KeyValue { attr.Value = r.applyValueLimits(attr.Value) return attr } -func (r Record) applyValueLimits(val log.Value) log.Value { +func (r *Record) applyValueLimits(val log.Value) log.Value { switch val.Kind() { case log.KindString: s := val.AsString() diff --git a/sdk/log/record_test.go b/sdk/log/record_test.go index f76634496dc..bfc823ecb30 100644 --- a/sdk/log/record_test.go +++ b/sdk/log/record_test.go @@ -446,6 +446,8 @@ func TestApplyAttrLimitsTruncation(t *testing.T) { name string limit int input, want log.Value + + expectedDropped int }{ { name: "Empty", @@ -531,6 +533,18 @@ func TestApplyAttrLimitsTruncation(t *testing.T) { log.Map("7", log.String("a", "")), ), }, + { + name: "MapWithDuplicate", + limit: 0, + input: log.MapValue( + log.Bool("subKey", true), + log.Bool("subKey", false), + ), + want: log.MapValue( + log.Bool("subKey", false), + ), + expectedDropped: 1, + }, } for _, tc := range testcases { @@ -548,6 +562,8 @@ func TestApplyAttrLimitsTruncation(t *testing.T) { r.SetAttributes(kv) assertKV(t, r, log.KeyValue{Key: key, Value: tc.want}) }) + + require.Equal(t, tc.expectedDropped, r.DroppedAttributes()) }) } } From 7258087f018fc68fc0799f1768833fe9029eed04 Mon Sep 17 00:00:00 2001 From: Anton Manakin Date: Mon, 3 Jun 2024 10:11:07 +0300 Subject: [PATCH 2/6] add changelog and fix test --- CHANGELOG.md | 1 + sdk/log/record_test.go | 21 +++++---------------- 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c6fe8f6621e..5a99bfd80bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - Identify the `Meter` returned from the global `MeterProvider` in `go.opentelemetry.io/otel/global` with its schema URL. (#5426) - Log a warning to the OpenTelemetry internal logger when a `Span` in `go.opentelemetry.io/otel/sdk/trace` drops an attribute, event, or link due to a limit being reached. (#5434) - Document instrument name requirements in `go.opentelemetry.io/otel/metric`. (#5435) +- Fix counting number of dropped attributes of `Record` in `go.opentelemetry.io/otel/sdk/log`. (#5464) ## [1.27.0/0.49.0/0.3.0] 2024-05-21 diff --git a/sdk/log/record_test.go b/sdk/log/record_test.go index bfc823ecb30..361b37097a6 100644 --- a/sdk/log/record_test.go +++ b/sdk/log/record_test.go @@ -349,6 +349,8 @@ func TestApplyAttrLimitsDeduplication(t *testing.T) { name string limit int input, want log.Value + + droppedAttrs int }{ { // No de-duplication @@ -419,6 +421,7 @@ func TestApplyAttrLimitsDeduplication(t *testing.T) { log.String("g", "GG"), log.String("h", "H"), ), + droppedAttrs: 10, }, } @@ -437,6 +440,8 @@ func TestApplyAttrLimitsDeduplication(t *testing.T) { r.SetAttributes(kv) assertKV(t, r, log.KeyValue{Key: key, Value: tc.want}) }) + + require.Equal(t, tc.droppedAttrs, r.DroppedAttributes()) }) } } @@ -446,8 +451,6 @@ func TestApplyAttrLimitsTruncation(t *testing.T) { name string limit int input, want log.Value - - expectedDropped int }{ { name: "Empty", @@ -533,18 +536,6 @@ func TestApplyAttrLimitsTruncation(t *testing.T) { log.Map("7", log.String("a", "")), ), }, - { - name: "MapWithDuplicate", - limit: 0, - input: log.MapValue( - log.Bool("subKey", true), - log.Bool("subKey", false), - ), - want: log.MapValue( - log.Bool("subKey", false), - ), - expectedDropped: 1, - }, } for _, tc := range testcases { @@ -562,8 +553,6 @@ func TestApplyAttrLimitsTruncation(t *testing.T) { r.SetAttributes(kv) assertKV(t, r, log.KeyValue{Key: key, Value: tc.want}) }) - - require.Equal(t, tc.expectedDropped, r.DroppedAttributes()) }) } } From 8a03c4c6fc078bbe473c85ed6fb94dc2ab6b0b85 Mon Sep 17 00:00:00 2001 From: Anton Manakin Date: Mon, 3 Jun 2024 11:46:42 +0300 Subject: [PATCH 3/6] better tests and fix comments --- sdk/log/record.go | 2 -- sdk/log/record_test.go | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/sdk/log/record.go b/sdk/log/record.go index 61416632a53..1c413fa6e24 100644 --- a/sdk/log/record.go +++ b/sdk/log/record.go @@ -275,8 +275,6 @@ func (r *Record) addAttrs(attrs []log.KeyValue) { // SetAttributes sets (and overrides) attributes to the log record. func (r *Record) SetAttributes(attrs ...log.KeyValue) { - // TODO: apply truncation to string and []string values. - // TODO: deduplicate map values. var drop int attrs, drop = dedup(attrs) r.setDropped(drop) diff --git a/sdk/log/record_test.go b/sdk/log/record_test.go index 361b37097a6..14bc04627e7 100644 --- a/sdk/log/record_test.go +++ b/sdk/log/record_test.go @@ -434,14 +434,14 @@ func TestApplyAttrLimitsDeduplication(t *testing.T) { t.Run("AddAttributes", func(t *testing.T) { r.AddAttributes(kv) assertKV(t, r, log.KeyValue{Key: key, Value: tc.want}) + require.Equal(t, tc.droppedAttrs, r.DroppedAttributes()) }) t.Run("SetAttributes", func(t *testing.T) { r.SetAttributes(kv) assertKV(t, r, log.KeyValue{Key: key, Value: tc.want}) + require.Equal(t, tc.droppedAttrs, r.DroppedAttributes()) }) - - require.Equal(t, tc.droppedAttrs, r.DroppedAttributes()) }) } } From c8955b87ca3398b781cd20d8bef2d12e14252b6f Mon Sep 17 00:00:00 2001 From: Anton Manakin Date: Mon, 3 Jun 2024 19:46:37 +0300 Subject: [PATCH 4/6] add set/add attribute benchmark --- sdk/log/record_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/sdk/log/record_test.go b/sdk/log/record_test.go index 14bc04627e7..5d8d10be82e 100644 --- a/sdk/log/record_test.go +++ b/sdk/log/record_test.go @@ -640,3 +640,12 @@ func TestTruncate(t *testing.T) { }) } } + +func BenchmarkSetAddAttributes(b *testing.B) { + for i := 0; i < b.N; i++ { + kv := log.String("key", "value") + var r Record + r.SetAttributes(kv) + r.AddAttributes(kv) + } +} From a5867440bea5b5d9b14a7d2c7143a8cd4485ddbe Mon Sep 17 00:00:00 2001 From: Anton Manakin Date: Mon, 3 Jun 2024 20:19:00 +0300 Subject: [PATCH 5/6] fix tests and benchmark --- sdk/log/record_test.go | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/sdk/log/record_test.go b/sdk/log/record_test.go index 5d8d10be82e..78dc499ab93 100644 --- a/sdk/log/record_test.go +++ b/sdk/log/record_test.go @@ -346,11 +346,10 @@ func TestRecordAttrDeduplication(t *testing.T) { func TestApplyAttrLimitsDeduplication(t *testing.T) { testcases := []struct { - name string - limit int - input, want log.Value - - droppedAttrs int + name string + limit int + input, want log.Value + wantDroppedAttrs int }{ { // No de-duplication @@ -434,13 +433,13 @@ func TestApplyAttrLimitsDeduplication(t *testing.T) { t.Run("AddAttributes", func(t *testing.T) { r.AddAttributes(kv) assertKV(t, r, log.KeyValue{Key: key, Value: tc.want}) - require.Equal(t, tc.droppedAttrs, r.DroppedAttributes()) + assert.Equal(t, tc.wantDroppedAttrs, r.DroppedAttributes()) }) t.Run("SetAttributes", func(t *testing.T) { r.SetAttributes(kv) assertKV(t, r, log.KeyValue{Key: key, Value: tc.want}) - require.Equal(t, tc.droppedAttrs, r.DroppedAttributes()) + assert.Equal(t, tc.wantDroppedAttrs, r.DroppedAttributes()) }) }) } @@ -642,10 +641,13 @@ func TestTruncate(t *testing.T) { } func BenchmarkSetAddAttributes(b *testing.B) { + kv := log.String("key", "value") + records := make([]Record, b.N) + + b.ReportAllocs() + b.ResetTimer() for i := 0; i < b.N; i++ { - kv := log.String("key", "value") - var r Record - r.SetAttributes(kv) - r.AddAttributes(kv) + records[i].SetAttributes(kv) + records[i].AddAttributes(kv) } } From 480a5648ea82b6016c2cbfec147c21321a05914b Mon Sep 17 00:00:00 2001 From: Anton Manakin Date: Mon, 3 Jun 2024 20:49:32 +0300 Subject: [PATCH 6/6] fix tests --- sdk/log/record_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/log/record_test.go b/sdk/log/record_test.go index 78dc499ab93..cbbd8942420 100644 --- a/sdk/log/record_test.go +++ b/sdk/log/record_test.go @@ -420,7 +420,7 @@ func TestApplyAttrLimitsDeduplication(t *testing.T) { log.String("g", "GG"), log.String("h", "H"), ), - droppedAttrs: 10, + wantDroppedAttrs: 10, }, }