From 5b657899210c3ff1f32eec94e8d1ea428a2831bc Mon Sep 17 00:00:00 2001 From: Caleb Gilmour Date: Wed, 8 Nov 2023 19:31:16 +0000 Subject: [PATCH 1/4] build(deps): bump dd-trace-cpp to v0.1.12 This also required minor modifications to the datadog tracer implementation and tests. Signed-off-by: David Goffredo --- bazel/repository_locations.bzl | 4 +- .../tracers/datadog/agent_http_client.cc | 22 ++-- .../tracers/datadog/agent_http_client.h | 9 +- source/extensions/tracers/datadog/config.cc | 7 +- source/extensions/tracers/datadog/tracer.cc | 14 ++- source/extensions/tracers/datadog/tracer.h | 3 +- .../tracers/datadog/agent_http_client_test.cc | 50 +++++--- .../extensions/tracers/datadog/config_test.cc | 107 ++++++++++++++++-- test/extensions/tracers/datadog/span_test.cc | 7 +- .../extensions/tracers/datadog/tracer_test.cc | 24 +++- 10 files changed, 184 insertions(+), 63 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 7c4d5ca49dfb1..2b884321f0c17 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -583,8 +583,8 @@ REPOSITORY_LOCATIONS_SPEC = dict( project_name = "Datadog C++ Tracing Library", project_desc = "Datadog distributed tracing for C++", project_url = "https://github.com/DataDog/dd-trace-cpp", - version = "0.1.8", - sha256 = "77162c26ba976b8b18e6daf50beaec39389286840733b08e1627d4e5572d4b51", + version = "0.1.12", + sha256 = "9609a9192fecf730473743662e9f5ed57b4c348c12a0e16dd11ed2e592462ebe", strip_prefix = "dd-trace-cpp-{version}", urls = ["https://github.com/DataDog/dd-trace-cpp/archive/v{version}.tar.gz"], use_category = ["observability_ext"], diff --git a/source/extensions/tracers/datadog/agent_http_client.cc b/source/extensions/tracers/datadog/agent_http_client.cc index 37a9fad328cac..d68995fb25382 100644 --- a/source/extensions/tracers/datadog/agent_http_client.cc +++ b/source/extensions/tracers/datadog/agent_http_client.cc @@ -24,9 +24,9 @@ namespace Datadog { AgentHTTPClient::AgentHTTPClient(Upstream::ClusterManager& cluster_manager, const std::string& cluster, const std::string& reference_host, - TracerStats& stats) + TracerStats& stats, TimeSource& time_source) : collector_cluster_(cluster_manager, cluster), cluster_(cluster), - reference_host_(reference_host), stats_(stats) {} + reference_host_(reference_host), stats_(stats), time_source_(time_source) {} AgentHTTPClient::~AgentHTTPClient() { for (const auto& [request_ptr, _] : handlers_) { @@ -38,12 +38,10 @@ AgentHTTPClient::~AgentHTTPClient() { // datadog::tracing::HTTPClient -datadog::tracing::Expected AgentHTTPClient::post(const URL& url, HeadersSetter set_headers, - std::string body, - ResponseHandler on_response, - ErrorHandler on_error) { - ENVOY_LOG(debug, "flushing traces"); - +datadog::tracing::Expected +AgentHTTPClient::post(const URL& url, HeadersSetter set_headers, std::string body, + ResponseHandler on_response, ErrorHandler on_error, + std::chrono::steady_clock::time_point deadline) { auto message = std::make_unique(); Http::RequestHeaderMap& headers = message->headers(); headers.setReferenceMethod(Http::Headers::get().MethodValues.Post); @@ -54,7 +52,10 @@ datadog::tracing::Expected AgentHTTPClient::post(const URL& url, HeadersSe set_headers(writer); message->body().add(body); - ENVOY_LOG(debug, "submitting trace(s) to {} with payload size {}", url.path, body.size()); + auto timeout = std::chrono::duration_cast( + deadline - time_source_.monotonicTime()); + ENVOY_LOG(debug, "submitting data to {} with payload size {} and {} ms timeout", url.path, + body.size(), timeout.count()); if (!collector_cluster_.threadLocalCluster().has_value()) { ENVOY_LOG(debug, "collector cluster '{}' does not exist", cluster_); @@ -64,8 +65,7 @@ datadog::tracing::Expected AgentHTTPClient::post(const URL& url, HeadersSe Http::AsyncClient::Request* request = collector_cluster_.threadLocalCluster()->get().httpAsyncClient().send( - std::move(message), *this, - Http::AsyncClient::RequestOptions().setTimeout(std::chrono::milliseconds(1000))); + std::move(message), *this, Http::AsyncClient::RequestOptions().setTimeout(timeout)); if (!request) { stats_.reports_failed_.inc(); return datadog::tracing::Error{datadog::tracing::Error::ENVOY_HTTP_CLIENT_FAILURE, diff --git a/source/extensions/tracers/datadog/agent_http_client.h b/source/extensions/tracers/datadog/agent_http_client.h index 900a101abb528..af39bb2daa5fd 100644 --- a/source/extensions/tracers/datadog/agent_http_client.h +++ b/source/extensions/tracers/datadog/agent_http_client.h @@ -45,9 +45,10 @@ class AgentHTTPClient : public datadog::tracing::HTTPClient, * @param reference_host the value to use for the "Host" HTTP request header * @param stats a collection of counters used to keep track of events, such as * when a request fails + * @param time_source supplies the time source */ AgentHTTPClient(Upstream::ClusterManager& cluster_manager, const std::string& cluster, - const std::string& reference_host, TracerStats& stats); + const std::string& reference_host, TracerStats& stats, TimeSource& time_source); ~AgentHTTPClient() override; // datadog::tracing::HTTPClient @@ -67,12 +68,13 @@ class AgentHTTPClient : public datadog::tracing::HTTPClient, * @param on_response callback to invoke when a complete response is received * @param on_error callback to invoke when an error occurs before a complete * response is received. + * @param deadline a timepoint that a request must be completed by. * @return \c datadog::tracing::Error if an error occurs, or * \c datadog::tracing::nullopt otherwise. */ datadog::tracing::Expected post(const URL& url, HeadersSetter set_headers, std::string body, - ResponseHandler on_response, - ErrorHandler on_error) override; + ResponseHandler on_response, ErrorHandler on_error, + std::chrono::steady_clock::time_point deadline) override; /** * \c drain has no effect. It's a part of the \c datadog::tracing::HTTPClient @@ -98,6 +100,7 @@ class AgentHTTPClient : public datadog::tracing::HTTPClient, const std::string cluster_; const std::string reference_host_; TracerStats& stats_; + TimeSource& time_source_; }; } // namespace Datadog diff --git a/source/extensions/tracers/datadog/config.cc b/source/extensions/tracers/datadog/config.cc index 56d49e7e3094d..4bd64259df839 100644 --- a/source/extensions/tracers/datadog/config.cc +++ b/source/extensions/tracers/datadog/config.cc @@ -1,5 +1,6 @@ #include "source/extensions/tracers/datadog/config.h" +#include #include #include @@ -23,6 +24,7 @@ DatadogTracerFactory::makeConfig(const envoy::config::trace::v3::DatadogConfig& datadog::tracing::TracerConfig config; config.defaults.version = "envoy " + Envoy::VersionInfo::version(); config.defaults.name = "envoy.proxy"; + config.runtime_id = datadog::tracing::RuntimeID::generate(); if (proto_config.service_name().empty()) { config.defaults.service = "envoy"; } else { @@ -43,10 +45,11 @@ std::string DatadogTracerFactory::makeCollectorReferenceHost( Tracing::DriverSharedPtr DatadogTracerFactory::createTracerDriverTyped( const envoy::config::trace::v3::DatadogConfig& proto_config, Server::Configuration::TracerFactoryContext& context) { + auto& factory_context = context.serverFactoryContext(); return std::make_shared( proto_config.collector_cluster(), makeCollectorReferenceHost(proto_config), - makeConfig(proto_config), context.serverFactoryContext().clusterManager(), - context.serverFactoryContext().scope(), context.serverFactoryContext().threadLocal()); + makeConfig(proto_config), factory_context.clusterManager(), factory_context.scope(), + factory_context.threadLocal(), factory_context.timeSource()); } /** diff --git a/source/extensions/tracers/datadog/tracer.cc b/source/extensions/tracers/datadog/tracer.cc index f22b097569587..4fee08432e0cd 100644 --- a/source/extensions/tracers/datadog/tracer.cc +++ b/source/extensions/tracers/datadog/tracer.cc @@ -31,11 +31,12 @@ namespace { std::shared_ptr makeThreadLocalTracer( datadog::tracing::TracerConfig config, Upstream::ClusterManager& cluster_manager, const std::string& collector_cluster, const std::string& collector_reference_host, - TracerStats& tracer_stats, Event::Dispatcher& dispatcher, spdlog::logger& logger) { + TracerStats& tracer_stats, Event::Dispatcher& dispatcher, spdlog::logger& logger, + TimeSource& time_source) { config.logger = std::make_shared(logger); config.agent.event_scheduler = std::make_shared(dispatcher); config.agent.http_client = std::make_shared( - cluster_manager, collector_cluster, collector_reference_host, tracer_stats); + cluster_manager, collector_cluster, collector_reference_host, tracer_stats, time_source); datadog::tracing::Expected maybe_config = datadog::tracing::finalize_config(config); @@ -57,7 +58,7 @@ Tracer::ThreadLocalTracer::ThreadLocalTracer(const datadog::tracing::FinalizedTr Tracer::Tracer(const std::string& collector_cluster, const std::string& collector_reference_host, const datadog::tracing::TracerConfig& config, Upstream::ClusterManager& cluster_manager, Stats::Scope& scope, - ThreadLocal::SlotAllocator& thread_local_slot_allocator) + ThreadLocal::SlotAllocator& thread_local_slot_allocator, TimeSource& time_source) : tracer_stats_(makeTracerStats(scope)), thread_local_slot_( ThreadLocal::TypedSlot::makeUnique(thread_local_slot_allocator)) { @@ -66,10 +67,11 @@ Tracer::Tracer(const std::string& collector_cluster, const std::string& collecto allow_added_via_api); thread_local_slot_->set([&logger = ENVOY_LOGGER(), collector_cluster, collector_reference_host, - config, &tracer_stats = tracer_stats_, - &cluster_manager](Event::Dispatcher& dispatcher) { + config, &tracer_stats = tracer_stats_, &cluster_manager, + &time_source](Event::Dispatcher& dispatcher) { return makeThreadLocalTracer(config, cluster_manager, collector_cluster, - collector_reference_host, tracer_stats, dispatcher, logger); + collector_reference_host, tracer_stats, dispatcher, logger, + time_source); }); } diff --git a/source/extensions/tracers/datadog/tracer.h b/source/extensions/tracers/datadog/tracer.h index f04f7253b51e5..c79c052c08bc6 100644 --- a/source/extensions/tracers/datadog/tracer.h +++ b/source/extensions/tracers/datadog/tracer.h @@ -52,11 +52,12 @@ class Tracer : public Tracing::Driver, private Logger::Loggable on_response_; testing::MockFunction on_error_; + Event::SimulatedTimeSystem time_; }; TEST_F(DatadogAgentHttpClientTest, PathFromURL) { @@ -86,7 +87,8 @@ TEST_F(DatadogAgentHttpClientTest, PathFromURL) { EXPECT_CALL(request_, cancel()); const auto ignore = [](auto&&...) {}; - datadog::tracing::Expected result = client_.post(url_, ignore, "", ignore, ignore); + datadog::tracing::Expected result = client_.post( + url_, ignore, "", ignore, ignore, time_.monotonicTime() + std::chrono::seconds(1)); EXPECT_TRUE(result) << result.error(); EXPECT_EQ(0, stats_.reports_skipped_no_cluster_.value()); EXPECT_EQ(0, stats_.reports_failed_.value()); @@ -98,10 +100,11 @@ TEST_F(DatadogAgentHttpClientTest, MissingThreadLocalCluster) { // the "reports skipped no cluster" counter. NiceMock cluster_manager; - AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats_); + AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats_, time_); const auto ignore = [](auto&&...) {}; - datadog::tracing::Expected result = client.post(url_, ignore, "", ignore, ignore); + datadog::tracing::Expected result = client.post( + url_, ignore, "", ignore, ignore, time_.monotonicTime() + std::chrono::seconds(1)); EXPECT_TRUE(result) << result.error(); EXPECT_EQ(1, stats_.reports_skipped_no_cluster_.value()); EXPECT_EQ(0, stats_.reports_failed_.value()); @@ -143,7 +146,8 @@ TEST_F(DatadogAgentHttpClientTest, RequestHeaders) { EXPECT_CALL(request_, cancel()); const auto ignore = [](auto&&...) {}; - datadog::tracing::Expected result = client_.post(url_, set_headers, "", ignore, ignore); + datadog::tracing::Expected result = client_.post( + url_, set_headers, "", ignore, ignore, time_.monotonicTime() + std::chrono::seconds(1)); EXPECT_TRUE(result) << result.error(); EXPECT_EQ(0, stats_.reports_skipped_no_cluster_.value()); EXPECT_EQ(0, stats_.reports_failed_.value()); @@ -182,7 +186,8 @@ TEST_F(DatadogAgentHttpClientTest, RequestBody) { EXPECT_CALL(request_, cancel()); const auto ignore = [](auto&&...) {}; - datadog::tracing::Expected result = client_.post(url_, ignore, body, ignore, ignore); + datadog::tracing::Expected result = client_.post( + url_, ignore, body, ignore, ignore, time_.monotonicTime() + std::chrono::seconds(1)); EXPECT_TRUE(result) << result.error(); EXPECT_EQ(0, stats_.reports_skipped_no_cluster_.value()); EXPECT_EQ(0, stats_.reports_failed_.value()); @@ -213,7 +218,8 @@ TEST_F(DatadogAgentHttpClientTest, OnResponse200) { const auto ignore = [](auto&&...) {}; datadog::tracing::Expected result = - client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction()); + client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction(), + time_.monotonicTime() + std::chrono::seconds(1)); EXPECT_TRUE(result) << result.error(); Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( @@ -252,7 +258,8 @@ TEST_F(DatadogAgentHttpClientTest, OnResponseNot200) { const auto ignore = [](auto&&...) {}; datadog::tracing::Expected result = - client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction()); + client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction(), + time_.monotonicTime() + std::chrono::seconds(1)); EXPECT_TRUE(result) << result.error(); // The "404" below is what causes `stats.reports_failed_` to be incremented @@ -293,7 +300,8 @@ TEST_F(DatadogAgentHttpClientTest, OnResponseBogusRequest) { const auto ignore = [](auto&&...) {}; datadog::tracing::Expected result = - client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction()); + client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction(), + time_.monotonicTime() + std::chrono::seconds(1)); EXPECT_TRUE(result) << result.error(); Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( @@ -333,7 +341,8 @@ TEST_F(DatadogAgentHttpClientTest, OnErrorStreamReset) { const auto ignore = [](auto&&...) {}; datadog::tracing::Expected result = - client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction()); + client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction(), + time_.monotonicTime() + std::chrono::seconds(1)); EXPECT_TRUE(result) << result.error(); Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( @@ -369,7 +378,8 @@ TEST_F(DatadogAgentHttpClientTest, OnErrorOther) { const auto ignore = [](auto&&...) {}; datadog::tracing::Expected result = - client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction()); + client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction(), + time_.monotonicTime() + std::chrono::seconds(1)); EXPECT_TRUE(result) << result.error(); Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( @@ -402,7 +412,8 @@ TEST_F(DatadogAgentHttpClientTest, OnErrorBogusRequest) { const auto ignore = [](auto&&...) {}; datadog::tracing::Expected result = - client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction()); + client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction(), + time_.monotonicTime() + std::chrono::seconds(1)); EXPECT_TRUE(result) << result.error(); Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( @@ -445,7 +456,8 @@ TEST_F(DatadogAgentHttpClientTest, SendFailReturnsError) { const auto ignore = [](auto&&...) {}; datadog::tracing::Expected result = - client_.post(url_, ignore, "", on_response_.AsStdFunction(), on_error_.AsStdFunction()); + client_.post(url_, ignore, "", on_response_.AsStdFunction(), on_error_.AsStdFunction(), + time_.monotonicTime() + std::chrono::seconds(1)); ASSERT_FALSE(result); EXPECT_EQ(datadog::tracing::Error::ENVOY_HTTP_CLIENT_FAILURE, result.error().code); EXPECT_EQ(1, stats_.reports_failed_.value()); @@ -491,7 +503,8 @@ TEST_F(DatadogAgentHttpClientTest, SkipReportIfCollectorClusterHasBeenRemoved) { // Attempt to send a request. const auto ignore = [](auto&&...) {}; - datadog::tracing::Expected result = client_.post(url_, ignore, "", ignore, ignore); + datadog::tracing::Expected result = client_.post( + url_, ignore, "", ignore, ignore, time_.monotonicTime() + std::chrono::seconds(1)); EXPECT_TRUE(result); // Verify observability. @@ -515,7 +528,8 @@ TEST_F(DatadogAgentHttpClientTest, SkipReportIfCollectorClusterHasBeenRemoved) { // Attempt to send a request. const auto ignore = [](auto&&...) {}; - datadog::tracing::Expected result = client_.post(url_, ignore, "", ignore, ignore); + datadog::tracing::Expected result = client_.post( + url_, ignore, "", ignore, ignore, time_.monotonicTime() + std::chrono::seconds(1)); EXPECT_TRUE(result); // Verify observability. @@ -543,7 +557,8 @@ TEST_F(DatadogAgentHttpClientTest, SkipReportIfCollectorClusterHasBeenRemoved) { // Attempt to send a request. const auto ignore = [](auto&&...) {}; - datadog::tracing::Expected result = client_.post(url_, ignore, "", ignore, ignore); + datadog::tracing::Expected result = client_.post( + url_, ignore, "", ignore, ignore, time_.monotonicTime() + std::chrono::seconds(1)); EXPECT_TRUE(result); // Complete in-flight request. @@ -570,7 +585,8 @@ TEST_F(DatadogAgentHttpClientTest, SkipReportIfCollectorClusterHasBeenRemoved) { // Attempt to send a request. const auto ignore = [](auto&&...) {}; - datadog::tracing::Expected result = client_.post(url_, ignore, "", ignore, ignore); + datadog::tracing::Expected result = client_.post( + url_, ignore, "", ignore, ignore, time_.monotonicTime() + std::chrono::seconds(1)); EXPECT_TRUE(result); // Complete in-flight request. diff --git a/test/extensions/tracers/datadog/config_test.cc b/test/extensions/tracers/datadog/config_test.cc index acb8de730e19a..59e1e17ecea86 100644 --- a/test/extensions/tracers/datadog/config_test.cc +++ b/test/extensions/tracers/datadog/config_test.cc @@ -68,7 +68,7 @@ class DatadogConfigTest : public testing::Test { tracer_ = std::make_unique( datadog_config.collector_cluster(), DatadogTracerFactory::makeCollectorReferenceHost(datadog_config), - DatadogTracerFactory::makeConfig(datadog_config), cm_, *stats_.rootScope(), tls_); + DatadogTracerFactory::makeConfig(datadog_config), cm_, *stats_.rootScope(), tls_, time_); } void setupValidDriver() { @@ -91,6 +91,7 @@ class DatadogConfigTest : public testing::Test { NiceMock config_; std::unique_ptr tracer_; + Event::SimulatedTimeSystem time_; }; TEST_F(DatadogConfigTest, ConfigureTracer) { @@ -111,9 +112,35 @@ TEST_F(DatadogConfigTest, ConfigureTracer) { { auto datadog_config = makeConfig("collector_cluster: fake_cluster"); - cm_.initializeClusters({"fake_cluster"}, {}); + + EXPECT_CALL(tls_.dispatcher_, createTimer_(testing::_)); + Http::MockAsyncClientRequest request(&cm_.thread_local_cluster_.async_client_); + Http::AsyncClient::Callbacks* callbacks; + EXPECT_CALL(cm_.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce( + Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks = &callbacks_arg; + return &request; + })); + setup(datadog_config, true); + + Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( + Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "202"}}})); + msg->body().add("{}"); + callbacks->onSuccess(request, std::move(msg)); + + EXPECT_CALL(cm_.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce( + Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks = &callbacks_arg; + return &request; + })); + EXPECT_CALL(request, cancel()); + tracer_.reset(); } } @@ -144,7 +171,33 @@ TEST_F(DatadogConfigTest, AllowCollectorClusterToBeAddedViaApi) { auto datadog_config = makeConfig("collector_cluster: fake_cluster"); + EXPECT_CALL(tls_.dispatcher_, createTimer_(testing::_)); + Http::MockAsyncClientRequest request(&cm_.thread_local_cluster_.async_client_); + Http::AsyncClient::Callbacks* callbacks; + EXPECT_CALL(cm_.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce( + Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks = &callbacks_arg; + return &request; + })); + setup(datadog_config, true); + + Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( + Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "202"}}})); + msg->body().add("{}"); + callbacks->onSuccess(request, std::move(msg)); + + EXPECT_CALL(cm_.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce( + Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks = &callbacks_arg; + return &request; + })); + EXPECT_CALL(request, cancel()); + tracer_.reset(); } TEST_F(DatadogConfigTest, CollectorHostname) { @@ -155,17 +208,33 @@ TEST_F(DatadogConfigTest, CollectorHostname) { collector_hostname: fake_host )EOF"); cm_.initializeClusters({"fake_cluster"}, {}); - setup(datadog_config, true); + EXPECT_CALL(tls_.dispatcher_, createTimer_(testing::_)); Http::MockAsyncClientRequest request(&cm_.thread_local_cluster_.async_client_); - Http::AsyncClient::Callbacks* callback; - const absl::optional timeout(std::chrono::seconds(1)); - EXPECT_CALL(cm_.thread_local_cluster_.async_client_, - send_(_, _, Http::AsyncClient::RequestOptions().setTimeout(timeout))) + Http::AsyncClient::Callbacks* callbacks; + EXPECT_CALL(cm_.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce( + Invoke([&](Http::RequestMessagePtr& message, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks = &callbacks_arg; + + EXPECT_EQ("fake_host", message->headers().getHostValue()); + + return &request; + })); + + setup(datadog_config, true); + + Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( + Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "202"}}})); + msg->body().add("{}"); + callbacks->onSuccess(request, std::move(msg)); + + EXPECT_CALL(cm_.thread_local_cluster_.async_client_, send_(_, _, _)) .WillOnce( - Invoke([&](Http::RequestMessagePtr& message, Http::AsyncClient::Callbacks& callbacks, + Invoke([&](Http::RequestMessagePtr& message, Http::AsyncClient::Callbacks& callbacks_arg, const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { - callback = &callbacks; + callbacks = &callbacks_arg; // This is the crux of this test. EXPECT_EQ("fake_host", message->headers().getHostValue()); @@ -182,9 +251,23 @@ TEST_F(DatadogConfigTest, CollectorHostname) { timer_->invokeCallback(); - Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl(Http::ResponseHeaderMapPtr{ - new Http::TestResponseHeaderMapImpl{{":status", "200"}, {"content-length", "0"}}})); - callback->onSuccess(request, std::move(msg)); + msg.reset(new Http::ResponseMessageImpl( + Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "200"}}})); + msg->body().add("{}"); + callbacks->onSuccess(request, std::move(msg)); + + EXPECT_CALL(cm_.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce( + Invoke([&](Http::RequestMessagePtr& message, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks = &callbacks_arg; + + EXPECT_EQ("fake_host", message->headers().getHostValue()); + + return &request; + })); + EXPECT_CALL(request, cancel()); + tracer_.reset(); } } // namespace diff --git a/test/extensions/tracers/datadog/span_test.cc b/test/extensions/tracers/datadog/span_test.cc index 20ea40e4c26f5..1c09eed42ea18 100644 --- a/test/extensions/tracers/datadog/span_test.cc +++ b/test/extensions/tracers/datadog/span_test.cc @@ -78,7 +78,9 @@ class MockIDGenerator : public datadog::tracing::IDGenerator { std::uint64_t span_id() const override { return id_; } - datadog::tracing::TraceID trace_id() const override { return datadog::tracing::TraceID{id_}; } + datadog::tracing::TraceID trace_id(const datadog::tracing::TimePoint&) const override { + return datadog::tracing::TraceID{id_}; + } }; class DatadogTracerSpanTest : public testing::Test { @@ -88,8 +90,7 @@ class DatadogTracerSpanTest : public testing::Test { tracer_( // Override the tracer's ID generator so that all trace IDs and span // IDs are 0xcafebabe. - *datadog::tracing::finalize_config(config_), std::make_shared(id_), - datadog::tracing::default_clock), + *datadog::tracing::finalize_config(config_), std::make_shared(id_)), span_(tracer_.create_span()) {} private: diff --git a/test/extensions/tracers/datadog/tracer_test.cc b/test/extensions/tracers/datadog/tracer_test.cc index 7cd37a8a33291..ad7ef1011e6f9 100644 --- a/test/extensions/tracers/datadog/tracer_test.cc +++ b/test/extensions/tracers/datadog/tracer_test.cc @@ -73,9 +73,11 @@ TEST_F(DatadogTracerTest, Breathing) { // does not throw exceptions. datadog::tracing::TracerConfig config; config.defaults.service = "envoy"; + config.report_traces = false; + config.report_telemetry = false; Tracer tracer("fake_cluster", "test_host", config, cluster_manager_, *store_.rootScope(), - thread_local_slot_allocator_); + thread_local_slot_allocator_, time_); } TEST_F(DatadogTracerTest, NoOpMode) { @@ -83,6 +85,8 @@ TEST_F(DatadogTracerTest, NoOpMode) { // `startSpan` subsequently returns `NullSpan` instances. datadog::tracing::TracerConfig config; config.defaults.service = "envoy"; + config.report_traces = false; + config.report_telemetry = false; datadog::tracing::TraceSamplerConfig::Rule invalid_rule; // The `sample_rate`, below, is invalid (should be between 0.0 and 1.0). // As a result, the constructor of `Tracer` will fail to initialize the @@ -92,7 +96,7 @@ TEST_F(DatadogTracerTest, NoOpMode) { config.trace_sampler.rules.push_back(invalid_rule); Tracer tracer("fake_cluster", "test_host", config, cluster_manager_, *store_.rootScope(), - thread_local_slot_allocator_); + thread_local_slot_allocator_, time_); Tracing::TestTraceContextImpl context{}; // Any values will do for the sake of this test. @@ -116,12 +120,14 @@ TEST_F(DatadogTracerTest, SpanProperties) { // resulting span. datadog::tracing::TracerConfig config; config.defaults.service = "envoy"; + config.report_traces = false; + config.report_telemetry = false; // Configure the tracer to keep all spans. We then override that // configuration in the `Tracing::Decision`, below. config.trace_sampler.sample_rate = 1.0; // 100% Tracer tracer("fake_cluster", "test_host", config, cluster_manager_, *store_.rootScope(), - thread_local_slot_allocator_); + thread_local_slot_allocator_, time_); Tracing::TestTraceContextImpl context{}; // A sampling decision of "false" forces the created trace to be dropped, @@ -165,9 +171,11 @@ TEST_F(DatadogTracerTest, ExtractionSuccess) { // the extracted trace. datadog::tracing::TracerConfig config; config.defaults.service = "envoy"; + config.report_traces = false; + config.report_telemetry = false; Tracer tracer("fake_cluster", "test_host", config, cluster_manager_, *store_.rootScope(), - thread_local_slot_allocator_); + thread_local_slot_allocator_, time_); // Any values will do for the sake of this test. Tracing::Decision decision; @@ -204,9 +212,11 @@ TEST_F(DatadogTracerTest, ExtractionFailure) { // will be the start of a new trace). datadog::tracing::TracerConfig config; config.defaults.service = "envoy"; + config.report_traces = false; + config.report_telemetry = false; Tracer tracer("fake_cluster", "test_host", config, cluster_manager_, *store_.rootScope(), - thread_local_slot_allocator_); + thread_local_slot_allocator_, time_); // Any values will do for the sake of this test. Tracing::Decision decision; @@ -311,8 +321,10 @@ TEST_F(DatadogTracerTest, EnvoySamplingVersusExtractedSampling) { EnvVarGuard guard{"DD_TRACE_PROPAGATION_STYLE", style_name}; datadog::tracing::TracerConfig config; config.defaults.service = "envoy"; + config.report_traces = false; + config.report_telemetry = false; Tracer tracer("fake_cluster", "test_host", config, cluster_manager_, *store_.rootScope(), - thread_local_slot_allocator_); + thread_local_slot_allocator_, time_); Tracing::Decision envoy_decision; envoy_decision.reason = Tracing::Reason::Sampling; From d42b11700275ca86d8924b990525d88673447eac Mon Sep 17 00:00:00 2001 From: Caleb Gilmour Date: Wed, 20 Dec 2023 10:24:58 +0000 Subject: [PATCH 2/4] Auto-generate runtime id instead of making it common for all threads Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/config.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/extensions/tracers/datadog/config.cc b/source/extensions/tracers/datadog/config.cc index 4bd64259df839..aa1e4832debc3 100644 --- a/source/extensions/tracers/datadog/config.cc +++ b/source/extensions/tracers/datadog/config.cc @@ -24,7 +24,6 @@ DatadogTracerFactory::makeConfig(const envoy::config::trace::v3::DatadogConfig& datadog::tracing::TracerConfig config; config.defaults.version = "envoy " + Envoy::VersionInfo::version(); config.defaults.name = "envoy.proxy"; - config.runtime_id = datadog::tracing::RuntimeID::generate(); if (proto_config.service_name().empty()) { config.defaults.service = "envoy"; } else { From f07e39850b2e0a34b66127abf533bb88464febb3 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Tue, 26 Dec 2023 18:05:08 -0500 Subject: [PATCH 3/4] more specific time parameter descriptions Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/agent_http_client.h | 8 ++++---- source/extensions/tracers/datadog/tracer.h | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/source/extensions/tracers/datadog/agent_http_client.h b/source/extensions/tracers/datadog/agent_http_client.h index af39bb2daa5fd..2a38d4ce23802 100644 --- a/source/extensions/tracers/datadog/agent_http_client.h +++ b/source/extensions/tracers/datadog/agent_http_client.h @@ -45,7 +45,7 @@ class AgentHTTPClient : public datadog::tracing::HTTPClient, * @param reference_host the value to use for the "Host" HTTP request header * @param stats a collection of counters used to keep track of events, such as * when a request fails - * @param time_source supplies the time source + * @param time_source clocks used for calculating request timeouts */ AgentHTTPClient(Upstream::ClusterManager& cluster_manager, const std::string& cluster, const std::string& reference_host, TracerStats& stats, TimeSource& time_source); @@ -67,10 +67,10 @@ class AgentHTTPClient : public datadog::tracing::HTTPClient, * @param body data to send as POST request body * @param on_response callback to invoke when a complete response is received * @param on_error callback to invoke when an error occurs before a complete - * response is received. - * @param deadline a timepoint that a request must be completed by. + * response is received + * @param deadline time after which a response is not expected * @return \c datadog::tracing::Error if an error occurs, or - * \c datadog::tracing::nullopt otherwise. + * \c datadog::tracing::nullopt otherwise */ datadog::tracing::Expected post(const URL& url, HeadersSetter set_headers, std::string body, ResponseHandler on_response, ErrorHandler on_error, diff --git a/source/extensions/tracers/datadog/tracer.h b/source/extensions/tracers/datadog/tracer.h index c79c052c08bc6..7baf3d6c12772 100644 --- a/source/extensions/tracers/datadog/tracer.h +++ b/source/extensions/tracers/datadog/tracer.h @@ -51,8 +51,8 @@ class Tracer : public Tracing::Driver, private Logger::Loggable Date: Tue, 26 Dec 2023 18:19:14 -0500 Subject: [PATCH 4/4] correct the release date of dd-trace-cpp v0.1.12 Signed-off-by: David Goffredo --- bazel/repository_locations.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 2b884321f0c17..6d9204acc7354 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -589,7 +589,7 @@ REPOSITORY_LOCATIONS_SPEC = dict( urls = ["https://github.com/DataDog/dd-trace-cpp/archive/v{version}.tar.gz"], use_category = ["observability_ext"], extensions = ["envoy.tracers.datadog"], - release_date = "2023-03-31", + release_date = "2023-11-17", cpe = "N/A", license = "Apache-2.0", license_url = "https://github.com/DataDog/dd-trace-cpp/blob/v{version}/LICENSE.md",