From 2e32f68239f55c089aa74668ad5611366bfaea80 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 18 Jan 2023 13:16:31 -0500 Subject: [PATCH 01/17] datadog: time_util without tests Signed-off-by: David Goffredo --- bazel/external/BUILD | 1 + bazel/repositories.bzl | 8 ++++++ bazel/repository_locations.bzl | 15 +++++++++++ source/extensions/tracers/datadog/BUILD | 12 ++++++++- .../extensions/tracers/datadog/time_util.cc | 27 +++++++++++++++++++ source/extensions/tracers/datadog/time_util.h | 22 +++++++++++++++ 6 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 source/extensions/tracers/datadog/time_util.cc create mode 100644 source/extensions/tracers/datadog/time_util.h 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..7d30626a89820 100644 --- a/source/extensions/tracers/datadog/BUILD +++ b/source/extensions/tracers/datadog/BUILD @@ -15,11 +15,21 @@ envoy_cc_library( name = "datadog_tracer_lib", srcs = [ "datadog_tracer_impl.cc", + "time_util.cc", ], hdrs = [ "datadog_tracer_impl.h", + "time_util.h", + ], + 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", ], - external_deps = ["dd_opentracing_cpp"], deps = [ "//source/common/config:utility_lib", "//source/common/http:async_client_utility_lib", diff --git a/source/extensions/tracers/datadog/time_util.cc b/source/extensions/tracers/datadog/time_util.cc new file mode 100644 index 0000000000000..2b4ab5c109d59 --- /dev/null +++ b/source/extensions/tracers/datadog/time_util.cc @@ -0,0 +1,27 @@ +#include "source/extensions/tracers/datadog/time_util.h" + +#include + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace Datadog { + +datadog::tracing::TimePoint estimateTime(SystemTime wall) { + return estimateTime(wall, datadog::tracing::default_clock); +} + +datadog::tracing::TimePoint estimateTime(SystemTime wall, const datadog::tracing::Clock& clock) { + auto point = clock(); + auto elapsed = point.wall - wall; + // We could be off by a second or so if the system clock has been adjusted + // since `wall` was taken. It's fine. + point.tick -= elapsed; + point.wall = wall; + return point; +} + +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/datadog/time_util.h b/source/extensions/tracers/datadog/time_util.h new file mode 100644 index 0000000000000..82279d6d8f39d --- /dev/null +++ b/source/extensions/tracers/datadog/time_util.h @@ -0,0 +1,22 @@ +#pragma once + +#include + +#include "envoy/common/time.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace Datadog { + +// Return the `datadog::tracing::TimePoint` that most closely matches the +// specified `SystemTime`. Use the optionally specified +// `datadog::tracing::Clock` to read the current time. If a clock is not +// specified, then the default clock is used. +datadog::tracing::TimePoint estimateTime(SystemTime); +datadog::tracing::TimePoint estimateTime(SystemTime, const datadog::tracing::Clock&); + +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy From 4a02d3601862c38bda007cced08ec3c7e139d1de Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 18 Jan 2023 14:52:27 -0500 Subject: [PATCH 02/17] datadog: time_util tests Signed-off-by: David Goffredo --- .../extensions/tracers/datadog/time_util.cc | 6 +-- test/extensions/tracers/datadog/BUILD | 9 ++++ .../tracers/datadog/time_util_test.cc | 53 +++++++++++++++++++ 3 files changed, 65 insertions(+), 3 deletions(-) create mode 100644 test/extensions/tracers/datadog/time_util_test.cc diff --git a/source/extensions/tracers/datadog/time_util.cc b/source/extensions/tracers/datadog/time_util.cc index 2b4ab5c109d59..55e4b3ff3871a 100644 --- a/source/extensions/tracers/datadog/time_util.cc +++ b/source/extensions/tracers/datadog/time_util.cc @@ -14,9 +14,9 @@ datadog::tracing::TimePoint estimateTime(SystemTime wall) { datadog::tracing::TimePoint estimateTime(SystemTime wall, const datadog::tracing::Clock& clock) { auto point = clock(); auto elapsed = point.wall - wall; - // We could be off by a second or so if the system clock has been adjusted - // since `wall` was taken. It's fine. - point.tick -= elapsed; + if (elapsed > std::chrono::system_clock::duration::zero()) { + point.tick -= elapsed; + } point.wall = wall; return point; } diff --git a/test/extensions/tracers/datadog/BUILD b/test/extensions/tracers/datadog/BUILD index fd70bcc5b2e8b..7b22bafa420be 100644 --- a/test/extensions/tracers/datadog/BUILD +++ b/test/extensions/tracers/datadog/BUILD @@ -15,8 +15,17 @@ envoy_extension_cc_test( name = "datadog_tracer_impl_test", srcs = [ "datadog_tracer_impl_test.cc", + "time_util_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 = [ diff --git a/test/extensions/tracers/datadog/time_util_test.cc b/test/extensions/tracers/datadog/time_util_test.cc new file mode 100644 index 0000000000000..c6cb03b62388b --- /dev/null +++ b/test/extensions/tracers/datadog/time_util_test.cc @@ -0,0 +1,53 @@ +#include + +#include "envoy/common/time.h" + +#include "source/extensions/tracers/datadog/time_util.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace Datadog { +namespace { + +TEST(DatadogTracerTimeUtilTest, EstimateTime) { + // Concerns: + // + // - If the current system time is after the specified time (likely case), + // then the resulting steady time point is set back accordingly. + // - If the current system time is before the specified time (rare case), + // the the resulting steady time point is whatever the clock returns. + + // Modify `now` to change the value returned by `clock`. + datadog::tracing::TimePoint now; + datadog::tracing::Clock clock = [&]() { return now; }; + + // A little time has elapsed since the SystemTime was measured. The + // resulting steady time point should be set back by the difference. + datadog::tracing::TimePoint clock_result = datadog::tracing::default_clock(); + SystemTime argument = clock_result.wall; + clock_result.wall += std::chrono::microseconds(100); + now = clock_result; + datadog::tracing::TimePoint result = estimateTime(argument, clock); + EXPECT_EQ(result.wall, argument); + EXPECT_EQ(result.tick, clock_result.tick - std::chrono::microseconds(100)); + + // The clock has been set back since the SystemTime was measured. The + // resulting steady time can't do better than whatever the clock returns + // (we wouldn't want to set the steady time point into the future). + clock_result = datadog::tracing::default_clock(); + argument = clock_result.wall; + clock_result.wall -= std::chrono::milliseconds(100); + now = clock_result; + result = estimateTime(argument, clock); + EXPECT_EQ(result.wall, argument); + EXPECT_EQ(result.tick, clock_result.tick); +} + +} // namespace +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy From fd4400166f6bdae514d78e910fe5a2d723f008f5 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 18 Jan 2023 15:03:44 -0500 Subject: [PATCH 03/17] typo Signed-off-by: David Goffredo --- test/extensions/tracers/datadog/time_util_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/tracers/datadog/time_util_test.cc b/test/extensions/tracers/datadog/time_util_test.cc index c6cb03b62388b..75ea61f4c501f 100644 --- a/test/extensions/tracers/datadog/time_util_test.cc +++ b/test/extensions/tracers/datadog/time_util_test.cc @@ -18,7 +18,7 @@ TEST(DatadogTracerTimeUtilTest, EstimateTime) { // - If the current system time is after the specified time (likely case), // then the resulting steady time point is set back accordingly. // - If the current system time is before the specified time (rare case), - // the the resulting steady time point is whatever the clock returns. + // then the resulting steady time point is whatever the clock returns. // Modify `now` to change the value returned by `clock`. datadog::tracing::TimePoint now; From 7f487054c03dce8d2184221b79ec0a2afc4a9b21 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 18 Jan 2023 15:35:05 -0500 Subject: [PATCH 04/17] 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 dbb80b1136c92fbb57dfb3a6a424fdba16505580 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 19 Jan 2023 10:59:30 -0500 Subject: [PATCH 05/17] review comments: doxygen, auto, int diff comparison Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/time_util.cc | 7 +++---- source/extensions/tracers/datadog/time_util.h | 16 ++++++++++------ 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/source/extensions/tracers/datadog/time_util.cc b/source/extensions/tracers/datadog/time_util.cc index 55e4b3ff3871a..201452aa3abcf 100644 --- a/source/extensions/tracers/datadog/time_util.cc +++ b/source/extensions/tracers/datadog/time_util.cc @@ -12,10 +12,9 @@ datadog::tracing::TimePoint estimateTime(SystemTime wall) { } datadog::tracing::TimePoint estimateTime(SystemTime wall, const datadog::tracing::Clock& clock) { - auto point = clock(); - auto elapsed = point.wall - wall; - if (elapsed > std::chrono::system_clock::duration::zero()) { - point.tick -= elapsed; + datadog::tracing::TimePoint point = clock(); + if (point.wall > wall) { + point.tick -= point.wall - wall; } point.wall = wall; return point; diff --git a/source/extensions/tracers/datadog/time_util.h b/source/extensions/tracers/datadog/time_util.h index 82279d6d8f39d..c23585dda72c5 100644 --- a/source/extensions/tracers/datadog/time_util.h +++ b/source/extensions/tracers/datadog/time_util.h @@ -9,12 +9,16 @@ namespace Extensions { namespace Tracers { namespace Datadog { -// Return the `datadog::tracing::TimePoint` that most closely matches the -// specified `SystemTime`. Use the optionally specified -// `datadog::tracing::Clock` to read the current time. If a clock is not -// specified, then the default clock is used. -datadog::tracing::TimePoint estimateTime(SystemTime); -datadog::tracing::TimePoint estimateTime(SystemTime, const datadog::tracing::Clock&); +/** + * Convert a specified system \p time to a datadog time point, estimating the + steady portion of the result by examining the current time as measured by + the optionally specified \p clock and comparing it with the given \p time. + * @param time system time to convert from + * @param clock datadog clock used to measure the current time (default clock if omitted) + * @return datadog time point whose steady portion is estimated from the given \p time. + */ +datadog::tracing::TimePoint estimateTime(SystemTime time); +datadog::tracing::TimePoint estimateTime(SystemTime time, const datadog::tracing::Clock& clock); } // namespace Datadog } // namespace Tracers From c8e308bee9fbc057cb7101cce5b323a9cd88eac3 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Mon, 23 Jan 2023 11:50:59 -0500 Subject: [PATCH 06/17] datadog: background info for time_util Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/time_util.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/source/extensions/tracers/datadog/time_util.h b/source/extensions/tracers/datadog/time_util.h index c23585dda72c5..723c4ded82d2a 100644 --- a/source/extensions/tracers/datadog/time_util.h +++ b/source/extensions/tracers/datadog/time_util.h @@ -1,5 +1,24 @@ #pragma once +/** + * This file contains functions related to time points and durations. + * + * Envoy has a time type that contains both a system time point and a steady + * ("monotonic") time point. However, only the system time is exposed to the + * tracing subsystem. This may be remedied in the future, but for now we work + * with the system time. + * + * This is problematic for the Datadog core tracing library (dd-trace-cpp), + * because it uses the steady time to calculate the duration of a span + * (end.tick - begin.tick). So, we need to get a steady clock time from a given system + * clock time. The scheme is to measure the current system/steady time, + * compare the system part with the given system time, and then adjust the + * measured steady time accordingly. This is correct if the system clock has not + * been adjusted since the given system time was measured. It's incorrect + * otherwise, hence only an estimate. This conversion is performed by the + * estimateTime function. + */ + #include #include "envoy/common/time.h" From aff76d197029acdeae8bf7a8d92cd11746d47da5 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 18 Jan 2023 22:31:38 -0500 Subject: [PATCH 07/17] TODO: AMEND: Span and tests Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/BUILD | 2 + source/extensions/tracers/datadog/span.cc | 112 ++++++++++++++++ source/extensions/tracers/datadog/span.h | 50 ++++++++ test/extensions/tracers/datadog/BUILD | 1 + test/extensions/tracers/datadog/span_test.cc | 128 +++++++++++++++++++ 5 files changed, 293 insertions(+) create mode 100644 source/extensions/tracers/datadog/span.cc create mode 100644 source/extensions/tracers/datadog/span.h create mode 100644 test/extensions/tracers/datadog/span_test.cc diff --git a/source/extensions/tracers/datadog/BUILD b/source/extensions/tracers/datadog/BUILD index 7d30626a89820..a718e027a3438 100644 --- a/source/extensions/tracers/datadog/BUILD +++ b/source/extensions/tracers/datadog/BUILD @@ -15,10 +15,12 @@ envoy_cc_library( name = "datadog_tracer_lib", srcs = [ "datadog_tracer_impl.cc", + "span.cc", "time_util.cc", ], hdrs = [ "datadog_tracer_impl.h", + "span.h", "time_util.h", ], copts = [ diff --git a/source/extensions/tracers/datadog/span.cc b/source/extensions/tracers/datadog/span.cc new file mode 100644 index 0000000000000..837a3bfcadf52 --- /dev/null +++ b/source/extensions/tracers/datadog/span.cc @@ -0,0 +1,112 @@ +#include "source/extensions/tracers/datadog/span.h" + +#include +#include +#include +#include + +#include + +#include "source/common/tracing/null_span_impl.h" +#include "source/extensions/tracers/datadog/time_util.h" + +#include "absl/strings/str_cat.h" +#include "absl/strings/string_view.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace Datadog { +namespace { + +class TraceContextWriter : public datadog::tracing::DictWriter { + Tracing::TraceContext* context_; + +public: + explicit TraceContextWriter(Tracing::TraceContext& context) : context_(&context) {} + + void set(datadog::tracing::StringView key, datadog::tracing::StringView value) override { + context_->setByKey(key, value); + } +}; + +} // namespace + +Span::Span(datadog::tracing::Span&& span) : span_(std::move(span)) {} + +const datadog::tracing::Optional& Span::impl() const { return span_; } + +void Span::setOperation(absl::string_view operation) { + if (!span_) { + return; + } + + span_->set_name(operation); +} + +void Span::setTag(absl::string_view name, absl::string_view value) { + if (!span_) { + return; + } + + span_->set_tag(name, value); +} + +void Span::log(SystemTime, const std::string&) { + // Datadog spans don't have in-bound "events" or "logs". +} + +void Span::finishSpan() { span_.reset(); } + +void Span::injectContext(Tracing::TraceContext& trace_context, + const Upstream::HostDescriptionConstSharedPtr&) { + if (!span_) { + return; + } + + TraceContextWriter writer{trace_context}; + span_->inject(writer); +} + +Tracing::SpanPtr Span::spawnChild(const Tracing::Config&, const std::string& name, + SystemTime start_time) { + if (!span_) { + // I don't expect this to happen. This means that `spawnChild` was called + // after `finishSpan`. + return std::make_unique(); + } + + // The OpenTracing implementation ignored the `Tracing::Config` argument, + // so we will as well. + datadog::tracing::SpanConfig config; + config.name = name; + config.start = estimateTime(start_time); + + return std::make_unique(span_->create_child(config)); +} + +void Span::setSampled(bool sampled) { + if (!span_) { + return; + } + + auto priority = int(sampled ? datadog::tracing::SamplingPriority::USER_KEEP + : datadog::tracing::SamplingPriority::USER_DROP); + span_->trace_segment().override_sampling_priority(priority); +} + +std::string Span::getBaggage(absl::string_view) { + // not implemented + return std::string{}; +} + +void Span::setBaggage(absl::string_view, absl::string_view) { + // not implemented +} + +std::string Span::getTraceIdAsHex() const { return absl::StrCat(absl::Hex(span_->id())); } + +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/datadog/span.h b/source/extensions/tracers/datadog/span.h new file mode 100644 index 0000000000000..5bc89f72c6ba7 --- /dev/null +++ b/source/extensions/tracers/datadog/span.h @@ -0,0 +1,50 @@ +#pragma once + +#include + +#include + +#include "envoy/common/time.h" +#include "envoy/tracing/trace_driver.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace Datadog { + +class Span : public Tracing::Span { +public: + explicit Span(datadog::tracing::Span&& span); + + const datadog::tracing::Optional& impl() const; + + void setOperation(absl::string_view operation) override; + + void setTag(absl::string_view name, absl::string_view value) override; + + void log(SystemTime, const std::string&) override; + + void finishSpan() override; + + void injectContext(Tracing::TraceContext& trace_context, + const Upstream::HostDescriptionConstSharedPtr& upstream) override; + + Tracing::SpanPtr spawnChild(const Tracing::Config& config, const std::string& name, + SystemTime start_time) override; + + void setSampled(bool sampled) override; + + std::string getBaggage(absl::string_view key) override; + + void setBaggage(absl::string_view key, absl::string_view value) override; + + std::string getTraceIdAsHex() const override; + +private: + datadog::tracing::Optional span_; +}; + +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/tracers/datadog/BUILD b/test/extensions/tracers/datadog/BUILD index 7b22bafa420be..81b7280380958 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", + "span_test.cc", "time_util_test.cc", ], copts = [ diff --git a/test/extensions/tracers/datadog/span_test.cc b/test/extensions/tracers/datadog/span_test.cc new file mode 100644 index 0000000000000..fc5a80c9cd51b --- /dev/null +++ b/test/extensions/tracers/datadog/span_test.cc @@ -0,0 +1,128 @@ +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +#include "source/extensions/tracers/datadog/span.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace Datadog { +namespace { + +class NullLogger : public datadog::tracing::Logger { +public: + ~NullLogger() override = default; + + void log_error(const LogFunc&) override {} + void log_startup(const LogFunc&) override {} + + void log_error(const datadog::tracing::Error&) override {} + void log_error(datadog::tracing::StringView) override {} +}; + +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 {}; + } + + nlohmann::json config_json() const override { + return nlohmann::json::object({{"type", "Envoy::Extensions::Tracers::Datadog::MockCollector"}}); + } + + ~MockCollector() override = default; + + std::vector>> chunks; +}; + +struct TestSetup { + const std::uint64_t span_id; + const std::shared_ptr collector; + const datadog::tracing::TracerConfig config; + datadog::tracing::Tracer tracer; + datadog::tracing::Span span; + + explicit TestSetup(std::uint64_t span_id = 0xcafebabe) + : span_id(span_id), collector(std::make_shared()), + config(makeConfig(collector)), + tracer( + *datadog::tracing::finalize_config(config), [span_id]() { return span_id; }, + datadog::tracing::default_clock), + span(tracer.create_span()) {} + +private: + static datadog::tracing::TracerConfig + makeConfig(const std::shared_ptr& collector) { + datadog::tracing::TracerConfig config; + config.defaults.service = "testsvc"; + config.collector = collector; + config.logger = std::make_shared(); + return config; + } +}; + +TEST(DatadogTracerSpanTest, SetOperation) { + TestSetup test; + Span span{std::move(test.span)}; + span.setOperation("gastric bypass"); + span.finishSpan(); + + ASSERT_EQ(1, test.collector->chunks.size()); + const auto& chunk = test.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; + + EXPECT_EQ("gastric bypass", data.name); +} + +TEST(DatadogTracerSpanTest, SetTag) { + TestSetup test; + Span span{std::move(test.span)}; + span.setTag("foo", "bar"); + span.setTag("boom", "bam"); + span.setTag("foo", "new"); + span.finishSpan(); + + ASSERT_EQ(1, test.collector->chunks.size()); + const auto& chunk = test.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); +} + +TEST(DatadogTracerSpanTest, InjectContext) { + // TODO + ASSERT_TRUE(false); +} + +} // namespace +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy From 4a0f316b7832c282a2356b04b3fc4b3482ef8cd9 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Mon, 23 Jan 2023 15:25:40 -0500 Subject: [PATCH 08/17] datadog: private data members at the end Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/span.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/source/extensions/tracers/datadog/span.cc b/source/extensions/tracers/datadog/span.cc index 837a3bfcadf52..d6aa5b339552c 100644 --- a/source/extensions/tracers/datadog/span.cc +++ b/source/extensions/tracers/datadog/span.cc @@ -20,14 +20,15 @@ namespace Datadog { namespace { class TraceContextWriter : public datadog::tracing::DictWriter { - Tracing::TraceContext* context_; - public: explicit TraceContextWriter(Tracing::TraceContext& context) : context_(&context) {} void set(datadog::tracing::StringView key, datadog::tracing::StringView value) override { context_->setByKey(key, value); } + +private: + Tracing::TraceContext* context_; }; } // namespace From be5ca87f7211f24c5bc449da320653360c209295 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Mon, 23 Jan 2023 15:26:10 -0500 Subject: [PATCH 09/17] datadog: add comment describing Envoy::Extensions::Tracers::Datadog::Span Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/span.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/source/extensions/tracers/datadog/span.h b/source/extensions/tracers/datadog/span.h index 5bc89f72c6ba7..3adf2d4bb4d04 100644 --- a/source/extensions/tracers/datadog/span.h +++ b/source/extensions/tracers/datadog/span.h @@ -12,6 +12,23 @@ namespace Extensions { namespace Tracers { namespace Datadog { +/** + * Tracing::Span implementation for use in Datadog tracing. This class contains + * an optional and forwards its member functions to the + * corresponding member functions of datadog::tracing::Span. + * + * datadog::tracing::Span is the span type used in Datadog's core tracing + * library, dd-trace-cpp. It's wrapped in an optional<> because the lifetime + * of a datadog::tracing::Span is tied to the scope of the object itself, + * whereas the Tracing::Span implemented here has a finishSpan() member + * function that allows the span's lifetime to end without destroying the + * object. + * + * For the same reason, this class has two states: one when the + * optional is not empty and member functions are + * forwarded to it, and another state when the optional + * is empty and member functions have no effect. + */ class Span : public Tracing::Span { public: explicit Span(datadog::tracing::Span&& span); From 78a3a6fd706d1b9857c3edb5c0c883b48200c742 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Mon, 23 Jan 2023 16:59:25 -0500 Subject: [PATCH 10/17] datadog: unit tests for class Span Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/BUILD | 2 +- source/extensions/tracers/datadog/span.cc | 7 +- source/extensions/tracers/datadog/span.h | 32 ++-- test/extensions/tracers/datadog/span_test.cc | 174 ++++++++++++++++++- 4 files changed, 190 insertions(+), 25 deletions(-) diff --git a/source/extensions/tracers/datadog/BUILD b/source/extensions/tracers/datadog/BUILD index a718e027a3438..3054f66359d70 100644 --- a/source/extensions/tracers/datadog/BUILD +++ b/source/extensions/tracers/datadog/BUILD @@ -29,8 +29,8 @@ envoy_cc_library( "-DDD_USE_ABSEIL_FOR_ENVOY", ], external_deps = [ - "dd_opentracing_cpp", "dd_trace_cpp", + "dd_opentracing_cpp", ], deps = [ "//source/common/config:utility_lib", diff --git a/source/extensions/tracers/datadog/span.cc b/source/extensions/tracers/datadog/span.cc index d6aa5b339552c..0c03eafc5241d 100644 --- a/source/extensions/tracers/datadog/span.cc +++ b/source/extensions/tracers/datadog/span.cc @@ -105,7 +105,12 @@ void Span::setBaggage(absl::string_view, absl::string_view) { // not implemented } -std::string Span::getTraceIdAsHex() const { return absl::StrCat(absl::Hex(span_->id())); } +std::string Span::getTraceIdAsHex() const { + if (!span_) { + return std::string{}; + } + return absl::StrCat(absl::Hex(span_->id())); +} } // namespace Datadog } // namespace Tracers diff --git a/source/extensions/tracers/datadog/span.h b/source/extensions/tracers/datadog/span.h index 3adf2d4bb4d04..e1450205913ab 100644 --- a/source/extensions/tracers/datadog/span.h +++ b/source/extensions/tracers/datadog/span.h @@ -13,22 +13,22 @@ namespace Tracers { namespace Datadog { /** - * Tracing::Span implementation for use in Datadog tracing. This class contains - * an optional and forwards its member functions to the - * corresponding member functions of datadog::tracing::Span. - * - * datadog::tracing::Span is the span type used in Datadog's core tracing - * library, dd-trace-cpp. It's wrapped in an optional<> because the lifetime - * of a datadog::tracing::Span is tied to the scope of the object itself, - * whereas the Tracing::Span implemented here has a finishSpan() member - * function that allows the span's lifetime to end without destroying the - * object. - * - * For the same reason, this class has two states: one when the - * optional is not empty and member functions are - * forwarded to it, and another state when the optional - * is empty and member functions have no effect. - */ + * Tracing::Span implementation for use in Datadog tracing. This class contains + * an optional and forwards its member functions to the + * corresponding member functions of datadog::tracing::Span. + * + * datadog::tracing::Span is the span type used in Datadog's core tracing + * library, dd-trace-cpp. It's wrapped in an optional<> because the lifetime + * of a datadog::tracing::Span is tied to the scope of the object itself, + * whereas the Tracing::Span implemented here has a finishSpan() member + * function that allows the span's lifetime to end without destroying the + * object. + * + * For the same reason, this class has two states: one when the + * optional is not empty and member functions are + * forwarded to it, and another state when the optional + * is empty and member functions have no effect. + */ class Span : public Tracing::Span { public: explicit Span(datadog::tracing::Span&& span); diff --git a/test/extensions/tracers/datadog/span_test.cc b/test/extensions/tracers/datadog/span_test.cc index fc5a80c9cd51b..bafb7767a6a21 100644 --- a/test/extensions/tracers/datadog/span_test.cc +++ b/test/extensions/tracers/datadog/span_test.cc @@ -3,19 +3,38 @@ #include #include #include +#include #include +#include #include #include #include +#include #include +#include #include #include +#include "source/common/tracing/null_span_impl.h" #include "source/extensions/tracers/datadog/span.h" +#include "source/extensions/tracers/datadog/time_util.h" + +#include "test/mocks/tracing/mocks.h" +#include "test/test_common/utility.h" #include "gtest/gtest.h" +namespace datadog { +namespace tracing { + +bool operator==(const TimePoint& left, const TimePoint& right) { + return left.wall == right.wall && left.tick == right.tick; +} + +} // namespace tracing +} // namespace datadog + namespace Envoy { namespace Extensions { namespace Tracers { @@ -51,17 +70,18 @@ struct MockCollector : public datadog::tracing::Collector { }; struct TestSetup { - const std::uint64_t span_id; + const std::uint64_t id; const std::shared_ptr collector; const datadog::tracing::TracerConfig config; datadog::tracing::Tracer tracer; datadog::tracing::Span span; - explicit TestSetup(std::uint64_t span_id = 0xcafebabe) - : span_id(span_id), collector(std::make_shared()), - config(makeConfig(collector)), + explicit TestSetup(std::uint64_t id = 0xcafebabe) + : id(id), collector(std::make_shared()), config(makeConfig(collector)), tracer( - *datadog::tracing::finalize_config(config), [span_id]() { return span_id; }, + // Override the tracer's ID generator so that all trace IDs and span + // IDs are `id`. + *datadog::tracing::finalize_config(config), [id]() { return id; }, datadog::tracing::default_clock), span(tracer.create_span()) {} @@ -72,6 +92,10 @@ struct TestSetup { config.defaults.service = "testsvc"; config.collector = collector; config.logger = std::make_shared(); + // Drop all spans. Equivalently, we could keep all spans. + datadog::tracing::TraceSamplerConfig::Rule rule; + rule.sample_rate = 0; + config.trace_sampler.rules.push_back(std::move(rule)); return config; } }; @@ -117,8 +141,144 @@ TEST(DatadogTracerSpanTest, SetTag) { } TEST(DatadogTracerSpanTest, InjectContext) { - // TODO - ASSERT_TRUE(false); + TestSetup test; + Span span{std::move(test.span)}; + + Tracing::TestTraceContextImpl context{}; + span.injectContext(context, nullptr); + // Span::injectContext doesn't modify any of named fields. + EXPECT_EQ("", context.context_protocol_); + EXPECT_EQ("", context.context_authority_); + EXPECT_EQ("", context.context_path_); + EXPECT_EQ("", context.context_method_); + + // Span::injectContext inserts propagation headers that depend on the + // propagation style configured (i.e. the DD_TRACE_PROPAGATION_STYLE_INJECT + // environment variable). The default style includes Datadog propagation + // 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(test.id), found->second); + found = context.context_map_.find("x-datadog-parent-id"); + ASSERT_NE(context.context_map_.end(), found); + EXPECT_EQ(std::to_string(test.id), 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. + EXPECT_EQ(std::to_string(int(datadog::tracing::SamplingPriority::USER_DROP)), found->second); +} + +TEST(DatadogTracerSpanTest, SpawnChild) { + TestSetup test; + const auto child_start = std::chrono::system_clock::now(); + { + Span parent{std::move(test.span)}; + auto child = parent.spawnChild(Tracing::MockConfig{}, "child", child_start); + child->finishSpan(); + parent.finishSpan(); + } + + EXPECT_EQ(1, test.collector->chunks.size()); + const auto& spans = test.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); + EXPECT_EQ("child", child.name); + EXPECT_EQ(test.id, child.trace_id); + EXPECT_EQ(test.id, child.span_id); + EXPECT_EQ(test.id, child.parent_id); +} + +TEST(DatadogTracerSpanTest, SetSampled) { + // `Span::setSampled(bool)` on any span causes the entire group (chunk) of + // spans to take that sampling override. In terms of dd-trace-cpp, this means + // 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). + for (bool sampled : {true, false}) { + TestSetup test; + { + Span local_root{std::move(test.span)}; + auto child = + local_root.spawnChild(Tracing::MockConfig{}, "child", std::chrono::system_clock::now()); + child->setSampled(sampled); + child->finishSpan(); + local_root.finishSpan(); + } + EXPECT_EQ(1, test.collector->chunks.size()); + const auto& spans = test.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); + if (sampled) { + EXPECT_EQ(2, found->second); + } else { + EXPECT_EQ(-1, found->second); + } + } +} + +TEST(DatadogTracerSpanTest, Baggage) { + // Baggage is not supported by dd-trace-cpp, so `Span::getBaggage` and + // `Span::setBaggage` do nothing. + TestSetup test; + Span span{std::move(test.span)}; + EXPECT_EQ("", span.getBaggage("foo")); + span.setBaggage("foo", "bar"); + EXPECT_EQ("", span.getBaggage("foo")); +} + +TEST(DatadogTracerSpanTest, GetTraceIdAsHex) { + TestSetup test{0xcafebabe}; + Span span{std::move(test.span)}; + EXPECT_EQ("cafebabe", span.getTraceIdAsHex()); +} + +TEST(DatadogTracerSpanTest, NoOpMode) { + // `Span::finishSpan` destroys its `datadog::tracing::Span` member. + // Subsequently, methods called on the `Span` do nothing. + // + // 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. + TestSetup test; + Span span{std::move(test.span)}; + span.finishSpan(); + + // `Span::finishSpan` is idempotent. + span.finishSpan(); + + // Inner `datadog::tracing::Span` really is destroyed. + const datadog::tracing::Optional& impl = span.impl(); + EXPECT_EQ(datadog::tracing::nullopt, impl); + + // Other methods. + span.setOperation("foo"); + span.setTag("foo", "bar"); + // `Span::log` doesn't do anything in any case. + span.log(std::chrono::system_clock::now(), "ignored"); + Tracing::TestTraceContextImpl context{}; + span.injectContext(context, nullptr); + EXPECT_EQ("", context.context_protocol_); + EXPECT_EQ("", context.context_authority_); + EXPECT_EQ("", context.context_path_); + EXPECT_EQ("", context.context_method_); + EXPECT_EQ(0, context.context_map_.size()); + const Tracing::SpanPtr child = + span.spawnChild(Tracing::MockConfig{}, "child", std::chrono::system_clock::now()); + EXPECT_NE(nullptr, child); + EXPECT_EQ(typeid(Tracing::NullSpan), typeid(*child)); + span.setSampled(true); + span.setSampled(false); + EXPECT_EQ("", span.getBaggage("foo")); + span.setBaggage("foo", "bar"); + EXPECT_EQ("", span.getTraceIdAsHex()); } } // namespace From 69440568223a0a26824aa4193a759212ca138afb Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 26 Jan 2023 12:28:48 -0500 Subject: [PATCH 11/17] don't use system clock in unit test Signed-off-by: David Goffredo --- test/extensions/tracers/datadog/span_test.cc | 26 +++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/test/extensions/tracers/datadog/span_test.cc b/test/extensions/tracers/datadog/span_test.cc index bafb7767a6a21..b5e148b346325 100644 --- a/test/extensions/tracers/datadog/span_test.cc +++ b/test/extensions/tracers/datadog/span_test.cc @@ -100,6 +100,22 @@ struct TestSetup { } }; +std::chrono::system_clock::time_point now() { + // We are frozen in time. + std::tm datetime = {}; + // May 6th, 2010 + datetime.tm_year = 2010 - 1900; + datetime.tm_mon = 5 - 1; + datetime.tm_mday = 6; + // 14:45 + datetime.tm_hour = 14; + datetime.tm_min = 45; + + const std::time_t time = std::mktime(&datetime); + EXPECT_NE(std::time_t(-1), time); + return std::chrono::system_clock::from_time_t(time); +} + TEST(DatadogTracerSpanTest, SetOperation) { TestSetup test; Span span{std::move(test.span)}; @@ -170,7 +186,7 @@ TEST(DatadogTracerSpanTest, InjectContext) { TEST(DatadogTracerSpanTest, SpawnChild) { TestSetup test; - const auto child_start = std::chrono::system_clock::now(); + const auto child_start = now(); { Span parent{std::move(test.span)}; auto child = parent.spawnChild(Tracing::MockConfig{}, "child", child_start); @@ -201,8 +217,7 @@ TEST(DatadogTracerSpanTest, SetSampled) { TestSetup test; { Span local_root{std::move(test.span)}; - auto child = - local_root.spawnChild(Tracing::MockConfig{}, "child", std::chrono::system_clock::now()); + auto child = local_root.spawnChild(Tracing::MockConfig{}, "child", now()); child->setSampled(sampled); child->finishSpan(); local_root.finishSpan(); @@ -262,7 +277,7 @@ TEST(DatadogTracerSpanTest, NoOpMode) { span.setOperation("foo"); span.setTag("foo", "bar"); // `Span::log` doesn't do anything in any case. - span.log(std::chrono::system_clock::now(), "ignored"); + span.log(now(), "ignored"); Tracing::TestTraceContextImpl context{}; span.injectContext(context, nullptr); EXPECT_EQ("", context.context_protocol_); @@ -270,8 +285,7 @@ TEST(DatadogTracerSpanTest, NoOpMode) { EXPECT_EQ("", context.context_path_); EXPECT_EQ("", context.context_method_); EXPECT_EQ(0, context.context_map_.size()); - const Tracing::SpanPtr child = - span.spawnChild(Tracing::MockConfig{}, "child", std::chrono::system_clock::now()); + const Tracing::SpanPtr child = span.spawnChild(Tracing::MockConfig{}, "child", now()); EXPECT_NE(nullptr, child); EXPECT_EQ(typeid(Tracing::NullSpan), typeid(*child)); span.setSampled(true); From 984d3a794a005e40320e97b99aa977705f457552 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 26 Jan 2023 16:17:39 -0500 Subject: [PATCH 12/17] use Abseil datetime facilities Signed-off-by: David Goffredo --- test/extensions/tracers/datadog/span_test.cc | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/test/extensions/tracers/datadog/span_test.cc b/test/extensions/tracers/datadog/span_test.cc index b5e148b346325..7040705ac6f63 100644 --- a/test/extensions/tracers/datadog/span_test.cc +++ b/test/extensions/tracers/datadog/span_test.cc @@ -16,6 +16,8 @@ #include #include +#include "absl/time/time.h" + #include "source/common/tracing/null_span_impl.h" #include "source/extensions/tracers/datadog/span.h" #include "source/extensions/tracers/datadog/time_util.h" @@ -101,19 +103,9 @@ struct TestSetup { }; std::chrono::system_clock::time_point now() { - // We are frozen in time. - std::tm datetime = {}; - // May 6th, 2010 - datetime.tm_year = 2010 - 1900; - datetime.tm_mon = 5 - 1; - datetime.tm_mday = 6; - // 14:45 - datetime.tm_hour = 14; - datetime.tm_min = 45; - - const std::time_t time = std::mktime(&datetime); - EXPECT_NE(std::time_t(-1), time); - return std::chrono::system_clock::from_time_t(time); + // 14:45 May 6th, 2010 in New York + absl::CivilSecond datetime{2010, 5, 3, 14, 45}; + return absl::ToChronoTime(absl::FromCivil(datetime, absl::FixedTimeZone(-4 * 60 * 60))); } TEST(DatadogTracerSpanTest, SetOperation) { From 09f0519fd995d66b46bb27fdbb38114785ffe267 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Fri, 27 Jan 2023 12:16:36 -0500 Subject: [PATCH 13/17] use SimulatedTimeSystem instead of hard-coded datetimes Signed-off-by: David Goffredo --- test/extensions/tracers/datadog/span_test.cc | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/test/extensions/tracers/datadog/span_test.cc b/test/extensions/tracers/datadog/span_test.cc index 7040705ac6f63..9cc6c2dd387bd 100644 --- a/test/extensions/tracers/datadog/span_test.cc +++ b/test/extensions/tracers/datadog/span_test.cc @@ -16,13 +16,12 @@ #include #include -#include "absl/time/time.h" - #include "source/common/tracing/null_span_impl.h" #include "source/extensions/tracers/datadog/span.h" #include "source/extensions/tracers/datadog/time_util.h" #include "test/mocks/tracing/mocks.h" +#include "test/test_common/simulated_time_system.h" #include "test/test_common/utility.h" #include "gtest/gtest.h" @@ -77,6 +76,7 @@ struct TestSetup { const datadog::tracing::TracerConfig config; datadog::tracing::Tracer tracer; datadog::tracing::Span span; + Event::SimulatedTimeSystem time; explicit TestSetup(std::uint64_t id = 0xcafebabe) : id(id), collector(std::make_shared()), config(makeConfig(collector)), @@ -102,12 +102,6 @@ struct TestSetup { } }; -std::chrono::system_clock::time_point now() { - // 14:45 May 6th, 2010 in New York - absl::CivilSecond datetime{2010, 5, 3, 14, 45}; - return absl::ToChronoTime(absl::FromCivil(datetime, absl::FixedTimeZone(-4 * 60 * 60))); -} - TEST(DatadogTracerSpanTest, SetOperation) { TestSetup test; Span span{std::move(test.span)}; @@ -178,7 +172,7 @@ TEST(DatadogTracerSpanTest, InjectContext) { TEST(DatadogTracerSpanTest, SpawnChild) { TestSetup test; - const auto child_start = now(); + const auto child_start = test.time.timeSystem().systemTime(); { Span parent{std::move(test.span)}; auto child = parent.spawnChild(Tracing::MockConfig{}, "child", child_start); @@ -209,7 +203,8 @@ TEST(DatadogTracerSpanTest, SetSampled) { TestSetup test; { Span local_root{std::move(test.span)}; - auto child = local_root.spawnChild(Tracing::MockConfig{}, "child", now()); + auto child = local_root.spawnChild(Tracing::MockConfig{}, "child", + test.time.timeSystem().systemTime()); child->setSampled(sampled); child->finishSpan(); local_root.finishSpan(); @@ -269,7 +264,7 @@ TEST(DatadogTracerSpanTest, NoOpMode) { span.setOperation("foo"); span.setTag("foo", "bar"); // `Span::log` doesn't do anything in any case. - span.log(now(), "ignored"); + span.log(test.time.timeSystem().systemTime(), "ignored"); Tracing::TestTraceContextImpl context{}; span.injectContext(context, nullptr); EXPECT_EQ("", context.context_protocol_); @@ -277,7 +272,8 @@ TEST(DatadogTracerSpanTest, NoOpMode) { EXPECT_EQ("", context.context_path_); EXPECT_EQ("", context.context_method_); EXPECT_EQ(0, context.context_map_.size()); - const Tracing::SpanPtr child = span.spawnChild(Tracing::MockConfig{}, "child", now()); + const Tracing::SpanPtr child = + span.spawnChild(Tracing::MockConfig{}, "child", test.time.timeSystem().systemTime()); EXPECT_NE(nullptr, child); EXPECT_EQ(typeid(Tracing::NullSpan), typeid(*child)); span.setSampled(true); From b58bc6d36c8bb37ae3b7454ff827a45eb77ce96e Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Mon, 30 Jan 2023 14:49:45 -0500 Subject: [PATCH 14/17] datadog: address some code review comments Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/span.cc | 6 +- source/extensions/tracers/datadog/span.h | 10 +- source/extensions/tracers/datadog/time_util.h | 8 +- test/extensions/tracers/datadog/span_test.cc | 163 ++++++++++-------- 4 files changed, 97 insertions(+), 90 deletions(-) diff --git a/source/extensions/tracers/datadog/span.cc b/source/extensions/tracers/datadog/span.cc index 0c03eafc5241d..546526340df96 100644 --- a/source/extensions/tracers/datadog/span.cc +++ b/source/extensions/tracers/datadog/span.cc @@ -21,14 +21,14 @@ namespace { class TraceContextWriter : public datadog::tracing::DictWriter { public: - explicit TraceContextWriter(Tracing::TraceContext& context) : context_(&context) {} + explicit TraceContextWriter(Tracing::TraceContext& context) : context_(context) {} void set(datadog::tracing::StringView key, datadog::tracing::StringView value) override { - context_->setByKey(key, value); + context_.setByKey(key, value); } private: - Tracing::TraceContext* context_; + Tracing::TraceContext& context_; }; } // namespace diff --git a/source/extensions/tracers/datadog/span.h b/source/extensions/tracers/datadog/span.h index e1450205913ab..81a1a2695b2bc 100644 --- a/source/extensions/tracers/datadog/span.h +++ b/source/extensions/tracers/datadog/span.h @@ -35,26 +35,18 @@ class Span : public Tracing::Span { const datadog::tracing::Optional& impl() const; + // Envoy::Tracing::Span void setOperation(absl::string_view operation) override; - void setTag(absl::string_view name, absl::string_view value) override; - void log(SystemTime, const std::string&) override; - void finishSpan() override; - void injectContext(Tracing::TraceContext& trace_context, const Upstream::HostDescriptionConstSharedPtr& upstream) override; - Tracing::SpanPtr spawnChild(const Tracing::Config& config, const std::string& name, SystemTime start_time) override; - void setSampled(bool sampled) override; - std::string getBaggage(absl::string_view key) override; - void setBaggage(absl::string_view key, absl::string_view value) override; - std::string getTraceIdAsHex() const override; private: diff --git a/source/extensions/tracers/datadog/time_util.h b/source/extensions/tracers/datadog/time_util.h index 723c4ded82d2a..cb93cc44206e3 100644 --- a/source/extensions/tracers/datadog/time_util.h +++ b/source/extensions/tracers/datadog/time_util.h @@ -3,10 +3,10 @@ /** * This file contains functions related to time points and durations. * - * Envoy has a time type that contains both a system time point and a steady - * ("monotonic") time point. However, only the system time is exposed to the - * tracing subsystem. This may be remedied in the future, but for now we work - * with the system time. + * Envoy has a TimeSource abstraction that provides both a system time point and + * a steady ("monotonic") time point. However, only the system time is exposed + * to the tracing subsystem. This may be remedied in the future, but for now we + * work with the system time. * * This is problematic for the Datadog core tracing library (dd-trace-cpp), * because it uses the steady time to calculate the duration of a span diff --git a/test/extensions/tracers/datadog/span_test.cc b/test/extensions/tracers/datadog/span_test.cc index 9cc6c2dd387bd..9ab85c54425b4 100644 --- a/test/extensions/tracers/datadog/span_test.cc +++ b/test/extensions/tracers/datadog/span_test.cc @@ -70,22 +70,17 @@ struct MockCollector : public datadog::tracing::Collector { std::vector>> chunks; }; -struct TestSetup { - const std::uint64_t id; - const std::shared_ptr collector; - const datadog::tracing::TracerConfig config; - datadog::tracing::Tracer tracer; - datadog::tracing::Span span; - Event::SimulatedTimeSystem time; - - explicit TestSetup(std::uint64_t id = 0xcafebabe) - : id(id), collector(std::make_shared()), config(makeConfig(collector)), - tracer( +class DatadogTracerSpanTest : public testing::Test { +public: + DatadogTracerSpanTest() + : id_(0xcafebabe), collector_(std::make_shared()), + config_(makeConfig(collector_)), + tracer_( // Override the tracer's ID generator so that all trace IDs and span - // IDs are `id`. - *datadog::tracing::finalize_config(config), [id]() { return id; }, + // IDs are 0xcafebabe. + *datadog::tracing::finalize_config(config_), [this]() { return id_; }, datadog::tracing::default_clock), - span(tracer.create_span()) {} + span_(tracer_.create_span()) {} private: static datadog::tracing::TracerConfig @@ -100,16 +95,23 @@ struct TestSetup { config.trace_sampler.rules.push_back(std::move(rule)); return config; } + +protected: + const std::uint64_t id_; + const std::shared_ptr collector_; + const datadog::tracing::TracerConfig config_; + datadog::tracing::Tracer tracer_; + datadog::tracing::Span span_; + Event::SimulatedTimeSystem time_; }; -TEST(DatadogTracerSpanTest, SetOperation) { - TestSetup test; - Span span{std::move(test.span)}; +TEST_F(DatadogTracerSpanTest, SetOperation) { + Span span{std::move(span_)}; span.setOperation("gastric bypass"); span.finishSpan(); - ASSERT_EQ(1, test.collector->chunks.size()); - const auto& chunk = test.collector->chunks[0]; + 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); @@ -118,16 +120,15 @@ TEST(DatadogTracerSpanTest, SetOperation) { EXPECT_EQ("gastric bypass", data.name); } -TEST(DatadogTracerSpanTest, SetTag) { - TestSetup test; - Span span{std::move(test.span)}; +TEST_F(DatadogTracerSpanTest, SetTag) { + Span span{std::move(span_)}; span.setTag("foo", "bar"); span.setTag("boom", "bam"); span.setTag("foo", "new"); span.finishSpan(); - ASSERT_EQ(1, test.collector->chunks.size()); - const auto& chunk = test.collector->chunks[0]; + 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); @@ -142,9 +143,8 @@ TEST(DatadogTracerSpanTest, SetTag) { EXPECT_EQ("bam", found->second); } -TEST(DatadogTracerSpanTest, InjectContext) { - TestSetup test; - Span span{std::move(test.span)}; +TEST_F(DatadogTracerSpanTest, InjectContext) { + Span span{std::move(span_)}; Tracing::TestTraceContextImpl context{}; span.injectContext(context, nullptr); @@ -160,97 +160,112 @@ TEST(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(test.id), found->second); + EXPECT_EQ(std::to_string(id_), found->second); found = context.context_map_.find("x-datadog-parent-id"); ASSERT_NE(context.context_map_.end(), found); - EXPECT_EQ(std::to_string(test.id), found->second); + EXPECT_EQ(std::to_string(id_), 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. EXPECT_EQ(std::to_string(int(datadog::tracing::SamplingPriority::USER_DROP)), found->second); } -TEST(DatadogTracerSpanTest, SpawnChild) { - TestSetup test; - const auto child_start = test.time.timeSystem().systemTime(); +TEST_F(DatadogTracerSpanTest, SpawnChild) { + const auto child_start = time_.timeSystem().systemTime(); { - Span parent{std::move(test.span)}; + Span parent{std::move(span_)}; auto child = parent.spawnChild(Tracing::MockConfig{}, "child", child_start); child->finishSpan(); parent.finishSpan(); } - EXPECT_EQ(1, test.collector->chunks.size()); - const auto& spans = test.collector->chunks[0]; + 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); EXPECT_EQ("child", child.name); - EXPECT_EQ(test.id, child.trace_id); - EXPECT_EQ(test.id, child.span_id); - EXPECT_EQ(test.id, child.parent_id); + EXPECT_EQ(id_, child.trace_id); + EXPECT_EQ(id_, child.span_id); + EXPECT_EQ(id_, child.parent_id); +} + +TEST_F(DatadogTracerSpanTest, SetSampledTrue) { + // `Span::setSampled(bool)` on any span causes the entire group (chunk) of + // spans to take that sampling override. In terms of dd-trace-cpp, this means + // 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). + { + 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); } -TEST(DatadogTracerSpanTest, SetSampled) { +TEST_F(DatadogTracerSpanTest, SetSampledFalse) { // `Span::setSampled(bool)` on any span causes the entire group (chunk) of // spans to take that sampling override. In terms of dd-trace-cpp, this means // 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). - for (bool sampled : {true, false}) { - TestSetup test; - { - Span local_root{std::move(test.span)}; - auto child = local_root.spawnChild(Tracing::MockConfig{}, "child", - test.time.timeSystem().systemTime()); - child->setSampled(sampled); - child->finishSpan(); - local_root.finishSpan(); - } - EXPECT_EQ(1, test.collector->chunks.size()); - const auto& spans = test.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); - if (sampled) { - EXPECT_EQ(2, found->second); - } else { - EXPECT_EQ(-1, found->second); - } + { + 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); } -TEST(DatadogTracerSpanTest, Baggage) { +TEST_F(DatadogTracerSpanTest, Baggage) { // Baggage is not supported by dd-trace-cpp, so `Span::getBaggage` and // `Span::setBaggage` do nothing. - TestSetup test; - Span span{std::move(test.span)}; + Span span{std::move(span_)}; EXPECT_EQ("", span.getBaggage("foo")); span.setBaggage("foo", "bar"); EXPECT_EQ("", span.getBaggage("foo")); } -TEST(DatadogTracerSpanTest, GetTraceIdAsHex) { - TestSetup test{0xcafebabe}; - Span span{std::move(test.span)}; +TEST_F(DatadogTracerSpanTest, GetTraceIdAsHex) { + Span span{std::move(span_)}; EXPECT_EQ("cafebabe", span.getTraceIdAsHex()); } -TEST(DatadogTracerSpanTest, NoOpMode) { +TEST_F(DatadogTracerSpanTest, NoOpMode) { // `Span::finishSpan` destroys its `datadog::tracing::Span` member. // Subsequently, methods called on the `Span` do nothing. // // 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. - TestSetup test; - Span span{std::move(test.span)}; + Span span{std::move(span_)}; span.finishSpan(); // `Span::finishSpan` is idempotent. @@ -264,7 +279,7 @@ TEST(DatadogTracerSpanTest, NoOpMode) { span.setOperation("foo"); span.setTag("foo", "bar"); // `Span::log` doesn't do anything in any case. - span.log(test.time.timeSystem().systemTime(), "ignored"); + span.log(time_.timeSystem().systemTime(), "ignored"); Tracing::TestTraceContextImpl context{}; span.injectContext(context, nullptr); EXPECT_EQ("", context.context_protocol_); @@ -273,7 +288,7 @@ TEST(DatadogTracerSpanTest, NoOpMode) { EXPECT_EQ("", context.context_method_); EXPECT_EQ(0, context.context_map_.size()); const Tracing::SpanPtr child = - span.spawnChild(Tracing::MockConfig{}, "child", test.time.timeSystem().systemTime()); + span.spawnChild(Tracing::MockConfig{}, "child", time_.timeSystem().systemTime()); EXPECT_NE(nullptr, child); EXPECT_EQ(typeid(Tracing::NullSpan), typeid(*child)); span.setSampled(true); From a3f290a37ab272df03e5def1647850925264cce0 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Fri, 3 Feb 2023 15:21:35 -0500 Subject: [PATCH 15/17] authority -> host, and rearrange external deps Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/BUILD | 2 +- test/extensions/tracers/datadog/span_test.cc | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/extensions/tracers/datadog/BUILD b/source/extensions/tracers/datadog/BUILD index a4a636325d445..cee4cd93feddb 100644 --- a/source/extensions/tracers/datadog/BUILD +++ b/source/extensions/tracers/datadog/BUILD @@ -32,8 +32,8 @@ envoy_cc_library( "-DDD_USE_ABSEIL_FOR_ENVOY", ], external_deps = [ - "dd_opentracing_cpp", "dd_trace_cpp", + "dd_opentracing_cpp", ], deps = [ "//source/common/config:utility_lib", diff --git a/test/extensions/tracers/datadog/span_test.cc b/test/extensions/tracers/datadog/span_test.cc index 9ab85c54425b4..7bce06a4fa28a 100644 --- a/test/extensions/tracers/datadog/span_test.cc +++ b/test/extensions/tracers/datadog/span_test.cc @@ -150,7 +150,7 @@ TEST_F(DatadogTracerSpanTest, InjectContext) { span.injectContext(context, nullptr); // Span::injectContext doesn't modify any of named fields. EXPECT_EQ("", context.context_protocol_); - EXPECT_EQ("", context.context_authority_); + EXPECT_EQ("", context.context_host_); EXPECT_EQ("", context.context_path_); EXPECT_EQ("", context.context_method_); @@ -283,7 +283,7 @@ TEST_F(DatadogTracerSpanTest, NoOpMode) { Tracing::TestTraceContextImpl context{}; span.injectContext(context, nullptr); EXPECT_EQ("", context.context_protocol_); - EXPECT_EQ("", context.context_authority_); + EXPECT_EQ("", context.context_host_); EXPECT_EQ("", context.context_path_); EXPECT_EQ("", context.context_method_); EXPECT_EQ(0, context.context_map_.size()); From 0ae865ac7c517856cfeee16cd77c3f48c4ea9df8 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Fri, 3 Feb 2023 15:35:31 -0500 Subject: [PATCH 16/17] review comments: #include "..." and static_cast Signed-off-by: David Goffredo --- .../tracers/datadog/datadog_tracer_impl.h | 3 +-- source/extensions/tracers/datadog/dict_util.h | 6 +++--- source/extensions/tracers/datadog/span.cc | 13 ++++++------ source/extensions/tracers/datadog/span.h | 4 ++-- source/extensions/tracers/datadog/time_util.h | 4 ++-- .../tracers/datadog/dict_util_test.cc | 3 +-- test/extensions/tracers/datadog/span_test.cc | 21 +++++++++---------- .../tracers/datadog/time_util_test.cc | 3 +-- 8 files changed, 26 insertions(+), 31 deletions(-) diff --git a/source/extensions/tracers/datadog/datadog_tracer_impl.h b/source/extensions/tracers/datadog/datadog_tracer_impl.h index d48253b9a4e96..bcdb6ba442c64 100644 --- a/source/extensions/tracers/datadog/datadog_tracer_impl.h +++ b/source/extensions/tracers/datadog/datadog_tracer_impl.h @@ -1,7 +1,5 @@ #pragma once -#include - #include "envoy/config/trace/v3/datadog.pb.h" #include "envoy/local_info/local_info.h" #include "envoy/runtime/runtime.h" @@ -14,6 +12,7 @@ #include "source/common/upstream/cluster_update_tracker.h" #include "source/extensions/tracers/common/ot/opentracing_driver_impl.h" +#include "datadog/opentracing.h" #include "fmt/ostream.h" namespace Envoy { diff --git a/source/extensions/tracers/datadog/dict_util.h b/source/extensions/tracers/datadog/dict_util.h index a844a5cce1adb..518e4ab164928 100644 --- a/source/extensions/tracers/datadog/dict_util.h +++ b/source/extensions/tracers/datadog/dict_util.h @@ -10,11 +10,11 @@ * headers, or writing HTTP request headers. */ -#include -#include - #include +#include "datadog/dict_reader.h" +#include "datadog/dict_writer.h" + namespace Envoy { namespace Tracing { class TraceContext; diff --git a/source/extensions/tracers/datadog/span.cc b/source/extensions/tracers/datadog/span.cc index 546526340df96..45922a20e620f 100644 --- a/source/extensions/tracers/datadog/span.cc +++ b/source/extensions/tracers/datadog/span.cc @@ -1,10 +1,5 @@ #include "source/extensions/tracers/datadog/span.h" -#include -#include -#include -#include - #include #include "source/common/tracing/null_span_impl.h" @@ -12,6 +7,10 @@ #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" +#include "datadog/dict_writer.h" +#include "datadog/sampling_priority.h" +#include "datadog/span_config.h" +#include "datadog/trace_segment.h" namespace Envoy { namespace Extensions { @@ -91,8 +90,8 @@ void Span::setSampled(bool sampled) { return; } - auto priority = int(sampled ? datadog::tracing::SamplingPriority::USER_KEEP - : datadog::tracing::SamplingPriority::USER_DROP); + auto priority = static_cast(sampled ? datadog::tracing::SamplingPriority::USER_KEEP + : datadog::tracing::SamplingPriority::USER_DROP); span_->trace_segment().override_sampling_priority(priority); } diff --git a/source/extensions/tracers/datadog/span.h b/source/extensions/tracers/datadog/span.h index 81a1a2695b2bc..85f3ed47a4d56 100644 --- a/source/extensions/tracers/datadog/span.h +++ b/source/extensions/tracers/datadog/span.h @@ -1,12 +1,12 @@ #pragma once -#include - #include #include "envoy/common/time.h" #include "envoy/tracing/trace_driver.h" +#include "datadog/span.h" + namespace Envoy { namespace Extensions { namespace Tracers { diff --git a/source/extensions/tracers/datadog/time_util.h b/source/extensions/tracers/datadog/time_util.h index cb93cc44206e3..d37b9818915e2 100644 --- a/source/extensions/tracers/datadog/time_util.h +++ b/source/extensions/tracers/datadog/time_util.h @@ -19,10 +19,10 @@ * estimateTime function. */ -#include - #include "envoy/common/time.h" +#include "datadog/clock.h" + namespace Envoy { namespace Extensions { namespace Tracers { diff --git a/test/extensions/tracers/datadog/dict_util_test.cc b/test/extensions/tracers/datadog/dict_util_test.cc index d20e669c101b4..855744fde7aec 100644 --- a/test/extensions/tracers/datadog/dict_util_test.cc +++ b/test/extensions/tracers/datadog/dict_util_test.cc @@ -1,5 +1,3 @@ -#include - #include #include @@ -10,6 +8,7 @@ #include "test/mocks/http/mocks.h" #include "test/test_common/utility.h" +#include "datadog/optional.h" #include "gtest/gtest.h" namespace Envoy { diff --git a/test/extensions/tracers/datadog/span_test.cc b/test/extensions/tracers/datadog/span_test.cc index 7bce06a4fa28a..39a24926855a7 100644 --- a/test/extensions/tracers/datadog/span_test.cc +++ b/test/extensions/tracers/datadog/span_test.cc @@ -1,15 +1,4 @@ -#include -#include -#include -#include -#include -#include -#include -#include -#include - #include -#include #include #include #include @@ -24,6 +13,16 @@ #include "test/test_common/simulated_time_system.h" #include "test/test_common/utility.h" +#include "datadog/clock.h" +#include "datadog/collector.h" +#include "datadog/expected.h" +#include "datadog/id_generator.h" +#include "datadog/json.hpp" +#include "datadog/logger.h" +#include "datadog/sampling_priority.h" +#include "datadog/span_data.h" +#include "datadog/tags.h" +#include "datadog/tracer.h" #include "gtest/gtest.h" namespace datadog { diff --git a/test/extensions/tracers/datadog/time_util_test.cc b/test/extensions/tracers/datadog/time_util_test.cc index 472677f6dde95..21efa6ab8d1f6 100644 --- a/test/extensions/tracers/datadog/time_util_test.cc +++ b/test/extensions/tracers/datadog/time_util_test.cc @@ -1,9 +1,8 @@ -#include - #include "envoy/common/time.h" #include "source/extensions/tracers/datadog/time_util.h" +#include "datadog/clock.h" #include "gtest/gtest.h" namespace Envoy { From be83634bbebb24052453152d44bd315b48725548 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Fri, 3 Feb 2023 19:10:11 -0500 Subject: [PATCH 17/17] satisfy -Wpotentially-evaluated-expression Signed-off-by: David Goffredo --- test/extensions/tracers/datadog/span_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/extensions/tracers/datadog/span_test.cc b/test/extensions/tracers/datadog/span_test.cc index 39a24926855a7..15e754dfceb3b 100644 --- a/test/extensions/tracers/datadog/span_test.cc +++ b/test/extensions/tracers/datadog/span_test.cc @@ -289,7 +289,8 @@ TEST_F(DatadogTracerSpanTest, NoOpMode) { const Tracing::SpanPtr child = span.spawnChild(Tracing::MockConfig{}, "child", time_.timeSystem().systemTime()); EXPECT_NE(nullptr, child); - EXPECT_EQ(typeid(Tracing::NullSpan), typeid(*child)); + const Tracing::Span& child_span = *child; + EXPECT_EQ(typeid(Tracing::NullSpan), typeid(child_span)); span.setSampled(true); span.setSampled(false); EXPECT_EQ("", span.getBaggage("foo"));