From dedc4a0cbda96b41b9b2d7ac77c1bf967ca7d540 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Mon, 22 Jan 2018 19:35:27 -0800 Subject: [PATCH] Reviewer feedback --- .../ext/filters/client_channel/subchannel.cc | 4 +- src/core/lib/channel/channel_tracer.cc | 63 ++++++++++++++++++- src/core/lib/channel/channel_tracer.h | 5 +- src/core/lib/support/object_registry.h | 5 ++ test/core/channel/channel_tracer_test.cc | 14 +++++ 5 files changed, 84 insertions(+), 7 deletions(-) diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc index 0310f152cce..bb6ba5fc27f 100644 --- a/src/core/ext/filters/client_channel/subchannel.cc +++ b/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 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; diff --git a/src/core/lib/channel/channel_tracer.cc b/src/core/lib/channel/channel_tracer.cc index df0e56d19a5..e03dd27675e 100644 --- a/src/core/lib/channel/channel_tracer.cc +++ b/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_; diff --git a/src/core/lib/channel/channel_tracer.h b/src/core/lib/channel/channel_tracer.h index 2c2554ec9bf..c2d8c1071a2 100644 --- a/src/core/lib/channel/channel_tracer.h +++ b/src/core/lib/channel/channel_tracer.h @@ -25,7 +25,7 @@ namespace grpc_core { -struct TraceEvent; +class TraceEvent; class ChannelTracer { public: @@ -51,10 +51,11 @@ class ChannelTracer { char* RenderTrace(bool recursive); /* util functions that perform the uuid -> tracer step for you, and then - returns the trace for the uuid given. */ + returns the trace for the uuid given. */ static char* GetChannelTraceFromUuid(intptr_t uuid, bool recursive); private: + // Internal helper that frees a TraceEvent. void FreeNode(TraceEvent* node); friend class ChannelTracerRenderer; diff --git a/src/core/lib/support/object_registry.h b/src/core/lib/support/object_registry.h index 1b8e13d054a..cc8fc6d878b 100644 --- a/src/core/lib/support/object_registry.h +++ b/src/core/lib/support/object_registry.h @@ -21,8 +21,13 @@ #include +// 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; diff --git a/test/core/channel/channel_tracer_test.cc b/test/core/channel/channel_tracer_test.cc index c1c9e5b57f5..c0ef2a4459e 100644 --- a/test/core/channel/channel_tracer_test.cc +++ b/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);