Merge pull request #85 from open-telemetry/main

[API] Comply with W3C Trace Context (open-telemetry#3115)

.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

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)
 

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,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<nostd::string_view, 4> 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 == "")
     {

api/include/opentelemetry/trace/trace_state.h

@@ -59,7 +59,8 @@ class 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

@@ -20,3 +20,19 @@ otel_cc_test(
         "@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/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
+```