-
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 20 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
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
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 |
---|---|---|
@@ -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 |
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,23 @@ | ||
#pragma once | ||
#include <algorithm> | ||
#include <memory> | ||
|
||
namespace zipkin { | ||
class Sampler { | ||
public: | ||
virtual ~Sampler() = default; | ||
virtual bool ShouldSample() = 0; | ||
}; | ||
|
||
class ProbabilisticSampler : public Sampler { | ||
public: | ||
ProbabilisticSampler(double sample_rate) | ||
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. Please use explicit here: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rc-explicit |
||
: 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.
Can you use std::make_shared here? https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rh-make_shared
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.
Nice, thanks, will do.
Can/should
SamplerPtr
andTracerPtr
be replaced withstd::make_unique
also? https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#c150-use-make_unique-to-construct-objects-owned-by-unique_ptrsThanks again for patiently sending me to the C++ idioms :)