feat: Add Dynamo TUI MVP for monitoring deployments #4296
Conversation
This commit introduces a new terminal UI for Dynamo, providing real-time insights into namespaces, components, endpoints, NATS statistics, and Prometheus metrics. Co-authored-by: asadblue2015 <[email protected]>
This commit introduces async-nats integration and refactors the application's state management for better organization and efficiency. Co-authored-by: asadblue2015 <[email protected]>
- Fix endpoint count expectation (1 instead of 2) - Fix instance count expectation (2 instead of 3) - Add readme field to workspace.package in Cargo.toml - Tests now correctly reflect the actual data structure
|
👋 Hi AsadShahid04! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
bot
added
the
external-contribution
AsadShahid04
changed the title
- Add Dockerfile.dev for Linux-based builds (fixes macOS inotify issues) - Add docker-build.sh, docker-run.sh, and docker-test.sh scripts - Add DOCKER.md documentation for Docker workflow - Enables cross-platform development and testing
AsadShahid04
force-pushed
the
feature/dynamo-tui-mvp
branch
from
e431461 to
c044d2a
Compare
- Support both old format (without 'type' field) and new format (with 'type' field) - Allows TUI to parse ETCD data from workers using older runtime versions - Fixes 'missing field type' parsing errors in TUI
WalkthroughThis pull request introduces a new TUI project for Dynamo as a workspace member that monitors namespaces, components, and endpoints via ETCD/NATS discovery with optional Prometheus metrics collection. It includes CLI configuration, Docker support, comprehensive documentation, and core Rust modules for state management, input handling, metrics processing, and UI rendering, alongside a library change enabling flexible JSON deserialization for discovery instances. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/runtime/src/discovery/mod.rs (1)
140-154: Add tests for multi-formatDiscoveryInstancedeserialization and enhance implicit-default documentationThe deserializer implementation is correct as assessed. However, recommended test coverage remains absent:
- New Endpoint format (
"type": "Endpoint"): no direct deserialization test- New Model format (
"type": "Model"): no direct deserialization test- Legacy format (no
"type"field): no direct deserialization test- Error cases (unknown or non-string
"type"): no testsAdditionally, the implicit Endpoint default when
"type"is missing (line 208 of mod.rs) is documented only in a minimal comment at line 203. Per prior guidance on keeping specifications explicit, add a clear docstring or expanded comment above the fallback branch explicitly stating: "Whentypeis absent, default toEndpointfor backward compatibility with pre-enum stored data."
🧹 Nitpick comments (6)
launch/dynamo-tui/Dockerfile.dev (1)
1-39: Good Docker practices for a development image.The Dockerfile follows best practices with proper layer caching and cleanup. Since this is a development image, the single-stage build is appropriate.
For production use, consider a multi-stage build to reduce the final image size by excluding build tools:
# Stage 1: Builder FROM rust:1.90.0-slim AS builder RUN apt-get update && apt-get install -y \ pkg-config libssl-dev curl git build-essential protobuf-compiler \ && rm -rf /var/lib/apt/lists/* WORKDIR /workspace COPY Cargo.toml Cargo.lock rust-toolchain.toml README.md pyproject.toml ./ COPY lib/ ./lib/ COPY launch/ ./launch/ RUN cargo build -p dynamo-tui --release # Stage 2: Runtime FROM debian:bookworm-slim RUN apt-get update && apt-get install -y libssl3 ca-certificates \ && rm -rf /var/lib/apt/lists/* COPY --from=builder /workspace/target/release/dynamo-tui /usr/local/bin/ CMD ["dynamo-tui"]launch/dynamo-tui/docker-run.sh (1)
14-43: Docker run wiring looks solid; only minor optional nitsThe flow to ensure
dynamo-tui:devexists, pick sensible ETCD/NATS defaults per-OS, and set--network hoston Linux is coherent and should work well for the intended environments.If you want to polish further (optional):
- Consider a brief comment that the
--network hostbehavior is Linux-specific and that non‑Linux platforms are expected to go through thedarwinbranch.- Optionally quote
NETWORK_MODE("$NETWORK_MODE") or gate it with${NETWORK_MODE:+$NETWORK_MODE}to avoid passing a spurious empty arg, though this is mostly cosmetic.launch/dynamo-tui/src/ui.rs (1)
16-430: UI rendering is solid; only very small cleanup opportunitiesThe overall layout (3 columns + endpoint detail + stats + status bar) and use of App state look correct and robust, including handling of empty/invalid selections via
filter_mapandendpoint_at.If you want to trim a few small nits (purely optional):
- In
render_metrics, the last entry usesformat!(...).to_string(), which is redundant sinceformat!already returns aString.- Several helpers repeatedly call
app.selection()inside loops; cachinglet sel = app.selection();once per render function would slightly reduce noise and repeated lookups.instance_linescurrently only matchesTransportType::NatsTcp; that’s fine given today’s enum, but if new variants are added later, consider including them in the UI (you’ll get a compile error anyway, so this is more of a future reminder).Functionally though, nothing here looks blocking.
launch/dynamo-tui/src/input.rs (1)
11-85: Input listener and key mapping look correct for the intended UXThe async input loop,
spawn_blockingaround crossterm polling, and the vim‑style key mappings all look good and align with the app’shandle_inputsemantics.A small, optional enhancement would be to log once when the listener exits (e.g., due to cancellation or channel close) to aid debugging, but behavior is otherwise sound.
launch/dynamo-tui/src/sources.rs (2)
20-47: Discovery snapshot + initial seeding look correct, but consider handlingsendfailures explicitlyThe initial
DiscoverySnapshotconstruction and filtering toDiscoveryInstance::Endpointlook good and align with the App’s expectations. One minor improvement: iftx.send(AppEvent::DiscoverySnapshot(...)).awaitfails (receiver dropped), you could bail out early instead of proceeding to set up the watch stream, since the TUI is unlikely to be alive anymore and the background task work becomes wasted.This is non-blocking and mostly about avoiding unnecessary work during shutdown.
156-235: Metrics pipeline: error handling is working correctlyVerification confirms the gzip feature is properly enabled in
launch/dynamo-tui/Cargo.tomlwithreqwest = { workspace = true, features = ["gzip"] }, so the.gzip(true)call will work as intended.The overall structure remains sound:
- Single
reqwest::Clientreused for all requests- Initial and periodic scraping with proper interval configuration
- Network/body errors logged and surfaced to UI
The error handling semantics are as described:
collect_metrics_oncealways returnsOk(())for HTTP/body errors and only returnsErrwhen the metrics channel closes. Theis_err()checks don't change runtime behavior—they only trigger when the channel is closed (at which point errors can't be sent anyway). This works correctly in practice for UI-only telemetry.For clarity, consider either removing the
is_err()checks if they don't serve a purpose, or restructuringcollect_metrics_onceto returnErrfor network failures—but the current implementation is functionally sound and acceptable as-is.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.gitignore(1 hunks)Cargo.toml(2 hunks)launch/dynamo-tui/Cargo.toml(1 hunks)launch/dynamo-tui/DOCKER.md(1 hunks)launch/dynamo-tui/Dockerfile.dev(1 hunks)launch/dynamo-tui/README.md(1 hunks)launch/dynamo-tui/docker-build.sh(1 hunks)launch/dynamo-tui/docker-run.sh(1 hunks)launch/dynamo-tui/docker-test.sh(1 hunks)launch/dynamo-tui/src/app.rs(1 hunks)launch/dynamo-tui/src/input.rs(1 hunks)launch/dynamo-tui/src/main.rs(1 hunks)launch/dynamo-tui/src/metrics.rs(1 hunks)launch/dynamo-tui/src/sources.rs(1 hunks)launch/dynamo-tui/src/ui.rs(1 hunks)lib/runtime/src/discovery/mod.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2025-08-05T22:51:59.230Z
Learnt from: dmitry-tokarev-nv
Repo: ai-dynamo/dynamo PR: 2300
File: pyproject.toml:64-66
Timestamp: 2025-08-05T22:51:59.230Z
Learning: The ai-dynamo/dynamo project does not ship ARM64 wheels, so platform markers to restrict dependencies to x86_64 are not needed in pyproject.toml dependencies.
Applied to files:
Cargo.toml
📚 Learning: 2025-10-17T22:57:12.526Z
Learnt from: oandreeva-nv
Repo: ai-dynamo/dynamo PR: 3700
File: lib/llm/src/block_manager.rs:19-19
Timestamp: 2025-10-17T22:57:12.526Z
Learning: The dynamo library (ai-dynamo/dynamo repository) is currently developed to run on Linux only, so Linux-specific code and syscalls do not require platform guards.
Applied to files:
Cargo.toml
📚 Learning: 2025-08-30T20:43:10.091Z
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 2797
File: .devcontainer/devcontainer.json:12-12
Timestamp: 2025-08-30T20:43:10.091Z
Learning: In the dynamo project, devcontainer.json files use templated container names (like "dynamo-vllm-devcontainer") that are automatically processed by the copy_devcontainer.sh script to generate framework-specific configurations with unique names, preventing container name collisions.
Applied to files:
launch/dynamo-tui/docker-test.shlaunch/dynamo-tui/Dockerfile.devlaunch/dynamo-tui/docker-build.shlaunch/dynamo-tui/docker-run.sh
📚 Learning: 2025-08-15T02:01:01.238Z
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 2453
File: deploy/dynamo_check.py:739-741
Timestamp: 2025-08-15T02:01:01.238Z
Learning: In the dynamo project, missing cargo should be treated as a fatal error in dynamo_check.py because developers need cargo to build the Rust components. The tool is designed to validate the complete development environment, not just import functionality.
Applied to files:
launch/dynamo-tui/Cargo.toml
📚 Learning: 2025-08-30T20:43:49.632Z
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 2797
File: container/Dockerfile:437-449
Timestamp: 2025-08-30T20:43:49.632Z
Learning: In the dynamo project's devcontainer setup, the team prioritizes consistency across framework-specific Dockerfiles (like container/Dockerfile, container/Dockerfile.vllm, etc.) by mirroring their structure, even when individual optimizations might be possible, to maintain uniformity in the development environment setup.
Applied to files:
launch/dynamo-tui/Dockerfile.devlaunch/dynamo-tui/docker-build.shlaunch/dynamo-tui/docker-run.sh
📚 Learning: 2025-09-16T17:16:07.820Z
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 3051
File: container/templates/Dockerfile.vllm.j2:221-233
Timestamp: 2025-09-16T17:16:07.820Z
Learning: In the dynamo project, when converting Dockerfiles to Jinja2 templates, the primary goal is backward compatibility - generated Dockerfiles must be identical to the originals. Security improvements or other enhancements that would change the generated output are out of scope for templating PRs and should be addressed separately.
Applied to files:
launch/dynamo-tui/Dockerfile.devlaunch/dynamo-tui/docker-build.shlaunch/dynamo-tui/docker-run.sh
📚 Learning: 2025-09-03T01:10:12.599Z
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 2822
File: container/Dockerfile.vllm:343-352
Timestamp: 2025-09-03T01:10:12.599Z
Learning: In the dynamo project's local-dev Docker targets, USER_UID and USER_GID build args are intentionally left without default values to force explicit UID/GID mapping during build time, preventing file permission issues in local development environments where container users need to match host user permissions for mounted volumes.
Applied to files:
launch/dynamo-tui/Dockerfile.dev
📚 Learning: 2025-08-30T20:43:10.091Z
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 2797
File: .devcontainer/devcontainer.json:12-12
Timestamp: 2025-08-30T20:43:10.091Z
Learning: In the dynamo project's devcontainer setup, hard-coded container names in devcontainer.json files serve as templates that are automatically processed by the copy_devcontainer.sh script to generate framework-specific configurations with unique names, preventing container name collisions.
Applied to files:
launch/dynamo-tui/Dockerfile.devlaunch/dynamo-tui/docker-build.shlaunch/dynamo-tui/docker-run.sh
📚 Learning: 2025-08-02T06:09:41.078Z
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 2255
File: .devcontainer/build-dev-image.sh:173-177
Timestamp: 2025-08-02T06:09:41.078Z
Learning: In bash scripts, SCRIPT_DIR="$(dirname "${BASH_SOURCE[0]}")" preserves the relative/absolute nature of how the script was invoked. When a script is run with a relative path like .devcontainer/build-dev-image.sh from the repo root, SCRIPT_DIR becomes .devcontainer (relative), making it valid for Docker COPY commands within the build context.
Applied to files:
launch/dynamo-tui/docker-build.sh
📚 Learning: 2025-08-02T06:09:41.078Z
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 2255
File: .devcontainer/build-dev-image.sh:173-177
Timestamp: 2025-08-02T06:09:41.078Z
Learning: In bash scripts, SCRIPT_DIR="$(dirname "${BASH_SOURCE[0]}")" preserves the relative/absolute nature of how the script was invoked. When a script is run with a relative path like ./build-dev-image.sh or .devcontainer/build-dev-image.sh, SCRIPT_DIR becomes a relative path (.devcontainer), making it valid for Docker COPY commands within the build context.
Applied to files:
launch/dynamo-tui/docker-build.sh
📚 Learning: 2025-09-02T16:46:54.015Z
Learnt from: GuanLuo
Repo: ai-dynamo/dynamo PR: 2714
File: lib/llm/src/discovery/model_entry.rs:38-42
Timestamp: 2025-09-02T16:46:54.015Z
Learning: In lib/llm/src/discovery/model_entry.rs, GuanLuo prefers not to add serde defaults for model_type and model_input fields to keep the specification explicit and avoid user errors, relying on atomic deployment strategy to avoid backward compatibility issues.
Applied to files:
lib/runtime/src/discovery/mod.rs
📚 Learning: 2025-09-16T00:26:43.641Z
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 3035
File: lib/runtime/examples/system_metrics/README.md:65-65
Timestamp: 2025-09-16T00:26:43.641Z
Learning: The team at ai-dynamo/dynamo prefers to use consistent metric naming patterns with _total suffixes across all metric types (including gauges) for internal consistency, even when this differs from strict Prometheus conventions that reserve _total for counters only. This design decision was confirmed by keivenchang in PR 3035, referencing examples in prometheus_names.rs and input from team members.
Applied to files:
launch/dynamo-tui/src/metrics.rs
📚 Learning: 2025-06-02T19:37:27.666Z
Learnt from: oandreeva-nv
Repo: ai-dynamo/dynamo PR: 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:
launch/dynamo-tui/src/sources.rs
🧬 Code graph analysis (6)
launch/dynamo-tui/src/ui.rs (1)
launch/dynamo-tui/src/app.rs (6)
default(47-57)default(74-76)default(130-132)selection(216-218)display_name(470-477)as_color(479-487)
launch/dynamo-tui/src/input.rs (1)
launch/dynamo-tui/src/main.rs (2)
mpsc(57-57)mpsc(58-58)
launch/dynamo-tui/src/main.rs (5)
launch/dynamo-tui/src/sources.rs (3)
spawn_discovery_pipeline(20-94)spawn_metrics_pipeline(156-202)spawn_nats_pipeline(96-136)launch/dynamo-tui/src/ui.rs (1)
render(17-56)launch/dynamo-tui/src/input.rs (1)
spawn_input_listener(21-53)launch/dynamo-tui/src/app.rs (5)
default(47-57)default(74-76)default(130-132)apply_event(136-173)handle_input(175-187)launch/dynamo-tui/src/metrics.rs (1)
default(33-45)
launch/dynamo-tui/src/metrics.rs (1)
lib/runtime/examples/system_metrics/tests/integration_test.rs (1)
line(164-167)
launch/dynamo-tui/src/sources.rs (1)
launch/dynamo-tui/src/metrics.rs (1)
process_metrics(48-88)
launch/dynamo-tui/src/app.rs (2)
launch/dynamo-tui/src/metrics.rs (1)
default(33-45)lib/runtime/src/component/client.rs (1)
instances(73-75)
🪛 GitHub Actions: Pre Merge Validation of (ai-dynamo/dynamo/refs/pull/4296/merge) by AsadShahid04.
lib/runtime/src/discovery/mod.rs
[error] 160-160: Trailing whitespace detected by pre-commit hook and modified in this file.
[error] 1-1: Command failed with exit code 1: pre-commit run --show-diff-on-failure --color=always --all-files
🪛 GitHub Actions: Rust pre-merge checks
lib/runtime/src/discovery/mod.rs
[error] 160-202: cargo fmt -- --check failed (exit code 1) due to formatting differences detected in lib/runtime/src/discovery/mod.rs. Run 'cargo fmt' to fix formatting.
🪛 Shellcheck (0.11.0)
launch/dynamo-tui/docker-test.sh
[error] 29-29: Argument mixes string and array. Use * or separate argument.
(SC2145)
⏰ 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). (1)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (20)
lib/runtime/src/discovery/mod.rs (1)
8-8: serde_json import aligns with expanded usageBringing
serde_jsoninto scope is consistent with its broader use in this module (manualDeserializeimpl andcard_jsonhandling); no issues here.Cargo.toml (1)
7-7: LGTM! Clean workspace configuration updates.The addition of the new
dynamo-tuiworkspace member and the readme field properly integrate the new TUI crate into the project structure.Also applies to: 45-45
launch/dynamo-tui/Cargo.toml (1)
1-32: LGTM! Well-structured crate manifest.The dependencies are appropriate for a TUI monitoring application, and the workspace inheritance promotes consistency across the project.
launch/dynamo-tui/README.md (1)
1-51: LGTM! Comprehensive and clear documentation.The README provides excellent guidance for users, including usage examples, configuration options, key bindings, and honest discussion of current limitations.
launch/dynamo-tui/DOCKER.md (1)
1-123: LGTM! Thorough Docker workflow documentation.The guide provides excellent coverage of Docker-based development workflows, including macOS-specific networking considerations and helpful troubleshooting steps.
launch/dynamo-tui/src/metrics.rs (3)
6-46: LGTM! Well-designed data structures.The
MetricsSnapshotandPrometheusSamplestructs are appropriately structured withOptiontypes for metrics that may be unavailable. TheDefaultimplementation provides sensible zero values.
48-88: LGTM! Solid metrics processing logic.The
process_metricsfunction correctly extracts Prometheus metrics, computes averages, and calculates rates. The hardcoded metric names (dynamo_frontend_*) are acknowledged as a current limitation in the README.
90-146: LGTM! Well-implemented helper functions.The parsing and calculation helpers are correctly implemented:
sum_metricandparse_sample_valueproperly extract values from Prometheus formataverage_msguards against division by near-zero countsrate_metricshandles counter resets gracefully by returningNonefor negative deltasvalue_or_nonefilters out near-zero gauge valuesThe defensive
max(0.0)inrate_metrics(lines 129, 134) guards against floating-point edge cases after the negativity check, which is good defensive programming..gitignore (1)
113-114: LGTM! Appropriate gitignore entry.Ignoring the
scripts/directory for local development tools is a sensible addition.launch/dynamo-tui/docker-build.sh (1)
1-26: LGTM! Well-structured build script.The script follows bash best practices with strict error handling (
set -e), proper directory resolution, and helpful user guidance. The approach aligns with established patterns in the project.launch/dynamo-tui/src/app.rs (7)
10-58: Event model and NATS snapshot defaults are coherent with the pipelines
AppEventvariants map cleanly onto whatsources.rsemits (discovery snapshot / up / down, NATS, metrics, info, error), which keeps the event-driven design clear.NatsSnapshot’s fields andDefaultimpl (Disconnected with zeroed counters andInstant::now()) give the UI a safe baseline before any real data arrives.No changes needed here.
79-188: CoreAppstate andapply_event/handle_inputlogic are solidA few points that look good:
Appkeeps all mutable UI state encapsulated (namespaces tree, selection, last_* markers, NATS/metrics), simplifying the rest of the TUI.apply_event:
- Rebuilds the tree on
DiscoverySnapshotand resets selection indices once.- Uses
insert_instance/remove_instanceto respond toInstanceUp/InstanceDown, so the tests exercise the same paths as the pipelines.- Stores latest NATS/metrics snapshots and info/error messages.
- Always calls
clamp_selection()at the end, maintaining navigation invariants after any mutation.handle_inputcleanly separates “quit” (returnstrue) from all other inputs, which only mutate state and then returnfalse.This section looks correct and maintainable as-is.
261-325: Instance insertion/removal semantics match the intended endpoint lifecycle
insert_instanceandremove_instancecorrectly maintain the namespaces/components/endpoints hierarchy:
- Lazily creates namespace/component/endpoint entries as needed with reasonable defaults (new endpoints start in
Provisioningbut immediately becomeReadyupon first instance insertion).- Updates
endpoint.updated_atand per-instancelast_seenon eachInstanceUp, so repeated “up” events for the same instance naturally refresh its timestamp.- On
InstanceDown, removes only the specific instance; if that was the last one, marks the endpointOfflinebut keeps it in the tree—this matches the test and is helpful for operators to see offline endpoints instead of having them disappear.Given the TUI’s goals, this behavior is exactly what you want.
327-407: Navigation (move_selection,move_focus,shift_index,clamp_selection) is well thought outThe navigation helpers implement a consistent, K9s-like behavior:
shift_indexwraps around and gracefully handles empty lists.move_selectionadjusts the appropriate index based on the focused column, resetting deeper indices when moving up a level (e.g., changing namespace resets component/endpoint indices).move_focusonly allows moving right if there is at least one component/endpoint to move into, and moves left unconditionally where appropriate.clamp_selection:
- Handles the “no namespaces” case early.
- Clamps all indices to valid ranges.
- Downgrades focus from Components/Endpoints to Namespaces when there’s nothing in those columns, preventing invalid focus states.
This should produce predictable, robust navigation even as the discovery tree changes in real time.
409-452: Health summaries andaggregate_statusencode a clear precedence model
endpoint_health_summaryandcomponent_health_summary:
- For endpoints, surface
status, instance count, and the most recentlast_seen, which is exactly what the UI needs.- For components, start from
Offlineand fold over endpoints withaggregate_status, tracking total/active endpoints and instance count.
aggregate_status’s ordering:
- Prefers
Readywhenever any endpoint is Ready.- Otherwise prefers
Provisioning.- Treats
Unknownas neutral, deferring to the other status.- Falls back to
Offlinewhen everything is Offline.This precedence is reasonable and deterministic. If you ever need a different ordering (e.g., treating Unknown as “worse” than Offline), it will be easy to adjust here centrally.
490-547: Tests cover the key discovery and offline-path behaviorsThe tests validate the most critical flows:
discovery_snapshot_populates_treeensures that:
- Multiple namespaces/components/endpoints are created from a snapshot.
- Component and endpoint instance counts are correct.
- Endpoint status is
Readyafter snapshot seeding.instance_down_marks_endpoint_offlineverifies that:
- Applying
InstanceDowndoes not remove the endpoint node.- The endpoint is marked
Offlineand its instances map is emptied.These directly exercise the same public methods the pipelines use (
apply_event,component_at,endpoint_at, summaries), which is ideal. As you add more UI features (e.g., metrics- or NATS-driven views), similar focused tests aroundAppwill keep things robust.No changes needed.
469-487: Color mapping is semantically sound and properly centralizedThe
as_colormethod uses standard terminal color conventions (Gray for unknown, Yellow for provisioning, Green for ready, Red for offline) that read well across dark and light terminal themes. The method is called from exactly three locations inui.rs, keeping styling logic in one place.launch/dynamo-tui/src/sources.rs (3)
96-136: The review comment is incorrect and should be disregarded.The documented behavior of
tokio::time::intervalis that the firsttick()completes immediately, not "typically waits forinterval" as the review claims. The comment "Fire first tick immediately" is therefore accurate and reflects the actual behavior of the code. The code is correct as written—it provides an immediate snapshot via the explicitbuild_nats_snapshotcall before the loop, and then the interval setup reinforces this with correct semantics.Likely an incorrect or invalid review comment.
138-154: NATS snapshot construction is correct and aligns with async-nats APIThe
Client::statistics()method returns anArc<Statistics>withAtomicU64fields, and theClient::connection_state()method returns theconnection::Stateenum. Your code correctly uses.load(Ordering::Relaxed)on the atomic counters and pattern-matches the connection state. UsingOrdering::Relaxedfor these read-only metrics is appropriate for UI display. The implementation is sound.
50-90: Discovery stream loop behavior confirmed correct; no additional event variants in useThe code appropriately handles the discovery API. The loop:
- Correctly terminates on
cancel.cancelled()or stream end- Tracks
Added(Endpoint)instances and sendsInstanceUp- Sends fully-populated
InstanceDownonRemoved(instance_id)lookups- Safely ignores non-endpoint events with
Ok(_), which is future-proof since onlyAddedandRemovedvariants are used in practice for endpoint discovery
| # Check if image exists | ||
| if ! docker image inspect dynamo-tui:dev >/dev/null 2>&1; then | ||
| echo "Image not found. Building first..." | ||
| ./launch/dynamo-tui/docker-build.sh | ||
| fi | ||
|
|
||
| echo "Running dynamo-tui tests in Docker..." | ||
| echo "" | ||
|
|
||
| # Run tests (build if needed, then test) | ||
| docker run --rm \ | ||
| -v "$PROJECT_ROOT:/workspace" \ | ||
| -w /workspace \ | ||
| rust:1.90.0-slim bash -c " | ||
| apt-get update -qq && apt-get install -y -qq pkg-config libssl-dev build-essential protobuf-compiler >/dev/null 2>&1 | ||
| cargo test -p dynamo-tui -- '$@' | ||
| " |
There was a problem hiding this comment.
🧩 Analysis chain
Fix argument forwarding into the container and avoid building an unused image
Two issues here:
- CLI args are not forwarded to
cargo testinside the container
The inner command runs:
cargo test -p dynamo-tui -- '$@'Because $@ is inside single quotes in the inner bash -c "..." script, it is never expanded; cargo test always receives the literal string $@, and outer-script arguments are effectively ignored. This also matches the ShellCheck SC2145 hint.
You can forward the outer script’s arguments safely using the standard bash -c ... bash "$@" pattern:
-# Run tests (build if needed, then test)
-docker run --rm \
- -v "$PROJECT_ROOT:/workspace" \
- -w /workspace \
- rust:1.90.0-slim bash -c "
- apt-get update -qq && apt-get install -y -qq pkg-config libssl-dev build-essential protobuf-compiler >/dev/null 2>&1
- cargo test -p dynamo-tui -- '$@'
- "
+# Run tests (build if needed, then test)
+docker run --rm \
+ -v "$PROJECT_ROOT:/workspace" \
+ -w /workspace \
+ rust:1.90.0-slim bash -lc '
+ apt-get update -qq && apt-get install -y -qq pkg-config libssl-dev build-essential protobuf-compiler >/dev/null 2>&1
+ cargo test -p dynamo-tui -- "$@"
+ ' bash "$@"This way, $@ in the inner script expands to the arguments you pass to docker-test.sh.
- Image existence check builds
dynamo-tui:devbut tests userust:1.90.0-slim
The script ensures dynamo-tui:dev exists, but then never uses it (tests run in rust:1.90.0-slim). That adds an unnecessary build step and may confuse users.
Either:
- Switch the
docker runto usedynamo-tui:devif that image is intended to be the dev/test environment, or - Drop the image-existence check entirely if tests are meant to run in the stock Rust image.
🏁 Script executed:
#!/bin/bash
# Verify the file exists and show its content
echo "=== Checking docker-test.sh ==="
if [ -f "launch/dynamo-tui/docker-test.sh" ]; then
cat "launch/dynamo-tui/docker-test.sh"
else
echo "File not found"
fiLength of output: 918
🏁 Script executed:
#!/bin/bash
# Check docker-build.sh to understand what dynamo-tui:dev image is
echo "=== Checking docker-build.sh ==="
if [ -f "launch/dynamo-tui/docker-build.sh" ]; then
cat "launch/dynamo-tui/docker-build.sh"
else
echo "File not found"
fiLength of output: 672
🏁 Script executed:
#!/bin/bash
# Search for other references to dynamo-tui:dev or docker-test.sh in the codebase
echo "=== Searching for dynamo-tui:dev references ==="
rg -n "dynamo-tui:dev" --type=sh --type=bash
echo ""
echo "=== Searching for docker-test.sh usage ==="
rg -n "docker-test\.sh" -C2Length of output: 815
Fix argument forwarding into the container and avoid building an unused image
Two issues verified:
- CLI args are not forwarded to
cargo testinside the container
Line 29 runs cargo test -p dynamo-tui -- '$@'. Because $@ is inside single quotes within the bash -c "..." script, it never expands; the DOCKER.md documents usage like ./launch/dynamo-tui/docker-test.sh discovery_snapshot_populates_tree, but those arguments are currently ignored.
Use the standard pattern to forward arguments:
-# Run tests (build if needed, then test)
-docker run --rm \
- -v "$PROJECT_ROOT:/workspace" \
- -w /workspace \
- rust:1.90.0-slim bash -c "
- apt-get update -qq && apt-get install -y -qq pkg-config libssl-dev build-essential protobuf-compiler >/dev/null 2>&1
- cargo test -p dynamo-tui -- '$@'
- "
+# Run tests (build if needed, then test)
+docker run --rm \
+ -v "$PROJECT_ROOT:/workspace" \
+ -w /workspace \
+ rust:1.90.0-slim bash -lc '
+ apt-get update -qq && apt-get install -y -qq pkg-config libssl-dev build-essential protobuf-compiler >/dev/null 2>&1
+ cargo test -p dynamo-tui -- "$@"
+ ' bash "$@"- Image existence check builds
dynamo-tui:devbut tests userust:1.90.0-slim
Lines 15–19 ensure dynamo-tui:dev exists, but line 27 never uses it; tests always run in the stock Rust image. This adds an unnecessary build step.
Either switch docker run to use dynamo-tui:dev or remove the image-existence check if tests are meant to run in the stock image.
📝 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.
| # Check if image exists | |
| if ! docker image inspect dynamo-tui:dev >/dev/null 2>&1; then | |
| echo "Image not found. Building first..." | |
| ./launch/dynamo-tui/docker-build.sh | |
| fi | |
| echo "Running dynamo-tui tests in Docker..." | |
| echo "" | |
| # Run tests (build if needed, then test) | |
| docker run --rm \ | |
| -v "$PROJECT_ROOT:/workspace" \ | |
| -w /workspace \ | |
| rust:1.90.0-slim bash -c " | |
| apt-get update -qq && apt-get install -y -qq pkg-config libssl-dev build-essential protobuf-compiler >/dev/null 2>&1 | |
| cargo test -p dynamo-tui -- '$@' | |
| " | |
| # Check if image exists | |
| if ! docker image inspect dynamo-tui:dev >/dev/null 2>&1; then | |
| echo "Image not found. Building first..." | |
| ./launch/dynamo-tui/docker-build.sh | |
| fi | |
| echo "Running dynamo-tui tests in Docker..." | |
| echo "" | |
| # Run tests (build if needed, then test) | |
| docker run --rm \ | |
| -v "$PROJECT_ROOT:/workspace" \ | |
| -w /workspace \ | |
| rust:1.90.0-slim bash -lc ' | |
| apt-get update -qq && apt-get install -y -qq pkg-config libssl-dev build-essential protobuf-compiler >/dev/null 2>&1 | |
| cargo test -p dynamo-tui -- "$@" | |
| ' bash "$@" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 29-29: Argument mixes string and array. Use * or separate argument.
(SC2145)
🤖 Prompt for AI Agents
In launch/dynamo-tui/docker-test.sh around lines 14 to 30, there are two fixes:
the script currently checks/bails to build dynamo-tui:dev but then runs tests in
rust:1.90.0-slim (so either remove the unused image check or change the docker
run to use dynamo-tui:dev), and CLI args are not forwarded into the container
because $@ is inside single quotes within the bash -c string; change the
invocation to pass arguments correctly (use the common pattern: bash -lc "cargo
test -p dynamo-tui -- \"\$@\"" -- "$@" or otherwise ensure $@ is expanded by the
outer shell into the container command) so that arguments supplied to
docker-test.sh reach cargo test.
| #[tokio::main] | ||
| async fn main() -> anyhow::Result<()> { | ||
| dynamo_runtime::logging::init(); | ||
|
|
||
| let args = Args::parse(); | ||
|
|
||
| let runtime = Runtime::from_settings().context("failed to create runtime from settings")?; | ||
| let distributed_runtime = Arc::new(DistributedRuntime::from_settings(runtime.clone()).await?); | ||
|
|
||
| let cancel_token = CancellationToken::new(); | ||
|
|
||
| let (app_event_tx, app_event_rx) = mpsc::channel::<AppEvent>(256); | ||
| let (input_tx, input_rx) = mpsc::channel::<InputEvent>(32); | ||
|
|
||
| let mut tasks = Vec::new(); | ||
|
|
||
| tasks.push( | ||
| spawn_discovery_pipeline( | ||
| distributed_runtime.clone(), | ||
| app_event_tx.clone(), | ||
| cancel_token.clone(), | ||
| ) | ||
| .await?, | ||
| ); | ||
|
|
||
| if let Some(nats_handle) = spawn_nats_pipeline( | ||
| distributed_runtime.clone(), | ||
| app_event_tx.clone(), | ||
| cancel_token.clone(), | ||
| args.nats_interval, | ||
| ) | ||
| .await? | ||
| { | ||
| tasks.push(nats_handle); | ||
| } | ||
|
|
||
| if let Some(url) = args.metrics_url.clone() { | ||
| if let Some(metrics_handle) = spawn_metrics_pipeline( | ||
| url, | ||
| args.metrics_interval, | ||
| app_event_tx.clone(), | ||
| cancel_token.clone(), | ||
| ) | ||
| .await? | ||
| { | ||
| tasks.push(metrics_handle); | ||
| } | ||
| } | ||
|
|
||
| tasks.push(input::spawn_input_listener(input_tx, cancel_token.clone())); | ||
|
|
||
| let backend = CrosstermBackend::new(std::io::stdout()); | ||
| let terminal = Terminal::new(backend)?; | ||
|
|
||
| execute!(std::io::stdout(), EnterAlternateScreen, EnableMouseCapture)?; | ||
| crossterm::terminal::enable_raw_mode()?; | ||
|
|
||
| let res = run_app(terminal, app_event_rx, input_rx, cancel_token.clone()).await; | ||
|
|
||
| cancel_token.cancel(); | ||
|
|
||
| // Ensure background tasks exit | ||
| for handle in tasks { | ||
| handle.abort(); | ||
| } | ||
|
|
||
| restore_terminal()?; | ||
|
|
||
| if let Err(err) = res { | ||
| tracing::error!(error = %err, "Dynamo TUI exited with error"); | ||
| return Err(err); | ||
| } | ||
|
|
||
| // Shutdown the runtime gracefully | ||
| distributed_runtime.shutdown(); | ||
| runtime.shutdown(); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Returns the terminal to its previous state. | ||
| fn restore_terminal() -> anyhow::Result<()> { | ||
| crossterm::terminal::disable_raw_mode()?; | ||
| execute!(std::io::stdout(), LeaveAlternateScreen)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| async fn run_app( | ||
| mut terminal: Terminal<CrosstermBackend<std::io::Stdout>>, | ||
| mut app_events: mpsc::Receiver<AppEvent>, | ||
| mut input_events: mpsc::Receiver<InputEvent>, | ||
| cancel_token: CancellationToken, | ||
| ) -> anyhow::Result<()> { | ||
| let mut app = App::default(); | ||
| let mut tick = tokio::time::interval(std::time::Duration::from_millis(100)); | ||
|
|
||
| loop { | ||
| tokio::select! { | ||
| _ = cancel_token.cancelled() => break, | ||
| _ = tick.tick() => { | ||
| terminal.draw(|frame| render(frame, &app))?; | ||
| } | ||
| Some(event) = app_events.recv() => { | ||
| app.apply_event(event); | ||
| } | ||
| Some(input) = input_events.recv() => { | ||
| if app.handle_input(input) { | ||
| break; | ||
| } | ||
| } | ||
| else => break, | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
App wiring and shutdown look good; consider symmetric mouse capture teardown
The orchestration from Args parsing through runtime creation, pipeline spawning, and the run_app loop is clean and the cancellation story (CancellationToken + aborting task handles) is reasonable for a TUI.
One minor UX nit: you enable mouse capture when entering the alternate screen:
execute!(std::io::stdout(), EnterAlternateScreen, EnableMouseCapture)?;
crossterm::terminal::enable_raw_mode()?;but restore_terminal only disables raw mode and leaves the alternate screen:
fn restore_terminal() -> anyhow::Result<()> {
crossterm::terminal::disable_raw_mode()?;
execute!(std::io::stdout(), LeaveAlternateScreen)?;
Ok(())
}For full symmetry with crossterm’s API, you may want to also call DisableMouseCapture in restore_terminal, so the terminal is always returned to its previous state even if some terminals don’t reset mouse mode automatically.
🌐 Web query:
What is the recommended pattern in crossterm for enabling and disabling mouse capture and alternate screen in a TUI application?
💡 Result:
Recommended pattern
- On startup: enable raw mode, enter alternate screen, enable mouse capture (and hide cursor if desired).
- On shutdown (or in a Drop/guard): disable mouse capture, leave alternate screen, disable raw mode (and show cursor).
- Wrap setup/teardown in an RAII guard (implement Drop) or use a scope/panic hook so teardown always runs even on panic.
Minimal example (conceptual):
use std::io::stdout;
use crossterm::{
execute,
terminal::{enable_raw_mode, disable_raw_mode, EnterAlternateScreen, LeaveAlternateScreen},
event::{EnableMouseCapture, DisableMouseCapture},
cursor::{Hide, Show},
};
// setup
enable_raw_mode()?;
execute!(stdout(), EnterAlternateScreen, EnableMouseCapture, Hide)?;
// ... run your TUI ...
// teardown (ensure this always runs—Drop/panic-hook/guard)
execute!(stdout(), DisableMouseCapture, LeaveAlternateScreen, Show)?;
disable_raw_mode()?;
References/examples: crossterm API (enter/leave alt screen, enable/disable mouse, raw mode). [1] and a common TUI pattern using Drop for cleanup (ratatui example). [2]
Sources
[1] crossterm docs (terminal & event commands).
[2] ratatui / tui setup & Drop-based teardown example.
Fix asymmetric terminal restoration and missing mouse capture disable
The startup/shutdown order matters for proper terminal state recovery. Currently, restore_terminal() disables raw mode before leaving the alternate screen and is missing DisableMouseCapture entirely.
Based on the recommended crossterm pattern, teardown should disable mouse capture, leave alternate screen, then disable raw mode. Update restore_terminal() to:
fn restore_terminal() -> anyhow::Result<()> {
execute!(std::io::stdout(), DisableMouseCapture, LeaveAlternateScreen)?;
crossterm::terminal::disable_raw_mode()?;
Ok(())
}Additionally, wrap setup/teardown in an RAII guard (implement Drop) or use a panic hook so teardown always runs even on panic, since a panic in run_app would skip restore_terminal() and leave the terminal in a broken state.
🤖 Prompt for AI Agents
In launch/dynamo-tui/src/main.rs around lines 46 to 161, restore_terminal
currently disables raw mode before leaving the alternate screen and omits
DisableMouseCapture, and teardown is not guaranteed on panic; update
restore_terminal to first disable mouse capture and leave the alternate screen,
then disable raw mode (use execute! with DisableMouseCapture and
LeaveAlternateScreen before crossterm::terminal::disable_raw_mode()), and ensure
teardown always runs by introducing an RAII guard type (struct TerminalGuard)
that performs the teardown in Drop (or install a panic hook that calls
restore_terminal) and create an instance at startup so the terminal is restored
even if run_app panics.
| impl<'de> serde::Deserialize<'de> for DiscoveryInstance { | ||
| fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> | ||
| where | ||
| D: serde::Deserializer<'de>, | ||
| { | ||
| // First, try to deserialize as a JSON value to inspect it | ||
| let value = serde_json::Value::deserialize(deserializer)?; | ||
|
|
||
| // Check if it has a "type" field (new format) | ||
| if let Some(type_value) = value.get("type") { | ||
| // New format with "type" field | ||
| match type_value.as_str() { | ||
| Some("Endpoint") => { | ||
| // Remove the "type" field before deserializing as Instance | ||
| let mut instance_value = value.clone(); | ||
| if let Some(obj) = instance_value.as_object_mut() { | ||
| obj.remove("type"); | ||
| } | ||
| let instance: crate::component::Instance = serde_json::from_value(instance_value) | ||
| .map_err(|e| serde::de::Error::custom(format!( | ||
| "Failed to deserialize Endpoint: {}", e | ||
| )))?; | ||
| Ok(DiscoveryInstance::Endpoint(instance)) | ||
| } | ||
| Some("Model") => { | ||
| // Deserialize as Model variant | ||
| let model: ModelVariant = serde_json::from_value(value) | ||
| .map_err(|e| serde::de::Error::custom(format!( | ||
| "Failed to deserialize Model: {}", e | ||
| )))?; | ||
| Ok(DiscoveryInstance::Model { | ||
| namespace: model.namespace, | ||
| component: model.component, | ||
| endpoint: model.endpoint, | ||
| instance_id: model.instance_id, | ||
| card_json: model.card_json, | ||
| }) | ||
| } | ||
| Some(unknown) => Err(serde::de::Error::custom(format!( | ||
| "Unknown DiscoveryInstance type: {}", unknown | ||
| ))), | ||
| None => Err(serde::de::Error::custom( | ||
| "DiscoveryInstance type field is not a string" | ||
| )), | ||
| } | ||
| } else { | ||
| // Old format: try to deserialize as an Endpoint (Instance) | ||
| // This handles backward compatibility with data stored before the DiscoveryInstance enum | ||
| let instance: crate::component::Instance = serde_json::from_value(value) | ||
| .map_err(|e| serde::de::Error::custom(format!( | ||
| "Failed to deserialize as old format Instance: {}", e | ||
| )))?; | ||
| Ok(DiscoveryInstance::Endpoint(instance)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix rustfmt and trailing whitespace to unblock CI
The impl<'de> Deserialize<'de> for DiscoveryInstance block currently fails cargo fmt -- --check, and there is trailing whitespace reported by pre-commit (around Line 160–163 in this file). Please run cargo fmt and clean up trailing whitespace so the Rust and pre-commit checks pass.
🧰 Tools
🪛 GitHub Actions: Pre Merge Validation of (ai-dynamo/dynamo/refs/pull/4296/merge) by AsadShahid04.
[error] 160-160: Trailing whitespace detected by pre-commit hook and modified in this file.
🪛 GitHub Actions: Rust pre-merge checks
[error] 160-202: cargo fmt -- --check failed (exit code 1) due to formatting differences detected in lib/runtime/src/discovery/mod.rs. Run 'cargo fmt' to fix formatting.
🤖 Prompt for AI Agents
In lib/runtime/src/discovery/mod.rs around lines 156 to 211, the Deserialize
impl is misformatted and contains trailing whitespace (notably around lines
160–163); run `cargo fmt` (or `rustfmt`) to fix formatting, remove any trailing
spaces on the reported lines, and re-run pre-commit/cargo fmt -- --check to
ensure the file is clean before committing.
|
@AsadShahid04 - are you planning to address the comments from CodeRabbit? If not, we will close PR in 7 days |
3 participants
Description
Implements the MVP Terminal User Interface (TUI) for Dynamo as requested in #1453. This provides a K9s-like interface for monitoring Dynamo deployments in real-time.
The TUI watches ETCD to discover namespaces, components, and endpoints, monitors NATS connection statistics, and optionally displays Prometheus metrics for performance monitoring.
The screenshot shows the TUI successfully discovering and displaying:
dynamonamespace with 1 componentbackendcomponent in Ready state with 2/2 endpoints and 2 instancesclear_kv_blocksandgenerateendpoints, both Ready with 1 instance eachFeatures Implemented
Core TUI (#1453)
Health Monitoring (#1454)
NATS Monitoring (#1455)
Metrics Display (#1456)
Changes
This PR includes the complete TUI implementation:
New Files:
launch/dynamo-tui/- Complete TUI packagesrc/app.rs- Core state management and event handlingsrc/ui.rs- Ratatui-based UI renderingsrc/sources.rs- ETCD, NATS, and metrics data sourcessrc/input.rs- Keyboard input handlingsrc/metrics.rs- Prometheus metrics parsingsrc/main.rs- Application entry pointREADME.md- User documentationCargo.toml- Package configurationDockerfile.dev- Docker-based development setupdocker-build.sh,docker-run.sh,docker-test.sh- Docker helper scriptsDOCKER.md- Docker development documentationBug Fixes:
discovery_snapshot_populates_treens-a/frontend/httpwith instances 1 and 2 (same endpoint, 2 instances)ns-a/router/grpcwith instance 3 (different component)ns-b/worker/enginewith instance 4 (different namespace)readmefield toworkspace.packageinCargo.tomlto fix workspace build errorsBackward Compatibility:
lib/runtime/src/discovery/mod.rs)"type": "Endpoint"field, but workers using pre-built wheels register without this field, causing parsing errors:"Failed to parse discovery instance from watch event missing field 'type'"DeserializeforDiscoveryInstancethat handles both formats:"type"field): Deserializes asDiscoveryInstance::EndpointorDiscoveryInstance::Model"type"field): Automatically wraps rawInstancedata asDiscoveryInstance::EndpointDocker Development Setup:
Dockerfile.dev: Linux-based build environment (fixes macOSinotifyissues)protobuf-compilerforetcd-clientcompilationUsage
Native Build
Docker (Recommended for macOS)
Keyboard Shortcuts
↑/k,↓/j- Navigate within focused column←/h,→/l- Move focus between columnsr- Manual refreshq,Esc,Ctrl+C- ExitTesting
Related Issues
Closes #1453 (TUI for Dynamo MVP)
Implements features from:
Reference
Based on the initial prototype in PR #689 by @ishandhanani
Notes
dynamo_frontend_*)rust-toolchain.toml)inotify)Summary by CodeRabbit
Release Notes
New Features
Documentation