feat: Rename dynamo_component_concurrent_requests #2515
Conversation
to dynamo_component_inflight_requests so that it matches the correspond frontend metric.
tzulingk
requested review from
hutm,
biswapanda,
ishandhanani,
julienmancuso,
hhzhang16,
nnshah1,
mohammedabdulwahhab,
tanmayv25,
piotrm-nvidia,
ptarasiewiczNV and
ryanolson
as code owners
tzulingk
requested review from
grahamking,
paulhendricks,
tmonty12,
GuanLuo,
rmccorm4,
tedzhouhk,
alec-flowers,
kkranen,
jthomson04,
PeaBrane and
a team
as code owners
WalkthroughRenamed the “concurrent_requests” metric to “inflight_requests” across runtime metrics, public constants, ingress handler fields/constructor, docs, examples, and tests. Updated Grafana dashboard queries to the dynamo_component_* namespace and dynamo_endpoint label; one errors panel label key appears misspelled. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
deploy/metrics/grafana_dashboards/grafana-dynamo-dashboard.json (1)
1000-1004: Fix label typo in PromQL (breaks the errors panel).The label key is misspelled as dynamendpoint; should be dynamo_endpoint. As-is, the panel won’t return data.
Apply this diff:
- "expr": "rate(dynamo_component_errors_total{dynamendpoint=\"generate\"}[1m])", + "expr": "rate(dynamo_component_errors_total{dynamo_endpoint=\"generate\"}[1m])",
🧹 Nitpick comments (3)
lib/runtime/src/metrics/prometheus_names.rs (1)
112-112: Add a deprecated alias for the renamed constantRenaming the public constant from
CONCURRENT_REQUESTStoINFLIGHT_REQUESTSis a breaking change for any external consumers. No internal references toCONCURRENT_REQUESTSwere found in the codebase, so this only affects downstream users. To smooth the migration:• In
lib/runtime/src/metrics/prometheus_names.rs(around line 112), add:#[deprecated( since = "X.Y.Z", note = "Please use `INFLIGHT_REQUESTS` instead" )] pub const CONCURRENT_REQUESTS: &str = INFLIGHT_REQUESTS;• In your
CHANGELOG.md, under “Breaking Changes,” note thatCONCURRENT_REQUESTShas been renamed toINFLIGHT_REQUESTS.Let me know if you’d like me to open a follow-up PR to add the alias and update the changelog.
tests/router/test_router_e2e_with_mockers.py (1)
284-337: Helper rename is consistent; optional micro-improvement for readiness probe.The function name matches the new terminology. Optionally, consider moving the initial sleep in send_request_with_retry to occur after the first failed attempt to avoid delaying the very first probe, but this is non-blocking.
lib/runtime/examples/system_metrics/tests/integration_test.rs (1)
145-173: Optionally assert presence (non-negativity) of inflight_requestsEven if we don’t require it to be >0, asserting that the metric exists and is non-negative gives extra confidence without flakiness.
Apply this diff inside verify_ingress_metrics_greater_than_0, after the existing loop:
for metric_name in &metrics_to_verify { let line = metrics_content .lines() .find(|l| l.contains(metric_name) && !l.contains("#")) .unwrap_or_else(|| panic!("{} metric not found", metric_name)); let value = extract_metric_value(line); assert!( value > 0.0, "{} should be greater than 0, got: {}", metric_name, value ); println!("{}: {}", metric_name, value); } + // inflight_requests can be 0 at rest, but should exist and be non-negative. + if let Some(line) = metrics_content + .lines() + .find(|l| l.contains("inflight_requests") && !l.contains('#')) + { + let inflight = extract_metric_value(line); + assert!( + inflight >= 0.0, + "inflight_requests should be >= 0, got: {}", + inflight + ); + println!("inflight_requests: {}", inflight); + } else { + panic!("inflight_requests metric not found"); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
deploy/metrics/README.md(1 hunks)deploy/metrics/grafana_dashboards/grafana-dynamo-dashboard.json(5 hunks)lib/runtime/examples/system_metrics/README.md(2 hunks)lib/runtime/examples/system_metrics/tests/integration_test.rs(2 hunks)lib/runtime/src/metrics.rs(1 hunks)lib/runtime/src/metrics/prometheus_names.rs(1 hunks)lib/runtime/src/pipeline/network/ingress/push_handler.rs(6 hunks)tests/router/test_router_e2e_with_mockers.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
lib/runtime/src/metrics.rs (1)
lib/runtime/src/metrics/prometheus_names.rs (1)
build_metric_name(10-12)
🪛 LanguageTool
deploy/metrics/README.md
[grammar] ~43-~43: There might be a mistake here.
Context: ...quests currently being processed (gauge) - dynamo_component_request_bytes_total: Total bytes received in requests (coun...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Mirror Repository to GitLab
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (.)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (9)
lib/runtime/examples/system_metrics/README.md (1)
65-65: Metric rename looks consistent and correct.The gauge name and example output align with the new inflight terminology and label set.
Also applies to: 83-85
deploy/metrics/README.md (1)
43-43: Docs sync with rename is correct.The component metric list now references inflight_requests; description remains accurate.
tests/router/test_router_e2e_with_mockers.py (1)
146-153: Call sites updated to new helper name — LGTM.Both tests now call send_inflight_requests; arguments unchanged and consistent.
Also applies to: 232-237
deploy/metrics/grafana_dashboards/grafana-dynamo-dashboard.json (1)
698-715: Dashboard queries updated to component-scoped metrics — LGTM.PromQL now uses dynamo_component_* metrics and the dynamo_endpoint label as intended.
Also applies to: 803-807, 908-912
lib/runtime/src/metrics.rs (1)
1627-1627: Allconcurrent_requestsrefs removed;inflight_requestsfully integrated
No stale occurrences ofconcurrent_requestsremain, and the newinflight_requestsmetric is referenced across README, code, and tests.lib/runtime/examples/system_metrics/tests/integration_test.rs (1)
121-123: Comment update aligns with renameExcluding inflight_requests from the >0 assertion is appropriate since it can legitimately be 0 at rest.
lib/runtime/src/pipeline/network/ingress/push_handler.rs (3)
27-33: Field rename to inflight_requests is accuratePublic API field rename is consistent with the PR objective.
36-43: Constructor parameter renamed consistentlyConstructor signature and usage updated to inflight_requests. No issues.
71-75: Metric creation uses the new 'inflight_requests' nameEndpoint gauge is correctly created as inflight_requests with clear help text.
Signed-off-by: Tzu-Ling Kan <[email protected]>
Signed-off-by: Tzu-Ling Kan <[email protected]>
Signed-off-by: Tzu-Ling Kan <[email protected]> Signed-off-by: Hannah Zhang <[email protected]>
Signed-off-by: Tzu-Ling Kan <[email protected]>
Overview:
Details:
Rename
dynamo_component_concurrent_requeststodynamo_component_inflight_requestsso that it matches the corresponding frontend metric.Where should the reviewer start?
push_handler.rs
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
DIS-350 fix: dynamo_frontend_* & dynamo_components_* not sync'ed
Summary by CodeRabbit
Refactor
Documentation
Tests