Fix lifetime for sdk::ReadWriteLogRecord #3147

owent · 2024-11-15T11:37:46Z

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 · 2024-11-15T11:38:10Z

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

Name	Link
🔨 Latest commit	`2f89435`
🔍 Latest deploy log	https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/676566bcc4ae31000837ca11

codecov · 2024-11-15T14:40:04Z

Codecov Report

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

Project coverage is 88.18%. Comparing base (4998eb1) to head (2f89435).

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   88.16%   88.18%   +0.02%     
==========================================
  Files         198      199       +1     
  Lines        6224     6412     +188     
==========================================
+ Hits         5487     5654     +167     
- Misses        737      758      +21

Files with missing lines	Coverage Δ
exporters/ostream/src/log_record_exporter.cc	`97.06% <100.00%> (ø)`
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 1 file with indirect coverage changes

sjinks · 2024-11-16T10:19:17Z

sdk/include/opentelemetry/sdk/common/attribute_utils.h

+struct MixedAttributeMapStorage
+{
+  std::unordered_map<std::string, opentelemetry::common::AttributeValue> attributes;
+  AttributeMap owened_attributes;


Looks like a typo:

Suggested change

AttributeMap owened_attributes;

AttributeMap owned_attributes;

Thanks, it's fixed now.

sjinks · 2024-11-16T11:19:06Z

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 · 2024-11-16T15:42:52Z

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:

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.

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 · 2024-11-16T17:58:39Z

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 · 2024-11-18T02:21:31Z

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 · 2024-11-22T01:47:53Z

@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 · 2024-11-22T05:59:13Z

@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.

…ecord

…ibutes

lalitb · 2024-12-06T02:41:23Z

sdk/include/opentelemetry/sdk/common/attribute_utils.h

+/**
+ * Set an owned copy (OwnedAttributeValue) and attribute view of a non-owning AttributeValue.
+ */
+class MixedAttributeViewSetter


@owent - Do we still need this class?

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

@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.

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

…ecord

Fix lifetime for sdk::ReadWriteLogRecord

aa4471d

owent requested a review from a team as a code owner November 15, 2024 11:37

owent added 2 commits November 15, 2024 20:21

Fix unit test

8c86bbd

Fix unit test

cae98c6

owent changed the title ~~[WIP]Fix lifetime for sdk::ReadWriteLogRecord~~ Fix lifetime for sdk::ReadWriteLogRecord Nov 15, 2024

Merge branch 'main' into fix_lifetime_in_log_record

b933ee8

sjinks reviewed Nov 16, 2024

View reviewed changes

Fix a typo

cfd5db3

sjinks mentioned this pull request Nov 16, 2024

Please add number version in SONAMEs. #3110

Open

marcalff requested review from lalitb and marcalff November 18, 2024 21:25

owent added 2 commits November 20, 2024 10:42

Merge branch 'main' into fix_lifetime_in_log_record

67f66c7

Merge branch 'main' into fix_lifetime_in_log_record

4b8012d

lalitb and others added 6 commits November 22, 2024 15:00

Merge branch 'main' into fix_lifetime_in_log_record

5595d31

Merge remote-tracking branch 'github/main' into fix_lifetime_in_log_r…

40c4632

…ecord

Implement a new LogRecordData to store both Owned attributes and attr…

282efec

…ibutes

Merge branch 'main' into fix_lifetime_in_log_record

3da4bed

Merge branch 'main' into fix_lifetime_in_log_record

cf758d1

Merge branch 'main' into fix_lifetime_in_log_record

da602f3

lalitb reviewed Dec 6, 2024

View reviewed changes

lalitb and others added 2 commits December 6, 2024 10:38

Merge branch 'main' into fix_lifetime_in_log_record

94cf407

Merge branch 'main' into fix_lifetime_in_log_record

68178bf

owent and others added 2 commits December 12, 2024 11:32

Merge branch 'main' into fix_lifetime_in_log_record

4327f87

Merge remote-tracking branch 'github/main' into fix_lifetime_in_log_r…

2f89435

…ecord

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix lifetime for sdk::ReadWriteLogRecord #3147

Fix lifetime for sdk::ReadWriteLogRecord #3147

owent commented Nov 15, 2024

netlify bot commented Nov 15, 2024 •

edited

Loading

codecov bot commented Nov 15, 2024 •

edited

Loading

sjinks Nov 16, 2024

owent Nov 16, 2024

sjinks commented Nov 16, 2024

owent commented Nov 16, 2024

sjinks commented Nov 16, 2024

owent commented Nov 18, 2024

lalitb commented Nov 22, 2024 •

edited

Loading

owent commented Nov 22, 2024 •

edited

Loading

lalitb Dec 6, 2024

owent Dec 7, 2024

lalitb Dec 12, 2024

owent Dec 20, 2024 •

edited

Loading

	AttributeMap owened_attributes;
	AttributeMap owned_attributes;

Fix lifetime for sdk::ReadWriteLogRecord #3147

Are you sure you want to change the base?

Fix lifetime for sdk::ReadWriteLogRecord #3147

Conversation

owent commented Nov 15, 2024

Changes

netlify bot commented Nov 15, 2024 • edited Loading

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

codecov bot commented Nov 15, 2024 • edited Loading

Codecov Report

sjinks Nov 16, 2024

Choose a reason for hiding this comment

owent Nov 16, 2024

Choose a reason for hiding this comment

sjinks commented Nov 16, 2024

owent commented Nov 16, 2024

sjinks commented Nov 16, 2024

owent commented Nov 18, 2024

lalitb commented Nov 22, 2024 • edited Loading

owent commented Nov 22, 2024 • edited Loading

lalitb Dec 6, 2024

Choose a reason for hiding this comment

owent Dec 7, 2024

Choose a reason for hiding this comment

lalitb Dec 12, 2024

Choose a reason for hiding this comment

owent Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

netlify bot commented Nov 15, 2024 •

edited

Loading

codecov bot commented Nov 15, 2024 •

edited

Loading

lalitb commented Nov 22, 2024 •

edited

Loading

owent commented Nov 22, 2024 •

edited

Loading

owent Dec 20, 2024 •

edited

Loading