docs: per-object state metrics#3104
Conversation
Signed-off-by: ianisimov <ianisimov@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3104.docs.buildwithfern.com/infra-controller |
Summary by CodeRabbit
WalkthroughThis PR adds a design document for per-object state progress metrics. It defines the metric set, endpoint behavior, cardinality limits, query patterns, exclusions, and implementation approach. ChangesPer-object state metrics design proposal
Estimated code review effort: 1 (Trivial) | ~5 minutes Related Issues: Not specified. Related PRs: Not specified. Suggested labels: documentation, design-proposal Suggested reviewers: Not specified. Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔍 Container Scan SummaryNo Grype artifacts were found to aggregate. |
![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)
docs/design/per-object-state-metrics.md (1)
222-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCall out the registry API break explicitly.
PerObjectMetricsRegistryis currently classification-specific and registers one hard-coded gauge on the supplied meter. The implementation plan should say this is a breaking expansion, not a drop-in reuse, so the existing health metric and the new state/_info gauges stay clearly separated.Suggested text change
- We extend it rather than adding a sibling: + We extend it rather than adding a sibling, but this is a breaking API expansion: the current classification-only gauge stays intact while new handles are added for state and `_info` metrics:🤖 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 `@docs/design/per-object-state-metrics.md` around lines 222 - 248, The plan should explicitly state that reusing PerObjectMetricsRegistry is a breaking API expansion, not a drop-in reuse, because it is currently classification-specific and only registers one hard-coded gauge. Update the description around PerObjectMetricsRegistry, gauge(name, description), and the processor.rs hookup to call out that the registry shape and handle API must change, while keeping the existing health metric separate from the new state and _info gauges on the per-object meter.
🤖 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 `@docs/design/per-object-state-metrics.md`:
- Around line 119-135: The `carbide_machine_instance_info` example currently
includes free-form `tenant_org`, which violates the bounded-label rule for
`_info` metrics. Update the documentation around the `carbide_object_info` and
`carbide_machine_instance_info` examples to remove the raw tenant string and, if
tenant attribution is needed, describe using a stable internal tenant ID/code
instead; keep the label set limited to closed-set traits plus object IDs.
- Around line 55-59: Qualify the scrape-interval guidance in the design doc so
it only applies to the state-transition series, since the `_info` and
association metrics can still change on inventory or topology updates. Update
the wording around the scrape advice near the `run.rs::create_metrics()`
discussion to explicitly distinguish the slow-changing state series from the
freshness-sensitive metrics, and avoid implying that all of the new metrics can
safely be scraped every 60–120s.
---
Nitpick comments:
In `@docs/design/per-object-state-metrics.md`:
- Around line 222-248: The plan should explicitly state that reusing
PerObjectMetricsRegistry is a breaking API expansion, not a drop-in reuse,
because it is currently classification-specific and only registers one
hard-coded gauge. Update the description around PerObjectMetricsRegistry,
gauge(name, description), and the processor.rs hookup to call out that the
registry shape and handle API must change, while keeping the existing health
metric separate from the new state and _info gauges on the per-object meter.
🪄 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: 10f87234-fdf5-450f-b024-f03862a708d3
📒 Files selected for processing (1)
docs/design/per-object-state-metrics.md
polarweasel
left a comment
polarweasel
left a comment
There was a problem hiding this comment.
All minor stuff, but hopefully worth thinking about...
| "machine_id", "$1", "object_id", "(.*)") | ||
| ``` | ||
|
|
||
| **Stuck-and-unhealthy — is it hardware?** (join with the #2168 metric): |
There was a problem hiding this comment.
Uh... GitHub issue numbers directly in docs? I'd prefer this at least be a real URL to the issue, and even better, a one-liner explanation from said issue (with a link).
There was a problem hiding this comment.
This is design doc, i do not think they are rendered in docs? At least i think they should not be. If there is wrong place, where we put design (RFC)?
| endpoint (moving it is a scrape-config-visible change; do it as a later, | ||
| separately announced step). |
There was a problem hiding this comment.
This feels like an internal note. If there's a plan to make the change, maybe link to the issue for tracking? And if there's no plan yet, I'd be inclined to remove.
There was a problem hiding this comment.
It is more about RFC/Design, not docs, so i think it is justified here
| Wiring mirrors #2168: constructed in `setup.rs`, registered on the per-object | ||
| meter, threaded via the controller builder. `carbide_object_info` and | ||
| associations are recorded from the machine-controller handler, which already | ||
| loads rack/SKU/DPU/instance data next to the existing per-object health call. |
There was a problem hiding this comment.
Again, add a real URL at least, but probably better to add real description (and also possibly a link to the issue/PR).
| Wiring mirrors #2168: constructed in `setup.rs`, registered on the per-object | |
| meter, threaded via the controller builder. `carbide_object_info` and | |
| associations are recorded from the machine-controller handler, which already | |
| loads rack/SKU/DPU/instance data next to the existing per-object health call. | |
| This wiring mirrors #2168: constructed in `setup.rs`, registered on the | |
| per-object meter, threaded via the controller builder. `carbide_object_info` | |
| and associations are recorded from the machine-controller handler, which | |
| already loads rack/SKU/DPU/instance data next to the existing per-object | |
| health call. |
There was a problem hiding this comment.
Added link, i find it is cleaner to look at source, it have more context than just explaining it in the RFC
Co-authored-by: Alex Ball <awball@polarweasel.org> Signed-off-by: Ivan Anisimov <inyokwetrust@gmail.com>
Signed-off-by: ianisimov <ianisimov@nvidia.com>
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 `@docs/design/per-object-state-metrics.md`:
- Around line 187-193: The reprovision suppression example in the
per-object-state metrics docs is too broad and includes a no-op label rewrite.
Update the expression around the suppression logic to remove the unnecessary
label_replace on carbide_machine_dpu_info and make the state selector on
carbide_object_state_entered_timestamp_seconds explicitly filter
object_type="machine". Keep the example anchored to the existing
labels/functions (carbide_forge_dpu_agent_last_call, carbide_machine_dpu_info,
carbide_object_state_entered_timestamp_seconds, and the unless on(machine_id)
clause) so it remains safe and technically correct to copy.
🪄 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: 2747f8d9-2813-4149-a16a-9a0b6c64fa65
📒 Files selected for processing (1)
docs/design/per-object-state-metrics.md
| ``` | ||
| (time() - carbide_forge_dpu_agent_last_call > 900) | ||
| and on(dpu_id) label_replace(carbide_machine_dpu_info, "dpu_id", "$1", "dpu_id", "(.*)") | ||
| unless on(machine_id) label_replace( | ||
| carbide_object_state_entered_timestamp_seconds{state="reprovisioning"}, | ||
| "machine_id", "$1", "object_id", "(.*)") | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Tighten the reprovision suppression example.
label_replace(..., "dpu_id", "$1", "dpu_id", "(.*)") is a no-op, and the state selector should pin object_type="machine" so the example cannot suppress DPU alerts for an unrelated object type that happens to expose reprovisioning. As per path instructions, examples should stay technically correct and safe to copy.
Suggested fix
-(time() - carbide_forge_dpu_agent_last_call > 900)
-and on(dpu_id) label_replace(carbide_machine_dpu_info, "dpu_id", "$1", "dpu_id", "(.*)")
-unless on(machine_id) label_replace(
- carbide_object_state_entered_timestamp_seconds{state="reprovisioning"},
- "machine_id", "$1", "object_id", "(.*)")
+(time() - carbide_forge_dpu_agent_last_call > 900)
+and on(dpu_id) carbide_machine_dpu_info
+unless on(machine_id) label_replace(
+ carbide_object_state_entered_timestamp_seconds{object_type="machine",state="reprovisioning"},
+ "machine_id", "$1", "object_id", "(.*)")📝 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.
| ``` | |
| (time() - carbide_forge_dpu_agent_last_call > 900) | |
| and on(dpu_id) label_replace(carbide_machine_dpu_info, "dpu_id", "$1", "dpu_id", "(.*)") | |
| unless on(machine_id) label_replace( | |
| carbide_object_state_entered_timestamp_seconds{state="reprovisioning"}, | |
| "machine_id", "$1", "object_id", "(.*)") | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 187-187: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/design/per-object-state-metrics.md` around lines 187 - 193, The
reprovision suppression example in the per-object-state metrics docs is too
broad and includes a no-op label rewrite. Update the expression around the
suppression logic to remove the unnecessary label_replace on
carbide_machine_dpu_info and make the state selector on
carbide_object_state_entered_timestamp_seconds explicitly filter
object_type="machine". Keep the example anchored to the existing
labels/functions (carbide_forge_dpu_agent_last_call, carbide_machine_dpu_info,
carbide_object_state_entered_timestamp_seconds, and the unless on(machine_id)
clause) so it remains safe and technically correct to copy.
Source: Path instructions
3 participants
Design describing #2186 implementation.
https://github.com/yoks/bare-metal-manager-core/blob/d7497715d555d0bdc120766a8ebda0c77d56ffb0/docs/design/per-object-state-metrics.md
Related issues
#2186
Type of Change
Breaking Changes
Testing
Additional Notes