From 12ca34d517cf9673129eaf85781f2eafff796da9 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 18 Jan 2023 13:16:31 -0500 Subject: [PATCH 01/10] dd-trace-cpp source/ dependency Signed-off-by: David Goffredo --- bazel/external/BUILD | 1 + bazel/repositories.bzl | 8 ++++++++ bazel/repository_locations.bzl | 15 +++++++++++++++ source/extensions/tracers/datadog/BUILD | 10 +++++++++- 4 files changed, 33 insertions(+), 1 deletion(-) diff --git a/bazel/external/BUILD b/bazel/external/BUILD index 62a6ca994d26a..4558a1cee2cd0 100644 --- a/bazel/external/BUILD +++ b/bazel/external/BUILD @@ -13,6 +13,7 @@ cc_library( tags = ["skip_on_windows"], deps = [ "@com_github_datadog_dd_opentracing_cpp//:dd_opentracing_cpp", + "@com_github_datadog_dd_trace_cpp//:dd_trace_cpp", "@com_google_googletest//:gtest", "@io_opentracing_cpp//:opentracing", ], diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 1100b5bd01f7e..5903b4df7c966 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -164,6 +164,7 @@ def envoy_dependencies(skip_targets = []): _com_github_circonus_labs_libcircllhist() _com_github_cyan4973_xxhash() _com_github_datadog_dd_opentracing_cpp() + _com_github_datadog_dd_trace_cpp() _com_github_mirror_tclap() _com_github_envoyproxy_sqlparser() _com_github_fmtlib_fmt() @@ -574,6 +575,13 @@ def _com_github_datadog_dd_opentracing_cpp(): actual = "@com_github_datadog_dd_opentracing_cpp//:dd_opentracing_cpp", ) +def _com_github_datadog_dd_trace_cpp(): + external_http_archive("com_github_datadog_dd_trace_cpp") + native.bind( + name = "dd_trace_cpp", + actual = "@com_github_datadog_dd_trace_cpp//:dd_trace_cpp", + ) + def _com_github_skyapm_cpp2sky(): external_http_archive( name = "com_github_skyapm_cpp2sky", diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index fce969321fe1f..fdfe6be0a0358 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -546,6 +546,21 @@ REPOSITORY_LOCATIONS_SPEC = dict( license = "Apache-2.0", license_url = "https://github.com/DataDog/dd-opentracing-cpp/blob/v{version}/LICENSE", ), + com_github_datadog_dd_trace_cpp = 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.5", + sha256 = "d76c98109822d6c3e8deb335117766b67c2be636169e60e5c813c9075b721aa1", + 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 = "2021-12-06", + cpe = "N/A", + license = "Apache-2.0", + license_url = "https://github.com/DataDog/dd-trace-cpp/blob/v{version}/LICENSE", + ), com_github_google_benchmark = dict( project_name = "Benchmark", project_desc = "Library to benchmark code snippets", diff --git a/source/extensions/tracers/datadog/BUILD b/source/extensions/tracers/datadog/BUILD index 0c314374f9683..6ec069a593614 100644 --- a/source/extensions/tracers/datadog/BUILD +++ b/source/extensions/tracers/datadog/BUILD @@ -19,7 +19,15 @@ envoy_cc_library( hdrs = [ "datadog_tracer_impl.h", ], - external_deps = ["dd_opentracing_cpp"], + copts = [ + # Make sure that headers included from dd_trace_cpp use Abseil + # equivalents of std::string_view and std::optional. + "-DDD_USE_ABSEIL_FOR_ENVOY", + ], + external_deps = [ + "dd_opentracing_cpp", + "dd_trace_cpp", + ], deps = [ "//source/common/config:utility_lib", "//source/common/http:async_client_utility_lib", From 5408e3c805ab6af688e6e9049a1e250e3e680521 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 18 Jan 2023 14:52:27 -0500 Subject: [PATCH 02/10] dd-trace-cpp test/ dependency Signed-off-by: David Goffredo --- test/extensions/tracers/datadog/BUILD | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/extensions/tracers/datadog/BUILD b/test/extensions/tracers/datadog/BUILD index fd70bcc5b2e8b..0dff983f70663 100644 --- a/test/extensions/tracers/datadog/BUILD +++ b/test/extensions/tracers/datadog/BUILD @@ -16,7 +16,15 @@ envoy_extension_cc_test( srcs = [ "datadog_tracer_impl_test.cc", ], + copts = [ + # Make sure that headers included from dd_trace_cpp use Abseil + # equivalents of std::string_view and std::optional. + "-DDD_USE_ABSEIL_FOR_ENVOY", + ], extension_names = ["envoy.tracers.datadog"], + external_deps = [ + "dd_trace_cpp", + ], # TODO(wrowe): envoy_extension_ rules don't currently exclude windows extensions tags = ["skip_on_windows"], deps = [ From d0332e45d136831b05f0144311a9426f5f03dc49 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 18 Jan 2023 15:35:05 -0500 Subject: [PATCH 03/10] wrong year 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 fdfe6be0a0358..8881dfabb0149 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -556,7 +556,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 = "2021-12-06", + release_date = "2022-12-06", cpe = "N/A", license = "Apache-2.0", license_url = "https://github.com/DataDog/dd-trace-cpp/blob/v{version}/LICENSE", From ef421641a29e319e68d786c48171980a8d86b8e4 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 25 Jan 2023 15:04:15 -0500 Subject: [PATCH 04/10] datadog: add class EventScheduler, untested Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/BUILD | 2 + .../tracers/datadog/event_scheduler.cc | 62 +++++++++++++++++++ .../tracers/datadog/event_scheduler.h | 52 ++++++++++++++++ 3 files changed, 116 insertions(+) create mode 100644 source/extensions/tracers/datadog/event_scheduler.cc create mode 100644 source/extensions/tracers/datadog/event_scheduler.h diff --git a/source/extensions/tracers/datadog/BUILD b/source/extensions/tracers/datadog/BUILD index 6ec069a593614..7cdee32d5a024 100644 --- a/source/extensions/tracers/datadog/BUILD +++ b/source/extensions/tracers/datadog/BUILD @@ -15,9 +15,11 @@ envoy_cc_library( name = "datadog_tracer_lib", srcs = [ "datadog_tracer_impl.cc", + "event_scheduler.cc", ], hdrs = [ "datadog_tracer_impl.h", + "event_scheduler.h", ], copts = [ # Make sure that headers included from dd_trace_cpp use Abseil diff --git a/source/extensions/tracers/datadog/event_scheduler.cc b/source/extensions/tracers/datadog/event_scheduler.cc new file mode 100644 index 0000000000000..56953c4d97a09 --- /dev/null +++ b/source/extensions/tracers/datadog/event_scheduler.cc @@ -0,0 +1,62 @@ +#include "source/extensions/tracers/datadog/event_scheduler.h" + +#include +#include +#include +#include + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace Datadog { + +EventScheduler::EventScheduler(Event::Dispatcher& dispatcher) : dispatcher_(dispatcher) {} + +EventScheduler::~EventScheduler() { + for (const auto& timer : timers_) { + timer->disableTimer(); + } +} + +EventScheduler::Cancel +EventScheduler::schedule_recurring_event(std::chrono::steady_clock::duration interval, + std::function callback) { + // Yes, a shared pointer to a pointer. + auto self = std::make_shared(); + + Event::TimerPtr timer = dispatcher_.createTimer([self, interval, callback = std::move(callback)] { + (**self).enableTimer(std::chrono::duration_cast(interval)); + callback(); + }); + + Event::Timer* timer_raw = timer.get(); + *self = timer_raw; + + timers_.emplace_back(std::move(timer)); + + timer_raw->enableTimer(std::chrono::duration_cast(interval)); + + return [this, timer = timer_raw]() mutable { + if (!timer) { + return; // idempotent + } + + timer->disableTimer(); + auto found = std::find_if(timers_.begin(), timers_.end(), + [&](const auto& element) { return element.get() == timer; }); + assert(found != timers_.end()); + timers_.erase(found); + timer = nullptr; + }; +} + +nlohmann::json EventScheduler::config_json() const { + return nlohmann::json::object({ + {"type", "Envoy::Extensions::Tracers::Datadog::EventScheduler"}, + }); +} + +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/datadog/event_scheduler.h b/source/extensions/tracers/datadog/event_scheduler.h new file mode 100644 index 0000000000000..7c118b52f928f --- /dev/null +++ b/source/extensions/tracers/datadog/event_scheduler.h @@ -0,0 +1,52 @@ +#pragma once + +#include + +#include +#include +#include + +#include "envoy/event/dispatcher.h" +#include "envoy/event/timer.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace Datadog { + +/** + * Registry of recurring events (timers). This class implements dd-trace-cpp's + * \c datadog::tracing::EventScheduler interface in terms of an + * \c Event::Dispatcher, which is what is used by Envoy. An instance of this class + * is passed into dd-trace-cpp when tracing is configured, allowing dd-trace-cpp + * to periodically send batches of traces to the Datadog Agent. + */ +class EventScheduler : public datadog::tracing::EventScheduler { +public: + explicit EventScheduler(Event::Dispatcher& dispatcher); + ~EventScheduler() override; + + /** + * Repeatedly execute the specified \p callback with approximately \p interval + * between each invocation, starting after an initial \p interval. Return a + * function that cancels future invocations. If the returned function is + * invoked after this \c EventScheduler is destroyed, the behavior is + * undefined. + * @param interval how often the event will occur + * @param callback the function invoked when the event occurs + * @return a zero-parameter function that cancels the recurring event + */ + Cancel schedule_recurring_event(std::chrono::steady_clock::duration interval, + std::function callback) override; + + nlohmann::json config_json() const override; + +private: + Event::Dispatcher& dispatcher_; + std::vector timers_; +}; + +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy From 246bf0e72fcdff2932ccd19362710841b831376e Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 25 Jan 2023 17:20:50 -0500 Subject: [PATCH 05/10] datadog: test EventScheduler Signed-off-by: David Goffredo --- .../tracers/datadog/event_scheduler.h | 2 - test/extensions/tracers/datadog/BUILD | 1 + .../tracers/datadog/event_scheduler_test.cc | 103 ++++++++++++++++++ 3 files changed, 104 insertions(+), 2 deletions(-) create mode 100644 test/extensions/tracers/datadog/event_scheduler_test.cc diff --git a/source/extensions/tracers/datadog/event_scheduler.h b/source/extensions/tracers/datadog/event_scheduler.h index 7c118b52f928f..3f9481f117b07 100644 --- a/source/extensions/tracers/datadog/event_scheduler.h +++ b/source/extensions/tracers/datadog/event_scheduler.h @@ -2,8 +2,6 @@ #include -#include -#include #include #include "envoy/event/dispatcher.h" diff --git a/test/extensions/tracers/datadog/BUILD b/test/extensions/tracers/datadog/BUILD index 0dff983f70663..a3d89cf5fb218 100644 --- a/test/extensions/tracers/datadog/BUILD +++ b/test/extensions/tracers/datadog/BUILD @@ -15,6 +15,7 @@ envoy_extension_cc_test( name = "datadog_tracer_impl_test", srcs = [ "datadog_tracer_impl_test.cc", + "event_scheduler_test.cc", ], copts = [ # Make sure that headers included from dd_trace_cpp use Abseil diff --git a/test/extensions/tracers/datadog/event_scheduler_test.cc b/test/extensions/tracers/datadog/event_scheduler_test.cc new file mode 100644 index 0000000000000..67efa49f97924 --- /dev/null +++ b/test/extensions/tracers/datadog/event_scheduler_test.cc @@ -0,0 +1,103 @@ +#include "source/extensions/tracers/datadog/event_scheduler.h" + +#include "test/mocks/event/mocks.h" +#include "test/mocks/thread_local/mocks.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +#include + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace Datadog { +namespace { + +TEST(DatadogEventSchedulerTest, ScheduleRecurringEventCallsCreatesATimer) { + testing::NiceMock thread_local_storage_; + + EventScheduler scheduler{thread_local_storage_.dispatcher_}; + 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. + // The only requirement is that it be divisible by milliseconds, because + // that's what `Timer::enableTimer` accepts. + const auto interval = std::chrono::milliseconds(2000); + + EXPECT_CALL(thread_local_storage_.dispatcher_, createTimer_(testing::_)); + + (void)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_}; + testing::MockFunction callback; + const auto interval = std::chrono::milliseconds(2000); + + EXPECT_CALL(*timer_, enableTimer(interval, testing::_)); + + (void)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_}; + testing::MockFunction callback; + const auto interval = std::chrono::milliseconds(2000); + + // Once for the initial round, and then again when the callback is invoked. + EXPECT_CALL(*timer_, enableTimer(interval, testing::_)).Times(2); + // The user-supplied callback is called once when the timer triggers. + EXPECT_CALL(callback, Call()); + + (void)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_}; + testing::MockFunction callback; + const auto interval = std::chrono::milliseconds(2000); + + EXPECT_CALL(*timer_, disableTimer()); + + const auto cancel = scheduler.schedule_recurring_event(interval, callback.AsStdFunction()); + cancel(); + cancel(); // idempotent + cancel(); // idempotent + cancel(); // idempotent + cancel(); // idempotent + cancel(); // idempotent +} + +TEST(DatadogEventSchedulerTest, DestructorCallsDisableTimerIfCancelDidNot) { + testing::NiceMock thread_local_storage_; + auto* const timer_ = new testing::NiceMock(&thread_local_storage_.dispatcher_); + + EventScheduler scheduler{thread_local_storage_.dispatcher_}; + testing::MockFunction callback; + const auto interval = std::chrono::milliseconds(2000); + + // when `scheduler` is destroyed + EXPECT_CALL(*timer_, disableTimer()); + + (void)scheduler.schedule_recurring_event(interval, callback.AsStdFunction()); +} + +} // namespace +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy From 55118c88cb0b478b974339e5b9d3972867413a85 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 26 Jan 2023 16:22:58 -0500 Subject: [PATCH 06/10] ci/run_envoy_docker.sh './ci/do_ci.sh format' Signed-off-by: David Goffredo --- test/extensions/tracers/datadog/event_scheduler_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/extensions/tracers/datadog/event_scheduler_test.cc b/test/extensions/tracers/datadog/event_scheduler_test.cc index 67efa49f97924..0abb25ff71280 100644 --- a/test/extensions/tracers/datadog/event_scheduler_test.cc +++ b/test/extensions/tracers/datadog/event_scheduler_test.cc @@ -1,3 +1,5 @@ +#include + #include "source/extensions/tracers/datadog/event_scheduler.h" #include "test/mocks/event/mocks.h" @@ -6,8 +8,6 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" -#include - namespace Envoy { namespace Extensions { namespace Tracers { From e3c8625b71d4cc41ee172a2f9a6b30ef1bd038f0 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Tue, 14 Feb 2023 14:57:57 -0500 Subject: [PATCH 07/10] datadog: more docs for EventScheduler::schedule_recurring_event Signed-off-by: David Goffredo --- .../tracers/datadog/event_scheduler.cc | 29 ++++++++++++++++--- .../tracers/datadog/event_scheduler.h | 4 +-- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/source/extensions/tracers/datadog/event_scheduler.cc b/source/extensions/tracers/datadog/event_scheduler.cc index 56953c4d97a09..10be1a5ce45d4 100644 --- a/source/extensions/tracers/datadog/event_scheduler.cc +++ b/source/extensions/tracers/datadog/event_scheduler.cc @@ -1,10 +1,12 @@ #include "source/extensions/tracers/datadog/event_scheduler.h" -#include -#include #include #include +#include "source/common/common/assert.h" + +#include "datadog/json.hpp" + namespace Envoy { namespace Extensions { namespace Tracers { @@ -21,8 +23,26 @@ EventScheduler::~EventScheduler() { EventScheduler::Cancel EventScheduler::schedule_recurring_event(std::chrono::steady_clock::duration interval, std::function callback) { - // Yes, a shared pointer to a pointer. auto self = std::make_shared(); + // Yes, a shared pointer to a pointer. + // + // Both the timer callback (argument to `createTimer`, below) and the returned + // cancellation function need a handle to the `Event::Timer`. The timer + // callback needs it so that it can reschedule the next round when the timer + // fires (that's how this is a _recurring_ event). The cancellation function + // needs it so that it can call `disableTimer` and remove the timer from + // `timers_`. + // + // Since we don't have a handle to the `Event::Timer` until `createTimer` + // returns, we need a "box" out of which the timer callback can extract the + // created timer. We then put the `Event::Timer*` in the "box" after + // `createTimer` returns the timer. `self` is the "box." + // + // The cancellation function returned by this function refers to the + // `Event::Timer` via a raw pointer (as does the timer callback, indirectly). + // The actual lifetime of the pointed-to `Event::Timer` is determined by its + // presence in `timers_`. The `Event::TimerPtr` returned by `createTimer` is + // moved into `timers_`. Event::TimerPtr timer = dispatcher_.createTimer([self, interval, callback = std::move(callback)] { (**self).enableTimer(std::chrono::duration_cast(interval)); @@ -44,7 +64,8 @@ EventScheduler::schedule_recurring_event(std::chrono::steady_clock::duration int timer->disableTimer(); auto found = std::find_if(timers_.begin(), timers_.end(), [&](const auto& element) { return element.get() == timer; }); - assert(found != timers_.end()); + RELEASE_ASSERT(found != timers_.end(), + "timer not found in registry of timers in Datadog::EventScheduler"); timers_.erase(found); timer = nullptr; }; diff --git a/source/extensions/tracers/datadog/event_scheduler.h b/source/extensions/tracers/datadog/event_scheduler.h index 3f9481f117b07..e0ce5ebc4a8bb 100644 --- a/source/extensions/tracers/datadog/event_scheduler.h +++ b/source/extensions/tracers/datadog/event_scheduler.h @@ -1,12 +1,12 @@ #pragma once -#include - #include #include "envoy/event/dispatcher.h" #include "envoy/event/timer.h" +#include "datadog/event_scheduler.h" + namespace Envoy { namespace Extensions { namespace Tracers { From d6f92d86f27121d14fca1018e55c7c4b7ac6c5c1 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 15 Feb 2023 13:02:05 -0500 Subject: [PATCH 08/10] datadog: set level in logger and sink in unit test Signed-off-by: David Goffredo --- .../extensions/tracers/datadog/logger_test.cc | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/test/extensions/tracers/datadog/logger_test.cc b/test/extensions/tracers/datadog/logger_test.cc index e9d101ca69d39..31de8b3f5f832 100644 --- a/test/extensions/tracers/datadog/logger_test.cc +++ b/test/extensions/tracers/datadog/logger_test.cc @@ -50,6 +50,11 @@ TEST(DatadogTracerLoggerTest, Logger) { spdlog::logger spdlogger{"test", sink}; Logger logger{spdlogger}; + const auto reset = [&]() { + sink->reset(); + spdlogger.set_level(spdlog::level::info); + }; + // callback-style error logger.log_error([](std::ostream& log) { log << "Beware the ides of March."; }); EXPECT_EQ("Beware the ides of March.", sink->payload_); @@ -57,7 +62,7 @@ TEST(DatadogTracerLoggerTest, Logger) { EXPECT_EQ(nullptr, sink->formatter_); EXPECT_FALSE(sink->flush_); - sink->reset(); + reset(); // callback-style startup banner logger.log_startup([](std::ostream& log) { log << "It's my stapler, the Swingline. It's been mine for a very long time."; @@ -67,7 +72,7 @@ TEST(DatadogTracerLoggerTest, Logger) { EXPECT_EQ(nullptr, sink->formatter_); EXPECT_FALSE(sink->flush_); - sink->reset(); + reset(); // Error-style error logger.log_error(datadog::tracing::Error{datadog::tracing::Error::OTHER, "I'm sorry, Dave, I'm afraid I can't do that."}); @@ -76,7 +81,7 @@ TEST(DatadogTracerLoggerTest, Logger) { EXPECT_EQ(nullptr, sink->formatter_); EXPECT_FALSE(sink->flush_); - sink->reset(); + reset(); // string-style error logger.log_error("I must make my witness. I must lead the people from the waters. I must stay " "their stampede to the sea."); @@ -93,18 +98,18 @@ TEST(DatadogTracerLoggerTest, Logger) { // so if we set the level to `critical`, our `error` messages will not be // logged. - sink->reset(); + reset(); // callback-style error - sink->set_level(spdlog::level::critical); + spdlogger.set_level(spdlog::level::critical); logger.log_error([](std::ostream& log) { log << "Beware the ides of March."; }); EXPECT_EQ(absl::nullopt, sink->payload_); EXPECT_EQ(absl::nullopt, sink->pattern_); EXPECT_EQ(nullptr, sink->formatter_); EXPECT_FALSE(sink->flush_); - sink->reset(); + reset(); // Error-style error - sink->set_level(spdlog::level::critical); + spdlogger.set_level(spdlog::level::critical); logger.log_error(datadog::tracing::Error{datadog::tracing::Error::OTHER, "I'm sorry, Dave, I'm afraid I can't do that."}); EXPECT_EQ(absl::nullopt, sink->payload_); @@ -112,9 +117,9 @@ TEST(DatadogTracerLoggerTest, Logger) { EXPECT_EQ(nullptr, sink->formatter_); EXPECT_FALSE(sink->flush_); - sink->reset(); + reset(); // string-style error - sink->set_level(spdlog::level::critical); + spdlogger.set_level(spdlog::level::critical); logger.log_error("I must make my witness. I must lead the people from the waters. I must stay " "their stampede to the sea."); EXPECT_EQ(absl::nullopt, sink->payload_); @@ -125,7 +130,7 @@ TEST(DatadogTracerLoggerTest, Logger) { // The startup banner is printed at "info" level, and so if the logger's level // threshold is more severe than that, the startup banner will not be logged. - sink->reset(); + reset(); // The default level is `info`, so the startup banner will be logged. logger.log_startup([](std::ostream& log) { log << "ג וַיֹּאמֶר אֱלֹהִים, יְהִי אוֹר; וַיְהִי-אוֹר."; }); EXPECT_EQ("ג וַיֹּאמֶר אֱלֹהִים, יְהִי אוֹר; וַיְהִי-אוֹר.", sink->payload_); @@ -133,10 +138,10 @@ TEST(DatadogTracerLoggerTest, Logger) { EXPECT_EQ(nullptr, sink->formatter_); EXPECT_FALSE(sink->flush_); - sink->reset(); + reset(); // Any level more severe than `info` (`warn`, `error`, `critical`) will // suppress the startup banner. `warn` suffices. - sink->set_level(spdlog::level::warn); + spdlogger.set_level(spdlog::level::warn); logger.log_startup([](std::ostream& log) { log << "R - ½ R g + Λ g = κ T"; }); EXPECT_EQ(absl::nullopt, sink->payload_); EXPECT_EQ(absl::nullopt, sink->pattern_); From 93e4020ccf35c2d0aac821b424934dd944c875f2 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 15 Feb 2023 13:21:25 -0500 Subject: [PATCH 09/10] datadog: add test to cover EventScheduler::config_json() Signed-off-by: David Goffredo --- test/extensions/tracers/datadog/event_scheduler_test.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/extensions/tracers/datadog/event_scheduler_test.cc b/test/extensions/tracers/datadog/event_scheduler_test.cc index 0abb25ff71280..754250d9ec167 100644 --- a/test/extensions/tracers/datadog/event_scheduler_test.cc +++ b/test/extensions/tracers/datadog/event_scheduler_test.cc @@ -5,6 +5,7 @@ #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" @@ -96,6 +97,13 @@ TEST(DatadogEventSchedulerTest, DestructorCallsDisableTimerIfCancelDidNot) { (void)scheduler.schedule_recurring_event(interval, callback.AsStdFunction()); } +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 From e28d5e79b02abeb2425a3d8f77b4fab5d16e6883 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Mon, 6 Mar 2023 13:58:09 -0500 Subject: [PATCH 10/10] datadog: address further review comments Signed-off-by: David Goffredo --- .../tracers/datadog/event_scheduler.cc | 11 +---- .../tracers/datadog/event_scheduler.h | 6 +-- .../tracers/datadog/event_scheduler_test.cc | 42 +++++++------------ 3 files changed, 18 insertions(+), 41 deletions(-) diff --git a/source/extensions/tracers/datadog/event_scheduler.cc b/source/extensions/tracers/datadog/event_scheduler.cc index 10be1a5ce45d4..823d93bfee572 100644 --- a/source/extensions/tracers/datadog/event_scheduler.cc +++ b/source/extensions/tracers/datadog/event_scheduler.cc @@ -14,12 +14,6 @@ namespace Datadog { EventScheduler::EventScheduler(Event::Dispatcher& dispatcher) : dispatcher_(dispatcher) {} -EventScheduler::~EventScheduler() { - for (const auto& timer : timers_) { - timer->disableTimer(); - } -} - EventScheduler::Cancel EventScheduler::schedule_recurring_event(std::chrono::steady_clock::duration interval, std::function callback) { @@ -52,7 +46,7 @@ EventScheduler::schedule_recurring_event(std::chrono::steady_clock::duration int Event::Timer* timer_raw = timer.get(); *self = timer_raw; - timers_.emplace_back(std::move(timer)); + timers_.insert(std::move(timer)); timer_raw->enableTimer(std::chrono::duration_cast(interval)); @@ -62,8 +56,7 @@ EventScheduler::schedule_recurring_event(std::chrono::steady_clock::duration int } timer->disableTimer(); - auto found = std::find_if(timers_.begin(), timers_.end(), - [&](const auto& element) { return element.get() == timer; }); + auto found = timers_.find(timer); RELEASE_ASSERT(found != timers_.end(), "timer not found in registry of timers in Datadog::EventScheduler"); timers_.erase(found); diff --git a/source/extensions/tracers/datadog/event_scheduler.h b/source/extensions/tracers/datadog/event_scheduler.h index e0ce5ebc4a8bb..9c75c8be6ead7 100644 --- a/source/extensions/tracers/datadog/event_scheduler.h +++ b/source/extensions/tracers/datadog/event_scheduler.h @@ -1,10 +1,9 @@ #pragma once -#include - #include "envoy/event/dispatcher.h" #include "envoy/event/timer.h" +#include "absl/container/flat_hash_set.h" #include "datadog/event_scheduler.h" namespace Envoy { @@ -22,7 +21,6 @@ namespace Datadog { class EventScheduler : public datadog::tracing::EventScheduler { public: explicit EventScheduler(Event::Dispatcher& dispatcher); - ~EventScheduler() override; /** * Repeatedly execute the specified \p callback with approximately \p interval @@ -41,7 +39,7 @@ class EventScheduler : public datadog::tracing::EventScheduler { private: Event::Dispatcher& dispatcher_; - std::vector timers_; + absl::flat_hash_set timers_; }; } // namespace Datadog diff --git a/test/extensions/tracers/datadog/event_scheduler_test.cc b/test/extensions/tracers/datadog/event_scheduler_test.cc index 754250d9ec167..d0449a06e144e 100644 --- a/test/extensions/tracers/datadog/event_scheduler_test.cc +++ b/test/extensions/tracers/datadog/event_scheduler_test.cc @@ -24,11 +24,11 @@ TEST(DatadogEventSchedulerTest, ScheduleRecurringEventCallsCreatesATimer) { // compare it to what was passed to the mocks. // The only requirement is that it be divisible by milliseconds, because // that's what `Timer::enableTimer` accepts. - const auto interval = std::chrono::milliseconds(2000); + const std::chrono::milliseconds interval(2000); EXPECT_CALL(thread_local_storage_.dispatcher_, createTimer_(testing::_)); - (void)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 @@ -36,43 +36,43 @@ TEST(DatadogEventSchedulerTest, ScheduleRecurringEventCallsCreatesATimer) { // separate tests. TEST(DatadogEventSchedulerTest, ScheduleRecurringEventEnablesATimer) { testing::NiceMock thread_local_storage_; - auto* const timer_ = new testing::NiceMock(&thread_local_storage_.dispatcher_); + auto* const timer = new testing::NiceMock(&thread_local_storage_.dispatcher_); EventScheduler scheduler{thread_local_storage_.dispatcher_}; testing::MockFunction callback; - const auto interval = std::chrono::milliseconds(2000); + const std::chrono::milliseconds interval(2000); - EXPECT_CALL(*timer_, enableTimer(interval, testing::_)); + EXPECT_CALL(*timer, enableTimer(interval, testing::_)); - (void)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_); + auto* const timer = new testing::NiceMock(&thread_local_storage_.dispatcher_); EventScheduler scheduler{thread_local_storage_.dispatcher_}; testing::MockFunction callback; - const auto interval = std::chrono::milliseconds(2000); + 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, testing::_)).Times(2); // The user-supplied callback is called once when the timer triggers. EXPECT_CALL(callback, Call()); - (void)scheduler.schedule_recurring_event(interval, callback.AsStdFunction()); - timer_->invokeCallback(); + 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_); + auto* const timer = new testing::NiceMock(&thread_local_storage_.dispatcher_); EventScheduler scheduler{thread_local_storage_.dispatcher_}; testing::MockFunction callback; - const auto interval = std::chrono::milliseconds(2000); + const std::chrono::milliseconds interval(2000); - EXPECT_CALL(*timer_, disableTimer()); + EXPECT_CALL(*timer, disableTimer()); const auto cancel = scheduler.schedule_recurring_event(interval, callback.AsStdFunction()); cancel(); @@ -83,20 +83,6 @@ TEST(DatadogEventSchedulerTest, CancellationFunctionCallsDisableTimerOnce) { cancel(); // idempotent } -TEST(DatadogEventSchedulerTest, DestructorCallsDisableTimerIfCancelDidNot) { - testing::NiceMock thread_local_storage_; - auto* const timer_ = new testing::NiceMock(&thread_local_storage_.dispatcher_); - - EventScheduler scheduler{thread_local_storage_.dispatcher_}; - testing::MockFunction callback; - const auto interval = std::chrono::milliseconds(2000); - - // when `scheduler` is destroyed - EXPECT_CALL(*timer_, disableTimer()); - - (void)scheduler.schedule_recurring_event(interval, callback.AsStdFunction()); -} - TEST(DatadogEventSchedulerTest, ConfigJson) { testing::NiceMock thread_local_storage_; EventScheduler scheduler{thread_local_storage_.dispatcher_};