You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by "acelyc111 (via GitHub)" <gi...@apache.org> on 2023/02/18 16:52:34 UTC
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1351: feat(new_metrics): migrate replica-level metrics for write service
acelyc111 commented on code in PR #1351:
URL: https://github.com/apache/incubator-pegasus/pull/1351#discussion_r1111057278
##########
src/server/pegasus_write_service.h:
##########
@@ -199,28 +197,29 @@ class pegasus_write_service : dsn::replication::replica_base
capacity_unit_calculator *_cu_calculator;
int64_t _dup_lagging_write_threshold_ms;
- ::dsn::perf_counter_wrapper _pfc_put_qps;
- ::dsn::perf_counter_wrapper _pfc_multi_put_qps;
- ::dsn::perf_counter_wrapper _pfc_remove_qps;
- ::dsn::perf_counter_wrapper _pfc_multi_remove_qps;
- ::dsn::perf_counter_wrapper _pfc_incr_qps;
- ::dsn::perf_counter_wrapper _pfc_check_and_set_qps;
- ::dsn::perf_counter_wrapper _pfc_check_and_mutate_qps;
- ::dsn::perf_counter_wrapper _pfc_duplicate_qps;
- ::dsn::perf_counter_wrapper _pfc_dup_time_lag;
- ::dsn::perf_counter_wrapper _pfc_dup_lagging_writes;
-
- ::dsn::perf_counter_wrapper _pfc_put_latency;
- ::dsn::perf_counter_wrapper _pfc_multi_put_latency;
- ::dsn::perf_counter_wrapper _pfc_remove_latency;
- ::dsn::perf_counter_wrapper _pfc_multi_remove_latency;
- ::dsn::perf_counter_wrapper _pfc_incr_latency;
- ::dsn::perf_counter_wrapper _pfc_check_and_set_latency;
- ::dsn::perf_counter_wrapper _pfc_check_and_mutate_latency;
-
- // Records all requests.
- std::vector<::dsn::perf_counter *> _batch_qps_perfcounters;
- std::vector<::dsn::perf_counter *> _batch_latency_perfcounters;
+ dsn::counter_ptr<> _put_counter;
+ dsn::counter_ptr<> _multi_put_counter;
+ dsn::counter_ptr<> _remove_counter;
+ dsn::counter_ptr<> _multi_remove_counter;
+ dsn::counter_ptr<> _incr_counter;
+ dsn::counter_ptr<> _check_and_set_counter;
+ dsn::counter_ptr<> _check_and_mutate_counter;
+
+ dsn::percentile_ptr<int64_t> _put_latency_ns;
+ dsn::percentile_ptr<int64_t> _multi_put_latency_ns;
+ dsn::percentile_ptr<int64_t> _remove_latency_ns;
+ dsn::percentile_ptr<int64_t> _multi_remove_latency_ns;
+ dsn::percentile_ptr<int64_t> _incr_latency_ns;
+ dsn::percentile_ptr<int64_t> _check_and_set_latency_ns;
+ dsn::percentile_ptr<int64_t> _check_and_mutate_latency_ns;
+
+ dsn::counter_ptr<> _dup_counter;
+ dsn::percentile_ptr<int64_t> _dup_time_lag_ms;
+ dsn::counter_ptr<> _dup_lagging_write_counter;
+
+ // Record batch size for put and remove requests.
+ uint32_t _put_batch_size;
Review Comment:
nit: Use atomic?
##########
src/server/pegasus_write_service.cpp:
##########
@@ -36,111 +124,36 @@ pegasus_write_service::pegasus_write_service(pegasus_server_impl *server)
_server(server),
_impl(new impl(server)),
_batch_start_time(0),
- _cu_calculator(server->_cu_calculator.get())
+ _cu_calculator(server->_cu_calculator.get()),
+ _put_counter(METRIC_put_requests.instantiate(replica_metric_entity())),
Review Comment:
Will the counter_ptr/percentile_ptr get any error if not initialized in c'tor? It will be helpful to prevent missing initialized.
##########
src/server/pegasus_write_service.cpp:
##########
@@ -36,111 +124,36 @@ pegasus_write_service::pegasus_write_service(pegasus_server_impl *server)
_server(server),
_impl(new impl(server)),
_batch_start_time(0),
- _cu_calculator(server->_cu_calculator.get())
+ _cu_calculator(server->_cu_calculator.get()),
Review Comment:
How about use macros to format/simplify these codes? (Maybe the member variables and metrics should have some similar naming?)
##########
src/server/pegasus_write_service.cpp:
##########
@@ -296,15 +307,23 @@ void pegasus_write_service::set_default_ttl(uint32_t ttl) { _impl->set_default_t
void pegasus_write_service::clear_up_batch_states()
{
- uint64_t latency = dsn_now_ns() - _batch_start_time;
- for (dsn::perf_counter *pfc : _batch_qps_perfcounters)
- pfc->increment();
- for (dsn::perf_counter *pfc : _batch_latency_perfcounters)
- pfc->set(latency);
-
- _batch_qps_perfcounters.clear();
- _batch_latency_perfcounters.clear();
+#define PROCESS_WRITE_BATCH(op) \
+ do { \
+ _##op##_counter->increment_by(static_cast<int64_t>(_##op##_batch_size)); \
+ for (uint32_t i = 0; i < _##op##_batch_size; ++i) { \
+ _##op##_latency_ns->set(latency_ns); \
Review Comment:
How about encapsulate another function like `set(count, value)` for percentile metrics?
##########
src/utils/time_utils.h:
##########
@@ -126,5 +130,29 @@ inline int64_t hh_mm_today_to_unix_sec(string_view hhmm_of_day)
return get_unix_sec_today_midnight() + sec_of_day;
}
+class chronograph
+{
+public:
+ chronograph() : chronograph(dsn_now_ns()) {}
+ chronograph(uint64_t start_time_ns) : _start_time_ns(start_time_ns) {}
+ ~chronograph() = default;
+
+ inline void reset_start_time() { _start_time_ns = dsn_now_ns(); }
+
+ template <typename T, typename = typename std::enable_if<std::is_integral<T>::value>::type>
Review Comment:
How about set a default type `uint64_t` for `T`?
##########
src/server/pegasus_write_service.h:
##########
@@ -199,28 +197,29 @@ class pegasus_write_service : dsn::replication::replica_base
capacity_unit_calculator *_cu_calculator;
int64_t _dup_lagging_write_threshold_ms;
- ::dsn::perf_counter_wrapper _pfc_put_qps;
- ::dsn::perf_counter_wrapper _pfc_multi_put_qps;
- ::dsn::perf_counter_wrapper _pfc_remove_qps;
- ::dsn::perf_counter_wrapper _pfc_multi_remove_qps;
- ::dsn::perf_counter_wrapper _pfc_incr_qps;
- ::dsn::perf_counter_wrapper _pfc_check_and_set_qps;
- ::dsn::perf_counter_wrapper _pfc_check_and_mutate_qps;
- ::dsn::perf_counter_wrapper _pfc_duplicate_qps;
- ::dsn::perf_counter_wrapper _pfc_dup_time_lag;
- ::dsn::perf_counter_wrapper _pfc_dup_lagging_writes;
-
- ::dsn::perf_counter_wrapper _pfc_put_latency;
- ::dsn::perf_counter_wrapper _pfc_multi_put_latency;
- ::dsn::perf_counter_wrapper _pfc_remove_latency;
- ::dsn::perf_counter_wrapper _pfc_multi_remove_latency;
- ::dsn::perf_counter_wrapper _pfc_incr_latency;
- ::dsn::perf_counter_wrapper _pfc_check_and_set_latency;
- ::dsn::perf_counter_wrapper _pfc_check_and_mutate_latency;
-
- // Records all requests.
- std::vector<::dsn::perf_counter *> _batch_qps_perfcounters;
- std::vector<::dsn::perf_counter *> _batch_latency_perfcounters;
Review Comment:
Have we lost some metrics?
--
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