Skip to content

feat: ensure the instrumentation pattern at compile/CI time#3463

Open
chet wants to merge 1 commit into
NVIDIA:mainfrom
chet:gh-issue-3180-docgate
Open

feat: ensure the instrumentation pattern at compile/CI time#3463
chet wants to merge 1 commit into
NVIDIA:mainfrom
chet:gh-issue-3180-docgate

Conversation

@chet

@chet chet commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Nothing stopped a new metric from landing undocumented or off-pattern -- the catalogue is regenerated from whatever test_integration happens to scrape, so a metric no integration test exercises could ship with no row and no one would notice. This makes the standard self-enforcing.

  • The #[derive(Event)] macro now requires a counter's describe to open with "Number of ", and rejects a name that still ends in _total after the framework strips one -- the doubled-_total_total footgun that has already shipped a bad series. Both have an escape hatch (describe_unchecked, alongside name_unchecked) for grandfathered cases; the sixteen existing counters whose describe predates the rule carry describe_unchecked with their text untouched.
  • cargo xtask check-metric-docs reads every #[event(...)] declaration (via syn, so it handles raw-string names and skips #[cfg(test)] fixtures) and fails CI in the lint-police job if a conforming counter or histogram is missing from core_metrics.md -- catching the undocumented metric the scrape-based regen can't, because it doesn't need the metric exercised at runtime. Twenty-five already-emitted-but-undocumented metrics are now documented (their exact source describes, additive only).
  • STYLE_GUIDE.md and instrumentation.md gain the now-enforced canonical pattern.

name_unchecked events whose exposed name is an OTel-sanitized transform are exempt from the doc-gate for now; cataloguing those precisely wants the declarative registry in #3221.

Tests added!

This supports #3180

Nothing stopped a new metric from landing undocumented or off-pattern -- the catalogue is regenerated from whatever `test_integration` happens to scrape, so a metric no integration test exercises could ship with no row and no one would notice. This makes the standard self-enforcing: the derive rejects an off-pattern counter at compile time, and a CI check rejects an undocumented one.

Primary callouts are:

- The `#[derive(Event)]` macro now requires a counter's `describe` to open with "Number of " (the tech-writer house rule), and rejects a name that still ends in `_total` after the framework strips one -- the doubled-`_total_total` footgun that has already shipped a bad series. Both have an escape hatch (`describe_unchecked`, alongside the existing `name_unchecked`) for the grandfathered cases; the sixteen existing counters whose describe predates the rule carry `describe_unchecked` with their text untouched, and the doubled-name events keep their `name_unchecked`.
- `cargo xtask check-metric-docs` scans every `#[event(...)]` declaration and fails CI (in the `lint-police` job) if a conforming counter or histogram is missing from `core_metrics.md` -- catching the undocumented metric the scrape-based regen can't, because it doesn't need the metric to be exercised at runtime. Twenty-five already-emitted-but-undocumented metrics are now documented (their exact source describes, additive only), so the tree starts green.
- `STYLE_GUIDE.md` gains an Instrumentation section pointing at the framework docs and stating the now-enforced canonical pattern.

`name_unchecked` events whose exposed name is an OTel-sanitized transform are exempt from the doc-gate for now; cataloguing those precisely wants the declarative registry in NVIDIA#3221.

Tests added!

This supports NVIDIA#3180

Signed-off-by: Chet Nichols III <chetn@nvidia.com>
@chet chet requested review from a team, lauragra-y and polarweasel as code owners July 14, 2026 00:40
@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, sounds good — kicking off a full review of all changes in this PR now.

ヽ(・∀・)ノ

✅ Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The PR enforces counter naming and description conventions, provides an escape hatch for existing metrics, adds a source-to-catalogue validation command, updates catalogue entries, and integrates the check into workspace verification and CI.

Changes

Metric instrumentation validation

Layer / File(s) Summary
Event metadata contract
crates/instrument-macros/..., crates/instrument/..., STYLE_GUIDE.md, docs/observability/instrumentation.md
Counter names must avoid doubled _total suffixes, and counter descriptions must use the Number of ... format unless describe_unchecked is specified.
Existing metric metadata migration
crates/api-core/..., crates/bmc-proxy/..., crates/dns/..., crates/dsx-exchange-consumer/..., crates/mqtt-common/..., crates/secrets/...
Existing counter event declarations add describe_unchecked metadata.
Metric catalogue validation and CI wiring
crates/xtask/..., docs/observability/core_metrics.md, Makefile.toml, .github/workflows/ci.yaml
A new xtask scans Event declarations, verifies catalogue rows, reports missing rows, and runs in workspace verification and CI.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CI
  participant Xtask
  participant RustSources
  participant CoreMetricsCatalogue
  CI->>Xtask: cargo xtask check-metric-docs
  Xtask->>RustSources: scan Event metric declarations
  Xtask->>CoreMetricsCatalogue: verify catalogue rows
  CoreMetricsCatalogue-->>Xtask: return catalogue contents
  Xtask-->>CI: pass or report missing metrics
Loading

Possibly related issues

Possibly related PRs

🚥 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 reflects the main change: enforcing instrumentation standards at compile and CI time.
Description check ✅ Passed The description is directly aligned with the changeset and accurately describes the new macro, CI gate, and docs updates.
✨ 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

@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.

Actionable comments posted: 4

🧹 Nitpick comments (3)
crates/instrument/tests/matrix.rs (1)

49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Keep ordinary test fixtures on the checked metadata path.

These counters use describe_unchecked without any describe, so the tests no longer exercise the required counter HELP-text validation. Add valid describe = "Number of ..." values and reserve describe_unchecked for a dedicated escape-hatch test.

Also applies to: 96-97, 213-214, 241-241

🤖 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/instrument/tests/matrix.rs` at line 49, Add valid describe = "Number
of ..." metadata to the ordinary counter fixtures using describe_unchecked at
the referenced locations, so they exercise HELP-text validation through
describe. Keep describe_unchecked only in a dedicated test that explicitly
verifies the escape hatch, preserving valid counter behavior elsewhere.
crates/xtask/src/metric_docs.rs (2)

77-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Prefer bail! over std::process::exit(1) for consistency with sibling xtask checks.

isolated_package_builds::check() reports failure via bail!(...), letting main()'s ? propagate it through the same eyre::Result path used everywhere else in this crate. Calling std::process::exit(1) directly here instead makes the eyre::Result<()> return type misleading (this function effectively never returns Err) and forecloses future reuse/testing of check() as a pure function.

♻️ Proposed fix to align with `bail!` convention
-    eprintln!(
-        "\nEvery counter/histogram declared through the instrumentation framework needs a row \
-         in {CATALOGUE}, so the metric is documented even when no test scrapes it. Add the \
-         row(s) above, in name-sorted position, with the Description column set to the event's \
-         `describe` text. (name_unchecked events are exempt -- their exposed name is a \
-         sanitized transform; see `#3221`.)"
-    );
-    std::process::exit(1);
+    eprintln!(
+        "\nEvery counter/histogram declared through the instrumentation framework needs a row \
+         in {CATALOGUE}, so the metric is documented even when no test scrapes it. Add the \
+         row(s) above, in name-sorted position, with the Description column set to the event's \
+         `describe` text. (name_unchecked events are exempt -- their exposed name is a \
+         sanitized transform; see `#3221`.)"
+    );
+    bail!("{} framework metric(s) are missing from {CATALOGUE}", missing.len());
🤖 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/xtask/src/metric_docs.rs` around lines 77 - 102, Replace the direct
std::process::exit(1) in the metric documentation check with bail!, preserving
the existing diagnostic output and failure message. Ensure the check returns an
eyre error through its Result path so main() can propagate the failure
consistently with sibling xtask checks.

104-118: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

WalkDir errors are silently discarded, contradicting the file's fail-closed design.

.filter_map(Result::ok) drops any walkdir::Error (permission-denied, broken symlink, etc.) instead of surfacing it. The module doc explicitly promises: "It fails closed: a counter/histogram whose name cannot be read, or an event-bearing file that cannot be parsed, is an error, never a silent skip." A directory that fails to be walked is a silent skip of everything beneath it, undermining that guarantee.

🛡️ Proposed fix to propagate WalkDir errors
-    for entry in WalkDir::new(dir)
-        .into_iter()
-        .filter_map(Result::ok)
-        .filter(|e| e.file_type().is_file())
-    {
+    for entry in WalkDir::new(dir) {
+        let entry = entry.with_context(|| format!("walking {}", dir.display()))?;
+        if !entry.file_type().is_file() {
+            continue;
+        }
🤖 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/xtask/src/metric_docs.rs` around lines 104 - 118, Update the collect
function’s WalkDir iteration to propagate each walkdir::Error instead of
discarding errors via filter_map(Result::ok), while preserving the existing file
filtering and processing behavior for successful entries. Ensure directory
traversal failures return the function’s eyre::Result error and maintain the
module’s fail-closed behavior.
🤖 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/instrument/src/lib.rs`:
- Around line 139-141: Update the documentation comment near the counter naming
guidance to clarify that names ending in a single `_total` are valid, while
doubled suffixes such as `carbide_demo_total_total` are rejected. Preserve the
explanation that the Prometheus exporter appends `_total`, and avoid wording
that implies every `_total`-suffixed name is invalid.

In `@docs/observability/instrumentation.md`:
- Around line 203-204: Clarify the counter naming rule in
docs/observability/instrumentation.md lines 203-204: state that the exposed
Prometheus metric name has exactly one `_total`, while the internal instrument
name must not retain `_total` after framework stripping. Apply the same
distinction to the canonical rule in STYLE_GUIDE.md lines 171-172; update both
documents only.
- Around line 219-223: Update the observability instrumentation documentation to
state that log-only events with metric = none must omit describe, while
histogram descriptions remain unrestricted. Revise the sentence beginning
“Histograms and log-only events” without changing the surrounding counter and
describe_unchecked guidance.
- Around line 225-228: Update docs/observability/instrumentation.md lines
225-228 to state that the metric-docs gate skips name_unchecked declarations, so
those metrics are exempt from catalogue-row requirements; update STYLE_GUIDE.md
lines 173-180 to apply the same exception when describing every framework
counter and histogram. Ensure both Markdown descriptions are consistent and
clear.

---

Nitpick comments:
In `@crates/instrument/tests/matrix.rs`:
- Line 49: Add valid describe = "Number of ..." metadata to the ordinary counter
fixtures using describe_unchecked at the referenced locations, so they exercise
HELP-text validation through describe. Keep describe_unchecked only in a
dedicated test that explicitly verifies the escape hatch, preserving valid
counter behavior elsewhere.

In `@crates/xtask/src/metric_docs.rs`:
- Around line 77-102: Replace the direct std::process::exit(1) in the metric
documentation check with bail!, preserving the existing diagnostic output and
failure message. Ensure the check returns an eyre error through its Result path
so main() can propagate the failure consistently with sibling xtask checks.
- Around line 104-118: Update the collect function’s WalkDir iteration to
propagate each walkdir::Error instead of discarding errors via
filter_map(Result::ok), while preserving the existing file filtering and
processing behavior for successful entries. Ensure directory traversal failures
return the function’s eyre::Result error and maintain the module’s fail-closed
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: 69251ece-89d5-43be-a5d5-dce1d2a6e837

📥 Commits

Reviewing files that changed from the base of the PR and between 69cd51c and c1b49b2.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (20)
  • .github/workflows/ci.yaml
  • Makefile.toml
  • STYLE_GUIDE.md
  • crates/api-core/src/handlers/utils.rs
  • crates/api-core/src/listener.rs
  • crates/api-core/src/secrets/metrics.rs
  • crates/bmc-proxy/src/bmc_proxy.rs
  • crates/dns/src/lib.rs
  • crates/dsx-exchange-consumer/src/metrics.rs
  • crates/instrument-macros/src/lib.rs
  • crates/instrument/src/lib.rs
  • crates/instrument/src/testing.rs
  • crates/instrument/tests/matrix.rs
  • crates/mqtt-common/src/metrics.rs
  • crates/secrets/src/forge_vault.rs
  • crates/xtask/Cargo.toml
  • crates/xtask/src/main.rs
  • crates/xtask/src/metric_docs.rs
  • docs/observability/core_metrics.md
  • docs/observability/instrumentation.md

Comment thread crates/instrument/src/lib.rs
Comment thread docs/observability/instrumentation.md
Comment thread docs/observability/instrumentation.md
Comment on lines +225 to +228
The catalogue is regenerated by `test_integration`, which scrapes `/metrics`, and checked in CI.
Because a metric no test exercises is never scraped, `cargo xtask check-metric-docs` also reads the
`#[event(...)]` declarations directly and fails if any framework counter or histogram is missing a
catalogue row -- so a new metric cannot land undocumented.

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.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Document the name_unchecked catalogue exception consistently.

The metric-docs gate skips name_unchecked declarations, so both documents currently overstate the set of metrics that must have catalogue rows.

  • docs/observability/instrumentation.md#L225-L228: add the name_unchecked exception to the CI description.
  • STYLE_GUIDE.md#L173-L180: qualify “every framework counter and histogram” with the same exception.

As per path instructions, Markdown changes must be correct and clear.

📍 Affects 2 files
  • docs/observability/instrumentation.md#L225-L228 (this comment)
  • STYLE_GUIDE.md#L173-L180
🤖 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/observability/instrumentation.md` around lines 225 - 228, Update
docs/observability/instrumentation.md lines 225-228 to state that the
metric-docs gate skips name_unchecked declarations, so those metrics are exempt
from catalogue-row requirements; update STYLE_GUIDE.md lines 173-180 to apply
the same exception when describing every framework counter and histogram. Ensure
both Markdown descriptions are consistent and clear.

Source: Path instructions

@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.

Actionable comments posted: 1

🧹 Nitpick comments (3)
crates/instrument/tests/matrix.rs (1)

49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Keep test fixtures inside the counter contract.

These test counters use describe_unchecked without any describe, bypassing the new required-help-text validation. Add a canonical describe = "Number of ..." to each fixture, reserving describe_unchecked for legacy descriptions that must remain unchanged.

Proposed fix
-        describe_unchecked
+        describe = "Number of matrix test events"

Also applies to: 96-97, 213-214, 241-241

🤖 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/instrument/tests/matrix.rs` at line 49, Update each affected test
counter fixture in matrix.rs to include a canonical describe value such as
“Number of ...” alongside its counter definition, and stop using
describe_unchecked for fixtures whose descriptions can be updated. Reserve
describe_unchecked only for legacy descriptions that must remain unchanged,
covering the occurrences around lines 49, 96-97, 213-214, and 241.
crates/xtask/src/metric_docs.rs (2)

85-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use bail! instead of std::process::exit(1) for consistency with the sibling check.

isolated_package_builds::check() in the same crate signals failure by returning Err via bail!, letting main's ? propagate the exit code. This function instead calls std::process::exit(1) directly after eprintln!, which is inconsistent with that established pattern and discards the message via manual output rather than the standard eyre error path.

♻️ Proposed refactor to bail! instead of exiting directly
-    eprintln!(
-        "{} framework metric(s) are declared with #[derive(Event)] but missing from \
-         {CATALOGUE}:\n",
-        missing.len(),
-    );
-    for m in &missing {
-        eprintln!("  {} ({}) -- {}", m.name, m.kind, m.source.display());
-        eprintln!("      {}", suggested_row(m));
-    }
-    eprintln!(
-        "\nEvery counter/histogram declared through the instrumentation framework needs a row \
-         in {CATALOGUE}, so the metric is documented even when no test scrapes it. Add the \
-         row(s) above, in name-sorted position, with the Description column set to the event's \
-         `describe` text. (name_unchecked events are exempt -- their exposed name is a \
-         sanitized transform; see `#3221`.)"
-    );
-    std::process::exit(1);
+    let mut msg = format!(
+        "{} framework metric(s) are declared with #[derive(Event)] but missing from \
+         {CATALOGUE}:\n",
+        missing.len(),
+    );
+    for m in &missing {
+        msg += &format!("  {} ({}) -- {}\n", m.name, m.kind, m.source.display());
+        msg += &format!("      {}\n", suggested_row(m));
+    }
+    msg += "\nEvery counter/histogram declared through the instrumentation framework needs a row \
+            in this catalogue, so the metric is documented even when no test scrapes it. Add the \
+            row(s) above, in name-sorted position, with the Description column set to the event's \
+            `describe` text. (name_unchecked events are exempt -- their exposed name is a \
+            sanitized transform; see `#3221`.)";
+    bail!(msg);
🤖 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/xtask/src/metric_docs.rs` around lines 85 - 101, Replace the direct
std::process::exit(1) in the missing-metrics reporting path with bail!,
preserving the existing diagnostic message by passing it through the eyre error
path. Follow the established error-propagation pattern used by
isolated_package_builds::check() and leave the missing-metric iteration and
message content unchanged.

333-444: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider table-driven scenarios per repo test convention.

These four tests are individually well-documented, but the repo's coding guideline for **/*.{rs,toml} calls for table-driven / value_scenarios! tests for input→output mapping functions such as declared(). Worth revisiting if/when more scenarios are added here.

As per coding guidelines: "Use table-driven tests for functions mapping inputs to outputs, errors, or observable results; use carbide-test-support scenarios with Outcome for Result-returning operations and value_scenarios! for total operations."

🤖 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/xtask/src/metric_docs.rs` around lines 333 - 444, The tests around
declared should follow the repository’s table-driven convention: consolidate the
input-to-output cases currently covered by raw_string_name_is_collected,
cooked_string_name_is_collected, cfg_test_events_are_excluded, and
non_metric_and_grandfathered_are_skipped into a value_scenarios!-style test,
preserving each scenario’s source input and expected metric names while
retaining clear scenario descriptions.

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.

Inline comments:
In `@crates/bmc-proxy/src/bmc_proxy.rs`:
- Around line 272-273: Update the describe text for TlsConnectionFailed to say
“tls connections” instead of “tcp connections,” matching the sibling
TlsConnectionAttempted and TlsConnectionSucceeded metrics; leave the metric name
and other attributes unchanged.

---

Nitpick comments:
In `@crates/instrument/tests/matrix.rs`:
- Line 49: Update each affected test counter fixture in matrix.rs to include a
canonical describe value such as “Number of ...” alongside its counter
definition, and stop using describe_unchecked for fixtures whose descriptions
can be updated. Reserve describe_unchecked only for legacy descriptions that
must remain unchanged, covering the occurrences around lines 49, 96-97, 213-214,
and 241.

In `@crates/xtask/src/metric_docs.rs`:
- Around line 85-101: Replace the direct std::process::exit(1) in the
missing-metrics reporting path with bail!, preserving the existing diagnostic
message by passing it through the eyre error path. Follow the established
error-propagation pattern used by isolated_package_builds::check() and leave the
missing-metric iteration and message content unchanged.
- Around line 333-444: The tests around declared should follow the repository’s
table-driven convention: consolidate the input-to-output cases currently covered
by raw_string_name_is_collected, cooked_string_name_is_collected,
cfg_test_events_are_excluded, and non_metric_and_grandfathered_are_skipped into
a value_scenarios!-style test, preserving each scenario’s source input and
expected metric names while retaining clear scenario descriptions.
🪄 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: 9a41f9fb-ef8b-4ec5-9b2b-b89dc5dcfe43

📥 Commits

Reviewing files that changed from the base of the PR and between 69cd51c and c1b49b2.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (20)
  • .github/workflows/ci.yaml
  • Makefile.toml
  • STYLE_GUIDE.md
  • crates/api-core/src/handlers/utils.rs
  • crates/api-core/src/listener.rs
  • crates/api-core/src/secrets/metrics.rs
  • crates/bmc-proxy/src/bmc_proxy.rs
  • crates/dns/src/lib.rs
  • crates/dsx-exchange-consumer/src/metrics.rs
  • crates/instrument-macros/src/lib.rs
  • crates/instrument/src/lib.rs
  • crates/instrument/src/testing.rs
  • crates/instrument/tests/matrix.rs
  • crates/mqtt-common/src/metrics.rs
  • crates/secrets/src/forge_vault.rs
  • crates/xtask/Cargo.toml
  • crates/xtask/src/main.rs
  • crates/xtask/src/metric_docs.rs
  • docs/observability/core_metrics.md
  • docs/observability/instrumentation.md

Comment on lines +272 to +273
describe = "The amount of tcp connections that were failures",
describe_unchecked

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.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

HELP text says "tcp" instead of "tls" for TlsConnectionFailed.

This describes a TLS-connection counter (carbide_bmc_proxy_tls_connection_fail_total), but the text reads "tcp connections" — inconsistent with the sibling TlsConnectionAttempted/TlsConnectionSucceeded describes just above, which correctly say "tls connections." This text also becomes the catalogue Description column.

✏️ Proposed fix
-    describe = "The amount of tcp connections that were failures",
+    describe = "The amount of tls connections that were failures",
     describe_unchecked
📝 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.

Suggested change
describe = "The amount of tcp connections that were failures",
describe_unchecked
describe = "The amount of tls connections that were failures",
describe_unchecked
🤖 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/bmc-proxy/src/bmc_proxy.rs` around lines 272 - 273, Update the
describe text for TlsConnectionFailed to say “tls connections” instead of “tcp
connections,” matching the sibling TlsConnectionAttempted and
TlsConnectionSucceeded metrics; leave the metric name and other attributes
unchanged.

@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.

@lachen-nv lachen-nv 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.

LGTM

@chet chet changed the title feat: enforce the instrumentation standard at compile and CI time feat: ensure the instrumentation pattern at compile/CI time Jul 14, 2026
chet added a commit that referenced this pull request Jul 14, 2026
…ates (#3466)

`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

Signed-off-by: Chet Nichols III <chetn@nvidia.com>
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