diff options
-rw-r--r-- | src/core/ext/filters/client_channel/client_channel_channelz.cc | 10 | ||||
-rw-r--r-- | src/core/ext/filters/client_channel/client_channel_channelz.h | 6 | ||||
-rw-r--r-- | src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc | 3 | ||||
-rw-r--r-- | src/core/lib/channel/channelz.cc | 15 | ||||
-rw-r--r-- | src/core/lib/channel/channelz.h | 11 | ||||
-rw-r--r-- | src/core/lib/channel/channelz_registry.cc | 5 | ||||
-rw-r--r-- | src/core/lib/channel/channelz_registry.h | 5 | ||||
-rw-r--r-- | src/core/lib/surface/channel.cc | 8 | ||||
-rw-r--r-- | test/core/channel/channel_trace_test.cc | 12 | ||||
-rw-r--r-- | test/core/channel/channelz_registry_test.cc | 45 | ||||
-rw-r--r-- | test/core/channel/channelz_test.cc | 27 |
11 files changed, 55 insertions, 92 deletions
diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.cc b/src/core/ext/filters/client_channel/client_channel_channelz.cc index 08ceb2dd05..88a936f9b7 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.cc +++ b/src/core/ext/filters/client_channel/client_channel_channelz.cc @@ -41,8 +41,9 @@ static const grpc_arg_pointer_vtable client_channel_channelz_vtable = { client_channel_channelz_cmp}; ClientChannelNode::ClientChannelNode(grpc_channel* channel, - size_t channel_tracer_max_nodes) - : ChannelNode(channel, channel_tracer_max_nodes) { + size_t channel_tracer_max_nodes, + bool is_top_level_channel) + : ChannelNode(channel, channel_tracer_max_nodes, is_top_level_channel) { client_channel_ = grpc_channel_stack_last_element(grpc_channel_get_channel_stack(channel)); GPR_ASSERT(client_channel_->filter == &grpc_client_channel_filter); @@ -71,9 +72,10 @@ grpc_arg ClientChannelNode::CreateChannelArg() { } RefCountedPtr<ChannelNode> ClientChannelNode::MakeClientChannelNode( - grpc_channel* channel, size_t channel_tracer_max_nodes) { + grpc_channel* channel, size_t channel_tracer_max_nodes, + bool is_top_level_channel) { return MakePolymorphicRefCounted<ChannelNode, ClientChannelNode>( - channel, channel_tracer_max_nodes); + channel, channel_tracer_max_nodes, is_top_level_channel); } } // namespace channelz diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.h b/src/core/ext/filters/client_channel/client_channel_channelz.h index cf3ef7b6f2..6fb475399f 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.h +++ b/src/core/ext/filters/client_channel/client_channel_channelz.h @@ -32,7 +32,8 @@ namespace channelz { class ClientChannelNode : public ChannelNode { public: static RefCountedPtr<ChannelNode> MakeClientChannelNode( - grpc_channel* channel, size_t channel_tracer_max_nodes); + grpc_channel* channel, size_t channel_tracer_max_nodes, + bool is_top_level_channel); // Override this functionality since client_channels have a notion of // channel connectivity. @@ -45,7 +46,8 @@ class ClientChannelNode : public ChannelNode { protected: GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW - ClientChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes); + ClientChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes, + bool is_top_level_channel); virtual ~ClientChannelNode() {} private: diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc index 01d05dbb14..e7526cb7e2 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc @@ -1004,7 +1004,8 @@ grpc_channel_args* BuildBalancerChannelArgs( // A channel arg indicating the target is a grpclb load balancer. grpc_channel_arg_integer_create( const_cast<char*>(GRPC_ARG_ADDRESS_IS_GRPCLB_LOAD_BALANCER), 1), - // A channel arg indicating the target is a grpclb load balancer. + // A channel arg indicating this is an internal channels, aka it is + // owned by components in Core, not by the user application. grpc_channel_arg_integer_create( const_cast<char*>(GRPC_ARG_CHANNELZ_CHANNEL_IS_INTERNAL_CHANNEL), 1), }; diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index c8a1d179fa..c78ea99e63 100644 --- a/src/core/lib/channel/channelz.cc +++ b/src/core/lib/channel/channelz.cc @@ -88,8 +88,12 @@ 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_(nullptr), channel_uuid_(-1) { +ChannelNode::ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes, + bool is_top_level_channel) + : channel_(channel), + target_(nullptr), + channel_uuid_(-1), + is_top_level_channel_(is_top_level_channel) { trace_.Init(channel_tracer_max_nodes); target_ = UniquePtr<char>(grpc_channel_get_target(channel_)); channel_uuid_ = ChannelzRegistry::RegisterChannelNode(this); @@ -136,7 +140,7 @@ grpc_json* ChannelNode::RenderJson() { // fill in the channel trace if applicable grpc_json* trace = trace_->RenderJson(); if (trace != nullptr) { - // we manuall link up and fill the child since it was created for us in + // we manually link up and fill the child since it was created for us in // ChannelTrace::RenderJson json_iterator = grpc_json_link_child(json, trace, json_iterator); trace->parent = json; @@ -175,9 +179,10 @@ char* ChannelNode::RenderJsonString() { } RefCountedPtr<ChannelNode> ChannelNode::MakeChannelNode( - grpc_channel* channel, size_t channel_tracer_max_nodes) { + grpc_channel* channel, size_t channel_tracer_max_nodes, + bool is_top_level_channel) { return MakeRefCounted<grpc_core::channelz::ChannelNode>( - channel, channel_tracer_max_nodes); + channel, channel_tracer_max_nodes, is_top_level_channel); } } // namespace channelz diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h index abcf907c2b..4794a280d0 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -49,7 +49,8 @@ class ChannelNodePeer; class ChannelNode : public RefCounted<ChannelNode> { public: static RefCountedPtr<ChannelNode> MakeChannelNode( - grpc_channel* channel, size_t channel_tracer_max_nodes); + grpc_channel* channel, size_t channel_tracer_max_nodes, + bool is_top_level_channel); void RecordCallStarted(); void RecordCallFailed() { @@ -78,14 +79,12 @@ class ChannelNode : public RefCounted<ChannelNode> { intptr_t channel_uuid() { return channel_uuid_; } bool is_top_level_channel() { return is_top_level_channel_; } - void set_is_top_level_channel(bool is_top_level_channel) { - is_top_level_channel_ = is_top_level_channel; - } protected: GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW - ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes); + ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes, + bool is_top_level_channel); virtual ~ChannelNode(); private: @@ -106,7 +105,7 @@ class ChannelNode : public RefCounted<ChannelNode> { // Creation functions typedef RefCountedPtr<ChannelNode> (*ChannelNodeCreationFunc)(grpc_channel*, - size_t); + size_t, bool); } // namespace channelz } // namespace grpc_core diff --git a/src/core/lib/channel/channelz_registry.cc b/src/core/lib/channel/channelz_registry.cc index b4dc3d1103..7ca3fc5836 100644 --- a/src/core/lib/channel/channelz_registry.cc +++ b/src/core/lib/channel/channelz_registry.cc @@ -86,7 +86,7 @@ char* ChannelzRegistry::InternalGetTopChannels(intptr_t start_channel_id) { grpc_json* json_iterator = nullptr; InlinedVector<ChannelNode*, 10> top_level_channels; // uuids index into entities one-off (idx 0 is really uuid 1, since 0 is - // reserver). However, we want to support requests coming in which + // reserved). However, we want to support requests coming in this // start_channel_id=0, which signifies "give me everything." Hence this // funky looking line below. size_t start_idx = start_channel_id == 0 ? 0 : start_channel_id - 1; @@ -108,9 +108,6 @@ char* ChannelzRegistry::InternalGetTopChannels(intptr_t start_channel_id) { json_iterator = grpc_json_link_child(array_parent, channel_json, json_iterator); channel_json->parent = array_parent; - channel_json->value = nullptr; - channel_json->key = nullptr; - channel_json->owns_value = false; } } // For now we do not have any pagination rules. In the future we could diff --git a/src/core/lib/channel/channelz_registry.h b/src/core/lib/channel/channelz_registry.h index 2fefc9f629..c378467f34 100644 --- a/src/core/lib/channel/channelz_registry.h +++ b/src/core/lib/channel/channelz_registry.h @@ -36,7 +36,7 @@ class ChannelzRegistry { // To be called in grpc_init() static void Init(); - // To be callen in grpc_shutdown(); + // To be called in grpc_shutdown(); static void Shutdown(); static intptr_t RegisterChannelNode(ChannelNode* channel_node) { @@ -51,7 +51,8 @@ class ChannelzRegistry { return gotten == nullptr ? nullptr : static_cast<ChannelNode*>(gotten); } - // todo, protect me + // Returns the allocated JSON string that represents the proto + // GetTopChannelsResponse as per channelz.proto. static char* GetTopChannels(intptr_t start_channel_id) { return Default()->InternalGetTopChannels(start_channel_id); } diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index dd3880f18e..7cbd61adef 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -167,11 +167,9 @@ grpc_channel* grpc_channel_create_with_builder( grpc_channel_args_destroy(args); if (channelz_enabled) { - channel->channelz_channel = - channel_node_create_func(channel, channel_tracer_max_nodes); - if (internal_channel || !channel->is_client) { - channel->channelz_channel->set_is_top_level_channel(false); - } + bool is_top_level_channel = channel->is_client && !internal_channel; + channel->channelz_channel = channel_node_create_func( + channel, channel_tracer_max_nodes, is_top_level_channel); 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 07b0e3de06..99d9a4847f 100644 --- a/test/core/channel/channel_trace_test.cc +++ b/test/core/channel/channel_trace_test.cc @@ -34,8 +34,6 @@ #include "test/core/util/test_config.h" #include "test/cpp/util/channel_trace_proto_helper.h" -// remove me -#include <grpc/support/string_util.h> #include <stdlib.h> #include <string.h> @@ -157,7 +155,7 @@ TEST_P(ChannelTracerTest, ComplexTest) { AddSimpleTrace(&tracer); ChannelFixture channel1(GetParam()); RefCountedPtr<ChannelNode> sc1 = - MakeRefCounted<ChannelNode>(channel1.channel(), GetParam()); + MakeRefCounted<ChannelNode>(channel1.channel(), GetParam(), true); tracer.AddTraceEventReferencingSubchannel( ChannelTrace::Severity::Info, grpc_slice_from_static_string("subchannel one created"), sc1); @@ -175,7 +173,7 @@ TEST_P(ChannelTracerTest, ComplexTest) { ValidateChannelTrace(&tracer, 5, GetParam()); ChannelFixture channel2(GetParam()); RefCountedPtr<ChannelNode> sc2 = - MakeRefCounted<ChannelNode>(channel2.channel(), GetParam()); + MakeRefCounted<ChannelNode>(channel2.channel(), GetParam(), true); tracer.AddTraceEventReferencingChannel( ChannelTrace::Severity::Info, grpc_slice_from_static_string("LB channel two created"), sc2); @@ -204,7 +202,7 @@ TEST_P(ChannelTracerTest, TestNesting) { ValidateChannelTrace(&tracer, 2, GetParam()); ChannelFixture channel1(GetParam()); RefCountedPtr<ChannelNode> sc1 = - MakeRefCounted<ChannelNode>(channel1.channel(), GetParam()); + MakeRefCounted<ChannelNode>(channel1.channel(), GetParam(), true); tracer.AddTraceEventReferencingChannel( ChannelTrace::Severity::Info, grpc_slice_from_static_string("subchannel one created"), sc1); @@ -212,7 +210,7 @@ TEST_P(ChannelTracerTest, TestNesting) { AddSimpleTrace(sc1->trace()); ChannelFixture channel2(GetParam()); RefCountedPtr<ChannelNode> conn1 = - MakeRefCounted<ChannelNode>(channel2.channel(), GetParam()); + MakeRefCounted<ChannelNode>(channel2.channel(), GetParam(), true); // nesting one level deeper. sc1->trace()->AddTraceEventReferencingSubchannel( ChannelTrace::Severity::Info, @@ -225,7 +223,7 @@ TEST_P(ChannelTracerTest, TestNesting) { ValidateChannelTrace(conn1->trace(), 1, GetParam()); ChannelFixture channel3(GetParam()); RefCountedPtr<ChannelNode> sc2 = - MakeRefCounted<ChannelNode>(channel3.channel(), GetParam()); + MakeRefCounted<ChannelNode>(channel3.channel(), GetParam(), true); tracer.AddTraceEventReferencingSubchannel( ChannelTrace::Severity::Info, grpc_slice_from_static_string("subchannel two created"), sc2); diff --git a/test/core/channel/channelz_registry_test.cc b/test/core/channel/channelz_registry_test.cc index 06363d00e2..990cd3d205 100644 --- a/test/core/channel/channelz_registry_test.cc +++ b/test/core/channel/channelz_registry_test.cc @@ -42,36 +42,9 @@ namespace grpc_core { namespace channelz { namespace testing { -namespace { -class ChannelFixture { - public: - ChannelFixture() { - grpc_arg client_a[1]; - client_a[0].type = GRPC_ARG_INTEGER; - client_a[0].key = const_cast<char*>(GRPC_ARG_ENABLE_CHANNELZ); - client_a[0].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); - } - - ~ChannelFixture() { grpc_channel_destroy(channel_); } - - grpc_channel* channel() { return channel_; } - - private: - grpc_channel* channel_; -}; - -} // namespace - -// Tests basic ChannelTrace functionality like construction, adding trace, and -// lookups by uuid. TEST(ChannelzRegistryTest, UuidStartsAboveZeroTest) { - ChannelFixture channel; - ChannelNode* channelz_channel = - grpc_channel_get_channelz_node(channel.channel()); + ChannelNode* channelz_channel = nullptr; intptr_t uuid = ChannelzRegistry::RegisterChannelNode(channelz_channel); EXPECT_GT(uuid, 0) << "First uuid chose must be greater than zero. Zero if " "reserved according to " @@ -81,9 +54,7 @@ TEST(ChannelzRegistryTest, UuidStartsAboveZeroTest) { } TEST(ChannelzRegistryTest, UuidsAreIncreasing) { - ChannelFixture channel; - ChannelNode* channelz_channel = - grpc_channel_get_channelz_node(channel.channel()); + ChannelNode* channelz_channel = nullptr; std::vector<intptr_t> uuids; uuids.reserve(10); for (int i = 0; i < 10; ++i) { @@ -96,18 +67,14 @@ TEST(ChannelzRegistryTest, UuidsAreIncreasing) { } TEST(ChannelzRegistryTest, RegisterGetTest) { - ChannelFixture channel; - ChannelNode* channelz_channel = - grpc_channel_get_channelz_node(channel.channel()); + ChannelNode* channelz_channel = nullptr; intptr_t uuid = ChannelzRegistry::RegisterChannelNode(channelz_channel); ChannelNode* retrieved = ChannelzRegistry::GetChannelNode(uuid); EXPECT_EQ(channelz_channel, retrieved); } TEST(ChannelzRegistryTest, RegisterManyItems) { - ChannelFixture channel; - ChannelNode* channelz_channel = - grpc_channel_get_channelz_node(channel.channel()); + ChannelNode* channelz_channel = nullptr; for (int i = 0; i < 100; i++) { intptr_t uuid = ChannelzRegistry::RegisterChannelNode(channelz_channel); ChannelNode* retrieved = ChannelzRegistry::GetChannelNode(uuid); @@ -116,9 +83,7 @@ TEST(ChannelzRegistryTest, RegisterManyItems) { } TEST(ChannelzRegistryTest, NullIfNotPresentTest) { - ChannelFixture channel; - ChannelNode* channelz_channel = - grpc_channel_get_channelz_node(channel.channel()); + ChannelNode* channelz_channel = nullptr; intptr_t uuid = ChannelzRegistry::RegisterChannelNode(channelz_channel); // try to pull out a uuid that does not exist. ChannelNode* nonexistant = ChannelzRegistry::GetChannelNode(uuid + 1); diff --git a/test/core/channel/channelz_test.cc b/test/core/channel/channelz_test.cc index 1ed36acb61..d12f529726 100644 --- a/test/core/channel/channelz_test.cc +++ b/test/core/channel/channelz_test.cc @@ -80,7 +80,7 @@ void ValidateJsonArraySize(grpc_json* json, const char* key, for (grpc_json* child = arr->child; child != nullptr; child = child->next) { ++count; } - ASSERT_EQ(count, expected_size); + EXPECT_EQ(count, expected_size); } void ValidateGetTopChannels(size_t expected_channels) { @@ -91,7 +91,7 @@ void ValidateGetTopChannels(size_t expected_channels) { // tracked: https://github.com/grpc/grpc/issues/16019. ValidateJsonArraySize(parsed_json, "channel", expected_channels); grpc_json* end = GetJsonChild(parsed_json, "end"); - EXPECT_NE(end, nullptr); + ASSERT_NE(end, nullptr); EXPECT_EQ(end->type, GRPC_JSON_TRUE); grpc_json_destroy(parsed_json); gpr_free(json_str); @@ -101,13 +101,11 @@ class ChannelFixture { public: ChannelFixture(int max_trace_nodes = 0) { grpc_arg client_a[2]; - client_a[0].type = GRPC_ARG_INTEGER; - client_a[0].key = - const_cast<char*>(GRPC_ARG_MAX_CHANNEL_TRACE_EVENTS_PER_NODE); - client_a[0].value.integer = max_trace_nodes; - client_a[1].type = GRPC_ARG_INTEGER; - client_a[1].key = const_cast<char*>(GRPC_ARG_ENABLE_CHANNELZ); - client_a[1].value.integer = true; + client_a[0] = grpc_channel_arg_integer_create( + const_cast<char*>(GRPC_ARG_MAX_CHANNEL_TRACE_EVENTS_PER_NODE), + max_trace_nodes); + client_a[1] = grpc_channel_arg_integer_create( + const_cast<char*>(GRPC_ARG_ENABLE_CHANNELZ), true); grpc_channel_args client_args = {GPR_ARRAY_SIZE(client_a), client_a}; channel_ = grpc_insecure_channel_create("fake_target", &client_args, nullptr); @@ -255,13 +253,10 @@ TEST(ChannelzGetTopChannelsTest, InternalChannelTest) { (void)channels; // suppress unused variable error // create an internal channel grpc_arg client_a[2]; - client_a[0].type = GRPC_ARG_INTEGER; - client_a[0].key = - const_cast<char*>(GRPC_ARG_CHANNELZ_CHANNEL_IS_INTERNAL_CHANNEL); - client_a[0].value.integer = 1; - client_a[1].type = GRPC_ARG_INTEGER; - client_a[1].key = const_cast<char*>(GRPC_ARG_ENABLE_CHANNELZ); - client_a[1].value.integer = true; + client_a[0] = grpc_channel_arg_integer_create( + const_cast<char*>(GRPC_ARG_CHANNELZ_CHANNEL_IS_INTERNAL_CHANNEL), true); + client_a[1] = grpc_channel_arg_integer_create( + const_cast<char*>(GRPC_ARG_ENABLE_CHANNELZ), true); grpc_channel_args client_args = {GPR_ARRAY_SIZE(client_a), client_a}; grpc_channel* internal_channel = grpc_insecure_channel_create("fake_target", &client_args, nullptr); |