Skip to content

Commit 858f391

Browse files
Fix code review findings: trace_flags default, header comment
- Default trace_flags to 0x00 (not-sampled) per W3C Trace Context spec when the field is absent, instead of defaulting to kIsSampled - Remove misleading "HTTP headers" from header doc comment - Apply clang-format adjustments Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 01f7e62 commit 858f391

File tree

2 files changed

+36
-33
lines changed

2 files changed

+36
-33
lines changed

include/xrpl/telemetry/TraceContextPropagator.h

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,9 @@
11
#pragma once
22

3-
/** Utilities for trace context propagation across nodes and HTTP headers.
3+
/** Utilities for trace context propagation across nodes.
44
5-
Provides serialization/deserialization of OTel trace context to/from:
6-
- Protocol Buffer TraceContext messages (P2P cross-node propagation)
7-
- HTTP headers (W3C Trace Context for RPC)
5+
Provides serialization/deserialization of OTel trace context to/from
6+
Protocol Buffer TraceContext messages (P2P cross-node propagation).
87
98
Only compiled when XRPL_ENABLE_TELEMETRY is defined.
109
*/
@@ -46,9 +45,11 @@ extractFromProtobuf(protocol::TraceContext const& proto)
4645
auto const* rawSpanId = reinterpret_cast<std::uint8_t const*>(proto.span_id().data());
4746
trace::TraceId traceId(opentelemetry::nostd::span<std::uint8_t const, 16>(rawTraceId, 16));
4847
trace::SpanId spanId(opentelemetry::nostd::span<std::uint8_t const, 8>(rawSpanId, 8));
48+
// Default to not-sampled (0x00) per W3C Trace Context spec when
49+
// the trace_flags field is absent.
4950
trace::TraceFlags flags(
5051
proto.has_trace_flags() ? static_cast<std::uint8_t>(proto.trace_flags())
51-
: trace::TraceFlags::kIsSampled);
52+
: static_cast<std::uint8_t>(0));
5253

5354
trace::SpanContext spanCtx(traceId, spanId, flags, /* remote = */ true);
5455

src/tests/libxrpl/telemetry/TraceContextPropagator.cpp

Lines changed: 30 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,11 @@
55
#include <xrpl/telemetry/TraceContextPropagator.h>
66

77
#include <opentelemetry/context/context.h>
8+
#include <opentelemetry/nostd/span.h>
9+
#include <opentelemetry/trace/context.h>
810
#include <opentelemetry/trace/default_span.h>
911
#include <opentelemetry/trace/span_context.h>
1012
#include <opentelemetry/trace/trace_flags.h>
11-
#include <opentelemetry/nostd/span.h>
12-
#include <opentelemetry/trace/context.h>
1313
#include <opentelemetry/trace/trace_id.h>
1414

1515
#include <cstring>
@@ -19,22 +19,32 @@ namespace trace = opentelemetry::trace;
1919
TEST(TraceContextPropagator, round_trip)
2020
{
2121
std::uint8_t traceIdBuf[16] = {
22-
0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
23-
0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10};
24-
std::uint8_t spanIdBuf[8] = {
25-
0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff, 0x11, 0x22};
26-
27-
trace::TraceId traceId(
28-
opentelemetry::nostd::span<const uint8_t, 16>(traceIdBuf, 16));
29-
trace::SpanId spanId(
30-
opentelemetry::nostd::span<const uint8_t, 8>(spanIdBuf, 8));
22+
0x01,
23+
0x02,
24+
0x03,
25+
0x04,
26+
0x05,
27+
0x06,
28+
0x07,
29+
0x08,
30+
0x09,
31+
0x0a,
32+
0x0b,
33+
0x0c,
34+
0x0d,
35+
0x0e,
36+
0x0f,
37+
0x10};
38+
std::uint8_t spanIdBuf[8] = {0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff, 0x11, 0x22};
39+
40+
trace::TraceId traceId(opentelemetry::nostd::span<uint8_t const, 16>(traceIdBuf, 16));
41+
trace::SpanId spanId(opentelemetry::nostd::span<uint8_t const, 8>(spanIdBuf, 8));
3142
trace::TraceFlags flags(trace::TraceFlags::kIsSampled);
3243
trace::SpanContext spanCtx(traceId, spanId, flags, true);
3344

3445
auto ctx = opentelemetry::context::Context{}.SetValue(
3546
trace::kSpanKey,
36-
opentelemetry::nostd::shared_ptr<trace::Span>(
37-
new trace::DefaultSpan(spanCtx)));
47+
opentelemetry::nostd::shared_ptr<trace::Span>(new trace::DefaultSpan(spanCtx)));
3848

3949
protocol::TraceContext proto;
4050
xrpl::telemetry::injectToProtobuf(ctx, proto);
@@ -43,14 +53,11 @@ TEST(TraceContextPropagator, round_trip)
4353
EXPECT_EQ(proto.trace_id().size(), 16u);
4454
EXPECT_TRUE(proto.has_span_id());
4555
EXPECT_EQ(proto.span_id().size(), 8u);
46-
EXPECT_EQ(
47-
proto.trace_flags(),
48-
static_cast<uint32_t>(trace::TraceFlags::kIsSampled));
56+
EXPECT_EQ(proto.trace_flags(), static_cast<uint32_t>(trace::TraceFlags::kIsSampled));
4957
EXPECT_EQ(std::memcmp(proto.trace_id().data(), traceIdBuf, 16), 0);
5058
EXPECT_EQ(std::memcmp(proto.span_id().data(), spanIdBuf, 8), 0);
5159

52-
auto extractedCtx =
53-
xrpl::telemetry::extractFromProtobuf(proto);
60+
auto extractedCtx = xrpl::telemetry::extractFromProtobuf(proto);
5461
auto extractedSpan = trace::GetSpan(extractedCtx);
5562
ASSERT_NE(extractedSpan, nullptr);
5663

@@ -59,8 +66,7 @@ TEST(TraceContextPropagator, round_trip)
5966
EXPECT_TRUE(extracted.IsRemote());
6067
EXPECT_EQ(extracted.trace_id(), traceId);
6168
EXPECT_EQ(extracted.span_id(), spanId);
62-
EXPECT_TRUE(
63-
extracted.trace_flags().IsSampled());
69+
EXPECT_TRUE(extracted.trace_flags().IsSampled());
6470
}
6571

6672
TEST(TraceContextPropagator, extract_empty_protobuf)
@@ -114,24 +120,20 @@ TEST(TraceContextPropagator, inject_invalid_span)
114120

115121
TEST(TraceContextPropagator, flags_preservation)
116122
{
117-
std::uint8_t traceIdBuf[16] = {
118-
1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16};
123+
std::uint8_t traceIdBuf[16] = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16};
119124
std::uint8_t spanIdBuf[8] = {1, 2, 3, 4, 5, 6, 7, 8};
120125

121126
// Test with flags NOT sampled (flags = 0)
122127
trace::TraceFlags flags(0);
123128
trace::SpanContext spanCtx(
124-
trace::TraceId(
125-
opentelemetry::nostd::span<const uint8_t, 16>(traceIdBuf, 16)),
126-
trace::SpanId(
127-
opentelemetry::nostd::span<const uint8_t, 8>(spanIdBuf, 8)),
129+
trace::TraceId(opentelemetry::nostd::span<uint8_t const, 16>(traceIdBuf, 16)),
130+
trace::SpanId(opentelemetry::nostd::span<uint8_t const, 8>(spanIdBuf, 8)),
128131
flags,
129132
true);
130133

131134
auto ctx = opentelemetry::context::Context{}.SetValue(
132135
trace::kSpanKey,
133-
opentelemetry::nostd::shared_ptr<trace::Span>(
134-
new trace::DefaultSpan(spanCtx)));
136+
opentelemetry::nostd::shared_ptr<trace::Span>(new trace::DefaultSpan(spanCtx)));
135137

136138
protocol::TraceContext proto;
137139
xrpl::telemetry::injectToProtobuf(ctx, proto);

0 commit comments

Comments
 (0)