From 7cc9b208cbef0a7717748b5fc058076fd7f7dc77 Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Sat, 14 Oct 2023 20:34:06 +0200 Subject: [PATCH 1/7] Set default timeout on curl handles - curl default implementation set a 2s default timeout - drain waits indefinitely until the predicate is valid --- src/datadog/curl.cpp | 50 ++++++++++++++++++++--------------- src/datadog/curl.h | 3 ++- src/datadog/datadog_agent.cpp | 3 +-- src/datadog/http_client.h | 2 +- test/mocks/http_clients.h | 2 +- test/test_curl.cpp | 12 ++++++--- 6 files changed, 42 insertions(+), 30 deletions(-) diff --git a/src/datadog/curl.cpp b/src/datadog/curl.cpp index 05fef196..8f0cbab8 100644 --- a/src/datadog/curl.cpp +++ b/src/datadog/curl.cpp @@ -25,6 +25,8 @@ namespace datadog { namespace tracing { namespace { +constexpr long k_default_timeout_ms = 2000; + // `libcurl` is the default implementation: it calls `curl_*` functions under // the hood. CurlLibrary libcurl; @@ -95,6 +97,10 @@ CURLcode CurlLibrary::easy_setopt_writefunction(CURL *handle, return curl_easy_setopt(handle, CURLOPT_WRITEFUNCTION, on_write); } +CURLcode CurlLibrary::easy_setopt_timeout(CURL *handle, long timeout_ms) { + return curl_easy_setopt(handle, CURLOPT_TIMEOUT_MS, timeout_ms); +} + const char *CurlLibrary::easy_strerror(CURLcode error) { return curl_easy_strerror(error); } @@ -229,7 +235,7 @@ class CurlImpl { std::string body, ResponseHandler on_response, ErrorHandler on_error); - void drain(std::chrono::steady_clock::time_point deadline); + void drain(); }; namespace { @@ -260,9 +266,7 @@ Expected Curl::post(const URL &url, HeadersSetter set_headers, return impl_->post(url, set_headers, body, on_response, on_error); } -void Curl::drain(std::chrono::steady_clock::time_point deadline) { - impl_->drain(deadline); -} +void Curl::drain() { impl_->drain(); } nlohmann::json Curl::config_json() const { return nlohmann::json::object({{"type", "datadog::tracing::Curl"}}); @@ -311,6 +315,9 @@ CurlImpl::~CurlImpl() { } log_on_error(curl_.multi_wakeup(multi_handle_)); event_loop_.join(); + + log_on_error(curl_.multi_cleanup(multi_handle_)); + curl_.global_cleanup(); } Expected CurlImpl::post(const HTTPClient::URL &url, @@ -323,9 +330,16 @@ Expected CurlImpl::post(const HTTPClient::URL &url, "failed to start."}; } + HeaderWriter writer{curl_}; + set_headers(writer); + auto cleanup_list = [&](auto list) { curl_.slist_free_all(list); }; + std::unique_ptr headers{ + writer.release(), std::move(cleanup_list)}; + auto request = std::make_unique(); request->curl = &curl_; + request->request_headers = headers.get(); request->request_body = std::move(body); request->on_response = std::move(on_response); request->on_error = std::move(on_error); @@ -339,6 +353,8 @@ Expected CurlImpl::post(const HTTPClient::URL &url, "unable to initialize a curl handle for request sending"}; } + throw_on_error( + curl_.easy_setopt_httpheader(handle.get(), request->request_headers)); throw_on_error(curl_.easy_setopt_private(handle.get(), request.get())); throw_on_error( curl_.easy_setopt_errorbuffer(handle.get(), request->error_buffer)); @@ -352,6 +368,7 @@ Expected CurlImpl::post(const HTTPClient::URL &url, throw_on_error(curl_.easy_setopt_headerdata(handle.get(), request.get())); throw_on_error(curl_.easy_setopt_writefunction(handle.get(), &on_read_body)); throw_on_error(curl_.easy_setopt_writedata(handle.get(), request.get())); + throw_on_error(curl_.easy_setopt_timeout(handle.get(), k_default_timeout_ms)); if (url.scheme == "unix" || url.scheme == "http+unix" || url.scheme == "https+unix") { throw_on_error(curl_.easy_setopt_unix_socket_path(handle.get(), @@ -365,25 +382,17 @@ Expected CurlImpl::post(const HTTPClient::URL &url, handle.get(), (url.scheme + "://" + url.authority + url.path).c_str())); } - HeaderWriter writer{curl_}; - set_headers(writer); - auto cleanup_list = [&](auto list) { curl_.slist_free_all(list); }; - std::unique_ptr headers{ - writer.release(), std::move(cleanup_list)}; - request->request_headers = headers.get(); - throw_on_error( - curl_.easy_setopt_httpheader(handle.get(), request->request_headers)); - std::list node; node.push_back(handle.get()); { std::lock_guard lock(mutex_); new_handles_.splice(new_handles_.end(), node); - headers.release(); - handle.release(); - request.release(); + (void)headers.release(); + (void)handle.release(); + (void)request.release(); } + log_on_error(curl_.multi_wakeup(multi_handle_)); return nullopt; @@ -391,9 +400,9 @@ Expected CurlImpl::post(const HTTPClient::URL &url, return Error{Error::CURL_REQUEST_SETUP_FAILED, curl_.easy_strerror(error)}; } -void CurlImpl::drain(std::chrono::steady_clock::time_point deadline) { +void CurlImpl::drain() { std::unique_lock lock(mutex_); - no_requests_.wait_until(lock, deadline, [this]() { + no_requests_.wait(lock, [this]() { return num_active_handles_ == 0 && new_handles_.empty(); }); } @@ -464,6 +473,7 @@ CURLMcode CurlImpl::log_on_error(CURLMcode result) { void CurlImpl::run() { int num_messages_remaining; CURLMsg *message; + const int max_wait_milliseconds = 1000; std::unique_lock lock(mutex_); for (;;) { @@ -478,8 +488,6 @@ void CurlImpl::run() { &num_messages_remaining))) { handle_message(*message, lock); } - - const int max_wait_milliseconds = 10 * 1000; lock.unlock(); log_on_error(curl_.multi_poll(multi_handle_, nullptr, 0, max_wait_milliseconds, nullptr)); @@ -510,8 +518,6 @@ void CurlImpl::run() { } request_handles_.clear(); - log_on_error(curl_.multi_cleanup(multi_handle_)); - curl_.global_cleanup(); } void CurlImpl::handle_message(const CURLMsg &message, diff --git a/src/datadog/curl.h b/src/datadog/curl.h index 3eeb0425..e046c5b8 100644 --- a/src/datadog/curl.h +++ b/src/datadog/curl.h @@ -59,6 +59,7 @@ class CurlLibrary { virtual CURLcode easy_setopt_url(CURL *handle, const char *url); virtual CURLcode easy_setopt_writedata(CURL *handle, void *data); virtual CURLcode easy_setopt_writefunction(CURL *handle, WriteCallback); + virtual CURLcode easy_setopt_timeout(CURL *handle, long timeout_ms); virtual const char *easy_strerror(CURLcode error); virtual void global_cleanup(); virtual CURLcode global_init(long flags); @@ -97,7 +98,7 @@ class Curl : public HTTPClient { std::string body, ResponseHandler on_response, ErrorHandler on_error) override; - void drain(std::chrono::steady_clock::time_point deadline) override; + void drain() override; nlohmann::json config_json() const override; }; diff --git a/src/datadog/datadog_agent.cpp b/src/datadog/datadog_agent.cpp index 072609a3..16609f42 100644 --- a/src/datadog/datadog_agent.cpp +++ b/src/datadog/datadog_agent.cpp @@ -185,7 +185,6 @@ DatadogAgent::DatadogAgent( } DatadogAgent::~DatadogAgent() { - const auto deadline = clock_().tick + std::chrono::seconds(2); cancel_scheduled_flush_(); flush(); if (tracer_telemetry_->enabled()) { @@ -196,7 +195,7 @@ DatadogAgent::~DatadogAgent() { // metric values. send_app_closing(); } - http_client_->drain(deadline); + http_client_->drain(); } Expected DatadogAgent::send( diff --git a/src/datadog/http_client.h b/src/datadog/http_client.h index 2a7310ac..57c4bb8d 100644 --- a/src/datadog/http_client.h +++ b/src/datadog/http_client.h @@ -51,7 +51,7 @@ class HTTPClient { // Wait until there are no more outstanding requests, or until the specified // `deadline`. - virtual void drain(std::chrono::steady_clock::time_point deadline) = 0; + virtual void drain() = 0; // Return a JSON representation of this object's configuration. The JSON // representation is an object with the following properties: diff --git a/test/mocks/http_clients.h b/test/mocks/http_clients.h index 1124861c..2d12937c 100644 --- a/test/mocks/http_clients.h +++ b/test/mocks/http_clients.h @@ -51,7 +51,7 @@ struct MockHTTPClient : public HTTPClient { return post_error; } - void drain(std::chrono::steady_clock::time_point /*deadline*/) override { + void drain() override { std::lock_guard lock{mutex_}; if (response_error && on_error_) { on_error_(*response_error); diff --git a/test/test_curl.cpp b/test/test_curl.cpp index 968f432e..2515ee44 100644 --- a/test/test_curl.cpp +++ b/test/test_curl.cpp @@ -69,6 +69,12 @@ class SingleRequestMockCurlLibrary : public CurlLibrary { on_write_ = on_write; return CURLE_OK; } + + CURLcode easy_setopt_timeout(CURL *, long) override { + // TBD? + return CURLE_OK; + } + CURLMcode multi_add_handle(CURLM *, CURL *easy_handle) override { added_handle_ = easy_handle; return CURLM_OK; @@ -188,7 +194,7 @@ TEST_CASE("parse response headers and body") { [&](const Error &error) { post_error = error; }); REQUIRE(result); - client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1)); + client->drain(); if (exception) { std::rethrow_exception(exception); } @@ -399,7 +405,7 @@ TEST_CASE("handles are always cleaned up") { [&](const Error &error) { post_error = error; }); REQUIRE(result); - client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1)); + client->drain(); if (exception) { std::rethrow_exception(exception); } @@ -416,7 +422,7 @@ TEST_CASE("handles are always cleaned up") { [&](const Error &error) { post_error = error; }); REQUIRE(result); - client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1)); + client->drain(); REQUIRE(post_error); } From 150d0d048c2c1d144317dcad4293e8decb50d0f2 Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Thu, 2 Nov 2023 16:06:08 +0100 Subject: [PATCH 2/7] Code review --- src/datadog/curl.cpp | 23 ++++++------ src/datadog/curl.h | 4 +-- src/datadog/datadog_agent.cpp | 5 +-- src/datadog/datadog_agent.h | 2 ++ src/datadog/datadog_agent_config.cpp | 16 +++++++++ src/datadog/datadog_agent_config.h | 6 ++++ src/datadog/http_client.h | 5 +-- test/mocks/http_clients.h | 5 +-- test/test_curl.cpp | 53 ++++++++++++++++++---------- 9 files changed, 81 insertions(+), 38 deletions(-) diff --git a/src/datadog/curl.cpp b/src/datadog/curl.cpp index 8f0cbab8..848b927f 100644 --- a/src/datadog/curl.cpp +++ b/src/datadog/curl.cpp @@ -25,8 +25,6 @@ namespace datadog { namespace tracing { namespace { -constexpr long k_default_timeout_ms = 2000; - // `libcurl` is the default implementation: it calls `curl_*` functions under // the hood. CurlLibrary libcurl; @@ -233,9 +231,9 @@ class CurlImpl { Expected post(const URL &url, HeadersSetter set_headers, std::string body, ResponseHandler on_response, - ErrorHandler on_error); + ErrorHandler on_error, HTTPClient::Timeout timeout); - void drain(); + void drain(std::chrono::steady_clock::time_point deadline); }; namespace { @@ -262,11 +260,13 @@ Curl::~Curl() { delete impl_; } Expected Curl::post(const URL &url, HeadersSetter set_headers, std::string body, ResponseHandler on_response, - ErrorHandler on_error) { - return impl_->post(url, set_headers, body, on_response, on_error); + ErrorHandler on_error, Timeout timeout) { + return impl_->post(url, set_headers, body, on_response, on_error, timeout); } -void Curl::drain() { impl_->drain(); } +void Curl::drain(std::chrono::steady_clock::time_point deadline) { + impl_->drain(deadline); +} nlohmann::json Curl::config_json() const { return nlohmann::json::object({{"type", "datadog::tracing::Curl"}}); @@ -323,7 +323,8 @@ CurlImpl::~CurlImpl() { Expected CurlImpl::post(const HTTPClient::URL &url, HeadersSetter set_headers, std::string body, ResponseHandler on_response, - ErrorHandler on_error) try { + ErrorHandler on_error, + HTTPClient::Timeout timeout) try { if (multi_handle_ == nullptr) { return Error{Error::CURL_HTTP_CLIENT_NOT_RUNNING, "Unable to send request via libcurl because the HTTP client " @@ -368,7 +369,7 @@ Expected CurlImpl::post(const HTTPClient::URL &url, throw_on_error(curl_.easy_setopt_headerdata(handle.get(), request.get())); throw_on_error(curl_.easy_setopt_writefunction(handle.get(), &on_read_body)); throw_on_error(curl_.easy_setopt_writedata(handle.get(), request.get())); - throw_on_error(curl_.easy_setopt_timeout(handle.get(), k_default_timeout_ms)); + throw_on_error(curl_.easy_setopt_timeout(handle.get(), timeout.count())); if (url.scheme == "unix" || url.scheme == "http+unix" || url.scheme == "https+unix") { throw_on_error(curl_.easy_setopt_unix_socket_path(handle.get(), @@ -400,9 +401,9 @@ Expected CurlImpl::post(const HTTPClient::URL &url, return Error{Error::CURL_REQUEST_SETUP_FAILED, curl_.easy_strerror(error)}; } -void CurlImpl::drain() { +void CurlImpl::drain(std::chrono::steady_clock::time_point deadline) { std::unique_lock lock(mutex_); - no_requests_.wait(lock, [this]() { + no_requests_.wait_until(lock, deadline, [this]() { return num_active_handles_ == 0 && new_handles_.empty(); }); } diff --git a/src/datadog/curl.h b/src/datadog/curl.h index e046c5b8..e793ac2f 100644 --- a/src/datadog/curl.h +++ b/src/datadog/curl.h @@ -96,9 +96,9 @@ class Curl : public HTTPClient { Expected post(const URL &url, HeadersSetter set_headers, std::string body, ResponseHandler on_response, - ErrorHandler on_error) override; + ErrorHandler on_error, Timeout timeout) override; - void drain() override; + void drain(std::chrono::steady_clock::time_point deadline) override; nlohmann::json config_json() const override; }; diff --git a/src/datadog/datadog_agent.cpp b/src/datadog/datadog_agent.cpp index 16609f42..4f33fcc5 100644 --- a/src/datadog/datadog_agent.cpp +++ b/src/datadog/datadog_agent.cpp @@ -185,6 +185,7 @@ DatadogAgent::DatadogAgent( } DatadogAgent::~DatadogAgent() { + const auto deadline = clock_().tick + shutdown_timeout_; cancel_scheduled_flush_(); flush(); if (tracer_telemetry_->enabled()) { @@ -195,7 +196,7 @@ DatadogAgent::~DatadogAgent() { // metric values. send_app_closing(); } - http_client_->drain(); + http_client_->drain(deadline); } Expected DatadogAgent::send( @@ -325,7 +326,7 @@ void DatadogAgent::flush() { tracer_telemetry_->metrics().trace_api.requests.inc(); auto post_result = http_client_->post( traces_endpoint_, std::move(set_request_headers), std::move(body), - std::move(on_response), std::move(on_error)); + std::move(on_response), std::move(on_error), request_timeout_); if (auto* error = post_result.if_error()) { logger_->log_error( error->with_prefix("Unexpected error submitting traces: ")); diff --git a/src/datadog/datadog_agent.h b/src/datadog/datadog_agent.h index a18fea61..583f0b6b 100644 --- a/src/datadog/datadog_agent.h +++ b/src/datadog/datadog_agent.h @@ -49,6 +49,8 @@ class DatadogAgent : public Collector { HTTPClient::HeadersSetter telemetry_set_request_headers_; HTTPClient::ResponseHandler telemetry_on_response_; HTTPClient::ErrorHandler telemetry_on_error_; + std::chrono::steady_clock::duration request_timeout_; + std::chrono::steady_clock::duration shutdown_timeout_; void flush(); void send_heartbeat_and_telemetry(); diff --git a/src/datadog/datadog_agent_config.cpp b/src/datadog/datadog_agent_config.cpp index 616be3c3..a10b0e56 100644 --- a/src/datadog/datadog_agent_config.cpp +++ b/src/datadog/datadog_agent_config.cpp @@ -114,6 +114,22 @@ Expected finalize_config( result.flush_interval = std::chrono::milliseconds(config.flush_interval_milliseconds); + if (config.request_timeout_milliseconds <= 0) { + return Error{Error::DATADOG_AGENT_INVALID_FLUSH_INTERVAL, + "DatadogAgent: Request timeout must be a positive number of " + "milliseconds."}; + } + result.request_timeout = + std::chrono::milliseconds(config.request_timeout_milliseconds); + + if (config.shutdown_timeout_milliseconds <= 0) { + return Error{Error::DATADOG_AGENT_INVALID_FLUSH_INTERVAL, + "DatadogAgent: Shutdown timeout must be a positive number of " + "milliseconds."}; + } + result.shutdown_timeout = + std::chrono::milliseconds(config.shutdown_timeout_milliseconds); + auto env_host = lookup(environment::DD_AGENT_HOST); auto env_port = lookup(environment::DD_TRACE_AGENT_PORT); diff --git a/src/datadog/datadog_agent_config.h b/src/datadog/datadog_agent_config.h index 18b1e690..6e91f209 100644 --- a/src/datadog/datadog_agent_config.h +++ b/src/datadog/datadog_agent_config.h @@ -49,6 +49,10 @@ struct DatadogAgentConfig { std::string url = "http://localhost:8126"; // How often, in milliseconds, to send batches of traces to the Datadog Agent. int flush_interval_milliseconds = 2000; + // TBD + int request_timeout_milliseconds = 2000; + // TBD + int shutdown_timeout_milliseconds = 2000; static Expected parse(StringView); }; @@ -64,6 +68,8 @@ class FinalizedDatadogAgentConfig { std::shared_ptr event_scheduler; HTTPClient::URL url; std::chrono::steady_clock::duration flush_interval; + std::chrono::steady_clock::duration request_timeout; + std::chrono::steady_clock::duration shutdown_timeout; }; Expected finalize_config( diff --git a/src/datadog/http_client.h b/src/datadog/http_client.h index 57c4bb8d..0edc0318 100644 --- a/src/datadog/http_client.h +++ b/src/datadog/http_client.h @@ -36,6 +36,7 @@ class HTTPClient { // `ErrorHandler` is for errors encountered by `HTTPClient`, not for // error-indicating HTTP responses. using ErrorHandler = std::function; + using Timeout = std::chrono::steady_clock::duration; // Send a POST request to the specified `url`. Set request headers by calling // the specified `set_headers` callback. Include the specified `body` at the @@ -47,11 +48,11 @@ class HTTPClient { // either of `on_response` or `on_error` throws an exception. virtual Expected post(const URL& url, HeadersSetter set_headers, std::string body, ResponseHandler on_response, - ErrorHandler on_error) = 0; + ErrorHandler on_error, Timeout timeout) = 0; // Wait until there are no more outstanding requests, or until the specified // `deadline`. - virtual void drain() = 0; + virtual void drain(std::chrono::steady_clock::time_point deadline) = 0; // Return a JSON representation of this object's configuration. The JSON // representation is an object with the following properties: diff --git a/test/mocks/http_clients.h b/test/mocks/http_clients.h index 2d12937c..e2b4efd3 100644 --- a/test/mocks/http_clients.h +++ b/test/mocks/http_clients.h @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -41,7 +42,7 @@ struct MockHTTPClient : public HTTPClient { Expected post(const URL& /* url */, HeadersSetter set_headers, std::string /*body*/, ResponseHandler on_response, - ErrorHandler on_error) override { + ErrorHandler on_error, Timeout /*timeout*/) override { std::lock_guard lock{mutex_}; if (!post_error) { on_response_ = on_response; @@ -51,7 +52,7 @@ struct MockHTTPClient : public HTTPClient { return post_error; } - void drain() override { + void drain(std::chrono::steady_clock::time_point /*deadline*/) override { std::lock_guard lock{mutex_}; if (response_error && on_error_) { on_error_(*response_error); diff --git a/test/test_curl.cpp b/test/test_curl.cpp index 2515ee44..f686cd42 100644 --- a/test/test_curl.cpp +++ b/test/test_curl.cpp @@ -15,6 +15,7 @@ #include "test.h" using namespace datadog::tracing; +using namespace std::chrono_literals; class SingleRequestMockCurlLibrary : public CurlLibrary { public: @@ -137,7 +138,7 @@ class SingleRequestMockCurlLibrary : public CurlLibrary { } }; -TEST_CASE("parse response headers and body") { +TEST_CASE("parse response headers and body", "[curl]") { const auto logger = std::make_shared(); SingleRequestMockCurlLibrary library; const auto client = std::make_shared(logger, library); @@ -191,10 +192,11 @@ TEST_CASE("parse response headers and body") { exception = std::current_exception(); } }, - [&](const Error &error) { post_error = error; }); + [&](const Error &error) { post_error = error; }, + std::chrono::seconds(0)); REQUIRE(result); - client->drain(); + client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1)); if (exception) { std::rethrow_exception(exception); } @@ -202,7 +204,7 @@ TEST_CASE("parse response headers and body") { } } -TEST_CASE("bad multi-handle means error mode") { +TEST_CASE("bad multi-handle means error mode", "[curl]") { // If libcurl fails to allocate a multi-handle, then the HTTP client enters a // mode where calls to `post` always return an error. class MockCurlLibrary : public CurlLibrary { @@ -216,12 +218,14 @@ TEST_CASE("bad multi-handle means error mode") { const auto ignore = [](auto &&...) {}; const HTTPClient::URL url = {"http", "whatever", ""}; - const auto result = client->post(url, ignore, "dummy body", ignore, ignore); + const auto ignore_timeout = std::chrono::seconds(10); + const auto result = + client->post(url, ignore, "dummy body", ignore, ignore, ignore_timeout); REQUIRE_FALSE(result); REQUIRE(result.error().code == Error::CURL_HTTP_CLIENT_NOT_RUNNING); } -TEST_CASE("bad std::thread means error mode") { +TEST_CASE("bad std::thread means error mode", "[curl]") { // If `Curl` is unable to start its event loop thread, then it enters a mode // where calls to `post` always return an error. const auto logger = std::make_shared(); @@ -234,13 +238,15 @@ TEST_CASE("bad std::thread means error mode") { REQUIRE(logger->first_error().code == Error::CURL_HTTP_CLIENT_SETUP_FAILED); const auto ignore = [](auto &&...) {}; + const auto ignore_timeout = std::chrono::seconds(10); const HTTPClient::URL url = {"http", "whatever", ""}; - const auto result = client->post(url, ignore, "dummy body", ignore, ignore); + const auto result = + client->post(url, ignore, "dummy body", ignore, ignore, ignore_timeout); REQUIRE_FALSE(result); REQUIRE(result.error().code == Error::CURL_HTTP_CLIENT_NOT_RUNNING); } -TEST_CASE("fail to allocate request handle") { +TEST_CASE("fail to allocate request handle", "[curl]") { // Each call to `Curl::post` allocates a new "easy handle." If that fails, // then `post` immediately returns an error. class MockCurlLibrary : public CurlLibrary { @@ -254,12 +260,14 @@ TEST_CASE("fail to allocate request handle") { const auto ignore = [](auto &&...) {}; const HTTPClient::URL url = {"http", "whatever", ""}; - const auto result = client->post(url, ignore, "dummy body", ignore, ignore); + const auto ignore_timeout = std::chrono::seconds(10); + const auto result = + client->post(url, ignore, "dummy body", ignore, ignore, ignore_timeout); REQUIRE_FALSE(result); REQUIRE(result.error().code == Error::CURL_REQUEST_SETUP_FAILED); } -TEST_CASE("setopt failures") { +TEST_CASE("setopt failures", "[curl]") { // Each call to `Curl::post` allocates a new "easy handle" and sets various // options on it. Any of those setters can fail. When one does, `post` // immediately returns an error. @@ -377,12 +385,15 @@ TEST_CASE("setopt failures") { url.authority = "localhost"; url.path = "/trace/thing"; } - const auto result = client->post(url, ignore, "dummy body", ignore, ignore); + + const auto ignore_timeout = std::chrono::seconds(0); + const auto result = + client->post(url, ignore, "dummy body", ignore, ignore, ignore_timeout); REQUIRE_FALSE(result); REQUIRE(result.error().code == Error::CURL_REQUEST_SETUP_FAILED); } -TEST_CASE("handles are always cleaned up") { +TEST_CASE("handles are always cleaned up", "[curl]") { const auto logger = std::make_shared(); SingleRequestMockCurlLibrary library; auto client = std::make_shared(logger, library); @@ -391,6 +402,7 @@ TEST_CASE("handles are always cleaned up") { Optional post_error; std::exception_ptr exception; const HTTPClient::URL url = {"http", "whatever", ""}; + const auto ignore_timeout = std::chrono::seconds(10); const auto result = client->post( url, [](const auto &) {}, "whatever", [&](int status, const DictReader & /*headers*/, std::string body) { @@ -402,10 +414,10 @@ TEST_CASE("handles are always cleaned up") { exception = std::current_exception(); } }, - [&](const Error &error) { post_error = error; }); + [&](const Error &error) { post_error = error; }, ignore_timeout); REQUIRE(result); - client->drain(); + client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1)); if (exception) { std::rethrow_exception(exception); } @@ -416,21 +428,24 @@ TEST_CASE("handles are always cleaned up") { Optional post_error; const HTTPClient::URL url = {"http", "whatever", ""}; const auto ignore = [](auto &&...) {}; + const auto ignore_timeout = std::chrono::seconds(10); library.message_result_ = CURLE_COULDNT_CONNECT; // any error would do - const auto result = - client->post(url, ignore, "whatever", ignore, - [&](const Error &error) { post_error = error; }); + const auto result = client->post( + url, ignore, "whatever", ignore, + [&](const Error &error) { post_error = error; }, ignore_timeout); REQUIRE(result); - client->drain(); + client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1)); REQUIRE(post_error); } SECTION("when we shut down while a request is in flight") { const HTTPClient::URL url = {"http", "whatever", ""}; const auto ignore = [](auto &&...) {}; + const auto ignore_timeout = std::chrono::seconds(10); library.delay_message_ = true; - const auto result = client->post(url, ignore, "whatever", ignore, ignore); + const auto result = + client->post(url, ignore, "whatever", ignore, ignore, ignore_timeout); REQUIRE(result); // Destroy the `Curl` object. From 8517ec7c642a80a133f93f3df1935ebc9a9e735e Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Thu, 9 Nov 2023 11:43:07 +0100 Subject: [PATCH 3/7] [skip ci] Code review: use deadline instead - Need to talk about `clock` --- src/datadog/curl.cpp | 38 +++++++++++++++++++++---- src/datadog/curl.h | 2 +- src/datadog/datadog_agent.cpp | 24 ++++++++-------- src/datadog/http_client.h | 6 ++-- src/datadog/tracer.cpp | 53 +++++++++++++---------------------- test/mocks/http_clients.h | 2 +- test/test_curl.cpp | 37 ++++++++++++++---------- 7 files changed, 91 insertions(+), 71 deletions(-) diff --git a/src/datadog/curl.cpp b/src/datadog/curl.cpp index 848b927f..fab568e4 100644 --- a/src/datadog/curl.cpp +++ b/src/datadog/curl.cpp @@ -13,6 +13,7 @@ #include #include +#include "clock.h" #include "dict_reader.h" #include "dict_writer.h" #include "http_client.h" @@ -173,6 +174,7 @@ class CurlImpl { int num_active_handles_; std::condition_variable no_requests_; std::thread event_loop_; + Clock clock_; struct Request { CurlLibrary *curl = nullptr; @@ -183,6 +185,7 @@ class CurlImpl { char error_buffer[CURL_ERROR_SIZE] = ""; std::unordered_map response_headers_lower; std::string response_body; + HTTPClient::Deadline deadline; ~Request(); }; @@ -231,7 +234,7 @@ class CurlImpl { Expected post(const URL &url, HeadersSetter set_headers, std::string body, ResponseHandler on_response, - ErrorHandler on_error, HTTPClient::Timeout timeout); + ErrorHandler on_error, HTTPClient::Deadline deadline); void drain(std::chrono::steady_clock::time_point deadline); }; @@ -260,8 +263,8 @@ Curl::~Curl() { delete impl_; } Expected Curl::post(const URL &url, HeadersSetter set_headers, std::string body, ResponseHandler on_response, - ErrorHandler on_error, Timeout timeout) { - return impl_->post(url, set_headers, body, on_response, on_error, timeout); + ErrorHandler on_error, Deadline deadline) { + return impl_->post(url, set_headers, body, on_response, on_error, deadline); } void Curl::drain(std::chrono::steady_clock::time_point deadline) { @@ -324,7 +327,7 @@ Expected CurlImpl::post(const HTTPClient::URL &url, HeadersSetter set_headers, std::string body, ResponseHandler on_response, ErrorHandler on_error, - HTTPClient::Timeout timeout) try { + HTTPClient::Deadline deadline) try { if (multi_handle_ == nullptr) { return Error{Error::CURL_HTTP_CLIENT_NOT_RUNNING, "Unable to send request via libcurl because the HTTP client " @@ -344,6 +347,7 @@ Expected CurlImpl::post(const HTTPClient::URL &url, request->request_body = std::move(body); request->on_response = std::move(on_response); request->on_error = std::move(on_error); + request->deadline = std::move(deadline); auto cleanup_handle = [&](auto handle) { curl_.easy_cleanup(handle); }; std::unique_ptr handle{ @@ -369,7 +373,6 @@ Expected CurlImpl::post(const HTTPClient::URL &url, throw_on_error(curl_.easy_setopt_headerdata(handle.get(), request.get())); throw_on_error(curl_.easy_setopt_writefunction(handle.get(), &on_read_body)); throw_on_error(curl_.easy_setopt_writedata(handle.get(), request.get())); - throw_on_error(curl_.easy_setopt_timeout(handle.get(), timeout.count())); if (url.scheme == "unix" || url.scheme == "http+unix" || url.scheme == "https+unix") { throw_on_error(curl_.easy_setopt_unix_socket_path(handle.get(), @@ -496,7 +499,30 @@ void CurlImpl::run() { // New requests might have been added while we were sleeping. for (; !new_handles_.empty(); new_handles_.pop_front()) { - CURL *const handle = new_handles_.front(); + CURL *handle = new_handles_.front(); + char *user_data; + if (log_on_error(curl_.easy_getinfo_private(handle, &user_data)) != + CURLE_OK) { + curl_.easy_cleanup(handle); + continue; + } + + auto *request = reinterpret_cast(user_data); + const auto timeout = request->deadline - clock_().tick; + if (timeout <= HTTPClient::Deadline::duration::zero()) { + request->on_error( + Error{Error::CURL_REQUEST_FAILURE, + "Request not processed as the deadline has been reached."}); + + curl_.easy_cleanup(handle); + delete request; + + continue; + } + + throw_on_error(curl_.easy_setopt_timeout( + handle, std::chrono::duration_cast(timeout) + .count())); log_on_error(curl_.multi_add_handle(multi_handle_, handle)); request_handles_.insert(handle); } diff --git a/src/datadog/curl.h b/src/datadog/curl.h index e793ac2f..7c9cf6f7 100644 --- a/src/datadog/curl.h +++ b/src/datadog/curl.h @@ -96,7 +96,7 @@ class Curl : public HTTPClient { Expected post(const URL &url, HeadersSetter set_headers, std::string body, ResponseHandler on_response, - ErrorHandler on_error, Timeout timeout) override; + ErrorHandler on_error, Deadline deadline) override; void drain(std::chrono::steady_clock::time_point deadline) override; diff --git a/src/datadog/datadog_agent.cpp b/src/datadog/datadog_agent.cpp index 4f33fcc5..a34ba585 100644 --- a/src/datadog/datadog_agent.cpp +++ b/src/datadog/datadog_agent.cpp @@ -144,7 +144,9 @@ DatadogAgent::DatadogAgent( event_scheduler_(config.event_scheduler), cancel_scheduled_flush_(event_scheduler_->schedule_recurring_event( config.flush_interval, [this]() { flush(); })), - flush_interval_(config.flush_interval) { + flush_interval_(config.flush_interval), + request_timeout_(config.request_timeout), + shutdown_timeout_(config.shutdown_timeout) { assert(logger_); assert(tracer_telemetry_); if (tracer_telemetry_->enabled()) { @@ -208,17 +210,14 @@ Expected DatadogAgent::send( } nlohmann::json DatadogAgent::config_json() const { - const auto flush_interval_milliseconds = - std::chrono::duration_cast(flush_interval_) - .count(); - // clang-format off return nlohmann::json::object({ {"type", "datadog::tracing::DatadogAgent"}, {"config", nlohmann::json::object({ {"traces_url", (traces_endpoint_.scheme + "://" + traces_endpoint_.authority + traces_endpoint_.path)}, {"telemetry_url", (telemetry_endpoint_.scheme + "://" + telemetry_endpoint_.authority + telemetry_endpoint_.path)}, - {"flush_interval_milliseconds", flush_interval_milliseconds}, + {"request_timeout_milliseconds", std::chrono::duration_cast(request_timeout_).count() }, + {"shutdown_timeout_milliseconds", std::chrono::duration_cast(request_timeout_).count() }, {"http_client", http_client_->config_json()}, {"event_scheduler", event_scheduler_->config_json()}, })}, @@ -324,9 +323,10 @@ void DatadogAgent::flush() { }; tracer_telemetry_->metrics().trace_api.requests.inc(); - auto post_result = http_client_->post( - traces_endpoint_, std::move(set_request_headers), std::move(body), - std::move(on_response), std::move(on_error), request_timeout_); + auto post_result = + http_client_->post(traces_endpoint_, std::move(set_request_headers), + std::move(body), std::move(on_response), + std::move(on_error), clock_().tick + request_timeout_); if (auto* error = post_result.if_error()) { logger_->log_error( error->with_prefix("Unexpected error submitting traces: ")); @@ -337,7 +337,7 @@ void DatadogAgent::send_app_started() { auto payload = tracer_telemetry_->app_started(); auto post_result = http_client_->post( telemetry_endpoint_, telemetry_set_request_headers_, std::move(payload), - telemetry_on_response_, telemetry_on_error_); + telemetry_on_response_, telemetry_on_error_, clock_().tick + request_timeout_); if (auto* error = post_result.if_error()) { logger_->log_error(error->with_prefix( "Unexpected error submitting telemetry app-started event: ")); @@ -348,7 +348,7 @@ void DatadogAgent::send_heartbeat_and_telemetry() { auto payload = tracer_telemetry_->heartbeat_and_telemetry(); auto post_result = http_client_->post( telemetry_endpoint_, telemetry_set_request_headers_, std::move(payload), - telemetry_on_response_, telemetry_on_error_); + telemetry_on_response_, telemetry_on_error_, clock_().tick + request_timeout_); if (auto* error = post_result.if_error()) { logger_->log_error(error->with_prefix( "Unexpected error submitting telemetry app-heartbeat event: ")); @@ -359,7 +359,7 @@ void DatadogAgent::send_app_closing() { auto payload = tracer_telemetry_->app_closing(); auto post_result = http_client_->post( telemetry_endpoint_, telemetry_set_request_headers_, std::move(payload), - telemetry_on_response_, telemetry_on_error_); + telemetry_on_response_, telemetry_on_error_, clock_().tick + request_timeout_); if (auto* error = post_result.if_error()) { logger_->log_error(error->with_prefix( "Unexpected error submitting telemetry app-closing event: ")); diff --git a/src/datadog/http_client.h b/src/datadog/http_client.h index 0edc0318..1eef244b 100644 --- a/src/datadog/http_client.h +++ b/src/datadog/http_client.h @@ -36,7 +36,7 @@ class HTTPClient { // `ErrorHandler` is for errors encountered by `HTTPClient`, not for // error-indicating HTTP responses. using ErrorHandler = std::function; - using Timeout = std::chrono::steady_clock::duration; + using Deadline = std::chrono::steady_clock::time_point; // Send a POST request to the specified `url`. Set request headers by calling // the specified `set_headers` callback. Include the specified `body` at the @@ -48,11 +48,11 @@ class HTTPClient { // either of `on_response` or `on_error` throws an exception. virtual Expected post(const URL& url, HeadersSetter set_headers, std::string body, ResponseHandler on_response, - ErrorHandler on_error, Timeout timeout) = 0; + ErrorHandler on_error, Deadline deadline) = 0; // Wait until there are no more outstanding requests, or until the specified // `deadline`. - virtual void drain(std::chrono::steady_clock::time_point deadline) = 0; + virtual void drain(Deadline deadline) = 0; // Return a JSON representation of this object's configuration. The JSON // representation is an object with the following properties: diff --git a/src/datadog/tracer.cpp b/src/datadog/tracer.cpp index aacdf8a6..931978c4 100644 --- a/src/datadog/tracer.cpp +++ b/src/datadog/tracer.cpp @@ -197,35 +197,6 @@ Expected extract_b3( return result; } -nlohmann::json make_config_json( - StringView tracer_version_string, const Collector& collector, - const SpanDefaults& defaults, const RuntimeID& runtime_id, - const TraceSampler& trace_sampler, const SpanSampler& span_sampler, - const std::vector& injection_styles, - const std::vector& extraction_styles, - const Optional& hostname, std::size_t tags_header_max_size) { - // clang-format off - auto config = nlohmann::json::object({ - {"version", tracer_version_string}, - {"defaults", to_json(defaults)}, - {"runtime_id", runtime_id.string()}, - {"collector", collector.config_json()}, - {"trace_sampler", trace_sampler.config_json()}, - {"span_sampler", span_sampler.config_json()}, - {"injection_styles", to_json(injection_styles)}, - {"extraction_styles", to_json(extraction_styles)}, - {"tags_header_size", tags_header_max_size}, - {"environment_variables", environment::to_json()}, - }); - // clang-format on - - if (hostname) { - config["hostname"] = *hostname; - } - - return config; -} - } // namespace Tracer::Tracer(const FinalizedTracerConfig& config) @@ -280,10 +251,26 @@ Tracer::Tracer(const FinalizedTracerConfig& config, } nlohmann::json Tracer::config_json() const { - return make_config_json(tracer_version_string, *collector_, *defaults_, - runtime_id_, *trace_sampler_, *span_sampler_, - injection_styles_, extraction_styles_, hostname_, - tags_header_max_size_); + // clang-format off + auto config = nlohmann::json::object({ + {"version", tracer_version_string}, + {"defaults", to_json(*defaults_)}, + {"runtime_id", runtime_id_.string()}, + {"collector", collector_->config_json()}, + {"trace_sampler", trace_sampler_->config_json()}, + {"span_sampler", span_sampler_->config_json()}, + {"injection_styles", to_json(injection_styles_)}, + {"extraction_styles", to_json(extraction_styles_)}, + {"tags_header_size", tags_header_max_size_}, + {"environment_variables", environment::to_json()}, + }); + // clang-format on + + if (hostname_) { + config["hostname"] = *hostname_; + } + + return config; } Span Tracer::create_span() { return create_span(SpanConfig{}); } diff --git a/test/mocks/http_clients.h b/test/mocks/http_clients.h index e2b4efd3..da7b6e33 100644 --- a/test/mocks/http_clients.h +++ b/test/mocks/http_clients.h @@ -42,7 +42,7 @@ struct MockHTTPClient : public HTTPClient { Expected post(const URL& /* url */, HeadersSetter set_headers, std::string /*body*/, ResponseHandler on_response, - ErrorHandler on_error, Timeout /*timeout*/) override { + ErrorHandler on_error, Deadline /*deadline*/) override { std::lock_guard lock{mutex_}; if (!post_error) { on_response_ = on_response; diff --git a/test/test_curl.cpp b/test/test_curl.cpp index f686cd42..fa7acefb 100644 --- a/test/test_curl.cpp +++ b/test/test_curl.cpp @@ -193,7 +193,7 @@ TEST_CASE("parse response headers and body", "[curl]") { } }, [&](const Error &error) { post_error = error; }, - std::chrono::seconds(0)); + std::chrono::steady_clock::now() + std::chrono::seconds(10)); REQUIRE(result); client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1)); @@ -218,9 +218,10 @@ TEST_CASE("bad multi-handle means error mode", "[curl]") { const auto ignore = [](auto &&...) {}; const HTTPClient::URL url = {"http", "whatever", ""}; - const auto ignore_timeout = std::chrono::seconds(10); + const auto dummy_timeout = + std::chrono::steady_clock::now() + std::chrono::seconds(10); const auto result = - client->post(url, ignore, "dummy body", ignore, ignore, ignore_timeout); + client->post(url, ignore, "dummy body", ignore, ignore, dummy_timeout); REQUIRE_FALSE(result); REQUIRE(result.error().code == Error::CURL_HTTP_CLIENT_NOT_RUNNING); } @@ -238,10 +239,11 @@ TEST_CASE("bad std::thread means error mode", "[curl]") { REQUIRE(logger->first_error().code == Error::CURL_HTTP_CLIENT_SETUP_FAILED); const auto ignore = [](auto &&...) {}; - const auto ignore_timeout = std::chrono::seconds(10); + const auto dummy_timeout = + std::chrono::steady_clock::now() + std::chrono::seconds(10); const HTTPClient::URL url = {"http", "whatever", ""}; const auto result = - client->post(url, ignore, "dummy body", ignore, ignore, ignore_timeout); + client->post(url, ignore, "dummy body", ignore, ignore, dummy_timeout); REQUIRE_FALSE(result); REQUIRE(result.error().code == Error::CURL_HTTP_CLIENT_NOT_RUNNING); } @@ -260,9 +262,10 @@ TEST_CASE("fail to allocate request handle", "[curl]") { const auto ignore = [](auto &&...) {}; const HTTPClient::URL url = {"http", "whatever", ""}; - const auto ignore_timeout = std::chrono::seconds(10); + const auto dummy_timeout = + std::chrono::steady_clock::now() + std::chrono::seconds(10); const auto result = - client->post(url, ignore, "dummy body", ignore, ignore, ignore_timeout); + client->post(url, ignore, "dummy body", ignore, ignore, dummy_timeout); REQUIRE_FALSE(result); REQUIRE(result.error().code == Error::CURL_REQUEST_SETUP_FAILED); } @@ -386,9 +389,10 @@ TEST_CASE("setopt failures", "[curl]") { url.path = "/trace/thing"; } - const auto ignore_timeout = std::chrono::seconds(0); + const auto dummy_timeout = + std::chrono::steady_clock::now() + std::chrono::seconds(10); const auto result = - client->post(url, ignore, "dummy body", ignore, ignore, ignore_timeout); + client->post(url, ignore, "dummy body", ignore, ignore, dummy_timeout); REQUIRE_FALSE(result); REQUIRE(result.error().code == Error::CURL_REQUEST_SETUP_FAILED); } @@ -402,7 +406,8 @@ TEST_CASE("handles are always cleaned up", "[curl]") { Optional post_error; std::exception_ptr exception; const HTTPClient::URL url = {"http", "whatever", ""}; - const auto ignore_timeout = std::chrono::seconds(10); + const auto dummy_timeout = + std::chrono::steady_clock::now() + std::chrono::seconds(10); const auto result = client->post( url, [](const auto &) {}, "whatever", [&](int status, const DictReader & /*headers*/, std::string body) { @@ -414,7 +419,7 @@ TEST_CASE("handles are always cleaned up", "[curl]") { exception = std::current_exception(); } }, - [&](const Error &error) { post_error = error; }, ignore_timeout); + [&](const Error &error) { post_error = error; }, dummy_timeout); REQUIRE(result); client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1)); @@ -428,11 +433,12 @@ TEST_CASE("handles are always cleaned up", "[curl]") { Optional post_error; const HTTPClient::URL url = {"http", "whatever", ""}; const auto ignore = [](auto &&...) {}; - const auto ignore_timeout = std::chrono::seconds(10); + const auto dummy_timeout = + std::chrono::steady_clock::now() + std::chrono::seconds(10); library.message_result_ = CURLE_COULDNT_CONNECT; // any error would do const auto result = client->post( url, ignore, "whatever", ignore, - [&](const Error &error) { post_error = error; }, ignore_timeout); + [&](const Error &error) { post_error = error; }, dummy_timeout); REQUIRE(result); client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1)); @@ -442,10 +448,11 @@ TEST_CASE("handles are always cleaned up", "[curl]") { SECTION("when we shut down while a request is in flight") { const HTTPClient::URL url = {"http", "whatever", ""}; const auto ignore = [](auto &&...) {}; - const auto ignore_timeout = std::chrono::seconds(10); + const auto dummy_timeout = + std::chrono::steady_clock::now() + std::chrono::seconds(10); library.delay_message_ = true; const auto result = - client->post(url, ignore, "whatever", ignore, ignore, ignore_timeout); + client->post(url, ignore, "whatever", ignore, ignore, dummy_timeout); REQUIRE(result); // Destroy the `Curl` object. From 4f747d995c380d38472b13b82f58ff7035239f60 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Fri, 10 Nov 2023 10:07:24 -0500 Subject: [PATCH 4/7] code review suggestions: (#69) - `easy_setopt_timeout` -> `easy_setopt_timeout_ms` - remove `HTTPClient::Deadline` alias - pass `Clock` into `CurlImpl` - `finalize_config(const TracerConfig&, const Clock&)` - `FinalizedTracerConfig::clock` - `finalize_config(const DatadogAgentConfig&, const Clock&)` - `default_http_client(const shared_ptr&, const Clock&)` - `FinalizedDatadogAgentConfig::clock` - `DatadogAgent::DatadogAgent` - more specific `Error::CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START`, and a little context in the error message - `dummy_timeout` -> `dummy_deadline` - `TEST_CASE` for `Error::CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START` --- src/datadog/curl.cpp | 61 ++++++++++++--------- src/datadog/curl.h | 13 +++-- src/datadog/datadog_agent.cpp | 4 +- src/datadog/datadog_agent.h | 2 +- src/datadog/datadog_agent_config.cpp | 7 ++- src/datadog/datadog_agent_config.h | 7 ++- src/datadog/default_http_client.h | 4 +- src/datadog/default_http_client_curl.cpp | 4 +- src/datadog/default_http_client_null.cpp | 4 +- src/datadog/error.h | 1 + src/datadog/http_client.h | 13 +++-- src/datadog/tracer.cpp | 20 +++---- src/datadog/tracer.h | 4 -- src/datadog/tracer_config.cpp | 8 ++- src/datadog/tracer_config.h | 9 +++- test/mocks/http_clients.h | 7 +-- test/test_curl.cpp | 67 +++++++++++++++--------- test/test_trace_sampler.cpp | 9 ++-- test/test_tracer.cpp | 4 +- 19 files changed, 147 insertions(+), 101 deletions(-) diff --git a/src/datadog/curl.cpp b/src/datadog/curl.cpp index fab568e4..fb5e1e0b 100644 --- a/src/datadog/curl.cpp +++ b/src/datadog/curl.cpp @@ -96,7 +96,7 @@ CURLcode CurlLibrary::easy_setopt_writefunction(CURL *handle, return curl_easy_setopt(handle, CURLOPT_WRITEFUNCTION, on_write); } -CURLcode CurlLibrary::easy_setopt_timeout(CURL *handle, long timeout_ms) { +CURLcode CurlLibrary::easy_setopt_timeout_ms(CURL *handle, long timeout_ms) { return curl_easy_setopt(handle, CURLOPT_TIMEOUT_MS, timeout_ms); } @@ -167,6 +167,7 @@ class CurlImpl { std::mutex mutex_; CurlLibrary &curl_; const std::shared_ptr logger_; + Clock clock_; CURLM *multi_handle_; std::unordered_set request_handles_; std::list new_handles_; @@ -174,7 +175,6 @@ class CurlImpl { int num_active_handles_; std::condition_variable no_requests_; std::thread event_loop_; - Clock clock_; struct Request { CurlLibrary *curl = nullptr; @@ -185,7 +185,7 @@ class CurlImpl { char error_buffer[CURL_ERROR_SIZE] = ""; std::unordered_map response_headers_lower; std::string response_body; - HTTPClient::Deadline deadline; + std::chrono::steady_clock::time_point deadline; ~Request(); }; @@ -228,13 +228,14 @@ class CurlImpl { static StringView trim(StringView); public: - explicit CurlImpl(const std::shared_ptr &, CurlLibrary &, - const Curl::ThreadGenerator &); + explicit CurlImpl(const std::shared_ptr &, const Clock &, + CurlLibrary &, const Curl::ThreadGenerator &); ~CurlImpl(); Expected post(const URL &url, HeadersSetter set_headers, std::string body, ResponseHandler on_response, - ErrorHandler on_error, HTTPClient::Deadline deadline); + ErrorHandler on_error, + std::chrono::steady_clock::time_point deadline); void drain(std::chrono::steady_clock::time_point deadline); }; @@ -249,21 +250,24 @@ void throw_on_error(CURLcode result) { } // namespace -Curl::Curl(const std::shared_ptr &logger) : Curl(logger, libcurl) {} +Curl::Curl(const std::shared_ptr &logger, const Clock &clock) + : Curl(logger, clock, libcurl) {} -Curl::Curl(const std::shared_ptr &logger, CurlLibrary &curl) - : Curl(logger, curl, +Curl::Curl(const std::shared_ptr &logger, const Clock &clock, + CurlLibrary &curl) + : Curl(logger, clock, curl, [](auto &&func) { return std::thread(std::move(func)); }) {} -Curl::Curl(const std::shared_ptr &logger, CurlLibrary &curl, - const Curl::ThreadGenerator &make_thread) - : impl_(new CurlImpl{logger, curl, make_thread}) {} +Curl::Curl(const std::shared_ptr &logger, const Clock &clock, + CurlLibrary &curl, const Curl::ThreadGenerator &make_thread) + : impl_(new CurlImpl{logger, clock, curl, make_thread}) {} Curl::~Curl() { delete impl_; } Expected Curl::post(const URL &url, HeadersSetter set_headers, std::string body, ResponseHandler on_response, - ErrorHandler on_error, Deadline deadline) { + ErrorHandler on_error, + std::chrono::steady_clock::time_point deadline) { return impl_->post(url, set_headers, body, on_response, on_error, deadline); } @@ -275,10 +279,11 @@ nlohmann::json Curl::config_json() const { return nlohmann::json::object({{"type", "datadog::tracing::Curl"}}); } -CurlImpl::CurlImpl(const std::shared_ptr &logger, CurlLibrary &curl, - const Curl::ThreadGenerator &make_thread) +CurlImpl::CurlImpl(const std::shared_ptr &logger, const Clock &clock, + CurlLibrary &curl, const Curl::ThreadGenerator &make_thread) : curl_(curl), logger_(logger), + clock_(clock), shutting_down_(false), num_active_handles_(0) { curl_.global_init(CURL_GLOBAL_ALL); @@ -323,11 +328,10 @@ CurlImpl::~CurlImpl() { curl_.global_cleanup(); } -Expected CurlImpl::post(const HTTPClient::URL &url, - HeadersSetter set_headers, std::string body, - ResponseHandler on_response, - ErrorHandler on_error, - HTTPClient::Deadline deadline) try { +Expected CurlImpl::post( + const HTTPClient::URL &url, HeadersSetter set_headers, std::string body, + ResponseHandler on_response, ErrorHandler on_error, + std::chrono::steady_clock::time_point deadline) try { if (multi_handle_ == nullptr) { return Error{Error::CURL_HTTP_CLIENT_NOT_RUNNING, "Unable to send request via libcurl because the HTTP client " @@ -509,10 +513,19 @@ void CurlImpl::run() { auto *request = reinterpret_cast(user_data); const auto timeout = request->deadline - clock_().tick; - if (timeout <= HTTPClient::Deadline::duration::zero()) { + if (timeout <= std::chrono::steady_clock::time_point::duration::zero()) { + std::string message; + message += + "Request deadline exceeded before request was even added to " + "libcurl " + "event loop. Deadline was "; + message += std::to_string( + -std::chrono::duration_cast(timeout) + .count()); + message += " nanoseconds ago."; request->on_error( - Error{Error::CURL_REQUEST_FAILURE, - "Request not processed as the deadline has been reached."}); + Error{Error::CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START, + std::move(message)}); curl_.easy_cleanup(handle); delete request; @@ -520,7 +533,7 @@ void CurlImpl::run() { continue; } - throw_on_error(curl_.easy_setopt_timeout( + log_on_error(curl_.easy_setopt_timeout_ms( handle, std::chrono::duration_cast(timeout) .count())); log_on_error(curl_.multi_add_handle(multi_handle_, handle)); diff --git a/src/datadog/curl.h b/src/datadog/curl.h index 7c9cf6f7..6c1ef377 100644 --- a/src/datadog/curl.h +++ b/src/datadog/curl.h @@ -17,6 +17,7 @@ #include #include +#include "clock.h" #include "http_client.h" #include "json_fwd.hpp" @@ -59,7 +60,7 @@ class CurlLibrary { virtual CURLcode easy_setopt_url(CURL *handle, const char *url); virtual CURLcode easy_setopt_writedata(CURL *handle, void *data); virtual CURLcode easy_setopt_writefunction(CURL *handle, WriteCallback); - virtual CURLcode easy_setopt_timeout(CURL *handle, long timeout_ms); + virtual CURLcode easy_setopt_timeout_ms(CURL *handle, long timeout_ms); virtual const char *easy_strerror(CURLcode error); virtual void global_cleanup(); virtual CURLcode global_init(long flags); @@ -87,16 +88,18 @@ class Curl : public HTTPClient { public: using ThreadGenerator = std::function &&)>; - explicit Curl(const std::shared_ptr &); - Curl(const std::shared_ptr &, CurlLibrary &); - Curl(const std::shared_ptr &, CurlLibrary &, const ThreadGenerator &); + explicit Curl(const std::shared_ptr &, const Clock &); + Curl(const std::shared_ptr &, const Clock &, CurlLibrary &); + Curl(const std::shared_ptr &, const Clock &, CurlLibrary &, + const ThreadGenerator &); ~Curl(); Curl(const Curl &) = delete; Expected post(const URL &url, HeadersSetter set_headers, std::string body, ResponseHandler on_response, - ErrorHandler on_error, Deadline deadline) override; + ErrorHandler on_error, + std::chrono::steady_clock::time_point deadline) override; void drain(std::chrono::steady_clock::time_point deadline) override; diff --git a/src/datadog/datadog_agent.cpp b/src/datadog/datadog_agent.cpp index a34ba585..739c2ea9 100644 --- a/src/datadog/datadog_agent.cpp +++ b/src/datadog/datadog_agent.cpp @@ -134,9 +134,9 @@ std::variant parse_agent_traces_response( DatadogAgent::DatadogAgent( const FinalizedDatadogAgentConfig& config, const std::shared_ptr& tracer_telemetry, - const Clock& clock, const std::shared_ptr& logger) + const std::shared_ptr& logger) : tracer_telemetry_(tracer_telemetry), - clock_(clock), + clock_(config.clock), logger_(logger), traces_endpoint_(traces_endpoint(config.url)), telemetry_endpoint_(telemetry_endpoint(config.url)), diff --git a/src/datadog/datadog_agent.h b/src/datadog/datadog_agent.h index 583f0b6b..30671fab 100644 --- a/src/datadog/datadog_agent.h +++ b/src/datadog/datadog_agent.h @@ -58,7 +58,7 @@ class DatadogAgent : public Collector { public: DatadogAgent(const FinalizedDatadogAgentConfig&, - const std::shared_ptr&, const Clock& clock, + const std::shared_ptr&, const std::shared_ptr&); ~DatadogAgent(); diff --git a/src/datadog/datadog_agent_config.cpp b/src/datadog/datadog_agent_config.cpp index a10b0e56..5ba97ccc 100644 --- a/src/datadog/datadog_agent_config.cpp +++ b/src/datadog/datadog_agent_config.cpp @@ -84,11 +84,14 @@ Expected DatadogAgentConfig::parse(StringView input) { } Expected finalize_config( - const DatadogAgentConfig& config, const std::shared_ptr& logger) { + const DatadogAgentConfig& config, const std::shared_ptr& logger, + const Clock& clock) { FinalizedDatadogAgentConfig result; + result.clock = clock; + if (!config.http_client) { - result.http_client = default_http_client(logger); + result.http_client = default_http_client(logger, clock); // `default_http_client` might return a `Curl` instance depending on how // this library was built. If it returns `nullptr`, then there's no // built-in default, and so the user must provide a value. diff --git a/src/datadog/datadog_agent_config.h b/src/datadog/datadog_agent_config.h index 6e91f209..9eb96f35 100644 --- a/src/datadog/datadog_agent_config.h +++ b/src/datadog/datadog_agent_config.h @@ -16,6 +16,7 @@ #include #include +#include "clock.h" #include "expected.h" #include "http_client.h" #include "string_view.h" @@ -59,7 +60,7 @@ struct DatadogAgentConfig { class FinalizedDatadogAgentConfig { friend Expected finalize_config( - const DatadogAgentConfig& config, const std::shared_ptr& logger); + const DatadogAgentConfig&, const std::shared_ptr&, const Clock&); FinalizedDatadogAgentConfig() = default; @@ -70,10 +71,12 @@ class FinalizedDatadogAgentConfig { std::chrono::steady_clock::duration flush_interval; std::chrono::steady_clock::duration request_timeout; std::chrono::steady_clock::duration shutdown_timeout; + Clock clock; }; Expected finalize_config( - const DatadogAgentConfig& config, const std::shared_ptr& logger); + const DatadogAgentConfig& config, const std::shared_ptr& logger, + const Clock& clock); } // namespace tracing } // namespace datadog diff --git a/src/datadog/default_http_client.h b/src/datadog/default_http_client.h index 83ab410b..cfeda7e5 100644 --- a/src/datadog/default_http_client.h +++ b/src/datadog/default_http_client.h @@ -9,6 +9,8 @@ #include +#include "clock.h" + namespace datadog { namespace tracing { @@ -16,7 +18,7 @@ class HTTPClient; class Logger; std::shared_ptr default_http_client( - const std::shared_ptr& logger); + const std::shared_ptr& logger, const Clock& clock); } // namespace tracing } // namespace datadog diff --git a/src/datadog/default_http_client_curl.cpp b/src/datadog/default_http_client_curl.cpp index 1970430e..375acede 100644 --- a/src/datadog/default_http_client_curl.cpp +++ b/src/datadog/default_http_client_curl.cpp @@ -12,8 +12,8 @@ namespace datadog { namespace tracing { std::shared_ptr default_http_client( - const std::shared_ptr& logger) { - return std::make_shared(logger); + const std::shared_ptr& logger, const Clock& clock) { + return std::make_shared(logger, clock); } } // namespace tracing diff --git a/src/datadog/default_http_client_null.cpp b/src/datadog/default_http_client_null.cpp index c15f4fb6..b5a73ece 100644 --- a/src/datadog/default_http_client_null.cpp +++ b/src/datadog/default_http_client_null.cpp @@ -9,8 +9,8 @@ namespace datadog { namespace tracing { -std::shared_ptr default_http_client( - const std::shared_ptr&) { +std::shared_ptr default_http_client(const std::shared_ptr &, + const Clock &) { return nullptr; } diff --git a/src/datadog/error.h b/src/datadog/error.h index 3bbe78b3..4326f08d 100644 --- a/src/datadog/error.h +++ b/src/datadog/error.h @@ -70,6 +70,7 @@ struct Error { MULTIPLE_PROPAGATION_STYLE_ENVIRONMENT_VARIABLES = 45, DUPLICATE_PROPAGATION_STYLE = 46, ZERO_TRACE_ID = 47, + CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START = 48, }; Code code; diff --git a/src/datadog/http_client.h b/src/datadog/http_client.h index 1eef244b..5cc994f2 100644 --- a/src/datadog/http_client.h +++ b/src/datadog/http_client.h @@ -36,7 +36,6 @@ class HTTPClient { // `ErrorHandler` is for errors encountered by `HTTPClient`, not for // error-indicating HTTP responses. using ErrorHandler = std::function; - using Deadline = std::chrono::steady_clock::time_point; // Send a POST request to the specified `url`. Set request headers by calling // the specified `set_headers` callback. Include the specified `body` at the @@ -44,15 +43,15 @@ class HTTPClient { // a response is delivered (even if that response contains an error HTTP // response status). Invoke the specified `on_error` if an error occurs // outside of HTTP, such as a connection failure. If an error occurs while - // preparing the request, return an `Error`. The behavior is undefined if - // either of `on_response` or `on_error` throws an exception. - virtual Expected post(const URL& url, HeadersSetter set_headers, - std::string body, ResponseHandler on_response, - ErrorHandler on_error, Deadline deadline) = 0; + // preparing the request, return an `Error`. + virtual Expected post( + const URL& url, HeadersSetter set_headers, std::string body, + ResponseHandler on_response, ErrorHandler on_error, + std::chrono::steady_clock::time_point deadline) = 0; // Wait until there are no more outstanding requests, or until the specified // `deadline`. - virtual void drain(Deadline deadline) = 0; + virtual void drain(std::chrono::steady_clock::time_point deadline) = 0; // Return a JSON representation of this object's configuration. The JSON // representation is an object with the following properties: diff --git a/src/datadog/tracer.cpp b/src/datadog/tracer.cpp index 931978c4..65f9d33e 100644 --- a/src/datadog/tracer.cpp +++ b/src/datadog/tracer.cpp @@ -200,19 +200,10 @@ Expected extract_b3( } // namespace Tracer::Tracer(const FinalizedTracerConfig& config) - : Tracer(config, default_id_generator(config.trace_id_128_bit), - default_clock) {} + : Tracer(config, default_id_generator(config.trace_id_128_bit)) {} Tracer::Tracer(const FinalizedTracerConfig& config, const std::shared_ptr& generator) - : Tracer(config, generator, default_clock) {} - -Tracer::Tracer(const FinalizedTracerConfig& config, const Clock& clock) - : Tracer(config, default_id_generator(config.trace_id_128_bit), clock) {} - -Tracer::Tracer(const FinalizedTracerConfig& config, - const std::shared_ptr& generator, - const Clock& clock) : logger_(config.logger), collector_(/* see constructor body */), defaults_(std::make_shared(config.defaults)), @@ -221,10 +212,11 @@ Tracer::Tracer(const FinalizedTracerConfig& config, tracer_telemetry_(std::make_shared( config.report_telemetry, clock, logger_, defaults_, runtime_id_)), trace_sampler_( - std::make_shared(config.trace_sampler, clock)), - span_sampler_(std::make_shared(config.span_sampler, clock)), + std::make_shared(config.trace_sampler, config.clock)), + span_sampler_( + std::make_shared(config.span_sampler, config.clock)), generator_(generator), - clock_(clock), + clock_(config.clock), injection_styles_(config.injection_styles), extraction_styles_(config.extraction_styles), hostname_(config.report_hostname ? get_hostname() : nullopt), @@ -236,7 +228,7 @@ Tracer::Tracer(const FinalizedTracerConfig& config, auto& agent_config = std::get(config.collector); auto agent = std::make_shared(agent_config, tracer_telemetry_, - clock, config.logger); + config.logger); collector_ = agent; if (tracer_telemetry_->enabled()) { agent->send_app_started(); diff --git a/src/datadog/tracer.h b/src/datadog/tracer.h index 55dd6393..27a6264c 100644 --- a/src/datadog/tracer.h +++ b/src/datadog/tracer.h @@ -50,10 +50,6 @@ class Tracer { explicit Tracer(const FinalizedTracerConfig& config); Tracer(const FinalizedTracerConfig& config, const std::shared_ptr& generator); - Tracer(const FinalizedTracerConfig& config, const Clock& clock); - Tracer(const FinalizedTracerConfig& config, - const std::shared_ptr& generator, - const Clock& clock); // Create a new trace and return the root span of the trace. Optionally // specify a `config` indicating the attributes of the root span. diff --git a/src/datadog/tracer_config.cpp b/src/datadog/tracer_config.cpp index 7da564e4..592a9c22 100644 --- a/src/datadog/tracer_config.cpp +++ b/src/datadog/tracer_config.cpp @@ -265,8 +265,14 @@ Expected finalize_propagation_styles(FinalizedTracerConfig &result, } // namespace Expected finalize_config(const TracerConfig &config) { + return finalize_config(config, default_clock); +} + +Expected finalize_config(const TracerConfig &config, + const Clock &clock) { FinalizedTracerConfig result; + result.clock = clock; result.defaults = config.defaults; if (auto service_env = lookup(environment::DD_SERVICE)) { @@ -314,7 +320,7 @@ Expected finalize_config(const TracerConfig &config) { if (!report_traces) { result.collector = std::make_shared(); } else if (!config.collector) { - auto finalized = finalize_config(config.agent, result.logger); + auto finalized = finalize_config(config.agent, result.logger, clock); if (auto *error = finalized.if_error()) { return std::move(*error); } diff --git a/src/datadog/tracer_config.h b/src/datadog/tracer_config.h index 5ef23270..ab851b20 100644 --- a/src/datadog/tracer_config.h +++ b/src/datadog/tracer_config.h @@ -9,6 +9,7 @@ #include #include +#include "clock.h" #include "datadog_agent_config.h" #include "error.h" #include "expected.h" @@ -123,7 +124,7 @@ struct TracerConfig { // `FinalizedTracerConfig` must be obtained by calling `finalize_config`. class FinalizedTracerConfig { friend Expected finalize_config( - const TracerConfig& config); + const TracerConfig& config, const Clock& clock); FinalizedTracerConfig() = default; public: @@ -146,12 +147,18 @@ class FinalizedTracerConfig { bool trace_id_128_bit; bool report_telemetry; Optional runtime_id; + Clock clock; }; // Return a `FinalizedTracerConfig` from the specified `config` and from any // relevant environment variables. If any configuration is invalid, return an // `Error`. +// Optionally specify a `clock` used to calculate span start times, span +// durations, and timeouts. If `clock` is not specified, then `default_clock` +// is used. Expected finalize_config(const TracerConfig& config); +Expected finalize_config(const TracerConfig& config, + const Clock& clock); } // namespace tracing } // namespace datadog diff --git a/test/mocks/http_clients.h b/test/mocks/http_clients.h index da7b6e33..ad59006c 100644 --- a/test/mocks/http_clients.h +++ b/test/mocks/http_clients.h @@ -40,9 +40,10 @@ struct MockHTTPClient : public HTTPClient { ResponseHandler on_response_; ErrorHandler on_error_; - Expected post(const URL& /* url */, HeadersSetter set_headers, - std::string /*body*/, ResponseHandler on_response, - ErrorHandler on_error, Deadline /*deadline*/) override { + Expected post( + const URL&, HeadersSetter set_headers, std::string /*body*/, + ResponseHandler on_response, ErrorHandler on_error, + std::chrono::steady_clock::time_point /*deadline*/) override { std::lock_guard lock{mutex_}; if (!post_error) { on_response_ = on_response; diff --git a/test/test_curl.cpp b/test/test_curl.cpp index fa7acefb..86169bdf 100644 --- a/test/test_curl.cpp +++ b/test/test_curl.cpp @@ -71,7 +71,7 @@ class SingleRequestMockCurlLibrary : public CurlLibrary { return CURLE_OK; } - CURLcode easy_setopt_timeout(CURL *, long) override { + CURLcode easy_setopt_timeout_ms(CURL *, long) override { // TBD? return CURLE_OK; } @@ -141,7 +141,7 @@ class SingleRequestMockCurlLibrary : public CurlLibrary { TEST_CASE("parse response headers and body", "[curl]") { const auto logger = std::make_shared(); SingleRequestMockCurlLibrary library; - const auto client = std::make_shared(logger, library); + const auto client = std::make_shared(logger, default_clock, library); SECTION("in the tracer") { // The tracer doesn't read response headers, at least as of this writing. @@ -213,15 +213,15 @@ TEST_CASE("bad multi-handle means error mode", "[curl]") { const auto logger = std::make_shared(); MockCurlLibrary library; - const auto client = std::make_shared(logger, library); + const auto client = std::make_shared(logger, default_clock, library); REQUIRE(logger->first_error().code == Error::CURL_HTTP_CLIENT_SETUP_FAILED); const auto ignore = [](auto &&...) {}; const HTTPClient::URL url = {"http", "whatever", ""}; - const auto dummy_timeout = + const auto dummy_deadline = std::chrono::steady_clock::now() + std::chrono::seconds(10); const auto result = - client->post(url, ignore, "dummy body", ignore, ignore, dummy_timeout); + client->post(url, ignore, "dummy body", ignore, ignore, dummy_deadline); REQUIRE_FALSE(result); REQUIRE(result.error().code == Error::CURL_HTTP_CLIENT_NOT_RUNNING); } @@ -231,19 +231,19 @@ TEST_CASE("bad std::thread means error mode", "[curl]") { // where calls to `post` always return an error. const auto logger = std::make_shared(); CurlLibrary libcurl; // the default implementation - const auto client = - std::make_shared(logger, libcurl, [](auto &&) -> std::thread { + const auto client = std::make_shared( + logger, default_clock, libcurl, [](auto &&) -> std::thread { throw std::system_error( std::make_error_code(std::errc::resource_unavailable_try_again)); }); REQUIRE(logger->first_error().code == Error::CURL_HTTP_CLIENT_SETUP_FAILED); const auto ignore = [](auto &&...) {}; - const auto dummy_timeout = + const auto dummy_deadline = std::chrono::steady_clock::now() + std::chrono::seconds(10); const HTTPClient::URL url = {"http", "whatever", ""}; const auto result = - client->post(url, ignore, "dummy body", ignore, ignore, dummy_timeout); + client->post(url, ignore, "dummy body", ignore, ignore, dummy_deadline); REQUIRE_FALSE(result); REQUIRE(result.error().code == Error::CURL_HTTP_CLIENT_NOT_RUNNING); } @@ -258,14 +258,14 @@ TEST_CASE("fail to allocate request handle", "[curl]") { const auto logger = std::make_shared(); MockCurlLibrary library; - const auto client = std::make_shared(logger, library); + const auto client = std::make_shared(logger, default_clock, library); const auto ignore = [](auto &&...) {}; const HTTPClient::URL url = {"http", "whatever", ""}; - const auto dummy_timeout = + const auto dummy_deadline = std::chrono::steady_clock::now() + std::chrono::seconds(10); const auto result = - client->post(url, ignore, "dummy body", ignore, ignore, dummy_timeout); + client->post(url, ignore, "dummy body", ignore, ignore, dummy_deadline); REQUIRE_FALSE(result); REQUIRE(result.error().code == Error::CURL_REQUEST_SETUP_FAILED); } @@ -376,7 +376,7 @@ TEST_CASE("setopt failures", "[curl]") { library.fail = which_fails; const auto logger = std::make_shared(); - const auto client = std::make_shared(logger, library); + const auto client = std::make_shared(logger, default_clock, library); const auto ignore = [](auto &&...) {}; HTTPClient::URL url; @@ -389,10 +389,10 @@ TEST_CASE("setopt failures", "[curl]") { url.path = "/trace/thing"; } - const auto dummy_timeout = + const auto dummy_deadline = std::chrono::steady_clock::now() + std::chrono::seconds(10); const auto result = - client->post(url, ignore, "dummy body", ignore, ignore, dummy_timeout); + client->post(url, ignore, "dummy body", ignore, ignore, dummy_deadline); REQUIRE_FALSE(result); REQUIRE(result.error().code == Error::CURL_REQUEST_SETUP_FAILED); } @@ -400,13 +400,13 @@ TEST_CASE("setopt failures", "[curl]") { TEST_CASE("handles are always cleaned up", "[curl]") { const auto logger = std::make_shared(); SingleRequestMockCurlLibrary library; - auto client = std::make_shared(logger, library); + auto client = std::make_shared(logger, default_clock, library); SECTION("when the response is delivered") { Optional post_error; std::exception_ptr exception; const HTTPClient::URL url = {"http", "whatever", ""}; - const auto dummy_timeout = + const auto dummy_deadline = std::chrono::steady_clock::now() + std::chrono::seconds(10); const auto result = client->post( url, [](const auto &) {}, "whatever", @@ -419,7 +419,7 @@ TEST_CASE("handles are always cleaned up", "[curl]") { exception = std::current_exception(); } }, - [&](const Error &error) { post_error = error; }, dummy_timeout); + [&](const Error &error) { post_error = error; }, dummy_deadline); REQUIRE(result); client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1)); @@ -433,12 +433,12 @@ TEST_CASE("handles are always cleaned up", "[curl]") { Optional post_error; const HTTPClient::URL url = {"http", "whatever", ""}; const auto ignore = [](auto &&...) {}; - const auto dummy_timeout = + const auto dummy_deadline = std::chrono::steady_clock::now() + std::chrono::seconds(10); library.message_result_ = CURLE_COULDNT_CONNECT; // any error would do const auto result = client->post( url, ignore, "whatever", ignore, - [&](const Error &error) { post_error = error; }, dummy_timeout); + [&](const Error &error) { post_error = error; }, dummy_deadline); REQUIRE(result); client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1)); @@ -448,11 +448,11 @@ TEST_CASE("handles are always cleaned up", "[curl]") { SECTION("when we shut down while a request is in flight") { const HTTPClient::URL url = {"http", "whatever", ""}; const auto ignore = [](auto &&...) {}; - const auto dummy_timeout = + const auto dummy_deadline = std::chrono::steady_clock::now() + std::chrono::seconds(10); library.delay_message_ = true; const auto result = - client->post(url, ignore, "whatever", ignore, ignore, dummy_timeout); + client->post(url, ignore, "whatever", ignore, ignore, dummy_deadline); REQUIRE(result); // Destroy the `Curl` object. @@ -464,5 +464,24 @@ TEST_CASE("handles are always cleaned up", "[curl]") { REQUIRE(library.created_handles_ == library.destroyed_handles_); } -// TODO: "multi_*" failures -// TODO: "getinfo" failures +TEST_CASE("post() deadline exceeded before request start", "[curl]") { + Curl client{std::make_shared(), default_clock}; + + const auto ignore = [](auto &&...) {}; + const HTTPClient::URL url = {"http", "whatever", ""}; + const std::string body; + const auto deadline = + std::chrono::steady_clock::now() - std::chrono::milliseconds(1); + Optional error_delivered; + + const auto result = client.post( + url, ignore, body, ignore, [&](Error error) { error_delivered = error; }, + deadline); + REQUIRE(result); + + client.drain(std::chrono::steady_clock::now() + std::chrono::seconds(1)); + + REQUIRE(error_delivered); + REQUIRE(error_delivered->code == + Error::CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START); +} diff --git a/test/test_trace_sampler.cpp b/test/test_trace_sampler.cpp index 76fb469a..68a117cf 100644 --- a/test/test_trace_sampler.cpp +++ b/test/test_trace_sampler.cpp @@ -127,13 +127,14 @@ TEST_CASE("trace sampling rate limiter") { config.collector = collector; config.logger = std::make_shared(); - auto finalized = finalize_config(config); - REQUIRE(finalized); - TimePoint current_time = default_clock(); // Modify `current_time` to advance the clock. auto clock = [¤t_time]() { return current_time; }; - Tracer tracer{*finalized, clock}; + + auto finalized = finalize_config(config, clock); + REQUIRE(finalized); + + Tracer tracer{*finalized}; for (std::size_t i = 0; i < test_case.burst_size; ++i) { auto span = tracer.create_span(); diff --git a/test/test_tracer.cpp b/test/test_tracer.cpp index 9947ddbf..3654c85a 100644 --- a/test/test_tracer.cpp +++ b/test/test_tracer.cpp @@ -1034,9 +1034,9 @@ TEST_CASE("128-bit trace IDs") { config.extraction_styles.push_back(PropagationStyle::W3C); config.extraction_styles.push_back(PropagationStyle::DATADOG); config.extraction_styles.push_back(PropagationStyle::B3); - const auto finalized = finalize_config(config); + const auto finalized = finalize_config(config, clock); REQUIRE(finalized); - Tracer tracer{*finalized, clock}; + Tracer tracer{*finalized}; TraceID trace_id; // used below the following SECTIONs SECTION("are generated") { From 900a1e56b96e53f330bb596a74555142ee557605 Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Mon, 13 Nov 2023 11:51:44 +0100 Subject: [PATCH 5/7] self code review --- src/datadog/curl.cpp | 2 +- src/datadog/datadog_agent_config.cpp | 4 ++-- src/datadog/datadog_agent_config.h | 4 ++-- src/datadog/error.h | 2 ++ test/test_curl.cpp | 2 -- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/datadog/curl.cpp b/src/datadog/curl.cpp index fb5e1e0b..a13557f0 100644 --- a/src/datadog/curl.cpp +++ b/src/datadog/curl.cpp @@ -481,7 +481,7 @@ CURLMcode CurlImpl::log_on_error(CURLMcode result) { void CurlImpl::run() { int num_messages_remaining; CURLMsg *message; - const int max_wait_milliseconds = 1000; + const int max_wait_milliseconds = 10000; std::unique_lock lock(mutex_); for (;;) { diff --git a/src/datadog/datadog_agent_config.cpp b/src/datadog/datadog_agent_config.cpp index 5ba97ccc..e29b5abf 100644 --- a/src/datadog/datadog_agent_config.cpp +++ b/src/datadog/datadog_agent_config.cpp @@ -118,7 +118,7 @@ Expected finalize_config( std::chrono::milliseconds(config.flush_interval_milliseconds); if (config.request_timeout_milliseconds <= 0) { - return Error{Error::DATADOG_AGENT_INVALID_FLUSH_INTERVAL, + return Error{Error::DATADOG_AGENT_INVALID_REQUEST_TIMEOUT, "DatadogAgent: Request timeout must be a positive number of " "milliseconds."}; } @@ -126,7 +126,7 @@ Expected finalize_config( std::chrono::milliseconds(config.request_timeout_milliseconds); if (config.shutdown_timeout_milliseconds <= 0) { - return Error{Error::DATADOG_AGENT_INVALID_FLUSH_INTERVAL, + return Error{Error::DATADOG_AGENT_INVALID_SHUTDOWN_TIMEOUT, "DatadogAgent: Shutdown timeout must be a positive number of " "milliseconds."}; } diff --git a/src/datadog/datadog_agent_config.h b/src/datadog/datadog_agent_config.h index 9eb96f35..1916860d 100644 --- a/src/datadog/datadog_agent_config.h +++ b/src/datadog/datadog_agent_config.h @@ -50,9 +50,9 @@ struct DatadogAgentConfig { std::string url = "http://localhost:8126"; // How often, in milliseconds, to send batches of traces to the Datadog Agent. int flush_interval_milliseconds = 2000; - // TBD + // Maximum amount of time an HTTP request is allowed to run. int request_timeout_milliseconds = 2000; - // TBD + // Maximum amount of time the process is allowed to wait before shutting down. int shutdown_timeout_milliseconds = 2000; static Expected parse(StringView); diff --git a/src/datadog/error.h b/src/datadog/error.h index 4326f08d..93e669c3 100644 --- a/src/datadog/error.h +++ b/src/datadog/error.h @@ -71,6 +71,8 @@ struct Error { DUPLICATE_PROPAGATION_STYLE = 46, ZERO_TRACE_ID = 47, CURL_DEADLINE_EXCEEDED_BEFORE_REQUEST_START = 48, + DATADOG_AGENT_INVALID_REQUEST_TIMEOUT = 49, + DATADOG_AGENT_INVALID_SHUTDOWN_TIMEOUT = 50, }; Code code; diff --git a/test/test_curl.cpp b/test/test_curl.cpp index 86169bdf..3feaa206 100644 --- a/test/test_curl.cpp +++ b/test/test_curl.cpp @@ -15,7 +15,6 @@ #include "test.h" using namespace datadog::tracing; -using namespace std::chrono_literals; class SingleRequestMockCurlLibrary : public CurlLibrary { public: @@ -72,7 +71,6 @@ class SingleRequestMockCurlLibrary : public CurlLibrary { } CURLcode easy_setopt_timeout_ms(CURL *, long) override { - // TBD? return CURLE_OK; } From abed062abe193872ce4b21cdde4d35663f2c72a8 Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Mon, 13 Nov 2023 13:09:58 +0100 Subject: [PATCH 6/7] rebase --- src/datadog/datadog_agent.cpp | 24 ++++++++++++++---------- src/datadog/tracer.cpp | 3 ++- test/test_curl.cpp | 4 +--- test/test_span_sampler.cpp | 6 +++--- 4 files changed, 20 insertions(+), 17 deletions(-) diff --git a/src/datadog/datadog_agent.cpp b/src/datadog/datadog_agent.cpp index 739c2ea9..f5250540 100644 --- a/src/datadog/datadog_agent.cpp +++ b/src/datadog/datadog_agent.cpp @@ -216,8 +216,9 @@ nlohmann::json DatadogAgent::config_json() const { {"config", nlohmann::json::object({ {"traces_url", (traces_endpoint_.scheme + "://" + traces_endpoint_.authority + traces_endpoint_.path)}, {"telemetry_url", (telemetry_endpoint_.scheme + "://" + telemetry_endpoint_.authority + telemetry_endpoint_.path)}, + {"flush_interval_milliseconds", std::chrono::duration_cast(flush_interval_).count() }, {"request_timeout_milliseconds", std::chrono::duration_cast(request_timeout_).count() }, - {"shutdown_timeout_milliseconds", std::chrono::duration_cast(request_timeout_).count() }, + {"shutdown_timeout_milliseconds", std::chrono::duration_cast(shutdown_timeout_).count() }, {"http_client", http_client_->config_json()}, {"event_scheduler", event_scheduler_->config_json()}, })}, @@ -335,9 +336,10 @@ void DatadogAgent::flush() { void DatadogAgent::send_app_started() { auto payload = tracer_telemetry_->app_started(); - auto post_result = http_client_->post( - telemetry_endpoint_, telemetry_set_request_headers_, std::move(payload), - telemetry_on_response_, telemetry_on_error_, clock_().tick + request_timeout_); + auto post_result = + http_client_->post(telemetry_endpoint_, telemetry_set_request_headers_, + std::move(payload), telemetry_on_response_, + telemetry_on_error_, clock_().tick + request_timeout_); if (auto* error = post_result.if_error()) { logger_->log_error(error->with_prefix( "Unexpected error submitting telemetry app-started event: ")); @@ -346,9 +348,10 @@ void DatadogAgent::send_app_started() { void DatadogAgent::send_heartbeat_and_telemetry() { auto payload = tracer_telemetry_->heartbeat_and_telemetry(); - auto post_result = http_client_->post( - telemetry_endpoint_, telemetry_set_request_headers_, std::move(payload), - telemetry_on_response_, telemetry_on_error_, clock_().tick + request_timeout_); + auto post_result = + http_client_->post(telemetry_endpoint_, telemetry_set_request_headers_, + std::move(payload), telemetry_on_response_, + telemetry_on_error_, clock_().tick + request_timeout_); if (auto* error = post_result.if_error()) { logger_->log_error(error->with_prefix( "Unexpected error submitting telemetry app-heartbeat event: ")); @@ -357,9 +360,10 @@ void DatadogAgent::send_heartbeat_and_telemetry() { void DatadogAgent::send_app_closing() { auto payload = tracer_telemetry_->app_closing(); - auto post_result = http_client_->post( - telemetry_endpoint_, telemetry_set_request_headers_, std::move(payload), - telemetry_on_response_, telemetry_on_error_, clock_().tick + request_timeout_); + auto post_result = + http_client_->post(telemetry_endpoint_, telemetry_set_request_headers_, + std::move(payload), telemetry_on_response_, + telemetry_on_error_, clock_().tick + request_timeout_); if (auto* error = post_result.if_error()) { logger_->log_error(error->with_prefix( "Unexpected error submitting telemetry app-closing event: ")); diff --git a/src/datadog/tracer.cpp b/src/datadog/tracer.cpp index 65f9d33e..795cf6f4 100644 --- a/src/datadog/tracer.cpp +++ b/src/datadog/tracer.cpp @@ -210,7 +210,8 @@ Tracer::Tracer(const FinalizedTracerConfig& config, runtime_id_(config.runtime_id ? *config.runtime_id : RuntimeID::generate()), tracer_telemetry_(std::make_shared( - config.report_telemetry, clock, logger_, defaults_, runtime_id_)), + config.report_telemetry, config.clock, logger_, defaults_, + runtime_id_)), trace_sampler_( std::make_shared(config.trace_sampler, config.clock)), span_sampler_( diff --git a/test/test_curl.cpp b/test/test_curl.cpp index 3feaa206..7b4198bf 100644 --- a/test/test_curl.cpp +++ b/test/test_curl.cpp @@ -70,9 +70,7 @@ class SingleRequestMockCurlLibrary : public CurlLibrary { return CURLE_OK; } - CURLcode easy_setopt_timeout_ms(CURL *, long) override { - return CURLE_OK; - } + CURLcode easy_setopt_timeout_ms(CURL *, long) override { return CURLE_OK; } CURLMcode multi_add_handle(CURLM *, CURL *easy_handle) override { added_handle_ = easy_handle; diff --git a/test/test_span_sampler.cpp b/test/test_span_sampler.cpp index a99a029b..3944309b 100644 --- a/test/test_span_sampler.cpp +++ b/test/test_span_sampler.cpp @@ -305,10 +305,10 @@ TEST_CASE("span rule limiter") { rule.max_per_second = test_case.max_per_second; config.span_sampler.rules.push_back(rule); - auto finalized = finalize_config(config); - REQUIRE(finalized); auto clock = [frozen_time = default_clock()]() { return frozen_time; }; - Tracer tracer{*finalized, clock}; + auto finalized = finalize_config(config, clock); + REQUIRE(finalized); + Tracer tracer{*finalized}; for (std::size_t i = 0; i < test_case.num_spans; ++i) { auto span = tracer.create_span(); From 5fad53eb700f7b7f14a613a75c7a0264b4f23eab Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Mon, 13 Nov 2023 13:41:04 +0100 Subject: [PATCH 7/7] use default clock instead of steady_clock --- src/datadog/http_client.h | 3 ++- test/test_curl.cpp | 56 +++++++++++++++++++-------------------- 2 files changed, 30 insertions(+), 29 deletions(-) diff --git a/src/datadog/http_client.h b/src/datadog/http_client.h index 5cc994f2..2931dde3 100644 --- a/src/datadog/http_client.h +++ b/src/datadog/http_client.h @@ -43,7 +43,8 @@ class HTTPClient { // a response is delivered (even if that response contains an error HTTP // response status). Invoke the specified `on_error` if an error occurs // outside of HTTP, such as a connection failure. If an error occurs while - // preparing the request, return an `Error`. + // preparing the request, return an `Error`. The behavior is undefined if + // either of `on_response` or `on_error` throws an exception. virtual Expected post( const URL& url, HeadersSetter set_headers, std::string body, ResponseHandler on_response, ErrorHandler on_error, diff --git a/test/test_curl.cpp b/test/test_curl.cpp index 7b4198bf..1c9873e4 100644 --- a/test/test_curl.cpp +++ b/test/test_curl.cpp @@ -11,6 +11,7 @@ #include #include +#include "datadog/clock.h" #include "mocks/loggers.h" #include "test.h" @@ -135,9 +136,10 @@ class SingleRequestMockCurlLibrary : public CurlLibrary { }; TEST_CASE("parse response headers and body", "[curl]") { + const auto clock = default_clock; const auto logger = std::make_shared(); SingleRequestMockCurlLibrary library; - const auto client = std::make_shared(logger, default_clock, library); + const auto client = std::make_shared(logger, clock, library); SECTION("in the tracer") { // The tracer doesn't read response headers, at least as of this writing. @@ -189,10 +191,10 @@ TEST_CASE("parse response headers and body", "[curl]") { } }, [&](const Error &error) { post_error = error; }, - std::chrono::steady_clock::now() + std::chrono::seconds(10)); + clock().tick + std::chrono::seconds(10)); REQUIRE(result); - client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1)); + client->drain(clock().tick + std::chrono::seconds(1)); if (exception) { std::rethrow_exception(exception); } @@ -207,15 +209,15 @@ TEST_CASE("bad multi-handle means error mode", "[curl]") { CURLM *multi_init() override { return nullptr; } }; + const auto clock = default_clock; const auto logger = std::make_shared(); MockCurlLibrary library; - const auto client = std::make_shared(logger, default_clock, library); + const auto client = std::make_shared(logger, clock, library); REQUIRE(logger->first_error().code == Error::CURL_HTTP_CLIENT_SETUP_FAILED); const auto ignore = [](auto &&...) {}; const HTTPClient::URL url = {"http", "whatever", ""}; - const auto dummy_deadline = - std::chrono::steady_clock::now() + std::chrono::seconds(10); + const auto dummy_deadline = clock().tick + std::chrono::seconds(10); const auto result = client->post(url, ignore, "dummy body", ignore, ignore, dummy_deadline); REQUIRE_FALSE(result); @@ -225,18 +227,18 @@ TEST_CASE("bad multi-handle means error mode", "[curl]") { TEST_CASE("bad std::thread means error mode", "[curl]") { // If `Curl` is unable to start its event loop thread, then it enters a mode // where calls to `post` always return an error. + const auto clock = default_clock; const auto logger = std::make_shared(); CurlLibrary libcurl; // the default implementation const auto client = std::make_shared( - logger, default_clock, libcurl, [](auto &&) -> std::thread { + logger, clock, libcurl, [](auto &&) -> std::thread { throw std::system_error( std::make_error_code(std::errc::resource_unavailable_try_again)); }); REQUIRE(logger->first_error().code == Error::CURL_HTTP_CLIENT_SETUP_FAILED); const auto ignore = [](auto &&...) {}; - const auto dummy_deadline = - std::chrono::steady_clock::now() + std::chrono::seconds(10); + const auto dummy_deadline = clock().tick + std::chrono::seconds(10); const HTTPClient::URL url = {"http", "whatever", ""}; const auto result = client->post(url, ignore, "dummy body", ignore, ignore, dummy_deadline); @@ -252,14 +254,14 @@ TEST_CASE("fail to allocate request handle", "[curl]") { CURL *easy_init() override { return nullptr; } }; + const auto clock = default_clock; const auto logger = std::make_shared(); MockCurlLibrary library; - const auto client = std::make_shared(logger, default_clock, library); + const auto client = std::make_shared(logger, clock, library); const auto ignore = [](auto &&...) {}; const HTTPClient::URL url = {"http", "whatever", ""}; - const auto dummy_deadline = - std::chrono::steady_clock::now() + std::chrono::seconds(10); + const auto dummy_deadline = clock().tick + std::chrono::seconds(10); const auto result = client->post(url, ignore, "dummy body", ignore, ignore, dummy_deadline); REQUIRE_FALSE(result); @@ -371,8 +373,9 @@ TEST_CASE("setopt failures", "[curl]") { MockCurlLibrary library; library.fail = which_fails; + const auto clock = default_clock; const auto logger = std::make_shared(); - const auto client = std::make_shared(logger, default_clock, library); + const auto client = std::make_shared(logger, clock, library); const auto ignore = [](auto &&...) {}; HTTPClient::URL url; @@ -385,8 +388,7 @@ TEST_CASE("setopt failures", "[curl]") { url.path = "/trace/thing"; } - const auto dummy_deadline = - std::chrono::steady_clock::now() + std::chrono::seconds(10); + const auto dummy_deadline = clock().tick + std::chrono::seconds(10); const auto result = client->post(url, ignore, "dummy body", ignore, ignore, dummy_deadline); REQUIRE_FALSE(result); @@ -394,16 +396,16 @@ TEST_CASE("setopt failures", "[curl]") { } TEST_CASE("handles are always cleaned up", "[curl]") { + const auto clock = default_clock; const auto logger = std::make_shared(); SingleRequestMockCurlLibrary library; - auto client = std::make_shared(logger, default_clock, library); + auto client = std::make_shared(logger, clock, library); SECTION("when the response is delivered") { Optional post_error; std::exception_ptr exception; const HTTPClient::URL url = {"http", "whatever", ""}; - const auto dummy_deadline = - std::chrono::steady_clock::now() + std::chrono::seconds(10); + const auto dummy_deadline = clock().tick + std::chrono::seconds(10); const auto result = client->post( url, [](const auto &) {}, "whatever", [&](int status, const DictReader & /*headers*/, std::string body) { @@ -418,7 +420,7 @@ TEST_CASE("handles are always cleaned up", "[curl]") { [&](const Error &error) { post_error = error; }, dummy_deadline); REQUIRE(result); - client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1)); + client->drain(clock().tick + std::chrono::seconds(1)); if (exception) { std::rethrow_exception(exception); } @@ -429,23 +431,21 @@ TEST_CASE("handles are always cleaned up", "[curl]") { Optional post_error; const HTTPClient::URL url = {"http", "whatever", ""}; const auto ignore = [](auto &&...) {}; - const auto dummy_deadline = - std::chrono::steady_clock::now() + std::chrono::seconds(10); + const auto dummy_deadline = clock().tick + std::chrono::seconds(10); library.message_result_ = CURLE_COULDNT_CONNECT; // any error would do const auto result = client->post( url, ignore, "whatever", ignore, [&](const Error &error) { post_error = error; }, dummy_deadline); REQUIRE(result); - client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1)); + client->drain(clock().tick + std::chrono::seconds(1)); REQUIRE(post_error); } SECTION("when we shut down while a request is in flight") { const HTTPClient::URL url = {"http", "whatever", ""}; const auto ignore = [](auto &&...) {}; - const auto dummy_deadline = - std::chrono::steady_clock::now() + std::chrono::seconds(10); + const auto dummy_deadline = clock().tick + std::chrono::seconds(10); library.delay_message_ = true; const auto result = client->post(url, ignore, "whatever", ignore, ignore, dummy_deadline); @@ -461,13 +461,13 @@ TEST_CASE("handles are always cleaned up", "[curl]") { } TEST_CASE("post() deadline exceeded before request start", "[curl]") { - Curl client{std::make_shared(), default_clock}; + const auto clock = default_clock; + Curl client{std::make_shared(), clock}; const auto ignore = [](auto &&...) {}; const HTTPClient::URL url = {"http", "whatever", ""}; const std::string body; - const auto deadline = - std::chrono::steady_clock::now() - std::chrono::milliseconds(1); + const auto deadline = clock().tick - std::chrono::milliseconds(1); Optional error_delivered; const auto result = client.post( @@ -475,7 +475,7 @@ TEST_CASE("post() deadline exceeded before request start", "[curl]") { deadline); REQUIRE(result); - client.drain(std::chrono::steady_clock::now() + std::chrono::seconds(1)); + client.drain(clock().tick + std::chrono::seconds(1)); REQUIRE(error_delivered); REQUIRE(error_delivered->code ==