Skip to content

Commit

Permalink
wip: add sampling. current limits include...
Browse files Browse the repository at this point in the history
* hardcoded sampling rate of 50%
* sampling decision should only be performed at root
  or honour parent span decision
* allow more than probabilistic strategy for sampling
* pretty sure i've not understood the model, code tidying to come
  • Loading branch information
pingles committed May 16, 2018
1 parent 4077589 commit 67d68c6
Show file tree
Hide file tree
Showing 8 changed files with 45 additions and 4 deletions.
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,5 @@
*.app

bazel-*

.build/
2 changes: 2 additions & 0 deletions zipkin/include/zipkin/span_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ class SpanContext {
: trace_id_{trace_id}, id_{id}, parent_id_{parent_id}, flags_{flags},
is_initialized_{true} {}

bool isSampled() const { return flags_ & zipkin::sampled_flag; }

/**
* @return the span id as an integer
*/
Expand Down
4 changes: 4 additions & 0 deletions zipkin/include/zipkin/zipkin_core_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,9 @@ class Span : public ZipkinBase {
Span()
: trace_id_(0), name_(), id_(0), debug_(false), monotonic_start_time_(0),
tracer_(nullptr) {}

void setSampled(const bool val) { sampled_ = val; }
bool isSampled() const { return sampled_; }

/**
* Sets the span's trace id attribute.
Expand Down Expand Up @@ -566,6 +569,7 @@ class Span : public ZipkinBase {
std::string name_;
uint64_t id_;
Optional<TraceId> parent_id_;
bool sampled_;
bool debug_;
std::vector<Annotation> annotations_;
std::vector<BinaryAnnotation> binary_annotations_;
Expand Down
5 changes: 5 additions & 0 deletions zipkin/src/span_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ SpanContext::SpanContext(const Span &span) {
trace_id_ = span.traceId();
id_ = span.id();
parent_id_ = span.isSetParentId() ? span.parentId() : 0;

if (span.isSampled()) {
flags_ |= static_cast<unsigned char>(zipkin::sampled_flag);
flags_ |= static_cast<unsigned char>(zipkin::sampling_set_flag);
}

for (const Annotation &annotation : span.annotations()) {
if (annotation.value() == ZipkinCoreConstants::get().CLIENT_RECV) {
Expand Down
1 change: 1 addition & 0 deletions zipkin_opentracing/include/zipkin/opentracing.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ struct ZipkinOtTracerOptions {
uint32_t collector_port = 9411;
SteadyClock::duration reporting_period = DEFAULT_REPORTING_PERIOD;
size_t max_buffered_spans = DEFAULT_SPAN_BUFFER_SIZE;
float sample_rate = 1.0f;

This comment has been minimized.

Copy link
@rnburn

rnburn May 16, 2018

Could you use double for sample rate instead of float. It's more common and otherwise something like options.sample_rate = .7 will trigger a warning because of loss of precision.


std::string service_name;
IpAddress service_address;
Expand Down
23 changes: 21 additions & 2 deletions zipkin_opentracing/src/opentracing.cc
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
#include <zipkin/opentracing.h>
#include <opentracing/util.h>

#include "propagation.h"
#include "utility.h"
#include <atomic>
#include <cstring>
#include <mutex>
#include <unordered_map>
#include <random>
#include <zipkin/tracer.h>
#include <zipkin/utility.h>
#include <zipkin/zipkin_core_types.h>
Expand Down Expand Up @@ -156,7 +158,7 @@ class OtSpan : public ot::Span {
parent_span_context->baggage_mutex_};
auto baggage = parent_span_context->baggage_;
span_context_ =
OtSpanContext{zipkin::SpanContext{*span_}, std::move(baggage)};
OtSpanContext{zipkin::SpanContext{*span_}, std::move(baggage)};
} else {
span_context_ = OtSpanContext{zipkin::SpanContext{*span_}};
}
Expand Down Expand Up @@ -277,11 +279,28 @@ class OtTracer : public ot::Tracer,
StartSpanWithOptions(string_view operation_name,
const ot::StartSpanOptions &options) const
noexcept override {

// Create the core zipkin span.
SpanPtr span{new zipkin::Span{}};
span->setName(operation_name);
span->setTracer(tracer_.get());

// TODO
// * sampling rate should be arg-based
// * sampling should probably be extracted into a sampler to allow
// different strategies
// * we should be guarding this to set sampling only when its a
// root span
auto value = RandomUtil::generateId();
auto max = std::numeric_limits<uint64_t>::max();
long double sampling_rate = 0.5;
auto boundary = sampling_rate * max; // be false 50% of the time
auto samplingBoundary = static_cast<uint64_t>(boundary);

if (value > samplingBoundary) {
span->setSampled(true);
}

This comment has been minimized.

Copy link
@rnburn

rnburn May 16, 2018

As an optimization, Jaeger was doing some trickery to avoid having to generate an extra random number. But I think it would be more straightforward (and safer: jaegertracing/jaeger-client-cpp#13) to just use the standard library functions for this. Something like

std::bernoulli_distribution distribution{sampling_range};
auto sampled = distribution(random_number_generator);

See http://en.cppreference.com/w/cpp/numeric/random/bernoulli_distribution


Endpoint endpoint{tracer_->serviceName(), tracer_->address()};

// Add a binary annotation for the serviceName.
Expand Down Expand Up @@ -329,7 +348,7 @@ class OtTracer : public ot::Tracer,

private:
TracerPtr tracer_;

template <class Carrier>
expected<void> InjectImpl(const ot::SpanContext &sc, Carrier &writer) const
try {
Expand Down
4 changes: 2 additions & 2 deletions zipkin_opentracing/src/propagation.cc
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ injectSpanContext(const opentracing::TextMapWriter &carrier,
if (!result) {
return result;
}
result = carrier.Set(zipkin_sampled, "1");
result = carrier.Set(zipkin_sampled, span_context.isSampled() ? "1" : "0");
if (!result) {
return result;
}
Expand Down Expand Up @@ -105,7 +105,7 @@ extractSpanContext(const opentracing::TextMapReader &carrier,
TraceId trace_id;
Optional<TraceId> parent_id;
uint64_t span_id;
flags_t flags = 0;
flags_t flags = 0;
auto result = carrier.ForeachKey(
[&](ot::string_view key, ot::string_view value) -> ot::expected<void> {
if (keyCompare(key, zipkin_trace_id)) {
Expand Down
8 changes: 8 additions & 0 deletions zipkin_opentracing/src/tracer_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ const char *const configuration_schema = R"(
"description":
"The maximum number of spans to buffer before sending them to the collector",
"minimum": 1
},
"sample_rate": {
"type": "float",
"minimum": 0.0,
"maxiumum": 1.0,
}
}
}
Expand Down Expand Up @@ -91,6 +96,9 @@ OtTracerFactory::MakeTracer(const char *configuration,
if (document.HasMember("max_buffered_spans")) {
options.max_buffered_spans = document["max_buffered_spans"].GetInt();
}
if (document.HasMember("sample_rate")) {
options.sample_rate = document["sample_rate"].GetFloat();
}
return makeZipkinOtTracer(options);
} catch (const std::bad_alloc &) {
return opentracing::make_unexpected(
Expand Down

0 comments on commit 67d68c6

Please sign in to comment.