Skip to content

Commit be1f43c

Browse files
psx95psx95
authored
[API] Comply with W3C Trace Context (open-telemetry#3115)
1 parent 6fdf797 commit be1f43c

File tree

13 files changed

+223
-35
lines changed

13 files changed

+223
-35
lines changed

‎.github/workflows/ci.yml‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -925,3 +925,42 @@ jobs:
925925
- name: run ./ci/docfx.cmd
926926
shell: cmd
927927
run: ./ci/docfx.cmd
928+
929+
w3c_trace_context_compliance_v1:
930+
name: W3C Distributed Tracing Validation V1
931+
runs-on: ubuntu-latest
932+
steps:
933+
- name: Checkout open-telemetry/opentelemetry-cpp
934+
uses: actions/checkout@v4
935+
with:
936+
submodules: 'recursive'
937+
- name: setup
938+
env:
939+
CC: /usr/bin/gcc-10
940+
CXX: /usr/bin/g++-10
941+
run: |
942+
sudo -E ./ci/setup_googletest.sh
943+
sudo -E ./ci/setup_ci_environment.sh
944+
- name: run w3c trace-context test server (background)
945+
env:
946+
CXX_STANDARD: '14'
947+
run: |
948+
./ci/do_ci.sh cmake.w3c.trace-context.build-server
949+
cd $HOME/build/ext/test/w3c_tracecontext_test
950+
./w3c_tracecontext_test &
951+
- name: Checkout w3c/trace-context repo
952+
uses: actions/checkout@v4
953+
with:
954+
repository: w3c/trace-context
955+
path: trace-context
956+
- name: install dependencies
957+
run: |
958+
sudo apt update && sudo apt install python3-pip
959+
sudo pip3 install aiohttp
960+
- name: run w3c trace-context test suite
961+
env:
962+
SPEC_LEVEL: 1
963+
run:
964+
|
965+
python ${GITHUB_WORKSPACE}/trace-context/test/test.py http://localhost:30000/test TraceContextTest AdvancedTest
966+
curl http://localhost:30000/stop

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ Increment the:
1515

1616
## [Unreleased]
1717

18+
* [API] Comply with W3C Trace Context [#3115](https://github.com/open-telemetry/opentelemetry-cpp/pull/3115)
19+
* Also adds CI check to ensure continued compliance
20+
1821
* [API] Jaeger Propagator should not be deprecated
1922
[#3086](https://github.com/open-telemetry/opentelemetry-cpp/pull/3086)
2023

‎api/include/opentelemetry/common/string_util.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,11 @@ class StringUtil
1515
public:
1616
static nostd::string_view Trim(nostd::string_view str, size_t left, size_t right) noexcept
1717
{
18-
while (left <= right && str[static_cast<std::size_t>(left)] == ' ')
18+
while (left <= right && isspace(str[left]))
1919
{
2020
left++;
2121
}
22-
while (left <= right && str[static_cast<std::size_t>(right)] == ' ')
22+
while (left <= right && isspace(str[right]))
2323
{
2424
right--;
2525
}

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

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -86,14 +86,8 @@ class HttpTraceContext : public context::propagation::TextMapPropagator
8686
}
8787

8888
private:
89-
static constexpr uint8_t kInvalidVersion = 0xFF;
90-
91-
static bool IsValidVersion(nostd::string_view version_hex)
92-
{
93-
uint8_t version;
94-
detail::HexToBinary(version_hex, &version, sizeof(version));
95-
return version != kInvalidVersion;
96-
}
89+
static constexpr uint8_t kInvalidVersion = 0xFF;
90+
static constexpr uint8_t kDefaultAssumedVersion = 0x00;
9791

9892
static void InjectImpl(context::propagation::TextMapCarrier &carrier,
9993
const SpanContext &span_context)
@@ -122,11 +116,6 @@ class HttpTraceContext : public context::propagation::TextMapPropagator
122116
static SpanContext ExtractContextFromTraceHeaders(nostd::string_view trace_parent,
123117
nostd::string_view trace_state)
124118
{
125-
if (trace_parent.size() != kTraceParentSize)
126-
{
127-
return SpanContext::GetInvalid();
128-
}
129-
130119
std::array<nostd::string_view, 4> fields{};
131120
if (detail::SplitString(trace_parent, '-', fields.data(), 4) != 4)
132121
{
@@ -150,11 +139,33 @@ class HttpTraceContext : public context::propagation::TextMapPropagator
150139
return SpanContext::GetInvalid();
151140
}
152141

153-
if (!IsValidVersion(version_hex))
142+
// hex is valid, convert it to binary
143+
uint8_t version_binary;
144+
detail::HexToBinary(version_hex, &version_binary, sizeof(version_binary));
145+
if (version_binary == kInvalidVersion)
154146
{
147+
// invalid version encountered
155148
return SpanContext::GetInvalid();
156149
}
157150

151+
// See https://www.w3.org/TR/trace-context/#versioning-of-traceparent
152+
if (version_binary > kDefaultAssumedVersion)
153+
{
154+
// higher than default version detected
155+
if (trace_parent.size() < kTraceParentSize)
156+
{
157+
return SpanContext::GetInvalid();
158+
}
159+
}
160+
else
161+
{
162+
// version is either lower or same as the default version
163+
if (trace_parent.size() != kTraceParentSize)
164+
{
165+
return SpanContext::GetInvalid();
166+
}
167+
}
168+
158169
TraceId trace_id = TraceIdFromHex(trace_id_hex);
159170
SpanId span_id = SpanIdFromHex(span_id_hex);
160171

@@ -169,7 +180,8 @@ class HttpTraceContext : public context::propagation::TextMapPropagator
169180

170181
static SpanContext ExtractImpl(const context::propagation::TextMapCarrier &carrier)
171182
{
172-
nostd::string_view trace_parent = carrier.Get(kTraceParent);
183+
// Get trace_parent after trimming the leading and trailing whitespaces
184+
nostd::string_view trace_parent = common::StringUtil::Trim(carrier.Get(kTraceParent));
173185
nostd::string_view trace_state = carrier.Get(kTraceState);
174186
if (trace_parent == "")
175187
{

‎api/include/opentelemetry/trace/trace_state.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,8 @@ class OPENTELEMETRY_EXPORT TraceState
5959
size_t cnt = kv_str_tokenizer.NumTokens(); // upper bound on number of kv pairs
6060
if (cnt > kMaxKeyValuePairs)
6161
{
62-
cnt = kMaxKeyValuePairs;
62+
// trace state should be discarded if count exceeds
63+
return GetDefault();
6364
}
6465

6566
nostd::shared_ptr<TraceState> ts(new TraceState(cnt));

‎api/test/common/string_util_test.cc‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,15 @@ TEST(StringUtilTest, TrimString)
3535
{"k1=v1,k2=v2, k3=v3", "k1=v1,k2=v2, k3=v3"},
3636
{" k1=v1", "k1=v1"},
3737
{"k1=v1 ", "k1=v1"},
38+
{"k1=v1\t", "k1=v1"},
39+
{"\t k1=v1 \t", "k1=v1"},
40+
{"\t\t k1=v1\t ", "k1=v1"},
41+
{"\t\t k1=v1\t ,k2=v2", "k1=v1\t ,k2=v2"},
3842
{" k1=v1 ", "k1=v1"},
3943
{" ", ""},
40-
{"", ""}};
44+
{"", ""},
45+
{"\n_some string_\t", "_some string_"}};
46+
4147
for (auto &testcase : testcases)
4248
{
4349
EXPECT_EQ(StringUtil::Trim(testcase.input), testcase.expected);

‎api/test/trace/propagation/detail/BUILD‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,3 +18,19 @@ cc_test(
1818
"@com_google_googletest//:gtest_main",
1919
],
2020
)
21+
22+
cc_test(
23+
name = "string_test",
24+
srcs = [
25+
"string_test.cc",
26+
],
27+
tags = [
28+
"api",
29+
"test",
30+
"trace",
31+
],
32+
deps = [
33+
"//api",
34+
"@com_google_googletest//:gtest_main",
35+
],
36+
)

‎api/test/trace/propagation/detail/CMakeLists.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
# Copyright The OpenTelemetry Authors
22
# SPDX-License-Identifier: Apache-2.0
33

4-
foreach(testname hex_test)
4+
foreach(testname hex_test string_test)
55
add_executable(${testname} "${testname}.cc")
66
target_link_libraries(${testname} ${GTEST_BOTH_LIBRARIES}
77
${CMAKE_THREAD_LIBS_INIT} opentelemetry_api)

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

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
// Copyright The OpenTelemetry Authors
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
#include <gtest/gtest.h>
5+
#include <opentelemetry/trace/propagation/b3_propagator.h>
6+
#include <string>
7+
8+
#include "opentelemetry/nostd/string_view.h"
9+
10+
using namespace opentelemetry;
11+
12+
namespace
13+
{
14+
15+
struct SplitStringTestData
16+
{
17+
opentelemetry::nostd::string_view input;
18+
char separator;
19+
size_t max_count;
20+
size_t expected_number_strings;
21+
22+
// When googletest registers parameterized tests, it uses this method to format the parameters.
23+
// The default implementation prints hex dump of all bytes in the object. If there is any padding
24+
// in these bytes, valgrind reports this as a warning - "Use of uninitialized bytes".
25+
// See https://github.com/google/googletest/issues/3805.
26+
friend void PrintTo(const SplitStringTestData &data, std::ostream *os)
27+
{
28+
*os << "(" << data.input << "," << data.separator << "," << data.max_count << ","
29+
<< data.expected_number_strings << ")";
30+
}
31+
};
32+
33+
const SplitStringTestData split_string_test_cases[] = {
34+
{"foo,bar,baz", ',', 4, 3},
35+
{"foo,bar,baz,foobar", ',', 4, 4},
36+
{"foo,bar,baz,foobar", '.', 4, 1},
37+
{"foo,bar,baz,", ',', 4, 4},
38+
{"foo,bar,baz,", ',', 2, 2},
39+
{"foo ,bar, baz ", ',', 4, 3},
40+
{"foo ,bar, baz ", ',', 4, 3},
41+
{"00-0af7651916cd43dd8448eb211c80319c-00f067aa0ba902b7-01", '-', 4, 4},
42+
};
43+
} // namespace
44+
45+
// Test fixture
46+
class SplitStringTestFixture : public ::testing::TestWithParam<SplitStringTestData>
47+
{};
48+
49+
TEST_P(SplitStringTestFixture, SplitsAsExpected)
50+
{
51+
const SplitStringTestData test_param = GetParam();
52+
std::vector<opentelemetry::nostd::string_view> fields(test_param.expected_number_strings);
53+
size_t got_splits_num = opentelemetry::trace::propagation::detail::SplitString(
54+
test_param.input, test_param.separator, fields.data(), test_param.max_count);
55+
56+
// Assert on the output
57+
EXPECT_EQ(got_splits_num, test_param.expected_number_strings);
58+
}
59+
60+
INSTANTIATE_TEST_SUITE_P(SplitStringTestCases,
61+
SplitStringTestFixture,
62+
::testing::ValuesIn(split_string_test_cases));

‎ci/do_ci.sh‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -393,6 +393,16 @@ elif [[ "$1" == "cmake.exporter.otprotocol.with_async_export.test" ]]; then
393393
make -j $(nproc)
394394
cd exporters/otlp && make test
395395
exit 0
396+
elif [[ "$1" == "cmake.w3c.trace-context.build-server" ]]; then
397+
echo "Building w3c trace context test server"
398+
cd "${BUILD_DIR}"
399+
rm -rf *
400+
cmake "${CMAKE_OPTIONS[@]}" \
401+
-DBUILD_W3CTRACECONTEXT_TEST=ON \
402+
-DCMAKE_CXX_STANDARD=${CXX_STANDARD} \
403+
"${SRC_DIR}"
404+
eval "$MAKE_COMMAND"
405+
exit 0
396406
elif [[ "$1" == "cmake.do_not_install.test" ]]; then
397407
cd "${BUILD_DIR}"
398408
rm -rf *

0 commit comments

Comments
 (0)