-
Notifications
You must be signed in to change notification settings - Fork 690
feat: enable custom metrics prefix #2432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| // SPDX-FileCopyrightText: Copyright (c) 2024-2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| use dynamo_llm::http::service::metrics::{self, Endpoint}; | ||
| use dynamo_llm::http::service::service_v2::HttpService; | ||
| use dynamo_runtime::CancellationToken; | ||
| use serial_test::serial; | ||
| use std::{env, time::Duration}; | ||
|
|
||
| #[tokio::test] | ||
| #[serial] | ||
| async fn metrics_prefix_default_then_env_override() { | ||
| // Case 1: default prefix | ||
| env::remove_var(metrics::METRICS_PREFIX_ENV); | ||
| let svc1 = HttpService::builder().port(9101).build().unwrap(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see this looks like it's following examples in http-service.rs, and a bit sad that many tests are not using random available ports (port 0). For tests, it's best to use random available ports so that tests running in parallel, don't run into collisions with other existing services and/or tests. Something like this would be preferred: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will use random ports in the future. |
||
| let token1 = CancellationToken::new(); | ||
| let _h1 = svc1.spawn(token1.clone()).await; | ||
| wait_for_metrics_ready(9101).await; | ||
|
|
||
| // Populate labeled metrics | ||
| let s1 = svc1.state_clone(); | ||
| { | ||
| let _g = s1.metrics_clone().create_inflight_guard( | ||
| "test-model", | ||
| Endpoint::ChatCompletions, | ||
| false, | ||
| ); | ||
| } | ||
| let body1 = reqwest::get("http://localhost:9101/metrics") | ||
| .await | ||
| .unwrap() | ||
| .text() | ||
| .await | ||
| .unwrap(); | ||
| assert!(body1.contains("dynamo_frontend_requests_total")); | ||
| token1.cancel(); | ||
|
|
||
| // Case 2: env override to prefix | ||
| env::set_var(metrics::METRICS_PREFIX_ENV, "custom_prefix"); | ||
| let svc2 = HttpService::builder().port(9102).build().unwrap(); | ||
| let token2 = CancellationToken::new(); | ||
| let _h2 = svc2.spawn(token2.clone()).await; | ||
| wait_for_metrics_ready(9102).await; | ||
|
|
||
| // Populate labeled metrics | ||
| let s2 = svc2.state_clone(); | ||
| { | ||
| let _g = | ||
| s2.metrics_clone() | ||
| .create_inflight_guard("test-model", Endpoint::ChatCompletions, true); | ||
| } | ||
| // Single fetch and assert | ||
| let body2 = reqwest::get("http://localhost:9102/metrics") | ||
| .await | ||
| .unwrap() | ||
| .text() | ||
| .await | ||
| .unwrap(); | ||
| assert!(body2.contains("custom_prefix_requests_total")); | ||
| assert!(!body2.contains("dynamo_frontend_requests_total")); | ||
| token2.cancel(); | ||
|
|
||
| // Case 3: invalid env prefix is sanitized | ||
| env::set_var(metrics::METRICS_PREFIX_ENV, "nv-llm/http service"); | ||
| let svc3 = HttpService::builder().port(9103).build().unwrap(); | ||
| let token3 = CancellationToken::new(); | ||
| let _h3 = svc3.spawn(token3.clone()).await; | ||
| wait_for_metrics_ready(9103).await; | ||
|
|
||
| let s3 = svc3.state_clone(); | ||
| { | ||
| let _g = | ||
| s3.metrics_clone() | ||
| .create_inflight_guard("test-model", Endpoint::ChatCompletions, true); | ||
| } | ||
| let body3 = reqwest::get("http://localhost:9103/metrics") | ||
| .await | ||
| .unwrap() | ||
| .text() | ||
| .await | ||
| .unwrap(); | ||
| assert!(body3.contains("nv_llm_http_service_requests_total")); | ||
| assert!(!body3.contains("dynamo_frontend_requests_total")); | ||
| token3.cancel(); | ||
|
|
||
| // Cleanup env to avoid leaking state | ||
| env::remove_var(metrics::METRICS_PREFIX_ENV); | ||
| } | ||
|
|
||
| // Poll /metrics until ready or timeout | ||
| async fn wait_for_metrics_ready(port: u16) { | ||
| let url = format!("http://localhost:{}/metrics", port); | ||
| let start = tokio::time::Instant::now(); | ||
| let timeout = Duration::from_secs(5); | ||
| loop { | ||
| if start.elapsed() > timeout { | ||
| panic!("Timed out waiting for metrics endpoint at {}", url); | ||
| } | ||
| match reqwest::get(&url).await { | ||
| Ok(resp) if resp.status().is_success() => break, | ||
| _ => tokio::time::sleep(Duration::from_millis(50)).await, | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.