refactor(api,bmc-proxy): pair TLS failure records with their counters#3741
Conversation
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughThe API listener and BMC proxy now emit structured events for TCP accept, TLS handshake, and certificate reload failures. Events include reason labels and diagnostic context, while tests validate logs and metric counter increments. Firmware upload assertions now use structured observations. ChangesFailure instrumentation
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
chet
changed the title
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3741.docs.buildwithfern.com/infra-controller |
chet
changed the title
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 21 minutes. |
|
Hey @coderabbitai can you do a full_review when you get a chance? CICD isn't letting me continue because the CodeRabbit check "failed". Thanks! |
|
🐇🔍 ✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 15 minutes. |
|
🐇🔍 ✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 15 minutes. |
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/preingestion-manager/src/metrics.rs`:
- Around line 691-698: Remove the redundant method_label and outcome_label
fields from FirmwareUploadInput, and update expected_firmware_upload and all
callers to derive labels directly from the method and outcome enum values via
their existing label conversion (such as label_value()). Eliminate the
corresponding raw string parameters and initializers while preserving the
current metric labels and expected 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: 83e53cb6-5abf-494c-abf8-afa125541912
📒 Files selected for processing (3)
crates/api-core/src/listener.rscrates/bmc-proxy/src/bmc_proxy.rscrates/preingestion-manager/src/metrics.rs
The API and BMC proxy TLS listeners incremented their existing failure counters separately from the ERROR records that explained those failures. Typed sibling `Event`s now pair TCP accept, certificate reload, and handshake failures with their historical records. Counter names, reason labels, messages, peer addresses, and error context remain unchanged; connection attempts and successes stay metric-only rather than adding new log volume. The branch also moves the merged NVIDIA#3743 firmware-upload outcome cases into a table-driven test. Production behavior in that area is unchanged. This supports NVIDIA#3728 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/preingestion-manager/src/metrics.rs (1)
772-932: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModel expected historical logs as a typed variant.
Option<(Level, &str, &str)>permits invalid level/event-name/message combinations for a closed set of records. Replace it with anExpectedFirmwareUploadLogenum that derives those fields, and use that enum in the cases.As per coding guidelines, “avoid stringly-typed values when a value has a known finite set of possibilities.”
🤖 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/preingestion-manager/src/metrics.rs` around lines 772 - 932, Replace the tuple-based log parameter in expected_firmware_upload with an ExpectedFirmwareUploadLog enum representing each valid firmware-upload historical log variant. Derive the tracing level, event name, and message from the enum inside expected_firmware_upload, and update all test cases to pass the corresponding typed variant instead of raw tuples.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/preingestion-manager/src/metrics.rs`:
- Around line 772-932: Replace the tuple-based log parameter in
expected_firmware_upload with an ExpectedFirmwareUploadLog enum representing
each valid firmware-upload historical log variant. Derive the tracing level,
event name, and message from the enum inside expected_firmware_upload, and
update all test cases to pass the corresponding typed variant instead of raw
tuples.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6eceef00-cd1e-47dd-99c1-262bbc8a159f
📒 Files selected for processing (3)
crates/api-core/src/listener.rscrates/bmc-proxy/src/bmc_proxy.rscrates/preingestion-manager/src/metrics.rs
4 participants
The API and BMC proxy TLS listeners incremented their existing failure counters separately from the ERROR records that explained those failures.
Typed sibling
Events now pair TCP accept, certificate reload, and handshake failures with their historical records. Counter names, reason labels, messages, peer addresses, and error context remain unchanged; connection attempts and successes stay metric-only rather than adding new log volume.The branch also moves the merged #3743 firmware-upload outcome cases into a table-driven test. Production behavior in that area is unchanged.
Related issues
Type of Change
Breaking Changes
Testing
The API and BMC proxy failure matrices use
check_valuesto verify that every reason produces one counter increment and one historical ERROR record. Since #3743 merged while this branch was open, it also converts that PR's seven firmware-upload outcome cases tocheck_values; the multipart fallback remains explicit because the ordered pair of attempts is the behavior under test.Verified with:
carbide-api-coreTLS failureEventtestcarbide-bmc-proxyTLS failureEventtestcarbide-preingestion-managerfirmware-upload outcome testcargo make format-nightlycargo make clippycargo make carbide-lintscargo make check-event-namescargo xtask check-metric-docsAdditional Notes
The firmware-upload changes are test-only; production behavior in that area remains the version merged through #3743.
Closes #3728