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

TraceState implementation as per spec #551

Merged
merged 24 commits into from
Feb 4, 2021
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
266 changes: 233 additions & 33 deletions api/include/opentelemetry/trace/trace_state.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@

#include <cstdint>
#include <cstring>
#include <string>

#include <regex>
#if (__GNUC__ == 4 && (__GNUC_MINOR__ == 8))
# define HAVE_WORKING_REGEX 0
#else
# define HAVE_WORKING_REGEX 1
#endif

#include "opentelemetry/nostd/span.h"
#include "opentelemetry/nostd/string_view.h"
Expand All @@ -36,9 +44,11 @@ namespace trace
class TraceState
{
public:
static constexpr int kKeyMaxSize = 256;
static constexpr int kValueMaxSize = 256;
static constexpr int kMaxKeyValuePairs = 32;
static constexpr int kKeyMaxSize = 256;
static constexpr int kValueMaxSize = 256;
static constexpr int kMaxKeyValuePairs = 32;
static constexpr auto kKeyValueSeparator = '=';
static constexpr auto kMembersSeparator = ',';

// Class to store key-value pairs.
class Entry
Expand Down Expand Up @@ -97,37 +107,163 @@ class TraceState
}
};

// An empty TraceState.
TraceState() noexcept : entries_(new Entry[kMaxKeyValuePairs]), num_entries_(0) {}
/**
* Returns a newly created Tracestate parsed from the header provided.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: change spelling of Tracestate in this comment to TraceState?

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. changed it now :)

* @param header Encoding of the tracestate header defined by
* the W3C Trace Context specification https://www.w3.org/TR/trace-context/
* @return Tracestate A new Tracestate instance or DEFAULT
*/
static TraceState FromHeader(const nostd::string_view &header)
lalitb marked this conversation as resolved.
Show resolved Hide resolved
{
TraceState ts;

// Returns false if no such key, otherwise returns true and populates the value parameter with the
// associated value.
bool Get(nostd::string_view key, nostd::string_view &value) const noexcept
std::size_t begin{0};
std::size_t end{0};
bool invalid_header = false;
while (begin < header.size() && ts.num_entries_ < kMaxKeyValuePairs)
{
// find list-member
end = header.find(kMembersSeparator, begin);
if (end == std::string::npos)
{
// last list member. `end` points to end of it.
end = header.size() - 1;
}
else
{
// `end` points to end of current list member
end = end - 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just --end? Also could it get -1 if the first character is ,?

Copy link
Member Author

Choose a reason for hiding this comment

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

Made it --end. If would be fine if first char is ,, as this will make control to go to below condition:

      if (list_member.size() == 0)
      {
        // empty list member, move to next in list
        begin = end + 2;  // begin points to start of next member
        continue;
      }

and process next list member. I have added test case to validate this.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the first char is ,, then end will be -1, so the call to TrimString will trigger access violation when accessing the input string, like header[static_cast<size_t>(-1)] which happens before the empty list_member check?

Copy link
Member Author

@lalitb lalitb Feb 3, 2021

Choose a reason for hiding this comment

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

Yes, you are right and bazel asan fails because of that. I am fixing it now :) Thanks for pointing out the issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, the easy solution was to add special conditional check for , as first char.

}
auto list_member = TrimString(header, begin, end); // OWS handling
if (list_member.size() == 0)
{
// empty list member, move to next in list
begin = end + 2; // begin points to start of next member
continue;
}
auto key_end_pos = list_member.find(kKeyValueSeparator);
if (key_end_pos == std::string::npos)
{
// Error: invalid list member, return empty TraceState
ts.entries_.reset(nullptr);
ts.num_entries_ = 0;
break;
}
auto key = list_member.substr(0, key_end_pos);
auto value = list_member.substr(key_end_pos + 1);
if (!IsValidKey(key) || !IsValidValue(value))
{
// invalid header. return empty TraceState
ts.entries_.reset(nullptr);
ts.num_entries_ = 0;
break;
}
Entry entry(key, value);
(ts.entries_.get())[ts.num_entries_] = entry;
ts.num_entries_++;

begin = end + 2;
}
return ts;
}

/**
* Creates a w3c tracestate header from TraceState object
*/
std::string ToHeader()
{
for (const auto &entry : Entries())
std::string header_s;
size_t count = num_entries_;
while (count)
{
auto entry = (entries_.get())[num_entries_ - count];
auto kv = std::string(entry.GetKey()) + kKeyValueSeparator + std::string(entry.GetValue());
if (--count)
{
// append "," if not last member
kv += ",";
}
header_s = header_s + kv;
}
lalitb marked this conversation as resolved.
Show resolved Hide resolved
return header_s;
}

/**
* Returns `value` associated with `key` passed as argument
* Returns empty string if key is invalid or not found
*/
std::string Get(nostd::string_view key) const noexcept
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should preserve the ability to distinguish between an key not being specified and a key given but having an empty value.

Copy link
Member Author

@lalitb lalitb Feb 2, 2021

Choose a reason for hiding this comment

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

As per w3c specs ( https://www.w3.org/TR/trace-context/#value) the value can't be empty string ( should have at-least one non-blank character ). This is validated before storing from header. But I don't have strong preference on this, we can switch back to earlier prototype if that's more clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

Have made comments consistent with method prototype.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. Your method signature is fine.

{
if (!IsValidKey(key))
{
return std::string();
}
for (size_t i = 0; i < num_entries_; i++)
{
auto entry = (entries_.get())[i];
if (key == entry.GetKey())
{
value = entry.GetValue();
return true;
return std::string(entry.GetValue());
}
}
return false;
return std::string();
}

// Creates an Entry for the key-value pair and adds it to entries. Returns true if pair was added
// successfully, false otherwise. If value is null or entries_ is full, this function is a no-op.
bool Set(nostd::string_view key, nostd::string_view value) noexcept
/**
* Returns `new` TransState object with following mutations applied to the existing instance:
* Update Key value: The updated value must be moved to beginning of List
* Add : The new key-value pair SHOULD be added to beginning of List
*
* If the provided key-value pair is invalid, or results in transtate that violates the
* tracecontext specification, empty tracestate will be returned.
*/
TraceState Set(const nostd::string_view &key, const nostd::string_view &value)
{
if (value.empty() || num_entries_ >= kMaxKeyValuePairs)
TraceState ts;
if ((!IsValidKey(key) || !IsValidValue(value)) || num_entries_ == kMaxKeyValuePairs)
{
return false;
// max size reached or invalid key/value. Returning empty tracestate
return ts; // empty instance
}

Entry entry(key, value);
(entries_.get())[num_entries_] = entry;
num_entries_++;
return true;
// add new key-value pair at beginning
Entry e(key, value);
(ts.entries_.get())[ts.num_entries_++] = e;
for (size_t i = 0; i < num_entries_; i++)
{
auto entry = (entries_.get())[i];
auto key = entry.GetKey();
auto value = entry.GetValue();
Entry e(key, value);
(ts.entries_.get())[ts.num_entries_++] = e;
}
return ts;
}

/**
* Returns `new` TransState object after removing the attribute with given key ( if present )
* @returns empty TransState object if key is invalid
* @returns copy of original TransState object if key is not present (??)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @returns copy of original TransState object if key is not present (??)
* @returns copy of original TraceState object if key is not present (??)

Seems for now an empty instance is returned if the key is not present, should we return original TraceState like mentioned in the comment?

Copy link
Member Author

@lalitb lalitb Feb 3, 2021

Choose a reason for hiding this comment

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

Thanks for suggestion TransState. Have changed it now as part of other changes.
Also,
empty instance is returned if the key is not present,

  • As of now, as per code it return empty instance if key is not valid, and copy of original instance if key is not present.

*/
TraceState Delete(const nostd::string_view &key)
{
TraceState ts;
if (!IsValidKey(key))
{
return ts;
}
for (size_t i = 0; i < num_entries_; i++)
{
auto entry = (entries_.get())[i];
if ((entries_.get())[i].GetKey() != key)
{
auto key = entry.GetKey();
auto value = entry.GetValue();
Entry e(key, value);
(ts.entries_.get())[ts.num_entries_++] = e;
}
}
return ts;
}

// Returns true if there are no keys, false otherwise.
Expand All @@ -139,8 +275,81 @@ class TraceState
return nostd::span<Entry>(entries_.get(), num_entries_);
}

// Returns whether key is a valid key. See https://www.w3.org/TR/trace-context/#key
/** Returns whether key is a valid key. See https://www.w3.org/TR/trace-context/#key
* Identifiers MUST begin with a lowercase letter or a digit, and can only contain
* lowercase letters (a-z), digits (0-9), underscores (_), dashes (-), asterisks (*),
* and forward slashes (/).
* For multi-tenant vendor scenarios, an at sign (@) can be used to prefix the vendor name.
*
*/
static bool IsValidKey(nostd::string_view key)
{
#if HAVE_WORKING_REGEX
return IsValidKeyRegEx(key);
#else
return IsValidKeyNonRegEx(key);
#endif
}

/** Returns whether value is a valid value. See https://www.w3.org/TR/trace-context/#value
* The value is an opaque string containing up to 256 printable ASCII (RFC0020)
* characters ((i.e., the range 0x20 to 0x7E) except comma , and equal =)
*/
static bool IsValidValue(nostd::string_view value)
{
#if HAVE_WORKING_REGEX
return IsValidValueRegEx(value);
#else
return IsValidValueNonRegEx(value);
#endif
}

private:
// Store entries in a C-style array to avoid using std::array or std::vector.
nostd::unique_ptr<Entry[]> entries_;

// Maintain the number of entries in entries_. Must be in the range [0, kMaxKeyValuePairs].
size_t num_entries_;

// An empty TraceState.
TraceState() noexcept : entries_(new Entry[kMaxKeyValuePairs]), num_entries_(0) {}

static nostd::string_view TrimString(nostd::string_view str, size_t left, size_t right)
{
while (str[(std::size_t)right] == ' ' && left < right)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: C++ style static_cast<std::size_t>?

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. fixed it now.

{
right--;
}
while (str[(std::size_t)left] == ' ' && left < right)
{
left++;
}
return str.substr(left, right - left + 1);
}

static bool IsValidKeyRegEx(nostd::string_view key)
{
static std::regex reg_key("^[a-z0-9][a-z0-9*_\\-/]{0,255}$");
static std::regex reg_key_multitenant(
"^[a-z0-9][a-z0-9*_\\-/]{0,240}(@)[a-z0-9][a-z0-9*_\\-/]{0,13}$");
std::string key_s(key);
if (std::regex_match(key_s, reg_key) || std::regex_match(key_s, reg_key_multitenant))
{
return true;
}
return false;
}

static bool IsValidValueRegEx(nostd::string_view value)
{
// Hex 0x20 to 0x2B, 0x2D to 0x3C, 0x3E to 0x7E
std::regex reg_value(
lalitb marked this conversation as resolved.
Show resolved Hide resolved
"^[\\x20-\\x2B\\x2D-\\x3C\\x3E-\\x7E]{0,255}[\\x21-\\x2B\\x2D-\\x3C\\x3E-\\x7E]$");
// Need to benchmark without regex, as a string object is created here.
return std::regex_match(std::string(value), reg_value);
}

static bool IsValidKeyNonRegEx(nostd::string_view key)
{
if (key.empty() || key.size() > kKeyMaxSize || !IsLowerCaseAlphaOrDigit(key[0]))
{
Expand All @@ -163,8 +372,7 @@ class TraceState
return true;
}

// Returns whether value is a valid value. See https://www.w3.org/TR/trace-context/#value
static bool IsValidValue(nostd::string_view value)
static bool IsValidValueNonRegEx(nostd::string_view value)
{
if (value.empty() || value.size() > kValueMaxSize)
{
Expand All @@ -181,15 +389,7 @@ class TraceState
return true;
}

private:
// Store entries in a C-style array to avoid using std::array or std::vector.
nostd::unique_ptr<Entry[]> entries_;

// Maintain the number of entries in entries_. Must be in the range [0, kMaxKeyValuePairs].
int num_entries_;

static bool IsLowerCaseAlphaOrDigit(char c) { return isdigit(c) || islower(c); }
};

} // namespace trace
OPENTELEMETRY_END_NAMESPACE
OPENTELEMETRY_END_NAMESPACE
1 change: 1 addition & 0 deletions api/test/trace/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ foreach(
span_context_test
scope_test
noop_test
trace_state_test
tracer_test)
add_executable(api_${testname} "${testname}.cc")
target_link_libraries(
Expand Down
Loading