Skip to content

sdk/log: Deduplicate key-value collections in Record.SetBody #7002

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Change `AssertEqual` in `go.opentelemetry.io/otel/log/logtest` to accept `TestingT` in order to support benchmarks and fuzz tests. (#6908)
- Change `SDKProcessorLogQueueCapacity`, `SDKProcessorLogQueueSize`, `SDKProcessorSpanQueueSize`, and `SDKProcessorSpanQueueCapacity` in `go.opentelemetry.io/otel/semconv/v1.36.0/otelconv` to use a `Int64ObservableUpDownCounter`. (#7041)

### Fixes

- `SetBody` method of `Record` in `go.opentelemetry.io/otel/sdk/log` now deduplicates key-value collections (`log.Value` of `log.KindMap` from `go.opentelemetry.io/otel/log`). (#7002)

<!-- Released section -->
<!-- Don't change this section unless doing release -->

Expand Down
24 changes: 23 additions & 1 deletion sdk/log/record.go
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,11 @@ func (r *Record) Body() log.Value {

// SetBody sets the body of the log record.
func (r *Record) SetBody(v log.Value) {
r.body = v
if !r.allowDupKeys {
r.body = r.dedupeBodyCollections(v)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What affect does this have on the performance of SetBody?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point of time we should do the deduplication anyway (to be compliant with the specification).

The performance benefit of not doing (when allowDupKeys is true) it is pretty irrelevant given we will remove this opt-in performance optimization probably in v1.39.0. Therefore, I do not think it is worth adding a new benchmark. More here: #7002 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should likely be done in a lazy manner. Having performance information about the approach presented here will provide evidence to how critical that work is going to be.

Copy link
Member

@pellared pellared Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have benchmarks for the same on log record attributes: #6968 (comment) this is also why I thought that maybe we do not more 😉

However, I think it would be easier and less-controversial to just add benchmarks for Record.SetBody.
@Mojachieee, can you add them and add the results to the PR description?

Copy link
Member

@pellared pellared Jul 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mojachieee, are you able to address the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I hope to get to it this week

} else {
r.body = v
}
}

// WalkAttributes walks all attributes the log record holds by calling f for
Expand Down Expand Up @@ -452,6 +456,24 @@ func (r *Record) applyValueLimits(val log.Value) log.Value {
return val
}

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])
}
val = log.SliceValue(sl...)
case log.KindMap:
kvs, _ := dedup(val.AsMap())
for i := range kvs {
kvs[i].Value = r.dedupeBodyCollections(kvs[i].Value)
}
val = log.MapValue(kvs...)
}
return val
}

// 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.
Expand Down
80 changes: 76 additions & 4 deletions sdk/log/record_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,82 @@ func TestRecordSeverityText(t *testing.T) {
}

func TestRecordBody(t *testing.T) {
v := log.BoolValue(true)
r := new(Record)
r.SetBody(v)
assert.True(t, v.Equal(r.Body()))
testcases := []struct {
name string
allowDuplicates bool
body log.Value
want log.Value
}{
{
name: "Bool",
body: log.BoolValue(true),
want: log.BoolValue(true),
},
{
name: "slice",
body: log.SliceValue(log.BoolValue(true), log.BoolValue(false)),
want: log.SliceValue(log.BoolValue(true), log.BoolValue(false)),
},
{
name: "map",
body: log.MapValue(
log.Bool("0", true),
log.Int64("1", 2), // This should be removed
log.Float64("2", 3.0),
log.String("3", "forth"),
log.Slice("4", log.Int64Value(1)),
log.Map("5", log.Int("key", 2)),
log.Bytes("6", []byte("six")),
log.Int64("1", 3),
),
want: log.MapValue(
log.Bool("0", true),
log.Float64("2", 3.0),
log.String("3", "forth"),
log.Slice("4", log.Int64Value(1)),
log.Map("5", log.Int("key", 2)),
log.Bytes("6", []byte("six")),
log.Int64("1", 3),
),
},
{
name: "nestedMap",
body: log.MapValue(
log.Map("key",
log.Int64("key", 1),
log.Int64("key", 2),
),
),
want: log.MapValue(
log.Map("key",
log.Int64("key", 2),
),
),
},
{
name: "map - allow duplicates",
allowDuplicates: true,
body: log.MapValue(
log.Int64("1", 2),
log.Int64("1", 3),
),
want: log.MapValue(
log.Int64("1", 2),
log.Int64("1", 3),
),
},
}
for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
r := new(Record)
r.allowDupKeys = tc.allowDuplicates
r.SetBody(tc.body)
got := r.Body()
if !got.Equal(tc.want) {
t.Errorf("r.Body() = %v, want %v", got, tc.want)
}
})
}
}

func TestRecordAttributes(t *testing.T) {
Expand Down
Loading