feat: measure every Redfish operation, count every IPMI command#3408
feat: measure every Redfish operation, count every IPMI command#3408chet wants to merge 1 commit into
Conversation
|
@coderabbitai full_review, thanks! |
|
ヽ(•‿•)ノ ✅ Action performedFull review finished. |
Summary by CodeRabbit
WalkthroughThe change adds a shared RED recording helper, instruments Redfish operations and client creation, and introduces completed-command counters for IPMI reset executions in tool and HTTP mock implementations. ChangesOutbound observability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RedfishCaller
participant InstrumentedRedfish
participant RedfishClient
participant REDHistogram
participant Logger
RedfishCaller->>InstrumentedRedfish: invoke operation
InstrumentedRedfish->>RedfishClient: delegate operation
RedfishClient-->>InstrumentedRedfish: return result
InstrumentedRedfish->>REDHistogram: record duration and outcome
InstrumentedRedfish->>Logger: WARN on genuine failure
InstrumentedRedfish-->>RedfishCaller: return original result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3408.docs.buildwithfern.com/infra-controller |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/redfish/src/libredfish/implementation.rs (1)
121-158: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the pool-level instrumentation contract.
The added tests construct
InstrumentedRedfishdirectly, so they cannot catch removal of thecreate_clientmetric at Line 125 or the pool decorator return at Line 158. Add a focusedRedfishClientPoolImpl::create_clienttest that asserts initialization and a subsequent operation are metered.As per coding guidelines, “Add or update focused tests for ... cross-module changes.”
🤖 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/redfish/src/libredfish/implementation.rs` around lines 121 - 158, Add a focused test for RedfishClientPoolImpl::create_client that uses the pool API rather than constructing InstrumentedRedfish directly, then verifies both the create_client initialization metric and a subsequent Redfish operation metric. Keep the test targeted to the pool-level instrumentation contract and cover the returned decorated client.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/ipmi/src/bmc_mock.rs`:
- Around line 98-147: Exclude HTTP client construction errors from the command
counter: restructure the async flow around reqwest::Client::builder().build() so
a build failure returns immediately without reaching count_ipmi_command(command,
&result), while failures after request.send() continue to be counted and
returned through the existing result path.
In `@crates/redfish/src/libredfish/instrumented.rs`:
- Around line 378-390: The change_uefi_password error handling must redact both
passwords from the original error text, rather than chaining redact_nonempty so
the first replacement can prevent matching the second or leave fragments
exposed. Update the redaction flow to find and redact all matching ranges
against the unmodified error, preserving the existing error result, and add an
overlapping-password regression case covering current “foobar” and new “foo”.
---
Nitpick comments:
In `@crates/redfish/src/libredfish/implementation.rs`:
- Around line 121-158: Add a focused test for
RedfishClientPoolImpl::create_client that uses the pool API rather than
constructing InstrumentedRedfish directly, then verifies both the create_client
initialization metric and a subsequent Redfish operation metric. Keep the test
targeted to the pool-level instrumentation contract and cover the returned
decorated client.
🪄 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: ae8a3d91-6631-4824-9ba5-7e624ca98707
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
crates/instrument/src/red.rscrates/ipmi/Cargo.tomlcrates/ipmi/src/bmc_mock.rscrates/ipmi/src/lib.rscrates/ipmi/src/metrics.rscrates/ipmi/src/tool.rscrates/redfish/Cargo.tomlcrates/redfish/src/libredfish/implementation.rscrates/redfish/src/libredfish/instrumented.rscrates/redfish/src/libredfish/mod.rsdocs/observability/core_metrics.md
There was a problem hiding this comment.
♻️ Duplicate comments (2)
crates/redfish/src/libredfish/instrumented.rs (1)
378-390:⚠️ Potential issue | 🟠 MajorSequential password redaction can still leak a fragment; still unresolved from a prior review.
change_uefi_passwordchainsredact_nonempty(error, new_uefi_password)thenredact_nonempty(error, current_uefi_password). If one password is a substring of the other (e.g. current"foobar", new"foo"), redactingnew_uefi_passwordfirst mutates the message so the originalcurrent_uefi_passwordsubstring no longer exists intact, so the second call finds nothing to redact and part of the password (e.g."bar") survives into the WARN log. The added test at Lines 550-569 still only exercises the single-password path — the overlapping-password regression case requested previously is still missing.🔒 Proposed fix
+ /// Redacts both UEFI passwords, longest first, so a shorter password + /// that is a substring of the longer one (e.g. current "foobar", new + /// "foo") cannot fragment the longer password's redaction. + fn redact_both_nonempty(error: RedfishError, a: &str, b: &str) -> RedfishError { + let (longer, shorter) = if a.len() >= b.len() { (a, b) } else { (b, a) }; + redact_nonempty(redact_nonempty(error, longer), shorter) + } + fn change_uefi_password<'a>( &'a self, current_uefi_password: &'a str, new_uefi_password: &'a str, ) -> RedfishFuture<'a, Result<Option<String>, RedfishError>> { Box::pin(instrumented_redfish("change_uefi_password", async move { self.inner .change_uefi_password(current_uefi_password, new_uefi_password) .await - .map_err(|error| redact_nonempty(error, new_uefi_password)) - .map_err(|error| redact_nonempty(error, current_uefi_password)) + .map_err(|error| redact_both_nonempty(error, current_uefi_password, new_uefi_password)) })) }As per path instructions, prefer a table-driven case (
current/new/expected) for the overlapping scenario rather than a single hand-rolled assertion. Based on learnings, please add a scenario with current"foobar"and new"foo"to cover the fix.Also applies to: 550-569
🤖 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/redfish/src/libredfish/instrumented.rs` around lines 378 - 390, Update change_uefi_password to redact current_uefi_password and new_uefi_password safely in one operation, preventing overlapping or substring passwords from leaving fragments in the error message. Extend the existing redaction tests with a table-driven current/new/expected case covering current "foobar" and new "foo", and verify neither password nor any remaining fragment is exposed.Source: Path instructions
crates/ipmi/src/bmc_mock.rs (1)
98-147:⚠️ Potential issue | 🟡 MinorClient-construction failures are still counted; move
Client::builder().build()out of the metered block.This is the same issue flagged on a previous commit:
reqwest::Client::builder().build()can fail before any request is dispatched, but that failure is still captured byresultand passed tocount_ipmi_commandat Line 146, contradicting the "dispatched command only" contract documented at Lines 98-100.🐛 Proposed fix
- let result = async { - let client = reqwest::Client::builder() - .danger_accept_invalid_certs(true) - .build() - .map_err(|e| eyre!("failed to create HTTP client: {}", e))?; + let client = reqwest::Client::builder() + .danger_accept_invalid_certs(true) + .build() + .map_err(|e| eyre!("failed to create HTTP client: {}", e))?; + + let result = async { let mut request = client .post(&url) .basic_auth(username, Some(password)) .json(&serde_json::json!({"action": action}));🤖 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/ipmi/src/bmc_mock.rs` around lines 98 - 147, Move reqwest client construction in the command-handling flow outside the async block assigned to result, handling any build failure before the metered dispatch section. Ensure only failures after a request is attempted are passed to count_ipmi_command, while preserving the existing HTTP request and response handling.
🤖 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.
Duplicate comments:
In `@crates/ipmi/src/bmc_mock.rs`:
- Around line 98-147: Move reqwest client construction in the command-handling
flow outside the async block assigned to result, handling any build failure
before the metered dispatch section. Ensure only failures after a request is
attempted are passed to count_ipmi_command, while preserving the existing HTTP
request and response handling.
In `@crates/redfish/src/libredfish/instrumented.rs`:
- Around line 378-390: Update change_uefi_password to redact
current_uefi_password and new_uefi_password safely in one operation, preventing
overlapping or substring passwords from leaving fragments in the error message.
Extend the existing redaction tests with a table-driven current/new/expected
case covering current "foobar" and new "foo", and verify neither password nor
any remaining fragment is exposed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4ab422cc-5858-4815-8eac-2c74e4f41b42
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
crates/instrument/src/red.rscrates/ipmi/Cargo.tomlcrates/ipmi/src/bmc_mock.rscrates/ipmi/src/lib.rscrates/ipmi/src/metrics.rscrates/ipmi/src/tool.rscrates/redfish/Cargo.tomlcrates/redfish/src/libredfish/implementation.rscrates/redfish/src/libredfish/instrumented.rscrates/redfish/src/libredfish/mod.rsdocs/observability/core_metrics.md
chet
force-pushed
the
gh-issue-3174-redfish
branch
from
9ac85b5 to
17f58d1
Compare
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
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/redfish/src/libredfish/instrumented.rs`:
- Around line 98-105: Update redact_both to perform single-pass redaction over
the original error text, masking the union of matches for both passwords so
partial overlaps cannot leak after the first replacement. Preserve handling of
empty passwords and add a regression case covering overlapping values such as
“abcdef” and “defghi” in “abcdefghi”, verifying the warning contains no password
fragment.
- Around line 429-494: Update the sim_client test helper to return the
InstrumentedRedfish produced directly by sim.create_client(...), removing the
additional InstrumentedRedfish::new wrapper. Preserve the existing client
creation arguments and test behavior so each operation records a single
histogram observation.
🪄 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: 405792b5-e384-4263-a742-137fbed480a5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
crates/instrument/src/red.rscrates/ipmi/Cargo.tomlcrates/ipmi/src/bmc_mock.rscrates/ipmi/src/lib.rscrates/ipmi/src/metrics.rscrates/ipmi/src/tool.rscrates/redfish/Cargo.tomlcrates/redfish/src/libredfish/implementation.rscrates/redfish/src/libredfish/instrumented.rscrates/redfish/src/libredfish/mod.rsdocs/observability/core_metrics.md
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
chet
force-pushed
the
gh-issue-3174-redfish
branch
from
17f58d1 to
48e516d
Compare
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/redfish/src/libredfish/mod.rs`:
- Around line 461-489: Update mask_all’s range discovery to use an overlap-aware
scan for each non-empty needle, so self-overlapping matches such as “aa” in
“xaaay” produce every matching range before sorting and merging. Preserve the
existing range-merging and REDACTED output behavior.
🪄 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: 60cdd9f9-4991-481d-aefb-fa7310770f19
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
crates/instrument/src/red.rscrates/ipmi/Cargo.tomlcrates/ipmi/src/bmc_mock.rscrates/ipmi/src/lib.rscrates/ipmi/src/metrics.rscrates/ipmi/src/tool.rscrates/redfish/Cargo.tomlcrates/redfish/src/libredfish/implementation.rscrates/redfish/src/libredfish/instrumented.rscrates/redfish/src/libredfish/mod.rsdocs/observability/core_metrics.md
✅ Files skipped from review due to trivial changes (1)
- docs/observability/core_metrics.md
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/ipmi/Cargo.toml
- crates/ipmi/src/lib.rs
- crates/instrument/src/red.rs
- crates/ipmi/src/metrics.rs
- crates/ipmi/src/bmc_mock.rs
- crates/ipmi/src/tool.rs
- crates/redfish/Cargo.toml
- crates/redfish/src/libredfish/implementation.rs
- crates/redfish/src/libredfish/instrumented.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/redfish/src/libredfish/mod.rs (1)
461-489: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winSelf-overlapping password matches still leak a fragment.
text.match_indices(needle)only returns non-overlapping matches, so a needle that overlaps itself in the text (e.g. password"aa"inside"xaaay", or"ana"inside"banana") only yields one range instead of every overlapping occurrence.mask_allthen redacts"xaaay"as"xREDACTEDay"— the trailing character survives, which defeats the purpose of this redaction path (these strings otherwise end up in atracing::warn!log line viaredact_nonempty/redact_bothininstrumented.rs). This is the exact issue raised on a previous revision of this hunk that has not yet been addressed.Scan for overlapping occurrences by advancing one character (not byte, to stay UTF-8 safe) after each match instead of using
match_indices.🔒 Proposed fix for overlap-aware matching
let mut ranges: Vec<(usize, usize)> = needles .iter() .filter(|needle| !needle.is_empty()) .flat_map(|needle| { - text.match_indices(needle) - .map(|(start, matched)| (start, start + matched.len())) + // `str::match_indices` skips overlapping matches of the *same* + // needle (e.g. only one "aa" inside "aaa"), which would leave a + // fragment of a repeated-character password unredacted below. + let mut start = 0; + std::iter::from_fn(move || { + let pos = text[start..].find(needle)?; + let match_start = start + pos; + let match_end = match_start + needle.len(); + // Advance by one character, not one byte, to stay on a + // char boundary while still finding overlapping matches. + start = match_start + + text[match_start..] + .chars() + .next() + .map_or(1, |c| c.len_utf8()); + Some((match_start, match_end)) + }) }) .collect();Add a regression case such as
mask_all("xaaay", &["aa"])(or aredact_nonempty("boom aaa", "aa")case) asserting no fragment survives.🤖 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/redfish/src/libredfish/mod.rs` around lines 461 - 489, Update mask_all’s range collection to detect overlapping needle occurrences by searching from each match’s start advanced by one UTF-8 character, rather than relying on text.match_indices. Preserve byte-based range slicing while ensuring cases such as “aa” in “xaaay” and “ana” in “banana” fully redact every overlapping match; add a regression test for the leakage case if tests are present.
🧹 Nitpick comments (1)
crates/redfish/src/libredfish/instrumented.rs (1)
558-613: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a table-driven form for the redaction tests.
redact_both_survives_contained_passwords,redact_both_survives_partially_overlapping_passwords, andredact_nonempty_redacts_only_a_nonempty_passwordall exercise the same "input text + needle(s) → expected redacted text" shape and would consolidate well into one table viacheck_values(a macro would obscure the multi-input rows, so usecheck_valuesdirectly per the row-with-several-inputs guidance). This would also make it a one-line addition to pin the self-overlapping-needle regression once themask_allfix inmod.rslands.As per coding guidelines: "Use
check_cases/check_valuesdirectly when a macro would obscure a table with several inputs or several expected fields per row in table-driven tests."🤖 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/redfish/src/libredfish/instrumented.rs` around lines 558 - 613, Consolidate redact_both_survives_contained_passwords, redact_both_survives_partially_overlapping_passwords, and redact_nonempty_redacts_only_a_nonempty_password into one table-driven test using check_values directly. Represent each row with its input error, one or more passwords, and expected redacted message, preserving all existing cases including the empty-password behavior. Add a row covering the self-overlapping-needle regression after the mask_all fix in mod.rs is available.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.
Duplicate comments:
In `@crates/redfish/src/libredfish/mod.rs`:
- Around line 461-489: Update mask_all’s range collection to detect overlapping
needle occurrences by searching from each match’s start advanced by one UTF-8
character, rather than relying on text.match_indices. Preserve byte-based range
slicing while ensuring cases such as “aa” in “xaaay” and “ana” in “banana” fully
redact every overlapping match; add a regression test for the leakage case if
tests are present.
---
Nitpick comments:
In `@crates/redfish/src/libredfish/instrumented.rs`:
- Around line 558-613: Consolidate redact_both_survives_contained_passwords,
redact_both_survives_partially_overlapping_passwords, and
redact_nonempty_redacts_only_a_nonempty_password into one table-driven test
using check_values directly. Represent each row with its input error, one or
more passwords, and expected redacted message, preserving all existing cases
including the empty-password behavior. Add a row covering the
self-overlapping-needle regression after the mask_all fix in mod.rs is
available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f8ae8972-31fb-4a7b-bdd1-da150e11eba9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
crates/instrument/src/red.rscrates/ipmi/Cargo.tomlcrates/ipmi/src/bmc_mock.rscrates/ipmi/src/lib.rscrates/ipmi/src/metrics.rscrates/ipmi/src/tool.rscrates/redfish/Cargo.tomlcrates/redfish/src/libredfish/implementation.rscrates/redfish/src/libredfish/instrumented.rscrates/redfish/src/libredfish/mod.rsdocs/observability/core_metrics.md
Redfish is the busiest hardware boundary in the system and its error accounting collapsed almost everything into one bucket -- `redfish_other_error` -- while IPMI's power-reset attempts weren't counted at all (their failure-reporting loop was literally commented out). Both boundaries now report per operation.
- Every client the Redfish pool hands out comes wrapped in `InstrumentedRedfish`, a decorator over all 104 trait methods that runs each call through the shared RED instrument (`carbide_external_call_duration_milliseconds{backend="redfish", operation, outcome}`) -- requests, errors, and latency per operation, covering every `libredfish` call site in the system with zero call-site edits. The two client-creation paths that do HTTP are wrapped too. The typed `nv_redfish` stack stays unmetered for now: its operation vocabulary is resource URLs, which is exactly the unbounded label this design forbids, so it needs its own treatment.
- This backend's `outcome` grows a third value: `unsupported`, for calls a vendor answers with a local `NotSupported` stub (lockdown status and other capability probes do this as a matter of course). Counting those as errors would inflate the failure rate with expected answers and write a warn per probe fleet-wide; as their own outcome they're quiet, truthful, and show per operation what a fleet's BMCs don't support. The shared RED helper gains a low-level `red::record` for backends that need their own outcome vocabulary.
- The five password-bearing methods redact the password from the error text before the failure warn fires -- callers redacted only after the error returned, too late for that log line -- with a guard for the empty-password probe that `uefi_setup` uses.
- `carbide_ipmi_commands_total{command, outcome}` counts every `ipmitool` execution (chassis power reset, the DPU legacy reset, BMC cold reset) in both the real tool runner and the bmc-mock implementation -- including the intermediate attempts whose errors are collected into a Vec and dropped. One count is one dispatched command: the real runner's internal subprocess retries (up to three) ride inside a single count, and a command that never reached the wire -- credentials unavailable -- does not count at all (the mock counts at its request boundary, mirroring the real runner).
- The old coarse `redfish_operation_metric_label` stays untouched: the state controller's per-state error metric keys on its values, and its per-operation replacement now lives at the boundary where the calls actually happen.
Tests added!
This supports NVIDIA#3174
Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
force-pushed
the
gh-issue-3174-redfish
branch
from
48e516d to
edc50dd
Compare
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/ipmi/src/tool.rs (1)
154-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEmit the command log as structured fields.
The formatted
Debugmessage makes command details opaque to logfmt queries. Keep the command and BMC address as tracing fields instead.Proposed change
- tracing::info!("Running command: {:?}", cmd); + tracing::info!(?command, %bmc_ip, "running IPMI command");As per coding guidelines, “All services should emit logs in 'logfmt' syntax” and tracing values should be fields rather than interpolated text.
🤖 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/ipmi/src/tool.rs` at line 154, Update the tracing::info call in the command execution flow to emit cmd and the BMC address as structured tracing fields rather than interpolating cmd into the message. Preserve the existing command-run context while ensuring both values remain queryable in logfmt output.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.
Nitpick comments:
In `@crates/ipmi/src/tool.rs`:
- Line 154: Update the tracing::info call in the command execution flow to emit
cmd and the BMC address as structured tracing fields rather than interpolating
cmd into the message. Preserve the existing command-run context while ensuring
both values remain queryable in logfmt output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f52461e5-2d97-49f5-a13a-f6452eb5f1f8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
crates/instrument/src/red.rscrates/ipmi/Cargo.tomlcrates/ipmi/src/bmc_mock.rscrates/ipmi/src/lib.rscrates/ipmi/src/metrics.rscrates/ipmi/src/tool.rscrates/redfish/Cargo.tomlcrates/redfish/src/libredfish/implementation.rscrates/redfish/src/libredfish/instrumented.rscrates/redfish/src/libredfish/mod.rsdocs/observability/core_metrics.md
1 participant
Redfish is the busiest hardware boundary in the system, and its error accounting collapsed almost everything into
redfish_other_error; IPMI's power-reset attempts weren't counted at all (the failure-reporting loop was literally commented out). Both now report per operation.InstrumentedRedfish: all 104 trait methods recordcarbide_external_call_duration_milliseconds{backend="redfish", operation, outcome}-- requests, errors, and latency per operation, covering everylibredfishcall site with zero call-site edits. (The typednv_redfishstack has a URL-shaped operation vocabulary -- exactly the unbounded label this design forbids -- and needs its own treatment.)outcomegains a third value,unsupported, for calls a vendor answers with a localNotSupportedstub: quiet and truthful, instead of inflating the error rate and writing a warn per capability probe fleet-wide -- and it shows per operation what a fleet's BMCs don't support. The shared helper gains a low-levelred::recordfor backends that need their own outcome vocabulary.uefi_setupuses.carbide_ipmi_commands_total{command, outcome}counts each dispatched IPMI command in both the real runner and bmc-mock -- including the legacy-then-standard intermediate attempts whose errors are collected and dropped. A command that never reaches the wire (credentials unavailable) does not count, and the runner's internal subprocess retries ride inside a single count.Tests added!
This supports #3174