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

[Mojachieee]

Fixes #6982

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@0485596). Learn more about missing BASE report.

Additional details and impacted files

@@          Coverage Diff           @@
##             main   #7002   +/-   ##
======================================
  Coverage        ?   82.8%           
======================================
  Files           ?     262           
  Lines           ?   24362           
  Branches        ?       0           
======================================
  Hits            ?   20176           
  Misses          ?    3811           
  Partials        ?     375           

Files with missing lines	Coverage Δ
sdk/log/record.go	100.0% <100.0%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[MrAlias]

This is not a part of the specification. Doesn't adding this move up further away from the defined specification behavior?

[pellared]

This is not a part of the specification. Doesn't adding this move up further away from the defined specification behavior?

@MrAlias, what do you have in mind?

"deduplication" of the key-values is part of the specification and it also involves Body of the log record.
Body is of any type (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#field-body).
any can have a map<string, any> (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#type-any).
map<string, any> does not allow duplicated keys (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/data-model.md#type-mapstring-any).

Or are you concerned about the option added in #6968?

Or maybe #7002 (comment)? EDIT: already addressed

[pellared]

nit comments

[MrAlias]

What affect does this have on the performance of SetBody?

[pellared]

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)

[MrAlias]

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.

[pellared]

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?

[pellared]

With OTEP: Extending attributes to support complex values #4485 finally being merged (which was not certain at all) we most likely would not need this change as we will get rid of log.Value in favor of attribute.Value which at this point of time does not allow key-value collections. The attribute.Map and attribute.MapValue that is going to be added is planned to be handling the deduplication on creation time (see #6809).

Still, I am fine having this PR merged as I think it is better to start changing the Logs API and SDK to use attribute.Value instead of log.Value (which is going to be removed) AFTER the upcoming release (so e.g. for v1.39.0).

EDIT: I created #7034