Skip to content

fix: reconcile the DHCP replies-sent counter's HELP across its two crates#3466

Merged
chet merged 1 commit into
NVIDIA:mainfrom
chet:gh-issue-3461
Jul 14, 2026
Merged

fix: reconcile the DHCP replies-sent counter's HELP across its two crates#3466
chet merged 1 commit into
NVIDIA:mainfrom
chet:gh-issue-3461

Conversation

@chet

@chet chet commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

carbide_dhcp_replies_sent_total is emitted by two separate binaries -- the DHCP relay in crates/dhcp and the standalone DHCP server in crates/dhcp-server -- which declared it with divergent HELP text ("replies sent" vs "replies successfully sent"). Same metric, same meaning, two strings, so a scraper sees inconsistent HELP depending on which process it hits.

The two are the same metric legitimately exported by two processes, so this unifies the description rather than renaming: dhcp-server's "successfully" is redundant (packets that never get a reply are counted separately under its own drop counter), so it now matches dhcp's shorter wording.

  • crates/dhcp-server/src/metrics.rs: drop "successfully" from the replies_sent describe so both crates declare identical HELP.

The counter isn't exercised by test_integration, so it isn't in core_metrics.md today and there's no catalogue regen here. When #3463's doc-gate documents it, the reconciled HELP is what it will record; if #3463 lands first, this rebases so the catalogue row reads "sent".

This supports #3461

…ates

`carbide_dhcp_replies_sent_total` is emitted by two separate binaries -- the DHCP relay in `crates/dhcp` and the standalone DHCP server in `crates/dhcp-server` -- which declared it with divergent HELP text. `dhcp` describes it as "Number of DHCP replies sent, by reply message type."; `dhcp-server` said "Number of DHCP replies successfully sent, by reply message type." Same metric, same meaning, two strings, so a scraper sees inconsistent HELP depending on which process it hits, and a catalogue built by scanning declarations would face an ambiguous entry.

The two are the same metric legitimately exported by two processes, so the fix is to unify the description, not to rename. `dhcp-server`'s "successfully" is redundant -- packets that never receive a reply are counted separately under its own drop counter -- so this aligns `dhcp-server` on `dhcp`'s shorter wording.

- `crates/dhcp-server/src/metrics.rs`: drop "successfully" from the `replies_sent` describe so both crates declare identical HELP.

The counter isn't exercised by `test_integration`, so it isn't in `core_metrics.md` today and there's no catalogue regen here; when the instrumentation doc-gate documents it, the reconciled HELP is what it will record.

This supports NVIDIA#3461

Signed-off-by: Chet Nichols III <chetn@nvidia.com>
@chet chet requested a review from a team as a code owner July 14, 2026 01:12
@chet

chet commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full_review, thanks!

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

@chet, you're welcome! I'll redo the full review of the PR now.

ᕦ(ò_óˇ)ᕤ

✅ Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3db1254c-63c7-4307-9bc2-f9c9f81f7899

📥 Commits

Reviewing files that changed from the base of the PR and between f983414 and 18381a7.

📒 Files selected for processing (1)
  • crates/dhcp-server/src/metrics.rs

Summary by CodeRabbit

  • Documentation
    • Updated the description for the “DHCP replies sent” metric to more accurately reflect that it covers all DHCP replies, grouped by reply message type.

Walkthrough

The DHCP reply metric description now refers to replies sent by message type without stating that they were successfully sent. No metric logic, labels, or event fields changed.

Changes

DHCP metrics

Layer / File(s) Summary
Update reply metric metadata
crates/dhcp-server/src/metrics.rs
The carbide_dhcp_replies_sent_total description was updated while preserving its existing labels and event fields.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Possibly related issues

  • NVIDIA/infra-controller issue 3461 — The metric description aligns with the divergent declaration identified in this issue.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the metric HELP reconciliation across the DHCP binaries and matches the change intent.
Description check ✅ Passed The description is directly related to the metric HELP text update and explains the rationale and scope clearly.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@github-actions

Copy link
Copy Markdown

🔍 Container Scan Summary

Service Total Critical High Medium Low Other
boot-artifacts-aarch64 3 0 0 3 0 0
boot-artifacts-x86_64 3 0 0 3 0 0
forge-admin-cli-x86_64 255 13 30 79 7 126
machine-validation-runner 800 40 234 292 36 198
machine_validation 800 40 234 292 36 198
machine_validation-aarch64 800 40 234 292 36 198
nvmetal-carbide 800 40 234 292 36 198
TOTAL 3461 173 966 1253 151 918

Per-CVE detail lives in the per-service grype-* artifacts (JSON + SARIF). Severity counts only — no CVE IDs published here.

@chet chet enabled auto-merge (squash) July 14, 2026 04:43
@chet chet merged commit da30d63 into NVIDIA:main Jul 14, 2026
59 checks passed
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