feat: ensure the instrumentation pattern at compile/CI time#3463
Conversation
|
@coderabbitai full_review, thanks! |
|
ヽ(・∀・)ﾉ ✅ Action performedFull review finished. |
Summary by CodeRabbit
WalkthroughThe PR adds counter metadata validation, introduces the 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 declarations
Xtask->>CoreMetricsCatalogue: compare metric names with catalogue rows
CoreMetricsCatalogue-->>Xtask: catalogue contents
Xtask-->>CI: success or missing-metric diagnostics
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 |
chet
changed the title
…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_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 rules have an escape hatch (`describe_unchecked`, alongside the existing `name_unchecked`) for a describe or exposed name that must stay as it is. No counter needs `describe_unchecked` today -- every existing describe already opens with "Number of " -- so the hatch stands ready for a future exception rather than papering over current ones; events whose exposed name is an OTel-sanitized transform still 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-nine 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
chet
force-pushed
the
gh-issue-3180-docgate
branch
from
c1b49b2 to
849b42d
Compare
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/xtask/src/metric_docs.rs (1)
333-444: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the four scenario tests into a table, and cover the fail-closed paths.
raw_string_name_is_collected,cooked_string_name_is_collected,cfg_test_events_are_excluded, andnon_metric_and_grandfathered_are_skippedall invoke the same operation (declared(src)) with different source snippets and assert on the resulting name list — a canonical case forcarbide-test-support'svalue_scenarios!/check_valuestable rather than four standalone#[test]fns.Separately, the module's stated guarantee is to "fail closed... never a silent skip" (lines 33-35), yet none of the tests exercise the two
bail!/error paths that are the whole point of that guarantee: an#[event(...)]with an unrecognized key (line 204) and a counter/histogram with no readablename(lines 219-225).metric = histogramis also untested. Please add coverage for these before merge — they're the safety net this gate exists to provide.Based on coding guidelines for
**/*.{rs,rs.in}: "When writing tests, prefer table-driven coverage, using groupedcarbide-test-supportscenarios such asscenarios!/value_scenarios!or explicitcheck_cases/check_valuesfor input variants."🤖 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, Consolidate the four standalone tests around declared into one table-driven value_scenarios!/check_values test, preserving each expected metric-name result. Extend the scenarios to cover metric = histogram, an unrecognized event key, and a counter/histogram event without a readable name. Assert that the latter two inputs return the existing collect_items error instead of being silently skipped, using the module’s established fail-closed behavior.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/xtask/src/metric_docs.rs`:
- Around line 333-444: Consolidate the four standalone tests around declared
into one table-driven value_scenarios!/check_values test, preserving each
expected metric-name result. Extend the scenarios to cover metric = histogram,
an unrecognized event key, and a counter/histogram event without a readable
name. Assert that the latter two inputs return the existing collect_items error
instead of being silently skipped, using the module’s established fail-closed
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 652f6d86-6ef4-43d1-a839-e5b3126952a1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/ci.yamlMakefile.tomlSTYLE_GUIDE.mdcrates/instrument-macros/src/lib.rscrates/instrument/src/lib.rscrates/instrument/src/testing.rscrates/instrument/tests/matrix.rscrates/xtask/Cargo.tomlcrates/xtask/src/main.rscrates/xtask/src/metric_docs.rsdocs/observability/core_metrics.mddocs/observability/instrumentation.md
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/xtask/Cargo.toml
- crates/instrument/tests/matrix.rs
- crates/xtask/src/main.rs
- crates/instrument/src/testing.rs
- docs/observability/instrumentation.md
- Makefile.toml
- STYLE_GUIDE.md
- docs/observability/core_metrics.md
- crates/instrument/src/lib.rs
- crates/instrument-macros/src/lib.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
crates/instrument/src/lib.rs (1)
139-141: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMisleading counter-suffix wording has resurfaced.
This phrasing implies any
_total-suffixed name is rejected, but the macro (crates/instrument-macros/src/lib.rs Lines 292-313) requires exactly one_totalsuffix and only rejects the doubled_total_totalform. This exact wording was previously flagged and reportedly fixed in an earlier commit, yet it is back in this revision.✏️ Proposed wording
-//! And a counter name must not carry `_total` itself -- the Prometheus exporter -//! appends it, so a `_total`-suffixed instrument name ships a doubled -//! `_total_total` series: +//! Counter names must end in `_total`, but must not end in `_total_total`. +//! Appending the suffix twice produces a doubled `_total_total` series:🤖 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/src/lib.rs` around lines 139 - 141, Update the documentation comment near the instrument naming rules to state that counter names must contain exactly one `_total` suffix and that only the doubled `_total_total` form is rejected, matching the validation in the instrument macro’s counter-name handling.docs/observability/instrumentation.md (2)
219-223: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLog-only "any
describe" claim contradicts the derive's validation.
crates/instrument-macros/src/lib.rs(Lines 340-348) unconditionally rejectsdescribewhenmetric = none, so log-only events must omitdescribeentirely — they cannot take "any" value. Only histogram descriptions are actually unrestricted. This mismatch was previously flagged for this exact range but has reappeared unchanged.As per path instructions,
docs/**documentation must stay "consistent with current deployment and CLI behavior."✏️ Proposed wording
-… Histograms and log-only events take any `describe` (or none). +… Histograms take any `describe`; log-only events (`metric = none`) must omit `describe` entirely.🤖 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 219 - 223, Update the documentation paragraph describing `describe` in log-only events so it states that `metric = none` must omit `describe`, while histogram descriptions remain unrestricted. Preserve the existing counter requirements and `describe_unchecked` guidance.Source: Path instructions
225-228: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCatalogue-check description omits the
name_uncheckedexemption in both documents. Per the PR objectives, OTel-sanitized names usingname_uncheckedare temporarily exempt fromcargo xtask check-metric-docs, but both documents state the check applies to "any/every" framework counter or histogram without qualifying that exception — a gap already raised in a prior review and still unresolved.
docs/observability/instrumentation.md#L225-L228: qualify "fails if any framework counter or histogram is missing a catalogue row" with thename_uncheckedexemption.STYLE_GUIDE.md#L173-L175: qualify "every framework counter and histogram must appear there" with the same exemption.As per path instructions,
docs/**and**/*.mddocumentation must stay accurate and "consistent with current deployment and CLI behavior."🤖 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 the catalogue-check descriptions to state that framework counters and histograms must have catalogue rows except for OTel-sanitized names using name_unchecked. Apply this qualification in docs/observability/instrumentation.md lines 225-228 and STYLE_GUIDE.md lines 173-175, keeping both documents consistent with the current check behavior.Source: Path instructions
🧹 Nitpick comments (2)
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()andworkspace_deps::check()both surface failure througheyre::Result/CheckOutcomesomain's?drives the exit path;metric_docs::check()instead callsstd::process::exit(1)directly after printing its own diagnostics. This makes the function untestable as aResult-returning unit and inconsistent with the established pattern in this crate.♻️ Proposed refactor: propagate via `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 message = format!( + "{} framework metric(s) are declared with #[derive(Event)] but missing from \ + {CATALOGUE}:\n", + missing.len(), + ); + for m in &missing { + message.push_str(&format!(" {} ({}) -- {}\n", m.name, m.kind, m.source.display())); + message.push_str(&format!(" {}\n", suggested_row(m))); + } + message.push_str( + "\nEvery counter/histogram declared through the instrumentation framework needs a row \ + in the catalogue, so the metric is documented even when no test scrapes it. \ + (name_unchecked events are exempt -- see `#3221`.)", + ); + bail!(message);🤖 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, Update metric_docs::check() to propagate the missing-metrics failure with the crate’s eyre::bail! pattern instead of calling std::process::exit(1). Preserve the existing diagnostic output and successful Ok(()) path, while returning the failure through the Result so the caller’s ? controls process termination and the check remains testable.
317-331: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
askama_escape::Htmlhere
The workspace already depends onaskama_escape, and this helper duplicates the same escaping rules. Calling the shared escaper keeps the catalog output aligned with the generator and removes a second implementation.🤖 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 317 - 331, Replace the custom escaping logic in html_escape with the existing askama_escape::Html implementation. Reuse the shared escaper for the input string, preserve html_escape’s String return type, and remove the now-unnecessary manual character-matching loop.
🤖 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 `@Makefile.toml`:
- Around line 822-826: Verify that Makefile.toml declares the
[tasks.check-metric-docs] table only once; remove any duplicate header or
duplicate task definition while preserving the existing workspace, description,
and script settings.
---
Duplicate comments:
In `@crates/instrument/src/lib.rs`:
- Around line 139-141: Update the documentation comment near the instrument
naming rules to state that counter names must contain exactly one `_total`
suffix and that only the doubled `_total_total` form is rejected, matching the
validation in the instrument macro’s counter-name handling.
In `@docs/observability/instrumentation.md`:
- Around line 219-223: Update the documentation paragraph describing `describe`
in log-only events so it states that `metric = none` must omit `describe`, while
histogram descriptions remain unrestricted. Preserve the existing counter
requirements and `describe_unchecked` guidance.
- Around line 225-228: Update the catalogue-check descriptions to state that
framework counters and histograms must have catalogue rows except for
OTel-sanitized names using name_unchecked. Apply this qualification in
docs/observability/instrumentation.md lines 225-228 and STYLE_GUIDE.md lines
173-175, keeping both documents consistent with the current check behavior.
---
Nitpick comments:
In `@crates/xtask/src/metric_docs.rs`:
- Around line 77-102: Update metric_docs::check() to propagate the
missing-metrics failure with the crate’s eyre::bail! pattern instead of calling
std::process::exit(1). Preserve the existing diagnostic output and successful
Ok(()) path, while returning the failure through the Result so the caller’s ?
controls process termination and the check remains testable.
- Around line 317-331: Replace the custom escaping logic in html_escape with the
existing askama_escape::Html implementation. Reuse the shared escaper for the
input string, preserve html_escape’s String return type, and remove the
now-unnecessary manual character-matching loop.
🪄 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: 7d4ee381-a95e-4136-bfc4-e417f4ad8b94
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/ci.yamlMakefile.tomlSTYLE_GUIDE.mdcrates/instrument-macros/src/lib.rscrates/instrument/src/lib.rscrates/instrument/src/testing.rscrates/instrument/tests/matrix.rscrates/xtask/Cargo.tomlcrates/xtask/src/main.rscrates/xtask/src/metric_docs.rsdocs/observability/core_metrics.mddocs/observability/instrumentation.md
| [tasks.check-metric-docs] | ||
| workspace = false | ||
| description = "Fail if a #[derive(Event)] counter/histogram is missing a row in docs/observability/core_metrics.md" | ||
| script = ["cargo --quiet xtask check-metric-docs"] | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Confirm [tasks.check-metric-docs] is not declared twice.
The provided snippet shows the [tasks.check-metric-docs] header rendered twice back-to-back (both anchored at Line 822). A duplicate TOML table header is a parse error and would break cargo make entirely, so this needs confirming before merge — it may simply be a diff-rendering artifact, but the risk of a build-breaking duplicate table justifies a quick check.
#!/bin/bash
grep -n 'tasks.check-metric-docs' Makefile.toml🤖 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 `@Makefile.toml` around lines 822 - 826, Verify that Makefile.toml declares the
[tasks.check-metric-docs] table only once; remove any duplicate header or
duplicate task definition while preserving the existing workspace, description,
and script settings.
2 participants
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 rules have an escape hatch (describe_unchecked, alongsidename_unchecked) for a describe or name that must stay as it is -- but no counter needsdescribe_uncheckedtoday: every existing describe already opens with "Number of ", so the hatch stands ready for a future exception rather than papering over current ones.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-nine 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.This supports #3180