reviewer comments

7 files changed, 71 insertions, 64 deletions

diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.cc b/src/core/ext/filters/client_channel/client_channel_channelz.cc
index f797ef1b09..7120ec57f1 100644
--- a/src/core/ext/filters/client_channel/client_channel_channelz.cc
+++ b/src/core/ext/filters/client_channel/client_channel_channelz.cc
@@ -123,9 +123,14 @@ grpc_json* ClientChannelNode::RenderJson() {
   GPR_ASSERT(target_view() != nullptr);
   grpc_json_create_child(nullptr, json, "target", target_view(),
                          GRPC_JSON_STRING, false);
-  // as CallCountingAndTracingNode to populate trace and call count data.
-  counter_and_tracer()->PopulateTrace(json);
-  counter_and_tracer()->PopulateCallData(json);
+  // fill in the channel trace if applicable
+  grpc_json* trace_json = trace()->RenderJson();
+  if (trace_json != nullptr) {
+    trace_json->key = "trace";  // this object is named trace in channelz.proto
+    grpc_json_link_child(json, trace_json, nullptr);
+  }
+  // ask CallCountingHelper to populate trace and call count data.
+  call_counter()->PopulateCallData(json);
   // reset to the top level
   json = top_level_json;
   PopulateChildRefs(json);
@@ -150,11 +155,12 @@ SubchannelNode::SubchannelNode(grpc_subchannel* subchannel,
                                size_t channel_tracer_max_nodes)
     : BaseNode(EntityType::kSubchannel),
       subchannel_(subchannel),
-      target_(
-          UniquePtr<char>(gpr_strdup(grpc_subchannel_get_target(subchannel_)))),
-      counter_and_tracer_(channel_tracer_max_nodes) {}
+      target_(UniquePtr<char>(
+          gpr_strdup(grpc_subchannel_get_target(subchannel_)))) {
+  trace_.Init(channel_tracer_max_nodes);
+}
 
-SubchannelNode::~SubchannelNode() {}
+SubchannelNode::~SubchannelNode() { trace_.Destroy(); }
 
 void SubchannelNode::PopulateConnectivityState(grpc_json* json) {
   grpc_connectivity_state state;
@@ -192,8 +198,14 @@ grpc_json* SubchannelNode::RenderJson() {
   GPR_ASSERT(target_.get() != nullptr);
   grpc_json_create_child(nullptr, json, "target", target_.get(),
                          GRPC_JSON_STRING, false);
-  counter_and_tracer_.PopulateTrace(json);
-  counter_and_tracer_.PopulateCallData(json);
+  // fill in the channel trace if applicable
+  grpc_json* trace_json = trace_->RenderJson();
+  if (trace_json != nullptr) {
+    trace_json->key = "trace";  // this object is named trace in channelz.proto
+    grpc_json_link_child(json, trace_json, nullptr);
+  }
+  // ask CallCountingHelper to populate trace and call count data.
+  call_counter_.PopulateCallData(json);
   return top_level_json;
 }
 
diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.h b/src/core/ext/filters/client_channel/client_channel_channelz.h
index f5344c049e..735ffacfd2 100644
--- a/src/core/ext/filters/client_channel/client_channel_channelz.h
+++ b/src/core/ext/filters/client_channel/client_channel_channelz.h
@@ -76,14 +76,17 @@ class SubchannelNode : public BaseNode {
 
   grpc_json* RenderJson() override;
 
-  CallCountingAndTracingNode* counter_and_tracer() {
-    return &counter_and_tracer_;
-  }
+  // proxy methods to composed classes.
+  ChannelTrace* trace() { return trace_.get(); }
+  void RecordCallStarted() { call_counter_.RecordCallStarted(); }
+  void RecordCallFailed() { call_counter_.RecordCallFailed(); }
+  void RecordCallSucceeded() { call_counter_.RecordCallSucceeded(); }
 
  private:
   grpc_subchannel* subchannel_;
   UniquePtr<char> target_;
-  CallCountingAndTracingNode counter_and_tracer_;
+  CallCountingHelper call_counter_;
+  ManualConstructor<ChannelTrace> trace_;
 
   void PopulateConnectivityState(grpc_json* json);
 };
diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc
index d7b64a900f..f0f8d94ef4 100644
--- a/src/core/ext/filters/client_channel/subchannel.cc
+++ b/src/core/ext/filters/client_channel/subchannel.cc
@@ -183,7 +183,7 @@ static void connection_destroy(void* arg, grpc_error* error) {
 static void subchannel_destroy(void* arg, grpc_error* error) {
   grpc_subchannel* c = static_cast<grpc_subchannel*>(arg);
   if (c->channelz_subchannel != nullptr) {
-    c->channelz_subchannel->counter_and_tracer()->trace()->AddTraceEvent(
+    c->channelz_subchannel->trace()->AddTraceEvent(
         grpc_core::channelz::ChannelTrace::Severity::Info,
         grpc_slice_from_static_string("Subchannel destroyed"));
     c->channelz_subchannel->MarkSubchannelDestroyed();
@@ -397,7 +397,7 @@ grpc_subchannel* grpc_subchannel_create(grpc_connector* connector,
     c->channelz_subchannel =
         grpc_core::MakeRefCounted<grpc_core::channelz::SubchannelNode>(
             c, channel_tracer_max_nodes);
-    c->channelz_subchannel->counter_and_tracer()->trace()->AddTraceEvent(
+    c->channelz_subchannel->trace()->AddTraceEvent(
         grpc_core::channelz::ChannelTrace::Severity::Info,
         grpc_slice_from_static_string("Subchannel created"));
   }
diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc
index ee1717ce9f..a75a05023a 100644
--- a/src/core/lib/channel/channelz.cc
+++ b/src/core/lib/channel/channelz.cc
@@ -53,33 +53,20 @@ char* BaseNode::RenderJsonString() {
   return json_str;
 }
 
-CallCountingAndTracingNode::CallCountingAndTracingNode(
-    size_t channel_tracer_max_nodes) {
-  trace_.Init(channel_tracer_max_nodes);
+CallCountingHelper::CallCountingHelper() {
   gpr_atm_no_barrier_store(&last_call_started_millis_,
                            (gpr_atm)ExecCtx::Get()->Now());
 }
 
-CallCountingAndTracingNode::~CallCountingAndTracingNode() { trace_.Destroy(); }
+CallCountingHelper::~CallCountingHelper() {}
 
-void CallCountingAndTracingNode::RecordCallStarted() {
+void CallCountingHelper::RecordCallStarted() {
   gpr_atm_no_barrier_fetch_add(&calls_started_, (gpr_atm)1);
   gpr_atm_no_barrier_store(&last_call_started_millis_,
                            (gpr_atm)ExecCtx::Get()->Now());
 }
 
-void CallCountingAndTracingNode::PopulateTrace(grpc_json* json) {
-  // fill in the channel trace if applicable
-  grpc_json* trace_json = trace_->RenderJson();
-  if (trace_json != nullptr) {
-    // we manually link up and fill the child since it was created for us in
-    // ChannelTrace::RenderJson
-    trace_json->key = "trace";  // this object is named trace in channelz.proto
-    grpc_json_link_child(json, trace_json, nullptr);
-  }
-}
-
-void CallCountingAndTracingNode::PopulateCallData(grpc_json* json) {
+void CallCountingHelper::PopulateCallData(grpc_json* json) {
   grpc_json* json_iterator = nullptr;
   if (calls_started_ != 0) {
     json_iterator = grpc_json_add_number_string_child(
@@ -105,10 +92,11 @@ ChannelNode::ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes,
     : BaseNode(is_top_level_channel ? EntityType::kTopLevelChannel
                                     : EntityType::kInternalChannel),
       channel_(channel),
-      target_(UniquePtr<char>(grpc_channel_get_target(channel_))),
-      counter_and_tracer_(channel_tracer_max_nodes) {}
+      target_(UniquePtr<char>(grpc_channel_get_target(channel_))) {
+  trace_.Init(channel_tracer_max_nodes);
+}
 
-ChannelNode::~ChannelNode() {}
+ChannelNode::~ChannelNode() { trace_.Destroy(); }
 
 grpc_json* ChannelNode::RenderJson() {
   // We need to track these three json objects to build our object
@@ -134,9 +122,14 @@ grpc_json* ChannelNode::RenderJson() {
   GPR_ASSERT(target_.get() != nullptr);
   grpc_json_create_child(nullptr, json, "target", target_.get(),
                          GRPC_JSON_STRING, false);
-  // as CallCountingAndTracingNode to populate trace and call count data.
-  counter_and_tracer_.PopulateTrace(json);
-  counter_and_tracer_.PopulateCallData(json);
+  // fill in the channel trace if applicable
+  grpc_json* trace_json = trace_->RenderJson();
+  if (trace_json != nullptr) {
+    trace_json->key = "trace";  // this object is named trace in channelz.proto
+    grpc_json_link_child(json, trace_json, nullptr);
+  }
+  // ask CallCountingHelper to populate trace and call count data.
+  call_counter_.PopulateCallData(json);
   return top_level_json;
 }
 
diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h
index 7bc4567ad2..6e20f31b68 100644
--- a/src/core/lib/channel/channelz.h
+++ b/src/core/lib/channel/channelz.h
@@ -43,7 +43,7 @@ namespace grpc_core {
 namespace channelz {
 
 namespace testing {
-class CallCountingAndTracingNodePeer;
+class CallCountingHelperPeer;
 }
 
 // base class for all channelz entities
@@ -60,7 +60,7 @@ class BaseNode : public RefCounted<BaseNode> {
     kSocket,
   };
 
-  BaseNode(EntityType type);
+  explicit BaseNode(EntityType type);
   virtual ~BaseNode();
 
   // All children must implement this function.
@@ -74,22 +74,20 @@ class BaseNode : public RefCounted<BaseNode> {
   intptr_t uuid() const { return uuid_; }
 
  private:
-  friend class ChannelTrace;
-  EntityType type_;
+  const EntityType type_;
   const intptr_t uuid_;
 };
 
-// This class is the parent for the channelz entities that deal with Channels
+// This class is a helper class for channelz entities that deal with Channels
 // Subchannels, and Servers, since those have similar proto definitions.
 // This class has the ability to:
 //   - track calls_{started,succeeded,failed}
 //   - track last_call_started_timestamp
-//   - hold the channel trace.
-//   - perform common rendering.
-class CallCountingAndTracingNode {
+//   - perform rendering of the above items
+class CallCountingHelper {
  public:
-  CallCountingAndTracingNode(size_t channel_tracer_max_nodes);
-  ~CallCountingAndTracingNode();
+  CallCountingHelper();
+  ~CallCountingHelper();
 
   void RecordCallStarted();
   void RecordCallFailed() {
@@ -98,23 +96,18 @@ class CallCountingAndTracingNode {
   void RecordCallSucceeded() {
     gpr_atm_no_barrier_fetch_add(&calls_succeeded_, (gpr_atm(1)));
   }
-  ChannelTrace* trace() { return trace_.get(); }
-
-  // Common rendering of the channel trace.
-  void PopulateTrace(grpc_json* json);
 
   // Common rendering of the call count data and last_call_started_timestamp.
   void PopulateCallData(grpc_json* json);
 
  private:
   // testing peer friend.
-  friend class testing::CallCountingAndTracingNodePeer;
+  friend class testing::CallCountingHelperPeer;
 
   gpr_atm calls_started_ = 0;
   gpr_atm calls_succeeded_ = 0;
   gpr_atm calls_failed_ = 0;
   gpr_atm last_call_started_millis_ = 0;
-  ManualConstructor<ChannelTrace> trace_;
 };
 
 // Handles channelz bookkeeping for channels
@@ -137,25 +130,31 @@ class ChannelNode : public BaseNode {
 
   bool ChannelIsDestroyed() { return channel_ == nullptr; }
 
-  CallCountingAndTracingNode* counter_and_tracer() {
-    return &counter_and_tracer_;
-  }
+  // proxy methods to composed classes.
+  ChannelTrace* trace() { return trace_.get(); }
+  void RecordCallStarted() { call_counter_.RecordCallStarted(); }
+  void RecordCallFailed() { call_counter_.RecordCallFailed(); }
+  void RecordCallSucceeded() { call_counter_.RecordCallSucceeded(); }
 
  protected:
   // provides view of target for child.
   char* target_view() { return target_.get(); }
+  // provides access to call_counter_ for child.
+  CallCountingHelper* call_counter() { return &call_counter_; }
 
  private:
   grpc_channel* channel_ = nullptr;
   UniquePtr<char> target_;
-  CallCountingAndTracingNode counter_and_tracer_;
+  CallCountingHelper call_counter_;
+  ManualConstructor<ChannelTrace> trace_;
 };
 
 // Handles channelz bookkeeping for servers
 // TODO(ncteisen): implement in subsequent PR.
 class ServerNode : public BaseNode {
  public:
-  ServerNode(size_t channel_tracer_max_nodes) : BaseNode(EntityType::kServer) {}
+  explicit ServerNode(size_t channel_tracer_max_nodes)
+      : BaseNode(EntityType::kServer) {}
   ~ServerNode() override {}
 };
 
diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc
index 03b7cedcdb..a6d626e495 100644
--- a/src/core/lib/surface/call.cc
+++ b/src/core/lib/surface/call.cc
@@ -489,7 +489,7 @@ grpc_error* grpc_call_create(const grpc_call_create_args* args,
   grpc_core::channelz::ChannelNode* channelz_channel =
       grpc_channel_get_channelz_node(call->channel);
   if (channelz_channel != nullptr) {
-    channelz_channel->counter_and_tracer()->RecordCallStarted();
+    channelz_channel->RecordCallStarted();
   }
 
   grpc_slice_unref_internal(path);
@@ -1269,9 +1269,9 @@ static void post_batch_completion(batch_control* bctl) {
         grpc_channel_get_channelz_node(call->channel);
     if (channelz_channel != nullptr) {
       if (*call->final_op.client.status != GRPC_STATUS_OK) {
-        channelz_channel->counter_and_tracer()->RecordCallFailed();
+        channelz_channel->RecordCallFailed();
       } else {
-        channelz_channel->counter_and_tracer()->RecordCallSucceeded();
+        channelz_channel->RecordCallSucceeded();
       }
     }
     GRPC_ERROR_UNREF(error);
diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc
index 16d3322a9d..01caadaaba 100644
--- a/src/core/lib/surface/channel.cc
+++ b/src/core/lib/surface/channel.cc
@@ -170,7 +170,7 @@ grpc_channel* grpc_channel_create_with_builder(
     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->counter_and_tracer()->trace()->AddTraceEvent(
+    channel->channelz_channel->trace()->AddTraceEvent(
         grpc_core::channelz::ChannelTrace::Severity::Info,
         grpc_slice_from_static_string("Channel created"));
   }
@@ -417,7 +417,7 @@ void grpc_channel_internal_unref(grpc_channel* c REF_ARG) {
 static void destroy_channel(void* arg, grpc_error* error) {
   grpc_channel* channel = static_cast<grpc_channel*>(arg);
   if (channel->channelz_channel != nullptr) {
-    channel->channelz_channel->counter_and_tracer()->trace()->AddTraceEvent(
+    channel->channelz_channel->trace()->AddTraceEvent(
         grpc_core::channelz::ChannelTrace::Severity::Info,
         grpc_slice_from_static_string("Channel destroyed"));
     channel->channelz_channel->MarkChannelDestroyed();