feat: add custom tracing macro to use reports (external version)#1835
feat: add custom tracing macro to use reports (external version)#1835drahnr wants to merge 21 commits into0xMiden:nextfrom
Conversation
|
Shouldn't be user visible, so |
|
Open question: Do we want to prefix the function name with the component in the otel? I.e. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new miden-node-tracing runtime crate plus an internal proc-macro crate (miden-node-tracing-macro) to standardize tracing instrumentation and logging across the node, including an OpenTelemetry field-key allowlist to prevent high-cardinality attribute drift. It also updates multiple crates/binaries to adopt the new macros and adds UI-style compile tests to enforce the allowlist and macro constraints.
Changes:
- Added
miden-node-tracingandmiden-node-tracing-macrocrates with an allowlisted, component-prefixed#[instrument]attribute anderror!/warn!/info!/debug!/trace!proc-macros. - Migrated instrumentation/logging imports across validator/store/rpc/ntx-builder/block-producer and some binaries to use
miden_node_tracing. - Added trybuild UI tests and an allowlist file to enforce valid OpenTelemetry field keys at compile time.
Reviewed changes
Copilot reviewed 90 out of 103 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/validator/src/tx_validation/mod.rs | Switches to miden_node_tracing::instrument attribute usage. |
| crates/validator/src/server/mod.rs | Switches to miden_node_tracing::instrument on RPC handler(s). |
| crates/validator/src/db/mod.rs | Replaces tracing::instrument usage with new instrument macro. |
| crates/validator/src/db/migrations.rs | Replaces tracing::instrument with new instrument macro. |
| crates/validator/src/block_validation/mod.rs | Adopts new instrument macro and adjusts imports. |
| crates/validator/Cargo.toml | Adds dependency on miden-node-tracing. |
| crates/tracing/tests/ui/pass/log.rs | Adds passing UI tests for new log macros and allowlisted fields. |
| crates/tracing/tests/ui/pass/instrument.rs | Adds passing UI tests for #[instrument] keyword forms. |
| crates/tracing/tests/ui/pass/instrument_root.rs | Adds passing UI test for root keyword form. |
| crates/tracing/tests/ui/log/field_not_in_allowlist.stderr | Adds expected stderr for unallowlisted log field. |
| crates/tracing/tests/ui/log/field_not_in_allowlist.rs | Adds compile-fail UI case for unallowlisted log field. |
| crates/tracing/tests/ui/log/dotted_field_not_in_allowlist.stderr | Adds expected stderr for unallowlisted dotted log field. |
| crates/tracing/tests/ui/log/dotted_field_not_in_allowlist.rs | Adds compile-fail UI case for unallowlisted dotted log field. |
| crates/tracing/tests/ui/log/component_with_unlisted_field.stderr | Adds expected stderr for component+unallowlisted log field. |
| crates/tracing/tests/ui/log/component_with_unlisted_field.rs | Adds compile-fail UI case for component+unallowlisted log field. |
| crates/tracing/tests/ui/instrument/ret_report_requires_result.stderr | Adds expected stderr for invalid ret, report on non-Result. |
| crates/tracing/tests/ui/instrument/ret_report_requires_result.rs | Adds compile-fail case for invalid ret, report on non-Result. |
| crates/tracing/tests/ui/instrument/report_requires_result.stderr | Adds expected stderr for invalid report on non-Result. |
| crates/tracing/tests/ui/instrument/report_requires_result.rs | Adds compile-fail case for invalid report on non-Result. |
| crates/tracing/tests/ui/instrument/report_on_pin_box_future.stderr | Adds expected stderr for unsupported boxed-future return type. |
| crates/tracing/tests/ui/instrument/report_on_pin_box_future.rs | Adds compile-fail case for report on boxed-future return type. |
| crates/tracing/tests/ui/instrument/report_on_impl_future.stderr | Adds expected stderr for unsupported impl Future return type. |
| crates/tracing/tests/ui/instrument/report_on_impl_future.rs | Adds compile-fail case for report on impl Future return type. |
| crates/tracing/tests/ui/instrument/report_on_async_trait.stderr | Adds expected stderr for async-trait transformed methods. |
| crates/tracing/tests/ui/instrument/report_on_async_trait.rs | Adds compile-fail case for report under async_trait. |
| crates/tracing/tests/ui/instrument/report_and_err_exclusive.stderr | Adds expected stderr for mutually-exclusive err+report. |
| crates/tracing/tests/ui/instrument/report_and_err_exclusive.rs | Adds compile-fail case for mutually-exclusive err+report. |
| crates/tracing/tests/ui/instrument/field_not_in_allowlist.stderr | Adds expected stderr for unallowlisted instrument field. |
| crates/tracing/tests/ui/instrument/field_not_in_allowlist.rs | Adds compile-fail case for unallowlisted instrument field. |
| crates/tracing/tests/ui/instrument/err_requires_result.stderr | Adds expected stderr for invalid err on non-Result. |
| crates/tracing/tests/ui/instrument/err_requires_result.rs | Adds compile-fail case for invalid err on non-Result. |
| crates/tracing/tests/ui/instrument/err_on_async_trait.stderr | Adds expected stderr for err under async_trait. |
| crates/tracing/tests/ui/instrument/err_on_async_trait.rs | Adds compile-fail case for err under async_trait. |
| crates/tracing/tests/ui/instrument/dotted_field_not_in_allowlist.stderr | Adds expected stderr for unallowlisted dotted instrument field. |
| crates/tracing/tests/ui/instrument/dotted_field_not_in_allowlist.rs | Adds compile-fail case for unallowlisted dotted instrument field. |
| crates/tracing/tests/ui.rs | Adds trybuild harness for UI pass/fail tests. |
| crates/tracing/src/lib.rs | Adds runtime crate re-exports + crate docs for tracing helpers. |
| crates/tracing/README.md | Adds README describing new tracing crate/macro usage. |
| crates/tracing/Cargo.toml | Defines miden-node-tracing crate and dependencies/dev-deps. |
| crates/tracing-macro/src/tests.rs | Adds unit tests for proc-macro parsing/expansion behavior. |
| crates/tracing-macro/src/log.rs | Implements parsing/expansion for log proc-macros with allowlist enforcement. |
| crates/tracing-macro/src/lib.rs | Implements proc-macro entry points and extensive reference docs. |
| crates/tracing-macro/src/instrument.rs | Implements #[instrument] parsing/validation/codegen (allowlist, report/root/ret/err). |
| crates/tracing-macro/src/allowed.rs | Implements allowlist loading + fuzzy suggestions. |
| crates/tracing-macro/README.md | Adds internal macro crate README and reference docs. |
| crates/tracing-macro/Cargo.toml | Defines miden-node-tracing-macro proc-macro crate + deps. |
| crates/tracing-macro/allowlist.txt | Adds OpenTelemetry field-key allowlist used by macros. |
| crates/store/src/state/sync_state.rs | Switches to miden_node_tracing::instrument in state sync methods. |
| crates/store/src/state/mod.rs | Switches to miden_node_tracing macros/imports in state module. |
| crates/store/src/state/loader.rs | Switches to new instrument macro across storage loading paths. |
| crates/store/src/state/apply_block.rs | Switches to miden_node_tracing macros/imports in apply-block flow. |
| crates/store/src/server/rpc_api.rs | Switches logging imports to miden_node_tracing macros. |
| crates/store/src/server/ntx_builder.rs | Switches debug logging import to miden_node_tracing::debug. |
| crates/store/src/server/mod.rs | Switches to miden_node_tracing in server bootstrap instrumentation. |
| crates/store/src/server/block_prover_client.rs | Switches to miden_node_tracing::instrument for prover client spans. |
| crates/store/src/server/api.rs | Switches to miden_node_tracing macros and updates instrumentation attributes. |
| crates/store/src/db/schema_hash.rs | Switches schema verification instrumentation to new macro. |
| crates/store/src/db/models/queries/transactions.rs | Switches Diesel query instrumentation to new macro. |
| crates/store/src/db/models/queries/nullifiers.rs | Switches Diesel query instrumentation to new macro. |
| crates/store/src/db/models/queries/notes.rs | Switches Diesel query instrumentation to new macro. |
| crates/store/src/db/models/queries/block_headers.rs | Switches Diesel query instrumentation to new macro. |
| crates/store/src/db/models/queries/accounts.rs | Switches Diesel query instrumentation to new macro. |
| crates/store/src/db/mod.rs | Switches DB layer instrumentation/logging imports to new macros. |
| crates/store/src/db/migrations.rs | Switches migration instrumentation to new macro. |
| crates/store/src/blocks.rs | Switches block store instrumentation to new macro and adjusts fields. |
| crates/store/src/account_state_forest/mod.rs | Switches forest instrumentation to new macro + allowlisted field. |
| crates/store/Cargo.toml | Adds dependency on miden-node-tracing. |
| crates/rpc/src/tests.rs | Makes tests more robust by waiting for store readiness and using non-lazy connect. |
| crates/rpc/src/server/mod.rs | Switches info logging import to miden_node_tracing::info. |
| crates/rpc/src/server/api.rs | Switches debug/info logging imports to miden_node_tracing. |
| crates/rpc/Cargo.toml | Adds dependency on miden-node-tracing. |
| crates/ntx-builder/src/db/schema_hash.rs | Switches schema verification instrumentation to new macro. |
| crates/ntx-builder/src/db/mod.rs | Switches DB setup instrumentation/logging imports to new macros. |
| crates/ntx-builder/src/db/migrations.rs | Switches migrations instrumentation to new macro. |
| crates/ntx-builder/src/coordinator.rs | Switches coordinator instrumentation to new macro. |
| crates/ntx-builder/src/clients/store.rs | Switches store client spans/logging imports to new macros. |
| crates/ntx-builder/src/clients/block_producer.rs | Switches block-producer client spans/logging imports to new macros. |
| crates/ntx-builder/src/builder.rs | Switches builder-level instrumentation to new macro. |
| crates/ntx-builder/src/actor/mod.rs | Switches actor instrumentation to new macro; adjusts unused param naming. |
| crates/ntx-builder/src/actor/execute.rs | Switches execute pipeline instrumentation to new macro; adjusts imports. |
| crates/ntx-builder/Cargo.toml | Adds dependency on miden-node-tracing. |
| crates/block-producer/src/validator/mod.rs | Switches validator client spans/logging imports to new macros. |
| crates/block-producer/src/store/mod.rs | Switches store client spans/logging imports to new macros. |
| crates/block-producer/src/server/mod.rs | Switches server logging/instrumentation to new macros; tweaks error log fields. |
| crates/block-producer/src/mempool/tests.rs | Updates span assertions to match new span naming/collection behavior. |
| crates/block-producer/src/mempool/mod.rs | Switches mempool instrumentation/logging imports to new macros; updates fields. |
| crates/block-producer/src/block_builder/mod.rs | Switches block builder instrumentation to new macro; uses root keyword. |
| crates/block-producer/src/batch_builder/mod.rs | Switches batch builder instrumentation to new macro; uses root keyword. |
| crates/block-producer/Cargo.toml | Adds dependency on miden-node-tracing. |
| Cargo.toml | Adds tracing crates to workspace members and workspace dependencies. |
| Cargo.lock | Locks new crates and dependencies (fuzzy-search, tracing crates, trybuild config). |
| bin/remote-prover/src/main.rs | Switches info logging import to miden_node_tracing::info. |
| bin/remote-prover/Cargo.toml | Adds dependency on miden-node-tracing. |
| bin/network-monitor/src/status.rs | Switches debug/info imports to miden_node_tracing; keeps tracing::instrument. |
| bin/network-monitor/src/remote_prover.rs | Switches info import to miden_node_tracing; keeps tracing::instrument. |
| bin/network-monitor/src/note_transport.rs | Switches info import to miden_node_tracing; keeps tracing::instrument. |
| bin/network-monitor/src/monitor/tasks.rs | Switches debug import to miden_node_tracing; keeps tracing::instrument. |
| bin/network-monitor/src/frontend.rs | Switches info import to miden_node_tracing; keeps tracing::instrument. |
| bin/network-monitor/src/faucet.rs | Switches debug/info/warn imports to miden_node_tracing; keeps tracing::instrument. |
| bin/network-monitor/src/explorer.rs | Switches info import to miden_node_tracing; keeps tracing::instrument. |
| bin/network-monitor/src/counter.rs | Switches error/info/warn imports to miden_node_tracing; keeps tracing::instrument. |
| bin/network-monitor/src/commands/start.rs | Switches debug/info/warn imports to miden_node_tracing; keeps tracing::instrument. |
| bin/network-monitor/Cargo.toml | Adds dependency on miden-node-tracing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub(crate) fn check(dotted: &str) -> Result<(), Vec<String>> { | ||
| if ALLOWED_OPENTELEMETRY_NAMES.contains(&dotted) { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
ALLOWED_OPENTELEMETRY_NAMES is Vec<&'static str>, but contains(&dotted) passes &&str, which does not match &&'static str and won’t compile. Consider storing owned Strings (or a HashSet<&'static str>) and checking via .iter().any(|s| *s == dotted) (or HashSet::contains(dotted)).
There was a problem hiding this comment.
☝️ is a false positive
| /// Emits `target = "…",` for use in `#[tracing::instrument(…)]`. | ||
| pub(crate) fn to_span_target_tokens(&self) -> TokenStream2 { | ||
| match self { | ||
| ComponentName::Literal(lit) => quote! { target = #lit, }, | ||
| ComponentName::Ident(ident) => { | ||
| let s = ident.to_string(); | ||
| let lit = LitStr::new(&s, ident.span()); | ||
| quote! { target = #lit, } | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn to_span_name_tokens(&self, fn_ident: &Ident) -> TokenStream2 { | ||
| let name = format!("{}.{}", self.as_string(), fn_ident); | ||
| let lit = LitStr::new(&name, fn_ident.span()); | ||
| quote! { name = #lit, } | ||
| } | ||
|
|
||
| /// Emits `target: "…",` for use in `tracing::<level>!(…)` event macros. | ||
| pub(crate) fn to_event_target_tokens(&self) -> TokenStream2 { | ||
| match self { | ||
| ComponentName::Literal(lit) => quote! { target: #lit, }, | ||
| ComponentName::Ident(ident) => { | ||
| let s = ident.to_string(); | ||
| let lit = LitStr::new(&s, ident.span()); | ||
| quote! { target: #lit, } | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
ComponentName::Ident is converted into a string literal (e.g. target = "COMPONENT"), so #[instrument(COMPONENT: ...)] uses the identifier name rather than the const’s value (e.g. "miden-store"). This breaks existing COMPONENT: &str patterns and results in misleading targets/span names. Consider emitting target = crate::COMPONENT/target: crate::COMPONENT (or target = COMPONENT if you require it in scope), and avoid baking the identifier into a string literal; similarly, consider not overriding name = ... based on Ident if you can’t incorporate the const’s value safely.
Formerly #1500