Skip to content

Commit

Permalink
[SHIM] Add size to all string view mappings between OT and OTel (#3181)
Browse files Browse the repository at this point in the history
  • Loading branch information
chusitoo authored Dec 2, 2024
1 parent fb6fde8 commit f167c36
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ class CarrierWriterShim : public opentelemetry::context::propagation::TextMapCar

virtual void Set(nostd::string_view key, nostd::string_view value) noexcept override
{
writer_.Set(key.data(), value.data());
writer_.Set(opentracing::string_view{key.data(), key.size()},
opentracing::string_view{value.data(), value.size()});
}

private:
Expand All @@ -46,17 +47,17 @@ class CarrierReaderShim : public opentelemetry::context::propagation::TextMapCar
nostd::string_view value;

// First try carrier.LookupKey since that can potentially be the fastest approach.
if (auto result = reader_.LookupKey(key.data()))
if (auto result = reader_.LookupKey(opentracing::string_view{key.data(), key.size()}))
{
value = result.value().data();
value = nostd::string_view{result.value().data(), result.value().size()};
}
else // Fall back to iterating through all of the keys.
{
reader_.ForeachKey([key, &value](opentracing::string_view k,
opentracing::string_view v) -> opentracing::expected<void> {
if (k == key.data())
if (key == nostd::string_view{k.data(), k.size()})
{
value = v.data();
value = nostd::string_view{v.data(), v.size()};
// Found key, so bail out of the loop with a success error code.
return opentracing::make_unexpected(std::error_code{});
}
Expand All @@ -77,8 +78,9 @@ class CarrierReaderShim : public opentelemetry::context::propagation::TextMapCar
return reader_
.ForeachKey([&callback](opentracing::string_view key,
opentracing::string_view) -> opentracing::expected<void> {
return callback(key.data()) ? opentracing::make_expected()
: opentracing::make_unexpected(std::error_code{});
return callback(nostd::string_view{key.data(), key.size()})
? opentracing::make_expected()
: opentracing::make_unexpected(std::error_code{});
})
.has_value();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ static inline opentelemetry::common::AttributeValue attributeFromValue(
AttributeValue operator()(int64_t v) { return v; }
AttributeValue operator()(uint64_t v) { return v; }
AttributeValue operator()(const std::string &v) { return nostd::string_view{v}; }
AttributeValue operator()(opentracing::string_view v) { return nostd::string_view{v.data()}; }
AttributeValue operator()(opentracing::string_view v)
{
return nostd::string_view{v.data(), v.size()};
}
AttributeValue operator()(std::nullptr_t) { return nostd::string_view{}; }
AttributeValue operator()(const char *v) { return v; }
AttributeValue operator()(opentracing::util::recursive_wrapper<opentracing::Values>)
Expand All @@ -54,7 +57,7 @@ static inline std::string stringFromValue(const opentracing::Value &value)
std::string operator()(int64_t v) { return std::to_string(v); }
std::string operator()(uint64_t v) { return std::to_string(v); }
std::string operator()(const std::string &v) { return v; }
std::string operator()(opentracing::string_view v) { return std::string{v.data()}; }
std::string operator()(opentracing::string_view v) { return std::string{v.data(), v.size()}; }
std::string operator()(std::nullptr_t) { return std::string{}; }
std::string operator()(const char *v) { return std::string{v}; }
std::string operator()(opentracing::util::recursive_wrapper<opentracing::Values>)
Expand Down
2 changes: 1 addition & 1 deletion opentracing-shim/src/span_context_shim.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ bool SpanContextShim::BaggageItem(nostd::string_view key, std::string &value) co
void SpanContextShim::ForeachBaggageItem(VisitBaggageItem f) const
{
baggage_->GetAllEntries([&f](nostd::string_view key, nostd::string_view value) {
return f(key.data(), value.data());
return f(std::string{key.data(), key.size()}, std::string{value.data(), value.size()});
});
}

Expand Down
14 changes: 9 additions & 5 deletions opentracing-shim/src/span_shim.cc
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ void SpanShim::FinishWithOptions(const opentracing::FinishSpanOptions &finish_sp

void SpanShim::SetOperationName(opentracing::string_view name) noexcept
{
span_->UpdateName(name.data());
span_->UpdateName(nostd::string_view{name.data(), name.size()});
}

void SpanShim::SetTag(opentracing::string_view key, const opentracing::Value &value) noexcept
Expand All @@ -57,7 +57,8 @@ void SpanShim::SetTag(opentracing::string_view key, const opentracing::Value &va
}
else
{
span_->SetAttribute(key.data(), utils::attributeFromValue(value));
auto key_view = nostd::string_view{key.data(), key.size()};
span_->SetAttribute(key_view, utils::attributeFromValue(value));
}
}

Expand All @@ -68,9 +69,11 @@ void SpanShim::SetBaggageItem(opentracing::string_view restricted_key,
// Baggage key/value pair, and sets it as the current instance for this Span Shim.
if (restricted_key.empty() || value.empty())
return;
auto restricted_key_view = nostd::string_view{restricted_key.data(), restricted_key.size()};
auto value_view = nostd::string_view{value.data(), value.size()};
// This operation MUST be safe to be called concurrently.
const std::lock_guard<decltype(context_lock_)> guard(context_lock_);
context_ = context_.newWithKeyValue(restricted_key.data(), value.data());
context_ = context_.newWithKeyValue(restricted_key_view, value_view);
}

std::string SpanShim::BaggageItem(opentracing::string_view restricted_key) const noexcept
Expand All @@ -82,7 +85,8 @@ std::string SpanShim::BaggageItem(opentracing::string_view restricted_key) const
// This operation MUST be safe to be called concurrently.
const std::lock_guard<decltype(context_lock_)> guard(context_lock_);
std::string value;
return context_.BaggageItem(restricted_key.data(), value) ? value : "";
auto restricted_key_view = nostd::string_view{restricted_key.data(), restricted_key.size()};
return context_.BaggageItem(restricted_key_view, value) ? value : "";
}

void SpanShim::Log(std::initializer_list<EventEntry> fields) noexcept
Expand Down Expand Up @@ -128,7 +132,7 @@ void SpanShim::logImpl(nostd::span<const EventEntry> fields,

for (const auto &entry : fields)
{
nostd::string_view key = entry.first.data();
nostd::string_view key{entry.first.data(), entry.first.size()};
// ... including mapping of the following key/value pairs:
if (is_error)
{
Expand Down
13 changes: 7 additions & 6 deletions opentracing-shim/src/tracer_shim.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,13 @@ std::unique_ptr<opentracing::Span> TracerShim::StartSpanWithOptions(
if (is_closed_)
return nullptr;

const auto &opts = utils::makeOptionsShim(options);
const auto &links = utils::makeIterableLinks(options);
const auto &attributes = utils::makeIterableTags(options);
const auto &baggage = utils::makeBaggage(options);
auto span = tracer_->StartSpan(operation_name.data(), attributes, links, opts);
auto span_shim = new (std::nothrow) SpanShim(*this, span, baggage);
const auto &opts = utils::makeOptionsShim(options);
const auto &links = utils::makeIterableLinks(options);
const auto &attributes = utils::makeIterableTags(options);
const auto &baggage = utils::makeBaggage(options);
auto operation_name_view = nostd::string_view{operation_name.data(), operation_name.size()};
auto span = tracer_->StartSpan(operation_name_view, attributes, links, opts);
auto span_shim = new (std::nothrow) SpanShim(*this, span, baggage);

// If an initial set of tags is specified and the OpenTracing error tag
// is included after the OpenTelemetry Span was created.
Expand Down

1 comment on commit f167c36

@github-actions
Copy link

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'OpenTelemetry-cpp api Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: f167c36 Previous: fb6fde8 Ratio
BM_SpinLockThrashing/2/process_time/real_time 0.3917441384912798 ms/iter 0.18420370062530259 ms/iter 2.13
BM_SpinLockThrashing/4/process_time/real_time 7.885935306549072 ms/iter 0.6938731228863751 ms/iter 11.37
BM_ProcYieldSpinLockThrashing/1/process_time/real_time 0.2181334951059605 ms/iter 0.09551608871128944 ms/iter 2.28
BM_ProcYieldSpinLockThrashing/4/process_time/real_time 1.9370583926930147 ms/iter 0.7629765130077619 ms/iter 2.54
BM_NaiveSpinLockThrashing/4/process_time/real_time 3.2636277815874886 ms/iter 0.7416451642961066 ms/iter 4.40

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.