Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions bazel/repository_locations.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -583,13 +583,13 @@ 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"],
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",
Expand Down
22 changes: 11 additions & 11 deletions source/extensions/tracers/datadog/agent_http_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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_) {
Expand All @@ -38,12 +38,10 @@ AgentHTTPClient::~AgentHTTPClient() {

// datadog::tracing::HTTPClient

datadog::tracing::Expected<void> 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<void>
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::RequestMessageImpl>();
Http::RequestHeaderMap& headers = message->headers();
headers.setReferenceMethod(Http::Headers::get().MethodValues.Post);
Expand All @@ -54,7 +52,10 @@ datadog::tracing::Expected<void> 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<std::chrono::milliseconds>(
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_);
Expand All @@ -64,8 +65,7 @@ datadog::tracing::Expected<void> 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,
Expand Down
13 changes: 8 additions & 5 deletions source/extensions/tracers/datadog/agent_http_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please mention time_source in this contract.

* @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);
const std::string& reference_host, TracerStats& stats, TimeSource& time_source);
~AgentHTTPClient() override;

// datadog::tracing::HTTPClient
Expand All @@ -66,13 +67,14 @@ 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.
* 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<void> 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
Expand All @@ -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
Expand Down
6 changes: 4 additions & 2 deletions source/extensions/tracers/datadog/config.cc
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "source/extensions/tracers/datadog/config.h"

#include <datadog/runtime_id.h>
#include <datadog/tracer_config.h>

#include <memory>
Expand Down Expand Up @@ -43,10 +44,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<Tracer>(
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());
}

/**
Expand Down
14 changes: 8 additions & 6 deletions source/extensions/tracers/datadog/tracer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,12 @@ namespace {
std::shared_ptr<Tracer::ThreadLocalTracer> 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>(logger);
config.agent.event_scheduler = std::make_shared<EventScheduler>(dispatcher);
config.agent.http_client = std::make_shared<AgentHTTPClient>(
cluster_manager, collector_cluster, collector_reference_host, tracer_stats);
cluster_manager, collector_cluster, collector_reference_host, tracer_stats, time_source);

datadog::tracing::Expected<datadog::tracing::FinalizedTracerConfig> maybe_config =
datadog::tracing::finalize_config(config);
Expand All @@ -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<ThreadLocalTracer>::makeUnique(thread_local_slot_allocator)) {
Expand All @@ -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);
});
}

Expand Down
5 changes: 3 additions & 2 deletions source/extensions/tracers/datadog/tracer.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,13 @@ class Tracer : public Tracing::Driver, private Logger::Loggable<Logger::Id::trac
* cluster is retrieved in order to send HTTP request containing traces
* @param scope statistics scope from which \c TracerStats can be created
* @param thread_local_slot_allocator slot allocator for installing a
* thread-local instance of the tracer.
* thread-local instance of the tracer
* @param time_source clocks used for calculating HTTP request timeouts
*/
explicit 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);
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please mention time_source in the contract above. I think it's reasonable to say that it's used to calculate request timeouts (could be used for something else in the future, but probably not).


struct ThreadLocalTracer : public ThreadLocal::ThreadLocalObject {
/**
Expand Down
50 changes: 33 additions & 17 deletions test/extensions/tracers/datadog/agent_http_client_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class DatadogAgentHttpClientTest : public testing::Test {
DatadogAgentHttpClientTest()
: request_(&cluster_manager_.instance_.thread_local_cluster_.async_client_),
stats_(makeTracerStats(*store_.rootScope())),
client_(cluster_manager_.instance_, "fake_cluster", "test_host", stats_) {
client_(cluster_manager_.instance_, "fake_cluster", "test_host", stats_, time_) {
url_.scheme = "http";
url_.authority = "localhost:8126";
url_.path = "/foo/bar";
Expand All @@ -67,6 +67,7 @@ class DatadogAgentHttpClientTest : public testing::Test {
std::string body)>
on_response_;
testing::MockFunction<void(datadog::tracing::Error)> on_error_;
Event::SimulatedTimeSystem time_;
};

TEST_F(DatadogAgentHttpClientTest, PathFromURL) {
Expand All @@ -86,7 +87,8 @@ TEST_F(DatadogAgentHttpClientTest, PathFromURL) {
EXPECT_CALL(request_, cancel());

const auto ignore = [](auto&&...) {};
datadog::tracing::Expected<void> result = client_.post(url_, ignore, "", ignore, ignore);
datadog::tracing::Expected<void> 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());
Expand All @@ -98,10 +100,11 @@ TEST_F(DatadogAgentHttpClientTest, MissingThreadLocalCluster) {
// the "reports skipped no cluster" counter.

NiceMock<Upstream::MockClusterManager> 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<void> result = client.post(url_, ignore, "", ignore, ignore);
datadog::tracing::Expected<void> 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());
Expand Down Expand Up @@ -143,7 +146,8 @@ TEST_F(DatadogAgentHttpClientTest, RequestHeaders) {
EXPECT_CALL(request_, cancel());

const auto ignore = [](auto&&...) {};
datadog::tracing::Expected<void> result = client_.post(url_, set_headers, "", ignore, ignore);
datadog::tracing::Expected<void> 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());
Expand Down Expand Up @@ -182,7 +186,8 @@ TEST_F(DatadogAgentHttpClientTest, RequestBody) {
EXPECT_CALL(request_, cancel());

const auto ignore = [](auto&&...) {};
datadog::tracing::Expected<void> result = client_.post(url_, ignore, body, ignore, ignore);
datadog::tracing::Expected<void> 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());
Expand Down Expand Up @@ -213,7 +218,8 @@ TEST_F(DatadogAgentHttpClientTest, OnResponse200) {

const auto ignore = [](auto&&...) {};
datadog::tracing::Expected<void> 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(
Expand Down Expand Up @@ -252,7 +258,8 @@ TEST_F(DatadogAgentHttpClientTest, OnResponseNot200) {

const auto ignore = [](auto&&...) {};
datadog::tracing::Expected<void> 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
Expand Down Expand Up @@ -293,7 +300,8 @@ TEST_F(DatadogAgentHttpClientTest, OnResponseBogusRequest) {

const auto ignore = [](auto&&...) {};
datadog::tracing::Expected<void> 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(
Expand Down Expand Up @@ -333,7 +341,8 @@ TEST_F(DatadogAgentHttpClientTest, OnErrorStreamReset) {

const auto ignore = [](auto&&...) {};
datadog::tracing::Expected<void> 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(
Expand Down Expand Up @@ -369,7 +378,8 @@ TEST_F(DatadogAgentHttpClientTest, OnErrorOther) {

const auto ignore = [](auto&&...) {};
datadog::tracing::Expected<void> 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(
Expand Down Expand Up @@ -402,7 +412,8 @@ TEST_F(DatadogAgentHttpClientTest, OnErrorBogusRequest) {

const auto ignore = [](auto&&...) {};
datadog::tracing::Expected<void> 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(
Expand Down Expand Up @@ -445,7 +456,8 @@ TEST_F(DatadogAgentHttpClientTest, SendFailReturnsError) {

const auto ignore = [](auto&&...) {};
datadog::tracing::Expected<void> 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());
Expand Down Expand Up @@ -491,7 +503,8 @@ TEST_F(DatadogAgentHttpClientTest, SkipReportIfCollectorClusterHasBeenRemoved) {

// Attempt to send a request.
const auto ignore = [](auto&&...) {};
datadog::tracing::Expected<void> result = client_.post(url_, ignore, "", ignore, ignore);
datadog::tracing::Expected<void> result = client_.post(
url_, ignore, "", ignore, ignore, time_.monotonicTime() + std::chrono::seconds(1));
EXPECT_TRUE(result);

// Verify observability.
Expand All @@ -515,7 +528,8 @@ TEST_F(DatadogAgentHttpClientTest, SkipReportIfCollectorClusterHasBeenRemoved) {

// Attempt to send a request.
const auto ignore = [](auto&&...) {};
datadog::tracing::Expected<void> result = client_.post(url_, ignore, "", ignore, ignore);
datadog::tracing::Expected<void> result = client_.post(
url_, ignore, "", ignore, ignore, time_.monotonicTime() + std::chrono::seconds(1));
EXPECT_TRUE(result);

// Verify observability.
Expand Down Expand Up @@ -543,7 +557,8 @@ TEST_F(DatadogAgentHttpClientTest, SkipReportIfCollectorClusterHasBeenRemoved) {

// Attempt to send a request.
const auto ignore = [](auto&&...) {};
datadog::tracing::Expected<void> result = client_.post(url_, ignore, "", ignore, ignore);
datadog::tracing::Expected<void> result = client_.post(
url_, ignore, "", ignore, ignore, time_.monotonicTime() + std::chrono::seconds(1));
EXPECT_TRUE(result);

// Complete in-flight request.
Expand All @@ -570,7 +585,8 @@ TEST_F(DatadogAgentHttpClientTest, SkipReportIfCollectorClusterHasBeenRemoved) {

// Attempt to send a request.
const auto ignore = [](auto&&...) {};
datadog::tracing::Expected<void> result = client_.post(url_, ignore, "", ignore, ignore);
datadog::tracing::Expected<void> result = client_.post(
url_, ignore, "", ignore, ignore, time_.monotonicTime() + std::chrono::seconds(1));
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a TEST_F for timeouts, if the mocking machinery permits it.

Regardless of whether we have a dedicated unit test, let's find out what timeouts actually do here within Envoy.

EXPECT_TRUE(result);

// Complete in-flight request.
Expand Down
Loading