feat(health): add primary system UUID and static labels to telemetry#4041
Open
jingxiang-z wants to merge 6 commits into
Open
feat(health): add primary system UUID and static labels to telemetry#4041jingxiang-z wants to merge 6 commits into
jingxiang-z wants to merge 6 commits into
Conversation
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughThe health service adds validated static endpoint labels and optional machine system UUID metadata, resolves primary Redfish system identities during discovery, and exports the resulting metadata through Prometheus, OTLP, JSONL, and tracing sinks. ChangesHealth metadata propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Discovery
participant BMC
participant EventContext
participant TelemetrySinks
Discovery->>BMC: Resolve primary ComputerSystem
BMC-->>Discovery: Return system UUID
Discovery->>EventContext: Attach system UUID and endpoint labels
EventContext->>TelemetrySinks: Export metadata with events and metrics
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jingxiang-z
marked this pull request as ready for review
July 23, 2026 20:36
Collaborator
|
@yoks @akorobkov-nvda @kensimon Seems like this'll come up again and again - since high cardinality destroys prometheus. |
![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: 1
🧹 Nitpick comments (2)
crates/health/src/discovery/identity.rs (1)
104-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a table-driven selection test.
These tests all map system collections to a selected identity. Consolidate them into
check_cases/value_scenarios!cases, including the empty collection case.As per coding guidelines, “Use table-driven tests for functions mapping inputs to outputs or errors.”
🤖 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/discovery/identity.rs` around lines 104 - 149, Consolidate the three tests for select_primary_system into a single table-driven test using the project’s check_cases/value_scenarios! pattern. Include cases for preferring the first BIOS-bearing system, falling back to the first member, and returning None for an empty collection, while preserving the current expected identities and UUIDs.Source: Coding guidelines
crates/health/src/sink/events.rs (1)
63-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the new context metadata.
The current tests only use an empty label map and
system_uuid: None, so they do not verify thatEventContext::from_endpointcopies labels or thatsystem_uuid()returns a populated UUID. Add table-driven cases covering populated and absent values.As per coding guidelines and path instructions, changed Rust behavior should have table-driven regression coverage, with particular attention to missing tests.
Also applies to: 96-101
🤖 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/sink/events.rs` around lines 63 - 74, Add table-driven regression cases for EventContext::from_endpoint and system_uuid(), covering both populated and absent labels and system UUID values. Assert that populated endpoint labels are copied and populated UUIDs are returned, while preserving expected behavior for empty or missing metadata; update the existing tests near endpoint_key() and labels() coverage.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/health/src/config.rs`:
- Around line 315-359: Update the static BMC endpoint label validation in the
configuration validation method containing RESERVED_LABELS so arbitrary
per-endpoint label values cannot become metric attributes; restrict labels to an
explicitly bounded, low-cardinality set or reject labels intended to carry
unique identifiers such as asset, machine, or instance IDs. Preserve the
existing name, count, reserved-name, and byte-length checks while ensuring the
accepted configuration cannot create unbounded telemetry series in the endpoint
source flow.
---
Nitpick comments:
In `@crates/health/src/discovery/identity.rs`:
- Around line 104-149: Consolidate the three tests for select_primary_system
into a single table-driven test using the project’s check_cases/value_scenarios!
pattern. Include cases for preferring the first BIOS-bearing system, falling
back to the first member, and returning None for an empty collection, while
preserving the current expected identities and UUIDs.
In `@crates/health/src/sink/events.rs`:
- Around line 63-74: Add table-driven regression cases for
EventContext::from_endpoint and system_uuid(), covering both populated and
absent labels and system UUID values. Assert that populated endpoint labels are
copied and populated UUIDs are returned, while preserving expected behavior for
empty or missing metadata; update the existing tests near endpoint_key() and
labels() coverage.
🪄 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: ef8229a7-e811-4515-b577-c99716aa6209
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
crates/health/Cargo.tomlcrates/health/benches/collector_pipeline.rscrates/health/benches/processor_pipeline.rscrates/health/benches/sink_pipeline.rscrates/health/example/config.example.tomlcrates/health/src/api_client.rscrates/health/src/collectors/nvue/gnmi/on_change_processor.rscrates/health/src/collectors/nvue/gnmi/sample_processor.rscrates/health/src/collectors/nvue/rest/collector.rscrates/health/src/config.rscrates/health/src/discovery/identity.rscrates/health/src/discovery/iteration.rscrates/health/src/discovery/mod.rscrates/health/src/discovery/spawn.rscrates/health/src/endpoint/cluster.rscrates/health/src/endpoint/mod.rscrates/health/src/endpoint/model.rscrates/health/src/endpoint/sources.rscrates/health/src/otlp/convert.rscrates/health/src/processor/health_report.rscrates/health/src/processor/intrusion_events.rscrates/health/src/processor/leak_events.rscrates/health/src/processor/mod.rscrates/health/src/processor/rack_leak.rscrates/health/src/sink/events.rscrates/health/src/sink/health_report.rscrates/health/src/sink/log_file.rscrates/health/src/sink/mod.rscrates/health/src/sink/otlp.rscrates/health/src/sink/prometheus.rscrates/health/src/sink/tracing.rsdocs/architecture/health_aggregation.mddocs/operations/monitoring-health.md
Contributor
|
@ajf this high cardinality data emited through non standart /telemetry endpoint and not scraped by default. In large clusters Prometheus endpoint is disabled and only OpenTelemetry one is exposed. |
yoks
reviewed
Jul 23, 2026
yoks
reviewed
Jul 23, 2026
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/health/src/discovery/identity.rs (1)
144-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep primary-system selection tests table-driven.
This adds another standalone input/output case; consolidate these cases into one table-driven test so selection rules remain easy to extend and compare.
As per coding guidelines, “Use table-driven tests for functions mapping inputs to outputs or errors.”
🤖 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/discovery/identity.rs` around lines 144 - 163, Consolidate the standalone primary-system selection cases, including primary_system_prefers_host_bios_over_auxiliary_uuid, into one table-driven test for select_primary_system. Define named input/expected-output cases and iterate over them, preserving each case’s selection and UUID assertions while making future cases easy to add.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/health/src/discovery/identity.rs`:
- Around line 144-163: Consolidate the standalone primary-system selection
cases, including primary_system_prefers_host_bios_over_auxiliary_uuid, into one
table-driven test for select_primary_system. Define named input/expected-output
cases and iterate over them, preserving each case’s selection and UUID
assertions while making future cases easy to add.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8610a4d1-2fb1-4e05-9461-7aeb99c64b4e
📒 Files selected for processing (17)
crates/health/benches/collector_pipeline.rscrates/health/benches/processor_pipeline.rscrates/health/benches/sink_pipeline.rscrates/health/src/api_client.rscrates/health/src/discovery/identity.rscrates/health/src/discovery/iteration.rscrates/health/src/discovery/spawn.rscrates/health/src/endpoint/mod.rscrates/health/src/endpoint/model.rscrates/health/src/endpoint/sources.rscrates/health/src/otlp/convert.rscrates/health/src/processor/health_report.rscrates/health/src/sink/events.rscrates/health/src/sink/health_report.rscrates/health/src/sink/log_file.rscrates/health/src/sink/mod.rscrates/health/src/sink/prometheus.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- crates/health/benches/sink_pipeline.rs
- crates/health/src/endpoint/mod.rs
- crates/health/src/processor/health_report.rs
- crates/health/src/discovery/spawn.rs
- crates/health/benches/processor_pipeline.rs
- crates/health/src/sink/health_report.rs
- crates/health/src/sink/log_file.rs
- crates/health/src/sink/events.rs
- crates/health/src/sink/mod.rs
- crates/health/src/sink/prometheus.rs
- crates/health/src/otlp/convert.rs
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
jingxiang-z
force-pushed
the
feat/primary-system-uuid-telemetry
branch
from
July 24, 2026 17:44
3b7773b to
bae7cd2
Compare
Contributor
|
/ok to test bae7cd2 |
yoks
enabled auto-merge (squash)
July 24, 2026 18:36
yoks
approved these changes
Jul 24, 2026
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4041.docs.buildwithfern.com/infra-controller |
polarweasel
requested changes
Jul 24, 2026
Co-authored-by: Alex Ball <awball@polarweasel.org> Signed-off-by: Jingxiang Zhang <72907213+jingxiang-z@users.noreply.github.com>
auto-merge was automatically disabled
July 24, 2026 19:33
Head branch was pushed to by a user without write access
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.
Fleet Intelligence needs hardware-health telemetry to carry stable hardware
identity and deployment-specific context. This change adds an optional
BMC-derived system UUID label and user-defined labels for static endpoints.
For machine endpoints, hardware health discovers the primary Redfish
ComputerSystemUUID, preferring a system with BIOS metadata and falling backto the first collection member. When available, the UUID is emitted as
system_uuidin Prometheus,system.uuidin OTLP resources, and through logand health-event context. Telemetry collection continues without this optional
label when identity discovery is unavailable.
Static BMC endpoints can also define user-defined telemetry labels. Label names
use the Prometheus-compatible identifier shape, built-in identity labels are
reserved, values are bounded, and each endpoint supports at most 32 custom
labels.
Related issues
None.
Type of Change
Breaking Changes
Testing
cargo test -p carbide-health(410 passed)Additional Notes
The system UUID is discovered from the BMC and cannot be overridden with a
user-defined label. This preserves upstream's optional
machine_idbehaviorfor static deployments that do not use NICo.