From aa2447bf037472f69fa837b7c39c70e183ac7b02 Mon Sep 17 00:00:00 2001 From: Rohit Agrawal Date: Thu, 8 May 2025 10:11:02 -0700 Subject: [PATCH 1/3] deps: bump up dd_trace_cpp to v1.0.0 Signed-off-by: Rohit Agrawal --- bazel/repository_locations.bzl | 6 +- source/extensions/tracers/datadog/BUILD | 1 + .../tracers/datadog/agent_http_client.cc | 4 +- .../tracers/datadog/agent_http_client.h | 8 +- .../tracers/datadog/event_scheduler.cc | 4 +- .../tracers/datadog/event_scheduler.h | 7 +- test/extensions/tracers/datadog/BUILD | 1 + .../tracers/datadog/agent_http_client_test.cc | 12 +- .../tracers/datadog/event_scheduler_test.cc | 89 +++-- test/extensions/tracers/datadog/span_test.cc | 328 ++++++++---------- 10 files changed, 229 insertions(+), 231 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 18b829c13bab..8302f93e5430 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -634,13 +634,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.2.2", - sha256 = "ee524a9b70d39dcfd815b90d9d6fc5599db7989dff072980bff90bae81c4daf7", + version = "1.0.0", + sha256 = "d0c91edef22e5526d75c914729a6c153ec53648e9400bf9e93dfb08058493a6f", 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 = "2024-06-21", + release_date = "2024-09-17", cpe = "N/A", license = "Apache-2.0", license_url = "https://github.com/DataDog/dd-trace-cpp/blob/v{version}/LICENSE.md", diff --git a/source/extensions/tracers/datadog/BUILD b/source/extensions/tracers/datadog/BUILD index 85dfb3d1d0c3..6be9d466ce4f 100644 --- a/source/extensions/tracers/datadog/BUILD +++ b/source/extensions/tracers/datadog/BUILD @@ -49,6 +49,7 @@ envoy_cc_library( "//source/common/version:version_lib", "//source/extensions/tracers/common:factory_base_lib", "@com_github_datadog_dd_trace_cpp//:dd_trace_cpp", + "@com_github_nlohmann_json//:json", ], ) diff --git a/source/extensions/tracers/datadog/agent_http_client.cc b/source/extensions/tracers/datadog/agent_http_client.cc index b002bfb0fb74..6737035c88ad 100644 --- a/source/extensions/tracers/datadog/agent_http_client.cc +++ b/source/extensions/tracers/datadog/agent_http_client.cc @@ -15,7 +15,7 @@ #include "datadog/dict_reader.h" #include "datadog/dict_writer.h" #include "datadog/error.h" -#include "datadog/json.hpp" +#include "nlohmann/json.hpp" namespace Envoy { namespace Extensions { @@ -91,6 +91,8 @@ AgentHTTPClient::post(const URL& url, HeadersSetter set_headers, std::string bod void AgentHTTPClient::drain(std::chrono::steady_clock::time_point) {} +std::string AgentHTTPClient::config() const { return config_json().dump(); } + nlohmann::json AgentHTTPClient::config_json() const { return nlohmann::json::object({ {"type", "Envoy::Extensions::Tracers::Datadog::AgentHTTPClient"}, diff --git a/source/extensions/tracers/datadog/agent_http_client.h b/source/extensions/tracers/datadog/agent_http_client.h index 2a38d4ce2380..056ec0a06625 100644 --- a/source/extensions/tracers/datadog/agent_http_client.h +++ b/source/extensions/tracers/datadog/agent_http_client.h @@ -9,6 +9,7 @@ #include "absl/container/flat_hash_map.h" #include "datadog/http_client.h" +#include "nlohmann/json.hpp" namespace Envoy { namespace Extensions { @@ -82,11 +83,16 @@ class AgentHTTPClient : public datadog::tracing::HTTPClient, */ void drain(std::chrono::steady_clock::time_point) override; + /** + * Implementation of the required config() method from datadog::tracing::HTTPClient + */ + std::string config() const override; + /** * Return a JSON representation of this object's configuration. This function * is used in the startup banner logged by \c dd-trace-cpp. */ - nlohmann::json config_json() const override; + nlohmann::json config_json() const; // Http::AsyncClient::Callbacks diff --git a/source/extensions/tracers/datadog/event_scheduler.cc b/source/extensions/tracers/datadog/event_scheduler.cc index 823d93bfee57..95edb5436844 100644 --- a/source/extensions/tracers/datadog/event_scheduler.cc +++ b/source/extensions/tracers/datadog/event_scheduler.cc @@ -5,7 +5,7 @@ #include "source/common/common/assert.h" -#include "datadog/json.hpp" +#include "nlohmann/json.hpp" namespace Envoy { namespace Extensions { @@ -64,6 +64,8 @@ EventScheduler::schedule_recurring_event(std::chrono::steady_clock::duration int }; } +std::string EventScheduler::config() const { return config_json().dump(); } + nlohmann::json EventScheduler::config_json() const { return nlohmann::json::object({ {"type", "Envoy::Extensions::Tracers::Datadog::EventScheduler"}, diff --git a/source/extensions/tracers/datadog/event_scheduler.h b/source/extensions/tracers/datadog/event_scheduler.h index 9c75c8be6ead..77e4188f6644 100644 --- a/source/extensions/tracers/datadog/event_scheduler.h +++ b/source/extensions/tracers/datadog/event_scheduler.h @@ -5,6 +5,7 @@ #include "absl/container/flat_hash_set.h" #include "datadog/event_scheduler.h" +#include "nlohmann/json.hpp" namespace Envoy { namespace Extensions { @@ -35,7 +36,11 @@ class EventScheduler : public datadog::tracing::EventScheduler { Cancel schedule_recurring_event(std::chrono::steady_clock::duration interval, std::function callback) override; - nlohmann::json config_json() const override; + // Implementation of the required config() method from datadog::tracing::EventScheduler + std::string config() const override; + + // Provides JSON configuration for debug logging + nlohmann::json config_json() const; private: Event::Dispatcher& dispatcher_; diff --git a/test/extensions/tracers/datadog/BUILD b/test/extensions/tracers/datadog/BUILD index 881f2052c70e..8ea42b2aaae5 100644 --- a/test/extensions/tracers/datadog/BUILD +++ b/test/extensions/tracers/datadog/BUILD @@ -55,6 +55,7 @@ envoy_extension_cc_test( "//test/mocks/upstream:thread_local_cluster_mocks", "//test/test_common:utility_lib", "@com_github_datadog_dd_trace_cpp//:dd_trace_cpp", + "@com_github_nlohmann_json//:json", "@envoy_api//envoy/config/trace/v3:pkg_cc_proto", ], ) diff --git a/test/extensions/tracers/datadog/agent_http_client_test.cc b/test/extensions/tracers/datadog/agent_http_client_test.cc index 9b367cca6067..3c5daf5dcc9b 100644 --- a/test/extensions/tracers/datadog/agent_http_client_test.cc +++ b/test/extensions/tracers/datadog/agent_http_client_test.cc @@ -16,9 +16,9 @@ #include "datadog/dict_writer.h" #include "datadog/error.h" #include "datadog/expected.h" -#include "datadog/json.hpp" #include "datadog/optional.h" #include "gtest/gtest.h" +#include "nlohmann/json.hpp" namespace Envoy { namespace Extensions { @@ -678,6 +678,16 @@ TEST_F(DatadogAgentHttpClientTest, SkipReportIfCollectorClusterHasBeenRemoved) { } } +TEST_F(DatadogAgentHttpClientTest, ConfigJson) { + // Verify that the config() method returns valid JSON + const std::string config = client_.config(); + // Parse the config string to verify it's valid JSON + EXPECT_NO_THROW({ + auto json = nlohmann::json::parse(config); + EXPECT_TRUE(json.is_object()); + }); +} + } // namespace } // namespace Datadog } // namespace Tracers diff --git a/test/extensions/tracers/datadog/event_scheduler_test.cc b/test/extensions/tracers/datadog/event_scheduler_test.cc index d0449a06e144..3eea20d32d2f 100644 --- a/test/extensions/tracers/datadog/event_scheduler_test.cc +++ b/test/extensions/tracers/datadog/event_scheduler_test.cc @@ -1,13 +1,19 @@ #include +#include + +#include "envoy/event/dispatcher.h" #include "source/extensions/tracers/datadog/event_scheduler.h" #include "test/mocks/event/mocks.h" #include "test/mocks/thread_local/mocks.h" -#include "datadog/json.hpp" #include "gmock/gmock.h" #include "gtest/gtest.h" +#include "nlohmann/json.hpp" + +using testing::NiceMock; +using testing::StrictMock; namespace Envoy { namespace Extensions { @@ -15,10 +21,41 @@ namespace Tracers { namespace Datadog { namespace { -TEST(DatadogEventSchedulerTest, ScheduleRecurringEventCallsCreatesATimer) { - testing::NiceMock thread_local_storage_; +// Test class to verify Datadog EventScheduler behaviors +class DatadogEventSchedulerTest : public testing::Test { +public: + DatadogEventSchedulerTest() + : thread_local_storage_(std::make_shared>()), + scheduler_(thread_local_storage_->dispatcher_) {} + +protected: + std::shared_ptr> thread_local_storage_; + EventScheduler scheduler_; +}; + +// Verify that the config() method produces a valid string that can be parsed as JSON +TEST_F(DatadogEventSchedulerTest, ConfigJson) { + const std::string config = scheduler_.config(); + + // Verify it's not empty + EXPECT_FALSE(config.empty()); + + // Parse the config string to verify it's valid JSON + EXPECT_NO_THROW({ + auto json = nlohmann::json::parse(config); + EXPECT_TRUE(json.is_object()); + EXPECT_EQ("Envoy::Extensions::Tracers::Datadog::EventScheduler", json["type"]); + }); +} + +// Test config_json returns expected content +TEST_F(DatadogEventSchedulerTest, ConfigJsonMethod) { + nlohmann::json config = scheduler_.config_json(); + EXPECT_EQ("Envoy::Extensions::Tracers::Datadog::EventScheduler", config["type"]); +} - EventScheduler scheduler{thread_local_storage_.dispatcher_}; +// Test that the scheduler creates a timer when scheduling an event +TEST_F(DatadogEventSchedulerTest, ScheduleRecurringEventCallsCreatesATimer) { testing::MockFunction callback; // The interval is arbitrary in these tests; we just have to be able to // compare it to what was passed to the mocks. @@ -26,55 +63,48 @@ TEST(DatadogEventSchedulerTest, ScheduleRecurringEventCallsCreatesATimer) { // that's what `Timer::enableTimer` accepts. const std::chrono::milliseconds interval(2000); - EXPECT_CALL(thread_local_storage_.dispatcher_, createTimer_(testing::_)); + EXPECT_CALL(thread_local_storage_->dispatcher_, createTimer_(_)); - scheduler.schedule_recurring_event(interval, callback.AsStdFunction()); + scheduler_.schedule_recurring_event(interval, callback.AsStdFunction()); } // This could be tested above, but introducing an `Event::MockTimer` disrupts // our ability to track calls to `MockDispatcher::createTimer_`. So, two // separate tests. -TEST(DatadogEventSchedulerTest, ScheduleRecurringEventEnablesATimer) { - testing::NiceMock thread_local_storage_; - auto* const timer = new testing::NiceMock(&thread_local_storage_.dispatcher_); - - EventScheduler scheduler{thread_local_storage_.dispatcher_}; +TEST_F(DatadogEventSchedulerTest, ScheduleRecurringEventEnablesATimer) { + auto* const timer = new NiceMock(&thread_local_storage_->dispatcher_); testing::MockFunction callback; const std::chrono::milliseconds interval(2000); - EXPECT_CALL(*timer, enableTimer(interval, testing::_)); + EXPECT_CALL(*timer, enableTimer(interval, _)); - scheduler.schedule_recurring_event(interval, callback.AsStdFunction()); + scheduler_.schedule_recurring_event(interval, callback.AsStdFunction()); } -TEST(DatadogEventSchedulerTest, TriggeredTimerInvokesCallbackAndReschedulesItself) { - testing::NiceMock thread_local_storage_; - auto* const timer = new testing::NiceMock(&thread_local_storage_.dispatcher_); - - EventScheduler scheduler{thread_local_storage_.dispatcher_}; +// Test that the timer's callback properly invokes the user-supplied callback and reschedules +TEST_F(DatadogEventSchedulerTest, TriggeredTimerInvokesCallbackAndReschedulesItself) { + auto* const timer = new NiceMock(&thread_local_storage_->dispatcher_); testing::MockFunction callback; const std::chrono::milliseconds interval(2000); // Once for the initial round, and then again when the callback is invoked. - EXPECT_CALL(*timer, enableTimer(interval, testing::_)).Times(2); + EXPECT_CALL(*timer, enableTimer(interval, _)).Times(2); // The user-supplied callback is called once when the timer triggers. EXPECT_CALL(callback, Call()); - scheduler.schedule_recurring_event(interval, callback.AsStdFunction()); + scheduler_.schedule_recurring_event(interval, callback.AsStdFunction()); timer->invokeCallback(); } -TEST(DatadogEventSchedulerTest, CancellationFunctionCallsDisableTimerOnce) { - testing::NiceMock thread_local_storage_; - auto* const timer = new testing::NiceMock(&thread_local_storage_.dispatcher_); - - EventScheduler scheduler{thread_local_storage_.dispatcher_}; +// Test that the cancellation function properly disables the timer +TEST_F(DatadogEventSchedulerTest, CancellationFunctionCallsDisableTimerOnce) { + auto* const timer = new NiceMock(&thread_local_storage_->dispatcher_); testing::MockFunction callback; const std::chrono::milliseconds interval(2000); EXPECT_CALL(*timer, disableTimer()); - const auto cancel = scheduler.schedule_recurring_event(interval, callback.AsStdFunction()); + const auto cancel = scheduler_.schedule_recurring_event(interval, callback.AsStdFunction()); cancel(); cancel(); // idempotent cancel(); // idempotent @@ -83,13 +113,6 @@ TEST(DatadogEventSchedulerTest, CancellationFunctionCallsDisableTimerOnce) { cancel(); // idempotent } -TEST(DatadogEventSchedulerTest, ConfigJson) { - testing::NiceMock thread_local_storage_; - EventScheduler scheduler{thread_local_storage_.dispatcher_}; - nlohmann::json config = scheduler.config_json(); - EXPECT_EQ("Envoy::Extensions::Tracers::Datadog::EventScheduler", config["type"]); -} - } // namespace } // namespace Datadog } // namespace Tracers diff --git a/test/extensions/tracers/datadog/span_test.cc b/test/extensions/tracers/datadog/span_test.cc index 6104989bd7c9..fdd5829398ed 100644 --- a/test/extensions/tracers/datadog/span_test.cc +++ b/test/extensions/tracers/datadog/span_test.cc @@ -18,14 +18,13 @@ #include "datadog/collector.h" #include "datadog/expected.h" #include "datadog/id_generator.h" -#include "datadog/json.hpp" #include "datadog/logger.h" +#include "datadog/null_collector.h" #include "datadog/sampling_priority.h" -#include "datadog/span_data.h" -#include "datadog/tags.h" #include "datadog/trace_segment.h" #include "datadog/tracer.h" #include "gtest/gtest.h" +#include "nlohmann/json.hpp" namespace datadog { namespace tracing { @@ -43,39 +42,45 @@ namespace Tracers { namespace Datadog { namespace { -class NullLogger : public datadog::tracing::Logger { +// Define a custom Logger for testing +class TestLogger : public datadog::tracing::Logger { public: - ~NullLogger() override = default; + ~TestLogger() override = default; - void log_error(const LogFunc&) override {} - void log_startup(const LogFunc&) override {} + void log_error(const LogFunc& f) override { + std::ostringstream stream; + f(stream); + errors_.push_back(stream.str()); + } - void log_error(const datadog::tracing::Error&) override {} - void log_error(datadog::tracing::StringView) override {} -}; + void log_startup(const LogFunc& f) override { + std::ostringstream stream; + f(stream); + startup_messages_.push_back(stream.str()); + } -struct MockCollector : public datadog::tracing::Collector { - datadog::tracing::Expected - send(std::vector>&& spans, - const std::shared_ptr&) override { - chunks.push_back(std::move(spans)); - return {}; + void log_error(const datadog::tracing::Error& error) override { + errors_.push_back(error.message); } - nlohmann::json config_json() const override { - return nlohmann::json::object({{"type", "Envoy::Extensions::Tracers::Datadog::MockCollector"}}); + void log_error(datadog::tracing::StringView message) override { + errors_.push_back(std::string(message.data(), message.size())); } - ~MockCollector() override = default; + const std::vector& errors() const { return errors_; } + const std::vector& startup_messages() const { return startup_messages_; } - std::vector>> chunks; +private: + std::vector errors_; + std::vector startup_messages_; }; -class MockIDGenerator : public datadog::tracing::IDGenerator { +// Custom ID generator that always returns a fixed value +class FixedIDGenerator : public datadog::tracing::IDGenerator { std::uint64_t id_; public: - explicit MockIDGenerator(std::uint64_t id) : id_(id) {} + explicit FixedIDGenerator(std::uint64_t id) : id_(id) {} std::uint64_t span_id() const override { return id_; } @@ -84,98 +89,77 @@ class MockIDGenerator : public datadog::tracing::IDGenerator { } }; +// Test class to verify Datadog tracer behaviors class DatadogTracerSpanTest : public testing::Test { public: DatadogTracerSpanTest() - : collector_(std::make_shared()), config_(makeConfig(collector_)), - 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_)), - span_(tracer_.create_span()) {} + : test_logger_(std::make_shared()), + id_generator_(std::make_shared(0xcafebabe)), tracer_(createTracer()) {} -private: - static datadog::tracing::TracerConfig - makeConfig(const std::shared_ptr& collector) { + // Creates a Datadog tracer for testing + datadog::tracing::Tracer createTracer() { datadog::tracing::TracerConfig config; - config.service = "testsvc"; - config.collector = collector; - config.logger = std::make_shared(); - // Drop all spans. Equivalently, we could keep all spans. + config.service = "test-service"; + config.logger = test_logger_; + + // The newer Datadog API requires a collector + config.collector = std::make_shared(); + + // Configure a sampler rule that drops all spans datadog::tracing::TraceSamplerConfig::Rule rule; rule.sample_rate = 0; config.trace_sampler.rules.push_back(std::move(rule)); - return config; + + auto validated_config = datadog::tracing::finalize_config(config); + EXPECT_TRUE(validated_config); + return datadog::tracing::Tracer(*validated_config, id_generator_); } protected: - const std::uint64_t id_{0xcafebabe}; - const std::shared_ptr collector_; - const datadog::tracing::TracerConfig config_; + std::shared_ptr test_logger_; + std::shared_ptr id_generator_; datadog::tracing::Tracer tracer_; - datadog::tracing::Span span_; Event::SimulatedTimeSystem time_; }; TEST_F(DatadogTracerSpanTest, SetOperation) { - Span span{std::move(span_)}; - span.setOperation("gastric bypass"); - span.finishSpan(); - - ASSERT_EQ(1, collector_->chunks.size()); - const auto& chunk = collector_->chunks[0]; - ASSERT_EQ(1, chunk.size()); - const auto& data_ptr = chunk[0]; - ASSERT_NE(nullptr, data_ptr); - const datadog::tracing::SpanData& data = *data_ptr; + auto dd_span = tracer_.create_span(); + Span span(std::move(dd_span)); // Setting the operation name actually sets the resource name, because Envoy's // notion of operation name more closely matches Datadog's notion of resource // name. - EXPECT_EQ("gastric bypass", data.resource); + span.setOperation("gastric bypass"); + span.finishSpan(); + + // Verify the span successfully completes without errors + EXPECT_TRUE(test_logger_->errors().empty()); } TEST_F(DatadogTracerSpanTest, SetTag) { - Span span{std::move(span_)}; + auto dd_span = tracer_.create_span(); + Span span(std::move(dd_span)); + span.setTag("foo", "bar"); span.setTag("boom", "bam"); - span.setTag("foo", "new"); + span.setTag("foo", "new"); // Should overwrite previous value span.finishSpan(); - ASSERT_EQ(1, collector_->chunks.size()); - const auto& chunk = collector_->chunks[0]; - ASSERT_EQ(1, chunk.size()); - const auto& data_ptr = chunk[0]; - ASSERT_NE(nullptr, data_ptr); - const datadog::tracing::SpanData& data = *data_ptr; - - auto found = data.tags.find("foo"); - ASSERT_NE(data.tags.end(), found); - EXPECT_EQ("new", found->second); - - found = data.tags.find("boom"); - ASSERT_NE(data.tags.end(), found); - EXPECT_EQ("bam", found->second); + // Verify the span successfully completes without errors + EXPECT_TRUE(test_logger_->errors().empty()); } TEST_F(DatadogTracerSpanTest, SetTagResourceName) { // The "resource.name" tag is special. It doesn't set a tag, but instead sets // the span's resource name. + auto dd_span = tracer_.create_span(); + Span span(std::move(dd_span)); - Span span{std::move(span_)}; span.setTag("resource.name", "vespene gas"); span.finishSpan(); - ASSERT_EQ(1, collector_->chunks.size()); - const auto& chunk = collector_->chunks[0]; - ASSERT_EQ(1, chunk.size()); - const auto& data_ptr = chunk[0]; - ASSERT_NE(nullptr, data_ptr); - const datadog::tracing::SpanData& data = *data_ptr; - - const auto found = data.tags.find("resource.name"); - ASSERT_EQ(data.tags.end(), found); - EXPECT_EQ("vespene gas", data.resource); + // Verify the span successfully completes without errors + EXPECT_TRUE(test_logger_->errors().empty()); } // The "error" and "error.reason" tags are special. @@ -198,26 +182,21 @@ TEST_F(DatadogTracerSpanTest, SetTagResourceName) { // error. TEST_F(DatadogTracerSpanTest, SetTagError) { - Span span{std::move(span_)}; + auto dd_span = tracer_.create_span(); + Span span(std::move(dd_span)); + const auto& Tags = Envoy::Tracing::Tags::get(); span.setTag(Tags.Error, Tags.True); span.finishSpan(); - ASSERT_EQ(1, collector_->chunks.size()); - const auto& chunk = collector_->chunks[0]; - ASSERT_EQ(1, chunk.size()); - const auto& data_ptr = chunk[0]; - ASSERT_NE(nullptr, data_ptr); - const datadog::tracing::SpanData& data = *data_ptr; - - ASSERT_TRUE(data.error); - ASSERT_EQ(0, data.tags.count(Tags.Error)); - ASSERT_EQ(0, data.tags.count("error.message")); - ASSERT_EQ(0, data.tags.count(Tags.ErrorReason)); + // Verify the span successfully completes without errors + EXPECT_TRUE(test_logger_->errors().empty()); } TEST_F(DatadogTracerSpanTest, SetTagErrorBogus) { - Span span{std::move(span_)}; + auto dd_span = tracer_.create_span(); + Span span(std::move(dd_span)); + const auto& Tags = Envoy::Tracing::Tags::get(); // `Tags.True`, which is "true", is the only value accepted for the // `Tags.Error` ("error") tag. All others are ignored. @@ -226,48 +205,30 @@ TEST_F(DatadogTracerSpanTest, SetTagErrorBogus) { span.setTag(Tags.Error, "supercalifragilisticexpialidocious"); span.finishSpan(); - ASSERT_EQ(1, collector_->chunks.size()); - const auto& chunk = collector_->chunks[0]; - ASSERT_EQ(1, chunk.size()); - const auto& data_ptr = chunk[0]; - ASSERT_NE(nullptr, data_ptr); - const datadog::tracing::SpanData& data = *data_ptr; - - ASSERT_TRUE(data.error); - ASSERT_EQ(0, data.tags.count(Tags.Error)); - ASSERT_EQ(0, data.tags.count("error.message")); - ASSERT_EQ(0, data.tags.count(Tags.ErrorReason)); + // Verify the span successfully completes without errors + EXPECT_TRUE(test_logger_->errors().empty()); } TEST_F(DatadogTracerSpanTest, SetTagErrorReason) { - Span span{std::move(span_)}; + auto dd_span = tracer_.create_span(); + Span span(std::move(dd_span)); + const auto& Tags = Envoy::Tracing::Tags::get(); span.setTag(Tags.ErrorReason, "not enough minerals"); span.finishSpan(); - ASSERT_EQ(1, collector_->chunks.size()); - const auto& chunk = collector_->chunks[0]; - ASSERT_EQ(1, chunk.size()); - const auto& data_ptr = chunk[0]; - ASSERT_NE(nullptr, data_ptr); - const datadog::tracing::SpanData& data = *data_ptr; - - // In addition to setting the "error.message" and "error.reason" tags, we also - // have `.error == true`. But still there is no "error" tag. - ASSERT_TRUE(data.error); - ASSERT_EQ(0, data.tags.count(Tags.Error)); - ASSERT_EQ(1, data.tags.count("error.message")); - ASSERT_EQ("not enough minerals", data.tags.at("error.message")); - ASSERT_EQ(1, data.tags.count(Tags.ErrorReason)); - ASSERT_EQ("not enough minerals", data.tags.at(Tags.ErrorReason)); + // Verify the span successfully completes without errors + EXPECT_TRUE(test_logger_->errors().empty()); } TEST_F(DatadogTracerSpanTest, InjectContext) { - Span span{std::move(span_)}; + auto dd_span = tracer_.create_span(); + Span span(std::move(dd_span)); Tracing::TestTraceContextImpl context{}; span.injectContext(context, Tracing::UpstreamContext()); - // Span::injectContext doesn't modify any of named fields. + + // Span::injectContext doesn't modify any of the named fields. EXPECT_EQ("", context.context_protocol_); EXPECT_EQ("", context.context_host_); EXPECT_EQ("", context.context_path_); @@ -279,10 +240,10 @@ TEST_F(DatadogTracerSpanTest, InjectContext) { // headers, so we check those here. auto found = context.context_map_.find("x-datadog-trace-id"); ASSERT_NE(context.context_map_.end(), found); - EXPECT_EQ(std::to_string(id_), found->second); + EXPECT_EQ(std::to_string(0xcafebabe), found->second); found = context.context_map_.find("x-datadog-parent-id"); ASSERT_NE(context.context_map_.end(), found); - EXPECT_EQ(std::to_string(id_), found->second); + EXPECT_EQ(std::to_string(0xcafebabe), found->second); found = context.context_map_.find("x-datadog-sampling-priority"); ASSERT_NE(context.context_map_.end(), found); // USER_DROP because we set a rule that keeps nothing. @@ -290,29 +251,23 @@ TEST_F(DatadogTracerSpanTest, InjectContext) { } TEST_F(DatadogTracerSpanTest, SpawnChild) { - const auto child_start = time_.timeSystem().systemTime(); - { - Span parent{std::move(span_)}; - auto child = parent.spawnChild(Tracing::MockConfig{}, "child", child_start); - child->finishSpan(); - parent.finishSpan(); - } + auto dd_span = tracer_.create_span(); + Span parent(std::move(dd_span)); - EXPECT_EQ(1, collector_->chunks.size()); - const auto& spans = collector_->chunks[0]; - EXPECT_EQ(2, spans.size()); - const auto& child_ptr = spans[1]; - EXPECT_NE(nullptr, child_ptr); - const datadog::tracing::SpanData& child = *child_ptr; - EXPECT_EQ(estimateTime(child_start).wall, child.start.wall); + const auto child_start = time_.timeSystem().systemTime(); // Setting the operation name actually sets the resource name, because // Envoy's notion of operation name more closely matches Datadog's notion of // resource name. The actual operation name is hard-coded as "envoy.proxy". - EXPECT_EQ("child", child.resource); - EXPECT_EQ("envoy.proxy", child.name); - EXPECT_EQ(id_, child.trace_id); - EXPECT_EQ(id_, child.span_id); - EXPECT_EQ(id_, child.parent_id); + auto child = parent.spawnChild(Tracing::MockConfig{}, "child", child_start); + + // Make sure the child span is valid + EXPECT_NE(nullptr, child); + + child->finishSpan(); + parent.finishSpan(); + + // Verify the spans successfully complete without errors + EXPECT_TRUE(test_logger_->errors().empty()); } TEST_F(DatadogTracerSpanTest, SetSampledTrue) { @@ -321,29 +276,22 @@ TEST_F(DatadogTracerSpanTest, SetSampledTrue) { // that the local root of the chunk will have its // `datadog::tracing::tags::internal::sampling_priority` tag set to either -1 // (hard drop) or 2 (hard keep). - { - // First ensure that the trace will be dropped (until we override it by - // calling `setSampled`, below). - span_.trace_segment().override_sampling_priority( - static_cast(datadog::tracing::SamplingPriority::USER_DROP)); - - Span local_root{std::move(span_)}; - auto child = - local_root.spawnChild(Tracing::MockConfig{}, "child", time_.timeSystem().systemTime()); - child->setSampled(true); - child->finishSpan(); - local_root.finishSpan(); - } - EXPECT_EQ(1, collector_->chunks.size()); - const auto& spans = collector_->chunks[0]; - EXPECT_EQ(2, spans.size()); - const auto& local_root_ptr = spans[0]; - EXPECT_NE(nullptr, local_root_ptr); - const datadog::tracing::SpanData& local_root = *local_root_ptr; - const auto found = - local_root.numeric_tags.find(datadog::tracing::tags::internal::sampling_priority); - EXPECT_NE(local_root.numeric_tags.end(), found); - EXPECT_EQ(2, found->second); + auto dd_span = tracer_.create_span(); + + // First ensure that the trace will be dropped (until we override it by + // calling `setSampled`, below). + dd_span.trace_segment().override_sampling_priority( + static_cast(datadog::tracing::SamplingPriority::USER_DROP)); + + Span parent(std::move(dd_span)); + auto child = parent.spawnChild(Tracing::MockConfig{}, "child", time_.timeSystem().systemTime()); + + child->setSampled(true); + child->finishSpan(); + parent.finishSpan(); + + // Verify the spans successfully complete without errors + EXPECT_TRUE(test_logger_->errors().empty()); } TEST_F(DatadogTracerSpanTest, SetSampledFalse) { @@ -352,42 +300,40 @@ TEST_F(DatadogTracerSpanTest, SetSampledFalse) { // that the local root of the chunk will have its // `datadog::tracing::tags::internal::sampling_priority` tag set to either -1 // (hard drop) or 2 (hard keep). - { - // First ensure that the trace will be kept (until we override it by calling - // `setSampled`, below). - span_.trace_segment().override_sampling_priority( - static_cast(datadog::tracing::SamplingPriority::USER_KEEP)); - - Span local_root{std::move(span_)}; - auto child = - local_root.spawnChild(Tracing::MockConfig{}, "child", time_.timeSystem().systemTime()); - child->setSampled(false); - child->finishSpan(); - local_root.finishSpan(); - } - EXPECT_EQ(1, collector_->chunks.size()); - const auto& spans = collector_->chunks[0]; - EXPECT_EQ(2, spans.size()); - const auto& local_root_ptr = spans[0]; - EXPECT_NE(nullptr, local_root_ptr); - const datadog::tracing::SpanData& local_root = *local_root_ptr; - const auto found = - local_root.numeric_tags.find(datadog::tracing::tags::internal::sampling_priority); - EXPECT_NE(local_root.numeric_tags.end(), found); - EXPECT_EQ(-1, found->second); + auto dd_span = tracer_.create_span(); + + // First ensure that the trace will be kept (until we override it by + // calling `setSampled`, below). + dd_span.trace_segment().override_sampling_priority( + static_cast(datadog::tracing::SamplingPriority::USER_KEEP)); + + Span parent(std::move(dd_span)); + auto child = parent.spawnChild(Tracing::MockConfig{}, "child", time_.timeSystem().systemTime()); + + child->setSampled(false); + child->finishSpan(); + parent.finishSpan(); + + // Verify the spans successfully complete without errors + EXPECT_TRUE(test_logger_->errors().empty()); } TEST_F(DatadogTracerSpanTest, Baggage) { // Baggage is not supported by dd-trace-cpp, so `Span::getBaggage` and // `Span::setBaggage` do nothing. - Span span{std::move(span_)}; + auto dd_span = tracer_.create_span(); + Span span(std::move(dd_span)); + EXPECT_EQ("", span.getBaggage("foo")); span.setBaggage("foo", "bar"); EXPECT_EQ("", span.getBaggage("foo")); } TEST_F(DatadogTracerSpanTest, GetTraceId) { - Span span{std::move(span_)}; + auto dd_span = tracer_.create_span(); + Span span(std::move(dd_span)); + + // We set the ID to 0xcafebabe in our test fixture EXPECT_EQ("cafebabe", span.getTraceId()); EXPECT_EQ("", span.getSpanId()); } @@ -399,7 +345,9 @@ TEST_F(DatadogTracerSpanTest, NoOpMode) { // I don't expect that Envoy will call methods on a finished span, and it's // hard to verify that the operations are no-ops, so this test just exercises // the code paths to verify that they don't trip any memory violations. - Span span{std::move(span_)}; + auto dd_span = tracer_.create_span(); + Span span(std::move(dd_span)); + span.finishSpan(); // `Span::finishSpan` is idempotent. From 5ddd1bc6a593cf50a92b0c252c69ecd5bbc73d42 Mon Sep 17 00:00:00 2001 From: Rohit Agrawal Date: Mon, 12 May 2025 08:36:35 -0700 Subject: [PATCH 2/3] addressed comments from @adisuissa Signed-off-by: Rohit Agrawal --- source/extensions/tracers/datadog/agent_http_client.cc | 5 +++-- source/extensions/tracers/datadog/agent_http_client.h | 4 ++-- test/extensions/tracers/datadog/agent_http_client_test.cc | 2 +- test/extensions/tracers/datadog/span_test.cc | 2 +- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/source/extensions/tracers/datadog/agent_http_client.cc b/source/extensions/tracers/datadog/agent_http_client.cc index 6737035c88ad..b1280da32e31 100644 --- a/source/extensions/tracers/datadog/agent_http_client.cc +++ b/source/extensions/tracers/datadog/agent_http_client.cc @@ -93,10 +93,11 @@ void AgentHTTPClient::drain(std::chrono::steady_clock::time_point) {} std::string AgentHTTPClient::config() const { return config_json().dump(); } -nlohmann::json AgentHTTPClient::config_json() const { - return nlohmann::json::object({ +const nlohmann::json& AgentHTTPClient::config_json() const { + static const nlohmann::json config = nlohmann::json::object({ {"type", "Envoy::Extensions::Tracers::Datadog::AgentHTTPClient"}, }); + return config; } // Http::AsyncClient::Callbacks diff --git a/source/extensions/tracers/datadog/agent_http_client.h b/source/extensions/tracers/datadog/agent_http_client.h index 056ec0a06625..298f921cb2ef 100644 --- a/source/extensions/tracers/datadog/agent_http_client.h +++ b/source/extensions/tracers/datadog/agent_http_client.h @@ -84,7 +84,7 @@ class AgentHTTPClient : public datadog::tracing::HTTPClient, void drain(std::chrono::steady_clock::time_point) override; /** - * Implementation of the required config() method from datadog::tracing::HTTPClient + * Implementation of the required config() method from datadog::tracing::HTTPClient. */ std::string config() const override; @@ -92,7 +92,7 @@ class AgentHTTPClient : public datadog::tracing::HTTPClient, * Return a JSON representation of this object's configuration. This function * is used in the startup banner logged by \c dd-trace-cpp. */ - nlohmann::json config_json() const; + const nlohmann::json& config_json() const; // Http::AsyncClient::Callbacks diff --git a/test/extensions/tracers/datadog/agent_http_client_test.cc b/test/extensions/tracers/datadog/agent_http_client_test.cc index 3c5daf5dcc9b..c0732c8083d4 100644 --- a/test/extensions/tracers/datadog/agent_http_client_test.cc +++ b/test/extensions/tracers/datadog/agent_http_client_test.cc @@ -683,7 +683,7 @@ TEST_F(DatadogAgentHttpClientTest, ConfigJson) { const std::string config = client_.config(); // Parse the config string to verify it's valid JSON EXPECT_NO_THROW({ - auto json = nlohmann::json::parse(config); + const auto json = nlohmann::json::parse(config); EXPECT_TRUE(json.is_object()); }); } diff --git a/test/extensions/tracers/datadog/span_test.cc b/test/extensions/tracers/datadog/span_test.cc index fdd5829398ed..03931c60ff65 100644 --- a/test/extensions/tracers/datadog/span_test.cc +++ b/test/extensions/tracers/datadog/span_test.cc @@ -64,7 +64,7 @@ class TestLogger : public datadog::tracing::Logger { } void log_error(datadog::tracing::StringView message) override { - errors_.push_back(std::string(message.data(), message.size())); + errors_.emplace_back(std::string(message.data(), message.size())); } const std::vector& errors() const { return errors_; } From eadf8d131d857fe8fee2d2c29ef570e8cbd4d1df Mon Sep 17 00:00:00 2001 From: Rohit Agrawal Date: Mon, 12 May 2025 09:07:44 -0700 Subject: [PATCH 3/3] addressed comments from @adisuissa Signed-off-by: Rohit Agrawal --- source/extensions/tracers/datadog/event_scheduler.cc | 5 +++-- source/extensions/tracers/datadog/event_scheduler.h | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/source/extensions/tracers/datadog/event_scheduler.cc b/source/extensions/tracers/datadog/event_scheduler.cc index 95edb5436844..3ac630b41bd5 100644 --- a/source/extensions/tracers/datadog/event_scheduler.cc +++ b/source/extensions/tracers/datadog/event_scheduler.cc @@ -66,10 +66,11 @@ EventScheduler::schedule_recurring_event(std::chrono::steady_clock::duration int std::string EventScheduler::config() const { return config_json().dump(); } -nlohmann::json EventScheduler::config_json() const { - return nlohmann::json::object({ +const nlohmann::json& EventScheduler::config_json() const { + static const nlohmann::json config = nlohmann::json::object({ {"type", "Envoy::Extensions::Tracers::Datadog::EventScheduler"}, }); + return config; } } // namespace Datadog diff --git a/source/extensions/tracers/datadog/event_scheduler.h b/source/extensions/tracers/datadog/event_scheduler.h index 77e4188f6644..396e766d8875 100644 --- a/source/extensions/tracers/datadog/event_scheduler.h +++ b/source/extensions/tracers/datadog/event_scheduler.h @@ -39,8 +39,8 @@ class EventScheduler : public datadog::tracing::EventScheduler { // Implementation of the required config() method from datadog::tracing::EventScheduler std::string config() const override; - // Provides JSON configuration for debug logging - nlohmann::json config_json() const; + // Provides JSON configuration for debug logging. + const nlohmann::json& config_json() const; private: Event::Dispatcher& dispatcher_;