-
Notifications
You must be signed in to change notification settings - Fork 46
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
Sampling #20
Merged
Merged
Sampling #20
Changes from 14 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
67d68c6
wip: add sampling. current limits include...
pingles 3a59143
extract probabilistic sampling into a class
pingles d7628b2
make sampling decision for root span
pingles 0f64ff8
tracer- check whether referenced context is set, fix impl of sampling
pingles 2203e13
set sampling_set_flag also
pingles 78aa3b2
propagation of sampling status
pingles a4c69e5
call Sampler::ShouldSample to determine whether to sample
pingles ce2ad7b
refactor: simplify sampling decision call
pingles 438942b
write tests and fix implementation
pingles c0fbf73
add support for old OtTracer constructor: defaults to always sampling
pingles 3b29eab
use static thread_local random generator
pingles 6647bef
move random include to file that uses it
pingles f521d15
Span: only report when sampled
pingles 772867e
tracer: check whether parent span context is valid to determine sampling
pingles 2dd68e2
simplify constructor of OtTracer when setting default sampler
pingles ab02764
default initialize sampled_ to true for Span
pingles 26974d1
clang formatted
pingles 0e1b57d
store test log artifacts
pingles e310180
TEMP: specify build dir to capture test artifact output
pingles e44455e
initialize flags when constructing context from span. only need sampl…
pingles d939ce3
ProbabilisticSampler: make constructor explicit
pingles 8e341c8
use std::make_shared to return OtTracer from makeZipkinOtTracer
pingles File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,3 +32,5 @@ | |
*.app | ||
|
||
bazel-* | ||
|
||
.build/ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,14 @@ | ||
#include <zipkin/opentracing.h> | ||
#include <opentracing/util.h> | ||
|
||
#include "sampling.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> | ||
|
@@ -99,6 +102,14 @@ 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_; | ||
|
@@ -156,7 +167,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_}}; | ||
} | ||
|
@@ -271,17 +282,27 @@ 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_{std::move(new ProbabilisticSampler(1.0))} {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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. | ||
|
@@ -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 | ||
|
@@ -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::shared_ptr<ot::Tracer>{new OtTracer{std::move(tracer), std::move(sampler)}}; | ||
} | ||
|
||
std::shared_ptr<ot::Tracer> | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
#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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
#pragma once | ||
#include <memory> | ||
#include <algorithm> | ||
|
||
namespace zipkin { | ||
class Sampler { | ||
public: | ||
virtual ~Sampler() = default; | ||
virtual bool ShouldSample() = 0; | ||
}; | ||
|
||
class ProbabilisticSampler : public Sampler { | ||
public: | ||
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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you default initialize sampled_ to true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely- just to check, do you mean via the default constructor of
Span
or do you mean in the definition ofsampled_
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in-class initialization is preferred for something like that: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c48-prefer-in-class-initializers-to-member-initializers-in-constructors-for-constant-initializers
A lot of the code was copied in from envoy, so not all of it is following some of the conventions I'd use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the link- super helpful for me. I'll make the initialization change now.