Skip to content
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

Fix lifetime for sdk::ReadWriteLogRecord #3147

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
aa4471d
Fix lifetime for sdk::ReadWriteLogRecord
owent Nov 15, 2024
8c86bbd
Fix unit test
owent Nov 15, 2024
cae98c6
Fix unit test
owent Nov 15, 2024
b933ee8
Merge branch 'main' into fix_lifetime_in_log_record
owent Nov 15, 2024
cfd5db3
Fix a typo
owent Nov 16, 2024
67f66c7
Merge branch 'main' into fix_lifetime_in_log_record
owent Nov 20, 2024
4b8012d
Merge branch 'main' into fix_lifetime_in_log_record
owent Nov 21, 2024
5595d31
Merge branch 'main' into fix_lifetime_in_log_record
lalitb Nov 22, 2024
40c4632
Merge remote-tracking branch 'github/main' into fix_lifetime_in_log_r…
owent Nov 25, 2024
282efec
Implement a new LogRecordData to store both Owned attributes and attr…
owent Nov 25, 2024
3da4bed
Merge branch 'main' into fix_lifetime_in_log_record
owent Nov 26, 2024
cf758d1
Merge branch 'main' into fix_lifetime_in_log_record
owent Nov 27, 2024
da602f3
Merge branch 'main' into fix_lifetime_in_log_record
owent Dec 5, 2024
94cf407
Merge branch 'main' into fix_lifetime_in_log_record
lalitb Dec 6, 2024
68178bf
Merge branch 'main' into fix_lifetime_in_log_record
owent Dec 7, 2024
4327f87
Merge branch 'main' into fix_lifetime_in_log_record
owent Dec 12, 2024
2f89435
Merge remote-tracking branch 'github/main' into fix_lifetime_in_log_r…
owent Dec 20, 2024
a3cffa7
Merge branch 'main' into fix_lifetime_in_log_record
owent Jan 8, 2025
1fc10b0
Merge branch 'main' into fix_lifetime_in_log_record
owent Jan 21, 2025
b4bfe00
Merge branch 'main' into fix_lifetime_in_log_record
owent Jan 22, 2025
b173dd7
Merge remote-tracking branch 'opentelemetry/main' into fix_lifetime_i…
owent Jan 28, 2025
b5198ae
Fixes iwyu
owent Jan 29, 2025
156c229
Merge remote-tracking branch 'opentelemetry/main' into fix_lifetime_i…
owent Jan 29, 2025
fabc4a0
Merge branch 'main' into fix_lifetime_in_log_record
owent Jan 31, 2025
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
3 changes: 3 additions & 0 deletions exporters/otlp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,9 @@ if(BUILD_TESTING)
else()
find_library(GMOCK_LIB gmock PATH_SUFFIXES lib)
endif()
if(NOT GMOCK_LIB)
unset(GMOCK_LIB CACHE)
endif()
endif()

if(WITH_OTLP_GRPC)
Expand Down
3 changes: 3 additions & 0 deletions exporters/zipkin/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ if(BUILD_TESTING)
else()
find_library(GMOCK_LIB gmock PATH_SUFFIXES lib)
endif()
if(NOT GMOCK_LIB)
unset(GMOCK_LIB CACHE)
endif()

add_executable(zipkin_exporter_test test/zipkin_exporter_test.cc)

Expand Down
245 changes: 245 additions & 0 deletions sdk/include/opentelemetry/sdk/common/attribute_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <cstdint>
#include <initializer_list>
#include <map>
#include <memory>
#include <string>
#include <unordered_map>
#include <utility>
Expand Down Expand Up @@ -49,6 +50,8 @@ using OwnedAttributeValue = nostd::variant<bool,
std::vector<uint64_t>,
std::vector<uint8_t>>;

using OwnedAttributeView = nostd::variant<std::vector<nostd::string_view>, std::unique_ptr<bool[]>>;

enum OwnedAttributeType
{
kTypeBool,
Expand Down Expand Up @@ -215,6 +218,248 @@ class OrderedAttributeMap : public std::map<std::string, OwnedAttributeValue>
AttributeConverter converter_;
};

/**
* Class for storing attributes.
*/
struct MixedAttributeMapStorage
{
std::unordered_map<std::string, opentelemetry::common::AttributeValue> attributes;
AttributeMap owened_attributes;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a typo:

Suggested change
AttributeMap owened_attributes;
AttributeMap owned_attributes;

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, it's fixed now.

std::unordered_map<std::string, OwnedAttributeView> owened_attributes_view;
};

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

Choose a reason for hiding this comment

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

@owent - Do we still need this class?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@owent owent Dec 20, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

{
public:
inline MixedAttributeViewSetter(const nostd::string_view &key,
MixedAttributeMapStorage &storage,
AttributeConverter &converter) noexcept
: key_(&key), storage_(&storage), converter_(&converter)
{}

void operator()(bool v)
{
storage_->owened_attributes[std::string(*key_)] = (*converter_)(v);
storage_->attributes[std::string(*key_)] = v;
}

void operator()(int32_t v)
{
storage_->owened_attributes[std::string(*key_)] = (*converter_)(v);
storage_->attributes[std::string(*key_)] = v;
}

void operator()(uint32_t v)
{
storage_->owened_attributes[std::string(*key_)] = (*converter_)(v);
storage_->attributes[std::string(*key_)] = v;
}

void operator()(int64_t v)
{
storage_->owened_attributes[std::string(*key_)] = (*converter_)(v);
storage_->attributes[std::string(*key_)] = v;
}

void operator()(uint64_t v)
{
storage_->owened_attributes[std::string(*key_)] = (*converter_)(v);
storage_->attributes[std::string(*key_)] = v;
}

void operator()(double v)
{
storage_->owened_attributes[std::string(*key_)] = (*converter_)(v);
storage_->attributes[std::string(*key_)] = v;
}

void operator()(nostd::string_view v)
{
auto &owned_value = storage_->owened_attributes[std::string(*key_)];
owned_value = (*converter_)(v);
storage_->attributes[std::string(*key_)] = nostd::get<std::string>(owned_value);
}

void operator()(const char *v)
{
auto &owned_value = storage_->owened_attributes[std::string(*key_)];
owned_value = (*converter_)(v);
storage_->attributes[std::string(*key_)] = nostd::get<std::string>(owned_value).c_str();
}

void operator()(nostd::span<const uint8_t> v)
{
auto &owned_value = storage_->owened_attributes[std::string(*key_)];
owned_value = (*converter_)(v);
storage_->attributes[std::string(*key_)] = nostd::get<std::vector<uint8_t>>(owned_value);
}

void operator()(nostd::span<const bool> v)
{
storage_->owened_attributes[std::string(*key_)] = (*converter_)(v);
if (v.empty())
{
storage_->attributes[std::string(*key_)] = nostd::span<const bool>{};
}
else
{
std::unique_ptr<bool[]> owned_view{new bool[v.size()]};
for (size_t i = 0; i < v.size(); i++)
{
owned_view[i] = v[i];
}

storage_->attributes[std::string(*key_)] =
nostd::span<const bool>{owned_view.get(), v.size()};
storage_->owened_attributes_view[std::string(*key_)] = std::move(owned_view);
}
}

void operator()(nostd::span<const int32_t> v)
{
auto &owned_value = storage_->owened_attributes[std::string(*key_)];
owned_value = (*converter_)(v);
storage_->attributes[std::string(*key_)] = nostd::get<std::vector<int32_t>>(owned_value);
}

void operator()(nostd::span<const uint32_t> v)
{
auto &owned_value = storage_->owened_attributes[std::string(*key_)];
owned_value = (*converter_)(v);
storage_->attributes[std::string(*key_)] = nostd::get<std::vector<uint32_t>>(owned_value);
}

void operator()(nostd::span<const int64_t> v)
{
auto &owned_value = storage_->owened_attributes[std::string(*key_)];
owned_value = (*converter_)(v);
storage_->attributes[std::string(*key_)] = nostd::get<std::vector<int64_t>>(owned_value);
}

void operator()(nostd::span<const uint64_t> v)
{
auto &owned_value = storage_->owened_attributes[std::string(*key_)];
owned_value = (*converter_)(v);
storage_->attributes[std::string(*key_)] = nostd::get<std::vector<uint64_t>>(owned_value);
}

void operator()(nostd::span<const double> v)
{
auto &owned_value = storage_->owened_attributes[std::string(*key_)];
owned_value = (*converter_)(v);
storage_->attributes[std::string(*key_)] = nostd::get<std::vector<double>>(owned_value);
}

void operator()(nostd::span<const nostd::string_view> v)
{
auto &owned_value = storage_->owened_attributes[std::string(*key_)];
owned_value = (*converter_)(v);

if (v.empty())
{
storage_->attributes[std::string(*key_)] = nostd::span<const nostd::string_view>{};
}
else
{
auto &owned_view = storage_->owened_attributes_view[std::string(*key_)];
owned_view = std::vector<nostd::string_view>{};
auto &owned_vector = nostd::get<std::vector<nostd::string_view>>(owned_view);

owned_vector.reserve(v.size());
for (auto &data : nostd::get<std::vector<std::string>>(owned_value))
{
owned_vector.push_back(data);
}

storage_->attributes[std::string(*key_)] =
nostd::span<const nostd::string_view>{owned_vector.data(), owned_vector.size()};
}
}

private:
const nostd::string_view *key_;
MixedAttributeMapStorage *storage_;
AttributeConverter *converter_;
};

/**
* Class for storing attributes and attribute view.
*/
class MixedAttributeMap
{
public:
// Construct empty attribute map
MixedAttributeMap() {}

// Construct attribute map and populate with attributes
MixedAttributeMap(const opentelemetry::common::KeyValueIterable &attributes) : MixedAttributeMap()
{
attributes.ForEachKeyValue(
[&](nostd::string_view key, opentelemetry::common::AttributeValue value) noexcept {
nostd::visit(MixedAttributeViewSetter(key, storage_, converter_), value);
return true;
});
}

// Construct attribute map and populate with optional attributes
MixedAttributeMap(const opentelemetry::common::KeyValueIterable *attributes) : MixedAttributeMap()
{
if (attributes != nullptr)
{
attributes->ForEachKeyValue(
[&](nostd::string_view key, opentelemetry::common::AttributeValue value) noexcept {
nostd::visit(MixedAttributeViewSetter(key, storage_, converter_), value);
return true;
});
}
}

// Construct map from initializer list by applying `SetAttribute` transform for every attribute
MixedAttributeMap(
std::initializer_list<std::pair<nostd::string_view, opentelemetry::common::AttributeValue>>
attributes)
: MixedAttributeMap()
{
for (auto &kv : attributes)
{
nostd::visit(MixedAttributeViewSetter(kv.first, storage_, converter_), kv.second);
}
}

// Returns a reference to this map
const std::unordered_map<std::string, opentelemetry::common::AttributeValue> &GetAttributes()
const noexcept
{
return storage_.attributes;
}

const AttributeMap &GetOwnedAttributes() const noexcept { return storage_.owened_attributes; }

// Convert non-owning key-value to owning std::string(key) and OwnedAttributeValue(value)
void SetAttribute(nostd::string_view key,
const opentelemetry::common::AttributeValue &value) noexcept
{
nostd::visit(MixedAttributeViewSetter(key, storage_, converter_), value);
}

void Reserve(AttributeMap::size_type size)
{
storage_.attributes.reserve(size);
storage_.owened_attributes.reserve(size);
}

AttributeMap::size_type Size() const noexcept { return storage_.attributes.size(); }

bool Empty() const noexcept { return storage_.attributes.empty(); }

private:
MixedAttributeMapStorage storage_;
AttributeConverter converter_;
};

} // namespace common
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE
5 changes: 3 additions & 2 deletions sdk/include/opentelemetry/sdk/logs/read_write_log_record.h
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,9 @@ class ReadWriteLogRecord final : public ReadableLogRecord
const opentelemetry::sdk::resource::Resource *resource_;
const opentelemetry::sdk::instrumentationscope::InstrumentationScope *instrumentation_scope_;

std::unordered_map<std::string, opentelemetry::common::AttributeValue> attributes_map_;
opentelemetry::common::AttributeValue body_;
common::MixedAttributeMap attributes_map_;
// We resue the same utility functions of MixedAttributeMap with key="" for the body field
common::MixedAttributeMap body_;
opentelemetry::common::SystemTimestamp timestamp_;
opentelemetry::common::SystemTimestamp observed_timestamp_;

Expand Down
13 changes: 7 additions & 6 deletions sdk/src/logs/read_write_log_record.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,12 @@ ReadWriteLogRecord::ReadWriteLogRecord()
: severity_(opentelemetry::logs::Severity::kInvalid),
resource_(nullptr),
instrumentation_scope_(nullptr),
body_(nostd::string_view()),
observed_timestamp_(std::chrono::system_clock::now()),
event_id_(0),
event_name_("")
{}
{
body_.SetAttribute("", nostd::string_view());
}

ReadWriteLogRecord::~ReadWriteLogRecord() {}

Expand Down Expand Up @@ -70,12 +71,12 @@ opentelemetry::logs::Severity ReadWriteLogRecord::GetSeverity() const noexcept

void ReadWriteLogRecord::SetBody(const opentelemetry::common::AttributeValue &message) noexcept
{
body_ = message;
body_.SetAttribute("", message);
}

const opentelemetry::common::AttributeValue &ReadWriteLogRecord::GetBody() const noexcept
{
return body_;
return body_.GetAttributes().begin()->second;
}

void ReadWriteLogRecord::SetEventId(int64_t id, nostd::string_view name) noexcept
Expand Down Expand Up @@ -160,13 +161,13 @@ const opentelemetry::trace::TraceFlags &ReadWriteLogRecord::GetTraceFlags() cons
void ReadWriteLogRecord::SetAttribute(nostd::string_view key,
const opentelemetry::common::AttributeValue &value) noexcept
{
attributes_map_[static_cast<std::string>(key)] = value;
attributes_map_.SetAttribute(key, value);
}

const std::unordered_map<std::string, opentelemetry::common::AttributeValue> &
ReadWriteLogRecord::GetAttributes() const noexcept
{
return attributes_map_;
return attributes_map_.GetAttributes();
}

const opentelemetry::sdk::resource::Resource &ReadWriteLogRecord::GetResource() const noexcept
Expand Down
Loading