From c7166ae67dd554d41b4d26286da2888aebc0153b Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 15 Jun 2018 11:04:37 -0400 Subject: Make channelz an opt in feature --- include/grpc/impl/codegen/grpc_types.h | 3 +++ 1 file changed, 3 insertions(+) (limited to 'include/grpc/impl/codegen') diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index a5961857c1..786c070c14 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -289,6 +289,9 @@ typedef struct { * subchannel. The default is 10. If set to 0, channel tracing is disabled. */ #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. */ +#define GRPC_ARG_ENABLE_CHANNELZ "grpc.enable_channelz" /** If non-zero, Cronet transport will coalesce packets to fewer frames * when possible. */ #define GRPC_ARG_USE_CRONET_PACKET_COALESCING \ -- cgit v1.2.3 From 1ab1287ac77972bee4e2f516d81230a7f0044f14 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 19 Jun 2018 17:09:53 -0700 Subject: Reviewer feedback --- include/grpc/impl/codegen/grpc_types.h | 3 ++- src/core/lib/channel/channelz.cc | 12 ++------- src/core/lib/channel/channelz.h | 8 +----- src/core/lib/surface/call.cc | 14 ++++++---- src/core/lib/surface/channel.cc | 24 ++++++++--------- test/core/channel/channel_trace_test.cc | 10 +++---- test/core/channel/channelz_test.cc | 3 +-- test/core/end2end/tests/channelz.cc | 46 +++------------------------------ 8 files changed, 36 insertions(+), 84 deletions(-) (limited to 'include/grpc/impl/codegen') diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index 786c070c14..c32e99ed4c 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/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. */ diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index 40c0932f3f..3550fc0551 100644 --- a/src/core/lib/channel/channelz.cc +++ b/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(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; diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h index 3352daccd0..2aad1e82f4 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -40,17 +40,14 @@ class ChannelNodePeer; class ChannelNode : public RefCounted { 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 { 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 { // 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 target_; gpr_atm calls_started_ = 0; diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index f94035459f..556eb234b4 100644 --- a/src/core/lib/surface/call.cc +++ b/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; diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index b825570605..d5d75fcb2a 100644 --- a/src/core/lib/surface/channel.cc +++ b/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( - 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( + 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(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; diff --git a/test/core/channel/channel_trace_test.cc b/test/core/channel/channel_trace_test.cc index e1bde4e6d4..bbddee3f14 100644 --- a/test/core/channel/channel_trace_test.cc +++ b/test/core/channel/channel_trace_test.cc @@ -157,7 +157,7 @@ TEST_P(ChannelTracerTest, ComplexTest) { AddSimpleTrace(&tracer); ChannelFixture channel1(GetParam()); RefCountedPtr sc1 = - MakeRefCounted(true, channel1.channel(), GetParam()); + MakeRefCounted(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 sc2 = - MakeRefCounted(true, channel2.channel(), GetParam()); + MakeRefCounted(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 sc1 = - MakeRefCounted(true, channel1.channel(), GetParam()); + MakeRefCounted(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 conn1 = - MakeRefCounted(true, channel2.channel(), GetParam()); + MakeRefCounted(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 sc2 = - MakeRefCounted(true, channel3.channel(), GetParam()); + MakeRefCounted(channel3.channel(), GetParam()); tracer.AddTraceEventReferencingSubchannel( ChannelTrace::Severity::Info, grpc_slice_from_static_string("subchannel two created"), sc2); diff --git a/test/core/channel/channelz_test.cc b/test/core/channel/channelz_test.cc index cfd029f8fc..058eea914c 100644 --- a/test/core/channel/channelz_test.cc +++ b/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); } diff --git a/test/core/end2end/tests/channelz.cc b/test/core/end2end/tests/channelz.cc index 06343c46b4..847b900af1 100644 --- a/test/core/end2end/tests/channelz.cc +++ b/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(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) {} -- cgit v1.2.3