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
• Unit tests have been added
• Changes in public API reviewed

[netlify]

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

Name	Link
🔨 Latest commit	d752e87
🔍 Latest deploy log	https://app.netlify.com/projects/opentelemetry-cpp-api-docs/deploys/682cb1d02511380008aa8dec

[codecov]

Codecov Report

Attention: Patch coverage is 97.56098% with 5 lines in your changes missing coverage. Please review.

Project coverage is 89.71%. Comparing base (2faa400) to head (d752e87).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
sdk/src/logs/log_record_data.cc	96.39%	3 Missing ⚠️
...include/opentelemetry/sdk/common/attribute_utils.h	98.10%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3147      +/-   ##
==========================================
- Coverage   90.06%   89.71%   -0.35%     
==========================================
  Files         212      213       +1     
  Lines        6939     7127     +188     
==========================================
+ Hits         6249     6393     +144     
- Misses        690      734      +44     

Files with missing lines	Coverage Δ
exporters/ostream/src/log_record_exporter.cc	89.71% <100.00%> (-7.35%)	⬇️
exporters/ostream/test/ostream_log_test.cc	96.18% <100.00%> (ø)
sdk/src/logs/read_write_log_record.cc	73.50% <100.00%> (-22.89%)	⬇️
...include/opentelemetry/sdk/common/attribute_utils.h	96.50% <98.10%> (+2.82%)	⬆️
sdk/src/logs/log_record_data.cc	96.39% <96.39%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[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.
Done.

[lalitb]

@owent - Do we still need this class?

[owent]

It's still used by MixedAttributeMap , which is used by LogRecordData.

[lalitb]

@owent As we agreed here, can we implement the LogRecordData to only contain the owning types, not the mixed types. The motive is to keep the public API simple and minimal. And this will be consistent with SpanData implementation.

[owent]

I think there are some differents between LogRecordData and SpanData.

SpanData::GetAttributes returns std::unordered_map<std::string, opentelemetry::sdk::common::OwnedAttributeValue> but LogRecordData::GetAttributes returns std::unordered_map<std::string, opentelemetry::common::AttributeValue>.
We need storage to store span<T> in opentelemetry::common::AttributeValue, or do you think we can break the API of
ReadableLogRecord::GetAttributes ?

@lalitb

[owent]

Any idea about keep the mixed types or change the API of ReadableLogRecord::GetAttributes?

[owent]

I just raised another approach which will change the return types in ABI v2 to keep the same as LogRecordData.
Could you please review again? @lalitb @sjinks @marcalff

[marcalff]

Another note to myself that I really really need to review this.

@owent Could you refresh the patch and merge with a recent main ? Also, make sure there are no changes under third_party.

Thanks.

[owent]

Another note to myself that I really really need to review this.

@owent Could you refresh the patch and merge with a recent main ? Also, make sure there are no changes under third_party.

Thanks.

Thanks, conflicts are resolved.

[ThomsonTan]

Is the update to the prometheus-cpp submodule expected?

[marcalff]

I don't understand all the reasoning that lead to this current patch, it looks way too complicated to me.

Either I totally missed something, or it can be done in a simpler way.

Please see a different attempt to resolve the same issues, and comment:

• [EXPORTER] ostream log exporter, fix memory ownership issues #3417

[owent]

Is the update to the prometheus-cpp submodule expected?

Sorry, it's a mistake and it's removed now.

[marcalff]

See comments, do not use the ABI version here.

[marcalff]

I disagree with using OPENTELEMETRY_ABI_VERSION_NO here:

• The content of api/include has not changed in this PR, the ABI for the instrumentation interface is strictly the same before and after the fix, so there is no ABI change. The symbol OPENTELEMETRY_ABI_VERSION_NO is dedicated to the API part, it is not meant to keep track of SDK ABI changes, that can happen independently.

• Using a flag to provide a migration path is fine. If we want to do it, the flag should be similar to WITH_DEPRECATED_SDK_FACTORY / OPENTELEMETRY_DEPRECATED_SDK_FACTORY introduced by PR [API/SDK] Provider cleanup #2664 for example. Do not use OPENTELEMETRY_ABI_VERSION_NO for this.

Now, there is more to this:

• This change does not affect the instrumentation interface, so no instrumented code will be broken by this.

• This change does not affect the SDK configuration interface (defining an exporter, etc), so no application simply using the SDK to configure opentelemetry-cpp will be affected.

• The only way some user code can be affected is if the user code implements its own log record exporter, and reuses the ReadableLogRecord class from the SDK instead of defining its own recordable for its own implementation.

• Even if this happen to be the case, the user log record exporter is then by definition broken and crashing, the same way the ostream exporter is crashing today, precisely because this class implementation uses stale pointers.

In conclusion, I don't think using a feature flag to provide the fix (or not) resolves anything. To the contrary, it complicates the implementation.

[owent]

I agree about the flag. However, regarding compatibility, the issue isn't a crash. The return type has changed, so if users implement their own exporters using ReadableLogRecord, their code won't compile. I think we can allow users time to migrate from the old SDK to the new version, rather than introducing an immediate breaking change.

[marcalff]

The return type has changed, so if users implement their own exporters using ReadableLogRecord, their code won't compile.

Correct.

But also note that if a user has such code in a custom exporter, that exporter should be currently crashing, when using stale pointers in body or attributes.

Forcing users to fix a broken custom exporter implementation might not be that bad after all, even if the visible effect is a build break.

I think we can allow users time to migrate from the old SDK to the new version, rather than introducing an immediate breaking change.

Assuming we want to take this path, it requires:

• to introduce a build flag, to expose the new return type versus the old one.
• to announce building with the old return type is deprecated, and will be removed in the future
• to raise a build deprecation warning, to force users to notice
• to document the migration path, and explain using the build flag is preferable
• in the next release, to remove the build flag, and only keep the new implementation
• in the next release notes, document that users have to adjust their code (they had 1 full release to do so)

This is technically possible, and has been done in the past.

But this has been done in cases where:

• the SDK configuration api got changed
• every application configuring the SDK was known to be affected

so in short when the amount of code / users affected was significant.

Here, the SDK configuration has not changed, and most if not all applications will require no changes at all.

Again, the only impact is for applications that:

• defines their own log record exporter
• reuse an SDK class in their implementation (instead of implementing their own recordable)

Taking dependencies on the SDK implementation implies some risks, applications doing this should be aware of possible consequences.

I understand this particular point is a matter of assessment and opinions, but in this case I think having the full build flag and migration path for a use case that is expected to be very narrow (custom exporter), is overkill.

[owent]

There are a lot of commits before, I also rebase and squash it from mian again.

[owent]

Close as #3417 is more simple when breaking the ABI and APIs is acceptable.

[marcalff]

Close as #3417 is more simple when breaking the ABI and APIs is acceptable.

Thanks @owent