Repository navigation
fix(metrics): preserve colons in /engine_metrics metric names - #2636
Conversation
openmetrics_parser rejects colons in metric names, so the aggregator
replaced every ':' in the scraped text with '_'. That renamed the engines'
`sglang:*` and `vllm:*` metrics, rewrote colons inside label values and
HELP text (e.g. model_name="org/model:v1"), and merged distinct `a:b` and
`a_b` families into one.
Swap colons for an escape sequence before parsing and restore them in the
rendered output. The escape is `xsmgcolon{n}z` with the first n that does
not already occur in any scraped text or extra label, so a literal escape
in the input is never rewritten; only its first character is `x`, so
adjacent escapes cannot combine into a false match.
Ported from sgl-project/sglang#37985.
Signed-off-by: James Liu <jamesl@modal.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughMetric aggregation now preserves colons in metric names and label values by using an unused escape string during parsing and restoring colons in the output. Regression tests cover colon and underscore name collisions, literal escape strings, and expected aggregated series. ChangesMetric Aggregation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to The 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR stack (merge bottom-up; each PR is based on
mainand includes the PRs below it)Bottom of the stack: review and merge this first.
Description
Problem
/engine_metricsrenames every engine metric that contains a colon.openmetrics_parserrejects colons in metric names, soaggregate_metricsreplaced every:in the scraped text with_. As a result:sglang:*and vLLM'svllm:*metrics come out assglang_*/vllm_*, so a query that works against an engine's/metricsdoesn't work against the gateway.model_name="org/model:v1"becomesorg/model_v1.a:banda_bare merged into one.Solution
Before parsing, swap each colon for an escape sequence, and swap it back in the rendered output. The escape is
xsmgcolon{n}z, using the firstnthat appears nowhere in the scraped text or the extra labels. That way a literal copy of the escape in the input is never rewritten. Only the first character isx, so two adjacent escapes can't combine into a false match.Ported from sgl-project/sglang#37985.
Changes
model_gateway/src/worker/metrics_aggregator.rs: escape and restore colons instead of replacing them with_.model_gateway/tests/metrics_aggregator_test.rs:test_realnow expectssglang:names. New tests cover colons in names, HELP text and label values;a:bvsa_btwins plus leading (:x) and doubled (a::b) colons; and a literal escape in the input.No dashboards, docs or examples in this repo query
sglang_*engine metrics, so nothing else needs updating.Test Plan
On
main(only the aggregator source reverted), the new and updated tests fail:Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses (run without--all-features/ OpenCV:cargo clippy --workspace --all-targets -- -D warnings)