Fix lifetime for sdk::ReadWriteLogRecord

[owent]

Fixes #3135 Fixes #2651

Changes

• Add MixedAttributeMap to store both std::unordered_map<std::string, opentelemetry::common::AttributeValue> and AttributeMap
• Copy both OwnedAttributeValue, std::vector<nostd::string_view> and bool array for ReadWriteLogRecord.

For significant contributions please make sure you have completed the following items:

• [ ] CHANGELOG.md updated for non-trivial changes
• [x] Unit tests have been added
• [ ] Changes in public API reviewed

[netlify[bot]]

✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name	Link
🔨 Latest commit	4b8012d3f5ab1186a0c5dff039d7cba4ea12f15d
🔍 Latest deploy log	https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/673ef2dc68097d00084a6b11

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 98.19820% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.06%. Comparing base (fe68d51) to head (4b8012d).

Files with missing lines	Patch %	Lines
...include/opentelemetry/sdk/common/attribute_utils.h	98.10%	2 Missing :warning:

Additional details and impacted files

[](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/3147?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) ```diff @@ Coverage Diff @@ ## main #3147 +/- ## ========================================== + Coverage 87.86% 88.06% +0.21% ========================================== Files 195 195 Lines 6151 6256 +105 ========================================== + Hits 5404 5509 +105 Misses 747 747 ``` | [Files with missing lines](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/3147?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | Coverage Δ | | |---|---|---| | [sdk/src/logs/read\_write\_log\_record.cc](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/3147?src=pr&el=tree&filepath=sdk%2Fsrc%2Flogs%2Fread_write_log_record.cc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-c2RrL3NyYy9sb2dzL3JlYWRfd3JpdGVfbG9nX3JlY29yZC5jYw==) | `96.39% <100.00%> (ø)` | | | [...include/opentelemetry/sdk/common/attribute\_utils.h](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/3147?src=pr&el=tree&filepath=sdk%2Finclude%2Fopentelemetry%2Fsdk%2Fcommon%2Fattribute_utils.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-c2RrL2luY2x1ZGUvb3BlbnRlbGVtZXRyeS9zZGsvY29tbW9uL2F0dHJpYnV0ZV91dGlscy5o) | `95.91% <98.10%> (+5.00%)` | :arrow_up: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/3147/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[sjinks]

I don't know if this is critical, but this change breaks the ABI:

• Problems with Data Types, High Severity
  • read_write_log_record.h, namespace opentelemetry::v1::sdk::logs, class ReadWriteLogRecord
  • Change: Size of this class has been increased from 176 bytes to 464 bytes.
  • Effect:
    1. An object of this class can be allocated by the applications and old size will be hardcoded at the compile time. Call of any exported constructor will break the memory of neighboring objects on the stack or heap.
    2. The memory layout and size of subclasses will be changed.
• Problems with Data Types, Low Severity
  • read_write_log_record.h, namespace opentelemetry::v1::sdk::logs, class ReadWriteLogRecord
  • Change: Type of field attributes_map_ has been changed from std::unordered_map<std::__cxx11::basic_string<char>, absl::otel_v1::variant<bool>, std::hash<std::__cxx11::basic_string<char> >, std::equal_to<std::__cxx11::basic_string<char> > > (56 bytes) to opentelemetry::v1::sdk::common::MixedAttributeMap (184 bytes)
  • Change: Type of field body_ has been changed from opentelemetry::v1::common::AttributeValue (24 bytes) to opentelemetry::v1::sdk::common::MixedAttributeMap (184 bytes)
  • Effect: Size of the inclusive type has been changed.

To reproduce:

cmake -B build -DBUILD_SHARED_LIBS=ON -DCMAKE_CXX_FLAGS="-Og -g3" --fresh
cmake --build build -j$(nproc) --clean-first
cmake --install build --prefix ORIG
wget https://patch-diff.githubusercontent.com/raw/open-telemetry/opentelemetry-cpp/pull/3147.diff
patch -p1 < 3147.diff
cmake --build build -j$(nproc)
cmake --install build --prefix NEW
mkdir dumps
for i in ORIG/lib/*.so; do abi-dumper "$i" -o "dumps/orig-$(basename "$i").dump" -lver orig; done
for i in NEW/lib/*.so; do abi-dumper "$i" -o "dumps/new-$(basename "$i").dump" -lver new; done
for i in NEW/lib/*.so; do abi-compliance-checker -l "$(basename "$i")" -old "dumps/orig-$(basename "$i").dump" -new "dumps/new-$(basename "$i").dump; done

It will show smth like

...
Preparing, please wait ...
Comparing ABIs ...
Comparing APIs ...
Creating compatibility report ...
Binary compatibility: 85.3%
Source compatibility: 100%
Total binary compatibility problems: 1, warnings: 2
Total source compatibility problems: 0, warnings: 0
Report: compat_reports/libopentelemetry_logs.so/orig_to_new/compat_report.html
...

[owent]

I don't know if this is critical, but this change breaks the ABI:

• Problems with Data Types, High Severity

  • read_write_log_record.h, namespace opentelemetry::v1::sdk::logs, class ReadWriteLogRecord

  • Change: Size of this class has been increased from 176 bytes to 464 bytes.

  • Effect:

    1. An object of this class can be allocated by the applications and old size will be hardcoded at the compile time. Call of any exported constructor will break the memory of neighboring objects on the stack or heap.
    2. The memory layout and size of subclasses will be changed.

• Problems with Data Types, Low Severity

  • read_write_log_record.h, namespace opentelemetry::v1::sdk::logs, class ReadWriteLogRecord

  • Change: Type of field attributes_map_ has been changed from std::unordered_map<std::__cxx11::basic_string<char>, absl::otel_v1::variant<bool>, std::hash<std::__cxx11::basic_string<char> >, std::equal_to<std::__cxx11::basic_string<char> > > (56 bytes) to opentelemetry::v1::sdk::common::MixedAttributeMap (184 bytes)

  • Change: Type of field body_ has been changed from opentelemetry::v1::common::AttributeValue (24 bytes) to opentelemetry::v1::sdk::common::MixedAttributeMap (184 bytes)

  • Effect: Size of the inclusive type has been changed.

To reproduce:

cmake -B build -DBUILD_SHARED_LIBS=ON -DCMAKE_CXX_FLAGS="-Og -g3" --fresh
cmake --build build -j$(nproc) --clean-first
cmake --install build --prefix ORIG
wget https://patch-diff.githubusercontent.com/raw/open-telemetry/opentelemetry-cpp/pull/3147.diff
patch -p1 < 3147.diff
cmake --build build -j$(nproc)
cmake --install build --prefix NEW
mkdir dumps
for i in ORIG/lib/*.so; do abi-dumper "$i" -o "dumps/orig-$(basename "$i").dump" -lver orig; done
for i in NEW/lib/*.so; do abi-dumper "$i" -o "dumps/new-$(basename "$i").dump" -lver new; done
for i in NEW/lib/*.so; do abi-compliance-checker -l "$(basename "$i")" -old "dumps/orig-$(basename "$i").dump" -new "dumps/new-$(basename "$i").dump; done

It will show smth like

...
Preparing, please wait ...
Comparing ABIs ...
Comparing APIs ...
Creating compatibility report ...
Binary compatibility: 85.3%
Source compatibility: 100%
Total binary compatibility problems: 1, warnings: 2
Total source compatibility problems: 0, warnings: 0
Report: compat_reports/libopentelemetry_logs.so/orig_to_new/compat_report.html
...

If I didn't miss anything, I think we only need to keep ABI compatibility for api. These changes keep API compatibility for sdk and exporters.

[sjinks]

I think we only need to keep ABI compatibility for API.

I noticed that the library does not have a SOVERSION and thought that the changes could break the existing consumers (I know that Alpine Linux ships OpenTelemetry C++ libraries). That's why I asked :-)

[owent]

I think we only need to keep ABI compatibility for API.

I noticed that the library does not have a SOVERSION and thought that the changes could break the existing consumers (I know that Alpine Linux ships OpenTelemetry C++ libraries). That's why I asked :-)

If we build otel-cpp with cmake, we can use -DOTELCPP_VERSIONED_LIBS=ON to set SOVERSION. I'm not familar with bazel

I think we only need to keep ABI compatibility for API.

I noticed that the library does not have a SOVERSION and thought that the changes could break the existing consumers (I know that Alpine Linux ships OpenTelemetry C++ libraries). That's why I asked :-)

Thanks. I noticed #3110 and agree with the SOVERSION idea. Let's continue discussing it there.

[lalitb]

@owent Thanks for the PR. With the use of the MixedAttributeMap construct, ReadWriteLogRecord now maintains both owning and non-owning values for attributes and body. While this ensures flexibility, it also introduces extra overhead when only non-owning types are required by specific custom exporters.

How about having two separate recordables:

• ReadWriteLogRecord - Retains the current behavior, maintaining non-owning values for attributes and body.
• LogRecordData (Similar to SpanData) - this new structure would store owning values, and used for ostream exporter.

[owent]

@owent Thanks for the PR. With the use of the MixedAttributeMap construct, ReadWriteLogRecord now maintains both owning and non-owning values for attributes and body. While this ensures flexibility, it also introduces extra overhead when only non-owning types are required by specific custom exporters.

How about having two separate recordables:

• ReadWriteLogRecord - Retains the current behavior, maintaining non-owning values for attributes and body.
• LogRecordData (Similar to SpanData) - this new structure would store owning values, and used for ostream exporter.

Good point. I will try to implement them later.