Build out the channelz unit test

4 files changed, 67 insertions, 44 deletions

diff --git a/src/core/lib/channel/channel_trace.cc b/src/core/lib/channel/channel_trace.cc
index efa034a188..c82e42d545 100644
--- a/src/core/lib/channel/channel_trace.cc
+++ b/src/core/lib/channel/channel_trace.cc
@@ -28,7 +28,6 @@
 #include <stdlib.h>
 #include <string.h>
 
-#include "src/core/lib/channel/channelz_registry.h"
 #include "src/core/lib/channel/status_util.h"
 #include "src/core/lib/gpr/string.h"
 #include "src/core/lib/gpr/useful.h"
@@ -63,15 +62,13 @@ ChannelTrace::TraceEvent::TraceEvent(Severity severity, grpc_slice data)
 ChannelTrace::TraceEvent::~TraceEvent() { grpc_slice_unref_internal(data_); }
 
 ChannelTrace::ChannelTrace(size_t max_events)
-    : channel_uuid_(-1),
-      num_events_logged_(0),
+    : num_events_logged_(0),
       list_size_(0),
       max_list_size_(max_events),
       head_trace_(nullptr),
       tail_trace_(nullptr) {
   if (max_list_size_ == 0) return;  // tracing is disabled if max_events == 0
   gpr_mu_init(&tracer_mu_);
-  channel_uuid_ = ChannelzRegistry::Register(this);
   time_created_ = grpc_millis_to_timespec(grpc_core::ExecCtx::Get()->Now(),
                                           GPR_CLOCK_REALTIME);
 }
@@ -84,12 +81,9 @@ ChannelTrace::~ChannelTrace() {
     it = it->next();
     Delete<TraceEvent>(to_free);
   }
-  ChannelzRegistry::Unregister(channel_uuid_);
   gpr_mu_destroy(&tracer_mu_);
 }
 
-intptr_t ChannelTrace::GetUuid() const { return channel_uuid_; }
-
 void ChannelTrace::AddTraceEventHelper(TraceEvent* new_trace_event) {
   ++num_events_logged_;
   // first event case
@@ -212,7 +206,7 @@ void ChannelTrace::TraceEvent::RenderTraceEvent(grpc_json* json) const {
   }
 }
 
-char* ChannelTrace::RenderTrace() const {
+grpc_json* ChannelTrace::RenderJSON() const {
   if (!max_list_size_)
     return nullptr;  // tracing is disabled if max_events == 0
   grpc_json* json = grpc_json_create(GRPC_JSON_OBJECT);
@@ -235,6 +229,12 @@ char* ChannelTrace::RenderTrace() const {
     it->RenderTraceEvent(json_iterator);
     it = it->next();
   }
+  return json;
+}
+
+char* ChannelTrace::RenderTrace() const {
+  grpc_json* json = RenderJSON();
+  if (json == nullptr) return nullptr;
   char* json_str = grpc_json_dump_to_string(json, 0);
   grpc_json_destroy(json);
   return json_str;
diff --git a/src/core/lib/channel/channel_trace.h b/src/core/lib/channel/channel_trace.h
index 687c6bc063..b1224bb96c 100644
--- a/src/core/lib/channel/channel_trace.h
+++ b/src/core/lib/channel/channel_trace.h
@@ -40,9 +40,6 @@ class ChannelTrace {
   ChannelTrace(size_t max_events);
   ~ChannelTrace();
 
-  // returns the tracer's uuid
-  intptr_t GetUuid() const;
-
   enum Severity {
     Unset = 0,  // never to be used
     Info,       // we start at 1 to avoid using proto default values
@@ -72,8 +69,13 @@ class ChannelTrace {
       Severity severity, grpc_slice data,
       RefCountedPtr<Channel> referenced_tracer);
 
-  // Returns the tracing data rendered as a grpc json string.
-  // The string is owned by the caller and must be freed.
+  // Creates and returns the raw grpc_json object, so a parent channelz
+  // object may incorporate the json before rendering.
+  grpc_json* RenderJSON() const;
+
+  // Returns the tracing data rendered as a grpc json string. The string
+  // is owned by the caller and must be freed. This is used for testing only
+  // so that we may unit test ChannelTrace in isolation.
   char* RenderTrace() const;
 
  private:
@@ -117,7 +119,6 @@ class ChannelTrace {
   void AddTraceEventHelper(TraceEvent* new_trace_event);
 
   gpr_mu tracer_mu_;
-  intptr_t channel_uuid_;
   uint64_t num_events_logged_;
   size_t list_size_;
   size_t max_list_size_;
diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc
index cf7545621e..5c3dc97e69 100644
--- a/src/core/lib/channel/channelz.cc
+++ b/src/core/lib/channel/channelz.cc
@@ -39,9 +39,6 @@
 #include "src/core/lib/transport/connectivity_state.h"
 #include "src/core/lib/transport/error_utils.h"
 
-// TODO(ncteisen): actually implement this
-char* grpc_channelz_get_channel(intptr_t channel_id) { return nullptr; }
-
 namespace grpc_core {
 namespace channelz {
 
@@ -82,9 +79,9 @@ char* fmt_time(gpr_timespec tm) {
 
 // TODO(ncteisen); move this to json library
 grpc_json* add_num_str(grpc_json* parent, grpc_json* it, const char* name,
-                       uint64_t num) {
+                       int64_t num) {
   char* num_str;
-  gpr_asprintf(&num_str, "%" PRIu64, num);
+  gpr_asprintf(&num_str, "%" PRId64, num);
   return grpc_json_create_child(it, parent, name, num_str, GRPC_JSON_STRING,
                                 true);
 }
@@ -96,10 +93,13 @@ Channel::Channel(grpc_channel* channel, size_t channel_tracer_max_nodes)
   trace_.Init(channel_tracer_max_nodes);
   target_ = grpc_channel_get_target(channel_);
   channel_uuid_ = ChannelzRegistry::Register(this);
+  last_call_started_timestamp_ =
+      grpc_millis_to_timespec(ExecCtx::Get()->Now(), GPR_CLOCK_REALTIME);
 }
 
 Channel::~Channel() {
   gpr_free(const_cast<char*>(target_));
+  trace_.Destroy();
   ChannelzRegistry::Unregister(channel_uuid_);
 }
 
@@ -125,7 +125,7 @@ char* Channel::RenderJSON() {
 
   // create and fill the ref child
   json_iterator = grpc_json_create_child(json_iterator, json, "ref", nullptr,
-                                         GRPC_JSON_OBJECT, true);
+                                         GRPC_JSON_OBJECT, false);
   json = json_iterator;
   json_iterator = nullptr;
   json_iterator = add_num_str(json, json_iterator, "channelId", channel_uuid_);
@@ -134,33 +134,55 @@ char* Channel::RenderJSON() {
   json = top_level_json;
   json_iterator = nullptr;
 
-  // create and fill the data child
-  json_iterator = grpc_json_create_child(json_iterator, json, "data", nullptr,
-                                         GRPC_JSON_OBJECT, true);
-  json = json_iterator;
+  // create and fill the data child.
+  grpc_json* data = grpc_json_create_child(json_iterator, json, "data", nullptr,
+                                           GRPC_JSON_OBJECT, false);
+
+  json = data;
   json_iterator = nullptr;
-  json_iterator =
-      add_num_str(json, json_iterator, "callsStarted", calls_started_);
-  json_iterator =
-      add_num_str(json, json_iterator, "callsSucceeded", calls_succeeded_);
-  json_iterator =
-      add_num_str(json, json_iterator, "callsFailed", calls_failed_);
-  json_iterator = grpc_json_create_child(
-      json_iterator, json, "lastCallStartedTimestamp",
-      fmt_time(last_call_started_timestamp_), GRPC_JSON_STRING, true);
+
+  // create and fill the connectivity state child.
+  grpc_connectivity_state connectivity_state = GetConnectivityState();
+  json_iterator = grpc_json_create_child(json_iterator, json, "state", nullptr,
+                                         GRPC_JSON_OBJECT, false);
+  json = json_iterator;
+  grpc_json_create_child(nullptr, json, "state",
+                         grpc_connectivity_state_name(connectivity_state),
+                         GRPC_JSON_STRING, false);
+
+  // reset the parent to be the data object.
+  json = data;
   json_iterator = grpc_json_create_child(json_iterator, json, "target", target_,
                                          GRPC_JSON_STRING, false);
-  grpc_connectivity_state connectivity_state = GetConnectivityState();
-  json_iterator =
-      grpc_json_create_child(json_iterator, json, "state",
-                             grpc_connectivity_state_name(connectivity_state),
-                             GRPC_JSON_STRING, false);
-  char* trace = trace_->RenderTrace();
+
+  // fill in the channel trace if applicable
+  grpc_json* trace = trace_->RenderJSON();
   if (trace != nullptr) {
-    json_iterator = grpc_json_create_child(json_iterator, json, "trace", trace,
-                                           GRPC_JSON_STRING, true);
+    // we manuall 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;
+    trace->value = nullptr;
+    trace->key = "trace";
+    trace->owns_value = false;
   }
 
+  // reset the parent to be the data object.
+  json = data;
+  json_iterator = nullptr;
+
+  // We use -1 as sentinel values since proto default value for integers is
+  // zero, and the confuses the parser into thinking the value weren't present
+  json_iterator = add_num_str(json, json_iterator, "callsStarted",
+                              calls_started_ ? calls_started_ : -1);
+  json_iterator = add_num_str(json, json_iterator, "callsSucceeded",
+                              calls_succeeded_ ? calls_succeeded_ : -1);
+  json_iterator = add_num_str(json, json_iterator, "callsFailed",
+                              calls_failed_ ? calls_failed_ : -1);
+  json_iterator = grpc_json_create_child(
+      json_iterator, json, "lastCallStartedTimestamp",
+      fmt_time(last_call_started_timestamp_), GRPC_JSON_STRING, true);
+
   // render and return the over json object
   char* json_str = grpc_json_dump_to_string(top_level_json, 0);
   grpc_json_destroy(top_level_json);
diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h
index 7cea00392f..63b90e90a0 100644
--- a/src/core/lib/channel/channelz.h
+++ b/src/core/lib/channel/channelz.h
@@ -59,9 +59,9 @@ class Channel : public RefCounted<Channel> {
   bool channel_destroyed_ = false;
   grpc_channel* channel_;
   const char* target_;
-  uint64_t calls_started_ = 0;
-  uint64_t calls_succeeded_ = 0;
-  uint64_t calls_failed_ = 0;
+  int64_t calls_started_ = 0;
+  int64_t calls_succeeded_ = 0;
+  int64_t calls_failed_ = 0;
   gpr_timespec last_call_started_timestamp_;
   intptr_t channel_uuid_;
   ManualConstructor<ChannelTrace> trace_;