[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1427: feat(new_metrics): add disk-level metric entity and migrate disk-level metrics for fs_manager

["acelyc111 (via GitHub)" <gi...@apache.org>]

acelyc111 commented on code in PR #1427:
URL: https://github.com/apache/incubator-pegasus/pull/1427#discussion_r1157457652


##########
src/common/fs_manager.cpp:
##########
@@ -108,6 +154,10 @@ bool dir_node::update_disk_stat(const bool update_disk_status)
     disk_available_ratio = static_cast<int>(
         disk_capacity_mb == 0 ? 0 : std::round(disk_available_mb * 100.0 / disk_capacity_mb));
 
+    METRIC_CALL_SET_METHOD(disk_capacity, total_disk_capacity_mb, disk_capacity_mb);
+    METRIC_CALL_SET_METHOD(disk_capacity, avail_disk_capacity_mb, disk_available_mb);
+    METRIC_CALL_SET_METHOD(disk_capacity, avail_disk_capacity_percentage, disk_available_ratio);

Review Comment:
   Now that `avail_disk_capacity_percentage` can be calculated from `total_disk_capacity_mb` and `avail_disk_capacity_mb`, why not leave it to the 3rd party monitor system to implement it?



##########
src/common/fs_manager.cpp:
##########
@@ -41,14 +41,31 @@
 
 #include "common/gpid.h"
 #include "common/replication_enums.h"
-#include "perf_counter/perf_counter.h"
+#include "fmt/core.h"
 #include "runtime/api_layer1.h"
 #include "runtime/rpc/rpc_address.h"
 #include "utils/fail_point.h"
 #include "utils/filesystem.h"
 #include "utils/fmt_logging.h"
 #include "utils/string_view.h"
 
+METRIC_DEFINE_entity(disk);
+
+METRIC_DEFINE_gauge_int64(disk,
+                          total_disk_capacity_mb,
+                          dsn::metric_unit::kMegaBytes,
+                          "The total disk capacity in MB");

Review Comment:
   `in MB` can be omitted since its unit is `kMegaBytes`?



##########
src/common/fs_manager.cpp:
##########
@@ -297,17 +321,29 @@ bool fs_manager::for_each_dir_node(const std::function<bool(const dir_node &)> &
 
 void fs_manager::update_disk_stat(bool check_status_changed)
 {
-    reset_disk_stat();
+    _total_capacity_mb = 0;
+    _total_available_mb = 0;

Review Comment:
   Is there any other place except this function use it? If not, how about define it as a local variable?



-- 
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: dev-unsubscribe@pegasus.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org