reviewer comments

include/grpc/grpc.h

@@ -286,6 +286,14 @@ GRPCAPI grpc_channel* grpc_lame_client_channel_create(
 /** Close and destroy a grpc channel */
 GRPCAPI void grpc_channel_destroy(grpc_channel* channel);
 
+// Returns the JSON formatted channel trace for this channel. If recursive
+// is true, it will render all of the trace for this channel's subchannels.
+GRPCAPI char* grpc_channel_get_trace(grpc_channel* channel, bool recursive);
+
+// Returns the channel uuid, which can be used to look up its trace at a
+// later time.
+GRPCAPI intptr_t grpc_channel_get_uuid(grpc_channel* channel);
+
 /** Error handling for grpc_call
    Most grpc_call functions return a grpc_error. If the error is not GRPC_OK
    then the operation failed due to some unsatisfied precondition.

src/core/lib/channel/object_registry.cc

@@ -18,6 +18,7 @@
 
 #include "src/core/lib/channel/object_registry.h"
 #include "src/core/lib/avl/avl.h"
+#include "src/core/lib/gpr/useful.h"
 
 #include <grpc/support/alloc.h>
 #include <grpc/support/log.h>
@@ -25,7 +26,7 @@
 // file global lock and avl.
 static gpr_mu g_mu;
 static grpc_avl g_avl;
-static intptr_t g_uuid = 0;
+static gpr_atm g_uuid = 0;
 
 typedef struct {
   void* object;
@@ -37,7 +38,7 @@ typedef struct {
 static void destroy_intptr(void* not_used, void* user_data) {}
 static void* copy_intptr(void* key, void* user_data) { return key; }
 static long compare_intptr(void* key1, void* key2, void* user_data) {
-  return (intptr_t)key1 - (intptr_t)key2;
+  return GPR_ICMP(key1, key2);
 }
 
 static void destroy_tracker(void* tracker, void* user_data) {

src/core/lib/channel/object_registry.h

@@ -21,7 +21,10 @@
 
 #include <stdint.h>
 
-// Different types that may be stored in the general object registry
+// TODO(ncteisen): convert this file to C++
+
+// Different types that may be stored in the general object registry. For now,
+// the only use case is channel tracers, but the design has been left general.
 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
@@ -34,9 +37,13 @@ typedef enum {
 void grpc_object_registry_init();
 void grpc_object_registry_shutdown();
 
+// globally registers the object. Returns its unique uuid
 intptr_t grpc_object_registry_register_object(void* object,
                                               grpc_object_registry_type type);
+// globally unregisters the object that is associated to uuid.
 void grpc_object_registry_unregister_object(intptr_t uuid);
+// if object with uuid has previously been registered, stores it in *object.
+// if not, returns GRPC_OBJECT_REGISTRY_UNKNOWN and sets *object unchanged.
 grpc_object_registry_type grpc_object_registry_get_object(intptr_t uuid,
                                                           void** object);
 

src/core/lib/surface/channel.cc

@@ -173,8 +173,8 @@ grpc_channel* grpc_channel_create_with_builder(
     } else if (0 == strcmp(args->args[i].key,
                            GRPC_ARG_MAX_CHANNEL_TRACE_EVENTS_PER_NODE)) {
       GPR_ASSERT(channel_tracer_max_nodes == 0);
-      // max_nodes defaults to 10, clamped between 0 and 100.
-      const grpc_integer_options options = {10, 0, 100};
+      // max_nodes defaults to 0 (which is off), clamped between 0 and 100.
+      const grpc_integer_options options = {0, 0, 100};
       channel_tracer_max_nodes =
           (size_t)grpc_channel_arg_get_integer(&args->args[i], options);
     }
@@ -405,7 +405,7 @@ static void destroy_channel(void* arg, grpc_error* error) {
     GRPC_MDELEM_UNREF(rc->authority);
     gpr_free(rc);
   }
-  channel->tracer.reset(nullptr);
+  channel->tracer.reset();
   GRPC_MDELEM_UNREF(channel->default_authority);
   gpr_mu_destroy(&channel->registered_call_mu);
   gpr_free(channel->target);

src/core/lib/surface/channel.h

@@ -60,14 +60,6 @@ grpc_mdelem grpc_channel_get_reffed_status_elem(grpc_channel* channel,
 size_t grpc_channel_get_call_size_estimate(grpc_channel* channel);
 void grpc_channel_update_call_size_estimate(grpc_channel* channel, size_t size);
 
-// Returns the JSON formatted channel trace for this channel. If recursive
-// is true, it will render all of the trace for this channel's subchannels.
-char* grpc_channel_get_trace(grpc_channel* channel, bool recursive);
-
-// Returns the channel uuid, which can be used to look up its trace at a
-// later time.
-intptr_t grpc_channel_get_uuid(grpc_channel* channel);
-
 #ifndef NDEBUG
 void grpc_channel_internal_ref(grpc_channel* channel, const char* reason);
 void grpc_channel_internal_unref(grpc_channel* channel, const char* reason);

test/core/channel/channel_tracer_test.cc

@@ -34,7 +34,7 @@ using grpc_core::ChannelTrace;
 using grpc_core::MakeRefCounted;
 using grpc_core::RefCountedPtr;
 
-static void add_simple_trace(RefCountedPtr<ChannelTrace> tracer) {
+static void add_simple_trace_event(RefCountedPtr<ChannelTrace> tracer) {
   tracer->AddTraceEvent(grpc_slice_from_static_string("simple trace"),
                         GRPC_ERROR_CREATE_FROM_STATIC_STRING("Error"),
                         GRPC_CHANNEL_READY);
@@ -47,8 +47,8 @@ static void validate_tracer(RefCountedPtr<ChannelTrace> tracer,
   if (!max_nodes) return;
   char* json_str = tracer->RenderTrace(true);
   grpc_json* json = grpc_json_parse_string(json_str);
-  validate_channel_data(json, expected_num_nodes_logged,
-                        GPR_MIN(expected_num_nodes_logged, max_nodes));
+  validate_channel_trace_data(json, expected_num_nodes_logged,
+                              GPR_MIN(expected_num_nodes_logged, max_nodes));
   grpc_json_destroy(json);
   gpr_free(json_str);
 }
@@ -84,8 +84,8 @@ static void validate_children(RefCountedPtr<ChannelTrace> tracer,
 static void test_basic_channel_tracing(size_t max_nodes) {
   grpc_core::ExecCtx exec_ctx;
   RefCountedPtr<ChannelTrace> tracer = MakeRefCounted<ChannelTrace>(max_nodes);
-  add_simple_trace(tracer);
-  add_simple_trace(tracer);
+  add_simple_trace_event(tracer);
+  add_simple_trace_event(tracer);
   validate_tracer_data_matches_uuid_lookup(tracer);
   tracer->AddTraceEvent(
       grpc_slice_from_static_string("trace three"),
@@ -95,13 +95,13 @@ static void test_basic_channel_tracing(size_t max_nodes) {
   tracer->AddTraceEvent(grpc_slice_from_static_string("trace four"),
                         GRPC_ERROR_NONE, GRPC_CHANNEL_SHUTDOWN);
   validate_tracer(tracer, 4, max_nodes);
-  add_simple_trace(tracer);
-  add_simple_trace(tracer);
+  add_simple_trace_event(tracer);
+  add_simple_trace_event(tracer);
   validate_tracer(tracer, 6, max_nodes);
-  add_simple_trace(tracer);
-  add_simple_trace(tracer);
-  add_simple_trace(tracer);
-  add_simple_trace(tracer);
+  add_simple_trace_event(tracer);
+  add_simple_trace_event(tracer);
+  add_simple_trace_event(tracer);
+  add_simple_trace_event(tracer);
   validate_tracer(tracer, 10, max_nodes);
   validate_tracer_data_matches_uuid_lookup(tracer);
   tracer.reset(nullptr);
@@ -124,22 +124,22 @@ static void test_basic_channel_sizing() {
 static void test_complex_channel_tracing(size_t max_nodes) {
   grpc_core::ExecCtx exec_ctx;
   RefCountedPtr<ChannelTrace> tracer = MakeRefCounted<ChannelTrace>(max_nodes);
-  add_simple_trace(tracer);
-  add_simple_trace(tracer);
+  add_simple_trace_event(tracer);
+  add_simple_trace_event(tracer);
   RefCountedPtr<ChannelTrace> sc1 = MakeRefCounted<ChannelTrace>(max_nodes);
   tracer->AddTraceEvent(grpc_slice_from_static_string("subchannel one created"),
                         GRPC_ERROR_NONE, GRPC_CHANNEL_IDLE, sc1);
   validate_tracer(tracer, 3, max_nodes);
-  add_simple_trace(sc1);
-  add_simple_trace(sc1);
-  add_simple_trace(sc1);
+  add_simple_trace_event(sc1);
+  add_simple_trace_event(sc1);
+  add_simple_trace_event(sc1);
   validate_tracer(sc1, 3, max_nodes);
-  add_simple_trace(sc1);
-  add_simple_trace(sc1);
-  add_simple_trace(sc1);
+  add_simple_trace_event(sc1);
+  add_simple_trace_event(sc1);
+  add_simple_trace_event(sc1);
   validate_tracer(sc1, 6, max_nodes);
-  add_simple_trace(tracer);
-  add_simple_trace(tracer);
+  add_simple_trace_event(tracer);
+  add_simple_trace_event(tracer);
   validate_tracer(tracer, 5, max_nodes);
   validate_tracer_data_matches_uuid_lookup(tracer);
   RefCountedPtr<ChannelTrace> sc2 = MakeRefCounted<ChannelTrace>(max_nodes);
@@ -149,12 +149,12 @@ static void test_complex_channel_tracing(size_t max_nodes) {
       grpc_slice_from_static_string("subchannel one inactive"), GRPC_ERROR_NONE,
       GRPC_CHANNEL_IDLE, sc1);
   validate_tracer(tracer, 7, max_nodes);
-  add_simple_trace(tracer);
-  add_simple_trace(tracer);
-  add_simple_trace(tracer);
-  add_simple_trace(tracer);
-  add_simple_trace(tracer);
-  add_simple_trace(tracer);
+  add_simple_trace_event(tracer);
+  add_simple_trace_event(tracer);
+  add_simple_trace_event(tracer);
+  add_simple_trace_event(tracer);
+  add_simple_trace_event(tracer);
+  add_simple_trace_event(tracer);
   validate_tracer_data_matches_uuid_lookup(tracer);
   tracer.reset(nullptr);
   sc1.reset(nullptr);
@@ -177,22 +177,22 @@ static void test_complex_channel_sizing() {
 static void test_nesting() {
   grpc_core::ExecCtx exec_ctx;
   RefCountedPtr<ChannelTrace> tracer = MakeRefCounted<ChannelTrace>(10);
-  add_simple_trace(tracer);
-  add_simple_trace(tracer);
+  add_simple_trace_event(tracer);
+  add_simple_trace_event(tracer);
   RefCountedPtr<ChannelTrace> sc1 = MakeRefCounted<ChannelTrace>(5);
   tracer->AddTraceEvent(grpc_slice_from_static_string("subchannel one created"),
                         GRPC_ERROR_NONE, GRPC_CHANNEL_IDLE, sc1);
   // channel has only one subchannel right here.
   validate_children(tracer, 1);
-  add_simple_trace(sc1);
+  add_simple_trace_event(sc1);
   RefCountedPtr<ChannelTrace> conn1 = MakeRefCounted<ChannelTrace>(5);
   // nesting one level deeper.
   sc1->AddTraceEvent(grpc_slice_from_static_string("connection one created"),
                      GRPC_ERROR_NONE, GRPC_CHANNEL_IDLE, conn1);
   validate_children(sc1, 1);
-  add_simple_trace(conn1);
-  add_simple_trace(tracer);
-  add_simple_trace(tracer);
+  add_simple_trace_event(conn1);
+  add_simple_trace_event(tracer);
+  add_simple_trace_event(tracer);
   RefCountedPtr<ChannelTrace> sc2 = MakeRefCounted<ChannelTrace>(5);
   tracer->AddTraceEvent(grpc_slice_from_static_string("subchannel two created"),
                         GRPC_ERROR_NONE, GRPC_CHANNEL_IDLE, sc2);
@@ -203,7 +203,7 @@ static void test_nesting() {
       grpc_slice_from_static_string("subchannel one inactive"), GRPC_ERROR_NONE,
       GRPC_CHANNEL_IDLE, sc1);
   validate_children(tracer, 2);
-  add_simple_trace(tracer);
+  add_simple_trace_event(tracer);
   tracer.reset(nullptr);
   sc1.reset(nullptr);
   sc2.reset(nullptr);

test/core/util/channel_tracing_utils.cc

@@ -45,8 +45,9 @@ void validate_json_array_size(grpc_json* json, const char* key,
   GPR_ASSERT(count == expected_size);
 }
 
-void validate_channel_data(grpc_json* json, size_t num_nodes_logged_expected,
-                           size_t actual_num_nodes_expected) {
+void validate_channel_trace_data(grpc_json* json,
+                                 size_t num_nodes_logged_expected,
+                                 size_t actual_num_nodes_expected) {
   GPR_ASSERT(json);
   grpc_json* channel_data = get_json_child(json, "channelData");
   grpc_json* num_nodes_logged_json =

test/core/util/channel_tracing_utils.h

@@ -24,7 +24,8 @@
 void validate_json_array_size(grpc_json* json, const char* key,
                               size_t expected_size);
 
-void validate_channel_data(grpc_json* json, size_t num_nodes_logged_expected,
-                           size_t actual_num_nodes_expected);
+void validate_channel_trace_data(grpc_json* json,
+                                 size_t num_nodes_logged_expected,
+                                 size_t actual_num_nodes_expected);
 
 #endif /* GRPC_TEST_CORE_UTIL_CHANNEL_TRACING_UTILS_H */