From b69f1f6aacad86bdc72e25085d74e64f17f32195 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 9 May 2018 10:38:44 -0700 Subject: [PATCH 01/28] Update channelz proto --- src/core/lib/channel/channel_trace.cc | 2 +- src/proto/grpc/channelz/channelz.proto | 143 ++++++++++++++------ test/core/channel/channel_trace_test.cc | 2 +- test/cpp/util/channel_trace_proto_helper.cc | 4 +- 4 files changed, 106 insertions(+), 45 deletions(-) diff --git a/src/core/lib/channel/channel_trace.cc b/src/core/lib/channel/channel_trace.cc index eb7214b3557..84f7f000f72 100644 --- a/src/core/lib/channel/channel_trace.cc +++ b/src/core/lib/channel/channel_trace.cc @@ -219,7 +219,7 @@ char* ChannelTrace::RenderTrace() const { grpc_json_create_child(json_iterator, json, "numEventsLogged", num_events_logged_str, GRPC_JSON_STRING, true); json_iterator = - grpc_json_create_child(json_iterator, json, "creationTime", + grpc_json_create_child(json_iterator, json, "creationTimestamp", fmt_time(time_created_), GRPC_JSON_STRING, true); grpc_json* events = grpc_json_create_child(json_iterator, json, "events", nullptr, GRPC_JSON_ARRAY, false); diff --git a/src/proto/grpc/channelz/channelz.proto b/src/proto/grpc/channelz/channelz.proto index 14db66a654b..d930dfcfb40 100644 --- a/src/proto/grpc/channelz/channelz.proto +++ b/src/proto/grpc/channelz/channelz.proto @@ -1,4 +1,4 @@ -// Copyright 2018 gRPC authors. +// Copyright 2018 The gRPC Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -12,20 +12,30 @@ // See the License for the specific language governing permissions and // limitations under the License. +// This file defines an interface for exporting monitoring information +// out of gRPC servers. See the full design at +// https://github.com/grpc/proposal/blob/master/A14-channelz.md +// +// The canonical version of this proto can be found at +// https://github.com/grpc/grpc-proto/blob/master/grpc/channelz/v1/channelz.proto + syntax = "proto3"; -package grpc.channelz; +package grpc.channelz.v1; import "google/protobuf/any.proto"; import "google/protobuf/duration.proto"; import "google/protobuf/timestamp.proto"; import "google/protobuf/wrappers.proto"; -// See go/grpc-channelz. +option go_package = "google.golang.org/grpc/channelz/grpc_channelz_v1"; +option java_multiple_files = true; +option java_package = "io.grpc.channelz.v1"; +option java_outer_classname = "ChannelzProto"; // Channel is a logical grouping of channels, subchannels, and sockets. message Channel { - // The identifier for this channel. + // The identifier for this channel. This should bet set. ChannelRef ref = 1; // Data specific to this channel. ChannelData data = 2; @@ -43,7 +53,7 @@ message Channel { repeated SubchannelRef subchannel_ref = 4; // There are no ordering guarantees on the order of sockets. - repeated SocketRef socket = 5; + repeated SocketRef socket_ref = 5; } // Subchannel is a logical grouping of channels, subchannels, and sockets. @@ -67,7 +77,7 @@ message Subchannel { repeated SubchannelRef subchannel_ref = 4; // There are no ordering guarantees on the order of sockets. - repeated SocketRef socket = 5; + repeated SocketRef socket_ref = 5; } // These come from the specified states in this document: @@ -84,20 +94,23 @@ message ChannelConnectivityState { State state = 1; } +// Channel data is data related to a specific Channel or Subchannel. message ChannelData { - + // The connectivity state of the channel or subchannel. Implementations + // should always set this. ChannelConnectivityState state = 1; // The target this channel originally tried to connect to. May be absent string target = 2; + // A trace of recent events on the channel. May be absent. ChannelTrace trace = 3; // The number of calls started on the channel int64 calls_started = 4; // The number of calls that have completed with an OK status int64 calls_succeeded = 5; - // The number of calls that have a completed with a non-OK status + // The number of calls that have completed with a non-OK status int64 calls_failed = 6; // The last time a call was started on the channel. @@ -130,26 +143,29 @@ message ChannelTraceEvent { } } +// ChannelTrace represents the recent events that have occurred on the channel. message ChannelTrace { // Number of events ever logged in this tracing object. This can differ from // events.size() because events can be overwritten or garbage collected by // implementations. int64 num_events_logged = 1; // Time that this channel was created. - google.protobuf.Timestamp creation_time = 2; + google.protobuf.Timestamp creation_timestamp = 2; // List of events that have occurred on this channel. repeated ChannelTraceEvent events = 3; } +// ChannelRef is a reference to a Channel. message ChannelRef { // The globally unique id for this channel. Must be a positive number. int64 channel_id = 1; // An optional name associated with the channel. string name = 2; // Intentionally don't use field numbers from other refs. - reserved 3, 4, 5, 6; + reserved 3, 4, 5, 6, 7, 8; } +// ChannelRef is a reference to a Subchannel. message SubchannelRef { // The globally unique id for this subchannel. Must be a positive number. int64 subchannel_id = 7; @@ -159,6 +175,7 @@ message SubchannelRef { reserved 1, 2, 3, 4, 5, 6; } +// SocketRef is a reference to a Socket. message SocketRef { int64 socket_id = 3; // An optional name associated with the socket. @@ -167,8 +184,9 @@ message SocketRef { reserved 1, 2, 5, 6, 7, 8; } +// ServerRef is a reference to a Server. message ServerRef { - // A globally unique identifier for this server. Must be a positive number. + // A globally unique identifier for this server. Must be a positive number. int64 server_id = 5; // An optional name associated with the server. string name = 6; @@ -176,16 +194,22 @@ message ServerRef { reserved 1, 2, 3, 4, 7, 8; } +// Server represents a single server. There may be multiple servers in a single +// program. message Server { + // The identifier for a Server. This should be set. ServerRef ref = 1; + // The associated data of the Server. ServerData data = 2; // The sockets that the server is listening on. There are no ordering - // guarantees. + // guarantees. This may be absent. repeated SocketRef listen_socket = 3; } +// ServerData is data for a specific Server. message ServerData { + // A trace of recent events on the server. May be absent. ChannelTrace trace = 1; // The number of incoming calls started on the server @@ -201,13 +225,17 @@ message ServerData { // Information about an actual connection. Pronounced "sock-ay". message Socket { + // The identifier for the Socket. SocketRef ref = 1; + // Data specific to this Socket. SocketData data = 2; // The locally bound address. Address local = 3; // The remote bound address. May be absent. Address remote = 4; + // Security details for this socket. May be absent if not available, or + // there is no security on the socket. Security security = 5; // Optional, represents the name of the remote endpoint, if different than @@ -215,17 +243,23 @@ message Socket { string remote_name = 6; } +// SocketData is data associated for a specific Socket. The fields present +// are specific to the implementation, so there may be minor differences in +// the semantics. (e.g. flow control windows) message SocketData { // The number of streams that have been started. int64 streams_started = 1; - // The number of streams that have ended successfully with the EoS bit set for - // both end points + // The number of streams that have ended successfully: + // On client side, received frame with eos bit set; + // On server side, sent frame with eos bit set. int64 streams_succeeded = 2; - // The number of incoming streams that have a completed with a non-OK status + // The number of streams that have ended unsuccessfully: + // On client side, ended without receiving frame with eos bit set; + // On server side, ended without sending frame with eos bit set. int64 streams_failed = 3; - - // The number of messages successfully sent on this socket. + // The number of grpc messages successfully sent on this socket. int64 messages_sent = 4; + // The number of grpc messages received on this socket. int64 messages_received = 5; // The number of keep alives sent. This is typically implemented with HTTP/2 @@ -254,12 +288,14 @@ message SocketData { // include stream level or TCP level flow control info. google.protobuf.Int64Value remote_flow_control_window = 12; + // Socket options set on this socket. May be absent. repeated SocketOption option = 13; } +// Address represents the address used to create the socket. message Address { message TcpIpAddress { - // Either the IPv4 or IPv6 address in bytes. Will either be 4 bytes or 16 + // Either the IPv4 or IPv6 address in bytes. Will be either 4 bytes or 16 // bytes in length. bytes ip_address = 1; // 0-64k, or -1 if not appropriate. @@ -271,7 +307,7 @@ message Address { } // An address type not included above. message OtherAddress { - // The human readable version of the value. + // The human readable version of the value. This value should be set. string name = 1; // The actual address message. google.protobuf.Any value = 2; @@ -284,12 +320,17 @@ message Address { } } +// Security represents details about how secure the socket is. message Security { message Tls { - // The key exchange used. e.g. X25519 - string key_exchange = 1; - // The cipher used. e.g. AES_128_GCM. - string cipher = 2; + oneof cipher_suite { + // The cipher suite name in the RFC 4346 format: + // https://tools.ietf.org/html/rfc4346#appendix-C + string standard_name = 1; + // Some other way to describe the cipher suite if + // the RFC 4346 name is not available. + string other_name = 2; + } // the certificate used by this endpoint. bytes local_certificate = 3; // the certificate used by the remote endpoint. @@ -307,7 +348,11 @@ message Security { } } +// SocketOption represents socket options for a socket. Specifically, these +// are the options returned by getsockopt(). message SocketOption { + // The full name of the socket option. Typically this will be the upper case + // name, such as "SO_REUSEPORT". string name = 1; // The human readable value of this socket option. At least one of value or // additional will be set. @@ -323,12 +368,17 @@ message SocketOptionTimeout { google.protobuf.Duration duration = 1; } +// For use with SocketOption's additional field. This is primarily used for +// SO_LINGER. message SocketOptionLinger { + // active maps to `struct linger.l_onoff` bool active = 1; + // duration maps to `struct linger.l_linger` google.protobuf.Duration duration = 2; } -// Tcp info for SOL_TCP, TCP_INFO +// For use with SocketOption's additional field. Tcp info for +// SOL_TCP and TCP_INFO. message SocketOptionTcpInfo { uint32 tcpi_state = 1; @@ -366,8 +416,10 @@ message SocketOptionTcpInfo { uint32 tcpi_reordering = 29; } +// Channelz is a service exposed by gRPC servers that provides detailed debug +// information. service Channelz { - // Gets all root channels (e.g. channels the application has directly + // Gets all root channels (i.e. channels the application has directly // created). This does not include subchannels nor non-top level channels. rpc GetTopChannels(GetTopChannelsRequest) returns (GetTopChannelsResponse); // Gets all servers that exist in the process. @@ -382,6 +434,22 @@ service Channelz { rpc GetSocket(GetSocketRequest) returns (GetSocketResponse); } +message GetTopChannelsRequest { + // start_channel_id indicates that only channels at or above this id should be + // included in the results. + int64 start_channel_id = 1; +} + +message GetTopChannelsResponse { + // list of channels that the connection detail service knows about. Sorted in + // ascending channel_id order. + repeated Channel channel = 1; + // If set, indicates that the list of channels is the final list. Requesting + // more channels can only return more if they are created after this RPC + // completes. + bool end = 2; +} + message GetServersRequest { // start_server_id indicates that only servers at or above this id should be // included in the results. @@ -415,42 +483,35 @@ message GetServerSocketsResponse { bool end = 2; } -message GetTopChannelsRequest { - // start_channel_id indicates that only channels at or above this id should be - // included in the results. - int64 start_channel_id = 1; -} - -message GetTopChannelsResponse { - // list of channels that the connection detail service knows about. Sorted in - // ascending channel_id order. - repeated Channel channel = 1; - // If set, indicates that the list of channels is the final list. Requesting - // more channels can only return more if they are created after this RPC - // completes. - bool end = 2; -} - message GetChannelRequest { + // channel_id is the identifier of the specific channel to get. int64 channel_id = 1; } message GetChannelResponse { + // The Channel that corresponds to the requested channel_id. This field + // should be set. Channel channel = 1; } message GetSubchannelRequest { + // subchannel_id is the identifier of the specific subchannel to get. int64 subchannel_id = 1; } message GetSubchannelResponse { + // The Subchannel that corresponds to the requested subchannel_id. This + // field should be set. Subchannel subchannel = 1; } message GetSocketRequest { + // socket_id is the identifier of the specific socket to get. int64 socket_id = 1; } message GetSocketResponse { + // The Socket that corresponds to the requested socket_id. This field + // should be set. Socket socket = 1; } diff --git a/test/core/channel/channel_trace_test.cc b/test/core/channel/channel_trace_test.cc index 64de05bc0a8..9befdb4fa37 100644 --- a/test/core/channel/channel_trace_test.cc +++ b/test/core/channel/channel_trace_test.cc @@ -69,7 +69,7 @@ void ValidateChannelTraceData(grpc_json* json, ASSERT_NE(json, nullptr); grpc_json* num_events_logged_json = GetJsonChild(json, "numEventsLogged"); ASSERT_NE(num_events_logged_json, nullptr); - grpc_json* start_time = GetJsonChild(json, "creationTime"); + grpc_json* start_time = GetJsonChild(json, "creationTimestamp"); ASSERT_NE(start_time, nullptr); size_t num_events_logged = (size_t)strtol(num_events_logged_json->value, nullptr, 0); diff --git a/test/cpp/util/channel_trace_proto_helper.cc b/test/cpp/util/channel_trace_proto_helper.cc index fbc9f1501c4..c395f607f48 100644 --- a/test/cpp/util/channel_trace_proto_helper.cc +++ b/test/cpp/util/channel_trace_proto_helper.cc @@ -32,13 +32,13 @@ namespace testing { void ValidateChannelTraceProtoJsonTranslation(char* tracer_json_c_str) { std::string tracer_json_str(tracer_json_c_str); - grpc::channelz::ChannelTrace channel_trace; + grpc::channelz::v1::ChannelTrace channel_trace; google::protobuf::util::JsonParseOptions parse_options; // If the following line is failing, then uncomment the last line of the // comment, and uncomment the lines that print the two strings. You can // then compare the output, and determine what fields are missing. // - // options.ignore_unknown_fields = true; + // parse_options.ignore_unknown_fields = true; ASSERT_EQ(google::protobuf::util::JsonStringToMessage( tracer_json_str, &channel_trace, parse_options), google::protobuf::util::Status::OK); From c3c6e064b33ab8b7a2d3cf5a56171029d0bb1edc Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 9 May 2018 11:10:21 -0700 Subject: [PATCH 02/28] Add basic support for GetChannel --- BUILD | 2 + CMakeLists.txt | 60 ++++++- Makefile | 62 ++++++- build.yaml | 23 ++- config.m4 | 1 + config.w32 | 1 + gRPC-C++.podspec | 2 + gRPC-Core.podspec | 3 + grpc.def | 1 + grpc.gemspec | 2 + grpc.gyp | 4 + include/grpc/grpc.h | 3 + package.xml | 2 + src/core/lib/channel/channelz.cc | 164 ++++++++++++++++++ src/core/lib/channel/channelz.h | 68 ++++++++ src/core/lib/surface/call.cc | 27 ++- src/core/lib/surface/channel.cc | 13 ++ src/core/lib/surface/channel.h | 5 + src/python/grpcio/grpc_core_dependencies.py | 1 + src/ruby/ext/grpc/rb_grpc_imports.generated.c | 2 + src/ruby/ext/grpc/rb_grpc_imports.generated.h | 3 + test/core/channel/BUILD | 9 + test/core/channel/channelz_test.cc | 57 ++++++ test/core/end2end/fuzzers/api_fuzzer.cc | 1 + test/core/end2end/tests/simple_request.cc | 7 + .../core/surface/public_headers_must_be_c89.c | 1 + test/cpp/util/channel_trace_proto_helper.cc | 34 +++- test/cpp/util/channel_trace_proto_helper.h | 1 + tools/doxygen/Doxyfile.c++.internal | 1 + tools/doxygen/Doxyfile.core.internal | 2 + .../generated/sources_and_headers.json | 26 ++- tools/run_tests/generated/tests.json | 24 +++ 32 files changed, 579 insertions(+), 33 deletions(-) create mode 100644 src/core/lib/channel/channelz.cc create mode 100644 src/core/lib/channel/channelz.h create mode 100644 test/core/channel/channelz_test.cc diff --git a/BUILD b/BUILD index db1bdaa9947..9ab216a0926 100644 --- a/BUILD +++ b/BUILD @@ -679,6 +679,7 @@ grpc_cc_library( "src/core/lib/channel/channel_stack_builder.cc", "src/core/lib/channel/channel_trace.cc", "src/core/lib/channel/channelz_registry.cc", + "src/core/lib/channel/channelz.cc", "src/core/lib/channel/connected_channel.cc", "src/core/lib/channel/handshaker.cc", "src/core/lib/channel/handshaker_factory.cc", @@ -826,6 +827,7 @@ grpc_cc_library( "src/core/lib/channel/channel_stack_builder.h", "src/core/lib/channel/channel_trace.h", "src/core/lib/channel/channelz_registry.h", + "src/core/lib/channel/channelz.h", "src/core/lib/channel/connected_channel.h", "src/core/lib/channel/context.h", "src/core/lib/channel/handshaker.h", diff --git a/CMakeLists.txt b/CMakeLists.txt index af56886cd98..37bde81a9d0 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -539,6 +539,7 @@ add_dependencies(buildtests_cxx channel_arguments_test) add_dependencies(buildtests_cxx channel_filter_test) add_dependencies(buildtests_cxx channel_trace_test) add_dependencies(buildtests_cxx channelz_registry_test) +add_dependencies(buildtests_cxx channelz_test) add_dependencies(buildtests_cxx check_gcp_environment_linux_test) add_dependencies(buildtests_cxx check_gcp_environment_windows_test) add_dependencies(buildtests_cxx chttp2_settings_timeout_test) @@ -923,6 +924,7 @@ add_library(grpc src/core/lib/channel/channel_stack.cc src/core/lib/channel/channel_stack_builder.cc src/core/lib/channel/channel_trace.cc + src/core/lib/channel/channelz.cc src/core/lib/channel/channelz_registry.cc src/core/lib/channel/connected_channel.cc src/core/lib/channel/handshaker.cc @@ -1316,6 +1318,7 @@ add_library(grpc_cronet src/core/lib/channel/channel_stack.cc src/core/lib/channel/channel_stack_builder.cc src/core/lib/channel/channel_trace.cc + src/core/lib/channel/channelz.cc src/core/lib/channel/channelz_registry.cc src/core/lib/channel/connected_channel.cc src/core/lib/channel/handshaker.cc @@ -1701,6 +1704,7 @@ add_library(grpc_test_util src/core/lib/channel/channel_stack.cc src/core/lib/channel/channel_stack_builder.cc src/core/lib/channel/channel_trace.cc + src/core/lib/channel/channelz.cc src/core/lib/channel/channelz_registry.cc src/core/lib/channel/connected_channel.cc src/core/lib/channel/handshaker.cc @@ -2005,6 +2009,7 @@ add_library(grpc_test_util_unsecure src/core/lib/channel/channel_stack.cc src/core/lib/channel/channel_stack_builder.cc src/core/lib/channel/channel_trace.cc + src/core/lib/channel/channelz.cc src/core/lib/channel/channelz_registry.cc src/core/lib/channel/connected_channel.cc src/core/lib/channel/handshaker.cc @@ -2288,6 +2293,7 @@ add_library(grpc_unsecure src/core/lib/channel/channel_stack.cc src/core/lib/channel/channel_stack_builder.cc src/core/lib/channel/channel_trace.cc + src/core/lib/channel/channelz.cc src/core/lib/channel/channelz_registry.cc src/core/lib/channel/connected_channel.cc src/core/lib/channel/handshaker.cc @@ -3114,6 +3120,7 @@ add_library(grpc++_cronet src/core/lib/channel/channel_stack.cc src/core/lib/channel/channel_stack_builder.cc src/core/lib/channel/channel_trace.cc + src/core/lib/channel/channelz.cc src/core/lib/channel/channelz_registry.cc src/core/lib/channel/connected_channel.cc src/core/lib/channel/handshaker.cc @@ -10472,17 +10479,10 @@ if (gRPC_BUILD_TESTS) add_executable(channel_trace_test test/core/channel/channel_trace_test.cc - ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/channelz/channelz.pb.cc - ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/channelz/channelz.grpc.pb.cc - ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/channelz/channelz.pb.h - ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/channelz/channelz.grpc.pb.h third_party/googletest/googletest/src/gtest-all.cc third_party/googletest/googlemock/src/gmock-all.cc ) -protobuf_generate_grpc_cpp( - src/proto/grpc/channelz/channelz.proto -) target_include_directories(channel_trace_test PRIVATE ${CMAKE_CURRENT_SOURCE_DIR} @@ -10555,6 +10555,52 @@ target_link_libraries(channelz_registry_test endif (gRPC_BUILD_TESTS) if (gRPC_BUILD_TESTS) +add_executable(channelz_test + test/core/channel/channelz_test.cc + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/channelz/channelz.pb.cc + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/channelz/channelz.grpc.pb.cc + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/channelz/channelz.pb.h + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/channelz/channelz.grpc.pb.h + third_party/googletest/googletest/src/gtest-all.cc + third_party/googletest/googlemock/src/gmock-all.cc +) + +protobuf_generate_grpc_cpp( + src/proto/grpc/channelz/channelz.proto +) + +target_include_directories(channelz_test + PRIVATE ${CMAKE_CURRENT_SOURCE_DIR} + PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/include + PRIVATE ${_gRPC_SSL_INCLUDE_DIR} + PRIVATE ${_gRPC_PROTOBUF_INCLUDE_DIR} + PRIVATE ${_gRPC_ZLIB_INCLUDE_DIR} + PRIVATE ${_gRPC_BENCHMARK_INCLUDE_DIR} + PRIVATE ${_gRPC_CARES_INCLUDE_DIR} + PRIVATE ${_gRPC_GFLAGS_INCLUDE_DIR} + PRIVATE ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR} + PRIVATE third_party/googletest/googletest/include + PRIVATE third_party/googletest/googletest + PRIVATE third_party/googletest/googlemock/include + PRIVATE third_party/googletest/googlemock + PRIVATE ${_gRPC_PROTO_GENS_DIR} +) + +target_link_libraries(channelz_test + ${_gRPC_PROTOBUF_LIBRARIES} + ${_gRPC_ALLTARGETS_LIBRARIES} + grpc_test_util + grpc++_test_util + grpc++ + grpc + gpr_test_util + gpr + ${_gRPC_GFLAGS_LIBRARIES} +) + +endif (gRPC_BUILD_TESTS) +if (gRPC_BUILD_TESTS) + add_executable(check_gcp_environment_linux_test test/core/security/check_gcp_environment_linux_test.cc third_party/googletest/googletest/src/gtest-all.cc diff --git a/Makefile b/Makefile index 1247566ffec..f5423ea6435 100644 --- a/Makefile +++ b/Makefile @@ -1123,6 +1123,7 @@ channel_arguments_test: $(BINDIR)/$(CONFIG)/channel_arguments_test channel_filter_test: $(BINDIR)/$(CONFIG)/channel_filter_test channel_trace_test: $(BINDIR)/$(CONFIG)/channel_trace_test channelz_registry_test: $(BINDIR)/$(CONFIG)/channelz_registry_test +channelz_test: $(BINDIR)/$(CONFIG)/channelz_test check_gcp_environment_linux_test: $(BINDIR)/$(CONFIG)/check_gcp_environment_linux_test check_gcp_environment_windows_test: $(BINDIR)/$(CONFIG)/check_gcp_environment_windows_test chttp2_settings_timeout_test: $(BINDIR)/$(CONFIG)/chttp2_settings_timeout_test @@ -1618,6 +1619,7 @@ buildtests_cxx: privatelibs_cxx \ $(BINDIR)/$(CONFIG)/channel_filter_test \ $(BINDIR)/$(CONFIG)/channel_trace_test \ $(BINDIR)/$(CONFIG)/channelz_registry_test \ + $(BINDIR)/$(CONFIG)/channelz_test \ $(BINDIR)/$(CONFIG)/check_gcp_environment_linux_test \ $(BINDIR)/$(CONFIG)/check_gcp_environment_windows_test \ $(BINDIR)/$(CONFIG)/chttp2_settings_timeout_test \ @@ -1793,6 +1795,7 @@ buildtests_cxx: privatelibs_cxx \ $(BINDIR)/$(CONFIG)/channel_filter_test \ $(BINDIR)/$(CONFIG)/channel_trace_test \ $(BINDIR)/$(CONFIG)/channelz_registry_test \ + $(BINDIR)/$(CONFIG)/channelz_test \ $(BINDIR)/$(CONFIG)/check_gcp_environment_linux_test \ $(BINDIR)/$(CONFIG)/check_gcp_environment_windows_test \ $(BINDIR)/$(CONFIG)/chttp2_settings_timeout_test \ @@ -2228,6 +2231,8 @@ test_cxx: buildtests_cxx $(Q) $(BINDIR)/$(CONFIG)/channel_trace_test || ( echo test channel_trace_test failed ; exit 1 ) $(E) "[RUN] Testing channelz_registry_test" $(Q) $(BINDIR)/$(CONFIG)/channelz_registry_test || ( echo test channelz_registry_test failed ; exit 1 ) + $(E) "[RUN] Testing channelz_test" + $(Q) $(BINDIR)/$(CONFIG)/channelz_test || ( echo test channelz_test failed ; exit 1 ) $(E) "[RUN] Testing check_gcp_environment_linux_test" $(Q) $(BINDIR)/$(CONFIG)/check_gcp_environment_linux_test || ( echo test check_gcp_environment_linux_test failed ; exit 1 ) $(E) "[RUN] Testing check_gcp_environment_windows_test" @@ -3304,6 +3309,7 @@ LIBGRPC_SRC = \ src/core/lib/channel/channel_stack.cc \ src/core/lib/channel/channel_stack_builder.cc \ src/core/lib/channel/channel_trace.cc \ + src/core/lib/channel/channelz.cc \ src/core/lib/channel/channelz_registry.cc \ src/core/lib/channel/connected_channel.cc \ src/core/lib/channel/handshaker.cc \ @@ -3697,6 +3703,7 @@ LIBGRPC_CRONET_SRC = \ src/core/lib/channel/channel_stack.cc \ src/core/lib/channel/channel_stack_builder.cc \ src/core/lib/channel/channel_trace.cc \ + src/core/lib/channel/channelz.cc \ src/core/lib/channel/channelz_registry.cc \ src/core/lib/channel/connected_channel.cc \ src/core/lib/channel/handshaker.cc \ @@ -4081,6 +4088,7 @@ LIBGRPC_TEST_UTIL_SRC = \ src/core/lib/channel/channel_stack.cc \ src/core/lib/channel/channel_stack_builder.cc \ src/core/lib/channel/channel_trace.cc \ + src/core/lib/channel/channelz.cc \ src/core/lib/channel/channelz_registry.cc \ src/core/lib/channel/connected_channel.cc \ src/core/lib/channel/handshaker.cc \ @@ -4377,6 +4385,7 @@ LIBGRPC_TEST_UTIL_UNSECURE_SRC = \ src/core/lib/channel/channel_stack.cc \ src/core/lib/channel/channel_stack_builder.cc \ src/core/lib/channel/channel_trace.cc \ + src/core/lib/channel/channelz.cc \ src/core/lib/channel/channelz_registry.cc \ src/core/lib/channel/connected_channel.cc \ src/core/lib/channel/handshaker.cc \ @@ -4639,6 +4648,7 @@ LIBGRPC_UNSECURE_SRC = \ src/core/lib/channel/channel_stack.cc \ src/core/lib/channel/channel_stack_builder.cc \ src/core/lib/channel/channel_trace.cc \ + src/core/lib/channel/channelz.cc \ src/core/lib/channel/channelz_registry.cc \ src/core/lib/channel/connected_channel.cc \ src/core/lib/channel/handshaker.cc \ @@ -5458,6 +5468,7 @@ LIBGRPC++_CRONET_SRC = \ src/core/lib/channel/channel_stack.cc \ src/core/lib/channel/channel_stack_builder.cc \ src/core/lib/channel/channel_trace.cc \ + src/core/lib/channel/channelz.cc \ src/core/lib/channel/channelz_registry.cc \ src/core/lib/channel/connected_channel.cc \ src/core/lib/channel/handshaker.cc \ @@ -16296,7 +16307,6 @@ endif CHANNEL_TRACE_TEST_SRC = \ test/core/channel/channel_trace_test.cc \ - $(GENDIR)/src/proto/grpc/channelz/channelz.pb.cc $(GENDIR)/src/proto/grpc/channelz/channelz.grpc.pb.cc \ CHANNEL_TRACE_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(CHANNEL_TRACE_TEST_SRC)))) ifeq ($(NO_SECURE),true) @@ -16329,8 +16339,6 @@ endif $(OBJDIR)/$(CONFIG)/test/core/channel/channel_trace_test.o: $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a -$(OBJDIR)/$(CONFIG)/src/proto/grpc/channelz/channelz.o: $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a - deps_channel_trace_test: $(CHANNEL_TRACE_TEST_OBJS:.o=.dep) ifneq ($(NO_SECURE),true) @@ -16338,7 +16346,6 @@ ifneq ($(NO_DEPS),true) -include $(CHANNEL_TRACE_TEST_OBJS:.o=.dep) endif endif -$(OBJDIR)/$(CONFIG)/test/core/channel/channel_trace_test.o: $(GENDIR)/src/proto/grpc/channelz/channelz.pb.cc $(GENDIR)/src/proto/grpc/channelz/channelz.grpc.pb.cc CHANNELZ_REGISTRY_TEST_SRC = \ @@ -16384,6 +16391,53 @@ endif endif +CHANNELZ_TEST_SRC = \ + test/core/channel/channelz_test.cc \ + $(GENDIR)/src/proto/grpc/channelz/channelz.pb.cc $(GENDIR)/src/proto/grpc/channelz/channelz.grpc.pb.cc \ + +CHANNELZ_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(CHANNELZ_TEST_SRC)))) +ifeq ($(NO_SECURE),true) + +# You can't build secure targets if you don't have OpenSSL. + +$(BINDIR)/$(CONFIG)/channelz_test: openssl_dep_error + +else + + + + +ifeq ($(NO_PROTOBUF),true) + +# You can't build the protoc plugins or protobuf-enabled targets if you don't have protobuf 3.5.0+. + +$(BINDIR)/$(CONFIG)/channelz_test: protobuf_dep_error + +else + +$(BINDIR)/$(CONFIG)/channelz_test: $(PROTOBUF_DEP) $(CHANNELZ_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + $(E) "[LD] Linking $@" + $(Q) mkdir -p `dirname $@` + $(Q) $(LDXX) $(LDFLAGS) $(CHANNELZ_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/channelz_test + +endif + +endif + +$(OBJDIR)/$(CONFIG)/test/core/channel/channelz_test.o: $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + +$(OBJDIR)/$(CONFIG)/src/proto/grpc/channelz/channelz.o: $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + +deps_channelz_test: $(CHANNELZ_TEST_OBJS:.o=.dep) + +ifneq ($(NO_SECURE),true) +ifneq ($(NO_DEPS),true) +-include $(CHANNELZ_TEST_OBJS:.o=.dep) +endif +endif +$(OBJDIR)/$(CONFIG)/test/core/channel/channelz_test.o: $(GENDIR)/src/proto/grpc/channelz/channelz.pb.cc $(GENDIR)/src/proto/grpc/channelz/channelz.grpc.pb.cc + + CHECK_GCP_ENVIRONMENT_LINUX_TEST_SRC = \ test/core/security/check_gcp_environment_linux_test.cc \ diff --git a/build.yaml b/build.yaml index 16c90fbd4c1..3df140e9222 100644 --- a/build.yaml +++ b/build.yaml @@ -235,6 +235,7 @@ filegroups: - src/core/lib/channel/channel_stack.cc - src/core/lib/channel/channel_stack_builder.cc - src/core/lib/channel/channel_trace.cc + - src/core/lib/channel/channelz.cc - src/core/lib/channel/channelz_registry.cc - src/core/lib/channel/connected_channel.cc - src/core/lib/channel/handshaker.cc @@ -405,6 +406,7 @@ filegroups: - src/core/lib/channel/channel_stack.h - src/core/lib/channel/channel_stack_builder.h - src/core/lib/channel/channel_trace.h + - src/core/lib/channel/channelz.h - src/core/lib/channel/channelz_registry.h - src/core/lib/channel/connected_channel.h - src/core/lib/channel/context.h @@ -4203,10 +4205,6 @@ targets: - grpc - gpr_test_util - gpr - filegroups: - - grpc++_channelz_proto - uses: - - grpc++_test - name: channelz_registry_test gtest: true build: test @@ -4223,6 +4221,23 @@ targets: uses: - grpc++_test uses_polling: false +- name: channelz_test + gtest: true + build: test + language: c++ + src: + - test/core/channel/channelz_test.cc + deps: + - grpc_test_util + - grpc++_test_util + - grpc++ + - grpc + - gpr_test_util + - gpr + filegroups: + - grpc++_channelz_proto + uses: + - grpc++_test - name: check_gcp_environment_linux_test build: test language: c++ diff --git a/config.m4 b/config.m4 index 8190485249c..da8dca7c3cf 100644 --- a/config.m4 +++ b/config.m4 @@ -89,6 +89,7 @@ if test "$PHP_GRPC" != "no"; then src/core/lib/channel/channel_stack.cc \ src/core/lib/channel/channel_stack_builder.cc \ src/core/lib/channel/channel_trace.cc \ + src/core/lib/channel/channelz.cc \ src/core/lib/channel/channelz_registry.cc \ src/core/lib/channel/connected_channel.cc \ src/core/lib/channel/handshaker.cc \ diff --git a/config.w32 b/config.w32 index db7679ce53b..b4311cb6d18 100644 --- a/config.w32 +++ b/config.w32 @@ -65,6 +65,7 @@ if (PHP_GRPC != "no") { "src\\core\\lib\\channel\\channel_stack.cc " + "src\\core\\lib\\channel\\channel_stack_builder.cc " + "src\\core\\lib\\channel\\channel_trace.cc " + + "src\\core\\lib\\channel\\channelz.cc " + "src\\core\\lib\\channel\\channelz_registry.cc " + "src\\core\\lib\\channel\\connected_channel.cc " + "src\\core\\lib\\channel\\handshaker.cc " + diff --git a/gRPC-C++.podspec b/gRPC-C++.podspec index 262de72971b..ce56e514dd6 100644 --- a/gRPC-C++.podspec +++ b/gRPC-C++.podspec @@ -347,6 +347,7 @@ Pod::Spec.new do |s| 'src/core/lib/channel/channel_stack.h', 'src/core/lib/channel/channel_stack_builder.h', 'src/core/lib/channel/channel_trace.h', + 'src/core/lib/channel/channelz.h', 'src/core/lib/channel/channelz_registry.h', 'src/core/lib/channel/connected_channel.h', 'src/core/lib/channel/context.h', @@ -532,6 +533,7 @@ Pod::Spec.new do |s| 'src/core/lib/channel/channel_stack.h', 'src/core/lib/channel/channel_stack_builder.h', 'src/core/lib/channel/channel_trace.h', + 'src/core/lib/channel/channelz.h', 'src/core/lib/channel/channelz_registry.h', 'src/core/lib/channel/connected_channel.h', 'src/core/lib/channel/context.h', diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index f3be7129b59..a10cfb5739f 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -357,6 +357,7 @@ Pod::Spec.new do |s| 'src/core/lib/channel/channel_stack.h', 'src/core/lib/channel/channel_stack_builder.h', 'src/core/lib/channel/channel_trace.h', + 'src/core/lib/channel/channelz.h', 'src/core/lib/channel/channelz_registry.h', 'src/core/lib/channel/connected_channel.h', 'src/core/lib/channel/context.h', @@ -507,6 +508,7 @@ Pod::Spec.new do |s| 'src/core/lib/channel/channel_stack.cc', 'src/core/lib/channel/channel_stack_builder.cc', 'src/core/lib/channel/channel_trace.cc', + 'src/core/lib/channel/channelz.cc', 'src/core/lib/channel/channelz_registry.cc', 'src/core/lib/channel/connected_channel.cc', 'src/core/lib/channel/handshaker.cc', @@ -935,6 +937,7 @@ Pod::Spec.new do |s| 'src/core/lib/channel/channel_stack.h', 'src/core/lib/channel/channel_stack_builder.h', 'src/core/lib/channel/channel_trace.h', + 'src/core/lib/channel/channelz.h', 'src/core/lib/channel/channelz_registry.h', 'src/core/lib/channel/connected_channel.h', 'src/core/lib/channel/context.h', diff --git a/grpc.def b/grpc.def index 6a91214e8c3..1a8c40f9509 100644 --- a/grpc.def +++ b/grpc.def @@ -47,6 +47,7 @@ EXPORTS grpc_channel_destroy grpc_channel_get_trace grpc_channel_get_uuid + grpc_channelz_get_channel grpc_call_cancel grpc_call_cancel_with_status grpc_call_ref diff --git a/grpc.gemspec b/grpc.gemspec index 5ab37136835..cf0e3081459 100644 --- a/grpc.gemspec +++ b/grpc.gemspec @@ -294,6 +294,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/channel/channel_stack.h ) s.files += %w( src/core/lib/channel/channel_stack_builder.h ) s.files += %w( src/core/lib/channel/channel_trace.h ) + s.files += %w( src/core/lib/channel/channelz.h ) s.files += %w( src/core/lib/channel/channelz_registry.h ) s.files += %w( src/core/lib/channel/connected_channel.h ) s.files += %w( src/core/lib/channel/context.h ) @@ -444,6 +445,7 @@ Gem::Specification.new do |s| s.files += %w( src/core/lib/channel/channel_stack.cc ) s.files += %w( src/core/lib/channel/channel_stack_builder.cc ) s.files += %w( src/core/lib/channel/channel_trace.cc ) + s.files += %w( src/core/lib/channel/channelz.cc ) s.files += %w( src/core/lib/channel/channelz_registry.cc ) s.files += %w( src/core/lib/channel/connected_channel.cc ) s.files += %w( src/core/lib/channel/handshaker.cc ) diff --git a/grpc.gyp b/grpc.gyp index 2ceae3fe36b..a3ea8674fc0 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -249,6 +249,7 @@ 'src/core/lib/channel/channel_stack.cc', 'src/core/lib/channel/channel_stack_builder.cc', 'src/core/lib/channel/channel_trace.cc', + 'src/core/lib/channel/channelz.cc', 'src/core/lib/channel/channelz_registry.cc', 'src/core/lib/channel/connected_channel.cc', 'src/core/lib/channel/handshaker.cc', @@ -598,6 +599,7 @@ 'src/core/lib/channel/channel_stack.cc', 'src/core/lib/channel/channel_stack_builder.cc', 'src/core/lib/channel/channel_trace.cc', + 'src/core/lib/channel/channelz.cc', 'src/core/lib/channel/channelz_registry.cc', 'src/core/lib/channel/connected_channel.cc', 'src/core/lib/channel/handshaker.cc', @@ -829,6 +831,7 @@ 'src/core/lib/channel/channel_stack.cc', 'src/core/lib/channel/channel_stack_builder.cc', 'src/core/lib/channel/channel_trace.cc', + 'src/core/lib/channel/channelz.cc', 'src/core/lib/channel/channelz_registry.cc', 'src/core/lib/channel/connected_channel.cc', 'src/core/lib/channel/handshaker.cc', @@ -1038,6 +1041,7 @@ 'src/core/lib/channel/channel_stack.cc', 'src/core/lib/channel/channel_stack_builder.cc', 'src/core/lib/channel/channel_trace.cc', + 'src/core/lib/channel/channelz.cc', 'src/core/lib/channel/channelz_registry.cc', 'src/core/lib/channel/connected_channel.cc', 'src/core/lib/channel/handshaker.cc', diff --git a/include/grpc/grpc.h b/include/grpc/grpc.h index dd8a5d7d5f8..de7c39b1834 100644 --- a/include/grpc/grpc.h +++ b/include/grpc/grpc.h @@ -294,6 +294,9 @@ GRPCAPI char* grpc_channel_get_trace(grpc_channel* channel); later time. */ GRPCAPI intptr_t grpc_channel_get_uuid(grpc_channel* channel); +/** channelz support */ +GRPCAPI char* grpc_channelz_get_channel(intptr_t channel_id); + /** 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. diff --git a/package.xml b/package.xml index b8cda9d79f0..1a13bf81b5f 100644 --- a/package.xml +++ b/package.xml @@ -299,6 +299,7 @@ + @@ -449,6 +450,7 @@ + diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc new file mode 100644 index 00000000000..aabc941dcd5 --- /dev/null +++ b/src/core/lib/channel/channelz.cc @@ -0,0 +1,164 @@ +/* + * + * Copyright 2017 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include + +#include "src/core/lib/channel/channelz.h" + +#include +#include +#include +#include +#include +#include +#include + +#include "src/core/lib/channel/channelz_registry.h" +#include "src/core/lib/channel/status_util.h" +#include "src/core/lib/gpr/string.h" +#include "src/core/lib/gpr/useful.h" +#include "src/core/lib/gprpp/memory.h" +#include "src/core/lib/iomgr/error.h" +#include "src/core/lib/slice/slice_internal.h" +#include "src/core/lib/surface/channel.h" +#include "src/core/lib/transport/connectivity_state.h" +#include "src/core/lib/transport/error_utils.h" + +// TODO(ncteisen): actually implement this +char* grpc_channelz_get_channel(intptr_t channel_id) { return nullptr; } + +namespace grpc_core { +namespace channelz { + +// TODO(ncteisen): more this functions to a loc where it can be used +namespace { + +// returns an allocated string that represents tm according to RFC-3339, and, +// more specifically, follows: +// https://developers.google.com/protocol-buffers/docs/proto3#json +// +// "Uses RFC 3339, where generated output will always be Z-normalized and uses +// 0, 3, 6 or 9 fractional digits." +char* fmt_time(gpr_timespec tm) { + char time_buffer[35]; + char ns_buffer[11]; // '.' + 9 digits of precision + struct tm* tm_info = localtime((const time_t*)&tm.tv_sec); + strftime(time_buffer, sizeof(time_buffer), "%Y-%m-%dT%H:%M:%S", tm_info); + snprintf(ns_buffer, 11, ".%09d", tm.tv_nsec); + // This loop trims off trailing zeros by inserting a null character that the + // right point. We iterate in chunks of three because we want 0, 3, 6, or 9 + // fractional digits. + for (int i = 7; i >= 1; i -= 3) { + if (ns_buffer[i] == '0' && ns_buffer[i + 1] == '0' && + ns_buffer[i + 2] == '0') { + ns_buffer[i] = '\0'; + // Edge case in which all fractional digits were 0. + if (i == 1) { + ns_buffer[0] = '\0'; + } + } else { + break; + } + } + char* full_time_str; + gpr_asprintf(&full_time_str, "%s%sZ", time_buffer, ns_buffer); + return full_time_str; +} + +// TODO(ncteisen); move this to json library +grpc_json* add_num_str(grpc_json* parent, grpc_json* it, const char* name, + uint64_t num) { + char* num_str; + gpr_asprintf(&num_str, "%" PRIu64, num); + return grpc_json_create_child(it, parent, name, num_str, GRPC_JSON_STRING, + true); +} + +} // namespace + +Channel::Channel(grpc_channel* channel) : channel_(channel) { + target_ = grpc_channel_get_target(channel_); + channel_uuid_ = ChannelzRegistry::Register(this); +} + +Channel::~Channel() { + gpr_free(const_cast(target_)); + ChannelzRegistry::Unregister(channel_uuid_); +} + +void Channel::CallStarted() { + calls_started_++; + last_call_started_timestamp_ = grpc_millis_to_timespec( + grpc_core::ExecCtx::Get()->Now(), GPR_CLOCK_REALTIME); +} + +grpc_connectivity_state Channel::GetConnectivityState() { + if (channel_destroyed_) { + return GRPC_CHANNEL_SHUTDOWN; + } else { + return grpc_channel_check_connectivity_state(channel_, false); + } +} + +char* Channel::RenderJSON() { + // 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; + grpc_json* json_iterator = nullptr; + + // create and fill the ref child + json_iterator = grpc_json_create_child(json_iterator, json, "ref", nullptr, + GRPC_JSON_OBJECT, true); + json = json_iterator; + json_iterator = nullptr; + json_iterator = add_num_str(json, json_iterator, "channelId", channel_uuid_); + + // reset json iterators to top level object + json = top_level_json; + json_iterator = nullptr; + + // create and fill the data child + json_iterator = grpc_json_create_child(json_iterator, json, "data", nullptr, + GRPC_JSON_OBJECT, true); + json = json_iterator; + json_iterator = nullptr; + json_iterator = + add_num_str(json, json_iterator, "callsStarted", calls_started_); + json_iterator = + add_num_str(json, json_iterator, "callsSucceeded", calls_succeeded_); + json_iterator = + add_num_str(json, json_iterator, "callsFailed", calls_failed_); + json_iterator = grpc_json_create_child( + json_iterator, json, "lastCallStartedTimestamp", + fmt_time(last_call_started_timestamp_), GRPC_JSON_STRING, true); + json_iterator = grpc_json_create_child(json_iterator, json, "target", target_, + GRPC_JSON_STRING, false); + grpc_connectivity_state connectivity_state = GetConnectivityState(); + json_iterator = + grpc_json_create_child(json_iterator, json, "state", + grpc_connectivity_state_name(connectivity_state), + GRPC_JSON_STRING, false); + + // render and return the over json object + char* json_str = grpc_json_dump_to_string(top_level_json, 0); + grpc_json_destroy(top_level_json); + return json_str; +} + +} // namespace channelz +} // namespace grpc_core diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h new file mode 100644 index 00000000000..661228cf446 --- /dev/null +++ b/src/core/lib/channel/channelz.h @@ -0,0 +1,68 @@ +/* + * + * Copyright 2018 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#ifndef GRPC_CORE_LIB_CHANNEL_CHANNELZ_H +#define GRPC_CORE_LIB_CHANNEL_CHANNELZ_H + +#include + +#include + +#include "src/core/ext/filters/client_channel/client_channel.h" +#include "src/core/lib/gprpp/ref_counted.h" +#include "src/core/lib/gprpp/ref_counted_ptr.h" +#include "src/core/lib/iomgr/error.h" +#include "src/core/lib/json/json.h" + +namespace grpc_core { +namespace channelz { + +// owned by the client_channel that it points to and tracks +class Channel : public RefCounted { + public: + Channel(grpc_channel* channel); + ~Channel(); + + void CallStarted(); + void CallFailed() { calls_failed_++; } + void CallSucceeded() { calls_succeeded_++; } + + char* RenderJSON(); + + void set_channel_destroyed() { + GPR_ASSERT(!channel_destroyed_); + channel_destroyed_ = true; + } + + private: + bool channel_destroyed_ = false; + grpc_channel* channel_; + const char* target_; + uint64_t calls_started_ = 0; + uint64_t calls_succeeded_ = 0; + uint64_t calls_failed_ = 0; + gpr_timespec last_call_started_timestamp_; + intptr_t channel_uuid_; + + grpc_connectivity_state GetConnectivityState(); +}; + +} // namespace channelz +} // namespace grpc_core + +#endif /* GRPC_CORE_LIB_CHANNEL_CHANNELZ_H */ diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index 86e0afa6ee3..924d633cb27 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -1077,13 +1077,23 @@ static void recv_trailing_filter(void* args, grpc_metadata_batch* b) { if (b->idx.named.grpc_status != nullptr) { grpc_status_code status_code = grpc_get_status_code_from_metadata(b->idx.named.grpc_status->md); - grpc_error* error = - status_code == GRPC_STATUS_OK - ? GRPC_ERROR_NONE - : grpc_error_set_int(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "Error received from peer"), - GRPC_ERROR_INT_GRPC_STATUS, - static_cast(status_code)); + grpc_error* error = GRPC_ERROR_NONE; + grpc_core::channelz::Channel* channelz_channel = + call->channel != nullptr + ? grpc_channel_get_channelz_channel(call->channel) + : nullptr; + if (status_code == GRPC_STATUS_OK) { + if (channelz_channel != nullptr) { + channelz_channel->CallSucceeded(); + } + } else { + if (channelz_channel != nullptr) { + channelz_channel->CallFailed(); + } + error = grpc_error_set_int( + GRPC_ERROR_CREATE_FROM_STATIC_STRING("Error received from peer"), + GRPC_ERROR_INT_GRPC_STATUS, static_cast(status_code)); + } if (b->idx.named.grpc_message != nullptr) { error = grpc_error_set_str( error, GRPC_ERROR_STR_GRPC_MESSAGE, @@ -1665,6 +1675,9 @@ static grpc_call_error call_start_batch(grpc_call* call, const grpc_op* ops, stream_op_payload->send_initial_metadata.peer_string = &call->peer_string; } + grpc_core::channelz::Channel* channelz_channel = + grpc_channel_get_channelz_channel(call->channel); + channelz_channel->CallStarted(); break; } case GRPC_OP_SEND_MESSAGE: { diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index a466b325bef..da66a120d57 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -32,6 +32,7 @@ #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/channel_trace.h" +#include "src/core/lib/channel/channelz.h" #include "src/core/lib/debug/stats.h" #include "src/core/lib/gpr/string.h" #include "src/core/lib/gprpp/manual_constructor.h" @@ -67,6 +68,7 @@ struct grpc_channel { registered_call* registered_calls; grpc_core::RefCountedPtr tracer; + grpc_core::RefCountedPtr channelz_channel; char* target; }; @@ -150,6 +152,8 @@ grpc_channel* grpc_channel_create_with_builder( channel->tracer->AddTraceEvent( grpc_core::ChannelTrace::Severity::Info, grpc_slice_from_static_string("Channel created")); + channel->channelz_channel = + grpc_core::MakeRefCounted(channel); return channel; } @@ -188,6 +192,15 @@ char* grpc_channel_get_trace(grpc_channel* channel) { return channel->tracer->RenderTrace(); } +char* grpc_channel_render_channelz(grpc_channel* channel) { + return channel->channelz_channel->RenderJSON(); +} + +grpc_core::channelz::Channel* grpc_channel_get_channelz_channel( + grpc_channel* channel) { + return channel->channelz_channel.get(); +} + intptr_t grpc_channel_get_uuid(grpc_channel* channel) { return channel->tracer->GetUuid(); } diff --git a/src/core/lib/surface/channel.h b/src/core/lib/surface/channel.h index 288313951e4..52290f05f7a 100644 --- a/src/core/lib/surface/channel.h +++ b/src/core/lib/surface/channel.h @@ -23,6 +23,7 @@ #include "src/core/lib/channel/channel_stack.h" #include "src/core/lib/channel/channel_stack_builder.h" +#include "src/core/lib/channel/channelz.h" #include "src/core/lib/surface/channel_stack_type.h" grpc_channel* grpc_channel_create(const char* target, @@ -50,6 +51,10 @@ grpc_call* grpc_channel_create_pollset_set_call( /** Get a (borrowed) pointer to this channels underlying channel stack */ grpc_channel_stack* grpc_channel_get_channel_stack(grpc_channel* channel); +grpc_core::channelz::Channel* grpc_channel_get_channelz_channel( + grpc_channel* channel); +char* grpc_channel_render_channelz(grpc_channel* channel); + /** Get a grpc_mdelem of grpc-status: X where X is the numeric value of status_code. diff --git a/src/python/grpcio/grpc_core_dependencies.py b/src/python/grpcio/grpc_core_dependencies.py index 5511bf9a322..a64fb42f9ba 100644 --- a/src/python/grpcio/grpc_core_dependencies.py +++ b/src/python/grpcio/grpc_core_dependencies.py @@ -64,6 +64,7 @@ CORE_SOURCE_FILES = [ 'src/core/lib/channel/channel_stack.cc', 'src/core/lib/channel/channel_stack_builder.cc', 'src/core/lib/channel/channel_trace.cc', + 'src/core/lib/channel/channelz.cc', 'src/core/lib/channel/channelz_registry.cc', 'src/core/lib/channel/connected_channel.cc', 'src/core/lib/channel/handshaker.cc', diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.c b/src/ruby/ext/grpc/rb_grpc_imports.generated.c index 02f84c0b962..075219a3af3 100644 --- a/src/ruby/ext/grpc/rb_grpc_imports.generated.c +++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.c @@ -70,6 +70,7 @@ grpc_lame_client_channel_create_type grpc_lame_client_channel_create_import; grpc_channel_destroy_type grpc_channel_destroy_import; grpc_channel_get_trace_type grpc_channel_get_trace_import; grpc_channel_get_uuid_type grpc_channel_get_uuid_import; +grpc_channelz_get_channel_type grpc_channelz_get_channel_import; grpc_call_cancel_type grpc_call_cancel_import; grpc_call_cancel_with_status_type grpc_call_cancel_with_status_import; grpc_call_ref_type grpc_call_ref_import; @@ -318,6 +319,7 @@ void grpc_rb_load_imports(HMODULE library) { grpc_channel_destroy_import = (grpc_channel_destroy_type) GetProcAddress(library, "grpc_channel_destroy"); grpc_channel_get_trace_import = (grpc_channel_get_trace_type) GetProcAddress(library, "grpc_channel_get_trace"); grpc_channel_get_uuid_import = (grpc_channel_get_uuid_type) GetProcAddress(library, "grpc_channel_get_uuid"); + grpc_channelz_get_channel_import = (grpc_channelz_get_channel_type) GetProcAddress(library, "grpc_channelz_get_channel"); grpc_call_cancel_import = (grpc_call_cancel_type) GetProcAddress(library, "grpc_call_cancel"); grpc_call_cancel_with_status_import = (grpc_call_cancel_with_status_type) GetProcAddress(library, "grpc_call_cancel_with_status"); grpc_call_ref_import = (grpc_call_ref_type) GetProcAddress(library, "grpc_call_ref"); diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.h b/src/ruby/ext/grpc/rb_grpc_imports.generated.h index b2186a69aa9..590a6b2439b 100644 --- a/src/ruby/ext/grpc/rb_grpc_imports.generated.h +++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.h @@ -185,6 +185,9 @@ extern grpc_channel_get_trace_type grpc_channel_get_trace_import; typedef intptr_t(*grpc_channel_get_uuid_type)(grpc_channel* channel); extern grpc_channel_get_uuid_type grpc_channel_get_uuid_import; #define grpc_channel_get_uuid grpc_channel_get_uuid_import +typedef char*(*grpc_channelz_get_channel_type)(intptr_t channel_id); +extern grpc_channelz_get_channel_type grpc_channelz_get_channel_import; +#define grpc_channelz_get_channel grpc_channelz_get_channel_import typedef grpc_call_error(*grpc_call_cancel_type)(grpc_call* call, void* reserved); extern grpc_call_cancel_type grpc_call_cancel_import; #define grpc_call_cancel grpc_call_cancel_import diff --git a/test/core/channel/BUILD b/test/core/channel/BUILD index c554b20148c..c336688209d 100644 --- a/test/core/channel/BUILD +++ b/test/core/channel/BUILD @@ -84,8 +84,13 @@ grpc_cc_test( ) grpc_cc_test( +<<<<<<< HEAD name = "channelz_registry_test", srcs = ["channelz_registry_test.cc"], +======= + name = "channelz_test", + srcs = ["channelz_test.cc"], +>>>>>>> Add channelz test language = "C++", deps = [ "//:gpr", @@ -93,6 +98,10 @@ grpc_cc_test( "//:grpc++", "//test/core/util:gpr_test_util", "//test/core/util:grpc_test_util", +<<<<<<< HEAD +======= + "//test/cpp/util:channel_trace_proto_helper", +>>>>>>> Add channelz test ], external_deps = [ "gtest", diff --git a/test/core/channel/channelz_test.cc b/test/core/channel/channelz_test.cc new file mode 100644 index 00000000000..503bb9065ba --- /dev/null +++ b/test/core/channel/channelz_test.cc @@ -0,0 +1,57 @@ +/* + * + * Copyright 2017 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include +#include + +#include + +#include +#include + +#include "src/core/lib/channel/channel_trace.h" +#include "src/core/lib/channel/channelz_registry.h" +#include "src/core/lib/gpr/useful.h" +#include "src/core/lib/iomgr/exec_ctx.h" +#include "src/core/lib/json/json.h" + +#include "test/core/util/test_config.h" +#include "test/cpp/util/channel_trace_proto_helper.h" + +// remove me +#include +#include +#include + +namespace grpc_core { +namespace testing { +namespace {} // anonymous namespace + +TEST(ChannelzTest, Channel) {} + +} // namespace testing +} // namespace grpc_core + +int main(int argc, char** argv) { + grpc_test_init(argc, argv); + grpc_init(); + ::testing::InitGoogleTest(&argc, argv); + int ret = RUN_ALL_TESTS(); + grpc_shutdown(); + return ret; +} diff --git a/test/core/end2end/fuzzers/api_fuzzer.cc b/test/core/end2end/fuzzers/api_fuzzer.cc index 36f257d6dab..ba9c04fd0e8 100644 --- a/test/core/end2end/fuzzers/api_fuzzer.cc +++ b/test/core/end2end/fuzzers/api_fuzzer.cc @@ -1048,6 +1048,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { op->reserved = nullptr; op->flags = grpc_fuzzer_get_next_uint32(&inp); } + if (g_channel == nullptr) ok = false; if (ok) { validator* v = make_finished_batch_validator(g_active_call, has_ops); g_active_call->pending_ops++; diff --git a/test/core/end2end/tests/simple_request.cc b/test/core/end2end/tests/simple_request.cc index 941d9ae3198..6e36e54cd37 100644 --- a/test/core/end2end/tests/simple_request.cc +++ b/test/core/end2end/tests/simple_request.cc @@ -21,6 +21,9 @@ #include #include +#include "src/core/ext/filters/client_channel/client_channel.h" +#include "src/core/lib/surface/channel.h" + #include #include #include @@ -198,6 +201,10 @@ static void simple_request_body(grpc_end2end_test_config config, CQ_EXPECT_COMPLETION(cqv, tag(1), 1); cq_verify(cqv); + char* json = grpc_channel_render_channelz(f.client); + gpr_log(GPR_ERROR, "%s", json); + gpr_free(json); + GPR_ASSERT(status == GRPC_STATUS_UNIMPLEMENTED); GPR_ASSERT(0 == grpc_slice_str_cmp(details, "xyz")); // the following sanity check makes sure that the requested error string is diff --git a/test/core/surface/public_headers_must_be_c89.c b/test/core/surface/public_headers_must_be_c89.c index 52a1b039980..bc9c2f2a13e 100644 --- a/test/core/surface/public_headers_must_be_c89.c +++ b/test/core/surface/public_headers_must_be_c89.c @@ -108,6 +108,7 @@ int main(int argc, char **argv) { printf("%lx", (unsigned long) grpc_channel_destroy); printf("%lx", (unsigned long) grpc_channel_get_trace); printf("%lx", (unsigned long) grpc_channel_get_uuid); + printf("%lx", (unsigned long) grpc_channelz_get_channel); printf("%lx", (unsigned long) grpc_call_cancel); printf("%lx", (unsigned long) grpc_call_cancel_with_status); printf("%lx", (unsigned long) grpc_call_ref); diff --git a/test/cpp/util/channel_trace_proto_helper.cc b/test/cpp/util/channel_trace_proto_helper.cc index c395f607f48..db9390163bc 100644 --- a/test/cpp/util/channel_trace_proto_helper.cc +++ b/test/cpp/util/channel_trace_proto_helper.cc @@ -30,26 +30,42 @@ namespace grpc { namespace testing { -void ValidateChannelTraceProtoJsonTranslation(char* tracer_json_c_str) { - std::string tracer_json_str(tracer_json_c_str); - grpc::channelz::v1::ChannelTrace channel_trace; +namespace { + +// Generic helper that takes in a json string, converts it to a proto, and +// then back to json. This ensures that the json string was correctly formatted +// according to https://developers.google.com/protocol-buffers/docs/proto3#json +template +void VaidateProtoJsonTranslation(char* json_c_str) { + std::string json_str(json_c_str); + Message msg; google::protobuf::util::JsonParseOptions parse_options; // If the following line is failing, then uncomment the last line of the // comment, and uncomment the lines that print the two strings. You can // then compare the output, and determine what fields are missing. // // parse_options.ignore_unknown_fields = true; - ASSERT_EQ(google::protobuf::util::JsonStringToMessage( - tracer_json_str, &channel_trace, parse_options), + ASSERT_EQ(google::protobuf::util::JsonStringToMessage(json_str, &msg, + parse_options), google::protobuf::util::Status::OK); std::string proto_json_str; - ASSERT_EQ(google::protobuf::util::MessageToJsonString(channel_trace, - &proto_json_str), + ASSERT_EQ(google::protobuf::util::MessageToJsonString(msg, &proto_json_str), google::protobuf::util::Status::OK); // uncomment these to compare the the json strings. - // gpr_log(GPR_ERROR, "tracer json: %s", tracer_json_str.c_str()); + // gpr_log(GPR_ERROR, "tracer json: %s", json_str.c_str()); // gpr_log(GPR_ERROR, "proto json: %s", proto_json_str.c_str()); - ASSERT_EQ(tracer_json_str, proto_json_str); + ASSERT_EQ(json_str, proto_json_str); +} + +} // namespace + +void ValidateChannelTraceProtoJsonTranslation(char* tracer_json_c_str) { + VaidateProtoJsonTranslation( + tracer_json_c_str); +} + +void ValidateChannelProtoJsonTranslation(char* channel_json_c_str) { + VaidateProtoJsonTranslation(channel_json_c_str); } } // namespace testing diff --git a/test/cpp/util/channel_trace_proto_helper.h b/test/cpp/util/channel_trace_proto_helper.h index d7043d9f063..d1a36033725 100644 --- a/test/cpp/util/channel_trace_proto_helper.h +++ b/test/cpp/util/channel_trace_proto_helper.h @@ -23,6 +23,7 @@ namespace grpc { namespace testing { void ValidateChannelTraceProtoJsonTranslation(char* tracer_json_c_str); +void ValidateChannelProtoJsonTranslation(char* channel_json_c_str); } // namespace testing } // namespace grpc diff --git a/tools/doxygen/Doxyfile.c++.internal b/tools/doxygen/Doxyfile.c++.internal index ea2f377c917..3e3c4dd7771 100644 --- a/tools/doxygen/Doxyfile.c++.internal +++ b/tools/doxygen/Doxyfile.c++.internal @@ -1012,6 +1012,7 @@ src/core/lib/channel/channel_args.h \ src/core/lib/channel/channel_stack.h \ src/core/lib/channel/channel_stack_builder.h \ src/core/lib/channel/channel_trace.h \ +src/core/lib/channel/channelz.h \ src/core/lib/channel/channelz_registry.h \ src/core/lib/channel/connected_channel.h \ src/core/lib/channel/context.h \ diff --git a/tools/doxygen/Doxyfile.core.internal b/tools/doxygen/Doxyfile.core.internal index 589e862b9a8..f95e0f38dbe 100644 --- a/tools/doxygen/Doxyfile.core.internal +++ b/tools/doxygen/Doxyfile.core.internal @@ -1038,6 +1038,8 @@ src/core/lib/channel/channel_stack_builder.cc \ src/core/lib/channel/channel_stack_builder.h \ src/core/lib/channel/channel_trace.cc \ src/core/lib/channel/channel_trace.h \ +src/core/lib/channel/channelz.cc \ +src/core/lib/channel/channelz.h \ src/core/lib/channel/channelz_registry.cc \ src/core/lib/channel/channelz_registry.h \ src/core/lib/channel/connected_channel.cc \ diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json index 0af4e19a1ad..46508d47494 100644 --- a/tools/run_tests/generated/sources_and_headers.json +++ b/tools/run_tests/generated/sources_and_headers.json @@ -3068,8 +3068,6 @@ "gpr_test_util", "grpc", "grpc++", - "grpc++_channelz_proto", - "grpc++_test", "grpc++_test_util", "grpc_test_util" ], @@ -3103,6 +3101,27 @@ "third_party": false, "type": "target" }, + { + "deps": [ + "gpr", + "gpr_test_util", + "grpc", + "grpc++", + "grpc++_channelz_proto", + "grpc++_test", + "grpc++_test_util", + "grpc_test_util" + ], + "headers": [], + "is_filegroup": false, + "language": "c++", + "name": "channelz_test", + "src": [ + "test/core/channel/channelz_test.cc" + ], + "third_party": false, + "type": "target" + }, { "deps": [ "gpr", @@ -9294,6 +9313,7 @@ "src/core/lib/channel/channel_stack.cc", "src/core/lib/channel/channel_stack_builder.cc", "src/core/lib/channel/channel_trace.cc", + "src/core/lib/channel/channelz.cc", "src/core/lib/channel/channelz_registry.cc", "src/core/lib/channel/connected_channel.cc", "src/core/lib/channel/handshaker.cc", @@ -9465,6 +9485,7 @@ "src/core/lib/channel/channel_stack.h", "src/core/lib/channel/channel_stack_builder.h", "src/core/lib/channel/channel_trace.h", + "src/core/lib/channel/channelz.h", "src/core/lib/channel/channelz_registry.h", "src/core/lib/channel/connected_channel.h", "src/core/lib/channel/context.h", @@ -9614,6 +9635,7 @@ "src/core/lib/channel/channel_stack.h", "src/core/lib/channel/channel_stack_builder.h", "src/core/lib/channel/channel_trace.h", + "src/core/lib/channel/channelz.h", "src/core/lib/channel/channelz_registry.h", "src/core/lib/channel/connected_channel.h", "src/core/lib/channel/context.h", diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index 00604f181c6..0a02b1df05f 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -3693,6 +3693,30 @@ ], "uses_polling": false }, + { + "args": [], + "benchmark": false, + "ci_platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "gtest": true, + "language": "c++", + "name": "channelz_test", + "platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "uses_polling": true + }, { "args": [], "benchmark": false, From 23c50fda51a38d0484f4b1537492fd93eeae33ac Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 18 May 2018 16:38:29 -0700 Subject: [PATCH 03/28] Incorperate channel trace into channelz --- src/core/lib/channel/channel_trace.cc | 37 +++++++++++++++------------ src/core/lib/channel/channel_trace.h | 21 ++++++++------- src/core/lib/channel/channelz.cc | 13 +++++++--- src/core/lib/channel/channelz.h | 9 ++++++- src/core/lib/surface/channel.cc | 19 +++++++------- 5 files changed, 58 insertions(+), 41 deletions(-) diff --git a/src/core/lib/channel/channel_trace.cc b/src/core/lib/channel/channel_trace.cc index 84f7f000f72..efa034a1887 100644 --- a/src/core/lib/channel/channel_trace.cc +++ b/src/core/lib/channel/channel_trace.cc @@ -40,16 +40,17 @@ #include "src/core/lib/transport/error_utils.h" namespace grpc_core { +namespace channelz { -ChannelTrace::TraceEvent::TraceEvent( - Severity severity, grpc_slice data, - RefCountedPtr referenced_tracer, ReferencedType type) +ChannelTrace::TraceEvent::TraceEvent(Severity severity, grpc_slice data, + RefCountedPtr referenced_channel, + ReferencedType type) : severity_(severity), data_(data), timestamp_(grpc_millis_to_timespec(grpc_core::ExecCtx::Get()->Now(), GPR_CLOCK_REALTIME)), next_(nullptr), - referenced_tracer_(std::move(referenced_tracer)), + referenced_channel_(std::move(referenced_channel)), referenced_type_(type) {} ChannelTrace::TraceEvent::TraceEvent(Severity severity, grpc_slice data) @@ -117,20 +118,21 @@ void ChannelTrace::AddTraceEvent(Severity severity, grpc_slice data) { void ChannelTrace::AddTraceEventReferencingChannel( Severity severity, grpc_slice data, - RefCountedPtr referenced_tracer) { + RefCountedPtr referenced_channel) { if (max_list_size_ == 0) return; // tracing is disabled if max_events == 0 // create and fill up the new event - AddTraceEventHelper( - New(severity, data, std::move(referenced_tracer), Channel)); + AddTraceEventHelper(New( + severity, data, std::move(referenced_channel), ReferencedType::Channel)); } void ChannelTrace::AddTraceEventReferencingSubchannel( Severity severity, grpc_slice data, - RefCountedPtr referenced_tracer) { + RefCountedPtr referenced_channel) { if (max_list_size_ == 0) return; // tracing is disabled if max_events == 0 // create and fill up the new event - AddTraceEventHelper(New( - severity, data, std::move(referenced_tracer), Subchannel)); + AddTraceEventHelper(New(severity, data, + std::move(referenced_channel), + ReferencedType::Subchannel)); } namespace { @@ -193,17 +195,19 @@ void ChannelTrace::TraceEvent::RenderTraceEvent(grpc_json* json) const { json_iterator = grpc_json_create_child(json_iterator, json, "timestamp", fmt_time(timestamp_), GRPC_JSON_STRING, true); - if (referenced_tracer_ != nullptr) { + if (referenced_channel_ != nullptr) { char* uuid_str; - gpr_asprintf(&uuid_str, "%" PRIdPTR, referenced_tracer_->channel_uuid_); + gpr_asprintf(&uuid_str, "%" PRIdPTR, referenced_channel_->channel_uuid()); grpc_json* child_ref = grpc_json_create_child( json_iterator, json, - (referenced_type_ == Channel) ? "channelRef" : "subchannelRef", nullptr, - GRPC_JSON_OBJECT, false); + (referenced_type_ == ReferencedType::Channel) ? "channelRef" + : "subchannelRef", + nullptr, GRPC_JSON_OBJECT, false); json_iterator = grpc_json_create_child( nullptr, child_ref, - (referenced_type_ == Channel) ? "channelId" : "subchannelId", uuid_str, - GRPC_JSON_STRING, true); + (referenced_type_ == ReferencedType::Channel) ? "channelId" + : "subchannelId", + uuid_str, GRPC_JSON_STRING, true); json_iterator = child_ref; } } @@ -236,4 +240,5 @@ char* ChannelTrace::RenderTrace() const { return json_str; } +} // namespace channelz } // namespace grpc_core diff --git a/src/core/lib/channel/channel_trace.h b/src/core/lib/channel/channel_trace.h index 1df1e585f25..687c6bc0630 100644 --- a/src/core/lib/channel/channel_trace.h +++ b/src/core/lib/channel/channel_trace.h @@ -28,11 +28,14 @@ #include "src/core/lib/json/json.h" namespace grpc_core { +namespace channelz { + +class Channel; // Object used to hold live data for a channel. This data is exposed via the // channelz service: // https://github.com/grpc/proposal/blob/master/A14-channelz.md -class ChannelTrace : public RefCounted { +class ChannelTrace { public: ChannelTrace(size_t max_events); ~ChannelTrace(); @@ -59,17 +62,15 @@ class ChannelTrace : public RefCounted { // created a new subchannel, then it would record that with a TraceEvent // referencing the new subchannel. // - // TODO(ncteisen): Once channelz is implemented, the events should reference - // the overall channelz object, not just the ChannelTrace object. // TODO(ncteisen): as this call is used more and more throughout the gRPC // stack, determine if it makes more sense to accept a char* instead of a // slice. void AddTraceEventReferencingChannel( Severity severity, grpc_slice data, - RefCountedPtr referenced_tracer); + RefCountedPtr referenced_tracer); void AddTraceEventReferencingSubchannel( Severity severity, grpc_slice data, - RefCountedPtr referenced_tracer); + RefCountedPtr referenced_tracer); // Returns the tracing data rendered as a grpc json string. // The string is owned by the caller and must be freed. @@ -77,17 +78,14 @@ class ChannelTrace : public RefCounted { private: // Types of objects that can be references by trace events. - enum ReferencedType { Channel, Subchannel }; + enum class ReferencedType { Channel, Subchannel }; // Private class to encapsulate all the data and bookkeeping needed for a // a trace event. class TraceEvent { public: // Constructor for a TraceEvent that references a different channel. - // TODO(ncteisen): once channelz is implemented, this should reference the - // overall channelz object, not just the ChannelTrace object TraceEvent(Severity severity, grpc_slice data, - RefCountedPtr referenced_tracer, - ReferencedType type); + RefCountedPtr referenced_tracer, ReferencedType type); // Constructor for a TraceEvent that does not reverence a different // channel. @@ -109,7 +107,7 @@ class ChannelTrace : public RefCounted { gpr_timespec timestamp_; TraceEvent* next_; // the tracer object for the (sub)channel that this trace event refers to. - RefCountedPtr referenced_tracer_; + RefCountedPtr referenced_channel_; // the type that the referenced tracer points to. Unused if this trace // does not point to any channel or subchannel ReferencedType referenced_type_; @@ -128,6 +126,7 @@ class ChannelTrace : public RefCounted { gpr_timespec time_created_; }; +} // namespace channelz } // namespace grpc_core #endif /* GRPC_CORE_LIB_CHANNEL_CHANNEL_TRACE_H */ diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index aabc941dcd5..cf7545621e5 100644 --- a/src/core/lib/channel/channelz.cc +++ b/src/core/lib/channel/channelz.cc @@ -91,7 +91,9 @@ grpc_json* add_num_str(grpc_json* parent, grpc_json* it, const char* name, } // namespace -Channel::Channel(grpc_channel* channel) : channel_(channel) { +Channel::Channel(grpc_channel* channel, size_t channel_tracer_max_nodes) + : channel_(channel) { + trace_.Init(channel_tracer_max_nodes); target_ = grpc_channel_get_target(channel_); channel_uuid_ = ChannelzRegistry::Register(this); } @@ -103,8 +105,8 @@ Channel::~Channel() { void Channel::CallStarted() { calls_started_++; - last_call_started_timestamp_ = grpc_millis_to_timespec( - grpc_core::ExecCtx::Get()->Now(), GPR_CLOCK_REALTIME); + last_call_started_timestamp_ = + grpc_millis_to_timespec(ExecCtx::Get()->Now(), GPR_CLOCK_REALTIME); } grpc_connectivity_state Channel::GetConnectivityState() { @@ -153,6 +155,11 @@ char* Channel::RenderJSON() { grpc_json_create_child(json_iterator, json, "state", grpc_connectivity_state_name(connectivity_state), GRPC_JSON_STRING, false); + char* trace = trace_->RenderTrace(); + if (trace != nullptr) { + json_iterator = grpc_json_create_child(json_iterator, json, "trace", trace, + GRPC_JSON_STRING, true); + } // render and return the over json object char* json_str = grpc_json_dump_to_string(top_level_json, 0); diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h index 661228cf446..7cea00392f5 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -24,6 +24,8 @@ #include #include "src/core/ext/filters/client_channel/client_channel.h" +#include "src/core/lib/channel/channel_trace.h" +#include "src/core/lib/gprpp/manual_constructor.h" #include "src/core/lib/gprpp/ref_counted.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/iomgr/error.h" @@ -35,7 +37,7 @@ namespace channelz { // owned by the client_channel that it points to and tracks class Channel : public RefCounted { public: - Channel(grpc_channel* channel); + Channel(grpc_channel* channel, size_t channel_tracer_max_nodes); ~Channel(); void CallStarted(); @@ -44,11 +46,15 @@ class Channel : public RefCounted { char* RenderJSON(); + ChannelTrace* Trace() { return trace_.get(); } + void set_channel_destroyed() { GPR_ASSERT(!channel_destroyed_); channel_destroyed_ = true; } + intptr_t channel_uuid() { return channel_uuid_; } + private: bool channel_destroyed_ = false; grpc_channel* channel_; @@ -58,6 +64,7 @@ class Channel : public RefCounted { uint64_t calls_failed_ = 0; gpr_timespec last_call_started_timestamp_; intptr_t channel_uuid_; + ManualConstructor trace_; grpc_connectivity_state GetConnectivityState(); }; diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index da66a120d57..22760f4ea93 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -67,7 +67,6 @@ struct grpc_channel { gpr_mu registered_call_mu; registered_call* registered_calls; - grpc_core::RefCountedPtr tracer; grpc_core::RefCountedPtr channelz_channel; char* target; @@ -147,13 +146,13 @@ grpc_channel* grpc_channel_create_with_builder( } grpc_channel_args_destroy(args); - channel->tracer = grpc_core::MakeRefCounted( - channel_tracer_max_nodes); - channel->tracer->AddTraceEvent( - grpc_core::ChannelTrace::Severity::Info, - grpc_slice_from_static_string("Channel created")); + channel_tracer_max_nodes = 10; channel->channelz_channel = - grpc_core::MakeRefCounted(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,7 +188,7 @@ static grpc_channel_args* build_channel_args( } char* grpc_channel_get_trace(grpc_channel* channel) { - return channel->tracer->RenderTrace(); + return channel->channelz_channel->Trace()->RenderTrace(); } char* grpc_channel_render_channelz(grpc_channel* channel) { @@ -202,7 +201,7 @@ grpc_core::channelz::Channel* grpc_channel_get_channelz_channel( } intptr_t grpc_channel_get_uuid(grpc_channel* channel) { - return channel->tracer->GetUuid(); + return channel->channelz_channel->Trace()->GetUuid(); } grpc_channel* grpc_channel_create(const char* target, @@ -416,7 +415,7 @@ static void destroy_channel(void* arg, grpc_error* error) { GRPC_MDELEM_UNREF(rc->authority); gpr_free(rc); } - channel->tracer.reset(); + channel->channelz_channel.reset(); gpr_mu_destroy(&channel->registered_call_mu); gpr_free(channel->target); gpr_free(channel); From 9a1bb051812a53462b2deb7e472f20e3e1dd785f Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 30 May 2018 16:17:06 -0700 Subject: [PATCH 04/28] Build out the channelz unit test --- include/grpc/grpc.h | 3 - src/core/lib/channel/channel_trace.cc | 16 +- src/core/lib/channel/channel_trace.h | 13 +- src/core/lib/channel/channelz.cc | 76 +++++--- src/core/lib/channel/channelz.h | 6 +- src/core/lib/surface/channel.cc | 5 +- test/core/channel/BUILD | 24 ++- test/core/channel/channel_trace_test.cc | 182 +++++++++++--------- test/core/channel/channelz_test.cc | 150 +++++++++++++++- test/cpp/util/channel_trace_proto_helper.cc | 11 +- 10 files changed, 340 insertions(+), 146 deletions(-) diff --git a/include/grpc/grpc.h b/include/grpc/grpc.h index de7c39b1834..dd8a5d7d5f8 100644 --- a/include/grpc/grpc.h +++ b/include/grpc/grpc.h @@ -294,9 +294,6 @@ GRPCAPI char* grpc_channel_get_trace(grpc_channel* channel); later time. */ GRPCAPI intptr_t grpc_channel_get_uuid(grpc_channel* channel); -/** channelz support */ -GRPCAPI char* grpc_channelz_get_channel(intptr_t channel_id); - /** 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. diff --git a/src/core/lib/channel/channel_trace.cc b/src/core/lib/channel/channel_trace.cc index efa034a1887..c82e42d5459 100644 --- a/src/core/lib/channel/channel_trace.cc +++ b/src/core/lib/channel/channel_trace.cc @@ -28,7 +28,6 @@ #include #include -#include "src/core/lib/channel/channelz_registry.h" #include "src/core/lib/channel/status_util.h" #include "src/core/lib/gpr/string.h" #include "src/core/lib/gpr/useful.h" @@ -63,15 +62,13 @@ ChannelTrace::TraceEvent::TraceEvent(Severity severity, grpc_slice data) ChannelTrace::TraceEvent::~TraceEvent() { grpc_slice_unref_internal(data_); } ChannelTrace::ChannelTrace(size_t max_events) - : channel_uuid_(-1), - num_events_logged_(0), + : num_events_logged_(0), list_size_(0), max_list_size_(max_events), head_trace_(nullptr), tail_trace_(nullptr) { if (max_list_size_ == 0) return; // tracing is disabled if max_events == 0 gpr_mu_init(&tracer_mu_); - channel_uuid_ = ChannelzRegistry::Register(this); time_created_ = grpc_millis_to_timespec(grpc_core::ExecCtx::Get()->Now(), GPR_CLOCK_REALTIME); } @@ -84,12 +81,9 @@ ChannelTrace::~ChannelTrace() { it = it->next(); Delete(to_free); } - ChannelzRegistry::Unregister(channel_uuid_); gpr_mu_destroy(&tracer_mu_); } -intptr_t ChannelTrace::GetUuid() const { return channel_uuid_; } - void ChannelTrace::AddTraceEventHelper(TraceEvent* new_trace_event) { ++num_events_logged_; // first event case @@ -212,7 +206,7 @@ void ChannelTrace::TraceEvent::RenderTraceEvent(grpc_json* json) const { } } -char* ChannelTrace::RenderTrace() const { +grpc_json* ChannelTrace::RenderJSON() const { if (!max_list_size_) return nullptr; // tracing is disabled if max_events == 0 grpc_json* json = grpc_json_create(GRPC_JSON_OBJECT); @@ -235,6 +229,12 @@ char* ChannelTrace::RenderTrace() const { it->RenderTraceEvent(json_iterator); it = it->next(); } + return json; +} + +char* ChannelTrace::RenderTrace() const { + grpc_json* json = RenderJSON(); + if (json == nullptr) return nullptr; char* json_str = grpc_json_dump_to_string(json, 0); grpc_json_destroy(json); return json_str; diff --git a/src/core/lib/channel/channel_trace.h b/src/core/lib/channel/channel_trace.h index 687c6bc0630..b1224bb96c4 100644 --- a/src/core/lib/channel/channel_trace.h +++ b/src/core/lib/channel/channel_trace.h @@ -40,9 +40,6 @@ class ChannelTrace { ChannelTrace(size_t max_events); ~ChannelTrace(); - // returns the tracer's uuid - intptr_t GetUuid() const; - enum Severity { Unset = 0, // never to be used Info, // we start at 1 to avoid using proto default values @@ -72,8 +69,13 @@ class ChannelTrace { Severity severity, grpc_slice data, RefCountedPtr referenced_tracer); - // Returns the tracing data rendered as a grpc json string. - // The string is owned by the caller and must be freed. + // Creates and returns the raw grpc_json object, so a parent channelz + // object may incorporate the json before rendering. + grpc_json* RenderJSON() const; + + // Returns the tracing data rendered as a grpc json string. The string + // is owned by the caller and must be freed. This is used for testing only + // so that we may unit test ChannelTrace in isolation. char* RenderTrace() const; private: @@ -117,7 +119,6 @@ class ChannelTrace { void AddTraceEventHelper(TraceEvent* new_trace_event); gpr_mu tracer_mu_; - intptr_t channel_uuid_; uint64_t num_events_logged_; size_t list_size_; size_t max_list_size_; diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index cf7545621e5..5c3dc97e690 100644 --- a/src/core/lib/channel/channelz.cc +++ b/src/core/lib/channel/channelz.cc @@ -39,9 +39,6 @@ #include "src/core/lib/transport/connectivity_state.h" #include "src/core/lib/transport/error_utils.h" -// TODO(ncteisen): actually implement this -char* grpc_channelz_get_channel(intptr_t channel_id) { return nullptr; } - namespace grpc_core { namespace channelz { @@ -82,9 +79,9 @@ char* fmt_time(gpr_timespec tm) { // TODO(ncteisen); move this to json library grpc_json* add_num_str(grpc_json* parent, grpc_json* it, const char* name, - uint64_t num) { + int64_t num) { char* num_str; - gpr_asprintf(&num_str, "%" PRIu64, num); + gpr_asprintf(&num_str, "%" PRId64, num); return grpc_json_create_child(it, parent, name, num_str, GRPC_JSON_STRING, true); } @@ -96,10 +93,13 @@ Channel::Channel(grpc_channel* channel, size_t channel_tracer_max_nodes) trace_.Init(channel_tracer_max_nodes); target_ = grpc_channel_get_target(channel_); channel_uuid_ = ChannelzRegistry::Register(this); + last_call_started_timestamp_ = + grpc_millis_to_timespec(ExecCtx::Get()->Now(), GPR_CLOCK_REALTIME); } Channel::~Channel() { gpr_free(const_cast(target_)); + trace_.Destroy(); ChannelzRegistry::Unregister(channel_uuid_); } @@ -125,7 +125,7 @@ char* Channel::RenderJSON() { // create and fill the ref child json_iterator = grpc_json_create_child(json_iterator, json, "ref", nullptr, - GRPC_JSON_OBJECT, true); + GRPC_JSON_OBJECT, false); json = json_iterator; json_iterator = nullptr; json_iterator = add_num_str(json, json_iterator, "channelId", channel_uuid_); @@ -134,33 +134,55 @@ char* Channel::RenderJSON() { json = top_level_json; json_iterator = nullptr; - // create and fill the data child - json_iterator = grpc_json_create_child(json_iterator, json, "data", nullptr, - GRPC_JSON_OBJECT, true); - json = json_iterator; + // create and fill the data child. + grpc_json* data = grpc_json_create_child(json_iterator, json, "data", nullptr, + GRPC_JSON_OBJECT, false); + + json = data; json_iterator = nullptr; - json_iterator = - add_num_str(json, json_iterator, "callsStarted", calls_started_); - json_iterator = - add_num_str(json, json_iterator, "callsSucceeded", calls_succeeded_); - json_iterator = - add_num_str(json, json_iterator, "callsFailed", calls_failed_); - json_iterator = grpc_json_create_child( - json_iterator, json, "lastCallStartedTimestamp", - fmt_time(last_call_started_timestamp_), GRPC_JSON_STRING, true); + + // create and fill the connectivity state child. + grpc_connectivity_state connectivity_state = GetConnectivityState(); + json_iterator = grpc_json_create_child(json_iterator, json, "state", nullptr, + GRPC_JSON_OBJECT, false); + json = json_iterator; + grpc_json_create_child(nullptr, json, "state", + grpc_connectivity_state_name(connectivity_state), + GRPC_JSON_STRING, false); + + // reset the parent to be the data object. + json = data; json_iterator = grpc_json_create_child(json_iterator, json, "target", target_, GRPC_JSON_STRING, false); - grpc_connectivity_state connectivity_state = GetConnectivityState(); - json_iterator = - grpc_json_create_child(json_iterator, json, "state", - grpc_connectivity_state_name(connectivity_state), - GRPC_JSON_STRING, false); - char* trace = trace_->RenderTrace(); + + // fill in the channel trace if applicable + grpc_json* trace = trace_->RenderJSON(); if (trace != nullptr) { - json_iterator = grpc_json_create_child(json_iterator, json, "trace", trace, - GRPC_JSON_STRING, true); + // we manuall 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; + trace->value = nullptr; + trace->key = "trace"; + trace->owns_value = false; } + // reset the parent to be the data object. + json = data; + json_iterator = nullptr; + + // We use -1 as sentinel values since proto default value for integers is + // zero, and the confuses the parser into thinking the value weren't present + json_iterator = add_num_str(json, json_iterator, "callsStarted", + calls_started_ ? calls_started_ : -1); + json_iterator = add_num_str(json, json_iterator, "callsSucceeded", + calls_succeeded_ ? calls_succeeded_ : -1); + json_iterator = add_num_str(json, json_iterator, "callsFailed", + calls_failed_ ? calls_failed_ : -1); + json_iterator = grpc_json_create_child( + json_iterator, json, "lastCallStartedTimestamp", + fmt_time(last_call_started_timestamp_), GRPC_JSON_STRING, true); + // render and return the over json object char* json_str = grpc_json_dump_to_string(top_level_json, 0); grpc_json_destroy(top_level_json); diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h index 7cea00392f5..63b90e90a0c 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -59,9 +59,9 @@ class Channel : public RefCounted { bool channel_destroyed_ = false; grpc_channel* channel_; const char* target_; - uint64_t calls_started_ = 0; - uint64_t calls_succeeded_ = 0; - uint64_t calls_failed_ = 0; + int64_t calls_started_ = 0; + int64_t calls_succeeded_ = 0; + int64_t calls_failed_ = 0; gpr_timespec last_call_started_timestamp_; intptr_t channel_uuid_; ManualConstructor trace_; diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index 22760f4ea93..be9869b08c4 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -146,7 +146,6 @@ grpc_channel* grpc_channel_create_with_builder( } grpc_channel_args_destroy(args); - channel_tracer_max_nodes = 10; channel->channelz_channel = grpc_core::MakeRefCounted( channel, channel_tracer_max_nodes); @@ -201,7 +200,7 @@ grpc_core::channelz::Channel* grpc_channel_get_channelz_channel( } intptr_t grpc_channel_get_uuid(grpc_channel* channel) { - return channel->channelz_channel->Trace()->GetUuid(); + return channel->channelz_channel->channel_uuid(); } grpc_channel* grpc_channel_create(const char* target, @@ -430,6 +429,8 @@ void grpc_channel_destroy(grpc_channel* channel) { GRPC_ERROR_CREATE_FROM_STATIC_STRING("Channel Destroyed"); elem = grpc_channel_stack_element(CHANNEL_STACK_FROM_CHANNEL(channel), 0); elem->filter->start_transport_op(elem, op); + channel->channelz_channel->set_channel_destroyed(); + channel->channelz_channel.reset(); GRPC_CHANNEL_INTERNAL_UNREF(channel, "channel"); } diff --git a/test/core/channel/BUILD b/test/core/channel/BUILD index c336688209d..81c43531293 100644 --- a/test/core/channel/BUILD +++ b/test/core/channel/BUILD @@ -84,13 +84,8 @@ grpc_cc_test( ) grpc_cc_test( -<<<<<<< HEAD - name = "channelz_registry_test", - srcs = ["channelz_registry_test.cc"], -======= name = "channelz_test", srcs = ["channelz_test.cc"], ->>>>>>> Add channelz test language = "C++", deps = [ "//:gpr", @@ -98,10 +93,23 @@ grpc_cc_test( "//:grpc++", "//test/core/util:gpr_test_util", "//test/core/util:grpc_test_util", -<<<<<<< HEAD -======= "//test/cpp/util:channel_trace_proto_helper", ->>>>>>> Add channelz test + ], + external_deps = [ + "gtest", + ], +) + +grpc_cc_test( + name = "channelz_registry_test", + srcs = ["channelz_registry_test.cc"], + language = "C++", + deps = [ + "//:gpr", + "//:grpc", + "//:grpc++", + "//test/core/util:gpr_test_util", + "//test/core/util:grpc_test_util", ], external_deps = [ "gtest", diff --git a/test/core/channel/channel_trace_test.cc b/test/core/channel/channel_trace_test.cc index 9befdb4fa37..c8619be0074 100644 --- a/test/core/channel/channel_trace_test.cc +++ b/test/core/channel/channel_trace_test.cc @@ -25,6 +25,7 @@ #include #include "src/core/lib/channel/channel_trace.h" +#include "src/core/lib/channel/channelz.h" #include "src/core/lib/channel/channelz_registry.h" #include "src/core/lib/gpr/useful.h" #include "src/core/lib/iomgr/exec_ctx.h" @@ -39,6 +40,7 @@ #include namespace grpc_core { +namespace channelz { namespace testing { namespace { @@ -77,13 +79,13 @@ void ValidateChannelTraceData(grpc_json* json, ValidateJsonArraySize(json, "events", actual_num_events_expected); } -void AddSimpleTrace(RefCountedPtr tracer) { +void AddSimpleTrace(ChannelTrace* tracer) { tracer->AddTraceEvent(ChannelTrace::Severity::Info, grpc_slice_from_static_string("simple trace")); } // checks for the existence of all the required members of the tracer. -void ValidateChannelTrace(RefCountedPtr tracer, +void ValidateChannelTrace(ChannelTrace* tracer, size_t expected_num_event_logged, size_t max_nodes) { if (!max_nodes) return; char* json_str = tracer->RenderTrace(); @@ -95,16 +97,26 @@ void ValidateChannelTrace(RefCountedPtr tracer, gpr_free(json_str); } -void ValidateTraceDataMatchedUuidLookup(RefCountedPtr tracer) { - intptr_t uuid = tracer->GetUuid(); - if (uuid == -1) return; // Doesn't make sense to lookup if tracing disabled - char* tracer_json_str = tracer->RenderTrace(); - ChannelTrace* uuid_lookup = ChannelzRegistry::Get(uuid); - char* uuid_lookup_json_str = uuid_lookup->RenderTrace(); - EXPECT_EQ(strcmp(tracer_json_str, uuid_lookup_json_str), 0); - gpr_free(tracer_json_str); - gpr_free(uuid_lookup_json_str); -} +class ChannelFixture { + public: + ChannelFixture(int max_trace_nodes) { + 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 = max_trace_nodes; + grpc_channel_args client_args = {1, &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_; +}; } // anonymous namespace @@ -114,25 +126,22 @@ class ChannelTracerTest : public ::testing::TestWithParam {}; // lookups by uuid. TEST_P(ChannelTracerTest, BasicTest) { grpc_core::ExecCtx exec_ctx; - RefCountedPtr tracer = MakeRefCounted(GetParam()); - AddSimpleTrace(tracer); - AddSimpleTrace(tracer); - ValidateTraceDataMatchedUuidLookup(tracer); - tracer->AddTraceEvent(ChannelTrace::Severity::Info, - grpc_slice_from_static_string("trace three")); - tracer->AddTraceEvent(ChannelTrace::Severity::Error, - grpc_slice_from_static_string("trace four error")); - ValidateChannelTrace(tracer, 4, GetParam()); - AddSimpleTrace(tracer); - AddSimpleTrace(tracer); - ValidateChannelTrace(tracer, 6, GetParam()); - AddSimpleTrace(tracer); - AddSimpleTrace(tracer); - AddSimpleTrace(tracer); - AddSimpleTrace(tracer); - ValidateChannelTrace(tracer, 10, GetParam()); - ValidateTraceDataMatchedUuidLookup(tracer); - tracer.reset(nullptr); + ChannelTrace tracer(GetParam()); + AddSimpleTrace(&tracer); + AddSimpleTrace(&tracer); + tracer.AddTraceEvent(ChannelTrace::Severity::Info, + grpc_slice_from_static_string("trace three")); + tracer.AddTraceEvent(ChannelTrace::Severity::Error, + grpc_slice_from_static_string("trace four error")); + ValidateChannelTrace(&tracer, 4, GetParam()); + AddSimpleTrace(&tracer); + AddSimpleTrace(&tracer); + ValidateChannelTrace(&tracer, 6, GetParam()); + AddSimpleTrace(&tracer); + AddSimpleTrace(&tracer); + AddSimpleTrace(&tracer); + AddSimpleTrace(&tracer); + ValidateChannelTrace(&tracer, 10, GetParam()); } // Tests more complex functionality, like a parent channel tracking @@ -140,42 +149,43 @@ TEST_P(ChannelTracerTest, BasicTest) { // and this function will both hold refs to the subchannel. TEST_P(ChannelTracerTest, ComplexTest) { grpc_core::ExecCtx exec_ctx; - RefCountedPtr tracer = MakeRefCounted(GetParam()); - AddSimpleTrace(tracer); - AddSimpleTrace(tracer); - RefCountedPtr sc1 = MakeRefCounted(GetParam()); - tracer->AddTraceEventReferencingSubchannel( + ChannelTrace tracer(GetParam()); + AddSimpleTrace(&tracer); + AddSimpleTrace(&tracer); + ChannelFixture channel1(GetParam()); + RefCountedPtr sc1 = + MakeRefCounted(channel1.channel(), GetParam()); + tracer.AddTraceEventReferencingSubchannel( ChannelTrace::Severity::Info, grpc_slice_from_static_string("subchannel one created"), sc1); - ValidateChannelTrace(tracer, 3, GetParam()); - AddSimpleTrace(sc1); - AddSimpleTrace(sc1); - AddSimpleTrace(sc1); - ValidateChannelTrace(sc1, 3, GetParam()); - AddSimpleTrace(sc1); - AddSimpleTrace(sc1); - AddSimpleTrace(sc1); - ValidateChannelTrace(sc1, 6, GetParam()); - AddSimpleTrace(tracer); - AddSimpleTrace(tracer); - ValidateChannelTrace(tracer, 5, GetParam()); - ValidateTraceDataMatchedUuidLookup(tracer); - RefCountedPtr sc2 = MakeRefCounted(GetParam()); - tracer->AddTraceEventReferencingChannel( + ValidateChannelTrace(&tracer, 3, GetParam()); + AddSimpleTrace(sc1->Trace()); + AddSimpleTrace(sc1->Trace()); + AddSimpleTrace(sc1->Trace()); + ValidateChannelTrace(sc1->Trace(), 3, GetParam()); + AddSimpleTrace(sc1->Trace()); + AddSimpleTrace(sc1->Trace()); + AddSimpleTrace(sc1->Trace()); + ValidateChannelTrace(sc1->Trace(), 6, GetParam()); + AddSimpleTrace(&tracer); + AddSimpleTrace(&tracer); + ValidateChannelTrace(&tracer, 5, GetParam()); + ChannelFixture channel2(GetParam()); + RefCountedPtr sc2 = + MakeRefCounted(channel2.channel(), GetParam()); + tracer.AddTraceEventReferencingChannel( ChannelTrace::Severity::Info, grpc_slice_from_static_string("LB channel two created"), sc2); - tracer->AddTraceEventReferencingSubchannel( + tracer.AddTraceEventReferencingSubchannel( ChannelTrace::Severity::Warning, grpc_slice_from_static_string("subchannel one inactive"), sc1); - ValidateChannelTrace(tracer, 7, GetParam()); - AddSimpleTrace(tracer); - AddSimpleTrace(tracer); - AddSimpleTrace(tracer); - AddSimpleTrace(tracer); - AddSimpleTrace(tracer); - AddSimpleTrace(tracer); - ValidateTraceDataMatchedUuidLookup(tracer); - tracer.reset(nullptr); + ValidateChannelTrace(&tracer, 7, GetParam()); + AddSimpleTrace(&tracer); + AddSimpleTrace(&tracer); + AddSimpleTrace(&tracer); + AddSimpleTrace(&tracer); + AddSimpleTrace(&tracer); + AddSimpleTrace(&tracer); sc1.reset(nullptr); sc2.reset(nullptr); } @@ -185,39 +195,44 @@ TEST_P(ChannelTracerTest, ComplexTest) { // gets deleted. TEST_P(ChannelTracerTest, TestNesting) { grpc_core::ExecCtx exec_ctx; - RefCountedPtr tracer = MakeRefCounted(GetParam()); - AddSimpleTrace(tracer); - AddSimpleTrace(tracer); - ValidateChannelTrace(tracer, 2, GetParam()); - RefCountedPtr sc1 = MakeRefCounted(GetParam()); - tracer->AddTraceEventReferencingChannel( + ChannelTrace tracer(GetParam()); + AddSimpleTrace(&tracer); + AddSimpleTrace(&tracer); + ValidateChannelTrace(&tracer, 2, GetParam()); + ChannelFixture channel1(GetParam()); + RefCountedPtr sc1 = + MakeRefCounted(channel1.channel(), GetParam()); + tracer.AddTraceEventReferencingChannel( ChannelTrace::Severity::Info, grpc_slice_from_static_string("subchannel one created"), sc1); - ValidateChannelTrace(tracer, 3, GetParam()); - AddSimpleTrace(sc1); - RefCountedPtr conn1 = MakeRefCounted(GetParam()); + ValidateChannelTrace(&tracer, 3, GetParam()); + AddSimpleTrace(sc1->Trace()); + ChannelFixture channel2(GetParam()); + RefCountedPtr conn1 = + MakeRefCounted(channel2.channel(), GetParam()); // nesting one level deeper. - sc1->AddTraceEventReferencingSubchannel( + sc1->Trace()->AddTraceEventReferencingSubchannel( ChannelTrace::Severity::Info, grpc_slice_from_static_string("connection one created"), conn1); - ValidateChannelTrace(tracer, 3, GetParam()); - AddSimpleTrace(conn1); - AddSimpleTrace(tracer); - AddSimpleTrace(tracer); - ValidateChannelTrace(tracer, 5, GetParam()); - ValidateChannelTrace(conn1, 1, GetParam()); - RefCountedPtr sc2 = MakeRefCounted(GetParam()); - tracer->AddTraceEventReferencingSubchannel( + ValidateChannelTrace(&tracer, 3, GetParam()); + AddSimpleTrace(conn1->Trace()); + AddSimpleTrace(&tracer); + AddSimpleTrace(&tracer); + ValidateChannelTrace(&tracer, 5, GetParam()); + ValidateChannelTrace(conn1->Trace(), 1, GetParam()); + ChannelFixture channel3(GetParam()); + RefCountedPtr sc2 = + MakeRefCounted(channel3.channel(), GetParam()); + tracer.AddTraceEventReferencingSubchannel( ChannelTrace::Severity::Info, grpc_slice_from_static_string("subchannel two created"), sc2); // this trace should not get added to the parents children since it is already // present in the tracer. - tracer->AddTraceEventReferencingChannel( + tracer.AddTraceEventReferencingChannel( ChannelTrace::Severity::Warning, grpc_slice_from_static_string("subchannel one inactive"), sc1); - AddSimpleTrace(tracer); - ValidateChannelTrace(tracer, 8, GetParam()); - tracer.reset(nullptr); + AddSimpleTrace(&tracer); + ValidateChannelTrace(&tracer, 8, GetParam()); sc1.reset(nullptr); sc2.reset(nullptr); conn1.reset(nullptr); @@ -227,6 +242,7 @@ INSTANTIATE_TEST_CASE_P(ChannelTracerTestSweep, ChannelTracerTest, ::testing::Values(0, 1, 2, 6, 10, 15)); } // namespace testing +} // namespace channelz } // namespace grpc_core int main(int argc, char** argv) { diff --git a/test/core/channel/channelz_test.cc b/test/core/channel/channelz_test.cc index 503bb9065ba..c4606426095 100644 --- a/test/core/channel/channelz_test.cc +++ b/test/core/channel/channelz_test.cc @@ -25,26 +25,170 @@ #include #include "src/core/lib/channel/channel_trace.h" +#include "src/core/lib/channel/channelz.h" #include "src/core/lib/channel/channelz_registry.h" #include "src/core/lib/gpr/useful.h" #include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/json/json.h" +#include "src/core/lib/surface/channel.h" #include "test/core/util/test_config.h" #include "test/cpp/util/channel_trace_proto_helper.h" -// remove me #include #include #include namespace grpc_core { +namespace channelz { namespace testing { -namespace {} // anonymous namespace +namespace { -TEST(ChannelzTest, Channel) {} +grpc_json* GetJsonChild(grpc_json* parent, const char* key) { + EXPECT_NE(parent, nullptr); + for (grpc_json* child = parent->child; child != nullptr; + child = child->next) { + if (child->key != nullptr && strcmp(child->key, key) == 0) return child; + } + return nullptr; +} + +class ChannelFixture { + public: + ChannelFixture(int max_trace_nodes) { + 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 = max_trace_nodes; + grpc_channel_args client_args = {1, &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_; +}; + +struct validate_channel_data_args { + int64_t calls_started; + int64_t calls_failed; + int64_t calls_succeeded; +}; + +void ValidateChildInteger(grpc_json* json, int64_t expect, const char* key) { + grpc_json* gotten_json = GetJsonChild(json, key); + EXPECT_NE(gotten_json, nullptr); + int64_t gotten_number = (int64_t)strtol(gotten_json->value, nullptr, 0); + EXPECT_EQ(gotten_number, expect); +} + +void ValidateChannel(Channel* channel, validate_channel_data_args args) { + char* json_str = channel->RenderJSON(); + grpc::testing::ValidateChannelProtoJsonTranslation(json_str); + grpc_json* json = grpc_json_parse_string(json_str); + EXPECT_NE(json, nullptr); + grpc_json* data = GetJsonChild(json, "data"); + ValidateChildInteger(data, args.calls_started, "callsStarted"); + ValidateChildInteger(data, args.calls_failed, "callsFailed"); + ValidateChildInteger(data, args.calls_succeeded, "callsSucceeded"); + grpc_json_destroy(json); + gpr_free(json_str); +} + +char* GetLastCallStartedTimestamp(Channel* channel) { + char* json_str = channel->RenderJSON(); + grpc_json* json = grpc_json_parse_string(json_str); + grpc_json* data = GetJsonChild(json, "data"); + grpc_json* timestamp = GetJsonChild(data, "lastCallStartedTimestamp"); + char* ts_str = grpc_json_dump_to_string(timestamp, 0); + grpc_json_destroy(json); + gpr_free(json_str); + return ts_str; +} + +void ChannelzSleep(int64_t sleep_us) { + gpr_sleep_until(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME), + gpr_time_from_micros(sleep_us, GPR_TIMESPAN))); + grpc_core::ExecCtx::Get()->InvalidateNow(); +} + +} // anonymous namespace + +class ChannelzChannelTest : public ::testing::TestWithParam {}; + +TEST_P(ChannelzChannelTest, BasicChannel) { + grpc_core::ExecCtx exec_ctx; + ChannelFixture channel(GetParam()); + intptr_t uuid = grpc_channel_get_uuid(channel.channel()); + Channel* channelz_channel = ChannelzRegistry::Get(uuid); + ValidateChannel(channelz_channel, {-1, -1, -1}); +} + +TEST_P(ChannelzChannelTest, BasicChannelAPIFunctionality) { + grpc_core::ExecCtx exec_ctx; + ChannelFixture channel(GetParam()); + intptr_t uuid = grpc_channel_get_uuid(channel.channel()); + Channel* channelz_channel = ChannelzRegistry::Get(uuid); + channelz_channel->CallStarted(); + channelz_channel->CallFailed(); + channelz_channel->CallSucceeded(); + ValidateChannel(channelz_channel, {1, 1, 1}); + channelz_channel->CallStarted(); + channelz_channel->CallFailed(); + channelz_channel->CallSucceeded(); + channelz_channel->CallStarted(); + channelz_channel->CallFailed(); + channelz_channel->CallSucceeded(); + ValidateChannel(channelz_channel, {3, 3, 3}); +} + +TEST_P(ChannelzChannelTest, LastCallStartedTimestamp) { + grpc_core::ExecCtx exec_ctx; + ChannelFixture channel(GetParam()); + intptr_t uuid = grpc_channel_get_uuid(channel.channel()); + Channel* channelz_channel = ChannelzRegistry::Get(uuid); + + // start a call to set the last call started timestamp + channelz_channel->CallStarted(); + char* ts1 = GetLastCallStartedTimestamp(channelz_channel); + + // time gone by should not affect the timestamp + ChannelzSleep(100); + char* ts2 = GetLastCallStartedTimestamp(channelz_channel); + EXPECT_STREQ(ts1, ts2); + + // calls succeeded or failed should not affect the timestamp + ChannelzSleep(100); + channelz_channel->CallFailed(); + channelz_channel->CallSucceeded(); + char* ts3 = GetLastCallStartedTimestamp(channelz_channel); + EXPECT_STREQ(ts1, ts3); + + // another call started should affect the timestamp + // sleep for extra long to avoid flakes (since we cache Now()) + ChannelzSleep(5000); + grpc_core::ExecCtx::Get()->InvalidateNow(); + channelz_channel->CallStarted(); + char* ts4 = GetLastCallStartedTimestamp(channelz_channel); + EXPECT_STRNE(ts1, ts4); + + // clean up + gpr_free(ts1); + gpr_free(ts2); + gpr_free(ts3); + gpr_free(ts4); +} + +INSTANTIATE_TEST_CASE_P(ChannelzChannelTestSweep, ChannelzChannelTest, + ::testing::Values(0, 1, 2, 6, 10, 15)); } // namespace testing +} // namespace channelz } // namespace grpc_core int main(int argc, char** argv) { diff --git a/test/cpp/util/channel_trace_proto_helper.cc b/test/cpp/util/channel_trace_proto_helper.cc index db9390163bc..ee310784c21 100644 --- a/test/cpp/util/channel_trace_proto_helper.cc +++ b/test/cpp/util/channel_trace_proto_helper.cc @@ -45,16 +45,21 @@ void VaidateProtoJsonTranslation(char* json_c_str) { // then compare the output, and determine what fields are missing. // // parse_options.ignore_unknown_fields = true; - ASSERT_EQ(google::protobuf::util::JsonStringToMessage(json_str, &msg, + EXPECT_EQ(google::protobuf::util::JsonStringToMessage(json_str, &msg, parse_options), google::protobuf::util::Status::OK); std::string proto_json_str; - ASSERT_EQ(google::protobuf::util::MessageToJsonString(msg, &proto_json_str), + google::protobuf::util::JsonPrintOptions print_options; + // We usually do not want this to be true, however it can be helpful to + // uncomment and see the output produced then all fields are printed. + // print_options.always_print_primitive_fields = true; + EXPECT_EQ(google::protobuf::util::MessageToJsonString(msg, &proto_json_str, + print_options), google::protobuf::util::Status::OK); // uncomment these to compare the the json strings. // gpr_log(GPR_ERROR, "tracer json: %s", json_str.c_str()); // gpr_log(GPR_ERROR, "proto json: %s", proto_json_str.c_str()); - ASSERT_EQ(json_str, proto_json_str); + EXPECT_EQ(json_str, proto_json_str); } } // namespace From c03c9685a2461b097c17e2298be506520e15a5b1 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 30 May 2018 23:21:03 -0700 Subject: [PATCH 05/28] Add sanity channelz test to simple_request --- src/core/lib/channel/channelz.h | 2 -- test/core/end2end/tests/simple_request.cc | 23 ++++++++++++++++++----- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h index 63b90e90a0c..5cfafc9e146 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -23,7 +23,6 @@ #include -#include "src/core/ext/filters/client_channel/client_channel.h" #include "src/core/lib/channel/channel_trace.h" #include "src/core/lib/gprpp/manual_constructor.h" #include "src/core/lib/gprpp/ref_counted.h" @@ -34,7 +33,6 @@ namespace grpc_core { namespace channelz { -// owned by the client_channel that it points to and tracks class Channel : public RefCounted { public: Channel(grpc_channel* channel, size_t channel_tracer_max_nodes); diff --git a/test/core/end2end/tests/simple_request.cc b/test/core/end2end/tests/simple_request.cc index 6e36e54cd37..a9341edff96 100644 --- a/test/core/end2end/tests/simple_request.cc +++ b/test/core/end2end/tests/simple_request.cc @@ -21,7 +21,6 @@ #include #include -#include "src/core/ext/filters/client_channel/client_channel.h" #include "src/core/lib/surface/channel.h" #include @@ -201,10 +200,6 @@ static void simple_request_body(grpc_end2end_test_config config, CQ_EXPECT_COMPLETION(cqv, tag(1), 1); cq_verify(cqv); - char* json = grpc_channel_render_channelz(f.client); - gpr_log(GPR_ERROR, "%s", json); - gpr_free(json); - GPR_ASSERT(status == GRPC_STATUS_UNIMPLEMENTED); GPR_ASSERT(0 == grpc_slice_str_cmp(details, "xyz")); // the following sanity check makes sure that the requested error string is @@ -259,6 +254,16 @@ static void test_invoke_simple_request(grpc_end2end_test_config config) { f = begin_test(config, "test_invoke_simple_request", nullptr, nullptr); simple_request_body(config, f); + + // The following is a quick sanity check on channelz functionality. It + // ensures that core properly tracked the one call that occurred in this + // simple end2end test. + char* json = grpc_channel_render_channelz(f.client); + GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"1\"")); + GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"1\"")); + GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"-1\"")); + gpr_free(json); + end_test(&f); config.tear_down_data(&f); } @@ -271,6 +276,14 @@ static void test_invoke_10_simple_requests(grpc_end2end_test_config config) { simple_request_body(config, f); gpr_log(GPR_INFO, "Running test: Passed simple request %d", i); } + + // The following is a quick sanity check on channelz functionality. It + // ensures that core properly tracked the ten calls that occurred. + char* json = grpc_channel_render_channelz(f.client); + GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"10\"")); + GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"10\"")); + GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"-1\"")); + gpr_free(json); end_test(&f); config.tear_down_data(&f); } From ec482847b29e4efb07e1eb23fc0fed1ea4a5aebc Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 30 May 2018 23:31:15 -0700 Subject: [PATCH 06/28] re run generate projects --- CMakeLists.txt | 7 +++++++ Makefile | 4 ++++ build.yaml | 4 ++++ grpc.def | 1 - src/ruby/ext/grpc/rb_grpc_imports.generated.c | 2 -- src/ruby/ext/grpc/rb_grpc_imports.generated.h | 3 --- test/core/surface/public_headers_must_be_c89.c | 1 - tools/run_tests/generated/sources_and_headers.json | 2 ++ 8 files changed, 17 insertions(+), 7 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 37bde81a9d0..29352da5f6c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -10479,10 +10479,17 @@ if (gRPC_BUILD_TESTS) add_executable(channel_trace_test test/core/channel/channel_trace_test.cc + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/channelz/channelz.pb.cc + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/channelz/channelz.grpc.pb.cc + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/channelz/channelz.pb.h + ${_gRPC_PROTO_GENS_DIR}/src/proto/grpc/channelz/channelz.grpc.pb.h third_party/googletest/googletest/src/gtest-all.cc third_party/googletest/googlemock/src/gmock-all.cc ) +protobuf_generate_grpc_cpp( + src/proto/grpc/channelz/channelz.proto +) target_include_directories(channel_trace_test PRIVATE ${CMAKE_CURRENT_SOURCE_DIR} diff --git a/Makefile b/Makefile index f5423ea6435..7d56686fe28 100644 --- a/Makefile +++ b/Makefile @@ -16307,6 +16307,7 @@ endif CHANNEL_TRACE_TEST_SRC = \ test/core/channel/channel_trace_test.cc \ + $(GENDIR)/src/proto/grpc/channelz/channelz.pb.cc $(GENDIR)/src/proto/grpc/channelz/channelz.grpc.pb.cc \ CHANNEL_TRACE_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(CHANNEL_TRACE_TEST_SRC)))) ifeq ($(NO_SECURE),true) @@ -16339,6 +16340,8 @@ endif $(OBJDIR)/$(CONFIG)/test/core/channel/channel_trace_test.o: $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a +$(OBJDIR)/$(CONFIG)/src/proto/grpc/channelz/channelz.o: $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + deps_channel_trace_test: $(CHANNEL_TRACE_TEST_OBJS:.o=.dep) ifneq ($(NO_SECURE),true) @@ -16346,6 +16349,7 @@ ifneq ($(NO_DEPS),true) -include $(CHANNEL_TRACE_TEST_OBJS:.o=.dep) endif endif +$(OBJDIR)/$(CONFIG)/test/core/channel/channel_trace_test.o: $(GENDIR)/src/proto/grpc/channelz/channelz.pb.cc $(GENDIR)/src/proto/grpc/channelz/channelz.grpc.pb.cc CHANNELZ_REGISTRY_TEST_SRC = \ diff --git a/build.yaml b/build.yaml index 3df140e9222..90a9f9c5a76 100644 --- a/build.yaml +++ b/build.yaml @@ -4205,6 +4205,10 @@ targets: - grpc - gpr_test_util - gpr + filegroups: + - grpc++_channelz_proto + uses: + - grpc++_test - name: channelz_registry_test gtest: true build: test diff --git a/grpc.def b/grpc.def index 1a8c40f9509..6a91214e8c3 100644 --- a/grpc.def +++ b/grpc.def @@ -47,7 +47,6 @@ EXPORTS grpc_channel_destroy grpc_channel_get_trace grpc_channel_get_uuid - grpc_channelz_get_channel grpc_call_cancel grpc_call_cancel_with_status grpc_call_ref diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.c b/src/ruby/ext/grpc/rb_grpc_imports.generated.c index 075219a3af3..02f84c0b962 100644 --- a/src/ruby/ext/grpc/rb_grpc_imports.generated.c +++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.c @@ -70,7 +70,6 @@ grpc_lame_client_channel_create_type grpc_lame_client_channel_create_import; grpc_channel_destroy_type grpc_channel_destroy_import; grpc_channel_get_trace_type grpc_channel_get_trace_import; grpc_channel_get_uuid_type grpc_channel_get_uuid_import; -grpc_channelz_get_channel_type grpc_channelz_get_channel_import; grpc_call_cancel_type grpc_call_cancel_import; grpc_call_cancel_with_status_type grpc_call_cancel_with_status_import; grpc_call_ref_type grpc_call_ref_import; @@ -319,7 +318,6 @@ void grpc_rb_load_imports(HMODULE library) { grpc_channel_destroy_import = (grpc_channel_destroy_type) GetProcAddress(library, "grpc_channel_destroy"); grpc_channel_get_trace_import = (grpc_channel_get_trace_type) GetProcAddress(library, "grpc_channel_get_trace"); grpc_channel_get_uuid_import = (grpc_channel_get_uuid_type) GetProcAddress(library, "grpc_channel_get_uuid"); - grpc_channelz_get_channel_import = (grpc_channelz_get_channel_type) GetProcAddress(library, "grpc_channelz_get_channel"); grpc_call_cancel_import = (grpc_call_cancel_type) GetProcAddress(library, "grpc_call_cancel"); grpc_call_cancel_with_status_import = (grpc_call_cancel_with_status_type) GetProcAddress(library, "grpc_call_cancel_with_status"); grpc_call_ref_import = (grpc_call_ref_type) GetProcAddress(library, "grpc_call_ref"); diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.h b/src/ruby/ext/grpc/rb_grpc_imports.generated.h index 590a6b2439b..b2186a69aa9 100644 --- a/src/ruby/ext/grpc/rb_grpc_imports.generated.h +++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.h @@ -185,9 +185,6 @@ extern grpc_channel_get_trace_type grpc_channel_get_trace_import; typedef intptr_t(*grpc_channel_get_uuid_type)(grpc_channel* channel); extern grpc_channel_get_uuid_type grpc_channel_get_uuid_import; #define grpc_channel_get_uuid grpc_channel_get_uuid_import -typedef char*(*grpc_channelz_get_channel_type)(intptr_t channel_id); -extern grpc_channelz_get_channel_type grpc_channelz_get_channel_import; -#define grpc_channelz_get_channel grpc_channelz_get_channel_import typedef grpc_call_error(*grpc_call_cancel_type)(grpc_call* call, void* reserved); extern grpc_call_cancel_type grpc_call_cancel_import; #define grpc_call_cancel grpc_call_cancel_import diff --git a/test/core/surface/public_headers_must_be_c89.c b/test/core/surface/public_headers_must_be_c89.c index bc9c2f2a13e..52a1b039980 100644 --- a/test/core/surface/public_headers_must_be_c89.c +++ b/test/core/surface/public_headers_must_be_c89.c @@ -108,7 +108,6 @@ int main(int argc, char **argv) { printf("%lx", (unsigned long) grpc_channel_destroy); printf("%lx", (unsigned long) grpc_channel_get_trace); printf("%lx", (unsigned long) grpc_channel_get_uuid); - printf("%lx", (unsigned long) grpc_channelz_get_channel); printf("%lx", (unsigned long) grpc_call_cancel); printf("%lx", (unsigned long) grpc_call_cancel_with_status); printf("%lx", (unsigned long) grpc_call_ref); diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json index 46508d47494..efe53d7da5c 100644 --- a/tools/run_tests/generated/sources_and_headers.json +++ b/tools/run_tests/generated/sources_and_headers.json @@ -3068,6 +3068,8 @@ "gpr_test_util", "grpc", "grpc++", + "grpc++_channelz_proto", + "grpc++_test", "grpc++_test_util", "grpc_test_util" ], From a8717043515fd1fdb661cb684fb952109a547660 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 5 Jun 2018 08:26:34 -0700 Subject: [PATCH 07/28] Make operations atomic --- src/core/lib/channel/channelz.cc | 28 ++++++++++++++++++++++------ src/core/lib/channel/channelz.h | 27 +++++++++++++++++++++------ 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index 5c3dc97e690..9e153a3f4f5 100644 --- a/src/core/lib/channel/channelz.cc +++ b/src/core/lib/channel/channelz.cc @@ -93,8 +93,6 @@ Channel::Channel(grpc_channel* channel, size_t channel_tracer_max_nodes) trace_.Init(channel_tracer_max_nodes); target_ = grpc_channel_get_target(channel_); channel_uuid_ = ChannelzRegistry::Register(this); - last_call_started_timestamp_ = - grpc_millis_to_timespec(ExecCtx::Get()->Now(), GPR_CLOCK_REALTIME); } Channel::~Channel() { @@ -103,10 +101,28 @@ Channel::~Channel() { ChannelzRegistry::Unregister(channel_uuid_); } +Channel::AtomicTimespec::AtomicTimespec() { + gpr_atm_no_barrier_store(&tv_sec_, (gpr_atm)0); + gpr_atm_no_barrier_store(&tv_nsec_, (gpr_atm)0); +} + +void Channel::AtomicTimespec::Update(gpr_timespec ts) { + gpr_atm_no_barrier_store(&tv_sec_, (gpr_atm)ts.tv_sec); + gpr_atm_no_barrier_store(&tv_nsec_, (gpr_atm)ts.tv_nsec); +} + +gpr_timespec Channel::AtomicTimespec::Get() { + gpr_timespec out; + out.clock_type = GPR_CLOCK_REALTIME; + out.tv_sec = static_cast(gpr_atm_no_barrier_load(&tv_sec_)); + out.tv_nsec = static_cast(gpr_atm_no_barrier_load(&tv_nsec_)); + return out; +} + void Channel::CallStarted() { - calls_started_++; - last_call_started_timestamp_ = - grpc_millis_to_timespec(ExecCtx::Get()->Now(), GPR_CLOCK_REALTIME); + gpr_atm_no_barrier_fetch_add(&calls_started_, (gpr_atm)1); + last_call_started_timestamp_.Update( + grpc_millis_to_timespec(ExecCtx::Get()->Now(), GPR_CLOCK_REALTIME)); } grpc_connectivity_state Channel::GetConnectivityState() { @@ -181,7 +197,7 @@ char* Channel::RenderJSON() { calls_failed_ ? calls_failed_ : -1); json_iterator = grpc_json_create_child( json_iterator, json, "lastCallStartedTimestamp", - fmt_time(last_call_started_timestamp_), GRPC_JSON_STRING, true); + fmt_time(last_call_started_timestamp_.Get()), GRPC_JSON_STRING, true); // render and return the over json object char* json_str = grpc_json_dump_to_string(top_level_json, 0); diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h index 5cfafc9e146..684402e08fc 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -39,8 +39,12 @@ class Channel : public RefCounted { ~Channel(); void CallStarted(); - void CallFailed() { calls_failed_++; } - void CallSucceeded() { calls_succeeded_++; } + void CallFailed() { + gpr_atm_no_barrier_fetch_add(&calls_failed_, (gpr_atm(1))); + } + void CallSucceeded() { + gpr_atm_no_barrier_fetch_add(&calls_succeeded_, (gpr_atm(1))); + } char* RenderJSON(); @@ -54,13 +58,24 @@ class Channel : public RefCounted { intptr_t channel_uuid() { return channel_uuid_; } private: + class AtomicTimespec { + public: + AtomicTimespec(); + void Update(gpr_timespec ts); + gpr_timespec Get(); + + private: + gpr_atm tv_sec_; + gpr_atm tv_nsec_; + }; + bool channel_destroyed_ = false; grpc_channel* channel_; const char* target_; - int64_t calls_started_ = 0; - int64_t calls_succeeded_ = 0; - int64_t calls_failed_ = 0; - gpr_timespec last_call_started_timestamp_; + gpr_atm calls_started_ = 0; + gpr_atm calls_succeeded_ = 0; + gpr_atm calls_failed_ = 0; + AtomicTimespec last_call_started_timestamp_; intptr_t channel_uuid_; ManualConstructor trace_; From c845ba66f30c44120f80098774652ba644ed7652 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 7 Jun 2018 10:14:55 -0700 Subject: [PATCH 08/28] Reviewer feedback --- src/core/lib/channel/channel_trace.cc | 12 +---- src/core/lib/channel/channel_trace.h | 13 ++--- src/core/lib/channel/channelz.cc | 26 ++-------- src/core/lib/channel/channelz.h | 27 +++++----- src/core/lib/surface/call.cc | 6 +-- src/core/lib/surface/channel.cc | 7 ++- test/core/channel/channel_trace_test.cc | 35 +++++++------ test/core/channel/channelz_test.cc | 68 ++++++++++++------------- 8 files changed, 83 insertions(+), 111 deletions(-) diff --git a/src/core/lib/channel/channel_trace.cc b/src/core/lib/channel/channel_trace.cc index c82e42d5459..4c29df42f42 100644 --- a/src/core/lib/channel/channel_trace.cc +++ b/src/core/lib/channel/channel_trace.cc @@ -121,11 +121,11 @@ void ChannelTrace::AddTraceEventReferencingChannel( void ChannelTrace::AddTraceEventReferencingSubchannel( Severity severity, grpc_slice data, - RefCountedPtr referenced_channel) { + RefCountedPtr referenced_subchannel) { if (max_list_size_ == 0) return; // tracing is disabled if max_events == 0 // create and fill up the new event AddTraceEventHelper(New(severity, data, - std::move(referenced_channel), + std::move(referenced_subchannel), ReferencedType::Subchannel)); } @@ -232,13 +232,5 @@ grpc_json* ChannelTrace::RenderJSON() const { return json; } -char* ChannelTrace::RenderTrace() const { - grpc_json* json = RenderJSON(); - if (json == nullptr) return nullptr; - char* json_str = grpc_json_dump_to_string(json, 0); - grpc_json_destroy(json); - return json_str; -} - } // namespace channelz } // namespace grpc_core diff --git a/src/core/lib/channel/channel_trace.h b/src/core/lib/channel/channel_trace.h index b1224bb96c4..17e95a32ee4 100644 --- a/src/core/lib/channel/channel_trace.h +++ b/src/core/lib/channel/channel_trace.h @@ -22,11 +22,13 @@ #include #include +// #include "src/core/lib/channel/channelz.h" #include "src/core/lib/gprpp/ref_counted.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/iomgr/error.h" #include "src/core/lib/json/json.h" + namespace grpc_core { namespace channelz { @@ -64,20 +66,15 @@ class ChannelTrace { // slice. void AddTraceEventReferencingChannel( Severity severity, grpc_slice data, - RefCountedPtr referenced_tracer); + RefCountedPtr referenced_channel); void AddTraceEventReferencingSubchannel( Severity severity, grpc_slice data, - RefCountedPtr referenced_tracer); + RefCountedPtr referenced_subchannel); // Creates and returns the raw grpc_json object, so a parent channelz // object may incorporate the json before rendering. grpc_json* RenderJSON() const; - // Returns the tracing data rendered as a grpc json string. The string - // is owned by the caller and must be freed. This is used for testing only - // so that we may unit test ChannelTrace in isolation. - char* RenderTrace() const; - private: // Types of objects that can be references by trace events. enum class ReferencedType { Channel, Subchannel }; @@ -87,7 +84,7 @@ class ChannelTrace { public: // Constructor for a TraceEvent that references a different channel. TraceEvent(Severity severity, grpc_slice data, - RefCountedPtr referenced_tracer, ReferencedType type); + RefCountedPtr referenced_channel, ReferencedType type); // Constructor for a TraceEvent that does not reverence a different // channel. diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index 9e153a3f4f5..41b3c4527ec 100644 --- a/src/core/lib/channel/channelz.cc +++ b/src/core/lib/channel/channelz.cc @@ -101,28 +101,9 @@ Channel::~Channel() { ChannelzRegistry::Unregister(channel_uuid_); } -Channel::AtomicTimespec::AtomicTimespec() { - gpr_atm_no_barrier_store(&tv_sec_, (gpr_atm)0); - gpr_atm_no_barrier_store(&tv_nsec_, (gpr_atm)0); -} - -void Channel::AtomicTimespec::Update(gpr_timespec ts) { - gpr_atm_no_barrier_store(&tv_sec_, (gpr_atm)ts.tv_sec); - gpr_atm_no_barrier_store(&tv_nsec_, (gpr_atm)ts.tv_nsec); -} - -gpr_timespec Channel::AtomicTimespec::Get() { - gpr_timespec out; - out.clock_type = GPR_CLOCK_REALTIME; - out.tv_sec = static_cast(gpr_atm_no_barrier_load(&tv_sec_)); - out.tv_nsec = static_cast(gpr_atm_no_barrier_load(&tv_nsec_)); - return out; -} - -void Channel::CallStarted() { +void Channel::RecordCallStarted() { gpr_atm_no_barrier_fetch_add(&calls_started_, (gpr_atm)1); - last_call_started_timestamp_.Update( - grpc_millis_to_timespec(ExecCtx::Get()->Now(), GPR_CLOCK_REALTIME)); + gpr_atm_no_barrier_store(&last_call_started_millis_, (gpr_atm)ExecCtx::Get()->Now()); } grpc_connectivity_state Channel::GetConnectivityState() { @@ -195,9 +176,10 @@ char* Channel::RenderJSON() { calls_succeeded_ ? calls_succeeded_ : -1); json_iterator = add_num_str(json, json_iterator, "callsFailed", calls_failed_ ? calls_failed_ : -1); + gpr_timespec ts = grpc_millis_to_timespec(last_call_started_millis_, GPR_CLOCK_REALTIME); json_iterator = grpc_json_create_child( json_iterator, json, "lastCallStartedTimestamp", - fmt_time(last_call_started_timestamp_.Get()), GRPC_JSON_STRING, true); + fmt_time(ts), GRPC_JSON_STRING, true); // render and return the over json object char* json_str = grpc_json_dump_to_string(top_level_json, 0); diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h index 684402e08fc..2d4c1a058a1 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -28,27 +28,32 @@ #include "src/core/lib/gprpp/ref_counted.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/iomgr/error.h" +#include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/json/json.h" namespace grpc_core { namespace channelz { +namespace testing { + class ChannelPeer; +} + class Channel : public RefCounted { public: Channel(grpc_channel* channel, size_t channel_tracer_max_nodes); ~Channel(); - void CallStarted(); - void CallFailed() { + void RecordCallStarted(); + void RecordCallFailed() { gpr_atm_no_barrier_fetch_add(&calls_failed_, (gpr_atm(1))); } - void CallSucceeded() { + void RecordCallSucceeded() { gpr_atm_no_barrier_fetch_add(&calls_succeeded_, (gpr_atm(1))); } char* RenderJSON(); - ChannelTrace* Trace() { return trace_.get(); } + ChannelTrace* trace() { return trace_.get(); } void set_channel_destroyed() { GPR_ASSERT(!channel_destroyed_); @@ -58,16 +63,8 @@ class Channel : public RefCounted { intptr_t channel_uuid() { return channel_uuid_; } private: - class AtomicTimespec { - public: - AtomicTimespec(); - void Update(gpr_timespec ts); - gpr_timespec Get(); - - private: - gpr_atm tv_sec_; - gpr_atm tv_nsec_; - }; + // testing peer friend. + friend class testing::ChannelPeer; bool channel_destroyed_ = false; grpc_channel* channel_; @@ -75,7 +72,7 @@ class Channel : public RefCounted { gpr_atm calls_started_ = 0; gpr_atm calls_succeeded_ = 0; gpr_atm calls_failed_ = 0; - AtomicTimespec last_call_started_timestamp_; + gpr_atm last_call_started_millis_; intptr_t channel_uuid_; ManualConstructor trace_; diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index 924d633cb27..e2212baa8c2 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -1084,11 +1084,11 @@ static void recv_trailing_filter(void* args, grpc_metadata_batch* b) { : nullptr; if (status_code == GRPC_STATUS_OK) { if (channelz_channel != nullptr) { - channelz_channel->CallSucceeded(); + channelz_channel->RecordCallSucceeded(); } } else { if (channelz_channel != nullptr) { - channelz_channel->CallFailed(); + channelz_channel->RecordCallFailed(); } error = grpc_error_set_int( GRPC_ERROR_CREATE_FROM_STATIC_STRING("Error received from peer"), @@ -1677,7 +1677,7 @@ static grpc_call_error call_start_batch(grpc_call* call, const grpc_op* ops, } grpc_core::channelz::Channel* channelz_channel = grpc_channel_get_channelz_channel(call->channel); - channelz_channel->CallStarted(); + channelz_channel->RecordCallStarted(); break; } case GRPC_OP_SEND_MESSAGE: { diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index be9869b08c4..5189c54b866 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -149,7 +149,7 @@ grpc_channel* grpc_channel_create_with_builder( channel->channelz_channel = grpc_core::MakeRefCounted( channel, channel_tracer_max_nodes); - channel->channelz_channel->Trace()->AddTraceEvent( + channel->channelz_channel->trace()->AddTraceEvent( grpc_core::channelz::ChannelTrace::Severity::Info, grpc_slice_from_static_string("Channel created")); return channel; @@ -187,7 +187,10 @@ static grpc_channel_args* build_channel_args( } char* grpc_channel_get_trace(grpc_channel* channel) { - return channel->channelz_channel->Trace()->RenderTrace(); + grpc_json* json = channel->channelz_channel->trace()->RenderJSON(); + char* json_str = grpc_json_dump_to_string(json, 0); + grpc_json_destroy(json); + return json_str; } char* grpc_channel_render_channelz(grpc_channel* channel) { diff --git a/test/core/channel/channel_trace_test.cc b/test/core/channel/channel_trace_test.cc index c8619be0074..ef69e631410 100644 --- a/test/core/channel/channel_trace_test.cc +++ b/test/core/channel/channel_trace_test.cc @@ -88,12 +88,15 @@ void AddSimpleTrace(ChannelTrace* tracer) { void ValidateChannelTrace(ChannelTrace* tracer, size_t expected_num_event_logged, size_t max_nodes) { if (!max_nodes) return; - char* json_str = tracer->RenderTrace(); + grpc_json* json = tracer->RenderJSON(); + EXPECT_NE(json, nullptr); + char* json_str = grpc_json_dump_to_string(json, 0); + grpc_json_destroy(json); grpc::testing::ValidateChannelTraceProtoJsonTranslation(json_str); - grpc_json* json = grpc_json_parse_string(json_str); - ValidateChannelTraceData(json, expected_num_event_logged, + grpc_json* parsed_json = grpc_json_parse_string(json_str); + ValidateChannelTraceData(parsed_json, expected_num_event_logged, GPR_MIN(expected_num_event_logged, max_nodes)); - grpc_json_destroy(json); + grpc_json_destroy(parsed_json); gpr_free(json_str); } @@ -159,14 +162,14 @@ TEST_P(ChannelTracerTest, ComplexTest) { ChannelTrace::Severity::Info, grpc_slice_from_static_string("subchannel one created"), sc1); ValidateChannelTrace(&tracer, 3, GetParam()); - AddSimpleTrace(sc1->Trace()); - AddSimpleTrace(sc1->Trace()); - AddSimpleTrace(sc1->Trace()); - ValidateChannelTrace(sc1->Trace(), 3, GetParam()); - AddSimpleTrace(sc1->Trace()); - AddSimpleTrace(sc1->Trace()); - AddSimpleTrace(sc1->Trace()); - ValidateChannelTrace(sc1->Trace(), 6, GetParam()); + AddSimpleTrace(sc1->trace()); + AddSimpleTrace(sc1->trace()); + AddSimpleTrace(sc1->trace()); + ValidateChannelTrace(sc1->trace(), 3, GetParam()); + AddSimpleTrace(sc1->trace()); + AddSimpleTrace(sc1->trace()); + AddSimpleTrace(sc1->trace()); + ValidateChannelTrace(sc1->trace(), 6, GetParam()); AddSimpleTrace(&tracer); AddSimpleTrace(&tracer); ValidateChannelTrace(&tracer, 5, GetParam()); @@ -206,20 +209,20 @@ TEST_P(ChannelTracerTest, TestNesting) { ChannelTrace::Severity::Info, grpc_slice_from_static_string("subchannel one created"), sc1); ValidateChannelTrace(&tracer, 3, GetParam()); - AddSimpleTrace(sc1->Trace()); + AddSimpleTrace(sc1->trace()); ChannelFixture channel2(GetParam()); RefCountedPtr conn1 = MakeRefCounted(channel2.channel(), GetParam()); // nesting one level deeper. - sc1->Trace()->AddTraceEventReferencingSubchannel( + sc1->trace()->AddTraceEventReferencingSubchannel( ChannelTrace::Severity::Info, grpc_slice_from_static_string("connection one created"), conn1); ValidateChannelTrace(&tracer, 3, GetParam()); - AddSimpleTrace(conn1->Trace()); + AddSimpleTrace(conn1->trace()); AddSimpleTrace(&tracer); AddSimpleTrace(&tracer); ValidateChannelTrace(&tracer, 5, GetParam()); - ValidateChannelTrace(conn1->Trace(), 1, GetParam()); + ValidateChannelTrace(conn1->trace(), 1, GetParam()); ChannelFixture channel3(GetParam()); RefCountedPtr sc2 = MakeRefCounted(channel3.channel(), GetParam()); diff --git a/test/core/channel/channelz_test.cc b/test/core/channel/channelz_test.cc index c4606426095..1c676e51cfe 100644 --- a/test/core/channel/channelz_test.cc +++ b/test/core/channel/channelz_test.cc @@ -42,6 +42,16 @@ namespace grpc_core { namespace channelz { namespace testing { + +// testing peer to access channel internals +class ChannelPeer { + public: + ChannelPeer (Channel* channel) : channel_(channel) {} + grpc_millis last_call_started_millis() { return (grpc_millis)gpr_atm_no_barrier_load(&channel_->last_call_started_millis_); } + private: + Channel* channel_; +}; + namespace { grpc_json* GetJsonChild(grpc_json* parent, const char* key) { @@ -100,15 +110,9 @@ void ValidateChannel(Channel* channel, validate_channel_data_args args) { gpr_free(json_str); } -char* GetLastCallStartedTimestamp(Channel* channel) { - char* json_str = channel->RenderJSON(); - grpc_json* json = grpc_json_parse_string(json_str); - grpc_json* data = GetJsonChild(json, "data"); - grpc_json* timestamp = GetJsonChild(data, "lastCallStartedTimestamp"); - char* ts_str = grpc_json_dump_to_string(timestamp, 0); - grpc_json_destroy(json); - gpr_free(json_str); - return ts_str; +grpc_millis GetLastCallStartedMillis(Channel* channel) { + ChannelPeer peer(channel); + return peer.last_call_started_millis(); } void ChannelzSleep(int64_t sleep_us) { @@ -134,16 +138,16 @@ TEST_P(ChannelzChannelTest, BasicChannelAPIFunctionality) { ChannelFixture channel(GetParam()); intptr_t uuid = grpc_channel_get_uuid(channel.channel()); Channel* channelz_channel = ChannelzRegistry::Get(uuid); - channelz_channel->CallStarted(); - channelz_channel->CallFailed(); - channelz_channel->CallSucceeded(); + channelz_channel->RecordCallStarted(); + channelz_channel->RecordCallFailed(); + channelz_channel->RecordCallSucceeded(); ValidateChannel(channelz_channel, {1, 1, 1}); - channelz_channel->CallStarted(); - channelz_channel->CallFailed(); - channelz_channel->CallSucceeded(); - channelz_channel->CallStarted(); - channelz_channel->CallFailed(); - channelz_channel->CallSucceeded(); + channelz_channel->RecordCallStarted(); + channelz_channel->RecordCallFailed(); + channelz_channel->RecordCallSucceeded(); + channelz_channel->RecordCallStarted(); + channelz_channel->RecordCallFailed(); + channelz_channel->RecordCallSucceeded(); ValidateChannel(channelz_channel, {3, 3, 3}); } @@ -154,34 +158,28 @@ TEST_P(ChannelzChannelTest, LastCallStartedTimestamp) { Channel* channelz_channel = ChannelzRegistry::Get(uuid); // start a call to set the last call started timestamp - channelz_channel->CallStarted(); - char* ts1 = GetLastCallStartedTimestamp(channelz_channel); + channelz_channel->RecordCallStarted(); + grpc_millis millis1 = GetLastCallStartedMillis(channelz_channel); // time gone by should not affect the timestamp ChannelzSleep(100); - char* ts2 = GetLastCallStartedTimestamp(channelz_channel); - EXPECT_STREQ(ts1, ts2); + grpc_millis millis2 = GetLastCallStartedMillis(channelz_channel); + EXPECT_EQ(millis1, millis2); // calls succeeded or failed should not affect the timestamp ChannelzSleep(100); - channelz_channel->CallFailed(); - channelz_channel->CallSucceeded(); - char* ts3 = GetLastCallStartedTimestamp(channelz_channel); - EXPECT_STREQ(ts1, ts3); + channelz_channel->RecordCallFailed(); + channelz_channel->RecordCallSucceeded(); + grpc_millis millis3 = GetLastCallStartedMillis(channelz_channel); + EXPECT_EQ(millis1, millis3); // another call started should affect the timestamp // sleep for extra long to avoid flakes (since we cache Now()) ChannelzSleep(5000); grpc_core::ExecCtx::Get()->InvalidateNow(); - channelz_channel->CallStarted(); - char* ts4 = GetLastCallStartedTimestamp(channelz_channel); - EXPECT_STRNE(ts1, ts4); - - // clean up - gpr_free(ts1); - gpr_free(ts2); - gpr_free(ts3); - gpr_free(ts4); + channelz_channel->RecordCallStarted(); + grpc_millis millis4 = GetLastCallStartedMillis(channelz_channel); + EXPECT_NE(millis1, millis4); } INSTANTIATE_TEST_CASE_P(ChannelzChannelTestSweep, ChannelzChannelTest, From d23739eda3081a274fad4f3a6af400fbca39dee1 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 7 Jun 2018 12:55:15 -0700 Subject: [PATCH 09/28] Reviewer feedback --- include/grpc/grpc.h | 4 --- src/core/lib/channel/channel_trace.h | 1 - src/core/lib/channel/channelz.cc | 23 +++++------------ src/core/lib/channel/channelz.h | 15 ++++++------ src/core/lib/surface/call.cc | 30 +++++++++++------------ src/core/lib/surface/channel.cc | 10 +------- src/core/lib/surface/channel.h | 3 +-- test/core/channel/channelz_test.cc | 7 +----- test/core/end2end/tests/simple_request.cc | 6 +++-- 9 files changed, 36 insertions(+), 63 deletions(-) diff --git a/include/grpc/grpc.h b/include/grpc/grpc.h index dd8a5d7d5f8..bb4a70f7b9f 100644 --- a/include/grpc/grpc.h +++ b/include/grpc/grpc.h @@ -286,10 +286,6 @@ 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. The caller - owns the returned string and is responsible for freeing it. */ -GRPCAPI char* grpc_channel_get_trace(grpc_channel* channel); - /** 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); diff --git a/src/core/lib/channel/channel_trace.h b/src/core/lib/channel/channel_trace.h index 17e95a32ee4..499cebd721b 100644 --- a/src/core/lib/channel/channel_trace.h +++ b/src/core/lib/channel/channel_trace.h @@ -22,7 +22,6 @@ #include #include -// #include "src/core/lib/channel/channelz.h" #include "src/core/lib/gprpp/ref_counted.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/iomgr/error.h" diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index 41b3c4527ec..06e2c0e13c9 100644 --- a/src/core/lib/channel/channelz.cc +++ b/src/core/lib/channel/channelz.cc @@ -42,9 +42,10 @@ namespace grpc_core { namespace channelz { -// TODO(ncteisen): more this functions to a loc where it can be used namespace { +// TODO(ncteisen): move this function to a common helper location. +// // returns an allocated string that represents tm according to RFC-3339, and, // more specifically, follows: // https://developers.google.com/protocol-buffers/docs/proto3#json @@ -89,14 +90,12 @@ grpc_json* add_num_str(grpc_json* parent, grpc_json* it, const char* name, } // namespace Channel::Channel(grpc_channel* channel, size_t channel_tracer_max_nodes) - : channel_(channel) { + : channel_(channel), target_(UniquePtr(grpc_channel_get_target(channel_))), channel_uuid_(ChannelzRegistry::Register(this)) { trace_.Init(channel_tracer_max_nodes); - target_ = grpc_channel_get_target(channel_); - channel_uuid_ = ChannelzRegistry::Register(this); + gpr_atm_no_barrier_store(&last_call_started_millis_, (gpr_atm)ExecCtx::Get()->Now()); } Channel::~Channel() { - gpr_free(const_cast(target_)); trace_.Destroy(); ChannelzRegistry::Unregister(channel_uuid_); } @@ -107,7 +106,7 @@ void Channel::RecordCallStarted() { } grpc_connectivity_state Channel::GetConnectivityState() { - if (channel_destroyed_) { + if (channel_ == nullptr) { return GRPC_CHANNEL_SHUTDOWN; } else { return grpc_channel_check_connectivity_state(channel_, false); @@ -119,25 +118,20 @@ char* Channel::RenderJSON() { grpc_json* top_level_json = grpc_json_create(GRPC_JSON_OBJECT); grpc_json* json = top_level_json; grpc_json* json_iterator = nullptr; - // create and fill the ref child json_iterator = grpc_json_create_child(json_iterator, json, "ref", nullptr, GRPC_JSON_OBJECT, false); json = json_iterator; json_iterator = nullptr; json_iterator = add_num_str(json, json_iterator, "channelId", channel_uuid_); - // reset json iterators to top level object json = top_level_json; json_iterator = nullptr; - // create and fill the data child. grpc_json* data = grpc_json_create_child(json_iterator, json, "data", nullptr, GRPC_JSON_OBJECT, false); - json = data; json_iterator = nullptr; - // create and fill the connectivity state child. grpc_connectivity_state connectivity_state = GetConnectivityState(); json_iterator = grpc_json_create_child(json_iterator, json, "state", nullptr, @@ -146,12 +140,10 @@ char* Channel::RenderJSON() { grpc_json_create_child(nullptr, json, "state", grpc_connectivity_state_name(connectivity_state), GRPC_JSON_STRING, false); - // reset the parent to be the data object. json = data; - json_iterator = grpc_json_create_child(json_iterator, json, "target", target_, + json_iterator = grpc_json_create_child(json_iterator, json, "target", target_.get(), GRPC_JSON_STRING, false); - // fill in the channel trace if applicable grpc_json* trace = trace_->RenderJSON(); if (trace != nullptr) { @@ -163,11 +155,9 @@ char* Channel::RenderJSON() { trace->key = "trace"; trace->owns_value = false; } - // reset the parent to be the data object. json = data; json_iterator = nullptr; - // We use -1 as sentinel values since proto default value for integers is // zero, and the confuses the parser into thinking the value weren't present json_iterator = add_num_str(json, json_iterator, "callsStarted", @@ -180,7 +170,6 @@ char* Channel::RenderJSON() { json_iterator = grpc_json_create_child( json_iterator, json, "lastCallStartedTimestamp", fmt_time(ts), GRPC_JSON_STRING, true); - // render and return the over json object char* json_str = grpc_json_dump_to_string(top_level_json, 0); grpc_json_destroy(top_level_json); diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h index 2d4c1a058a1..863447a5a46 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -56,8 +56,8 @@ class Channel : public RefCounted { ChannelTrace* trace() { return trace_.get(); } void set_channel_destroyed() { - GPR_ASSERT(!channel_destroyed_); - channel_destroyed_ = true; + GPR_ASSERT(channel_ != nullptr); + channel_ = nullptr; } intptr_t channel_uuid() { return channel_uuid_; } @@ -66,17 +66,18 @@ class Channel : public RefCounted { // testing peer friend. friend class testing::ChannelPeer; - bool channel_destroyed_ = false; + // helper for getting connectivity state. + grpc_connectivity_state GetConnectivityState(); + + // Not owned. Will be set to nullptr when the channel is destroyed. grpc_channel* channel_; - const char* target_; + UniquePtr target_; gpr_atm calls_started_ = 0; gpr_atm calls_succeeded_ = 0; gpr_atm calls_failed_ = 0; gpr_atm last_call_started_millis_; - intptr_t channel_uuid_; + const intptr_t channel_uuid_; ManualConstructor trace_; - - grpc_connectivity_state GetConnectivityState(); }; } // namespace channelz diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index e2212baa8c2..2e7d9360a98 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -478,6 +478,10 @@ grpc_error* grpc_call_create(const grpc_call_create_args* args, &call->pollent); } + grpc_core::channelz::Channel* channelz_channel = + grpc_channel_get_channelz_channel_node(call->channel); + channelz_channel->RecordCallStarted(); + grpc_slice_unref_internal(path); return error; @@ -1078,18 +1082,7 @@ static void recv_trailing_filter(void* args, grpc_metadata_batch* b) { grpc_status_code status_code = grpc_get_status_code_from_metadata(b->idx.named.grpc_status->md); grpc_error* error = GRPC_ERROR_NONE; - grpc_core::channelz::Channel* channelz_channel = - call->channel != nullptr - ? grpc_channel_get_channelz_channel(call->channel) - : nullptr; - if (status_code == GRPC_STATUS_OK) { - if (channelz_channel != nullptr) { - channelz_channel->RecordCallSucceeded(); - } - } else { - if (channelz_channel != nullptr) { - channelz_channel->RecordCallFailed(); - } + if (status_code != GRPC_STATUS_OK) { error = grpc_error_set_int( GRPC_ERROR_CREATE_FROM_STATIC_STRING("Error received from peer"), GRPC_ERROR_INT_GRPC_STATUS, static_cast(status_code)); @@ -1268,6 +1261,16 @@ static void post_batch_completion(batch_control* bctl) { call->final_op.server.cancelled, nullptr, nullptr); } + if (call->channel != nullptr) { + grpc_core::channelz::Channel* channelz_channel = grpc_channel_get_channelz_channel_node(call->channel); + if (*call->final_op.client.status != GRPC_STATUS_OK) { + channelz_channel->RecordCallFailed(); + } else { + channelz_channel->RecordCallSucceeded(); + } + } + + GRPC_ERROR_UNREF(error); error = GRPC_ERROR_NONE; } @@ -1675,9 +1678,6 @@ static grpc_call_error call_start_batch(grpc_call* call, const grpc_op* ops, stream_op_payload->send_initial_metadata.peer_string = &call->peer_string; } - grpc_core::channelz::Channel* channelz_channel = - grpc_channel_get_channelz_channel(call->channel); - channelz_channel->RecordCallStarted(); break; } case GRPC_OP_SEND_MESSAGE: { diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index 5189c54b866..c95af0f2bdf 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -186,18 +186,11 @@ static grpc_channel_args* build_channel_args( return grpc_channel_args_copy_and_add(input_args, new_args, num_new_args); } -char* grpc_channel_get_trace(grpc_channel* channel) { - grpc_json* json = channel->channelz_channel->trace()->RenderJSON(); - char* json_str = grpc_json_dump_to_string(json, 0); - grpc_json_destroy(json); - return json_str; -} - char* grpc_channel_render_channelz(grpc_channel* channel) { return channel->channelz_channel->RenderJSON(); } -grpc_core::channelz::Channel* grpc_channel_get_channelz_channel( +grpc_core::channelz::Channel* grpc_channel_get_channelz_channel_node( grpc_channel* channel) { return channel->channelz_channel.get(); } @@ -417,7 +410,6 @@ static void destroy_channel(void* arg, grpc_error* error) { GRPC_MDELEM_UNREF(rc->authority); gpr_free(rc); } - channel->channelz_channel.reset(); gpr_mu_destroy(&channel->registered_call_mu); gpr_free(channel->target); gpr_free(channel); diff --git a/src/core/lib/surface/channel.h b/src/core/lib/surface/channel.h index 52290f05f7a..bfeec9ec9be 100644 --- a/src/core/lib/surface/channel.h +++ b/src/core/lib/surface/channel.h @@ -51,9 +51,8 @@ grpc_call* grpc_channel_create_pollset_set_call( /** Get a (borrowed) pointer to this channels underlying channel stack */ grpc_channel_stack* grpc_channel_get_channel_stack(grpc_channel* channel); -grpc_core::channelz::Channel* grpc_channel_get_channelz_channel( +grpc_core::channelz::Channel* grpc_channel_get_channelz_channel_node( grpc_channel* channel); -char* grpc_channel_render_channelz(grpc_channel* channel); /** Get a grpc_mdelem of grpc-status: X where X is the numeric value of status_code. diff --git a/test/core/channel/channelz_test.cc b/test/core/channel/channelz_test.cc index 1c676e51cfe..537f1c5b189 100644 --- a/test/core/channel/channelz_test.cc +++ b/test/core/channel/channelz_test.cc @@ -151,32 +151,27 @@ TEST_P(ChannelzChannelTest, BasicChannelAPIFunctionality) { ValidateChannel(channelz_channel, {3, 3, 3}); } -TEST_P(ChannelzChannelTest, LastCallStartedTimestamp) { +TEST_P(ChannelzChannelTest, LastCallStartedMillis) { grpc_core::ExecCtx exec_ctx; ChannelFixture channel(GetParam()); intptr_t uuid = grpc_channel_get_uuid(channel.channel()); Channel* channelz_channel = ChannelzRegistry::Get(uuid); - // start a call to set the last call started timestamp channelz_channel->RecordCallStarted(); grpc_millis millis1 = GetLastCallStartedMillis(channelz_channel); - // time gone by should not affect the timestamp ChannelzSleep(100); grpc_millis millis2 = GetLastCallStartedMillis(channelz_channel); EXPECT_EQ(millis1, millis2); - // calls succeeded or failed should not affect the timestamp ChannelzSleep(100); channelz_channel->RecordCallFailed(); channelz_channel->RecordCallSucceeded(); grpc_millis millis3 = GetLastCallStartedMillis(channelz_channel); EXPECT_EQ(millis1, millis3); - // another call started should affect the timestamp // sleep for extra long to avoid flakes (since we cache Now()) ChannelzSleep(5000); - grpc_core::ExecCtx::Get()->InvalidateNow(); channelz_channel->RecordCallStarted(); grpc_millis millis4 = GetLastCallStartedMillis(channelz_channel); EXPECT_NE(millis1, millis4); diff --git a/test/core/end2end/tests/simple_request.cc b/test/core/end2end/tests/simple_request.cc index a9341edff96..57c11616412 100644 --- a/test/core/end2end/tests/simple_request.cc +++ b/test/core/end2end/tests/simple_request.cc @@ -258,7 +258,8 @@ static void test_invoke_simple_request(grpc_end2end_test_config config) { // The following is a quick sanity check on channelz functionality. It // ensures that core properly tracked the one call that occurred in this // simple end2end test. - char* json = grpc_channel_render_channelz(f.client); + grpc_core::channelz::Channel* channelz_channel = grpc_channel_get_channelz_channel_node(f.client); + char* json = channelz_channel->RenderJSON(); GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"1\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"1\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"-1\"")); @@ -279,7 +280,8 @@ static void test_invoke_10_simple_requests(grpc_end2end_test_config config) { // The following is a quick sanity check on channelz functionality. It // ensures that core properly tracked the ten calls that occurred. - char* json = grpc_channel_render_channelz(f.client); + grpc_core::channelz::Channel* channelz_channel = grpc_channel_get_channelz_channel_node(f.client); + char* json = channelz_channel->RenderJSON(); GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"10\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"10\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"-1\"")); From ea424b88fa8ed07df27cb51d07b8dafc9110a9ec Mon Sep 17 00:00:00 2001 From: Noah Eisen Date: Thu, 7 Jun 2018 13:02:48 -0700 Subject: [PATCH 10/28] clang fmt --- src/core/lib/channel/channel_trace.h | 1 - src/core/lib/channel/channelz.cc | 23 ++++++++++++++--------- src/core/lib/channel/channelz.h | 2 +- src/core/lib/surface/call.cc | 4 ++-- test/core/channel/channelz_test.cc | 8 ++++++-- test/core/end2end/tests/simple_request.cc | 6 ++++-- 6 files changed, 27 insertions(+), 17 deletions(-) diff --git a/src/core/lib/channel/channel_trace.h b/src/core/lib/channel/channel_trace.h index 499cebd721b..f6021c32243 100644 --- a/src/core/lib/channel/channel_trace.h +++ b/src/core/lib/channel/channel_trace.h @@ -27,7 +27,6 @@ #include "src/core/lib/iomgr/error.h" #include "src/core/lib/json/json.h" - namespace grpc_core { namespace channelz { diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index 06e2c0e13c9..17c3fc35d99 100644 --- a/src/core/lib/channel/channelz.cc +++ b/src/core/lib/channel/channelz.cc @@ -90,9 +90,12 @@ grpc_json* add_num_str(grpc_json* parent, grpc_json* it, const char* name, } // namespace Channel::Channel(grpc_channel* channel, size_t channel_tracer_max_nodes) - : channel_(channel), target_(UniquePtr(grpc_channel_get_target(channel_))), channel_uuid_(ChannelzRegistry::Register(this)) { + : channel_(channel), + target_(UniquePtr(grpc_channel_get_target(channel_))), + channel_uuid_(ChannelzRegistry::Register(this)) { trace_.Init(channel_tracer_max_nodes); - gpr_atm_no_barrier_store(&last_call_started_millis_, (gpr_atm)ExecCtx::Get()->Now()); + gpr_atm_no_barrier_store(&last_call_started_millis_, + (gpr_atm)ExecCtx::Get()->Now()); } Channel::~Channel() { @@ -102,7 +105,8 @@ Channel::~Channel() { void Channel::RecordCallStarted() { 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()); + gpr_atm_no_barrier_store(&last_call_started_millis_, + (gpr_atm)ExecCtx::Get()->Now()); } grpc_connectivity_state Channel::GetConnectivityState() { @@ -142,8 +146,8 @@ char* Channel::RenderJSON() { GRPC_JSON_STRING, false); // reset the parent to be the data object. json = data; - json_iterator = grpc_json_create_child(json_iterator, json, "target", target_.get(), - GRPC_JSON_STRING, false); + json_iterator = grpc_json_create_child( + json_iterator, json, "target", target_.get(), GRPC_JSON_STRING, false); // fill in the channel trace if applicable grpc_json* trace = trace_->RenderJSON(); if (trace != nullptr) { @@ -166,10 +170,11 @@ char* Channel::RenderJSON() { calls_succeeded_ ? calls_succeeded_ : -1); json_iterator = add_num_str(json, json_iterator, "callsFailed", calls_failed_ ? calls_failed_ : -1); - gpr_timespec ts = grpc_millis_to_timespec(last_call_started_millis_, GPR_CLOCK_REALTIME); - json_iterator = grpc_json_create_child( - json_iterator, json, "lastCallStartedTimestamp", - fmt_time(ts), GRPC_JSON_STRING, true); + gpr_timespec ts = + grpc_millis_to_timespec(last_call_started_millis_, GPR_CLOCK_REALTIME); + json_iterator = + grpc_json_create_child(json_iterator, json, "lastCallStartedTimestamp", + fmt_time(ts), GRPC_JSON_STRING, true); // render and return the over json object char* json_str = grpc_json_dump_to_string(top_level_json, 0); grpc_json_destroy(top_level_json); diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h index 863447a5a46..68b7c8d26b9 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -35,7 +35,7 @@ namespace grpc_core { namespace channelz { namespace testing { - class ChannelPeer; +class ChannelPeer; } class Channel : public RefCounted { diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index 2e7d9360a98..807ab7ae436 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -1262,7 +1262,8 @@ static void post_batch_completion(batch_control* bctl) { } if (call->channel != nullptr) { - grpc_core::channelz::Channel* channelz_channel = grpc_channel_get_channelz_channel_node(call->channel); + grpc_core::channelz::Channel* channelz_channel = + grpc_channel_get_channelz_channel_node(call->channel); if (*call->final_op.client.status != GRPC_STATUS_OK) { channelz_channel->RecordCallFailed(); } else { @@ -1270,7 +1271,6 @@ static void post_batch_completion(batch_control* bctl) { } } - GRPC_ERROR_UNREF(error); error = GRPC_ERROR_NONE; } diff --git a/test/core/channel/channelz_test.cc b/test/core/channel/channelz_test.cc index 537f1c5b189..95cf5f83ea3 100644 --- a/test/core/channel/channelz_test.cc +++ b/test/core/channel/channelz_test.cc @@ -46,8 +46,12 @@ namespace testing { // testing peer to access channel internals class ChannelPeer { public: - ChannelPeer (Channel* channel) : channel_(channel) {} - grpc_millis last_call_started_millis() { return (grpc_millis)gpr_atm_no_barrier_load(&channel_->last_call_started_millis_); } + ChannelPeer(Channel* channel) : channel_(channel) {} + grpc_millis last_call_started_millis() { + return (grpc_millis)gpr_atm_no_barrier_load( + &channel_->last_call_started_millis_); + } + private: Channel* channel_; }; diff --git a/test/core/end2end/tests/simple_request.cc b/test/core/end2end/tests/simple_request.cc index 57c11616412..d24f7bc6206 100644 --- a/test/core/end2end/tests/simple_request.cc +++ b/test/core/end2end/tests/simple_request.cc @@ -258,7 +258,8 @@ static void test_invoke_simple_request(grpc_end2end_test_config config) { // The following is a quick sanity check on channelz functionality. It // ensures that core properly tracked the one call that occurred in this // simple end2end test. - grpc_core::channelz::Channel* channelz_channel = grpc_channel_get_channelz_channel_node(f.client); + grpc_core::channelz::Channel* channelz_channel = + grpc_channel_get_channelz_channel_node(f.client); char* json = channelz_channel->RenderJSON(); GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"1\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"1\"")); @@ -280,7 +281,8 @@ static void test_invoke_10_simple_requests(grpc_end2end_test_config config) { // The following is a quick sanity check on channelz functionality. It // ensures that core properly tracked the ten calls that occurred. - grpc_core::channelz::Channel* channelz_channel = grpc_channel_get_channelz_channel_node(f.client); + grpc_core::channelz::Channel* channelz_channel = + grpc_channel_get_channelz_channel_node(f.client); char* json = channelz_channel->RenderJSON(); GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"10\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"10\"")); From 70f7558a18fd92320813b6ba4107b94c83cc7b0b Mon Sep 17 00:00:00 2001 From: Noah Eisen Date: Thu, 7 Jun 2018 13:03:32 -0700 Subject: [PATCH 11/28] re generate project --- grpc.def | 1 - src/ruby/ext/grpc/rb_grpc_imports.generated.c | 2 -- src/ruby/ext/grpc/rb_grpc_imports.generated.h | 3 --- test/core/surface/public_headers_must_be_c89.c | 1 - 4 files changed, 7 deletions(-) diff --git a/grpc.def b/grpc.def index 6a91214e8c3..fc1d02a0352 100644 --- a/grpc.def +++ b/grpc.def @@ -45,7 +45,6 @@ EXPORTS grpc_insecure_channel_create grpc_lame_client_channel_create grpc_channel_destroy - grpc_channel_get_trace grpc_channel_get_uuid grpc_call_cancel grpc_call_cancel_with_status diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.c b/src/ruby/ext/grpc/rb_grpc_imports.generated.c index 02f84c0b962..8b67a7ce2d2 100644 --- a/src/ruby/ext/grpc/rb_grpc_imports.generated.c +++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.c @@ -68,7 +68,6 @@ grpc_channel_get_info_type grpc_channel_get_info_import; grpc_insecure_channel_create_type grpc_insecure_channel_create_import; grpc_lame_client_channel_create_type grpc_lame_client_channel_create_import; grpc_channel_destroy_type grpc_channel_destroy_import; -grpc_channel_get_trace_type grpc_channel_get_trace_import; grpc_channel_get_uuid_type grpc_channel_get_uuid_import; grpc_call_cancel_type grpc_call_cancel_import; grpc_call_cancel_with_status_type grpc_call_cancel_with_status_import; @@ -316,7 +315,6 @@ void grpc_rb_load_imports(HMODULE library) { grpc_insecure_channel_create_import = (grpc_insecure_channel_create_type) GetProcAddress(library, "grpc_insecure_channel_create"); grpc_lame_client_channel_create_import = (grpc_lame_client_channel_create_type) GetProcAddress(library, "grpc_lame_client_channel_create"); grpc_channel_destroy_import = (grpc_channel_destroy_type) GetProcAddress(library, "grpc_channel_destroy"); - grpc_channel_get_trace_import = (grpc_channel_get_trace_type) GetProcAddress(library, "grpc_channel_get_trace"); grpc_channel_get_uuid_import = (grpc_channel_get_uuid_type) GetProcAddress(library, "grpc_channel_get_uuid"); grpc_call_cancel_import = (grpc_call_cancel_type) GetProcAddress(library, "grpc_call_cancel"); grpc_call_cancel_with_status_import = (grpc_call_cancel_with_status_type) GetProcAddress(library, "grpc_call_cancel_with_status"); diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.h b/src/ruby/ext/grpc/rb_grpc_imports.generated.h index b2186a69aa9..c7919871c86 100644 --- a/src/ruby/ext/grpc/rb_grpc_imports.generated.h +++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.h @@ -179,9 +179,6 @@ extern grpc_lame_client_channel_create_type grpc_lame_client_channel_create_impo typedef void(*grpc_channel_destroy_type)(grpc_channel* channel); extern grpc_channel_destroy_type grpc_channel_destroy_import; #define grpc_channel_destroy grpc_channel_destroy_import -typedef char*(*grpc_channel_get_trace_type)(grpc_channel* channel); -extern grpc_channel_get_trace_type grpc_channel_get_trace_import; -#define grpc_channel_get_trace grpc_channel_get_trace_import typedef intptr_t(*grpc_channel_get_uuid_type)(grpc_channel* channel); extern grpc_channel_get_uuid_type grpc_channel_get_uuid_import; #define grpc_channel_get_uuid grpc_channel_get_uuid_import diff --git a/test/core/surface/public_headers_must_be_c89.c b/test/core/surface/public_headers_must_be_c89.c index 52a1b039980..96b25ebdaff 100644 --- a/test/core/surface/public_headers_must_be_c89.c +++ b/test/core/surface/public_headers_must_be_c89.c @@ -106,7 +106,6 @@ int main(int argc, char **argv) { printf("%lx", (unsigned long) grpc_insecure_channel_create); printf("%lx", (unsigned long) grpc_lame_client_channel_create); printf("%lx", (unsigned long) grpc_channel_destroy); - printf("%lx", (unsigned long) grpc_channel_get_trace); printf("%lx", (unsigned long) grpc_channel_get_uuid); printf("%lx", (unsigned long) grpc_call_cancel); printf("%lx", (unsigned long) grpc_call_cancel_with_status); From 7e7edc95b348b30b508cc0da9b09dc6495ce62ef Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 7 Jun 2018 17:25:44 -0700 Subject: [PATCH 12/28] reviewer feedback: --- src/core/lib/surface/call.cc | 17 +++++++---------- src/core/lib/surface/channel.cc | 2 +- src/core/lib/surface/channel.h | 2 +- test/core/end2end/tests/simple_request.cc | 4 ++-- 4 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index 807ab7ae436..65071672f27 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -479,7 +479,7 @@ grpc_error* grpc_call_create(const grpc_call_create_args* args, } grpc_core::channelz::Channel* channelz_channel = - grpc_channel_get_channelz_channel_node(call->channel); + grpc_channel_get_channelz_node(call->channel); channelz_channel->RecordCallStarted(); grpc_slice_unref_internal(path); @@ -525,7 +525,6 @@ static void release_call(void* call, grpc_error* error) { GRPC_CHANNEL_INTERNAL_UNREF(channel, "call"); } -static void set_status_value_directly(grpc_status_code status, void* dest); static void destroy_call(void* call, grpc_error* error) { GPR_TIMER_SCOPE("destroy_call", 0); size_t i; @@ -1261,14 +1260,12 @@ static void post_batch_completion(batch_control* bctl) { call->final_op.server.cancelled, nullptr, nullptr); } - if (call->channel != nullptr) { - grpc_core::channelz::Channel* channelz_channel = - grpc_channel_get_channelz_channel_node(call->channel); - if (*call->final_op.client.status != GRPC_STATUS_OK) { - channelz_channel->RecordCallFailed(); - } else { - channelz_channel->RecordCallSucceeded(); - } + grpc_core::channelz::Channel* 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(); } GRPC_ERROR_UNREF(error); diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index c95af0f2bdf..d2b52dad2fd 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -190,7 +190,7 @@ char* grpc_channel_render_channelz(grpc_channel* channel) { return channel->channelz_channel->RenderJSON(); } -grpc_core::channelz::Channel* grpc_channel_get_channelz_channel_node( +grpc_core::channelz::Channel* grpc_channel_get_channelz_node( grpc_channel* channel) { return channel->channelz_channel.get(); } diff --git a/src/core/lib/surface/channel.h b/src/core/lib/surface/channel.h index bfeec9ec9be..6d2e1931e68 100644 --- a/src/core/lib/surface/channel.h +++ b/src/core/lib/surface/channel.h @@ -51,7 +51,7 @@ grpc_call* grpc_channel_create_pollset_set_call( /** Get a (borrowed) pointer to this channels underlying channel stack */ grpc_channel_stack* grpc_channel_get_channel_stack(grpc_channel* channel); -grpc_core::channelz::Channel* grpc_channel_get_channelz_channel_node( +grpc_core::channelz::Channel* grpc_channel_get_channelz_node( grpc_channel* channel); /** Get a grpc_mdelem of grpc-status: X where X is the numeric value of diff --git a/test/core/end2end/tests/simple_request.cc b/test/core/end2end/tests/simple_request.cc index d24f7bc6206..16fa4906085 100644 --- a/test/core/end2end/tests/simple_request.cc +++ b/test/core/end2end/tests/simple_request.cc @@ -259,7 +259,7 @@ static void test_invoke_simple_request(grpc_end2end_test_config config) { // ensures that core properly tracked the one call that occurred in this // simple end2end test. grpc_core::channelz::Channel* channelz_channel = - grpc_channel_get_channelz_channel_node(f.client); + grpc_channel_get_channelz_node(f.client); char* json = channelz_channel->RenderJSON(); GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"1\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"1\"")); @@ -282,7 +282,7 @@ static void test_invoke_10_simple_requests(grpc_end2end_test_config config) { // The following is a quick sanity check on channelz functionality. It // ensures that core properly tracked the ten calls that occurred. grpc_core::channelz::Channel* channelz_channel = - grpc_channel_get_channelz_channel_node(f.client); + grpc_channel_get_channelz_node(f.client); char* json = channelz_channel->RenderJSON(); GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"10\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"10\"")); From 9b488f788b2575d737558425ad98279abac9f844 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 7 Jun 2018 18:38:28 -0700 Subject: [PATCH 13/28] Review feedback --- src/core/lib/channel/channelz.cc | 6 +++--- test/core/channel/channelz_test.cc | 15 +++++++++++---- test/core/end2end/tests/simple_request.cc | 4 ++-- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index 17c3fc35d99..1d2be00f1c4 100644 --- a/src/core/lib/channel/channelz.cc +++ b/src/core/lib/channel/channelz.cc @@ -165,11 +165,11 @@ char* Channel::RenderJSON() { // We use -1 as sentinel values since proto default value for integers is // zero, and the confuses the parser into thinking the value weren't present json_iterator = add_num_str(json, json_iterator, "callsStarted", - calls_started_ ? calls_started_ : -1); + calls_started_); json_iterator = add_num_str(json, json_iterator, "callsSucceeded", - calls_succeeded_ ? calls_succeeded_ : -1); + calls_succeeded_); json_iterator = add_num_str(json, json_iterator, "callsFailed", - calls_failed_ ? calls_failed_ : -1); + calls_failed_); gpr_timespec ts = grpc_millis_to_timespec(last_call_started_millis_, GPR_CLOCK_REALTIME); json_iterator = diff --git a/test/core/channel/channelz_test.cc b/test/core/channel/channelz_test.cc index 95cf5f83ea3..df97d34fd0d 100644 --- a/test/core/channel/channelz_test.cc +++ b/test/core/channel/channelz_test.cc @@ -101,9 +101,7 @@ void ValidateChildInteger(grpc_json* json, int64_t expect, const char* key) { EXPECT_EQ(gotten_number, expect); } -void ValidateChannel(Channel* channel, validate_channel_data_args args) { - char* json_str = channel->RenderJSON(); - grpc::testing::ValidateChannelProtoJsonTranslation(json_str); +void ValidateCounters(char* json_str, validate_channel_data_args args) { grpc_json* json = grpc_json_parse_string(json_str); EXPECT_NE(json, nullptr); grpc_json* data = GetJsonChild(json, "data"); @@ -111,7 +109,14 @@ void ValidateChannel(Channel* channel, validate_channel_data_args args) { ValidateChildInteger(data, args.calls_failed, "callsFailed"); ValidateChildInteger(data, args.calls_succeeded, "callsSucceeded"); grpc_json_destroy(json); +} + +void ValidateChannel(Channel* channel, validate_channel_data_args args) { + char* json_str = channel->RenderJSON(); + grpc::testing::ValidateChannelProtoJsonTranslation(json_str); + ValidateCounters(json_str, args); gpr_free(json_str); + } grpc_millis GetLastCallStartedMillis(Channel* channel) { @@ -134,7 +139,9 @@ TEST_P(ChannelzChannelTest, BasicChannel) { ChannelFixture channel(GetParam()); intptr_t uuid = grpc_channel_get_uuid(channel.channel()); Channel* channelz_channel = ChannelzRegistry::Get(uuid); - ValidateChannel(channelz_channel, {-1, -1, -1}); + char* json_str = channelz_channel->RenderJSON(); + ValidateCounters(json_str, {0, 0, 0}); + gpr_free(json_str); } TEST_P(ChannelzChannelTest, BasicChannelAPIFunctionality) { diff --git a/test/core/end2end/tests/simple_request.cc b/test/core/end2end/tests/simple_request.cc index 16fa4906085..8082bf93d54 100644 --- a/test/core/end2end/tests/simple_request.cc +++ b/test/core/end2end/tests/simple_request.cc @@ -263,7 +263,7 @@ static void test_invoke_simple_request(grpc_end2end_test_config config) { char* json = channelz_channel->RenderJSON(); GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"1\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"1\"")); - GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"-1\"")); + GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"0\"")); gpr_free(json); end_test(&f); @@ -286,7 +286,7 @@ static void test_invoke_10_simple_requests(grpc_end2end_test_config config) { char* json = channelz_channel->RenderJSON(); GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"10\"")); GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"10\"")); - GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"-1\"")); + GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"0\"")); gpr_free(json); end_test(&f); config.tear_down_data(&f); From b8a52e0cf3e271f80b32f16783c76c065f5c2167 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 7 Jun 2018 18:58:03 -0700 Subject: [PATCH 14/28] Add new end2end test for channelz --- CMakeLists.txt | 2 + Makefile | 2 + gRPC-Core.podspec | 1 + grpc.gyp | 2 + src/core/lib/channel/channelz.cc | 12 +- test/core/channel/channelz_test.cc | 1 - test/core/end2end/end2end_nosec_tests.cc | 8 + test/core/end2end/end2end_tests.cc | 8 + test/core/end2end/gen_build_yaml.py | 1 + test/core/end2end/generate_tests.bzl | 1 + test/core/end2end/tests/channelz.cc | 263 +++++ test/core/end2end/tests/simple_request.cc | 24 - .../generated/sources_and_headers.json | 2 + tools/run_tests/generated/tests.json | 927 ++++++++++++++++-- 14 files changed, 1154 insertions(+), 100 deletions(-) create mode 100644 test/core/end2end/tests/channelz.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index 29352da5f6c..de3cb55fd44 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -5281,6 +5281,7 @@ add_library(end2end_tests test/core/end2end/tests/cancel_before_invoke.cc test/core/end2end/tests/cancel_in_a_vacuum.cc test/core/end2end/tests/cancel_with_status.cc + test/core/end2end/tests/channelz.cc test/core/end2end/tests/compressed_payload.cc test/core/end2end/tests/connectivity.cc test/core/end2end/tests/default_host.cc @@ -5399,6 +5400,7 @@ add_library(end2end_nosec_tests test/core/end2end/tests/cancel_before_invoke.cc test/core/end2end/tests/cancel_in_a_vacuum.cc test/core/end2end/tests/cancel_with_status.cc + test/core/end2end/tests/channelz.cc test/core/end2end/tests/compressed_payload.cc test/core/end2end/tests/connectivity.cc test/core/end2end/tests/default_host.cc diff --git a/Makefile b/Makefile index 7d56686fe28..f68ea71df8f 100644 --- a/Makefile +++ b/Makefile @@ -9979,6 +9979,7 @@ LIBEND2END_TESTS_SRC = \ test/core/end2end/tests/cancel_before_invoke.cc \ test/core/end2end/tests/cancel_in_a_vacuum.cc \ test/core/end2end/tests/cancel_with_status.cc \ + test/core/end2end/tests/channelz.cc \ test/core/end2end/tests/compressed_payload.cc \ test/core/end2end/tests/connectivity.cc \ test/core/end2end/tests/default_host.cc \ @@ -10095,6 +10096,7 @@ LIBEND2END_NOSEC_TESTS_SRC = \ test/core/end2end/tests/cancel_before_invoke.cc \ test/core/end2end/tests/cancel_in_a_vacuum.cc \ test/core/end2end/tests/cancel_with_status.cc \ + test/core/end2end/tests/channelz.cc \ test/core/end2end/tests/compressed_payload.cc \ test/core/end2end/tests/connectivity.cc \ test/core/end2end/tests/default_host.cc \ diff --git a/gRPC-Core.podspec b/gRPC-Core.podspec index a10cfb5739f..2a9ad86f03c 100644 --- a/gRPC-Core.podspec +++ b/gRPC-Core.podspec @@ -1178,6 +1178,7 @@ Pod::Spec.new do |s| 'test/core/end2end/tests/cancel_before_invoke.cc', 'test/core/end2end/tests/cancel_in_a_vacuum.cc', 'test/core/end2end/tests/cancel_with_status.cc', + 'test/core/end2end/tests/channelz.cc', 'test/core/end2end/tests/compressed_payload.cc', 'test/core/end2end/tests/connectivity.cc', 'test/core/end2end/tests/default_host.cc', diff --git a/grpc.gyp b/grpc.gyp index a3ea8674fc0..6f259b215a9 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -2611,6 +2611,7 @@ 'test/core/end2end/tests/cancel_before_invoke.cc', 'test/core/end2end/tests/cancel_in_a_vacuum.cc', 'test/core/end2end/tests/cancel_with_status.cc', + 'test/core/end2end/tests/channelz.cc', 'test/core/end2end/tests/compressed_payload.cc', 'test/core/end2end/tests/connectivity.cc', 'test/core/end2end/tests/default_host.cc', @@ -2701,6 +2702,7 @@ 'test/core/end2end/tests/cancel_before_invoke.cc', 'test/core/end2end/tests/cancel_in_a_vacuum.cc', 'test/core/end2end/tests/cancel_with_status.cc', + 'test/core/end2end/tests/channelz.cc', 'test/core/end2end/tests/compressed_payload.cc', 'test/core/end2end/tests/connectivity.cc', 'test/core/end2end/tests/default_host.cc', diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index 1d2be00f1c4..42b169dffd1 100644 --- a/src/core/lib/channel/channelz.cc +++ b/src/core/lib/channel/channelz.cc @@ -164,12 +164,12 @@ char* Channel::RenderJSON() { json_iterator = nullptr; // We use -1 as sentinel values since proto default value for integers is // zero, and the confuses the parser into thinking the value weren't present - json_iterator = add_num_str(json, json_iterator, "callsStarted", - calls_started_); - json_iterator = add_num_str(json, json_iterator, "callsSucceeded", - calls_succeeded_); - json_iterator = add_num_str(json, json_iterator, "callsFailed", - calls_failed_); + json_iterator = + add_num_str(json, json_iterator, "callsStarted", calls_started_); + json_iterator = + add_num_str(json, json_iterator, "callsSucceeded", calls_succeeded_); + json_iterator = + add_num_str(json, json_iterator, "callsFailed", calls_failed_); gpr_timespec ts = grpc_millis_to_timespec(last_call_started_millis_, GPR_CLOCK_REALTIME); json_iterator = diff --git a/test/core/channel/channelz_test.cc b/test/core/channel/channelz_test.cc index df97d34fd0d..262ca38ee05 100644 --- a/test/core/channel/channelz_test.cc +++ b/test/core/channel/channelz_test.cc @@ -116,7 +116,6 @@ void ValidateChannel(Channel* channel, validate_channel_data_args args) { grpc::testing::ValidateChannelProtoJsonTranslation(json_str); ValidateCounters(json_str, args); gpr_free(json_str); - } grpc_millis GetLastCallStartedMillis(Channel* channel) { diff --git a/test/core/end2end/end2end_nosec_tests.cc b/test/core/end2end/end2end_nosec_tests.cc index 59eb643a93b..5ee0bb83fda 100644 --- a/test/core/end2end/end2end_nosec_tests.cc +++ b/test/core/end2end/end2end_nosec_tests.cc @@ -54,6 +54,8 @@ extern void cancel_in_a_vacuum(grpc_end2end_test_config config); extern void cancel_in_a_vacuum_pre_init(void); extern void cancel_with_status(grpc_end2end_test_config config); extern void cancel_with_status_pre_init(void); +extern void channelz(grpc_end2end_test_config config); +extern void channelz_pre_init(void); extern void compressed_payload(grpc_end2end_test_config config); extern void compressed_payload_pre_init(void); extern void connectivity(grpc_end2end_test_config config); @@ -199,6 +201,7 @@ void grpc_end2end_tests_pre_init(void) { cancel_before_invoke_pre_init(); cancel_in_a_vacuum_pre_init(); cancel_with_status_pre_init(); + channelz_pre_init(); compressed_payload_pre_init(); connectivity_pre_init(); default_host_pre_init(); @@ -284,6 +287,7 @@ void grpc_end2end_tests(int argc, char **argv, cancel_before_invoke(config); cancel_in_a_vacuum(config); cancel_with_status(config); + channelz(config); compressed_payload(config); connectivity(config); default_host(config); @@ -400,6 +404,10 @@ void grpc_end2end_tests(int argc, char **argv, cancel_with_status(config); continue; } + if (0 == strcmp("channelz", argv[i])) { + channelz(config); + continue; + } if (0 == strcmp("compressed_payload", argv[i])) { compressed_payload(config); continue; diff --git a/test/core/end2end/end2end_tests.cc b/test/core/end2end/end2end_tests.cc index 9f164b4eadf..ca5c18bcc27 100644 --- a/test/core/end2end/end2end_tests.cc +++ b/test/core/end2end/end2end_tests.cc @@ -56,6 +56,8 @@ extern void cancel_in_a_vacuum(grpc_end2end_test_config config); extern void cancel_in_a_vacuum_pre_init(void); extern void cancel_with_status(grpc_end2end_test_config config); extern void cancel_with_status_pre_init(void); +extern void channelz(grpc_end2end_test_config config); +extern void channelz_pre_init(void); extern void compressed_payload(grpc_end2end_test_config config); extern void compressed_payload_pre_init(void); extern void connectivity(grpc_end2end_test_config config); @@ -202,6 +204,7 @@ void grpc_end2end_tests_pre_init(void) { cancel_before_invoke_pre_init(); cancel_in_a_vacuum_pre_init(); cancel_with_status_pre_init(); + channelz_pre_init(); compressed_payload_pre_init(); connectivity_pre_init(); default_host_pre_init(); @@ -288,6 +291,7 @@ void grpc_end2end_tests(int argc, char **argv, cancel_before_invoke(config); cancel_in_a_vacuum(config); cancel_with_status(config); + channelz(config); compressed_payload(config); connectivity(config); default_host(config); @@ -408,6 +412,10 @@ void grpc_end2end_tests(int argc, char **argv, cancel_with_status(config); continue; } + if (0 == strcmp("channelz", argv[i])) { + channelz(config); + continue; + } if (0 == strcmp("compressed_payload", argv[i])) { compressed_payload(config); continue; diff --git a/test/core/end2end/gen_build_yaml.py b/test/core/end2end/gen_build_yaml.py index c355fc24b54..53f4ab0d52b 100755 --- a/test/core/end2end/gen_build_yaml.py +++ b/test/core/end2end/gen_build_yaml.py @@ -106,6 +106,7 @@ END2END_TESTS = { needs_compression=True), 'connectivity': connectivity_test_options._replace(needs_names=True, proxyable=False, cpu_cost=LOWCPU, exclude_iomgrs=['uv']), + 'channelz': default_test_options, 'default_host': default_test_options._replace( needs_fullstack=True, needs_dns=True, needs_names=True), 'call_host_override': default_test_options._replace( diff --git a/test/core/end2end/generate_tests.bzl b/test/core/end2end/generate_tests.bzl index 11fc576165b..94bc092b59d 100755 --- a/test/core/end2end/generate_tests.bzl +++ b/test/core/end2end/generate_tests.bzl @@ -113,6 +113,7 @@ END2END_TESTS = { 'compressed_payload': test_options(proxyable=False, exclude_inproc=True), 'connectivity': test_options(needs_fullstack=True, needs_names=True, proxyable=False), + 'channelz': test_options(), 'default_host': test_options(needs_fullstack=True, needs_dns=True, needs_names=True), 'disappearing_server': test_options(needs_fullstack=True,needs_names=True), diff --git a/test/core/end2end/tests/channelz.cc b/test/core/end2end/tests/channelz.cc new file mode 100644 index 00000000000..95d6e3880d7 --- /dev/null +++ b/test/core/end2end/tests/channelz.cc @@ -0,0 +1,263 @@ +/* + * + * Copyright 2015 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include "test/core/end2end/end2end_tests.h" + +#include +#include + +#include "src/core/lib/surface/channel.h" + +#include +#include +#include +#include +#include +#include "src/core/lib/gpr/string.h" +#include "test/core/end2end/cq_verifier.h" + +static void* tag(intptr_t t) { return (void*)t; } + +static grpc_end2end_test_fixture begin_test(grpc_end2end_test_config config, + const char* test_name, + grpc_channel_args* client_args, + grpc_channel_args* server_args) { + grpc_end2end_test_fixture f; + gpr_log(GPR_INFO, "Running test: %s/%s", test_name, config.name); + f = config.create_fixture(client_args, server_args); + config.init_server(&f, server_args); + config.init_client(&f, client_args); + return f; +} + +static gpr_timespec n_seconds_from_now(int n) { + return grpc_timeout_seconds_to_deadline(n); +} + +static gpr_timespec five_seconds_from_now(void) { + return n_seconds_from_now(5); +} + +static void drain_cq(grpc_completion_queue* cq) { + grpc_event ev; + do { + ev = grpc_completion_queue_next(cq, five_seconds_from_now(), nullptr); + } while (ev.type != GRPC_QUEUE_SHUTDOWN); +} + +static void shutdown_server(grpc_end2end_test_fixture* f) { + if (!f->server) return; + grpc_server_shutdown_and_notify(f->server, f->shutdown_cq, tag(1000)); + GPR_ASSERT(grpc_completion_queue_pluck(f->shutdown_cq, tag(1000), + grpc_timeout_seconds_to_deadline(5), + nullptr) + .type == GRPC_OP_COMPLETE); + grpc_server_destroy(f->server); + f->server = nullptr; +} + +static void shutdown_client(grpc_end2end_test_fixture* f) { + if (!f->client) return; + grpc_channel_destroy(f->client); + f->client = nullptr; +} + +static void end_test(grpc_end2end_test_fixture* f) { + shutdown_server(f); + shutdown_client(f); + + grpc_completion_queue_shutdown(f->cq); + drain_cq(f->cq); + grpc_completion_queue_destroy(f->cq); + grpc_completion_queue_destroy(f->shutdown_cq); +} + +static void run_one_request(grpc_end2end_test_config config, + grpc_end2end_test_fixture f, + bool request_is_success) { + grpc_call* c; + grpc_call* s; + cq_verifier* cqv = cq_verifier_create(f.cq); + grpc_op ops[6]; + grpc_op* op; + grpc_metadata_array initial_metadata_recv; + grpc_metadata_array trailing_metadata_recv; + grpc_metadata_array request_metadata_recv; + grpc_call_details call_details; + grpc_status_code status; + grpc_call_error error; + grpc_slice details; + int was_cancelled = 2; + + gpr_timespec deadline = five_seconds_from_now(); + c = grpc_channel_create_call(f.client, nullptr, GRPC_PROPAGATE_DEFAULTS, f.cq, + grpc_slice_from_static_string("/foo"), nullptr, + deadline, nullptr); + GPR_ASSERT(c); + + grpc_metadata_array_init(&initial_metadata_recv); + grpc_metadata_array_init(&trailing_metadata_recv); + grpc_metadata_array_init(&request_metadata_recv); + grpc_call_details_init(&call_details); + + memset(ops, 0, sizeof(ops)); + op = ops; + op->op = GRPC_OP_SEND_INITIAL_METADATA; + op->data.send_initial_metadata.count = 0; + op->flags = 0; + op->reserved = nullptr; + op++; + op->op = GRPC_OP_SEND_CLOSE_FROM_CLIENT; + op->flags = 0; + op->reserved = nullptr; + op++; + op->op = GRPC_OP_RECV_INITIAL_METADATA; + op->data.recv_initial_metadata.recv_initial_metadata = &initial_metadata_recv; + op->flags = 0; + op->reserved = nullptr; + op++; + op->op = GRPC_OP_RECV_STATUS_ON_CLIENT; + op->data.recv_status_on_client.trailing_metadata = &trailing_metadata_recv; + op->data.recv_status_on_client.status = &status; + op->data.recv_status_on_client.status_details = &details; + op->data.recv_status_on_client.error_string = nullptr; + op->flags = 0; + op->reserved = nullptr; + op++; + error = grpc_call_start_batch(c, ops, static_cast(op - ops), tag(1), + nullptr); + GPR_ASSERT(GRPC_CALL_OK == error); + + error = + grpc_server_request_call(f.server, &s, &call_details, + &request_metadata_recv, f.cq, f.cq, tag(101)); + GPR_ASSERT(GRPC_CALL_OK == error); + CQ_EXPECT_COMPLETION(cqv, tag(101), 1); + cq_verify(cqv); + + memset(ops, 0, sizeof(ops)); + op = ops; + op->op = GRPC_OP_SEND_INITIAL_METADATA; + op->data.send_initial_metadata.count = 0; + op->flags = 0; + op->reserved = nullptr; + op++; + op->op = GRPC_OP_SEND_STATUS_FROM_SERVER; + op->data.send_status_from_server.trailing_metadata_count = 0; + op->data.send_status_from_server.status = + request_is_success ? GRPC_STATUS_OK : GRPC_STATUS_UNIMPLEMENTED; + grpc_slice status_details = grpc_slice_from_static_string("xyz"); + op->data.send_status_from_server.status_details = &status_details; + op->flags = 0; + op->reserved = nullptr; + op++; + op->op = GRPC_OP_RECV_CLOSE_ON_SERVER; + op->data.recv_close_on_server.cancelled = &was_cancelled; + op->flags = 0; + op->reserved = nullptr; + op++; + error = grpc_call_start_batch(s, ops, static_cast(op - ops), tag(102), + nullptr); + GPR_ASSERT(GRPC_CALL_OK == error); + + CQ_EXPECT_COMPLETION(cqv, tag(102), 1); + CQ_EXPECT_COMPLETION(cqv, tag(1), 1); + cq_verify(cqv); + + GPR_ASSERT(0 == grpc_slice_str_cmp(details, "xyz")); + GPR_ASSERT(0 == grpc_slice_str_cmp(call_details.method, "/foo")); + GPR_ASSERT(0 == call_details.flags); + + grpc_slice_unref(details); + grpc_metadata_array_destroy(&initial_metadata_recv); + grpc_metadata_array_destroy(&trailing_metadata_recv); + grpc_metadata_array_destroy(&request_metadata_recv); + grpc_call_details_destroy(&call_details); + + grpc_call_unref(c); + grpc_call_unref(s); + + cq_verifier_destroy(cqv); +} + +static void test_channelz(grpc_end2end_test_config config) { + grpc_end2end_test_fixture f; + + f = begin_test(config, "test_channelz", nullptr, nullptr); + grpc_core::channelz::Channel* channelz_channel = + grpc_channel_get_channelz_node(f.client); + + char* json = channelz_channel->RenderJSON(); + GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"0\"")); + GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"0\"")); + GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"0\"")); + gpr_free(json); + + // one successful request + run_one_request(config, f, true); + + json = channelz_channel->RenderJSON(); + GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"1\"")); + GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"0\"")); + GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"1\"")); + gpr_free(json); + + // one failed request + run_one_request(config, f, false); + + json = channelz_channel->RenderJSON(); + 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\"")); + gpr_free(json); + + end_test(&f); + config.tear_down_data(&f); +} + +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}; + + f = begin_test(config, "test_channelz_with_channel_trace", &client_args, + nullptr); + grpc_core::channelz::Channel* channelz_channel = + grpc_channel_get_channelz_node(f.client); + + char* json = channelz_channel->RenderJSON(); + gpr_log(GPR_INFO, "%s", json); + 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); + config.tear_down_data(&f); +} +void channelz(grpc_end2end_test_config config) { + test_channelz(config); + test_channelz_with_channel_trace(config); +} + +void channelz_pre_init(void) {} diff --git a/test/core/end2end/tests/simple_request.cc b/test/core/end2end/tests/simple_request.cc index 8082bf93d54..941d9ae3198 100644 --- a/test/core/end2end/tests/simple_request.cc +++ b/test/core/end2end/tests/simple_request.cc @@ -21,8 +21,6 @@ #include #include -#include "src/core/lib/surface/channel.h" - #include #include #include @@ -254,18 +252,6 @@ static void test_invoke_simple_request(grpc_end2end_test_config config) { f = begin_test(config, "test_invoke_simple_request", nullptr, nullptr); simple_request_body(config, f); - - // The following is a quick sanity check on channelz functionality. It - // ensures that core properly tracked the one call that occurred in this - // simple end2end test. - grpc_core::channelz::Channel* channelz_channel = - grpc_channel_get_channelz_node(f.client); - char* json = channelz_channel->RenderJSON(); - GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"1\"")); - GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"1\"")); - GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"0\"")); - gpr_free(json); - end_test(&f); config.tear_down_data(&f); } @@ -278,16 +264,6 @@ static void test_invoke_10_simple_requests(grpc_end2end_test_config config) { simple_request_body(config, f); gpr_log(GPR_INFO, "Running test: Passed simple request %d", i); } - - // The following is a quick sanity check on channelz functionality. It - // ensures that core properly tracked the ten calls that occurred. - grpc_core::channelz::Channel* channelz_channel = - grpc_channel_get_channelz_node(f.client); - char* json = channelz_channel->RenderJSON(); - GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"10\"")); - GPR_ASSERT(nullptr != strstr(json, "\"callsFailed\":\"10\"")); - GPR_ASSERT(nullptr != strstr(json, "\"callsSucceeded\":\"0\"")); - gpr_free(json); end_test(&f); config.tear_down_data(&f); } diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json index efe53d7da5c..564f39b6ca1 100644 --- a/tools/run_tests/generated/sources_and_headers.json +++ b/tools/run_tests/generated/sources_and_headers.json @@ -8744,6 +8744,7 @@ "test/core/end2end/tests/cancel_in_a_vacuum.cc", "test/core/end2end/tests/cancel_test_helpers.h", "test/core/end2end/tests/cancel_with_status.cc", + "test/core/end2end/tests/channelz.cc", "test/core/end2end/tests/compressed_payload.cc", "test/core/end2end/tests/connectivity.cc", "test/core/end2end/tests/default_host.cc", @@ -8843,6 +8844,7 @@ "test/core/end2end/tests/cancel_in_a_vacuum.cc", "test/core/end2end/tests/cancel_test_helpers.h", "test/core/end2end/tests/cancel_with_status.cc", + "test/core/end2end/tests/channelz.cc", "test/core/end2end/tests/compressed_payload.cc", "test/core/end2end/tests/connectivity.cc", "test/core/end2end/tests/default_host.cc", diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index 0a02b1df05f..83d4f32eb60 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -7287,6 +7287,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "language": "c", + "name": "h2_census_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -9039,6 +9062,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "language": "c", + "name": "h2_compress_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -10755,6 +10801,28 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "language": "c", + "name": "h2_fakesec_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -12398,6 +12466,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_fd_test", + "platforms": [ + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -13640,6 +13731,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "language": "c", + "name": "h2_full_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -15340,6 +15454,25 @@ "linux" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "linux" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_full+pipe_test", + "platforms": [ + "linux" + ] + }, { "args": [ "compressed_payload" @@ -16836,6 +16969,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "language": "c", + "name": "h2_full+trace_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -18542,6 +18698,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "language": "c", + "name": "h2_full+workarounds_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -20307,6 +20486,30 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_http_proxy_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -22142,6 +22345,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "language": "c", + "name": "h2_load_reporting_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -23909,7 +24135,7 @@ }, { "args": [ - "compressed_payload" + "channelz" ], "ci_platforms": [ "windows", @@ -23933,14 +24159,14 @@ }, { "args": [ - "connectivity" + "compressed_payload" ], "ci_platforms": [ "windows", "linux", "posix" ], - "cpu_cost": 0.1, + "cpu_cost": 1.0, "exclude_configs": [], "exclude_iomgrs": [ "uv" @@ -23957,14 +24183,14 @@ }, { "args": [ - "default_host" + "connectivity" ], "ci_platforms": [ "windows", "linux", "posix" ], - "cpu_cost": 1.0, + "cpu_cost": 0.1, "exclude_configs": [], "exclude_iomgrs": [ "uv" @@ -23981,7 +24207,7 @@ }, { "args": [ - "disappearing_server" + "default_host" ], "ci_platforms": [ "windows", @@ -23993,30 +24219,6 @@ "exclude_iomgrs": [ "uv" ], - "flaky": true, - "language": "c", - "name": "h2_oauth2_test", - "platforms": [ - "windows", - "linux", - "mac", - "posix" - ] - }, - { - "args": [ - "empty_batch" - ], - "ci_platforms": [ - "windows", - "linux", - "posix" - ], - "cpu_cost": 0.1, - "exclude_configs": [], - "exclude_iomgrs": [ - "uv" - ], "flaky": false, "language": "c", "name": "h2_oauth2_test", @@ -24029,7 +24231,7 @@ }, { "args": [ - "filter_call_init_fails" + "disappearing_server" ], "ci_platforms": [ "windows", @@ -24041,7 +24243,7 @@ "exclude_iomgrs": [ "uv" ], - "flaky": false, + "flaky": true, "language": "c", "name": "h2_oauth2_test", "platforms": [ @@ -24053,7 +24255,55 @@ }, { "args": [ - "filter_causes_close" + "empty_batch" + ], + "ci_platforms": [ + "windows", + "linux", + "posix" + ], + "cpu_cost": 0.1, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_oauth2_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, + { + "args": [ + "filter_call_init_fails" + ], + "ci_platforms": [ + "windows", + "linux", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_oauth2_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, + { + "args": [ + "filter_causes_close" ], "ci_platforms": [ "windows", @@ -25707,6 +25957,30 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_proxy_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "default_host" @@ -26859,6 +27133,30 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_sockpair_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -28107,6 +28405,30 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_sockpair+trace_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -29305,6 +29627,32 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [ + "msan" + ], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_sockpair_1byte_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -30644,6 +30992,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "language": "c", + "name": "h2_ssl_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -32385,6 +32756,30 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_ssl_proxy_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "default_host" @@ -33549,6 +33944,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_uds_test", + "platforms": [ + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -35207,30 +35625,7 @@ }, { "args": [ - "empty_batch" - ], - "ci_platforms": [ - "windows", - "linux", - "mac", - "posix" - ], - "cpu_cost": 0.1, - "exclude_configs": [], - "exclude_iomgrs": [], - "flaky": false, - "language": "c", - "name": "inproc_test", - "platforms": [ - "windows", - "linux", - "mac", - "posix" - ] - }, - { - "args": [ - "filter_call_init_fails" + "channelz" ], "ci_platforms": [ "windows", @@ -35253,7 +35648,7 @@ }, { "args": [ - "filter_causes_close" + "empty_batch" ], "ci_platforms": [ "windows", @@ -35276,7 +35671,7 @@ }, { "args": [ - "filter_latency" + "filter_call_init_fails" ], "ci_platforms": [ "windows", @@ -35284,7 +35679,7 @@ "mac", "posix" ], - "cpu_cost": 0.1, + "cpu_cost": 1.0, "exclude_configs": [], "exclude_iomgrs": [], "flaky": false, @@ -35299,7 +35694,7 @@ }, { "args": [ - "filter_status_code" + "filter_causes_close" ], "ci_platforms": [ "windows", @@ -35322,7 +35717,7 @@ }, { "args": [ - "high_initial_seqno" + "filter_latency" ], "ci_platforms": [ "windows", @@ -35345,7 +35740,7 @@ }, { "args": [ - "hpack_size" + "filter_status_code" ], "ci_platforms": [ "windows", @@ -35368,7 +35763,7 @@ }, { "args": [ - "idempotent_request" + "high_initial_seqno" ], "ci_platforms": [ "windows", @@ -35376,7 +35771,7 @@ "mac", "posix" ], - "cpu_cost": 1.0, + "cpu_cost": 0.1, "exclude_configs": [], "exclude_iomgrs": [], "flaky": false, @@ -35391,7 +35786,7 @@ }, { "args": [ - "invoke_large_request" + "hpack_size" ], "ci_platforms": [ "windows", @@ -35399,7 +35794,7 @@ "mac", "posix" ], - "cpu_cost": 1.0, + "cpu_cost": 0.1, "exclude_configs": [], "exclude_iomgrs": [], "flaky": false, @@ -35414,7 +35809,7 @@ }, { "args": [ - "large_metadata" + "idempotent_request" ], "ci_platforms": [ "windows", @@ -35437,7 +35832,53 @@ }, { "args": [ - "load_reporting_hook" + "invoke_large_request" + ], + "ci_platforms": [ + "windows", + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "language": "c", + "name": "inproc_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, + { + "args": [ + "large_metadata" + ], + "ci_platforms": [ + "windows", + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "language": "c", + "name": "inproc_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, + { + "args": [ + "load_reporting_hook" ], "ci_platforms": [ "windows", @@ -36194,6 +36635,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "language": "c", + "name": "h2_census_nosec_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -37923,6 +38387,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "language": "c", + "name": "h2_compress_nosec_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -39583,6 +40070,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_fd_nosec_test", + "platforms": [ + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -40802,6 +41312,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "language": "c", + "name": "h2_full_nosec_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -42483,6 +43016,25 @@ "linux" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "linux" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_full+pipe_nosec_test", + "platforms": [ + "linux" + ] + }, { "args": [ "compressed_payload" @@ -43956,6 +44508,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "language": "c", + "name": "h2_full+trace_nosec_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -45639,6 +46214,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "language": "c", + "name": "h2_full+workarounds_nosec_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -47380,6 +47978,30 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_http_proxy_nosec_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -49192,6 +49814,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "language": "c", + "name": "h2_load_reporting_nosec_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -50909,6 +51554,30 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_proxy_nosec_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "default_host" @@ -52037,6 +52706,30 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_sockpair_nosec_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -53261,6 +53954,30 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_sockpair+trace_nosec_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -54433,6 +55150,32 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [ + "msan" + ], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_sockpair_1byte_nosec_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -55726,6 +56469,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [ + "uv" + ], + "flaky": false, + "language": "c", + "name": "h2_uds_nosec_test", + "platforms": [ + "linux", + "mac", + "posix" + ] + }, { "args": [ "compressed_payload" @@ -57359,6 +58125,29 @@ "posix" ] }, + { + "args": [ + "channelz" + ], + "ci_platforms": [ + "windows", + "linux", + "mac", + "posix" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "language": "c", + "name": "inproc_nosec_test", + "platforms": [ + "windows", + "linux", + "mac", + "posix" + ] + }, { "args": [ "empty_batch" From 6c987cbf0911b6d97eeec2e14bff2e8fc5bc40e5 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 8 Jun 2018 09:30:12 -0700 Subject: [PATCH 15/28] Rename channelz Channel to ChannelNode --- src/core/lib/channel/channel_trace.cc | 10 +++++----- src/core/lib/channel/channel_trace.h | 11 ++++++----- src/core/lib/channel/channelz.cc | 10 +++++----- src/core/lib/channel/channelz.h | 10 +++++----- src/core/lib/surface/call.cc | 4 ++-- src/core/lib/surface/channel.cc | 6 +++--- src/core/lib/surface/channel.h | 2 +- test/core/channel/channelz_test.cc | 18 +++++++++--------- 8 files changed, 36 insertions(+), 35 deletions(-) diff --git a/src/core/lib/channel/channel_trace.cc b/src/core/lib/channel/channel_trace.cc index 4c29df42f42..0f655d87160 100644 --- a/src/core/lib/channel/channel_trace.cc +++ b/src/core/lib/channel/channel_trace.cc @@ -41,9 +41,9 @@ namespace grpc_core { namespace channelz { -ChannelTrace::TraceEvent::TraceEvent(Severity severity, grpc_slice data, - RefCountedPtr referenced_channel, - ReferencedType type) +ChannelTrace::TraceEvent::TraceEvent( + Severity severity, grpc_slice data, + RefCountedPtr referenced_channel, ReferencedType type) : severity_(severity), data_(data), timestamp_(grpc_millis_to_timespec(grpc_core::ExecCtx::Get()->Now(), @@ -112,7 +112,7 @@ void ChannelTrace::AddTraceEvent(Severity severity, grpc_slice data) { void ChannelTrace::AddTraceEventReferencingChannel( Severity severity, grpc_slice data, - RefCountedPtr referenced_channel) { + RefCountedPtr referenced_channel) { if (max_list_size_ == 0) return; // tracing is disabled if max_events == 0 // create and fill up the new event AddTraceEventHelper(New( @@ -121,7 +121,7 @@ void ChannelTrace::AddTraceEventReferencingChannel( void ChannelTrace::AddTraceEventReferencingSubchannel( Severity severity, grpc_slice data, - RefCountedPtr referenced_subchannel) { + RefCountedPtr referenced_subchannel) { if (max_list_size_ == 0) return; // tracing is disabled if max_events == 0 // create and fill up the new event AddTraceEventHelper(New(severity, data, diff --git a/src/core/lib/channel/channel_trace.h b/src/core/lib/channel/channel_trace.h index f6021c32243..0dd162a7778 100644 --- a/src/core/lib/channel/channel_trace.h +++ b/src/core/lib/channel/channel_trace.h @@ -30,7 +30,7 @@ namespace grpc_core { namespace channelz { -class Channel; +class ChannelNode; // Object used to hold live data for a channel. This data is exposed via the // channelz service: @@ -64,10 +64,10 @@ class ChannelTrace { // slice. void AddTraceEventReferencingChannel( Severity severity, grpc_slice data, - RefCountedPtr referenced_channel); + RefCountedPtr referenced_channel); void AddTraceEventReferencingSubchannel( Severity severity, grpc_slice data, - RefCountedPtr referenced_subchannel); + RefCountedPtr referenced_subchannel); // Creates and returns the raw grpc_json object, so a parent channelz // object may incorporate the json before rendering. @@ -82,7 +82,8 @@ class ChannelTrace { public: // Constructor for a TraceEvent that references a different channel. TraceEvent(Severity severity, grpc_slice data, - RefCountedPtr referenced_channel, ReferencedType type); + RefCountedPtr referenced_channel, + ReferencedType type); // Constructor for a TraceEvent that does not reverence a different // channel. @@ -104,7 +105,7 @@ class ChannelTrace { gpr_timespec timestamp_; TraceEvent* next_; // the tracer object for the (sub)channel that this trace event refers to. - RefCountedPtr referenced_channel_; + RefCountedPtr referenced_channel_; // the type that the referenced tracer points to. Unused if this trace // does not point to any channel or subchannel ReferencedType referenced_type_; diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index 42b169dffd1..799eb8bed1a 100644 --- a/src/core/lib/channel/channelz.cc +++ b/src/core/lib/channel/channelz.cc @@ -89,7 +89,7 @@ grpc_json* add_num_str(grpc_json* parent, grpc_json* it, const char* name, } // namespace -Channel::Channel(grpc_channel* channel, size_t channel_tracer_max_nodes) +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)) { @@ -98,18 +98,18 @@ Channel::Channel(grpc_channel* channel, size_t channel_tracer_max_nodes) (gpr_atm)ExecCtx::Get()->Now()); } -Channel::~Channel() { +ChannelNode::~ChannelNode() { trace_.Destroy(); ChannelzRegistry::Unregister(channel_uuid_); } -void Channel::RecordCallStarted() { +void ChannelNode::RecordCallStarted() { 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()); } -grpc_connectivity_state Channel::GetConnectivityState() { +grpc_connectivity_state ChannelNode::GetConnectivityState() { if (channel_ == nullptr) { return GRPC_CHANNEL_SHUTDOWN; } else { @@ -117,7 +117,7 @@ grpc_connectivity_state Channel::GetConnectivityState() { } } -char* Channel::RenderJSON() { +char* ChannelNode::RenderJSON() { // 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 68b7c8d26b9..62dc817b6ad 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -35,13 +35,13 @@ namespace grpc_core { namespace channelz { namespace testing { -class ChannelPeer; +class ChannelNodePeer; } -class Channel : public RefCounted { +class ChannelNode : public RefCounted { public: - Channel(grpc_channel* channel, size_t channel_tracer_max_nodes); - ~Channel(); + ChannelNode(grpc_channel* channel, size_t channel_tracer_max_nodes); + ~ChannelNode(); void RecordCallStarted(); void RecordCallFailed() { @@ -64,7 +64,7 @@ class Channel : public RefCounted { private: // testing peer friend. - friend class testing::ChannelPeer; + friend class testing::ChannelNodePeer; // helper for getting connectivity state. grpc_connectivity_state GetConnectivityState(); diff --git a/src/core/lib/surface/call.cc b/src/core/lib/surface/call.cc index 65071672f27..33b9908a574 100644 --- a/src/core/lib/surface/call.cc +++ b/src/core/lib/surface/call.cc @@ -478,7 +478,7 @@ grpc_error* grpc_call_create(const grpc_call_create_args* args, &call->pollent); } - grpc_core::channelz::Channel* channelz_channel = + grpc_core::channelz::ChannelNode* channelz_channel = grpc_channel_get_channelz_node(call->channel); channelz_channel->RecordCallStarted(); @@ -1260,7 +1260,7 @@ static void post_batch_completion(batch_control* bctl) { call->final_op.server.cancelled, nullptr, nullptr); } - grpc_core::channelz::Channel* channelz_channel = + 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(); diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index d2b52dad2fd..7dcfbc97cc1 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -67,7 +67,7 @@ struct grpc_channel { gpr_mu registered_call_mu; registered_call* registered_calls; - grpc_core::RefCountedPtr channelz_channel; + grpc_core::RefCountedPtr channelz_channel; char* target; }; @@ -147,7 +147,7 @@ grpc_channel* grpc_channel_create_with_builder( grpc_channel_args_destroy(args); channel->channelz_channel = - grpc_core::MakeRefCounted( + grpc_core::MakeRefCounted( channel, channel_tracer_max_nodes); channel->channelz_channel->trace()->AddTraceEvent( grpc_core::channelz::ChannelTrace::Severity::Info, @@ -190,7 +190,7 @@ char* grpc_channel_render_channelz(grpc_channel* channel) { return channel->channelz_channel->RenderJSON(); } -grpc_core::channelz::Channel* grpc_channel_get_channelz_node( +grpc_core::channelz::ChannelNode* grpc_channel_get_channelz_node( grpc_channel* channel) { return channel->channelz_channel.get(); } diff --git a/src/core/lib/surface/channel.h b/src/core/lib/surface/channel.h index 6d2e1931e68..e5ff2c35969 100644 --- a/src/core/lib/surface/channel.h +++ b/src/core/lib/surface/channel.h @@ -51,7 +51,7 @@ grpc_call* grpc_channel_create_pollset_set_call( /** Get a (borrowed) pointer to this channels underlying channel stack */ grpc_channel_stack* grpc_channel_get_channel_stack(grpc_channel* channel); -grpc_core::channelz::Channel* grpc_channel_get_channelz_node( +grpc_core::channelz::ChannelNode* grpc_channel_get_channelz_node( grpc_channel* channel); /** Get a grpc_mdelem of grpc-status: X where X is the numeric value of diff --git a/test/core/channel/channelz_test.cc b/test/core/channel/channelz_test.cc index 262ca38ee05..8ef91c3b09f 100644 --- a/test/core/channel/channelz_test.cc +++ b/test/core/channel/channelz_test.cc @@ -44,16 +44,16 @@ namespace channelz { namespace testing { // testing peer to access channel internals -class ChannelPeer { +class ChannelNodePeer { public: - ChannelPeer(Channel* channel) : channel_(channel) {} + ChannelNodePeer(ChannelNode* channel) : channel_(channel) {} grpc_millis last_call_started_millis() { return (grpc_millis)gpr_atm_no_barrier_load( &channel_->last_call_started_millis_); } private: - Channel* channel_; + ChannelNode* channel_; }; namespace { @@ -111,15 +111,15 @@ void ValidateCounters(char* json_str, validate_channel_data_args args) { grpc_json_destroy(json); } -void ValidateChannel(Channel* channel, validate_channel_data_args args) { +void ValidateChannel(ChannelNode* channel, validate_channel_data_args args) { char* json_str = channel->RenderJSON(); grpc::testing::ValidateChannelProtoJsonTranslation(json_str); ValidateCounters(json_str, args); gpr_free(json_str); } -grpc_millis GetLastCallStartedMillis(Channel* channel) { - ChannelPeer peer(channel); +grpc_millis GetLastCallStartedMillis(ChannelNode* channel) { + ChannelNodePeer peer(channel); return peer.last_call_started_millis(); } @@ -137,7 +137,7 @@ TEST_P(ChannelzChannelTest, BasicChannel) { grpc_core::ExecCtx exec_ctx; ChannelFixture channel(GetParam()); intptr_t uuid = grpc_channel_get_uuid(channel.channel()); - Channel* channelz_channel = ChannelzRegistry::Get(uuid); + ChannelNode* channelz_channel = ChannelzRegistry::Get(uuid); char* json_str = channelz_channel->RenderJSON(); ValidateCounters(json_str, {0, 0, 0}); gpr_free(json_str); @@ -147,7 +147,7 @@ TEST_P(ChannelzChannelTest, BasicChannelAPIFunctionality) { grpc_core::ExecCtx exec_ctx; ChannelFixture channel(GetParam()); intptr_t uuid = grpc_channel_get_uuid(channel.channel()); - Channel* channelz_channel = ChannelzRegistry::Get(uuid); + ChannelNode* channelz_channel = ChannelzRegistry::Get(uuid); channelz_channel->RecordCallStarted(); channelz_channel->RecordCallFailed(); channelz_channel->RecordCallSucceeded(); @@ -165,7 +165,7 @@ TEST_P(ChannelzChannelTest, LastCallStartedMillis) { grpc_core::ExecCtx exec_ctx; ChannelFixture channel(GetParam()); intptr_t uuid = grpc_channel_get_uuid(channel.channel()); - Channel* channelz_channel = ChannelzRegistry::Get(uuid); + ChannelNode* channelz_channel = ChannelzRegistry::Get(uuid); // start a call to set the last call started timestamp channelz_channel->RecordCallStarted(); grpc_millis millis1 = GetLastCallStartedMillis(channelz_channel); From 16280c7398809f374a90f1e6150d788cf7f2f6f2 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 8 Jun 2018 09:44:25 -0700 Subject: [PATCH 16/28] Remove unused API --- grpc.def | 1 - include/grpc/grpc.h | 4 ---- src/core/lib/surface/channel.cc | 4 ---- src/ruby/ext/grpc/rb_grpc_imports.generated.c | 2 -- src/ruby/ext/grpc/rb_grpc_imports.generated.h | 3 --- test/core/channel/channelz_test.cc | 12 ++++++------ test/core/surface/public_headers_must_be_c89.c | 1 - 7 files changed, 6 insertions(+), 21 deletions(-) diff --git a/grpc.def b/grpc.def index fc1d02a0352..e7f93396b2d 100644 --- a/grpc.def +++ b/grpc.def @@ -45,7 +45,6 @@ EXPORTS grpc_insecure_channel_create grpc_lame_client_channel_create grpc_channel_destroy - grpc_channel_get_uuid grpc_call_cancel grpc_call_cancel_with_status grpc_call_ref diff --git a/include/grpc/grpc.h b/include/grpc/grpc.h index bb4a70f7b9f..c129a66949f 100644 --- a/include/grpc/grpc.h +++ b/include/grpc/grpc.h @@ -286,10 +286,6 @@ GRPCAPI grpc_channel* grpc_lame_client_channel_create( /** Close and destroy a grpc channel */ GRPCAPI void grpc_channel_destroy(grpc_channel* channel); -/** 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. diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index 7dcfbc97cc1..69c1329cdaa 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -195,10 +195,6 @@ grpc_core::channelz::ChannelNode* grpc_channel_get_channelz_node( return channel->channelz_channel.get(); } -intptr_t grpc_channel_get_uuid(grpc_channel* channel) { - return channel->channelz_channel->channel_uuid(); -} - grpc_channel* grpc_channel_create(const char* target, const grpc_channel_args* input_args, grpc_channel_stack_type channel_stack_type, diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.c b/src/ruby/ext/grpc/rb_grpc_imports.generated.c index 8b67a7ce2d2..031699ce8e2 100644 --- a/src/ruby/ext/grpc/rb_grpc_imports.generated.c +++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.c @@ -68,7 +68,6 @@ grpc_channel_get_info_type grpc_channel_get_info_import; grpc_insecure_channel_create_type grpc_insecure_channel_create_import; grpc_lame_client_channel_create_type grpc_lame_client_channel_create_import; grpc_channel_destroy_type grpc_channel_destroy_import; -grpc_channel_get_uuid_type grpc_channel_get_uuid_import; grpc_call_cancel_type grpc_call_cancel_import; grpc_call_cancel_with_status_type grpc_call_cancel_with_status_import; grpc_call_ref_type grpc_call_ref_import; @@ -315,7 +314,6 @@ void grpc_rb_load_imports(HMODULE library) { grpc_insecure_channel_create_import = (grpc_insecure_channel_create_type) GetProcAddress(library, "grpc_insecure_channel_create"); grpc_lame_client_channel_create_import = (grpc_lame_client_channel_create_type) GetProcAddress(library, "grpc_lame_client_channel_create"); grpc_channel_destroy_import = (grpc_channel_destroy_type) GetProcAddress(library, "grpc_channel_destroy"); - grpc_channel_get_uuid_import = (grpc_channel_get_uuid_type) GetProcAddress(library, "grpc_channel_get_uuid"); grpc_call_cancel_import = (grpc_call_cancel_type) GetProcAddress(library, "grpc_call_cancel"); grpc_call_cancel_with_status_import = (grpc_call_cancel_with_status_type) GetProcAddress(library, "grpc_call_cancel_with_status"); grpc_call_ref_import = (grpc_call_ref_type) GetProcAddress(library, "grpc_call_ref"); diff --git a/src/ruby/ext/grpc/rb_grpc_imports.generated.h b/src/ruby/ext/grpc/rb_grpc_imports.generated.h index c7919871c86..3cc6492d04f 100644 --- a/src/ruby/ext/grpc/rb_grpc_imports.generated.h +++ b/src/ruby/ext/grpc/rb_grpc_imports.generated.h @@ -179,9 +179,6 @@ extern grpc_lame_client_channel_create_type grpc_lame_client_channel_create_impo typedef void(*grpc_channel_destroy_type)(grpc_channel* channel); extern grpc_channel_destroy_type grpc_channel_destroy_import; #define grpc_channel_destroy grpc_channel_destroy_import -typedef intptr_t(*grpc_channel_get_uuid_type)(grpc_channel* channel); -extern grpc_channel_get_uuid_type grpc_channel_get_uuid_import; -#define grpc_channel_get_uuid grpc_channel_get_uuid_import typedef grpc_call_error(*grpc_call_cancel_type)(grpc_call* call, void* reserved); extern grpc_call_cancel_type grpc_call_cancel_import; #define grpc_call_cancel grpc_call_cancel_import diff --git a/test/core/channel/channelz_test.cc b/test/core/channel/channelz_test.cc index 8ef91c3b09f..facf1c03f76 100644 --- a/test/core/channel/channelz_test.cc +++ b/test/core/channel/channelz_test.cc @@ -136,8 +136,8 @@ class ChannelzChannelTest : public ::testing::TestWithParam {}; TEST_P(ChannelzChannelTest, BasicChannel) { grpc_core::ExecCtx exec_ctx; ChannelFixture channel(GetParam()); - intptr_t uuid = grpc_channel_get_uuid(channel.channel()); - ChannelNode* channelz_channel = ChannelzRegistry::Get(uuid); + ChannelNode* channelz_channel = + grpc_channel_get_channelz_node(channel.channel()); char* json_str = channelz_channel->RenderJSON(); ValidateCounters(json_str, {0, 0, 0}); gpr_free(json_str); @@ -146,8 +146,8 @@ TEST_P(ChannelzChannelTest, BasicChannel) { TEST_P(ChannelzChannelTest, BasicChannelAPIFunctionality) { grpc_core::ExecCtx exec_ctx; ChannelFixture channel(GetParam()); - intptr_t uuid = grpc_channel_get_uuid(channel.channel()); - ChannelNode* channelz_channel = ChannelzRegistry::Get(uuid); + ChannelNode* channelz_channel = + grpc_channel_get_channelz_node(channel.channel()); channelz_channel->RecordCallStarted(); channelz_channel->RecordCallFailed(); channelz_channel->RecordCallSucceeded(); @@ -164,8 +164,8 @@ TEST_P(ChannelzChannelTest, BasicChannelAPIFunctionality) { TEST_P(ChannelzChannelTest, LastCallStartedMillis) { grpc_core::ExecCtx exec_ctx; ChannelFixture channel(GetParam()); - intptr_t uuid = grpc_channel_get_uuid(channel.channel()); - ChannelNode* channelz_channel = ChannelzRegistry::Get(uuid); + ChannelNode* channelz_channel = + grpc_channel_get_channelz_node(channel.channel()); // start a call to set the last call started timestamp channelz_channel->RecordCallStarted(); grpc_millis millis1 = GetLastCallStartedMillis(channelz_channel); diff --git a/test/core/surface/public_headers_must_be_c89.c b/test/core/surface/public_headers_must_be_c89.c index 96b25ebdaff..29083061bbb 100644 --- a/test/core/surface/public_headers_must_be_c89.c +++ b/test/core/surface/public_headers_must_be_c89.c @@ -106,7 +106,6 @@ int main(int argc, char **argv) { printf("%lx", (unsigned long) grpc_insecure_channel_create); printf("%lx", (unsigned long) grpc_lame_client_channel_create); printf("%lx", (unsigned long) grpc_channel_destroy); - printf("%lx", (unsigned long) grpc_channel_get_uuid); printf("%lx", (unsigned long) grpc_call_cancel); printf("%lx", (unsigned long) grpc_call_cancel_with_status); printf("%lx", (unsigned long) grpc_call_ref); From db6593eb4045f2bcf0aabf61945dbddc8c7f791e Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 8 Jun 2018 09:46:55 -0700 Subject: [PATCH 17/28] fixup! Rename channelz Channel to ChannelNode --- test/core/end2end/tests/channelz.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/core/end2end/tests/channelz.cc b/test/core/end2end/tests/channelz.cc index 95d6e3880d7..9766646c138 100644 --- a/test/core/end2end/tests/channelz.cc +++ b/test/core/end2end/tests/channelz.cc @@ -199,7 +199,7 @@ static void test_channelz(grpc_end2end_test_config config) { grpc_end2end_test_fixture f; f = begin_test(config, "test_channelz", nullptr, nullptr); - grpc_core::channelz::Channel* channelz_channel = + grpc_core::channelz::ChannelNode* channelz_channel = grpc_channel_get_channelz_node(f.client); char* json = channelz_channel->RenderJSON(); @@ -242,7 +242,7 @@ static void test_channelz_with_channel_trace(grpc_end2end_test_config config) { f = begin_test(config, "test_channelz_with_channel_trace", &client_args, nullptr); - grpc_core::channelz::Channel* channelz_channel = + grpc_core::channelz::ChannelNode* channelz_channel = grpc_channel_get_channelz_node(f.client); char* json = channelz_channel->RenderJSON(); From 5644c5928d748e44478bbddc97fcc75488878d8c Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 8 Jun 2018 11:09:44 -0700 Subject: [PATCH 18/28] fixup! fixup! Rename channelz Channel to ChannelNode --- test/core/channel/channel_trace_test.cc | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/core/channel/channel_trace_test.cc b/test/core/channel/channel_trace_test.cc index ef69e631410..bbddee3f14e 100644 --- a/test/core/channel/channel_trace_test.cc +++ b/test/core/channel/channel_trace_test.cc @@ -156,8 +156,8 @@ TEST_P(ChannelTracerTest, ComplexTest) { AddSimpleTrace(&tracer); AddSimpleTrace(&tracer); ChannelFixture channel1(GetParam()); - RefCountedPtr sc1 = - MakeRefCounted(channel1.channel(), GetParam()); + RefCountedPtr sc1 = + MakeRefCounted(channel1.channel(), GetParam()); tracer.AddTraceEventReferencingSubchannel( ChannelTrace::Severity::Info, grpc_slice_from_static_string("subchannel one created"), sc1); @@ -174,8 +174,8 @@ TEST_P(ChannelTracerTest, ComplexTest) { AddSimpleTrace(&tracer); ValidateChannelTrace(&tracer, 5, GetParam()); ChannelFixture channel2(GetParam()); - RefCountedPtr sc2 = - MakeRefCounted(channel2.channel(), GetParam()); + RefCountedPtr sc2 = + MakeRefCounted(channel2.channel(), GetParam()); tracer.AddTraceEventReferencingChannel( ChannelTrace::Severity::Info, grpc_slice_from_static_string("LB channel two created"), sc2); @@ -203,16 +203,16 @@ TEST_P(ChannelTracerTest, TestNesting) { AddSimpleTrace(&tracer); ValidateChannelTrace(&tracer, 2, GetParam()); ChannelFixture channel1(GetParam()); - RefCountedPtr sc1 = - MakeRefCounted(channel1.channel(), GetParam()); + RefCountedPtr sc1 = + MakeRefCounted(channel1.channel(), GetParam()); tracer.AddTraceEventReferencingChannel( ChannelTrace::Severity::Info, grpc_slice_from_static_string("subchannel one created"), sc1); ValidateChannelTrace(&tracer, 3, GetParam()); AddSimpleTrace(sc1->trace()); ChannelFixture channel2(GetParam()); - RefCountedPtr conn1 = - MakeRefCounted(channel2.channel(), GetParam()); + RefCountedPtr conn1 = + MakeRefCounted(channel2.channel(), GetParam()); // nesting one level deeper. sc1->trace()->AddTraceEventReferencingSubchannel( ChannelTrace::Severity::Info, @@ -224,8 +224,8 @@ TEST_P(ChannelTracerTest, TestNesting) { ValidateChannelTrace(&tracer, 5, GetParam()); ValidateChannelTrace(conn1->trace(), 1, GetParam()); ChannelFixture channel3(GetParam()); - RefCountedPtr sc2 = - MakeRefCounted(channel3.channel(), GetParam()); + RefCountedPtr sc2 = + MakeRefCounted(channel3.channel(), GetParam()); tracer.AddTraceEventReferencingSubchannel( ChannelTrace::Severity::Info, grpc_slice_from_static_string("subchannel two created"), sc2); From e5047b5244486b4e80b702442137060aa18fd54b Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 8 Jun 2018 11:49:39 -0700 Subject: [PATCH 19/28] Fix ASAN --- src/core/lib/surface/channel.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index 69c1329cdaa..087b7778bfd 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -398,6 +398,8 @@ 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(); grpc_channel_stack_destroy(CHANNEL_STACK_FROM_CHANNEL(channel)); while (channel->registered_calls) { registered_call* rc = channel->registered_calls; @@ -420,8 +422,6 @@ void grpc_channel_destroy(grpc_channel* channel) { GRPC_ERROR_CREATE_FROM_STATIC_STRING("Channel Destroyed"); elem = grpc_channel_stack_element(CHANNEL_STACK_FROM_CHANNEL(channel), 0); elem->filter->start_transport_op(elem, op); - channel->channelz_channel->set_channel_destroyed(); - channel->channelz_channel.reset(); GRPC_CHANNEL_INTERNAL_UNREF(channel, "channel"); } From 7a28a34ca78b8fa1a02c2fe6882a355d0ff672c3 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Mon, 11 Jun 2018 17:06:19 -0700 Subject: [PATCH 20/28] log granular ruby end2end test times --- .../helper_scripts/run_ruby_end2end_tests.sh | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tools/run_tests/helper_scripts/run_ruby_end2end_tests.sh b/tools/run_tests/helper_scripts/run_ruby_end2end_tests.sh index 5784745bac0..1ae1d598dea 100755 --- a/tools/run_tests/helper_scripts/run_ruby_end2end_tests.sh +++ b/tools/run_tests/helper_scripts/run_ruby_end2end_tests.sh @@ -19,14 +19,14 @@ set -ex cd "$(dirname "$0")/../../.." EXIT_CODE=0 -ruby src/ruby/end2end/sig_handling_driver.rb || EXIT_CODE=1 -ruby src/ruby/end2end/channel_state_driver.rb || EXIT_CODE=1 -ruby src/ruby/end2end/channel_closing_driver.rb || EXIT_CODE=1 -ruby src/ruby/end2end/sig_int_during_channel_watch_driver.rb || EXIT_CODE=1 -ruby src/ruby/end2end/killed_client_thread_driver.rb || EXIT_CODE=1 -ruby src/ruby/end2end/forking_client_driver.rb || EXIT_CODE=1 -ruby src/ruby/end2end/grpc_class_init_driver.rb || EXIT_CODE=1 -ruby src/ruby/end2end/multiple_killed_watching_threads_driver.rb || EXIT_CODE=1 -ruby src/ruby/end2end/load_grpc_with_gc_stress_driver.rb || EXIT_CODE=1 -ruby src/ruby/end2end/client_memory_usage_driver.rb || EXIT_CODE=1 +time ruby src/ruby/end2end/sig_handling_driver.rb || EXIT_CODE=1 +time ruby src/ruby/end2end/channel_state_driver.rb || EXIT_CODE=1 +time ruby src/ruby/end2end/channel_closing_driver.rb || EXIT_CODE=1 +time ruby src/ruby/end2end/sig_int_during_channel_watch_driver.rb || EXIT_CODE=1 +time ruby src/ruby/end2end/killed_client_thread_driver.rb || EXIT_CODE=1 +time ruby src/ruby/end2end/forking_client_driver.rb || EXIT_CODE=1 +time ruby src/ruby/end2end/grpc_class_init_driver.rb || EXIT_CODE=1 +time ruby src/ruby/end2end/multiple_killed_watching_threads_driver.rb || EXIT_CODE=1 +time ruby src/ruby/end2end/load_grpc_with_gc_stress_driver.rb || EXIT_CODE=1 +time ruby src/ruby/end2end/client_memory_usage_driver.rb || EXIT_CODE=1 exit $EXIT_CODE From 2a881a3a05ade7211657e10bd6a324daebc56677 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Mon, 11 Jun 2018 17:25:23 -0700 Subject: [PATCH 21/28] double timeout to check if finishes --- tools/run_tests/run_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/run_tests/run_tests.py b/tools/run_tests/run_tests.py index ea4c7c3c655..014fb7d33a3 100755 --- a/tools/run_tests/run_tests.py +++ b/tools/run_tests/run_tests.py @@ -883,7 +883,7 @@ class RubyLanguage(object): tests.append( self.config.job_spec( ['tools/run_tests/helper_scripts/run_ruby_end2end_tests.sh'], - timeout_seconds=10 * 60, + timeout_seconds=20 * 60, environ=_FORCE_ENVIRON_FOR_WRAPPERS)) return tests From cb5471c2eead73a90dca739fbfbfb8819a3ec31b Mon Sep 17 00:00:00 2001 From: ncteisen Date: Mon, 11 Jun 2018 20:07:43 -0700 Subject: [PATCH 22/28] Sleep less for ruby test --- src/ruby/end2end/multiple_killed_watching_threads_driver.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ruby/end2end/multiple_killed_watching_threads_driver.rb b/src/ruby/end2end/multiple_killed_watching_threads_driver.rb index 8ec2073d988..dbfdf61599f 100755 --- a/src/ruby/end2end/multiple_killed_watching_threads_driver.rb +++ b/src/ruby/end2end/multiple_killed_watching_threads_driver.rb @@ -60,8 +60,8 @@ def main run_multiple_killed_watches(1000, 0.001) STDERR.puts '10000 iterations, sleep 0.00001 before killing thread' run_multiple_killed_watches(10_000, 0.00001) - STDERR.puts '20000 iterations, sleep 0.00001 before killing thread' - run_multiple_killed_watches(20_000, 0.00001) + STDERR.puts '20000 iterations, sleep 0.000001 before killing thread' + run_multiple_killed_watches(20_000, 0.000001) end main From 19ac7c0baf298be51c0dbfa7d5bcd1f45eec7c78 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 13 Jun 2018 15:06:07 -0700 Subject: [PATCH 23/28] Fix ruby test by backing off on the concurrency --- src/ruby/end2end/multiple_killed_watching_threads_driver.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/ruby/end2end/multiple_killed_watching_threads_driver.rb b/src/ruby/end2end/multiple_killed_watching_threads_driver.rb index dbfdf61599f..7b39f5a3476 100755 --- a/src/ruby/end2end/multiple_killed_watching_threads_driver.rb +++ b/src/ruby/end2end/multiple_killed_watching_threads_driver.rb @@ -58,10 +58,6 @@ def main run_multiple_killed_watches(10, 0.1) STDERR.puts '1000 iterations, sleep 0.001 before killing thread' run_multiple_killed_watches(1000, 0.001) - STDERR.puts '10000 iterations, sleep 0.00001 before killing thread' - run_multiple_killed_watches(10_000, 0.00001) - STDERR.puts '20000 iterations, sleep 0.000001 before killing thread' - run_multiple_killed_watches(20_000, 0.000001) end main From c7166ae67dd554d41b4d26286da2888aebc0153b Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 15 Jun 2018 11:04:37 -0400 Subject: [PATCH 24/28] 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) {} From 4d1da600b525a514f0a5c1d768fa3a58d65ecded Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 15 Jun 2018 14:54:26 -0400 Subject: [PATCH 25/28] Fix ASAN and sanity --- CMakeLists.txt | 1 + test/core/channel/channelz_test.cc | 1 + 2 files changed, 2 insertions(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index 0fd612b5c6d..a875b8c73e4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -10797,6 +10797,7 @@ target_include_directories(channelz_test PRIVATE ${_gRPC_CARES_INCLUDE_DIR} PRIVATE ${_gRPC_GFLAGS_INCLUDE_DIR} PRIVATE ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR} + PRIVATE ${_gRPC_NANOPB_INCLUDE_DIR} PRIVATE third_party/googletest/googletest/include PRIVATE third_party/googletest/googletest PRIVATE third_party/googletest/googlemock/include diff --git a/test/core/channel/channelz_test.cc b/test/core/channel/channelz_test.cc index e1dc76e4aa5..cfd029f8fce 100644 --- a/test/core/channel/channelz_test.cc +++ b/test/core/channel/channelz_test.cc @@ -153,6 +153,7 @@ TEST(ChannelzChannelTest, ChannelzDisabled) { ChannelNode* channelz_channel = grpc_channel_get_channelz_node(channel); char* json_str = channelz_channel->RenderJSON(); ASSERT_EQ(json_str, nullptr); + grpc_channel_destroy(channel); } TEST_P(ChannelzChannelTest, BasicChannelAPIFunctionality) { From 0f20212d38f7f55229467f26293a5e2b3f39d5a0 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 19 Jun 2018 16:05:08 -0700 Subject: [PATCH 26/28] Regenerate project --- CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index 96858473511..8d878ae7c76 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -7520,6 +7520,7 @@ target_include_directories(handshake_verify_peer_options PRIVATE ${_gRPC_CARES_INCLUDE_DIR} PRIVATE ${_gRPC_GFLAGS_INCLUDE_DIR} PRIVATE ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR} + PRIVATE ${_gRPC_NANOPB_INCLUDE_DIR} ) target_link_libraries(handshake_verify_peer_options From 1ab1287ac77972bee4e2f516d81230a7f0044f14 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 19 Jun 2018 17:09:53 -0700 Subject: [PATCH 27/28] 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(-) diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index 786c070c140..c32e99ed4c0 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 40c0932f3f8..3550fc0551e 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 3352daccd09..2aad1e82f48 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 f94035459f0..556eb234b44 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 b8255706058..d5d75fcb2aa 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 e1bde4e6d4b..bbddee3f14e 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 cfd029f8fce..058eea914c2 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 06343c46b4f..847b900af1b 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) {} From 48d03394066c48fdeb2818187eaf356135b04b6d Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 20 Jun 2018 08:52:27 -0700 Subject: [PATCH 28/28] reviewer feedback --- test/core/end2end/tests/channelz.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/core/end2end/tests/channelz.cc b/test/core/end2end/tests/channelz.cc index 847b900af1b..eb052119ca8 100644 --- a/test/core/end2end/tests/channelz.cc +++ b/test/core/end2end/tests/channelz.cc @@ -208,7 +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); + GPR_ASSERT(channelz_channel != nullptr); char* json = channelz_channel->RenderJSON(); GPR_ASSERT(json != nullptr); GPR_ASSERT(nullptr != strstr(json, "\"callsStarted\":\"0\"")); @@ -263,7 +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); + GPR_ASSERT(channelz_channel != nullptr); char* json = channelz_channel->RenderJSON(); GPR_ASSERT(json != nullptr); gpr_log(GPR_INFO, "%s", json);