feat: ensure the instrumentation pattern at compile/CI time#3463
Conversation
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>
|
@coderabbitai full_review, thanks! |
|
ヽ(・∀・)ノ ✅ Action performedFull review finished. |
WalkthroughThe 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. ChangesMetric instrumentation validation
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
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3463.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
crates/instrument/tests/matrix.rs (1)
49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep ordinary test fixtures on the checked metadata path.
These counters use
describe_uncheckedwithout anydescribe, so the tests no longer exercise the required counter HELP-text validation. Add validdescribe = "Number of ..."values and reservedescribe_uncheckedfor 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 winPrefer
bail!overstd::process::exit(1)for consistency with sibling xtask checks.
isolated_package_builds::check()reports failure viabail!(...), lettingmain()'s?propagate it through the sameeyre::Resultpath used everywhere else in this crate. Callingstd::process::exit(1)directly here instead makes theeyre::Result<()>return type misleading (this function effectively never returnsErr) and forecloses future reuse/testing ofcheck()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 winWalkDir errors are silently discarded, contradicting the file's fail-closed design.
.filter_map(Result::ok)drops anywalkdir::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
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.github/workflows/ci.yamlMakefile.tomlSTYLE_GUIDE.mdcrates/api-core/src/handlers/utils.rscrates/api-core/src/listener.rscrates/api-core/src/secrets/metrics.rscrates/bmc-proxy/src/bmc_proxy.rscrates/dns/src/lib.rscrates/dsx-exchange-consumer/src/metrics.rscrates/instrument-macros/src/lib.rscrates/instrument/src/lib.rscrates/instrument/src/testing.rscrates/instrument/tests/matrix.rscrates/mqtt-common/src/metrics.rscrates/secrets/src/forge_vault.rscrates/xtask/Cargo.tomlcrates/xtask/src/main.rscrates/xtask/src/metric_docs.rsdocs/observability/core_metrics.mddocs/observability/instrumentation.md
| 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. |
There was a problem hiding this comment.
📐 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 thename_uncheckedexception 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
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/instrument/tests/matrix.rs (1)
49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep test fixtures inside the counter contract.
These test counters use
describe_uncheckedwithout anydescribe, bypassing the new required-help-text validation. Add a canonicaldescribe = "Number of ..."to each fixture, reservingdescribe_uncheckedfor 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 winUse
bail!instead ofstd::process::exit(1)for consistency with the sibling check.
isolated_package_builds::check()in the same crate signals failure by returningErrviabail!, lettingmain's?propagate the exit code. This function instead callsstd::process::exit(1)directly aftereprintln!, which is inconsistent with that established pattern and discards the message via manual output rather than the standardeyreerror 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 valueConsider 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 asdeclared(). 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
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.github/workflows/ci.yamlMakefile.tomlSTYLE_GUIDE.mdcrates/api-core/src/handlers/utils.rscrates/api-core/src/listener.rscrates/api-core/src/secrets/metrics.rscrates/bmc-proxy/src/bmc_proxy.rscrates/dns/src/lib.rscrates/dsx-exchange-consumer/src/metrics.rscrates/instrument-macros/src/lib.rscrates/instrument/src/lib.rscrates/instrument/src/testing.rscrates/instrument/tests/matrix.rscrates/mqtt-common/src/metrics.rscrates/secrets/src/forge_vault.rscrates/xtask/Cargo.tomlcrates/xtask/src/main.rscrates/xtask/src/metric_docs.rsdocs/observability/core_metrics.mddocs/observability/instrumentation.md
| describe = "The amount of tcp connections that were failures", | ||
| describe_unchecked |
There was a problem hiding this comment.
📐 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.
| 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.
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
…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>
Nothing stopped a new metric from landing undocumented or off-pattern -- the catalogue is regenerated from whatever
test_integrationhappens 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.#[derive(Event)]macro now requires a counter'sdescribeto open with "Number of ", and rejects a name that still ends in_totalafter the framework strips one -- the doubled-_total_totalfootgun that has already shipped a bad series. Both have an escape hatch (describe_unchecked, alongsidename_unchecked) for grandfathered cases; the sixteen existing counters whose describe predates the rule carrydescribe_uncheckedwith their text untouched.cargo xtask check-metric-docsreads every#[event(...)]declaration (viasyn, so it handles raw-string names and skips#[cfg(test)]fixtures) and fails CI in thelint-policejob if a conforming counter or histogram is missing fromcore_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.mdandinstrumentation.mdgain the now-enforced canonical pattern.name_uncheckedevents 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