From c1002b7b7e7c82ffe4774d45ea7e91a8c3da687c Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Tue, 27 Feb 2024 17:10:50 +0100 Subject: [PATCH 1/6] Dynatrace sampler to fetch configuration from an API Signed-off-by: thomas.ebner --- .../samplers/v3/dynatrace_sampler.proto | 2 +- .../samplers/dynatrace/sampler_config.h | 4 + .../dynatrace/sampler_config_provider.cc | 61 +++++++- .../dynatrace/sampler_config_provider.h | 20 ++- .../dynatrace/sampler_config_provider_test.cc | 144 +++++++++++++++++- 5 files changed, 223 insertions(+), 8 deletions(-) diff --git a/api/envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.proto b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.proto index b74e96a6a4164..e4d64e24eb07a 100644 --- a/api/envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.proto +++ b/api/envoy/extensions/tracers/opentelemetry/samplers/v3/dynatrace_sampler.proto @@ -33,7 +33,7 @@ message DynatraceSamplerConfig { // .. code-block:: yaml // // http_uri: - // uri: .dev.dynatracelabs.com/api/v2/otlp/v1/traces + // uri: .dev.dynatracelabs.com/api/v2/samplingConfiguration // cluster: dynatrace // timeout: 10s // diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.h b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.h index 6f576b2d5afb9..a2f40889fd990 100644 --- a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.h +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.h @@ -22,6 +22,10 @@ class SamplerConfig { ? default_root_spans_per_minute : ROOT_SPANS_PER_MINUTE_DEFAULT), root_spans_per_minute_(default_root_spans_per_minute_) {} + + SamplerConfig(const SamplerConfig&) = delete; + SamplerConfig& operator=(const SamplerConfig&) = delete; + /** * @brief Parses a json string containing the expected root spans per minute. * diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc index 465cb9e1bd5f2..c1e73a849e73e 100644 --- a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc @@ -1,5 +1,7 @@ #include "source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h" +#include + #include "source/common/common/enum_to_int.h" #include "source/common/http/utility.h" @@ -8,13 +10,68 @@ namespace Extensions { namespace Tracers { namespace OpenTelemetry { +static constexpr std::chrono::seconds INITIAL_TIMER_DURATION{10}; +static constexpr std::chrono::minutes TIMER_INTERVAL{5}; + SamplerConfigProviderImpl::SamplerConfigProviderImpl( - Server::Configuration::TracerFactoryContext& /*context*/, + Server::Configuration::TracerFactoryContext& context, const envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig& config) - : sampler_config_(config.root_spans_per_minute()) {} + : cluster_manager_(context.serverFactoryContext().clusterManager()), + http_uri_(config.http_uri()), + authorization_header_value_(absl::StrCat("Api-Token ", config.token())), + sampler_config_(config.root_spans_per_minute()) { + + timer_ = context.serverFactoryContext().mainThreadDispatcher().createTimer([this]() -> void { + const auto thread_local_cluster = cluster_manager_.getThreadLocalCluster(http_uri_.cluster()); + if (thread_local_cluster == nullptr) { + ENVOY_LOG(error, "SamplerConfigProviderImpl failed: [cluster = {}] is not configured", + http_uri_.cluster()); + } else { + Http::RequestMessagePtr message = Http::Utility::prepareHeaders(http_uri_); + message->headers().setReferenceMethod(Http::Headers::get().MethodValues.Get); + message->headers().setReference(Http::CustomHeaders::get().Authorization, + authorization_header_value_); + active_request_ = thread_local_cluster->httpAsyncClient().send( + std::move(message), *this, + Http::AsyncClient::RequestOptions().setTimeout(std::chrono::milliseconds(6000))); + } + }); + + timer_->enableTimer(std::chrono::seconds(INITIAL_TIMER_DURATION)); +} + +SamplerConfigProviderImpl::~SamplerConfigProviderImpl() { + if (active_request_) { + active_request_->cancel(); + } +} + +void SamplerConfigProviderImpl::onSuccess(const Http::AsyncClient::Request& /*request*/, + Http::ResponseMessagePtr&& http_response) { + onRequestDone(); + const auto response_code = Http::Utility::getResponseStatus(http_response->headers()); + if (response_code == enumToInt(Http::Code::OK)) { + ENVOY_LOG(debug, "Received sampling configuration from Dynatrace: {}", + http_response->bodyAsString()); + sampler_config_.parse(http_response->bodyAsString()); + } else { + ENVOY_LOG(warn, "Failed to get sampling configuration from Dynatrace: {}", response_code); + } +} + +void SamplerConfigProviderImpl::onFailure(const Http::AsyncClient::Request& /*request*/, + Http::AsyncClient::FailureReason reason) { + onRequestDone(); + ENVOY_LOG(info, "The OTLP export request failed. Reason {}", enumToInt(reason)); +} const SamplerConfig& SamplerConfigProviderImpl::getSamplerConfig() const { return sampler_config_; } +void SamplerConfigProviderImpl::onRequestDone() { + active_request_ = nullptr; + timer_->enableTimer(std::chrono::seconds(TIMER_INTERVAL)); +} + } // namespace OpenTelemetry } // namespace Tracers } // namespace Extensions diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h index 6dd464d4f6c74..927861cc3dbe0 100644 --- a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h @@ -40,19 +40,35 @@ class SamplerConfigProvider { }; class SamplerConfigProviderImpl : public SamplerConfigProvider, - public Logger::Loggable { + public Logger::Loggable, + public Http::AsyncClient::Callbacks { public: SamplerConfigProviderImpl( Server::Configuration::TracerFactoryContext& context, const envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig& config); + void onSuccess(const Http::AsyncClient::Request& request, + Http::ResponseMessagePtr&& response) override; + + void onFailure(const Http::AsyncClient::Request& request, + Http::AsyncClient::FailureReason reason) override; + void onBeforeFinalizeUpstreamSpan(Envoy::Tracing::Span& /*span*/, + const Http::ResponseHeaderMap* /*response_headers*/) override{}; + const SamplerConfig& getSamplerConfig() const override; - ~SamplerConfigProviderImpl() override = default; + ~SamplerConfigProviderImpl() override; private: + Event::TimerPtr timer_; + Upstream::ClusterManager& cluster_manager_; + envoy::config::core::v3::HttpUri http_uri_; + const std::string authorization_header_value_; + Http::AsyncClient::Request* active_request_{}; SamplerConfig sampler_config_; + + void onRequestDone(); }; using SamplerConfigProviderPtr = std::unique_ptr; diff --git a/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider_test.cc b/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider_test.cc index d5a8c68d71b80..1d77f6ff7c83e 100644 --- a/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider_test.cc @@ -18,17 +18,155 @@ namespace Envoy { namespace Extensions { namespace Tracers { namespace OpenTelemetry { - using testing::NiceMock; using testing::Return; using testing::ReturnRef; class SamplerConfigProviderTest : public testing::Test { public: + SamplerConfigProviderTest() + : request_(&tracer_factory_context_.server_factory_context_.cluster_manager_ + .thread_local_cluster_.async_client_) { + + const std::string yaml_string = R"EOF( + tenant: "abc12345" + cluster_id: -1743916452 + token: "tokenval" + http_uri: + cluster: "cluster_name" + uri: "https://testhost.com/api/v2/samplingConfiguration" + timeout: 0.250s + root_spans_per_minute: 1000 + )EOF"; + TestUtility::loadFromYaml(yaml_string, proto_config_); + + ON_CALL(tracer_factory_context_.server_factory_context_.cluster_manager_, + getThreadLocalCluster(_)) + .WillByDefault(Return(&tracer_factory_context_.server_factory_context_.cluster_manager_ + .thread_local_cluster_)); + timer_ = new NiceMock( + &tracer_factory_context_.server_factory_context_.dispatcher_); + ON_CALL(tracer_factory_context_.server_factory_context_.dispatcher_, createTimer_(_)) + .WillByDefault(Invoke([this](Event::TimerCb) { return timer_; })); + } + protected: NiceMock tracer_factory_context_; + envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig proto_config_; + NiceMock* timer_; + Http::MockAsyncClientRequest request_; }; +MATCHER_P(MessageMatcher, unusedArg, "") { + // prefix 'Api-Token' should be added to 'tokenval' set via SamplerConfigProvider constructor + return (arg->headers() + .get(Http::CustomHeaders::get().Authorization)[0] + ->value() + .getStringView() == "Api-Token tokenval") && + (arg->headers().get(Http::Headers::get().Path)[0]->value().getStringView() == + "/api/v2/samplingConfiguration") && + (arg->headers().get(Http::Headers::get().Host)[0]->value().getStringView() == + "testhost.com") && + (arg->headers().get(Http::Headers::get().Method)[0]->value().getStringView() == "GET"); +} + +// Test that a request is sent if timer fires +TEST_F(SamplerConfigProviderTest, TestRequestIsSent) { + EXPECT_CALL(tracer_factory_context_.server_factory_context_.cluster_manager_.thread_local_cluster_ + .async_client_, + send_(MessageMatcher("unused-arg"), _, _)); + SamplerConfigProviderImpl config_provider(tracer_factory_context_, proto_config_); + timer_->invokeCallback(); +} + +// Test that a pending request is canceled +TEST_F(SamplerConfigProviderTest, TestPendingRequestIsCanceled) { + class TestRequest : public Http::AsyncClient::Request { + public: + MOCK_METHOD(void, cancel, ()); + }; + + NiceMock test_request; + EXPECT_CALL(test_request, cancel()); + ON_CALL(tracer_factory_context_.server_factory_context_.cluster_manager_.thread_local_cluster_ + .async_client_, + send_(_, _, _)) + .WillByDefault(Return(&test_request)); + SamplerConfigProviderImpl config_provider(tracer_factory_context_, proto_config_); + timer_->invokeCallback(); +} + +// Test receiving http response code 200 and valid json +TEST_F(SamplerConfigProviderTest, TestResponseOkValidJson) { + SamplerConfigProviderImpl config_provider(tracer_factory_context_, proto_config_); + timer_->invokeCallback(); + + Http::ResponseMessagePtr message(new Http::ResponseMessageImpl( + Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "200"}}})); + message->body().add("{\n \"rootSpansPerMinute\" : 4356 \n }"); + config_provider.onSuccess(request_, std::move(message)); + EXPECT_EQ(config_provider.getSamplerConfig().getRootSpansPerMinute(), 4356); + EXPECT_TRUE(timer_->enabled()); +} + +// Test receiving http response code 200 and invalid json +TEST_F(SamplerConfigProviderTest, TestResponseOkInvalidJson) { + SamplerConfigProviderImpl config_provider(tracer_factory_context_, proto_config_); + timer_->invokeCallback(); + + Http::ResponseMessagePtr message(new Http::ResponseMessageImpl( + Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "200"}}})); + message->body().add("{\n "); + config_provider.onSuccess(request_, std::move(message)); + EXPECT_EQ(config_provider.getSamplerConfig().getRootSpansPerMinute(), + SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); + EXPECT_TRUE(timer_->enabled()); +} + +// Test receiving http response code != 200 +TEST_F(SamplerConfigProviderTest, TestResponseErrorCode) { + SamplerConfigProviderImpl config_provider(tracer_factory_context_, proto_config_); + timer_->invokeCallback(); + + Http::ResponseMessagePtr message(new Http::ResponseMessageImpl( + Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "401"}}})); + message->body().add("{\n \"rootSpansPerMinute\" : 4356 \n }"); + config_provider.onSuccess(request_, std::move(message)); + EXPECT_EQ(config_provider.getSamplerConfig().getRootSpansPerMinute(), + SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); + EXPECT_TRUE(timer_->enabled()); +} + +// Test sending failed +TEST_F(SamplerConfigProviderTest, TestOnFailure) { + SamplerConfigProviderImpl config_provider(tracer_factory_context_, proto_config_); + timer_->invokeCallback(); + config_provider.onFailure(request_, Http::AsyncClient::FailureReason::Reset); + EXPECT_EQ(config_provider.getSamplerConfig().getRootSpansPerMinute(), + SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); + EXPECT_TRUE(timer_->enabled()); +} + +// Test calling onBeforeFinalizeUpstreamSpan +TEST_F(SamplerConfigProviderTest, TestOnBeforeFinalizeUpstreamSpan) { + Tracing::MockSpan child_span_; + SamplerConfigProviderImpl config_provider(tracer_factory_context_, proto_config_); + // onBeforeFinalizeUpstreamSpan() is an empty method, nothing to ASSERT, nothing should happen + config_provider.onBeforeFinalizeUpstreamSpan(child_span_, nullptr); +} + +// Test invoking the timer if no cluster can be found +TEST_F(SamplerConfigProviderTest, TestNoCluster) { + // simulate no configured cluster, return nullptr. + ON_CALL(tracer_factory_context_.server_factory_context_.cluster_manager_, + getThreadLocalCluster(_)) + .WillByDefault(Return(nullptr)); + SamplerConfigProviderImpl config_provider(tracer_factory_context_, proto_config_); + timer_->invokeCallback(); + // nothing to assert, should not crash or throw. +} + +// Test that configured value is used TEST_F(SamplerConfigProviderTest, TestValueConfigured) { const std::string yaml_string = R"EOF( tenant: "abc12345" @@ -39,7 +177,6 @@ TEST_F(SamplerConfigProviderTest, TestValueConfigured) { uri: "https://testhost.com/otlp/v1/traces" timeout: 0.250s root_spans_per_minute: 3456 - )EOF"; envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig proto_config; @@ -49,6 +186,7 @@ TEST_F(SamplerConfigProviderTest, TestValueConfigured) { EXPECT_EQ(config_provider.getSamplerConfig().getRootSpansPerMinute(), 3456); } +// Test using a config without a setting for configured root spans TEST_F(SamplerConfigProviderTest, TestNoValueConfigured) { const std::string yaml_string = R"EOF( tenant: "abc12345" @@ -58,7 +196,6 @@ TEST_F(SamplerConfigProviderTest, TestNoValueConfigured) { cluster: "cluster_name" uri: "https://testhost.com/otlp/v1/traces" timeout: 0.250s - )EOF"; envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig proto_config; @@ -69,6 +206,7 @@ TEST_F(SamplerConfigProviderTest, TestNoValueConfigured) { SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); } +// Test using a config with 0 configured root spans TEST_F(SamplerConfigProviderTest, TestValueZeroConfigured) { const std::string yaml_string = R"EOF( tenant: "abc12345" From 1e3f851f4a3c45efeb7d92c2f252617941b7ec16 Mon Sep 17 00:00:00 2001 From: Thomas Ebner <96168670+samohte@users.noreply.github.com> Date: Fri, 8 Mar 2024 13:28:22 +0100 Subject: [PATCH 2/6] Update source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h Co-authored-by: Joao Grassi <5938087+joaopgrassi@users.noreply.github.com> Signed-off-by: Thomas Ebner <96168670+samohte@users.noreply.github.com> --- .../opentelemetry/samplers/dynatrace/sampler_config_provider.h | 1 + 1 file changed, 1 insertion(+) diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h index 927861cc3dbe0..d4dd38dcc2b29 100644 --- a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h @@ -53,6 +53,7 @@ class SamplerConfigProviderImpl : public SamplerConfigProvider, void onFailure(const Http::AsyncClient::Request& request, Http::AsyncClient::FailureReason reason) override; + void onBeforeFinalizeUpstreamSpan(Envoy::Tracing::Span& /*span*/, const Http::ResponseHeaderMap* /*response_headers*/) override{}; From c0a4aeda71e471740c7b960c0684fc20b184af39 Mon Sep 17 00:00:00 2001 From: Thomas Ebner <96168670+samohte@users.noreply.github.com> Date: Fri, 8 Mar 2024 13:28:54 +0100 Subject: [PATCH 3/6] Update source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc Co-authored-by: Joao Grassi <5938087+joaopgrassi@users.noreply.github.com> Signed-off-by: Thomas Ebner <96168670+samohte@users.noreply.github.com> --- .../opentelemetry/samplers/dynatrace/sampler_config_provider.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc index c1e73a849e73e..e120134db29b3 100644 --- a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc @@ -62,7 +62,7 @@ void SamplerConfigProviderImpl::onSuccess(const Http::AsyncClient::Request& /*re void SamplerConfigProviderImpl::onFailure(const Http::AsyncClient::Request& /*request*/, Http::AsyncClient::FailureReason reason) { onRequestDone(); - ENVOY_LOG(info, "The OTLP export request failed. Reason {}", enumToInt(reason)); + ENVOY_LOG(warn, "Failed to get sampling configuration from Dynatrace. Reason {}", enumToInt(reason)); } const SamplerConfig& SamplerConfigProviderImpl::getSamplerConfig() const { return sampler_config_; } From 21ec9c9efa7a7b11ab007f4a6c2a2f9de21836c5 Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Mon, 11 Mar 2024 10:29:06 +0100 Subject: [PATCH 4/6] review feedback Signed-off-by: thomas.ebner --- .../samplers/dynatrace/sampler_config.cc | 5 +- .../samplers/dynatrace/sampler_config.h | 4 +- .../dynatrace/sampler_config_provider.cc | 58 +++++++++++++++---- .../dynatrace/sampler_config_provider.h | 5 +- .../dynatrace/sampler_config_provider_test.cc | 41 ++++++++++++- .../samplers/dynatrace/sampler_config_test.cc | 20 +++---- 6 files changed, 103 insertions(+), 30 deletions(-) diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.cc b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.cc index b3a6f2b91c45b..eee907c002f4f 100644 --- a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.cc +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.cc @@ -8,18 +8,19 @@ namespace Extensions { namespace Tracers { namespace OpenTelemetry { -void SamplerConfig::parse(const std::string& json) { +bool SamplerConfig::parse(const std::string& json) { const auto result = Envoy::Json::Factory::loadFromStringNoThrow(json); if (result.ok()) { const auto& obj = result.value(); if (obj->hasObject("rootSpansPerMinute")) { const auto value = obj->getInteger("rootSpansPerMinute", default_root_spans_per_minute_); root_spans_per_minute_.store(value); - return; + return true; } } // Didn't get a value, reset to default root_spans_per_minute_.store(default_root_spans_per_minute_); + return false; } } // namespace OpenTelemetry diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.h b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.h index a2f40889fd990..3763de3f7a256 100644 --- a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.h +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config.h @@ -30,8 +30,10 @@ class SamplerConfig { * @brief Parses a json string containing the expected root spans per minute. * * @param json A string containing the configuration. + * + * @return true if parsing was successful, false otherwise */ - void parse(const std::string& json); + bool parse(const std::string& json); /** * @brief Returns wanted root spans per minute diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc index e120134db29b3..6590215672ded 100644 --- a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc @@ -13,13 +13,42 @@ namespace OpenTelemetry { static constexpr std::chrono::seconds INITIAL_TIMER_DURATION{10}; static constexpr std::chrono::minutes TIMER_INTERVAL{5}; +namespace { + +bool reEnableTimer(Http::Code response_code) { + switch (response_code) { + case Http::Code::OK: + case Http::Code::TooManyRequests: + case Http::Code::InternalServerError: + case Http::Code::BadGateway: + case Http::Code::ServiceUnavailable: + case Http::Code::GatewayTimeout: + case Http::Code::InsufficientStorage: + return true; + default: + return false; + } +} + +std::chrono::milliseconds getTimeout(envoy::config::core::v3::HttpUri& http_uri) { + auto timeout = + std::chrono::milliseconds(DurationUtil::durationToMilliseconds(http_uri.timeout())); + // we want to ensure that we don't have more than one pending request + if (timeout > (TIMER_INTERVAL / 2)) { + timeout = (TIMER_INTERVAL / 2); + } + return timeout; +} + +} // namespace + SamplerConfigProviderImpl::SamplerConfigProviderImpl( Server::Configuration::TracerFactoryContext& context, const envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig& config) : cluster_manager_(context.serverFactoryContext().clusterManager()), http_uri_(config.http_uri()), authorization_header_value_(absl::StrCat("Api-Token ", config.token())), - sampler_config_(config.root_spans_per_minute()) { + sampler_config_(config.root_spans_per_minute()), timeout_(getTimeout(http_uri_)) { timer_ = context.serverFactoryContext().mainThreadDispatcher().createTimer([this]() -> void { const auto thread_local_cluster = cluster_manager_.getThreadLocalCluster(http_uri_.cluster()); @@ -32,8 +61,7 @@ SamplerConfigProviderImpl::SamplerConfigProviderImpl( message->headers().setReference(Http::CustomHeaders::get().Authorization, authorization_header_value_); active_request_ = thread_local_cluster->httpAsyncClient().send( - std::move(message), *this, - Http::AsyncClient::RequestOptions().setTimeout(std::chrono::milliseconds(6000))); + std::move(message), *this, Http::AsyncClient::RequestOptions().setTimeout(timeout_)); } }); @@ -48,30 +76,36 @@ SamplerConfigProviderImpl::~SamplerConfigProviderImpl() { void SamplerConfigProviderImpl::onSuccess(const Http::AsyncClient::Request& /*request*/, Http::ResponseMessagePtr&& http_response) { - onRequestDone(); + active_request_ = nullptr; const auto response_code = Http::Utility::getResponseStatus(http_response->headers()); + bool json_valid = false; if (response_code == enumToInt(Http::Code::OK)) { ENVOY_LOG(debug, "Received sampling configuration from Dynatrace: {}", http_response->bodyAsString()); - sampler_config_.parse(http_response->bodyAsString()); + json_valid = sampler_config_.parse(http_response->bodyAsString()); + if (!json_valid) { + ENVOY_LOG(warn, "Failed to parse sampling configuration received from Dynatrace: {}", + http_response->bodyAsString()); + } } else { ENVOY_LOG(warn, "Failed to get sampling configuration from Dynatrace: {}", response_code); } + + if (json_valid || reEnableTimer(static_cast(response_code))) { + timer_->enableTimer(std::chrono::seconds(TIMER_INTERVAL)); + } } void SamplerConfigProviderImpl::onFailure(const Http::AsyncClient::Request& /*request*/, Http::AsyncClient::FailureReason reason) { - onRequestDone(); - ENVOY_LOG(warn, "Failed to get sampling configuration from Dynatrace. Reason {}", enumToInt(reason)); -} - -const SamplerConfig& SamplerConfigProviderImpl::getSamplerConfig() const { return sampler_config_; } - -void SamplerConfigProviderImpl::onRequestDone() { active_request_ = nullptr; timer_->enableTimer(std::chrono::seconds(TIMER_INTERVAL)); + ENVOY_LOG(warn, "Failed to get sampling configuration from Dynatrace. Reason {}", + enumToInt(reason)); } +const SamplerConfig& SamplerConfigProviderImpl::getSamplerConfig() const { return sampler_config_; } + } // namespace OpenTelemetry } // namespace Tracers } // namespace Extensions diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h index d4dd38dcc2b29..95eedc627cf4c 100644 --- a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.h @@ -53,7 +53,7 @@ class SamplerConfigProviderImpl : public SamplerConfigProvider, void onFailure(const Http::AsyncClient::Request& request, Http::AsyncClient::FailureReason reason) override; - + void onBeforeFinalizeUpstreamSpan(Envoy::Tracing::Span& /*span*/, const Http::ResponseHeaderMap* /*response_headers*/) override{}; @@ -68,8 +68,7 @@ class SamplerConfigProviderImpl : public SamplerConfigProvider, const std::string authorization_header_value_; Http::AsyncClient::Request* active_request_{}; SamplerConfig sampler_config_; - - void onRequestDone(); + const std::chrono::milliseconds timeout_; }; using SamplerConfigProviderPtr = std::unique_ptr; diff --git a/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider_test.cc b/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider_test.cc index 1d77f6ff7c83e..944c653df019f 100644 --- a/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider_test.cc @@ -123,6 +123,43 @@ TEST_F(SamplerConfigProviderTest, TestResponseOkInvalidJson) { EXPECT_TRUE(timer_->enabled()); } +void doTestResponseCode(Http::Code response_code, bool timer_enabled, + SamplerConfigProviderImpl& config_provider, + Http::MockAsyncClientRequest& request, NiceMock* timer, + int line_num) { + SCOPED_TRACE(absl::StrCat(__FUNCTION__, " called from line ", line_num)); + Http::ResponseMessagePtr message(new Http::ResponseMessageImpl(Http::ResponseHeaderMapPtr{ + new Http::TestResponseHeaderMapImpl{{":status", std::to_string(enumToInt(response_code))}}})); + message->body().add("{\n \"rootSpansPerMinute\" : 1000 \n }"); + config_provider.onSuccess(request, std::move(message)); + EXPECT_EQ(timer->enabled(), timer_enabled); +} + +// Test that timer is re-enabled depending on the response code +TEST_F(SamplerConfigProviderTest, TestReenableTimer) { + SamplerConfigProviderImpl config_provider(tracer_factory_context_, proto_config_); + timer_->invokeCallback(); + doTestResponseCode(Http::Code::Forbidden, false, config_provider, request_, timer_, __LINE__); + doTestResponseCode(Http::Code::NotFound, false, config_provider, request_, timer_, __LINE__); + doTestResponseCode(Http::Code::OK, true, config_provider, request_, timer_, __LINE__); + timer_->invokeCallback(); + doTestResponseCode(Http::Code::TooManyRequests, true, config_provider, request_, timer_, + __LINE__); + timer_->invokeCallback(); + doTestResponseCode(Http::Code::InternalServerError, true, config_provider, request_, timer_, + __LINE__); + timer_->invokeCallback(); + doTestResponseCode(Http::Code::BadGateway, true, config_provider, request_, timer_, __LINE__); + timer_->invokeCallback(); + doTestResponseCode(Http::Code::ServiceUnavailable, true, config_provider, request_, timer_, + __LINE__); + timer_->invokeCallback(); + doTestResponseCode(Http::Code::GatewayTimeout, true, config_provider, request_, timer_, __LINE__); + timer_->invokeCallback(); + doTestResponseCode(Http::Code::InsufficientStorage, true, config_provider, request_, timer_, + __LINE__); +} + // Test receiving http response code != 200 TEST_F(SamplerConfigProviderTest, TestResponseErrorCode) { SamplerConfigProviderImpl config_provider(tracer_factory_context_, proto_config_); @@ -134,7 +171,7 @@ TEST_F(SamplerConfigProviderTest, TestResponseErrorCode) { config_provider.onSuccess(request_, std::move(message)); EXPECT_EQ(config_provider.getSamplerConfig().getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); - EXPECT_TRUE(timer_->enabled()); + EXPECT_FALSE(timer_->enabled()); } // Test sending failed @@ -195,7 +232,7 @@ TEST_F(SamplerConfigProviderTest, TestNoValueConfigured) { http_uri: cluster: "cluster_name" uri: "https://testhost.com/otlp/v1/traces" - timeout: 0.250s + timeout: 500s )EOF"; envoy::extensions::tracers::opentelemetry::samplers::v3::DynatraceSamplerConfig proto_config; diff --git a/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_test.cc b/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_test.cc index 8cb45a8ca3e65..f849b582c5f16 100644 --- a/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_test.cc +++ b/test/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_test.cc @@ -14,25 +14,25 @@ namespace OpenTelemetry { TEST(SamplerConfigTest, TestParsing) { // default_root_spans_per_minute not set, ROOT_SPANS_PER_MINUTE_DEFAULT should be used SamplerConfig config(0); - config.parse("{\n \"rootSpansPerMinute\" : 2000 \n }"); + EXPECT_TRUE(config.parse("{\n \"rootSpansPerMinute\" : 2000 \n }")); EXPECT_EQ(config.getRootSpansPerMinute(), 2000u); - config.parse("{\n \"rootSpansPerMinute\" : 10000 \n }"); + EXPECT_TRUE(config.parse("{\n \"rootSpansPerMinute\" : 10000 \n }")); EXPECT_EQ(config.getRootSpansPerMinute(), 10000u); // unexpected json, default value should be used - config.parse("{}"); + EXPECT_FALSE(config.parse("{}")); EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); - config.parse(""); + EXPECT_FALSE(config.parse("")); EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); - config.parse("\\"); + EXPECT_FALSE(config.parse("\\")); EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); - config.parse(" { "); + EXPECT_FALSE(config.parse(" { ")); EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); - config.parse("{\n \"rootSpansPerMinute\" : 10000 "); // closing } is missing + EXPECT_FALSE(config.parse("{\n \"rootSpansPerMinute\" : 10000 ")); // closing } is missing EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); } @@ -41,19 +41,19 @@ TEST(SamplerConfigTest, TestDefaultConfig) { { SamplerConfig config(0); EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); - config.parse(" { "); // parse invalid json, default value should still be used + EXPECT_FALSE(config.parse(" { ")); // parse invalid json, default value should still be used EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); } { SamplerConfig config(900); EXPECT_EQ(config.getRootSpansPerMinute(), 900); - config.parse(" { "); + EXPECT_FALSE(config.parse(" { ")); EXPECT_EQ(config.getRootSpansPerMinute(), 900); } { SamplerConfig config(SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); - config.parse(" { "); + EXPECT_FALSE(config.parse(" { ")); EXPECT_EQ(config.getRootSpansPerMinute(), SamplerConfig::ROOT_SPANS_PER_MINUTE_DEFAULT); } } From 8c3867b2f8cc5a119e1f82c2a50e1717f5af0128 Mon Sep 17 00:00:00 2001 From: "thomas.ebner" Date: Tue, 12 Mar 2024 11:02:47 +0100 Subject: [PATCH 5/6] add changelog Signed-off-by: thomas.ebner --- changelogs/current.yaml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 1ce3212d3aae9..4c1696395b121 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -282,6 +282,9 @@ new_features: - area: tracing change: | Added support for variant span attribute type for the OpenTelemetry tracer. +- area: tracing + change: | + Dynatrace sampler fetches configuration from Dynatrace API. deprecated: - area: listener From 3ebe1a8f655756b2066b883768f0122a9980d49d Mon Sep 17 00:00:00 2001 From: Thomas Ebner <96168670+samohte@users.noreply.github.com> Date: Tue, 12 Mar 2024 12:20:45 +0100 Subject: [PATCH 6/6] Update source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc Co-authored-by: Joao Grassi <5938087+joaopgrassi@users.noreply.github.com> Signed-off-by: Thomas Ebner <96168670+samohte@users.noreply.github.com> --- .../opentelemetry/samplers/dynatrace/sampler_config_provider.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc index 6590215672ded..60d6a82f1f606 100644 --- a/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc +++ b/source/extensions/tracers/opentelemetry/samplers/dynatrace/sampler_config_provider.cc @@ -23,7 +23,6 @@ bool reEnableTimer(Http::Code response_code) { case Http::Code::BadGateway: case Http::Code::ServiceUnavailable: case Http::Code::GatewayTimeout: - case Http::Code::InsufficientStorage: return true; default: return false;