reviewer feedback

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

@@ -77,7 +77,14 @@ class SubchannelNode : public BaseNode {
   grpc_json* RenderJson() override;
 
   // proxy methods to composed classes.
-  ChannelTrace* trace() { return trace_.get(); }
+  void AddTraceEvent(ChannelTrace::Severity severity, grpc_slice data) {
+    trace_->AddTraceEvent(severity, data);
+  }
+  void AddTraceEventWithReference(ChannelTrace::Severity severity,
+                                  grpc_slice data,
+                                  RefCountedPtr<BaseNode> referenced_channel) {
+    trace_->AddTraceEventWithReference(severity, data, referenced_channel);
+  }
   void RecordCallStarted() { call_counter_.RecordCallStarted(); }
   void RecordCallFailed() { call_counter_.RecordCallFailed(); }
   void RecordCallSucceeded() { call_counter_.RecordCallSucceeded(); }

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->trace()->AddTraceEvent(
+    c->channelz_subchannel->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->trace()->AddTraceEvent(
+    c->channelz_subchannel->AddTraceEvent(
         grpc_core::channelz::ChannelTrace::Severity::Info,
         grpc_slice_from_static_string("Subchannel created"));
   }

src/core/lib/channel/channelz.h

@@ -44,7 +44,8 @@ namespace channelz {
 
 namespace testing {
 class CallCountingHelperPeer;
-}
+class ChannelNodePeer;
+}  // namespace testing
 
 // base class for all channelz entities
 class BaseNode : public RefCounted<BaseNode> {
@@ -131,7 +132,14 @@ class ChannelNode : public BaseNode {
   bool ChannelIsDestroyed() { return channel_ == nullptr; }
 
   // proxy methods to composed classes.
-  ChannelTrace* trace() { return trace_.get(); }
+  void AddTraceEvent(ChannelTrace::Severity severity, grpc_slice data) {
+    trace_->AddTraceEvent(severity, data);
+  }
+  void AddTraceEventWithReference(ChannelTrace::Severity severity,
+                                  grpc_slice data,
+                                  RefCountedPtr<BaseNode> referenced_channel) {
+    trace_->AddTraceEventWithReference(severity, data, referenced_channel);
+  }
   void RecordCallStarted() { call_counter_.RecordCallStarted(); }
   void RecordCallFailed() { call_counter_.RecordCallFailed(); }
   void RecordCallSucceeded() { call_counter_.RecordCallSucceeded(); }
@@ -141,8 +149,12 @@ class ChannelNode : public BaseNode {
   char* target_view() { return target_.get(); }
   // provides access to call_counter_ for child.
   CallCountingHelper* call_counter() { return &call_counter_; }
+  // provides access to channel trace for child.
+  ChannelTrace* trace() { return trace_.get(); }
 
  private:
+  // to allow the channel trace test to access trace();
+  friend class testing::ChannelNodePeer;
   grpc_channel* channel_ = nullptr;
   UniquePtr<char> target_;
   CallCountingHelper call_counter_;

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->trace()->AddTraceEvent(
+    channel->channelz_channel->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->trace()->AddTraceEvent(
+    channel->channelz_channel->AddTraceEvent(
         grpc_core::channelz::ChannelTrace::Severity::Info,
         grpc_slice_from_static_string("Channel destroyed"));
     channel->channelz_channel->MarkChannelDestroyed();

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:
+  ChannelNodePeer(ChannelNode* node) : node_(node) {}
+  ChannelTrace* trace() { return node_->trace_.get(); }
+
+ private:
+  ChannelNode* node_;
+};
+
 namespace {
 
 grpc_json* GetJsonChild(grpc_json* parent, const char* key) {
@@ -156,18 +167,19 @@ TEST_P(ChannelTracerTest, ComplexTest) {
   ChannelFixture channel1(GetParam());
   RefCountedPtr<ChannelNode> sc1 =
       MakeRefCounted<ChannelNode>(channel1.channel(), GetParam(), true);
+  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());
-  AddSimpleTrace(sc1->trace());
+  AddSimpleTrace(sc1_peer.trace());
-  AddSimpleTrace(sc1->trace());
+  AddSimpleTrace(sc1_peer.trace());
-  ValidateChannelTrace(sc1->trace(), 3, GetParam());
+  ValidateChannelTrace(sc1_peer.trace(), 3, GetParam());
-  AddSimpleTrace(sc1->trace());
+  AddSimpleTrace(sc1_peer.trace());
-  AddSimpleTrace(sc1->trace());
+  AddSimpleTrace(sc1_peer.trace());
-  AddSimpleTrace(sc1->trace());
+  AddSimpleTrace(sc1_peer.trace());
-  ValidateChannelTrace(sc1->trace(), 6, GetParam());
+  ValidateChannelTrace(sc1_peer.trace(), 6, GetParam());
   AddSimpleTrace(&tracer);
   AddSimpleTrace(&tracer);
   ValidateChannelTrace(&tracer, 5, GetParam());
@@ -203,24 +215,26 @@ TEST_P(ChannelTracerTest, TestNesting) {
   ChannelFixture channel1(GetParam());
   RefCountedPtr<ChannelNode> sc1 =
       MakeRefCounted<ChannelNode>(channel1.channel(), GetParam(), true);
+  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()->AddTraceEventWithReference(
+  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);