From c7166ae67dd554d41b4d26286da2888aebc0153b Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 15 Jun 2018 11:04:37 -0400 Subject: [PATCH] Make channelz an opt in feature --- include/grpc/impl/codegen/grpc_types.h | 3 + src/core/lib/channel/channelz.cc | 16 +++-- src/core/lib/channel/channelz.h | 13 ++-- src/core/lib/surface/channel.cc | 5 +- test/core/channel/channel_trace_test.cc | 10 +-- test/core/channel/channelz_test.cc | 26 ++++++-- test/core/end2end/tests/channelz.cc | 86 +++++++++++++++++++++++-- 7 files changed, 132 insertions(+), 27 deletions(-) diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index a5961857c10..786c070c140 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 \ diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index 799eb8bed1a..40c0932f3f8 100644 --- a/src/core/lib/channel/channelz.cc +++ b/src/core/lib/channel/channelz.cc @@ -89,21 +89,28 @@ grpc_json* add_num_str(grpc_json* parent, grpc_json* it, const char* name, } // namespace -ChannelNode::ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes) - : channel_(channel), - target_(UniquePtr(grpc_channel_get_target(channel_))), - channel_uuid_(ChannelzRegistry::Register(this)) { +ChannelNode::ChannelNode(bool enabled, grpc_channel* channel, + size_t channel_tracer_max_nodes) + : enabled_(enabled), + 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_, (gpr_atm)ExecCtx::Get()->Now()); } 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()); @@ -118,6 +125,7 @@ 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 62dc817b6ad..3352daccd09 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -40,14 +40,17 @@ class ChannelNodePeer; class ChannelNode : public RefCounted { public: - ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes); + ChannelNode(bool enabled, 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))); } @@ -56,6 +59,7 @@ class ChannelNode : public RefCounted { ChannelTrace* trace() { return trace_.get(); } void set_channel_destroyed() { + if (!enabled_) return; GPR_ASSERT(channel_ != nullptr); channel_ = nullptr; } @@ -70,13 +74,14 @@ class ChannelNode : public RefCounted { grpc_connectivity_state GetConnectivityState(); // Not owned. Will be set to nullptr when the channel is destroyed. - grpc_channel* channel_; + const bool enabled_; + grpc_channel* channel_ = nullptr; UniquePtr target_; gpr_atm calls_started_ = 0; gpr_atm calls_succeeded_ = 0; gpr_atm calls_failed_ = 0; - gpr_atm last_call_started_millis_; - const intptr_t channel_uuid_; + gpr_atm last_call_started_millis_ = 0; + intptr_t channel_uuid_; ManualConstructor trace_; }; diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index 087b7778bfd..b8255706058 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -104,6 +104,7 @@ grpc_channel* grpc_channel_create_with_builder( channel->target = target; channel->is_client = grpc_channel_stack_type_is_client(channel_stack_type); size_t channel_tracer_max_nodes = 0; // default to off + bool channelz_enabled = false; gpr_mu_init(&channel->registered_call_mu); channel->registered_calls = nullptr; @@ -142,13 +143,15 @@ grpc_channel* grpc_channel_create_with_builder( const grpc_integer_options options = {0, 0, INT_MAX}; channel_tracer_max_nodes = (size_t)grpc_channel_arg_get_integer(&args->args[i], options); + } else if (0 == strcmp(args->args[i].key, GRPC_ARG_ENABLE_CHANNELZ)) { + channelz_enabled = grpc_channel_arg_get_bool(&args->args[i], false); } } grpc_channel_args_destroy(args); channel->channelz_channel = grpc_core::MakeRefCounted( - channel, channel_tracer_max_nodes); + 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")); diff --git a/test/core/channel/channel_trace_test.cc b/test/core/channel/channel_trace_test.cc index bbddee3f14e..e1bde4e6d4b 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(channel1.channel(), GetParam()); + MakeRefCounted(true, 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(channel2.channel(), GetParam()); + MakeRefCounted(true, 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(channel1.channel(), GetParam()); + MakeRefCounted(true, 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(channel2.channel(), GetParam()); + MakeRefCounted(true, 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(channel3.channel(), GetParam()); + MakeRefCounted(true, 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 facf1c03f76..e1dc76e4aa5 100644 --- a/test/core/channel/channelz_test.cc +++ b/test/core/channel/channelz_test.cc @@ -70,12 +70,15 @@ grpc_json* GetJsonChild(grpc_json* parent, const char* key) { class ChannelFixture { public: ChannelFixture(int max_trace_nodes) { - grpc_arg client_a; - client_a.type = GRPC_ARG_INTEGER; - client_a.key = + grpc_arg client_a[2]; + client_a[0].type = GRPC_ARG_INTEGER; + client_a[0].key = const_cast(GRPC_ARG_MAX_CHANNEL_TRACE_EVENTS_PER_NODE); - client_a.value.integer = max_trace_nodes; - grpc_channel_args client_args = {1, &client_a}; + client_a[0].value.integer = max_trace_nodes; + client_a[1].type = GRPC_ARG_INTEGER; + client_a[1].key = const_cast(GRPC_ARG_ENABLE_CHANNELZ); + client_a[1].value.integer = true; + grpc_channel_args client_args = {GPR_ARRAY_SIZE(client_a), client_a}; channel_ = grpc_insecure_channel_create("fake_target", &client_args, nullptr); } @@ -96,14 +99,14 @@ struct validate_channel_data_args { void ValidateChildInteger(grpc_json* json, int64_t expect, const char* key) { grpc_json* gotten_json = GetJsonChild(json, key); - EXPECT_NE(gotten_json, nullptr); + ASSERT_NE(gotten_json, nullptr); int64_t gotten_number = (int64_t)strtol(gotten_json->value, nullptr, 0); EXPECT_EQ(gotten_number, expect); } void ValidateCounters(char* json_str, validate_channel_data_args args) { grpc_json* json = grpc_json_parse_string(json_str); - EXPECT_NE(json, nullptr); + ASSERT_NE(json, nullptr); grpc_json* data = GetJsonChild(json, "data"); ValidateChildInteger(data, args.calls_started, "callsStarted"); ValidateChildInteger(data, args.calls_failed, "callsFailed"); @@ -143,6 +146,15 @@ TEST_P(ChannelzChannelTest, BasicChannel) { gpr_free(json_str); } +TEST(ChannelzChannelTest, ChannelzDisabled) { + grpc_core::ExecCtx exec_ctx; + 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); +} + TEST_P(ChannelzChannelTest, BasicChannelAPIFunctionality) { grpc_core::ExecCtx exec_ctx; ChannelFixture channel(GetParam()); diff --git a/test/core/end2end/tests/channelz.cc b/test/core/end2end/tests/channelz.cc index 9766646c138..06343c46b4f 100644 --- a/test/core/end2end/tests/channelz.cc +++ b/test/core/end2end/tests/channelz.cc @@ -198,11 +198,18 @@ static void run_one_request(grpc_end2end_test_config config, static void test_channelz(grpc_end2end_test_config config) { grpc_end2end_test_fixture f; - f = begin_test(config, "test_channelz", nullptr, nullptr); + grpc_arg client_a; + client_a.type = GRPC_ARG_INTEGER; + client_a.key = const_cast(GRPC_ARG_ENABLE_CHANNELZ); + client_a.value.integer = true; + grpc_channel_args client_args = {1, &client_a}; + + f = begin_test(config, "test_channelz", &client_args, nullptr); grpc_core::channelz::ChannelNode* channelz_channel = grpc_channel_get_channelz_node(f.client); char* json = channelz_channel->RenderJSON(); + GPR_ASSERT(json != nullptr); GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"0\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"0\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"0\"")); @@ -212,6 +219,7 @@ static void test_channelz(grpc_end2end_test_config config) { run_one_request(config, f, true); json = channelz_channel->RenderJSON(); + GPR_ASSERT(json != nullptr); GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"1\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"0\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"1\"")); @@ -221,10 +229,15 @@ static void test_channelz(grpc_end2end_test_config config) { run_one_request(config, f, false); json = channelz_channel->RenderJSON(); + GPR_ASSERT(json != nullptr); gpr_log(GPR_INFO, "%s", json); GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"2\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"1\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"1\"")); + // channel tracing is not enables, so these should not be preset. + GPR_ASSERT(nullptr == strstr(json, "\"trace\"")); + GPR_ASSERT(nullptr == strstr(json, "\"description\":\"Channel created\"")); + GPR_ASSERT(nullptr == strstr(json, "\"severity\":\"CT_INFO\"")); gpr_free(json); end_test(&f); @@ -234,11 +247,15 @@ static void test_channelz(grpc_end2end_test_config config) { static void test_channelz_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}; + grpc_arg client_a[2]; + client_a[0].type = GRPC_ARG_INTEGER; + client_a[0].key = + const_cast(GRPC_ARG_MAX_CHANNEL_TRACE_EVENTS_PER_NODE); + client_a[0].value.integer = 5; + client_a[1].type = GRPC_ARG_INTEGER; + client_a[1].key = const_cast(GRPC_ARG_ENABLE_CHANNELZ); + client_a[1].value.integer = true; + grpc_channel_args client_args = {GPR_ARRAY_SIZE(client_a), client_a}; f = begin_test(config, "test_channelz_with_channel_trace", &client_args, nullptr); @@ -246,6 +263,7 @@ static void test_channelz_with_channel_trace(grpc_end2end_test_config config) { grpc_channel_get_channelz_node(f.client); char* json = channelz_channel->RenderJSON(); + GPR_ASSERT(json != nullptr); gpr_log(GPR_INFO, "%s", json); GPR_ASSERT(nullptr != strstr(json, "\"trace\"")); GPR_ASSERT(nullptr != strstr(json, "\"description\":\"Channel created\"")); @@ -255,9 +273,65 @@ static void test_channelz_with_channel_trace(grpc_end2end_test_config config) { end_test(&f); config.tear_down_data(&f); } + +static void test_channelz_disabled(grpc_end2end_test_config config) { + grpc_end2end_test_fixture f; + + 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); + // 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); + + end_test(&f); + config.tear_down_data(&f); +} + 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) {}