feat(health): Emit latency and health metrics for BMC interactions - #4720
Open
Matthias247 wants to merge 2 commits into
Open
feat(health): Emit latency and health metrics for BMC interactions#4720Matthias247 wants to merge 2 commits into
Matthias247 wants to merge 2 commits into
Conversation
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.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
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}
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/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
Comment on lines
+790
to
+802
| 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()) | ||
| }) | ||
| } |
Contributor
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.
Comment on lines
+58
to
+94
| #[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", | ||
| } | ||
| } | ||
| } |
Contributor
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
Comment on lines
+113
to
+162
| impl BmcLatencyMetrics { | ||
| pub fn new(registry: &Registry, prefix: &str) -> Result<Self, prometheus::Error> { | ||
| Self::new_with_attributes(registry, prefix, &BmcLatencyAttribute::ATTRIBUTES) | ||
| } | ||
|
|
||
| pub fn new_with_attributes( | ||
| registry: &Registry, | ||
| prefix: &str, | ||
| attributes: &[BmcLatencyAttribute], | ||
| ) -> Result<Self, prometheus::Error> { | ||
| let label_names = attributes | ||
| .iter() | ||
| .map(|attribute| attribute.label_name()) | ||
| .collect::<Vec<_>>(); | ||
| 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, | ||
| )?; | ||
| registry.register(Box::new(latency_ms.clone()))?; | ||
|
|
||
| Ok(Self { | ||
| latency_ms, | ||
| attributes: attributes.to_vec(), | ||
| }) | ||
| } | ||
|
|
||
| pub fn observe(&self, observation: BmcLatencyObservation<'_>) { | ||
| let labels = self | ||
| .attributes | ||
| .iter() | ||
| .map(|attribute| match attribute { | ||
| BmcLatencyAttribute::All => unreachable!("all is not a concrete metric label"), | ||
| BmcLatencyAttribute::HttpResponseStatusCode => observation.status_code, | ||
| BmcLatencyAttribute::HttpRequestMethod => observation.method, | ||
| BmcLatencyAttribute::HttpPath => observation.path, | ||
| BmcLatencyAttribute::ServerAddress => observation.server_address, | ||
| BmcLatencyAttribute::UrlScheme => observation.url_scheme, | ||
| BmcLatencyAttribute::BmcVendor => observation.bmc_vendor.unwrap_or("unknown"), | ||
| BmcLatencyAttribute::BmcModel => observation.bmc_model.unwrap_or("unknown"), | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
| self.latency_ms | ||
| .with_label_values(&labels) | ||
| .observe(observation.duration.as_secs_f64() * 1_000.0); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard against All reaching observe() as a concrete label.
new_with_attributes is pub and stores attributes without filtering BmcLatencyAttribute::All. Every current caller (config.rs::bmc_latency_attributes(), the test helpers in bmc.rs) filters All out first, but nothing in new_with_attributes itself enforces that. A caller who passes &[BmcLatencyAttribute::All] directly builds a histogram with a label literally named "all", and every call to observe() then panics at unreachable!("all is not a concrete metric label").
Filter All out of attributes inside new_with_attributes, or return an error when it is present, so this public constructor cannot be misused into a runtime panic.
🔒️ Proposed fix to make `new_with_attributes` reject/filter `All`
pub fn new_with_attributes(
registry: &Registry,
prefix: &str,
attributes: &[BmcLatencyAttribute],
) -> Result<Self, prometheus::Error> {
- let label_names = attributes
+ let attributes: Vec<BmcLatencyAttribute> = attributes
.iter()
+ .copied()
+ .filter(|attribute| *attribute != BmcLatencyAttribute::All)
+ .collect();
+ let label_names = attributes
+ .iter()
.map(|attribute| attribute.label_name())
.collect::<Vec<_>>();
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,
)?;
registry.register(Box::new(latency_ms.clone()))?;
Ok(Self {
latency_ms,
- attributes: attributes.to_vec(),
+ attributes,
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| impl BmcLatencyMetrics { | |
| pub fn new(registry: &Registry, prefix: &str) -> Result<Self, prometheus::Error> { | |
| Self::new_with_attributes(registry, prefix, &BmcLatencyAttribute::ATTRIBUTES) | |
| } | |
| pub fn new_with_attributes( | |
| registry: &Registry, | |
| prefix: &str, | |
| attributes: &[BmcLatencyAttribute], | |
| ) -> Result<Self, prometheus::Error> { | |
| let label_names = attributes | |
| .iter() | |
| .map(|attribute| attribute.label_name()) | |
| .collect::<Vec<_>>(); | |
| 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, | |
| )?; | |
| registry.register(Box::new(latency_ms.clone()))?; | |
| Ok(Self { | |
| latency_ms, | |
| attributes: attributes.to_vec(), | |
| }) | |
| } | |
| pub fn observe(&self, observation: BmcLatencyObservation<'_>) { | |
| let labels = self | |
| .attributes | |
| .iter() | |
| .map(|attribute| match attribute { | |
| BmcLatencyAttribute::All => unreachable!("all is not a concrete metric label"), | |
| BmcLatencyAttribute::HttpResponseStatusCode => observation.status_code, | |
| BmcLatencyAttribute::HttpRequestMethod => observation.method, | |
| BmcLatencyAttribute::HttpPath => observation.path, | |
| BmcLatencyAttribute::ServerAddress => observation.server_address, | |
| BmcLatencyAttribute::UrlScheme => observation.url_scheme, | |
| BmcLatencyAttribute::BmcVendor => observation.bmc_vendor.unwrap_or("unknown"), | |
| BmcLatencyAttribute::BmcModel => observation.bmc_model.unwrap_or("unknown"), | |
| }) | |
| .collect::<Vec<_>>(); | |
| self.latency_ms | |
| .with_label_values(&labels) | |
| .observe(observation.duration.as_secs_f64() * 1_000.0); | |
| } | |
| } | |
| impl BmcLatencyMetrics { | |
| pub fn new(registry: &Registry, prefix: &str) -> Result<Self, prometheus::Error> { | |
| Self::new_with_attributes(registry, prefix, &BmcLatencyAttribute::ATTRIBUTES) | |
| } | |
| pub fn new_with_attributes( | |
| registry: &Registry, | |
| prefix: &str, | |
| attributes: &[BmcLatencyAttribute], | |
| ) -> Result<Self, prometheus::Error> { | |
| let attributes: Vec<BmcLatencyAttribute> = attributes | |
| .iter() | |
| .copied() | |
| .filter(|attribute| *attribute != BmcLatencyAttribute::All) | |
| .collect(); | |
| let label_names = attributes | |
| .iter() | |
| .map(|attribute| attribute.label_name()) | |
| .collect::<Vec<_>>(); | |
| 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, | |
| )?; | |
| registry.register(Box::new(latency_ms.clone()))?; | |
| Ok(Self { | |
| latency_ms, | |
| attributes, | |
| }) | |
| } | |
| pub fn observe(&self, observation: BmcLatencyObservation<'_>) { | |
| let labels = self | |
| .attributes | |
| .iter() | |
| .map(|attribute| match attribute { | |
| BmcLatencyAttribute::All => unreachable!("all is not a concrete metric label"), | |
| BmcLatencyAttribute::HttpResponseStatusCode => observation.status_code, | |
| BmcLatencyAttribute::HttpRequestMethod => observation.method, | |
| BmcLatencyAttribute::HttpPath => observation.path, | |
| BmcLatencyAttribute::ServerAddress => observation.server_address, | |
| BmcLatencyAttribute::UrlScheme => observation.url_scheme, | |
| BmcLatencyAttribute::BmcVendor => observation.bmc_vendor.unwrap_or("unknown"), | |
| BmcLatencyAttribute::BmcModel => observation.bmc_model.unwrap_or("unknown"), | |
| }) | |
| .collect::<Vec<_>>(); | |
| self.latency_ms | |
| .with_label_values(&labels) | |
| .observe(observation.duration.as_secs_f64() * 1_000.0); | |
| } | |
| } |
🤖 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 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.
Source: Coding guidelines
Comment on lines
+127
to
+134
| 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, | ||
| )?; |
Contributor
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
Contributor
@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 |
Contributor
Author
That sounds like a good path! I'll try it |
Wraps the nvredfish HttpClient and emits metrics in it. Signed-off-by: Matthias Einwag <meinwag@nvidia.com>
Contributor
Author
|
@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
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. |
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
4 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.
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