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

rule based sampling decisions use the "user" priority values #205

Merged
merged 5 commits into from
Oct 27, 2021
Merged

rule based sampling decisions use the "user" priority values #205

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f451516
rule based sampling decisions use the "user" priority values
dgoffredo Oct 20, 2021
fe9d302
clang-format
dgoffredo Oct 20, 2021
311afcc
fix bug in proposed unit test
dgoffredo Oct 22, 2021
9033b81
update expected sampling priorities in nginx integration test
dgoffredo Oct 22, 2021
1367df2
test/sample_test.cpp: address review comment
dgoffredo Oct 26, 2021
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
2 changes: 1 addition & 1 deletion scripts/run_unit_tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,4 @@ mkdir -p "$BUILD_DIR"
cd "$BUILD_DIR"
cmake -DBUILD_TESTING=ON ..
make --jobs=$(nproc)
ctest --output-on-failure
ctest --output-on-failure "$@"
21 changes: 18 additions & 3 deletions src/sample.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,21 +81,36 @@ SampleResult RulesSampler::sample(const std::string& environment, const std::str
return priority_sampler_.sample(environment, service, trace_id);
}

// A sampling rule applies to (matches) the current span.
//
// Whatever sampling decision we make here (keep or drop) will be of "user"
// type, i.e. `SamplingPriority::UserKeep` or `SamplingPriority::UserDrop`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Any special reason for markdown-ish backticks in comments? Just curious.

Copy link
Contributor Author

@dgoffredo dgoffredo Oct 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This was the convention in some code I've worked on before, and generally I like markdown, and especially I like to call out this-is-a-literal-sequence-of-characters-significant-in-code with markup.

The BDE style, probably the fussiest coding standard I've ever had to adhere to, uses a comment markup that uses single quotes for this instead of backticks. That always bothered me. This is also where I get my mechanical function contract language: "Return a blah using the specified blah blah. The behavior is undefined unless blah blah blah."

I notice that our comments don't escape identifiers, so I've been going against the grain. What do you think?

Alternative to this, I also like the Go style, where they make a point of not using markup of any kind. Still, I feel like it is sometimes awkward to talk about a foo::DingleBertFactory instead of a foo::DingleBertFactory.

//
// The matching rule's rate was configured by a user, and so we want to make
// sure that after sending the span to the agent, that the agent does not
// override our sampling decision as it might for "automated" sampling
// decisions, i.e. `SamplingPriority::SamplerKeep` or
// `SamplingPriority::SamplerDrop`.

SampleResult result;
result.rule_rate = rule_result.rate;
auto max_hash = maxIdFromSampleRate(rule_result.rate);
uint64_t hashed_id = trace_id * constant_rate_hash_factor;
if (hashed_id >= max_hash) {
result.sampling_priority = std::make_unique<SamplingPriority>(SamplingPriority::SamplerDrop);
result.sampling_priority = std::make_unique<SamplingPriority>(SamplingPriority::UserDrop);
return result;
}

// Even though both the priority sampler and the matching sampling rule,
// above, did not drop this span, we still might drop the span in order to
// satify the configured maximum sampling rate for spans selected by rule
// based sampling overall.
auto limit_result = sampling_limiter_.allow();
result.limiter_rate = limit_result.effective_rate;
if (limit_result.allowed) {
result.sampling_priority = std::make_unique<SamplingPriority>(SamplingPriority::SamplerKeep);
result.sampling_priority = std::make_unique<SamplingPriority>(SamplingPriority::UserKeep);
} else {
result.sampling_priority = std::make_unique<SamplingPriority>(SamplingPriority::SamplerDrop);
result.sampling_priority = std::make_unique<SamplingPriority>(SamplingPriority::UserDrop);
}
return result;
}
Expand Down
6 changes: 3 additions & 3 deletions test/integration/nginx/expected_tc1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"metrics": {
"_dd.limit_psr": 1,
"_dd.rule_psr": 1,
"_sampling_priority_v1": 1
"_sampling_priority_v1": 2
},
"name": "nginx.handle",
"resource": "/",
Expand All @@ -37,7 +37,7 @@
"metrics": {
"_dd.limit_psr": 1,
"_dd.rule_psr": 1,
"_sampling_priority_v1": 1
"_sampling_priority_v1": 2
},
"name": "nginx.handle",
"resource": "/",
Expand All @@ -60,7 +60,7 @@
"metrics": {
"_dd.limit_psr": 1,
"_dd.rule_psr": 1,
"_sampling_priority_v1": 1
"_sampling_priority_v1": 2
},
"name": "nginx.handle",
"resource": "/",
Expand Down
107 changes: 100 additions & 7 deletions test/sample_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,20 +62,28 @@ TEST_CASE("priority sampler unit test") {
}

TEST_CASE("rules sampler") {
// `RulesSampler`'s constructor parameters are used to configure the
// sampler's `Limiter`. Here we prepare those arguments.
std::tm start{};
start.tm_mday = 12;
start.tm_mon = 2;
start.tm_year = 107;
TimePoint time{std::chrono::system_clock::from_time_t(timegm(&start)),
std::chrono::steady_clock::time_point{}};
TimeProvider get_time = [&time]() { return time; }; // Mock clock.
const TimePoint time{std::chrono::system_clock::from_time_t(timegm(&start)),
std::chrono::steady_clock::time_point{}};
const TimeProvider get_time = [&time]() { return time; }; // Mock clock.
// A `Limiter` configured with these parameters will allow the first, but
// none afterward.
const long max_tokens = 1;
const double refresh_rate = 1.0;
const long tokens_per_refresh = 1;
const auto sampler =
std::make_shared<RulesSampler>(get_time, max_tokens, refresh_rate, tokens_per_refresh);

auto sampler = std::make_shared<RulesSampler>(get_time, 1, 1.0, 1);
const ot::StartSpanOptions span_options;
const ot::FinishSpanOptions finish_options;

auto mwriter = std::make_shared<MockWriter>(sampler);
auto writer = std::shared_ptr<Writer>(mwriter);
const auto mwriter = std::make_shared<MockWriter>(sampler);
const auto writer = std::shared_ptr<Writer>(mwriter);

SECTION("rule matching applied") {
TracerOptions tracer_options;
Expand Down Expand Up @@ -144,7 +152,7 @@ TEST_CASE("rules sampler") {
REQUIRE(metrics["_dd.rule_psr"] == 0.4);
}

SECTION("applies limiter to sampled spans") {
SECTION("applies limiter to sampled spans only") {
TracerOptions tracer_options;
tracer_options.service = "test.service";
tracer_options.sampling_rules = R"([
Expand All @@ -161,4 +169,89 @@ TEST_CASE("rules sampler") {
REQUIRE(metrics.find("_dd.limit_psr") == metrics.end());
REQUIRE(metrics.find("_dd.agent_psr") == metrics.end());
}

SECTION("sampling based on rule yields a \"user\" sampling priority") {
// See the comments in `RulesSampler::sample` for an explanation of this
// section.

// There are three cases:
// 1. Create a rule that matches the trace, and has rate `0.0`. Expect
// priority `UserDrop`.
// 2. Create a rule that matches the trace, and has rate `1.0`. Expect
// priority `UserKeep`.
// 3. Create a rule that matches the trace, and has rate `1.0`, but the
// limiter drops it. Expect `UserDrop`.

SECTION("when the matching rule drops a trace") {
TracerOptions tracer_options;
tracer_options.service = "test.service";
tracer_options.sampling_rules = R"([
{"sample_rate": 0.0}
])";
const auto tracer = std::make_shared<Tracer>(tracer_options, writer, sampler);

const auto span = tracer->StartSpanWithOptions("operation name", span_options);
span->FinishWithOptions(finish_options);

REQUIRE(mwriter->traces.size() == 1);
REQUIRE(mwriter->traces[0].size() == 1);
const auto& metrics = mwriter->traces[0][0]->metrics;
REQUIRE(metrics.count("_sampling_priority_v1"));
REQUIRE(metrics.at("_sampling_priority_v1") ==
static_cast<double>(SamplingPriority::UserDrop));
}

SECTION("when the matching rule keeps a trace") {
TracerOptions tracer_options;
tracer_options.service = "test.service";
tracer_options.sampling_rules = R"([
{"sample_rate": 1.0}
])";
const auto tracer = std::make_shared<Tracer>(tracer_options, writer, sampler);

const auto span = tracer->StartSpanWithOptions("operation name", span_options);
span->FinishWithOptions(finish_options);

REQUIRE(mwriter->traces.size() == 1);
REQUIRE(mwriter->traces[0].size() == 1);
const auto& metrics = mwriter->traces[0][0]->metrics;
REQUIRE(metrics.count("_sampling_priority_v1"));
REQUIRE(metrics.at("_sampling_priority_v1") ==
static_cast<double>(SamplingPriority::UserKeep));
}

SECTION("when the limiter drops a trace") {
TracerOptions tracer_options;
tracer_options.service = "test.service";
tracer_options.sampling_rules = R"([
{"sample_rate": 1.0}
])";
const auto tracer = std::make_shared<Tracer>(tracer_options, writer, sampler);

auto span = tracer->StartSpanWithOptions("operation name", span_options);
span->FinishWithOptions(finish_options);
// The first trace will be allowed by the limiter.
{
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 consider skipping the checks on the first span, as it's covered by another test and isn't the focus point of this specific test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, will change.

REQUIRE(mwriter->traces.size() == 1);
REQUIRE(mwriter->traces[0].size() == 1);
const auto& metrics = mwriter->traces[0][0]->metrics;
REQUIRE(metrics.count("_sampling_priority_v1"));
REQUIRE(metrics.at("_sampling_priority_v1") ==
static_cast<double>(SamplingPriority::UserKeep));
}

span = tracer->StartSpanWithOptions("operation name", span_options);
span->FinishWithOptions(finish_options);
// The second trace will be dropped by the limiter, and the priority will
// be `UserDrop`.
{
REQUIRE(mwriter->traces.size() == 2);
REQUIRE(mwriter->traces[1].size() == 1);
const auto& metrics = mwriter->traces[1][0]->metrics;
REQUIRE(metrics.count("_sampling_priority_v1"));
REQUIRE(metrics.at("_sampling_priority_v1") ==
static_cast<double>(SamplingPriority::UserDrop));
}
}
}
}