[core][gcs] Add a bounded TableName label to GCS Store count and latency metrics

[zzchun]

Description

gcs_storage_operation_count and gcs_storage_operation_latency_ms are labeled by Operation only, so a slow GetAll on the actor table can't be told apart from one on the job table without correlating logs.

ObservableStoreClient already receives a table_name on every call. This PR:

• records it as a bounded TableName label on both metrics, for Put, Get, GetAll, MultiGet, Delete, BatchDelete, GetKeys and Exists;
• instruments GetNextJobID, the one storage operation the wrapper used to skip;
• measures latency on the monotonic clock instead of wall time;
• documents both metrics in system-metrics.md.

Exporting these metrics on the external-Redis backend, where they don't exist today, was split out of this PR at review into #66882.

Related issues

Closes #65582.

Additional information

Label cardinality

StoreClient accepts any string as a table name (the shared test base passes "test_table"), so the label is bounded explicitly: NormalizeTableNameLabel keeps names from the rpc::TablePrefix enum and maps anything else to OTHER.

In practice the GCS produces seven values: JOB, ACTOR, ACTOR_TASK_SPEC, PLACEMENT_GROUP, NODE, WORKERS and KV. The hard upper bound is 161 Operation × TableName combinations per GCS process (8 table operations × 20 values, plus GetNextJobID × JobCounter), independent of cluster size or actor count.

No user-controlled string reaches a label: the internal KV puts the user's namespace in the key (MakeKey in store_client_kv.cc), not in the table name. NoUserDataInLabels covers this.

GetNextJobID

It addresses a counter key rather than a table, so it reports the JobCounter sentinel. It can't simply leave TableName unset: tag_keys is the tag allowlist on both metric stacks, and a declared tag that a call site doesn't record is exported as an empty string, so it would show up as TableName="".

Latency clock

Latency used to be the difference of two NowUnixNanos() readings, which is wall time, so an NTP step could record a negative or inflated value. Every recording site is rewritten here anyway, so they now use SteadyNow(), which ClockInterface documents as the clock for durations.

Compatibility / release note

This changes series identity: each Operation now has up to seven series instead of one, and GetNextJobID is a new Operation value. Label matchers are unaffected. A dashboard or alert that assumed one series per operation can aggregate the new label away:

sum without (TableName) (rate(ray_gcs_storage_operation_count_total[5m]))

Use without (TableName) rather than by (Operation), because by also drops SessionName, Component and the other global labels. Aggregations sensitive to series count (avg, max, count) still change.

Not a duplicate

Searching open PRs for "TableName gcs_storage_operation", "observable store client", "gcs_storage_operation redis" and "MaybeObserve" finds no other PR labeling these metrics. #66847 edits the same Record call sites to move recording onto a separate io_context. It changes where recording runs, not what is recorded, so the two compose and whichever lands second rebases.

Testing

bazel test //src/ray/gcs/store_client/tests:table_name_label_test \
  //src/ray/gcs/store_client/tests:observable_store_client_test \
  //src/ray/gcs/store_client/tests:in_memory_store_client_test
# Executed 3 out of 3 tests: 3 tests pass.

pre-commit run --from-ref upstream/master --to-ref HEAD
# All hooks passed.

AI assistance (Claude Code) was used to prepare this change. I reviewed every changed line and ran the tests above locally.

[gemini-code-assist]

Code Review

This pull request enhances GCS storage metrics by adding a TableName label to both operation count and latency metrics, allowing breakdowns by specific GCS tables. It introduces a normalization helper to map table names to a closed domain, preventing unbounded label cardinality, and adds a configuration flag to toggle these metrics on the external Redis backend. The review feedback points out that since clock_.NowUnixNanos() is non-monotonic, the latency calculation could occasionally yield negative values, and suggests guarding against this by using std::max with zero.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

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

[github-actions]

This pull request has been automatically marked as stale because it has not had
any activity for 14 days. It will be closed in another 14 days if no further activity occurs.
Thank you for your contributions.

You can always ask for help on our discussion forum or Ray's public slack channel.

If you'd like to keep this open, just leave any comment, and the stale label will be removed.

[zzchun]

Hi @karticam , could you please help to review #65583 when you have a moment, thank you !

[karticam]

Hi @karticam , could you please help to review #65583 when you have a moment, thank you !

Sorry for the delay. Was caught up with something else. I will do it by today/tomorrow.

[karticam]

Hi @zzchun . Thanks for the PR and the detailed writeup. I went through the changes and it seems like this PR is trying to solve two issues:

1. Adding table name to existing metrics which is the tag_keys change and other related changes.
2. Exporting these metrics on redis backend which is the MaybeObserve plus the addition of the new flag.

I think this can be split into two PRs. It would be much easier to review them separately.

Also, I see some optimizations around string_view and string, which seem quite complex. Did you do any benchmarking to see what are the benefits to use string_view over string in this case?

[zzchun]

@karticam Thanks for taking the time to go through it !

Splitting: Agreed, the two changes are independent. I'll keep this PR scoped to the TableName label, plus the switch to the monotonic clock for latency since it touches the same lines. The Redis backend wrapping moves to a separate PR: MaybeObserve, the gcs_redis_storage_metrics_enabled flag and the shared_ptr ownership change. I'll link it here once it's up.

string_view vs std::string: Good question. I didn't benchmark it, and looking at it again I don't think the difference would be measurable. The table names are short enough to fit in SSO, MetricTags already allocates a vector on every Record, and both costs are negligible next to the storage round trip itself. So the extra complexity isn't justified. I'll capture the label as a plain std::string and drop the lifetime/SSO comments and the tests that only existed to support the string_view. The enum-based normalization stays, since that's what keeps the TableName label bounded.

[zzchun]

@karticam I've addressed your comments. Please take another look when you have time, thanks!

Splitting: Done. This PR now only adds the TableName label, plus moving latency onto the monotonic clock since that touches the same lines. The Redis backend wrapping (MaybeObserve, the gcs_redis_storage_metrics_enabled flag and the shared_ptr ownership change) is now #66882.

string_view: No, I didn't benchmark it, and I don't think a benchmark would show anything: the table names fit in SSO, MetricTags already allocates a vector on every Record, and all of it is dwarfed by the storage round trip. It wasn't worth the complexity, so the label is now a plain std::string captured by value, and the comments and tests that supported the string_view are gone. The enum-based normalization stays, since that's what keeps the label bounded.

I also rebased onto master to resolve a conflict (the metrics doc moved to system-metrics.md), so the force-push diff includes unrelated master changes. The review changes are all in the last commit, 3549453.

[karticam]

These changes LGTM besides a few nits. Also wanted to make sure that the string_view v/s string optimizations were only for the table name strings right (and they are all pretty small right)

[zzchun]

@karticam Thanks for the review! Rebased onto the latest master and updated the copyright year to 2026 in all three new files (176eb4e).

On string_view: yes, the optimization only ever applied to the TableName label value, and that is now a plain std::string captured by value. The values come from the rpc::TablePrefix enum names (plus OTHER / JobCounter), so they're all short and fit in SSO. The string_view that remains in the diff is just the conventional parameter type for MetricTags / the test helpers and the two constexpr label constants, not an optimization.

[karticam]

Thanks for the PR. LGTM

[dstrodtman]

Docs-side style and grammar pass from Douglas Strodtman (Ray docs team), drafted with Claude Code assistance. I've read and stand behind the comment.

Scope is prose and docs conventions on system-metrics.md only. The metric semantics, label domain, and backend behavior are for the GCS maintainers, and I didn't change any of them. The latency row reads cleanly as written. The docs side has no objection to merging.