From 5dcfa7b44999f8493a343796ba9dc7194c9f05c4 Mon Sep 17 00:00:00 2001 From: Marco Magdy Date: Thu, 23 Apr 2020 15:33:08 -0700 Subject: [PATCH] tracer: Improve test coverage for x-ray (#10890) - Removed unused functions that were showing up untested - Renamed a function to match the style guide - Small refactor (now that I understand how to use buffers) - Added a test that exercises the sending bytes to the x-ray daemon Signed-off-by: Marco Magdy --- .../extensions/tracers/xray/daemon_broker.cc | 10 ++-- .../tracers/xray/localized_sampling.h | 6 --- source/extensions/tracers/xray/tracer.cc | 22 +++------ source/extensions/tracers/xray/tracer.h | 2 +- test/extensions/tracers/xray/BUILD | 2 + test/extensions/tracers/xray/tracer_test.cc | 49 +++++++++++++++++-- 6 files changed, 60 insertions(+), 31 deletions(-) diff --git a/source/extensions/tracers/xray/daemon_broker.cc b/source/extensions/tracers/xray/daemon_broker.cc index d5667c423aa8..39d7de50ef99 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 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/BUILD b/test/extensions/tracers/xray/BUILD index e00d7e395bb9..8d1d57c436be 100644 --- a/test/extensions/tracers/xray/BUILD +++ b/test/extensions/tracers/xray/BUILD @@ -31,6 +31,8 @@ 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 023dd00a40ec..5323122f2e9e 100644 --- a/test/extensions/tracers/xray/tracer_test.cc +++ b/test/extensions/tracers/xray/tracer_test.cc @@ -12,6 +12,8 @@ #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" #include "absl/strings/str_split.h" @@ -108,15 +110,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 +185,47 @@ TEST_F(XRayTracerTest, TraceIDFormatTest) { ASSERT_EQ(24, parts[2].length()); } +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(GetParam()); + 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(), + absl::nullopt /*headers*/); + + 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 } // namespace XRay } // namespace Tracers