compute: add ComputeRuntimeRole and role-labeled metrics - #37884
Conversation
958feb6 to
a804e03
Compare
| /// process-global initializers. | ||
| Maintenance, | ||
| /// The interactive runtime of a two-runtime process. Shares the process globals owned by | ||
| /// maintenance and serves reads. | ||
| Interactive, |
There was a problem hiding this comment.
Lets make this a single variant for now until we actually land the second runtime.
There was a problem hiding this comment.
Fair, and publishes() was worse: it had no caller at all here, only in the two-runtime PR. Removed, it belongs there.
On the variants, dropping to Solo alone makes the rest of this PR inert rather than smaller. label() returns None for Solo, so with_role becomes a no-op on all 25 registrations, owns_process_globals() is always true, and both tests go with it. What is left is a one-variant enum threaded through serve.
So the real choice is to keep the three variants here, where the test is what shows why the label exists at all (two roles registering into one process registry panic with AlreadyReg without it), or to drop this PR and let the role arrive with the runtime that actually constructs the other two. I lean to the second: the with_role churn is mechanical, and reviewing it next to its first real caller is not much worse than reviewing it alone. Which would you rather have?
There was a problem hiding this comment.
Done differently than either of us proposed: Interactive is now #[cfg(test)], so a release binary has the two variants that something constructs, and the test keeps a second named role to work with.
The reason it can't simply go is worth recording, since I got it wrong first: Solo and a named role cannot share a registry at all. Solo registers the same metric names with no role label, so prometheus rejects the pair for differing label dimensions rather than recording two series. I tried it, the error is has different label names or a different help string. So dropping to one named role would not weaken the non-collision test, it would delete it.
Also removed publishes() and noted on owns_process_globals() that it is constantly true in a release build, since every role shipped here owns the globals.
There was a problem hiding this comment.
Claude came up with a solution, and I'm not sure I very much like it. It leaves the variants in place, although we only ever construct Solo, and it argues that it otherwise can't test the feature. Kinda makes sense, and I feel it's easy enough to revert if we need to, so going with it.
DAlperin
left a comment
There was a problem hiding this comment.
One note but otherwise lgtm.
|
Rebased onto main. The only conflict was with #37989, whose two new index-peek histograms land in the same field lists and construction blocks this commit rewrites. Both now carry the Note for future metrics: leaving one unwrapped is not a cosmetic miss. Two roles then register the same unlabeled name into one process registry and it panics at registration with |
a804e03 to
26db6f6
Compare
Introduce `ComputeRuntimeRole` and thread a role through `serve` into `ComputeMetrics::register_with`. A named role stamps a distinct `role` const label on its metrics so that two compute runtimes sharing one process registry register distinct series rather than colliding, and gates the whole-registry `workload_class` postprocessor on the globals-owning role so it is installed exactly once. `Solo` is the single-runtime default and is behaviorally identical to compute before a second runtime existed: it emits no `role` label, so exact-match dashboards and alerts are byte-unchanged, and it owns the process globals as the sole runtime always has. Every call site passes `Solo`. `Maintenance` is what the two-runtime work will pass for the runtime that keeps the globals. `Interactive` is `#[cfg(test)]` until that work constructs it. It exists because the label's purpose is that two named roles coexist in one registry, and only a second named role can express that: `Solo` registers the same metric names without a `role` label, so prometheus rejects it alongside a named role for differing label dimensions rather than recording a second series. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019G29DBfgE8LXpE5jamm2Zi
26db6f6 to
1a7b9fe
Compare
|
Thanks for the review! |
Motivation
The two-runtime read-isolation work (#37770) runs two compute runtimes in a
single clusterd process, sharing one metrics registry. Without per-runtime
labels the two runtimes register the same metric series and collide. This PR
adds the role concept and the metric labeling on its own, so that the
two-runtime PR is left with only the runtime wiring.
Description
Introduce
ComputeRuntimeRole(Solo,Maintenance,Interactive) inmz_compute::serverand thread a role throughserveintoComputeMetrics::register_with. Each named role stamps a distinctroleconst label on its metrics, so two runtimes sharing one process registry
register distinct series rather than colliding. The whole-registry
workload_classpostprocessor is gated on the globals-owning role so it isinstalled exactly once.
Solois the single-runtime default and is behaviorally identical to computebefore a second runtime existed: it emits no
rolelabel, so exact-matchdashboards and alerts are byte-unchanged, and it owns the process globals as
the sole runtime always has. Every call site passes
Solohere.Two unit tests assert the two halves of the contract:
Solometrics carry norolelabel, and the two named roles carry distinct labels so they coexist onone registry.
Context
Prerequisite for #37770, which flips the call sites to
MaintenanceandInteractiveand adds the runtimes that use them. Independent of the other#37770 prerequisites (#37880, #37881).
Verification
cargo check -p mz-compute -p mz-clusterdpasses with no new warnings. The twonew
metrics::testspass.🤖 Generated with Claude Code
https://claude.ai/code/session_01CY7GHdrTBfAG4tgvSgPGJ9