diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0cf9a5771f..26dda5449c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -925,3 +925,42 @@ 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-latest + steps: + - name: Checkout open-telemetry/opentelemetry-cpp + uses: actions/checkout@v4 + with: + submodules: 'recursive' + - name: setup + env: + CC: /usr/bin/gcc-10 + CXX: /usr/bin/g++-10 + 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 + env: + SPEC_LEVEL: 1 + run: + | + python ${GITHUB_WORKSPACE}/trace-context/test/test.py http://localhost:30000/test TraceContextTest AdvancedTest + curl http://localhost:30000/stop diff --git a/CHANGELOG.md b/CHANGELOG.md index 540479eb97..650aebcea5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,9 @@ Increment the: ## [Unreleased] +* [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 [#3086](https://github.com/open-telemetry/opentelemetry-cpp/pull/3086) 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--; } diff --git a/api/include/opentelemetry/trace/propagation/http_trace_context.h b/api/include/opentelemetry/trace/propagation/http_trace_context.h index a6b7e3b219..81d4c308ec 100644 --- a/api/include/opentelemetry/trace/propagation/http_trace_context.h +++ b/api/include/opentelemetry/trace/propagation/http_trace_context.h @@ -86,14 +86,8 @@ class HttpTraceContext : public context::propagation::TextMapPropagator } private: - static constexpr uint8_t kInvalidVersion = 0xFF; - - static bool IsValidVersion(nostd::string_view version_hex) - { - uint8_t version; - detail::HexToBinary(version_hex, &version, sizeof(version)); - return version != kInvalidVersion; - } + static constexpr uint8_t kInvalidVersion = 0xFF; + static constexpr uint8_t kDefaultAssumedVersion = 0x00; static void InjectImpl(context::propagation::TextMapCarrier &carrier, const SpanContext &span_context) @@ -122,11 +116,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) { @@ -150,11 +139,33 @@ 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 (version_binary == kInvalidVersion) { + // invalid version encountered return SpanContext::GetInvalid(); } + // See https://www.w3.org/TR/trace-context/#versioning-of-traceparent + if (version_binary > kDefaultAssumedVersion) + { + // higher than default version detected + if (trace_parent.size() < kTraceParentSize) + { + return SpanContext::GetInvalid(); + } + } + else + { + // 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); SpanId span_id = SpanIdFromHex(span_id_hex); @@ -169,7 +180,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 = common::StringUtil::Trim(carrier.Get(kTraceParent)); nostd::string_view trace_state = carrier.Get(kTraceState); if (trace_parent == "") { 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)); 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); 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..67a48a354c --- /dev/null +++ b/api/test/trace/propagation/detail/string_test.cc @@ -0,0 +1,62 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#include +#include +#include + +#include "opentelemetry/nostd/string_view.h" + +using namespace opentelemetry; + +namespace +{ + +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) + { + *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_P(SplitStringTestFixture, SplitsAsExpected) +{ + const SplitStringTestData test_param = GetParam(); + 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); + + // 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)); diff --git a/ci/do_ci.sh b/ci/do_ci.sh index c31d5e887a..11dbe4ef80 100755 --- a/ci/do_ci.sh +++ b/ci/do_ci.sh @@ -393,6 +393,16 @@ 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" + exit 0 elif [[ "$1" == "cmake.do_not_install.test" ]]; then cd "${BUILD_DIR}" rm -rf * diff --git a/ext/include/opentelemetry/ext/http/server/http_server.h b/ext/include/opentelemetry/ext/http/server/http_server.h index 5ea7debc43..21efac94e0 100644 --- a/ext/include/opentelemetry/ext/http/server/http_server.h +++ b/ext/include/opentelemetry/ext/http/server/http_server.h @@ -648,7 +648,15 @@ class HttpServer : private SocketTools::Reactor::SocketCallback { ptr++; } - conn.request.headers[name] = std::string(begin, ptr); + 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] = std::string(begin, ptr); + } if (*ptr == '\r') { ptr++; 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 +``` diff --git a/ext/test/w3c_tracecontext_test/main.cc b/ext/test/w3c_tracecontext_test/main.cc index fed07a2398..d2ed772510 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" @@ -45,16 +40,34 @@ 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: 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 ""; } @@ -161,6 +174,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) @@ -207,16 +221,28 @@ 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 + // Wait for signal to stop server + 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(); }