From e828a07ba09d6e347a87bdf3a3eae315c3405e85 Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Tue, 21 Apr 2020 14:51:59 -0700 Subject: [PATCH 1/5] Improve test coverage Signed-off-by: Marco Magdy --- .../extensions/tracers/xray/daemon_broker.cc | 18 +++++---------- .../tracers/xray/localized_sampling.h | 6 ----- source/extensions/tracers/xray/tracer.cc | 22 +++++++------------ source/extensions/tracers/xray/tracer.h | 2 +- test/extensions/tracers/xray/tracer_test.cc | 16 +++++++++++--- 5 files changed, 28 insertions(+), 36 deletions(-) diff --git a/source/extensions/tracers/xray/daemon_broker.cc b/source/extensions/tracers/xray/daemon_broker.cc index d5667c423aa8..742382957a08 100644 --- a/source/extensions/tracers/xray/daemon_broker.cc +++ b/source/extensions/tracers/xray/daemon_broker.cc @@ -4,6 +4,7 @@ #include "common/buffer/buffer_impl.h" #include "common/network/utility.h" +#include "common/protobuf/utility.h" #include "source/extensions/tracers/xray/daemon.pb.h" @@ -20,13 +21,8 @@ std::string createHeader(const std::string& format, uint32_t version) { source::extensions::tracers::xray::daemon::Header header; header.set_format(format); header.set_version(version); - - Protobuf::util::JsonPrintOptions json_options; - json_options.preserve_proto_field_names = true; - std::string json; - const auto status = Protobuf::util::MessageToJsonString(header, &json, json_options); - ASSERT(status.ok()); - return json; + return MessageUtil::getJsonStringFromMessage(header, false /* pretty_print */, + false /* always_print_primitive_fields */); } } // namespace @@ -40,11 +36,9 @@ void DaemonBrokerImpl::send(const std::string& data) const { constexpr auto version = 1; constexpr auto format = "json"; const std::string payload = absl::StrCat(createHeader(format, version), "\n", data); - Buffer::RawSlice buf; - buf.mem_ = const_cast(payload.data()); - buf.len_ = payload.length(); - const auto rc = Network::Utility::writeToSocket(*io_handle_, &buf, 1 /*num_slices*/, - nullptr /*local_ip*/, *address_); + Buffer::OwnedImpl buf(payload); + const auto rc = + Network::Utility::writeToSocket(*io_handle_, buf, nullptr /*local_ip*/, *address_); if (rc.rc_ != payload.length()) { // TODO(marcomagdy): report this in stats diff --git a/source/extensions/tracers/xray/localized_sampling.h b/source/extensions/tracers/xray/localized_sampling.h index 709ec144a32b..aefcd795b058 100644 --- a/source/extensions/tracers/xray/localized_sampling.h +++ b/source/extensions/tracers/xray/localized_sampling.h @@ -74,13 +74,7 @@ class LocalizedSamplingRule { * Set the percentage of requests to sample _after_ sampling |fixed_target| requests per second. */ void setRate(double rate) { rate_ = rate; } - - const std::string& host() const { return host_; } - const std::string& httpMethod() const { return http_method_; } - const std::string& urlPath() const { return url_path_; } - uint32_t fixedTarget() const { return fixed_target_; } double rate() const { return rate_; } - const Reservoir& reservoir() const { return reservoir_; } Reservoir& reservoir() { return reservoir_; } private: diff --git a/source/extensions/tracers/xray/tracer.cc b/source/extensions/tracers/xray/tracer.cc index 1d4768fcc025..d28fd8ff3066 100644 --- a/source/extensions/tracers/xray/tracer.cc +++ b/source/extensions/tracers/xray/tracer.cc @@ -71,31 +71,25 @@ void Span::finishSpan() { daemon::Segment s; s.set_name(name()); - s.set_id(Id()); + s.set_id(id()); s.set_trace_id(traceId()); s.set_start_time(time_point_cast(startTime()).time_since_epoch().count()); s.set_end_time( time_point_cast(time_source_.systemTime()).time_since_epoch().count()); s.set_parent_id(parentId()); - // HTTP annotations - using StructField = Protobuf::MapPair; - - ProtobufWkt::Struct* request = s.mutable_http()->mutable_request(); - auto* request_fields = request->mutable_fields(); + auto* request_fields = s.mutable_http()->mutable_request()->mutable_fields(); for (const auto& field : http_request_annotations_) { - request_fields->insert(StructField{field.first, field.second}); + request_fields->insert({field.first, field.second}); } - ProtobufWkt::Struct* response = s.mutable_http()->mutable_response(); - auto* response_fields = response->mutable_fields(); + auto* response_fields = s.mutable_http()->mutable_response()->mutable_fields(); for (const auto& field : http_response_annotations_) { - response_fields->insert(StructField{field.first, field.second}); + response_fields->insert({field.first, field.second}); } - using KeyValue = Protobuf::Map::value_type; for (const auto& item : custom_annotations_) { - s.mutable_annotations()->insert(KeyValue{item.first, item.second}); + s.mutable_annotations()->insert({item.first, item.second}); } const std::string json = MessageUtil::getJsonStringFromMessage( @@ -106,7 +100,7 @@ void Span::finishSpan() { void Span::injectContext(Http::RequestHeaderMap& request_headers) { const std::string xray_header_value = - fmt::format("Root={};Parent={};Sampled={}", traceId(), Id(), sampled() ? "1" : "0"); + fmt::format("Root={};Parent={};Sampled={}", traceId(), id(), sampled() ? "1" : "0"); request_headers.setCopy(Http::LowerCaseString(XRayTraceHeader), xray_header_value); } @@ -118,7 +112,7 @@ Tracing::SpanPtr Span::spawnChild(const Tracing::Config&, const std::string& ope child_span->setName(name()); child_span->setOperation(operation_name); child_span->setStartTime(start_time); - child_span->setParentId(Id()); + child_span->setParentId(id()); child_span->setTraceId(traceId()); child_span->setSampled(sampled()); return child_span; diff --git a/source/extensions/tracers/xray/tracer.h b/source/extensions/tracers/xray/tracer.h index cf7c977d8fbc..b0f92da9c941 100644 --- a/source/extensions/tracers/xray/tracer.h +++ b/source/extensions/tracers/xray/tracer.h @@ -113,7 +113,7 @@ class Span : public Tracing::Span, Logger::Loggable { /** * Gets this Span's ID. */ - const std::string& Id() const { return id_; } + const std::string& id() const { return id_; } const std::string& parentId() const { return parent_segment_id_; } diff --git a/test/extensions/tracers/xray/tracer_test.cc b/test/extensions/tracers/xray/tracer_test.cc index 023dd00a40ec..62a494e06705 100644 --- a/test/extensions/tracers/xray/tracer_test.cc +++ b/test/extensions/tracers/xray/tracer_test.cc @@ -108,15 +108,15 @@ TEST_F(XRayTracerTest, ChildSpanHasParentInfo) { absl::nullopt /*headers*/); const XRay::Span* xray_parent_span = static_cast(parent_span.get()); - const std::string expected_parent_id = xray_parent_span->Id(); - auto on_send = [&](const std::string& json) { + const std::string expected_parent_id = xray_parent_span->id(); + auto on_send = [xray_parent_span, expected_parent_id](const std::string& json) { ASSERT_FALSE(json.empty()); daemon::Segment s; MessageUtil::loadFromJson(json, s, ProtobufMessage::getNullValidationVisitor()); ASSERT_STREQ(expected_parent_id.c_str(), s.parent_id().c_str()); ASSERT_STREQ(expected_span_name, s.name().c_str()); ASSERT_STREQ(xray_parent_span->traceId().c_str(), s.trace_id().c_str()); - ASSERT_STRNE(xray_parent_span->Id().c_str(), s.id().c_str()); + ASSERT_STRNE(xray_parent_span->id().c_str(), s.id().c_str()); }; EXPECT_CALL(broker, send(_)).WillOnce(Invoke(on_send)); @@ -183,6 +183,16 @@ TEST_F(XRayTracerTest, TraceIDFormatTest) { ASSERT_EQ(24, parts[2].length()); } +TEST(XRayDaemon, SendBytesOverUDP) { + NiceMock server; + DaemonBrokerPtr broker_ptr = std::make_unique("127.0.0.1:2000"); + Tracer tracer{"my_segment", std::move(broker_ptr), server.timeSource()}; + auto span = tracer.startSpan("ingress" /*operation name*/, server.timeSource().systemTime(), + absl::nullopt /*headers*/); + span->setTag("user_agent", "envoy"); + span->finishSpan(); +} + } // namespace } // namespace XRay } // namespace Tracers From c22e9c81154b9b1bc9f73e632edcadf9c5b38468 Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Wed, 22 Apr 2020 13:54:48 -0700 Subject: [PATCH 2/5] revert copying the payload into the buffer Signed-off-by: Marco Magdy --- source/extensions/tracers/xray/daemon_broker.cc | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/source/extensions/tracers/xray/daemon_broker.cc b/source/extensions/tracers/xray/daemon_broker.cc index 742382957a08..39d7de50ef99 100644 --- a/source/extensions/tracers/xray/daemon_broker.cc +++ b/source/extensions/tracers/xray/daemon_broker.cc @@ -36,9 +36,11 @@ void DaemonBrokerImpl::send(const std::string& data) const { constexpr auto version = 1; constexpr auto format = "json"; const std::string payload = absl::StrCat(createHeader(format, version), "\n", data); - Buffer::OwnedImpl buf(payload); - const auto rc = - Network::Utility::writeToSocket(*io_handle_, buf, nullptr /*local_ip*/, *address_); + Buffer::RawSlice buf; + buf.mem_ = const_cast(payload.data()); + buf.len_ = payload.length(); + const auto rc = Network::Utility::writeToSocket(*io_handle_, &buf, 1 /*num_slices*/, + nullptr /*local_ip*/, *address_); if (rc.rc_ != payload.length()) { // TODO(marcomagdy): report this in stats From 057e3825a7f4676f29e0292a01f114b2b410a8e1 Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Wed, 22 Apr 2020 18:39:46 -0700 Subject: [PATCH 3/5] Verify UDP packet contents Signed-off-by: Marco Magdy --- test/extensions/tracers/xray/BUILD | 1 + test/extensions/tracers/xray/tracer_test.cc | 35 ++++++++++++++++++--- test/test_common/network_utility.cc | 9 ++++-- test/test_common/network_utility.h | 3 ++ 4 files changed, 41 insertions(+), 7 deletions(-) diff --git a/test/extensions/tracers/xray/BUILD b/test/extensions/tracers/xray/BUILD index e00d7e395bb9..9bbb65815265 100644 --- a/test/extensions/tracers/xray/BUILD +++ b/test/extensions/tracers/xray/BUILD @@ -31,6 +31,7 @@ envoy_extension_cc_test( "//test/mocks/stats:stats_mocks", "//test/mocks/thread_local:thread_local_mocks", "//test/mocks/tracing:tracing_mocks", + "//test/test_common:network_utility_lib", "//test/test_common:simulated_time_system_lib", ], ) diff --git a/test/extensions/tracers/xray/tracer_test.cc b/test/extensions/tracers/xray/tracer_test.cc index 62a494e06705..4c356cca1cc3 100644 --- a/test/extensions/tracers/xray/tracer_test.cc +++ b/test/extensions/tracers/xray/tracer_test.cc @@ -12,6 +12,7 @@ #include "test/mocks/server/mocks.h" #include "test/mocks/tracing/mocks.h" +#include "test/test_common/network_utility.h" #include "absl/strings/str_format.h" #include "absl/strings/str_split.h" @@ -183,14 +184,40 @@ TEST_F(XRayTracerTest, TraceIDFormatTest) { ASSERT_EQ(24, parts[2].length()); } -TEST(XRayDaemon, SendBytesOverUDP) { +TEST(XRayDaemon, VerifyUdpPacketContents) { + constexpr auto daemon_endpoint = "127.0.0.1:2000"; NiceMock server; - DaemonBrokerPtr broker_ptr = std::make_unique("127.0.0.1:2000"); - Tracer tracer{"my_segment", std::move(broker_ptr), server.timeSource()}; + Network::Test::UdpSyncPeer xray_fake_daemon( + Network::Utility::parseInternetAddressAndPort(daemon_endpoint, false /*v6only*/)); + Tracer tracer{"my_segment", std::make_unique(daemon_endpoint), + server.timeSource()}; auto span = tracer.startSpan("ingress" /*operation name*/, server.timeSource().systemTime(), absl::nullopt /*headers*/); - span->setTag("user_agent", "envoy"); + + span->setTag("http.status_code", "202"); span->finishSpan(); + + Network::UdpRecvData datagram; + xray_fake_daemon.recv(datagram); + + const std::string header_json = R"EOF({"format":"json","version":1})EOF"; + // The UDP datagram contains two independent, consecutive JSON documents; a header and a body. + const std::string payload = datagram.buffer_->toString(); + // Make sure the payload has enough data. + ASSERT_GT(payload.length(), header_json.length()); + // Skip the header since we're only interested in the body. + const std::string body = payload.substr(header_json.length()); + + EXPECT_EQ(0, payload.find(header_json)); + + // Deserialize the body to verify it. + source::extensions::tracers::xray::daemon::Segment seg; + MessageUtil::loadFromJson(body, seg, ProtobufMessage::getNullValidationVisitor()); + EXPECT_STREQ("my_segment", seg.name().c_str()); + for (auto&& f : seg.http().request().fields()) { + // there should only be a single field + EXPECT_EQ(202, f.second.number_value()); + } } } // namespace diff --git a/test/test_common/network_utility.cc b/test/test_common/network_utility.cc index 1f40191733f1..0bd8fcb705f4 100644 --- a/test/test_common/network_utility.cc +++ b/test/test_common/network_utility.cc @@ -225,14 +225,16 @@ Api::IoCallUint64Result readFromSocket(IoHandle& handle, const Address::Instance MonotonicTime(std::chrono::seconds(0)), nullptr); } -UdpSyncPeer::UdpSyncPeer(Network::Address::IpVersion version) - : socket_( - std::make_unique(getCanonicalLoopbackAddress(version), nullptr, true)) { +UdpSyncPeer::UdpSyncPeer(Network::Address::InstanceConstSharedPtr address) + : socket_(std::make_unique(address, nullptr, true)) { RELEASE_ASSERT( Api::OsSysCallsSingleton::get().setsocketblocking(socket_->ioHandle().fd(), true).rc_ != -1, ""); } +UdpSyncPeer::UdpSyncPeer(Network::Address::IpVersion version) + : UdpSyncPeer(getCanonicalLoopbackAddress(version)) {} + void UdpSyncPeer::write(const std::string& buffer, const Network::Address::Instance& peer) { const auto rc = Network::Utility::writeToSocket(socket_->ioHandle(), Buffer::OwnedImpl(buffer), nullptr, peer); @@ -245,6 +247,7 @@ void UdpSyncPeer::recv(Network::UdpRecvData& datagram) { received_datagrams_); ASSERT_TRUE(rc.ok()); } + datagram = std::move(received_datagrams_.front()); received_datagrams_.pop_front(); } diff --git a/test/test_common/network_utility.h b/test/test_common/network_utility.h index 965ec8b6dfad..ee33c994b612 100644 --- a/test/test_common/network_utility.h +++ b/test/test_common/network_utility.h @@ -180,8 +180,11 @@ Api::IoCallUint64Result readFromSocket(IoHandle& handle, const Address::Instance */ class UdpSyncPeer { public: + // Create a peer that listens on the given version of the loopback address. UdpSyncPeer(Network::Address::IpVersion version); + // Create a peer that listens on the given address:port. + UdpSyncPeer(Network::Address::InstanceConstSharedPtr address); // Writer a datagram to a remote peer. void write(const std::string& buffer, const Network::Address::Instance& peer); From cfe071ed4b2726b41c5a2cabd42899c517378b37 Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Thu, 23 Apr 2020 11:24:26 -0700 Subject: [PATCH 4/5] Use dynamic port Signed-off-by: Marco Magdy --- test/extensions/tracers/xray/tracer_test.cc | 5 ++--- test/test_common/network_utility.cc | 9 +++------ test/test_common/network_utility.h | 3 --- 3 files changed, 5 insertions(+), 12 deletions(-) diff --git a/test/extensions/tracers/xray/tracer_test.cc b/test/extensions/tracers/xray/tracer_test.cc index 4c356cca1cc3..d28419ec4695 100644 --- a/test/extensions/tracers/xray/tracer_test.cc +++ b/test/extensions/tracers/xray/tracer_test.cc @@ -185,10 +185,9 @@ TEST_F(XRayTracerTest, TraceIDFormatTest) { } TEST(XRayDaemon, VerifyUdpPacketContents) { - constexpr auto daemon_endpoint = "127.0.0.1:2000"; NiceMock server; - Network::Test::UdpSyncPeer xray_fake_daemon( - Network::Utility::parseInternetAddressAndPort(daemon_endpoint, false /*v6only*/)); + Network::Test::UdpSyncPeer xray_fake_daemon(Network::Address::IpVersion::v4); + const std::string daemon_endpoint = xray_fake_daemon.localAddress()->asString(); Tracer tracer{"my_segment", std::make_unique(daemon_endpoint), server.timeSource()}; auto span = tracer.startSpan("ingress" /*operation name*/, server.timeSource().systemTime(), diff --git a/test/test_common/network_utility.cc b/test/test_common/network_utility.cc index 0bd8fcb705f4..1f40191733f1 100644 --- a/test/test_common/network_utility.cc +++ b/test/test_common/network_utility.cc @@ -225,16 +225,14 @@ Api::IoCallUint64Result readFromSocket(IoHandle& handle, const Address::Instance MonotonicTime(std::chrono::seconds(0)), nullptr); } -UdpSyncPeer::UdpSyncPeer(Network::Address::InstanceConstSharedPtr address) - : socket_(std::make_unique(address, nullptr, true)) { +UdpSyncPeer::UdpSyncPeer(Network::Address::IpVersion version) + : socket_( + std::make_unique(getCanonicalLoopbackAddress(version), nullptr, true)) { RELEASE_ASSERT( Api::OsSysCallsSingleton::get().setsocketblocking(socket_->ioHandle().fd(), true).rc_ != -1, ""); } -UdpSyncPeer::UdpSyncPeer(Network::Address::IpVersion version) - : UdpSyncPeer(getCanonicalLoopbackAddress(version)) {} - void UdpSyncPeer::write(const std::string& buffer, const Network::Address::Instance& peer) { const auto rc = Network::Utility::writeToSocket(socket_->ioHandle(), Buffer::OwnedImpl(buffer), nullptr, peer); @@ -247,7 +245,6 @@ void UdpSyncPeer::recv(Network::UdpRecvData& datagram) { received_datagrams_); ASSERT_TRUE(rc.ok()); } - datagram = std::move(received_datagrams_.front()); received_datagrams_.pop_front(); } diff --git a/test/test_common/network_utility.h b/test/test_common/network_utility.h index ee33c994b612..965ec8b6dfad 100644 --- a/test/test_common/network_utility.h +++ b/test/test_common/network_utility.h @@ -180,11 +180,8 @@ Api::IoCallUint64Result readFromSocket(IoHandle& handle, const Address::Instance */ class UdpSyncPeer { public: - // Create a peer that listens on the given version of the loopback address. UdpSyncPeer(Network::Address::IpVersion version); - // Create a peer that listens on the given address:port. - UdpSyncPeer(Network::Address::InstanceConstSharedPtr address); // Writer a datagram to a remote peer. void write(const std::string& buffer, const Network::Address::Instance& peer); From e5eb6a670c00f966bb34d3c52284cd82a958b2be Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Thu, 23 Apr 2020 13:52:03 -0700 Subject: [PATCH 5/5] Parameterize the test for IPv4 and IPv6 Signed-off-by: Marco Magdy --- test/extensions/tracers/xray/BUILD | 1 + test/extensions/tracers/xray/tracer_test.cc | 11 +++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/test/extensions/tracers/xray/BUILD b/test/extensions/tracers/xray/BUILD index 9bbb65815265..8d1d57c436be 100644 --- a/test/extensions/tracers/xray/BUILD +++ b/test/extensions/tracers/xray/BUILD @@ -31,6 +31,7 @@ envoy_extension_cc_test( "//test/mocks/stats:stats_mocks", "//test/mocks/thread_local:thread_local_mocks", "//test/mocks/tracing:tracing_mocks", + "//test/test_common:environment_lib", "//test/test_common:network_utility_lib", "//test/test_common:simulated_time_system_lib", ], diff --git a/test/extensions/tracers/xray/tracer_test.cc b/test/extensions/tracers/xray/tracer_test.cc index d28419ec4695..5323122f2e9e 100644 --- a/test/extensions/tracers/xray/tracer_test.cc +++ b/test/extensions/tracers/xray/tracer_test.cc @@ -12,6 +12,7 @@ #include "test/mocks/server/mocks.h" #include "test/mocks/tracing/mocks.h" +#include "test/test_common/environment.h" #include "test/test_common/network_utility.h" #include "absl/strings/str_format.h" @@ -184,9 +185,15 @@ TEST_F(XRayTracerTest, TraceIDFormatTest) { ASSERT_EQ(24, parts[2].length()); } -TEST(XRayDaemon, VerifyUdpPacketContents) { +class XRayDaemonTest : public testing::TestWithParam {}; + +INSTANTIATE_TEST_SUITE_P(IpVersions, XRayDaemonTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + +TEST_P(XRayDaemonTest, VerifyUdpPacketContents) { NiceMock server; - Network::Test::UdpSyncPeer xray_fake_daemon(Network::Address::IpVersion::v4); + Network::Test::UdpSyncPeer xray_fake_daemon(GetParam()); const std::string daemon_endpoint = xray_fake_daemon.localAddress()->asString(); Tracer tracer{"my_segment", std::make_unique(daemon_endpoint), server.timeSource()};