From a5fa021979f498a990570722c2ef0b6a03b9b4ea Mon Sep 17 00:00:00 2001 From: ndaliya Date: Fri, 16 Apr 2021 11:35:04 +0530 Subject: [PATCH 1/7] Adding baggage implementation --- api/include/opentelemetry/baggage/baggage.h | 291 ++++++++++++++++++ .../opentelemetry/common/string_util.h | 10 + api/test/CMakeLists.txt | 1 + api/test/baggage/BUILD | 12 + api/test/baggage/CMakeLists.txt | 12 + api/test/baggage/baggage_test.cc | 211 +++++++++++++ api/test/common/string_util_test.cc | 21 +- 7 files changed, 557 insertions(+), 1 deletion(-) create mode 100644 api/include/opentelemetry/baggage/baggage.h create mode 100644 api/test/baggage/BUILD create mode 100644 api/test/baggage/CMakeLists.txt create mode 100644 api/test/baggage/baggage_test.cc diff --git a/api/include/opentelemetry/baggage/baggage.h b/api/include/opentelemetry/baggage/baggage.h new file mode 100644 index 0000000000..9c701f9e2c --- /dev/null +++ b/api/include/opentelemetry/baggage/baggage.h @@ -0,0 +1,291 @@ +// Copyright 2021, OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include "opentelemetry/common/kv_properties.h" +#include "opentelemetry/nostd/shared_ptr.h" +#include "opentelemetry/nostd/string_view.h" +#include "opentelemetry/version.h" + +OPENTELEMETRY_BEGIN_NAMESPACE + +namespace baggage +{ + +class Baggage +{ +public: + static constexpr size_t kMaxKeyValuePairs = 180; + static constexpr size_t kMaxKeyValueSize = 4096; + static constexpr size_t kMaxSize = 8192; + static constexpr char kKeyValueSeparator = '='; + static constexpr char kMembersSeparator = ','; + static constexpr char kMetadataSeparator = ';'; + + Baggage() : kv_properties_(new opentelemetry::common::KeyValueProperties()) {} + Baggage(size_t size) : kv_properties_(new opentelemetry::common::KeyValueProperties(size)){}; + + template + Baggage(const T &keys_and_values) + : kv_properties_(new opentelemetry::common::KeyValueProperties(keys_and_values)) + {} + + static nostd::shared_ptr GetDefault() + { + static nostd::shared_ptr baggage{new Baggage()}; + return baggage; + } + + /* Get value for key in the baggage + @returns true if key is found, false otherwise + */ + bool GetValue(const nostd::string_view &key, std::string &value) const + { + return kv_properties_->GetValue(key, value); + } + + /* Returns shared_ptr of new baggage object which contains new key-value pair. If key or value is + invalid, copy of current baggage is returned + */ + nostd::shared_ptr Set(const nostd::string_view &key, const nostd::string_view &value) + { + + nostd::shared_ptr baggage(new Baggage(kv_properties_->Size() + 1)); + const bool valid_kv = IsValidKey(key) && IsValidValue(value); + + if (valid_kv) + { + baggage->kv_properties_->AddEntry(key, value); + } + + // add rest of the fields. + kv_properties_->GetAllEntries( + [&baggage, &key, &valid_kv](nostd::string_view e_key, nostd::string_view e_value) { + // if key or value was not valid, add all the entries. Add only remaining entries + // otherwise. + if (!valid_kv || key != e_key) + { + baggage->kv_properties_->AddEntry(e_key, e_value); + } + + return true; + }); + + return baggage; + } + + // @return all key-values entries by repeatedly invoking the function reference passed as argument + // for each entry + bool GetAllEntries( + nostd::function_ref callback) const noexcept + { + return kv_properties_->GetAllEntries(callback); + } + + // delete key from the baggage if it exists. Returns shared_ptr of new baggage object. + nostd::shared_ptr Delete(const nostd::string_view &key) + { + // keeping size of baggage same as key might not be found in it + nostd::shared_ptr baggage(new Baggage(kv_properties_->Size())); + kv_properties_->GetAllEntries( + [&baggage, &key](nostd::string_view e_key, nostd::string_view e_value) { + if (key != e_key) + baggage->kv_properties_->AddEntry(e_key, e_value); + return true; + }); + return baggage; + } + + // Returns shared_ptr of baggage after extracting key-value pairs from header + static nostd::shared_ptr FromHeader(nostd::string_view header) + { + if (header.size() > kMaxSize) + { + // header size exceeds maximum threshold, return empty baggage + return GetDefault(); + } + + common::KeyValueStringTokenizer kv_str_tokenizer(header); + size_t cnt = kv_str_tokenizer.NumTokens(); // upper bound on number of kv pairs + if (cnt > kMaxKeyValuePairs) + { + cnt = kMaxKeyValuePairs; + } + + nostd::shared_ptr baggage(new Baggage(cnt)); + bool kv_valid; + nostd::string_view key, value; + + while (kv_str_tokenizer.next(kv_valid, key, value) && baggage->kv_properties_->Size() < cnt) + { + if (!kv_valid || (key.size() + value.size() > kMaxKeyValueSize)) + { + // if kv pair is not valid, skip it + continue; + } + + // NOTE : metadata is kept as part of value only as it does not have any semantic meaning. + // but, we need to extract it (else Decode on value will return error) + nostd::string_view metadata; + auto metadata_separator = value.find(kMetadataSeparator); + if (metadata_separator != std::string::npos) + { + metadata = value.substr(metadata_separator); + value = value.substr(0, metadata_separator); + } + + int err = 0; + key = Decode(common::StringUtil::Trim(key), err); + value = Decode(common::StringUtil::Trim(value), err); + + if (err == 0 && IsValidKey(key) && IsValidValue(value)) + { + if (!metadata.empty()) + { + std::string value_with_metadata(value.data(), value.size()); + value_with_metadata.append(metadata.data(), metadata.size()); + value = value_with_metadata; + } + baggage->kv_properties_->AddEntry(key, value); + } + } + + return baggage; + } + + // Creates string from baggage object. + std::string ToHeader() const + { + std::string header_s; + bool first = true; + kv_properties_->GetAllEntries([&](nostd::string_view key, nostd::string_view value) { + if (!first) + { + header_s.push_back(kMembersSeparator); + } + else + { + first = false; + } + header_s.append(Encode(key)); + header_s.push_back(kKeyValueSeparator); + header_s.append(Encode(value)); + return true; + }); + return header_s; + } + +private: + static bool IsPrintableString(nostd::string_view str) + { + for (const auto &ch : str) + { + if (ch < ' ' || ch > '~') + { + return false; + } + } + + return true; + } + + static bool IsValidKey(nostd::string_view key) { return key.size() && IsPrintableString(key); } + + static bool IsValidValue(nostd::string_view value) { return IsPrintableString(value); } + + // Uri encode key value pairs before injecting into header + // Implementation inspired from : https://golang.org/src/net/url/url.go?s=7851:7884#L264 + static std::string Encode(nostd::string_view str) + { + auto to_hex = [](char c) -> char { + static const char *hex = "0123456789ABCDEF"; + return hex[c & 15]; + }; + + std::string ret; + + for (auto c : str) + { + if (std::isalnum(c) || c == '-' || c == '_' || c == '.' || c == '~') + { + ret.push_back(c); + } + else if (c == ' ') + { + ret.push_back('+'); + } + else + { + ret.push_back('%'); + ret.push_back(to_hex(c >> 4)); + ret.push_back(to_hex(c & 15)); + } + } + + return ret; + } + + // Uri decode key value pairs after extracting from header + static std::string Decode(nostd::string_view str, int &err) + { + auto IsHex = [](char c) { + return std::isdigit(c) || (c >= 'A' && c <= 'F') || (c >= 'a' && c <= 'f'); + }; + + auto from_hex = [](char c) -> char { + return std::isdigit(c) ? c - '0' : std::toupper(c) - 'A' + 10; + }; + + std::string ret; + + for (int i = 0; i < str.size(); i++) + { + if (str[i] == '%') + { + if (i + 2 >= str.size() || !IsHex(str[i + 1]) || !IsHex(str[i + 2])) + { + err = 1; + return ""; + } + ret.push_back(from_hex(str[i + 1]) << 4 | from_hex(str[i + 2])); + i += 2; + } + else if (str[i] == '+') + { + ret.push_back(' '); + } + else if (std::isalnum(str[i]) || str[i] == '-' || str[i] == '_' || str[i] == '.' || + str[i] == '~') + { + ret.push_back(str[i]); + } + else + { + err = 1; + return ""; + } + } + + return ret; + } + +private: + // Store entries in a C-style array to avoid using std::array or std::vector. + nostd::unique_ptr kv_properties_; +}; + +} // namespace baggage + +OPENTELEMETRY_END_NAMESPACE diff --git a/api/include/opentelemetry/common/string_util.h b/api/include/opentelemetry/common/string_util.h index 353a76fbc9..9b7740992b 100644 --- a/api/include/opentelemetry/common/string_util.h +++ b/api/include/opentelemetry/common/string_util.h @@ -35,6 +35,16 @@ class StringUtil } return str.substr(left, 1 + right - left); } + + static nostd::string_view Trim(nostd::string_view str) + { + if (str.empty()) + { + return str; + } + + return Trim(str, 0, str.size() - 1); + } }; } // namespace common diff --git a/api/test/CMakeLists.txt b/api/test/CMakeLists.txt index 5dfac29e4c..eaf09e44e0 100644 --- a/api/test/CMakeLists.txt +++ b/api/test/CMakeLists.txt @@ -6,3 +6,4 @@ add_subdirectory(trace) add_subdirectory(metrics) add_subdirectory(logs) add_subdirectory(common) +add_subdirectory(baggage) diff --git a/api/test/baggage/BUILD b/api/test/baggage/BUILD new file mode 100644 index 0000000000..205bed68fe --- /dev/null +++ b/api/test/baggage/BUILD @@ -0,0 +1,12 @@ +load("//bazel:otel_cc_benchmark.bzl", "otel_cc_benchmark") + +cc_test( + name = "baggage_test", + srcs = [ + "baggage_test.cc", + ], + deps = [ + "//api", + "@com_google_googletest//:gtest_main", + ], +) diff --git a/api/test/baggage/CMakeLists.txt b/api/test/baggage/CMakeLists.txt new file mode 100644 index 0000000000..3650e4afa8 --- /dev/null +++ b/api/test/baggage/CMakeLists.txt @@ -0,0 +1,12 @@ +include(GoogleTest) + +foreach(testname baggage_test) + add_executable(${testname} "${testname}.cc") + target_link_libraries( + ${testname} ${GTEST_BOTH_LIBRARIES} ${CORE_RUNTIME_LIBS} + ${CMAKE_THREAD_LIBS_INIT} opentelemetry_api) + gtest_add_tests( + TARGET ${testname} + TEST_PREFIX baggage. + TEST_LIST ${testname}) +endforeach() diff --git a/api/test/baggage/baggage_test.cc b/api/test/baggage/baggage_test.cc new file mode 100644 index 0000000000..cc8b00a7b0 --- /dev/null +++ b/api/test/baggage/baggage_test.cc @@ -0,0 +1,211 @@ +#include "opentelemetry/nostd/string_view.h" + +#include +#include +#include + +#include "opentelemetry/baggage/baggage.h" + +using namespace opentelemetry; +using namespace opentelemetry::baggage; + +std::string header_with_custom_entries(size_t num_entries) +{ + std::string header; + for (int i = 0; i < num_entries; i++) + { + std::string key = "key" + std::to_string(i); + std::string value = "value" + std::to_string(i); + header += key + "=" + value; + if (i != num_entries - 1) + { + header += ","; + } + } + return header; +} + +std::string header_with_custom_size(size_t key_value_size, size_t num_entries) +{ + std::string header = ""; + for (int i = 0; i < num_entries; i++) + { + std::string str = std::to_string(i + 1); + str += "="; + assert(key_value_size > str.size()); + for (int j = str.size(); j < key_value_size; j++) + { + str += "a"; + } + + header += str + ','; + } + + header.pop_back(); + return header; +} + +TEST(BaggageTest, ValidateExtractHeader) +{ + auto invalid_key_value_size_header = header_with_custom_size(Baggage::kMaxKeyValueSize + 5, 1); + + struct + { + const char *input; + std::vector keys; + std::vector values; + } testcases[] = { + {"k1=v1", {"k1"}, {"v1"}}, + {"k1=V1,K2=v2;metadata,k3=v3", + {"k1", "K2", "k3"}, + {"V1", "v2;metadata", "v3"}}, // metadata is part of value + {",k1 =v1,k2=v2 ; metadata,", + {"k1", "k2"}, + {"v1", "v2; metadata"}}, // key andd value are trimmed + {"1a-2f%40foo=bar%251,a%2A%2Ffoo-_%2Fbar=bar+4", + {"1a-2f@foo", "a*/foo-_/bar"}, + {"bar%1", "bar 4"}}, // decoding is done properly + {"k1=v1,invalidmember,k2=v2", {"k1", "k2"}, {"v1", "v2"}}, // invalid member is skipped + {",", {}, {}}, + {",=,", {}, {}}, + {"", {}, {}}, + {"k1=%5zv", {}, {}}, // invalid hex : invalid second digit + {"k1=%5", {}, {}}, // invalid hex : missing two digits + {"k%z2=v1", {}, {}}, // invalid hex : invalid first digit + {"k%00=v1", {}, {}}, // key not valid + {"k=v%7f", {}, {}}, // value not valid + {invalid_key_value_size_header.data(), {}, {}}}; + for (auto &testcase : testcases) + { + auto baggage = Baggage::FromHeader(testcase.input); + size_t index = 0; + baggage->GetAllEntries([&testcase, &index](nostd::string_view key, nostd::string_view value) { + EXPECT_EQ(key, testcase.keys[index]); + EXPECT_EQ(value, testcase.values[index]); + index++; + return true; + }); + } + + // For header with maximum threshold pairs, no pair is dropped + auto max_pairs_header = header_with_custom_entries(Baggage::kMaxKeyValuePairs); + EXPECT_EQ(Baggage::FromHeader(max_pairs_header.data())->ToHeader(), max_pairs_header.data()); + + // Entries beyond threshold are dropped + auto baggage = Baggage::FromHeader(header_with_custom_entries(Baggage::kMaxKeyValuePairs + 1)); + auto header = baggage->ToHeader(); + common::KeyValueStringTokenizer kv_str_tokenizer(header); + int expected_tokens = Baggage::kMaxKeyValuePairs; + EXPECT_EQ(kv_str_tokenizer.NumTokens(), expected_tokens); + + // For header with total size more than threshold, baggage is empty + int num_pairs_with_max_size = Baggage::kMaxSize / Baggage::kMaxKeyValueSize; + auto invalid_total_size_header = + header_with_custom_size(Baggage::kMaxKeyValueSize, num_pairs_with_max_size + 1); + EXPECT_EQ(Baggage::FromHeader(invalid_total_size_header.data())->ToHeader(), ""); +} + +TEST(BaggageTest, ValidateInjectHeader) +{ + struct + { + std::vector keys; + std::vector values; + const char *header; + } testcases[] = {{{"k1"}, {"v1"}, "k1=v1"}, + {{"k3", "k2", "k1"}, {"", "v2", "v1"}, "k1=v1,k2=v2,k3="}, // empty value + {{"1a-2f@foo", "a*/foo-_/bar"}, + {"bar%1", "bar 4"}, + "a%2A%2Ffoo-_%2Fbar=bar+4,1a-2f%40foo=bar%251"}}; // encoding is done properly + for (auto &testcase : testcases) + { + nostd::shared_ptr baggage(new Baggage{}); + for (int i = 0; i < testcase.keys.size(); i++) + { + baggage = baggage->Set(testcase.keys[i], testcase.values[i]); + } + EXPECT_EQ(baggage->ToHeader(), testcase.header); + } +} + +TEST(BaggageTest, BaggageGet) +{ + auto header = header_with_custom_entries(Baggage::kMaxKeyValuePairs); + auto baggage = Baggage::FromHeader(header); + + std::string value; + EXPECT_TRUE(baggage->GetValue("key0", value)); + EXPECT_EQ(value, "value0"); + EXPECT_TRUE(baggage->GetValue("key16", value)); + EXPECT_EQ(value, "value16"); + + EXPECT_TRUE(baggage->GetValue("key31", value)); + EXPECT_EQ(value, "value31"); + + EXPECT_FALSE(baggage->GetValue("key181", value)); +} + +TEST(BaggageTest, BaggageSet) +{ + std::string header = "k1=v1,k2=v2"; + auto baggage = Baggage::FromHeader(header); + + std::string value; + baggage = baggage->Set("k3", "v3"); + EXPECT_TRUE(baggage->GetValue("k3", value)); + EXPECT_EQ(value, "v3"); + + baggage = baggage->Set("k3", "v3_1"); // key should be updated with the latest value + EXPECT_TRUE(baggage->GetValue("k3", value)); + EXPECT_EQ(value, "v3_1"); + + header = header_with_custom_entries(Baggage::kMaxKeyValuePairs); + baggage = Baggage::FromHeader(header); + baggage = baggage->Set("key0", "0"); // updating on max list should work + EXPECT_TRUE(baggage->GetValue("key0", value)); + EXPECT_EQ(value, "0"); + + header = "k1=v1,k2=v2"; + baggage = Baggage::FromHeader(header); + baggage = baggage->Set("", "n_v1"); // adding invalid key, should return copy of same baggage + EXPECT_EQ(baggage->ToHeader(), header); + + header = "k1=v1,k2=v2"; + baggage = Baggage::FromHeader(header); + baggage = baggage->Set("k1", "\x1A"); // adding invalid value, should return copy of same baggage + EXPECT_EQ(baggage->ToHeader(), header); +} + +TEST(BaggageTest, BaggageRemove) +{ + auto header = header_with_custom_entries(Baggage::kMaxKeyValuePairs); + auto baggage = Baggage::FromHeader(header); + std::string value; + + // existing key is removed + EXPECT_TRUE(baggage->GetValue("key0", value)); + auto new_baggage = baggage->Delete("key0"); + EXPECT_FALSE(new_baggage->GetValue("key0", value)); + + // trying Delete on non existant key + EXPECT_FALSE(baggage->GetValue("key181", value)); + auto new_baggage_2 = baggage->Delete("key181"); + EXPECT_FALSE(new_baggage_2->GetValue("key181", value)); +} + +TEST(BaggageTest, BaggageGetAll) +{ + std::string baggage_header = "k1=v1,k2=v2,k3=v3"; + auto baggage = Baggage::FromHeader(baggage_header); + const int kNumPairs = 3; + nostd::string_view keys[kNumPairs] = {"k1", "k2", "k3"}; + nostd::string_view values[kNumPairs] = {"v1", "v2", "v3"}; + size_t index = 0; + baggage->GetAllEntries( + [&keys, &values, &index](nostd::string_view key, nostd::string_view value) { + EXPECT_EQ(key, keys[index]); + EXPECT_EQ(value, values[index]); + index++; + return true; + }); +} \ No newline at end of file diff --git a/api/test/common/string_util_test.cc b/api/test/common/string_util_test.cc index 9a43b69ef9..55d1310975 100644 --- a/api/test/common/string_util_test.cc +++ b/api/test/common/string_util_test.cc @@ -23,7 +23,7 @@ using opentelemetry::common::StringUtil; -TEST(StringUtilTest, TrimString) +TEST(StringUtilTest, TrimStringWithIndex) { struct { @@ -37,3 +37,22 @@ TEST(StringUtilTest, TrimString) EXPECT_EQ(StringUtil::Trim(testcase.input, 0, strlen(testcase.input) - 1), testcase.expected); } } + +TEST(StringUtilTest, TrimString) +{ + struct + { + const char *input; + const char *expected; + } testcases[] = {{"k1=v1", "k1=v1"}, + {"k1=v1,k2=v2, k3=v3", "k1=v1,k2=v2, k3=v3"}, + {" k1=v1", "k1=v1"}, + {"k1=v1 ", "k1=v1"}, + {" k1=v1 ", "k1=v1"}, + {" ", ""}, + {"", ""}}; + for (auto &testcase : testcases) + { + EXPECT_EQ(StringUtil::Trim(testcase.input), testcase.expected); + } +} From 4fdf1610fcf30a49332cc66ae27ef5e505bb59d9 Mon Sep 17 00:00:00 2001 From: ndaliya Date: Fri, 16 Apr 2021 11:39:07 +0530 Subject: [PATCH 2/7] Added newlline at the eof --- api/test/baggage/baggage_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/test/baggage/baggage_test.cc b/api/test/baggage/baggage_test.cc index cc8b00a7b0..2c57c5fcbe 100644 --- a/api/test/baggage/baggage_test.cc +++ b/api/test/baggage/baggage_test.cc @@ -208,4 +208,4 @@ TEST(BaggageTest, BaggageGetAll) index++; return true; }); -} \ No newline at end of file +} From 26394fd11b88ea9c16a9fd647eb133e446e3ddc2 Mon Sep 17 00:00:00 2001 From: ndaliya Date: Mon, 19 Apr 2021 13:12:00 +0530 Subject: [PATCH 3/7] Fixing string_view errors --- api/include/opentelemetry/baggage/baggage.h | 14 ++++++-------- api/test/baggage/baggage_test.cc | 4 ++-- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/api/include/opentelemetry/baggage/baggage.h b/api/include/opentelemetry/baggage/baggage.h index 9c701f9e2c..54cc6a5207 100644 --- a/api/include/opentelemetry/baggage/baggage.h +++ b/api/include/opentelemetry/baggage/baggage.h @@ -146,19 +146,17 @@ class Baggage value = value.substr(0, metadata_separator); } - int err = 0; - key = Decode(common::StringUtil::Trim(key), err); - value = Decode(common::StringUtil::Trim(value), err); + int err = 0; + auto key_str = Decode(common::StringUtil::Trim(key), err); + auto value_str = Decode(common::StringUtil::Trim(value), err); - if (err == 0 && IsValidKey(key) && IsValidValue(value)) + if (err == 0 && IsValidKey(key_str) && IsValidValue(value_str)) { if (!metadata.empty()) { - std::string value_with_metadata(value.data(), value.size()); - value_with_metadata.append(metadata.data(), metadata.size()); - value = value_with_metadata; + value_str.append(metadata.data(), metadata.size()); } - baggage->kv_properties_->AddEntry(key, value); + baggage->kv_properties_->AddEntry(key_str, value_str); } } diff --git a/api/test/baggage/baggage_test.cc b/api/test/baggage/baggage_test.cc index 2c57c5fcbe..dfe102f966 100644 --- a/api/test/baggage/baggage_test.cc +++ b/api/test/baggage/baggage_test.cc @@ -61,7 +61,7 @@ TEST(BaggageTest, ValidateExtractHeader) {"V1", "v2;metadata", "v3"}}, // metadata is part of value {",k1 =v1,k2=v2 ; metadata,", {"k1", "k2"}, - {"v1", "v2; metadata"}}, // key andd value are trimmed + {"v1", "v2; metadata"}}, // key and value are trimmed {"1a-2f%40foo=bar%251,a%2A%2Ffoo-_%2Fbar=bar+4", {"1a-2f@foo", "a*/foo-_/bar"}, {"bar%1", "bar 4"}}, // decoding is done properly @@ -187,7 +187,7 @@ TEST(BaggageTest, BaggageRemove) auto new_baggage = baggage->Delete("key0"); EXPECT_FALSE(new_baggage->GetValue("key0", value)); - // trying Delete on non existant key + // trying Delete on non existent key EXPECT_FALSE(baggage->GetValue("key181", value)); auto new_baggage_2 = baggage->Delete("key181"); EXPECT_FALSE(new_baggage_2->GetValue("key181", value)); From c9cbb1a1139684c5a961b0713caa2d6c4e5cf365 Mon Sep 17 00:00:00 2001 From: ndaliya Date: Mon, 19 Apr 2021 14:54:32 +0530 Subject: [PATCH 4/7] Fixing buffer overflow issue --- .../opentelemetry/common/kv_properties.h | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/api/include/opentelemetry/common/kv_properties.h b/api/include/opentelemetry/common/kv_properties.h index 18c517f480..9c01ff9eae 100644 --- a/api/include/opentelemetry/common/kv_properties.h +++ b/api/include/opentelemetry/common/kv_properties.h @@ -47,6 +47,12 @@ class KeyValueStringTokenizer : str_(str), opts_(opts), index_(0) {} + static nostd::string_view GetDefaultKeyOrValue() + { + static std::string default_str = ""; + return default_str; + } + // Returns next key value in the string header // @param valid_kv : if the found kv pair is valid or not // @param key : key in kv pair @@ -57,29 +63,34 @@ class KeyValueStringTokenizer valid_kv = true; while (index_ < str_.size()) { - size_t end = str_.find(opts_.member_separator, index_); + bool is_empty_pair = false; + size_t end = str_.find(opts_.member_separator, index_); if (end == std::string::npos) { end = str_.size() - 1; } + else if (end == index_) // empty pair. do not update end + { + is_empty_pair = true; + } else { end--; } auto list_member = StringUtil::Trim(str_, index_, end); - if (list_member.size() == 0) + if (list_member.size() == 0 || is_empty_pair) { // empty list member - index_ = end + 2; + index_ = end + 2 - is_empty_pair; if (opts_.ignore_empty_members) { continue; } valid_kv = true; - key = ""; - value = ""; + key = GetDefaultKeyOrValue(); + value = GetDefaultKeyOrValue(); return true; } From 086f2cab9776d8edcf9c0d15408b9d4e9ebbb1a1 Mon Sep 17 00:00:00 2001 From: ndaliya Date: Tue, 20 Apr 2021 22:23:29 +0530 Subject: [PATCH 5/7] Addressing review comments --- api/include/opentelemetry/baggage/baggage.h | 33 +++++++++++++++------ api/test/baggage/baggage_test.cc | 14 +++++---- 2 files changed, 33 insertions(+), 14 deletions(-) diff --git a/api/include/opentelemetry/baggage/baggage.h b/api/include/opentelemetry/baggage/baggage.h index 54cc6a5207..97216d61cd 100644 --- a/api/include/opentelemetry/baggage/baggage.h +++ b/api/include/opentelemetry/baggage/baggage.h @@ -95,6 +95,9 @@ class Baggage } // delete key from the baggage if it exists. Returns shared_ptr of new baggage object. + // if key does not exist, copy of current baggage is returned. + // Validity of key is not checked as invalid keys should never be populated in baggage in the + // first place. nostd::shared_ptr Delete(const nostd::string_view &key) { // keeping size of baggage same as key might not be found in it @@ -146,11 +149,11 @@ class Baggage value = value.substr(0, metadata_separator); } - int err = 0; - auto key_str = Decode(common::StringUtil::Trim(key), err); - auto value_str = Decode(common::StringUtil::Trim(value), err); + bool err = 0; + auto key_str = UrlDecode(common::StringUtil::Trim(key), err); + auto value_str = UrlDecode(common::StringUtil::Trim(value), err); - if (err == 0 && IsValidKey(key_str) && IsValidValue(value_str)) + if (err == false && IsValidKey(key_str) && IsValidValue(value_str)) { if (!metadata.empty()) { @@ -177,9 +180,21 @@ class Baggage { first = false; } - header_s.append(Encode(key)); + header_s.append(UrlEncode(key)); header_s.push_back(kKeyValueSeparator); - header_s.append(Encode(value)); + + // extracting metadata from value. We do not encode metadata + auto metadata_separator = value.find(kMetadataSeparator); + if (metadata_separator != std::string::npos) + { + header_s.append(UrlEncode(value.substr(0, metadata_separator))); + auto metadata = value.substr(metadata_separator); + header_s.append(std::string(metadata.data(), metadata.size())); + } + else + { + header_s.append(UrlEncode(value)); + } return true; }); return header_s; @@ -205,7 +220,7 @@ class Baggage // Uri encode key value pairs before injecting into header // Implementation inspired from : https://golang.org/src/net/url/url.go?s=7851:7884#L264 - static std::string Encode(nostd::string_view str) + static std::string UrlEncode(nostd::string_view str) { auto to_hex = [](char c) -> char { static const char *hex = "0123456789ABCDEF"; @@ -236,7 +251,7 @@ class Baggage } // Uri decode key value pairs after extracting from header - static std::string Decode(nostd::string_view str, int &err) + static std::string UrlDecode(nostd::string_view str, bool &err) { auto IsHex = [](char c) { return std::isdigit(c) || (c >= 'A' && c <= 'F') || (c >= 'a' && c <= 'f'); @@ -248,7 +263,7 @@ class Baggage std::string ret; - for (int i = 0; i < str.size(); i++) + for (size_t i = 0; i < str.size(); i++) { if (str[i] == '%') { diff --git a/api/test/baggage/baggage_test.cc b/api/test/baggage/baggage_test.cc index dfe102f966..4dc741cf8f 100644 --- a/api/test/baggage/baggage_test.cc +++ b/api/test/baggage/baggage_test.cc @@ -12,7 +12,7 @@ using namespace opentelemetry::baggage; std::string header_with_custom_entries(size_t num_entries) { std::string header; - for (int i = 0; i < num_entries; i++) + for (size_t i = 0; i < num_entries; i++) { std::string key = "key" + std::to_string(i); std::string value = "value" + std::to_string(i); @@ -28,12 +28,12 @@ std::string header_with_custom_entries(size_t num_entries) std::string header_with_custom_size(size_t key_value_size, size_t num_entries) { std::string header = ""; - for (int i = 0; i < num_entries; i++) + for (size_t i = 0; i < num_entries; i++) { std::string str = std::to_string(i + 1); str += "="; assert(key_value_size > str.size()); - for (int j = str.size(); j < key_value_size; j++) + for (size_t j = str.size(); j < key_value_size; j++) { str += "a"; } @@ -116,11 +116,15 @@ TEST(BaggageTest, ValidateInjectHeader) {{"k3", "k2", "k1"}, {"", "v2", "v1"}, "k1=v1,k2=v2,k3="}, // empty value {{"1a-2f@foo", "a*/foo-_/bar"}, {"bar%1", "bar 4"}, - "a%2A%2Ffoo-_%2Fbar=bar+4,1a-2f%40foo=bar%251"}}; // encoding is done properly + "a%2A%2Ffoo-_%2Fbar=bar+4,1a-2f%40foo=bar%251"}, // encoding is done properly + {{"foo 1"}, + {"bar 1; metadata ; ;;"}, + "foo+1=bar+1; metadata ; ;;"}}; // metadata is added without encoding + for (auto &testcase : testcases) { nostd::shared_ptr baggage(new Baggage{}); - for (int i = 0; i < testcase.keys.size(); i++) + for (size_t i = 0; i < testcase.keys.size(); i++) { baggage = baggage->Set(testcase.keys[i], testcase.values[i]); } From 2d84c69f0ed5b133d21443712c679c662ad052fa Mon Sep 17 00:00:00 2001 From: ndaliya Date: Wed, 21 Apr 2021 00:20:13 +0530 Subject: [PATCH 6/7] Removing const ref from string_view --- api/include/opentelemetry/baggage/baggage.h | 4 ++-- api/include/opentelemetry/common/kv_properties.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/include/opentelemetry/baggage/baggage.h b/api/include/opentelemetry/baggage/baggage.h index 97216d61cd..afdd998bbd 100644 --- a/api/include/opentelemetry/baggage/baggage.h +++ b/api/include/opentelemetry/baggage/baggage.h @@ -51,7 +51,7 @@ class Baggage /* Get value for key in the baggage @returns true if key is found, false otherwise */ - bool GetValue(const nostd::string_view &key, std::string &value) const + bool GetValue(nostd::string_view key, std::string &value) const { return kv_properties_->GetValue(key, value); } @@ -98,7 +98,7 @@ class Baggage // if key does not exist, copy of current baggage is returned. // Validity of key is not checked as invalid keys should never be populated in baggage in the // first place. - nostd::shared_ptr Delete(const nostd::string_view &key) + nostd::shared_ptr Delete(nostd::string_view key) { // keeping size of baggage same as key might not be found in it nostd::shared_ptr baggage(new Baggage(kv_properties_->Size())); diff --git a/api/include/opentelemetry/common/kv_properties.h b/api/include/opentelemetry/common/kv_properties.h index 9c01ff9eae..8864776c96 100644 --- a/api/include/opentelemetry/common/kv_properties.h +++ b/api/include/opentelemetry/common/kv_properties.h @@ -237,7 +237,7 @@ class KeyValueProperties } // Adds new kv pair into kv properties - void AddEntry(const nostd::string_view &key, const nostd::string_view &value) + void AddEntry(nostd::string_view key, nostd::string_view value) { if (num_entries_ < max_num_entries_) { @@ -262,7 +262,7 @@ class KeyValueProperties } // Return value for key if exists, return false otherwise - bool GetValue(const nostd::string_view key, std::string &value) const + bool GetValue(nostd::string_view key, std::string &value) const { for (size_t i = 0; i < num_entries_; i++) { From 6100c6be612ff66b5faf113ef2606d4bcfc809d6 Mon Sep 17 00:00:00 2001 From: ndaliya Date: Wed, 21 Apr 2021 01:08:35 +0530 Subject: [PATCH 7/7] Removing reference for char in loop --- api/include/opentelemetry/baggage/baggage.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/include/opentelemetry/baggage/baggage.h b/api/include/opentelemetry/baggage/baggage.h index afdd998bbd..73f83a3dba 100644 --- a/api/include/opentelemetry/baggage/baggage.h +++ b/api/include/opentelemetry/baggage/baggage.h @@ -203,7 +203,7 @@ class Baggage private: static bool IsPrintableString(nostd::string_view str) { - for (const auto &ch : str) + for (const auto ch : str) { if (ch < ' ' || ch > '~') {