From 12ca34d517cf9673129eaf85781f2eafff796da9 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 18 Jan 2023 13:16:31 -0500 Subject: [PATCH 01/20] 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/20] 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/20] 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 d44b37d9d5ed70a351eaf0125394f1b7798801a2 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 18 Jan 2023 15:30:38 -0500 Subject: [PATCH 04/20] TODO: AMEND Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/BUILD | 2 + .../extensions/tracers/datadog/dict_util.cc | 78 +++++++++++++++++++ source/extensions/tracers/datadog/dict_util.h | 59 ++++++++++++++ .../tracers/datadog/dict_util_test.cc | 67 ++++++++++++++++ 4 files changed, 206 insertions(+) create mode 100644 source/extensions/tracers/datadog/dict_util.cc create mode 100644 source/extensions/tracers/datadog/dict_util.h create mode 100644 test/extensions/tracers/datadog/dict_util_test.cc diff --git a/source/extensions/tracers/datadog/BUILD b/source/extensions/tracers/datadog/BUILD index 6ec069a593614..04c82ffac9b23 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", + "dict_util.cc", ], hdrs = [ "datadog_tracer_impl.h", + "dict_util.h", ], copts = [ # Make sure that headers included from dd_trace_cpp use Abseil diff --git a/source/extensions/tracers/datadog/dict_util.cc b/source/extensions/tracers/datadog/dict_util.cc new file mode 100644 index 0000000000000..c56fda2859150 --- /dev/null +++ b/source/extensions/tracers/datadog/dict_util.cc @@ -0,0 +1,78 @@ +#include "source/extensions/tracers/datadog/dict_util.h" + +#include + +#include "envoy/http/header_map.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace Datadog { + +RequestHeaderWriter::RequestHeaderWriter(Http::RequestHeaderMap& headers) : headers_(headers) {} + +void RequestHeaderWriter::set(datadog::tracing::StringView key, + datadog::tracing::StringView value) { + headers_.setCopy(Http::LowerCaseString{key}, value); +} + +ResponseHeaderReader::ResponseHeaderReader(const Http::ResponseHeaderMap& headers) + : headers_(headers) {} + +datadog::tracing::Optional +ResponseHeaderReader::lookup(datadog::tracing::StringView key) const { + auto result = headers_.get(Http::LowerCaseString{key}); + if (result.empty()) { + // `headers_.get` can return multiple header entries. It conveys + // "not found" by returning zero header entries. + return datadog::tracing::nullopt; + } + + if (result.size() == 1) { + return result[0]->value().getStringView(); + } + + // There's more than one matching header entry. + // Per RFC 2616, this is the same as if there were only one entry whose + // value is the comma-separated concatenation of the multiple values. + // I don't expect the Agent to repeat response headers, and we don't even + // examine the Agent's response headers, but here's a solution anyway. + std::size_t i = 0; + datadog::tracing::assign(buffer_, result[i]->value().getStringView()); + for (++i; i < result.size(); ++i) { + buffer_ += ", "; + datadog::tracing::append(buffer_, result[i]->value().getStringView()); + } + return buffer_; +} + +void ResponseHeaderReader::visit( + const std::function& + visitor) const { + headers_.iterate([&](const Http::HeaderEntry& entry) { + visitor(entry.key().getStringView(), entry.value().getStringView()); + return Http::ResponseHeaderMap::Iterate::Continue; + }); +} + +TraceContextReader::TraceContextReader(const Tracing::TraceContext& context) : context_(context) {} + +datadog::tracing::Optional +TraceContextReader::lookup(datadog::tracing::StringView key) const { + return context_.getByKey(key); +} + +void TraceContextReader::visit( + const std::function& + visitor) const { + context_.forEach([&](absl::string_view key, absl::string_view value) { + visitor(key, value); + const bool continue_iterating = true; + return continue_iterating; + }); +} + +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/datadog/dict_util.h b/source/extensions/tracers/datadog/dict_util.h new file mode 100644 index 0000000000000..bba8b4b460d6c --- /dev/null +++ b/source/extensions/tracers/datadog/dict_util.h @@ -0,0 +1,59 @@ +#pragma once + +#include +#include + +#include + +namespace Envoy { +namespace Tracing { +class TraceContext; +} // namespace Tracing +namespace Http { +class RequestHeaderMap; +class ResponseHeaderMap; +} // namespace Http +namespace Extensions { +namespace Tracers { +namespace Datadog { + +class RequestHeaderWriter : public datadog::tracing::DictWriter { + Http::RequestHeaderMap& headers_; + +public: + explicit RequestHeaderWriter(Http::RequestHeaderMap& headers); + + void set(datadog::tracing::StringView key, datadog::tracing::StringView value) override; +}; + +class ResponseHeaderReader : public datadog::tracing::DictReader { + const Http::ResponseHeaderMap& headers_; + mutable std::string buffer_; + +public: + explicit ResponseHeaderReader(const Http::ResponseHeaderMap& headers); + + datadog::tracing::Optional + lookup(datadog::tracing::StringView key) const override; + + void visit(const std::function& visitor) const override; +}; + +class TraceContextReader : public datadog::tracing::DictReader { + const Tracing::TraceContext& context_; + +public: + explicit TraceContextReader(const Tracing::TraceContext& context); + + datadog::tracing::Optional + lookup(datadog::tracing::StringView key) const override; + + void visit(const std::function& visitor) const override; +}; + +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/tracers/datadog/dict_util_test.cc b/test/extensions/tracers/datadog/dict_util_test.cc new file mode 100644 index 0000000000000..63ebaf203799a --- /dev/null +++ b/test/extensions/tracers/datadog/dict_util_test.cc @@ -0,0 +1,67 @@ +// TODO + +TEST(DatadogTracerDictUtilTest, RequestHeaderWriter) { + Http::TestRequestHeaderMapImpl headers; + RequestHeaderWriter writer{headers}; + + writer.set("foo", "bar"); + writer.set("FOO", "baz"); + writer.set("sniff", "wiggle"); + + auto result = headers.get(Http::LowerCaseString{"foo"}); + ASSERT_EQ(1, result.size()); + EXPECT_EQ("baz", result[0]->value().getStringView()); + + result = headers.get(Http::LowerCaseString{"sniff"}); + ASSERT_EQ(1, result.size()); + EXPECT_EQ("wiggle", result[0]->value().getStringView()); + + result = headers.get(Http::LowerCaseString{"missing"}); + EXPECT_EQ(0, result.size()); +} + +TEST(DatadogTracerDictUtilTest, ResponseHeaderReader) { + Http::TestResponseHeaderMapImpl headers{ + {"fish", "face"}, + {"fish", "flakes"}, + {"UPPER", "case"}, + }; + ResponseHeaderReader reader{headers}; + + auto result = reader.lookup("fish"); + EXPECT_EQ("face, flakes", result); + + result = reader.lookup("upper"); + EXPECT_EQ("case", result); + + result = reader.lookup("missing"); + EXPECT_EQ(datadog::tracing::nullopt, result); + + std::vector> expected_visitation{ + // These entries are in reverse. We'll `pop_back` as we go. + {"upper", "case"}, + // `lookup` comma-separates duplicate headers, but `visit` does not. + {"fish", "flakes"}, + {"fish", "face"}, + }; + reader.visit([&](const auto& key, const auto& value) { + ASSERT_FALSE(expected_visitation.empty()); + const auto& [expected_key, expected_value] = expected_visitation.back(); + EXPECT_EQ(expected_key, key); + EXPECT_EQ(expected_value, value); + expected_visitation.pop_back(); + }); +} + +TEST(DatadogTracerDictUtilTest, TraceContextReader) { + // `dd-trace-cpp` doesn't call `visit` when it's extracting trace context, but + // the method is nonetheless required by the `DictReader` interface. + // TODO: test the rest, too. + const Tracing::TestTraceContextImpl context{{"foo", "bar"}, {"boo", "yah"}}; + const TraceContextReader reader{context}; + reader.visit([&](const auto& key, const auto& value) { + const auto found = context.context_map_.find(key); + ASSERT_NE(context.context_map_.end(), found); + EXPECT_EQ(found->second, value); + }); +} From 68cb72328dd160121a42068b275bf027d3962e6e Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 18 Jan 2023 16:23:34 -0500 Subject: [PATCH 05/20] datadog: dict_util with tests Signed-off-by: David Goffredo --- test/extensions/tracers/datadog/BUILD | 1 + .../tracers/datadog/dict_util_test.cc | 39 +++++++++++++++++-- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/test/extensions/tracers/datadog/BUILD b/test/extensions/tracers/datadog/BUILD index 0dff983f70663..5b3d7c6a5c926 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", + "dict_util_test.cc", ], copts = [ # Make sure that headers included from dd_trace_cpp use Abseil diff --git a/test/extensions/tracers/datadog/dict_util_test.cc b/test/extensions/tracers/datadog/dict_util_test.cc index 63ebaf203799a..d20e669c101b4 100644 --- a/test/extensions/tracers/datadog/dict_util_test.cc +++ b/test/extensions/tracers/datadog/dict_util_test.cc @@ -1,4 +1,22 @@ -// TODO +#include + +#include +#include + +#include "envoy/http/header_map.h" + +#include "source/extensions/tracers/datadog/dict_util.h" + +#include "test/mocks/http/mocks.h" +#include "test/test_common/utility.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace Datadog { +namespace { TEST(DatadogTracerDictUtilTest, RequestHeaderWriter) { Http::TestRequestHeaderMapImpl headers; @@ -54,14 +72,27 @@ TEST(DatadogTracerDictUtilTest, ResponseHeaderReader) { } TEST(DatadogTracerDictUtilTest, TraceContextReader) { - // `dd-trace-cpp` doesn't call `visit` when it's extracting trace context, but - // the method is nonetheless required by the `DictReader` interface. - // TODO: test the rest, too. const Tracing::TestTraceContextImpl context{{"foo", "bar"}, {"boo", "yah"}}; const TraceContextReader reader{context}; + + auto result = reader.lookup("foo"); + EXPECT_EQ(result, "bar"); + result = reader.lookup("boo"); + EXPECT_EQ(result, "yah"); + result = reader.lookup("snark"); + EXPECT_EQ(result, datadog::tracing::nullopt); + + // `dd-trace-cpp` doesn't call `visit` when it's extracting trace context, but + // the method is nonetheless required by the `DictReader` interface. reader.visit([&](const auto& key, const auto& value) { const auto found = context.context_map_.find(key); ASSERT_NE(context.context_map_.end(), found); EXPECT_EQ(found->second, value); }); } + +} // namespace +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy From 63ca86f8ef8764eaca0056cbc9c4cf91f33be784 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 18 Jan 2023 16:39:09 -0500 Subject: [PATCH 06/20] datadog: add tracer_stats without tests Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/BUILD | 1 + .../extensions/tracers/datadog/tracer_stats.h | 28 +++++++++++++++++++ 2 files changed, 29 insertions(+) create mode 100644 source/extensions/tracers/datadog/tracer_stats.h diff --git a/source/extensions/tracers/datadog/BUILD b/source/extensions/tracers/datadog/BUILD index 6ec069a593614..b978705191d83 100644 --- a/source/extensions/tracers/datadog/BUILD +++ b/source/extensions/tracers/datadog/BUILD @@ -18,6 +18,7 @@ envoy_cc_library( ], hdrs = [ "datadog_tracer_impl.h", + "tracer_stats.h", ], copts = [ # Make sure that headers included from dd_trace_cpp use Abseil diff --git a/source/extensions/tracers/datadog/tracer_stats.h b/source/extensions/tracers/datadog/tracer_stats.h new file mode 100644 index 0000000000000..543507d95b95a --- /dev/null +++ b/source/extensions/tracers/datadog/tracer_stats.h @@ -0,0 +1,28 @@ +#include "envoy/stats/scope.h" +#include "envoy/stats/stats_macros.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace Datadog { + +#define DATADOG_TRACER_STATS(COUNTER) \ + COUNTER(reports_skipped_no_cluster) \ + COUNTER(reports_sent) \ + COUNTER(reports_dropped) \ + COUNTER(reports_failed) + +struct TracerStats { + DATADOG_TRACER_STATS(GENERATE_COUNTER_STRUCT) +}; + +inline TracerStats makeTracerStats(Stats::Scope& scope) { + return TracerStats{DATADOG_TRACER_STATS(POOL_COUNTER_PREFIX(scope, "tracing.datadog."))}; +} + +#undef DATADOG_TRACER_STATS + +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy From 63f5f68717734cb9241947db4b1c81d699de72b0 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 18 Jan 2023 17:09:17 -0500 Subject: [PATCH 07/20] datadog: add tracer_stats tests Signed-off-by: David Goffredo --- test/extensions/tracers/datadog/BUILD | 1 + .../tracers/datadog/tracer_stats_test.cc | 54 +++++++++++++++++++ 2 files changed, 55 insertions(+) create mode 100644 test/extensions/tracers/datadog/tracer_stats_test.cc diff --git a/test/extensions/tracers/datadog/BUILD b/test/extensions/tracers/datadog/BUILD index 0dff983f70663..c098f43df7c98 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", + "tracer_stats_test.cc", ], copts = [ # Make sure that headers included from dd_trace_cpp use Abseil diff --git a/test/extensions/tracers/datadog/tracer_stats_test.cc b/test/extensions/tracers/datadog/tracer_stats_test.cc new file mode 100644 index 0000000000000..d88cb207a95f0 --- /dev/null +++ b/test/extensions/tracers/datadog/tracer_stats_test.cc @@ -0,0 +1,54 @@ +#include "source/extensions/tracers/datadog/tracer_stats.h" + +#include "test/common/stats/stat_test_utility.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace Datadog { +namespace { + +TEST(DatadogTracerTracerStatsTest, TracerStats) { + Stats::TestUtil::TestStore store; + TracerStats stats = makeTracerStats(*store.rootScope()); + + // Verify the names. + EXPECT_EQ(stats.reports_skipped_no_cluster_.name(), "tracing.datadog.reports_skipped_no_cluster"); + EXPECT_EQ(stats.reports_sent_.name(), "tracing.datadog.reports_sent"); + EXPECT_EQ(stats.reports_dropped_.name(), "tracing.datadog.reports_dropped"); + EXPECT_EQ(stats.reports_failed_.name(), "tracing.datadog.reports_failed"); + + // Counters begin at zero. + EXPECT_EQ(0, store.counter("tracing.datadog.reports_skipped_no_cluster").value()); + EXPECT_EQ(0, store.counter("tracing.datadog.reports_sent").value()); + EXPECT_EQ(0, store.counter("tracing.datadog.reports_dropped").value()); + EXPECT_EQ(0, store.counter("tracing.datadog.reports_failed").value()); + + // Increments are reflected in the value. + stats.reports_skipped_no_cluster_.inc(); + EXPECT_EQ(1, store.counter("tracing.datadog.reports_skipped_no_cluster").value()); + stats.reports_sent_.inc(); + EXPECT_EQ(1, store.counter("tracing.datadog.reports_sent").value()); + stats.reports_dropped_.inc(); + EXPECT_EQ(1, store.counter("tracing.datadog.reports_dropped").value()); + stats.reports_failed_.inc(); + EXPECT_EQ(1, store.counter("tracing.datadog.reports_failed").value()); + + // And again. + stats.reports_skipped_no_cluster_.inc(); + EXPECT_EQ(2, store.counter("tracing.datadog.reports_skipped_no_cluster").value()); + stats.reports_sent_.inc(); + EXPECT_EQ(2, store.counter("tracing.datadog.reports_sent").value()); + stats.reports_dropped_.inc(); + EXPECT_EQ(2, store.counter("tracing.datadog.reports_dropped").value()); + stats.reports_failed_.inc(); + EXPECT_EQ(2, store.counter("tracing.datadog.reports_failed").value()); +} + +} // namespace +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy From 255587521a4917b33273e48b90e997b6d409b84a Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Mon, 23 Jan 2023 11:54:26 -0500 Subject: [PATCH 08/20] datadog: add comment describing dict_util Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/dict_util.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/source/extensions/tracers/datadog/dict_util.h b/source/extensions/tracers/datadog/dict_util.h index bba8b4b460d6c..c9e6bb080463b 100644 --- a/source/extensions/tracers/datadog/dict_util.h +++ b/source/extensions/tracers/datadog/dict_util.h @@ -1,5 +1,15 @@ #pragma once +/* + * This file contains implementations of the datadog::tracing::DictReader and + * datadog::tracing::DictWriter interfaces. + * + * The Datadog core tracing library, dd-trace-cpp, uses these interfaces + * anywhere it needs to read from or write to mappings of string, such as when + * extracting trace context, injecting trace context, reading HTTP response + * headers, or writing HTTP request headers. + */ + #include #include From 9da7087ad15ee835b18c3581df654f499f1b6af7 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Mon, 23 Jan 2023 12:06:17 -0500 Subject: [PATCH 09/20] datadog: move private data members to bottom of class definition Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/dict_util.h | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/source/extensions/tracers/datadog/dict_util.h b/source/extensions/tracers/datadog/dict_util.h index c9e6bb080463b..a844a5cce1adb 100644 --- a/source/extensions/tracers/datadog/dict_util.h +++ b/source/extensions/tracers/datadog/dict_util.h @@ -28,18 +28,16 @@ namespace Tracers { namespace Datadog { class RequestHeaderWriter : public datadog::tracing::DictWriter { - Http::RequestHeaderMap& headers_; - public: explicit RequestHeaderWriter(Http::RequestHeaderMap& headers); void set(datadog::tracing::StringView key, datadog::tracing::StringView value) override; + +private: + Http::RequestHeaderMap& headers_; }; class ResponseHeaderReader : public datadog::tracing::DictReader { - const Http::ResponseHeaderMap& headers_; - mutable std::string buffer_; - public: explicit ResponseHeaderReader(const Http::ResponseHeaderMap& headers); @@ -48,11 +46,13 @@ class ResponseHeaderReader : public datadog::tracing::DictReader { void visit(const std::function& visitor) const override; + +private: + const Http::ResponseHeaderMap& headers_; + mutable std::string buffer_; }; class TraceContextReader : public datadog::tracing::DictReader { - const Tracing::TraceContext& context_; - public: explicit TraceContextReader(const Tracing::TraceContext& context); @@ -61,6 +61,9 @@ class TraceContextReader : public datadog::tracing::DictReader { void visit(const std::function& visitor) const override; + +private: + const Tracing::TraceContext& context_; }; } // namespace Datadog From 0c930aa0700c9531474e9910a3821955673b23f2 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 26 Jan 2023 16:52:04 -0500 Subject: [PATCH 10/20] datadog: AgentHttpClient without tests Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/BUILD | 2 + .../tracers/datadog/agent_http_client.cc | 144 ++++++++++++++++++ .../tracers/datadog/agent_http_client.h | 68 +++++++++ 3 files changed, 214 insertions(+) create mode 100644 source/extensions/tracers/datadog/agent_http_client.cc create mode 100644 source/extensions/tracers/datadog/agent_http_client.h diff --git a/source/extensions/tracers/datadog/BUILD b/source/extensions/tracers/datadog/BUILD index f892332fa35d8..a51afb2d36bd8 100644 --- a/source/extensions/tracers/datadog/BUILD +++ b/source/extensions/tracers/datadog/BUILD @@ -14,10 +14,12 @@ envoy_extension_package() envoy_cc_library( name = "datadog_tracer_lib", srcs = [ + "agent_http_client.cc", "datadog_tracer_impl.cc", "dict_util.cc", ], hdrs = [ + "agent_http_client.h", "datadog_tracer_impl.h", "dict_util.h", "tracer_stats.h", diff --git a/source/extensions/tracers/datadog/agent_http_client.cc b/source/extensions/tracers/datadog/agent_http_client.cc new file mode 100644 index 0000000000000..f7278884ed4e8 --- /dev/null +++ b/source/extensions/tracers/datadog/agent_http_client.cc @@ -0,0 +1,144 @@ +#include "source/extensions/tracers/datadog/agent_http_client.h" + +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +#include "source/common/http/message_impl.h" +#include "source/common/http/utility.h" +#include "source/extensions/tracers/datadog/dict_util.h" +#include "source/extensions/tracers/datadog/tracer_stats.h" + +#include "absl/strings/str_format.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace Datadog { + +AgentHTTPClient::AgentHTTPClient(Upstream::ClusterManager& cluster_manager, + const std::string& cluster, const std::string& reference_host, + TracerStats& stats) + : collector_cluster_(cluster_manager, cluster), cluster_(cluster), + reference_host_(reference_host), stats_(stats) {} + +AgentHTTPClient::~AgentHTTPClient() { + for (const auto& [request_ptr, _] : handlers_) { + assert(request_ptr); + request_ptr->cancel(); + } +} + +// datadog::tracing::HTTPClient + +datadog::tracing::Expected AgentHTTPClient::post(const URL& url, HeadersSetter set_headers, + std::string body, + ResponseHandler on_response, + ErrorHandler on_error) { + ENVOY_LOG(debug, "flushing traces"); + + auto message = std::make_unique(); + Http::RequestHeaderMap& headers = message->headers(); + headers.setReferenceMethod(Http::Headers::get().MethodValues.Post); + headers.setReferencePath(url.path); + headers.setReferenceHost(reference_host_); + + RequestHeaderWriter writer{message->headers()}; + set_headers(writer); + + message->body().add(body); + ENVOY_LOG(debug, "submitting trace(s) to {} with payload size {}", url.path, body.size()); + + if (!collector_cluster_.threadLocalCluster().has_value()) { + ENVOY_LOG(debug, "collector cluster '{}' does not exist", cluster_); + stats_.reports_skipped_no_cluster_.inc(); + return datadog::tracing::nullopt; + } + + Http::AsyncClient::Request* request = + collector_cluster_.threadLocalCluster()->get().httpAsyncClient().send( + std::move(message), *this, + Http::AsyncClient::RequestOptions().setTimeout(std::chrono::milliseconds(1000))); + if (!request) { + stats_.reports_failed_.inc(); + return datadog::tracing::Error{datadog::tracing::Error::ENVOY_HTTP_CLIENT_FAILURE, + "Failed to create request."}; + } + + handlers_.emplace(request, Handlers{std::move(on_response), std::move(on_error)}); + return datadog::tracing::nullopt; +} + +void AgentHTTPClient::drain(std::chrono::steady_clock::time_point) {} + +nlohmann::json AgentHTTPClient::config_json() const { + return nlohmann::json::object({ + {"type", "Envoy::Extensions::Tracers::Datadog::AgentHTTPClient"}, + }); +} + +// Http::AsyncClient::Callbacks + +void AgentHTTPClient::onSuccess(const Http::AsyncClient::Request& request, + Http::ResponseMessagePtr&& response) { + const std::uint64_t status = Http::Utility::getResponseStatus(response->headers()); + if (status != std::uint64_t(Http::Code::OK)) { + stats_.reports_dropped_.inc(); + } else { + ENVOY_LOG(debug, "traces successfully submitted to datadog agent"); + stats_.reports_sent_.inc(); + } + + auto found = handlers_.find(const_cast(&request)); + if (found == handlers_.end()) { + ENVOY_LOG(debug, "request at address 0x{} is not in the map of {} Handlers objects", + absl::StrFormat("%p", &request), handlers_.size()); + return; + } + + Handlers& handlers = found->second; + ResponseHeaderReader reader{response->headers()}; + handlers.on_response(status, reader, response->bodyAsString()); + + handlers_.erase(found); +} + +void AgentHTTPClient::onFailure(const Http::AsyncClient::Request& request, + Http::AsyncClient::FailureReason reason) { + auto found = handlers_.find(const_cast(&request)); + // If the request failed to start, then `post` never even registered the request. + if (found == handlers_.end()) { + return; + } + + stats_.reports_failed_.inc(); + + Handlers& handlers = found->second; + std::string message = "Failed to send request to Datadog Agent: "; + switch (reason) { + case Http::AsyncClient::FailureReason::Reset: + message += "The stream has been reset."; + break; + default: + message += "Unknown error."; + } + handlers.on_error(datadog::tracing::Error{datadog::tracing::Error::ENVOY_HTTP_CLIENT_FAILURE, + std::move(message)}); + + handlers_.erase(found); +} + +void AgentHTTPClient::onBeforeFinalizeUpstreamSpan(Tracing::Span&, const Http::ResponseHeaderMap*) { +} + +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/tracers/datadog/agent_http_client.h b/source/extensions/tracers/datadog/agent_http_client.h new file mode 100644 index 0000000000000..441e0b13fb561 --- /dev/null +++ b/source/extensions/tracers/datadog/agent_http_client.h @@ -0,0 +1,68 @@ +#pragma once + +#include + +#include + +#include "envoy/http/async_client.h" + +#include "source/common/common/logger.h" +#include "source/common/upstream/cluster_update_tracker.h" + +#include "absl/container/flat_hash_map.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace Datadog { + +struct TracerStats; + +class AgentHTTPClient : public datadog::tracing::HTTPClient, + public Http::AsyncClient::Callbacks, + private Logger::Loggable { +public: + struct Handlers { + ResponseHandler on_response; + ErrorHandler on_error; + }; + +public: + // Create an `AgentHTTPClient` that uses the specified `ClusterManager` to + // make requests to the specified `cluster`, where requests include the + // specified `reference_host` as the "Host" header. Use the specified + // `TracerStats` to keep track of usage statistics. + AgentHTTPClient(Upstream::ClusterManager&, const std::string& cluster, + const std::string& reference_host, TracerStats&); + + ~AgentHTTPClient() override; + + // datadog::tracing::HTTPClient + + datadog::tracing::Expected post(const URL& url, HeadersSetter set_headers, std::string body, + ResponseHandler on_response, + ErrorHandler on_error) override; + + // `drain` has no effect. + void drain(std::chrono::steady_clock::time_point) override; + + nlohmann::json config_json() const override; + + // Http::AsyncClient::Callbacks + + void onSuccess(const Http::AsyncClient::Request&, Http::ResponseMessagePtr&&) override; + void onFailure(const Http::AsyncClient::Request&, Http::AsyncClient::FailureReason) override; + void onBeforeFinalizeUpstreamSpan(Tracing::Span&, const Http::ResponseHeaderMap*) override; + +private: + absl::flat_hash_map handlers_; + Upstream::ClusterUpdateTracker collector_cluster_; + std::string cluster_; + std::string reference_host_; + TracerStats& stats_; +}; + +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy From d9e989d648d98cb152e3504489faac9bbf887d60 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 1 Feb 2023 16:21:02 -0500 Subject: [PATCH 11/20] datadog: AgentHttpClient dummy test and some documentation Signed-off-by: David Goffredo --- .../tracers/datadog/agent_http_client.h | 48 +++++++++++++++---- test/extensions/tracers/datadog/BUILD | 1 + .../tracers/datadog/agent_http_client_test.cc | 27 +++++++++++ 3 files changed, 68 insertions(+), 8 deletions(-) create mode 100644 test/extensions/tracers/datadog/agent_http_client_test.cc diff --git a/source/extensions/tracers/datadog/agent_http_client.h b/source/extensions/tracers/datadog/agent_http_client.h index 441e0b13fb561..7c6bb9ac11b19 100644 --- a/source/extensions/tracers/datadog/agent_http_client.h +++ b/source/extensions/tracers/datadog/agent_http_client.h @@ -28,24 +28,56 @@ class AgentHTTPClient : public datadog::tracing::HTTPClient, }; public: - // Create an `AgentHTTPClient` that uses the specified `ClusterManager` to - // make requests to the specified `cluster`, where requests include the - // specified `reference_host` as the "Host" header. Use the specified - // `TracerStats` to keep track of usage statistics. - AgentHTTPClient(Upstream::ClusterManager&, const std::string& cluster, - const std::string& reference_host, TracerStats&); - + /** + * Create an \c AgentHTTPClient that uses the specified \p cluster_manager + * to make requests to the specified \p cluster, where requests include the + * specified \p reference_host as the "Host" header. Use the specified + * \p stats to keep track of usage statistics. + * @param cluster_manager cluster manager from which the thread local cluster + * is retrieved in order to make HTTP requests + * @param cluster the name of the cluster to which HTTP requests are made + * @param reference_host the value to use for the "Host" HTTP request header + * @param stats a collection of counters used to keep track of events, such as + * when a request fails + */ + AgentHTTPClient(Upstream::ClusterManager& cluster_manager, const std::string& cluster, + const std::string& reference_host, TracerStats& stats); ~AgentHTTPClient() override; // datadog::tracing::HTTPClient + /** + * Send an HTTP POST request to the cluster, where the requested resource is + * \p url.path. Invoke \p set_headers immediately to obtain the HTTP request + * headers. Send \p body as the request body. Return a + * \c datadog::tracing::Error if one occurs, otherwise return + * \c datadog::tracing::nullopt. When a complete response is received, + * invoke \p on_response with the response status, response headers, and + * response body. If an error occurs before a complete response is received, + * invoke \p on_error with a \c datadog::tracing::Error. + * @param url URL from which the request path is taken + * @param set_headers callback invoked immediately to obtain request headers + * @param body data to send as POST request body + * @param on_response callback to invoke when a complete response is received + * @param on_error callback to invoke when an error occurs before a complete + * response is received. + * @return \c datadog::tracing::Error if an error occurs, or + * \c datadog::tracing::nullopt otherwise. + */ datadog::tracing::Expected post(const URL& url, HeadersSetter set_headers, std::string body, ResponseHandler on_response, ErrorHandler on_error) override; - // `drain` has no effect. + /** + * \c drain has no effect. It's a part of the \c datadog::tracing::HTTPClient + * that we do not need. + */ void drain(std::chrono::steady_clock::time_point) override; + /** + * Return a JSON representation of this object's configuration. This function + * is used in the startup banner logged by \c dd-trace-cpp. + */ nlohmann::json config_json() const override; // Http::AsyncClient::Callbacks diff --git a/test/extensions/tracers/datadog/BUILD b/test/extensions/tracers/datadog/BUILD index 7df5d4c46ea1a..12c4d03e3ff41 100644 --- a/test/extensions/tracers/datadog/BUILD +++ b/test/extensions/tracers/datadog/BUILD @@ -14,6 +14,7 @@ envoy_package() envoy_extension_cc_test( name = "datadog_tracer_impl_test", srcs = [ + "agent_http_client_test.cc", "datadog_tracer_impl_test.cc", "dict_util_test.cc", "tracer_stats_test.cc", diff --git a/test/extensions/tracers/datadog/agent_http_client_test.cc b/test/extensions/tracers/datadog/agent_http_client_test.cc new file mode 100644 index 0000000000000..8d5308106851c --- /dev/null +++ b/test/extensions/tracers/datadog/agent_http_client_test.cc @@ -0,0 +1,27 @@ +#include + +#include +#include + +#include "envoy/http/header_map.h" + +#include "source/extensions/tracers/datadog/dict_util.h" + +#include "test/mocks/http/mocks.h" +#include "test/test_common/utility.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace Datadog { +namespace { + +TEST(DatadogAgentHttpClientTest, TODO) { EXPECT_TRUE(false); } + +} // namespace +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy From 7c019cf4447207e95edc511041791f9f99bf1c5d Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Fri, 3 Feb 2023 16:13:31 -0500 Subject: [PATCH 12/20] document AgentHTTPClient Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/agent_http_client.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/source/extensions/tracers/datadog/agent_http_client.h b/source/extensions/tracers/datadog/agent_http_client.h index c1e2f85d1870f..2e38a938ab183 100644 --- a/source/extensions/tracers/datadog/agent_http_client.h +++ b/source/extensions/tracers/datadog/agent_http_client.h @@ -17,6 +17,13 @@ namespace Datadog { struct TracerStats; +/** + * \c datadog::tracing::HTTPClient implementation that uses Envoy's + * \c Http::AsyncClient. The class is called \c AgentHTTPClient because it is + * not a general-purpose HTTP client. Instead, it sends requests to a specified + * cluster only. The idea is that the cluster is configured to point to a + * Datadog Agent instance. + */ class AgentHTTPClient : public datadog::tracing::HTTPClient, public Http::AsyncClient::Callbacks, private Logger::Loggable { From e08c21429a2f2a60b809eb52761b5d8cb532f2ae Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Fri, 3 Feb 2023 17:30:37 -0500 Subject: [PATCH 13/20] datadog: have one AgentHTTPClient test working Signed-off-by: David Goffredo --- .../tracers/datadog/agent_http_client_test.cc | 42 ++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/test/extensions/tracers/datadog/agent_http_client_test.cc b/test/extensions/tracers/datadog/agent_http_client_test.cc index 00509fdbebbf8..5320688c3194f 100644 --- a/test/extensions/tracers/datadog/agent_http_client_test.cc +++ b/test/extensions/tracers/datadog/agent_http_client_test.cc @@ -1,13 +1,19 @@ +#include #include #include #include "envoy/http/header_map.h" +#include "source/extensions/tracers/datadog/agent_http_client.h" #include "source/extensions/tracers/datadog/dict_util.h" +#include "source/extensions/tracers/datadog/tracer_stats.h" #include "test/mocks/http/mocks.h" +#include "test/mocks/upstream/cluster_manager.h" #include "test/test_common/utility.h" +#include "absl/types/optional.h" +#include "datadog/expected.h" #include "datadog/optional.h" #include "gtest/gtest.h" @@ -17,7 +23,41 @@ namespace Tracers { namespace Datadog { namespace { -TEST(DatadogAgentHttpClientTest, TODO) { EXPECT_TRUE(false); } +TEST(DatadogAgentHttpClientTest, PathFromURL) { + // The `.path` portion of the `URL` argument to `AgentHTTPClient::post` ends + // up as the "reference path" of the `Http::RequestHeaderMap`. + // That is, the URL "http://foobar.com/trace/v04" results in "/trace/v04". + + NiceMock cluster_manager; + cluster_manager.initializeClusters({"fake_cluster"}, {}); + cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; + cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); + Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); + Stats::TestUtil::TestStore store; + TracerStats stats = makeTracerStats(*store.rootScope()); + AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); + datadog::tracing::HTTPClient::URL url; + url.scheme = "http"; + url.authority = "localhost:8126"; + url.path = "/foo/bar"; + + EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce( + Invoke([&](Http::RequestMessagePtr& message, Http::AsyncClient::Callbacks&, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + EXPECT_EQ(url.path, message->headers().path()); + return &request; + })); + + // `~AgentHTTPClient()` will cancel the request since we don't finish it here. + EXPECT_CALL(request, cancel()); + + const auto ignore = [](auto&&...) {}; + datadog::tracing::Expected result = client.post(url, ignore, "", ignore, ignore); + EXPECT_TRUE(result) << result.error(); + EXPECT_EQ(0, stats.reports_skipped_no_cluster_.value()); + EXPECT_EQ(0, stats.reports_failed_.value()); +} } // namespace } // namespace Datadog From 1dee2ffb0d7ab7c1d5910f779dd989ed05670147 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Fri, 3 Feb 2023 18:53:01 -0500 Subject: [PATCH 14/20] datadog: a few more AgentHTTPClient tests Signed-off-by: David Goffredo --- .../tracers/datadog/agent_http_client_test.cc | 115 +++++++++++++++++- 1 file changed, 113 insertions(+), 2 deletions(-) diff --git a/test/extensions/tracers/datadog/agent_http_client_test.cc b/test/extensions/tracers/datadog/agent_http_client_test.cc index 5320688c3194f..02e0a5a4fd6e7 100644 --- a/test/extensions/tracers/datadog/agent_http_client_test.cc +++ b/test/extensions/tracers/datadog/agent_http_client_test.cc @@ -1,6 +1,4 @@ #include -#include -#include #include "envoy/http/header_map.h" @@ -13,6 +11,7 @@ #include "test/test_common/utility.h" #include "absl/types/optional.h" +#include "datadog/dict_writer.h" #include "datadog/expected.h" #include "datadog/optional.h" #include "gtest/gtest.h" @@ -59,6 +58,118 @@ TEST(DatadogAgentHttpClientTest, PathFromURL) { EXPECT_EQ(0, stats.reports_failed_.value()); } +TEST(DatadogAgentHttpClientTest, MissingThreadLocalCluster) { + // If ...`threadLocalCluster().has_value()` is false, then `post` cannot + // create a request and so will immediately return successfully but increment + // the "reports skipped no cluster" counter. + + NiceMock cluster_manager; + Stats::TestUtil::TestStore store; + TracerStats stats = makeTracerStats(*store.rootScope()); + AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); + datadog::tracing::HTTPClient::URL url; + url.scheme = "http"; + url.authority = "localhost:8126"; + url.path = "/foo/bar"; + + const auto ignore = [](auto&&...) {}; + datadog::tracing::Expected result = client.post(url, ignore, "", ignore, ignore); + EXPECT_TRUE(result) << result.error(); + EXPECT_EQ(1, stats.reports_skipped_no_cluster_.value()); + EXPECT_EQ(0, stats.reports_failed_.value()); +} + +TEST(DatadogAgentHttpClientTest, RequestHeaders) { + // The `set_headers` argument to `post(...)` results in the corresponding + // headers being set in `Http::RequestMessage::headers()`. + // Additionally, the "Host" header will always be the same as the + // corresponding parameter of `AgentHTTPClient`'s constructor. + + NiceMock cluster_manager; + cluster_manager.initializeClusters({"fake_cluster"}, {}); + cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; + cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); + Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); + Stats::TestUtil::TestStore store; + TracerStats stats = makeTracerStats(*store.rootScope()); + AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); + datadog::tracing::HTTPClient::URL url; + url.scheme = "http"; + url.authority = "localhost:8126"; + url.path = "/foo/bar"; + const auto set_headers = [&](datadog::tracing::DictWriter& headers) { + headers.set("foo", "bar"); + headers.set("baz-boing", "boing boing"); + headers.set("boing-boing", "boing boing"); + headers.set("boing-boing", "boing boing boing"); + }; + + EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce( + Invoke([&](Http::RequestMessagePtr& message, Http::AsyncClient::Callbacks&, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + EXPECT_EQ("test_host", message->headers().getHostValue()); + + EXPECT_EQ("bar", message->headers().getByKey("foo")); + EXPECT_EQ("boing boing", message->headers().getByKey("baz-boing")); + EXPECT_EQ("boing boing boing", message->headers().getByKey("boing-boing")); + + return &request; + })); + + // `~AgentHTTPClient()` will cancel the request since we don't finish it here. + EXPECT_CALL(request, cancel()); + + const auto ignore = [](auto&&...) {}; + datadog::tracing::Expected result = client.post(url, set_headers, "", ignore, ignore); + EXPECT_TRUE(result) << result.error(); + EXPECT_EQ(0, stats.reports_skipped_no_cluster_.value()); + EXPECT_EQ(0, stats.reports_failed_.value()); +} + +TEST(DatadogAgentHttpClientTest, RequestBody) { + // The `body` parameter to `AgentHTTPClient::post` corresponds to the + // resulting `Http::RequestMessage::body()`. + + NiceMock cluster_manager; + cluster_manager.initializeClusters({"fake_cluster"}, {}); + cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; + cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); + Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); + Stats::TestUtil::TestStore store; + TracerStats stats = makeTracerStats(*store.rootScope()); + AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); + datadog::tracing::HTTPClient::URL url; + url.scheme = "http"; + url.authority = "localhost:8126"; + url.path = "/foo/bar"; + const std::string body = R"latin( + Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod + tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, + quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo + consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse + cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat + non proident, sunt in culpa qui officia deserunt mollit anim id est + laborum.)latin"; + + EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce( + Invoke([&](Http::RequestMessagePtr& message, Http::AsyncClient::Callbacks&, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + EXPECT_EQ(body, message->body().toString()); + return &request; + })); + + // `~AgentHTTPClient()` will cancel the request since we don't finish it here. + EXPECT_CALL(request, cancel()); + + const auto ignore = [](auto&&...) {}; + datadog::tracing::Expected result = client.post(url, ignore, body, ignore, ignore); + EXPECT_TRUE(result) << result.error(); + EXPECT_EQ(0, stats.reports_skipped_no_cluster_.value()); + EXPECT_EQ(0, stats.reports_failed_.value()); +} + } // namespace } // namespace Datadog } // namespace Tracers From 4cd97c98bac887adcd0de2f1a34fd896c79677e6 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Tue, 7 Feb 2023 20:02:54 -0500 Subject: [PATCH 15/20] datadog: AgentHTTPClient unit tests Signed-off-by: David Goffredo --- .../tracers/datadog/agent_http_client_test.cc | 349 ++++++++++++++++++ 1 file changed, 349 insertions(+) diff --git a/test/extensions/tracers/datadog/agent_http_client_test.cc b/test/extensions/tracers/datadog/agent_http_client_test.cc index 02e0a5a4fd6e7..836b54797e407 100644 --- a/test/extensions/tracers/datadog/agent_http_client_test.cc +++ b/test/extensions/tracers/datadog/agent_http_client_test.cc @@ -2,6 +2,7 @@ #include "envoy/http/header_map.h" +#include "source/common/http/message_impl.h" #include "source/extensions/tracers/datadog/agent_http_client.h" #include "source/extensions/tracers/datadog/dict_util.h" #include "source/extensions/tracers/datadog/tracer_stats.h" @@ -12,7 +13,9 @@ #include "absl/types/optional.h" #include "datadog/dict_writer.h" +#include "datadog/error.h" #include "datadog/expected.h" +#include "datadog/json.hpp" #include "datadog/optional.h" #include "gtest/gtest.h" @@ -170,6 +173,352 @@ TEST(DatadogAgentHttpClientTest, RequestBody) { EXPECT_EQ(0, stats.reports_failed_.value()); } +TEST(DatadogAgentHttpClientTest, OnResponse200) { + // When `onSuccess` is invoked on the `Http::AsyncClient::Callbacks`, the + // associated `on_response` callback is invoked with corresponding arguments. + // Additionally, if the HTTP response status is 200, `stats_.reports_sent_` is + // incremented. + + NiceMock cluster_manager; + cluster_manager.initializeClusters({"fake_cluster"}, {}); + cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; + cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); + Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); + Stats::TestUtil::TestStore store; + TracerStats stats = makeTracerStats(*store.rootScope()); + AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); + datadog::tracing::HTTPClient::URL url; + url.scheme = "http"; + url.authority = "localhost:8126"; + url.path = "/foo/bar"; + Http::AsyncClient::Callbacks* callbacks; + testing::MockFunction + on_response; + testing::MockFunction on_error; + + EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce( + Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks = &callbacks_arg; + return &request; + })); + + // `callbacks->onSuccess(...)` will cause `on_response` to be called. + // `on_error` will not be called. + EXPECT_CALL(on_response, Call(200, _, "{}")); + EXPECT_CALL(on_error, Call(_)).Times(0); + + // The request will not be canceled; neither explicitly nor in + // `~AgentHTTPClient`, because it will have been successfully fulfilled. + EXPECT_CALL(request, cancel()).Times(0); + + const auto ignore = [](auto&&...) {}; + datadog::tracing::Expected result = + client.post(url, ignore, "{}", on_response.AsStdFunction(), on_error.AsStdFunction()); + EXPECT_TRUE(result) << result.error(); + + Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( + Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "200"}}})); + msg->body().add("{}"); + + callbacks->onSuccess(request, std::move(msg)); + EXPECT_EQ(1, stats.reports_sent_.value()); + EXPECT_EQ(0, stats.reports_failed_.value()); + EXPECT_EQ(0, stats.reports_skipped_no_cluster_.value()); +} + +TEST(DatadogAgentHttpClientTest, OnResponseNot200) { + // When `onSuccess` is invoked on the `Http::AsyncClient::Callbacks`, the + // associated `on_response` callback is invoked with corresponding arguments. + // Additionally, if the HTTP response status is not 200, + // `stats_.reports_dropped_` is incremented. + + NiceMock cluster_manager; + cluster_manager.initializeClusters({"fake_cluster"}, {}); + cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; + cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); + Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); + Stats::TestUtil::TestStore store; + TracerStats stats = makeTracerStats(*store.rootScope()); + AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); + datadog::tracing::HTTPClient::URL url; + url.scheme = "http"; + url.authority = "localhost:8126"; + url.path = "/foo/bar"; + Http::AsyncClient::Callbacks* callbacks; + testing::MockFunction + on_response; + testing::MockFunction on_error; + + EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce( + Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks = &callbacks_arg; + return &request; + })); + + // `callbacks->onSuccess(...)` will cause `on_response` to be called. + // The `404` value corresponds to the response sent below. + // `on_error` will not be called. + EXPECT_CALL(on_response, Call(404, _, "{}")); + EXPECT_CALL(on_error, Call(_)).Times(0); + + // The request will not be canceled; neither explicitly nor in + // `~AgentHTTPClient`, because it will have been successfully fulfilled. + EXPECT_CALL(request, cancel()).Times(0); + + const auto ignore = [](auto&&...) {}; + datadog::tracing::Expected result = + client.post(url, ignore, "{}", on_response.AsStdFunction(), on_error.AsStdFunction()); + EXPECT_TRUE(result) << result.error(); + + // The "404" below is what causes `stats.reports_failed_` to be incremented + // instead of `stats.reports_sent_`. + Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( + Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "404"}}})); + msg->body().add("{}"); + + callbacks->onSuccess(request, std::move(msg)); + EXPECT_EQ(1, stats.reports_dropped_.value()); + EXPECT_EQ(0, stats.reports_sent_.value()); + EXPECT_EQ(0, stats.reports_failed_.value()); + EXPECT_EQ(0, stats.reports_skipped_no_cluster_.value()); +} + +TEST(DatadogAgentHttpClientTest, OnResponseBogusRequest) { + // When `onSuccess` is invoked on the `Http::AsyncClient::Callbacks` with a + // request that is not registered with the HTTP client, then no callback is + // invoked (how would we look it up?). + + NiceMock cluster_manager; + cluster_manager.initializeClusters({"fake_cluster"}, {}); + cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; + cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); + Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); + Stats::TestUtil::TestStore store; + TracerStats stats = makeTracerStats(*store.rootScope()); + AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); + datadog::tracing::HTTPClient::URL url; + url.scheme = "http"; + url.authority = "localhost:8126"; + url.path = "/foo/bar"; + Http::AsyncClient::Callbacks* callbacks; + testing::MockFunction + on_response; + testing::MockFunction on_error; + + EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce( + Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks = &callbacks_arg; + return &request; + })); + + // `callbacks->onSuccess(...)` will not invoke any callbacks, because the + // request argument passed in is not registered with the HTTP client. + EXPECT_CALL(on_response, Call(_, _, _)).Times(0); + EXPECT_CALL(on_error, Call(_)).Times(0); + + // The request will will canceled by `~AgentHTTPClient` because `onSuccess` + // was passed the wrong request, and so the real request is never removed from + // the HTTP client's registry. + EXPECT_CALL(request, cancel()); + + const auto ignore = [](auto&&...) {}; + datadog::tracing::Expected result = + client.post(url, ignore, "{}", on_response.AsStdFunction(), on_error.AsStdFunction()); + EXPECT_TRUE(result) << result.error(); + + Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( + Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "200"}}})); + msg->body().add("{}"); + + // The first argument to `onSuccess` should be `request`, but instead we pass + // `bogus_request`. + Http::MockAsyncClientRequest bogus_request(&cluster_manager.thread_local_cluster_.async_client_); + callbacks->onSuccess(bogus_request, std::move(msg)); +} + +TEST(DatadogAgentHttpClientTest, OnErrorStreamReset) { + // When `onFailure` is invoked on the `Http::AsyncClient::Callbacks` with + // `FailureReason::Reset`, the associated `on_error` callback is invoked with + // a corresponding `datadog::tracing::Error`. + + NiceMock cluster_manager; + cluster_manager.initializeClusters({"fake_cluster"}, {}); + cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; + cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); + Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); + Stats::TestUtil::TestStore store; + TracerStats stats = makeTracerStats(*store.rootScope()); + AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); + datadog::tracing::HTTPClient::URL url; + url.scheme = "http"; + url.authority = "localhost:8126"; + url.path = "/foo/bar"; + Http::AsyncClient::Callbacks* callbacks; + testing::MockFunction + on_response; + testing::MockFunction on_error; + + EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce( + Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks = &callbacks_arg; + return &request; + })); + + // `callbacks->onFailure(...)` will cause `on_error` to be called. + // `on_response` will not be called. + EXPECT_CALL(on_error, Call(_)).WillOnce(Invoke([](datadog::tracing::Error error) { + EXPECT_EQ(error.code, datadog::tracing::Error::ENVOY_HTTP_CLIENT_FAILURE); + })); + EXPECT_CALL(on_response, Call(_, _, _)).Times(0); + + // The request will not be canceled; neither explicitly nor in + // `~AgentHTTPClient`, because it will have been successfully fulfilled. + EXPECT_CALL(request, cancel()).Times(0); + + const auto ignore = [](auto&&...) {}; + datadog::tracing::Expected result = + client.post(url, ignore, "{}", on_response.AsStdFunction(), on_error.AsStdFunction()); + EXPECT_TRUE(result) << result.error(); + + Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( + Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "200"}}})); + msg->body().add("{}"); + + callbacks->onFailure(request, Http::AsyncClient::FailureReason::Reset); +} + +TEST(DatadogAgentHttpClientTest, OnErrorOther) { + // When `onFailure` is invoked on the `Http::AsyncClient::Callbacks` with any + // value other than `FailureReason::Reset`, the associated `on_error` callback + // is invoked with a corresponding `datadog::tracing::Error`. + + NiceMock cluster_manager; + cluster_manager.initializeClusters({"fake_cluster"}, {}); + cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; + cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); + Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); + Stats::TestUtil::TestStore store; + TracerStats stats = makeTracerStats(*store.rootScope()); + AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); + datadog::tracing::HTTPClient::URL url; + url.scheme = "http"; + url.authority = "localhost:8126"; + url.path = "/foo/bar"; + Http::AsyncClient::Callbacks* callbacks; + testing::MockFunction + on_response; + testing::MockFunction on_error; + + EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce( + Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks = &callbacks_arg; + return &request; + })); + + // `callbacks->onFailure(...)` will cause `on_error` to be called. + // `on_response` will not be called. + EXPECT_CALL(on_error, Call(_)).WillOnce(Invoke([](datadog::tracing::Error error) { + EXPECT_EQ(error.code, datadog::tracing::Error::ENVOY_HTTP_CLIENT_FAILURE); + })); + EXPECT_CALL(on_response, Call(_, _, _)).Times(0); + + // The request will not be canceled; neither explicitly nor in + // `~AgentHTTPClient`, because it will have been successfully fulfilled. + EXPECT_CALL(request, cancel()).Times(0); + + const auto ignore = [](auto&&...) {}; + datadog::tracing::Expected result = + client.post(url, ignore, "{}", on_response.AsStdFunction(), on_error.AsStdFunction()); + EXPECT_TRUE(result) << result.error(); + + Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( + Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "200"}}})); + msg->body().add("{}"); + + const auto bogus_value = static_cast(-1); + callbacks->onFailure(request, bogus_value); +} + +TEST(DatadogAgentHttpClientTest, SendFailReturnsError) { + // If the underlying call to `httpAsyncClient().send(...)` returns an error, + // then the enclosing call to `AgentHTTPClient::post(...)` returns an error. + + NiceMock cluster_manager; + cluster_manager.initializeClusters({"fake_cluster"}, {}); + cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; + cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); + Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); + Stats::TestUtil::TestStore store; + TracerStats stats = makeTracerStats(*store.rootScope()); + AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); + datadog::tracing::HTTPClient::URL url; + url.scheme = "http"; + url.authority = "localhost:8126"; + url.path = "/foo/bar"; + Http::AsyncClient::Callbacks* callbacks; + + EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce( + Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks = &callbacks_arg; + return nullptr; // indicates error + })); + + const auto ignore = [](auto&&...) {}; + datadog::tracing::Expected result = client.post(url, ignore, "", ignore, ignore); + ASSERT_FALSE(result); + EXPECT_EQ(datadog::tracing::Error::ENVOY_HTTP_CLIENT_FAILURE, result.error().code); + EXPECT_EQ(1, stats.reports_failed_.value()); + EXPECT_EQ(0, stats.reports_skipped_no_cluster_.value()); +} + +TEST(DatadogAgentHttpClientTest, DrainIsANoOp) { + // `AgentHTTPClient::drain` doesn't do anything. It only makes sense in + // multi-threaded contexts. + // This test is for the sake of coverage. + + NiceMock cluster_manager; + cluster_manager.initializeClusters({"fake_cluster"}, {}); + cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; + cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); + Stats::TestUtil::TestStore store; + TracerStats stats = makeTracerStats(*store.rootScope()); + AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); + + // `deadline` value doesn't matter; `drain` ignores it. + const auto deadline = std::chrono::steady_clock::time_point::min(); + client.drain(deadline); +} + +TEST(DatadogAgentHttpClientTest, ConfigJSONContainsTypeName) { + NiceMock cluster_manager; + cluster_manager.initializeClusters({"fake_cluster"}, {}); + cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; + cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); + Stats::TestUtil::TestStore store; + TracerStats stats = makeTracerStats(*store.rootScope()); + AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); + + nlohmann::json config = client.config_json(); + EXPECT_EQ("Envoy::Extensions::Tracers::Datadog::AgentHTTPClient", config["type"]); +} + } // namespace } // namespace Datadog } // namespace Tracers From d600c11cc7e08049f800137774af073483c08fa0 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Tue, 7 Feb 2023 21:10:23 -0500 Subject: [PATCH 16/20] datadog: consolidate unit test boilerplate Signed-off-by: David Goffredo --- .../tracers/datadog/agent_http_client_test.cc | 462 +++++++----------- 1 file changed, 172 insertions(+), 290 deletions(-) diff --git a/test/extensions/tracers/datadog/agent_http_client_test.cc b/test/extensions/tracers/datadog/agent_http_client_test.cc index 836b54797e407..64c76ab120adc 100644 --- a/test/extensions/tracers/datadog/agent_http_client_test.cc +++ b/test/extensions/tracers/datadog/agent_http_client_test.cc @@ -25,81 +25,86 @@ namespace Tracers { namespace Datadog { namespace { -TEST(DatadogAgentHttpClientTest, PathFromURL) { +struct InitializedMockClusterManager { + InitializedMockClusterManager() { + instance_.initializeClusters({"fake_cluster"}, {}); + instance_.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; + instance_.initializeThreadLocalClusters({"fake_cluster"}); + } + + NiceMock instance_; +}; + +class DatadogAgentHttpClientTest : public testing::Test { +public: + DatadogAgentHttpClientTest() + : request_(&cluster_manager_.instance_.thread_local_cluster_.async_client_), + stats_(makeTracerStats(*store_.rootScope())), + client_(cluster_manager_.instance_, "fake_cluster", "test_host", stats_) { + url_.scheme = "http"; + url_.authority = "localhost:8126"; + url_.path = "/foo/bar"; + } + +protected: + InitializedMockClusterManager cluster_manager_; + int dummy_; + Http::MockAsyncClientRequest request_; + Stats::TestUtil::TestStore store_; + TracerStats stats_; + AgentHTTPClient client_; + datadog::tracing::HTTPClient::URL url_; + Http::AsyncClient::Callbacks* callbacks_; + testing::MockFunction + on_response_; + testing::MockFunction on_error_; +}; + +TEST_F(DatadogAgentHttpClientTest, PathFromURL) { // The `.path` portion of the `URL` argument to `AgentHTTPClient::post` ends // up as the "reference path" of the `Http::RequestHeaderMap`. // That is, the URL "http://foobar.com/trace/v04" results in "/trace/v04". - NiceMock cluster_manager; - cluster_manager.initializeClusters({"fake_cluster"}, {}); - cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; - cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); - Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); - Stats::TestUtil::TestStore store; - TracerStats stats = makeTracerStats(*store.rootScope()); - AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); - datadog::tracing::HTTPClient::URL url; - url.scheme = "http"; - url.authority = "localhost:8126"; - url.path = "/foo/bar"; - - EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) + EXPECT_CALL(cluster_manager_.instance_.thread_local_cluster_.async_client_, send_(_, _, _)) .WillOnce( - Invoke([&](Http::RequestMessagePtr& message, Http::AsyncClient::Callbacks&, - const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { - EXPECT_EQ(url.path, message->headers().path()); - return &request; + Invoke([this](Http::RequestMessagePtr& message, Http::AsyncClient::Callbacks&, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + EXPECT_EQ(url_.path, message->headers().path()); + return &request_; })); // `~AgentHTTPClient()` will cancel the request since we don't finish it here. - EXPECT_CALL(request, cancel()); + EXPECT_CALL(request_, cancel()); const auto ignore = [](auto&&...) {}; - datadog::tracing::Expected result = client.post(url, ignore, "", ignore, ignore); + datadog::tracing::Expected result = client_.post(url_, ignore, "", ignore, ignore); EXPECT_TRUE(result) << result.error(); - EXPECT_EQ(0, stats.reports_skipped_no_cluster_.value()); - EXPECT_EQ(0, stats.reports_failed_.value()); + EXPECT_EQ(0, stats_.reports_skipped_no_cluster_.value()); + EXPECT_EQ(0, stats_.reports_failed_.value()); } -TEST(DatadogAgentHttpClientTest, MissingThreadLocalCluster) { +TEST_F(DatadogAgentHttpClientTest, MissingThreadLocalCluster) { // If ...`threadLocalCluster().has_value()` is false, then `post` cannot // create a request and so will immediately return successfully but increment // the "reports skipped no cluster" counter. NiceMock cluster_manager; - Stats::TestUtil::TestStore store; - TracerStats stats = makeTracerStats(*store.rootScope()); - AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); - datadog::tracing::HTTPClient::URL url; - url.scheme = "http"; - url.authority = "localhost:8126"; - url.path = "/foo/bar"; + AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats_); const auto ignore = [](auto&&...) {}; - datadog::tracing::Expected result = client.post(url, ignore, "", ignore, ignore); + datadog::tracing::Expected result = client.post(url_, ignore, "", ignore, ignore); EXPECT_TRUE(result) << result.error(); - EXPECT_EQ(1, stats.reports_skipped_no_cluster_.value()); - EXPECT_EQ(0, stats.reports_failed_.value()); + EXPECT_EQ(1, stats_.reports_skipped_no_cluster_.value()); + EXPECT_EQ(0, stats_.reports_failed_.value()); } -TEST(DatadogAgentHttpClientTest, RequestHeaders) { +TEST_F(DatadogAgentHttpClientTest, RequestHeaders) { // The `set_headers` argument to `post(...)` results in the corresponding // headers being set in `Http::RequestMessage::headers()`. // Additionally, the "Host" header will always be the same as the // corresponding parameter of `AgentHTTPClient`'s constructor. - NiceMock cluster_manager; - cluster_manager.initializeClusters({"fake_cluster"}, {}); - cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; - cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); - Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); - Stats::TestUtil::TestStore store; - TracerStats stats = makeTracerStats(*store.rootScope()); - AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); - datadog::tracing::HTTPClient::URL url; - url.scheme = "http"; - url.authority = "localhost:8126"; - url.path = "/foo/bar"; const auto set_headers = [&](datadog::tracing::DictWriter& headers) { headers.set("foo", "bar"); headers.set("baz-boing", "boing boing"); @@ -107,173 +112,133 @@ TEST(DatadogAgentHttpClientTest, RequestHeaders) { headers.set("boing-boing", "boing boing boing"); }; - EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) + EXPECT_CALL(cluster_manager_.instance_.thread_local_cluster_.async_client_, send_(_, _, _)) .WillOnce( - Invoke([&](Http::RequestMessagePtr& message, Http::AsyncClient::Callbacks&, - const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + Invoke([this](Http::RequestMessagePtr& message, Http::AsyncClient::Callbacks&, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { EXPECT_EQ("test_host", message->headers().getHostValue()); EXPECT_EQ("bar", message->headers().getByKey("foo")); EXPECT_EQ("boing boing", message->headers().getByKey("baz-boing")); EXPECT_EQ("boing boing boing", message->headers().getByKey("boing-boing")); - return &request; + return &request_; })); // `~AgentHTTPClient()` will cancel the request since we don't finish it here. - EXPECT_CALL(request, cancel()); + EXPECT_CALL(request_, cancel()); const auto ignore = [](auto&&...) {}; - datadog::tracing::Expected result = client.post(url, set_headers, "", ignore, ignore); + datadog::tracing::Expected result = client_.post(url_, set_headers, "", ignore, ignore); EXPECT_TRUE(result) << result.error(); - EXPECT_EQ(0, stats.reports_skipped_no_cluster_.value()); - EXPECT_EQ(0, stats.reports_failed_.value()); + EXPECT_EQ(0, stats_.reports_skipped_no_cluster_.value()); + EXPECT_EQ(0, stats_.reports_failed_.value()); } -TEST(DatadogAgentHttpClientTest, RequestBody) { +TEST_F(DatadogAgentHttpClientTest, RequestBody) { // The `body` parameter to `AgentHTTPClient::post` corresponds to the // resulting `Http::RequestMessage::body()`. - NiceMock cluster_manager; - cluster_manager.initializeClusters({"fake_cluster"}, {}); - cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; - cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); - Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); - Stats::TestUtil::TestStore store; - TracerStats stats = makeTracerStats(*store.rootScope()); - AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); - datadog::tracing::HTTPClient::URL url; - url.scheme = "http"; - url.authority = "localhost:8126"; - url.path = "/foo/bar"; - const std::string body = R"latin( - Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod - tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, - quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo - consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse - cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat - non proident, sunt in culpa qui officia deserunt mollit anim id est - laborum.)latin"; - - EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) - .WillOnce( - Invoke([&](Http::RequestMessagePtr& message, Http::AsyncClient::Callbacks&, - const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + const std::string body = R"body( + Butterfly in the sky + I can go twice as high + Take a look + It's in a book + A reading rainbow + + I can go anywhere + Friends to know + And ways to grow + A reading rainbow + + I can be anything + Take a look + It's in a book + A reading rainbow)body"; + + EXPECT_CALL(cluster_manager_.instance_.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce(Invoke( + [this, &body](Http::RequestMessagePtr& message, Http::AsyncClient::Callbacks&, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { EXPECT_EQ(body, message->body().toString()); - return &request; + return &request_; })); // `~AgentHTTPClient()` will cancel the request since we don't finish it here. - EXPECT_CALL(request, cancel()); + EXPECT_CALL(request_, cancel()); const auto ignore = [](auto&&...) {}; - datadog::tracing::Expected result = client.post(url, ignore, body, ignore, ignore); + datadog::tracing::Expected result = client_.post(url_, ignore, body, ignore, ignore); EXPECT_TRUE(result) << result.error(); - EXPECT_EQ(0, stats.reports_skipped_no_cluster_.value()); - EXPECT_EQ(0, stats.reports_failed_.value()); + EXPECT_EQ(0, stats_.reports_skipped_no_cluster_.value()); + EXPECT_EQ(0, stats_.reports_failed_.value()); } -TEST(DatadogAgentHttpClientTest, OnResponse200) { +TEST_F(DatadogAgentHttpClientTest, OnResponse200) { // When `onSuccess` is invoked on the `Http::AsyncClient::Callbacks`, the // associated `on_response` callback is invoked with corresponding arguments. // Additionally, if the HTTP response status is 200, `stats_.reports_sent_` is // incremented. - NiceMock cluster_manager; - cluster_manager.initializeClusters({"fake_cluster"}, {}); - cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; - cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); - Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); - Stats::TestUtil::TestStore store; - TracerStats stats = makeTracerStats(*store.rootScope()); - AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); - datadog::tracing::HTTPClient::URL url; - url.scheme = "http"; - url.authority = "localhost:8126"; - url.path = "/foo/bar"; - Http::AsyncClient::Callbacks* callbacks; - testing::MockFunction - on_response; - testing::MockFunction on_error; - - EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) + EXPECT_CALL(cluster_manager_.instance_.thread_local_cluster_.async_client_, send_(_, _, _)) .WillOnce( - Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, - const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { - callbacks = &callbacks_arg; - return &request; + Invoke([this](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks_ = &callbacks_arg; + return &request_; })); - // `callbacks->onSuccess(...)` will cause `on_response` to be called. - // `on_error` will not be called. - EXPECT_CALL(on_response, Call(200, _, "{}")); - EXPECT_CALL(on_error, Call(_)).Times(0); + // `callbacks_->onSuccess(...)` will cause `on_response_` to be called. + // `on_error_` will not be called. + EXPECT_CALL(on_response_, Call(200, _, "{}")); + EXPECT_CALL(on_error_, Call(_)).Times(0); // The request will not be canceled; neither explicitly nor in // `~AgentHTTPClient`, because it will have been successfully fulfilled. - EXPECT_CALL(request, cancel()).Times(0); + EXPECT_CALL(request_, cancel()).Times(0); const auto ignore = [](auto&&...) {}; datadog::tracing::Expected result = - client.post(url, ignore, "{}", on_response.AsStdFunction(), on_error.AsStdFunction()); + client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction()); EXPECT_TRUE(result) << result.error(); Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "200"}}})); msg->body().add("{}"); - callbacks->onSuccess(request, std::move(msg)); - EXPECT_EQ(1, stats.reports_sent_.value()); - EXPECT_EQ(0, stats.reports_failed_.value()); - EXPECT_EQ(0, stats.reports_skipped_no_cluster_.value()); + callbacks_->onSuccess(request_, std::move(msg)); + EXPECT_EQ(1, stats_.reports_sent_.value()); + EXPECT_EQ(0, stats_.reports_failed_.value()); + EXPECT_EQ(0, stats_.reports_skipped_no_cluster_.value()); } -TEST(DatadogAgentHttpClientTest, OnResponseNot200) { +TEST_F(DatadogAgentHttpClientTest, OnResponseNot200) { // When `onSuccess` is invoked on the `Http::AsyncClient::Callbacks`, the // associated `on_response` callback is invoked with corresponding arguments. // Additionally, if the HTTP response status is not 200, // `stats_.reports_dropped_` is incremented. - NiceMock cluster_manager; - cluster_manager.initializeClusters({"fake_cluster"}, {}); - cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; - cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); - Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); - Stats::TestUtil::TestStore store; - TracerStats stats = makeTracerStats(*store.rootScope()); - AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); - datadog::tracing::HTTPClient::URL url; - url.scheme = "http"; - url.authority = "localhost:8126"; - url.path = "/foo/bar"; - Http::AsyncClient::Callbacks* callbacks; - testing::MockFunction - on_response; - testing::MockFunction on_error; - - EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) + EXPECT_CALL(cluster_manager_.instance_.thread_local_cluster_.async_client_, send_(_, _, _)) .WillOnce( - Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, - const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { - callbacks = &callbacks_arg; - return &request; + Invoke([this](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks_ = &callbacks_arg; + return &request_; })); - // `callbacks->onSuccess(...)` will cause `on_response` to be called. + // `callbacks_->onSuccess(...)` will cause `on_response_` to be called. // The `404` value corresponds to the response sent below. - // `on_error` will not be called. - EXPECT_CALL(on_response, Call(404, _, "{}")); - EXPECT_CALL(on_error, Call(_)).Times(0); + // `on_error_` will not be called. + EXPECT_CALL(on_response_, Call(404, _, "{}")); + EXPECT_CALL(on_error_, Call(_)).Times(0); // The request will not be canceled; neither explicitly nor in // `~AgentHTTPClient`, because it will have been successfully fulfilled. - EXPECT_CALL(request, cancel()).Times(0); + EXPECT_CALL(request_, cancel()).Times(0); const auto ignore = [](auto&&...) {}; datadog::tracing::Expected result = - client.post(url, ignore, "{}", on_response.AsStdFunction(), on_error.AsStdFunction()); + client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction()); EXPECT_TRUE(result) << result.error(); // The "404" below is what causes `stats.reports_failed_` to be incremented @@ -282,57 +247,39 @@ TEST(DatadogAgentHttpClientTest, OnResponseNot200) { Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "404"}}})); msg->body().add("{}"); - callbacks->onSuccess(request, std::move(msg)); - EXPECT_EQ(1, stats.reports_dropped_.value()); - EXPECT_EQ(0, stats.reports_sent_.value()); - EXPECT_EQ(0, stats.reports_failed_.value()); - EXPECT_EQ(0, stats.reports_skipped_no_cluster_.value()); + callbacks_->onSuccess(request_, std::move(msg)); + EXPECT_EQ(1, stats_.reports_dropped_.value()); + EXPECT_EQ(0, stats_.reports_sent_.value()); + EXPECT_EQ(0, stats_.reports_failed_.value()); + EXPECT_EQ(0, stats_.reports_skipped_no_cluster_.value()); } -TEST(DatadogAgentHttpClientTest, OnResponseBogusRequest) { +TEST_F(DatadogAgentHttpClientTest, OnResponseBogusRequest) { // When `onSuccess` is invoked on the `Http::AsyncClient::Callbacks` with a // request that is not registered with the HTTP client, then no callback is // invoked (how would we look it up?). - NiceMock cluster_manager; - cluster_manager.initializeClusters({"fake_cluster"}, {}); - cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; - cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); - Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); - Stats::TestUtil::TestStore store; - TracerStats stats = makeTracerStats(*store.rootScope()); - AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); - datadog::tracing::HTTPClient::URL url; - url.scheme = "http"; - url.authority = "localhost:8126"; - url.path = "/foo/bar"; - Http::AsyncClient::Callbacks* callbacks; - testing::MockFunction - on_response; - testing::MockFunction on_error; - - EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) + EXPECT_CALL(cluster_manager_.instance_.thread_local_cluster_.async_client_, send_(_, _, _)) .WillOnce( - Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, - const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { - callbacks = &callbacks_arg; - return &request; + Invoke([this](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks_ = &callbacks_arg; + return &request_; })); - // `callbacks->onSuccess(...)` will not invoke any callbacks, because the + // `callbacks_->onSuccess(...)` will not invoke any callbacks, because the // request argument passed in is not registered with the HTTP client. - EXPECT_CALL(on_response, Call(_, _, _)).Times(0); - EXPECT_CALL(on_error, Call(_)).Times(0); + EXPECT_CALL(on_response_, Call(_, _, _)).Times(0); + EXPECT_CALL(on_error_, Call(_)).Times(0); // The request will will canceled by `~AgentHTTPClient` because `onSuccess` // was passed the wrong request, and so the real request is never removed from // the HTTP client's registry. - EXPECT_CALL(request, cancel()); + EXPECT_CALL(request_, cancel()); const auto ignore = [](auto&&...) {}; datadog::tracing::Expected result = - client.post(url, ignore, "{}", on_response.AsStdFunction(), on_error.AsStdFunction()); + client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction()); EXPECT_TRUE(result) << result.error(); Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( @@ -341,109 +288,74 @@ TEST(DatadogAgentHttpClientTest, OnResponseBogusRequest) { // The first argument to `onSuccess` should be `request`, but instead we pass // `bogus_request`. - Http::MockAsyncClientRequest bogus_request(&cluster_manager.thread_local_cluster_.async_client_); - callbacks->onSuccess(bogus_request, std::move(msg)); + Http::MockAsyncClientRequest bogus_request( + &cluster_manager_.instance_.thread_local_cluster_.async_client_); + callbacks_->onSuccess(bogus_request, std::move(msg)); } -TEST(DatadogAgentHttpClientTest, OnErrorStreamReset) { +TEST_F(DatadogAgentHttpClientTest, OnErrorStreamReset) { // When `onFailure` is invoked on the `Http::AsyncClient::Callbacks` with // `FailureReason::Reset`, the associated `on_error` callback is invoked with // a corresponding `datadog::tracing::Error`. - NiceMock cluster_manager; - cluster_manager.initializeClusters({"fake_cluster"}, {}); - cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; - cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); - Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); - Stats::TestUtil::TestStore store; - TracerStats stats = makeTracerStats(*store.rootScope()); - AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); - datadog::tracing::HTTPClient::URL url; - url.scheme = "http"; - url.authority = "localhost:8126"; - url.path = "/foo/bar"; - Http::AsyncClient::Callbacks* callbacks; - testing::MockFunction - on_response; - testing::MockFunction on_error; - - EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) + EXPECT_CALL(cluster_manager_.instance_.thread_local_cluster_.async_client_, send_(_, _, _)) .WillOnce( - Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, - const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { - callbacks = &callbacks_arg; - return &request; + Invoke([this](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks_ = &callbacks_arg; + return &request_; })); - // `callbacks->onFailure(...)` will cause `on_error` to be called. - // `on_response` will not be called. - EXPECT_CALL(on_error, Call(_)).WillOnce(Invoke([](datadog::tracing::Error error) { + // `callbacks_->onFailure(...)` will cause `on_error_` to be called. + // `on_response_` will not be called. + EXPECT_CALL(on_error_, Call(_)).WillOnce(Invoke([](datadog::tracing::Error error) { EXPECT_EQ(error.code, datadog::tracing::Error::ENVOY_HTTP_CLIENT_FAILURE); })); - EXPECT_CALL(on_response, Call(_, _, _)).Times(0); + EXPECT_CALL(on_response_, Call(_, _, _)).Times(0); // The request will not be canceled; neither explicitly nor in // `~AgentHTTPClient`, because it will have been successfully fulfilled. - EXPECT_CALL(request, cancel()).Times(0); + EXPECT_CALL(request_, cancel()).Times(0); const auto ignore = [](auto&&...) {}; datadog::tracing::Expected result = - client.post(url, ignore, "{}", on_response.AsStdFunction(), on_error.AsStdFunction()); + client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction()); EXPECT_TRUE(result) << result.error(); Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "200"}}})); msg->body().add("{}"); - callbacks->onFailure(request, Http::AsyncClient::FailureReason::Reset); + callbacks_->onFailure(request_, Http::AsyncClient::FailureReason::Reset); } -TEST(DatadogAgentHttpClientTest, OnErrorOther) { +TEST_F(DatadogAgentHttpClientTest, OnErrorOther) { // When `onFailure` is invoked on the `Http::AsyncClient::Callbacks` with any // value other than `FailureReason::Reset`, the associated `on_error` callback // is invoked with a corresponding `datadog::tracing::Error`. - NiceMock cluster_manager; - cluster_manager.initializeClusters({"fake_cluster"}, {}); - cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; - cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); - Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); - Stats::TestUtil::TestStore store; - TracerStats stats = makeTracerStats(*store.rootScope()); - AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); - datadog::tracing::HTTPClient::URL url; - url.scheme = "http"; - url.authority = "localhost:8126"; - url.path = "/foo/bar"; - Http::AsyncClient::Callbacks* callbacks; - testing::MockFunction - on_response; - testing::MockFunction on_error; - - EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) + EXPECT_CALL(cluster_manager_.instance_.thread_local_cluster_.async_client_, send_(_, _, _)) .WillOnce( - Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, - const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { - callbacks = &callbacks_arg; - return &request; + Invoke([this](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks_ = &callbacks_arg; + return &request_; })); - // `callbacks->onFailure(...)` will cause `on_error` to be called. - // `on_response` will not be called. - EXPECT_CALL(on_error, Call(_)).WillOnce(Invoke([](datadog::tracing::Error error) { + // `callbacks->onFailure(...)` will cause `on_error_` to be called. + // `on_response_` will not be called. + EXPECT_CALL(on_error_, Call(_)).WillOnce(Invoke([](datadog::tracing::Error error) { EXPECT_EQ(error.code, datadog::tracing::Error::ENVOY_HTTP_CLIENT_FAILURE); })); - EXPECT_CALL(on_response, Call(_, _, _)).Times(0); + EXPECT_CALL(on_response_, Call(_, _, _)).Times(0); // The request will not be canceled; neither explicitly nor in // `~AgentHTTPClient`, because it will have been successfully fulfilled. - EXPECT_CALL(request, cancel()).Times(0); + EXPECT_CALL(request_, cancel()).Times(0); const auto ignore = [](auto&&...) {}; datadog::tracing::Expected result = - client.post(url, ignore, "{}", on_response.AsStdFunction(), on_error.AsStdFunction()); + client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction()); EXPECT_TRUE(result) << result.error(); Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( @@ -451,71 +363,41 @@ TEST(DatadogAgentHttpClientTest, OnErrorOther) { msg->body().add("{}"); const auto bogus_value = static_cast(-1); - callbacks->onFailure(request, bogus_value); + callbacks_->onFailure(request_, bogus_value); } -TEST(DatadogAgentHttpClientTest, SendFailReturnsError) { +TEST_F(DatadogAgentHttpClientTest, SendFailReturnsError) { // If the underlying call to `httpAsyncClient().send(...)` returns an error, // then the enclosing call to `AgentHTTPClient::post(...)` returns an error. - NiceMock cluster_manager; - cluster_manager.initializeClusters({"fake_cluster"}, {}); - cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; - cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); - Http::MockAsyncClientRequest request(&cluster_manager.thread_local_cluster_.async_client_); - Stats::TestUtil::TestStore store; - TracerStats stats = makeTracerStats(*store.rootScope()); - AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); - datadog::tracing::HTTPClient::URL url; - url.scheme = "http"; - url.authority = "localhost:8126"; - url.path = "/foo/bar"; - Http::AsyncClient::Callbacks* callbacks; - - EXPECT_CALL(cluster_manager.thread_local_cluster_.async_client_, send_(_, _, _)) + EXPECT_CALL(cluster_manager_.instance_.thread_local_cluster_.async_client_, send_(_, _, _)) .WillOnce( - Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, - const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { - callbacks = &callbacks_arg; + Invoke([this](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks_ = &callbacks_arg; return nullptr; // indicates error })); const auto ignore = [](auto&&...) {}; - datadog::tracing::Expected result = client.post(url, ignore, "", ignore, ignore); + datadog::tracing::Expected result = client_.post(url_, ignore, "", ignore, ignore); ASSERT_FALSE(result); EXPECT_EQ(datadog::tracing::Error::ENVOY_HTTP_CLIENT_FAILURE, result.error().code); - EXPECT_EQ(1, stats.reports_failed_.value()); - EXPECT_EQ(0, stats.reports_skipped_no_cluster_.value()); + EXPECT_EQ(1, stats_.reports_failed_.value()); + EXPECT_EQ(0, stats_.reports_skipped_no_cluster_.value()); } -TEST(DatadogAgentHttpClientTest, DrainIsANoOp) { +TEST_F(DatadogAgentHttpClientTest, DrainIsANoOp) { // `AgentHTTPClient::drain` doesn't do anything. It only makes sense in // multi-threaded contexts. // This test is for the sake of coverage. - NiceMock cluster_manager; - cluster_manager.initializeClusters({"fake_cluster"}, {}); - cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; - cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); - Stats::TestUtil::TestStore store; - TracerStats stats = makeTracerStats(*store.rootScope()); - AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); - // `deadline` value doesn't matter; `drain` ignores it. const auto deadline = std::chrono::steady_clock::time_point::min(); - client.drain(deadline); + client_.drain(deadline); } -TEST(DatadogAgentHttpClientTest, ConfigJSONContainsTypeName) { - NiceMock cluster_manager; - cluster_manager.initializeClusters({"fake_cluster"}, {}); - cluster_manager.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; - cluster_manager.initializeThreadLocalClusters({"fake_cluster"}); - Stats::TestUtil::TestStore store; - TracerStats stats = makeTracerStats(*store.rootScope()); - AgentHTTPClient client(cluster_manager, "fake_cluster", "test_host", stats); - - nlohmann::json config = client.config_json(); +TEST_F(DatadogAgentHttpClientTest, ConfigJSONContainsTypeName) { + nlohmann::json config = client_.config_json(); EXPECT_EQ("Envoy::Extensions::Tracers::Datadog::AgentHTTPClient", config["type"]); } From 52cdeee708cb496341d402968106572ca3d9dca6 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Tue, 7 Feb 2023 21:25:13 -0500 Subject: [PATCH 17/20] datadog: more unit tests Signed-off-by: David Goffredo --- .../tracers/datadog/agent_http_client_test.cc | 50 +++++++++++++++++-- 1 file changed, 47 insertions(+), 3 deletions(-) diff --git a/test/extensions/tracers/datadog/agent_http_client_test.cc b/test/extensions/tracers/datadog/agent_http_client_test.cc index 64c76ab120adc..412b5fc7b8d89 100644 --- a/test/extensions/tracers/datadog/agent_http_client_test.cc +++ b/test/extensions/tracers/datadog/agent_http_client_test.cc @@ -3,6 +3,7 @@ #include "envoy/http/header_map.h" #include "source/common/http/message_impl.h" +#include "source/common/tracing/null_span_impl.h" #include "source/extensions/tracers/datadog/agent_http_client.h" #include "source/extensions/tracers/datadog/dict_util.h" #include "source/extensions/tracers/datadog/tracer_stats.h" @@ -286,7 +287,7 @@ TEST_F(DatadogAgentHttpClientTest, OnResponseBogusRequest) { Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "200"}}})); msg->body().add("{}"); - // The first argument to `onSuccess` should be `request`, but instead we pass + // The first argument to `onSuccess` should be `request_`, but instead we pass // `bogus_request`. Http::MockAsyncClientRequest bogus_request( &cluster_manager_.instance_.thread_local_cluster_.async_client_); @@ -314,7 +315,7 @@ TEST_F(DatadogAgentHttpClientTest, OnErrorStreamReset) { EXPECT_CALL(on_response_, Call(_, _, _)).Times(0); // The request will not be canceled; neither explicitly nor in - // `~AgentHTTPClient`, because it will have been successfully fulfilled. + // `~AgentHTTPClient`, because it will have been fulfilled. EXPECT_CALL(request_, cancel()).Times(0); const auto ignore = [](auto&&...) {}; @@ -350,7 +351,7 @@ TEST_F(DatadogAgentHttpClientTest, OnErrorOther) { EXPECT_CALL(on_response_, Call(_, _, _)).Times(0); // The request will not be canceled; neither explicitly nor in - // `~AgentHTTPClient`, because it will have been successfully fulfilled. + // `~AgentHTTPClient`, because it will have been fulfilled. EXPECT_CALL(request_, cancel()).Times(0); const auto ignore = [](auto&&...) {}; @@ -366,6 +367,42 @@ TEST_F(DatadogAgentHttpClientTest, OnErrorOther) { callbacks_->onFailure(request_, bogus_value); } +TEST_F(DatadogAgentHttpClientTest, OnErrorBogusRequest) { + // When `onFailure` is invoked with a request that's not registered with the + // HTTP client, no callbacks are invoked and no stats are incremented. + + EXPECT_CALL(cluster_manager_.instance_.thread_local_cluster_.async_client_, send_(_, _, _)) + .WillOnce( + Invoke([this](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, + const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { + callbacks_ = &callbacks_arg; + return &request_; + })); + + EXPECT_CALL(on_error_, Call(_)).Times(0); + EXPECT_CALL(on_response_, Call(_, _, _)).Times(0); + + // The request will will canceled by `~AgentHTTPClient` because `onFailure` + // was passed the wrong request, and so the real request is never removed from + // the HTTP client's registry. + EXPECT_CALL(request_, cancel()); + + const auto ignore = [](auto&&...) {}; + datadog::tracing::Expected result = + client_.post(url_, ignore, "{}", on_response_.AsStdFunction(), on_error_.AsStdFunction()); + EXPECT_TRUE(result) << result.error(); + + Http::ResponseMessagePtr msg(new Http::ResponseMessageImpl( + Http::ResponseHeaderMapPtr{new Http::TestResponseHeaderMapImpl{{":status", "200"}}})); + msg->body().add("{}"); + + // The first argument to `onFailure` should be `request_`, but instead we pass + // `bogus_request`. + Http::MockAsyncClientRequest bogus_request( + &cluster_manager_.instance_.thread_local_cluster_.async_client_); + callbacks_->onFailure(bogus_request, Http::AsyncClient::FailureReason::Reset); +} + TEST_F(DatadogAgentHttpClientTest, SendFailReturnsError) { // If the underlying call to `httpAsyncClient().send(...)` returns an error, // then the enclosing call to `AgentHTTPClient::post(...)` returns an error. @@ -401,6 +438,13 @@ TEST_F(DatadogAgentHttpClientTest, ConfigJSONContainsTypeName) { EXPECT_EQ("Envoy::Extensions::Tracers::Datadog::AgentHTTPClient", config["type"]); } +TEST_F(DatadogAgentHttpClientTest, OnBeforeFinalizeUpstreamSpanIsANoOp) { + // `AgentHTTPClient::onBeforeFinalizeUpstreamSpan` doesn't do anything. + // This test is for the sake of coverage. + Tracing::NullSpan null_span; + client_.onBeforeFinalizeUpstreamSpan(null_span, nullptr); +} + } // namespace } // namespace Datadog } // namespace Tracers From 07d7d50f03b82a6c36fda2925c16b964be2b1869 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 8 Feb 2023 11:06:54 -0500 Subject: [PATCH 18/20] datadog: fix and comment and remove dead code Signed-off-by: David Goffredo --- test/extensions/tracers/datadog/agent_http_client_test.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/extensions/tracers/datadog/agent_http_client_test.cc b/test/extensions/tracers/datadog/agent_http_client_test.cc index 412b5fc7b8d89..0f1c41e4e2470 100644 --- a/test/extensions/tracers/datadog/agent_http_client_test.cc +++ b/test/extensions/tracers/datadog/agent_http_client_test.cc @@ -49,7 +49,6 @@ class DatadogAgentHttpClientTest : public testing::Test { protected: InitializedMockClusterManager cluster_manager_; - int dummy_; Http::MockAsyncClientRequest request_; Stats::TestUtil::TestStore store_; TracerStats stats_; @@ -369,7 +368,7 @@ TEST_F(DatadogAgentHttpClientTest, OnErrorOther) { TEST_F(DatadogAgentHttpClientTest, OnErrorBogusRequest) { // When `onFailure` is invoked with a request that's not registered with the - // HTTP client, no callbacks are invoked and no stats are incremented. + // HTTP client, no callbacks are invoked. EXPECT_CALL(cluster_manager_.instance_.thread_local_cluster_.async_client_, send_(_, _, _)) .WillOnce( From 9d8c148e72fa3beae74f18b3f2cf6dc0682137c4 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Tue, 14 Feb 2023 13:16:11 -0500 Subject: [PATCH 19/20] datadog: address review comments for AgentHTTPClient Signed-off-by: David Goffredo --- source/extensions/tracers/datadog/agent_http_client.cc | 5 +++-- source/extensions/tracers/datadog/agent_http_client.h | 4 ++-- test/extensions/tracers/datadog/agent_http_client_test.cc | 7 ++++++- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/source/extensions/tracers/datadog/agent_http_client.cc b/source/extensions/tracers/datadog/agent_http_client.cc index 4f65d9a4bb8b5..37a9fad328cac 100644 --- a/source/extensions/tracers/datadog/agent_http_client.cc +++ b/source/extensions/tracers/datadog/agent_http_client.cc @@ -1,11 +1,11 @@ #include "source/extensions/tracers/datadog/agent_http_client.h" -#include #include #include #include #include +#include "source/common/common/assert.h" #include "source/common/http/message_impl.h" #include "source/common/http/utility.h" #include "source/extensions/tracers/datadog/dict_util.h" @@ -30,7 +30,8 @@ AgentHTTPClient::AgentHTTPClient(Upstream::ClusterManager& cluster_manager, AgentHTTPClient::~AgentHTTPClient() { for (const auto& [request_ptr, _] : handlers_) { - assert(request_ptr); + RELEASE_ASSERT(request_ptr, + "null Http::AsyncClient::Request* in handler map of Datadog::AgentHTTPClient"); request_ptr->cancel(); } } diff --git a/source/extensions/tracers/datadog/agent_http_client.h b/source/extensions/tracers/datadog/agent_http_client.h index 2e38a938ab183..900a101abb528 100644 --- a/source/extensions/tracers/datadog/agent_http_client.h +++ b/source/extensions/tracers/datadog/agent_http_client.h @@ -95,8 +95,8 @@ class AgentHTTPClient : public datadog::tracing::HTTPClient, private: absl::flat_hash_map handlers_; Upstream::ClusterUpdateTracker collector_cluster_; - std::string cluster_; - std::string reference_host_; + const std::string cluster_; + const std::string reference_host_; TracerStats& stats_; }; diff --git a/test/extensions/tracers/datadog/agent_http_client_test.cc b/test/extensions/tracers/datadog/agent_http_client_test.cc index 0f1c41e4e2470..f68b8a019598a 100644 --- a/test/extensions/tracers/datadog/agent_http_client_test.cc +++ b/test/extensions/tracers/datadog/agent_http_client_test.cc @@ -414,8 +414,13 @@ TEST_F(DatadogAgentHttpClientTest, SendFailReturnsError) { return nullptr; // indicates error })); + // Neither callback will be invoked, because `post` fails immediately (synchronously). + EXPECT_CALL(on_error_, Call(_)).Times(0); + EXPECT_CALL(on_response_, Call(_, _, _)).Times(0); + const auto ignore = [](auto&&...) {}; - datadog::tracing::Expected result = client_.post(url_, ignore, "", ignore, ignore); + datadog::tracing::Expected result = + client_.post(url_, ignore, "", on_response_.AsStdFunction(), on_error_.AsStdFunction()); ASSERT_FALSE(result); EXPECT_EQ(datadog::tracing::Error::ENVOY_HTTP_CLIENT_FAILURE, result.error().code); EXPECT_EQ(1, stats_.reports_failed_.value()); From fe7826ed1968579fd8dc4a4b13a0379a10614c62 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Fri, 17 Feb 2023 15:23:28 -0500 Subject: [PATCH 20/20] datadog: replicate request failure behavior in unit test Signed-off-by: David Goffredo --- .../tracers/datadog/agent_http_client_test.cc | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/extensions/tracers/datadog/agent_http_client_test.cc b/test/extensions/tracers/datadog/agent_http_client_test.cc index f68b8a019598a..3791d89b9cc9d 100644 --- a/test/extensions/tracers/datadog/agent_http_client_test.cc +++ b/test/extensions/tracers/datadog/agent_http_client_test.cc @@ -411,6 +411,17 @@ TEST_F(DatadogAgentHttpClientTest, SendFailReturnsError) { Invoke([this](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& callbacks_arg, const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { callbacks_ = &callbacks_arg; + // As of this writing, any time that `send` returns `nullptr`, + // `onSuccess` will also be called with a status of 503, even though + // no request was sent and so no response was received. + // `AgentHTTPClient` does not depend on this behavior, but we + // reproduce it here for authenticity. + // The relevant branch in `AgentHTTPClient::onSuccess` is the one + // where `handlers_.find` returns `handlers.end()`. + Http::ResponseMessagePtr response( + new Http::ResponseMessageImpl(Http::ResponseHeaderMapPtr{ + new Http::TestResponseHeaderMapImpl{{":status", "503"}}})); + callbacks_arg.onSuccess(request_, std::move(response)); return nullptr; // indicates error }));