Design: managed metrics collection - #363
Conversation
|
|
||
| ## Interaction with #264 | ||
|
|
||
| The [#264](https://github.com/modelplaneai/modelplane/issues/264) example |
There was a problem hiding this comment.
In Release notes we need to add a line telling existing users to delete their hand-written podmonitor.yaml, or they get a duplicate scrape job against the same pods after upgrade.
There was a problem hiding this comment.
Yes, without deleting it they'd get a duplicate scrape of the same pods after upgrade. Called it out as a release note in the #264 interaction section. Pushed.
|
|
||
| - **Port name.** `http` (the serving port that also serves `/metrics`) versus | ||
| `metrics`. Leaning `http`, since it's the one serving port. | ||
| - **Configurability.** `interval` and `path` are fixed (30s, `/metrics`) for now. |
There was a problem hiding this comment.
wonder if 30s is too Long If it comes to KV Cache utilization e.g.
There was a problem hiding this comment.
For routing this interval doesn't matter: the EPP scrapes engine /metrics on its own fast internal loop (~100ms) to make routing decisions, so routing isn't gated on it. This PodMonitor feeds Prometheus for dashboards and alerting, where 30s is a normal default. If finer KV series are wanted, interval is a knob on the metrics object (open question). Made that distinction explicit in What gets scraped.
| example). Nothing owns that wiring. The operator builds it by hand and keeps it in | ||
| sync with the deployment's shape. They delete it on teardown. | ||
|
|
||
| Instead, Modelplane composes the collection, per source: |
There was a problem hiding this comment.
is Monitoring for Cache / PVC here out of scope?
There was a problem hiding this comment.
Out of scope here, yes, this doc is serving metrics (engine + EPP). ModelCache PVC and hydration observability is a separate concern that belongs with the ModelCache work. Added a scope line to the summary.
|
|
||
| ### What gets scraped | ||
|
|
||
| The `PodMonitor` ingests everything the engine exposes on `/metrics`. What the |
There was a problem hiding this comment.
what are the expected series-per-pod ? is the Prometheus Stack configured with rolling / retention of the storage ?
There was a problem hiding this comment.
Order of dozens of series per engine pod (request/latency histograms, KV-cache, throughput), plus the llm_d_epp_* set wherever an EPP runs. Retention and storage sizing are the serving stack's Prometheus config in compose-serving-stack, not this composition, worth a sane default there but out of scope for this doc. Noted it in What gets scraped.
| replicas: 1 | ||
| template: | ||
| spec: | ||
| metrics: |
There was a problem hiding this comment.
consider observability/monitoring as the object name if it's going to grow past scraping... later
There was a problem hiding this comment.
Agree, and it lines up with #77 (ModelService.observability.traces). Renamed to spec.template.spec.observability.metrics.enabled so traces and logs can slot beside metrics later. (Metrics live on the deployment since we scrape its pods; #77's traces live on ModelService, different resource, same observability umbrella.) Pushed.
There was a problem hiding this comment.
Superseded by the reframe: after Nic's review the doc drops the per-deployment opt-out field entirely. Collection is now always-on at every layer (engine, EPP, serving-stack) with no opt-in or opt-out, feeding a central Modelplane Prometheus. So there's no longer a field to name under observability. Leaving this for context; the observability grouping question may come back if we add a deployment-level knob later.
| matchLabels: | ||
| modelplane.ai/serving: <replica-name> | ||
| podMetricsEndpoints: | ||
| - port: http |
There was a problem hiding this comment.
is http the right name here?
There was a problem hiding this comment.
It's an open question in the doc (http vs metrics). Leaning http since it's the one serving port that also serves /metrics, but happy to go metrics if that reads clearer.
|
@dennis-upbound I think we need to step back and think about what we want to help folks monitor. There's a few things you could monitor in Modelplane:
I think MD authors will care about 1. The rest feel more like platform team concerns. This proposal helps with 1, but it feels incomplete. If I understand correctly, it configures the Prometheus instance we already deploy to each cluster to scrape ModelReplica (i.e. vLLM, EPP) metrics, but what then? How does the MD author see and consume those metrics? If the platform team needs to setup plumbing to actually expose the metrics somewhere the MD author can use them (e.g. a dashboard), why ask the MD author to opt-in (or out)? My hunch is Modelplane should do something like:
This'd give you one central Prom instance you can scrape to get your entire Modelplane deployment's metrics. Metrics you'd presumably feed onwards into your monitoring and alerting system of choice. |
|
Completely agree, reframed the doc around this. It now opens with the four things to monitor (data plane / substrate / control plane / fleet roll-up), makes The per-deployment opt-out field is dropped (now the first rejected alternative). Open questions left: remote-write vs federation, central retention sizing, and phasing (data-plane + substrate first, control-plane + roll-up after). |
@dennis-upbound did you forget to push? 🤔 Doc looks unchanged to me. |
|
Agree — pulled the scope to exactly this. The doc now composes collection always-on inside each IC (engines, EPPs, serving-stack), no MD-author opt-in/out, and exposes the cluster's Prometheus URL on the (Also: the doc genuinely hadn't updated when I said it had — I'd pushed to the wrong remote. Fixed now, the diff reflects the change.) |
|
Revved the doc. It now aggregates every cluster's metrics up to one Modelplane-level view at the control plane, rebranded under |
|
@dennis-upbound thanks! I like the direction. I think this needs some POC work before I can approve. Specifically I'd like to lock down:
|
|
Locked both. Collector: OpenTelemetry. The GenAI semantic conventions are the naming target, the rename runs in the collector's transform processor in-pipeline, and one pipeline carries metrics, traces (#77), and logs. A Prometheus stack would push the rename into per-cluster recording rules and stay metrics-only, so it's now a rejected alternative. Metrics + normalization. Added the On staying correct for an engine we don't recognize: an engine-type label picks the mapping, the same mechanism the GAIE picker already uses for routing, and an unmapped engine degrades to raw names rather than a wrong guess. See the new "Capture from an opaque engine" and "Normalize to Still needs the POC to confirm the derived TRT-LLM rows and the collector's rename config, agreed. |
06d0dbe to
3007f9a
Compare
3c04631 to
5071242
Compare
Say plainly what a mapping is, a selector plus the engine's source names, the modelplane_* name each becomes, and the labels, and show a concrete vLLM MetricMapping. Make the registry first-class Modelplane resources rather than a ConfigMap or an EnvironmentConfig: compose-serving-stack reads every MetricMapping as a required resource, the way compose-model-deployment reads InferenceCluster and ModelCache, and renders them into the collector's config. A new engine is a new MetricMapping, validated on apply and discoverable, with no fork and no release. Schedulers use the same kind. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
…tor-scoped rename The picker and collector share the engine-type label, not a mapping registry, so say the label serves both rather than claim one shared registry. Add a tokens_total row so the table delivers the tokens-per-second the doc lists under what to monitor. State that a MetricMapping's rename applies only to metrics from pods its selector matches, which is what the selector is for. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
…osticism Push is the default connectivity: each collector remote-writes or OTLP-exports outbound, so the workload cluster needs only egress and it works across regions and firewalls, with pull kept for when the center already reaches the cluster. Modelplane needs no persistent store: the control-plane collector rolls up in memory, a short-retention in-memory Prometheus handles PromQL and quantile roll-ups with no volume so it runs in a Space, and durable storage is the operator's own backend. State that the picker routes any engine and only its KV- and queue-aware scoring needs the standard metrics, degrading without them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
Every fleet roll-up is a spatial aggregation the OTel collector does in memory: it sums gauges and counters across clusters and merges histograms, and SLO attainment is a ratio of buckets when a boundary sits at the target. Computing a percentile value or an ad-hoc query is read-time work for whatever consumes the export, so Modelplane runs no store and the control plane stays stateless, which is what runs in a Space. Removes the short-retention Prometheus the earlier draft kept for quantiles. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
After moving to a stateless collector, three store references were left dangling. The summary said series roll up to a single store, which contradicts runs no store, so it now says one view. The PodMonitor-per-replica alternative said cluster-wide store where it meant collector. The Prometheus-stack alternative implied our design keeps a central store, now scoped to an operator that wants one. Add cost to the aggregate roll-up list so it matches what to monitor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
The doc read as entirely unbuilt while the MetricMapping kind has since been implemented, so a reader could not tell which parts describe a proposal and which describe code. Note it in the status line and next to the section that introduces the kind, with the two caveats that matter for reading the rest: nothing consumes a MetricMapping yet, because the collector this hands them to does not exist, so applying one reports Ready and changes no behaviour; and the reading side sits in its own function rather than in compose-serving-stack as the doc describes, which is a difference to close rather than keep. Signed-off-by: Dennis Ramdass <dennis@upbound.io>
Engine metric names are already namespaced, so a flat name-to-name map would rename them unambiguously with no selector at all, and the field reads redundant. It is not, and the reasons were nowhere in the doc, so the next reader is one step from removing it. Four of them. The degradation this section promises is label-based by construction: reporting "no mapping for X" means reading a pod's claimed engine and finding none, which name matching cannot tell from a successful rename of nothing. The consistent label set is per pod rather than per series, so name matching cannot label the series a mapping does not rename. A forked engine emits the upstream names while needing its own mapping, and two mappings matching one name cannot be told apart without the pod. And kube-scheduler's names carry no vendor prefix at all, so the scheduler section needs the selector most. Note the collector consequence while it is fresh: the rename is an OTTL transform gated on a resource attribute, not the simpler metrics-transform processor, which matches on metric name only. Signed-off-by: Dennis Ramdass <dennis@upbound.io>
Two things in the doc are no longer accurate now that the collector exists and has been run against a real engine. Passthrough is not byte-identical. The doc said an unmapped engine keeps its "native names"; the collector's Prometheus exporter sanitizes `:` to `_`, so `vllm:gpu_cache_usage_perc` is published as `vllm_gpu_cache_usage_perc`. That changes the upgrade story from one rename to two, so it belongs in the degradation rule rather than in a reader's surprise later. The implementation-status note said nothing consumed a MetricMapping and that the reading side sat in the wrong function. Both are fixed in modelplaneai#412, so the note now says what is built and what was measured, and narrows the remaining gap to the endpoint picker, which still exposes no metrics port. Signed-off-by: Dennis Ramdass <dennis@upbound.io>
modelplaneai#406 merged on 31 August, so a workload cluster's substrate now depends on InferenceCluster.spec.stack and this document described only one of the two. The scraping mechanism it specifies doesn't work today, on any stack. It says the backends name the engine port `http` in native.py, llmd.py and routing.py. None of them do: all three compose {"containerPort": 8000} with no name, and grove.py makes a fourth. A PodMonitor matching `port: http` matches nothing. The reasoning for going by name still holds, since the decode engine serves on 8001 with the pd-sidecar on 8000, so naming the port is now stated as a prerequisite of this design rather than an existing property. The serving label did survive, which is the more useful half. Grove puts modelplane.ai/serving on the leader clique and propagates clique labels to pods, so one cluster-wide selector still covers every shape. A fourth workload kind arrived without touching the collection design, which is the test it needed to pass, and the document now says so instead of listing three shapes. Substrate health named cert-manager, the LeaderWorkerSet controller, the DRA driver and the pod scheduler. LeaderWorkerSet is `Standard` only; a `Dynamo` cluster runs Grove, the KAI Scheduler and a ModelExpress server. The substrate selector has to follow the stack, and ModelExpress is a component we own and have not checked for a metrics endpoint. Cluster scheduler metrics read as though a platform team might bring a gang scheduler. Modelplane installs one: compose-serving-stack composes the KAI Scheduler with an unbounded modelplane-root queue and a modelplane child, and labels every Grove pod into it, so KAI's series are first-party and the queue name is known. Gang readiness has to come from KAI rather than Grove, because PodCliqueSet.status.podGangStatuses is on the type and nothing writes it, leaving availableReplicas unable to tell a gang that never formed from one still forming. Finally the status. This claimed the MetricMapping kind had landed and was implemented in modelplaneai#412. modelplaneai#412 is closed unmerged, waiting on this design, the same as modelplaneai#411 waits on the model delivery one. What it proved stands and the branch stays. Signed-off-by: Dennis Ramdass <dennis@upbound.io>
2da92a4 to
bb87ccd
Compare
The selection mechanism can't be built as written. Normalization picks a MetricMapping from `modelplane.ai/engine: vllm` on the pod, set by the ML team. The ModelDeployment XRD rejects it: a CEL rule forbids any label key under the reserved modelplane.ai/ prefix, on the deployment and on the member pod template both, so that apply fails validation. The prefix is Modelplane's, which is how serving, workload and pool labels reach pods today. So the engine type becomes `engines[].type`, an enum on the ModelDeployment, and compose-model-replica stamps `modelplane.ai/engine` from it alongside the labels it already applies. One field still feeds both consumers, the picker for routing and the collector for normalization, and being typed it validates on apply rather than selecting a mapping by a value nothing produces. That also corrects a claim resting on it. This said the engine-type label already exists if Modelplane runs the GAIE picker, and normalization reuses it. No such label exists anywhere in the tree. The pattern is GAIE's and worth following; the label is ours to add. Separately the document never said what happens to the Prometheus already on every cluster. compose_prometheus is unconditional on both stacks, and two things here lean on it: it scrapes the substrate and the gateway's Envoy proxies, and its operator defines the PodMonitor CRD this design composes against. The collector is added beside it. What it removes is per-cluster recording rules and a second Prometheus at the center, not the one already there, and the Prometheus alternative no longer claims otherwise. That exposes one open thing rather than hiding it: a plain OTel collector doesn't read PodMonitors. Either it runs under the OpenTelemetry Operator, whose target allocator consumes them, or it carries its own service-discovery scrape config and PodMonitor is the wrong word throughout. The first is smaller and keeps the modelplaneai#264 upgrade path, and nobody has run it, so it is recorded as a decision rather than a tested one. The vLLM example mapped three of the capture contract's four signals, missing KV-cache occupancy. It maps four now, and inter-token latency lands on time_per_output_token to match the OpenTelemetry GenAI name the document adopts. Signed-off-by: Dennis Ramdass <dennis@upbound.io>
lsviben
left a comment
lsviben
left a comment
There was a problem hiding this comment.
I like the direction this design is taking with using the power of OTEL to collect and send the data from Modelplane. Discussed this with Dennis in a 1on1 as well and just left here some notes on things the desing could clarify better.
| **Collect with OpenTelemetry.** The collector is an OpenTelemetry collector, added beside | ||
| the per-cluster Prometheus rather than replacing it. The section below gives the reasons. |
There was a problem hiding this comment.
Is this Prometheus mention a leftover from the previous solution. With OTEL collector we dont need it anymore.
There was a problem hiding this comment.
Leftover, and one I added rather than inherited. I'd argued Prometheus had to stay because its operator defines the PodMonitor CRD this design composes against, which gets the dependency backwards: PodMonitor is a consequence of having chosen Prometheus, not a requirement of collection. The prometheus receiver does Kubernetes SD itself.
The stack is gone now, with each thing it did named: k8s_cluster for kube-state-metrics, kubeletstats for cAdvisor, hostmetrics for node-exporter. Two costs are written down rather than left to be found — no local store to query ad-hoc, and the collecting-engine-metrics guide loses both halves of its workflow.
|
|
||
| ## Collect on every cluster | ||
|
|
||
| On each cluster Modelplane collects from every source it owns, with no opt-in or opt-out. |
There was a problem hiding this comment.
Seems ok with setting it default as on, but why no opt-out? If the OTEL collector is not pushing the data anywhere, for what do we need it running?
There was a problem hiding this comment.
Good question, and it changes the shape rather than just the wording. "Always on" was arguing against a per-deployment toggle, which I still think is right: a ModelDeployment author shouldn't opt out of telemetry the platform team consumes. But you're right that a collector with nowhere to send is cost with no reader.
So the switch moves up a level. No export destination configured, no collector composed. Once a destination exists, collection is on for every source Modelplane owns, with no per-deployment switch.
There was a problem hiding this comment.
👍 on if no export destination configured, no collector composed.
| - **The stack already scrapes.** `compose-serving-stack` runs a metrics stack on every | ||
| workload cluster and already scrapes the gateway's Envoy proxies, so adding a target is | ||
| composition, not new infrastructure. |
There was a problem hiding this comment.
sorry i am not that familiar with Modelplane yet. Would that Prometheus composed here be replaced with the OTEL collector?
There was a problem hiding this comment.
Yes, replaced. And the migration is smaller than it looks: compose-serving-stack scrapes the Envoy proxies today through the Prometheus chart's additionalScrapeConfigs, which is a kubernetes_sd_configs block. That's the same format the collector's prometheus receiver takes, so the Envoy target moves across as it stands rather than being rewritten.
What it does add is a second collector tier. kubeletstats and hostmetrics are node-scoped, so they need a DaemonSet, where k8s_cluster and the engine and EPP scrapes run as one Deployment. The doc now says so, and asks whether the DaemonSet tier belongs in the first cut at all.
| `compose-serving-stack` reads every `MetricMapping` as a required resource, the same way | ||
| `compose-model-deployment` reads `InferenceCluster` and `ModelCache`. It renders them into | ||
| the collector's config, the ConfigMap the OTel collector loads on each cluster. The | ||
| `rename` map becomes transform-processor rules, applied to metrics from the pods the | ||
| `selector` matches. A new engine is a new `MetricMapping`, not a package change. |
There was a problem hiding this comment.
it would be good to see an example of the OTEL config generated based on a MetricMapping
There was a problem hiding this comment.
Added — the vLLM mapping now sits beside the config it renders, so the OTTL is visible: one transform statement per rename, gated on the engine resource attribute that k8sattributes lifts off the pod label, plus the scrape config and its two relabel rules.
It also makes the degradation concrete. An unmapped engine matches the scrape config and no transform block, so it arrives under its own names without a rule having to say so.
| The kube-prometheus-stack `compose-serving-stack` installs stays. It is unconditional | ||
| today, on both stacks, and two things here depend on it: it already scrapes the substrate | ||
| and the gateway's Envoy proxies, and its operator is what defines the `PodMonitor` CRD this | ||
| design composes against. So the collector is added beside it rather than in place of it, | ||
| and what the collector removes is per-cluster recording rules and a second Prometheus at | ||
| the center, not the one already on each cluster. | ||
|
|
||
| That leaves one thing to settle: a plain OTel collector doesn't read `PodMonitor`s. Either | ||
| it runs under the OpenTelemetry Operator, whose target allocator consumes `PodMonitor` and | ||
| `ServiceMonitor` directly and keeps the cluster-wide-selector shape below, or it carries | ||
| its own Kubernetes service-discovery scrape config and `PodMonitor` is the wrong word | ||
| throughout this document. The target allocator is the smaller change and preserves the | ||
| #264 upgrade path, but nobody has run it here, so it is a decision this design records |
There was a problem hiding this comment.
Is this Prometheus/PodMonitors/ServiceMonitors also a leftover? The OTEL Collector can scrape the same as Prometheus using its receiver, and as we are sending the data out we dont need Prom as a data storage.
There was a problem hiding this comment.
Same leftover, same answer as above — the stack and the PodMonitor framing are both out. Discovery is now a scrape config in the collector's own ConfigMap, so no CRD is involved and there's nothing to reconcile between an operator and a receiver.
The alternatives entry keeps "a Prometheus stack" as a considered option, since the local store it gives an operator is a real thing to give up, but it's recorded as rejected rather than half-adopted.
| carries over, punctuation caveat included. An unmapped scheduler still gets scraped | ||
| under its own names, and Modelplane surfaces that rather than guessing. | ||
|
|
||
| ## Aggregate to one view |
There was a problem hiding this comment.
would be good to have a section as well on cardinality and what labels we dont really need in the metrics. We can remove some in the OTEL collector processing before sending it out.
There was a problem hiding this comment.
Agreed, added. Dropping pod, pod_uid and container_id — each is new on every restart, so each turns a rolling update into a fresh set of series nothing writes to again. Keeping engine, cluster, model, deployment and namespace, the dimensions the roll-up and dashboards group by.
One thing worth flagging since the obvious processor is the wrong one: attributes/delete_key removes the label but leaves the series that collided on it as separate undefined points. Merging needs metricstransform with an aggregation action. Getting it wrong looks like it worked.
Histogram buckets stay as the GenAI conventions define them — le is what makes the fleet histogram and the SLO ratio possible.
| The cluster sends by pushing outbound. Each collector remote-writes or OTLP-exports to a | ||
| control-plane endpoint, so the workload cluster needs only egress, which is what makes it | ||
| work across regions and through firewalls. Nothing inbound to the cluster is required, and | ||
| nothing is exposed outside it. The center can pull instead where it already reaches the | ||
| cluster, publishing the cluster's endpoint on the `InferenceCluster` status, but push is the | ||
| default for the regional and firewalled case. |
There was a problem hiding this comment.
Would be good to state which exporters we would support and how to configure them as a ModelPlane user. Auth is also something not touched yet in this doc, and the OTEL collector will need a way to auth to wherever it is sending the data.
There was a problem hiding this comment.
The biggest gap in the doc, and it needed a section rather than a sentence. Both are in now.
Exporters: OTLP (otlp over gRPC, or otlphttp) as the contract, prometheusremotewrite for an operator who wants a Prometheus-compatible store. Vendor exporters go behind the gateway, so one configuration serves the fleet instead of one per cluster. A user doesn't write collector YAML: the destination is fleet-level config propagated to each ServingStack. Where exactly that field or kind lives I've left open rather than guessed.
Transport is the part I'd most like your eyes on. Modelplane has exactly one connection it can count on and it runs the wrong way: the control plane reaches a cluster's API server through the kubeconfig, and nothing guarantees a path back from a firewalled GPU cluster. So push to a gateway collector is the default, with its credential riding the same Secret propagation that already carries a HuggingFace token to every matched cluster. Where a cluster has no egress, the center scrapes the collector through the API server proxy over the connection it already has — no inbound exposure, no second credential, at the cost of putting every series through the API server. Direct pull per cluster is named only to rule it out.
Addresses @lsviben's review. Six changes, and the first five follow from the same correction. The Prometheus stack goes. I'd argued last week that it had to stay because its operator defines the PodMonitor CRD this design composes against, which has the dependency backwards: PodMonitor is a consequence of choosing Prometheus, not a requirement of collection. The collector's prometheus receiver does Kubernetes service discovery itself. The Envoy target we scrape today is already an additionalScrapeConfigs block of kubernetes_sd_configs, the format that receiver takes, so it moves across as it stands. Each thing the stack did now has a named replacement: k8s_cluster for kube-state-metrics, kubeletstats for cAdvisor, hostmetrics for node-exporter. That splits the collector into a node-scoped DaemonSet and a cluster-scoped Deployment, which the document didn't describe, and it raises whether the DaemonSet tier belongs in the first cut at all. Two costs are written down rather than left to be discovered: no local store to query ad-hoc, and the published collecting-engine-metrics guide loses both halves of its workflow. A stale hand-written PodMonitor goes inert rather than double-scraping, which is quieter and worse, so it needs a release note. Collection is no longer unconditionally always-on. Lovro's question was why run a collector that pushes nowhere, and the answer moves the switch up a level rather than reinstating a per-deployment toggle: no destination configured, no collector composed. Once one exists, every source Modelplane owns is collected and an MD author still gets no say over telemetry the platform team consumes. Transport was one hand-waving sentence and is now a section, because Modelplane has exactly one connection it can count on and it runs the wrong way. The control plane reaches a cluster's API server through the kubeconfig; nothing guarantees a path back from a firewalled GPU cluster. Push to a gateway collector is the default, and its credential rides the same Secret propagation that already carries a HuggingFace token to every matched cluster. Where a cluster has no egress, the center scrapes it through the API server proxy over the connection it already has, needing no inbound exposure and no second credential, at the cost of putting every series through the API server. Direct pull is recorded and dismissed. Exporters, destination and auth get a section: OTLP as the contract, prometheusremotewrite for a Prometheus-compatible store, vendor exporters behind the gateway. Where the fleet-level destination lives is called out as open rather than guessed at. Cardinality gets one too. Dropping pod, pod_uid and container_id, which are new on every restart; keeping the dimensions the roll-up groups by. With the trap named, because the obvious processor is wrong: attributes/delete_key removes a label but leaves the collided series undefined, where metricstransform with an aggregation action merges them. And the config a MetricMapping renders is shown rather than described, since that is what makes the selector and the degradation concrete. Signed-off-by: Dennis Ramdass <dennis@upbound.io>
The last commit reintroduced the tic this document was already cleaned of once: announcing how many things are coming before saying any of them. "Two existing pieces make it cheap." "Two things, both worth stating rather than discovering." "Two upgrade notes come with it." "One trap worth naming." Each asks a reader to carry a count through a passage that reads without it, and every one of them introduced bold-led or self-contained paragraphs that needed no introduction. Removed, along with three that predate the change: the four-things opener under What to monitor, "All four are in the central view", and "Five signals matter". Where a count carries weight it stays inside a sentence that says something, as "Two API additions carry it" does. Also dropped three "which is what X" clauses that stopped to explain the significance of a sentence just finished, tidied two bold leads that had split into fragments, and fixed an awkward possessive and a short-wrapped line. Signed-off-by: Dennis Ramdass <dennis@upbound.io>
Explaining the design in plain words broke two sentences, both added in the last two commits. The registry is open and the selector was closed. A platform team can apply a MetricMapping for a forked or unreleased engine, and `engines[].type` was an enum of the kinds Modelplane ships mappings for, so nothing could ever carry the value that mapping selects on. The justification I wrote for closing it, that a mapping can't be selected by a value nothing produces, argues for closing the one thing that has to stay open. It's a free-form string validated as a label value now, and a value with no mapping degrades to passthrough, which is already the behaviour the section describes. The collector gate forbade the fallback. "A cluster with no export destination configured composes no collector" and "where a cluster has no egress the center pulls from it" cannot both hold, because a pulled cluster has no exporter by definition. The gate is the fleet destination rather than a per-cluster exporter: with nowhere configured for the fleet, no cluster composes a collector, and a cluster the center pulls from still delivers. Two things that followed from looking at it that way. Pull mode changes the exporter and not only the transport, since a pulled cluster exposes a scrape endpoint where a pushing one exposes nothing, and the doc said the exporter contract was OTLP without qualification. And a fresh install inverts: today Modelplane installs a working per-cluster store with no aggregation, where after this it aggregates and stores nothing, so an install with no destination collects nothing at all. Right for a fleet, wrong for a first afternoon, which is an argument about the getting started path rather than about this design, but it belongs written down. Signed-off-by: Dennis Ramdass <dennis@upbound.io>
The same test the model delivery design got: write the page a user would read and see whether the API holds up. This one replaces the collecting-engine-metrics guide, since that guide is the manual path this removes. It stays out of docs/content so Hugo doesn't build a page for something unbuilt. Two things it found. Nobody chooses push or pull. The document said the mode is per cluster and reported on status, and never said who decides or how. The draft wrote "Modelplane notices it can't reach out", which is what a reader wants and is not implementable: a composition function does no network probing, so nothing at compose time knows whether a cluster can reach the destination. The platform team declares it, on the InferenceCluster, defaulting to push, with the resolved mode on status and as a printer column. The open destination blocks the docs, not just the build. Configuring one is the first thing a user does, because nothing is collected until it exists, so it is the first thing the page describes, and the draft had to invent a MetricsDestination kind to say anything at all. Recorded as needing a decision before implementation rather than during, with scope and owner attached: the platform team owns it and one serves the fleet, which points cluster-scoped. The page also wants two things the document leaves implicit and the built-in mapping names, `vllm`, `sglang` and something for Triton/TensorRT-LLM, are one of them. Its SGLang queue-depth metric name is a guess and marked as illustrative rather than carried into the design. Signed-off-by: Dennis Ramdass <dennis@upbound.io>
Reading it start to finish rather than around the last edit, on the theory that four rounds of local changes leave stale sentences behind. It found seven, and five were mine. The worst was a contradiction twelve lines wide. One paragraph still called `engines[].type` "an enum of the kinds Modelplane ships a mapping for" while the bullet below it said free-form string and not an enum, because the previous commit fixed the bullet and not the prose that led into it. The same edit had also run two paragraphs together with no blank line between them. The vLLM example mapped `vllm:inter_token_latency_seconds` to `modelplane_time_per_output_token`, collapsing two metrics this document argues at length should stay separate, and it invented `modelplane_kv_cache_utilization` where the vocabulary table and the disaggregation section both say `modelplane_kv_cache_usage`. Every `modelplane_*` name in the document and the docs draft now appears in that table. The table's own vLLM source name for that row disagreed with the one the proof of concept measured, so it takes the measured one. Three claims had gone stale under their own section. "Collect on every cluster, always on" outlived the destination gate that replaced always-on. "One query covers the whole deployment" promises something Modelplane doesn't hold, since it runs no store and exports onward, so the view is whatever consumes the stream. And "Two API additions carry it" was written before the destination and the push-or-pull field existed; there are four, two settled and two needing a decision. Two more bare counts the earlier sweep missed because they sit mid-sentence rather than at the start of a line: "I propose four things" and "for four reasons worth writing down". And three lines that had drifted past the wrap. Signed-off-by: Dennis Ramdass <dennis@upbound.io>
The page was beside the design where nothing checked it. In docs/content it gets the same treatment as any other page: docs-vale lints it, and Hugo would build it if it were allowed to. `draft: true` keeps it out of the site, which Hugo honours by default since buildDrafts isn't set, and a local build confirms it produces no output. The filename says draft too, so it's obvious in a directory listing and easy to drop before merge. Passing docs-vale needed two vocabulary entries and several rewrites. MetricMapping and OTLP go into accept.txt: the first is a kind this design adds, alongside ModelCache and the rest, and the second is a standard acronym. OpenTelemetry goes in with it. The rest were mine to fix rather than to accept, so queue-depth, Pod-scoped, roll-up and Ad-hoc are reworded, PodMonitor in a heading is backticked as it already is in prose, and the opening sentence loses a tricolon. The API line is plain text where every other page links to its reference. Hugo's refLinksErrorLevel defaults to ERROR, and the reference pages are generated from CRDs, so a `ref` to metricmappings would fail the build the moment anyone flips draft to false. Better a dead-looking line than a trap. Signed-off-by: Dennis Ramdass <dennis@upbound.io>
| **Pull through the API server proxy.** The fallback, for a cluster with no egress. The | ||
| center scrapes it over the connection it already has. The Kubernetes API server proxies to | ||
| in-cluster Services, so the collector is reachable at | ||
| `/api/v1/namespaces/<ns>/services/<collector>:<port>/proxy/metrics` with the credential | ||
| Modelplane already holds. No inbound exposure, no firewall change, no second credential; | ||
| the only addition is a `ClusterRole` granting `services/proxy`. |
There was a problem hiding this comment.
Do we want to support 2 modes. Maybe going with just pull from the clusters to the control plane makes more sense.
Makes it also easier considering that we want to roll up all telemetry in one place fleet-wide, as it seems that InferenceGateways are becoming their own clusters as well.
Then we have just one place where to set up exporters and auth for them.
There was a problem hiding this comment.
Scratch that above 😄 . I did some digging and agree with your sentance below that this would be too intensive.
Anyway, i still think we should at least start with 1 mode only to keep things simpler, and that should be the push one.
There was a problem hiding this comment.
Agreed, and it's out. One transport, push, and no field selecting it.
You talked me into it with the retraction rather than the original suggestion: once pull-only is off the table, the second mode is a fallback whose own analysis in this section admitted it was degraded, and I'd still have been paying for it in full. It isn't only a second transport, either. A pulled cluster exposes a scrape endpoint where a pushing one exports and exposes nothing, so it's a second collector configuration, a second exporter shape, a ClusterRole granting services/proxy, and a field with a status mirror and a printer column.
What makes dropping it easy is that deferring is free. The field would be optional and default to push, so adding it the day a cluster that genuinely can't egress turns up is additive and breaks nobody. So there's nothing to migrate later and no reason to build it now.
I've kept the analysis in the section rather than deleting it: what would be built, who declares the mode, and why Modelplane can't detect it for itself, since a composition function does no network probing. Pull direct stays ruled out on its own terms. The diagram now has both clusters pushing, and the API-change count drops from four to three.
|
|
||
| It blocks more than the implementation. Configuring a destination is the first thing a user | ||
| does, since nothing is collected until one exists, so it is the first thing the docs | ||
| describe, and a draft of that page had to invent a `MetricsDestination` kind to say |
There was a problem hiding this comment.
NIT: suggest going with TelemetryDestination/ObservabilityDestination instead, to keep it open for more signals in the future
There was a problem hiding this comment.
Taking it, and it's cheaper than a nit. TelemetryDestination.
Two things push it past preference. The name was never really proposed: it's a placeholder a draft of the docs page had to invent to have something to write, on a question the doc explicitly leaves open, so there's nothing to reverse. And an OTLP endpoint carries metrics, logs and traces on the same wire, so the destination genuinely is signal-agnostic already. MetricsDestination would describe it narrower than it is, which makes the broader name more accurate rather than more speculative.
Also the asymmetry: the rename is free while nothing is built, and costs a deprecation window plus a migration for every user once something is.
I went with telemetry over observability since that's OTel's own word for the data, where observability is the property. And the answer holds whichever shape it lands in, so the doc now says TelemetryDestination as a kind or spec.telemetry.destination as a field, with the kind-or-field question still open.
Two review points from Lovro, both of which shrink the design. The transport section offered push as the default and a pull through the API server proxy as a fallback for a cluster with no egress, selected by a field on the InferenceCluster. Its own analysis undercut it: every series would cross an API server not built to carry them, which caps what a cluster in that mode could send. And a second mode isn't only a second transport, because a pulled cluster exposes a scrape endpoint where a pushing one exports and exposes nothing, so it costs a second collector configuration, a second exporter shape, a ClusterRole granting services/proxy, and a field with a status mirror and a printer column. So there is one transport and no field selecting it. What makes that easy is that deferring costs nothing: the field would be optional and default to push, so adding it when a cluster that can't egress actually turns up is additive and breaks nobody. The analysis stays in the section, as what would be built and who would declare it, including why Modelplane can't detect the mode itself. Pull direct stays ruled out on its own terms. The architecture diagram has both clusters pushing, and the API-change count drops from four to three. The destination's name is settled ahead of its shape: telemetry, not metrics. An OTLP endpoint carries metrics, logs and traces on the same wire, so the destination is already signal-agnostic and MetricsDestination would describe it narrower than it is. TelemetryDestination as a kind, or spec.telemetry.destination as a field, with the kind-or-field question still open. The rename is free while nothing is built and costs a deprecation window and a migration for every user once something is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
4 participants
Description of your changes
A design doc, and a draft docs page. No code. Addresses #269.
Collecting a deployment's metrics is hand-wired today. vLLM exposes
/metricson its serving port, the serving stack runs a Prometheus per workload cluster with openPodMonitordiscovery, and an operator writes aPodMonitorby hand, keeps it in sync with the serving shape, and removes it on teardown. Each engine also names its metrics its own way, so an operator reading two deployments reads two vocabularies, and each cluster is an island they stitch together themselves.The doc proposes four things:
Collect on every cluster. Modelplane collects from every source it owns — engines, endpoint pickers, the substrate — with no per-deployment toggle. The switch is at the fleet: with no destination configured anywhere, no cluster composes a collector, since a collector nothing reads is cost with no reader. A cluster-wide selector on
modelplane.ai/servingalready spans standalone, leader/worker, Grove cliques and prefill/decode, so the shape needs no special casing. Scraping the engine port by name rather than number also fixes a latent prefill/decode gap — the decode engine serves on 8001 because the pd-sidecar takes 8000 — and needs a change first, since no backend names that port today.Normalize to
modelplane_*. AMetricMappingkind carries the rename per engine, selected by amodelplane.ai/enginelabel that Modelplane stamps from a newengines[].type. The label can't come from the user: the XRD reserves themodelplane.ai/prefix. Built-ins for vLLM, SGLang and Triton/TensorRT-LLM; a platform team applies one more for a forked engine, with no Modelplane release. An unmapped engine still gets scraped under its native names and Modelplane says so rather than guessing.Aggregate to one view. Each cluster's collector pushes outbound by default, so a workload cluster needs only egress and nothing inbound. Where a cluster has no egress, the center pulls it through the API server proxy, over the kubeconfig connection Modelplane already holds. The roll-up is computed in memory, so Modelplane runs no store and the control plane stays stateless, as running in a Space requires.
Collect with OpenTelemetry, replacing the per-cluster Prometheus. The normalization target is already an OpenTelemetry standard, the rename happens in the pipeline instead of through per-cluster recording rules, and one pipeline carries metrics, the #77 traces, and logs. Each thing kube-prometheus-stack does today has a receiver that does it, and the Envoy scrape config we already have moves across verbatim.
Approving this means agreeing that normalization and aggregation are Modelplane's job rather than the platform team's, that collection is on for every source once a destination exists, and that the collector is OpenTelemetry in place of the Prometheus stack we install today.
The draft docs page
docs/content/guides/draft-collect-metrics.mdis docs-driven development: the page a user would read, written to check the API before it's built. It'sdraft: trueso Hugo excludes it, it's named draft so it's obvious in a listing, and it should be removed before this merges.Writing it earned its place. It found that nothing in the design chose push or pull — the page said "Modelplane notices it can't reach out", which is what a reader wants and isn't implementable, since a composition function does no network probing. And it found that leaving the metrics destination's shape open blocks the docs, not just the build: configuring one is the first thing a user does, so the draft had to invent a
MetricsDestinationkind to say anything at all.It also needed two vocabulary entries,
MetricMappingandOTLP, which are in this PR.Status
Nothing here is built. The
MetricMappingkind and the collector that reads it were proven in #412 against vLLM 0.23.0 on a real GKE cluster, and that PR is closed unmerged waiting on this design; the branchdennis/metrics-pocstays.engines[].typepostdates it and is unproven.Where I want judgement
Four API changes carry this and two aren't settled: where a fleet's metrics destination lives (a field or its own kind, the same call
MetricMappingfaced), and the field that says whether a cluster pushes or is pulled from.Beyond those: whether the node-scoped DaemonSet tier —
kubeletstatsandhostmetrics, replacing cAdvisor and node-exporter — belongs in the first cut, or whether engines, EPP andk8s_clusterare enough. The engine port name (httpagainst a dedicatedmetrics). Disabling EPP metrics auth against composing theClusterRoleand token. And whether the fleet roll-up series are the right set.Two upgrade notes come out of removing the Prometheus stack. A hand-written
PodMonitorleft in place goes inert rather than double-scraping, which is quieter and worse. And an operator relying on that Prometheus for anything of their own loses it, so a release note has to say where the series go instead.I have:
Run(design doc and a draft docs page, no code paths)nix flake checkAdded or updated tests(no code)git commit -s.🤖 Generated with Claude Code