feat(health): Emit latency and health metrics for BMC interactions - #4720
Conversation
|
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:
Summary by CodeRabbit
WalkthroughThe change adds optional, configurable BMC latency histograms. ChangesBMC latency metrics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HealthService
participant MetricsRegistry
participant EndpointSource
participant BmcClient
HealthService->>MetricsRegistry: create BmcLatencyMetrics when enabled
HealthService->>EndpointSource: pass optional metrics handle
EndpointSource->>BmcClient: construct BmcClient with metrics handle
BmcClient->>BmcClient: record Redfish request latency and labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Implementation thoughts: This implementation generates the metrics in the Bmc client wrapper. It thereby is limited to the info that the nvredfish exposes via its public interface and involves a fair amount of code changes. Other approaches we can consider are:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/health/src/bmc.rs`:
- Around line 790-802: Update http_error_status_code to locate the “HTTP ”
marker and parse only the immediately following three-digit status token,
avoiding unrelated numeric values such as URL IP octets; preserve validation
through http::StatusCode and add a regression test covering a message containing
https://10.0.0.100 alongside HTTP 500.
In `@crates/health/src/metrics.rs`:
- Around line 58-94: Remove BmcLatencyAttribute::ServerAddress from the default
BmcLatencyAttribute::ATTRIBUTES set so the All selector does not automatically
expose unbounded BMC IP addresses as Prometheus labels. Leave the enum and
label_name mapping available for explicit opt-in, while preserving the other
default attributes.
- Around line 113-162: Update BmcLatencyMetrics::new_with_attributes to prevent
BmcLatencyAttribute::All from being stored or used as a concrete label. Filter
All from the attributes before constructing label_names and saving the
attributes, or reject it through the constructor’s existing Result contract;
ensure direct callers cannot cause observe() to reach its unreachable! branch.
- Around line 127-134: Rename the metric constructed in the latency HistogramVec
to use the `{prefix}_bmc_latency_milliseconds` suffix, then update all existing
test and example configuration references to the new name. Extend
`test_integration` to exercise the health endpoint so the generated metric
catalogue includes this metric and its HELP text.
🪄 Autofix
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: 7cd43cfc-4a56-408a-831c-e1525610c00b
📒 Files selected for processing (10)
crates/health/example/config.example.tomlcrates/health/src/api_client.rscrates/health/src/bmc.rscrates/health/src/config.rscrates/health/src/discovery/spawn.rscrates/health/src/endpoint/cluster.rscrates/health/src/endpoint/mod.rscrates/health/src/endpoint/sources.rscrates/health/src/lib.rscrates/health/src/metrics.rs
| fn http_error_status_code(message: &str) -> Option<String> { | ||
| message | ||
| .split(|character: char| !character.is_ascii_digit()) | ||
| .find_map(|token| { | ||
| if token.len() != 3 { | ||
| return None; | ||
| } | ||
| let code = token.parse::<u16>().ok()?; | ||
| http::StatusCode::from_u16(code) | ||
| .ok() | ||
| .map(|status| status.as_u16().to_string()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect how HealthError::HttpError messages are constructed to confirm whether they can embed IPs or other 3-digit numbers before the real status code.
rg -nP -C4 'HttpError\s*\(' --type=rust crates/healthRepository: NVIDIA/infra-controller
Length of output: 17517
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- function and nearby tests ---'
sed -n '780,825p' crates/health/src/bmc.rs
rg -n -C6 'http_error_status_code|error_status_code|http_response_status_code' crates/health/src/bmc.rs
printf '%s\n' '--- focused behavioral probe ---'
python3 - <<'PY'
import re
def status_code_current(message):
for token in re.split(r'[^0-9]', message):
if len(token) != 3:
continue
code = int(token)
# http::StatusCode::from_u16 accepts 100..=599.
if 100 <= code <= 599:
return str(code)
return None
def status_code_proposed(message):
parts = message.split("HTTP ", 1)
if len(parts) != 2:
return None
for token in re.split(r'[^0-9]', parts[1]):
if len(token) == 3:
code = int(token)
if 100 <= code <= 599:
return str(code)
return None
cases = [
"https://10.0.0.100:8443: HTTP 500 for switch 12",
"https://10.0.0.7:8443: HTTP 500 for switch 12",
"request failed with HTTP 404",
"request failed with HTTP 500: body contains 404",
"request failed with HTTP 500: retry 404",
"request failed without a status: retry 500",
]
for message in cases:
print(f"{message!r}")
print(f" current={status_code_current(message)!r}")
print(f" proposed={status_code_proposed(message)!r}")
PYRepository: NVIDIA/infra-controller
Length of output: 5847
Parse only the status token after HTTP .
HealthError::HttpError includes URLs and other free-form values. The current scan can report an IP octet such as 100 from https://10.0.0.100 instead of HTTP 500. Parse the 3-digit token immediately after HTTP and add a regression test for this input.
🤖 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/health/src/bmc.rs` around lines 790 - 802, Update
http_error_status_code to locate the “HTTP ” marker and parse only the
immediately following three-digit status token, avoiding unrelated numeric
values such as URL IP octets; preserve validation through http::StatusCode and
add a regression test covering a message containing https://10.0.0.100 alongside
HTTP 500.
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum BmcLatencyAttribute { | ||
| All, | ||
| HttpResponseStatusCode, | ||
| HttpRequestMethod, | ||
| HttpPath, | ||
| ServerAddress, | ||
| UrlScheme, | ||
| BmcVendor, | ||
| BmcModel, | ||
| } | ||
|
|
||
| impl BmcLatencyAttribute { | ||
| pub const ATTRIBUTES: [Self; 7] = [ | ||
| Self::HttpResponseStatusCode, | ||
| Self::HttpRequestMethod, | ||
| Self::HttpPath, | ||
| Self::ServerAddress, | ||
| Self::UrlScheme, | ||
| Self::BmcVendor, | ||
| Self::BmcModel, | ||
| ]; | ||
|
|
||
| pub fn label_name(self) -> &'static str { | ||
| match self { | ||
| Self::All => "all", | ||
| Self::HttpResponseStatusCode => "http_response_status_code", | ||
| Self::HttpRequestMethod => "http_request_method", | ||
| Self::HttpPath => "http_path", | ||
| Self::ServerAddress => "server_address", | ||
| Self::UrlScheme => "url_scheme", | ||
| Self::BmcVendor => "bmc_vendor", | ||
| Self::BmcModel => "bmc_model", | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Bound the label cardinality before enabling this by default.
BmcLatencyAttribute::ServerAddress puts the BMC IP address directly into a Prometheus label. ATTRIBUTES includes it, and the All selector (the documented default in config.rs and config.example.toml) pulls it in automatically. Combined with HttpPath, HttpRequestMethod, HttpResponseStatusCode, BmcVendor, and BmcModel, every distinct BMC endpoint multiplies the number of histogram series. On a large fleet, this can produce a very high cardinality series set once an operator sets enable_bmc_latency_metrics = true without narrowing bmc_latency_attributes.
Exclude ServerAddress from the default attribute set, or document the cardinality cost prominently next to enable_bmc_latency_metrics in config.rs and config.example.toml so operators narrow the label set deliberately before enabling this in production.
As per coding guidelines: "Keep metric label cardinality bounded; use typed bounded labels and put machine IDs, IPs, and error text in context or logs rather than labels."
🤖 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/health/src/metrics.rs` around lines 58 - 94, Remove
BmcLatencyAttribute::ServerAddress from the default
BmcLatencyAttribute::ATTRIBUTES set so the All selector does not automatically
expose unbounded BMC IP addresses as Prometheus labels. Leave the enum and
label_name mapping available for explicit opt-in, while preserving the other
default attributes.
Source: Coding guidelines
| let latency_ms = HistogramVec::new( | ||
| HistogramOpts::new( | ||
| format!("{prefix}_bmc_latency_ms"), | ||
| "Duration of outbound Redfish HTTP requests to BMCs, in milliseconds", | ||
| ) | ||
| .buckets(bmc_latency_buckets_ms()), | ||
| &label_names, | ||
| )?; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Look for a metrics catalogue file that documents framework/hardware-health metrics.
rg -n --iglob '*metric*catalog*' -l . 2>/dev/null
fd -i 'metric' -e md -e toml | xargs -I{} rg -n 'bmc_latency|carbide_hardware_health' {} 2>/dev/nullRepository: NVIDIA/infra-controller
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file structure ---'
ast-grep outline crates/health/src/metrics.rs 2>/dev/null || true
printf '%s\n' '--- target implementation ---'
sed -n '1,220p' crates/health/src/metrics.rs
printf '%s\n' '--- metric references and registration ---'
rg -n -C 3 'bmc_latency|HistogramVec|metrics catalogue|metric catalogue|catalogue|Registry::register|register\(' crates .github 2>/dev/null | head -n 500
printf '%s\n' '--- candidate catalogue and integration-test files ---'
git ls-files | rg -i '(^|/)(metric|metrics|catalog|catalogue|test_integration|integration)' | head -n 300Repository: NVIDIA/infra-controller
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- core metrics catalogue ---'
if [ -f docs/observability/core_metrics.md ]; then
rg -n -C 3 'bmc|hardware_health|latency|component_failures|component_duration' docs/observability/core_metrics.md | head -n 300
wc -l docs/observability/core_metrics.md
else
printf '%s\n' 'catalogue file not found'
fi
printf '%s\n' '--- metric documentation tooling ---'
sed -n '1,260p' crates/xtask/src/metric_docs.rs
rg -n -C 5 'test_integration|core_metrics\.md|check-metric-docs|metrics scrape|/metrics' crates/xtask .github Makefile* Cargo.toml docs 2>/dev/null | head -n 500
printf '%s\n' '--- health metric construction and callers ---'
rg -n -C 5 'BmcLatencyMetrics::|enable_bmc_latency_metrics|MetricsConfig|create_.*registry|framework_registry' crates/health crates | head -n 500Repository: NVIDIA/infra-controller
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact catalogue and health metric names ---'
rg -n 'carbide_hardware_health|bmc_latency_ms|bmc_latency_milliseconds|component_failures|component_duration' docs crates .github Makefile.toml 2>/dev/null | head -n 200
printf '%s\n' '--- integration catalogue generation implementation ---'
rg -n -C 8 'core_metrics\.md|test_integration|generate.*metric|metric.*catalog|scrape.*metrics|/metrics' --glob '*.rs' --glob '*.toml' --glob '*.md' . 2>/dev/null | rg -v 'target/' | head -n 500
printf '%s\n' '--- health tests and callers of BmcLatencyMetrics ---'
rg -n -C 8 'BmcLatencyMetrics|BmcLatencyObservation|bmc_latency' crates/health crates/*/tests crates/*/src 2>/dev/null | head -n 500Repository: NVIDIA/infra-controller
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
metrics = Path("crates/health/src/metrics.rs").read_text()
config = Path("crates/health/src/config.rs").read_text()
catalogue = Path("docs/observability/core_metrics.md").read_text()
integration = Path("crates/api-integration-tests/tests/lib.rs").read_text()
health_bmc = Path("crates/health/src/bmc.rs").read_text()
name_expr = re.search(r'format!\("\{prefix\}_bmc_latency_ms"\)', metrics)
default_prefix = re.search(r'prefix:\s*"([^"]+)"\.to_string\(\)', config).group(1)
actual = f"{default_prefix}_bmc_latency_ms"
print(f"metric_name_expression_found={bool(name_expr)}")
print(f"default_prefix={default_prefix}")
print(f"default_metric_name={actual}")
print(f"catalogue_contains_default_metric={f'<td>{actual}</td>' in catalogue}")
print(f"catalogue_contains_canonical_candidate={f'<td>{default_prefix}_bmc_latency_milliseconds</td>' in catalogue}")
print(f"test_integration_scrapes_metrics={bool(re.search(r'generate_core_metric_docs\\(&test_env\\.carbide_metrics_addrs\\)', integration))}")
print(f"health_bmc_tests_assert_current_name={bool(re.search(r'test_health_bmc_latency_ms', health_bmc))}")
print(f"health_endpoint_in_integration_addresses={('health' in integration.lower() and 'metrics_addrs' in integration)}")
PY
printf '%s\n' '--- focused integration setup and health metric tests ---'
sed -n '260,330p' crates/api-integration-tests/tests/lib.rs
sed -n '2390,2470p' crates/health/src/bmc.rs
sed -n '400,425p' crates/health/example/config.example.tomlRepository: NVIDIA/infra-controller
Length of output: 546
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
metrics = Path("crates/health/src/metrics.rs").read_text()
config = Path("crates/health/src/config.rs").read_text()
catalogue = Path("docs/observability/core_metrics.md").read_text()
integration = Path("crates/api-integration-tests/tests/lib.rs").read_text()
health_bmc = Path("crates/health/src/bmc.rs").read_text()
name_expr = re.search(r'format!\("\{prefix\}_bmc_latency_ms"\)', metrics)
default_prefix = re.search(r'prefix:\s*"([^"]+)"\.to_string\(\)', config).group(1)
actual = f"{default_prefix}_bmc_latency_ms"
integration_scrape = bool(re.search(r'generate_core_metric_docs\(&test_env\.carbide_metrics_addrs\)', integration))
health_test_name = bool(re.search(r'test_health_bmc_latency_ms', health_bmc))
health_endpoint = 'health' in integration.lower() and 'metrics_addrs' in integration
print(f"metric_name_expression_found={bool(name_expr)}")
print(f"default_prefix={default_prefix}")
print(f"default_metric_name={actual}")
print(f"catalogue_contains_default_metric={f'<td>{actual}</td>' in catalogue}")
print(f"catalogue_contains_canonical_candidate={f'<td>{default_prefix}_bmc_latency_milliseconds</td>' in catalogue}")
print(f"test_integration_scrapes_metrics={integration_scrape}")
print(f"health_bmc_tests_assert_current_name={health_test_name}")
print(f"health_endpoint_in_integration_addresses={health_endpoint}")
PY
printf '%s\n' '--- focused integration catalogue generation ---'
sed -n '260,330p' crates/api-integration-tests/tests/lib.rs
printf '%s\n' '--- focused health metric tests ---'
sed -n '2390,2470p' crates/health/src/bmc.rs
printf '%s\n' '--- example configuration ---'
sed -n '400,425p' crates/health/example/config.example.tomlRepository: NVIDIA/infra-controller
Length of output: 6797
Use _milliseconds and add catalogue coverage.
Rename {prefix}_bmc_latency_ms to {prefix}_bmc_latency_milliseconds. Add the health endpoint to test_integration coverage so the generated catalogue includes the metric and its HELP text. Update the existing test and example configuration references.
🤖 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/health/src/metrics.rs` around lines 127 - 134, Rename the metric
constructed in the latency HistogramVec to use the
`{prefix}_bmc_latency_milliseconds` suffix, then update all existing test and
example configuration references to the new name. Extend `test_integration` to
exercise the health endpoint so the generated metric catalogue includes this
metric and its HELP text.
Source: Coding guidelines
@Matthias247 nv-redfish by itself is transport-agnostic, you can replace BMC or HTTP client implementation, including to those that emits any metrics you want. To do this you can just create HTTP client by implementing the trait: And use it here: Instead of using |
That sounds like a good path! I'll try it |
|
@poroh I changed to that implementation in the second commit by adding This would either require rebuilding most of the client, or changing nv-redfish. I'd probably lean to the latter. And either
Options 2 to 4 all require changes to function signatures, and I'm not sure if the use-case is significant enough for other applications to justify. Option 5 is somewhat elegant - but adding custom attributes to the metric that are not visible to nvredfish is probably trickier. E.g. also showing But I personally can also live with the inaccuracy of the 2xx codes. It's probably more the question on what is most useful for other services which want to have the same metric. |
kensimon
left a comment
kensimon
left a comment
There was a problem hiding this comment.
Issues aren't really blocking, feel free to ignore.
I think at this point if we're going to default to per-machine/per-BMC metrics like we are, we may want to change the guidance in STYLE_GUIDE.md to no longer say that per-machine metrics are discouraged. I think the people using the health service find per-machine metrics useful enough that maybe we just declare that "the number of machines in an environment" no longer constitutes "high-cardinality"? Something to think about at least.
| Arc::new(FailingProvider), | ||
| None, | ||
| 10, | ||
| None, |
There was a problem hiding this comment.
Getting pretty close to needing a proper BmcClientArgs struct or something here, so that we can at least see name: value pairs... seeing 6 arguments with no names gets super hard to read after a while.
67bf8a8 to
3c1c167
Compare
|
@poroh @kensimon Pushed one more change which adds the additional labels I mostly see the use-case here as being able to differentiate whether the BMC implementation is slow for polling sensors or for other requests. |
I still think its a good best practice. In this case I made the set of labels configurable so that deployers can choose whether to have per-machine metrics or whether to roll up by certain parameters. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/health/src/api_client.rs (1)
605-627: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftCached clients keep the endpoint labels captured at first construction.
cache_or_create_bmc_clientinvokes the factory only on a cache miss, and the cache is keyed by MAC address. On a cache hit thebmc_latency_instrumentationcomputed at line 605 is dropped, and the cachedBmcClientcontinues to use themachine_idandrack_idvalues captured when it was first created.
endpoint_forrecomputesmetadataandrack_idfrom a fresh API response on every discovery cycle, so those values can legitimately change for a stable MAC. Two reachable cases:
- A machine is relocated and
machine.rack_idchanges. The histogram keeps publishing the previousrack_id.machine.idtransitions from absent to present after registration. The histogram keeps publishingmachine_id="unknown".
prune_bmc_client_cacheonly evicts MACs that disappeared, so neither case ever refreshes the labels. The result is silently incorrect identity dimensions with no error and no failing test. Static endpoints inendpoint/sources.rsare unaffected because they build a fresh client per configuration load.Consider holding the endpoint labels behind a shared cell that
endpoint_forupdates on every cycle, so the cached client observes the current identity.#!/bin/bash # Description: Confirm that cache_or_create_bmc_client skips the factory on a cache hit # and that nothing else refreshes a cached client's latency labels. set -eu printf '%s\n' '--- cache_or_create_bmc_client definition ---' ast-grep run --pattern 'fn cache_or_create_bmc_client($$$) { $$$ }' --lang rust crates/health printf '%s\n' '--- fallback text search if the pattern did not match ---' rg -n -C 20 'fn cache_or_create_bmc_client' crates/health printf '%s\n' '--- all call sites and cache eviction logic ---' rg -n -C 6 'cache_or_create_bmc_client|bmc_client_cache' crates/health🤖 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/health/src/api_client.rs` around lines 605 - 627, Update the cached BMC client flow around BmcLatencyInstrumentation::new and endpoint_for so endpoint identity labels are stored in shared mutable state rather than captured only during BmcClient construction. Refresh that state with the current machine_id and rack_id on every discovery cycle, and have the cached client read the latest values when publishing latency metrics while preserving cache reuse by MAC address.
🧹 Nitpick comments (8)
crates/health/src/config.rs (3)
1700-1702: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the precedence of the
allsentinel.When the list contains
all, this function discards every other entry and returns the complete attribute set. An operator writing["http_path", "all"]receives all ten labels, not one. The behaviour is reasonable, and the test at line 3196 pins it, but the field doc comment at line 1641 does not state it. Add a short note there so the resolution rule is discoverable from the configuration contract.As per coding guidelines: "Define and document create, update, patch, replacement, field-mask, clear, preserve, precedence, fallback, validation, and explicit-default semantics".
📝 Proposed documentation change
- /// Label attributes emitted on the BMC latency histogram. + /// Label attributes emitted on the BMC latency histogram. + /// + /// Accepts the snake_case label names plus the `all` sentinel. If the list + /// contains `all`, every supported attribute is selected and the remaining + /// entries are ignored. Unknown names are rejected at load time. The + /// resolved order always follows `BmcLatencyAttribute::ATTRIBUTES`. #[serde(🤖 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/health/src/config.rs` around lines 1700 - 1702, Update the field documentation near the configuration parsing logic to state that including the `all` sentinel takes precedence over every other listed attribute and selects the complete attribute set. Keep the existing `has_all` behavior in the surrounding function unchanged.Source: Coding guidelines
3169-3215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a rejection case for an unknown label.
The parser has a dedicated branch that collects unrecognised labels and reports them with the allowed-values list. No test reaches it. That branch is the only validation feedback an operator receives for a typo such as
machineid, and a silent regression there would turn a misspelled label into a hard-to-diagnose missing dimension.Add a case that parses an invalid label and asserts the extraction fails.
As per coding guidelines: "Prefer table-driven tests for functions mapping inputs to outputs or errors."
💚 Proposed test addition
#[test] fn bmc_latency_attributes_reject_unknown_labels() { let toml_content = r#" [metrics] bmc_latency_attributes = ["http_path", "machineid"] "#; let error = Figment::new() .merge(Serialized::defaults(Config::default())) .merge(Toml::string(toml_content)) .extract::<Config>() .expect_err("unknown BMC latency attribute must be rejected"); let message = error.to_string(); assert!(message.contains("machineid"), "unexpected error: {message}"); assert!(message.contains("machine_id"), "unexpected error: {message}"); }🤖 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/health/src/config.rs` around lines 3169 - 3215, Add a table-driven rejection test near bmc_latency_metrics_config_parsing that feeds unknown BMC latency labels such as “machineid” through Config extraction, asserts extraction fails, and verifies the error includes both the invalid label and an allowed label such as “machine_id”.Source: Coding guidelines
1722-1729: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the accessor so it cannot be confused with the field.
MetricsConfigexposes both the public fieldbmc_latency_attributesand the methodbmc_latency_attributes(). They differ only by parentheses and return different values whenever the field holds a non-canonical or duplicated list, which is reachable because the field is public andConfig::default()bypasses the custom deserializer. The assertions at lines 3149 to 3156 check both, which shows how close the two names sit. A caller that reads the field directly silently skips canonicalisation.Rename the method to
resolved_bmc_latency_attributesand update the call sites invalidateand in the wiring layer.As per coding guidelines: "design APIs that are hard to misuse".
♻️ Proposed rename
impl MetricsConfig { - pub fn bmc_latency_attributes(&self) -> Vec<BmcLatencyAttribute> { + pub fn resolved_bmc_latency_attributes(&self) -> Vec<BmcLatencyAttribute> { BmcLatencyAttribute::ATTRIBUTES .iter() .copied() .filter(|attribute| self.bmc_latency_attributes.contains(attribute)) .collect() } fn validate(&self) -> Result<(), String> { - if self.enable_bmc_latency_metrics && self.bmc_latency_attributes().is_empty() { + if self.enable_bmc_latency_metrics && self.resolved_bmc_latency_attributes().is_empty() {🤖 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/health/src/config.rs` around lines 1722 - 1729, Rename the MetricsConfig accessor bmc_latency_attributes() to resolved_bmc_latency_attributes() to distinguish it from the public field, preserving its canonicalizing behavior. Update every call site, specifically the validate logic and wiring layer, to use the renamed method while leaving direct field access unchanged.Source: Coding guidelines
crates/health/src/bmc.rs (4)
270-279: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
Anydowncast is contained, but consider a narrower hook.
note_bmc_identity_frominspects the runtime type of every decoded entity to findServiceRoot. The cost is oneTypeIdcomparison per successful read, which is negligible, and the function is small and clearly named. The trade-off is that the identity capture is invisible from theServiceRoottype itself: a future rename or a second identity-bearing schema type will not produce a compile error here, only a silent loss of thebmc_vendorandbmc_modellabels.If a follow-up adds more identity sources, prefer a small local trait with an explicit implementation over widening this downcast chain.
As per coding guidelines: "Prefer simple, explicit, readable, maintainable Rust over cleverness or unnecessary abstraction".
🤖 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/health/src/bmc.rs` around lines 270 - 279, Keep note_bmc_identity_from focused on the existing ServiceRoot identity source; no immediate change is required. If adding additional identity-bearing schema types later, replace the expanding Any downcast chain with a small local trait and explicit implementations for each supported type, preserving the existing vendor and product label capture.Source: Coding guidelines
224-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider a parameter struct for the six-argument constructor.
BmcClient::newnow takes six positional parameters, two of which areOptionvalues with unrelated meanings:proxy_urlandbmc_latency_instrumentation. Call sites readBmcClient::new(reqwest(), test_addr(), provider, None, 10, None). Transposing the twoNonearguments compiles without error and silently disables either the proxy or the instrumentation. The bare10forcache_sizehas the same weakness.A
BmcClientParamsstruct built with a struct literal would make each value self-describing at every call site and would absorb future optional parameters without another signature change. The change touches roughly twenty-five call sites in this file, so it is a deliberate follow-up rather than a blocker.As per coding guidelines: "design APIs that are hard to misuse" and "builders only when they improve large optional initialization without weakening required-field checks".
🤖 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/health/src/bmc.rs` around lines 224 - 243, Introduce a BmcClientParams parameter struct containing the current BmcClient::new arguments, including explicitly named proxy_url, cache_size, and bmc_latency_instrumentation fields; change BmcClient::new to accept this struct while keeping required dependencies validated through the struct literal. Update all call sites in the file to construct BmcClientParams with named fields, preserving each existing value and behavior.Source: Coding guidelines
796-814: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the estimated-success-code caveat in the code.
The success status codes are estimates.
getandssereport200,post_sessionreports201, andpostselects between200and201from the URL path. The underlying client does not expose the observed 2xx code, so a204or206response is published underhttp_response_status_code="200". Error codes are accurate.The pull request description explains this, but an operator reading a dashboard will see only the label. Add a short doc comment on the trait implementation so the constraint travels with the code.
📝 Proposed documentation change
+/// Times every Redfish request and records the result on the BMC latency +/// histogram. +/// +/// The underlying client does not surface the observed 2xx status code, so the +/// success codes below are estimates: reads report `200` and creations report +/// `201`. Task and empty responses are distinguished, and error status codes +/// are taken from the transport error and are exact. impl HttpClient for InstrumentedHttpClient { type Error = BmcError;🤖 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/health/src/bmc.rs` around lines 796 - 814, Add a short documentation comment on the trait implementation containing get, sse, post_session, and post to state that success status codes are estimated because the underlying client does not expose the observed 2xx code, while error codes remain accurate; mention the current reported values and post’s URL-based 200/201 selection.
2679-2715: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit coverage for the pure status-selection helpers.
The three metric tests exercise the wire path well, including the 503 error case and the restricted attribute set. Two new branches remain uncovered:
post_entity_status_codereturns200for a path containing/Actions/and201otherwise. No test reaches either arm. This branch encodes a Redfish URI convention and would regress silently.bmc_error_status_codereturnsNonefor every error variant other thanInvalidResponse, so transport failures fall back to the unknown bucket. No test pins that fallback.Both are pure functions, so table-driven tests need no scripted server.
As per coding guidelines: "Prefer table-driven tests for functions mapping inputs to outputs or errors. Use scenarios! with Outcome for Result operations, value_scenarios! for plain values".
💚 Proposed test addition
#[test] fn post_entity_status_code_distinguishes_actions_from_creation() { value_scenarios!(run = |path: &str| { let url = Url::parse(&format!("https://10.0.0.1{path}")).expect("valid url"); post_entity_status_code(&url) }; "action invocations report 200" { "/redfish/v1/Systems/1/Actions/ComputerSystem.Reset" => "200", } "resource creation reports 201" { "/redfish/v1/SessionService/Sessions" => "201", } ); } #[test] fn transport_errors_fall_back_to_the_unknown_status_bucket() { let result: Result<(), BmcError> = Err(bmc_status_error(http::StatusCode::SERVICE_UNAVAILABLE)); assert_eq!(result_status_code(&result, "200"), "503"); let decode_failure: Result<(), BmcError> = Err(BmcError::InvalidResponse { url: Url::parse("https://10.0.0.1/redfish/v1").expect("valid url"), status: http::StatusCode::BAD_GATEWAY, text: String::new(), }); assert_eq!(result_status_code(&decode_failure, "200"), "502"); }🤖 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/health/src/bmc.rs` around lines 2679 - 2715, Add table-driven unit tests for the pure status-selection helpers: cover both `/Actions/` and non-action paths in `post_entity_status_code`, and verify `bmc_error_status_code`/`result_status_code` maps non-`InvalidResponse` transport errors to the unknown bucket while preserving the HTTP status for `InvalidResponse`. Use the project’s `value_scenarios!` or `scenarios!` conventions without adding scripted-server setup.Source: Coding guidelines
crates/health/src/metrics.rs (1)
87-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
DisplayandFromStrinstead of the bespoke label conversions.
label_nameandfrom_label_nameimplement a string mapping that the standard traits already model.DisplayplusFromStrwould letconfig.rsparse throughstr::parseand would compose withserdehelpers. The current linear scan infrom_label_nameis acceptable at ten variants and only runs at configuration load.As per coding guidelines: "Model finite possibilities with enums or typed structures implementing Display and FromStr".
🤖 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/health/src/metrics.rs` around lines 87 - 107, The metric attribute enum’s bespoke label conversion methods should use standard string-conversion traits instead. Implement Display for the enum using the existing label mappings, implement FromStr with the current matching behavior and appropriate parse error type, then update callers such as config parsing to use str::parse and adjust serde integration to reuse these traits; remove or replace label_name and from_label_name while preserving all mappings.Source: Coding guidelines
🤖 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/health/src/bmc.rs`:
- Around line 1025-1032: Replace the compiler-derived entity_type_name<T>
implementation with stable Redfish identifiers, using an explicit
type-to-identifier mapping for the supported entity types. Update callers to use
that mapping and ensure metric labels remain unchanged across Rust toolchain
upgrades.
---
Outside diff comments:
In `@crates/health/src/api_client.rs`:
- Around line 605-627: Update the cached BMC client flow around
BmcLatencyInstrumentation::new and endpoint_for so endpoint identity labels are
stored in shared mutable state rather than captured only during BmcClient
construction. Refresh that state with the current machine_id and rack_id on
every discovery cycle, and have the cached client read the latest values when
publishing latency metrics while preserving cache reuse by MAC address.
---
Nitpick comments:
In `@crates/health/src/bmc.rs`:
- Around line 270-279: Keep note_bmc_identity_from focused on the existing
ServiceRoot identity source; no immediate change is required. If adding
additional identity-bearing schema types later, replace the expanding Any
downcast chain with a small local trait and explicit implementations for each
supported type, preserving the existing vendor and product label capture.
- Around line 224-243: Introduce a BmcClientParams parameter struct containing
the current BmcClient::new arguments, including explicitly named proxy_url,
cache_size, and bmc_latency_instrumentation fields; change BmcClient::new to
accept this struct while keeping required dependencies validated through the
struct literal. Update all call sites in the file to construct BmcClientParams
with named fields, preserving each existing value and behavior.
- Around line 796-814: Add a short documentation comment on the trait
implementation containing get, sse, post_session, and post to state that success
status codes are estimated because the underlying client does not expose the
observed 2xx code, while error codes remain accurate; mention the current
reported values and post’s URL-based 200/201 selection.
- Around line 2679-2715: Add table-driven unit tests for the pure
status-selection helpers: cover both `/Actions/` and non-action paths in
`post_entity_status_code`, and verify
`bmc_error_status_code`/`result_status_code` maps non-`InvalidResponse`
transport errors to the unknown bucket while preserving the HTTP status for
`InvalidResponse`. Use the project’s `value_scenarios!` or `scenarios!`
conventions without adding scripted-server setup.
In `@crates/health/src/config.rs`:
- Around line 1700-1702: Update the field documentation near the configuration
parsing logic to state that including the `all` sentinel takes precedence over
every other listed attribute and selects the complete attribute set. Keep the
existing `has_all` behavior in the surrounding function unchanged.
- Around line 3169-3215: Add a table-driven rejection test near
bmc_latency_metrics_config_parsing that feeds unknown BMC latency labels such as
“machineid” through Config extraction, asserts extraction fails, and verifies
the error includes both the invalid label and an allowed label such as
“machine_id”.
- Around line 1722-1729: Rename the MetricsConfig accessor
bmc_latency_attributes() to resolved_bmc_latency_attributes() to distinguish it
from the public field, preserving its canonicalizing behavior. Update every call
site, specifically the validate logic and wiring layer, to use the renamed
method while leaving direct field access unchanged.
In `@crates/health/src/metrics.rs`:
- Around line 87-107: The metric attribute enum’s bespoke label conversion
methods should use standard string-conversion traits instead. Implement Display
for the enum using the existing label mappings, implement FromStr with the
current matching behavior and appropriate parse error type, then update callers
such as config parsing to use str::parse and adjust serde integration to reuse
these traits; remove or replace label_name and from_label_name while preserving
all mappings.
🪄 Autofix
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: abd12f8e-6dcb-4cba-a157-3cb3a28198cc
📒 Files selected for processing (7)
crates/health/example/config.example.tomlcrates/health/src/api_client.rscrates/health/src/bmc.rscrates/health/src/config.rscrates/health/src/endpoint/cluster.rscrates/health/src/endpoint/sources.rscrates/health/src/metrics.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/health/example/config.example.toml
- crates/health/src/endpoint/cluster.rs
| fn entity_type_name<T>() -> &'static str { | ||
| let type_name = type_name::<T>(); | ||
| let without_generics = type_name.split('<').next().unwrap_or(type_name); | ||
| without_generics | ||
| .rsplit("::") | ||
| .next() | ||
| .unwrap_or(without_generics) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does the Rust standard library guarantee stability of std::any::type_name output across compiler versions?
💡 Result:
No, the Rust standard library does not guarantee the stability of the output from std::any::type_name across compiler versions [1][2]. The official documentation explicitly states that the exact contents and format of the string returned are not specified, serving only as a best-effort description of the type [1][3]. Because the implementation is intended strictly for diagnostic purposes, the output is subject to change between different versions of the compiler [1][4]. For example, the inclusion of certain details, such as lifetime specifiers, has changed in past compiler releases [1][5]. Furthermore, the documentation warns against using the output of std::any::type_name as a unique identifier for a type or to affect program behavior, as it may not be consistent or unique [1][3]. Users are encouraged to rely on it only for debugging and logging [1][3].
Citations:
- 1: https://doc.rust-lang.org/stable/std/any/fn.type_name.html
- 2: https://doc.rust-lang.org/stable/core/any/fn.type_name.html
- 3: https://doc.rust-lang.org/nightly/src/core/any.rs.html
- 4: https://doc.rust-lang.org/stable/src/core/any.rs.html
- 5: https://github.com/rust-lang/rust/blob/eeff92ad32c2627876112ccfe812e19d38494087/library/core/src/any.rs
🏁 Script executed:
#!/bin/bash
set -eu
file=$(git ls-files | rg '^crates/health/src/bmc\.rs$')
wc -l "$file"
sed -n '1,140p' "$file"
sed -n '980,1065p' "$file"
rg -n -C 4 'entity_type_name|entity_type|odata\.type|OData|type_name' "$file" crates/healthRepository: NVIDIA/infra-controller
Length of output: 1935
🌐 Web query:
site:github.com/NVIDIA/infra-controller crates/health/src/bmc.rs entity_type_name
💡 Result:
In the context of the NVIDIA infra-controller repository, crates/health/src/bmc.rs is a source file associated with the health monitoring subsystem [1][2]. The identifier entity_type_name is commonly used within this codebase as a trait method or field associated with BMC (Baseboard Management Controller) and other hardware entities to provide a human-readable or system-identifiable name for the entity type (e.g., distinguishing between different hardware classes like Switches, DPUs, or Servers) [1][3]. While the exact implementation of entity_type_name in crates/health/src/bmc.rs may evolve with the codebase, it is part of the broader logic used for hardware discovery, BMC credential management, and Redfish-based monitoring [1][4]. This identifier facilitates the categorization of hardware nodes within the controller, allowing the system to apply appropriate collectors (e.g., leak detectors, power state management, or firmware inventory) based on the specific hardware type identified [1][2][5].
Citations:
- 1: bug(health): Redfish SSE applies to Switch Hosts #1744
- 2: bug: Since deploying version v0.8.4-0-gcd75a802, large uplift in HTTP 500 from Bluefield 2 DPUs #2066
- 3: Switches should link to their BMC via machine_interfaces #1722
- 4: feat: support per-model BMC factory default credentials for DPUs #3774
- 5: bd64311
Use a stable Redfish identifier for the entity_type label.
std::any::type_name does not guarantee stable output across compiler versions. Deriving a published metric label from it can break dashboards and recording rules after a toolchain upgrade. Use a stable Redfish identifier or an explicit mapping instead.
🤖 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/health/src/bmc.rs` around lines 1025 - 1032, Replace the
compiler-derived entity_type_name<T> implementation with stable Redfish
identifiers, using an explicit type-to-identifier mapping for the supported
entity types. Update callers to use that mapping and ensure metric labels remain
unchanged across Rust toolchain upgrades.
Adds new config file flags to hw-health which make it emit a histogram metric for requests to BMCs.
The metric with all properties attached will look like:
```
carbide_hardware_health_bmc_latency_ms_bucket{http_response_status_code="200",http_request_method="GET",http_path="/redfish/
v1",server_address="1.3.5.12",url_scheme="https",bmc_vendor="NVIDIA",bmc_model="GB200 BMC",le="100"} 1
```
The metric can be explicitly enabled via setting
```toml
[metrics]
enable_bmc_latency_metrics = true
```
By default it will contain all available fields. If only a subset of fields should be emitted to reduce the amount of time series, the setting `metrics.bmc_latency_attributes` can be used to explicitly specify the attributes that should be included. Adding `all` to the list leads to emitting all attributes (default) again.
```toml
[metrics]
bmc_latency_attributes = ["http_response_status_code", "server_address", "url_scheme"]
```
Limitation:
The status code for success responses are estimated since nv-redfish does not expose it. It is always set to 200 for GET requests and 201 for creation requests. Status codes for error responses are accurate.
3c1c167 to
a209227
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4720.docs.buildwithfern.com/infra-controller |
Wraps the nvredfish HttpClient and emits metrics in it. Signed-off-by: Matthias Einwag <meinwag@nvidia.com>
a209227 to
2ad4c49
Compare
5 participants
Adds new config file flags to hw-health which make it emit a histogram metric for requests to BMCs. The metric with all properties attached will look like:
The metric can be explicitly enabled via setting
By default it will contain all available fields. If only a subset of fields should be emitted to reduce the amount of time series, the setting
metrics.bmc_latency_attributescan be used to explicitly specify the attributes that should be included. Addingallto the list leads to emitting all attributes (default) again.Limitation:
The status code for success responses are estimated since nv-redfish does not expose it. It is always set to 200 for GET requests and 201 for creation requests. Status codes for error responses are accurate.
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes