From 06b097691e08851bd63c23dba5b538738187129a Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Sun, 12 May 2024 10:54:15 -0700 Subject: [PATCH] [logging] Add a helper to drop a list of variables out in a LOG statement (#36554) Not the fastest implementation possible, but it's a log helper so I'm not particularly fussed either -- but a useful utility that we can iterate on later to help debugging. Closes #36554 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/36554 from ctiller:args 33b4802fccb1a667ab81c1e2b2e1c5a55c993119 PiperOrigin-RevId: 632997320 --- CMakeLists.txt | 36 +++++++++++++++ build_autogenerated.yaml | 14 ++++++ src/core/BUILD | 19 ++++++++ src/core/lib/gprpp/dump_args.cc | 54 ++++++++++++++++++++++ src/core/lib/gprpp/dump_args.h | 69 ++++++++++++++++++++++++++++ test/core/gprpp/BUILD | 14 ++++++ test/core/gprpp/dump_args_test.cc | 44 ++++++++++++++++++ tools/distrib/fix_build_deps.py | 9 ++-- tools/run_tests/generated/tests.json | 24 ++++++++++ 9 files changed, 280 insertions(+), 3 deletions(-) create mode 100644 src/core/lib/gprpp/dump_args.cc create mode 100644 src/core/lib/gprpp/dump_args.h create mode 100644 test/core/gprpp/dump_args_test.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index 449d17ec767..1b107dddb0f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1052,6 +1052,7 @@ if(gRPC_BUILD_TESTS) if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_MAC OR _gRPC_PLATFORM_POSIX) add_dependencies(buildtests_cxx dualstack_socket_test) endif() + add_dependencies(buildtests_cxx dump_args_test) add_dependencies(buildtests_cxx duplicate_header_bad_client_test) add_dependencies(buildtests_cxx empty_batch_test) if(_gRPC_PLATFORM_LINUX OR _gRPC_PLATFORM_POSIX) @@ -12390,6 +12391,41 @@ endif() endif() if(gRPC_BUILD_TESTS) +add_executable(dump_args_test + src/core/lib/gprpp/dump_args.cc + test/core/gprpp/dump_args_test.cc +) +target_compile_features(dump_args_test PUBLIC cxx_std_14) +target_include_directories(dump_args_test + PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR} + ${CMAKE_CURRENT_SOURCE_DIR}/include + ${_gRPC_ADDRESS_SORTING_INCLUDE_DIR} + ${_gRPC_RE2_INCLUDE_DIR} + ${_gRPC_SSL_INCLUDE_DIR} + ${_gRPC_UPB_GENERATED_DIR} + ${_gRPC_UPB_GRPC_GENERATED_DIR} + ${_gRPC_UPB_INCLUDE_DIR} + ${_gRPC_XXHASH_INCLUDE_DIR} + ${_gRPC_ZLIB_INCLUDE_DIR} + third_party/googletest/googletest/include + third_party/googletest/googletest + third_party/googletest/googlemock/include + third_party/googletest/googlemock + ${_gRPC_PROTO_GENS_DIR} +) + +target_link_libraries(dump_args_test + ${_gRPC_ALLTARGETS_LIBRARIES} + gtest + absl::any_invocable + absl::check +) + + +endif() +if(gRPC_BUILD_TESTS) + add_executable(duplicate_header_bad_client_test test/core/bad_client/bad_client.cc test/core/bad_client/tests/duplicate_header.cc diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index d15f905f347..7458cc04b7f 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -8661,6 +8661,20 @@ targets: - linux - posix - mac +- name: dump_args_test + gtest: true + build: test + language: c++ + headers: + - src/core/lib/gprpp/dump_args.h + src: + - src/core/lib/gprpp/dump_args.cc + - test/core/gprpp/dump_args_test.cc + deps: + - gtest + - absl/functional:any_invocable + - absl/log:check + uses_polling: false - name: duplicate_header_bad_client_test gtest: true build: test diff --git a/src/core/BUILD b/src/core/BUILD index 87225db6d7c..b15bf14f933 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -48,6 +48,25 @@ grpc_cc_library( language = "c++", ) +grpc_cc_library( + name = "dump_args", + srcs = [ + "lib/gprpp/dump_args.cc", + ], + hdrs = [ + "lib/gprpp/dump_args.h", + ], + external_deps = [ + "absl/functional:any_invocable", + "absl/log:check", + "absl/strings", + ], + language = "c++", + deps = [ + "//:gpr_platform", + ], +) + grpc_cc_library( name = "slice_cast", hdrs = [ diff --git a/src/core/lib/gprpp/dump_args.cc b/src/core/lib/gprpp/dump_args.cc new file mode 100644 index 00000000000..e5bc183246b --- /dev/null +++ b/src/core/lib/gprpp/dump_args.cc @@ -0,0 +1,54 @@ +// Copyright 2024 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 "src/core/lib/gprpp/dump_args.h" + +#include "absl/log/check.h" +#include "absl/strings/ascii.h" +#include "absl/strings/string_view.h" + +namespace grpc_core { +namespace dump_args_detail { + +std::ostream& operator<<(std::ostream& out, const DumpArgs& args) { + // Parse the argument string into a vector of keys. + // #__VA_ARGS__ produces a stringified version of the arguments passed to the + // macro. It's comma separated, and we can use that to split the string into + // keys. Those keys might include parenthesis for e.g. argument lists, and so + // we need to skip commas that are inside parenthesis. + std::vector keys; + int depth = 0; + const char* start = args.arg_string_; + for (const char* p = args.arg_string_; *p; ++p) { + if (*p == '(') { + ++depth; + } else if (*p == ')') { + --depth; + } else if (*p == ',' && depth == 0) { + keys.push_back(absl::string_view(start, p - start)); + start = p + 1; + } + } + keys.push_back(start); + CHECK_EQ(keys.size(), args.arg_dumpers_.size()); + for (size_t i = 0; i < keys.size(); i++) { + if (i != 0) out << ", "; + out << absl::StripAsciiWhitespace(keys[i]) << " = "; + args.arg_dumpers_[i](out); + } + return out; +} + +} // namespace dump_args_detail +} // namespace grpc_core \ No newline at end of file diff --git a/src/core/lib/gprpp/dump_args.h b/src/core/lib/gprpp/dump_args.h new file mode 100644 index 00000000000..c2b66ce2be3 --- /dev/null +++ b/src/core/lib/gprpp/dump_args.h @@ -0,0 +1,69 @@ +// Copyright 2024 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_SRC_CORE_LIB_GPRPP_DUMP_ARGS_H +#define GRPC_SRC_CORE_LIB_GPRPP_DUMP_ARGS_H + +#include +#include + +#include "absl/functional/any_invocable.h" + +namespace grpc_core { +namespace dump_args_detail { + +// Helper function... just ignore the initializer list passed into it. +// Allows doing 'statements' via parameter pack expansion in C++11 - given +// template : +// do_these_things({foo()...}); +// will execute foo() for each T in Ts. +template +void do_these_things(std::initializer_list) {} + +class DumpArgs { + public: + template + explicit DumpArgs(const char* arg_string, const Args&... args) + : arg_string_(arg_string) { + do_these_things( + {AddDumper([a = &args](std::ostream& os) { os << *a; })...}); + } + + friend std::ostream& operator<<(std::ostream& out, const DumpArgs& args); + + private: + int AddDumper(absl::AnyInvocable dumper) { + arg_dumpers_.push_back(std::move(dumper)); + return 0; + } + + const char* arg_string_; + std::vector> arg_dumpers_; +}; + +} // namespace dump_args_detail +} // namespace grpc_core + +// Helper to print a list of variables and their values. +// Each type must be streamable to std::ostream. +// Usage: +// int a = 1; +// int b = 2; +// LOG(INFO) << GRPC_DUMP_ARGS(a, b) +// Output: +// a = 1, b = 2 +#define GRPC_DUMP_ARGS(...) \ + grpc_core::dump_args_detail::DumpArgs(#__VA_ARGS__, __VA_ARGS__) + +#endif // GRPC_SRC_CORE_LIB_GPRPP_DUMP_ARGS_H diff --git a/test/core/gprpp/BUILD b/test/core/gprpp/BUILD index b6ca769f46d..177c5809b0c 100644 --- a/test/core/gprpp/BUILD +++ b/test/core/gprpp/BUILD @@ -60,6 +60,20 @@ grpc_cc_test( ], ) +grpc_cc_test( + name = "dump_args_test", + srcs = ["dump_args_test.cc"], + external_deps = [ + "gtest", + ], + language = "C++", + uses_event_engine = False, + uses_polling = False, + deps = [ + "//src/core:dump_args", + ], +) + # TODO(hork): solidify fork support requirements for EventEngines grpc_cc_test( name = "fork_test", diff --git a/test/core/gprpp/dump_args_test.cc b/test/core/gprpp/dump_args_test.cc new file mode 100644 index 00000000000..2207d4eab62 --- /dev/null +++ b/test/core/gprpp/dump_args_test.cc @@ -0,0 +1,44 @@ +// Copyright 2024 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 "src/core/lib/gprpp/dump_args.h" + +#include + +#include "gtest/gtest.h" + +template +std::string Stringify(const T& t) { + std::ostringstream oss; + oss << t; + return oss.str(); +} + +int add(int a, int b) { return a + b; } + +TEST(DumpArgsTest, Basic) { + int a = 1; + int b = 2; + int c = 3; + EXPECT_EQ("a = 1, b = 2, c = 3", Stringify(GRPC_DUMP_ARGS(a, b, c))); +} + +TEST(DumpArgsTest, FunctionCall) { + EXPECT_EQ("add(1, 2) = 3", Stringify(GRPC_DUMP_ARGS(add(1, 2)))); +} + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/tools/distrib/fix_build_deps.py b/tools/distrib/fix_build_deps.py index 03dbcc9137b..0a114910e3e 100755 --- a/tools/distrib/fix_build_deps.py +++ b/tools/distrib/fix_build_deps.py @@ -64,6 +64,7 @@ EXTERNAL_DEPS = { "absl/functional/bind_front.h": "absl/functional:bind_front", "absl/functional/function_ref.h": "absl/functional:function_ref", "absl/hash/hash.h": "absl/hash", + "absl/log/check.h": "absl/log:check", "absl/memory/memory.h": "absl/memory", "absl/meta/type_traits.h": "absl/meta:type_traits", "absl/numeric/int128.h": "absl/numeric:int128", @@ -543,9 +544,11 @@ def make_library(library): # once EventEngine lands we can clean this up deps = Choices( library, - {"//:grpc_base": ["//:grpc", "//:grpc_unsecure"]} - if library.startswith("//test/") - else {}, + ( + {"//:grpc_base": ["//:grpc", "//:grpc_unsecure"]} + if library.startswith("//test/") + else {} + ), ) external_deps = Choices(None, {}) for hdr in hdrs: diff --git a/tools/run_tests/generated/tests.json b/tools/run_tests/generated/tests.json index 61b98ca9bd1..a733817a370 100644 --- a/tools/run_tests/generated/tests.json +++ b/tools/run_tests/generated/tests.json @@ -2969,6 +2969,30 @@ ], "uses_polling": true }, + { + "args": [], + "benchmark": false, + "ci_platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "gtest": true, + "language": "c++", + "name": "dump_args_test", + "platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "uses_polling": false + }, { "args": [], "benchmark": false,