-
Notifications
You must be signed in to change notification settings - Fork 689
feat(metrics): add NATS client metrics to prometheus_metrics_fmt #2292
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
feat(metrics): add NATS client metrics to prometheus_metrics_fmt #2292
Conversation
WalkthroughThis change refactors and extends the metrics system in the distributed runtime. It introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DistributedRuntime
participant MetricsRegistryEntry
participant NatsClient
participant Prometheus
User->>DistributedRuntime: add_prometheus_metric(prefix, collector)
DistributedRuntime->>MetricsRegistryEntry: add collector to registry
User->>DistributedRuntime: add_metrics_callback(prefix, callback)
DistributedRuntime->>MetricsRegistryEntry: add callback to callbacks list
User->>DistributedRuntime: execute_metrics_callbacks(prefix)
DistributedRuntime->>MetricsRegistryEntry: execute all callbacks
MetricsRegistryEntry-->>DistributedRuntime: return results
NatsClient->>DistributedRuntime: add_metrics(drt)
DistributedRuntime->>MetricsRegistryEntry: register NATS metrics & callback
Prometheus->>DistributedRuntime: scrape metrics endpoint
DistributedRuntime->>MetricsRegistryEntry: execute callbacks
DistributedRuntime->>Prometheus: return formatted metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 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 comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (3)
lib/runtime/src/lib.rs (1)
191-198: Document the partial clone behavior more prominentlyThe
Cloneimplementation only clones the Prometheus registry and resets callbacks to empty. While this is documented in the inline comment, this surprising behavior should be more prominently documented at the struct level to prevent confusion.Add documentation to the struct definition:
/// Structure to hold Prometheus registries and associated callbacks for a given prefix +/// +/// Note: When cloning this struct, only the Prometheus registry is cloned. +/// Callbacks cannot be cloned and will be reset to an empty list in the cloned instance. pub struct MetricsRegistryEntry {lib/runtime/src/transports/nats.rs (1)
76-76: Remove or explain commented codeThe commented line
//nats_metrics.copy_from_nats_client_stats(&self.client);should either be removed or explained with a comment if it's intentionally disabled.- //nats_metrics.copy_from_nats_client_stats(&self.client); -lib/runtime/src/http_server.rs (1)
351-357: Make test assertion order-agnosticPrometheus doesn’t guarantee metric ordering. Building a single string and asserting exact equality can cause flaky tests when the registry order changes. Prefer comparing sets, sorting both strings, or asserting that expected lines are contained in the output.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/runtime/src/distributed.rs(5 hunks)lib/runtime/src/http_server.rs(12 hunks)lib/runtime/src/lib.rs(2 hunks)lib/runtime/src/metrics.rs(6 hunks)lib/runtime/src/transports/nats.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: kthui
PR: ai-dynamo/dynamo#1424
File: lib/runtime/src/pipeline/network/egress/push_router.rs:204-209
Timestamp: 2025-06-13T22:07:24.843Z
Learning: The codebase uses async-nats version 0.40, not the older nats crate. Error handling should use async_nats::error::Error variants, not nats::Error variants.
📚 Learning: the asyncenginecontextprovider trait in lib/runtime/src/engine.rs was intentionally changed from `se...
Learnt from: ryanolson
PR: ai-dynamo/dynamo#1919
File: lib/runtime/src/engine.rs:168-168
Timestamp: 2025-07-14T21:25:56.930Z
Learning: The AsyncEngineContextProvider trait in lib/runtime/src/engine.rs was intentionally changed from `Send + Sync + Debug` to `Send + Debug` because the Sync bound was overly constraining. The trait should only require Send + Debug as designed.
Applied to files:
lib/runtime/src/distributed.rslib/runtime/src/http_server.rslib/runtime/src/lib.rslib/runtime/src/metrics.rs
📚 Learning: in lib/runtime/src/component/client.rs, the current mutex usage in get_or_create_dynamic_instance_so...
Learnt from: grahamking
PR: ai-dynamo/dynamo#1962
File: lib/runtime/src/component/client.rs:270-273
Timestamp: 2025-07-16T12:41:12.543Z
Learning: In lib/runtime/src/component/client.rs, the current mutex usage in get_or_create_dynamic_instance_source is temporary while evaluating whether the mutex can be dropped entirely. The code currently has a race condition between try_lock and lock().await, but this is acknowledged as an interim state during the performance optimization process.
Applied to files:
lib/runtime/src/distributed.rslib/runtime/src/http_server.rslib/runtime/src/lib.rslib/runtime/src/metrics.rs
📚 Learning: in rust/tokio applications, when background tasks use channels for communication, dropping the sende...
Learnt from: oandreeva-nv
PR: ai-dynamo/dynamo#1195
File: lib/llm/tests/block_manager.rs:150-152
Timestamp: 2025-06-02T19:37:27.666Z
Learning: In Rust/Tokio applications, when background tasks use channels for communication, dropping the sender automatically signals task termination when the receiver gets `None`. The `start_batching_publisher` function in `lib/llm/tests/block_manager.rs` demonstrates this pattern: when the `KVBMDynamoRuntimeComponent` is dropped, its `batch_tx` sender is dropped, causing `rx.recv()` to return `None`, which triggers cleanup and task termination.
Applied to files:
lib/runtime/src/http_server.rs
📚 Learning: the codebase uses async-nats version 0.40, not the older nats crate. error handling should use async...
Learnt from: kthui
PR: ai-dynamo/dynamo#1424
File: lib/runtime/src/pipeline/network/egress/push_router.rs:204-209
Timestamp: 2025-06-13T22:07:24.843Z
Learning: The codebase uses async-nats version 0.40, not the older nats crate. Error handling should use async_nats::error::Error variants, not nats::Error variants.
Applied to files:
lib/runtime/src/transports/nats.rs
📚 Learning: in async-nats, the "no responders" error is represented as async_nats::client::requesterrorkind::nor...
Learnt from: kthui
PR: ai-dynamo/dynamo#1424
File: lib/runtime/src/pipeline/network/egress/push_router.rs:204-209
Timestamp: 2025-06-13T22:32:05.022Z
Learning: In async-nats, the "no responders" error is represented as async_nats::client::RequestErrorKind::NoResponders, not async_nats::Error::NoResponders. Use err.downcast_ref::<async_nats::client::RequestError>() and then check request_err.kind() against RequestErrorKind::NoResponders.
Applied to files:
lib/runtime/src/transports/nats.rs
⏰ 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). (4)
- 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 - vllm
🔇 Additional comments (4)
lib/runtime/src/distributed.rs (2)
113-118: LGTM! Good error handling for metrics registrationThe approach of logging warnings instead of failing initialization is appropriate for metrics registration. This ensures the runtime remains functional even if metrics setup fails.
297-338: Well-structured test coverageThe test module provides comprehensive coverage of the metrics registry functionality, including callback execution, prefix handling, and Prometheus metric registration.
lib/runtime/src/metrics.rs (1)
1064-1157: Excellent test strategy for dynamic metricsThe approach of filtering NATS metrics for deterministic comparisons while separately verifying their presence is well-designed. This pattern effectively handles testing systems with dynamic components.
lib/runtime/src/http_server.rs (1)
183-185: Terminology update looks consistentRenaming the log prefix to “System server (HTTP)” keeps the wording aligned across the file.
b00f369 to
1184a7e
Compare
1184a7e to
ab57449
Compare
- Add NATS client metrics collection via callbacks in transports/nats.rs - Execute callbacks before TextEncoder to include NATS metrics in output - Add comprehensive test coverage for NATS client metrics - Centralize NATS metric names in prometheus_names module - Fixed runtime prefix hierarchy (bug previously)
…ons, reset on scrape errors; tidy tests and logs
…ling - Remove unnecessary _runtime parameter from MetricsRegistryEntry::add_callback - Update add_callback calls to use new signature - Replace brittle string formatting with proper enum matching for NATS State - Update eprintln to tracing::error for consistent logging - Fix test code to use updated callback signature
… store NATS client in metrics
… tests; log callback errors
…er error handling
- Use RuntimeCallback type alias with Arc for explicit sharing - Replace Mutex with RwLock for better concurrent reads - Minimize lock duration when executing callbacks - Add tests with counter verification - Document that metrics are snapshots, not live references
- Remove unused MetricsRegistry imports from multiple files - Fix type mismatch in component retrieval logic (and_then -> map) - Restore labels logic in metrics component - Clean up unused code and improve code quality - All tests passing except one pre-existing logging test issue
a8e18b5 to
7c1d9fd
Compare
- Remove add_labels method calls from all example files since method doesn't exist - Clean up unused MetricsRegistry imports from example files - Fix syntax errors in method chaining after removing add_labels - All examples now compile and run successfully - Examples are included in workspace for type inference during development
- Remove add_labels method from Python bindings to match Rust API changes - Python bindings now properly reflect the current Rust runtime interface - Ensures consistency between Rust core and Python wrapper APIs
- Format import statements for better readability - Remove unnecessary blank lines in method chains - Apply consistent code style across all example files - Improve code formatting and organization
Co-authored-by: Keiven Chang <keivenchang@users.noreply.github.com> Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
Overview:
This PR adds comprehensive NATS metrics collection and Prometheus metrics management to the Dynamo runtime, including endpoint-grouped metrics and callback-based metric registration.
Details:
Simple Flow:
Where should the reviewer start?
lib/runtime/src/metrics.rs- Core metrics registry implementation and callback systemlib/runtime/src/transports/nats.rs- NATS metrics collection and registrationlib/runtime/src/component.rs- Component metrics integration and automatic registrationlib/runtime/src/distributed.rs- DistributedRuntime metrics management methodsRelated Issues: (use one of the action words Closes / Fixes / Resolves / Relates to)
Relates to DIS-298
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor