Skip to content

feat(health): include alert details in OTLP health report records - #4500

Open
ambermingxin wants to merge 1 commit into
NVIDIA:mainfrom
ambermingxin:feat/alert-details-in-otlp
Open

feat(health): include alert details in OTLP health report records#4500
ambermingxin wants to merge 1 commit into
NVIDIA:mainfrom
ambermingxin:feat/alert-details-in-otlp

Conversation

@ambermingxin

@ambermingxin ambermingxin commented Aug 3, 2026

Copy link
Copy Markdown

Health reports exported over OTLP today carry only counts. An operator looking
at a degraded machine sees health report: 2 alerts, 35 ok (source: BmcSensors)
and has no way to tell which component alerted or why, because
convert_event uses report.alerts only for .len(). The detail exists on
CollectorEvent::HealthReport and the TracingSink already logs it, but only
to stdout, so it is unreachable for any OTLP consumer.

This adds an opt-in, per-target include_alert_details flag. When enabled, a
health report record with alerts gains a health_report.alerts attribute
holding a JSON array of the individual alerts (probe_id, message,
classifications, and target when the alert names one). include_diagnostics
does not help here: it is folded in at enqueue time and applies only to
CollectorEvent::Log records.

The flag is per target rather than sink-global because OtlpSink::new_many
builds one sink and drain per configured target, so a debugging destination can
receive detail while a long-term store keeps receiving only counts.

Probe and classification identities use the existing stable wire names rather
than new serde derives, so OOB GPU inventory alerts appear as SkuValidation,
matching how they already dedup against the machine-controller's in-band SKU
alerts.

Backward compatibility

The body string, severity mapping, and event.type attribute are unchanged, and
the new attribute is purely additive. With the flag off the emitted record is
byte-identical to today's; health_report_omits_alert_details_when_disabled
guards this by asserting the record carries exactly one attribute.

Bounding the attribute size

At most 64 alerts are serialized per record. A truncated record also carries
health_report.alerts.dropped with the number omitted.

The cap is not cosmetic. Measured against a mock reporting every sensor as
failed, 64 alerts serialize to ~15.5 KB, or roughly 242 bytes per alert. On
hardware where the BmcSensors probe covers 256 sensors, an uncapped attribute
would be ~62 KB per record, and at the default batch_size = 512 a single
export could approach 30 MB against a collector defaulting to a 4 MB receive
limit. ResourceExhausted is in the drain's retryable set, so an oversized
batch would be retried five times and then dropped in full, taking unrelated
records with it.

Reviewer input welcome on whether 64 is the right limit and whether it should be
configurable rather than a constant.

Related issues

None.

Type of Change

  • Add - New feature or capability

Breaking Changes

  • This PR contains breaking changes

Testing

  • Unit tests added/updated
  • Manual testing performed

Unit tests cover the flag off, the flag on with multiple alerts parsed field by
field, a report with no alerts, an alert with no target (key omitted rather than
null), and truncation past the cap.

Manual testing ran the service against real hardware exporting to an
otel/opentelemetry-collector, on a machine reporting an actual leak condition.
All three probes produced detail correctly:

Body: Str(health report: 2 alerts, 2 ok (source: BmcLeakDetectors))
Attributes:
     -> event.type: Str(health_report)
     -> health_report.alerts: Str([{"probe_id":"BmcLeakDetection","target":"/redfish/v1/Chassis/Chassis_0/ThermalSubsystem/LeakDetection/LeakDetectors/Chassis_0_LeakDetector_0_Manifold","message":"Leak detector '...' reports critical","classifications":["LeakDetector"]}, ...])

The TrayLeakDetection report exercised the absent-target path, and the
BmcSensors report carried the underlying detector voltages against their
thresholds. Re-running the same fault with the flag off produced records with
only event.type.

Additional Notes

Serialization failure logs a warning and skips the attribute rather than
unwrapping, so a malformed report costs the detail and not the record.

Signed-off-by: Amber Xue <ambermingxin@nvidia.com>
@ambermingxin
ambermingxin requested review from a team and polarweasel as code owners August 3, 2026 18:02
@copy-pr-bot

copy-pr-bot Bot commented Aug 3, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Summary by CodeRabbit

  • New Features

    • OTLP health reports can now optionally include detailed alert information per target.
    • Alert details are limited to 64 entries, with omitted counts recorded when truncated.
    • Alert-detail export is disabled by default for active configurations.
  • Documentation

    • Added configuration guidance and details about alert formatting and probe-name normalization.
  • Tests

    • Added coverage for defaults, serialization, truncation, and unsupported or absent alert details.

Walkthrough

The change adds per-target OTLP health alert-detail configuration. Enabled targets serialize up to 64 alert details into health-report records and report truncation. The export pipeline passes the setting from OtlpTargetConfig through flush and event conversion.

Changes

OTLP health alert details

Layer / File(s) Summary
Alert-detail configuration and documentation
crates/health/src/config.rs, crates/health/example/config.example.toml, docs/operations/monitoring-health.md
OtlpTargetConfig now includes include_alert_details, disabled by default. Examples, parsing tests, fixtures, and monitoring documentation describe the setting.
Alert-detail serialization
crates/health/src/otlp/convert.rs
Health-report conversion serializes probe, target, message, and classification fields as JSON. It limits output to 64 alerts and records omitted-alert counts.
Export policy propagation and validation
crates/health/src/otlp/convert.rs, crates/health/src/otlp/drain.rs, crates/health/src/otlp/mod.rs, crates/health/src/sink/otlp.rs
build_export_request accepts the alert-detail policy. flush passes the target setting, and affected tests use the updated API and configuration.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant OtlpTargetConfig
  participant OtlpDrain
  participant build_export_request
  participant HealthReportConverter
  OtlpTargetConfig->>OtlpDrain: include_alert_details
  OtlpDrain->>build_export_request: pass alert-detail policy
  build_export_request->>HealthReportConverter: convert health-report events
  HealthReportConverter->>HealthReportConverter: serialize up to 64 alerts
  HealthReportConverter-->>build_export_request: OTLP record with alert attributes
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the main change: adding alert details to OTLP health report records.
Description check ✅ Passed The description directly explains the new per-target flag, alert serialization, size limits, compatibility, and testing.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
docs/operations/monitoring-health.md (1)

274-284: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Document the OTLP alert-detail fallback.

Probe::GpuInventory maps to "SkuValidation", so retain that description. Add that serialization failure logs a warning, omits the alert attributes, and still emits the health report record.

🤖 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/operations/monitoring-health.md` around lines 274 - 284, Update the OTLP
alert-details documentation near the existing Probe::GpuInventory/SkuValidation
description to state that serialization failures log a warning, omit the alert
attributes, and still emit the health report record.

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 `@docs/operations/monitoring-health.md`:
- Around line 274-284: Update the OTLP alert-details documentation near the
existing Probe::GpuInventory/SkuValidation description to state that
serialization failures log a warning, omit the alert attributes, and still emit
the health report record.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: d839b230-b3a9-432a-874c-c7fbcb425228

📥 Commits

Reviewing files that changed from the base of the PR and between 627c800 and accc262.

📒 Files selected for processing (7)
  • crates/health/example/config.example.toml
  • crates/health/src/config.rs
  • crates/health/src/otlp/convert.rs
  • crates/health/src/otlp/drain.rs
  • crates/health/src/otlp/mod.rs
  • crates/health/src/sink/otlp.rs
  • docs/operations/monitoring-health.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants