chore: standardize count metric descriptions on "Number of"#3314
Open
chet wants to merge 1 commit into
Open
Conversation
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
(=^-ω-^=) 🥕 ✅ Action performedFull review finished. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR standardizes human-readable metric and event descriptions across observability code, Prometheus fixtures, and documentation. Metric names, types, labels, callbacks, observed values, and recording behavior remain unchanged, except for one measured-boot fixture cleanup. ChangesMetric description standardization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3314.docs.buildwithfern.com/infra-controller |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/api-core/src/listener.rs (1)
299-310: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWiden the failure wording.
Line 309 is too narrow: this counter is incremented for both TCP accept errors and TLS handshake failures, so “TCP connections” misstates what operators will see in Prometheus. Consider wording it as “Number of connection attempts that failed” or “Number of TCP/TLS connections that failed.”
Suggested wording
- .with_description("Number of TCP connections that failed") + .with_description("Number of connection attempts that failed")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/listener.rs` around lines 299 - 310, The description for the failure counter in listener.rs is too narrow for what connection_failed_counter actually measures. Update the with_description text in the metric setup near connection_total_counter/connection_succeeded_counter/connection_failed_counter so it reflects both TCP accept errors and TLS handshake failures, using wording like “Number of connection attempts that failed” or “Number of TCP/TLS connections that failed.”
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-core/src/machine_update_manager/dpu_nic_firmware_metrics.rs`:
- Line 50: Update the DPU firmware metrics fixture so its HELP text matches the
current metric descriptions in dpu_nic_firmware_metrics.rs. The fixture
test_dpu_nic_firmware_metrics.txt still contains the old wording for all three
metrics, so sync each HELP line with the corresponding definitions in the
DpuNicFirmwareMetrics setup, especially the metric descriptions built with
with_description().
In `@crates/api-core/src/machine_update_manager/host_firmware.rs`:
- Line 261: The host firmware metrics fixture is out of sync with the current
metric descriptions for carbide_pending_host_firmware_update_count and
carbide_active_host_firmware_update_count. Update the expected # HELP text in
the host firmware metrics test fixture so it matches the descriptions emitted by
the related host firmware metrics definitions in
machine_update_manager::host_firmware, keeping both metric names aligned with
the current wording.
In `@crates/api-core/src/machine_validation/metrics.rs`:
- Line 50: The machine-validation metrics HELP text in the metrics definition no
longer matches the fixture used by the direct parsed-metrics comparison test.
Update the description in the machine-validation metrics setup so it matches the
expected text in the test metrics fixture, and verify the referenced metric in
the metrics collector test still uses the same HELP string as the definition.
In `@crates/api-core/src/measured_boot/metrics_collector/metrics.rs`:
- Line 85: The measured-boot metrics HELP text has changed in
MetricsCollector::with_description, but the golden fixture still expects the old
wording. Update the HELP lines in the measured-boot test data so they match the
current description strings from metrics.rs, keeping the fixture aligned with
the metrics emitted by the collector.
In `@crates/bmc-proxy/src/bmc_proxy.rs`:
- Around line 249-253: The failure counter description in bmc_proxy::BmcProxy is
misleading because it is used for both TCP accept errors and TLS handshake
failures. Update the metric help text in the connection_failed_counter setup to
reflect both paths, or split the counting in the relevant accept/handshake logic
into separate counters if you want distinct TCP and TLS failure metrics.
---
Outside diff comments:
In `@crates/api-core/src/listener.rs`:
- Around line 299-310: The description for the failure counter in listener.rs is
too narrow for what connection_failed_counter actually measures. Update the
with_description text in the metric setup near
connection_total_counter/connection_succeeded_counter/connection_failed_counter
so it reflects both TCP accept errors and TLS handshake failures, using wording
like “Number of connection attempts that failed” or “Number of TCP/TLS
connections that failed.”
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 54696b23-4273-4797-aed7-a4c52803d906
📒 Files selected for processing (34)
crates/agent/src/instrumentation.rscrates/api-core/src/handlers/machine_scout.rscrates/api-core/src/listener.rscrates/api-core/src/logging/service_health_metrics.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware_metrics.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/machine_validation/metrics.rscrates/api-core/src/measured_boot/metrics_collector/metrics.rscrates/api-core/src/secrets/metrics.rscrates/bmc-proxy/src/bmc_proxy.rscrates/dhcp/src/metrics.rscrates/dns/src/lib.rscrates/dpa-manager/src/metrics.rscrates/dsx-exchange-consumer/src/metrics.rscrates/fmds/src/http_request_metrics.rscrates/health-metrics/src/lib.rscrates/health/src/collectors/runtime.rscrates/health/src/lib.rscrates/health/src/metrics.rscrates/ib-fabric/src/metrics.rscrates/machine-controller/src/metrics.rscrates/mqtt-common/src/metrics.rscrates/network-segment-controller/src/metrics.rscrates/preingestion-manager/src/metrics.rscrates/secrets/src/forge_vault.rscrates/site-explorer/src/metrics.rscrates/sqlx-query-tracing/src/lib.rscrates/ssh-console/src/bmc/client_pool.rscrates/ssh-console/src/ssh_server.rscrates/state-controller/src/controller/periodic_enqueuer.rscrates/state-controller/src/controller/processor.rscrates/state-controller/src/metrics.rsdocs/observability/core_metrics.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
crates/bmc-proxy/src/bmc_proxy.rs (1)
241-253: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winBroaden the failure metric description.
This is the same mismatch previously called out:
connection_failed_counterstill records TCP accept errors, TLS certificate reload failures, and TLS handshake failures, so “TCP connections that failed” under-describes the metric.♻️ Proposed fix
- .with_description("Number of TCP connections that failed") + .with_description("Number of connections that failed")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/bmc-proxy/src/bmc_proxy.rs` around lines 241 - 253, The `connection_failed_counter` metric description is too narrow for what it counts. Update the description in `BmcProxy::...` where `connection_failed_counter` is built so it covers all failure cases the counter records, including TCP accept errors, TLS certificate reload failures, and TLS handshake failures. Keep the metric name the same and just broaden the wording in the `with_description` call to match its actual usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-core/src/listener.rs`:
- Line 309: The metric description in the listener counter is too narrow because
it covers both TCP accept failures and TLS handshake failures. Update the
description used in the listener setup around with_description so it reflects
all connection failures, not just TCP, and keep it aligned with the counter’s
actual scope in the listener logic.
In `@crates/api-core/src/logging/service_health_metrics.rs`:
- Line 74: The HELP description in service_health_metrics was changed, so update
the matching expected strings in the service-health metrics fixture as well.
Keep the descriptions in ServiceHealthMetrics aligned with the exact text
asserted by test_service_health_metrics.txt, including the other affected metric
descriptions at the referenced spots, so the exact metrics comparison continues
to pass.
In `@crates/ib-fabric/src/metrics.rs`:
- Line 351: The description for
carbide_ib_monitor_allow_insecure_fabric_configuration_count duplicates the
insecure_fabric_configuration wording, so update the metric metadata in
metrics.rs to clearly describe the allow_insecure_fabric_configuration boolean
tracked by the builder around the gauge definition. Use the existing metric
registration code for the two gauges to give each a distinct, accurate
description that reflects its own configuration state and avoids contract
ambiguity.
In `@docs/observability/core_metrics.md`:
- Line 55: The metric descriptions generated from
`crates/ib-fabric/src/metrics.rs` and `crates/nvlink-manager/src/metrics.rs`
still use internal field-style wording. Update the description strings in the
relevant metric definitions so the docs read naturally, following the
`*_status_updates_count` entries and phrasing them like “Number of machines
whose InfiniBand status observation was updated” and “Number of machines whose
NVLink status observation was updated.”
---
Duplicate comments:
In `@crates/bmc-proxy/src/bmc_proxy.rs`:
- Around line 241-253: The `connection_failed_counter` metric description is too
narrow for what it counts. Update the description in `BmcProxy::...` where
`connection_failed_counter` is built so it covers all failure cases the counter
records, including TCP accept errors, TLS certificate reload failures, and TLS
handshake failures. Keep the metric name the same and just broaden the wording
in the `with_description` call to match its actual usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e7305efe-607f-46e6-8ab2-edc05d5a2033
📒 Files selected for processing (34)
crates/agent/src/instrumentation.rscrates/api-core/src/handlers/machine_scout.rscrates/api-core/src/listener.rscrates/api-core/src/logging/service_health_metrics.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware_metrics.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/machine_validation/metrics.rscrates/api-core/src/measured_boot/metrics_collector/metrics.rscrates/api-core/src/secrets/metrics.rscrates/bmc-proxy/src/bmc_proxy.rscrates/dhcp/src/metrics.rscrates/dns/src/lib.rscrates/dpa-manager/src/metrics.rscrates/dsx-exchange-consumer/src/metrics.rscrates/fmds/src/http_request_metrics.rscrates/health-metrics/src/lib.rscrates/health/src/collectors/runtime.rscrates/health/src/lib.rscrates/health/src/metrics.rscrates/ib-fabric/src/metrics.rscrates/machine-controller/src/metrics.rscrates/mqtt-common/src/metrics.rscrates/network-segment-controller/src/metrics.rscrates/preingestion-manager/src/metrics.rscrates/secrets/src/forge_vault.rscrates/site-explorer/src/metrics.rscrates/sqlx-query-tracing/src/lib.rscrates/ssh-console/src/bmc/client_pool.rscrates/ssh-console/src/ssh_server.rscrates/state-controller/src/controller/periodic_enqueuer.rscrates/state-controller/src/controller/processor.rscrates/state-controller/src/metrics.rsdocs/observability/core_metrics.md
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
(=^･ω･^=) ✅ Action performedFull review finished. |
Contributor
Author
|
@coderabbitai full_review, thanks! (Rebased over the merged instrumentation sweeps -- the only conflicts were with the Kea-hook PR, which had already deleted the counter constructions and fixture lines this branch re-worded; both sides wanted "Number of", so the resolution was taking main's framework events, which already comply.) |
Contributor
|
(=^-ω-^=) ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/dsx-exchange-consumer/src/metrics.rs`:
- Line 39: Update the DSX metrics documentation table in README.md to match the
standardized descriptions defined by the metric declarations in metrics.rs,
including the metadata cache metric and the other affected entries. Ensure every
listed description matches the `/metrics` HELP output exactly.
In `@crates/ib-fabric/src/metrics.rs`:
- Line 376: Update the metric descriptions near the affected entries to use
complete wording: replace “incl non Forge” with “including non-Forge” and “incl
non Forge managed” with “including non-Forge-managed”.
In `@crates/machine-controller/src/metrics.rs`:
- Around line 437-443: Update the descriptions for the u64_histogram instruments
machine_reboot_attempts_in_booting_with_discovery_image and
machine_reboot_attempts_in_failed_during_discovery to describe recorded
reboot-attempt values/samples rather than counting machines; preserve the
existing state and failure-context details.
In `@crates/site-explorer/src/metrics.rs`:
- Line 526: Update the description for the
carbide_endpoint_exploration_machines_explored_overall_count metric to mention
both emitted label dimensions, expectation and machine_type, using wording such
as “Number of machines explored by expectation and machine type.”
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6250076a-4897-41f2-a790-04495375f278
📒 Files selected for processing (44)
crates/agent/src/instrumentation.rscrates/api-core/src/handlers/machine_scout.rscrates/api-core/src/listener.rscrates/api-core/src/logging/service_health_metrics.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware_metrics.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/machine_validation/fixtures/test_metrics_collector.txtcrates/api-core/src/machine_validation/metrics.rscrates/api-core/src/measured_boot/metrics_collector/metrics.rscrates/api-core/src/measured_boot/tests/test_data/test_metrics_collector.txtcrates/api-core/src/secrets/metrics.rscrates/api-core/src/tests/metrics_fixtures/test_dpu_nic_firmware_metrics.txtcrates/api-core/src/tests/metrics_fixtures/test_host_firmware_metrics.txtcrates/api-core/src/tests/metrics_fixtures/test_machine_update_manager_metrics.txtcrates/api-core/src/tests/metrics_fixtures/test_network_segment_metrics_admin.txtcrates/api-core/src/tests/metrics_fixtures/test_network_segment_metrics_tenant.txtcrates/api-core/src/tests/metrics_fixtures/test_network_segment_metrics_tor.txtcrates/api-core/src/tests/metrics_fixtures/test_service_health_metrics.txtcrates/bmc-proxy/src/bmc_proxy.rscrates/dns/src/lib.rscrates/dpa-manager/src/metrics.rscrates/dsx-exchange-consumer/src/metrics.rscrates/fmds/src/http_request_metrics.rscrates/health-metrics/src/lib.rscrates/health/src/collectors/runtime.rscrates/health/src/lib.rscrates/health/src/metrics.rscrates/ib-fabric/src/metrics.rscrates/machine-controller/src/metrics.rscrates/mqtt-common/src/metrics.rscrates/network-segment-controller/src/metrics.rscrates/nvlink-manager/src/metrics.rscrates/preingestion-manager/src/fixtures/test_metrics_collector.txtcrates/preingestion-manager/src/metrics.rscrates/secrets/src/forge_vault.rscrates/site-explorer/src/metrics.rscrates/sqlx-query-tracing/src/lib.rscrates/ssh-console/src/bmc/client_pool.rscrates/ssh-console/src/ssh_server.rscrates/state-controller/src/controller/periodic_enqueuer.rscrates/state-controller/src/controller/processor.rscrates/state-controller/src/metrics.rsdocs/observability/core_metrics.md
chet
force-pushed
the
gh-issue-3307
branch
2 times, most recently
from
5c79d2b to
afc6fb4
Compare
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
(=^･ω･^=) ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/api-core/src/measured_boot/metrics_collector/metrics.rs (1)
163-186: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the “per a given” wording in metric descriptions.
Use “for a given measured boot bundle state” and “for a given measured boot machine state”; the current phrasing is grammatically incorrect in exported HELP text.
As per path instructions, Markdown and user-facing text should be reviewed for grammar and clarity.
Proposed wording
- .with_description("Number of machines per a given measured boot bundle state.") + .with_description("Number of machines for a given measured boot bundle state.") - .with_description("Number of machines per a given measured boot machine state.") + .with_description("Number of machines for a given measured boot machine state.")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/measured_boot/metrics_collector/metrics.rs` around lines 163 - 186, Update the descriptions in the observable gauges for the bundle-state and machine-state metrics to replace “per a given” with “for a given,” using “Number of machines for a given measured boot bundle state.” and “Number of machines for a given measured boot machine state.”Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/bmc-proxy/src/bmc_proxy.rs`:
- Around line 241-242: The description for connection_total_counter inaccurately
says “TLS connections” although the counter increments before listener.accept()
and includes TCP accept failures. Update its with_description text to “Number of
inbound connection attempts,” or move the increment to the successful accept
path if the metric should count only accepted connections.
In `@crates/ib-fabric/src/metrics.rs`:
- Around line 167-169: Update the descriptions for the gauge near
carbide_ib_monitor_machines_by_port_state_count and the adjacent partition
metric to use consistent grammar: write “Number of machines whose total and
active port counts match the attribute values” and lowercase “machines” in the
partition description.
In `@crates/machine-controller/src/metrics.rs`:
- Around line 445-450: Update the description in the
carbide_hosts_with_bios_password_set observable gauge registration to use
sentence case, changing “Number of Hosts...” to “Number of hosts...”, and
synchronize the corresponding generated documentation row.
In `@crates/site-explorer/src/metrics.rs`:
- Around line 738-740: Update the metric HELP text associated with the blocker
description to capitalize the acronym consistently, replacing “host+dpu” with
“host+DPU” and “dpu(s)” with “DPU(s)”.
In `@crates/state-controller/src/metrics.rs`:
- Around line 344-347: Update the description in the object-state observable
gauge within the metrics registration code to use present-tense wording,
replacing “had been longer in a state than allowed per SLA” with language
indicating objects currently exceeding their allowed state SLA.
- Around line 374-376: Update the metric description in the builder for the
callback observing handling_errors_per_type and its sum to describe the count of
state-handling errors, not the number of objects; retain the object_type context
while aligning the wording with the observed value.
---
Outside diff comments:
In `@crates/api-core/src/measured_boot/metrics_collector/metrics.rs`:
- Around line 163-186: Update the descriptions in the observable gauges for the
bundle-state and machine-state metrics to replace “per a given” with “for a
given,” using “Number of machines for a given measured boot bundle state.” and
“Number of machines for a given measured boot machine state.”
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 73ac0ab2-122d-4fa5-af43-07e6133659ad
📒 Files selected for processing (45)
crates/agent/src/instrumentation.rscrates/api-core/src/handlers/machine_scout.rscrates/api-core/src/listener.rscrates/api-core/src/logging/service_health_metrics.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware_metrics.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/machine_validation/fixtures/test_metrics_collector.txtcrates/api-core/src/machine_validation/metrics.rscrates/api-core/src/measured_boot/metrics_collector/metrics.rscrates/api-core/src/measured_boot/tests/test_data/test_metrics_collector.txtcrates/api-core/src/secrets/metrics.rscrates/api-core/src/tests/metrics_fixtures/test_dpu_nic_firmware_metrics.txtcrates/api-core/src/tests/metrics_fixtures/test_host_firmware_metrics.txtcrates/api-core/src/tests/metrics_fixtures/test_machine_update_manager_metrics.txtcrates/api-core/src/tests/metrics_fixtures/test_network_segment_metrics_admin.txtcrates/api-core/src/tests/metrics_fixtures/test_network_segment_metrics_tenant.txtcrates/api-core/src/tests/metrics_fixtures/test_network_segment_metrics_tor.txtcrates/api-core/src/tests/metrics_fixtures/test_service_health_metrics.txtcrates/bmc-proxy/src/bmc_proxy.rscrates/dns/src/lib.rscrates/dpa-manager/src/metrics.rscrates/dsx-exchange-consumer/README.mdcrates/dsx-exchange-consumer/src/metrics.rscrates/fmds/src/http_request_metrics.rscrates/health-metrics/src/lib.rscrates/health/src/collectors/runtime.rscrates/health/src/lib.rscrates/health/src/metrics.rscrates/ib-fabric/src/metrics.rscrates/machine-controller/src/metrics.rscrates/mqtt-common/src/metrics.rscrates/network-segment-controller/src/metrics.rscrates/nvlink-manager/src/metrics.rscrates/preingestion-manager/src/fixtures/test_metrics_collector.txtcrates/preingestion-manager/src/metrics.rscrates/secrets/src/forge_vault.rscrates/site-explorer/src/metrics.rscrates/sqlx-query-tracing/src/lib.rscrates/ssh-console/src/bmc/client_pool.rscrates/ssh-console/src/ssh_server.rscrates/state-controller/src/controller/periodic_enqueuer.rscrates/state-controller/src/controller/processor.rscrates/state-controller/src/metrics.rsdocs/observability/core_metrics.md
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
(=^･ω･^=) ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/site-explorer/src/metrics.rs`:
- Line 661: The metric descriptions for the machine-pair and power-shelf gauges
use awkward past-tense wording and unnecessary capitalization. Update the
strings passed to the relevant with_description calls near the machine-pair
metric and the corresponding power-shelf metric to concise present-tense wording
that directly describes the observed count, using lowercase common nouns.
- Around line 759-761: Update the metric description associated with the
migration signal callback to describe the dimension as “by signal type” rather
than “by kind,” while preserving the existing signal categories and `signal`
label.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d0e9abf8-242a-478d-bd68-9b5f86bb81a5
📒 Files selected for processing (45)
crates/agent/src/instrumentation.rscrates/api-core/src/handlers/machine_scout.rscrates/api-core/src/listener.rscrates/api-core/src/logging/service_health_metrics.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware_metrics.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/machine_validation/fixtures/test_metrics_collector.txtcrates/api-core/src/machine_validation/metrics.rscrates/api-core/src/measured_boot/metrics_collector/metrics.rscrates/api-core/src/measured_boot/tests/test_data/test_metrics_collector.txtcrates/api-core/src/secrets/metrics.rscrates/api-core/src/tests/metrics_fixtures/test_dpu_nic_firmware_metrics.txtcrates/api-core/src/tests/metrics_fixtures/test_host_firmware_metrics.txtcrates/api-core/src/tests/metrics_fixtures/test_machine_update_manager_metrics.txtcrates/api-core/src/tests/metrics_fixtures/test_network_segment_metrics_admin.txtcrates/api-core/src/tests/metrics_fixtures/test_network_segment_metrics_tenant.txtcrates/api-core/src/tests/metrics_fixtures/test_network_segment_metrics_tor.txtcrates/api-core/src/tests/metrics_fixtures/test_service_health_metrics.txtcrates/bmc-proxy/src/bmc_proxy.rscrates/dns/src/lib.rscrates/dpa-manager/src/metrics.rscrates/dsx-exchange-consumer/README.mdcrates/dsx-exchange-consumer/src/metrics.rscrates/fmds/src/http_request_metrics.rscrates/health-metrics/src/lib.rscrates/health/src/collectors/runtime.rscrates/health/src/lib.rscrates/health/src/metrics.rscrates/ib-fabric/src/metrics.rscrates/machine-controller/src/metrics.rscrates/mqtt-common/src/metrics.rscrates/network-segment-controller/src/metrics.rscrates/nvlink-manager/src/metrics.rscrates/preingestion-manager/src/fixtures/test_metrics_collector.txtcrates/preingestion-manager/src/metrics.rscrates/secrets/src/forge_vault.rscrates/site-explorer/src/metrics.rscrates/sqlx-query-tracing/src/lib.rscrates/ssh-console/src/bmc/client_pool.rscrates/ssh-console/src/ssh_server.rscrates/state-controller/src/controller/periodic_enqueuer.rscrates/state-controller/src/controller/processor.rscrates/state-controller/src/metrics.rsdocs/observability/core_metrics.md
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-core/src/measured_boot/metrics_collector/metrics.rs`:
- Line 164: The metric HELP descriptions in the measured boot metrics
definitions are grammatically awkward; update the bundle-state description to
“Number of machines in each measured boot bundle state” and the machine-state
description to “Number of machines in each measured boot machine state,” then
update the corresponding golden fixture lines.
In `@crates/ib-fabric/src/metrics.rs`:
- Around line 397-400: Update the description passed to the
carbide_ib_monitor_ufm_ports_by_state_count gauge registration to explicitly
state that the metric reports port counts grouped by port_state, while retaining
the note about UFM and non-Forge-managed ports.
In `@crates/machine-controller/src/metrics.rs`:
- Around line 126-127: Update the description passed to the
carbide_gpus_total_count gauge in the metrics registration code to describe the
total GPUs accumulated from object_metrics.num_gpus, not GPU availability;
reserve availability/allocatable wording for the separate gpus_usable metric.
In `@crates/network-segment-controller/src/metrics.rs`:
- Line 64: Update the descriptions of all three metrics in the metrics
definitions, including the entries near the currently referenced line and the
additional occurrences, to describe per-network-segment values rather than
site-wide totals. Use consistent wording such as “Number of available IPs per
network segment” for each metric’s with_description call.
In `@crates/ssh-console/src/bmc/client_pool.rs`:
- Around line 219-221: Update the description of the _failed_machines observable
gauge in the client pool metric initialization to accurately state that it
counts host BMCs in the ConnectionError state, using wording such as “Number of
host BMCs with connection errors” rather than implying multiple errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 21d20f37-07a9-4f4e-aec9-4066b3d0590f
📒 Files selected for processing (45)
crates/agent/src/instrumentation.rscrates/api-core/src/handlers/machine_scout.rscrates/api-core/src/listener.rscrates/api-core/src/logging/service_health_metrics.rscrates/api-core/src/machine_update_manager/dpu_nic_firmware_metrics.rscrates/api-core/src/machine_update_manager/host_firmware.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/machine_validation/fixtures/test_metrics_collector.txtcrates/api-core/src/machine_validation/metrics.rscrates/api-core/src/measured_boot/metrics_collector/metrics.rscrates/api-core/src/measured_boot/tests/test_data/test_metrics_collector.txtcrates/api-core/src/secrets/metrics.rscrates/api-core/src/tests/metrics_fixtures/test_dpu_nic_firmware_metrics.txtcrates/api-core/src/tests/metrics_fixtures/test_host_firmware_metrics.txtcrates/api-core/src/tests/metrics_fixtures/test_machine_update_manager_metrics.txtcrates/api-core/src/tests/metrics_fixtures/test_network_segment_metrics_admin.txtcrates/api-core/src/tests/metrics_fixtures/test_network_segment_metrics_tenant.txtcrates/api-core/src/tests/metrics_fixtures/test_network_segment_metrics_tor.txtcrates/api-core/src/tests/metrics_fixtures/test_service_health_metrics.txtcrates/bmc-proxy/src/bmc_proxy.rscrates/dns/src/lib.rscrates/dpa-manager/src/metrics.rscrates/dsx-exchange-consumer/README.mdcrates/dsx-exchange-consumer/src/metrics.rscrates/fmds/src/http_request_metrics.rscrates/health-metrics/src/lib.rscrates/health/src/collectors/runtime.rscrates/health/src/lib.rscrates/health/src/metrics.rscrates/ib-fabric/src/metrics.rscrates/machine-controller/src/metrics.rscrates/mqtt-common/src/metrics.rscrates/network-segment-controller/src/metrics.rscrates/nvlink-manager/src/metrics.rscrates/preingestion-manager/src/fixtures/test_metrics_collector.txtcrates/preingestion-manager/src/metrics.rscrates/secrets/src/forge_vault.rscrates/site-explorer/src/metrics.rscrates/sqlx-query-tracing/src/lib.rscrates/ssh-console/src/bmc/client_pool.rscrates/ssh-console/src/ssh_server.rscrates/state-controller/src/controller/periodic_enqueuer.rscrates/state-controller/src/controller/processor.rscrates/state-controller/src/metrics.rsdocs/observability/core_metrics.md
Contributor
Author
|
CI fix: the three failing tests were the network-segment exposition fixtures -- they byte-pin the whole scrape, so the state-controller SLA/handling-errors template wording (and the per-segment IP descriptions) from the review rounds had to land in the fixture files too. All three suites pass locally now, plus the firmware/validation fixture suites re-verified. That failing job was never the flake I first suspected -- my miss for syncing the catalogue but not these fixtures. |
Every count-style metric description now opens with `Number of` -- the preferred prefix @polarweasel called out in the NVIDIA#3266 review -- instead of the previous mix of "The amount of", "The number of", "The total number of", "Count of", etc. Primary callouts are: - The vault client counters in `forge_vault.rs` (`carbide-api.vault.requests_*`) claimed to count "tls connections" -- a copy-paste from the bmc-proxy metrics. They describe Vault requests now. - The `state-controller` templates lose their mangled grammar: "The amount of types that object handling tasks that have been ..." becomes "Number of object handling tasks that have been ...", and the state entered/exited counters now read "Number of times objects of type X have entered a certain state". - Small grammar fixes ride along inside touched strings only -- "machine validation runs", "pairs that have been identified", "machines that report software components", and lowercase "ips"/"tls"/"tcp" become "IPs"/"TLS"/"TCP". - The golden `/metrics` fixtures assert exposition output verbatim, so their `# HELP` lines get the same rewrite -- 67 lines across 11 fixture files. - Time and duration descriptions ("The amount of time ...", "Total time ...") are not counts and stay untouched, as does the multi-kind `carbide_site_explorer_update_explored_endpoints_count` row. Tests updated! Verified by running `test_integration` locally -- the regenerated `core_metrics.md` comes out identical to what's committed here. This supports NVIDIA#3307 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
Author
|
CI: the repo-clean check was failing because several catalogue rows had drifted from the code's descriptions during the review-round amends -- this push replaces the hand-synced rows with the regeneration's own output (local test_integration green, regen-stable by construction). The check should pass this cycle. |
nick-vetro
approved these changes
Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every count-style metric description now opens with
Number of-- the preferred prefix @polarweasel called out in the #3266 review -- instead of the previous mix of "The amount of", "The number of", "The total number of", "Count of", etc.Primary callouts are:
forge_vault.rs(carbide-api.vault.requests_*) claimed to count "tls connections" -- a copy-paste from the bmc-proxy metrics. They describe Vault requests now.state-controllertemplates lose their mangled grammar: "The amount of types that object handling tasks that have been ..." becomes "Number of object handling tasks that have been ...", and the state entered/exited counters now read "Number of times objects of type X have entered a certain state"./metricsfixtures assert exposition output verbatim, so their# HELPlines get the same rewrite -- 67 lines across 11 fixture files.carbide_site_explorer_update_explored_endpoints_countrow.Tests updated!
Verified by running
test_integrationlocally -- the regeneratedcore_metrics.mdcomes out identical to what's committed here.This supports #3307