Copilot commented on code in PR #3557:
URL: https://github.com/apache/kvrocks/pull/3557#discussion_r3576921677
##########
src/server/server.cc:
##########
@@ -872,7 +858,18 @@ uint64_t Server::GetClientID() { return
client_id_.fetch_add(1, std::memory_orde
void Server::recordInstantaneousMetrics() {
auto rocksdb_stats = storage->GetDB()->GetDBOptions().statistics;
- stats.TrackInstantaneousMetric(STATS_METRIC_COMMAND, stats.total_calls);
+ // Sample each namespace's command metric, and feed the sum into the global
metric so the
+ // admin/default view reports aggregate ops/sec without keeping a global
command counter on the hot path.
+ uint64_t total_calls = 0;
+ {
+ std::shared_lock<std::shared_mutex> lock(ns_stats_mu_);
+ for (const auto &[ns, ns_stats] : ns_stats_) {
+ auto calls = ns_stats->total_calls.load();
+ ns_stats->TrackInstantaneousMetric(STATS_METRIC_COMMAND, calls);
+ total_calls += calls;
+ }
+ }
+ stats.TrackInstantaneousMetric(STATS_METRIC_COMMAND, total_calls);
Review Comment:
recordInstantaneousMetrics() now iterates over every namespace and sums
total_calls on every cron tick (~100ms). In deployments with many namespaces,
this adds O(namespace_count) work and a shared_mutex traversal to a very hot
periodic path. Consider restoring an always-updated global command counter
(e.g. keep srv_->stats.total_calls increment on the command hot path in
addition to per-namespace) so the cron sampler can remain O(1) for the
aggregate view, and (optionally) sample per-namespace ops/sec less aggressively
or on demand.
##########
src/server/server.cc:
##########
@@ -1392,11 +1389,82 @@ int64_t Server::GetLastBgsaveTime() {
return last_bgsave_timestamp_secs_ == -1 ? start_time_secs_ :
last_bgsave_timestamp_secs_;
}
-Server::InfoEntries Server::GetStatsInfo() {
+void Server::initCommandStats(Stats *stats) {
+ auto commands = redis::CommandTable::GetOriginal();
+ for (const auto &iter : *commands) {
+ stats->commands_stats[iter.first].calls = 0;
+ stats->commands_stats[iter.first].latency = 0;
+
+ if (stats->bucket_boundaries.size() > 0) {
+ // NB: Extra index for the last bucket (Inf)
+ for (std::size_t i{0}; i <= stats->bucket_boundaries.size(); ++i) {
+
stats->commands_histogram[iter.first].buckets.push_back(std::make_unique<std::atomic<uint64_t>>(0));
+ }
+ stats->commands_histogram[iter.first].calls = 0;
+ stats->commands_histogram[iter.first].sum = 0;
+ }
+ }
+}
+
+std::shared_ptr<Stats> Server::GetOrCreateNamespaceStats(const std::string
&ns) {
+ {
+ std::shared_lock<std::shared_mutex> lock(ns_stats_mu_);
+ if (auto it = ns_stats_.find(ns); it != ns_stats_.end()) {
+ return it->second;
+ }
+ }
+
+ std::unique_lock<std::shared_mutex> lock(ns_stats_mu_);
+ if (auto it = ns_stats_.find(ns); it != ns_stats_.end()) {
+ return it->second;
+ }
+ auto ns_stats =
std::make_shared<Stats>(config_->histogram_bucket_boundaries);
+ initCommandStats(ns_stats.get());
+ ns_stats_[ns] = ns_stats;
+ return ns_stats;
Review Comment:
GetOrCreateNamespaceStats() builds new Stats instances from
config_->histogram_bucket_boundaries. Since histogram-bucket-boundaries is a
runtime-configurable field, namespace Stats created before/after a CONFIG SET
can end up with different bucket counts, which later breaks
aggregation/printing assumptions. Consider constructing per-namespace Stats
from a single, stable bucket-boundary snapshot (e.g. the Server's global
stats.bucket_boundaries) so all namespaces share the same histogram shape.
##########
src/server/server.cc:
##########
@@ -1392,11 +1389,82 @@ int64_t Server::GetLastBgsaveTime() {
return last_bgsave_timestamp_secs_ == -1 ? start_time_secs_ :
last_bgsave_timestamp_secs_;
}
-Server::InfoEntries Server::GetStatsInfo() {
+void Server::initCommandStats(Stats *stats) {
+ auto commands = redis::CommandTable::GetOriginal();
+ for (const auto &iter : *commands) {
+ stats->commands_stats[iter.first].calls = 0;
+ stats->commands_stats[iter.first].latency = 0;
+
+ if (stats->bucket_boundaries.size() > 0) {
+ // NB: Extra index for the last bucket (Inf)
+ for (std::size_t i{0}; i <= stats->bucket_boundaries.size(); ++i) {
+
stats->commands_histogram[iter.first].buckets.push_back(std::make_unique<std::atomic<uint64_t>>(0));
+ }
+ stats->commands_histogram[iter.first].calls = 0;
+ stats->commands_histogram[iter.first].sum = 0;
+ }
+ }
+}
+
+std::shared_ptr<Stats> Server::GetOrCreateNamespaceStats(const std::string
&ns) {
+ {
+ std::shared_lock<std::shared_mutex> lock(ns_stats_mu_);
+ if (auto it = ns_stats_.find(ns); it != ns_stats_.end()) {
+ return it->second;
+ }
+ }
+
+ std::unique_lock<std::shared_mutex> lock(ns_stats_mu_);
+ if (auto it = ns_stats_.find(ns); it != ns_stats_.end()) {
+ return it->second;
+ }
+ auto ns_stats =
std::make_shared<Stats>(config_->histogram_bucket_boundaries);
+ initCommandStats(ns_stats.get());
+ ns_stats_[ns] = ns_stats;
+ return ns_stats;
+}
+
+std::shared_ptr<Stats> Server::AggregateNamespaceStats() {
+ auto agg = std::make_shared<Stats>(config_->histogram_bucket_boundaries);
+ initCommandStats(agg.get());
+
+ std::shared_lock<std::shared_mutex> lock(ns_stats_mu_);
+ for (const auto &[ns, ns_stats] : ns_stats_) {
+ agg->total_calls.fetch_add(ns_stats->total_calls.load(),
std::memory_order_relaxed);
+ for (const auto &[cmd, stat] : ns_stats->commands_stats) {
+ agg->commands_stats[cmd].calls.fetch_add(stat.calls.load(),
std::memory_order_relaxed);
+ agg->commands_stats[cmd].latency.fetch_add(stat.latency.load(),
std::memory_order_relaxed);
+ }
+ for (const auto &[cmd, hist] : ns_stats->commands_histogram) {
+ auto &agg_hist = agg->commands_histogram[cmd];
+ agg_hist.calls.fetch_add(hist.calls.load(), std::memory_order_relaxed);
+ agg_hist.sum.fetch_add(hist.sum.load(), std::memory_order_relaxed);
+ for (std::size_t i = 0; i < hist.buckets.size(); ++i) {
+ agg_hist.buckets[i]->fetch_add(hist.buckets[i]->load(),
std::memory_order_relaxed);
+ }
+ }
+ }
Review Comment:
AggregateNamespaceStats() assumes every namespace Stats has the same
histogram bucket count as the newly constructed aggregate Stats. If
histogram-bucket-boundaries was changed at runtime, hist.buckets.size() can
exceed agg_hist.buckets.size(), and agg_hist.buckets[i] will go out of bounds.
Guard the aggregation by iterating up to the min bucket count (and ideally
ensure all Stats share the same bucket-boundary snapshot).
##########
src/server/server.cc:
##########
@@ -1392,11 +1389,82 @@ int64_t Server::GetLastBgsaveTime() {
return last_bgsave_timestamp_secs_ == -1 ? start_time_secs_ :
last_bgsave_timestamp_secs_;
}
-Server::InfoEntries Server::GetStatsInfo() {
+void Server::initCommandStats(Stats *stats) {
+ auto commands = redis::CommandTable::GetOriginal();
+ for (const auto &iter : *commands) {
+ stats->commands_stats[iter.first].calls = 0;
+ stats->commands_stats[iter.first].latency = 0;
+
+ if (stats->bucket_boundaries.size() > 0) {
+ // NB: Extra index for the last bucket (Inf)
+ for (std::size_t i{0}; i <= stats->bucket_boundaries.size(); ++i) {
+
stats->commands_histogram[iter.first].buckets.push_back(std::make_unique<std::atomic<uint64_t>>(0));
+ }
+ stats->commands_histogram[iter.first].calls = 0;
+ stats->commands_histogram[iter.first].sum = 0;
+ }
+ }
+}
+
+std::shared_ptr<Stats> Server::GetOrCreateNamespaceStats(const std::string
&ns) {
+ {
+ std::shared_lock<std::shared_mutex> lock(ns_stats_mu_);
+ if (auto it = ns_stats_.find(ns); it != ns_stats_.end()) {
+ return it->second;
+ }
+ }
+
+ std::unique_lock<std::shared_mutex> lock(ns_stats_mu_);
+ if (auto it = ns_stats_.find(ns); it != ns_stats_.end()) {
+ return it->second;
+ }
+ auto ns_stats =
std::make_shared<Stats>(config_->histogram_bucket_boundaries);
+ initCommandStats(ns_stats.get());
+ ns_stats_[ns] = ns_stats;
+ return ns_stats;
+}
+
+std::shared_ptr<Stats> Server::AggregateNamespaceStats() {
+ auto agg = std::make_shared<Stats>(config_->histogram_bucket_boundaries);
+ initCommandStats(agg.get());
+
+ std::shared_lock<std::shared_mutex> lock(ns_stats_mu_);
+ for (const auto &[ns, ns_stats] : ns_stats_) {
+ agg->total_calls.fetch_add(ns_stats->total_calls.load(),
std::memory_order_relaxed);
+ for (const auto &[cmd, stat] : ns_stats->commands_stats) {
+ agg->commands_stats[cmd].calls.fetch_add(stat.calls.load(),
std::memory_order_relaxed);
+ agg->commands_stats[cmd].latency.fetch_add(stat.latency.load(),
std::memory_order_relaxed);
+ }
+ for (const auto &[cmd, hist] : ns_stats->commands_histogram) {
+ auto &agg_hist = agg->commands_histogram[cmd];
+ agg_hist.calls.fetch_add(hist.calls.load(), std::memory_order_relaxed);
+ agg_hist.sum.fetch_add(hist.sum.load(), std::memory_order_relaxed);
+ for (std::size_t i = 0; i < hist.buckets.size(); ++i) {
+ agg_hist.buckets[i]->fetch_add(hist.buckets[i]->load(),
std::memory_order_relaxed);
+ }
+ }
+ }
+ return agg;
+}
+
+void Server::ClearNamespaceStats(const std::string &ns) {
+ std::unique_lock<std::shared_mutex> lock(ns_stats_mu_);
+ ns_stats_.erase(ns);
+}
+
+Server::InfoEntries Server::GetStatsInfo(const std::string &ns) {
+ // Command stats are per namespace; the admin/default namespace sees the
aggregate across all of them.
+ auto cmd_stats_ptr = ns == kDefaultNamespace ? AggregateNamespaceStats() :
GetOrCreateNamespaceStats(ns);
+ const Stats &cmd_stats = *cmd_stats_ptr;
+
Server::InfoEntries entries;
entries.emplace_back("total_connections_received", total_clients_.load());
- entries.emplace_back("total_commands_processed", stats.total_calls.load());
- entries.emplace_back("instantaneous_ops_per_sec",
stats.GetInstantaneousMetric(STATS_METRIC_COMMAND));
+ entries.emplace_back("total_commands_processed",
cmd_stats.total_calls.load());
+ // Per-namespace ops/sec comes from the namespace's own sampled metric; the
admin/default view uses
+ // the global metric, which the sampler feeds with the sum across all
namespaces.
+ auto ops_per_sec = ns == kDefaultNamespace ?
stats.GetInstantaneousMetric(STATS_METRIC_COMMAND)
+ :
cmd_stats.GetInstantaneousMetric(STATS_METRIC_COMMAND);
+ entries.emplace_back("instantaneous_ops_per_sec", ops_per_sec);
Review Comment:
GetStatsInfo(kDefaultNamespace) currently calls AggregateNamespaceStats(),
which allocates and initializes per-command stats/histogram structures even
though this section only needs total_commands_processed (sum of total_calls)
plus the already-sampled global ops/sec. This can make frequent INFO monitoring
calls unexpectedly expensive. Consider summing total_calls directly under
ns_stats_mu_ for the default namespace instead of building an aggregated Stats
object.
--
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]
