Reviewer feedback

include/grpc/impl/codegen/grpc_types.h

@@ -290,7 +290,8 @@ typedef struct {
 #define GRPC_ARG_MAX_CHANNEL_TRACE_EVENTS_PER_NODE \
   "grpc.max_channel_trace_events_per_node"
 /** If non-zero, gRPC library will track stats and information at at per channel
- * level. Disabling channelz naturally disabled channel tracing. */
+ * level. Disabling channelz naturally disables channel tracing. The default
+ * is for channelz to be disabled. */
 #define GRPC_ARG_ENABLE_CHANNELZ "grpc.enable_channelz"
 /** If non-zero, Cronet transport will coalesce packets to fewer frames
  * when possible. */

src/core/lib/channel/channelz.cc

@@ -89,14 +89,9 @@ grpc_json* add_num_str(grpc_json* parent, grpc_json* it, const char* name,
 
 }  // namespace
 
-ChannelNode::ChannelNode(bool enabled, grpc_channel* channel,
-                         size_t channel_tracer_max_nodes)
-    : enabled_(enabled),
-      channel_(channel),
-      target_(nullptr),
-      channel_uuid_(-1) {
+ChannelNode::ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes)
+    : channel_(channel), target_(nullptr), channel_uuid_(-1) {
   trace_.Init(channel_tracer_max_nodes);
-  if (!enabled_) return;
   target_ = UniquePtr<char>(grpc_channel_get_target(channel_));
   channel_uuid_ = ChannelzRegistry::Register(this);
   gpr_atm_no_barrier_store(&last_call_started_millis_,
@@ -105,12 +100,10 @@ ChannelNode::ChannelNode(bool enabled, grpc_channel* channel,
 
 ChannelNode::~ChannelNode() {
   trace_.Destroy();
-  if (!enabled_) return;
   ChannelzRegistry::Unregister(channel_uuid_);
 }
 
 void ChannelNode::RecordCallStarted() {
-  if (!enabled_) return;
   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());
@@ -125,7 +118,6 @@ grpc_connectivity_state ChannelNode::GetConnectivityState() {
 }
 
 char* ChannelNode::RenderJSON() {
-  if (!enabled_) return nullptr;
   // We need to track these three json objects to build our object
   grpc_json* top_level_json = grpc_json_create(GRPC_JSON_OBJECT);
   grpc_json* json = top_level_json;

src/core/lib/channel/channelz.h

@@ -40,17 +40,14 @@ class ChannelNodePeer;
 
 class ChannelNode : public RefCounted<ChannelNode> {
  public:
-  ChannelNode(bool enabled, grpc_channel* channel,
-              size_t channel_tracer_max_nodes);
+  ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes);
   ~ChannelNode();
 
   void RecordCallStarted();
   void RecordCallFailed() {
-    if (!enabled_) return;
     gpr_atm_no_barrier_fetch_add(&calls_failed_, (gpr_atm(1)));
   }
   void RecordCallSucceeded() {
-    if (!enabled_) return;
     gpr_atm_no_barrier_fetch_add(&calls_succeeded_, (gpr_atm(1)));
   }
 
@@ -59,7 +56,6 @@ class ChannelNode : public RefCounted<ChannelNode> {
   ChannelTrace* trace() { return trace_.get(); }
 
   void set_channel_destroyed() {
-    if (!enabled_) return;
     GPR_ASSERT(channel_ != nullptr);
     channel_ = nullptr;
   }
@@ -73,8 +69,6 @@ class ChannelNode : public RefCounted<ChannelNode> {
   // helper for getting connectivity state.
   grpc_connectivity_state GetConnectivityState();
 
-  // Not owned. Will be set to nullptr when the channel is destroyed.
-  const bool enabled_;
   grpc_channel* channel_ = nullptr;
   UniquePtr<char> target_;
   gpr_atm calls_started_ = 0;

src/core/lib/surface/call.cc

@@ -491,7 +491,9 @@ grpc_error* grpc_call_create(const grpc_call_create_args* args,
 
   grpc_core::channelz::ChannelNode* channelz_channel =
       grpc_channel_get_channelz_node(call->channel);
-  channelz_channel->RecordCallStarted();
+  if (channelz_channel != nullptr) {
+    channelz_channel->RecordCallStarted();
+  }
 
   grpc_slice_unref_internal(path);
 
@@ -1264,10 +1266,12 @@ static void post_batch_completion(batch_control* bctl) {
     }
     grpc_core::channelz::ChannelNode* channelz_channel =
         grpc_channel_get_channelz_node(call->channel);
-    if (*call->final_op.client.status != GRPC_STATUS_OK) {
-      channelz_channel->RecordCallFailed();
-    } else {
-      channelz_channel->RecordCallSucceeded();
+    if (channelz_channel != nullptr) {
+      if (*call->final_op.client.status != GRPC_STATUS_OK) {
+        channelz_channel->RecordCallFailed();
+      } else {
+        channelz_channel->RecordCallSucceeded();
+      }
     }
     GRPC_ERROR_UNREF(error);
     error = GRPC_ERROR_NONE;

src/core/lib/surface/channel.cc

@@ -149,12 +149,14 @@ grpc_channel* grpc_channel_create_with_builder(
   }
 
   grpc_channel_args_destroy(args);
-  channel->channelz_channel =
-      grpc_core::MakeRefCounted<grpc_core::channelz::ChannelNode>(
-          channelz_enabled, channel, channel_tracer_max_nodes);
-  channel->channelz_channel->trace()->AddTraceEvent(
-      grpc_core::channelz::ChannelTrace::Severity::Info,
-      grpc_slice_from_static_string("Channel created"));
+  if (channelz_enabled) {
+    channel->channelz_channel =
+        grpc_core::MakeRefCounted<grpc_core::channelz::ChannelNode>(
+            channel, channel_tracer_max_nodes);
+    channel->channelz_channel->trace()->AddTraceEvent(
+        grpc_core::channelz::ChannelTrace::Severity::Info,
+        grpc_slice_from_static_string("Channel created"));
+  }
   return channel;
 }
 
@@ -189,10 +191,6 @@ static grpc_channel_args* build_channel_args(
   return grpc_channel_args_copy_and_add(input_args, new_args, num_new_args);
 }
 
-char* grpc_channel_render_channelz(grpc_channel* channel) {
-  return channel->channelz_channel->RenderJSON();
-}
-
 grpc_core::channelz::ChannelNode* grpc_channel_get_channelz_node(
     grpc_channel* channel) {
   return channel->channelz_channel.get();
@@ -401,8 +399,10 @@ 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);
-  channel->channelz_channel->set_channel_destroyed();
-  channel->channelz_channel.reset();
+  if (channel->channelz_channel != nullptr) {
+    channel->channelz_channel->set_channel_destroyed();
+    channel->channelz_channel.reset();
+  }
   grpc_channel_stack_destroy(CHANNEL_STACK_FROM_CHANNEL(channel));
   while (channel->registered_calls) {
     registered_call* rc = channel->registered_calls;

test/core/channel/channel_trace_test.cc

@@ -157,7 +157,7 @@ TEST_P(ChannelTracerTest, ComplexTest) {
   AddSimpleTrace(&tracer);
   ChannelFixture channel1(GetParam());
   RefCountedPtr<ChannelNode> sc1 =
-      MakeRefCounted<ChannelNode>(true, channel1.channel(), GetParam());
+      MakeRefCounted<ChannelNode>(channel1.channel(), GetParam());
   tracer.AddTraceEventReferencingSubchannel(
       ChannelTrace::Severity::Info,
       grpc_slice_from_static_string("subchannel one created"), sc1);
@@ -175,7 +175,7 @@ TEST_P(ChannelTracerTest, ComplexTest) {
   ValidateChannelTrace(&tracer, 5, GetParam());
   ChannelFixture channel2(GetParam());
   RefCountedPtr<ChannelNode> sc2 =
-      MakeRefCounted<ChannelNode>(true, channel2.channel(), GetParam());
+      MakeRefCounted<ChannelNode>(channel2.channel(), GetParam());
   tracer.AddTraceEventReferencingChannel(
       ChannelTrace::Severity::Info,
       grpc_slice_from_static_string("LB channel two created"), sc2);
@@ -204,7 +204,7 @@ TEST_P(ChannelTracerTest, TestNesting) {
   ValidateChannelTrace(&tracer, 2, GetParam());
   ChannelFixture channel1(GetParam());
   RefCountedPtr<ChannelNode> sc1 =
-      MakeRefCounted<ChannelNode>(true, channel1.channel(), GetParam());
+      MakeRefCounted<ChannelNode>(channel1.channel(), GetParam());
   tracer.AddTraceEventReferencingChannel(
       ChannelTrace::Severity::Info,
       grpc_slice_from_static_string("subchannel one created"), sc1);
@@ -212,7 +212,7 @@ TEST_P(ChannelTracerTest, TestNesting) {
   AddSimpleTrace(sc1->trace());
   ChannelFixture channel2(GetParam());
   RefCountedPtr<ChannelNode> conn1 =
-      MakeRefCounted<ChannelNode>(true, channel2.channel(), GetParam());
+      MakeRefCounted<ChannelNode>(channel2.channel(), GetParam());
   // nesting one level deeper.
   sc1->trace()->AddTraceEventReferencingSubchannel(
       ChannelTrace::Severity::Info,
@@ -225,7 +225,7 @@ TEST_P(ChannelTracerTest, TestNesting) {
   ValidateChannelTrace(conn1->trace(), 1, GetParam());
   ChannelFixture channel3(GetParam());
   RefCountedPtr<ChannelNode> sc2 =
-      MakeRefCounted<ChannelNode>(true, channel3.channel(), GetParam());
+      MakeRefCounted<ChannelNode>(channel3.channel(), GetParam());
   tracer.AddTraceEventReferencingSubchannel(
       ChannelTrace::Severity::Info,
       grpc_slice_from_static_string("subchannel two created"), sc2);

test/core/channel/channelz_test.cc

@@ -151,8 +151,7 @@ TEST(ChannelzChannelTest, ChannelzDisabled) {
   grpc_channel* channel =
       grpc_insecure_channel_create("fake_target", nullptr, nullptr);
   ChannelNode* channelz_channel = grpc_channel_get_channelz_node(channel);
-  char* json_str = channelz_channel->RenderJSON();
-  ASSERT_EQ(json_str, nullptr);
+  ASSERT_EQ(channelz_channel, nullptr);
   grpc_channel_destroy(channel);
 }
 

test/core/end2end/tests/channelz.cc

@@ -208,6 +208,7 @@ static void test_channelz(grpc_end2end_test_config config) {
   grpc_core::channelz::ChannelNode* channelz_channel =
       grpc_channel_get_channelz_node(f.client);
 
+  GPR_ASSERT(channelz_channel);
   char* json = channelz_channel->RenderJSON();
   GPR_ASSERT(json != nullptr);
   GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"0\""));
@@ -262,6 +263,7 @@ static void test_channelz_with_channel_trace(grpc_end2end_test_config config) {
   grpc_core::channelz::ChannelNode* channelz_channel =
       grpc_channel_get_channelz_node(f.client);
 
+  GPR_ASSERT(channelz_channel);
   char* json = channelz_channel->RenderJSON();
   GPR_ASSERT(json != nullptr);
   gpr_log(GPR_INFO, "%s", json);
@@ -280,49 +282,10 @@ static void test_channelz_disabled(grpc_end2end_test_config config) {
   f = begin_test(config, "test_channelz_disabled", nullptr, nullptr);
   grpc_core::channelz::ChannelNode* channelz_channel =
       grpc_channel_get_channelz_node(f.client);
-  char* json_str = channelz_channel->RenderJSON();
-  GPR_ASSERT(json_str == nullptr);
-  grpc_json* json = channelz_channel->trace()->RenderJSON();
-  GPR_ASSERT(json == nullptr);
+  GPR_ASSERT(channelz_channel == nullptr);
   // one successful request
   run_one_request(config, f, true);
-  json_str = channelz_channel->RenderJSON();
-  GPR_ASSERT(json_str == nullptr);
-  GPR_ASSERT(json == nullptr);
-  end_test(&f);
-  config.tear_down_data(&f);
-}
-
-static void test_channelz_disabled_with_channel_trace(
-    grpc_end2end_test_config config) {
-  grpc_end2end_test_fixture f;
-
-  grpc_arg client_a;
-  client_a.type = GRPC_ARG_INTEGER;
-  client_a.key = const_cast<char*>(GRPC_ARG_MAX_CHANNEL_TRACE_EVENTS_PER_NODE);
-  client_a.value.integer = 5;
-  grpc_channel_args client_args = {1, &client_a};
-
-  f = begin_test(config, "test_channelz_disabled_with_channel_trace",
-                 &client_args, nullptr);
-  grpc_core::channelz::ChannelNode* channelz_channel =
-      grpc_channel_get_channelz_node(f.client);
-  // channelz is disabled so rendering return null.
-  char* json_str = channelz_channel->RenderJSON();
-  GPR_ASSERT(json_str == nullptr);
-  // channel trace is explicitly requested, so this works as it should
-  grpc_json* json = channelz_channel->trace()->RenderJSON();
-  GPR_ASSERT(json != nullptr);
-  json_str = grpc_json_dump_to_string(json, 0);
-  GPR_ASSERT(json_str != nullptr);
-  gpr_log(GPR_INFO, "%s", json_str);
-  GPR_ASSERT(nullptr !=
-             strstr(json_str, "\"description\":\"Channel created\""));
-  GPR_ASSERT(nullptr != strstr(json_str, "\"severity\":\"CT_INFO\""));
-  GPR_ASSERT(nullptr != strstr(json_str, "\"numEventsLogged\":"));
-  grpc_json_destroy(json);
-  gpr_free(json_str);
-
+  GPR_ASSERT(channelz_channel == nullptr);
   end_test(&f);
   config.tear_down_data(&f);
 }
@@ -331,7 +294,6 @@ void channelz(grpc_end2end_test_config config) {
   test_channelz(config);
   test_channelz_with_channel_trace(config);
   test_channelz_disabled(config);
-  test_channelz_disabled_with_channel_trace(config);
 }
 
 void channelz_pre_init(void) {}