[TEST] Fix failing W3C Trace Context compliance tests

.github/workflows/ci.yml

@@ -923,3 +923,43 @@ 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
+        env:
+          SPEC_LEVEL: 1
+        run:
+          |
+          python ${GITHUB_WORKSPACE}/trace-context/test/test.py http://localhost:30000/test TraceContextTest AdvancedTest
+          curl http://localhost:30000/stop

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)
 

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<std::size_t>(left)] == ' ')
+    while (left <= right && isspace(str[left]))
     {
       left++;
     }
-    while (left <= right && str[static_cast<std::size_t>(right)] == ' ')
+    while (left <= right && isspace(str[right]))
     {
       right--;
     }

api/include/opentelemetry/trace/propagation/http_trace_context.h

@@ -86,7 +86,8 @@ 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)
   {
@@ -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<nostd::string_view, 4> 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);
 
@@ -169,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 = common::StringUtil::Trim(carrier.Get(kTraceParent));
     nostd::string_view trace_state  = carrier.Get(kTraceState);
     if (trace_parent == "")
     {

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<TraceState> ts(new TraceState(cnt));

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);

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",
+    ],
+)

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)

api/test/trace/propagation/detail/string_test.cc

@@ -0,0 +1,62 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+#include <gtest/gtest.h>
+#include <opentelemetry/trace/propagation/b3_propagator.h>
+#include <string>
+
+#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<SplitStringTestData>
+{};
+
+TEST_P(SplitStringTestFixture, SplitsAsExpected)
+{
+  const SplitStringTestData test_param = GetParam();
+  std::vector<opentelemetry::nostd::string_view> 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));

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 *

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++;

ext/test/w3c_tracecontext_test/main.cc

@@ -3,28 +3,23 @@
 
 #include <stdint.h>
 #include <stdlib.h>
-#include <algorithm>
-#include <initializer_list>
 #include <iostream>
 #include <map>
 #include <nlohmann/json.hpp>
 #include <string>
 #include <utility>
 #include <vector>
 
-#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<std::string, std::string> &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();
 }