feat(instrument): give instrumented events stable event_name identities#3581
Open
chet wants to merge 1 commit into
Open
feat(instrument): give instrumented events stable event_name identities#3581chet wants to merge 1 commit into
chet wants to merge 1 commit into
Conversation
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
🐇✨ ✅ Action performedFull review finished. |
Contributor
Summary by CodeRabbit
WalkthroughThis PR separates stable ChangesStable Event identity
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3581.docs.buildwithfern.com/infra-controller |
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/api-core/src/listener.rs (1)
174-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent
event_nameprefixing across sibling TLS events.
TlsConnectionAttempted,TlsConnectionSucceeded, andTlsConnectionFailedall use theapi_tls_connection_*prefix, butTlsCertsRefreshed(Line 174) is named justtls_certs_refreshedwithout theapi_prefix. Since these events all live in the same listener module and represent the same TLS subsystem, consider aligningTlsCertsRefreshedtoapi_tls_certs_refreshedfor consistency.✏️ Suggested fix
- event_name = "tls_certs_refreshed", + event_name = "api_tls_certs_refreshed",Also applies to: 188-189, 201-202, 231-232
🤖 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/api-core/src/listener.rs` around lines 174 - 175, Align the `event_name` values for `TlsCertsRefreshed` and the other affected sibling TLS events with the module’s `api_tls_*` prefix convention, while leaving their `metric_name` values unchanged.
🤖 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/xtask/src/event_names.rs`:
- Around line 887-893: Update cfg_truth to recognize the doctest path alongside
test, doc, and docsrs, returning Truth::False so doctest-only declarations are
excluded from production event scanning.
- Around line 717-738: Update has_production_possible_derive to recursively
inspect nested cfg_attr metadata after confirming each condition is not false,
while preserving direct derive detection and ignoring disabled branches. Add a
regression test covering nested cfg_attr conditions that eventually contain
derive(Event), ensuring the declaration reaches the uniqueness scan.
In `@docs/observability/instrumentation.md`:
- Around line 228-231: In the explanatory paragraph about the Prometheus
exporter and metric_name, replace “the two cancel out to exactly the attribute
string” with wording that uses “cancel” without “out,” preserving the existing
meaning and documentation content.
---
Nitpick comments:
In `@crates/api-core/src/listener.rs`:
- Around line 174-175: Align the `event_name` values for `TlsCertsRefreshed` and
the other affected sibling TLS events with the module’s `api_tls_*` prefix
convention, while leaving their `metric_name` values unchanged.
🪄 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: cffd9af7-604a-4dd5-aa2e-549b77e34898
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
.github/workflows/ci.yamlAGENTS.mdMakefile.tomlSTYLE_GUIDE.mdcrates/api-core/src/attestation/measured_boot.rscrates/api-core/src/auth/middleware.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/listener.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/machine_validation/mod.rscrates/api-core/src/measured_boot/metrics_collector/mod.rscrates/api-core/src/secrets/metrics.rscrates/authn/src/middleware.rscrates/bmc-proxy/src/bmc_proxy.rscrates/bmc-proxy/src/metrics.rscrates/certs/src/cert_renewal.rscrates/dhcp-server/src/metrics.rscrates/dhcp/src/metrics.rscrates/dns/src/lib.rscrates/dsx-exchange-consumer/src/metrics.rscrates/firmware/src/downloader.rscrates/health/src/otlp/mod.rscrates/health/src/sink/mod.rscrates/instrument-macros/Cargo.tomlcrates/instrument-macros/src/lib.rscrates/instrument/Cargo.tomlcrates/instrument/src/lib.rscrates/instrument/src/testing.rscrates/instrument/tests/matrix.rscrates/ipmi/src/metrics.rscrates/machine-controller/src/handler.rscrates/mqtt-common/src/metrics.rscrates/mqttea/src/client/core.rscrates/observability-schema/Cargo.tomlcrates/observability-schema/src/lib.rscrates/preingestion-manager/src/metrics.rscrates/pxe/src/metrics.rscrates/scout/src/metrics.rscrates/secrets/src/forge_vault.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/utils/src/managed_loop.rscrates/xtask/Cargo.tomlcrates/xtask/src/event_names.rscrates/xtask/src/main.rsdocs/observability/instrumentation.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/xtask/src/event_names.rs`:
- Around line 100-104: Update the TargetRoot construction in the target mapping
to convert target.src_path from camino::Utf8PathBuf into the expected
std::path::PathBuf, using an explicit conversion such as into_std_path_buf or
into while preserving the existing cloned path value.
- Around line 573-613: Update NestedItemVisitor’s expression and match-arm
traversal to check cfg_is_production_possible before recursing into
attribute-gated subtrees, preventing inactive nested Events from being scanned;
preserve traversal for production-possible nodes and add a regression test
proving #[cfg(test)] nested Events are ignored without duplicate-name
diagnostics.
🪄 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: 46ca98e4-e243-4667-afa0-28d6ac5cb230
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
.github/workflows/ci.yamlAGENTS.mdMakefile.tomlSTYLE_GUIDE.mdcrates/api-core/src/attestation/measured_boot.rscrates/api-core/src/auth/middleware.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/listener.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/machine_validation/mod.rscrates/api-core/src/measured_boot/metrics_collector/mod.rscrates/api-core/src/secrets/metrics.rscrates/authn/src/middleware.rscrates/bmc-proxy/src/bmc_proxy.rscrates/bmc-proxy/src/metrics.rscrates/certs/src/cert_renewal.rscrates/dhcp-server/src/metrics.rscrates/dhcp/src/metrics.rscrates/dns/src/lib.rscrates/dsx-exchange-consumer/src/metrics.rscrates/firmware/src/downloader.rscrates/health/src/otlp/mod.rscrates/health/src/sink/mod.rscrates/instrument-macros/Cargo.tomlcrates/instrument-macros/src/lib.rscrates/instrument/Cargo.tomlcrates/instrument/src/lib.rscrates/instrument/src/testing.rscrates/instrument/tests/matrix.rscrates/ipmi/src/metrics.rscrates/machine-controller/src/handler.rscrates/mqtt-common/src/metrics.rscrates/mqttea/src/client/core.rscrates/observability-schema/Cargo.tomlcrates/observability-schema/src/lib.rscrates/preingestion-manager/src/metrics.rscrates/pxe/src/metrics.rscrates/scout/src/metrics.rscrates/secrets/src/forge_vault.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/utils/src/managed_loop.rscrates/xtask/Cargo.tomlcrates/xtask/src/event_names.rscrates/xtask/src/main.rsdocs/observability/instrumentation.md
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
(๑˃̵ᴗ˂̵)ノ✨🐇 ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/observability/instrumentation.md (1)
184-192: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign both metric-backed examples with the
describecontract.Both examples declare metrics but omit
describe, contradicting the documentation’s requirement that every metric provide HELP/catalogue text.
docs/observability/instrumentation.md#L184-L192: add a histogram description such asdescribe = "BFB copy duration.".AGENTS.md#L185-L187: adddescribe = "Power control has failed.".As per path instructions, Markdown examples must be technically correct and realistic.
🤖 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 184 - 192, Update the metric-backed examples to include required HELP text via the derive attribute’s describe field: in docs/observability/instrumentation.md lines 184-192, add a histogram description such as “BFB copy duration.” to the bfb_copy_finished event; in AGENTS.md lines 185-187, add “Power control has failed.” to the corresponding power-control metric example.Source: Path instructions
crates/instrument-macros/src/lib.rs (1)
394-403: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject conflicting units on unchecked histograms.
With
metric_name_unchecked, a_secondsname plusunit = "ms"is accepted, and the explicit unit wins. Duration observations are then recorded in milliseconds under a metric named as seconds.Proposed fix
+ if let (Some(explicit), Some(from_suffix)) = (&args.unit, histogram_unit) + && explicit.value() != from_suffix + { + return Err(syn::Error::new_spanned( + explicit, + "explicit unit conflicts with the metric_name suffix", + )); + } let unit_value: String = match (&args.unit, histogram_unit) {Add a regression case covering the conflicting declaration. As per coding guidelines, histogram metric names must carry their units and existing metric names must not change.
🤖 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-macros/src/lib.rs` around lines 394 - 403, Update the histogram unit validation around unit_value so metric_name_unchecked rejects an explicit unit that conflicts with a recognized unit suffix, rather than letting the explicit unit override it. Preserve accepted matching declarations and existing metric names, and add a regression case covering a suffixed histogram such as _seconds with a conflicting explicit unit such as ms.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.
Outside diff comments:
In `@crates/instrument-macros/src/lib.rs`:
- Around line 394-403: Update the histogram unit validation around unit_value so
metric_name_unchecked rejects an explicit unit that conflicts with a recognized
unit suffix, rather than letting the explicit unit override it. Preserve
accepted matching declarations and existing metric names, and add a regression
case covering a suffixed histogram such as _seconds with a conflicting explicit
unit such as ms.
In `@docs/observability/instrumentation.md`:
- Around line 184-192: Update the metric-backed examples to include required
HELP text via the derive attribute’s describe field: in
docs/observability/instrumentation.md lines 184-192, add a histogram description
such as “BFB copy duration.” to the bfb_copy_finished event; in AGENTS.md lines
185-187, add “Power control has failed.” to the corresponding power-control
metric example.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: df34b54b-bb30-43ae-a233-061a91838d53
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
.github/workflows/ci.yamlAGENTS.mdMakefile.tomlSTYLE_GUIDE.mdcrates/api-core/src/attestation/measured_boot.rscrates/api-core/src/auth/middleware.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/listener.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/machine_validation/mod.rscrates/api-core/src/measured_boot/metrics_collector/mod.rscrates/api-core/src/secrets/metrics.rscrates/authn/src/middleware.rscrates/bmc-proxy/src/bmc_proxy.rscrates/bmc-proxy/src/metrics.rscrates/certs/src/cert_renewal.rscrates/dhcp-server/src/metrics.rscrates/dhcp/src/metrics.rscrates/dns/src/lib.rscrates/dsx-exchange-consumer/src/metrics.rscrates/firmware/src/downloader.rscrates/health/src/otlp/mod.rscrates/health/src/sink/mod.rscrates/instrument-macros/Cargo.tomlcrates/instrument-macros/src/lib.rscrates/instrument/Cargo.tomlcrates/instrument/src/lib.rscrates/instrument/src/testing.rscrates/instrument/tests/matrix.rscrates/ipmi/src/metrics.rscrates/machine-controller/src/handler.rscrates/mqtt-common/src/metrics.rscrates/mqttea/src/client/core.rscrates/observability-schema/Cargo.tomlcrates/observability-schema/src/lib.rscrates/preingestion-manager/src/metrics.rscrates/pxe/src/metrics.rscrates/scout/src/metrics.rscrates/secrets/src/forge_vault.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/utils/src/managed_loop.rscrates/xtask/Cargo.tomlcrates/xtask/src/event_names.rscrates/xtask/src/main.rsdocs/observability/instrumentation.md
lachen-nv
approved these changes
Jul 15, 2026
Contributor
|
How is connected to #3221 ? Do we need complex validation part if the generation will be implemented? |
Split typed Event identity from its optional Prometheus metric name and emit both as structured fields when an Event logs. Migrate all 59 production Events to unique semantic names, preserve existing metric contracts, and enforce the shared grammar, reserved fields, and workspace uniqueness in CI. This supports NVIDIA#3576 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
🐇✨ ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/instrument/src/lib.rs (1)
351-363: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCentralize metric suffix normalization.
crates/instrument-macros/src/lib.rsalready owns the histogram suffix mapping used during validation, while this runtime path duplicates it. If either mapping changes independently, an event can compile successfully but register under a name inconsistent with its declaredmetric_name. Move the conversion into one shared helper and cover each supported suffix with an end-to-end test.Also applies to: 370-381
🤖 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 351 - 363, The runtime suffix normalization in new_instrument duplicates the mapping maintained by the instrument macros. Centralize conversion in a shared helper owned by the macros/shared layer, reuse it for counter and histogram registration, and remove the duplicate mapping from new_instrument. Add an end-to-end test covering every supported suffix and verifying the registered metric name matches the declared metric_name.
🤖 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/instrument/src/lib.rs`:
- Around line 351-363: The runtime suffix normalization in new_instrument
duplicates the mapping maintained by the instrument macros. Centralize
conversion in a shared helper owned by the macros/shared layer, reuse it for
counter and histogram registration, and remove the duplicate mapping from
new_instrument. Add an end-to-end test covering every supported suffix and
verifying the registered metric name matches the declared metric_name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ce053b3a-b4cf-4770-934b-a40e6aa4d334
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
.github/workflows/ci.yamlAGENTS.mdMakefile.tomlSTYLE_GUIDE.mdcrates/api-core/src/attestation/measured_boot.rscrates/api-core/src/auth/middleware.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/listener.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/machine_validation/mod.rscrates/api-core/src/measured_boot/metrics_collector/mod.rscrates/api-core/src/secrets/metrics.rscrates/authn/src/middleware.rscrates/bmc-proxy/src/bmc_proxy.rscrates/bmc-proxy/src/metrics.rscrates/certs/src/cert_renewal.rscrates/dhcp-server/src/metrics.rscrates/dhcp/src/metrics.rscrates/dns/src/lib.rscrates/dsx-exchange-consumer/src/metrics.rscrates/firmware/src/downloader.rscrates/health/src/otlp/mod.rscrates/health/src/sink/mod.rscrates/instrument-macros/Cargo.tomlcrates/instrument-macros/src/lib.rscrates/instrument/Cargo.tomlcrates/instrument/src/lib.rscrates/instrument/src/testing.rscrates/instrument/tests/matrix.rscrates/ipmi/src/metrics.rscrates/machine-controller/src/handler.rscrates/mqtt-common/src/metrics.rscrates/mqttea/src/client/core.rscrates/observability-schema/Cargo.tomlcrates/observability-schema/src/lib.rscrates/preingestion-manager/src/metrics.rscrates/pxe/src/metrics.rscrates/scout/src/metrics.rscrates/secrets/src/forge_vault.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/utils/src/managed_loop.rs
🚧 Files skipped from review as they are similar to previous changes (35)
- .github/workflows/ci.yaml
- crates/scout/src/metrics.rs
- crates/bmc-proxy/src/metrics.rs
- crates/site-explorer/src/bmc_endpoint_explorer.rs
- crates/pxe/src/metrics.rs
- crates/api-core/src/attestation/measured_boot.rs
- crates/authn/src/middleware.rs
- crates/api-core/src/machine_validation/mod.rs
- crates/ipmi/src/metrics.rs
- crates/api-core/src/auth/middleware.rs
- crates/api-core/src/machine_update_manager/metrics.rs
- crates/api-core/src/secrets/metrics.rs
- crates/instrument/Cargo.toml
- crates/api-core/src/measured_boot/metrics_collector/mod.rs
- crates/mqttea/src/client/core.rs
- crates/health/src/sink/mod.rs
- crates/dhcp-server/src/metrics.rs
- crates/preingestion-manager/src/metrics.rs
- AGENTS.md
- crates/mqtt-common/src/metrics.rs
- crates/dns/src/lib.rs
- crates/observability-schema/src/lib.rs
- crates/instrument/src/testing.rs
- crates/certs/src/cert_renewal.rs
- Makefile.toml
- crates/utils/src/managed_loop.rs
- crates/secrets/src/forge_vault.rs
- crates/machine-controller/src/handler.rs
- crates/api-core/src/listener.rs
- STYLE_GUIDE.md
- crates/health/src/otlp/mod.rs
- crates/bmc-proxy/src/bmc_proxy.rs
- crates/dhcp/src/metrics.rs
- crates/instrument/tests/matrix.rs
- crates/instrument-macros/src/lib.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-macros/src/lib.rs`:
- Around line 157-160: Update the attribute parsing branches for event_name and
metric_name to reject a key when its corresponding args field is already
populated, rather than overwriting the earlier value. Return an appropriate
duplicate-attribute parse error and preserve the existing parsing behavior for
the first occurrence; apply this within the metadata parsing logic containing
args.event_name and args.metric_name.
In `@crates/xtask/src/event_names.rs`:
- Around line 988-1007: Update is_test_or_bench to recursively traverse nested
cfg_attr metadata, reusing the existing recursive cfg_attr traversal helper used
elsewhere instead of only checking direct arguments. Ensure nested conditions
such as cfg_attr(not(test), cfg_attr(not(doc), test)) classify the item as
test/bench, and add a regression case covering this chain.
In `@docs/observability/instrumentation.md`:
- Around line 19-22: Update the observability documentation sentence describing
#[context] fields so it states they appear only when the Event emits a log
record, rather than unconditionally appearing on a log line. Preserve the
clarification that these fields cannot become metric labels and ensure
metric-only Events with log = off are excluded.
🪄 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: ff882d7c-8221-47ec-90c4-9f4a766d4d3e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
.github/workflows/ci.yamlAGENTS.mdMakefile.tomlSTYLE_GUIDE.mdcrates/api-core/src/attestation/measured_boot.rscrates/api-core/src/auth/middleware.rscrates/api-core/src/handlers/utils.rscrates/api-core/src/listener.rscrates/api-core/src/machine_update_manager/metrics.rscrates/api-core/src/machine_validation/mod.rscrates/api-core/src/measured_boot/metrics_collector/mod.rscrates/api-core/src/secrets/metrics.rscrates/authn/src/middleware.rscrates/bmc-proxy/src/bmc_proxy.rscrates/bmc-proxy/src/metrics.rscrates/certs/src/cert_renewal.rscrates/dhcp-server/src/metrics.rscrates/dhcp/src/metrics.rscrates/dns/src/lib.rscrates/dsx-exchange-consumer/src/metrics.rscrates/firmware/src/downloader.rscrates/health/src/otlp/mod.rscrates/health/src/sink/mod.rscrates/instrument-macros/Cargo.tomlcrates/instrument-macros/src/lib.rscrates/instrument/Cargo.tomlcrates/instrument/src/lib.rscrates/instrument/src/testing.rscrates/instrument/tests/matrix.rscrates/ipmi/src/metrics.rscrates/machine-controller/src/handler.rscrates/mqtt-common/src/metrics.rscrates/mqttea/src/client/core.rscrates/observability-schema/Cargo.tomlcrates/observability-schema/src/lib.rscrates/preingestion-manager/src/metrics.rscrates/pxe/src/metrics.rscrates/scout/src/metrics.rscrates/secrets/src/forge_vault.rscrates/site-explorer/src/bmc_endpoint_explorer.rscrates/utils/src/managed_loop.rscrates/xtask/Cargo.tomlcrates/xtask/src/event_names.rscrates/xtask/src/main.rsdocs/observability/instrumentation.md
Comment on lines
+157
to
+160
| if meta.path.is_ident("event_name") { | ||
| args.event_name = Some(meta.value()?.parse()?); | ||
| } else if meta.path.is_ident("metric_name") { | ||
| args.metric_name = Some(meta.value()?.parse()?); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject duplicate identity attributes.
Repeated event_name or metric_name keys silently overwrite the earlier value, making the generated identity depend on attribute order. Reject duplicates here; the production checker already rejects duplicate event_name declarations.
🤖 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-macros/src/lib.rs` around lines 157 - 160, Update the
attribute parsing branches for event_name and metric_name to reject a key when
its corresponding args field is already populated, rather than overwriting the
earlier value. Return an appropriate duplicate-attribute parse error and
preserve the existing parsing behavior for the first occurrence; apply this
within the metadata parsing logic containing args.event_name and
args.metric_name.
Comment on lines
+988
to
+1007
| fn is_test_or_bench(attrs: &[Attribute]) -> bool { | ||
| attrs.iter().any(|attr| { | ||
| if path_ends_in_test_or_bench(attr.path()) { | ||
| return true; | ||
| } | ||
| if !attr.path().is_ident("cfg_attr") { | ||
| return false; | ||
| } | ||
|
|
||
| let Ok(parts) = attr.parse_args_with(Punctuated::<Meta, Token![,]>::parse_terminated) | ||
| else { | ||
| return false; | ||
| }; | ||
| let mut parts = parts.iter(); | ||
| let Some(condition) = parts.next() else { | ||
| return false; | ||
| }; | ||
| cfg_truth(condition) == Truth::True | ||
| && parts.any(|meta| path_ends_in_test_or_bench(meta.path())) | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '982,1015p' crates/xtask/src/event_names.rs
rg -n 'cfg_attr\(.*cfg_attr\(.*(test|bench)' crates/xtask/src/event_names.rsRepository: NVIDIA/infra-controller
Length of output: 1252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== outline =="
ast-grep outline crates/xtask/src/event_names.rs --view expanded || true
echo
echo "== relevant symbols =="
rg -n "fn (cfg_attribute_truth|cfg_truth|is_test_or_bench|cfg_is_production_possible|path_ends_in_test_or_bench)|cfg_attr" crates/xtask/src/event_names.rs
echo
echo "== tests mentioning test/bench/cfg_attr =="
rg -n "test|bench|cfg_attr|doctest|Event" crates/xtask/src/event_names.rsRepository: NVIDIA/infra-controller
Length of output: 18986
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "cfg_attr_has_active_test_or_bench|path_ends_in_test_or_bench|cfg_attribute_truth|cfg_truth" crates/xtask/srcRepository: NVIDIA/infra-controller
Length of output: 1877
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
p = Path("crates/xtask/src/event_names.rs")
text = p.read_text()
for name in ["cfg_attribute_truth", "cfg_truth", "is_test_or_bench", "path_ends_in_test_or_bench"]:
m = re.search(rf"fn {name}[^{{]*\{{", text)
if m:
start = m.start()
print(f"\n--- {name} ---")
# print a compact slice around the function
print(text[start:start+1500])
PYRepository: NVIDIA/infra-controller
Length of output: 6275
Recurse through nested cfg_attr when classifying test/bench items.
Nested chains like #[cfg_attr(not(test), cfg_attr(not(doc), test))] still fall through this check, so test-only bodies can be scanned and emit duplicate-event diagnostics. Reuse the recursive cfg_attr traversal already used elsewhere and add a regression case.
🤖 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/event_names.rs` around lines 988 - 1007, Update
is_test_or_bench to recursively traverse nested cfg_attr metadata, reusing the
existing recursive cfg_attr traversal helper used elsewhere instead of only
checking direct arguments. Ensure nested conditions such as cfg_attr(not(test),
cfg_attr(not(doc), test)) classify the item as test/bench, and add a regression
case covering this chain.
Comment on lines
19
to
22
| - **Cardinality is enforced by the types.** `#[label]` fields must be bounded via | ||
| `LabelValue` -- usually a fieldless enum, with a manual impl on a bounded newtype as the | ||
| reviewed escape hatch; high-cardinality detail (`machine_id`, IPs, error text) goes in | ||
| `#[context]` fields, which appear on the log line only and *cannot* become a metric label. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify when #[context] fields are emitted.
This says context fields “appear on the log line only,” but log = off Events produce no log line. Specify that they appear only when the Event emits a log record, so metric-only Events are not documented as rendering those fields.
Suggested wording
- `#[context]` fields, which appear on the log line only and *cannot* become a metric label.
+ `#[context]` fields, which appear only when the Event emits a log line and *cannot*
+ become metric labels.As per path instructions, Markdown must be reviewed for technical correctness and clarity.
📝 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
| - **Cardinality is enforced by the types.** `#[label]` fields must be bounded via | |
| `LabelValue` -- usually a fieldless enum, with a manual impl on a bounded newtype as the | |
| reviewed escape hatch; high-cardinality detail (`machine_id`, IPs, error text) goes in | |
| `#[context]` fields, which appear on the log line only and *cannot* become a metric label. | |
| - **Cardinality is enforced by the types.** `#[label]` fields must be bounded via | |
| `LabelValue` -- usually a fieldless enum, with a manual impl on a bounded newtype as the | |
| reviewed escape hatch; high-cardinality detail (`machine_id`, IPs, error text) goes in | |
| `#[context]` fields, which appear only when the Event emits a log line and *cannot* | |
| become metric labels. |
🤖 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 19 - 22, Update the
observability documentation sentence describing #[context] fields so it states
they appear only when the Event emits a log record, rather than unconditionally
appearing on a log line. Preserve the clarification that these fields cannot
become metric labels and ensure metric-only Events with log = off are excluded.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
This gives every
carbide_instrument::Eventa stable semanticevent_name, separate from its optional Prometheusmetric_name. When a typed Event logs, those identities now show up as structured fields, and the tracing metadata name uses the sameevent_name.Big picture: this starts establishing common, stable identities we can use for searchability, docs, and future schemas instead of inferring identity from messages or Rust type names.
event_namevaluescargo xtask check-event-namesto pre-commit and CI so duplicate or invalid production identities fail closedevent_name,metric_name, messages, plain tracing, and metric cardinality fit togetherVerification
cargo make format-nightlycargo make check-format-nightlycargo make clippycargo make carbide-lintscargo make check-workspace-depscargo make check-event-namescargo test -p carbide-instrument -p carbide-observability-schema -p xtask -p carbide-dsx-exchange-consumer --all-featuresThis supports #3576