add unit test source code documentation

test/extensions/tracers/opentelemetry/samplers/dynatrace/config_test.cc

@@ -12,6 +12,7 @@ namespace Extensions {
 namespace Tracers {
 namespace OpenTelemetry {
 
+// Test creating a Dynatrace sampler via factory
 TEST(DynatraceSamplerFactoryTest, Test) {
   auto* factory = Registry::FactoryRegistry<SamplerFactory>::getFactory(
       "envoy.tracers.opentelemetry.samplers.dynatrace");
@@ -29,7 +30,6 @@ TEST(DynatraceSamplerFactoryTest, Test) {
 
   NiceMock<Server::Configuration::MockTracerFactoryContext> context;
   EXPECT_NE(factory->createSampler(typed_config.typed_config(), context), nullptr);
-  EXPECT_STREQ(factory->name().c_str(), "envoy.tracers.opentelemetry.samplers.dynatrace");
 }
 
 } // namespace OpenTelemetry

test/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler_integration_test.cc

@@ -60,6 +60,7 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, DynatraceSamplerIntegrationTest,
 
 // Sends a request with traceparent and tracestate header.
 TEST_P(DynatraceSamplerIntegrationTest, TestWithTraceparentAndTracestate) {
+  // tracestate does not contain a Dynatrace tag
   Http::TestRequestHeaderMapImpl request_headers{
       {":method", "GET"},     {":path", "/test/long/url"}, {":scheme", "http"},
       {":authority", "host"}, {"tracestate", "key=value"}, {"traceparent", TRACEPARENT_VALUE}};
@@ -77,12 +78,12 @@ TEST_P(DynatraceSamplerIntegrationTest, TestWithTraceparentAndTracestate) {
                                             .getStringView();
   EXPECT_TRUE(absl::StartsWith(traceparent_value, TRACEPARENT_VALUE_START));
   EXPECT_NE(TRACEPARENT_VALUE, traceparent_value);
-  // tracestate should be forwarded
+  // Dynatrace tracestate should added to existing tracestate
   absl::string_view tracestate_value = upstream_request_->headers()
                                            .get(Http::LowerCaseString("tracestate"))[0]
                                            ->value()
                                            .getStringView();
-  // use StartsWith because pathinfo (last element in trace state contains a random value)
+  // use StartsWith because pathinfo (last element in trace state) contains a random value
   EXPECT_TRUE(absl::StartsWith(tracestate_value, "5b3f9fed-980df25c@dt=fw4;0;0;0;0;0;0;"))
       << "Received tracestate: " << tracestate_value;
   EXPECT_TRUE(absl::StrContains(tracestate_value, ",key=value"))
@@ -109,7 +110,7 @@ TEST_P(DynatraceSamplerIntegrationTest, TestWithTraceparentOnly) {
                                             .getStringView();
   EXPECT_TRUE(absl::StartsWith(traceparent_value, TRACEPARENT_VALUE_START));
   EXPECT_NE(TRACEPARENT_VALUE, traceparent_value);
-  // OTLP tracer adds an empty tracestate
+  // Dynatrace tag should be added to tracestate
   absl::string_view tracestate_value = upstream_request_->headers()
                                            .get(Http::LowerCaseString("tracestate"))[0]
                                            ->value()
@@ -134,7 +135,7 @@ TEST_P(DynatraceSamplerIntegrationTest, TestWithoutTraceparentAndTracestate) {
   // assert
   EXPECT_EQ(upstream_request_->headers().get(::Envoy::Http::LowerCaseString("traceparent")).size(),
             1);
-  // OTLP tracer adds an empty tracestate
+  // Dynatrace tag should be added to tracestate
   absl::string_view tracestate_value = upstream_request_->headers()
                                            .get(Http::LowerCaseString("tracestate"))[0]
                                            ->value()

test/extensions/tracers/opentelemetry/samplers/dynatrace/dynatrace_sampler_test.cc

@@ -85,23 +85,7 @@ TEST_F(DynatraceSamplerTest, TestWithoutParentContext) {
   EXPECT_TRUE(sampling_result.isSampled());
 }
 
-// Verify sampler being invoked with existing Dynatrace trace state tag set
-TEST_F(DynatraceSamplerTest, TestWithParentContext) {
-  SpanContext parent_context = SpanContext("00", trace_id, "b7ad6b7169203331", true,
-                                           "ot=foo:bar,5b3f9fed-980df25c@dt=fw4;0;0;0;0;0;0;ad");
-
-  SamplingResult sampling_result =
-      sampler_->shouldSample(parent_context, trace_id, "parent_span",
-                             ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, {}, {});
-  EXPECT_EQ(sampling_result.decision, Decision::RecordAndSample);
-  EXPECT_EQ(sampling_result.attributes->size(), 1);
-  EXPECT_STREQ(sampling_result.tracestate.c_str(),
-               "ot=foo:bar,5b3f9fed-980df25c@dt=fw4;0;0;0;0;0;0;ad");
-  EXPECT_TRUE(sampling_result.isRecording());
-  EXPECT_TRUE(sampling_result.isSampled());
-}
-
-// Verify sampler being invoked with parent span context
+// Verify sampler being invoked without a Dynatrace tracestate
 TEST_F(DynatraceSamplerTest, TestWithUnknownParentContext) {
   SpanContext parent_context("00", trace_id, parent_span_id, true, "some_vendor=some_value");
 
@@ -110,13 +94,14 @@ TEST_F(DynatraceSamplerTest, TestWithUnknownParentContext) {
                              ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, {}, {});
   EXPECT_EQ(sampling_result.decision, Decision::RecordAndSample);
   EXPECT_EQ(sampling_result.attributes->size(), 1);
+  // Dynatrace tracesate should be prepended
   EXPECT_STREQ(sampling_result.tracestate.c_str(),
                "5b3f9fed-980df25c@dt=fw4;0;0;0;0;0;0;95,some_vendor=some_value");
   EXPECT_TRUE(sampling_result.isRecording());
   EXPECT_TRUE(sampling_result.isSampled());
 }
 
-// Verify sampler being invoked with dynatrace trace parent
+// Verify sampler being invoked with dynatrace trace state
 TEST_F(DynatraceSamplerTest, TestWithDynatraceParentContextSampled) {
   SpanContext parent_context("00", trace_id, parent_span_id, true, dt_tracestate_sampled);
 
@@ -125,7 +110,9 @@ TEST_F(DynatraceSamplerTest, TestWithDynatraceParentContextSampled) {
                              ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, {}, {});
   EXPECT_EQ(sampling_result.decision, Decision::RecordAndSample);
   EXPECT_EQ(sampling_result.attributes->size(), 1);
+  // tracestate should be forwarded
   EXPECT_STREQ(sampling_result.tracestate.c_str(), dt_tracestate_sampled);
+  // sampling decision from parent should be respected
   EXPECT_TRUE(sampling_result.isRecording());
   EXPECT_TRUE(sampling_result.isSampled());
 }
@@ -139,7 +126,9 @@ TEST_F(DynatraceSamplerTest, TestWithDynatraceParentContextIgnored) {
                              ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, {}, {});
   EXPECT_EQ(sampling_result.decision, Decision::Drop);
   EXPECT_EQ(sampling_result.attributes->size(), 1);
+  // tracestate should be forwarded
   EXPECT_STREQ(sampling_result.tracestate.c_str(), dt_tracestate_ignored);
+  // sampling decision from parent should be respected
   EXPECT_FALSE(sampling_result.isRecording());
   EXPECT_FALSE(sampling_result.isSampled());
 }
@@ -155,6 +144,7 @@ TEST_F(DynatraceSamplerTest, TestWithDynatraceParentContextFromDifferentTenant)
   // sampling decision on tracestate should be ignored because it is from a different tenant.
   EXPECT_EQ(sampling_result.decision, Decision::RecordAndSample);
   EXPECT_EQ(sampling_result.attributes->size(), 1);
+  // new Dynatrace tag should be prepended, already existing tag should be kept
   const char* exptected =
       "5b3f9fed-980df25c@dt=fw4;0;0;0;0;0;0;95,6666ad40-980df25c@dt=fw4;4;4af38366;0;0;1;2;123;"
       "8eae;2h01;3h4af38366;4h00;5h01;6h67a9a23155e1741b5b35368e08e6ece5;7h9d83def9a4939b7b";
@@ -192,29 +182,36 @@ TEST_F(DynatraceSamplerTest, TestWarmup) {
                                          trace_context_1, {});
     result.isSampled() ? sampled++ : ignored++;
   }
-  // should be 50, but the used "random" in shouldSample does not produce the same odd/even numbers.
+  // should be 50 ignored, but the used "random" in shouldSample does not produce the same odd/even
+  // numbers.
   EXPECT_EQ(ignored, 41);
   EXPECT_EQ(sampled, 158);
 
+  // send more requests
   for (int i = 0; i < 100; i++) {
     auto result = sampler_->shouldSample({}, std::to_string(1000 + i), "operation_name",
                                          ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER,
                                          trace_context_1, {});
     result.isSampled() ? sampled++ : ignored++;
   }
+  // exponend should be 2, with a perfect random we would get 25 sampled and 75 ignored.
   EXPECT_EQ(ignored, 113);
   EXPECT_EQ(sampled, 186);
 
+  // send more requests.
   for (int i = 0; i < 700; i++) {
     auto result = sampler_->shouldSample({}, std::to_string(1000 + i), "operation_name",
                                          ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER,
                                          trace_context_1, {});
     result.isSampled() ? sampled++ : ignored++;
   }
+  // with a perfect random, the number of sampled paths would be lower than threshold (200)
+  // We don't care exceeding the threshold because it is not a hard limit
   EXPECT_EQ(ignored, 791);
   EXPECT_EQ(sampled, 208);
 }
 
+// Verify sampling if number of configured spans per minute is exceeded.
 TEST_F(DynatraceSamplerTest, TestSampling) {
   // config should allow 200 root spans per minute
   sampler_config_.parse("{\n \"rootSpansPerMinute\" : 200 \n }");
@@ -229,7 +226,7 @@ TEST_F(DynatraceSamplerTest, TestSampling) {
   trace_context_3.context_method_ = "POST";
   trace_context_3.context_path_ = "/another_path";
 
-  // send  requests
+  // simulate requests
   for (int i = 0; i < 180; i++) {
     sampler_->shouldSample({}, trace_id, "operation_name",
                            ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER,
@@ -243,7 +240,7 @@ TEST_F(DynatraceSamplerTest, TestSampling) {
                          ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER, trace_context_3,
                          {});
 
-  // sampler should read update sampling exponents
+  // sampler should update sampling exponents based on number of requests in the previous period
   timer_->invokeCallback();
 
   // the sampler should not sample every span for 'trace_context_1'
@@ -261,7 +258,7 @@ TEST_F(DynatraceSamplerTest, TestSampling) {
   }
   EXPECT_TRUE(ignored);
 
-  // trace_context_3 should be sampled
+  // trace_context_3 should always be sampled.
   for (int i = 0; i < 10; i++) {
     auto result = sampler_->shouldSample({}, std::to_string(i), "operation_name",
                                          ::opentelemetry::proto::trace::v1::Span::SPAN_KIND_SERVER,

test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_fetcher_test.cc

@@ -63,17 +63,17 @@ MATCHER_P(MessageMatcher, unusedArg, "") {
          (arg->headers().get(Http::Headers::get().Method)[0]->value().getStringView() == "GET");
 }
 
-// Test a request is sent if timer fires
+// Test that a request is sent if timer fires
 TEST_F(SamplerConfigFetcherTest, TestRequestIsSent) {
   EXPECT_CALL(tracer_factory_context_.server_factory_context_.cluster_manager_.thread_local_cluster_
                   .async_client_,
-              send_(MessageMatcher("unused-but-machtes-requires-an-arg"), _, _));
+              send_(MessageMatcher("unused-but-matchet-requires-an-arg"), _, _));
   SamplerConfigFetcherImpl config_fetcher(tracer_factory_context_, http_uri_, "tokenval");
   timer_->invokeCallback();
 }
 
-// Test receiving a response with code 200 and valid json
-TEST_F(SamplerConfigFetcherTest, TestResponseOk) {
+// Test receiving http response code 200 and valid json
+TEST_F(SamplerConfigFetcherTest, TestResponseOkValidJson) {
   SamplerConfigFetcherImpl config_fetcher(tracer_factory_context_, http_uri_, "tokenXASSD");
   timer_->invokeCallback();
 
@@ -85,7 +85,7 @@ TEST_F(SamplerConfigFetcherTest, TestResponseOk) {
   EXPECT_TRUE(timer_->enabled());
 }
 
-// Test receiving a response with code 200 and unexpected json
+// Test receiving http response code 200 and invalid json
 TEST_F(SamplerConfigFetcherTest, TestResponseOkInvalidJson) {
   SamplerConfigFetcherImpl config_fetcher(tracer_factory_context_, http_uri_, "tokenXASSD");
   timer_->invokeCallback();
@@ -99,7 +99,7 @@ TEST_F(SamplerConfigFetcherTest, TestResponseOkInvalidJson) {
   EXPECT_TRUE(timer_->enabled());
 }
 
-// Test receiving a response with code != 200
+// Test receiving http response code != 200
 TEST_F(SamplerConfigFetcherTest, TestResponseErrorCode) {
   SamplerConfigFetcherImpl config_fetcher(tracer_factory_context_, http_uri_, "tokenXASSD");
   timer_->invokeCallback();
@@ -127,7 +127,7 @@ TEST_F(SamplerConfigFetcherTest, TestOnFailure) {
 TEST_F(SamplerConfigFetcherTest, TestOnBeforeFinalizeUpstreamSpan) {
   Tracing::MockSpan child_span_;
   SamplerConfigFetcherImpl config_fetcher(tracer_factory_context_, http_uri_, "tokenXASSD");
-  // onBeforeFinalizeUpstreamSpan() is an empty method, nothing should happen
+  // onBeforeFinalizeUpstreamSpan() is an empty method, nothing to ASSERT, nothing should happen
   config_fetcher.onBeforeFinalizeUpstreamSpan(child_span_, nullptr);
 }
 
@@ -139,7 +139,7 @@ TEST_F(SamplerConfigFetcherTest, TestNoCluster) {
       .WillByDefault(Return(nullptr));
   SamplerConfigFetcherImpl config_fetcher(tracer_factory_context_, http_uri_, "tokenXASSD");
   timer_->invokeCallback();
-  // should not crash or throw.
+  // nothing to assert, should not crash or throw.
 }
 
 } // namespace OpenTelemetry

test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_test.cc

@@ -11,7 +11,7 @@ namespace Tracers {
 namespace OpenTelemetry {
 
 // Test sampler config json parsing
-TEST(SamplerConfigTest, test) {
+TEST(SamplerConfigTest, TestParsing) {
   SamplerConfig config;
   config.parse("{\n \"rootSpansPerMinute\" : 2000 \n }");
   EXPECT_EQ(config.getRootSpansPerMinute(), 2000u);
@@ -31,7 +31,13 @@ TEST(SamplerConfigTest, test) {
   config.parse(" { ");
   EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT);
 
-  config.parse(" } ");
+  config.parse("{\n \"rootSpansPerMinute\" : 10000 "); // closing } is missing
+  EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT);
+}
+
+// Test default sampler config (not yet parsed a json)
+TEST(SamplerConfigTest, TestDefaultConfig) {
+  SamplerConfig config;
   EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT);
 }