[core][gcs] Export Redis payload byte metrics by command and table

[zzchun]

Description

GCS Redis command metrics can't tell an operator whether load is many small values or a few very large payloads. Today there is nothing to answer that with:

• The Redis backend emits no per-operation count at all. gcs_storage_operation_count / gcs_storage_operation_latency_ms are recorded by the ObservableStoreClient decorator, and RedisStoreClient is the one StoreClient implementation never wrapped in it (gcs_server.cc, REDIS_PERSIST branches for both table storage and internal KV — compare the IN_MEMORY / ROCKSDB_PERSIST branches right beside them).
• The only Redis-specific metric carries no labels. gcs_latency declares a CustomKey tag key but is recorded via Record(double), which passes an empty tag set, so no latency is attributable to a command or a table.

This PR adds three metrics on the GCS Redis client, labeled by a normalized Redis verb and the GCS table name:

Measure	Aggregation	Exported as
gcs_redis_request_payload_bytes	Sum	ray_gcs_redis_request_payload_bytes_total
gcs_redis_response_payload_bytes	Sum	ray_gcs_redis_response_payload_bytes_total
gcs_redis_command_count	Count	ray_gcs_redis_command_count

Labels on all three: Command, TableName.

rate(ray_gcs_redis_response_payload_bytes_total[5m]) / rate(ray_gcs_redis_command_count[5m]), broken down by TableName, is the query that separates "this table has many small rows" from "something is writing multi-megabyte records" — which is the whole point of the change.

Gated by RAY_gcs_redis_payload_metrics_enabled (default true), read once in the RedisStoreClient constructor.

Related issues

Related to #65552 .

Additional information

The payload definition is published with the metric

A metric described as "data size" isn't actionable, so the definition ships in the metric description, in system-metrics.rst, and is pinned by exact-delta tests:

• Request bytes — the sum of strlen(argv[i]) over the whole RESP argument vector, including the command verb, the Redis key, and hash field names. Excludes RESP framing, TLS and TCP/IP overhead. GCS values aren't compressed anywhere between GcsTable::Put and the socket.
• Response bytes — the sum of redisReply::len over every bulk-string and status node, recursing into arrays. Field names returned by HSCAN are included. Integer and nil replies contribute zero. An error reply is retried rather than delivered and contributes nothing.
• Both counted once per logical command, not per retry.

To reconcile against Redis' own total_net_input_bytes, add framing: 3 + digits(nargs) per command plus 5 + digits(len) per argument.

Contribution checks

Duplicate-work check: searching open PRs for Redis payload metrics found no other implementation; PR #65554 was the only result. This update only resolves its conflicts with current master.

Tests run locally for the conflict resolution:

bazel test //src/ray/gcs/store_client/tests:redis_store_client_test
PASSED

pre-commit run --files src/ray/common/ray_config_def.h src/ray/gcs/store_client/redis_store_client.cc src/ray/gcs/store_client/tests/redis_store_client_test.cc
PASSED

AI assistance was used to resolve the upstream merge conflicts. The human submitter must review every changed line and understand and defend the resolution before requesting maintainer review.

[gemini-code-assist]

Code Review

This pull request introduces new system metrics to track Redis request and response payload sizes, as well as command counts, within the GCS storage backend. It includes a configuration option to enable or disable these metrics and adds comprehensive unit tests to verify exact byte tracking and label boundaries. A review comment suggests adding defensive null checks in ResponsePayloadBytes when traversing redisReply elements to prevent potential segmentation faults from malformed replies.

[gemini-code-assist]

In defensive programming, it is important to handle potential null pointers when dealing with external library structures like redisReply. If reply.elements > 0 but reply.element is nullptr (e.g., due to allocation failure or malformed replies), or if any individual reply.element[i] is nullptr, dereferencing them will cause a segmentation fault. Adding explicit null checks ensures robustness.

    size_t total = 0;
    if (reply.elements > 0 && reply.element != nullptr) {
      for (size_t i = 0; i < reply.elements; ++i) {
        if (reply.element[i] != nullptr) {
          total += ResponsePayloadBytes(*reply.element[i]);
        }
      }
    }

[zzchun]

Thanks for looking at this. I dug into it and I'm going to leave it as-is, for two reasons.

The guard wouldn't actually prevent the crash. ResponsePayloadBytes is called from RedisRequestContext::RedisResponseFn immediately before std::make_shared<CallbackReply>(*redis_reply), and CallbackReply::ParseAsStringArrayOrScanArray dereferences the same pointers — element[0], element[1], and array_entry->element[i] — with no null checks (pre-existing, untouched by this PR). On a malformed reply, adding the guard here just relocates the segfault a few lines down while implying a robustness that isn't there.

elements > 0 implies element != nullptr is a hiredis invariant. createArrayObject allocates element with calloc and frees the whole reply if that allocation fails, so a reply delivered to a callback never has non-zero elements with a null element. The surrounding code already relies on this.

The one case that genuinely is unspecified — hiredis leaves element uninitialized when elements == 0 — is handled by the loop bound, and pinned by TestResponsePayloadBytes.EmptyArrayDoesNotDereferenceElements, which deliberately passes a garbage pointer to prove the bound is what stops us.

Happy to add the checks if a maintainer prefers defense-in-depth here, but I'd want to add them to CallbackReply in the same change so the two consumers of these pointers stay consistent.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

^{Reviewed by Cursor Bugbot for commit 0c1e0ca. Configure here.}

[Sparks0219]

@rueian could you PTAL 🙏

[rueian]

Hi @zzchun, please resolve the conflicts.

[rueian]

Why don't we just use the verb directly but limit its length?

[zzchun]

Good point. I removed the closed allowlist and now derive the Command label directly from the actual Redis verb in args[0]. The label is normalized to uppercase ASCII and truncated to 16 bytes. Deriving it inside RedisRequestContext also prevents a call site from supplying a label that differs from the command being sent. Production verbs remain code-controlled. Tests cover case normalization and the truncation boundary.

Updated in 6400b8f.

[rueian]

Is it correct to use 0 for these types? For integers, shouldn't we count the decimal length?

[zzchun]

You're right. The decimal representation between the RESP integer prefix and CRLF is application payload, so returning zero undercounted it. Integer replies now count their decimal text length, including a minus sign, using allocation-free std::to_chars.

I also audited the other scalar reply types: doubles use the original text length preserved by hiredis, booleans contribute one byte, and nil remains zero. Tests cover zero, negative values, LLONG_MIN/LLONG_MAX, doubles, booleans, and nil.

Updated in 6400b8f.