Skip to content

Conversation

@keivenchang
Copy link
Contributor

@keivenchang keivenchang commented Oct 7, 2025

Overview:

This PR removes the deprecated components/metrics aggregation service and all references to it. The metrics functionality has been replaced by the new MetricsRegistry system built into the runtime.

Details:

  • Remove components/metrics directory (6 files: Cargo.toml, README.md, source files, and mock worker)
  • Remove deprecated grafana-llm-metrics.json dashboard that relied on old llm_kv_* metrics
  • Update workspace configuration in Cargo.toml to remove metrics component
  • Clean up deploy/metrics/README.md by removing references to deprecated metrics component
  • Update deploy/metrics/prometheus.yml to remove the deprecated metrics-aggregation-service scrape job
  • Fix frontend default port in prometheus.yml from 8080 to correct default 8000

Where should the reviewer start?

  • Cargo.toml - verify workspace members are correct
  • deploy/metrics/prometheus.yml - verify scrape configs are still valid
  • deploy/metrics/README.md - verify documentation is still accurate

/coderabbit profile chill

Summary by CodeRabbit

  • New Features

    • Added a new Grafana dashboard for KVBM metrics.
  • Documentation

    • Updated deployment docs to reference Prometheus targets for validation and removed legacy metrics instructions.
  • Chores

    • Removed the deprecated metrics component and its related binaries, library, and documentation.
    • Deleted the legacy Grafana LLM metrics dashboard.
    • Updated Prometheus configuration: changed demo frontend scrape port to 8000 and removed the deprecated aggregation job.
  • Refactor

    • Streamlined metrics setup by eliminating legacy paths and aligning configs with current monitoring practices.

- Remove components/metrics directory and all associated files
- Remove deprecated grafana-llm-metrics.json dashboard (used old llm_kv_* metrics)
- Update Cargo.toml workspace members
- Update deploy/metrics/README.md to remove references to deprecated metrics component
- Update deploy/metrics/prometheus.yml to remove metrics-aggregation-service job
- Fix frontend port from 8080 to correct default 8000

Signed-off-by: Keiven Chang <[email protected]>
@keivenchang keivenchang requested review from a team as code owners October 7, 2025 22:49
@keivenchang keivenchang self-assigned this Oct 7, 2025
@github-actions github-actions bot added the chore label Oct 7, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 7, 2025

Walkthrough

Removed the deprecated metrics component crate (library, binaries, manifest, docs) and legacy Grafana dashboard. Updated workspace Cargo configuration, deployment README, and Prometheus scrape config (frontend port 8080→8000; removed metrics-aggregation job). Documentation now references Prometheus targets and a new KVBM dashboard.

Changes

Cohort / File(s) Change summary
Workspace configuration
Cargo.toml
Removed components/metrics from workspace members and default-members.
Metrics component removal
components/metrics/Cargo.toml, components/metrics/README.md, components/metrics/src/lib.rs, components/metrics/src/main.rs, components/metrics/src/bin/mock_worker.rs
Deleted the entire deprecated metrics crate, including manifest, library, CLI, mock worker binary, and documentation.
Deploy docs update
deploy/metrics/README.md
Updated instructions to use Prometheus targets; removed references to the deprecated metrics program; added KVBM dashboard reference.
Grafana dashboards
deploy/metrics/grafana_dashboards/grafana-llm-metrics.json
Removed legacy LLM metrics dashboard JSON.
Prometheus configuration
deploy/metrics/prometheus.yml
Changed demo frontend scrape port 8080→8000; removed metrics-aggregation-service job; updated comments.

Sequence Diagram(s)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I thump my paws—old charts depart,
The metrics crate hops off the cart.
New targets gleam at 9090’s gate,
KVBM panels radiate.
Ports realigned, the scrapes feel right—
Fewer hops, a cleaner night.
Carrot-laden, I commit with delight. 🥕✨

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description follows the template for Overview, Details, and Where should the reviewer start sections but omits the required Related Issues section, leaving out any reference to associated issue numbers. Please add a Related Issues section at the end of the description using one of the action keywords (e.g., Closes #123) to reference the GitHub issue(s) addressed by this PR.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly and accurately summarizes the primary change, which is removing the deprecated components/metrics directory and related references, without including extraneous details or vague terminology.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bdad6f1 and a893607.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • components/metrics/images/dynamo_metrics_grafana.png is excluded by !**/*.png
📒 Files selected for processing (9)
  • Cargo.toml (0 hunks)
  • components/metrics/Cargo.toml (0 hunks)
  • components/metrics/README.md (0 hunks)
  • components/metrics/src/bin/mock_worker.rs (0 hunks)
  • components/metrics/src/lib.rs (0 hunks)
  • components/metrics/src/main.rs (0 hunks)
  • deploy/metrics/README.md (3 hunks)
  • deploy/metrics/grafana_dashboards/grafana-llm-metrics.json (0 hunks)
  • deploy/metrics/prometheus.yml (1 hunks)
💤 Files with no reviewable changes (7)
  • components/metrics/src/main.rs
  • components/metrics/Cargo.toml
  • components/metrics/src/bin/mock_worker.rs
  • components/metrics/README.md
  • Cargo.toml
  • deploy/metrics/grafana_dashboards/grafana-llm-metrics.json
  • components/metrics/src/lib.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). (11)
  • GitHub Check: trtllm (amd64)
  • GitHub Check: sglang
  • GitHub Check: vllm (amd64)
  • GitHub Check: tests (launch/dynamo-run)
  • GitHub Check: clippy (launch/dynamo-run)
  • GitHub Check: tests (lib/runtime/examples)
  • GitHub Check: clippy (.)
  • GitHub Check: tests (.)
  • GitHub Check: tests (lib/bindings/python)
  • GitHub Check: clippy (lib/bindings/python)
  • GitHub Check: Build and Test - dynamo
🔇 Additional comments (2)
deploy/metrics/prometheus.yml (1)

36-42: Frontend scrape config aligns with new default.

Commentary and target now consistently point to port 8000; this matches the updated frontend defaults. Looks good.

deploy/metrics/README.md (1)

177-178: KVBM dashboard reference looks good.

Thanks for linking the kvbm dashboard; this keeps the doc aligned with the new Grafana asset.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@rmccorm4 rmccorm4 merged commit be001a5 into main Oct 9, 2025
28 checks passed
@rmccorm4 rmccorm4 deleted the keivenchang/remove-component_metrics branch October 9, 2025 17:25
ziqifan617 pushed a commit that referenced this pull request Oct 20, 2025
nv-tusharma pushed a commit that referenced this pull request Oct 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants