Open nic-godz opened 2 weeks ago
I created a specific Boost Beast request carrier:
template <typename T>
class BoostBeastRequestCarrier : public opentelemetry::context::propagation::TextMapCarrier
{
public:
BoostBeastRequestCarrier(boost::beast::http::request<T>& request) :
request_(request),
traceparent_(request_.find(std::string(opentelemetry::trace::propagation::kTraceParent))->value()),
tracestate_(request_.find(std::string(opentelemetry::trace::propagation::kTraceState))->value())
{}
virtual opentelemetry::nostd::string_view Get(opentelemetry::nostd::string_view key) const noexcept override
{
if (key == opentelemetry::trace::propagation::kTraceParent)
{
return traceparent_;
}
else if (key == opentelemetry::trace::propagation::kTraceState)
{
return tracestate_;
}
return "";
}
virtual void Set(opentelemetry::nostd::string_view key, opentelemetry::nostd::string_view value) noexcept override
{
request_.set(std::string(key), std::string(value));
if (key == opentelemetry::trace::propagation::kTraceParent)
{
traceparent_ = std::string(value);
}
else if (key == opentelemetry::trace::propagation::kTraceState)
{
tracestate_ = std::string(value);
}
}
boost::beast::http::request<T>& request_;
// Needed as temp buffer as opentelemetry::nostd::string_view is not compatible with std::string_view:
std::string traceparent_;
std::string tracestate_;
};
Would be nice to avoid all creation of temporary std::string:s. Apart from that, as you can see, really easy to create custom carriers. Clean interface.
Thanks for the report.
Per file api/include/opentelemetry/nostd/string_view.h
, the following are missing and probably could help:
nostd::string_view()
constructor that takes a std::string_view
as inputstd::string_view
operator, to convert nostd::string_view
to std::string_view
.The same exist for std::string
.
Now, about removing nostd
classes entirely and using std
instead to avoid making intermediate copies:
The problem with the internal opentelemetry::v1::nostd::string_view only being compatible with std::string and not std::string_view is really obvious when using opentelemetry::context::propagation::TextMapCarrier
By using TextMapCarrier as a base class you need to implement "virtual nostd::string_view Get(nostd::string_view key) const noexcept = 0;" for instance. My experience is that C++ HTTP libraries are commonly based on std::string_view in order to avoid unnecessary copying of strings. Boost Beast for instance uses std::string_view for header parameters.
I know that it is possible to copy data/strings from a std::map<std::string_view, std::string_view> into a std::map<std::string, std::string>. But doing that for every request is a bad design. Am I missing something here? Is there a way of using the internal string_view implementation together with the std version? Why not use std::string_view in opentelemetry?
Maybe the TextMapCarrier base class should be a template instead. But I guess it will affect the TextMapPropagator as well then.
What's your take on this issue? I doubt other third party libraries will implement support for opentelemetry::v1::nostd::string_view.