From 2e32f68239f55c089aa74668ad5611366bfaea80 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 18 Jan 2023 13:16:31 -0500 Subject: [PATCH 1/7] 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 2/7] 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 3/7] 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 4/7] 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 5/7] 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 6/7] 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 d253bfb30e50b6a9f0eae29854415469f6859a27 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 25 Jan 2023 14:32:35 -0500 Subject: [PATCH 7/7] datadog: test the one-parameter overload of estimateTime Signed-off-by: David Goffredo --- .../tracers/datadog/time_util_test.cc | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/extensions/tracers/datadog/time_util_test.cc b/test/extensions/tracers/datadog/time_util_test.cc index 75ea61f4c501f..472677f6dde95 100644 --- a/test/extensions/tracers/datadog/time_util_test.cc +++ b/test/extensions/tracers/datadog/time_util_test.cc @@ -46,6 +46,25 @@ TEST(DatadogTracerTimeUtilTest, EstimateTime) { EXPECT_EQ(result.tick, clock_result.tick); } +TEST(DatadogTracerTimeUtilTest, DefaultClock) { + // The one-parameter overload of `estimateTime` uses the system clock. + // We can at least check that the steady (monotonic, `.tick`) portion is + // approximately non-decreasing along "before," "during," and "after." + // Only "approximately," because the `datadog::tracing::default_clock` can't + // measure both clocks exactly simultaneously, so its correction to the + // steady time might actually set it back in time some tiny amount. + const datadog::tracing::Clock clock = datadog::tracing::default_clock; + const datadog::tracing::TimePoint before = clock(); + const datadog::tracing::TimePoint estimated_before = estimateTime(before.wall); + const datadog::tracing::TimePoint after = clock(); + + const auto tolerance = std::chrono::microseconds(100); + + EXPECT_LE(before.tick, after.tick); + EXPECT_LE(estimated_before.tick - before.tick, tolerance); + EXPECT_LE(after.tick - estimated_before.tick, tolerance); +} + } // namespace } // namespace Datadog } // namespace Tracers