Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API] Comply with W3C Trace Context #3115

Merged
merged 34 commits into from
Nov 6, 2024
Merged
Changes from 29 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
111f234
fix test_traceparent_header_name_valid_casing
psx95 Oct 28, 2024
e73c58b
Fix test_tracestate_empty_header
psx95 Oct 28, 2024
c4816de
Fix test_traceparent_duplicated
psx95 Oct 29, 2024
3696baf
Fix test_tracestate_member_count_limit
psx95 Oct 29, 2024
b1d0623
Fix TraceContextTest.test_traceparent_version_* tests
psx95 Oct 30, 2024
b0732da
Add TrimString function
psx95 Oct 30, 2024
9fcef5b
Fix test_traceparent_ows_handling
psx95 Oct 30, 2024
cc31fbb
Fix the Trim function to include all whitespace characters
psx95 Oct 31, 2024
6374424
Replace the use of detail::TrimString with common::Trim
psx95 Oct 31, 2024
a756114
Avoid unnecessary style change
psx95 Oct 31, 2024
b856c55
Update string util test cases to include other whitespaces
psx95 Oct 31, 2024
66599ce
Fix cmake clang warnings
psx95 Oct 31, 2024
b460bd4
Add tests for SplitString
psx95 Oct 31, 2024
75aa431
Run format tool
psx95 Oct 31, 2024
0b0ef58
Fix valgrind issues with gtest
psx95 Oct 31, 2024
ffab2ee
Replace parameterized test with standard one
psx95 Oct 31, 2024
e7cfb9a
Move the test cases within test scope
psx95 Nov 1, 2024
06283f7
Remove vector from testcase struct
psx95 Nov 1, 2024
5d1d476
Add CI for trace context validation
psx95 Nov 1, 2024
22b8eb1
Stop server using a signal callback
psx95 Nov 1, 2024
175df2e
Remove debugging statements from CI
psx95 Nov 1, 2024
e2c2805
Specify SPEC_LEVEL and test suites to run
psx95 Nov 1, 2024
444abe6
Update Changelog
psx95 Nov 1, 2024
975ac71
Remove extraneous thread and busy loop
psx95 Nov 4, 2024
a90c053
Move static function outside class
psx95 Nov 4, 2024
1785610
Update test
psx95 Nov 5, 2024
a4c121a
Merge branch 'main' into w3c-trace-context
psx95 Nov 5, 2024
8dbf8e5
Update stop instructions in README
psx95 Nov 5, 2024
ebaaaf3
Update CI step to run on ubuntu-latest
psx95 Nov 5, 2024
35e6ec0
Prevent repeated conversions of hex versions to binary
psx95 Nov 5, 2024
bb5f91a
Remove one-liner functions with inline code
psx95 Nov 5, 2024
73a9b86
Merge branch 'main' into w3c-trace-context
marcalff Nov 6, 2024
92f198a
Update changelog
psx95 Nov 6, 2024
894c042
Remove unused env variable
psx95 Nov 6, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
@@ -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-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
PROTOBUF_VERSION: 21.12
psx95 marked this conversation as resolved.
Show resolved Hide resolved
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 &
psx95 marked this conversation as resolved.
Show resolved Hide resolved
- 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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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)
psx95 marked this conversation as resolved.
Show resolved Hide resolved
* 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)

4 changes: 2 additions & 2 deletions api/include/opentelemetry/common/string_util.h
Original file line number Diff line number Diff line change
@@ -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--;
}
Original file line number Diff line number Diff line change
@@ -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();
}
psx95 marked this conversation as resolved.
Show resolved Hide resolved

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 == "")
{
3 changes: 2 additions & 1 deletion api/include/opentelemetry/trace/trace_state.h
Original file line number Diff line number Diff line change
@@ -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));
8 changes: 7 additions & 1 deletion api/test/common/string_util_test.cc
Original file line number Diff line number Diff line change
@@ -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);
16 changes: 16 additions & 0 deletions api/test/trace/propagation/detail/BUILD
Original file line number Diff line number Diff line change
@@ -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",
],
)
2 changes: 1 addition & 1 deletion api/test/trace/propagation/detail/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -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)
62 changes: 62 additions & 0 deletions api/test/trace/propagation/detail/string_test.cc
Original file line number Diff line number Diff line change
@@ -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));
10 changes: 10 additions & 0 deletions ci/do_ci.sh
Original file line number Diff line number Diff line change
@@ -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"
ThomsonTan marked this conversation as resolved.
Show resolved Hide resolved
exit 0
elif [[ "$1" == "cmake.do_not_install.test" ]]; then
cd "${BUILD_DIR}"
rm -rf *
10 changes: 9 additions & 1 deletion ext/include/opentelemetry/ext/http/server/http_server.h
Original file line number Diff line number Diff line change
@@ -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++;
9 changes: 7 additions & 2 deletions ext/test/w3c_tracecontext_test/README.md
Original file line number Diff line number Diff line change
@@ -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
```
Loading