feat(info): report command statistics per namespace

[nhancdt2602]

Summary

This change tracks command calls, latency, and histograms per namespace. A namespaced connection sees its own numbers in INFO (Stats, CommandStats) and LATENCY HISTOGRAM; the admin/default namespace sees the aggregate across all namespaces.

Behavior:

• In namespace ns1, call INFO commandstats -> returns stats scoped by ns1 only
• In namespace admin, call INFO commandstats -> returns stats aggregated from all namespaces

Closes #755.

Changes

• Per-namespace Stats kept in ns_stats_ field in the Server class (guarded by a shared_mutex), created lazily via GetOrCreateNamespaceStats.
• Command hot path records into the connection's cached per-namespace Stats (no lock needed).
• GetStatsInfo, GetCommandsStatsInfo, LATENCY HISTOGRAM resolve their source by caller namespace, or AggregateNamespaceStats() for the admin/default view.
• Go integration test TestNamespaceStats.

Implementation notes

• Stats such as command counts, ops-per-sec, commandstats, hist I/O bytes and keyspace hits/misses stay global for now. They're counted where the namespace isn't in scope (request parsing / storage layer). Open for follow-ups.

Test

• Passed regression tests under unit/namespace module
• Added and passed tests in unit/info module

AI disclosure

• Human: outlined the behaviors, interfaces scaffolding and finalized approaches.
• AI: implementation and writing tests

[nhancdt2602]

@PragmaTwice I'd love to hear your thought about this change. Especially, some stats still stay global in this PR for the sack of simplicity.

[nhancdt2602]

@jihuayu Thanks for running the CI pipeline. The only failing test is:

--- FAIL: TestSlaveLostMaster (10.74s)
    client.go:99: forwarding tcp stream stopped
    client.go:147: forward tcp stream failed, err: read tcp 127.0.0.1:49515->127.0.0.1:49516: read: connection reset by peer
FAIL
exit status 1

This seems unrelated to the PR change. (as it was reported as flaky test in #3515).

Could you kindly re-run the pipeline again?

[Copilot]

Pull request overview

This PR adds per-namespace command statistics so that INFO (Stats/CommandStats) and LATENCY HISTOGRAM report metrics scoped to the caller’s namespace, while the default/admin namespace view returns an aggregate across all namespaces.

Changes:

• Introduces a per-namespace Stats map in Server, with lazy creation and aggregation for the default/admin namespace view.
• Records command call/latency stats into a connection-cached per-namespace Stats on the command execution hot path.
• Adds a Go integration test validating namespace scoping, aggregation, and namespace deletion behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/gocase/unit/info/info_test.go	Adds TestNamespaceStats to validate INFO/LATENCY stats scoping and aggregation semantics.
src/server/server.h	Extends Server APIs to accept namespace for Stats/CommandStats and adds per-namespace Stats storage/aggregation helpers.
src/server/server.cc	Implements per-namespace Stats creation, aggregation, and updates INFO sections to use namespace-aware sources.
src/server/redis_connection.h	Adds cached per-namespace Stats pointer on the connection object.
src/server/redis_connection.cc	Switches command stats recording to the connection’s cached per-namespace Stats.
src/commands/cmd_server.cc	Clears namespace stats on NAMESPACE DEL and makes LATENCY HISTOGRAM use namespace-aware histograms/aggregate.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jihuayu]

Hello @nhancdt2602.Thank you for your contribution. I think this PR still has a few issues.

1. latency command is admin only. It will make the namespace user could not get it's latency.

   kvrocks/src/commands/cmd_server.cc

   Line 1827 in 634ed15

   MakeCmdAttr<CommandLatency>("latency", -2, "read-only admin", NO_KEY), )

2. Deleting a namespace does not disconnect existing connections. This causes the old connections’ ns_stats_ references to diverge.

After authentication:
ns_stats_["ns1"] -> Stats A
Active connection -> Stats A

After deleting `ns1`:
ns_stats_ no longer contains ns1
Active connection -> Stats A  // kept alive by shared_ptr

After the old connection executes `INFO`:
ns_stats_["ns1"] -> newly created Stats B
Active connection -> Stats A

3. Deleting a namespace may cause an underflow here.
   

   kvrocks/src/stats/stats.cc

   Lines 107 to 117 in 8c26549

   	void Stats::TrackInstantaneousMetric(int metric, uint64_t current_reading) {
   	uint64_t curr_time_ms = util::GetTimeStampMS();
   	std::unique_lock<std::shared_mutex> lock(inst_metrics_mutex);
   	uint64_t t = curr_time_ms - inst_metrics[metric].last_sample_time_ms;
   	uint64_t ops = current_reading - inst_metrics[metric].last_sample_count;
   	uint64_t ops_sec = t > 0 ? (ops * 1000 / t) : 0;
   	inst_metrics[metric].samples[inst_metrics[metric].idx] = ops_sec;
   	inst_metrics[metric].idx++;
   	inst_metrics[metric].idx %= STATS_METRIC_SAMPLES;
   	inst_metrics[metric].last_sample_time_ms = curr_time_ms;
   	inst_metrics[metric].last_sample_count = current_reading;

[nhancdt2602]

@jihuayu Thanks for your reply. To fix points 2 and 3, can I keep the stats entry in the namespace map even after the namespace is deleted? This would be consistent with the current behavior of db_scan_infos_

[nhancdt2602]

@jihuayu Regarding point 1, I’m still not sure I understand your concern. Are you suggesting that we should not expose latency and histogram metrics for namespace-scoped connections?

[jihuayu]

@nhancdt2602 Sorry, I’m late.

@jihuayu Thanks for your reply. To fix points 2 and 3, can I keep the stats entry in the namespace map even after the namespace is deleted? This would be consistent with the current behavior of db_scan_infos_

I have no objection to that, since deleting a namespace is a low-frequency operation.

What I meant is that this command should not be restricted to ADMIN users. Namespace users are not ADMIN users, so they would be unable to retrieve latency information.

[nhancdt2602]

@nhancdt2602 Sorry, I’m late.

@jihuayu Thanks for your reply. To fix points 2 and 3, can I keep the stats entry in the namespace map even after the namespace is deleted? This would be consistent with the current behavior of db_scan_infos_

I have no objection to that, since deleting a namespace is a low-frequency operation.

What I meant is that this command should not be restricted to ADMIN users. Namespace users are not ADMIN users, so they would be unable to retrieve latency information.

@jihuayu Regarding point 1, LATENCY command already is scoped to ADMIN before this PR. If I'm not mistaken, you want to relax it by removing admin flag in LATENCY command config in this PR?

[nhancdt2602]

@jihuayu I've addressed all three points from your review. Looking for your feedback when you I have time.

1. latency command is admin only -> remove admin tag from LATENCY command
2. Deleting a namespace does not disconnect existing connections. This causes the old connections’ ns_stats_ references to diverge. -> keep the stats entry in the map without deleting it.
3. Deleting a namespace may cause an underflow here -> solved by not deleting stats from the map.

[nhancdt2602]

@jihuayu Could you re-run the flaky test in CI pipeline again. All other tests pass.

[sonarqubecloud]

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 50%)
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

💡 Need a hand with PR review? Try Gitar by Sonar!

[git-hulk]

@nhancdt2602 It seems only the calls/latency are related to the namespace, so I'm wondering if it's better to extend the existing functions in stats:

void IncrCalls(const string &ns, const string &cmd) {
  stats->total_calls++;
  if (ns != kDefaultNamespace) {
     stats->ns_command_calls[ns]++;
  }
}

so that we don't need to intercepting the stats and namespace in serval places. cc @jihuayu @PragmaTwice

[nhancdt2602]

@nhancdt2602 It seems only the calls/latency are related to the namespace, so I'm wondering if it's better to extend the existing functions in stats:

void IncrCalls(const string &ns, const string &cmd) {
  stats->total_calls++;
  if (ns != kDefaultNamespace) {
     stats->ns_command_calls[ns]++;
  }
}

so that we don't need to intercepting the stats and namespace in serval places. cc @jihuayu @PragmaTwice

Thanks @git-hulk for your review. I saw your point about moving the namespace resolution logic into stats fields/methods like IncrCall, IncrLatency or commands_histogram. The reason I resolve the namespace at the caller layer rather than inside IncrCalls/IncrLatency is to keep the command hot path lock-free.

I want to keep the overhead of adding namespace telemetry information as minimal as possible. By keeping the namespace stats pointer when a connection is established, we are free of lock when accessing the namespace stat entry.

That said, if you and project members prefer consolidating into Stats, accepting a per-command lock. I'm happy to refactor that way; just let me know the preferred trade-off.

I'd love to hear your opinion about this ^^

[git-hulk]

@nhancdt2602 It seems only the calls/latency are related to the namespace, so I'm wondering if it's better to extend the existing functions in stats:

void IncrCalls(const string &ns, const string &cmd) {
  stats->total_calls++;
  if (ns != kDefaultNamespace) {
     stats->ns_command_calls[ns]++;
  }
}

so that we don't need to intercepting the stats and namespace in serval places. cc @jihuayu @PragmaTwice

Thanks @git-hulk for your review. I saw your point about moving the namespace resolution logic into stats fields/methods like IncrCall, IncrLatency or commands_histogram. The reason I resolve the namespace at the caller layer rather than inside IncrCalls/IncrLatency is to keep the command hot path lock-free.

I want to keep the overhead of adding namespace telemetry information as minimal as possible. By keeping the namespace stats pointer when a connection is established, we are free of lock when accessing the namespace stat entry.

That said, if you and project members prefer consolidating into Stats, accepting a per-command lock. I'm happy to refactor that way; just let me know the preferred trade-off.

I'd love to hear your opinion about this ^^

@nhancdt2602 I'm not sure how much performance would be impacted. Consolidating into one stats would be more consistent from my personal perspective since only a few metrics are related to the namespace.

[nhancdt2602]

@git-hulk I can relate your preference. From my side, I lean toward keeping the stat object scoped to namespace level and move the stat entry resolution to caller layer.

However, I'm happy to apply your suggestion if it's the direction of the project. Shall I consolidate them or wait for others opinion ^^

[git-hulk]

@git-hulk I can relate your preference. From my side, I lean toward keeping the stat object scoped to namespace level and move the stat entry resolution to caller layer.

However, I'm happy to apply your suggestion if it's the direction of the project. Shall I consolidate them or wait for others opinion ^^

We can wait for a while to see if other maintainers have any comments.

[jihuayu]

That said, if you and project members prefer consolidating into Stats, accepting a per-command lock. I'm happy to refactor that way; just let me know the preferred trade-off.

@nhancdt2602 Do you mean the lock in this piece of code?

https://github.com/apache/kvrocks/pull/3557/changes#diff-0782ead7a805bf491f44cf0bbbab901ba0ae3f18bc57181dd6e58a8b5c594171R1409-R1424

[nhancdt2602]

That said, if you and project members prefer consolidating into Stats, accepting a per-command lock. I'm happy to refactor that way; just let me know the preferred trade-off.

@nhancdt2602 Do you mean the lock in this piece of code?

https://github.com/apache/kvrocks/pull/3557/changes#diff-0782ead7a805bf491f44cf0bbbab901ba0ae3f18bc57181dd6e58a8b5c594171R1409-R1424

@jihuayu Exactly. Or do you have any better ideas?