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 <mmagdy@gmail.com>

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

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:

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<SecondsWithFraction>(startTime()).time_since_epoch().count());
   s.set_end_time(
       time_point_cast<SecondsWithFraction>(time_source_.systemTime()).time_since_epoch().count());
   s.set_parent_id(parentId());
 
-  // HTTP annotations
-  using StructField = Protobuf::MapPair<std::string, ProtobufWkt::Value>;
-
-  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<std::string, std::string>::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;

source/extensions/tracers/xray/tracer.h

@@ -113,7 +113,7 @@ class Span : public Tracing::Span, Logger::Loggable<Logger::Id::config> {
   /**
    * 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_; }
 

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",
     ],
 )

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<XRay::Span*>(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<Network::Address::IpVersion> {};
+
+INSTANTIATE_TEST_SUITE_P(IpVersions, XRayDaemonTest,
+                         testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
+                         TestUtility::ipTestParamsToString);
+
+TEST_P(XRayDaemonTest, VerifyUdpPacketContents) {
+  NiceMock<Server::MockInstance> 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<DaemonBrokerImpl>(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