From 111f2346cad9bede2e0ef2d5fc84e31a062deb6e Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Mon, 28 Oct 2024 22:34:37 +0000 Subject: [PATCH 01/32] fix test_traceparent_header_name_valid_casing --- ext/test/w3c_tracecontext_test/main.cc | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/ext/test/w3c_tracecontext_test/main.cc b/ext/test/w3c_tracecontext_test/main.cc index fed07a2398..6af7f20f37 100644 --- a/ext/test/w3c_tracecontext_test/main.cc +++ b/ext/test/w3c_tracecontext_test/main.cc @@ -51,10 +51,12 @@ class TextMapCarrierTest : public context::propagation::TextMapCarrier TextMapCarrierTest(std::map &headers) : headers_(headers) {} nostd::string_view Get(nostd::string_view key) const noexcept override { - auto it = headers_.find(std::string(key)); - if (it != headers_.end()) + for(const auto& elem : headers_) { - return nostd::string_view(it->second); + if (equalsIgnoreCase(elem.first, std::string(key))) + { + return nostd::string_view(elem.second); + } } return ""; } @@ -63,6 +65,22 @@ class TextMapCarrierTest : public context::propagation::TextMapCarrier headers_[std::string(key)] = std::string(value); } + static bool equalsIgnoreCase(const std::string &str1, const std::string &str2) + { + if (str1.length() != str2.length()) + { + return false; + } + for (int i = 0; i < str1.length(); i++) + { + if (tolower(str1[i]) != tolower(str2[i])) + { + return false; + } + } + return true; + } + std::map &headers_; }; From e73c58b657318014ff230ba2ebb61a27379fdb2f Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Mon, 28 Oct 2024 23:27:20 +0000 Subject: [PATCH 02/32] Fix test_tracestate_empty_header --- ext/include/opentelemetry/ext/http/server/http_server.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/ext/include/opentelemetry/ext/http/server/http_server.h b/ext/include/opentelemetry/ext/http/server/http_server.h index 5ea7debc43..e632051017 100644 --- a/ext/include/opentelemetry/ext/http/server/http_server.h +++ b/ext/include/opentelemetry/ext/http/server/http_server.h @@ -648,7 +648,13 @@ class HttpServer : private SocketTools::Reactor::SocketCallback { ptr++; } - conn.request.headers[name] = std::string(begin, ptr); + if (!conn.request.headers[name].empty() && equalsLowercased(name, "tracestate")) + { + conn.request.headers[name] = conn.request.headers[name].append(",").append(std::string(begin, ptr)); + } else + { + conn.request.headers[name] = std::string(begin, ptr); + } if (*ptr == '\r') { ptr++; From c4816dea0913406bc791274dcc688466189a754b Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Tue, 29 Oct 2024 14:31:51 +0000 Subject: [PATCH 03/32] Fix test_traceparent_duplicated --- ext/include/opentelemetry/ext/http/server/http_server.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/include/opentelemetry/ext/http/server/http_server.h b/ext/include/opentelemetry/ext/http/server/http_server.h index e632051017..67b123358a 100644 --- a/ext/include/opentelemetry/ext/http/server/http_server.h +++ b/ext/include/opentelemetry/ext/http/server/http_server.h @@ -648,7 +648,7 @@ class HttpServer : private SocketTools::Reactor::SocketCallback { ptr++; } - if (!conn.request.headers[name].empty() && equalsLowercased(name, "tracestate")) + if (!conn.request.headers[name].empty()) { conn.request.headers[name] = conn.request.headers[name].append(",").append(std::string(begin, ptr)); } else From 3696baf8cef49f28903a503a063f8611fb2374be Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Tue, 29 Oct 2024 16:14:36 +0000 Subject: [PATCH 04/32] Fix test_tracestate_member_count_limit --- api/include/opentelemetry/trace/trace_state.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api/include/opentelemetry/trace/trace_state.h b/api/include/opentelemetry/trace/trace_state.h index 753f48c99e..df7a0e0fe9 100644 --- a/api/include/opentelemetry/trace/trace_state.h +++ b/api/include/opentelemetry/trace/trace_state.h @@ -59,7 +59,8 @@ class OPENTELEMETRY_EXPORT TraceState size_t cnt = kv_str_tokenizer.NumTokens(); // upper bound on number of kv pairs if (cnt > kMaxKeyValuePairs) { - cnt = kMaxKeyValuePairs; + // trace state should be discarded if count exceeds + return GetDefault(); } nostd::shared_ptr ts(new TraceState(cnt)); From b1d0623f5849c4c42891a1f7bbb2e3859e8802ac Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Wed, 30 Oct 2024 18:56:38 +0000 Subject: [PATCH 05/32] Fix TraceContextTest.test_traceparent_version_* tests --- .../trace/propagation/http_trace_context.h | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/api/include/opentelemetry/trace/propagation/http_trace_context.h b/api/include/opentelemetry/trace/propagation/http_trace_context.h index a6b7e3b219..2f61b23689 100644 --- a/api/include/opentelemetry/trace/propagation/http_trace_context.h +++ b/api/include/opentelemetry/trace/propagation/http_trace_context.h @@ -87,6 +87,7 @@ class HttpTraceContext : public context::propagation::TextMapPropagator private: static constexpr uint8_t kInvalidVersion = 0xFF; + static constexpr uint8_t kDefaultAssumedVersion = 0x00; static bool IsValidVersion(nostd::string_view version_hex) { @@ -95,6 +96,13 @@ class HttpTraceContext : public context::propagation::TextMapPropagator return version != kInvalidVersion; } + static bool IsHigherVersion(nostd::string_view version_hex) + { + uint8_t version; + detail::HexToBinary(version_hex, &version, sizeof(version)); + return version > kDefaultAssumedVersion; + } + static void InjectImpl(context::propagation::TextMapCarrier &carrier, const SpanContext &span_context) { @@ -122,11 +130,6 @@ class HttpTraceContext : public context::propagation::TextMapPropagator static SpanContext ExtractContextFromTraceHeaders(nostd::string_view trace_parent, nostd::string_view trace_state) { - if (trace_parent.size() != kTraceParentSize) - { - return SpanContext::GetInvalid(); - } - std::array fields{}; if (detail::SplitString(trace_parent, '-', fields.data(), 4) != 4) { @@ -155,6 +158,16 @@ class HttpTraceContext : public context::propagation::TextMapPropagator return SpanContext::GetInvalid(); } + if (IsHigherVersion(version_hex) && trace_parent.size() < kTraceParentSize) + { + return SpanContext::GetInvalid(); + } + + if (!IsHigherVersion(version_hex) && trace_parent.size() != kTraceParentSize) + { + return SpanContext::GetInvalid(); + } + TraceId trace_id = TraceIdFromHex(trace_id_hex); SpanId span_id = SpanIdFromHex(span_id_hex); From b0732da5dcdc0d972115249d6a15917a3f6cd8c2 Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Wed, 30 Oct 2024 21:39:58 +0000 Subject: [PATCH 06/32] Add TrimString function --- .../trace/propagation/detail/string.h | 24 +++++++++++++++++++ api/test/trace/propagation/detail/BUILD | 16 +++++++++++++ .../trace/propagation/detail/CMakeLists.txt | 2 +- .../trace/propagation/detail/string_test.cc | 13 ++++++++++ 4 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 api/test/trace/propagation/detail/string_test.cc diff --git a/api/include/opentelemetry/trace/propagation/detail/string.h b/api/include/opentelemetry/trace/propagation/detail/string.h index de4699952f..43b31725bc 100644 --- a/api/include/opentelemetry/trace/propagation/detail/string.h +++ b/api/include/opentelemetry/trace/propagation/detail/string.h @@ -57,6 +57,30 @@ inline size_t SplitString(nostd::string_view s, return filled; } +inline nostd::string_view TrimString(nostd::string_view s) +{ + size_t str_size = s.size(); + size_t trimmed_str_start_pos = 0; + size_t trimmed_str_end_pos = 0; + bool start_pos_found = false; + for (size_t i = 0; i < str_size; i++) + { + if (!isspace(s[i])) + { + if (!start_pos_found) + { + trimmed_str_start_pos = i; + trimmed_str_end_pos = i; + start_pos_found = true; + } else + { + trimmed_str_end_pos = i; + } + } + } + return s.substr(trimmed_str_start_pos, trimmed_str_end_pos - trimmed_str_start_pos + 1); +} + } // namespace detail } // namespace propagation } // namespace trace diff --git a/api/test/trace/propagation/detail/BUILD b/api/test/trace/propagation/detail/BUILD index a082586650..322c892937 100644 --- a/api/test/trace/propagation/detail/BUILD +++ b/api/test/trace/propagation/detail/BUILD @@ -18,3 +18,19 @@ cc_test( "@com_google_googletest//:gtest_main", ], ) + +cc_test( + name = "string_test", + srcs = [ + "string_test.cc", + ], + tags = [ + "api", + "test", + "trace", + ], + deps = [ + "//api", + "@com_google_googletest//:gtest_main", + ], +) diff --git a/api/test/trace/propagation/detail/CMakeLists.txt b/api/test/trace/propagation/detail/CMakeLists.txt index 34e2f6ae40..c70478efa1 100644 --- a/api/test/trace/propagation/detail/CMakeLists.txt +++ b/api/test/trace/propagation/detail/CMakeLists.txt @@ -1,7 +1,7 @@ # Copyright The OpenTelemetry Authors # SPDX-License-Identifier: Apache-2.0 -foreach(testname hex_test) +foreach(testname hex_test string_test) add_executable(${testname} "${testname}.cc") target_link_libraries(${testname} ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT} opentelemetry_api) diff --git a/api/test/trace/propagation/detail/string_test.cc b/api/test/trace/propagation/detail/string_test.cc new file mode 100644 index 0000000000..920410edfb --- /dev/null +++ b/api/test/trace/propagation/detail/string_test.cc @@ -0,0 +1,13 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#include +#include +#include + +#include "opentelemetry/nostd/string_view.h" + +TEST(StringTest, TrimStrings) +{ + //TODO(psx95): Add tests +} \ No newline at end of file From 9fcef5b0270ed19d319afb78533b2dae21e38e18 Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Wed, 30 Oct 2024 21:54:04 +0000 Subject: [PATCH 07/32] Fix test_traceparent_ows_handling --- .../opentelemetry/trace/propagation/http_trace_context.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api/include/opentelemetry/trace/propagation/http_trace_context.h b/api/include/opentelemetry/trace/propagation/http_trace_context.h index 2f61b23689..d80f8e7c14 100644 --- a/api/include/opentelemetry/trace/propagation/http_trace_context.h +++ b/api/include/opentelemetry/trace/propagation/http_trace_context.h @@ -182,7 +182,8 @@ class HttpTraceContext : public context::propagation::TextMapPropagator static SpanContext ExtractImpl(const context::propagation::TextMapCarrier &carrier) { - nostd::string_view trace_parent = carrier.Get(kTraceParent); + // Get trace_parent after trimming the leading and trailing whitespaces + nostd::string_view trace_parent = detail::TrimString(carrier.Get(kTraceParent)); nostd::string_view trace_state = carrier.Get(kTraceState); if (trace_parent == "") { From cc31fbbd34dbfc2c3c7fbae3801951ba12401254 Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Thu, 31 Oct 2024 01:44:51 +0000 Subject: [PATCH 08/32] Fix the Trim function to include all whitespace characters --- api/include/opentelemetry/common/string_util.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/include/opentelemetry/common/string_util.h b/api/include/opentelemetry/common/string_util.h index a7070a0acd..273a65272b 100644 --- a/api/include/opentelemetry/common/string_util.h +++ b/api/include/opentelemetry/common/string_util.h @@ -15,11 +15,11 @@ class StringUtil public: static nostd::string_view Trim(nostd::string_view str, size_t left, size_t right) noexcept { - while (left <= right && str[static_cast(left)] == ' ') + while (left <= right && isspace(str[left])) { left++; } - while (left <= right && str[static_cast(right)] == ' ') + while (left <= right && isspace(str[right])) { right--; } From 637442465d0121fb1b4651692df48ba719fb1b8d Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Thu, 31 Oct 2024 01:46:10 +0000 Subject: [PATCH 09/32] Replace the use of detail::TrimString with common::Trim --- .../trace/propagation/detail/string.h | 25 ------------------- .../trace/propagation/http_trace_context.h | 2 +- .../trace/propagation/detail/string_test.cc | 2 +- 3 files changed, 2 insertions(+), 27 deletions(-) diff --git a/api/include/opentelemetry/trace/propagation/detail/string.h b/api/include/opentelemetry/trace/propagation/detail/string.h index 43b31725bc..2a0d149869 100644 --- a/api/include/opentelemetry/trace/propagation/detail/string.h +++ b/api/include/opentelemetry/trace/propagation/detail/string.h @@ -56,31 +56,6 @@ inline size_t SplitString(nostd::string_view s, return filled; } - -inline nostd::string_view TrimString(nostd::string_view s) -{ - size_t str_size = s.size(); - size_t trimmed_str_start_pos = 0; - size_t trimmed_str_end_pos = 0; - bool start_pos_found = false; - for (size_t i = 0; i < str_size; i++) - { - if (!isspace(s[i])) - { - if (!start_pos_found) - { - trimmed_str_start_pos = i; - trimmed_str_end_pos = i; - start_pos_found = true; - } else - { - trimmed_str_end_pos = i; - } - } - } - return s.substr(trimmed_str_start_pos, trimmed_str_end_pos - trimmed_str_start_pos + 1); -} - } // namespace detail } // namespace propagation } // namespace trace diff --git a/api/include/opentelemetry/trace/propagation/http_trace_context.h b/api/include/opentelemetry/trace/propagation/http_trace_context.h index d80f8e7c14..7dcf07ae00 100644 --- a/api/include/opentelemetry/trace/propagation/http_trace_context.h +++ b/api/include/opentelemetry/trace/propagation/http_trace_context.h @@ -183,7 +183,7 @@ class HttpTraceContext : public context::propagation::TextMapPropagator static SpanContext ExtractImpl(const context::propagation::TextMapCarrier &carrier) { // Get trace_parent after trimming the leading and trailing whitespaces - nostd::string_view trace_parent = detail::TrimString(carrier.Get(kTraceParent)); + nostd::string_view trace_parent = common::StringUtil::Trim(carrier.Get(kTraceParent)); nostd::string_view trace_state = carrier.Get(kTraceState); if (trace_parent == "") { diff --git a/api/test/trace/propagation/detail/string_test.cc b/api/test/trace/propagation/detail/string_test.cc index 920410edfb..183914f291 100644 --- a/api/test/trace/propagation/detail/string_test.cc +++ b/api/test/trace/propagation/detail/string_test.cc @@ -7,7 +7,7 @@ #include "opentelemetry/nostd/string_view.h" -TEST(StringTest, TrimStrings) +TEST(StringTest, SplitString) { //TODO(psx95): Add tests } \ No newline at end of file From a756114a706c4bebb6b20d54b008d8e8ee60c00d Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Thu, 31 Oct 2024 01:51:46 +0000 Subject: [PATCH 10/32] Avoid unnecessary style change --- api/include/opentelemetry/trace/propagation/detail/string.h | 1 + 1 file changed, 1 insertion(+) diff --git a/api/include/opentelemetry/trace/propagation/detail/string.h b/api/include/opentelemetry/trace/propagation/detail/string.h index 2a0d149869..de4699952f 100644 --- a/api/include/opentelemetry/trace/propagation/detail/string.h +++ b/api/include/opentelemetry/trace/propagation/detail/string.h @@ -56,6 +56,7 @@ inline size_t SplitString(nostd::string_view s, return filled; } + } // namespace detail } // namespace propagation } // namespace trace From b856c55b316537dd72e8e4af3c7749aa50132dca Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Thu, 31 Oct 2024 01:59:24 +0000 Subject: [PATCH 11/32] Update string util test cases to include other whitespaces --- api/test/common/string_util_test.cc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/api/test/common/string_util_test.cc b/api/test/common/string_util_test.cc index f074d4fbe3..8746607766 100644 --- a/api/test/common/string_util_test.cc +++ b/api/test/common/string_util_test.cc @@ -35,9 +35,15 @@ TEST(StringUtilTest, TrimString) {"k1=v1,k2=v2, k3=v3", "k1=v1,k2=v2, k3=v3"}, {" k1=v1", "k1=v1"}, {"k1=v1 ", "k1=v1"}, + {"k1=v1\t", "k1=v1"}, + {"\t k1=v1 \t", "k1=v1"}, + {"\t\t k1=v1\t ", "k1=v1"}, + {"\t\t k1=v1\t ,k2=v2", "k1=v1\t ,k2=v2"}, {" k1=v1 ", "k1=v1"}, {" ", ""}, - {"", ""}}; + {"", ""}, + {"\n_some string_\t", "_some string_"}}; + for (auto &testcase : testcases) { EXPECT_EQ(StringUtil::Trim(testcase.input), testcase.expected); From 66599cefaf4d10aaf5982180e4bff13105e7f1fc Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Thu, 31 Oct 2024 02:15:35 +0000 Subject: [PATCH 12/32] Fix cmake clang warnings --- api/test/trace/propagation/detail/string_test.cc | 2 +- ext/test/w3c_tracecontext_test/main.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/api/test/trace/propagation/detail/string_test.cc b/api/test/trace/propagation/detail/string_test.cc index 183914f291..52cbdb8833 100644 --- a/api/test/trace/propagation/detail/string_test.cc +++ b/api/test/trace/propagation/detail/string_test.cc @@ -9,5 +9,5 @@ TEST(StringTest, SplitString) { - //TODO(psx95): Add tests + // TODO(psx95): Add tests } \ No newline at end of file diff --git a/ext/test/w3c_tracecontext_test/main.cc b/ext/test/w3c_tracecontext_test/main.cc index 6af7f20f37..d3a7602c43 100644 --- a/ext/test/w3c_tracecontext_test/main.cc +++ b/ext/test/w3c_tracecontext_test/main.cc @@ -71,7 +71,7 @@ class TextMapCarrierTest : public context::propagation::TextMapCarrier { return false; } - for (int i = 0; i < str1.length(); i++) + for (size_t i = 0; i < str1.length(); i++) { if (tolower(str1[i]) != tolower(str2[i])) { From b460bd4903e03048198a6e1b2314e45184573095 Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Thu, 31 Oct 2024 17:09:21 +0000 Subject: [PATCH 13/32] Add tests for SplitString --- .../trace/propagation/detail/string_test.cc | 53 +++++++++++++++++-- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/api/test/trace/propagation/detail/string_test.cc b/api/test/trace/propagation/detail/string_test.cc index 52cbdb8833..6402e98e20 100644 --- a/api/test/trace/propagation/detail/string_test.cc +++ b/api/test/trace/propagation/detail/string_test.cc @@ -2,12 +2,57 @@ // SPDX-License-Identifier: Apache-2.0 #include -#include +#include #include #include "opentelemetry/nostd/string_view.h" -TEST(StringTest, SplitString) +struct SplitStringTestData { - // TODO(psx95): Add tests -} \ No newline at end of file + opentelemetry::nostd::string_view input; + char separator; + size_t max_count; + std::vector splits; + size_t expected_number_strings; +}; + +const SplitStringTestData split_string_test_cases[] = { + {"foo,bar,baz", ',', 4, + std::vector{"foo", "bar", "baz"}, 3}, + {"foo,bar,baz,foobar", ',', 4, + std::vector{"foo", "bar", "baz", "foobar"}, 4}, + {"foo,bar,baz,foobar", '.', 4, + std::vector{"foo,bar,baz,foobar"}, 1}, + {"foo,bar,baz,", ',', 4, + std::vector{"foo","bar","baz",""}, 4}, + {"foo,bar,baz,", ',', 2, + std::vector{"foo","bar"}, 2}, + {"foo ,bar, baz ", ',', 4, + std::vector{"foo ","bar"," baz "}, 3}, + {"foo ,bar, baz ", ',', 4, + std::vector{"foo ","bar"," baz "}, 3}, + {"00-0af7651916cd43dd8448eb211c80319c-00f067aa0ba902b7-01", '-', 4, + std::vector{"00","0af7651916cd43dd8448eb211c80319c","00f067aa0ba902b7", "01"}, 4}, +}; + +// Test fixture +class SplitStringTestFixture : public ::testing::TestWithParam {}; + +TEST_P(SplitStringTestFixture, SplitsAsExpected) +{ + SplitStringTestData test_param = GetParam(); + std::vector fields{}; + fields.reserve(test_param.expected_number_strings); + size_t got_splits_num = opentelemetry::trace::propagation::detail::SplitString(test_param.input, test_param.separator, fields.data(), test_param.max_count); + + + // Assert on the output + EXPECT_EQ(got_splits_num, test_param.expected_number_strings); + for (size_t i = 0; i < got_splits_num; i++) + { + // Checks for resulting strings in-order + EXPECT_EQ(fields[i], test_param.splits[i]); + } +} + +INSTANTIATE_TEST_SUITE_P(SplitStringTestCases, SplitStringTestFixture, ::testing::ValuesIn(split_string_test_cases)); From 75aa431b5e31f7a5416e0f7dcfd91b837a2d79be Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Thu, 31 Oct 2024 18:37:55 +0000 Subject: [PATCH 14/32] Run format tool --- .../trace/propagation/http_trace_context.h | 2 +- .../trace/propagation/detail/string_test.cc | 27 ++++++++++--------- .../ext/http/server/http_server.h | 6 +++-- ext/test/w3c_tracecontext_test/main.cc | 2 +- 4 files changed, 21 insertions(+), 16 deletions(-) diff --git a/api/include/opentelemetry/trace/propagation/http_trace_context.h b/api/include/opentelemetry/trace/propagation/http_trace_context.h index 7dcf07ae00..608f93e9dc 100644 --- a/api/include/opentelemetry/trace/propagation/http_trace_context.h +++ b/api/include/opentelemetry/trace/propagation/http_trace_context.h @@ -86,7 +86,7 @@ class HttpTraceContext : public context::propagation::TextMapPropagator } private: - static constexpr uint8_t kInvalidVersion = 0xFF; + static constexpr uint8_t kInvalidVersion = 0xFF; static constexpr uint8_t kDefaultAssumedVersion = 0x00; static bool IsValidVersion(nostd::string_view version_hex) diff --git a/api/test/trace/propagation/detail/string_test.cc b/api/test/trace/propagation/detail/string_test.cc index 6402e98e20..c7c0931678 100644 --- a/api/test/trace/propagation/detail/string_test.cc +++ b/api/test/trace/propagation/detail/string_test.cc @@ -17,34 +17,35 @@ struct SplitStringTestData }; const SplitStringTestData split_string_test_cases[] = { - {"foo,bar,baz", ',', 4, - std::vector{"foo", "bar", "baz"}, 3}, + {"foo,bar,baz", ',', 4, std::vector{"foo", "bar", "baz"}, 3}, {"foo,bar,baz,foobar", ',', 4, std::vector{"foo", "bar", "baz", "foobar"}, 4}, {"foo,bar,baz,foobar", '.', 4, std::vector{"foo,bar,baz,foobar"}, 1}, {"foo,bar,baz,", ',', 4, - std::vector{"foo","bar","baz",""}, 4}, - {"foo,bar,baz,", ',', 2, - std::vector{"foo","bar"}, 2}, + std::vector{"foo", "bar", "baz", ""}, 4}, + {"foo,bar,baz,", ',', 2, std::vector{"foo", "bar"}, 2}, {"foo ,bar, baz ", ',', 4, - std::vector{"foo ","bar"," baz "}, 3}, + std::vector{"foo ", "bar", " baz "}, 3}, {"foo ,bar, baz ", ',', 4, - std::vector{"foo ","bar"," baz "}, 3}, + std::vector{"foo ", "bar", " baz "}, 3}, {"00-0af7651916cd43dd8448eb211c80319c-00f067aa0ba902b7-01", '-', 4, - std::vector{"00","0af7651916cd43dd8448eb211c80319c","00f067aa0ba902b7", "01"}, 4}, + std::vector{"00", "0af7651916cd43dd8448eb211c80319c", + "00f067aa0ba902b7", "01"}, + 4}, }; // Test fixture -class SplitStringTestFixture : public ::testing::TestWithParam {}; +class SplitStringTestFixture : public ::testing::TestWithParam +{}; TEST_P(SplitStringTestFixture, SplitsAsExpected) { SplitStringTestData test_param = GetParam(); std::vector fields{}; fields.reserve(test_param.expected_number_strings); - size_t got_splits_num = opentelemetry::trace::propagation::detail::SplitString(test_param.input, test_param.separator, fields.data(), test_param.max_count); - + size_t got_splits_num = opentelemetry::trace::propagation::detail::SplitString( + test_param.input, test_param.separator, fields.data(), test_param.max_count); // Assert on the output EXPECT_EQ(got_splits_num, test_param.expected_number_strings); @@ -55,4 +56,6 @@ TEST_P(SplitStringTestFixture, SplitsAsExpected) } } -INSTANTIATE_TEST_SUITE_P(SplitStringTestCases, SplitStringTestFixture, ::testing::ValuesIn(split_string_test_cases)); +INSTANTIATE_TEST_SUITE_P(SplitStringTestCases, + SplitStringTestFixture, + ::testing::ValuesIn(split_string_test_cases)); diff --git a/ext/include/opentelemetry/ext/http/server/http_server.h b/ext/include/opentelemetry/ext/http/server/http_server.h index 67b123358a..21efac94e0 100644 --- a/ext/include/opentelemetry/ext/http/server/http_server.h +++ b/ext/include/opentelemetry/ext/http/server/http_server.h @@ -650,8 +650,10 @@ class HttpServer : private SocketTools::Reactor::SocketCallback } if (!conn.request.headers[name].empty()) { - conn.request.headers[name] = conn.request.headers[name].append(",").append(std::string(begin, ptr)); - } else + conn.request.headers[name] = + conn.request.headers[name].append(",").append(std::string(begin, ptr)); + } + else { conn.request.headers[name] = std::string(begin, ptr); } diff --git a/ext/test/w3c_tracecontext_test/main.cc b/ext/test/w3c_tracecontext_test/main.cc index d3a7602c43..2b4fa85500 100644 --- a/ext/test/w3c_tracecontext_test/main.cc +++ b/ext/test/w3c_tracecontext_test/main.cc @@ -51,7 +51,7 @@ class TextMapCarrierTest : public context::propagation::TextMapCarrier TextMapCarrierTest(std::map &headers) : headers_(headers) {} nostd::string_view Get(nostd::string_view key) const noexcept override { - for(const auto& elem : headers_) + for (const auto &elem : headers_) { if (equalsIgnoreCase(elem.first, std::string(key))) { From 0b0ef583cf1e3974952fd92fd319ecef0a9ae9da Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Thu, 31 Oct 2024 20:03:52 +0000 Subject: [PATCH 15/32] Fix valgrind issues with gtest --- api/test/trace/propagation/detail/string_test.cc | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/api/test/trace/propagation/detail/string_test.cc b/api/test/trace/propagation/detail/string_test.cc index c7c0931678..3a500d2027 100644 --- a/api/test/trace/propagation/detail/string_test.cc +++ b/api/test/trace/propagation/detail/string_test.cc @@ -14,6 +14,21 @@ struct SplitStringTestData size_t max_count; std::vector splits; size_t expected_number_strings; + + // When googletest registers parameterized tests, it uses this method to format the parameters. + // The default implementation prints hex dump of all bytes in the object. If there is any padding + // in these bytes, valgrind reports this as a warning - "Use of uninitialized bytes". + // See https://github.com/google/googletest/issues/3805. + friend void PrintTo(const SplitStringTestData& data, std::ostream* os) { + std::stringstream ss; + for (auto it = data.splits.begin(); it != data.splits.end(); it++) { + if (it != data.splits.begin()) { + ss << ","; + } + ss << *it; + } + *os << "(" << data.input << "," << data.separator << "," << data.max_count << "," << ss.str() << "," << data.expected_number_strings << ")"; + } }; const SplitStringTestData split_string_test_cases[] = { From ffab2ee2407e9b619edc5fd6f7c69fa626283140 Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Thu, 31 Oct 2024 21:55:00 +0000 Subject: [PATCH 16/32] Replace parameterized test with standard one --- .../trace/propagation/detail/string_test.cc | 116 ++++++++---------- 1 file changed, 53 insertions(+), 63 deletions(-) diff --git a/api/test/trace/propagation/detail/string_test.cc b/api/test/trace/propagation/detail/string_test.cc index 3a500d2027..f8c355e1cd 100644 --- a/api/test/trace/propagation/detail/string_test.cc +++ b/api/test/trace/propagation/detail/string_test.cc @@ -7,70 +7,60 @@ #include "opentelemetry/nostd/string_view.h" -struct SplitStringTestData -{ - opentelemetry::nostd::string_view input; - char separator; - size_t max_count; - std::vector splits; - size_t expected_number_strings; - - // When googletest registers parameterized tests, it uses this method to format the parameters. - // The default implementation prints hex dump of all bytes in the object. If there is any padding - // in these bytes, valgrind reports this as a warning - "Use of uninitialized bytes". - // See https://github.com/google/googletest/issues/3805. - friend void PrintTo(const SplitStringTestData& data, std::ostream* os) { - std::stringstream ss; - for (auto it = data.splits.begin(); it != data.splits.end(); it++) { - if (it != data.splits.begin()) { - ss << ","; - } - ss << *it; - } - *os << "(" << data.input << "," << data.separator << "," << data.max_count << "," << ss.str() << "," << data.expected_number_strings << ")"; - } -}; - -const SplitStringTestData split_string_test_cases[] = { - {"foo,bar,baz", ',', 4, std::vector{"foo", "bar", "baz"}, 3}, - {"foo,bar,baz,foobar", ',', 4, - std::vector{"foo", "bar", "baz", "foobar"}, 4}, - {"foo,bar,baz,foobar", '.', 4, - std::vector{"foo,bar,baz,foobar"}, 1}, - {"foo,bar,baz,", ',', 4, - std::vector{"foo", "bar", "baz", ""}, 4}, - {"foo,bar,baz,", ',', 2, std::vector{"foo", "bar"}, 2}, - {"foo ,bar, baz ", ',', 4, - std::vector{"foo ", "bar", " baz "}, 3}, - {"foo ,bar, baz ", ',', 4, - std::vector{"foo ", "bar", " baz "}, 3}, - {"00-0af7651916cd43dd8448eb211c80319c-00f067aa0ba902b7-01", '-', 4, - std::vector{"00", "0af7651916cd43dd8448eb211c80319c", - "00f067aa0ba902b7", "01"}, - 4}, -}; +using namespace opentelemetry; -// Test fixture -class SplitStringTestFixture : public ::testing::TestWithParam -{}; +// struct SplitStringTestData +//{ +// opentelemetry::nostd::string_view input; +// char separator; +// size_t max_count; +// std::vector splits; +// size_t expected_number_strings; +// }; +// +// const SplitStringTestData split_string_test_cases[] = { +// {"foo,bar,baz", ',', 4, std::vector{"foo", "bar", "baz"}, +// 3}, +// {"foo,bar,baz,foobar", ',', 4, +// std::vector{"foo", "bar", "baz", "foobar"}, 4}, +// {"foo,bar,baz,foobar", '.', 4, +// std::vector{"foo,bar,baz,foobar"}, 1}, +// {"foo,bar,baz,", ',', 4, +// std::vector{"foo", "bar", "baz", ""}, 4}, +// {"foo,bar,baz,", ',', 2, std::vector{"foo", "bar"}, 2}, +// {"foo ,bar, baz ", ',', 4, +// std::vector{"foo ", "bar", " baz "}, 3}, +// {"foo ,bar, baz ", ',', 4, +// std::vector{"foo ", "bar", " baz "}, 3}, +// {"00-0af7651916cd43dd8448eb211c80319c-00f067aa0ba902b7-01", '-', 4, +// std::vector{"00", "0af7651916cd43dd8448eb211c80319c", +// "00f067aa0ba902b7", "01"}, +// 4}, +// }; +// +// +// TEST(StringTest, SplitStringTest) +//{ +// for (auto &test_param : split_string_test_cases) { +// std::vector fields{}; +// fields.reserve(test_param.expected_number_strings); +// size_t got_splits_num = opentelemetry::trace::propagation::detail::SplitString( +// test_param.input, test_param.separator, fields.data(), test_param.max_count); +// +// // Assert on the output +// EXPECT_EQ(got_splits_num, test_param.expected_number_strings); +// for (size_t i = 0; i < got_splits_num; i++) +// { +// // Checks for resulting strings in-order +// EXPECT_EQ(fields[i], test_param.splits[i]); +// } +// } +// } -TEST_P(SplitStringTestFixture, SplitsAsExpected) +TEST(StringTest, SimpleTest) { - SplitStringTestData test_param = GetParam(); - std::vector fields{}; - fields.reserve(test_param.expected_number_strings); - size_t got_splits_num = opentelemetry::trace::propagation::detail::SplitString( - test_param.input, test_param.separator, fields.data(), test_param.max_count); - - // Assert on the output - EXPECT_EQ(got_splits_num, test_param.expected_number_strings); - for (size_t i = 0; i < got_splits_num; i++) - { - // Checks for resulting strings in-order - EXPECT_EQ(fields[i], test_param.splits[i]); - } + nostd::string_view input = "foo,bar,baz"; + std::array fields{}; + size_t got_splits = ::trace::propagation::detail::SplitString(input, ',', fields.data(), 4); + EXPECT_EQ(3, got_splits); } - -INSTANTIATE_TEST_SUITE_P(SplitStringTestCases, - SplitStringTestFixture, - ::testing::ValuesIn(split_string_test_cases)); From e7cfb9ae60a93222883cf491e374d2c33d91473d Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Fri, 1 Nov 2024 03:30:46 +0000 Subject: [PATCH 17/32] Move the test cases within test scope --- .../trace/propagation/detail/string_test.cc | 65 +++++++++++++------ 1 file changed, 45 insertions(+), 20 deletions(-) diff --git a/api/test/trace/propagation/detail/string_test.cc b/api/test/trace/propagation/detail/string_test.cc index f8c355e1cd..43c81ad93c 100644 --- a/api/test/trace/propagation/detail/string_test.cc +++ b/api/test/trace/propagation/detail/string_test.cc @@ -8,7 +8,7 @@ #include "opentelemetry/nostd/string_view.h" using namespace opentelemetry; - +// // struct SplitStringTestData //{ // opentelemetry::nostd::string_view input; @@ -37,25 +37,50 @@ using namespace opentelemetry; // "00f067aa0ba902b7", "01"}, // 4}, // }; -// -// -// TEST(StringTest, SplitStringTest) -//{ -// for (auto &test_param : split_string_test_cases) { -// std::vector fields{}; -// fields.reserve(test_param.expected_number_strings); -// size_t got_splits_num = opentelemetry::trace::propagation::detail::SplitString( -// test_param.input, test_param.separator, fields.data(), test_param.max_count); -// -// // Assert on the output -// EXPECT_EQ(got_splits_num, test_param.expected_number_strings); -// for (size_t i = 0; i < got_splits_num; i++) -// { -// // Checks for resulting strings in-order -// EXPECT_EQ(fields[i], test_param.splits[i]); -// } -// } -// } + +TEST(StringTest, SplitStringTest) +{ + struct + { + opentelemetry::nostd::string_view input; + char separator; + size_t max_count; + std::vector splits; + size_t expected_number_strings; + } test_cases[] = { + {"foo,bar,baz", ',', 4, std::vector{"foo", "bar", "baz"}, + 3}, +// {"foo,bar,baz,foobar", ',', 4, +// std::vector{"foo", "bar", "baz", "foobar"}, 4}, +// {"foo,bar,baz,foobar", '.', 4, +// std::vector{"foo,bar,baz,foobar"}, 1}, +// {"foo,bar,baz,", ',', 4, +// std::vector{"foo", "bar", "baz", ""}, 4}, +// {"foo,bar,baz,", ',', 2, std::vector{"foo", "bar"}, 2}, +// {"foo ,bar, baz ", ',', 4, +// std::vector{"foo ", "bar", " baz "}, 3}, +// {"foo ,bar, baz ", ',', 4, +// std::vector{"foo ", "bar", " baz "}, 3}, +// {"00-0af7651916cd43dd8448eb211c80319c-00f067aa0ba902b7-01", '-', 4, +// std::vector{"00", "0af7651916cd43dd8448eb211c80319c", +// "00f067aa0ba902b7", "01"},4} + }; + for (auto &test_param : test_cases) + { + std::vector fields{}; + fields.reserve(test_param.expected_number_strings); + size_t got_splits_num = opentelemetry::trace::propagation::detail::SplitString( + test_param.input, test_param.separator, fields.data(), test_param.max_count); + + // Assert on the output + EXPECT_EQ(got_splits_num, test_param.expected_number_strings); + for (size_t i = 0; i < got_splits_num; i++) + { + // Checks for resulting strings in-order + EXPECT_EQ(fields[i], test_param.splits[i]); + } + } +} TEST(StringTest, SimpleTest) { From 06283f7acd6f20e229a285568d48dca1fc42c866 Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Fri, 1 Nov 2024 04:40:53 +0000 Subject: [PATCH 18/32] Remove vector from testcase struct --- .../trace/propagation/detail/string_test.cc | 121 +++++++----------- 1 file changed, 47 insertions(+), 74 deletions(-) diff --git a/api/test/trace/propagation/detail/string_test.cc b/api/test/trace/propagation/detail/string_test.cc index 43c81ad93c..96a506cfc9 100644 --- a/api/test/trace/propagation/detail/string_test.cc +++ b/api/test/trace/propagation/detail/string_test.cc @@ -8,84 +8,57 @@ #include "opentelemetry/nostd/string_view.h" using namespace opentelemetry; -// -// struct SplitStringTestData -//{ -// opentelemetry::nostd::string_view input; -// char separator; -// size_t max_count; -// std::vector splits; -// size_t expected_number_strings; -// }; -// -// const SplitStringTestData split_string_test_cases[] = { -// {"foo,bar,baz", ',', 4, std::vector{"foo", "bar", "baz"}, -// 3}, -// {"foo,bar,baz,foobar", ',', 4, -// std::vector{"foo", "bar", "baz", "foobar"}, 4}, -// {"foo,bar,baz,foobar", '.', 4, -// std::vector{"foo,bar,baz,foobar"}, 1}, -// {"foo,bar,baz,", ',', 4, -// std::vector{"foo", "bar", "baz", ""}, 4}, -// {"foo,bar,baz,", ',', 2, std::vector{"foo", "bar"}, 2}, -// {"foo ,bar, baz ", ',', 4, -// std::vector{"foo ", "bar", " baz "}, 3}, -// {"foo ,bar, baz ", ',', 4, -// std::vector{"foo ", "bar", " baz "}, 3}, -// {"00-0af7651916cd43dd8448eb211c80319c-00f067aa0ba902b7-01", '-', 4, -// std::vector{"00", "0af7651916cd43dd8448eb211c80319c", -// "00f067aa0ba902b7", "01"}, -// 4}, -// }; -TEST(StringTest, SplitStringTest) +namespace { - struct - { - opentelemetry::nostd::string_view input; - char separator; - size_t max_count; - std::vector splits; - size_t expected_number_strings; - } test_cases[] = { - {"foo,bar,baz", ',', 4, std::vector{"foo", "bar", "baz"}, - 3}, -// {"foo,bar,baz,foobar", ',', 4, -// std::vector{"foo", "bar", "baz", "foobar"}, 4}, -// {"foo,bar,baz,foobar", '.', 4, -// std::vector{"foo,bar,baz,foobar"}, 1}, -// {"foo,bar,baz,", ',', 4, -// std::vector{"foo", "bar", "baz", ""}, 4}, -// {"foo,bar,baz,", ',', 2, std::vector{"foo", "bar"}, 2}, -// {"foo ,bar, baz ", ',', 4, -// std::vector{"foo ", "bar", " baz "}, 3}, -// {"foo ,bar, baz ", ',', 4, -// std::vector{"foo ", "bar", " baz "}, 3}, -// {"00-0af7651916cd43dd8448eb211c80319c-00f067aa0ba902b7-01", '-', 4, -// std::vector{"00", "0af7651916cd43dd8448eb211c80319c", -// "00f067aa0ba902b7", "01"},4} - }; - for (auto &test_param : test_cases) - { - std::vector fields{}; - fields.reserve(test_param.expected_number_strings); - size_t got_splits_num = opentelemetry::trace::propagation::detail::SplitString( - test_param.input, test_param.separator, fields.data(), test_param.max_count); - // Assert on the output - EXPECT_EQ(got_splits_num, test_param.expected_number_strings); - for (size_t i = 0; i < got_splits_num; i++) - { - // Checks for resulting strings in-order - EXPECT_EQ(fields[i], test_param.splits[i]); - } +struct SplitStringTestData +{ + opentelemetry::nostd::string_view input; + char separator; + size_t max_count; + size_t expected_number_strings; + + // When googletest registers parameterized tests, it uses this method to format the parameters. + // The default implementation prints hex dump of all bytes in the object. If there is any padding + // in these bytes, valgrind reports this as a warning - "Use of uninitialized bytes". + // See https://github.com/google/googletest/issues/3805. + friend void PrintTo(const SplitStringTestData &data, std::ostream *os) + { + std::stringstream ss; + *os << "(" << data.input << "," << data.separator << "," << data.max_count << "," + << data.expected_number_strings << ")"; } -} +}; + +const SplitStringTestData split_string_test_cases[] = { + {"foo,bar,baz", ',', 4, 3}, + {"foo,bar,baz,foobar", ',', 4, 4}, + {"foo,bar,baz,foobar", '.', 4, 1}, + {"foo,bar,baz,", ',', 4, 4}, + {"foo,bar,baz,", ',', 2, 2}, + {"foo ,bar, baz ", ',', 4, 3}, + {"foo ,bar, baz ", ',', 4, 3}, + {"00-0af7651916cd43dd8448eb211c80319c-00f067aa0ba902b7-01", '-', 4, 4}, +}; +} // namespace + +// Test fixture +class SplitStringTestFixture : public ::testing::TestWithParam +{}; -TEST(StringTest, SimpleTest) +TEST_P(SplitStringTestFixture, SplitsAsExpected) { - nostd::string_view input = "foo,bar,baz"; - std::array fields{}; - size_t got_splits = ::trace::propagation::detail::SplitString(input, ',', fields.data(), 4); - EXPECT_EQ(3, got_splits); + const SplitStringTestData test_param = GetParam(); + std::vector fields{}; + fields.reserve(test_param.expected_number_strings); + size_t got_splits_num = opentelemetry::trace::propagation::detail::SplitString( + test_param.input, test_param.separator, fields.data(), test_param.max_count); + + // Assert on the output + EXPECT_EQ(got_splits_num, test_param.expected_number_strings); } + +INSTANTIATE_TEST_SUITE_P(SplitStringTestCases, + SplitStringTestFixture, + ::testing::ValuesIn(split_string_test_cases)); From 5d1d47655b56f84c665a66340c43e52ce92c1667 Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Fri, 1 Nov 2024 18:40:43 +0000 Subject: [PATCH 19/32] Add CI for trace context validation --- .github/workflows/ci.yml | 37 +++++++++++++++++++++++++++++++++++++ ci/do_ci.sh | 13 +++++++++++++ 2 files changed, 50 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8c17bbece0..ed6ec661d9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -923,3 +923,40 @@ jobs: - name: run ./ci/docfx.cmd shell: cmd run: ./ci/docfx.cmd + + w3c_trace_context_compliance_v1: + name: W3C Distributed Tracing Validation V1 + runs-on: ubuntu-24.04 + steps: + - name: Checkout open-telemetry/opentelemetry-cpp + uses: actions/checkout@v4 + with: + submodules: 'recursive' + - name: setup + env: + CC: /usr/bin/gcc-14 + CXX: /usr/bin/g++-14 + PROTOBUF_VERSION: 21.12 + run: | + sudo -E ./ci/setup_googletest.sh + sudo -E ./ci/setup_ci_environment.sh + - name: run w3c trace-context test server (background) + env: + CXX_STANDARD: '14' + run: | + ./ci/do_ci.sh cmake.w3c.trace-context.build-server + cd $HOME/build/ext/test/w3c_tracecontext_test + ./w3c_tracecontext_test & + - name: Checkout w3c/trace-context repo + uses: actions/checkout@v4 + with: + repository: w3c/trace-context + path: trace-context + - name: install dependencies + run: | + sudo apt update && sudo apt install python3-pip + sudo pip3 install aiohttp + - name: run w3c trace-context test suite + run: + | + python ${GITHUB_WORKSPACE}/trace-context/test/test.py http://localhost:30000/test diff --git a/ci/do_ci.sh b/ci/do_ci.sh index c31d5e887a..ebc3e45db2 100755 --- a/ci/do_ci.sh +++ b/ci/do_ci.sh @@ -393,6 +393,19 @@ elif [[ "$1" == "cmake.exporter.otprotocol.with_async_export.test" ]]; then make -j $(nproc) cd exporters/otlp && make test exit 0 +elif [[ "$1" == "cmake.w3c.trace-context.build-server" ]]; then + echo "Building w3c trace context test server" + cd "${BUILD_DIR}" + rm -rf * + cmake "${CMAKE_OPTIONS[@]}" \ + -DBUILD_W3CTRACECONTEXT_TEST=ON \ + -DCMAKE_CXX_STANDARD=${CXX_STANDARD} \ + "${SRC_DIR}" + eval "$MAKE_COMMAND" + echo "currently at $(pwd)" + echo "${BUILD_DIR}" + ls -lah "${BUILD_DIR}/ext/test/w3c_tracecontext_test" + exit 0 elif [[ "$1" == "cmake.do_not_install.test" ]]; then cd "${BUILD_DIR}" rm -rf * From 22b8eb11b1c572ffa88c32bf1e6f23da9cf6b5de Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Fri, 1 Nov 2024 22:48:43 +0000 Subject: [PATCH 20/32] Stop server using a signal callback Using cin.get() as a means to stop the server listening for requests is preventing the server from being used in a CI environment. This also allows the test server to run in background since exit is now controlled by a http request. --- ext/test/w3c_tracecontext_test/main.cc | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/ext/test/w3c_tracecontext_test/main.cc b/ext/test/w3c_tracecontext_test/main.cc index 2b4fa85500..7033efeaa6 100644 --- a/ext/test/w3c_tracecontext_test/main.cc +++ b/ext/test/w3c_tracecontext_test/main.cc @@ -179,6 +179,7 @@ int main(int argc, char *argv[]) constexpr char default_host[] = "localhost"; constexpr uint16_t default_port = 30000; uint16_t port; + std::atomic_bool stop_server(false); // The port the validation service listens to can be specified via the command line. if (argc > 1) @@ -225,16 +226,31 @@ int main(int argc, char *argv[]) return 0; }}; + testing::HttpRequestCallback stop_cb{ + [&](testing::HttpRequest const & /*req*/, testing::HttpResponse &resp) { + std::cout << "Received request to stop server \n"; + stop_server.store(true); + resp.code = 200; + return 0; + }}; + server["/test"] = test_cb; + server["/stop"] = stop_cb; // Start server server.start(); std::cout << "Listening at http://" << default_host << ":" << port << "/test\n"; - // Wait for console input - std::cin.get(); - - // Stop server - server.stop(); + // Wait for signal to stop server + std::thread server_check([&stop_server, &server]() { + while (!stop_server.load()) + { + // keep running the thread + } + // received signal to stop server + std::cout << "stopping server \n"; + server.stop(); + }); + server_check.join(); } From 175df2eeabf974abc3684f20a832a3931594d869 Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Fri, 1 Nov 2024 22:59:13 +0000 Subject: [PATCH 21/32] Remove debugging statements from CI Also explicitly stops the background server serving requests. --- .github/workflows/ci.yml | 1 + ci/do_ci.sh | 3 --- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ed6ec661d9..ee39e2a243 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -960,3 +960,4 @@ jobs: run: | python ${GITHUB_WORKSPACE}/trace-context/test/test.py http://localhost:30000/test + curl http://localhost:30000/stop diff --git a/ci/do_ci.sh b/ci/do_ci.sh index ebc3e45db2..11dbe4ef80 100755 --- a/ci/do_ci.sh +++ b/ci/do_ci.sh @@ -402,9 +402,6 @@ elif [[ "$1" == "cmake.w3c.trace-context.build-server" ]]; then -DCMAKE_CXX_STANDARD=${CXX_STANDARD} \ "${SRC_DIR}" eval "$MAKE_COMMAND" - echo "currently at $(pwd)" - echo "${BUILD_DIR}" - ls -lah "${BUILD_DIR}/ext/test/w3c_tracecontext_test" exit 0 elif [[ "$1" == "cmake.do_not_install.test" ]]; then cd "${BUILD_DIR}" From e2c2805ac85e4968111b1fbc269d9c514767c8b1 Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Fri, 1 Nov 2024 23:06:12 +0000 Subject: [PATCH 22/32] Specify SPEC_LEVEL and test suites to run --- .github/workflows/ci.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ee39e2a243..f5f49540e4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -957,7 +957,9 @@ jobs: sudo apt update && sudo apt install python3-pip sudo pip3 install aiohttp - name: run w3c trace-context test suite + env: + SPEC_LEVEL: 1 run: | - python ${GITHUB_WORKSPACE}/trace-context/test/test.py http://localhost:30000/test + python ${GITHUB_WORKSPACE}/trace-context/test/test.py http://localhost:30000/test TraceContextTest AdvancedTest curl http://localhost:30000/stop From 444abe69482f77e7881c8e900a519d4b32f9b67a Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Fri, 1 Nov 2024 23:13:57 +0000 Subject: [PATCH 23/32] Update Changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 540479eb97..76ea291ec7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,9 @@ Increment the: ## [Unreleased] +* [TEST] Fix failing W3C Trace Context compliance tests [#3115](https://github.com/open-telemetry/opentelemetry-cpp/pull/3115) + * Also adds CI check to ensure continued compliance + * [API] Jaeger Propagator should not be deprecated [#3086](https://github.com/open-telemetry/opentelemetry-cpp/pull/3086) From 975ac71b15843d80c8a7ebcb5a4751e4ba9cfbc6 Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Mon, 4 Nov 2024 20:20:01 +0000 Subject: [PATCH 24/32] Remove extraneous thread and busy loop --- ext/test/w3c_tracecontext_test/main.cc | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/ext/test/w3c_tracecontext_test/main.cc b/ext/test/w3c_tracecontext_test/main.cc index 7033efeaa6..ea3fabbbdc 100644 --- a/ext/test/w3c_tracecontext_test/main.cc +++ b/ext/test/w3c_tracecontext_test/main.cc @@ -3,8 +3,6 @@ #include #include -#include -#include #include #include #include @@ -12,19 +10,16 @@ #include #include -#include "opentelemetry/context/context_value.h" #include "opentelemetry/context/propagation/text_map_propagator.h" #include "opentelemetry/context/runtime_context.h" #include "opentelemetry/exporters/ostream/span_exporter.h" #include "opentelemetry/ext/http/client/curl/http_client_curl.h" -#include "opentelemetry/ext/http/client/curl/http_operation_curl.h" #include "opentelemetry/ext/http/client/http_client.h" #include "opentelemetry/ext/http/server/http_server.h" #include "opentelemetry/nostd/shared_ptr.h" #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/sdk/trace/exporter.h" #include "opentelemetry/sdk/trace/processor.h" -#include "opentelemetry/sdk/trace/recordable.h" #include "opentelemetry/sdk/trace/simple_processor.h" #include "opentelemetry/sdk/trace/tracer_context.h" #include "opentelemetry/sdk/trace/tracer_provider.h" @@ -243,14 +238,11 @@ int main(int argc, char *argv[]) std::cout << "Listening at http://" << default_host << ":" << port << "/test\n"; // Wait for signal to stop server - std::thread server_check([&stop_server, &server]() { - while (!stop_server.load()) - { - // keep running the thread - } - // received signal to stop server - std::cout << "stopping server \n"; - server.stop(); - }); - server_check.join(); + while (!stop_server.load()) + { + std::this_thread::sleep_for(std::chrono::seconds(1)); + } + // received signal to stop server + std::cout << "Stopping server \n"; + server.stop(); } From a90c0537c5aaef2f48d7ef41e741825af24ae113 Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Mon, 4 Nov 2024 20:26:00 +0000 Subject: [PATCH 25/32] Move static function outside class --- ext/test/w3c_tracecontext_test/main.cc | 32 +++++++++++++------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/ext/test/w3c_tracecontext_test/main.cc b/ext/test/w3c_tracecontext_test/main.cc index ea3fabbbdc..d2ed772510 100644 --- a/ext/test/w3c_tracecontext_test/main.cc +++ b/ext/test/w3c_tracecontext_test/main.cc @@ -40,6 +40,22 @@ namespace { static trace_api::propagation::HttpTraceContext propagator_format; +static bool equalsIgnoreCase(const std::string &str1, const std::string &str2) +{ + if (str1.length() != str2.length()) + { + return false; + } + for (size_t i = 0; i < str1.length(); i++) + { + if (tolower(str1[i]) != tolower(str2[i])) + { + return false; + } + } + return true; +} + class TextMapCarrierTest : public context::propagation::TextMapCarrier { public: @@ -60,22 +76,6 @@ class TextMapCarrierTest : public context::propagation::TextMapCarrier headers_[std::string(key)] = std::string(value); } - static bool equalsIgnoreCase(const std::string &str1, const std::string &str2) - { - if (str1.length() != str2.length()) - { - return false; - } - for (size_t i = 0; i < str1.length(); i++) - { - if (tolower(str1[i]) != tolower(str2[i])) - { - return false; - } - } - return true; - } - std::map &headers_; }; From 17856108308145e9ea6ba3d5921df3c1f1a63701 Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Tue, 5 Nov 2024 05:29:12 +0000 Subject: [PATCH 26/32] Update test --- api/test/trace/propagation/detail/string_test.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/api/test/trace/propagation/detail/string_test.cc b/api/test/trace/propagation/detail/string_test.cc index 96a506cfc9..67a48a354c 100644 --- a/api/test/trace/propagation/detail/string_test.cc +++ b/api/test/trace/propagation/detail/string_test.cc @@ -25,7 +25,6 @@ struct SplitStringTestData // See https://github.com/google/googletest/issues/3805. friend void PrintTo(const SplitStringTestData &data, std::ostream *os) { - std::stringstream ss; *os << "(" << data.input << "," << data.separator << "," << data.max_count << "," << data.expected_number_strings << ")"; } @@ -50,8 +49,7 @@ class SplitStringTestFixture : public ::testing::TestWithParam fields{}; - fields.reserve(test_param.expected_number_strings); + std::vector fields(test_param.expected_number_strings); size_t got_splits_num = opentelemetry::trace::propagation::detail::SplitString( test_param.input, test_param.separator, fields.data(), test_param.max_count); From 8dbf8e5feecbc7e9bbbb53b28d183e47da020f80 Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Tue, 5 Nov 2024 21:16:14 +0000 Subject: [PATCH 27/32] Update stop instructions in README --- ext/test/w3c_tracecontext_test/README.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/ext/test/w3c_tracecontext_test/README.md b/ext/test/w3c_tracecontext_test/README.md index 8eda092f82..6f4db4513d 100644 --- a/ext/test/w3c_tracecontext_test/README.md +++ b/ext/test/w3c_tracecontext_test/README.md @@ -3,7 +3,7 @@ This test application is intended to be used as a test service for the [W3C Distributed Tracing Validation Service](https://github.com/w3c/trace-context/tree/master/test). It is -implemented according to [this +implemented according to [these instructions](https://github.com/w3c/trace-context/tree/master/test#implement-test-service). ## Usage @@ -47,4 +47,9 @@ docker run --network host w3c_driver http://localhost:31339/test 3: The validation service will run the test suite and print detailed test results. -4: Stop the test service by pressing enter. +4: Stop the test service by invoking `/stop`. Make sure to use the correct port number. + +```sh +# Assuming the service is currently running at port 30000 +curl http://localhost:30000/stop +``` From ebaaaf3f1e9811114bb58f1d742b8f1b01bfe45a Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Tue, 5 Nov 2024 21:21:28 +0000 Subject: [PATCH 28/32] Update CI step to run on ubuntu-latest --- .github/workflows/ci.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f5f49540e4..c415d5b01c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -926,7 +926,7 @@ jobs: w3c_trace_context_compliance_v1: name: W3C Distributed Tracing Validation V1 - runs-on: ubuntu-24.04 + runs-on: ubuntu-latest steps: - name: Checkout open-telemetry/opentelemetry-cpp uses: actions/checkout@v4 @@ -934,8 +934,8 @@ jobs: submodules: 'recursive' - name: setup env: - CC: /usr/bin/gcc-14 - CXX: /usr/bin/g++-14 + CC: /usr/bin/gcc-10 + CXX: /usr/bin/g++-10 PROTOBUF_VERSION: 21.12 run: | sudo -E ./ci/setup_googletest.sh From 35e6ec0186275ce644dfe6a0cd2d5eb86d50f5d0 Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Tue, 5 Nov 2024 21:50:27 +0000 Subject: [PATCH 29/32] Prevent repeated conversions of hex versions to binary --- .../trace/propagation/http_trace_context.h | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/api/include/opentelemetry/trace/propagation/http_trace_context.h b/api/include/opentelemetry/trace/propagation/http_trace_context.h index 608f93e9dc..e6a436d110 100644 --- a/api/include/opentelemetry/trace/propagation/http_trace_context.h +++ b/api/include/opentelemetry/trace/propagation/http_trace_context.h @@ -89,18 +89,14 @@ class HttpTraceContext : public context::propagation::TextMapPropagator static constexpr uint8_t kInvalidVersion = 0xFF; static constexpr uint8_t kDefaultAssumedVersion = 0x00; - static bool IsValidVersion(nostd::string_view version_hex) + static bool IsValidVersion(uint8_t version_binary) { - uint8_t version; - detail::HexToBinary(version_hex, &version, sizeof(version)); - return version != kInvalidVersion; + return version_binary != kInvalidVersion; } - static bool IsHigherVersion(nostd::string_view version_hex) + static bool IsHigherVersion(uint8_t version_binary) { - uint8_t version; - detail::HexToBinary(version_hex, &version, sizeof(version)); - return version > kDefaultAssumedVersion; + return version_binary > kDefaultAssumedVersion; } static void InjectImpl(context::propagation::TextMapCarrier &carrier, @@ -153,17 +149,20 @@ class HttpTraceContext : public context::propagation::TextMapPropagator return SpanContext::GetInvalid(); } - if (!IsValidVersion(version_hex)) + // hex is valid, convert it to binary + uint8_t version_binary; + detail::HexToBinary(version_hex, &version_binary, sizeof(version_binary)); + if (!IsValidVersion(version_binary)) { return SpanContext::GetInvalid(); } - if (IsHigherVersion(version_hex) && trace_parent.size() < kTraceParentSize) + if (IsHigherVersion(version_binary) && trace_parent.size() < kTraceParentSize) { return SpanContext::GetInvalid(); } - if (!IsHigherVersion(version_hex) && trace_parent.size() != kTraceParentSize) + if (!IsHigherVersion(version_binary) && trace_parent.size() != kTraceParentSize) { return SpanContext::GetInvalid(); } From bb5f91a028eeabab5e92f4353934c9d36e54507f Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Tue, 5 Nov 2024 22:03:48 +0000 Subject: [PATCH 30/32] Remove one-liner functions with inline code --- .../trace/propagation/http_trace_context.h | 31 +++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/api/include/opentelemetry/trace/propagation/http_trace_context.h b/api/include/opentelemetry/trace/propagation/http_trace_context.h index e6a436d110..81d4c308ec 100644 --- a/api/include/opentelemetry/trace/propagation/http_trace_context.h +++ b/api/include/opentelemetry/trace/propagation/http_trace_context.h @@ -89,16 +89,6 @@ class HttpTraceContext : public context::propagation::TextMapPropagator static constexpr uint8_t kInvalidVersion = 0xFF; static constexpr uint8_t kDefaultAssumedVersion = 0x00; - static bool IsValidVersion(uint8_t version_binary) - { - return version_binary != kInvalidVersion; - } - - static bool IsHigherVersion(uint8_t version_binary) - { - return version_binary > kDefaultAssumedVersion; - } - static void InjectImpl(context::propagation::TextMapCarrier &carrier, const SpanContext &span_context) { @@ -152,19 +142,28 @@ class HttpTraceContext : public context::propagation::TextMapPropagator // hex is valid, convert it to binary uint8_t version_binary; detail::HexToBinary(version_hex, &version_binary, sizeof(version_binary)); - if (!IsValidVersion(version_binary)) + if (version_binary == kInvalidVersion) { + // invalid version encountered return SpanContext::GetInvalid(); } - if (IsHigherVersion(version_binary) && trace_parent.size() < kTraceParentSize) + // See https://www.w3.org/TR/trace-context/#versioning-of-traceparent + if (version_binary > kDefaultAssumedVersion) { - return SpanContext::GetInvalid(); + // higher than default version detected + if (trace_parent.size() < kTraceParentSize) + { + return SpanContext::GetInvalid(); + } } - - if (!IsHigherVersion(version_binary) && trace_parent.size() != kTraceParentSize) + else { - return SpanContext::GetInvalid(); + // version is either lower or same as the default version + if (trace_parent.size() != kTraceParentSize) + { + return SpanContext::GetInvalid(); + } } TraceId trace_id = TraceIdFromHex(trace_id_hex); From 92f198a33dede3091d5bbe60677d6da359fecd03 Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Wed, 6 Nov 2024 15:42:57 +0000 Subject: [PATCH 31/32] Update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 76ea291ec7..650aebcea5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,7 @@ Increment the: ## [Unreleased] -* [TEST] Fix failing W3C Trace Context compliance tests [#3115](https://github.com/open-telemetry/opentelemetry-cpp/pull/3115) +* [API] Comply with W3C Trace Context [#3115](https://github.com/open-telemetry/opentelemetry-cpp/pull/3115) * Also adds CI check to ensure continued compliance * [API] Jaeger Propagator should not be deprecated From 894c042412f3fb88793742000337e4bb20ec8169 Mon Sep 17 00:00:00 2001 From: Pranav Sharma Date: Wed, 6 Nov 2024 15:47:10 +0000 Subject: [PATCH 32/32] Remove unused env variable --- .github/workflows/ci.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ffb5699ab1..26dda5449c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -938,7 +938,6 @@ jobs: env: CC: /usr/bin/gcc-10 CXX: /usr/bin/g++-10 - PROTOBUF_VERSION: 21.12 run: | sudo -E ./ci/setup_googletest.sh sudo -E ./ci/setup_ci_environment.sh