Design: managed metrics collection - #363
Conversation
Collecting a deployment's metrics is hand-wired: an operator writes a PodMonitor per deployment, keeps it in sync with the serving shape (leader/worker, prefill/decode), and removes it on teardown. This design has Modelplane compose the collection instead, per source it owns, the engine and the endpoint picker where present, on by default with an opt-out. It covers modelplaneai#269, with a section per source and a diagram. Signed-off-by: Dennis Ramdass <dennis@upbound.io>
|
|
||
| ## 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.
Align the opt-out field with modelplaneai#77 by nesting it at observability.metrics.enabled, so traces and logs can join it later. Clarify that the 30s scrape is an observability default and not a routing input (the EPP scrapes engines on its own fast loop), that cache/PVC observability is out of scope, and that an existing hand-written PodMonitor must be deleted on upgrade. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
|
@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. |
Adopt Nic's direction: step back to what's worth monitoring (data plane, substrate, control plane, fleet roll-up), make PodMonitor collection always-on at every layer instead of a per-deployment opt-out, and add a central Modelplane Prometheus that the per-cluster instances feed and that also scrapes the control plane. Drops the observability.metrics.enabled toggle. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
|
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. |
Pull the design back from a central aggregated Prometheus to what this layer should own: always-on PodMonitors inside each InferenceCluster and a Prometheus URL on the cluster status, so the platform team can scrape or federate without reaching into Modelplane internals. Leave cross-cluster aggregation and control-plane monitoring to the platform team, as the main open question. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
|
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.) |
Agree the per-replica composition earns nothing once collection is always-on, so compose one cluster-wide PodMonitor per source in the serving stack instead. State the proposal and what approving it covers up front, answer how the per-cluster Prometheus is reached from outside (the platform team's existing cross-cluster path; Modelplane only publishes the URL), and drop the opt-in mention that was reacting to the earlier draft. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
Reverse the earlier leave-aggregation-to-the-platform-team scope: aggregate every cluster's metrics up to one Modelplane store at the control plane, with recording rules rebranding every series under modelplane_* and a normalized label set. Weigh two collection mechanisms, the incumbent Prometheus stack and an OpenTelemetry collector, and lean toward the collector. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
|
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:
|
…rmalization The doc left the collector as an open Prometheus-vs-OTel choice and asserted a modelplane_* namespace without saying how metrics from an engine Modelplane doesn't recognize become modelplane_* series. Commit to the OpenTelemetry collector, with the reasons (the GenAI conventions are the naming target, the rename runs in-pipeline, one pipeline carries metrics, traces, and logs). Add a capture section: an engine exposes Prometheus /metrics, an engine-type label picks a mapping from an extensible registry the way the GAIE picker already selects one for routing, and an unmapped engine degrades to raw names rather than a wrong guess. Add the normalized metric set and the per-engine mapping table with TTFT, ITL, TPOT, and the prefill/decode split. Move the Prometheus stack to a rejected alternative and re-tone throughout. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
|
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. |
… metrics Add input and output sequence length to the normalized set, the two disaggregation bottleneck signals (queued prefill tokens, in-flight decode KV tokens), and a note that an autoscaler or SLA planner such as NVIDIA's Dynamo Planner reads these faster than a dashboard, so the scrape interval is a knob. Add SLO attainment to the fleet roll-up. Add a Cluster scheduler metrics section: the pod scheduler (kube-scheduler, or a gang scheduler like NVIDIA KAI or Volcano) is captured the way an engine is, a per-scheduler mapping normalized to modelplane_cluster_scheduler_*, with the name reserving modelplane_fleet_scheduler_* for a future fleet scheduler. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
Fix five things a close read surfaced. A managed cluster's kube-scheduler runs in the provider's control plane and may not be scrapable, so say so rather than claim the collector reaches it. Drop the Dynamo-specific forward-pass-metrics label from the general engine signal. Reword so the Dynamo Planner reads as the reference pattern for a consumer of the normalized series, not a consumer of them. Add a request-outcome row so the table matches the error rate the doc promises. Call the control-plane scheduler the fleet scheduler now that a cluster scheduler exists. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
Take a stance on each rather than leave it open. The central store is a single Prometheus-compatible instance, moving to a horizontally scaled backend such as Mimir when one instance can't hold the fleet. The mapping registry is a ConfigMap the collector reads, with a CRD reserved for outside authors who need validation. The metrics port is http, since it is the one serving port. Drop the Open questions section. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
The store is one Prometheus-compatible instance with a short retention window, and scaling it horizontally is out of scope, so drop the Mimir path. Move the mapping registry from a hand-edited ConfigMap to the serving-stack Composition, which renders the collector's config and versions the mappings with the package; a platform team extends the set through composition input. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Dennis Ramdass <dennis@upbound.io>
06d0dbe to
3007f9a
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>
3c04631 to
5071242
Compare
…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>
Description of your changes
A design doc, not code yet — opening as a draft for direction before implementation.
Collecting a deployment's metrics is hand-wired today (#269): 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 (leader/worker, prefill/decode), and removes it on teardown (the #264 example).This proposes having Modelplane compose the collection, per source it owns, on by default with an opt-out (
spec.template.spec.metrics.enabled):compose-model-replicacomposes aPodMonitorselecting the replica's serving pods, scraping the engine's/metricsby a named port. Naming the port fixes a latent prefill/decode gap: the decode engine serves on 8001 (the pd-sidecar takes 8000), so the manualtargetPort: 8000scrapes the sidecar, not the engine.llm_d_epp_*metrics too. Since we own the EPP Deployment (llm-d-router-endpoint-picker:v0.9.0), we set--metrics-endpoint-auth=falseand scrape:9090with a plainPodMonitor— no ClusterRole, token, or TLS.Managed lifecycle and shape-syncing fall out of the existing serving label and open Prometheus discovery.
Where I'd like judgment (in the doc): the metrics field shape and default-on, the engine port name (
httpvsmetrics), and disabling EPP metrics auth vs composing RBAC. Two open questions are flagged.design/metrics.mdonly; no code.I have:
Run(design doc only, no code paths)nix flake check(or./nix.sh flake check) and made sure it passes.Added or updated tests covering any composition function changes.(design doc only)git commit -s.