From bc94565486f51cf98f268459c1258b17766b8e88 Mon Sep 17 00:00:00 2001 From: Dan Wang Date: Wed, 31 May 2023 22:06:13 +0800 Subject: [PATCH] refactor(new_metrics): refactor enum definition for metric types and units (#1491) https://github.com/apache/incubator-pegasus/issues/1490 For the enum classes, such as metric type and unit, in addition to the declaration of all enumerators, extra code is needed to implement `enum_to_string` for each enumerator. However, the "extra code" tended to be forgotten, just as what has been said in the above issue #1490. Therefore, we should find a way that could share all the enumerators between the declaration and `enum_to_string`. Each enumerator would be written only once, and there is no need to remember to add `enum_to_string` for each enumerator. We could implement the "sharing" by function-like macros. They have the names of all the enumerators, and accept an argument to perform custom actions over the names to implement the "sharing". Both metric types and units would be refactored in this way. While building ASAN Github actions failed due to running out of disk space. This problem is resolved by dropping all directories of `CMakeFiles`, which is also tracked another issue: https://github.com/apache/incubator-pegasus/issues/1497 --- src/runtime/task/task_queue.cpp | 2 +- src/utils/enum_helper.h | 63 ++++++++- src/utils/metrics.h | 107 +++++++------- src/utils/test/enum_helper_test.cpp | 212 ++++++++++++++++++++++++++++ 4 files changed, 328 insertions(+), 56 deletions(-) create mode 100644 src/utils/test/enum_helper_test.cpp diff --git a/src/runtime/task/task_queue.cpp b/src/runtime/task/task_queue.cpp index 19de1954c2..425880909c 100644 --- a/src/runtime/task/task_queue.cpp +++ b/src/runtime/task/task_queue.cpp @@ -55,7 +55,7 @@ METRIC_DEFINE_counter(queue, METRIC_DEFINE_counter(queue, queue_rejected_tasks, dsn::metric_unit::kTasks, - "The accumulative number of rejeced tasks by throttling before enqueue"); + "The accumulative number of rejected tasks by throttling before enqueue"); namespace dsn { diff --git a/src/utils/enum_helper.h b/src/utils/enum_helper.h index 25d78b9094..fd9ba764f1 100644 --- a/src/utils/enum_helper.h +++ b/src/utils/enum_helper.h @@ -36,9 +36,14 @@ #pragma once #include -#include -#include #include +#include +#include + +namespace dsn { +template +class enum_helper_xxx; +} // namespace dsn // an invalid enum value must be provided so as to be the default value when parsing failed #define ENUM_BEGIN2(type, name, invalid_value) \ @@ -52,6 +57,8 @@ #define ENUM_REG_WITH_CUSTOM_NAME(type, name) helper->register_enum(#name, type); #define ENUM_REG(e) helper->register_enum(#e, e); +// Argument `type invalid_value` for enum_from_string, albeit unused, has to be provided due to +// overloading. #define ENUM_END2(type, name) \ return helper; \ } \ @@ -66,6 +73,58 @@ #define ENUM_END(type) ENUM_END2(type, type) +// Google Style (https://google.github.io/styleguide/cppguide.html#Enumerator_Names) has recommended +// using k-prefixed camelCase to name an enumerator instead of MACRO_CASE. That is, use kEnumName, +// rather than ENUM_NAME. +// +// On the other hand, the string representation for an enumerator is often needed. After declaring +// the enumerator, ENUM_REG* macros would also be used to register the string representation, which +// has some drawbacks: +// * the enumerator has to appear again, leading to redundant code; +// * once there are numerous enumerators, ENUM_REG* tend to be forgotten and the registers for the +// string representation would be missing. +// +// To solve these problems, ENUM_CONST* macros are introduced: +// * firstly, naming for the string representation should be UpperCamelCase style (namely EnumName); +// * ENUM_CONST() could be used to generate the enumerators according to the string representation; +// * only support `enum class` declarations; +// * ENUM_CONST_DEF() could be used to declare enumerators in the enum class; +// * ENUM_CONST_REG_STR could be used to register the string representation. +// +// The usage of these macros would be described as below. For example, Status::code of rocksdb +// (include/rocksdb/status.h) could be defined by ENUM_CONST* as following steps: +// +// 1. List string representation for each enumerator by a user-defined macro: +// -------------------------------------------------------------------------- +/* + * #define ENUM_FOREACH_STATUS_CODE(DEF) \ + * DEF(Ok) \ + * DEF(NotFound) \ + * DEF(Corruption) \ + * DEF(IOError) + */ +// 2. Declare an enum class by above user-defined macro, with an Invalid and an Count enumerator if +// necessary: +// ------------------------------------------------------------------------------------------------ +// enum class status_code +// { +// ENUM_FOREACH_STATUS_CODE(ENUM_CONST_DEF) kCount, kInvalidCode, +// }; +// +// 3. Define another user-defined macro to register string representations: +// ------------------------------------------------------------------------ +// #define ENUM_CONST_REG_STR_STATUS_CODE(str) ENUM_CONST_REG_STR(status_code, str) +// +// 4. Define enum helper class: +// ---------------------------- +// ENUM_BEGIN(status_code, status_code::kInvalidCode) +// ENUM_FOREACH_STATUS_CODE(ENUM_CONST_REG_STR_STATUS_CODE) +// ENUM_END(status_code) +#define ENUM_CONST(str) k##str +#define ENUM_CONST_DEF(str) ENUM_CONST(str), +#define ENUM_CONST_REG_STR(enum_class, str) \ + helper->register_enum(#str, enum_class::ENUM_CONST(str)); + namespace dsn { template diff --git a/src/utils/metrics.h b/src/utils/metrics.h index 789a5bac2b..b474538e96 100644 --- a/src/utils/metrics.h +++ b/src/utils/metrics.h @@ -644,62 +644,65 @@ class metric_registry : public utils::singleton // On the other hand, it is also needed when some special operation should be done // for a metric type. For example, percentile should be closed while it's no longer // used. +#define ENUM_FOREACH_METRIC_TYPE(DEF) \ + DEF(Gauge) \ + DEF(Counter) \ + DEF(VolatileCounter) \ + DEF(Percentile) + enum class metric_type { - kGauge, - kCounter, - kVolatileCounter, - kPercentile, - kInvalidUnit, + ENUM_FOREACH_METRIC_TYPE(ENUM_CONST_DEF) kInvalidType, }; -ENUM_BEGIN(metric_type, metric_type::kInvalidUnit) -ENUM_REG_WITH_CUSTOM_NAME(metric_type::kGauge, gauge) -ENUM_REG_WITH_CUSTOM_NAME(metric_type::kCounter, counter) -ENUM_REG_WITH_CUSTOM_NAME(metric_type::kVolatileCounter, volatile_counter) -ENUM_REG_WITH_CUSTOM_NAME(metric_type::kPercentile, percentile) +#define ENUM_CONST_REG_STR_METRIC_TYPE(str) ENUM_CONST_REG_STR(metric_type, str) + +ENUM_BEGIN(metric_type, metric_type::kInvalidType) +ENUM_FOREACH_METRIC_TYPE(ENUM_CONST_REG_STR_METRIC_TYPE) ENUM_END(metric_type) +#define ENUM_FOREACH_METRIC_UNIT(DEF) \ + DEF(NanoSeconds) \ + DEF(MicroSeconds) \ + DEF(MilliSeconds) \ + DEF(Seconds) \ + DEF(Bytes) \ + DEF(MegaBytes) \ + DEF(CapacityUnits) \ + DEF(Percent) \ + DEF(Replicas) \ + DEF(Partitions) \ + DEF(PartitionSplittings) \ + DEF(Servers) \ + DEF(Requests) \ + DEF(Responses) \ + DEF(Seeks) \ + DEF(PointLookups) \ + DEF(Values) \ + DEF(Keys) \ + DEF(Files) \ + DEF(Dirs) \ + DEF(Amplification) \ + DEF(Checkpoints) \ + DEF(Flushes) \ + DEF(Compactions) \ + DEF(Mutations) \ + DEF(Writes) \ + DEF(Changes) \ + DEF(Operations) \ + DEF(Tasks) \ + DEF(Disconnections) \ + DEF(Learns) \ + DEF(Rounds) \ + DEF(Resets) \ + DEF(Backups) \ + DEF(FileLoads) \ + DEF(FileUploads) \ + DEF(BulkLoads) + enum class metric_unit : size_t { - kNanoSeconds, - kMicroSeconds, - kMilliSeconds, - kSeconds, - kBytes, - kMegaBytes, - kCapacityUnits, - kPercent, - kReplicas, - kPartitions, - kPartitionSplittings, - kServers, - kRequests, - kResponses, - kSeeks, - kPointLookups, - kValues, - kKeys, - kFiles, - kDirs, - kAmplification, - kCheckpoints, - kFlushes, - kCompactions, - kMutations, - kWrites, - kChanges, - kOperations, - kTasks, - kDisconnections, - kLearns, - kRounds, - kResets, - kBackups, - kFileLoads, - kFileUploads, - kBulkLoads, - kInvalidUnit, + ENUM_FOREACH_METRIC_UNIT(ENUM_CONST_DEF) kInvalidUnit, }; #define METRIC_ASSERT_UNIT_LATENCY(unit, index) \ @@ -727,12 +730,10 @@ inline uint64_t convert_metric_latency_from_ns(uint64_t latency_ns, metric_unit return latency_ns / kMetricLatencyConverterFromNS[index]; } +#define ENUM_CONST_REG_STR_METRIC_UNIT(str) ENUM_CONST_REG_STR(metric_unit, str) + ENUM_BEGIN(metric_unit, metric_unit::kInvalidUnit) -ENUM_REG_WITH_CUSTOM_NAME(metric_unit::kNanoSeconds, nanoseconds) -ENUM_REG_WITH_CUSTOM_NAME(metric_unit::kMicroSeconds, microseconds) -ENUM_REG_WITH_CUSTOM_NAME(metric_unit::kMilliSeconds, milliseconds) -ENUM_REG_WITH_CUSTOM_NAME(metric_unit::kSeconds, seconds) -ENUM_REG_WITH_CUSTOM_NAME(metric_unit::kRequests, requests) +ENUM_FOREACH_METRIC_UNIT(ENUM_CONST_REG_STR_METRIC_UNIT) ENUM_END(metric_unit) class metric_prototype diff --git a/src/utils/test/enum_helper_test.cpp b/src/utils/test/enum_helper_test.cpp new file mode 100644 index 0000000000..ec2472ed26 --- /dev/null +++ b/src/utils/test/enum_helper_test.cpp @@ -0,0 +1,212 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you 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 "utils/enum_helper.h" + +// IWYU pragma: no_include +// IWYU pragma: no_include +// IWYU pragma: no_include +#include +#include + +namespace dsn { + +enum class command_type +{ + START, + RESTART, + STOP, + COUNT, + INVALID_TYPE, +}; + +ENUM_BEGIN(command_type, command_type::INVALID_TYPE) +ENUM_REG2(command_type, START) +ENUM_REG_WITH_CUSTOM_NAME(command_type::RESTART, restart) +ENUM_REG(command_type::STOP) +ENUM_END(command_type) + +using command_type_enum_from_string_case = std::tuple; + +class CommandTypeEnumFromStringTest + : public testing::TestWithParam +{ +}; + +TEST_P(CommandTypeEnumFromStringTest, EnumFromString) +{ + std::string str; + command_type expected_type; + std::tie(str, expected_type) = GetParam(); + + auto actual_type = enum_from_string(str.c_str(), command_type::INVALID_TYPE); + EXPECT_EQ(expected_type, actual_type); +} + +const std::vector command_type_enum_from_string_tests = { + {"START", command_type::START}, + {"Start", command_type::INVALID_TYPE}, + {"start", command_type::INVALID_TYPE}, + {"command_type::START", command_type::INVALID_TYPE}, + {"command_type::Start", command_type::INVALID_TYPE}, + {"command_type::start", command_type::INVALID_TYPE}, + {"RESTART", command_type::INVALID_TYPE}, + {"Restart", command_type::INVALID_TYPE}, + {"restart", command_type::RESTART}, + {"command_type::RESTART", command_type::INVALID_TYPE}, + {"command_type::Restart", command_type::INVALID_TYPE}, + {"command_type::restart", command_type::INVALID_TYPE}, + {"STOP", command_type::INVALID_TYPE}, + {"Stop", command_type::INVALID_TYPE}, + {"stop", command_type::INVALID_TYPE}, + {"command_type::STOP", command_type::STOP}, + {"command_type::Stop", command_type::INVALID_TYPE}, + {"command_type::stop", command_type::INVALID_TYPE}, + {"COUNT", command_type::INVALID_TYPE}, // Since COUNT was not registered with specified string + {"UNDEFINE_TYPE", command_type::INVALID_TYPE}, +}; + +INSTANTIATE_TEST_CASE_P(EnumHelperTest, + CommandTypeEnumFromStringTest, + testing::ValuesIn(command_type_enum_from_string_tests)); + +using command_type_enum_to_string_case = std::tuple; + +class CommandTypeEnumToStringTest : public testing::TestWithParam +{ +}; + +TEST_P(CommandTypeEnumToStringTest, EnumToString) +{ + command_type type; + std::string expected_str; + std::tie(type, expected_str) = GetParam(); + + std::string actual_str(enum_to_string(type)); + EXPECT_EQ(expected_str, actual_str); +} + +const std::vector command_type_enum_to_string_tests = { + {command_type::START, "START"}, + {command_type::RESTART, "restart"}, + {command_type::STOP, "command_type::STOP"}, + {command_type::COUNT, "Unknown"}, // Since COUNT was not registered with specified string + {command_type::INVALID_TYPE, "Unknown"}, +}; + +INSTANTIATE_TEST_CASE_P(EnumHelperTest, + CommandTypeEnumToStringTest, + testing::ValuesIn(command_type_enum_to_string_tests)); + +#define ENUM_FOREACH_STATUS_CODE(DEF) \ + DEF(Ok) \ + DEF(NotFound) \ + DEF(Corruption) \ + DEF(IOError) + +enum class status_code +{ + ENUM_FOREACH_STATUS_CODE(ENUM_CONST_DEF) kCount, + kInvalidCode, +}; + +#define ENUM_CONST_REG_STR_STATUS_CODE(str) ENUM_CONST_REG_STR(status_code, str) + +ENUM_BEGIN(status_code, status_code::kInvalidCode) +ENUM_FOREACH_STATUS_CODE(ENUM_CONST_REG_STR_STATUS_CODE) +ENUM_END(status_code) + +using status_code_enum_from_string_case = std::tuple; + +class StatusCodeEnumFromStringTest + : public testing::TestWithParam +{ +}; + +TEST_P(StatusCodeEnumFromStringTest, EnumFromString) +{ + std::string str; + status_code expected_code; + std::tie(str, expected_code) = GetParam(); + + auto actual_code = enum_from_string(str.c_str(), status_code::kInvalidCode); + EXPECT_EQ(expected_code, actual_code); +} + +const std::vector status_code_enum_from_string_tests = { + {"OK", status_code::kInvalidCode}, + {"Ok", status_code::kOk}, + {"ok", status_code::kInvalidCode}, + {"status_code::OK", status_code::kInvalidCode}, + {"status_code::Ok", status_code::kInvalidCode}, + {"status_code::ok", status_code::kInvalidCode}, + {"NOTFOUND", status_code::kInvalidCode}, + {"NotFound", status_code::kNotFound}, + {"notfound", status_code::kInvalidCode}, + {"status_code::NOTFOUND", status_code::kInvalidCode}, + {"status_code::NotFound", status_code::kInvalidCode}, + {"status_code::notfound", status_code::kInvalidCode}, + {"CORRUPTION", status_code::kInvalidCode}, + {"Corruption", status_code::kCorruption}, + {"corruption", status_code::kInvalidCode}, + {"status_code::CORRUPTION", status_code::kInvalidCode}, + {"status_code::Corruption", status_code::kInvalidCode}, + {"status_code::corruption", status_code::kInvalidCode}, + {"IOERROR", status_code::kInvalidCode}, + {"IOError", status_code::kIOError}, + {"ioerror", status_code::kInvalidCode}, + {"status_code::IOERROR", status_code::kInvalidCode}, + {"status_code::IOError", status_code::kInvalidCode}, + {"status_code::ioerror", status_code::kInvalidCode}, + {"Count", status_code::kInvalidCode}, // Since kCount was not registered with specified string + {"UndefinedCode", status_code::kInvalidCode}, +}; + +INSTANTIATE_TEST_CASE_P(EnumHelperTest, + StatusCodeEnumFromStringTest, + testing::ValuesIn(status_code_enum_from_string_tests)); + +using status_code_enum_to_string_case = std::tuple; + +class StatusCodeEnumToStringTest : public testing::TestWithParam +{ +}; + +TEST_P(StatusCodeEnumToStringTest, EnumToString) +{ + status_code code; + std::string expected_str; + std::tie(code, expected_str) = GetParam(); + + std::string actual_str(enum_to_string(code)); + EXPECT_EQ(expected_str, actual_str); +} + +const std::vector status_code_enum_to_string_tests = { + {status_code::kOk, "Ok"}, + {status_code::kNotFound, "NotFound"}, + {status_code::kCorruption, "Corruption"}, + {status_code::kIOError, "IOError"}, + {status_code::kCount, "Unknown"}, // Since kCount was not registered with specified string + {status_code::kInvalidCode, "Unknown"}, +}; + +INSTANTIATE_TEST_CASE_P(EnumHelperTest, + StatusCodeEnumToStringTest, + testing::ValuesIn(status_code_enum_to_string_tests)); + +} // namespace dsn