Merge pull request #16055 from ncteisen/channelz-subchannels

Channelz Part 4: Add Subchannel Support

7 files changed, 71 insertions, 53 deletions

diff --git a/test/core/channel/channel_stack_test.cc b/test/core/channel/channel_stack_test.cc
index 2f5329a96d..6f0bfa06d2 100644
--- a/test/core/channel/channel_stack_test.cc
+++ b/test/core/channel/channel_stack_test.cc
@@ -124,7 +124,7 @@ static void test_create_channel_stack(void) {
       gpr_now(GPR_CLOCK_MONOTONIC), /* start_time */
       GRPC_MILLIS_INF_FUTURE,       /* deadline */
       nullptr,                      /* arena */
-      nullptr                       /* call_combiner */
+      nullptr,                      /* call_combiner */
   };
   grpc_error* error =
       grpc_call_stack_init(channel_stack, 1, free_call, call_stack, &args);
diff --git a/test/core/channel/channel_trace_test.cc b/test/core/channel/channel_trace_test.cc
index f224457a55..d594445bfb 100644
--- a/test/core/channel/channel_trace_test.cc
+++ b/test/core/channel/channel_trace_test.cc
@@ -40,6 +40,17 @@
 namespace grpc_core {
 namespace channelz {
 namespace testing {
+
+// testing peer to access channel internals
+class ChannelNodePeer {
+ public:
+  explicit ChannelNodePeer(ChannelNode* node) : node_(node) {}
+  ChannelTrace* trace() const { return &node_->trace_; }
+
+ private:
+  ChannelNode* node_;
+};
+
 namespace {
 
 grpc_json* GetJsonChild(grpc_json* parent, const char* key) {
@@ -156,28 +167,29 @@ TEST_P(ChannelTracerTest, ComplexTest) {
   ChannelFixture channel1(GetParam());
   RefCountedPtr<ChannelNode> sc1 =
       MakeRefCounted<ChannelNode>(channel1.channel(), GetParam(), true);
-  tracer.AddTraceEventReferencingSubchannel(
+  ChannelNodePeer sc1_peer(sc1.get());
+  tracer.AddTraceEventWithReference(
       ChannelTrace::Severity::Info,
       grpc_slice_from_static_string("subchannel one created"), sc1);
   ValidateChannelTrace(&tracer, 3, GetParam());
-  AddSimpleTrace(sc1->trace());
-  AddSimpleTrace(sc1->trace());
-  AddSimpleTrace(sc1->trace());
-  ValidateChannelTrace(sc1->trace(), 3, GetParam());
-  AddSimpleTrace(sc1->trace());
-  AddSimpleTrace(sc1->trace());
-  AddSimpleTrace(sc1->trace());
-  ValidateChannelTrace(sc1->trace(), 6, GetParam());
+  AddSimpleTrace(sc1_peer.trace());
+  AddSimpleTrace(sc1_peer.trace());
+  AddSimpleTrace(sc1_peer.trace());
+  ValidateChannelTrace(sc1_peer.trace(), 3, GetParam());
+  AddSimpleTrace(sc1_peer.trace());
+  AddSimpleTrace(sc1_peer.trace());
+  AddSimpleTrace(sc1_peer.trace());
+  ValidateChannelTrace(sc1_peer.trace(), 6, GetParam());
   AddSimpleTrace(&tracer);
   AddSimpleTrace(&tracer);
   ValidateChannelTrace(&tracer, 5, GetParam());
   ChannelFixture channel2(GetParam());
   RefCountedPtr<ChannelNode> sc2 =
       MakeRefCounted<ChannelNode>(channel2.channel(), GetParam(), true);
-  tracer.AddTraceEventReferencingChannel(
+  tracer.AddTraceEventWithReference(
       ChannelTrace::Severity::Info,
       grpc_slice_from_static_string("LB channel two created"), sc2);
-  tracer.AddTraceEventReferencingSubchannel(
+  tracer.AddTraceEventWithReference(
       ChannelTrace::Severity::Warning,
       grpc_slice_from_static_string("subchannel one inactive"), sc1);
   ValidateChannelTrace(&tracer, 7, GetParam());
@@ -203,33 +215,35 @@ TEST_P(ChannelTracerTest, TestNesting) {
   ChannelFixture channel1(GetParam());
   RefCountedPtr<ChannelNode> sc1 =
       MakeRefCounted<ChannelNode>(channel1.channel(), GetParam(), true);
-  tracer.AddTraceEventReferencingChannel(
+  ChannelNodePeer sc1_peer(sc1.get());
+  tracer.AddTraceEventWithReference(
       ChannelTrace::Severity::Info,
       grpc_slice_from_static_string("subchannel one created"), sc1);
   ValidateChannelTrace(&tracer, 3, GetParam());
-  AddSimpleTrace(sc1->trace());
+  AddSimpleTrace(sc1_peer.trace());
   ChannelFixture channel2(GetParam());
   RefCountedPtr<ChannelNode> conn1 =
       MakeRefCounted<ChannelNode>(channel2.channel(), GetParam(), true);
+  ChannelNodePeer conn1_peer(conn1.get());
   // nesting one level deeper.
-  sc1->trace()->AddTraceEventReferencingSubchannel(
+  sc1_peer.trace()->AddTraceEventWithReference(
       ChannelTrace::Severity::Info,
       grpc_slice_from_static_string("connection one created"), conn1);
   ValidateChannelTrace(&tracer, 3, GetParam());
-  AddSimpleTrace(conn1->trace());
+  AddSimpleTrace(conn1_peer.trace());
   AddSimpleTrace(&tracer);
   AddSimpleTrace(&tracer);
   ValidateChannelTrace(&tracer, 5, GetParam());
-  ValidateChannelTrace(conn1->trace(), 1, GetParam());
+  ValidateChannelTrace(conn1_peer.trace(), 1, GetParam());
   ChannelFixture channel3(GetParam());
   RefCountedPtr<ChannelNode> sc2 =
       MakeRefCounted<ChannelNode>(channel3.channel(), GetParam(), true);
-  tracer.AddTraceEventReferencingSubchannel(
+  tracer.AddTraceEventWithReference(
       ChannelTrace::Severity::Info,
       grpc_slice_from_static_string("subchannel two created"), sc2);
   // this trace should not get added to the parents children since it is already
   // present in the tracer.
-  tracer.AddTraceEventReferencingChannel(
+  tracer.AddTraceEventWithReference(
       ChannelTrace::Severity::Warning,
       grpc_slice_from_static_string("subchannel one inactive"), sc1);
   AddSimpleTrace(&tracer);
diff --git a/test/core/channel/channelz_registry_test.cc b/test/core/channel/channelz_registry_test.cc
index 581e867584..c02d525c81 100644
--- a/test/core/channel/channelz_registry_test.cc
+++ b/test/core/channel/channelz_registry_test.cc
@@ -44,22 +44,22 @@ namespace channelz {
 namespace testing {
 
 TEST(ChannelzRegistryTest, UuidStartsAboveZeroTest) {
-  ChannelNode* channelz_channel = nullptr;
-  intptr_t uuid = ChannelzRegistry::RegisterChannelNode(channelz_channel);
+  BaseNode* channelz_channel = nullptr;
+  intptr_t uuid = ChannelzRegistry::Register(channelz_channel);
   EXPECT_GT(uuid, 0) << "First uuid chose must be greater than zero. Zero if "
                         "reserved according to "
                         "https://github.com/grpc/proposal/blob/master/"
                         "A14-channelz.md";
-  ChannelzRegistry::UnregisterChannelNode(uuid);
+  ChannelzRegistry::Unregister(uuid);
 }
 
 TEST(ChannelzRegistryTest, UuidsAreIncreasing) {
-  ChannelNode* channelz_channel = nullptr;
+  BaseNode* channelz_channel = nullptr;
   std::vector<intptr_t> uuids;
   uuids.reserve(10);
   for (int i = 0; i < 10; ++i) {
     // reregister the same object. It's ok since we are just testing uuids
-    uuids.push_back(ChannelzRegistry::RegisterChannelNode(channelz_channel));
+    uuids.push_back(ChannelzRegistry::Register(channelz_channel));
   }
   for (size_t i = 1; i < uuids.size(); ++i) {
     EXPECT_LT(uuids[i - 1], uuids[i]) << "Uuids must always be increasing";
@@ -68,30 +68,30 @@ TEST(ChannelzRegistryTest, UuidsAreIncreasing) {
 
 TEST(ChannelzRegistryTest, RegisterGetTest) {
   // we hackily jam an intptr_t into this pointer to check for equality later
-  ChannelNode* channelz_channel = (ChannelNode*)42;
-  intptr_t uuid = ChannelzRegistry::RegisterChannelNode(channelz_channel);
-  ChannelNode* retrieved = ChannelzRegistry::GetChannelNode(uuid);
+  BaseNode* channelz_channel = (BaseNode*)42;
+  intptr_t uuid = ChannelzRegistry::Register(channelz_channel);
+  BaseNode* retrieved = ChannelzRegistry::Get(uuid);
   EXPECT_EQ(channelz_channel, retrieved);
 }
 
 TEST(ChannelzRegistryTest, RegisterManyItems) {
   // we hackily jam an intptr_t into this pointer to check for equality later
-  ChannelNode* channelz_channel = (ChannelNode*)42;
+  BaseNode* channelz_channel = (BaseNode*)42;
   for (int i = 0; i < 100; i++) {
-    intptr_t uuid = ChannelzRegistry::RegisterChannelNode(channelz_channel);
-    ChannelNode* retrieved = ChannelzRegistry::GetChannelNode(uuid);
+    intptr_t uuid = ChannelzRegistry::Register(channelz_channel);
+    BaseNode* retrieved = ChannelzRegistry::Get(uuid);
     EXPECT_EQ(channelz_channel, retrieved);
   }
 }
 
 TEST(ChannelzRegistryTest, NullIfNotPresentTest) {
   // we hackily jam an intptr_t into this pointer to check for equality later
-  ChannelNode* channelz_channel = (ChannelNode*)42;
-  intptr_t uuid = ChannelzRegistry::RegisterChannelNode(channelz_channel);
+  BaseNode* channelz_channel = (BaseNode*)42;
+  intptr_t uuid = ChannelzRegistry::Register(channelz_channel);
   // try to pull out a uuid that does not exist.
-  ChannelNode* nonexistant = ChannelzRegistry::GetChannelNode(uuid + 1);
+  BaseNode* nonexistant = ChannelzRegistry::Get(uuid + 1);
   EXPECT_EQ(nonexistant, nullptr);
-  ChannelNode* retrieved = ChannelzRegistry::GetChannelNode(uuid);
+  BaseNode* retrieved = ChannelzRegistry::Get(uuid);
   EXPECT_EQ(channelz_channel, retrieved);
 }
 
diff --git a/test/core/channel/channelz_test.cc b/test/core/channel/channelz_test.cc
index ad5f86d934..09189fa36d 100644
--- a/test/core/channel/channelz_test.cc
+++ b/test/core/channel/channelz_test.cc
@@ -44,16 +44,16 @@ namespace channelz {
 namespace testing {
 
 // testing peer to access channel internals
-class ChannelNodePeer {
+class CallCountingHelperPeer {
  public:
-  ChannelNodePeer(ChannelNode* channel) : channel_(channel) {}
-  grpc_millis last_call_started_millis() {
+  explicit CallCountingHelperPeer(CallCountingHelper* node) : node_(node) {}
+  grpc_millis last_call_started_millis() const {
     return (grpc_millis)gpr_atm_no_barrier_load(
-        &channel_->last_call_started_millis_);
+        &node_->last_call_started_millis_);
   }
 
  private:
-  ChannelNode* channel_;
+  CallCountingHelper* node_;
 };
 
 namespace {
@@ -157,14 +157,14 @@ void ValidateChannel(ChannelNode* channel, validate_channel_data_args args) {
   ValidateCounters(json_str, args);
   gpr_free(json_str);
   // also check that the core API formats this the correct way
-  char* core_api_json_str = grpc_channelz_get_channel(channel->channel_uuid());
+  char* core_api_json_str = grpc_channelz_get_channel(channel->uuid());
   grpc::testing::ValidateGetChannelResponseProtoJsonTranslation(
       core_api_json_str);
   gpr_free(core_api_json_str);
 }
 
-grpc_millis GetLastCallStartedMillis(ChannelNode* channel) {
-  ChannelNodePeer peer(channel);
+grpc_millis GetLastCallStartedMillis(CallCountingHelper* channel) {
+  CallCountingHelperPeer peer(channel);
   return peer.last_call_started_millis();
 }
 
@@ -215,27 +215,25 @@ TEST_P(ChannelzChannelTest, BasicChannelAPIFunctionality) {
 
 TEST_P(ChannelzChannelTest, LastCallStartedMillis) {
   grpc_core::ExecCtx exec_ctx;
-  ChannelFixture channel(GetParam());
-  ChannelNode* channelz_channel =
-      grpc_channel_get_channelz_node(channel.channel());
+  CallCountingHelper counter;
   // start a call to set the last call started timestamp
-  channelz_channel->RecordCallStarted();
-  grpc_millis millis1 = GetLastCallStartedMillis(channelz_channel);
+  counter.RecordCallStarted();
+  grpc_millis millis1 = GetLastCallStartedMillis(&counter);
   // time gone by should not affect the timestamp
   ChannelzSleep(100);
-  grpc_millis millis2 = GetLastCallStartedMillis(channelz_channel);
+  grpc_millis millis2 = GetLastCallStartedMillis(&counter);
   EXPECT_EQ(millis1, millis2);
   // calls succeeded or failed should not affect the timestamp
   ChannelzSleep(100);
-  channelz_channel->RecordCallFailed();
-  channelz_channel->RecordCallSucceeded();
-  grpc_millis millis3 = GetLastCallStartedMillis(channelz_channel);
+  counter.RecordCallFailed();
+  counter.RecordCallSucceeded();
+  grpc_millis millis3 = GetLastCallStartedMillis(&counter);
   EXPECT_EQ(millis1, millis3);
   // another call started should affect the timestamp
   // sleep for extra long to avoid flakes (since we cache Now())
   ChannelzSleep(5000);
-  channelz_channel->RecordCallStarted();
-  grpc_millis millis4 = GetLastCallStartedMillis(channelz_channel);
+  counter.RecordCallStarted();
+  grpc_millis millis4 = GetLastCallStartedMillis(&counter);
   EXPECT_NE(millis1, millis4);
 }
 
diff --git a/test/core/surface/public_headers_must_be_c89.c b/test/core/surface/public_headers_must_be_c89.c
index b832a1661b..a4d2a5bf62 100644
--- a/test/core/surface/public_headers_must_be_c89.c
+++ b/test/core/surface/public_headers_must_be_c89.c
@@ -137,6 +137,7 @@ int main(int argc, char **argv) {
   printf("%lx", (unsigned long) grpc_resource_quota_arg_vtable);
   printf("%lx", (unsigned long) grpc_channelz_get_top_channels);
   printf("%lx", (unsigned long) grpc_channelz_get_channel);
+  printf("%lx", (unsigned long) grpc_channelz_get_subchannel);
   printf("%lx", (unsigned long) grpc_auth_property_iterator_next);
   printf("%lx", (unsigned long) grpc_auth_context_property_iterator);
   printf("%lx", (unsigned long) grpc_auth_context_peer_identity);
diff --git a/test/cpp/util/channel_trace_proto_helper.cc b/test/cpp/util/channel_trace_proto_helper.cc
index b4704bfe6a..e416a0375f 100644
--- a/test/cpp/util/channel_trace_proto_helper.cc
+++ b/test/cpp/util/channel_trace_proto_helper.cc
@@ -82,5 +82,9 @@ void ValidateGetChannelResponseProtoJsonTranslation(char* json_c_str) {
       json_c_str);
 }
 
+void ValidateSubchannelProtoJsonTranslation(char* json_c_str) {
+  VaidateProtoJsonTranslation<grpc::channelz::v1::Subchannel>(json_c_str);
+}
+
 }  // namespace testing
 }  // namespace grpc
diff --git a/test/cpp/util/channel_trace_proto_helper.h b/test/cpp/util/channel_trace_proto_helper.h
index 18e3d54b6b..a1ebbcd110 100644
--- a/test/cpp/util/channel_trace_proto_helper.h
+++ b/test/cpp/util/channel_trace_proto_helper.h
@@ -26,6 +26,7 @@ void ValidateChannelTraceProtoJsonTranslation(char* json_c_str);
 void ValidateChannelProtoJsonTranslation(char* json_c_str);
 void ValidateGetTopChannelsResponseProtoJsonTranslation(char* json_c_str);
 void ValidateGetChannelResponseProtoJsonTranslation(char* json_c_str);
+void ValidateSubchannelProtoJsonTranslation(char* json_c_str);
 
 }  // namespace testing
 }  // namespace grpc