From 29e127be9bd45dd83041ebad3be38244ea0ff9c5 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Thu, 25 Jun 2020 12:16:02 -0400 Subject: [PATCH 01/34] Add ProbabilitySampler --- .../sdk/trace/probability_sampler.h | 47 ++++++++++ sdk/src/trace/CMakeLists.txt | 3 +- sdk/src/trace/probability_sampler.cc | 92 +++++++++++++++++++ 3 files changed, 141 insertions(+), 1 deletion(-) create mode 100644 sdk/include/opentelemetry/sdk/trace/probability_sampler.h create mode 100644 sdk/src/trace/probability_sampler.cc diff --git a/sdk/include/opentelemetry/sdk/trace/probability_sampler.h b/sdk/include/opentelemetry/sdk/trace/probability_sampler.h new file mode 100644 index 0000000000..503463fec8 --- /dev/null +++ b/sdk/include/opentelemetry/sdk/trace/probability_sampler.h @@ -0,0 +1,47 @@ +#pragma once + +#include "opentelemetry/sdk/trace/sampler.h" + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace trace +{ +namespace trace_api = opentelemetry::trace; +/** + * The probability sampler ... + * + */ +class ProbabilitySampler : public Sampler +{ +public: + explicit ProbabilitySampler(double probability); + + /** + * @return Returns NOT_RECORD always + */ + SamplingResult ShouldSample( + const SpanContext *parent_context, + trace_api::TraceId trace_id, + nostd::string_view name, + trace_api::SpanKind span_kind, + const trace_api::KeyValueIterable &attributes) noexcept override; + + /** + * @return Description MUST be ProbabilitySampler{0.000100} + */ + std::string GetDescription() const noexcept override; + + uint64_t CalculateThreshold(double probability) const noexcept; + + uint64_t CalculateThresholdFromBuffer(const trace_api::TraceId& trace_id) const noexcept; + + double GetProbability() const noexcept; + +private: + const uint64_t threshold_; + const double probability_; +}; +} // namespace trace +} // namespace sdk +OPENTELEMETRY_END_NAMESPACE \ No newline at end of file diff --git a/sdk/src/trace/CMakeLists.txt b/sdk/src/trace/CMakeLists.txt index 98f949997e..925ee8bd02 100644 --- a/sdk/src/trace/CMakeLists.txt +++ b/sdk/src/trace/CMakeLists.txt @@ -1 +1,2 @@ -add_library(opentelemetry_trace tracer_provider.cc tracer.cc span.cc samplers/parent_or_else.cc) +add_library(opentelemetry_trace tracer_provider.cc tracer.cc span.cc + samplers/parent_or_else.cc probability_sampler.cc) diff --git a/sdk/src/trace/probability_sampler.cc b/sdk/src/trace/probability_sampler.cc new file mode 100644 index 0000000000..bd4a66c3e9 --- /dev/null +++ b/sdk/src/trace/probability_sampler.cc @@ -0,0 +1,92 @@ +#include "opentelemetry/sdk/trace/probability_sampler.h" + +#include +#include + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace trace +{ +namespace trace_api = opentelemetry::trace; + +ProbabilitySampler::ProbabilitySampler(double probability) + : threshold_(CalculateThreshold(probability)), probability_(probability) {} + +SamplingResult ProbabilitySampler::ShouldSample( + const SpanContext *parent_context, + trace_api::TraceId trace_id, + nostd::string_view name, + trace_api::SpanKind span_kind, + const trace_api::KeyValueIterable &attributes) noexcept +{ + if (threshold_ == 0) return { Decision::NOT_RECORD, nullptr }; + + if (parent_context == nullptr) + { + if (CalculateThresholdFromBuffer(trace_id) <= threshold_) + { + return { Decision::RECORD_AND_SAMPLE, nullptr }; + } + } + else + { + if (parent_context->is_recording && parent_context->sampled_flag) + { + return { Decision::RECORD_AND_SAMPLE, nullptr }; + } + else if (parent_context->is_recording) + { + return { Decision::RECORD, nullptr}; + } + } + + return { Decision::NOT_RECORD, nullptr }; +} + +/** + * @return Description MUST be ProbabilitySampler{0.000100} + */ +std::string ProbabilitySampler::GetDescription() const noexcept +{ + char buffer[30]; + sprintf(buffer, "ProbabilitySampler{%.6f}", GetProbability()); + return std::string(buffer); +} + +uint64_t ProbabilitySampler::CalculateThreshold(double probability) const noexcept +{ + if (probability <= 0.0) return 0; + if (probability >= 1.0) return UINT64_MAX; + // We can't directly return probability * UINT64_MAX. + // + // UINT64_MAX is (2^64)-1, but as a double rounds up to 2^64. + // For probabilities >= 1-(2^-54), the product wraps to zero! + // Instead, calculate the high and low 32 bits separately. + const double product = UINT32_MAX * probability; + double hi_bits, lo_bits = ldexp(modf(product, &hi_bits), 32) + product; + return (static_cast(hi_bits) << 32) + + static_cast(lo_bits); +} + +uint64_t ProbabilitySampler::CalculateThresholdFromBuffer(const trace_api::TraceId& trace_id) const noexcept +{ + nostd::span buf = trace_id.Id(); + uint64_t res = 0; + // We only use the first 8 bytes of TraceId. + static_assert(trace_id.kSize >= 8, "TraceID must be at least 8 bytes long."); + for (int i = 0; i < 8; ++i) { + res |= (static_cast(buf[i]) << (i * 8)); + } + return res; +} + +double ProbabilitySampler::GetProbability() const noexcept +{ + if (probability_ <= 0.0) return 0; + if (probability_ >= 1.0) return 1.0; + return probability_; +} +} // namespace trace +} // namespace sdk +OPENTELEMETRY_END_NAMESPACE \ No newline at end of file From f38322fb28f67907f1625d14ac42d22f68f5d67d Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Thu, 25 Jun 2020 12:16:18 -0400 Subject: [PATCH 02/34] Add ProbabilitySampler unit tests --- sdk/test/trace/BUILD | 11 +++ sdk/test/trace/CMakeLists.txt | 2 +- sdk/test/trace/probability_sampler_test.cc | 96 ++++++++++++++++++++++ 3 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 sdk/test/trace/probability_sampler_test.cc diff --git a/sdk/test/trace/BUILD b/sdk/test/trace/BUILD index a3c3c77f91..d09a209788 100644 --- a/sdk/test/trace/BUILD +++ b/sdk/test/trace/BUILD @@ -74,3 +74,14 @@ cc_test( "@com_google_googletest//:gtest_main", ], ) + +cc_test( + name = "probability_sampler_test", + srcs = [ + "probability_sampler_test.cc", + ] + deps = [ + "//sdk/src/trace", + "@com_google_googletest//:gtest_main", + ], +) diff --git a/sdk/test/trace/CMakeLists.txt b/sdk/test/trace/CMakeLists.txt index b4524da0f2..be3a99719e 100644 --- a/sdk/test/trace/CMakeLists.txt +++ b/sdk/test/trace/CMakeLists.txt @@ -1,6 +1,6 @@ foreach(testname tracer_provider_test span_data_test simple_processor_test tracer_test always_off_sampler_test always_on_sampler_test - parent_or_else_sampler_test) + parent_or_else_sampler_test probability_sampler_test) add_executable(${testname} "${testname}.cc") target_link_libraries(${testname} ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT} opentelemetry_trace) diff --git a/sdk/test/trace/probability_sampler_test.cc b/sdk/test/trace/probability_sampler_test.cc new file mode 100644 index 0000000000..2ac1f436e2 --- /dev/null +++ b/sdk/test/trace/probability_sampler_test.cc @@ -0,0 +1,96 @@ +#include "opentelemetry/sdk/trace/probability_sampler.h" + +#include + +using opentelemetry::sdk::trace::ProbabilitySampler; +using opentelemetry::sdk::trace::Decision; + +TEST(ProbabilitySampler, ShouldSampleWithoutContext) +{ + ProbabilitySampler s1(0.01); + + opentelemetry::trace::TraceId invalid_trace_id; + + opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; + + using M = std::map; + M m1 = {{}}; + opentelemetry::trace::KeyValueIterableView view{m1}; + + auto sampling_result = s1.ShouldSample(nullptr, invalid_trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + constexpr uint8_t buf[] = {1, 2, 3, 4, 5, 6, 7, 8, 8, 7, 6, 5, 4, 3, 2, 1}; + opentelemetry::trace::TraceId valid_trace_id(buf); + + sampling_result = s1.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + ProbabilitySampler s2(0.123457); + + sampling_result = s2.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); +} + +TEST(ProbabilitySampler, ShouldSampleWithContext) +{ + ProbabilitySampler s1(0.01); + + opentelemetry::trace::TraceId trace_id; + opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; + opentelemetry::sdk::trace::Sampler::SpanContext c1(false, false); + opentelemetry::sdk::trace::Sampler::SpanContext c2(false, true); + opentelemetry::sdk::trace::Sampler::SpanContext c3(true, false); + opentelemetry::sdk::trace::Sampler::SpanContext c4(true, true); + + using M = std::map; + M m1 = {{}}; + opentelemetry::trace::KeyValueIterableView view{m1}; + + auto sampling_result = s1.ShouldSample(&c1, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s1.ShouldSample(&c2, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s1.ShouldSample(&c3, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s1.ShouldSample(&c4, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); +} + +TEST(ProbabilitySampler, GetDescription) +{ + ProbabilitySampler s1(0.01); + ASSERT_EQ("ProbabilitySampler{0.010000}", s1.GetDescription()); + + ProbabilitySampler s2(0.00); + ASSERT_EQ("ProbabilitySampler{0.000000}", s2.GetDescription()); + + ProbabilitySampler s3(1.00); + ASSERT_EQ("ProbabilitySampler{1.000000}", s3.GetDescription()); + + ProbabilitySampler s4(3.00); + ASSERT_EQ("ProbabilitySampler{1.000000}", s4.GetDescription()); + + ProbabilitySampler s5(-3.00); + ASSERT_EQ("ProbabilitySampler{0.000000}", s5.GetDescription()); + + ProbabilitySampler s6(0.102030405); + ASSERT_EQ("ProbabilitySampler{0.102030}", s6.GetDescription()); +} \ No newline at end of file From 70615883d33d0b2da1ae9cc7275dc32fb52bc61c Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Tue, 30 Jun 2020 11:49:41 -0400 Subject: [PATCH 03/34] Add sampler config and copyright attribution --- .../sdk/trace/probability_sampler.h | 37 +++- sdk/src/trace/probability_sampler.cc | 46 ++++- sdk/test/trace/probability_sampler_test.cc | 168 +++++++++++++++++- 3 files changed, 232 insertions(+), 19 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/trace/probability_sampler.h b/sdk/include/opentelemetry/sdk/trace/probability_sampler.h index 503463fec8..04f8b34c7c 100644 --- a/sdk/include/opentelemetry/sdk/trace/probability_sampler.h +++ b/sdk/include/opentelemetry/sdk/trace/probability_sampler.h @@ -1,3 +1,18 @@ +// Copyright 2020, Open Telemetry Authors +// Copyright 2017, OpenCensus Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + #pragma once #include "opentelemetry/sdk/trace/sampler.h" @@ -8,6 +23,23 @@ namespace sdk namespace trace { namespace trace_api = opentelemetry::trace; + +/** The default behavior is to apply the sampling probability only for Spans + * that are root spans (no parent) and Spans with remote parent. However + * there should be configuration to change this to "root spans only", + * or "all spans". + */ +enum class SamplingBehavior +{ + // Root Spans (no parent) and Spans with a remote parent + DETACHED_SPANS_ONLY, + // Root Spans (no parent) only + ROOT_SPANS_ONLY, + // All Spans: Root Spans (no parent), Spans with a remote parent, and + // Spans with a direct parent + ALL_SPANS +}; + /** * The probability sampler ... * @@ -15,7 +47,8 @@ namespace trace_api = opentelemetry::trace; class ProbabilitySampler : public Sampler { public: - explicit ProbabilitySampler(double probability); + explicit ProbabilitySampler(double probability, bool defer_parent = true, + SamplingBehavior sampling_behavior = SamplingBehavior::DETACHED_SPANS_ONLY); /** * @return Returns NOT_RECORD always @@ -41,6 +74,8 @@ class ProbabilitySampler : public Sampler private: const uint64_t threshold_; const double probability_; + const bool defer_parent_; + const SamplingBehavior sampling_behavior_; }; } // namespace trace } // namespace sdk diff --git a/sdk/src/trace/probability_sampler.cc b/sdk/src/trace/probability_sampler.cc index bd4a66c3e9..b27e191355 100644 --- a/sdk/src/trace/probability_sampler.cc +++ b/sdk/src/trace/probability_sampler.cc @@ -1,3 +1,18 @@ +// Copyright 2020, Open Telemetry Authors +// Copyright 2017, OpenCensus Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + #include "opentelemetry/sdk/trace/probability_sampler.h" #include @@ -10,8 +25,11 @@ namespace trace { namespace trace_api = opentelemetry::trace; -ProbabilitySampler::ProbabilitySampler(double probability) - : threshold_(CalculateThreshold(probability)), probability_(probability) {} +ProbabilitySampler::ProbabilitySampler( + double probability, bool defer_parent, SamplingBehavior sampling_behavior +) +: threshold_(CalculateThreshold(probability)), probability_(probability), + defer_parent_(defer_parent), sampling_behavior_(sampling_behavior) {} SamplingResult ProbabilitySampler::ShouldSample( const SpanContext *parent_context, @@ -20,10 +38,23 @@ SamplingResult ProbabilitySampler::ShouldSample( trace_api::SpanKind span_kind, const trace_api::KeyValueIterable &attributes) noexcept { - if (threshold_ == 0) return { Decision::NOT_RECORD, nullptr }; + // Check if current Span should be sampled with current Sampling Behavior + if (sampling_behavior_ == SamplingBehavior::DETACHED_SPANS_ONLY + && parent_context != nullptr && !parent_context->is_remote) { + return { Decision::NOT_RECORD, nullptr }; + } + + if (sampling_behavior_ == SamplingBehavior::ROOT_SPANS_ONLY + && parent_context != nullptr) { + return { Decision::NOT_RECORD, nullptr }; + } - if (parent_context == nullptr) + // Check if sampling decision should be defered to parent span, if applicable + if (parent_context == nullptr || !defer_parent_) { + if (threshold_ == 0) return { Decision::NOT_RECORD, nullptr }; + + // Calculcate probability based decision based on trace_id and threshold if (CalculateThresholdFromBuffer(trace_id) <= threshold_) { return { Decision::RECORD_AND_SAMPLE, nullptr }; @@ -31,13 +62,10 @@ SamplingResult ProbabilitySampler::ShouldSample( } else { - if (parent_context->is_recording && parent_context->sampled_flag) + // Return sampling decision based on parent span_context config + if (parent_context->sampled) { return { Decision::RECORD_AND_SAMPLE, nullptr }; - } - else if (parent_context->is_recording) - { - return { Decision::RECORD, nullptr}; } } diff --git a/sdk/test/trace/probability_sampler_test.cc b/sdk/test/trace/probability_sampler_test.cc index 2ac1f436e2..0fe47a0146 100644 --- a/sdk/test/trace/probability_sampler_test.cc +++ b/sdk/test/trace/probability_sampler_test.cc @@ -4,11 +4,13 @@ using opentelemetry::sdk::trace::ProbabilitySampler; using opentelemetry::sdk::trace::Decision; +using opentelemetry::sdk::trace::SamplingBehavior; TEST(ProbabilitySampler, ShouldSampleWithoutContext) { ProbabilitySampler s1(0.01); - + ProbabilitySampler s2(0.01, false, SamplingBehavior::ALL_SPANS); + ProbabilitySampler s3(0.01, false, SamplingBehavior::ROOT_SPANS_ONLY); opentelemetry::trace::TraceId invalid_trace_id; opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; @@ -17,11 +19,27 @@ TEST(ProbabilitySampler, ShouldSampleWithoutContext) M m1 = {{}}; opentelemetry::trace::KeyValueIterableView view{m1}; + // [DEFAULT] SamplingBehavior::DETACHED_SPANS_ONLY + auto sampling_result = s1.ShouldSample(nullptr, invalid_trace_id, "", span_kind, view); ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); ASSERT_EQ(nullptr, sampling_result.attributes); + // SamplingBehavior::ALL_SPANS + + sampling_result = s2.ShouldSample(nullptr, invalid_trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + // SamplingBehavior::ROOT_SPANS_ONLY + + sampling_result = s3.ShouldSample(nullptr, invalid_trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + constexpr uint8_t buf[] = {1, 2, 3, 4, 5, 6, 7, 8, 8, 7, 6, 5, 4, 3, 2, 1}; opentelemetry::trace::TraceId valid_trace_id(buf); @@ -30,29 +48,33 @@ TEST(ProbabilitySampler, ShouldSampleWithoutContext) ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); ASSERT_EQ(nullptr, sampling_result.attributes); - ProbabilitySampler s2(0.123457); + ProbabilitySampler s4(0.123457); - sampling_result = s2.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); + sampling_result = s4.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); ASSERT_EQ(nullptr, sampling_result.attributes); } -TEST(ProbabilitySampler, ShouldSampleWithContext) +TEST(ProbabilitySampler, ShouldSampleWithContextDeferParent) { - ProbabilitySampler s1(0.01); + ProbabilitySampler s1(0.01, true); + ProbabilitySampler s2(0.01, true, SamplingBehavior::ALL_SPANS); + ProbabilitySampler s3(0.01, true, SamplingBehavior::ROOT_SPANS_ONLY); opentelemetry::trace::TraceId trace_id; opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; opentelemetry::sdk::trace::Sampler::SpanContext c1(false, false); - opentelemetry::sdk::trace::Sampler::SpanContext c2(false, true); - opentelemetry::sdk::trace::Sampler::SpanContext c3(true, false); - opentelemetry::sdk::trace::Sampler::SpanContext c4(true, true); + opentelemetry::sdk::trace::Sampler::SpanContext c2(true, false); + opentelemetry::sdk::trace::Sampler::SpanContext c3(false, false); + opentelemetry::sdk::trace::Sampler::SpanContext c4(true, false); using M = std::map; M m1 = {{}}; opentelemetry::trace::KeyValueIterableView view{m1}; + // [DEFAULT] SamplingBehavior::DETACHED_SPANS_ONLY + auto sampling_result = s1.ShouldSample(&c1, trace_id, "", span_kind, view); ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); @@ -65,13 +87,141 @@ TEST(ProbabilitySampler, ShouldSampleWithContext) sampling_result = s1.ShouldSample(&c3, trace_id, "", span_kind, view); - ASSERT_EQ(Decision::RECORD, sampling_result.decision); + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); ASSERT_EQ(nullptr, sampling_result.attributes); sampling_result = s1.ShouldSample(&c4, trace_id, "", span_kind, view); + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + // SamplingBehavior::ALL_SPANS + + sampling_result = s2.ShouldSample(&c1, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s2.ShouldSample(&c2, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s2.ShouldSample(&c3, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s2.ShouldSample(&c4, trace_id, "", span_kind, view); + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); ASSERT_EQ(nullptr, sampling_result.attributes); + + // SamplingBehavior::ROOT_SPANS_ONLY + + sampling_result = s3.ShouldSample(&c1, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s3.ShouldSample(&c2, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s3.ShouldSample(&c3, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s3.ShouldSample(&c4, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); +} + +TEST(ProbabilitySampler, ShouldSampleWithContextNoDeferParent) +{ + ProbabilitySampler s1(0.01, false); + ProbabilitySampler s2(0.01, false, SamplingBehavior::ALL_SPANS); + ProbabilitySampler s3(0.01, false, SamplingBehavior::ROOT_SPANS_ONLY); + + opentelemetry::trace::TraceId trace_id; + opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; + opentelemetry::sdk::trace::Sampler::SpanContext c1(false, false); + opentelemetry::sdk::trace::Sampler::SpanContext c2(true, false); + opentelemetry::sdk::trace::Sampler::SpanContext c3(false, false); + opentelemetry::sdk::trace::Sampler::SpanContext c4(true, false); + + using M = std::map; + M m1 = {{}}; + opentelemetry::trace::KeyValueIterableView view{m1}; + + // [DEFAULT] SamplingBehavior::DETACHED_SPANS_ONLY + + auto sampling_result = s1.ShouldSample(&c1, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s1.ShouldSample(&c2, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s1.ShouldSample(&c3, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s1.ShouldSample(&c4, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + // SamplingBehavior::ALL_SPANS + + sampling_result = s2.ShouldSample(&c1, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s2.ShouldSample(&c2, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s2.ShouldSample(&c3, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s2.ShouldSample(&c4, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + // SamplingBehavior::ROOT_SPANS_ONLY + + sampling_result = s3.ShouldSample(&c1, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s3.ShouldSample(&c2, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s3.ShouldSample(&c3, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + sampling_result = s3.ShouldSample(&c4, trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); } TEST(ProbabilitySampler, GetDescription) From e7637326f7dd4be4148efae86b3478db6149dcd4 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Tue, 30 Jun 2020 12:17:00 -0400 Subject: [PATCH 04/34] Fix syntax issue --- sdk/test/trace/BUILD | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/test/trace/BUILD b/sdk/test/trace/BUILD index d09a209788..bef910e9e5 100644 --- a/sdk/test/trace/BUILD +++ b/sdk/test/trace/BUILD @@ -79,7 +79,7 @@ cc_test( name = "probability_sampler_test", srcs = [ "probability_sampler_test.cc", - ] + ], deps = [ "//sdk/src/trace", "@com_google_googletest//:gtest_main", From e994b31d14e4e310afa7556fc82c632e2ae1c427 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Tue, 30 Jun 2020 12:21:12 -0400 Subject: [PATCH 05/34] Add newlines to EOF --- sdk/include/opentelemetry/sdk/trace/probability_sampler.h | 2 +- sdk/src/trace/probability_sampler.cc | 2 +- sdk/test/trace/probability_sampler_test.cc | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/trace/probability_sampler.h b/sdk/include/opentelemetry/sdk/trace/probability_sampler.h index 04f8b34c7c..e3375e0fc9 100644 --- a/sdk/include/opentelemetry/sdk/trace/probability_sampler.h +++ b/sdk/include/opentelemetry/sdk/trace/probability_sampler.h @@ -79,4 +79,4 @@ class ProbabilitySampler : public Sampler }; } // namespace trace } // namespace sdk -OPENTELEMETRY_END_NAMESPACE \ No newline at end of file +OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/src/trace/probability_sampler.cc b/sdk/src/trace/probability_sampler.cc index b27e191355..1756814122 100644 --- a/sdk/src/trace/probability_sampler.cc +++ b/sdk/src/trace/probability_sampler.cc @@ -117,4 +117,4 @@ double ProbabilitySampler::GetProbability() const noexcept } } // namespace trace } // namespace sdk -OPENTELEMETRY_END_NAMESPACE \ No newline at end of file +OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/test/trace/probability_sampler_test.cc b/sdk/test/trace/probability_sampler_test.cc index 0fe47a0146..7dd8581592 100644 --- a/sdk/test/trace/probability_sampler_test.cc +++ b/sdk/test/trace/probability_sampler_test.cc @@ -243,4 +243,4 @@ TEST(ProbabilitySampler, GetDescription) ProbabilitySampler s6(0.102030405); ASSERT_EQ("ProbabilitySampler{0.102030}", s6.GetDescription()); -} \ No newline at end of file +} From d371d7aa9c1a25f4bc2d70edc6a6356187163064 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Tue, 30 Jun 2020 13:14:21 -0400 Subject: [PATCH 06/34] Update trace buffer type --- sdk/src/trace/probability_sampler.cc | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/sdk/src/trace/probability_sampler.cc b/sdk/src/trace/probability_sampler.cc index 1756814122..84f0e35059 100644 --- a/sdk/src/trace/probability_sampler.cc +++ b/sdk/src/trace/probability_sampler.cc @@ -99,10 +99,17 @@ uint64_t ProbabilitySampler::CalculateThreshold(double probability) const noexce uint64_t ProbabilitySampler::CalculateThresholdFromBuffer(const trace_api::TraceId& trace_id) const noexcept { - nostd::span buf = trace_id.Id(); - uint64_t res = 0; // We only use the first 8 bytes of TraceId. static_assert(trace_id.kSize >= 8, "TraceID must be at least 8 bytes long."); + + uint8_t buf[trace_api::TraceId::kSize]; + + for (int i = 0; i < 8; ++i) { + buf[i] = trace_id.Id()[i]; + } + + uint64_t res = 0; + for (int i = 0; i < 8; ++i) { res |= (static_cast(buf[i]) << (i * 8)); } From 85c9dbe40207f5d3a2d3aeff7ff59210619f4053 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Tue, 30 Jun 2020 13:27:51 -0400 Subject: [PATCH 07/34] Update static_assert --- sdk/src/trace/probability_sampler.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/src/trace/probability_sampler.cc b/sdk/src/trace/probability_sampler.cc index 84f0e35059..2f62133f0b 100644 --- a/sdk/src/trace/probability_sampler.cc +++ b/sdk/src/trace/probability_sampler.cc @@ -100,7 +100,7 @@ uint64_t ProbabilitySampler::CalculateThreshold(double probability) const noexce uint64_t ProbabilitySampler::CalculateThresholdFromBuffer(const trace_api::TraceId& trace_id) const noexcept { // We only use the first 8 bytes of TraceId. - static_assert(trace_id.kSize >= 8, "TraceID must be at least 8 bytes long."); + static_assert(trace_api::TraceId::kSize >= 8, "TraceID must be at least 8 bytes long."); uint8_t buf[trace_api::TraceId::kSize]; From 5dea575d552bd017e5e7d1b3518d345fea9697de Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 1 Jul 2020 08:29:19 -0400 Subject: [PATCH 08/34] Fix inconsistent spacing --- sdk/include/opentelemetry/sdk/trace/probability_sampler.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/trace/probability_sampler.h b/sdk/include/opentelemetry/sdk/trace/probability_sampler.h index e3375e0fc9..b3ffefbc57 100644 --- a/sdk/include/opentelemetry/sdk/trace/probability_sampler.h +++ b/sdk/include/opentelemetry/sdk/trace/probability_sampler.h @@ -72,8 +72,8 @@ class ProbabilitySampler : public Sampler double GetProbability() const noexcept; private: - const uint64_t threshold_; - const double probability_; + const uint64_t threshold_; + const double probability_; const bool defer_parent_; const SamplingBehavior sampling_behavior_; }; From 1049c3f31b9a045e0e311e34fe8ca9233a2b8cf9 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 1 Jul 2020 08:50:05 -0400 Subject: [PATCH 09/34] Move variable definitions --- sdk/test/trace/probability_sampler_test.cc | 33 ++++++++++++++-------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/sdk/test/trace/probability_sampler_test.cc b/sdk/test/trace/probability_sampler_test.cc index 7dd8581592..c8792370b7 100644 --- a/sdk/test/trace/probability_sampler_test.cc +++ b/sdk/test/trace/probability_sampler_test.cc @@ -8,9 +8,6 @@ using opentelemetry::sdk::trace::SamplingBehavior; TEST(ProbabilitySampler, ShouldSampleWithoutContext) { - ProbabilitySampler s1(0.01); - ProbabilitySampler s2(0.01, false, SamplingBehavior::ALL_SPANS); - ProbabilitySampler s3(0.01, false, SamplingBehavior::ROOT_SPANS_ONLY); opentelemetry::trace::TraceId invalid_trace_id; opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; @@ -21,6 +18,8 @@ TEST(ProbabilitySampler, ShouldSampleWithoutContext) // [DEFAULT] SamplingBehavior::DETACHED_SPANS_ONLY + ProbabilitySampler s1(0.01); + auto sampling_result = s1.ShouldSample(nullptr, invalid_trace_id, "", span_kind, view); ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); @@ -28,6 +27,8 @@ TEST(ProbabilitySampler, ShouldSampleWithoutContext) // SamplingBehavior::ALL_SPANS + ProbabilitySampler s2(0.01, false, SamplingBehavior::ALL_SPANS); + sampling_result = s2.ShouldSample(nullptr, invalid_trace_id, "", span_kind, view); ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); @@ -35,6 +36,8 @@ TEST(ProbabilitySampler, ShouldSampleWithoutContext) // SamplingBehavior::ROOT_SPANS_ONLY + ProbabilitySampler s3(0.01, false, SamplingBehavior::ROOT_SPANS_ONLY); + sampling_result = s3.ShouldSample(nullptr, invalid_trace_id, "", span_kind, view); ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); @@ -50,6 +53,8 @@ TEST(ProbabilitySampler, ShouldSampleWithoutContext) ProbabilitySampler s4(0.123457); + // [DEFAULT] SamplingBehavior::DETACHED_SPANS_ONLY + sampling_result = s4.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); @@ -58,10 +63,6 @@ TEST(ProbabilitySampler, ShouldSampleWithoutContext) TEST(ProbabilitySampler, ShouldSampleWithContextDeferParent) { - ProbabilitySampler s1(0.01, true); - ProbabilitySampler s2(0.01, true, SamplingBehavior::ALL_SPANS); - ProbabilitySampler s3(0.01, true, SamplingBehavior::ROOT_SPANS_ONLY); - opentelemetry::trace::TraceId trace_id; opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; opentelemetry::sdk::trace::Sampler::SpanContext c1(false, false); @@ -75,6 +76,8 @@ TEST(ProbabilitySampler, ShouldSampleWithContextDeferParent) // [DEFAULT] SamplingBehavior::DETACHED_SPANS_ONLY + ProbabilitySampler s1(0.01, true); + auto sampling_result = s1.ShouldSample(&c1, trace_id, "", span_kind, view); ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); @@ -97,6 +100,8 @@ TEST(ProbabilitySampler, ShouldSampleWithContextDeferParent) // SamplingBehavior::ALL_SPANS + ProbabilitySampler s2(0.01, true, SamplingBehavior::ALL_SPANS); + sampling_result = s2.ShouldSample(&c1, trace_id, "", span_kind, view); ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); @@ -119,6 +124,8 @@ TEST(ProbabilitySampler, ShouldSampleWithContextDeferParent) // SamplingBehavior::ROOT_SPANS_ONLY + ProbabilitySampler s3(0.01, true, SamplingBehavior::ROOT_SPANS_ONLY); + sampling_result = s3.ShouldSample(&c1, trace_id, "", span_kind, view); ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); @@ -142,10 +149,6 @@ TEST(ProbabilitySampler, ShouldSampleWithContextDeferParent) TEST(ProbabilitySampler, ShouldSampleWithContextNoDeferParent) { - ProbabilitySampler s1(0.01, false); - ProbabilitySampler s2(0.01, false, SamplingBehavior::ALL_SPANS); - ProbabilitySampler s3(0.01, false, SamplingBehavior::ROOT_SPANS_ONLY); - opentelemetry::trace::TraceId trace_id; opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; opentelemetry::sdk::trace::Sampler::SpanContext c1(false, false); @@ -159,6 +162,8 @@ TEST(ProbabilitySampler, ShouldSampleWithContextNoDeferParent) // [DEFAULT] SamplingBehavior::DETACHED_SPANS_ONLY + ProbabilitySampler s1(0.01, false); + auto sampling_result = s1.ShouldSample(&c1, trace_id, "", span_kind, view); ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); @@ -181,6 +186,8 @@ TEST(ProbabilitySampler, ShouldSampleWithContextNoDeferParent) // SamplingBehavior::ALL_SPANS + ProbabilitySampler s2(0.01, false, SamplingBehavior::ALL_SPANS); + sampling_result = s2.ShouldSample(&c1, trace_id, "", span_kind, view); ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); @@ -203,6 +210,8 @@ TEST(ProbabilitySampler, ShouldSampleWithContextNoDeferParent) // SamplingBehavior::ROOT_SPANS_ONLY + ProbabilitySampler s3(0.01, false, SamplingBehavior::ROOT_SPANS_ONLY); + sampling_result = s3.ShouldSample(&c1, trace_id, "", span_kind, view); ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); @@ -243,4 +252,4 @@ TEST(ProbabilitySampler, GetDescription) ProbabilitySampler s6(0.102030405); ASSERT_EQ("ProbabilitySampler{0.102030}", s6.GetDescription()); -} +} \ No newline at end of file From a52cc952f844d81929fb7168a3f5861170a274f8 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 1 Jul 2020 08:51:29 -0400 Subject: [PATCH 10/34] Fix function to remove unnecessary copying of values --- sdk/src/trace/probability_sampler.cc | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/sdk/src/trace/probability_sampler.cc b/sdk/src/trace/probability_sampler.cc index 2f62133f0b..e0db39b369 100644 --- a/sdk/src/trace/probability_sampler.cc +++ b/sdk/src/trace/probability_sampler.cc @@ -102,16 +102,10 @@ uint64_t ProbabilitySampler::CalculateThresholdFromBuffer(const trace_api::Trace // We only use the first 8 bytes of TraceId. static_assert(trace_api::TraceId::kSize >= 8, "TraceID must be at least 8 bytes long."); - uint8_t buf[trace_api::TraceId::kSize]; - - for (int i = 0; i < 8; ++i) { - buf[i] = trace_id.Id()[i]; - } - uint64_t res = 0; for (int i = 0; i < 8; ++i) { - res |= (static_cast(buf[i]) << (i * 8)); + res |= (static_cast(trace_id.Id()[i]) << (i * 8)); } return res; } From 4436867c1c1be5a244a4740cd02ca82f4092d472 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 1 Jul 2020 09:02:27 -0400 Subject: [PATCH 11/34] Move probability sampler to correct directory --- .../sdk/trace/{probability_sampler.h => samplers/probability.h} | 0 sdk/src/trace/{probability_sampler.cc => samplers/probability.cc} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename sdk/include/opentelemetry/sdk/trace/{probability_sampler.h => samplers/probability.h} (100%) rename sdk/src/trace/{probability_sampler.cc => samplers/probability.cc} (100%) diff --git a/sdk/include/opentelemetry/sdk/trace/probability_sampler.h b/sdk/include/opentelemetry/sdk/trace/samplers/probability.h similarity index 100% rename from sdk/include/opentelemetry/sdk/trace/probability_sampler.h rename to sdk/include/opentelemetry/sdk/trace/samplers/probability.h diff --git a/sdk/src/trace/probability_sampler.cc b/sdk/src/trace/samplers/probability.cc similarity index 100% rename from sdk/src/trace/probability_sampler.cc rename to sdk/src/trace/samplers/probability.cc From b40793aaac0e796c17bbe46cff186f1566a49978 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 1 Jul 2020 09:07:09 -0400 Subject: [PATCH 12/34] Update sampler import paths --- sdk/src/trace/samplers/probability.cc | 2 +- sdk/test/trace/probability_sampler_test.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/src/trace/samplers/probability.cc b/sdk/src/trace/samplers/probability.cc index e0db39b369..d198157c41 100644 --- a/sdk/src/trace/samplers/probability.cc +++ b/sdk/src/trace/samplers/probability.cc @@ -13,7 +13,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "opentelemetry/sdk/trace/probability_sampler.h" +#include "opentelemetry/sdk/trace/samplers/probability.h" #include #include diff --git a/sdk/test/trace/probability_sampler_test.cc b/sdk/test/trace/probability_sampler_test.cc index c8792370b7..d078531b8e 100644 --- a/sdk/test/trace/probability_sampler_test.cc +++ b/sdk/test/trace/probability_sampler_test.cc @@ -1,4 +1,4 @@ -#include "opentelemetry/sdk/trace/probability_sampler.h" +#include "opentelemetry/sdk/trace/samplers/probability.h" #include From 7cdb78362457e5e99c1780004cf1f1bd848d2051 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 1 Jul 2020 09:17:21 -0400 Subject: [PATCH 13/34] Update comments --- .../opentelemetry/sdk/trace/samplers/probability.h | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/trace/samplers/probability.h b/sdk/include/opentelemetry/sdk/trace/samplers/probability.h index b3ffefbc57..ff1126ed65 100644 --- a/sdk/include/opentelemetry/sdk/trace/samplers/probability.h +++ b/sdk/include/opentelemetry/sdk/trace/samplers/probability.h @@ -41,12 +41,22 @@ enum class SamplingBehavior }; /** - * The probability sampler ... - * + * The probability sampler, based on it's configuration, should either defer the + * decision to sample to it's parent, or compute and return a decision based on + * the provided trace_id and probability. */ class ProbabilitySampler : public Sampler { public: + /** + * @param probability a required value, 1 >= probability >= 0, that given any + * random trace_id, ShouldSample will return RECORD_AND_SAMPLE + * @param defer_parent an optional configuration setting, specifying whether + * this sampler should defer to it's parent in cases where there is a + * parent SpanContext specified, and is not remote + * @param sampling_behavior an optional configuration setting, specifying + * what types of Spans should processed. all other types will return NOT_RECORD + */ explicit ProbabilitySampler(double probability, bool defer_parent = true, SamplingBehavior sampling_behavior = SamplingBehavior::DETACHED_SPANS_ONLY); From d5f0f0540b89e5052c0f5e2bae965edaf230b3b9 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 1 Jul 2020 09:45:47 -0400 Subject: [PATCH 14/34] Update CMakeList --- sdk/src/trace/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/src/trace/CMakeLists.txt b/sdk/src/trace/CMakeLists.txt index 925ee8bd02..1e6d302461 100644 --- a/sdk/src/trace/CMakeLists.txt +++ b/sdk/src/trace/CMakeLists.txt @@ -1,2 +1,2 @@ add_library(opentelemetry_trace tracer_provider.cc tracer.cc span.cc - samplers/parent_or_else.cc probability_sampler.cc) + samplers/parent_or_else.cc samplers/probability.cc) From 12991df808689193d12a3ce02df5c926715ebf09 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 1 Jul 2020 11:21:28 -0400 Subject: [PATCH 15/34] Update comments --- .../sdk/trace/samplers/probability.h | 23 +++++++++++++++---- sdk/src/trace/samplers/probability.cc | 3 --- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/trace/samplers/probability.h b/sdk/include/opentelemetry/sdk/trace/samplers/probability.h index ff1126ed65..78c8d7b4da 100644 --- a/sdk/include/opentelemetry/sdk/trace/samplers/probability.h +++ b/sdk/include/opentelemetry/sdk/trace/samplers/probability.h @@ -61,22 +61,37 @@ class ProbabilitySampler : public Sampler SamplingBehavior sampling_behavior = SamplingBehavior::DETACHED_SPANS_ONLY); /** - * @return Returns NOT_RECORD always + * @return Returns either RECORD_AND_SAMPLE or NOT_RECORD based on current + * sampler configuration and provided parent_context / tracer_id. tracer_id + * is used as a pseudorandom value in conjunction with the predefined + * threshold to determine whether this trace should be sampled */ SamplingResult ShouldSample( const SpanContext *parent_context, trace_api::TraceId trace_id, - nostd::string_view name, - trace_api::SpanKind span_kind, - const trace_api::KeyValueIterable &attributes) noexcept override; + nostd::string_view /*name*/, + trace_api::SpanKind /*span_kind*/, + const trace_api::KeyValueIterable & /*attributes*/) noexcept override; /** * @return Description MUST be ProbabilitySampler{0.000100} */ std::string GetDescription() const noexcept override; + /** + * @param probability a required value top be converted to uint64_t. is + * bounded by 1 >= probability >= 0. + * @return Returns threshold value computed after converting probability to + * uint64_t datatype + */ uint64_t CalculateThreshold(double probability) const noexcept; + /** + * @param trace_id a required value to be converted to uint64_t. trace_id must + * at least 8 bytes long + * @return Returns threshold value computed after converting trace_id to + * uint64_t datatype + */ uint64_t CalculateThresholdFromBuffer(const trace_api::TraceId& trace_id) const noexcept; double GetProbability() const noexcept; diff --git a/sdk/src/trace/samplers/probability.cc b/sdk/src/trace/samplers/probability.cc index d198157c41..c349b77dcd 100644 --- a/sdk/src/trace/samplers/probability.cc +++ b/sdk/src/trace/samplers/probability.cc @@ -72,9 +72,6 @@ SamplingResult ProbabilitySampler::ShouldSample( return { Decision::NOT_RECORD, nullptr }; } -/** - * @return Description MUST be ProbabilitySampler{0.000100} - */ std::string ProbabilitySampler::GetDescription() const noexcept { char buffer[30]; From eb38fb62e701bc4159dfeb1c1c85f62bf0dc45f5 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 1 Jul 2020 13:29:41 -0400 Subject: [PATCH 16/34] Fixed inconsistent spacing --- sdk/src/trace/samplers/probability.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/sdk/src/trace/samplers/probability.cc b/sdk/src/trace/samplers/probability.cc index c349b77dcd..ac976873bc 100644 --- a/sdk/src/trace/samplers/probability.cc +++ b/sdk/src/trace/samplers/probability.cc @@ -50,16 +50,16 @@ SamplingResult ProbabilitySampler::ShouldSample( } // Check if sampling decision should be defered to parent span, if applicable - if (parent_context == nullptr || !defer_parent_) + if (parent_context == nullptr || !defer_parent_) { if (threshold_ == 0) return { Decision::NOT_RECORD, nullptr }; // Calculcate probability based decision based on trace_id and threshold - if (CalculateThresholdFromBuffer(trace_id) <= threshold_) + if (CalculateThresholdFromBuffer(trace_id) <= threshold_) { return { Decision::RECORD_AND_SAMPLE, nullptr }; } - } + } else { // Return sampling decision based on parent span_context config @@ -67,15 +67,15 @@ SamplingResult ProbabilitySampler::ShouldSample( { return { Decision::RECORD_AND_SAMPLE, nullptr }; } - } + } return { Decision::NOT_RECORD, nullptr }; } std::string ProbabilitySampler::GetDescription() const noexcept { - char buffer[30]; - sprintf(buffer, "ProbabilitySampler{%.6f}", GetProbability()); + char buffer[30]; + sprintf(buffer, "ProbabilitySampler{%.6f}", GetProbability()); return std::string(buffer); } From 2db9c1169b57122e2f980dc8af4e13330efca753 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 1 Jul 2020 14:19:53 -0400 Subject: [PATCH 17/34] Move functions to private --- sdk/include/opentelemetry/sdk/trace/samplers/probability.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/include/opentelemetry/sdk/trace/samplers/probability.h b/sdk/include/opentelemetry/sdk/trace/samplers/probability.h index 78c8d7b4da..0237bf79ad 100644 --- a/sdk/include/opentelemetry/sdk/trace/samplers/probability.h +++ b/sdk/include/opentelemetry/sdk/trace/samplers/probability.h @@ -78,6 +78,7 @@ class ProbabilitySampler : public Sampler */ std::string GetDescription() const noexcept override; +private: /** * @param probability a required value top be converted to uint64_t. is * bounded by 1 >= probability >= 0. @@ -96,7 +97,6 @@ class ProbabilitySampler : public Sampler double GetProbability() const noexcept; -private: const uint64_t threshold_; const double probability_; const bool defer_parent_; From 88fdf9651e8b88272eefa579563a1ac4fba5a417 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 1 Jul 2020 14:44:11 -0400 Subject: [PATCH 18/34] Change GetDescription to cache string --- .../opentelemetry/sdk/trace/samplers/probability.h | 1 + sdk/src/trace/samplers/probability.cc | 10 ++++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/trace/samplers/probability.h b/sdk/include/opentelemetry/sdk/trace/samplers/probability.h index 0237bf79ad..6880421330 100644 --- a/sdk/include/opentelemetry/sdk/trace/samplers/probability.h +++ b/sdk/include/opentelemetry/sdk/trace/samplers/probability.h @@ -97,6 +97,7 @@ class ProbabilitySampler : public Sampler double GetProbability() const noexcept; + std::string sampler_description_; const uint64_t threshold_; const double probability_; const bool defer_parent_; diff --git a/sdk/src/trace/samplers/probability.cc b/sdk/src/trace/samplers/probability.cc index ac976873bc..74bb965699 100644 --- a/sdk/src/trace/samplers/probability.cc +++ b/sdk/src/trace/samplers/probability.cc @@ -29,7 +29,11 @@ ProbabilitySampler::ProbabilitySampler( double probability, bool defer_parent, SamplingBehavior sampling_behavior ) : threshold_(CalculateThreshold(probability)), probability_(probability), - defer_parent_(defer_parent), sampling_behavior_(sampling_behavior) {} + defer_parent_(defer_parent), sampling_behavior_(sampling_behavior) { + char buffer[30]; + sprintf(buffer, "ProbabilitySampler{%.6f}", GetProbability()); + sampler_description_ = std::string(buffer); + } SamplingResult ProbabilitySampler::ShouldSample( const SpanContext *parent_context, @@ -74,9 +78,7 @@ SamplingResult ProbabilitySampler::ShouldSample( std::string ProbabilitySampler::GetDescription() const noexcept { - char buffer[30]; - sprintf(buffer, "ProbabilitySampler{%.6f}", GetProbability()); - return std::string(buffer); + return sampler_description_; } uint64_t ProbabilitySampler::CalculateThreshold(double probability) const noexcept From 9c0f807da487e2b56cdc8145775d000bfefe1cfc Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 1 Jul 2020 14:45:10 -0400 Subject: [PATCH 19/34] Modify function to use memcpy --- sdk/src/trace/samplers/probability.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sdk/src/trace/samplers/probability.cc b/sdk/src/trace/samplers/probability.cc index 74bb965699..c108537f10 100644 --- a/sdk/src/trace/samplers/probability.cc +++ b/sdk/src/trace/samplers/probability.cc @@ -102,10 +102,8 @@ uint64_t ProbabilitySampler::CalculateThresholdFromBuffer(const trace_api::Trace static_assert(trace_api::TraceId::kSize >= 8, "TraceID must be at least 8 bytes long."); uint64_t res = 0; + std::memcpy(&res, &trace_id, 8); - for (int i = 0; i < 8; ++i) { - res |= (static_cast(trace_id.Id()[i]) << (i * 8)); - } return res; } From 482cf3543fd8204953be17260ab9b0222ce2c9b1 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 1 Jul 2020 16:59:52 -0400 Subject: [PATCH 20/34] Add proper error handling for invalid arguments --- sdk/src/trace/samplers/probability.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/sdk/src/trace/samplers/probability.cc b/sdk/src/trace/samplers/probability.cc index c108537f10..e2e1b07145 100644 --- a/sdk/src/trace/samplers/probability.cc +++ b/sdk/src/trace/samplers/probability.cc @@ -28,8 +28,14 @@ namespace trace_api = opentelemetry::trace; ProbabilitySampler::ProbabilitySampler( double probability, bool defer_parent, SamplingBehavior sampling_behavior ) -: threshold_(CalculateThreshold(probability)), probability_(probability), +: threshold_(CalculateThreshold(probability)), defer_parent_(defer_parent), sampling_behavior_(sampling_behavior) { + if (probability > 1 || probability < 0) { + throws std::invalid_argument("Invalid value received for probability. Must fall within bounds [1, 0].") + } + + probability_ = probability; + char buffer[30]; sprintf(buffer, "ProbabilitySampler{%.6f}", GetProbability()); sampler_description_ = std::string(buffer); @@ -83,8 +89,6 @@ std::string ProbabilitySampler::GetDescription() const noexcept uint64_t ProbabilitySampler::CalculateThreshold(double probability) const noexcept { - if (probability <= 0.0) return 0; - if (probability >= 1.0) return UINT64_MAX; // We can't directly return probability * UINT64_MAX. // // UINT64_MAX is (2^64)-1, but as a double rounds up to 2^64. From 3586063e2ce5ce66a5fa4d04061189713593514d Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 1 Jul 2020 19:01:40 -0400 Subject: [PATCH 21/34] Remove optional config --- .../sdk/trace/samplers/probability.h | 33 +--- sdk/src/trace/samplers/probability.cc | 58 ++---- sdk/test/trace/probability_sampler_test.cc | 186 ++---------------- 3 files changed, 32 insertions(+), 245 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/trace/samplers/probability.h b/sdk/include/opentelemetry/sdk/trace/samplers/probability.h index 6880421330..f4fc118eb3 100644 --- a/sdk/include/opentelemetry/sdk/trace/samplers/probability.h +++ b/sdk/include/opentelemetry/sdk/trace/samplers/probability.h @@ -23,23 +23,6 @@ namespace sdk namespace trace { namespace trace_api = opentelemetry::trace; - -/** The default behavior is to apply the sampling probability only for Spans - * that are root spans (no parent) and Spans with remote parent. However - * there should be configuration to change this to "root spans only", - * or "all spans". - */ -enum class SamplingBehavior -{ - // Root Spans (no parent) and Spans with a remote parent - DETACHED_SPANS_ONLY, - // Root Spans (no parent) only - ROOT_SPANS_ONLY, - // All Spans: Root Spans (no parent), Spans with a remote parent, and - // Spans with a direct parent - ALL_SPANS -}; - /** * The probability sampler, based on it's configuration, should either defer the * decision to sample to it's parent, or compute and return a decision based on @@ -49,16 +32,11 @@ class ProbabilitySampler : public Sampler { public: /** - * @param probability a required value, 1 >= probability >= 0, that given any + * @param probability a required value, 1.0 >= probability >= 0.0, that given any * random trace_id, ShouldSample will return RECORD_AND_SAMPLE - * @param defer_parent an optional configuration setting, specifying whether - * this sampler should defer to it's parent in cases where there is a - * parent SpanContext specified, and is not remote - * @param sampling_behavior an optional configuration setting, specifying - * what types of Spans should processed. all other types will return NOT_RECORD + * @throws invalid_argument if probability is out of bounds [0.0, 1.0] */ - explicit ProbabilitySampler(double probability, bool defer_parent = true, - SamplingBehavior sampling_behavior = SamplingBehavior::DETACHED_SPANS_ONLY); + explicit ProbabilitySampler(double probability); /** * @return Returns either RECORD_AND_SAMPLE or NOT_RECORD based on current @@ -95,13 +73,8 @@ class ProbabilitySampler : public Sampler */ uint64_t CalculateThresholdFromBuffer(const trace_api::TraceId& trace_id) const noexcept; - double GetProbability() const noexcept; - std::string sampler_description_; const uint64_t threshold_; - const double probability_; - const bool defer_parent_; - const SamplingBehavior sampling_behavior_; }; } // namespace trace } // namespace sdk diff --git a/sdk/src/trace/samplers/probability.cc b/sdk/src/trace/samplers/probability.cc index e2e1b07145..61504ebc01 100644 --- a/sdk/src/trace/samplers/probability.cc +++ b/sdk/src/trace/samplers/probability.cc @@ -17,6 +17,7 @@ #include #include +#include OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk @@ -25,19 +26,15 @@ namespace trace { namespace trace_api = opentelemetry::trace; -ProbabilitySampler::ProbabilitySampler( - double probability, bool defer_parent, SamplingBehavior sampling_behavior -) -: threshold_(CalculateThreshold(probability)), - defer_parent_(defer_parent), sampling_behavior_(sampling_behavior) { - if (probability > 1 || probability < 0) { - throws std::invalid_argument("Invalid value received for probability. Must fall within bounds [1, 0].") +ProbabilitySampler::ProbabilitySampler(double probability) +: threshold_(CalculateThreshold(probability)) +{ + if (probability > 1.0 || probability < 0) { + throw std::invalid_argument("Invalid value received for probability. Must fall within bounds [1, 0]."); } - probability_ = probability; - char buffer[30]; - sprintf(buffer, "ProbabilitySampler{%.6f}", GetProbability()); + sprintf(buffer, "ProbabilitySampler{%.6f}", probability); sampler_description_ = std::string(buffer); } @@ -48,35 +45,19 @@ SamplingResult ProbabilitySampler::ShouldSample( trace_api::SpanKind span_kind, const trace_api::KeyValueIterable &attributes) noexcept { - // Check if current Span should be sampled with current Sampling Behavior - if (sampling_behavior_ == SamplingBehavior::DETACHED_SPANS_ONLY - && parent_context != nullptr && !parent_context->is_remote) { - return { Decision::NOT_RECORD, nullptr }; - } - - if (sampling_behavior_ == SamplingBehavior::ROOT_SPANS_ONLY - && parent_context != nullptr) { - return { Decision::NOT_RECORD, nullptr }; - } - - // Check if sampling decision should be defered to parent span, if applicable - if (parent_context == nullptr || !defer_parent_) - { - if (threshold_ == 0) return { Decision::NOT_RECORD, nullptr }; - - // Calculcate probability based decision based on trace_id and threshold - if (CalculateThresholdFromBuffer(trace_id) <= threshold_) - { + if (parent_context && !parent_context->is_remote) { + if (parent_context->sampled) { return { Decision::RECORD_AND_SAMPLE, nullptr }; + } else { + return { Decision::NOT_RECORD, nullptr }; } } - else + + if (threshold_ == 0) return { Decision::NOT_RECORD, nullptr }; + + if (CalculateThresholdFromBuffer(trace_id) <= threshold_) { - // Return sampling decision based on parent span_context config - if (parent_context->sampled) - { - return { Decision::RECORD_AND_SAMPLE, nullptr }; - } + return { Decision::RECORD_AND_SAMPLE, nullptr }; } return { Decision::NOT_RECORD, nullptr }; @@ -110,13 +91,6 @@ uint64_t ProbabilitySampler::CalculateThresholdFromBuffer(const trace_api::Trace return res; } - -double ProbabilitySampler::GetProbability() const noexcept -{ - if (probability_ <= 0.0) return 0; - if (probability_ >= 1.0) return 1.0; - return probability_; -} } // namespace trace } // namespace sdk OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/test/trace/probability_sampler_test.cc b/sdk/test/trace/probability_sampler_test.cc index d078531b8e..1fc2fdc2cd 100644 --- a/sdk/test/trace/probability_sampler_test.cc +++ b/sdk/test/trace/probability_sampler_test.cc @@ -4,7 +4,6 @@ using opentelemetry::sdk::trace::ProbabilitySampler; using opentelemetry::sdk::trace::Decision; -using opentelemetry::sdk::trace::SamplingBehavior; TEST(ProbabilitySampler, ShouldSampleWithoutContext) { @@ -16,8 +15,6 @@ TEST(ProbabilitySampler, ShouldSampleWithoutContext) M m1 = {{}}; opentelemetry::trace::KeyValueIterableView view{m1}; - // [DEFAULT] SamplingBehavior::DETACHED_SPANS_ONLY - ProbabilitySampler s1(0.01); auto sampling_result = s1.ShouldSample(nullptr, invalid_trace_id, "", span_kind, view); @@ -25,24 +22,6 @@ TEST(ProbabilitySampler, ShouldSampleWithoutContext) ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); ASSERT_EQ(nullptr, sampling_result.attributes); - // SamplingBehavior::ALL_SPANS - - ProbabilitySampler s2(0.01, false, SamplingBehavior::ALL_SPANS); - - sampling_result = s2.ShouldSample(nullptr, invalid_trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - // SamplingBehavior::ROOT_SPANS_ONLY - - ProbabilitySampler s3(0.01, false, SamplingBehavior::ROOT_SPANS_ONLY); - - sampling_result = s3.ShouldSample(nullptr, invalid_trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - constexpr uint8_t buf[] = {1, 2, 3, 4, 5, 6, 7, 8, 8, 7, 6, 5, 4, 3, 2, 1}; opentelemetry::trace::TraceId valid_trace_id(buf); @@ -51,32 +30,28 @@ TEST(ProbabilitySampler, ShouldSampleWithoutContext) ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); ASSERT_EQ(nullptr, sampling_result.attributes); - ProbabilitySampler s4(0.123457); - - // [DEFAULT] SamplingBehavior::DETACHED_SPANS_ONLY + ProbabilitySampler s2(0.123457); - sampling_result = s4.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); + sampling_result = s2.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); ASSERT_EQ(nullptr, sampling_result.attributes); } -TEST(ProbabilitySampler, ShouldSampleWithContextDeferParent) +TEST(ProbabilitySampler, ShouldSampleWithContext) { opentelemetry::trace::TraceId trace_id; opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; opentelemetry::sdk::trace::Sampler::SpanContext c1(false, false); opentelemetry::sdk::trace::Sampler::SpanContext c2(true, false); - opentelemetry::sdk::trace::Sampler::SpanContext c3(false, false); - opentelemetry::sdk::trace::Sampler::SpanContext c4(true, false); + opentelemetry::sdk::trace::Sampler::SpanContext c3(false, true); + opentelemetry::sdk::trace::Sampler::SpanContext c4(true, true); using M = std::map; M m1 = {{}}; opentelemetry::trace::KeyValueIterableView view{m1}; - // [DEFAULT] SamplingBehavior::DETACHED_SPANS_ONLY - - ProbabilitySampler s1(0.01, true); + ProbabilitySampler s1(0.01); auto sampling_result = s1.ShouldSample(&c1, trace_id, "", span_kind, view); @@ -85,152 +60,18 @@ TEST(ProbabilitySampler, ShouldSampleWithContextDeferParent) sampling_result = s1.ShouldSample(&c2, trace_id, "", span_kind, view); - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - sampling_result = s1.ShouldSample(&c3, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - sampling_result = s1.ShouldSample(&c4, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - // SamplingBehavior::ALL_SPANS - - ProbabilitySampler s2(0.01, true, SamplingBehavior::ALL_SPANS); - - sampling_result = s2.ShouldSample(&c1, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - sampling_result = s2.ShouldSample(&c2, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - sampling_result = s2.ShouldSample(&c3, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - sampling_result = s2.ShouldSample(&c4, trace_id, "", span_kind, view); - ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); ASSERT_EQ(nullptr, sampling_result.attributes); - // SamplingBehavior::ROOT_SPANS_ONLY - - ProbabilitySampler s3(0.01, true, SamplingBehavior::ROOT_SPANS_ONLY); - - sampling_result = s3.ShouldSample(&c1, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - sampling_result = s3.ShouldSample(&c2, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - sampling_result = s3.ShouldSample(&c3, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - sampling_result = s3.ShouldSample(&c4, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); -} - -TEST(ProbabilitySampler, ShouldSampleWithContextNoDeferParent) -{ - opentelemetry::trace::TraceId trace_id; - opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; - opentelemetry::sdk::trace::Sampler::SpanContext c1(false, false); - opentelemetry::sdk::trace::Sampler::SpanContext c2(true, false); - opentelemetry::sdk::trace::Sampler::SpanContext c3(false, false); - opentelemetry::sdk::trace::Sampler::SpanContext c4(true, false); - - using M = std::map; - M m1 = {{}}; - opentelemetry::trace::KeyValueIterableView view{m1}; - - // [DEFAULT] SamplingBehavior::DETACHED_SPANS_ONLY - - ProbabilitySampler s1(0.01, false); - - auto sampling_result = s1.ShouldSample(&c1, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - sampling_result = s1.ShouldSample(&c2, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - sampling_result = s1.ShouldSample(&c3, trace_id, "", span_kind, view); - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - sampling_result = s1.ShouldSample(&c4, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - // SamplingBehavior::ALL_SPANS - - ProbabilitySampler s2(0.01, false, SamplingBehavior::ALL_SPANS); - - sampling_result = s2.ShouldSample(&c1, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - sampling_result = s2.ShouldSample(&c2, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - sampling_result = s2.ShouldSample(&c3, trace_id, "", span_kind, view); - ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); ASSERT_EQ(nullptr, sampling_result.attributes); - sampling_result = s2.ShouldSample(&c4, trace_id, "", span_kind, view); + sampling_result = s1.ShouldSample(&c4, trace_id, "", span_kind, view); ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); ASSERT_EQ(nullptr, sampling_result.attributes); - - // SamplingBehavior::ROOT_SPANS_ONLY - - ProbabilitySampler s3(0.01, false, SamplingBehavior::ROOT_SPANS_ONLY); - - sampling_result = s3.ShouldSample(&c1, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - sampling_result = s3.ShouldSample(&c2, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - sampling_result = s3.ShouldSample(&c3, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); - - sampling_result = s3.ShouldSample(&c4, trace_id, "", span_kind, view); - - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); } TEST(ProbabilitySampler, GetDescription) @@ -244,12 +85,11 @@ TEST(ProbabilitySampler, GetDescription) ProbabilitySampler s3(1.00); ASSERT_EQ("ProbabilitySampler{1.000000}", s3.GetDescription()); - ProbabilitySampler s4(3.00); - ASSERT_EQ("ProbabilitySampler{1.000000}", s4.GetDescription()); - - ProbabilitySampler s5(-3.00); - ASSERT_EQ("ProbabilitySampler{0.000000}", s5.GetDescription()); + ProbabilitySampler s4(0.102030405); + ASSERT_EQ("ProbabilitySampler{0.102030}", s4.GetDescription()); - ProbabilitySampler s6(0.102030405); - ASSERT_EQ("ProbabilitySampler{0.102030}", s6.GetDescription()); + ASSERT_THROW(ProbabilitySampler(3.00), std::invalid_argument); + ASSERT_THROW(ProbabilitySampler(-3.00), std::invalid_argument); + ASSERT_THROW(ProbabilitySampler(1.000000001), std::invalid_argument); + ASSERT_THROW(ProbabilitySampler(-1.000000001), std::invalid_argument); } \ No newline at end of file From 42da2e7bdb363653fae94d16826907fa377ab2b7 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Mon, 6 Jul 2020 07:54:41 -0600 Subject: [PATCH 22/34] Remove constructor exception --- sdk/src/trace/samplers/probability.cc | 9 +++++---- sdk/test/trace/probability_sampler_test.cc | 19 +++++++++++++------ 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/sdk/src/trace/samplers/probability.cc b/sdk/src/trace/samplers/probability.cc index 61504ebc01..f7919fc0a3 100644 --- a/sdk/src/trace/samplers/probability.cc +++ b/sdk/src/trace/samplers/probability.cc @@ -29,11 +29,9 @@ namespace trace_api = opentelemetry::trace; ProbabilitySampler::ProbabilitySampler(double probability) : threshold_(CalculateThreshold(probability)) { - if (probability > 1.0 || probability < 0) { - throw std::invalid_argument("Invalid value received for probability. Must fall within bounds [1, 0]."); - } - char buffer[30]; + if (probability > 1.0) probability = 1.0; + if (probability < 0.0) probability = 0.0; sprintf(buffer, "ProbabilitySampler{%.6f}", probability); sampler_description_ = std::string(buffer); } @@ -70,6 +68,9 @@ std::string ProbabilitySampler::GetDescription() const noexcept uint64_t ProbabilitySampler::CalculateThreshold(double probability) const noexcept { + if (probability <= 0.0) return 0; + if (probability >= 1.0) return UINT64_MAX; + // We can't directly return probability * UINT64_MAX. // // UINT64_MAX is (2^64)-1, but as a double rounds up to 2^64. diff --git a/sdk/test/trace/probability_sampler_test.cc b/sdk/test/trace/probability_sampler_test.cc index 1fc2fdc2cd..747ced0e79 100644 --- a/sdk/test/trace/probability_sampler_test.cc +++ b/sdk/test/trace/probability_sampler_test.cc @@ -51,7 +51,7 @@ TEST(ProbabilitySampler, ShouldSampleWithContext) M m1 = {{}}; opentelemetry::trace::KeyValueIterableView view{m1}; - ProbabilitySampler s1(0.01); + ProbabilitySampler s1(0.01); auto sampling_result = s1.ShouldSample(&c1, trace_id, "", span_kind, view); @@ -88,8 +88,15 @@ TEST(ProbabilitySampler, GetDescription) ProbabilitySampler s4(0.102030405); ASSERT_EQ("ProbabilitySampler{0.102030}", s4.GetDescription()); - ASSERT_THROW(ProbabilitySampler(3.00), std::invalid_argument); - ASSERT_THROW(ProbabilitySampler(-3.00), std::invalid_argument); - ASSERT_THROW(ProbabilitySampler(1.000000001), std::invalid_argument); - ASSERT_THROW(ProbabilitySampler(-1.000000001), std::invalid_argument); -} \ No newline at end of file + ProbabilitySampler s5(3.00); + ASSERT_EQ("ProbabilitySampler{1.000000}", s5.GetDescription()); + + ProbabilitySampler s6(-3.00); + ASSERT_EQ("ProbabilitySampler{0.000000}", s6.GetDescription()); + + ProbabilitySampler s7(1.00000000001); + ASSERT_EQ("ProbabilitySampler{1.000000}", s7.GetDescription()); + + ProbabilitySampler s8(-1.00000000001); + ASSERT_EQ("ProbabilitySampler{0.000000}", s8.GetDescription()); +} From 1e52ec83c5795d5d99863dbba09cecd8e26fc806 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 8 Jul 2020 07:49:49 -0600 Subject: [PATCH 23/34] Update comments --- sdk/include/opentelemetry/sdk/trace/samplers/probability.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sdk/include/opentelemetry/sdk/trace/samplers/probability.h b/sdk/include/opentelemetry/sdk/trace/samplers/probability.h index f4fc118eb3..bc518498ea 100644 --- a/sdk/include/opentelemetry/sdk/trace/samplers/probability.h +++ b/sdk/include/opentelemetry/sdk/trace/samplers/probability.h @@ -58,6 +58,8 @@ class ProbabilitySampler : public Sampler private: /** + * Converts a probability in [0, 1] to a threshold in [0, UINT64_MAX] + * * @param probability a required value top be converted to uint64_t. is * bounded by 1 >= probability >= 0. * @return Returns threshold value computed after converting probability to From 81c00e9b8dc3e63ddd37723e4e31946b9aecb730 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 8 Jul 2020 07:50:20 -0600 Subject: [PATCH 24/34] Update description string construction --- sdk/src/trace/samplers/probability.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sdk/src/trace/samplers/probability.cc b/sdk/src/trace/samplers/probability.cc index f7919fc0a3..05a33a7d63 100644 --- a/sdk/src/trace/samplers/probability.cc +++ b/sdk/src/trace/samplers/probability.cc @@ -29,11 +29,9 @@ namespace trace_api = opentelemetry::trace; ProbabilitySampler::ProbabilitySampler(double probability) : threshold_(CalculateThreshold(probability)) { - char buffer[30]; if (probability > 1.0) probability = 1.0; if (probability < 0.0) probability = 0.0; - sprintf(buffer, "ProbabilitySampler{%.6f}", probability); - sampler_description_ = std::string(buffer); + sampler_description_ = "ProbabilitySampler{" + std::to_string(probability) + "}"; } SamplingResult ProbabilitySampler::ShouldSample( From 38350ab03eaabc75080d91f199c88d7eace9b4e9 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 8 Jul 2020 08:10:12 -0600 Subject: [PATCH 25/34] Move functions to anonymous namespace --- .../sdk/trace/samplers/probability.h | 33 -------- sdk/src/trace/samplers/probability.cc | 75 ++++++++++++------- 2 files changed, 46 insertions(+), 62 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/trace/samplers/probability.h b/sdk/include/opentelemetry/sdk/trace/samplers/probability.h index bc518498ea..a6a1838762 100644 --- a/sdk/include/opentelemetry/sdk/trace/samplers/probability.h +++ b/sdk/include/opentelemetry/sdk/trace/samplers/probability.h @@ -1,18 +1,3 @@ -// Copyright 2020, Open Telemetry Authors -// Copyright 2017, OpenCensus Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - #pragma once #include "opentelemetry/sdk/trace/sampler.h" @@ -57,24 +42,6 @@ class ProbabilitySampler : public Sampler std::string GetDescription() const noexcept override; private: - /** - * Converts a probability in [0, 1] to a threshold in [0, UINT64_MAX] - * - * @param probability a required value top be converted to uint64_t. is - * bounded by 1 >= probability >= 0. - * @return Returns threshold value computed after converting probability to - * uint64_t datatype - */ - uint64_t CalculateThreshold(double probability) const noexcept; - - /** - * @param trace_id a required value to be converted to uint64_t. trace_id must - * at least 8 bytes long - * @return Returns threshold value computed after converting trace_id to - * uint64_t datatype - */ - uint64_t CalculateThresholdFromBuffer(const trace_api::TraceId& trace_id) const noexcept; - std::string sampler_description_; const uint64_t threshold_; }; diff --git a/sdk/src/trace/samplers/probability.cc b/sdk/src/trace/samplers/probability.cc index 05a33a7d63..eb58547729 100644 --- a/sdk/src/trace/samplers/probability.cc +++ b/sdk/src/trace/samplers/probability.cc @@ -19,13 +19,57 @@ #include #include +namespace trace_api = opentelemetry::trace; + +namespace +{ + /** + * Converts a probability in [0, 1] to a threshold in [0, UINT64_MAX] + * + * @param probability a required value top be converted to uint64_t. is + * bounded by 1 >= probability >= 0. + * @return Returns threshold value computed after converting probability to + * uint64_t datatype + */ + uint64_t CalculateThreshold(double probability) noexcept + { + if (probability <= 0.0) return 0; + if (probability >= 1.0) return UINT64_MAX; + + // We can't directly return probability * UINT64_MAX. + // + // UINT64_MAX is (2^64)-1, but as a double rounds up to 2^64. + // For probabilities >= 1-(2^-54), the product wraps to zero! + // Instead, calculate the high and low 32 bits separately. + const double product = UINT32_MAX * probability; + double hi_bits, lo_bits = ldexp(modf(product, &hi_bits), 32) + product; + return (static_cast(hi_bits) << 32) + + static_cast(lo_bits); + } + + /** + * @param trace_id a required value to be converted to uint64_t. trace_id must + * at least 8 bytes long + * @return Returns threshold value computed after converting trace_id to + * uint64_t datatype + */ + uint64_t CalculateThresholdFromBuffer(const trace_api::TraceId& trace_id) noexcept + { + // We only use the first 8 bytes of TraceId. + static_assert(trace_api::TraceId::kSize >= 8, "TraceID must be at least 8 bytes long."); + + uint64_t res = 0; + std::memcpy(&res, &trace_id, 8); + + return res; + } +} // namespace + OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk { namespace trace { -namespace trace_api = opentelemetry::trace; - ProbabilitySampler::ProbabilitySampler(double probability) : threshold_(CalculateThreshold(probability)) { @@ -63,33 +107,6 @@ std::string ProbabilitySampler::GetDescription() const noexcept { return sampler_description_; } - -uint64_t ProbabilitySampler::CalculateThreshold(double probability) const noexcept -{ - if (probability <= 0.0) return 0; - if (probability >= 1.0) return UINT64_MAX; - - // We can't directly return probability * UINT64_MAX. - // - // UINT64_MAX is (2^64)-1, but as a double rounds up to 2^64. - // For probabilities >= 1-(2^-54), the product wraps to zero! - // Instead, calculate the high and low 32 bits separately. - const double product = UINT32_MAX * probability; - double hi_bits, lo_bits = ldexp(modf(product, &hi_bits), 32) + product; - return (static_cast(hi_bits) << 32) + - static_cast(lo_bits); -} - -uint64_t ProbabilitySampler::CalculateThresholdFromBuffer(const trace_api::TraceId& trace_id) const noexcept -{ - // We only use the first 8 bytes of TraceId. - static_assert(trace_api::TraceId::kSize >= 8, "TraceID must be at least 8 bytes long."); - - uint64_t res = 0; - std::memcpy(&res, &trace_id, 8); - - return res; -} } // namespace trace } // namespace sdk OPENTELEMETRY_END_NAMESPACE From ca91bf9241816646484da44ccda9419d5c8bb4c7 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 8 Jul 2020 10:24:54 -0600 Subject: [PATCH 26/34] Update function to calculate threshold from buffer correctly --- sdk/src/trace/samplers/probability.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/sdk/src/trace/samplers/probability.cc b/sdk/src/trace/samplers/probability.cc index eb58547729..079d20f3bf 100644 --- a/sdk/src/trace/samplers/probability.cc +++ b/sdk/src/trace/samplers/probability.cc @@ -60,8 +60,10 @@ namespace uint64_t res = 0; std::memcpy(&res, &trace_id, 8); - - return res; + + double probability = (double) res / UINT64_MAX; + + return CalculateThreshold(probability); } } // namespace From 13f96399ec2e885bdf30de0d082ca9af39c8fb52 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 8 Jul 2020 10:25:28 -0600 Subject: [PATCH 27/34] Add unit tests to check probability --- sdk/test/trace/probability_sampler_test.cc | 169 ++++++++++++++++++++- 1 file changed, 167 insertions(+), 2 deletions(-) diff --git a/sdk/test/trace/probability_sampler_test.cc b/sdk/test/trace/probability_sampler_test.cc index 747ced0e79..4f03110166 100644 --- a/sdk/test/trace/probability_sampler_test.cc +++ b/sdk/test/trace/probability_sampler_test.cc @@ -1,6 +1,8 @@ #include "opentelemetry/sdk/trace/samplers/probability.h" #include +#include +#include using opentelemetry::sdk::trace::ProbabilitySampler; using opentelemetry::sdk::trace::Decision; @@ -22,7 +24,7 @@ TEST(ProbabilitySampler, ShouldSampleWithoutContext) ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); ASSERT_EQ(nullptr, sampling_result.attributes); - constexpr uint8_t buf[] = {1, 2, 3, 4, 5, 6, 7, 8, 8, 7, 6, 5, 4, 3, 2, 1}; + constexpr uint8_t buf[] = {0, 0, 0, 0, 0, 0, 0, 0x80, 0, 0, 0, 0, 0, 0, 0, 0}; opentelemetry::trace::TraceId valid_trace_id(buf); sampling_result = s1.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); @@ -30,12 +32,26 @@ TEST(ProbabilitySampler, ShouldSampleWithoutContext) ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); ASSERT_EQ(nullptr, sampling_result.attributes); - ProbabilitySampler s2(0.123457); + ProbabilitySampler s2(0.50000001); sampling_result = s2.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); ASSERT_EQ(nullptr, sampling_result.attributes); + + ProbabilitySampler s3(0.49999999); + + sampling_result = s3.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); + + ProbabilitySampler s4(0.50000000); + + sampling_result = s4.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); + + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); } TEST(ProbabilitySampler, ShouldSampleWithContext) @@ -74,6 +90,152 @@ TEST(ProbabilitySampler, ShouldSampleWithContext) ASSERT_EQ(nullptr, sampling_result.attributes); } +TEST(ProbabilitySampler, ProbabilitySamplerHalf) +{ + double probability = 0.001; + int iterations = 100000, expected_count = iterations * probability, + actual_count = 0, variance = 1000; + + opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); + ProbabilitySampler s(probability); + opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; + + using M = std::map; + M m1 = {{}}; + opentelemetry::trace::KeyValueIterableView view{m1}; + + srand(time(0)); + + for (int i = 0; i < iterations; ++i) + { + uint8_t buf[16]; + for (int j = 0; j < 16; ++j) + { + buf[j] = (std::rand() % 255) + 1; + } + + opentelemetry::trace::TraceId trace_id(buf); + + uint64_t r = 0; + std::memcpy(&r, &trace_id, 8); + + if (s.ShouldSample(&c, trace_id, "", span_kind, view).decision == Decision::RECORD_AND_SAMPLE) + { ++actual_count; } + } + + ASSERT_TRUE(actual_count < (expected_count + variance)); + ASSERT_TRUE(actual_count > (expected_count - variance)); +} + +TEST(ProbabilitySampler, ProbabilitySamplerOnePercent) +{ + double probability = 0.01; + int iterations = 100000, expected_count = iterations * probability, + actual_count = 0, variance = 1000; + + opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); + ProbabilitySampler s(probability); + opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; + + using M = std::map; + M m1 = {{}}; + opentelemetry::trace::KeyValueIterableView view{m1}; + + srand(time(0)); + + for (int i = 0; i < iterations; ++i) + { + uint8_t buf[16]; + for (int j = 0; j < 16; ++j) + { + buf[j] = (std::rand() % 255) + 1; + } + + opentelemetry::trace::TraceId trace_id(buf); + + uint64_t r = 0; + std::memcpy(&r, &trace_id, 8); + + if (s.ShouldSample(&c, trace_id, "", span_kind, view).decision == Decision::RECORD_AND_SAMPLE) + { ++actual_count; } + } + + ASSERT_TRUE(actual_count < (expected_count + variance)); + ASSERT_TRUE(actual_count > (expected_count - variance)); +} + +TEST(ProbabilitySampler, ProbabilitySamplerAll) +{ + double probability = 1.0; + int iterations = 100000, expected_count = iterations * probability, + actual_count = 0; + + opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); + ProbabilitySampler s(probability); + opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; + + using M = std::map; + M m1 = {{}}; + opentelemetry::trace::KeyValueIterableView view{m1}; + + srand(time(0)); + + for (int i = 0; i < iterations; ++i) + { + uint8_t buf[16]; + for (int j = 0; j < 16; ++j) + { + buf[j] = (std::rand() % 255) + 1; + } + + opentelemetry::trace::TraceId trace_id(buf); + + uint64_t r = 0; + std::memcpy(&r, &trace_id, 8); + + if (s.ShouldSample(&c, trace_id, "", span_kind, view).decision == Decision::RECORD_AND_SAMPLE) + { ++actual_count; } + } + + ASSERT_EQ(actual_count, expected_count); +} + +TEST(ProbabilitySampler, ProbabilitySamplerNone) +{ + double probability = 0.0; + int iterations = 100000, expected_count = iterations * probability, + actual_count = 0; + + opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); + ProbabilitySampler s(probability); + opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; + + using M = std::map; + M m1 = {{}}; + opentelemetry::trace::KeyValueIterableView view{m1}; + + srand(time(0)); + + for (int i = 0; i < iterations; ++i) + { + uint8_t buf[16]; + for (int j = 0; j < 16; ++j) + { + buf[j] = (std::rand() % 255) + 1; + } + + opentelemetry::trace::TraceId trace_id(buf); + + uint64_t r = 0; + std::memcpy(&r, &trace_id, 8); + + if (s.ShouldSample(&c, trace_id, "", span_kind, view).decision == Decision::RECORD_AND_SAMPLE) + { ++actual_count; } + } + + ASSERT_EQ(actual_count, expected_count); +} + TEST(ProbabilitySampler, GetDescription) { ProbabilitySampler s1(0.01); @@ -99,4 +261,7 @@ TEST(ProbabilitySampler, GetDescription) ProbabilitySampler s8(-1.00000000001); ASSERT_EQ("ProbabilitySampler{0.000000}", s8.GetDescription()); + + ProbabilitySampler s9(0.50); + ASSERT_EQ("ProbabilitySampler{0.500000}", s9.GetDescription()); } From 363f3b828d5d12866179dbc0bdae4cc91569274c Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 8 Jul 2020 13:49:33 -0600 Subject: [PATCH 28/34] Update probability unit tests --- sdk/test/trace/BUILD | 1 + sdk/test/trace/probability_sampler_test.cc | 142 +++++++-------------- 2 files changed, 44 insertions(+), 99 deletions(-) diff --git a/sdk/test/trace/BUILD b/sdk/test/trace/BUILD index bef910e9e5..280acc4fe3 100644 --- a/sdk/test/trace/BUILD +++ b/sdk/test/trace/BUILD @@ -82,6 +82,7 @@ cc_test( ], deps = [ "//sdk/src/trace", + "//sdk/src/common:random", "@com_google_googletest//:gtest_main", ], ) diff --git a/sdk/test/trace/probability_sampler_test.cc b/sdk/test/trace/probability_sampler_test.cc index 4f03110166..2053a8df94 100644 --- a/sdk/test/trace/probability_sampler_test.cc +++ b/sdk/test/trace/probability_sampler_test.cc @@ -1,4 +1,5 @@ #include "opentelemetry/sdk/trace/samplers/probability.h" +#include "src/common/random.h" #include #include @@ -6,6 +7,39 @@ using opentelemetry::sdk::trace::ProbabilitySampler; using opentelemetry::sdk::trace::Decision; +using opentelemetry::sdk::common::Random; + +namespace +{ + int RunShouldSampleCountDecision( + opentelemetry::sdk::trace::Sampler::SpanContext& context, + ProbabilitySampler& sampler, int iterations) + { + int actual_count = 0; + + opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; + + using M = std::map; + M m1 = {{}}; + opentelemetry::trace::KeyValueIterableView view{m1}; + + srand(time(0)); + + for (int i = 0; i < iterations; ++i) + { + uint8_t buf[16] = {0}; + Random::GenerateRandomBuffer(buf); + + opentelemetry::trace::TraceId trace_id(buf); + + auto result = sampler.ShouldSample(&context, trace_id, "", span_kind, view); + if (result.decision == Decision::RECORD_AND_SAMPLE) + { ++actual_count; } + } + + return actual_count; + } +} TEST(ProbabilitySampler, ShouldSampleWithoutContext) { @@ -92,36 +126,14 @@ TEST(ProbabilitySampler, ShouldSampleWithContext) TEST(ProbabilitySampler, ProbabilitySamplerHalf) { - double probability = 0.001; + double probability = 0.5; int iterations = 100000, expected_count = iterations * probability, - actual_count = 0, variance = 1000; + variance = iterations * 0.01; opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); ProbabilitySampler s(probability); - opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; - - using M = std::map; - M m1 = {{}}; - opentelemetry::trace::KeyValueIterableView view{m1}; - srand(time(0)); - - for (int i = 0; i < iterations; ++i) - { - uint8_t buf[16]; - for (int j = 0; j < 16; ++j) - { - buf[j] = (std::rand() % 255) + 1; - } - - opentelemetry::trace::TraceId trace_id(buf); - - uint64_t r = 0; - std::memcpy(&r, &trace_id, 8); - - if (s.ShouldSample(&c, trace_id, "", span_kind, view).decision == Decision::RECORD_AND_SAMPLE) - { ++actual_count; } - } + int actual_count= RunShouldSampleCountDecision(c, s, iterations); ASSERT_TRUE(actual_count < (expected_count + variance)); ASSERT_TRUE(actual_count > (expected_count - variance)); @@ -131,34 +143,12 @@ TEST(ProbabilitySampler, ProbabilitySamplerOnePercent) { double probability = 0.01; int iterations = 100000, expected_count = iterations * probability, - actual_count = 0, variance = 1000; + variance = iterations * 0.01; opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); ProbabilitySampler s(probability); - opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; - - using M = std::map; - M m1 = {{}}; - opentelemetry::trace::KeyValueIterableView view{m1}; - - srand(time(0)); - - for (int i = 0; i < iterations; ++i) - { - uint8_t buf[16]; - for (int j = 0; j < 16; ++j) - { - buf[j] = (std::rand() % 255) + 1; - } - opentelemetry::trace::TraceId trace_id(buf); - - uint64_t r = 0; - std::memcpy(&r, &trace_id, 8); - - if (s.ShouldSample(&c, trace_id, "", span_kind, view).decision == Decision::RECORD_AND_SAMPLE) - { ++actual_count; } - } + int actual_count= RunShouldSampleCountDecision(c, s, iterations); ASSERT_TRUE(actual_count < (expected_count + variance)); ASSERT_TRUE(actual_count > (expected_count - variance)); @@ -167,35 +157,12 @@ TEST(ProbabilitySampler, ProbabilitySamplerOnePercent) TEST(ProbabilitySampler, ProbabilitySamplerAll) { double probability = 1.0; - int iterations = 100000, expected_count = iterations * probability, - actual_count = 0; + int iterations = 100000, expected_count = iterations * probability; opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); ProbabilitySampler s(probability); - opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; - - using M = std::map; - M m1 = {{}}; - opentelemetry::trace::KeyValueIterableView view{m1}; - - srand(time(0)); - - for (int i = 0; i < iterations; ++i) - { - uint8_t buf[16]; - for (int j = 0; j < 16; ++j) - { - buf[j] = (std::rand() % 255) + 1; - } - opentelemetry::trace::TraceId trace_id(buf); - - uint64_t r = 0; - std::memcpy(&r, &trace_id, 8); - - if (s.ShouldSample(&c, trace_id, "", span_kind, view).decision == Decision::RECORD_AND_SAMPLE) - { ++actual_count; } - } + int actual_count= RunShouldSampleCountDecision(c, s, iterations); ASSERT_EQ(actual_count, expected_count); } @@ -203,35 +170,12 @@ TEST(ProbabilitySampler, ProbabilitySamplerAll) TEST(ProbabilitySampler, ProbabilitySamplerNone) { double probability = 0.0; - int iterations = 100000, expected_count = iterations * probability, - actual_count = 0; + int iterations = 100000, expected_count = iterations * probability; opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); ProbabilitySampler s(probability); - opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; - - using M = std::map; - M m1 = {{}}; - opentelemetry::trace::KeyValueIterableView view{m1}; - srand(time(0)); - - for (int i = 0; i < iterations; ++i) - { - uint8_t buf[16]; - for (int j = 0; j < 16; ++j) - { - buf[j] = (std::rand() % 255) + 1; - } - - opentelemetry::trace::TraceId trace_id(buf); - - uint64_t r = 0; - std::memcpy(&r, &trace_id, 8); - - if (s.ShouldSample(&c, trace_id, "", span_kind, view).decision == Decision::RECORD_AND_SAMPLE) - { ++actual_count; } - } + int actual_count= RunShouldSampleCountDecision(c, s, iterations); ASSERT_EQ(actual_count, expected_count); } From 0211c7dca80ea6856bb5a27461246f11db07c9ef Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 8 Jul 2020 13:55:55 -0600 Subject: [PATCH 29/34] Update function header --- sdk/src/trace/samplers/probability.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sdk/src/trace/samplers/probability.cc b/sdk/src/trace/samplers/probability.cc index 079d20f3bf..0358530dc5 100644 --- a/sdk/src/trace/samplers/probability.cc +++ b/sdk/src/trace/samplers/probability.cc @@ -83,9 +83,9 @@ ProbabilitySampler::ProbabilitySampler(double probability) SamplingResult ProbabilitySampler::ShouldSample( const SpanContext *parent_context, trace_api::TraceId trace_id, - nostd::string_view name, - trace_api::SpanKind span_kind, - const trace_api::KeyValueIterable &attributes) noexcept + nostd::string_view /*name*/, + trace_api::SpanKind /*span_kind*/, + const trace_api::KeyValueIterable & /*attributes*/) noexcept { if (parent_context && !parent_context->is_remote) { if (parent_context->sampled) { From 93c3e981279b5ae065efbfffb26f9dde6a4e645a Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Wed, 8 Jul 2020 14:21:49 -0600 Subject: [PATCH 30/34] Update trace/test CMakeList --- sdk/test/trace/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/test/trace/CMakeLists.txt b/sdk/test/trace/CMakeLists.txt index be3a99719e..8326d62a7b 100644 --- a/sdk/test/trace/CMakeLists.txt +++ b/sdk/test/trace/CMakeLists.txt @@ -3,6 +3,6 @@ foreach(testname tracer_provider_test span_data_test simple_processor_test parent_or_else_sampler_test probability_sampler_test) add_executable(${testname} "${testname}.cc") target_link_libraries(${testname} ${GTEST_BOTH_LIBRARIES} - ${CMAKE_THREAD_LIBS_INIT} opentelemetry_trace) + ${CMAKE_THREAD_LIBS_INIT} opentelemetry_common opentelemetry_trace) gtest_add_tests(TARGET ${testname} TEST_PREFIX trace. TEST_LIST ${testname}) endforeach() From 9348b32352fd4e2b60a0fe238ee5cf0190d06254 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Fri, 10 Jul 2020 07:28:45 -0600 Subject: [PATCH 31/34] Update function description --- sdk/test/trace/probability_sampler_test.cc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/sdk/test/trace/probability_sampler_test.cc b/sdk/test/trace/probability_sampler_test.cc index 2053a8df94..735e498d21 100644 --- a/sdk/test/trace/probability_sampler_test.cc +++ b/sdk/test/trace/probability_sampler_test.cc @@ -11,6 +11,18 @@ using opentelemetry::sdk::common::Random; namespace { + /* + * Helper function for running probability sampler tests. + * Given a span context, sampler, and number of iterations this function + * will return the number of RECORD_AND_SAMPLE decision based on randomly + * generated traces. + * + * @param context a required valid span context + * @param sampler a required valid sampler + * @param iterations a requried number specifying the number of times to + * generate a random trace_id and check if it should sample using the provided + * provider and context + */ int RunShouldSampleCountDecision( opentelemetry::sdk::trace::Sampler::SpanContext& context, ProbabilitySampler& sampler, int iterations) From 289fab9223fd24a1626f43f5c5529f9d78cb30b0 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Fri, 10 Jul 2020 07:51:18 -0600 Subject: [PATCH 32/34] Format probability sampler test --- sdk/test/trace/probability_sampler_test.cc | 272 ++++++++++----------- 1 file changed, 136 insertions(+), 136 deletions(-) diff --git a/sdk/test/trace/probability_sampler_test.cc b/sdk/test/trace/probability_sampler_test.cc index 735e498d21..fa8fb190f1 100644 --- a/sdk/test/trace/probability_sampler_test.cc +++ b/sdk/test/trace/probability_sampler_test.cc @@ -5,219 +5,219 @@ #include #include -using opentelemetry::sdk::trace::ProbabilitySampler; -using opentelemetry::sdk::trace::Decision; using opentelemetry::sdk::common::Random; +using opentelemetry::sdk::trace::Decision; +using opentelemetry::sdk::trace::ProbabilitySampler; namespace { - /* - * Helper function for running probability sampler tests. - * Given a span context, sampler, and number of iterations this function - * will return the number of RECORD_AND_SAMPLE decision based on randomly - * generated traces. - * - * @param context a required valid span context - * @param sampler a required valid sampler - * @param iterations a requried number specifying the number of times to - * generate a random trace_id and check if it should sample using the provided - * provider and context - */ - int RunShouldSampleCountDecision( - opentelemetry::sdk::trace::Sampler::SpanContext& context, - ProbabilitySampler& sampler, int iterations) - { - int actual_count = 0; - - opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; - - using M = std::map; - M m1 = {{}}; - opentelemetry::trace::KeyValueIterableView view{m1}; - - srand(time(0)); - - for (int i = 0; i < iterations; ++i) - { - uint8_t buf[16] = {0}; - Random::GenerateRandomBuffer(buf); - - opentelemetry::trace::TraceId trace_id(buf); - - auto result = sampler.ShouldSample(&context, trace_id, "", span_kind, view); - if (result.decision == Decision::RECORD_AND_SAMPLE) - { ++actual_count; } - } - - return actual_count; - } +/* + * Helper function for running probability sampler tests. + * Given a span context, sampler, and number of iterations this function + * will return the number of RECORD_AND_SAMPLE decision based on randomly + * generated traces. + * + * @param context a required valid span context + * @param sampler a required valid sampler + * @param iterations a requried number specifying the number of times to + * generate a random trace_id and check if it should sample using the provided + * provider and context + */ +int RunShouldSampleCountDecision(opentelemetry::sdk::trace::Sampler::SpanContext &context, + ProbabilitySampler &sampler, + int iterations) +{ + int actual_count = 0; + + opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; + + using M = std::map; + M m1 = {{}}; + opentelemetry::trace::KeyValueIterableView view{m1}; + + srand(time(0)); + + for (int i = 0; i < iterations; ++i) + { + uint8_t buf[16] = {0}; + Random::GenerateRandomBuffer(buf); + + opentelemetry::trace::TraceId trace_id(buf); + + auto result = sampler.ShouldSample(&context, trace_id, "", span_kind, view); + if (result.decision == Decision::RECORD_AND_SAMPLE) + { + ++actual_count; + } + } + + return actual_count; } +} // namespace TEST(ProbabilitySampler, ShouldSampleWithoutContext) { - opentelemetry::trace::TraceId invalid_trace_id; + opentelemetry::trace::TraceId invalid_trace_id; - opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; + opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; - using M = std::map; - M m1 = {{}}; - opentelemetry::trace::KeyValueIterableView view{m1}; + using M = std::map; + M m1 = {{}}; + opentelemetry::trace::KeyValueIterableView view{m1}; - ProbabilitySampler s1(0.01); + ProbabilitySampler s1(0.01); - auto sampling_result = s1.ShouldSample(nullptr, invalid_trace_id, "", span_kind, view); + auto sampling_result = s1.ShouldSample(nullptr, invalid_trace_id, "", span_kind, view); - ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); - constexpr uint8_t buf[] = {0, 0, 0, 0, 0, 0, 0, 0x80, 0, 0, 0, 0, 0, 0, 0, 0}; - opentelemetry::trace::TraceId valid_trace_id(buf); + constexpr uint8_t buf[] = {0, 0, 0, 0, 0, 0, 0, 0x80, 0, 0, 0, 0, 0, 0, 0, 0}; + opentelemetry::trace::TraceId valid_trace_id(buf); - sampling_result = s1.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); + sampling_result = s1.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); - ProbabilitySampler s2(0.50000001); + ProbabilitySampler s2(0.50000001); - sampling_result = s2.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); + sampling_result = s2.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); - ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); - ProbabilitySampler s3(0.49999999); + ProbabilitySampler s3(0.49999999); - sampling_result = s3.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); + sampling_result = s3.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); - ProbabilitySampler s4(0.50000000); + ProbabilitySampler s4(0.50000000); - sampling_result = s4.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); + sampling_result = s4.ShouldSample(nullptr, valid_trace_id, "", span_kind, view); - ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); } TEST(ProbabilitySampler, ShouldSampleWithContext) { - opentelemetry::trace::TraceId trace_id; - opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; - opentelemetry::sdk::trace::Sampler::SpanContext c1(false, false); - opentelemetry::sdk::trace::Sampler::SpanContext c2(true, false); - opentelemetry::sdk::trace::Sampler::SpanContext c3(false, true); - opentelemetry::sdk::trace::Sampler::SpanContext c4(true, true); + opentelemetry::trace::TraceId trace_id; + opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; + opentelemetry::sdk::trace::Sampler::SpanContext c1(false, false); + opentelemetry::sdk::trace::Sampler::SpanContext c2(true, false); + opentelemetry::sdk::trace::Sampler::SpanContext c3(false, true); + opentelemetry::sdk::trace::Sampler::SpanContext c4(true, true); - using M = std::map; - M m1 = {{}}; - opentelemetry::trace::KeyValueIterableView view{m1}; + using M = std::map; + M m1 = {{}}; + opentelemetry::trace::KeyValueIterableView view{m1}; - ProbabilitySampler s1(0.01); + ProbabilitySampler s1(0.01); - auto sampling_result = s1.ShouldSample(&c1, trace_id, "", span_kind, view); + auto sampling_result = s1.ShouldSample(&c1, trace_id, "", span_kind, view); - ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); + ASSERT_EQ(Decision::NOT_RECORD, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); - sampling_result = s1.ShouldSample(&c2, trace_id, "", span_kind, view); + sampling_result = s1.ShouldSample(&c2, trace_id, "", span_kind, view); - ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); - sampling_result = s1.ShouldSample(&c3, trace_id, "", span_kind, view); + sampling_result = s1.ShouldSample(&c3, trace_id, "", span_kind, view); - ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); - sampling_result = s1.ShouldSample(&c4, trace_id, "", span_kind, view); + sampling_result = s1.ShouldSample(&c4, trace_id, "", span_kind, view); - ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); - ASSERT_EQ(nullptr, sampling_result.attributes); + ASSERT_EQ(Decision::RECORD_AND_SAMPLE, sampling_result.decision); + ASSERT_EQ(nullptr, sampling_result.attributes); } TEST(ProbabilitySampler, ProbabilitySamplerHalf) { - double probability = 0.5; - int iterations = 100000, expected_count = iterations * probability, - variance = iterations * 0.01; + double probability = 0.5; + int iterations = 100000, expected_count = iterations * probability, variance = iterations * 0.01; - opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); - ProbabilitySampler s(probability); + opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); + ProbabilitySampler s(probability); - int actual_count= RunShouldSampleCountDecision(c, s, iterations); + int actual_count = RunShouldSampleCountDecision(c, s, iterations); - ASSERT_TRUE(actual_count < (expected_count + variance)); - ASSERT_TRUE(actual_count > (expected_count - variance)); + ASSERT_TRUE(actual_count < (expected_count + variance)); + ASSERT_TRUE(actual_count > (expected_count - variance)); } TEST(ProbabilitySampler, ProbabilitySamplerOnePercent) { - double probability = 0.01; - int iterations = 100000, expected_count = iterations * probability, - variance = iterations * 0.01; + double probability = 0.01; + int iterations = 100000, expected_count = iterations * probability, variance = iterations * 0.01; - opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); - ProbabilitySampler s(probability); + opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); + ProbabilitySampler s(probability); - int actual_count= RunShouldSampleCountDecision(c, s, iterations); + int actual_count = RunShouldSampleCountDecision(c, s, iterations); - ASSERT_TRUE(actual_count < (expected_count + variance)); - ASSERT_TRUE(actual_count > (expected_count - variance)); + ASSERT_TRUE(actual_count < (expected_count + variance)); + ASSERT_TRUE(actual_count > (expected_count - variance)); } TEST(ProbabilitySampler, ProbabilitySamplerAll) { - double probability = 1.0; - int iterations = 100000, expected_count = iterations * probability; + double probability = 1.0; + int iterations = 100000, expected_count = iterations * probability; - opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); - ProbabilitySampler s(probability); + opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); + ProbabilitySampler s(probability); - int actual_count= RunShouldSampleCountDecision(c, s, iterations); + int actual_count = RunShouldSampleCountDecision(c, s, iterations); - ASSERT_EQ(actual_count, expected_count); + ASSERT_EQ(actual_count, expected_count); } TEST(ProbabilitySampler, ProbabilitySamplerNone) { - double probability = 0.0; - int iterations = 100000, expected_count = iterations * probability; + double probability = 0.0; + int iterations = 100000, expected_count = iterations * probability; - opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); - ProbabilitySampler s(probability); + opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); + ProbabilitySampler s(probability); - int actual_count= RunShouldSampleCountDecision(c, s, iterations); + int actual_count = RunShouldSampleCountDecision(c, s, iterations); - ASSERT_EQ(actual_count, expected_count); + ASSERT_EQ(actual_count, expected_count); } TEST(ProbabilitySampler, GetDescription) { - ProbabilitySampler s1(0.01); - ASSERT_EQ("ProbabilitySampler{0.010000}", s1.GetDescription()); + ProbabilitySampler s1(0.01); + ASSERT_EQ("ProbabilitySampler{0.010000}", s1.GetDescription()); - ProbabilitySampler s2(0.00); - ASSERT_EQ("ProbabilitySampler{0.000000}", s2.GetDescription()); + ProbabilitySampler s2(0.00); + ASSERT_EQ("ProbabilitySampler{0.000000}", s2.GetDescription()); - ProbabilitySampler s3(1.00); - ASSERT_EQ("ProbabilitySampler{1.000000}", s3.GetDescription()); + ProbabilitySampler s3(1.00); + ASSERT_EQ("ProbabilitySampler{1.000000}", s3.GetDescription()); - ProbabilitySampler s4(0.102030405); - ASSERT_EQ("ProbabilitySampler{0.102030}", s4.GetDescription()); + ProbabilitySampler s4(0.102030405); + ASSERT_EQ("ProbabilitySampler{0.102030}", s4.GetDescription()); - ProbabilitySampler s5(3.00); - ASSERT_EQ("ProbabilitySampler{1.000000}", s5.GetDescription()); + ProbabilitySampler s5(3.00); + ASSERT_EQ("ProbabilitySampler{1.000000}", s5.GetDescription()); - ProbabilitySampler s6(-3.00); - ASSERT_EQ("ProbabilitySampler{0.000000}", s6.GetDescription()); + ProbabilitySampler s6(-3.00); + ASSERT_EQ("ProbabilitySampler{0.000000}", s6.GetDescription()); - ProbabilitySampler s7(1.00000000001); - ASSERT_EQ("ProbabilitySampler{1.000000}", s7.GetDescription()); + ProbabilitySampler s7(1.00000000001); + ASSERT_EQ("ProbabilitySampler{1.000000}", s7.GetDescription()); - ProbabilitySampler s8(-1.00000000001); - ASSERT_EQ("ProbabilitySampler{0.000000}", s8.GetDescription()); + ProbabilitySampler s8(-1.00000000001); + ASSERT_EQ("ProbabilitySampler{0.000000}", s8.GetDescription()); - ProbabilitySampler s9(0.50); - ASSERT_EQ("ProbabilitySampler{0.500000}", s9.GetDescription()); + ProbabilitySampler s9(0.50); + ASSERT_EQ("ProbabilitySampler{0.500000}", s9.GetDescription()); } From 920404de126c983cf6ccf0cb9b382d9657d4e8b7 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Mon, 13 Jul 2020 16:50:24 -0400 Subject: [PATCH 33/34] Update sampler usage --- .../sdk/trace/samplers/probability.h | 2 +- sdk/src/trace/samplers/probability.cc | 6 +++--- sdk/test/trace/probability_sampler_test.cc | 19 ++++++++++--------- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/trace/samplers/probability.h b/sdk/include/opentelemetry/sdk/trace/samplers/probability.h index a6a1838762..18bca7042a 100644 --- a/sdk/include/opentelemetry/sdk/trace/samplers/probability.h +++ b/sdk/include/opentelemetry/sdk/trace/samplers/probability.h @@ -30,7 +30,7 @@ class ProbabilitySampler : public Sampler * threshold to determine whether this trace should be sampled */ SamplingResult ShouldSample( - const SpanContext *parent_context, + const trace_api::SpanContext *parent_context, trace_api::TraceId trace_id, nostd::string_view /*name*/, trace_api::SpanKind /*span_kind*/, diff --git a/sdk/src/trace/samplers/probability.cc b/sdk/src/trace/samplers/probability.cc index 0358530dc5..2f8c9453cd 100644 --- a/sdk/src/trace/samplers/probability.cc +++ b/sdk/src/trace/samplers/probability.cc @@ -81,14 +81,14 @@ ProbabilitySampler::ProbabilitySampler(double probability) } SamplingResult ProbabilitySampler::ShouldSample( - const SpanContext *parent_context, + const trace_api::SpanContext *parent_context, trace_api::TraceId trace_id, nostd::string_view /*name*/, trace_api::SpanKind /*span_kind*/, const trace_api::KeyValueIterable & /*attributes*/) noexcept { - if (parent_context && !parent_context->is_remote) { - if (parent_context->sampled) { + if (parent_context && !parent_context->HasRemoteParent()) { + if (parent_context->IsSampled()) { return { Decision::RECORD_AND_SAMPLE, nullptr }; } else { return { Decision::NOT_RECORD, nullptr }; diff --git a/sdk/test/trace/probability_sampler_test.cc b/sdk/test/trace/probability_sampler_test.cc index fa8fb190f1..a4cbf8ac70 100644 --- a/sdk/test/trace/probability_sampler_test.cc +++ b/sdk/test/trace/probability_sampler_test.cc @@ -8,6 +8,7 @@ using opentelemetry::sdk::common::Random; using opentelemetry::sdk::trace::Decision; using opentelemetry::sdk::trace::ProbabilitySampler; +using opentelemetry::trace::SpanContext; namespace { @@ -23,7 +24,7 @@ namespace * generate a random trace_id and check if it should sample using the provided * provider and context */ -int RunShouldSampleCountDecision(opentelemetry::sdk::trace::Sampler::SpanContext &context, +int RunShouldSampleCountDecision(SpanContext &context, ProbabilitySampler &sampler, int iterations) { @@ -106,10 +107,10 @@ TEST(ProbabilitySampler, ShouldSampleWithContext) { opentelemetry::trace::TraceId trace_id; opentelemetry::trace::SpanKind span_kind = opentelemetry::trace::SpanKind::kInternal; - opentelemetry::sdk::trace::Sampler::SpanContext c1(false, false); - opentelemetry::sdk::trace::Sampler::SpanContext c2(true, false); - opentelemetry::sdk::trace::Sampler::SpanContext c3(false, true); - opentelemetry::sdk::trace::Sampler::SpanContext c4(true, true); + SpanContext c1(false, false); + SpanContext c2(true, false); + SpanContext c3(false, true); + SpanContext c4(true, true); using M = std::map; M m1 = {{}}; @@ -143,7 +144,7 @@ TEST(ProbabilitySampler, ProbabilitySamplerHalf) double probability = 0.5; int iterations = 100000, expected_count = iterations * probability, variance = iterations * 0.01; - opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); + SpanContext c(true, true); ProbabilitySampler s(probability); int actual_count = RunShouldSampleCountDecision(c, s, iterations); @@ -157,7 +158,7 @@ TEST(ProbabilitySampler, ProbabilitySamplerOnePercent) double probability = 0.01; int iterations = 100000, expected_count = iterations * probability, variance = iterations * 0.01; - opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); + SpanContext c(true, true); ProbabilitySampler s(probability); int actual_count = RunShouldSampleCountDecision(c, s, iterations); @@ -171,7 +172,7 @@ TEST(ProbabilitySampler, ProbabilitySamplerAll) double probability = 1.0; int iterations = 100000, expected_count = iterations * probability; - opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); + SpanContext c(true, true); ProbabilitySampler s(probability); int actual_count = RunShouldSampleCountDecision(c, s, iterations); @@ -184,7 +185,7 @@ TEST(ProbabilitySampler, ProbabilitySamplerNone) double probability = 0.0; int iterations = 100000, expected_count = iterations * probability; - opentelemetry::sdk::trace::Sampler::SpanContext c(true, true); + SpanContext c(true, true); ProbabilitySampler s(probability); int actual_count = RunShouldSampleCountDecision(c, s, iterations); From a788576447c4665506f5f251fbdf000ed533ff50 Mon Sep 17 00:00:00 2001 From: Nick Holbrook Date: Tue, 14 Jul 2020 09:35:57 -0400 Subject: [PATCH 34/34] Remove pseudorandom config from test --- sdk/test/trace/probability_sampler_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/sdk/test/trace/probability_sampler_test.cc b/sdk/test/trace/probability_sampler_test.cc index a4cbf8ac70..37e2a6c943 100644 --- a/sdk/test/trace/probability_sampler_test.cc +++ b/sdk/test/trace/probability_sampler_test.cc @@ -36,8 +36,6 @@ int RunShouldSampleCountDecision(SpanContext &context, M m1 = {{}}; opentelemetry::trace::KeyValueIterableView view{m1}; - srand(time(0)); - for (int i = 0; i < iterations; ++i) { uint8_t buf[16] = {0};