From 4941e9ec309a7c9b100813fc862658c6eb575900 Mon Sep 17 00:00:00 2001 From: Yaroslav Skopets Date: Mon, 16 Mar 2020 14:51:02 +0100 Subject: [PATCH 1/2] tracing: Datadog: do cleanup on tracer destruction in order to support dynamic (re-)configuration Signed-off-by: Yaroslav Skopets --- source/extensions/tracers/datadog/BUILD | 1 + .../tracers/datadog/datadog_tracer_impl.cc | 19 ++++-- .../tracers/datadog/datadog_tracer_impl.h | 4 ++ .../datadog/datadog_tracer_impl_test.cc | 62 +++++++++++++++++++ 4 files changed, 80 insertions(+), 6 deletions(-) diff --git a/source/extensions/tracers/datadog/BUILD b/source/extensions/tracers/datadog/BUILD index 840ab08c00ad1..0402595135267 100644 --- a/source/extensions/tracers/datadog/BUILD +++ b/source/extensions/tracers/datadog/BUILD @@ -22,6 +22,7 @@ envoy_cc_library( external_deps = ["dd_opentracing_cpp"], deps = [ "//source/common/config:utility_lib", + "//source/common/http:async_client_utility_lib", "//source/common/tracing:http_tracer_lib", "//source/extensions/tracers:well_known_names", "//source/extensions/tracers/common/ot:opentracing_driver_lib", diff --git a/source/extensions/tracers/datadog/datadog_tracer_impl.cc b/source/extensions/tracers/datadog/datadog_tracer_impl.cc index 71206182a63f2..baf239bf94498 100644 --- a/source/extensions/tracers/datadog/datadog_tracer_impl.cc +++ b/source/extensions/tracers/datadog/datadog_tracer_impl.cc @@ -100,22 +100,29 @@ void TraceReporter::flushTraces() { ENVOY_LOG(debug, "submitting {} trace(s) to {} with payload size {}", pendingTraces, encoder_->path(), encoder_->payload().size()); - driver_.clusterManager() - .httpAsyncClientForCluster(driver_.cluster()->name()) - .send(std::move(message), *this, - Http::AsyncClient::RequestOptions().setTimeout(std::chrono::milliseconds(1000U))); + auto* request = + driver_.clusterManager() + .httpAsyncClientForCluster(driver_.cluster()->name()) + .send(std::move(message), *this, + Http::AsyncClient::RequestOptions().setTimeout(std::chrono::milliseconds(1000U))); + if (request) { + active_requests_.add(*request); + } encoder_->clearTraces(); } } -void TraceReporter::onFailure(const Http::AsyncClient::Request&, Http::AsyncClient::FailureReason) { +void TraceReporter::onFailure(const Http::AsyncClient::Request& request, + Http::AsyncClient::FailureReason) { + active_requests_.remove(request); ENVOY_LOG(debug, "failure submitting traces to datadog agent"); driver_.tracerStats().reports_failed_.inc(); } -void TraceReporter::onSuccess(const Http::AsyncClient::Request&, +void TraceReporter::onSuccess(const Http::AsyncClient::Request& request, Http::ResponseMessagePtr&& http_response) { + active_requests_.remove(request); uint64_t responseStatus = Http::Utility::getResponseStatus(http_response->headers()); if (responseStatus != enumToInt(Http::Code::OK)) { // TODO: Consider adding retries for failed submissions. diff --git a/source/extensions/tracers/datadog/datadog_tracer_impl.h b/source/extensions/tracers/datadog/datadog_tracer_impl.h index b36384b716b99..8a6f9d382770f 100644 --- a/source/extensions/tracers/datadog/datadog_tracer_impl.h +++ b/source/extensions/tracers/datadog/datadog_tracer_impl.h @@ -9,6 +9,7 @@ #include "envoy/tracing/http_tracer.h" #include "envoy/upstream/cluster_manager.h" +#include "common/http/async_client_utility.h" #include "common/http/header_map_impl.h" #include "common/json/json_loader.h" @@ -127,6 +128,9 @@ class TraceReporter : public Http::AsyncClient::Callbacks, TraceEncoderSharedPtr encoder_; std::map lower_case_headers_; + + // Track active HTTP requests to be able to cancel them on destruction. + Http::AsyncClientRequestTracker active_requests_; }; } // namespace Datadog } // namespace Tracers diff --git a/test/extensions/tracers/datadog/datadog_tracer_impl_test.cc b/test/extensions/tracers/datadog/datadog_tracer_impl_test.cc index 0a0e28d2b6285..e2f4c6734dc29 100644 --- a/test/extensions/tracers/datadog/datadog_tracer_impl_test.cc +++ b/test/extensions/tracers/datadog/datadog_tracer_impl_test.cc @@ -29,11 +29,14 @@ #include "gtest/gtest.h" using testing::_; +using testing::DoAll; using testing::Eq; using testing::Invoke; using testing::NiceMock; using testing::Return; using testing::ReturnRef; +using testing::StrictMock; +using testing::WithArg; namespace Envoy { namespace Extensions { @@ -165,6 +168,65 @@ TEST_F(DatadogDriverTest, FlushSpansTimer) { EXPECT_EQ(0U, stats_.counter("tracing.datadog.reports_failed").value()); } +TEST_F(DatadogDriverTest, CancelInflightRequestsOnDestruction) { + setupValidDriver(); + + StrictMock request1(&cm_.async_client_), + request2(&cm_.async_client_), request3(&cm_.async_client_), request4(&cm_.async_client_); + Http::AsyncClient::Callbacks* callback{}; + const absl::optional timeout(std::chrono::seconds(1)); + + // Expect 4 separate report requests to be made. + EXPECT_CALL(cm_.async_client_, + send_(_, _, Http::AsyncClient::RequestOptions().setTimeout(timeout))) + .WillOnce(DoAll(WithArg<1>(SaveArgAddress(&callback)), Return(&request1))) + .WillOnce(Return(&request2)) + .WillOnce(Return(&request3)) + .WillOnce(Return(&request4)); + // Expect timer to be re-enabled on each tick. + EXPECT_CALL(*timer_, enableTimer(std::chrono::milliseconds(900), _)).Times(4); + + // Trigger 1st report request. + driver_ + ->startSpan(config_, request_headers_, operation_name_, start_time_, + {Tracing::Reason::Sampling, true}) + ->finishSpan(); + timer_->invokeCallback(); + // Trigger 2nd report request. + driver_ + ->startSpan(config_, request_headers_, operation_name_, start_time_, + {Tracing::Reason::Sampling, true}) + ->finishSpan(); + timer_->invokeCallback(); + // Trigger 3rd report request. + driver_ + ->startSpan(config_, request_headers_, operation_name_, start_time_, + {Tracing::Reason::Sampling, true}) + ->finishSpan(); + timer_->invokeCallback(); + // Trigger 4th report request. + driver_ + ->startSpan(config_, request_headers_, operation_name_, start_time_, + {Tracing::Reason::Sampling, true}) + ->finishSpan(); + timer_->invokeCallback(); + + Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( + Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "404"}}})); + // Simulate completion of the 2nd report request. + callback->onSuccess(request2, std::move(msg)); + + // Simulate failure of the 3rd report request. + callback->onFailure(request3, Http::AsyncClient::FailureReason::Reset); + + // Expect 1st and 4th requests to be cancelled on destruction. + EXPECT_CALL(request1, cancel()); + EXPECT_CALL(request4, cancel()); + + // Trigger destruction. + driver_.reset(); +} + } // namespace } // namespace Datadog } // namespace Tracers From b65045692f900017e51ffc6d24110e840edb2abe Mon Sep 17 00:00:00 2001 From: Yaroslav Skopets Date: Tue, 24 Mar 2020 01:41:31 +0100 Subject: [PATCH 2/2] code review: use explicit type Signed-off-by: Yaroslav Skopets --- source/extensions/tracers/datadog/datadog_tracer_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/tracers/datadog/datadog_tracer_impl.cc b/source/extensions/tracers/datadog/datadog_tracer_impl.cc index baf239bf94498..c3d659bc121ff 100644 --- a/source/extensions/tracers/datadog/datadog_tracer_impl.cc +++ b/source/extensions/tracers/datadog/datadog_tracer_impl.cc @@ -100,7 +100,7 @@ void TraceReporter::flushTraces() { ENVOY_LOG(debug, "submitting {} trace(s) to {} with payload size {}", pendingTraces, encoder_->path(), encoder_->payload().size()); - auto* request = + Http::AsyncClient::Request* request = driver_.clusterManager() .httpAsyncClientForCluster(driver_.cluster()->name()) .send(std::move(message), *this,