feat(nvue-client): measure the switch-config REST calls#3537
Conversation
The nvue REST client -- the switch-configuration path -- logged and measured nothing, so a failed NVUE request was invisible. Every public method funnels through one `execute`, so metering that once covers the client.
- `execute` takes an `operation` name and wraps its request in `red::instrumented("nvue", operation, ...)`, recording `carbide_external_call_duration_milliseconds{backend = "nvue", operation, outcome}` and a WARN on failure. A non-success HTTP status counts as an error, not only a transport failure.
- Each public method passes its own operation (`get_applied_config`, `apply_config_revision`, the DELETE and PATCH halves of a config replace, and so on); the composite helpers inherit the measurement from the primitives they call.
It reuses the outbound-RED family, so there are no new catalogue rows.
This supports NVIDIA#3173
Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
ヽ(•‿•)ノ ✅ Action performedFull review finished. |
Summary by CodeRabbit
WalkthroughNVUE client HTTP operations now pass explicit operation identifiers to an instrumented execution helper. The helper wraps ChangesNVUE request instrumentation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant NvueClient
participant red_instrumented
participant reqwest
NvueClient->>red_instrumented: instrumented("nvue", operation, request)
red_instrumented->>reqwest: execute HTTP request
reqwest-->>red_instrumented: response or error
red_instrumented-->>NvueClient: response or RequestFailed
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/nvue-client/src/client.rs (2)
79-105: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the instrumented boundary.
Add table-driven tests covering successful responses, transport failures, HTTP failures, preserved
RequestFailedmetadata, and the operation labels—including bothreplace_config.deleteandreplace_config.patch.As per coding guidelines, tests for observable input/output and error mappings should use table-driven cases.
🤖 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/nvue-client/src/client.rs` around lines 79 - 105, Add table-driven regression tests for the execute method, covering successful responses, transport errors, HTTP status errors, RequestFailed metadata preservation, and operation labels. Include both replace_config.delete and replace_config.patch labels, and assert the observable response and error mappings at the instrumented boundary.Source: Coding guidelines
79-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepresent operation names as a typed finite set.
The operation values are a closed set, but they are currently raw string literals. Introduce a private
NvueOperationenum with a canonical string representation, then convert to the required static label only at thered::instrumentedboundary. This prevents typos from silently creating new metric series.As per coding guidelines and path instructions, known finite Rust values should be modeled explicitly rather than passed as bare strings.
Also applies to: 110-110, 118-118, 127-127, 149-154, 164-164, 189-189, 227-227
🤖 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/nvue-client/src/client.rs` around lines 79 - 82, Introduce a private NvueOperation enum covering every operation currently passed to execute, with a canonical string representation. Update execute and all its call sites to use this enum, and convert the enum to the required &'static str only at the red::instrumented boundary, preserving the existing operation labels.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.
Nitpick comments:
In `@crates/nvue-client/src/client.rs`:
- Around line 79-105: Add table-driven regression tests for the execute method,
covering successful responses, transport errors, HTTP status errors,
RequestFailed metadata preservation, and operation labels. Include both
replace_config.delete and replace_config.patch labels, and assert the observable
response and error mappings at the instrumented boundary.
- Around line 79-82: Introduce a private NvueOperation enum covering every
operation currently passed to execute, with a canonical string representation.
Update execute and all its call sites to use this enum, and convert the enum to
the required &'static str only at the red::instrumented boundary, preserving the
existing operation labels.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1f4aa237-f4f4-4e62-be02-e6c1013938ec
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/nvue-client/Cargo.tomlcrates/nvue-client/src/client.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/nvue-client/src/client.rs (2)
79-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd table-driven coverage for the instrumented execution boundary.
Please cover successful responses, HTTP error responses handled by
error_for_status(), transport failures, and preservation ofRequestFaileddetails. Also verify the distinct delete/patch operation labels if the existing instrumentation test harness supports it.As per coding guidelines, Rust tests for input-to-output, error, or observable-result mappings should be table-driven.
🤖 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/nvue-client/src/client.rs` around lines 79 - 105, Add table-driven tests around Client::execute covering successful responses, HTTP status failures, transport errors, and preservation of RequestFailed method, URL, body, and source details. Reuse the existing instrumentation test harness to assert the delete and patch operation labels when supported, while keeping each case focused on the observable result and recorded operation.Source: Coding guidelines
79-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepresent operation labels with a Rust enum.
operationand all call sites currently use raw&'static strvalues, even though this is a finite metric-label set. Introduce aNvueOperationenum withDisplayand convert to the instrumentation string only at thered::instrumentedboundary. This prevents typo-driven metric fragmentation and makes future call sites harder to misuse.Suggested shape
- operation: &'static str, + operation: NvueOperation, - self.execute("get_api", request).await + self.execute(NvueOperation::GetApi, request).awaitAs per coding guidelines, known finite Rust values must use enums rather than bare strings, with a derived string representation.
Also applies to: 107-118, 127-127, 149-154, 164-164, 189-189, 227-227
🤖 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/nvue-client/src/client.rs` around lines 79 - 105, Introduce a finite NvueOperation enum covering every operation label used by execute and its call sites, derive or implement Display for its instrumentation value, and change execute plus all callers to use the enum instead of &'static str. Convert the enum to a string only at the red::instrumented boundary, preserving the existing metric labels exactly.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/nvue-client/src/client.rs`:
- Around line 79-105: Add table-driven tests around Client::execute covering
successful responses, HTTP status failures, transport errors, and preservation
of RequestFailed method, URL, body, and source details. Reuse the existing
instrumentation test harness to assert the delete and patch operation labels
when supported, while keeping each case focused on the observable result and
recorded operation.
- Around line 79-105: Introduce a finite NvueOperation enum covering every
operation label used by execute and its call sites, derive or implement Display
for its instrumentation value, and change execute plus all callers to use the
enum instead of &'static str. Convert the enum to a string only at the
red::instrumented boundary, preserving the existing metric labels exactly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fccc4ab7-7e69-4f20-a295-2c336f618374
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/nvue-client/Cargo.tomlcrates/nvue-client/src/client.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
1 participant
The nvue REST client -- the switch-configuration path -- logged and measured nothing, so a failed NVUE request was invisible. Every public method funnels through one
execute, so metering that once covers the client.executetakes anoperationname and wraps its request inred::instrumented("nvue", operation, ...), recordingcarbide_external_call_duration_milliseconds{backend = "nvue", operation, outcome}and a WARN on failure. A non-success HTTP status counts as an error, not only a transport failure.get_applied_config,apply_config_revision, the DELETE and PATCH halves of a config replace, and so on); the composite helpers inherit the measurement from the primitives they call.It reuses the outbound-RED family, so there are no new catalogue rows.
This supports #3173