fix(observability): include gRPC workers in GET /engine_metrics - #1779
fix(observability): include gRPC workers in GET /engine_metrics#1779slin1237 wants to merge 1 commit into
Conversation
slin1237
added
enhancement
github-actions
Bot
added
tests
📝 WalkthroughWalkthroughAdds gRPC worker metrics endpoint discovery, worker-level ChangesgRPC engine metrics scraping
Sequence Diagram(s)sequenceDiagram
participant Client
participant MetadataDiscovery
participant WorkerManager
participant Worker
participant MetricsEndpoint
Client->>MetadataDiscovery: discover gRPC worker labels
MetadataDiscovery->>MetadataDiscovery: derive_grpc_metrics_url()
MetadataDiscovery-->>WorkerManager: workers with metrics_url labels
WorkerManager->>WorkerManager: get_engine_metrics()
WorkerManager->>Worker: metrics_url()
WorkerManager->>MetricsEndpoint: GET /metrics
MetricsEndpoint-->>WorkerManager: response or error
WorkerManager->>WorkerManager: record_engine_metrics_scrape()
WorkerManager-->>Client: aggregated metrics or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
👋 The PR description doesn't fully follow
Please update the PR description so reviewers have the context they need. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for scraping engine /metrics from gRPC workers by resolving their scrape endpoints via metadata discovery (such as metrics_url or prometheus_port labels) and fanning out the requests in parallel. It also adds tracking metrics for scrape attempts and comprehensive tests. The feedback highlights a performance bottleneck where the response bodies (r.text().await) are downloaded sequentially inside a loop, negating the benefits of the parallel fan-out. It is recommended to fetch the response text concurrently within the parallel futures of scrape_engine_metrics as suggested.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-structured PR — clean refactoring from a generic fan-out to a purpose-specific scrape, good separation of concerns across discovery/worker/manager, solid test coverage (unit + integration), and proper observability with the new counter. Three minor nits posted (IPv6 in grpc_host, mixed skip+fail error message, test TOCTOU race), none blocking.
Summary: 0 🔴 Important · 3 🟡 Nit · 0 🟣 Pre-existing
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c1017a580
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@model_gateway/src/worker/manager.rs`:
- Around line 84-87: The bearer_auth call in the manager is applying the api_key
to all metrics_url requests unconditionally, but for discovery-driven gRPC
worker metrics endpoints, this can leak backend credentials to untrusted
endpoints. Modify the code to conditionally apply bearer_auth only when the
metrics_url is not from a discovery-driven gRPC worker source, ensuring that
api_key authentication is skipped for metrics endpoints discovered through gRPC
worker discovery mechanisms to prevent credential leakage to potentially
untrusted endpoints.
In `@model_gateway/src/workflow/steps/local/discover_metadata.rs`:
- Around line 320-322: The grpc_host extraction currently uses split_once(':')
which splits on the first colon, truncating bracketed IPv6 addresses like
[2001:db8::1]:30001 into an invalid host. Replace split_once(':') with
rsplit_once(':') to split on the last colon instead, which will correctly
extract the host portion while preserving the full IPv6 address in brackets.
This way the port is properly separated from the IPv6 host without breaking the
address format.
- Around line 276-290: The code currently accepts metrics_url values and
constructs metrics endpoints from host values in gRPC metadata without
validating that these endpoints are trusted. After constructing the resolved
metrics endpoint URL (in the `resolved` variable), add validation to ensure the
endpoint's host and scheme match the worker's expected origin (which can be
derived from grpc_url and grpc_host). Extract and compare the hostname from the
resolved endpoint URL against the expected worker origin, and only return the
resolved endpoint if it passes validation; otherwise, skip or reject the metrics
URL. This prevents a malicious backend from using metrics scraping as an
outbound pivot point.
In `@model_gateway/tests/engine_metrics_grpc_test.rs`:
- Around line 32-35: Replace the std::net::TcpListener with
tokio::net::TcpListener to avoid the race condition of binding, dropping, and
rebinding the same port. Instead of extracting the port from the listener and
then dropping it, keep the tokio::net::TcpListener instance alive and pass it
directly to the server startup code. Remove the drop(listener) call and
eliminate any fixed sleep delays in the test server initialization, using the
listener's assigned address directly throughout the test setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 138e6511-4805-4f17-a2ab-a07dc7902ecf
📒 Files selected for processing (6)
model_gateway/src/observability/metrics.rsmodel_gateway/src/routers/grpc/client.rsmodel_gateway/src/worker/manager.rsmodel_gateway/src/worker/worker.rsmodel_gateway/src/workflow/steps/local/discover_metadata.rsmodel_gateway/tests/engine_metrics_grpc_test.rs
…rape (#1779) Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
8c1017a to
a3cd00b
Compare
|
Addressed review feedback (new commit
Skipped (intentionally): coderabbit's " Rebased onto |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3cd00baf8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@model_gateway/src/worker/worker.rs`:
- Around line 706-712: The Prometheus metrics URL construction in the format!
macro on line 711 does not account for IPv6 addresses, which require brackets
when used in URLs. Modify the URL construction to check if the host is an IPv6
address and wrap it with brackets accordingly, so that addresses like `::1` are
properly formatted as `[::1]` in the final URL instead of producing invalid URLs
like `http://::1:9100/metrics`.
In `@model_gateway/src/workflow/steps/local/discover_metadata.rs`:
- Around line 286-288: The metrics_url construction in the format strings at
lines 287 and 294 creates invalid IPv6 URLs because the grpc_host function
strips brackets from IPv6 addresses. When h is an IPv6 address like ::1, the
resulting URL becomes http://::1:9100/metrics which is malformed. Fix this by
wrapping the host variable h in square brackets when constructing the
metrics_url format strings (change format!("http://{h}:{p}/metrics") to
format!("http://[{h}]:{p}/metrics")), and apply the same fix to all occurrences
including the test assertions around line 587-594 that currently expect the
malformed format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bead844a-5155-4dbe-bff6-41437e64d7a9
📒 Files selected for processing (6)
model_gateway/src/observability/metrics.rsmodel_gateway/src/routers/grpc/client.rsmodel_gateway/src/worker/manager.rsmodel_gateway/src/worker/worker.rsmodel_gateway/src/workflow/steps/local/discover_metadata.rsmodel_gateway/tests/engine_metrics_grpc_test.rs
Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
|
Round-2 review fixes:
Resolved 11 review threads addressed across round-1 + round-2 (sequential body fmt-only locally ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb6ae92410
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…scovery (#1779) Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
GET /engine_metrics fanned out HTTP GET {url}/metrics to every worker;
gRPC workers yielded grpc://host:port/metrics, failed, and were silently
dropped, so an all-gRPC fleet returned "All backend requests failed".
Make the scrape transport-aware:
- Discover a gRPC worker's HTTP metrics endpoint from GetServerInfo
server_args during metadata discovery, stored as a metrics_url label
(kept out of WorkerSpec). The scrape host comes from the gRPC worker
URL, and an explicit metrics_url is honored only when its host matches,
so a backend cannot redirect the scrape (and its bearer token) to a
foreign origin. IPv6 hosts are bracketed.
- Add Worker::metrics_url(); the scrape branches per connection_mode.
Workers with no known endpoint are skipped and counted, not dropped.
- Collapse DP ranks that share one engine endpoint so it is scraped once
rather than dp_size times, which would inflate every summed series.
- Add counter smg_engine_metrics_scrape_total{connection_mode,result}
and replace the blanket failure message when the cause is "no endpoint".
Part of #1773. Closes #1774.
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
2584add to
938eab3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 938eab34ea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "prometheus_port", | ||
| "enable_metrics", | ||
| "host", | ||
| "port", |
There was a problem hiding this comment.
For current SGLang --grpc-mode --enable-metrics, the HTTP /metrics endpoint is on grpc_http_sidecar_port (docs: “Serves Prometheus metrics and profiling endpoints. Defaults to --port + 1”), not on port, which is the gRPC listener. Because this extraction list omits that key, discovery falls through to the port fallback and publishes http://<worker>:<grpc-port>/metrics; /engine_metrics then sends an HTTP request to the gRPC socket and records the scrape as failed unless users manually set metrics_url/prometheus_port.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@model_gateway/src/routers/grpc/client.rs`:
- Around line 838-845: The SGLang discovery path lacks direct coverage for the
new grpc discovery keys, so a typo in SGLANG_GRPC_KEYS could go unnoticed. Add a
focused test around the SGLang extraction flow in client.rs that exercises
pick_prost_fields(..., SGLANG_GRPC_KEYS) and verifies it surfaces metrics_url,
prometheus_port, enable_metrics, host, and port as expected; use
derive_grpc_metrics_url and the SGLANG_GRPC_KEYS constant to locate the code
path and confirm the short-form pick_prost_fields output is preserved without
later normalization.
In `@model_gateway/src/worker/worker.rs`:
- Around line 712-726: The fallback in Worker::grpc_metrics_url currently
returns a non-empty metrics_url label as-is, which bypasses the host-pinning
used by discover_metadata::derive_grpc_metrics_url(). Update this method to
validate or normalize the label against the worker’s allowed host before
returning it, and only expose it when it matches the pinned host; otherwise fall
back to the derived http://{metrics_authority(host, port)}/metrics path or None.
Also review the metrics_url_grpc_prefers_explicit_label test to ensure it no
longer accepts an arbitrary external label like grpc://w:30001 →
http://sidecar:9000/metrics.
- Around line 705-709: The HTTP branch of `Worker::metrics_url()` is building
scrape targets from `self.spec.url`, which can still include the DP `@rank`
suffix and produce invalid metrics URLs. Update the `ConnectionMode::Http` path
to construct the metrics endpoint from `self.base_url()` or
`self.endpoint_url("/metrics")` instead of `self.spec.url`, while leaving the
`Grpc` branch unchanged. This should ensure `scrape_engine_metrics()` gets a
clean request URL and deduplicates correctly per engine.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 013fb0af-c0be-4089-9b90-3bd676b26bf5
📒 Files selected for processing (6)
model_gateway/src/observability/metrics.rsmodel_gateway/src/routers/grpc/client.rsmodel_gateway/src/worker/manager.rsmodel_gateway/src/worker/worker.rsmodel_gateway/src/workflow/steps/local/discover_metadata.rsmodel_gateway/tests/engine_metrics_grpc_test.rs
| // Engine /metrics scrape discovery (W1). Consumed by | ||
| // `discover_metadata::derive_grpc_metrics_url` to compute the `metrics_url` | ||
| // label; never scraped unless `enable_metrics` is truthy. | ||
| "metrics_url", | ||
| "prometheus_port", | ||
| "enable_metrics", | ||
| "host", | ||
| "port", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a direct SGLang extraction test for these new discovery keys.
derive_grpc_metrics_url tests currently build label maps by hand, so they never prove that pick_prost_fields(..., SGLANG_GRPC_KEYS) actually surfaces metrics_url, prometheus_port, enable_metrics, host, and port. A typo here would break SGLang scraping while the new discovery tests still stay green. Based on learnings, SGLang uses the short-form pick_prost_fields() path without later normalization, so this exact path needs direct coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@model_gateway/src/routers/grpc/client.rs` around lines 838 - 845, The SGLang
discovery path lacks direct coverage for the new grpc discovery keys, so a typo
in SGLANG_GRPC_KEYS could go unnoticed. Add a focused test around the SGLang
extraction flow in client.rs that exercises pick_prost_fields(...,
SGLANG_GRPC_KEYS) and verifies it surfaces metrics_url, prometheus_port,
enable_metrics, host, and port as expected; use derive_grpc_metrics_url and the
SGLANG_GRPC_KEYS constant to locate the code path and confirm the short-form
pick_prost_fields output is preserved without later normalization.
Source: Learnings
| pub fn metrics_url(&self) -> Option<String> { | ||
| match self.spec.connection_mode { | ||
| ConnectionMode::Http => Some(format!("{}/metrics", self.spec.url)), | ||
| ConnectionMode::Grpc => self.grpc_metrics_url(), | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use base_url() for HTTP metrics targets.
Line 707 builds /metrics from self.spec.url, but DP-aware HTTP workers keep the internal @rank suffix there. scrape_engine_metrics() uses worker.metrics_url() as the actual request URL, so this produces invalid targets like http://w:8080@1/metrics and also defeats per-engine deduplication. Build the HTTP scrape URL from self.base_url() (or self.endpoint_url("/metrics")) instead.
🛠️ Minimal fix
pub fn metrics_url(&self) -> Option<String> {
match self.spec.connection_mode {
- ConnectionMode::Http => Some(format!("{}/metrics", self.spec.url)),
+ ConnectionMode::Http => Some(self.endpoint_url("/metrics")),
ConnectionMode::Grpc => self.grpc_metrics_url(),
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn metrics_url(&self) -> Option<String> { | |
| match self.spec.connection_mode { | |
| ConnectionMode::Http => Some(format!("{}/metrics", self.spec.url)), | |
| ConnectionMode::Grpc => self.grpc_metrics_url(), | |
| } | |
| pub fn metrics_url(&self) -> Option<String> { | |
| match self.spec.connection_mode { | |
| ConnectionMode::Http => Some(self.endpoint_url("/metrics")), | |
| ConnectionMode::Grpc => self.grpc_metrics_url(), | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@model_gateway/src/worker/worker.rs` around lines 705 - 709, The HTTP branch
of `Worker::metrics_url()` is building scrape targets from `self.spec.url`,
which can still include the DP `@rank` suffix and produce invalid metrics URLs.
Update the `ConnectionMode::Http` path to construct the metrics endpoint from
`self.base_url()` or `self.endpoint_url("/metrics")` instead of `self.spec.url`,
while leaving the `Grpc` branch unchanged. This should ensure
`scrape_engine_metrics()` gets a clean request URL and deduplicates correctly
per engine.
| fn grpc_metrics_url(&self) -> Option<String> { | ||
| let labels = &self.spec.labels; | ||
| if let Some(url) = labels.get("metrics_url").filter(|s| !s.is_empty()) { | ||
| return Some(url.clone()); | ||
| } | ||
| // Discovery sets an explicit `metrics_url` whenever one is derivable, so | ||
| // this is a fallback for workers configured out-of-band. Use the worker's | ||
| // own URL host (`bootstrap_host`, parsed from `spec.url`); skip when it's | ||
| // empty rather than emitting a hostless scrape target. | ||
| let port = labels.get("prometheus_port").filter(|s| !s.is_empty())?; | ||
| let host = self.spec.bootstrap_host.as_str(); | ||
| if host.is_empty() { | ||
| return None; | ||
| } | ||
| Some(format!("http://{}/metrics", metrics_authority(host, port))) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not re-trust raw gRPC metrics_url labels here.
discover_metadata::derive_grpc_metrics_url() already host-pins discovered endpoints, but this fallback returns any non-empty metrics_url label unchanged. That reopens the SSRF / credential-leak path for statically registered gRPC workers because scrape_engine_metrics() will fetch that URL and still attach the worker bearer token. The current metrics_url_grpc_prefers_explicit_label test also codifies grpc://w:30001 → http://sidecar:9000/metrics, which bypasses the PR’s host-pinning guarantee. Mirror the same host validation here before exposing the label through Worker::metrics_url().
🛡️ Minimal hardening
fn grpc_metrics_url(&self) -> Option<String> {
let labels = &self.spec.labels;
+ let host = self.spec.bootstrap_host.as_str();
if let Some(url) = labels.get("metrics_url").filter(|s| !s.is_empty()) {
- return Some(url.clone());
+ let same_host = url::Url::parse(url)
+ .ok()
+ .and_then(|u| u.host_str())
+ .map(|h| h.trim_start_matches('[').trim_end_matches(']'))
+ == Some(host);
+ if same_host {
+ return Some(url.clone());
+ }
}
// Discovery sets an explicit `metrics_url` whenever one is derivable, so
// this is a fallback for workers configured out-of-band. Use the worker's
// own URL host (`bootstrap_host`, parsed from `spec.url`); skip when it's
// empty rather than emitting a hostless scrape target.
let port = labels.get("prometheus_port").filter(|s| !s.is_empty())?;
- let host = self.spec.bootstrap_host.as_str();
if host.is_empty() {
return None;
}
Some(format!("http://{}/metrics", metrics_authority(host, port)))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn grpc_metrics_url(&self) -> Option<String> { | |
| let labels = &self.spec.labels; | |
| if let Some(url) = labels.get("metrics_url").filter(|s| !s.is_empty()) { | |
| return Some(url.clone()); | |
| } | |
| // Discovery sets an explicit `metrics_url` whenever one is derivable, so | |
| // this is a fallback for workers configured out-of-band. Use the worker's | |
| // own URL host (`bootstrap_host`, parsed from `spec.url`); skip when it's | |
| // empty rather than emitting a hostless scrape target. | |
| let port = labels.get("prometheus_port").filter(|s| !s.is_empty())?; | |
| let host = self.spec.bootstrap_host.as_str(); | |
| if host.is_empty() { | |
| return None; | |
| } | |
| Some(format!("http://{}/metrics", metrics_authority(host, port))) | |
| fn grpc_metrics_url(&self) -> Option<String> { | |
| let labels = &self.spec.labels; | |
| let host = self.spec.bootstrap_host.as_str(); | |
| if let Some(url) = labels.get("metrics_url").filter(|s| !s.is_empty()) { | |
| let same_host = url::Url::parse(url) | |
| .ok() | |
| .and_then(|u| u.host_str()) | |
| .map(|h| h.trim_start_matches('[').trim_end_matches(']')) | |
| == Some(host); | |
| if same_host { | |
| return Some(url.clone()); | |
| } | |
| } | |
| // Discovery sets an explicit `metrics_url` whenever one is derivable, so | |
| // this is a fallback for workers configured out-of-band. Use the worker's | |
| // own URL host (`bootstrap_host`, parsed from `spec.url`); skip when it's | |
| // empty rather than emitting a hostless scrape target. | |
| let port = labels.get("prometheus_port").filter(|s| !s.is_empty())?; | |
| if host.is_empty() { | |
| return None; | |
| } | |
| Some(format!("http://{}/metrics", metrics_authority(host, port))) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@model_gateway/src/worker/worker.rs` around lines 712 - 726, The fallback in
Worker::grpc_metrics_url currently returns a non-empty metrics_url label as-is,
which bypasses the host-pinning used by
discover_metadata::derive_grpc_metrics_url(). Update this method to validate or
normalize the label against the worker’s allowed host before returning it, and
only expose it when it matches the pinned host; otherwise fall back to the
derived http://{metrics_authority(host, port)}/metrics path or None. Also review
the metrics_url_grpc_prefers_explicit_label test to ensure it no longer accepts
an arbitrary external label like grpc://w:30001 → http://sidecar:9000/metrics.
|
This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you! |
|
Hi @slin1237, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
1 participant
Part of #1773. Closes #1774.
What
GET /engine_metricsfanned out HTTPGET {url}/metricsto every worker; gRPC workers yieldedgrpc://host:port/metrics, failed, and were silently dropped (an all-gRPC fleet returned "All backend requests failed"). This makes the scrape transport-aware:GetServerInfo.server_argsduring metadata discovery, stored as ametrics_urllabel. Precedence: explicitmetrics_url→prometheus_port(+host) → derivedhttp://{host}:{port}/metrics, only whenenable_metricsis truthy (never a dark port). Kept out ofWorkerSpec(protocols) per the iron law — it rides the label pipeline.Worker::metrics_url()accessor;scrape_engine_metricsbranches perconnection_mode. Workers with no known endpoint are skipped and counted, not dropped.smg_engine_metrics_scrape_total{connection_mode,result}(success/failure/skipped); the blanket "All backend requests failed" is replaced when the real cause is "no endpoint known".metrics_aggregator.rs) unchanged.Always-on (no flag). Works for upstream engines (e.g. SGLang) that already expose
/metrics; SMG's own servicers gain it via W4 (#1777).Tests
Unit (server_args→metrics_url; per-mode URL selection) + integration (
engine_metrics_grpc_test.rs: gRPC stub advertisingmetrics_url+ stub/metricsmerges into/engine_metrics). Verified green in an isolated target dir; CI is authoritative.Cross-workstream notes
observability/metrics.rsandrouters/grpc/client.rswith W2 ([Metrics] W2: normalized engine + PD re-export from GetLoads (smg_engine_*) #1775) / W3 ([Metrics] W3: SMG-measured PD instrumentation (smg_pd_*) #1776) — additive edits; expect trivial merge conflicts when stacking.metrics_port/metrics_url; this PR allowlistsmetrics_url/prometheus_portinSGLANG_GRPC_KEYSonly. The routablemetrics_urlpath already matches end-to-end for SGLang. Follow-ups: allowlist ametrics_portalias and extendTOKENSPEED_GRPC_KEYS.WorkerMetadata::grpc_metrics_url()'sprometheus_portfallback derives the host frombootstrap_host, while discovery derives it from the gRPC URL — worth consolidating (discovery normalizesprometheus_port→metrics_url, so the fallback is effectively dead in prod).Summary by CodeRabbit
Release Notes
New Features
/metricsscraping observability with per-worker labeled outcomes (success, failure, skipped).metrics_urlprecedence, and per-worker metrics URL support.Bug Fixes
get_engine_metricserror reporting to clearly distinguish “all targets skipped (no endpoint)” from usable scrape failures.Tests