Skip to content

Commit

Permalink
Sampling (#20)
Browse files Browse the repository at this point in the history
* wip: add sampling. current limits include...
* 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

* extract probabilistic sampling into a class
* change sample_rate to double
* use bernoulli distribution to decide whether to sample

* make sampling decision for root span
* inspired by the jaeger module we check whether any of the references are
  ChildOf to identify a parent span.

* tracer- check whether referenced context is set, fix impl of sampling

* set sampling_set_flag also

* propagation of sampling status
* propagation works but seems to be non-deterministic when incoming
  context is incomplete (i.e. just sampling header but no trace id)

* call Sampler::ShouldSample to determine whether to sample
* currently hardcoded to prob sample with fixed rate but will
  be replaced next

* refactor: simplify sampling decision call

* write tests and fix implementation

* add support for old OtTracer constructor: defaults to always sampling

* use static thread_local random generator

* move random include to file that uses it

* Span: only report when sampled

* tracer: check whether parent span context is valid to determine sampling
* parent context may be not-null but empty- not sufficient to just check
  if there's a parent span, must also check whether it's valid.
* probabilistic sampler bounds p(sampled) between 0 and 1

* simplify constructor of OtTracer when setting default sampler

* default initialize sampled_ to true for Span

* clang formatted

* store test log artifacts

* TEMP: specify build dir to capture test artifact output
- normally this looks like it would be configured with the circleci
  config but this should work without repo write access

* initialize flags when constructing context from span. only need sampled_flag set

* ProbabilisticSampler: make constructor explicit

* use std::make_shared to return OtTracer from makeZipkinOtTracer
  • Loading branch information
pingles authored and rnburn committed May 29, 2018
1 parent 4077589 commit 6194d20
Show file tree
Hide file tree
Showing 15 changed files with 141 additions and 10 deletions.
9 changes: 8 additions & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,20 @@ jobs:
- checkout
- run: ./ci/install_dependencies.sh
- run: ./ci/do_ci.sh cmake.asan
- store_artifacts:
path: /build/Testing/Temporary/LastTest.log
destination: Test.log
tsan:
docker:
- image: ubuntu:17.10
steps:
- checkout
- run: mkdir -p /build
- run: ./ci/install_dependencies.sh
- run: ./ci/do_ci.sh cmake.tsan
- run: BUILD_DIR=/build ./ci/do_ci.sh cmake.tsan
- store_artifacts:
path: /build/Testing/Temporary/LastTest.log
destination: Test.log
bazel:
docker:
- image: ubuntu:17.10
Expand Down
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 @@ -332,6 +332,9 @@ class Span : public ZipkinBase {
: 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_{true};
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;
flags_ = 0;

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

for (const Annotation &annotation : span.annotations()) {
if (annotation.value() == ZipkinCoreConstants::get().CLIENT_RECV) {
Expand Down
4 changes: 3 additions & 1 deletion zipkin/src/zipkin_core_types.cc
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,9 @@ const std::string Span::toJson() {

void Span::finish() {
if (auto t = tracer()) {
t->reportSpan(std::move(*this));
if (this->isSampled()) {
t->reportSpan(std::move(*this));
}
}
}

Expand Down
3 changes: 2 additions & 1 deletion zipkin_opentracing/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ set(ZIPKIN_OPENTRACING_SRCS src/utility.cc
src/propagation.cc
src/dynamic_load.cc
src/tracer_factory.cc
src/opentracing.cc)
src/opentracing.cc
src/sampling.cc)

if (BUILD_SHARED_LIBS)
add_library(zipkin_opentracing SHARED ${ZIPKIN_OPENTRACING_SRCS})
Expand Down
4 changes: 2 additions & 2 deletions zipkin_opentracing/example/text_map_carrier.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
#include <string>
#include <unordered_map>

using opentracing::TextMapReader;
using opentracing::TextMapWriter;
using opentracing::expected;
using opentracing::string_view;
using opentracing::TextMapReader;
using opentracing::TextMapWriter;

class TextMapCarrier : public TextMapReader, public TextMapWriter {
public:
Expand Down
2 changes: 2 additions & 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;
double sample_rate = 1.0;

std::string service_name;
IpAddress service_address;
Expand All @@ -19,4 +20,5 @@ makeZipkinOtTracer(const ZipkinOtTracerOptions &options);
std::shared_ptr<opentracing::Tracer>
makeZipkinOtTracer(const ZipkinOtTracerOptions &options,
std::unique_ptr<Reporter> &&reporter);

} // namespace zipkin
29 changes: 26 additions & 3 deletions zipkin_opentracing/src/opentracing.cc
Original file line number Diff line number Diff line change
@@ -1,19 +1,22 @@
#include <opentracing/util.h>
#include <zipkin/opentracing.h>

#include "propagation.h"
#include "sampling.h"
#include "utility.h"
#include <atomic>
#include <cstring>
#include <mutex>
#include <random>
#include <unordered_map>
#include <zipkin/tracer.h>
#include <zipkin/utility.h>
#include <zipkin/zipkin_core_types.h>

using opentracing::Value;
using opentracing::expected;
using opentracing::make_unexpected;
using opentracing::string_view;
using opentracing::Value;

namespace ot = opentracing;

Expand Down Expand Up @@ -99,6 +102,12 @@ class OtSpanContext : public ot::SpanContext {
return injectSpanContext(writer, span_context_, baggage_);
}

bool isSampled() const { return span_context_.isSampled(); }

bool isValid() const {
return span_context_.id() != 0 && !span_context_.trace_id().empty();
}

private:
zipkin::SpanContext span_context_;
mutable std::mutex baggage_mutex_;
Expand Down Expand Up @@ -271,17 +280,29 @@ class OtSpan : public ot::Span {
class OtTracer : public ot::Tracer,
public std::enable_shared_from_this<OtTracer> {
public:
explicit OtTracer(TracerPtr &&tracer) : tracer_{std::move(tracer)} {}
explicit OtTracer(TracerPtr &&tracer)
: tracer_{std::move(tracer)}, sampler_{new ProbabilisticSampler(1.0)} {}
explicit OtTracer(TracerPtr &&tracer, SamplerPtr &&sampler)
: tracer_{std::move(tracer)}, sampler_{std::move(sampler)} {}

std::unique_ptr<ot::Span>
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());

auto parent = findSpanContext(options.references);

if (parent && parent->isValid()) {
span->setSampled(parent->isSampled());
} else {
span->setSampled(sampler_->ShouldSample());
}

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

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

private:
TracerPtr tracer_;
SamplerPtr sampler_;

template <class Carrier>
expected<void> InjectImpl(const ot::SpanContext &sc, Carrier &writer) const
Expand Down Expand Up @@ -368,7 +390,8 @@ makeZipkinOtTracer(const ZipkinOtTracerOptions &options,
std::unique_ptr<Reporter> &&reporter) {
TracerPtr tracer{new Tracer{options.service_name, options.service_address}};
tracer->setReporter(std::move(reporter));
return std::shared_ptr<ot::Tracer>{new OtTracer{std::move(tracer)}};
SamplerPtr sampler{new ProbabilisticSampler{options.sample_rate}};
return std::make_shared<OtTracer>(std::move(tracer), std::move(sampler));
}

std::shared_ptr<ot::Tracer>
Expand Down
6 changes: 4 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 @@ -135,7 +135,9 @@ extractSpanContext(const opentracing::TextMapReader &carrier,
if (!parseBool(value, sampled)) {
return ot::make_unexpected(ot::span_context_corrupted_error);
}
flags |= sampled_flag;
if (sampled) {
flags |= sampled_flag;
}
} else if (keyCompare(key, zipkin_parent_span_id)) {
parent_id = Hex::hexToTraceId(value);
if (!parent_id.valid()) {
Expand Down
10 changes: 10 additions & 0 deletions zipkin_opentracing/src/sampling.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#include "sampling.h"
#include <random>

namespace zipkin {
bool ProbabilisticSampler::ShouldSample() {
static thread_local std::mt19937 rng(std::random_device{}());
std::bernoulli_distribution dist(sample_rate_);
return dist(rng);
}
} // namespace zipkin
23 changes: 23 additions & 0 deletions zipkin_opentracing/src/sampling.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#pragma once
#include <algorithm>
#include <memory>

namespace zipkin {
class Sampler {
public:
virtual ~Sampler() = default;
virtual bool ShouldSample() = 0;
};

class ProbabilisticSampler : public Sampler {
public:
explicit ProbabilisticSampler(double sample_rate)
: sample_rate_(std::max(0.0, std::min(sample_rate, 1.0))){};
bool ShouldSample() override;

private:
double sample_rate_;
};

typedef std::unique_ptr<Sampler> SamplerPtr;
} // namespace zipkin
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"].GetDouble();
}
return makeZipkinOtTracer(options);
} catch (const std::bad_alloc &) {
return opentracing::make_unexpected(
Expand Down
40 changes: 40 additions & 0 deletions zipkin_opentracing/test/ot_tracer_test.cc
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include "../src/sampling.h"
#include "../src/utility.h"
#include "in_memory_reporter.h"
#include <algorithm>
Expand Down Expand Up @@ -55,6 +56,45 @@ TEST_CASE("ot_tracer") {
CHECK(hasTag(span, "xyz", true));
}

SECTION("Uses sampling rate to determine whether to sample a span.") {
auto r = new InMemoryReporter();

ZipkinOtTracerOptions options;
options.sample_rate = 0.0;
auto t = makeZipkinOtTracer(options, std::unique_ptr<Reporter>(r));

auto span_a = t->StartSpan("a");
CHECK(span_a);
span_a->Finish();

CHECK(r->spans().empty());
}

SECTION("Propagates sampling decision to child span") {
ZipkinOtTracerOptions no_sampling;
no_sampling.sample_rate = 0.0;
auto r1 = new InMemoryReporter();
auto no_sampling_tracer =
makeZipkinOtTracer(no_sampling, std::unique_ptr<Reporter>(r1));

ZipkinOtTracerOptions always_sample;
always_sample.sample_rate = 1.0;
auto r2 = new InMemoryReporter();
auto sampling_tracer =
makeZipkinOtTracer(always_sample, std::unique_ptr<Reporter>(r2));

auto span_a = no_sampling_tracer->StartSpan("a");
CHECK(span_a);
span_a->Finish();
auto span_b =
sampling_tracer->StartSpan("b", {ChildOf(&span_a->context())});
CHECK(span_b);
span_b->Finish();

CHECK(r1->spans().empty());
CHECK(r2->spans().empty());
}

SECTION("You can set a single child-of reference when starting a span.") {
auto span_a = tracer->StartSpan("a");
CHECK(span_a);
Expand Down

0 comments on commit 6194d20

Please sign in to comment.