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.

Sign up for GitHub

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

tracing: Support 128bit trace id with zipkin tracer #3386

Merged
merged 9 commits into from
May 22, 2018
Merged

tracing: Support 128bit trace id with zipkin tracer #3386

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7b3d3ef
WIP: tracing: Support 128bit trace id with zipkin tracer
objectiser May 15, 2018
8d1d02a
Fix review comments
objectiser May 21, 2018
5c25180
Make 128 bit trace id usage configuration, defaulting to 64 bit to av…
objectiser May 21, 2018
66330c4
Fix docs
objectiser May 21, 2018
39e64ad
Fix schema, default value and add flag in trace.proto
objectiser May 21, 2018
92513fd
Revert v1 api docs change and schema validation
objectiser May 21, 2018
c574638
Add const and remove 'trace_id_128bit' field from json in tests
objectiser May 22, 2018
062d6c4
Move the 128bit trace id flag to Tracer constructor
objectiser May 22, 2018
926f968
Add @param for trace_id_128bit
objectiser May 22, 2018
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
1 change: 1 addition & 0 deletions source/extensions/tracers/zipkin/span_context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ namespace Zipkin {

SpanContext::SpanContext(const Span& span) {
trace_id_ = span.traceId();
trace_id_high_ = span.isSetTraceIdHigh() ? span.traceIdHigh() : 0;
id_ = span.id();
parent_id_ = span.isSetParentId() ? span.parentId() : 0;
sampled_ = span.sampled();
Expand Down
27 changes: 21 additions & 6 deletions source/extensions/tracers/zipkin/span_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,24 @@ class SpanContext {
/**
* Default constructor. Creates an empty context.
*/
SpanContext() : trace_id_(0), id_(0), parent_id_(0), is_initialized_(false), sampled_(false) {}
SpanContext()
: trace_id_(0), trace_id_high_(0), id_(0), parent_id_(0), is_initialized_(false),
sampled_(false) {}

/**
* Constructor that creates a context object from the supplied trace, span and
* parent ids, and sampled flag.
*
* @param trace_id The trace id.
* @param trace_id The low 64 bits of the trace id.
* @param trace_id_high The high 64 bits of the trace id.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: usually we put the high bits before the low bits when ordering

ex trace_id_high then trace_id

* @param id The span id.
* @param parent_id The parent id.
* @param sampled The sampled flag.
*/
SpanContext(const uint64_t trace_id, const uint64_t id, const uint64_t parent_id, bool sampled)
: trace_id_(trace_id), id_(id), parent_id_(parent_id), is_initialized_(true),
sampled_(sampled) {}
SpanContext(const uint64_t trace_id, const uint64_t trace_id_high, const uint64_t id,
const uint64_t parent_id, bool sampled)
: trace_id_(trace_id), trace_id_high_(trace_id_high), id_(id), parent_id_(parent_id),
is_initialized_(true), sampled_(sampled) {}

/**
* Constructor that creates a context object from the given Zipkin span object.
Expand All @@ -53,17 +57,28 @@ class SpanContext {
uint64_t parent_id() const { return parent_id_; }

/**
* @return the trace id as an integer.
* @return the low 64 bits of the trace id as an integer.
*/
uint64_t trace_id() const { return trace_id_; }

/**
* @return the high 64 bits of the trace id as an integer.
*/
uint64_t trace_id_high() const { return trace_id_high_; }

/**
* @return whether using 128 bit trace id.
*/
bool is128BitTraceId() const { return trace_id_high_ > 0; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you mean != 0

zero is an invalid set of high bits, negative is fine


/**
* @return the sampled flag.
*/
bool sampled() const { return sampled_; }

private:
uint64_t trace_id_;
uint64_t trace_id_high_;
uint64_t id_;
uint64_t parent_id_;
bool is_initialized_;
Expand Down
4 changes: 4 additions & 0 deletions source/extensions/tracers/zipkin/tracer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ SpanPtr Tracer::startSpan(const Tracing::Config& config, const std::string& span
uint64_t random_number = random_generator_.random();
span_ptr->setId(random_number);
span_ptr->setTraceId(random_number);
span_ptr->setTraceIdHigh(random_generator_.random());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this always uses 128-bit? Probably fine by now, but might break some folks. Usually, we make this optional setting. ex https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/Tracing.java#L270

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS this comment is about the implications of rollout. As-is, an old downstream service might break if they aren't prepared to consume 128-bit trace IDs. usually the behavior is to restart the trace, but not everything is consistent. That said, many libraries handle big IDs by now.

openzipkin/b3-propagation#6

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @mattklein123 on this one..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adriancole Originally wasn't sure whether to make it configurable, as seems like 128 is fairly standard now - but there is potential for it causing disruption in a mesh if using different versions of envoy. So I will look to make it configurable, with default of 64 bit - but atleast the newer version will be able to handle a 128bit id even if it doesn't use it for new trace instances by default.

int64_t start_time_micro =
std::chrono::duration_cast<std::chrono::microseconds>(
ProdMonotonicTimeSource::instance_.currentTime().time_since_epoch())
Expand Down Expand Up @@ -104,6 +105,9 @@ SpanPtr Tracer::startSpan(const Tracing::Config& config, const std::string& span

// Keep the same trace id
span_ptr->setTraceId(previous_context.trace_id());
if (previous_context.is128BitTraceId()) {
span_ptr->setTraceIdHigh(previous_context.trace_id_high());
}

// Keep the same sampled flag
span_ptr->setSampled(previous_context.sampled());
Expand Down
8 changes: 4 additions & 4 deletions source/extensions/tracers/zipkin/zipkin_core_types.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,9 @@ const std::string Span::EMPTY_HEX_STRING_ = "0000000000000000";

Span::Span(const Span& span) {
trace_id_ = span.traceId();
if (span.isSetTraceIdHigh()) {
trace_id_high_ = span.traceIdHigh();
}
name_ = span.name();
id_ = span.id();
if (span.isSetParentId()) {
Expand All @@ -158,9 +161,6 @@ Span::Span(const Span& span) {
if (span.isSetDuration()) {
duration_ = span.duration();
}
if (span.isSetTraceIdHigh()) {
trace_id_high_ = span.traceIdHigh();
}
monotonic_start_time_ = span.startTime();
tracer_ = span.tracer();
}
Expand All @@ -176,7 +176,7 @@ const std::string Span::toJson() {
rapidjson::Writer<rapidjson::StringBuffer> writer(s);
writer.StartObject();
writer.Key(ZipkinJsonFieldNames::get().SPAN_TRACE_ID.c_str());
writer.String(Hex::uint64ToHex(trace_id_).c_str());
writer.String(traceIdAsHexString().c_str());
writer.Key(ZipkinJsonFieldNames::get().SPAN_NAME.c_str());
writer.String(name_.c_str());
writer.Key(ZipkinJsonFieldNames::get().SPAN_ID.c_str());
Expand Down
5 changes: 4 additions & 1 deletion source/extensions/tracers/zipkin/zipkin_core_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,10 @@ class Span : public ZipkinBase {
/**
* @return the span's trace id as a hexadecimal string.
*/
const std::string traceIdAsHexString() const { return Hex::uint64ToHex(trace_id_); }
const std::string traceIdAsHexString() const {
return (trace_id_high_.has_value() ? Hex::uint64ToHex(trace_id_high_.value()) : "") +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know cpp, but guessing the conditional would be quicker made in a way that doesn't always cat values?

ex.

return trace_id_high_.has_value()
           ? Hex::uint64ToHex(trace_id_high_.value()) +  Hex::uint64ToHex(trace_id_)
           :  Hex::uint64ToHex(trace_id_);

Hex::uint64ToHex(trace_id_);
}

/**
* @return the higher 64 bits of a 128-bit trace id.
Expand Down
21 changes: 18 additions & 3 deletions source/extensions/tracers/zipkin/zipkin_tracer_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,16 +96,31 @@ Tracing::SpanPtr Driver::startSpan(const Tracing::Config& config, Http::HeaderMa

if (request_headers.XB3TraceId() && request_headers.XB3SpanId()) {
uint64_t trace_id(0);
uint64_t trace_id_high(0);
uint64_t span_id(0);
uint64_t parent_id(0);
if (!StringUtil::atoul(request_headers.XB3TraceId()->value().c_str(), trace_id, 16) ||
!StringUtil::atoul(request_headers.XB3SpanId()->value().c_str(), span_id, 16) ||

// Extract trace id - which can either be 128 or 64 bit. For 128 bit,
// it needs to be divided into two 64 bit numbers (high and low).
if (request_headers.XB3TraceId()->value().size() == 32) {
std::string tid = request_headers.XB3TraceId()->value().c_str();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: const for all these local vars

std::string high_tid = tid.substr(0, 16);
std::string low_tid = tid.substr(16, 16);
if (!StringUtil::atoul(high_tid.c_str(), trace_id_high, 16) ||
!StringUtil::atoul(low_tid.c_str(), trace_id, 16)) {
return Tracing::SpanPtr(new Tracing::NullSpan());
}
} else if (!StringUtil::atoul(request_headers.XB3TraceId()->value().c_str(), trace_id, 16)) {
return Tracing::SpanPtr(new Tracing::NullSpan());
}

if (!StringUtil::atoul(request_headers.XB3SpanId()->value().c_str(), span_id, 16) ||
(request_headers.XB3ParentSpanId() &&
!StringUtil::atoul(request_headers.XB3ParentSpanId()->value().c_str(), parent_id, 16))) {
return Tracing::SpanPtr(new Tracing::NullSpan());
}

SpanContext context(trace_id, span_id, parent_id, sampled);
SpanContext context(trace_id, trace_id_high, span_id, parent_id, sampled);

new_zipkin_span =
tracer.startSpan(config, request_headers.Host()->value().c_str(), start_time, context);
Expand Down
3 changes: 2 additions & 1 deletion test/extensions/tracers/zipkin/tracer_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,8 @@ TEST(ZipkinTracerTest, spanCreation) {

ON_CALL(config, operationName()).WillByDefault(Return(Tracing::OperationName::Ingress));
const uint generated_parent_id = Util::generateRandom64();
SpanContext modified_root_span_context(root_span_context.trace_id(), root_span_context.id(),
SpanContext modified_root_span_context(root_span_context.trace_id(),
root_span_context.trace_id_high(), root_span_context.id(),
generated_parent_id, root_span_context.sampled());
SpanPtr new_shared_context_span =
tracer.startSpan(config, "new_shared_context_span", timestamp, modified_root_span_context);
Expand Down
25 changes: 25 additions & 0 deletions test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,31 @@ TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3HeadersTest) {
EXPECT_TRUE(zipkin_span->span().sampled());
}

TEST_F(ZipkinDriverTest, ZipkinSpanContextFromB3Headers128TraceIdTest) {
setupValidDriver();

const std::string trace_id =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually test this.. below you aren't really testing this literally. For example, test that the high bits are in the right spot (so the input left hex characters are trace_id_high and right ones are trace_id)

Hex::uint64ToHex(Util::generateRandom64()) + Hex::uint64ToHex(Util::generateRandom64());
const std::string span_id = Hex::uint64ToHex(Util::generateRandom64());
const std::string parent_id = Hex::uint64ToHex(Util::generateRandom64());

request_headers_.insertXB3TraceId().value(trace_id);
request_headers_.insertXB3SpanId().value(span_id);
request_headers_.insertXB3ParentSpanId().value(parent_id);

// New span will have an SR annotation - so its span and parent ids will be
// the same as the supplied span context (i.e. shared context)
Tracing::SpanPtr span = driver_->startSpan(config_, request_headers_, operation_name_,
start_time_, {Tracing::Reason::Sampling, true});

ZipkinSpanPtr zipkin_span(dynamic_cast<ZipkinSpan*>(span.release()));

EXPECT_EQ(trace_id, zipkin_span->span().traceIdAsHexString());
EXPECT_EQ(span_id, zipkin_span->span().idAsHexString());
EXPECT_EQ(parent_id, zipkin_span->span().parentIdAsHexString());
EXPECT_TRUE(zipkin_span->span().sampled());
}

TEST_F(ZipkinDriverTest, ZipkinSpanContextFromInvalidTraceIdB3HeadersTest) {
setupValidDriver();

Expand Down