acelyc111 commented on code in PR #1219:
URL:
https://github.com/apache/incubator-pegasus/pull/1219#discussion_r1010235148
##########
src/utils/metrics.h:
##########
@@ -393,6 +397,27 @@ class gauge : public metric
value_type value() const { return _value.load(std::memory_order_relaxed); }
+ // The snapshot collected has following json format:
+ // {
+ // "name": "<metric_name>",
+ // "value": ...
+ // }
+ // where "name" is the name of the gauge in string type, and "value" is
just current value
+ // of the gauge fetched by `value()`, in numeric types (i.e. integral or
floating-point type,
+ // determined by `value_type`).
+ virtual void take_snapshot(json::JsonWriter &writer) override
Review Comment:
better to omit `virtual` coz `override` occured.
##########
src/utils/metrics.h:
##########
@@ -493,6 +518,26 @@ class counter : public metric
return _adder.fetch_and_reset();
}
+ // The snapshot collected has following json format:
+ // {
+ // "name": "<metric_name>",
+ // "value": ...
+ // }
+ // where "name" is the name of the counter in string type, and "value" is
just current value
+ // of the counter fetched by `value()`, in integral type (namely int64_t).
+ virtual void take_snapshot(json::JsonWriter &writer) override
Review Comment:
better to omit `virtual` coz `override` occured.
##########
src/utils/metrics.h:
##########
@@ -559,6 +604,7 @@ ENUM_REG(kth_percentile_type::P999)
ENUM_END(kth_percentile_type)
const std::vector<double> kKthDecimals = {0.5, 0.9, 0.95, 0.99, 0.999};
+const std::vector<std::string> kKthNames = {"p50", "p90", "p95", "p99",
"p999"};
Review Comment:
how about make them as a map or something, then we are sure that they are in
pairs.
##########
src/utils/test/metrics_test.cpp:
##########
@@ -903,4 +906,254 @@ TEST(metrics_test, percentile_double)
floating_checker<value_type>>(METRIC_test_percentile_double);
}
+std::string take_snapshot_and_get_json_string(metric *m)
+{
+ std::stringstream out;
+ rapidjson::OStreamWrapper wrapper(out);
+ json::JsonWriter writer(wrapper);
+
+ m->take_snapshot(writer);
+
+ return out.str();
+}
+
+template <typename T, typename = typename
std::enable_if<std::is_arithmetic<T>::value>::type>
+using metric_value_map = std::map<std::string, T>;
+
+template <typename T, typename = typename
std::enable_if<std::is_arithmetic<T>::value>::type>
+void check_and_extract_metric_value_map_from_json_string(const std::string
&json_string,
+ const std::string
&metric_name,
+ const bool
is_integral,
+ metric_value_map<T>
&value_map)
+{
+ rapidjson::Document doc;
+ rapidjson::ParseResult result = doc.Parse(json_string.c_str());
+ ASSERT_FALSE(result.IsError());
+
+ ASSERT_TRUE(doc.IsObject());
+ for (const auto &elem : doc.GetObject()) {
+ ASSERT_TRUE(elem.name.IsString());
+
+ if (elem.value.IsString()) {
+ ASSERT_STREQ(elem.name.GetString(), "name");
+
+ ASSERT_STREQ(elem.value.GetString(), metric_name.c_str());
+ } else {
+ T value;
+ if (is_integral) {
+ ASSERT_TRUE(elem.value.IsInt64());
+ value = elem.value.GetInt64();
+ } else {
+ ASSERT_TRUE(elem.value.IsDouble());
+ value = elem.value.GetDouble();
+ }
+ value_map[elem.name.GetString()] = value;
+ }
+ }
+}
+
+template <typename T, typename = typename
std::enable_if<std::is_arithmetic<T>::value>::type>
+void generate_metric_value_map(metric *my_metric,
+ const bool is_integral,
+ metric_value_map<T> &value_map)
+{
+ auto json_string = take_snapshot_and_get_json_string(my_metric);
+ check_and_extract_metric_value_map_from_json_string(
+ json_string, my_metric->prototype()->name().data(), is_integral,
value_map);
+}
+
+template <typename T, typename = typename
std::enable_if<std::is_integral<T>::value>::type>
+void compare_integral_metric_value_map(const metric_value_map<T>
&actual_value_map,
+ const metric_value_map<T>
&expected_value_map)
+{
+ ASSERT_EQ(actual_value_map, expected_value_map);
+}
+
+template <typename T, typename = typename
std::enable_if<std::is_floating_point<T>::value>::type>
+void compare_floating_metric_value_map(const metric_value_map<T>
&actual_value_map,
+ const metric_value_map<T>
&expected_value_map)
+{
+ ASSERT_EQ(actual_value_map.size(), expected_value_map.size());
+
+ typename metric_value_map<T>::const_iterator actual_iter =
actual_value_map.begin();
+ typename metric_value_map<T>::const_iterator expected_iter =
expected_value_map.begin();
+ for (; actual_iter != actual_value_map.end() && expected_iter !=
expected_value_map.end();
+ ++actual_iter, ++expected_iter) {
+ ASSERT_EQ(actual_iter->first, expected_iter->first);
+ ASSERT_DOUBLE_EQ(actual_iter->second, expected_iter->second);
+ }
+}
+
+#define test_metric_snapshot_with_single_value(
\
Review Comment:
How about use all uppper case for macros?
##########
src/utils/test/metrics_test.cpp:
##########
@@ -903,4 +906,254 @@ TEST(metrics_test, percentile_double)
floating_checker<value_type>>(METRIC_test_percentile_double);
}
+std::string take_snapshot_and_get_json_string(metric *m)
+{
+ std::stringstream out;
+ rapidjson::OStreamWrapper wrapper(out);
+ json::JsonWriter writer(wrapper);
+
+ m->take_snapshot(writer);
+
+ return out.str();
+}
+
+template <typename T, typename = typename
std::enable_if<std::is_arithmetic<T>::value>::type>
+using metric_value_map = std::map<std::string, T>;
+
+template <typename T, typename = typename
std::enable_if<std::is_arithmetic<T>::value>::type>
+void check_and_extract_metric_value_map_from_json_string(const std::string
&json_string,
+ const std::string
&metric_name,
+ const bool
is_integral,
+ metric_value_map<T>
&value_map)
+{
+ rapidjson::Document doc;
+ rapidjson::ParseResult result = doc.Parse(json_string.c_str());
+ ASSERT_FALSE(result.IsError());
+
+ ASSERT_TRUE(doc.IsObject());
+ for (const auto &elem : doc.GetObject()) {
+ ASSERT_TRUE(elem.name.IsString());
+
+ if (elem.value.IsString()) {
+ ASSERT_STREQ(elem.name.GetString(), "name");
Review Comment:
isn't it always string?
##########
src/utils/test/metrics_test.cpp:
##########
@@ -903,4 +906,254 @@ TEST(metrics_test, percentile_double)
floating_checker<value_type>>(METRIC_test_percentile_double);
}
+std::string take_snapshot_and_get_json_string(metric *m)
+{
+ std::stringstream out;
+ rapidjson::OStreamWrapper wrapper(out);
+ json::JsonWriter writer(wrapper);
+
+ m->take_snapshot(writer);
+
+ return out.str();
+}
+
+template <typename T, typename = typename
std::enable_if<std::is_arithmetic<T>::value>::type>
+using metric_value_map = std::map<std::string, T>;
+
+template <typename T, typename = typename
std::enable_if<std::is_arithmetic<T>::value>::type>
+void check_and_extract_metric_value_map_from_json_string(const std::string
&json_string,
+ const std::string
&metric_name,
+ const bool
is_integral,
+ metric_value_map<T>
&value_map)
+{
+ rapidjson::Document doc;
+ rapidjson::ParseResult result = doc.Parse(json_string.c_str());
+ ASSERT_FALSE(result.IsError());
+
+ ASSERT_TRUE(doc.IsObject());
+ for (const auto &elem : doc.GetObject()) {
+ ASSERT_TRUE(elem.name.IsString());
+
+ if (elem.value.IsString()) {
+ ASSERT_STREQ(elem.name.GetString(), "name");
+
+ ASSERT_STREQ(elem.value.GetString(), metric_name.c_str());
+ } else {
+ T value;
+ if (is_integral) {
+ ASSERT_TRUE(elem.value.IsInt64());
+ value = elem.value.GetInt64();
+ } else {
+ ASSERT_TRUE(elem.value.IsDouble());
+ value = elem.value.GetDouble();
+ }
+ value_map[elem.name.GetString()] = value;
+ }
+ }
+}
+
+template <typename T, typename = typename
std::enable_if<std::is_arithmetic<T>::value>::type>
+void generate_metric_value_map(metric *my_metric,
+ const bool is_integral,
+ metric_value_map<T> &value_map)
+{
+ auto json_string = take_snapshot_and_get_json_string(my_metric);
+ check_and_extract_metric_value_map_from_json_string(
+ json_string, my_metric->prototype()->name().data(), is_integral,
value_map);
+}
+
+template <typename T, typename = typename
std::enable_if<std::is_integral<T>::value>::type>
+void compare_integral_metric_value_map(const metric_value_map<T>
&actual_value_map,
+ const metric_value_map<T>
&expected_value_map)
+{
+ ASSERT_EQ(actual_value_map, expected_value_map);
+}
+
+template <typename T, typename = typename
std::enable_if<std::is_floating_point<T>::value>::type>
+void compare_floating_metric_value_map(const metric_value_map<T>
&actual_value_map,
+ const metric_value_map<T>
&expected_value_map)
+{
+ ASSERT_EQ(actual_value_map.size(), expected_value_map.size());
+
+ typename metric_value_map<T>::const_iterator actual_iter =
actual_value_map.begin();
+ typename metric_value_map<T>::const_iterator expected_iter =
expected_value_map.begin();
Review Comment:
does use `auto` work?
##########
src/utils/metrics.h:
##########
@@ -662,10 +715,40 @@ class percentile : public closeable_metric
const auto index = static_cast<size_t>(type);
CHECK_LT(index, static_cast<size_t>(kth_percentile_type::COUNT));
- val = _full_nth_elements[index].load(std::memory_order_relaxed);
+ val = value(index);
return _kth_percentile_bitset.test(index);
}
+ // The snapshot collected has following json format:
+ // {
+ // "name": "<metric_name>",
+ // "p50": ...,
+ // "p90": ...,
+ // "p95": ...,
+ // ...
+ // }
+ // where "name" is the name of the percentile in string type, with each
configured kth
+ // percentile followed, such as "p50", "p90", "p95", etc. All of them are
in numeric types
+ // (i.e. integral or floating-point type, determined by `value_type`).
+ virtual void take_snapshot(json::JsonWriter &writer) override
Review Comment:
better to omit `virtual` coz `override` occured.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
