jihuayu commented on code in PR #3557:
URL: https://github.com/apache/kvrocks/pull/3557#discussion_r3701039947
##########
src/server/server.cc:
##########
@@ -1392,11 +1389,75 @@ 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;
+}
Review Comment:
<!-- devin-review-comment {"id":
"BUG_pr-review-job-e446a84cb0d64a7a826e45c7a08c225f_0002", "file_path":
"src/server/server.cc", "start_line": 1427, "end_line": 1448, "side": "RIGHT",
"based_on_repo_rules": false} -->
🟡 **Every INFO or latency query from an admin connection rebuilds a full
statistics snapshot from scratch**
A complete statistics object for every registered command is allocated and
zero-initialized (`AggregateNamespaceStats()` at
`src/server/server.cc:1427-1448`) on each admin-scoped INFO or latency request,
so routine monitoring polls do thousands of small allocations per call.
Impact: Frequent monitoring of the server does noticeably more CPU and
allocation work than before, and requesting several sections at once repeats
the whole computation.
<details>
<summary>Cost breakdown and duplicated work</summary>
`AggregateNamespaceStats()` calls `initCommandStats()`
(`src/server/server.cc:1392-1407`), which inserts an entry for every command in
`CommandTable::GetOriginal()` into two `std::map`s, and — when
`histogram-bucket-boundaries` is configured — heap-allocates `boundaries+1`
`std::atomic<uint64_t>` per command (≈250 commands × (N+1) `make_unique`
calls). It then sums every namespace's maps.
This runs once for `INFO stats` and again for `INFO commandstats`
(`src/server/server.cc:1452` and `src/server/server.cc:1485`), so `INFO all`
builds the aggregate twice, and `LATENCY HISTOGRAM`
(`src/commands/cmd_server.cc:1726-1728`) builds a third one. Caching the
aggregate per request, or summing directly into the output without
materializing a full `Stats`, would avoid this.
</details>
<!-- devin-review-badge-begin -->
<a href="https://app.devin.ai/review/apache/kvrocks/pull/3557";
target="_blank">
<picture>
<source media="(prefers-color-scheme: dark)"
srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1";>
<img
src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1";
alt="Open in Devin Review">
</picture>
</a>
<!-- devin-review-badge-end -->
---
*Was this helpful? React with 👍 or 👎 to provide feedback.*
##########
src/server/server.cc:
##########
@@ -1392,11 +1389,75 @@ 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;
+}
+
+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;
Review Comment:
<!-- devin-review-comment {"id":
"BUG_pr-review-job-e446a84cb0d64a7a826e45c7a08c225f_0001", "file_path":
"src/server/server.cc", "start_line": 1450, "end_line": 1453, "side": "RIGHT",
"based_on_repo_rules": false} -->
🟡 **Command statistics become per-database when Redis SELECT compatibility
mode is enabled**
Command counters are looked up by the connection's namespace
(`GetStatsInfo(ns)`/`GetCommandsStatsInfo(ns)` at
`src/server/server.cc:1450-1487`), and in SELECT-compatibility mode each
database is its own namespace, so a client that selected database 1 or higher
only sees the commands issued on that database instead of the whole server.
Impact: Monitoring tools that select a non-zero database get partial,
Redis-incompatible command statistics and latency histograms.
<details>
<summary>How database indexes become namespaces and split the stats</summary>
When `redis-databases > 0`, `CommandSelect` maps db 0 to `kDefaultNamespace`
and db N>0 to `"db" + N` and calls `conn->SetNamespace(ns)`
(`src/commands/cmd_server.cc:227-232`). `Server::GetInfo` passes
`conn->GetNamespace()` (`src/commands/cmd_server.cc:296`) down to
`GetStatsInfo`/`GetCommandsStatsInfo`, which now resolve a per-namespace
`Stats` unless the namespace is the default one. `CommandLatency::getHistogram`
(`src/commands/cmd_server.cc:1725-1729`) does the same. Consequently, after
`SELECT 3`, `INFO commandstats`, `total_commands_processed`,
`instantaneous_ops_per_sec` and `LATENCY HISTOGRAM` all report only db3's
activity, whereas real Redis reports these server-wide regardless of the
selected database.
A possible fix is to treat all `kDatabaseNamespacePrefix` namespaces (i.e.
when `redis_databases > 0`) as the aggregate/default view.
</details>
<details>
<summary>Prompt for agents</summary>
```
In SELECT-compatibility mode (config redis_databases > 0), Connection
namespaces are synthesized per database ("db1", "db2", ... with db0 mapped to
kDefaultNamespace, see CommandSelect in src/commands/cmd_server.cc). Because
Server::GetStatsInfo, Server::GetCommandsStatsInfo and
CommandLatency::getHistogram now select the stats source by namespace, a client
that issued SELECT 1 sees only that database's command counters, latency and
histograms, while real Redis reports them server-wide irrespective of the
selected DB. Consider resolving the aggregate view when
config_->redis_databases > 0 (or when the namespace has the
kDatabaseNamespacePrefix form), so SELECT-mode clients keep the server-wide
view.
```
</details>
<!-- devin-review-badge-begin -->
<a href="https://app.devin.ai/review/apache/kvrocks/pull/3557";
target="_blank">
<picture>
<source media="(prefers-color-scheme: dark)"
srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1";>
<img
src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1";
alt="Open in Devin Review">
</picture>
</a>
<!-- devin-review-badge-end -->
---
*Was this helpful? React with 👍 or 👎 to provide feedback.*
##########
src/commands/cmd_server.cc:
##########
@@ -1824,5 +1830,5 @@ REDIS_REGISTER_COMMANDS(
MakeCmdAttr<CommandSST>("sst", -3, "write exclusive admin", 1, 1, 1),
MakeCmdAttr<CommandFlushMemTable>("flushmemtable", -1, "exclusive write",
NO_KEY),
MakeCmdAttr<CommandFlushBlockCache>("flushblockcache", 1, "exclusive
write", NO_KEY),
- MakeCmdAttr<CommandLatency>("latency", -2, "read-only admin", NO_KEY), )
+ MakeCmdAttr<CommandLatency>("latency", -2, "read-only", NO_KEY), )
Review Comment:
<!-- devin-review-comment {"id":
"SEC_pr-review-job-e446a84cb0d64a7a826e45c7a08c225f_0001", "file_path":
"src/commands/cmd_server.cc", "start_line": 1833, "end_line": 1833, "side":
"RIGHT"} -->
🟨 **LATENCY command is no longer admin-only, exposing per-namespace latency
data to any authenticated user**
The `admin` flag was removed from the LATENCY command registration
(`src/commands/cmd_server.cc:1833`), so any authenticated namespace user can
now run `LATENCY HISTOGRAM` and `LATENCY RESET`. The histogram is scoped to the
caller's namespace (`src/commands/cmd_server.cc:1725-1729`), so cross-namespace
data is not leaked to non-default users, but this is still a widening of the
command's access control surface that was previously restricted to
administrators.
<!-- devin-review-badge-begin -->
<a href="https://app.devin.ai/review/apache/kvrocks/pull/3557";
target="_blank">
<picture>
<source media="(prefers-color-scheme: dark)"
srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1";>
<img
src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1";
alt="Open in Devin Review">
</picture>
</a>
<!-- devin-review-badge-end -->
---
*Was this helpful? React with 👍 or 👎 to provide feedback.*
--
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]
![https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"](https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"){kind=link}
![https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1"](https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1"){kind=link}