Add cpu-usec metric support under CLUSTER SLOT-STATS command

[kyle-yh-kim]

The metric tracks cpu time in micro-seconds, sharing the same value as INFO COMMANDSTATS, aggregated under per-slot context.

[kyle-yh-kim]

First revision only holds implementation changes for initial feedback purposes, with pending perf testing.
Integration tests are not up-to-date, and thus failing. This will soon be followed-up.

[zvi-code]

@kyle-yh-kim

I totally get the value in per-slot information! But wondering about the how part.

Have you considered alternatives? For example:

1. extend server.duration_stats[<type>] to server.duration_stats[<type>][max_slots +1] ?
2. extend the real command to have slot dimension?
3. keep the info on the client until the slot of the client is reset...

The reason I raise this is we have many slot array in many places in the code and this seems to be arbitrary addition to that and on the other hand we have several duration tracking mechanisms and this adds another one. I am wondering if we can merge/reuse these. In addition I can see we would want additional information like command count and maybe histogram.. this will not extend natively and we should strive to reuse what we have or enhance it, IMO.

[kyle-yh-kim]

Thanks for your comments. To answer your concerns;

Have you considered alternatives? For example:

1. extend server.duration_stats[] to server.duration_stats[][max_slots +1] ?
2. extend the real command to have slot dimension?
3. keep the info on the client until the slot of the client is reset...

The reason I raise this is we have many slot array in many places in the code and this seems to be arbitrary addition to that and on the other hand we have several duration tracking mechanisms and this adds another one.

For cpu-usec explicitly, I agree that this metric alone could be squeezed into other adjacent storages mentioned above, with added refactoring efforts.

However, when reviewing the per-slot metrics holistically, with the additions of per-slot 1) network-bytes-in [PR link], 2) network-bytes-out [PR-link] and 3) memory-bytes [PR-pending], this sparse disconnection between cpu-usec and the remaining per-slot metrics no longer makes sense.

Plus, we obtain cache locality benefits from keeping all metrics memory contiguous, as the CLUSTER SLOT-STATS command accesses all tracked metrics (cpu, network-bytes-in, network-bytes-out, memory-bytes) under the specified range of slots.

Given considerations of Valkey 8.0 rc1 code-cutoff timeline, refactoring efforts, remaining per-slot metrics storage cohesiveness, and cache locality benefits, my preference is to keep the slot_stats[] location as is. The only remaining change would be to follow the latest proposal from @zuiderkwast, which recommends to relocate the slot statistics array under clusterState such that it only consumes this memory in cluster mode;

typedef struct slotStat {
    uint64_t cpu_usec;
    uint64_t network_bytes_in;
    uint64_t network_bytes_out;
    uint64_t memory_bytes;
} slotStat;

struct clusterState {
    ...
    slotStat slot_stats[CLUSTER_SLOTS];
};

[madolson]

The reason I raise this is we have many slot array in many places in the code and this seems to be arbitrary addition to that and on the other hand we have several duration tracking mechanisms and this adds another one. I am wondering if we can merge/reuse these. In addition I can see we would want additional information like command count and maybe histogram.. this will not extend natively and we should strive to reuse what we have or enhance it, IMO.

Command count might make, I'm not sure we really want a histogram on a per-slot basis seems to be missing the point of statistics though. The point of these metrics is for identify hot shards for migration for auto-balancing, it's not for trying to introspect too much into a specific slot.

[madolson]

LGTM now, just some minor comments, please merge and we can address the comments and merge.

[codecov]

Codecov Report

Attention: Patch coverage is 94.28571% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.36%. Comparing base (14e09e9) to head (de6c75e).
Report is 1 commits behind head on unstable.

Additional details and impacted files

@@            Coverage Diff            @@
##           unstable     #712   +/-   ##
=========================================
  Coverage     70.36%   70.36%           
=========================================
  Files           112      112           
  Lines         61275    61308   +33     
=========================================
+ Hits          43115    43141   +26     
- Misses        18160    18167    +7     

Files	Coverage Δ
src/blocked.c	91.48% <100.00%> (+0.03%)	⬆️
src/cluster.c	88.35% <100.00%> (+0.05%)	⬆️
src/cluster_legacy.c	85.94% <100.00%> (+0.21%)	⬆️
src/config.c	78.69% <100.00%> (+0.01%)	⬆️
src/script.c	87.50% <100.00%> (+0.09%)	⬆️
src/server.c	88.56% <100.00%> (+<0.01%)	⬆️
src/server.h	100.00% <ø> (ø)
src/cluster_slot_stats.c	90.32% <92.00%> (+1.10%)	⬆️

... and 7 files with indirect coverage changes

[madolson]

I don't love this error, it would be nice to be more useful. We can follow up on this though.

[madolson]

Reply schema looked good: https://github.com/valkey-io/valkey/actions/runs/10050729040