Reviewer feedback

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

@@ -79,7 +79,6 @@ typedef struct external_state_watcher {
 } external_state_watcher;
 
 struct grpc_subchannel {
-  intptr_t uuid;
   grpc_connector* connector;
 
   /** refcount
@@ -137,7 +136,7 @@ struct grpc_subchannel {
   /** our alarm */
   grpc_timer alarm;
 
-  grpc_core::ChannelTracer* tracer;
+  grpc_core::ManualConstructor<grpc_core::ChannelTracer> tracer;
 };
 
 struct grpc_subchannel_call {
@@ -344,7 +343,6 @@ grpc_subchannel* grpc_subchannel_create(grpc_connector* connector,
 
   GRPC_STATS_INC_CLIENT_SUBCHANNELS_CREATED();
   c = (grpc_subchannel*)gpr_zalloc(sizeof(*c));
-  c->uuid = -1;
   c->key = key;
   gpr_atm_no_barrier_store(&c->ref_pair, 1 << INTERNAL_REF_BITS);
   c->connector = connector;

src/core/lib/channel/channel_tracer.cc

@@ -35,7 +35,8 @@
 
 namespace grpc_core {
 
-struct TraceEvent {
+class TraceEvent {
+ public:
   TraceEvent(grpc_slice data, grpc_error* error,
              grpc_connectivity_state connectivity_state,
              ChannelTracer* referenced_tracer)
@@ -46,6 +47,10 @@ struct TraceEvent {
     referenced_tracer_ = referenced_tracer ? referenced_tracer->Ref() : nullptr;
     time_created_ = gpr_now(GPR_CLOCK_REALTIME);
   }
+
+ private:
+  friend class ChannelTracer;
+  friend class ChannelTracerRenderer;
   grpc_slice data_;
   grpc_error* error_;
   gpr_timespec time_created_;
@@ -138,8 +143,50 @@ static char* fmt_time(gpr_timespec tm) {
   return full_time_str;
 }
 
+// Helper class that is responsible for walking the tree of ChannelTracer
+// objects and rendering the trace as JSON according to:
+// https://github.com/grpc/proposal/pull/7
+
+// The rendered JSON should be of this format:
+// {
+//   "channelData": {
+//     "numNodesLogged": number,
+//     "startTime": timestamp string,
+//     "nodes": [
+//       {
+//         "uuid": string,
+//         "data": string,
+//         "error": string,
+//         "time": timestamp string,
+//         // can only be one of the states in connectivity_state.h
+//         "state": enum string,
+//         // uuid of referenced subchannel
+//         "subchannel_uuid": string
+//       },
+//     ]
+//   },
+//   "numSubchannelsSeen": number,
+//   "subchannelData": [
+//     {
+//       "uuid": string,
+//       "numNodesLogged": number,
+//       "startTime": timestamp string,
+//       "nodes": [
+//         {
+//           "data": string,
+//           "error": string,
+//           "time": timestamp string,
+//           "state": enum string,
+//         },
+//       ]
+//     },
+//   ]
+// }
+
 class ChannelTracerRenderer {
  public:
+  // If recursive==true, then the entire tree of trace will be rendered.
+  // If not, then only the top level data will be.
   ChannelTracerRenderer(ChannelTracer* tracer, bool recursive)
       : current_tracer_(tracer),
         recursive_(recursive),
@@ -147,17 +194,20 @@ class ChannelTracerRenderer {
         size_(0),
         cap_(0) {}
 
+  // Renders the trace and returns an allocated char* with the formatted JSON
   char* Run() {
     grpc_json* json = grpc_json_create(GRPC_JSON_OBJECT);
     AddSeenTracer(current_tracer_);
     RecursivelyPopulateJson(json);
     gpr_free(seen_tracers_);
-    char* json_str = grpc_json_dump_to_string(json, 1);
+    char* json_str = grpc_json_dump_to_string(json, 0);
     grpc_json_destroy(json);
     return json_str;
   }
 
  private:
+  // tracks that a tracer has already been rendered to avoid infinite
+  // recursion.
   void AddSeenTracer(ChannelTracer* newly_seen) {
     if (size_ >= cap_) {
       cap_ = GPR_MAX(5 * sizeof(newly_seen), 3 * cap_ / 2);
@@ -166,6 +216,7 @@ class ChannelTracerRenderer {
     seen_tracers_[size_++] = newly_seen;
   }
 
+  // Checks if a tracer has already been seen.
   bool TracerAlreadySeen(ChannelTracer* tracer) {
     for (size_t i = 0; i < size_; ++i) {
       if (seen_tracers_[i] == tracer) return true;
@@ -173,6 +224,8 @@ class ChannelTracerRenderer {
     return false;
   }
 
+  // Recursively fills up json by walking over all of the trace of
+  // current_tracer_.
   void RecursivelyPopulateJson(grpc_json* json) {
     grpc_json* channel_data = grpc_json_create_child(
         nullptr, json, "channelData", nullptr, GRPC_JSON_OBJECT, false);
@@ -251,8 +304,14 @@ class ChannelTracerRenderer {
     }
   }
 
+  // Tracks the current tracer we are rendering as we walk the tree of tracers.
   ChannelTracer* current_tracer_;
+
+  // If true, we will render the data of all of this tracer's children.
   bool recursive_;
+
+  // These members are used to track tracers we have already rendered. This is
+  // a dynamically growing array that is deallocated when the rendering is done.
   ChannelTracer** seen_tracers_;
   size_t size_;
   size_t cap_;

src/core/lib/channel/channel_tracer.h

@@ -25,7 +25,7 @@
 
 namespace grpc_core {
 
-struct TraceEvent;
+class TraceEvent;
 
 class ChannelTracer {
  public:
@@ -55,6 +55,7 @@ class ChannelTracer {
   static char* GetChannelTraceFromUuid(intptr_t uuid, bool recursive);
 
  private:
+  // Internal helper that frees a TraceEvent.
   void FreeNode(TraceEvent* node);
 
   friend class ChannelTracerRenderer;

src/core/lib/support/object_registry.h

@@ -21,8 +21,13 @@
 
 #include <stdint.h>
 
+// Different types that may be stored in the general object registry
 typedef enum {
+  // Used to hold uuid -> ChannelTracer mappings to allow for the trace data
+  // to be looked up by uuid, rather then have to walk the entire tree of
+  // trace.
   GRPC_OBJECT_REGISTRY_CHANNEL_TRACER,
+  // Usually represents an error has occurred in the object lookup.
   GRPC_OBJECT_REGISTRY_UNKNOWN,
 } grpc_object_registry_type;
 

test/core/channel/channel_tracer_test.cc

@@ -71,6 +71,8 @@ static void validate_children(ChannelTracer* tracer,
   gpr_free(json_str);
 }
 
+// Tests basic ChannelTracer functionality like construction, adding trace, and
+// lookups by uuid.
 static void test_basic_channel_tracing(size_t max_nodes) {
   grpc_core::ExecCtx exec_ctx;
   ChannelTracer tracer(max_nodes);
@@ -97,6 +99,8 @@ static void test_basic_channel_tracing(size_t max_nodes) {
   tracer.Unref();
 }
 
+// Calls basic test with various values for max_nodes (including 0, which turns
+// the tracer off).
 static void test_basic_channel_sizing() {
   test_basic_channel_tracing(0);
   test_basic_channel_tracing(1);
@@ -106,6 +110,9 @@ static void test_basic_channel_sizing() {
   test_basic_channel_tracing(15);
 }
 
+// Tests more complex functionality, like a parent channel tracking
+// subchannles. This exercises the ref/unref patterns since the parent tracer
+// and this function will both hold refs to the subchannel.
 static void test_complex_channel_tracing(size_t max_nodes) {
   grpc_core::ExecCtx exec_ctx;
   ChannelTracer tracer(max_nodes);
@@ -145,6 +152,7 @@ static void test_complex_channel_tracing(size_t max_nodes) {
   tracer.Unref();
 }
 
+// Calls the complex test with a sweep of sizes for max_nodes.
 static void test_complex_channel_sizing() {
   test_complex_channel_tracing(0);
   test_complex_channel_tracing(1);
@@ -154,6 +162,9 @@ static void test_complex_channel_sizing() {
   test_complex_channel_tracing(15);
 }
 
+// Tests edge case in which the parent channel tracer is destroyed before the
+// subchannel. Not sure if it is a realistic scenario, but the refcounting
+// balance should still hold.
 static void test_delete_parent_first() {
   grpc_core::ExecCtx exec_ctx;
   ChannelTracer tracer(3);
@@ -167,6 +178,9 @@ static void test_delete_parent_first() {
   sc1.Unref();
 }
 
+// Test a case in which the parent channel has subchannels and the subchannels
+// have connections. Ensures that everything lives as long as it should then
+// gets deleted.
 static void test_nesting() {
   grpc_core::ExecCtx exec_ctx;
   ChannelTracer tracer(10);