reviewer feedback

src/core/ext/filters/client_channel/client_channel_channelz.cc

@@ -41,8 +41,9 @@ static const grpc_arg_pointer_vtable client_channel_channelz_vtable = {
     client_channel_channelz_cmp};
 
 ClientChannelNode::ClientChannelNode(grpc_channel* channel,
-                                     size_t channel_tracer_max_nodes)
-    : ChannelNode(channel, channel_tracer_max_nodes) {
+                                     size_t channel_tracer_max_nodes,
+                                     bool is_top_level_channel)
+    : ChannelNode(channel, channel_tracer_max_nodes, is_top_level_channel) {
   client_channel_ =
       grpc_channel_stack_last_element(grpc_channel_get_channel_stack(channel));
   GPR_ASSERT(client_channel_->filter == &grpc_client_channel_filter);
@@ -71,9 +72,10 @@ grpc_arg ClientChannelNode::CreateChannelArg() {
 }
 
 RefCountedPtr<ChannelNode> ClientChannelNode::MakeClientChannelNode(
-    grpc_channel* channel, size_t channel_tracer_max_nodes) {
+    grpc_channel* channel, size_t channel_tracer_max_nodes,
+    bool is_top_level_channel) {
   return MakePolymorphicRefCounted<ChannelNode, ClientChannelNode>(
-      channel, channel_tracer_max_nodes);
+      channel, channel_tracer_max_nodes, is_top_level_channel);
 }
 
 }  // namespace channelz

src/core/ext/filters/client_channel/client_channel_channelz.h

@@ -32,7 +32,8 @@ namespace channelz {
 class ClientChannelNode : public ChannelNode {
  public:
   static RefCountedPtr<ChannelNode> MakeClientChannelNode(
-      grpc_channel* channel, size_t channel_tracer_max_nodes);
+      grpc_channel* channel, size_t channel_tracer_max_nodes,
+      bool is_top_level_channel);
 
   // Override this functionality since client_channels have a notion of
   // channel connectivity.
@@ -45,7 +46,8 @@ class ClientChannelNode : public ChannelNode {
  protected:
   GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
   GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW
-  ClientChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes);
+  ClientChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes,
+                    bool is_top_level_channel);
   virtual ~ClientChannelNode() {}
 
  private:

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc

@@ -1004,7 +1004,8 @@ grpc_channel_args* BuildBalancerChannelArgs(
       // A channel arg indicating the target is a grpclb load balancer.
       grpc_channel_arg_integer_create(
           const_cast<char*>(GRPC_ARG_ADDRESS_IS_GRPCLB_LOAD_BALANCER), 1),
-      // A channel arg indicating the target is a grpclb load balancer.
+      // A channel arg indicating this is an internal channels, aka it is
+      // owned by components in Core, not by the user application.
       grpc_channel_arg_integer_create(
           const_cast<char*>(GRPC_ARG_CHANNELZ_CHANNEL_IS_INTERNAL_CHANNEL), 1),
   };

src/core/lib/channel/channelz.cc

@@ -88,8 +88,12 @@ grpc_json* add_num_str(grpc_json* parent, grpc_json* it, const char* name,
 
 }  // namespace
 
-ChannelNode::ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes)
-    : channel_(channel), target_(nullptr), channel_uuid_(-1) {
+ChannelNode::ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes,
+                         bool is_top_level_channel)
+    : channel_(channel),
+      target_(nullptr),
+      channel_uuid_(-1),
+      is_top_level_channel_(is_top_level_channel) {
   trace_.Init(channel_tracer_max_nodes);
   target_ = UniquePtr<char>(grpc_channel_get_target(channel_));
   channel_uuid_ = ChannelzRegistry::RegisterChannelNode(this);
@@ -136,7 +140,7 @@ grpc_json* ChannelNode::RenderJson() {
   // fill in the channel trace if applicable
   grpc_json* trace = trace_->RenderJson();
   if (trace != nullptr) {
-    // we manuall link up and fill the child since it was created for us in
+    // we manually link up and fill the child since it was created for us in
     // ChannelTrace::RenderJson
     json_iterator = grpc_json_link_child(json, trace, json_iterator);
     trace->parent = json;
@@ -175,9 +179,10 @@ char* ChannelNode::RenderJsonString() {
 }
 
 RefCountedPtr<ChannelNode> ChannelNode::MakeChannelNode(
-    grpc_channel* channel, size_t channel_tracer_max_nodes) {
+    grpc_channel* channel, size_t channel_tracer_max_nodes,
+    bool is_top_level_channel) {
   return MakeRefCounted<grpc_core::channelz::ChannelNode>(
-      channel, channel_tracer_max_nodes);
+      channel, channel_tracer_max_nodes, is_top_level_channel);
 }
 
 }  // namespace channelz

src/core/lib/channel/channelz.h

@@ -49,7 +49,8 @@ class ChannelNodePeer;
 class ChannelNode : public RefCounted<ChannelNode> {
  public:
   static RefCountedPtr<ChannelNode> MakeChannelNode(
-      grpc_channel* channel, size_t channel_tracer_max_nodes);
+      grpc_channel* channel, size_t channel_tracer_max_nodes,
+      bool is_top_level_channel);
 
   void RecordCallStarted();
   void RecordCallFailed() {
@@ -78,14 +79,12 @@ class ChannelNode : public RefCounted<ChannelNode> {
 
   intptr_t channel_uuid() { return channel_uuid_; }
   bool is_top_level_channel() { return is_top_level_channel_; }
-  void set_is_top_level_channel(bool is_top_level_channel) {
-    is_top_level_channel_ = is_top_level_channel;
-  }
 
  protected:
   GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE
   GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW
-  ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes);
+  ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes,
+              bool is_top_level_channel);
   virtual ~ChannelNode();
 
  private:
@@ -106,7 +105,7 @@ class ChannelNode : public RefCounted<ChannelNode> {
 // Creation functions
 
 typedef RefCountedPtr<ChannelNode> (*ChannelNodeCreationFunc)(grpc_channel*,
-                                                              size_t);
+                                                              size_t, bool);
 
 }  // namespace channelz
 }  // namespace grpc_core

src/core/lib/channel/channelz_registry.cc

@@ -86,7 +86,7 @@ char* ChannelzRegistry::InternalGetTopChannels(intptr_t start_channel_id) {
   grpc_json* json_iterator = nullptr;
   InlinedVector<ChannelNode*, 10> top_level_channels;
   // uuids index into entities one-off (idx 0 is really uuid 1, since 0 is
-  // reserver). However, we want to support requests coming in which
+  // reserved). However, we want to support requests coming in this
   // start_channel_id=0, which signifies "give me everything." Hence this
   // funky looking line below.
   size_t start_idx = start_channel_id == 0 ? 0 : start_channel_id - 1;
@@ -108,9 +108,6 @@ char* ChannelzRegistry::InternalGetTopChannels(intptr_t start_channel_id) {
       json_iterator =
           grpc_json_link_child(array_parent, channel_json, json_iterator);
       channel_json->parent = array_parent;
-      channel_json->value = nullptr;
-      channel_json->key = nullptr;
-      channel_json->owns_value = false;
     }
   }
   // For now we do not have any pagination rules. In the future we could

src/core/lib/channel/channelz_registry.h

@@ -36,7 +36,7 @@ class ChannelzRegistry {
   // To be called in grpc_init()
   static void Init();
 
-  // To be callen in grpc_shutdown();
+  // To be called in grpc_shutdown();
   static void Shutdown();
 
   static intptr_t RegisterChannelNode(ChannelNode* channel_node) {
@@ -51,7 +51,8 @@ class ChannelzRegistry {
     return gotten == nullptr ? nullptr : static_cast<ChannelNode*>(gotten);
   }
 
-  // todo, protect me
+  // Returns the allocated JSON string that represents the proto
+  // GetTopChannelsResponse as per channelz.proto.
   static char* GetTopChannels(intptr_t start_channel_id) {
     return Default()->InternalGetTopChannels(start_channel_id);
   }

src/core/lib/surface/channel.cc

@@ -167,11 +167,9 @@ grpc_channel* grpc_channel_create_with_builder(
 
   grpc_channel_args_destroy(args);
   if (channelz_enabled) {
-    channel->channelz_channel =
-        channel_node_create_func(channel, channel_tracer_max_nodes);
-    if (internal_channel || !channel->is_client) {
-      channel->channelz_channel->set_is_top_level_channel(false);
-    }
+    bool is_top_level_channel = channel->is_client && !internal_channel;
+    channel->channelz_channel = channel_node_create_func(
+        channel, channel_tracer_max_nodes, is_top_level_channel);
     channel->channelz_channel->trace()->AddTraceEvent(
         grpc_core::channelz::ChannelTrace::Severity::Info,
         grpc_slice_from_static_string("Channel created"));

test/core/channel/channel_trace_test.cc

@@ -34,8 +34,6 @@
 #include "test/core/util/test_config.h"
 #include "test/cpp/util/channel_trace_proto_helper.h"
 
-// remove me
-#include <grpc/support/string_util.h>
 #include <stdlib.h>
 #include <string.h>
 
@@ -157,7 +155,7 @@ TEST_P(ChannelTracerTest, ComplexTest) {
   AddSimpleTrace(&tracer);
   ChannelFixture channel1(GetParam());
   RefCountedPtr<ChannelNode> sc1 =
-      MakeRefCounted<ChannelNode>(channel1.channel(), GetParam());
+      MakeRefCounted<ChannelNode>(channel1.channel(), GetParam(), true);
   tracer.AddTraceEventReferencingSubchannel(
       ChannelTrace::Severity::Info,
       grpc_slice_from_static_string("subchannel one created"), sc1);
@@ -175,7 +173,7 @@ TEST_P(ChannelTracerTest, ComplexTest) {
   ValidateChannelTrace(&tracer, 5, GetParam());
   ChannelFixture channel2(GetParam());
   RefCountedPtr<ChannelNode> sc2 =
-      MakeRefCounted<ChannelNode>(channel2.channel(), GetParam());
+      MakeRefCounted<ChannelNode>(channel2.channel(), GetParam(), true);
   tracer.AddTraceEventReferencingChannel(
       ChannelTrace::Severity::Info,
       grpc_slice_from_static_string("LB channel two created"), sc2);
@@ -204,7 +202,7 @@ TEST_P(ChannelTracerTest, TestNesting) {
   ValidateChannelTrace(&tracer, 2, GetParam());
   ChannelFixture channel1(GetParam());
   RefCountedPtr<ChannelNode> sc1 =
-      MakeRefCounted<ChannelNode>(channel1.channel(), GetParam());
+      MakeRefCounted<ChannelNode>(channel1.channel(), GetParam(), true);
   tracer.AddTraceEventReferencingChannel(
       ChannelTrace::Severity::Info,
       grpc_slice_from_static_string("subchannel one created"), sc1);
@@ -212,7 +210,7 @@ TEST_P(ChannelTracerTest, TestNesting) {
   AddSimpleTrace(sc1->trace());
   ChannelFixture channel2(GetParam());
   RefCountedPtr<ChannelNode> conn1 =
-      MakeRefCounted<ChannelNode>(channel2.channel(), GetParam());
+      MakeRefCounted<ChannelNode>(channel2.channel(), GetParam(), true);
   // nesting one level deeper.
   sc1->trace()->AddTraceEventReferencingSubchannel(
       ChannelTrace::Severity::Info,
@@ -225,7 +223,7 @@ TEST_P(ChannelTracerTest, TestNesting) {
   ValidateChannelTrace(conn1->trace(), 1, GetParam());
   ChannelFixture channel3(GetParam());
   RefCountedPtr<ChannelNode> sc2 =
-      MakeRefCounted<ChannelNode>(channel3.channel(), GetParam());
+      MakeRefCounted<ChannelNode>(channel3.channel(), GetParam(), true);
   tracer.AddTraceEventReferencingSubchannel(
       ChannelTrace::Severity::Info,
       grpc_slice_from_static_string("subchannel two created"), sc2);

test/core/channel/channelz_registry_test.cc

@@ -42,36 +42,9 @@
 namespace grpc_core {
 namespace channelz {
 namespace testing {
-namespace {
 
-class ChannelFixture {
- public:
-  ChannelFixture() {
-    grpc_arg client_a[1];
-    client_a[0].type = GRPC_ARG_INTEGER;
-    client_a[0].key = const_cast<char*>(GRPC_ARG_ENABLE_CHANNELZ);
-    client_a[0].value.integer = true;
-    grpc_channel_args client_args = {GPR_ARRAY_SIZE(client_a), client_a};
-    channel_ =
-        grpc_insecure_channel_create("fake_target", &client_args, nullptr);
-  }
-
-  ~ChannelFixture() { grpc_channel_destroy(channel_); }
-
-  grpc_channel* channel() { return channel_; }
-
- private:
-  grpc_channel* channel_;
-};
-
-}  // namespace
-
-// Tests basic ChannelTrace functionality like construction, adding trace, and
-// lookups by uuid.
 TEST(ChannelzRegistryTest, UuidStartsAboveZeroTest) {
-  ChannelFixture channel;
-  ChannelNode* channelz_channel =
-      grpc_channel_get_channelz_node(channel.channel());
+  ChannelNode* channelz_channel = nullptr;
   intptr_t uuid = ChannelzRegistry::RegisterChannelNode(channelz_channel);
   EXPECT_GT(uuid, 0) << "First uuid chose must be greater than zero. Zero if "
                         "reserved according to "
@@ -81,9 +54,7 @@ TEST(ChannelzRegistryTest, UuidStartsAboveZeroTest) {
 }
 
 TEST(ChannelzRegistryTest, UuidsAreIncreasing) {
-  ChannelFixture channel;
-  ChannelNode* channelz_channel =
-      grpc_channel_get_channelz_node(channel.channel());
+  ChannelNode* channelz_channel = nullptr;
   std::vector<intptr_t> uuids;
   uuids.reserve(10);
   for (int i = 0; i < 10; ++i) {
@@ -96,18 +67,14 @@ TEST(ChannelzRegistryTest, UuidsAreIncreasing) {
 }
 
 TEST(ChannelzRegistryTest, RegisterGetTest) {
-  ChannelFixture channel;
-  ChannelNode* channelz_channel =
-      grpc_channel_get_channelz_node(channel.channel());
+  ChannelNode* channelz_channel = nullptr;
   intptr_t uuid = ChannelzRegistry::RegisterChannelNode(channelz_channel);
   ChannelNode* retrieved = ChannelzRegistry::GetChannelNode(uuid);
   EXPECT_EQ(channelz_channel, retrieved);
 }
 
 TEST(ChannelzRegistryTest, RegisterManyItems) {
-  ChannelFixture channel;
-  ChannelNode* channelz_channel =
-      grpc_channel_get_channelz_node(channel.channel());
+  ChannelNode* channelz_channel = nullptr;
   for (int i = 0; i < 100; i++) {
     intptr_t uuid = ChannelzRegistry::RegisterChannelNode(channelz_channel);
     ChannelNode* retrieved = ChannelzRegistry::GetChannelNode(uuid);
@@ -116,9 +83,7 @@ TEST(ChannelzRegistryTest, RegisterManyItems) {
 }
 
 TEST(ChannelzRegistryTest, NullIfNotPresentTest) {
-  ChannelFixture channel;
-  ChannelNode* channelz_channel =
-      grpc_channel_get_channelz_node(channel.channel());
+  ChannelNode* channelz_channel = nullptr;
   intptr_t uuid = ChannelzRegistry::RegisterChannelNode(channelz_channel);
   // try to pull out a uuid that does not exist.
   ChannelNode* nonexistant = ChannelzRegistry::GetChannelNode(uuid + 1);

test/core/channel/channelz_test.cc

@@ -80,7 +80,7 @@ void ValidateJsonArraySize(grpc_json* json, const char* key,
   for (grpc_json* child = arr->child; child != nullptr; child = child->next) {
     ++count;
   }
-  ASSERT_EQ(count, expected_size);
+  EXPECT_EQ(count, expected_size);
 }
 
 void ValidateGetTopChannels(size_t expected_channels) {
@@ -91,7 +91,7 @@ void ValidateGetTopChannels(size_t expected_channels) {
   // tracked: https://github.com/grpc/grpc/issues/16019.
   ValidateJsonArraySize(parsed_json, "channel", expected_channels);
   grpc_json* end = GetJsonChild(parsed_json, "end");
-  EXPECT_NE(end, nullptr);
+  ASSERT_NE(end, nullptr);
   EXPECT_EQ(end->type, GRPC_JSON_TRUE);
   grpc_json_destroy(parsed_json);
   gpr_free(json_str);
@@ -101,13 +101,11 @@ class ChannelFixture {
  public:
   ChannelFixture(int max_trace_nodes = 0) {
     grpc_arg client_a[2];
-    client_a[0].type = GRPC_ARG_INTEGER;
-    client_a[0].key =
-        const_cast<char*>(GRPC_ARG_MAX_CHANNEL_TRACE_EVENTS_PER_NODE);
-    client_a[0].value.integer = max_trace_nodes;
-    client_a[1].type = GRPC_ARG_INTEGER;
-    client_a[1].key = const_cast<char*>(GRPC_ARG_ENABLE_CHANNELZ);
-    client_a[1].value.integer = true;
+    client_a[0] = grpc_channel_arg_integer_create(
+        const_cast<char*>(GRPC_ARG_MAX_CHANNEL_TRACE_EVENTS_PER_NODE),
+        max_trace_nodes);
+    client_a[1] = grpc_channel_arg_integer_create(
+        const_cast<char*>(GRPC_ARG_ENABLE_CHANNELZ), true);
     grpc_channel_args client_args = {GPR_ARRAY_SIZE(client_a), client_a};
     channel_ =
         grpc_insecure_channel_create("fake_target", &client_args, nullptr);
@@ -255,13 +253,10 @@ TEST(ChannelzGetTopChannelsTest, InternalChannelTest) {
   (void)channels;  // suppress unused variable error
   // create an internal channel
   grpc_arg client_a[2];
-  client_a[0].type = GRPC_ARG_INTEGER;
-  client_a[0].key =
-      const_cast<char*>(GRPC_ARG_CHANNELZ_CHANNEL_IS_INTERNAL_CHANNEL);
-  client_a[0].value.integer = 1;
-  client_a[1].type = GRPC_ARG_INTEGER;
-  client_a[1].key = const_cast<char*>(GRPC_ARG_ENABLE_CHANNELZ);
-  client_a[1].value.integer = true;
+  client_a[0] = grpc_channel_arg_integer_create(
+      const_cast<char*>(GRPC_ARG_CHANNELZ_CHANNEL_IS_INTERNAL_CHANNEL), true);
+  client_a[1] = grpc_channel_arg_integer_create(
+      const_cast<char*>(GRPC_ARG_ENABLE_CHANNELZ), true);
   grpc_channel_args client_args = {GPR_ARRAY_SIZE(client_a), client_a};
   grpc_channel* internal_channel =
       grpc_insecure_channel_create("fake_target", &client_args, nullptr);