diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index c8396fcb71..24853d9c0b 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -1352,6 +1352,9 @@ jobs: - name: Check workspace deps run: cargo xtask check-workspace-deps + - name: Check metric docs + run: cargo xtask check-metric-docs + - name: Check licenses run: cargo make --no-workspace check-licenses diff --git a/Cargo.lock b/Cargo.lock index a1d3cde614..2d61209217 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13157,8 +13157,10 @@ dependencies = [ "chrono", "clap", "eyre", + "proc-macro2", "similar 3.1.1", "sqlx", + "syn 2.0.117", "tokio", "toml_edit 0.25.12+spec-1.1.0", "tracing", diff --git a/Makefile.toml b/Makefile.toml index 4e9674dce7..71f48aaa1e 100644 --- a/Makefile.toml +++ b/Makefile.toml @@ -579,6 +579,7 @@ dependencies = [ "carbide-lints", "check-format-nightly", "check-workspace-deps", + "check-metric-docs", "check-licenses", "check-bans", ] @@ -818,6 +819,11 @@ script = [ "cargo --quiet xtask check-workspace-deps || (RC=$?; echo 'To fix automatically, run `cargo xtask check-workspace-deps --fix`'; exit $RC)", ] +[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"] + [tasks.check-isolated-package-builds] workspace = false description = "Check that workspace packages build independently with default features" diff --git a/STYLE_GUIDE.md b/STYLE_GUIDE.md index d216af2c52..9a4c9ea8f1 100644 --- a/STYLE_GUIDE.md +++ b/STYLE_GUIDE.md @@ -157,6 +157,29 @@ Networking technologies should be integrated using the workflows described in [N When designing metrics, be careful with cardinality. Do not attach highly unique labels that explode time-series count, like per-machine or per-instance attributes. +## Instrumentation + +A significant event -- a count, a rate, or a duration -- is declared with the instrumentation framework +(`carbide-instrument`) rather than by hand-rolling an OpenTelemetry instrument. One `#[derive(Event)]` declaration +produces a structured log line, a Prometheus metric, or both from a single `emit()`, with metric cardinality bounded by +the type system. See [Instrumentation](docs/observability/instrumentation.md) for the full model and when to use it. + +The canonical pattern, checked at compile time by the derive: + +- **`carbide_` prefix** on every metric name. The name in the attribute is the exposed name, verbatim, so a dashboard + greps straight back to the source line. +- **`_total` suffix for counters** (never a doubled `_total_total` -- the Prometheus exporter appends the suffix), and a + **unit suffix for histograms** (`_seconds`, `_milliseconds`, `_microseconds`, `_bytes`). +- **A counter's `describe` opens with "Number of ..."**. That HELP text is the row the + [core metrics catalogue](docs/observability/core_metrics.md) records, and every framework counter and histogram -- + apart from a `name_unchecked` one, whose exposed name is an OTel-sanitized transform -- must appear there, enforced by + `cargo xtask check-metric-docs`. +- **Bounded `#[label]` fields; high-cardinality detail in `#[context]`**. Labels come from `LabelValue` enums; machine + IDs, IPs, and error text stay on the log line only. + +`name_unchecked` and `describe_unchecked` are the greppable escape hatches for grandfathered metrics; new metrics use +the standard form. + ## Logging All services should emit logs in "logfmt" syntax. This structured logging format allows administrators to efficiently diff --git a/crates/instrument-macros/src/lib.rs b/crates/instrument-macros/src/lib.rs index 65432a4e4a..7051102521 100644 --- a/crates/instrument-macros/src/lib.rs +++ b/crates/instrument-macros/src/lib.rs @@ -93,7 +93,10 @@ fn expand_label_value(input: DeriveInput) -> syn::Result { /// (enum via `LabelValue`; goes to both the log line and the metric), /// `#[context]` (any `Display`; log-only), or `#[observation]` (the histogram /// value). The metric name is validated at compile time: `carbide_` prefix, -/// `_total` for counters, a unit suffix for histograms. +/// `_total` for counters (never a doubled `_total_total`), a unit suffix for +/// histograms. A counter's `describe` is checked too -- present and opening +/// with "Number of ..." -- with `describe_unchecked` as the escape hatch for +/// grandfathered text, mirroring `name_unchecked` for names. #[proc_macro_derive(Event, attributes(event, label, context, observation))] pub fn derive_event(input: TokenStream) -> TokenStream { let input = syn::parse_macro_input!(input as DeriveInput); @@ -130,6 +133,7 @@ struct EventArgs { log: LogSpec, metric: MetricSpec, name_unchecked: bool, + describe_unchecked: bool, } fn parse_event_args(input: &DeriveInput) -> syn::Result { @@ -142,6 +146,7 @@ fn parse_event_args(input: &DeriveInput) -> syn::Result { log: LogSpec::Info, metric: MetricSpec::None, name_unchecked: false, + describe_unchecked: false, }; let mut saw_attr = false; @@ -163,6 +168,8 @@ fn parse_event_args(input: &DeriveInput) -> syn::Result { args.unit = Some(meta.value()?.parse()?); } else if meta.path.is_ident("name_unchecked") { args.name_unchecked = true; + } else if meta.path.is_ident("describe_unchecked") { + args.describe_unchecked = true; } else if meta.path.is_ident("log") { let ident: Ident = meta.value()?.parse()?; args.log = match ident.to_string().as_str() { @@ -195,7 +202,7 @@ fn parse_event_args(input: &DeriveInput) -> syn::Result { } else { return Err(meta.error( "unknown `event` key; expected name, component, message, describe, log, \ - metric, unit, or name_unchecked", + metric, unit, name_unchecked, or describe_unchecked", )); } Ok(()) @@ -282,11 +289,28 @@ fn expand_event(input: DeriveInput) -> syn::Result { )); } match args.metric { - MetricSpec::Counter if !name_value.ends_with("_total") => { - return Err(syn::Error::new_spanned( - name, - "counter names end in `_total` (Prometheus convention)", - )); + MetricSpec::Counter => { + if !name_value.ends_with("_total") { + return Err(syn::Error::new_spanned( + name, + "counter names end in `_total` (Prometheus convention)", + )); + } + // The OpenTelemetry instrument name must not carry `_total` + // itself: the Prometheus exporter appends it, so a name that + // still ends in `_total` after one is stripped ships a doubled + // `_total_total` series (the #3431 footgun). + if name_value + .strip_suffix("_total") + .is_some_and(|base| base.ends_with("_total")) + { + return Err(syn::Error::new_spanned( + name, + "counter name ends in `_total_total`: the Prometheus exporter appends the \ + `_total` suffix, so the instrument name must carry only one. Drop a \ + `_total` (use name_unchecked only to keep a grandfathered doubled name)", + )); + } } MetricSpec::Histogram if histogram_unit.is_none() => { return Err(syn::Error::new_spanned( @@ -322,6 +346,32 @@ fn expand_event(input: DeriveInput) -> syn::Result { metric = none", )); } + // A counter's `describe` is its Prometheus HELP text and the row the + // `core_metrics.md` catalogue records, so a counter must document itself, + // and the tech-writer house rule is that the text opens with "Number of ". + // `describe_unchecked` is the escape hatch for a grandfathered describe -- + // legacy phrasings, or the "Total number of ..." on a name_unchecked + // counter -- mirroring `name_unchecked` for names. + if args.metric == MetricSpec::Counter && !args.describe_unchecked { + match &args.describe { + None => { + return Err(syn::Error::new_spanned( + &input.ident, + "a counter must document itself: add describe = \"Number of ...\" (its \ + Prometheus HELP text, and the core_metrics.md catalogue row). Use \ + describe_unchecked to keep a grandfathered counter's describe", + )); + } + Some(describe) if !describe.value().starts_with("Number of ") => { + return Err(syn::Error::new_spanned( + describe, + "a counter's describe opens with \"Number of ...\" (the tech-writer house \ + rule). Use describe_unchecked to keep a grandfathered describe", + )); + } + Some(_) => {} + } + } let unit_value: String = match (&args.unit, histogram_unit) { (Some(explicit), _) => explicit.value(), (None, Some(from_suffix)) => from_suffix.to_string(), diff --git a/crates/instrument/src/lib.rs b/crates/instrument/src/lib.rs index 19c209cfbf..ab94b686e1 100644 --- a/crates/instrument/src/lib.rs +++ b/crates/instrument/src/lib.rs @@ -37,6 +37,7 @@ //! component = "component_manager", //! log = warn, // error|warn|info|debug|trace|off //! metric = counter, // counter | histogram | none +//! describe = "Number of power control operations that failed", // counter HELP text //! message = "power control failed", //! )] //! struct PowerControlFailed { @@ -69,7 +70,8 @@ //! //! ```compile_fail //! #[derive(carbide_instrument::Event)] -//! #[event(name = "carbide_demo_total", component = "demo", log = off, metric = counter)] +//! #[event(name = "carbide_demo_total", component = "demo", log = off, metric = counter, +//! describe = "Number of demo events")] //! struct Demo { //! #[label] //! machine_id: String, // ERROR: String is not a LabelValue @@ -116,6 +118,45 @@ //! #[event(name = "carbide_demo", component = "demo", message = "demo", describe = "demo")] //! struct Demo {} // ERROR: `describe` documents a metric; this event has metric = none //! ``` +//! +//! A counter documents itself: `describe` is required and opens with +//! "Number of ..." (the tech-writer house rule, so the `core_metrics.md` +//! catalogue reads consistently): +//! +//! ```compile_fail +//! #[derive(carbide_instrument::Event)] +//! #[event(name = "carbide_demo_total", component = "demo", log = off, metric = counter)] +//! struct Demo {} // ERROR: a counter must document itself with describe = "Number of ..." +//! ``` +//! +//! ```compile_fail +//! #[derive(carbide_instrument::Event)] +//! #[event(name = "carbide_demo_total", component = "demo", log = off, metric = counter, +//! describe = "Total number of demos")] +//! struct Demo {} // ERROR: a counter's describe opens with "Number of ..." +//! ``` +//! +//! A counter name ends in `_total` (Prometheus convention) but not +//! `_total_total` -- the framework strips one `_total` before registering and +//! the exporter appends it back, so a doubled suffix ships a `_total_total` +//! series: +//! +//! ```compile_fail +//! #[derive(carbide_instrument::Event)] +//! #[event(name = "carbide_demo_total_total", component = "demo", log = off, metric = counter, +//! describe = "Number of demos")] +//! struct Demo {} // ERROR: counter name ends in `_total_total` +//! ``` +//! +//! Both checks have a greppable escape hatch for grandfathered metrics -- +//! `describe_unchecked` for the text, `name_unchecked` for the name: +//! +//! ``` +//! #[derive(carbide_instrument::Event)] +//! #[event(name = "carbide_demo_total_total", component = "demo", log = off, metric = counter, +//! name_unchecked, describe = "Total number of demos", describe_unchecked)] +//! struct Demo {} +//! ``` use std::time::Duration; diff --git a/crates/instrument/src/testing.rs b/crates/instrument/src/testing.rs index 9a6c4e2c29..a95ce0f827 100644 --- a/crates/instrument/src/testing.rs +++ b/crates/instrument/src/testing.rs @@ -25,7 +25,8 @@ //! //! #[derive(Event)] //! #[event(name = "carbide_doc_demo_total", component = "demo", -//! log = warn, metric = counter, message = "demo fired")] +//! log = warn, metric = counter, message = "demo fired", +//! describe = "Number of demo events fired")] //! struct Demo {} //! //! let metrics = MetricsCapture::start(); diff --git a/crates/instrument/tests/matrix.rs b/crates/instrument/tests/matrix.rs index e717e1a939..e882c8f1e2 100644 --- a/crates/instrument/tests/matrix.rs +++ b/crates/instrument/tests/matrix.rs @@ -46,6 +46,7 @@ fn both_sides_from_one_emit() { component = "matrix-test", log = warn, metric = counter, + describe_unchecked, message = "matrix both fired" )] struct BothSides { @@ -92,7 +93,8 @@ fn metric_only_writes_no_log() { name = "carbide_test_matrix_quiet_total", component = "matrix-test", log = off, - metric = counter + metric = counter, + describe_unchecked )] struct Quiet { #[label] @@ -208,7 +210,8 @@ fn unit_struct_and_declared_knobs() { name = "carbide_test_matrix_unit_total", component = "matrix-test", log = off, - metric = counter + metric = counter, + describe_unchecked )] struct Tick; @@ -235,6 +238,7 @@ fn per_instance_log_at_override() { component = "matrix-test", log = dynamic, metric = counter, + describe_unchecked, message = "call finished" )] struct CallFinished { diff --git a/crates/xtask/Cargo.toml b/crates/xtask/Cargo.toml index 0d29a8d425..f8a7d3a973 100644 --- a/crates/xtask/Cargo.toml +++ b/crates/xtask/Cargo.toml @@ -29,6 +29,8 @@ clap = { workspace = true } cargo_metadata = { workspace = true } similar = { workspace = true } version-compare = { workspace = true } +syn = { workspace = true, features = ["full", "parsing"] } +proc-macro2 = { workspace = true, features = ["proc-macro"] } chrono = { workspace = true } tracing = { workspace = true } diff --git a/crates/xtask/src/main.rs b/crates/xtask/src/main.rs index a386a4e0af..423861c3ad 100644 --- a/crates/xtask/src/main.rs +++ b/crates/xtask/src/main.rs @@ -15,6 +15,7 @@ * limitations under the License. */ mod isolated_package_builds; +mod metric_docs; mod squash_migrations; mod workspace_deps; @@ -33,6 +34,11 @@ enum Xtask { about = "Check that each workspace package builds independently with its default features" )] IsolatedPackageBuilds, + #[clap( + name = "check-metric-docs", + about = "Check that every #[derive(Event)] counter/histogram has a row in docs/observability/core_metrics.md" + )] + CheckMetricDocs, #[clap( name = "squash-migrations", about = "Create a single squashed migration from all existing migrations in crates/api-db/migrations" @@ -57,6 +63,7 @@ async fn main() -> eyre::Result<()> { workspace_deps::check(fix)?.report_and_exit() } Xtask::IsolatedPackageBuilds => isolated_package_builds::check()?, + Xtask::CheckMetricDocs => metric_docs::check()?, Xtask::SquashMigrations(args) => squash_migrations::run(args).await?, } Ok(()) diff --git a/crates/xtask/src/metric_docs.rs b/crates/xtask/src/metric_docs.rs new file mode 100644 index 0000000000..02f376e659 --- /dev/null +++ b/crates/xtask/src/metric_docs.rs @@ -0,0 +1,444 @@ +/* + * SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +//! Doc-gate: every counter and histogram declared through the instrumentation +//! framework (`#[derive(Event)]`) must have a row in the checked-in metrics +//! catalogue, `docs/observability/core_metrics.md`. +//! +//! The catalogue is regenerated by the `test_integration` metrics test, which +//! scrapes `/metrics`. That leaves a gap the CI repo-clean check cannot see: a +//! metric no test exercises is never scraped, so it can land with no catalogue +//! row and nothing complains. This check closes it from the source side -- +//! it reads the `#[event(...)]` declarations directly, so a metric is caught +//! whether or not any test drives it at runtime. +//! +//! The scan parses each source file with `syn` and walks its item tree, so the +//! metric name is read as a proper Rust string literal (raw strings included), +//! `#[cfg(test)]` fixtures are excluded by their attribute or an enclosing +//! module's, and `#[event(...)]` in a doc-comment example is never seen (a doc +//! comment is not an item). 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. +//! +//! `name_unchecked` events are exempt: their exposed `/metrics` name is a +//! sanitized/suffixed transform of the declared name (dotted OpenTelemetry +//! names, the doubled `_total_total`), not the verbatim string, so a source +//! match is not meaningful. Cataloguing those precisely wants the events +//! registry from #3221; until then they stay documented by scrape-and-retain. + +use std::path::{Path, PathBuf}; + +use eyre::{Context, bail}; +use proc_macro2::{TokenStream, TokenTree}; +use syn::{Attribute, Item, ItemStruct, Meta}; +use walkdir::WalkDir; + +static REPO_ROOT: &str = concat!(env!("CARGO_MANIFEST_DIR"), "/../.."); // crates/xtask/../.. +const CATALOGUE: &str = "docs/observability/core_metrics.md"; + +/// A counter or histogram declared by a `#[derive(Event)]` `#[event(...)]` +/// site, keyed by its verbatim exposed name. +struct DeclaredMetric { + name: String, + kind: &'static str, + describe: Option, + source: PathBuf, +} + +pub fn check() -> eyre::Result<()> { + let repo_root = PathBuf::from(REPO_ROOT).canonicalize()?; + let catalogue = std::fs::read_to_string(repo_root.join(CATALOGUE)) + .with_context(|| format!("reading {CATALOGUE}"))?; + + let mut declared = Vec::new(); + for dir in ["crates", "rest-api"] { + collect(&repo_root.join(dir), &repo_root, &mut declared)?; + } + + let missing: Vec<&DeclaredMetric> = declared + .iter() + .filter(|m| !catalogue.contains(&format!("{}", m.name))) + .collect(); + + if missing.is_empty() { + println!( + "OK: all {} framework counters/histograms are documented in {CATALOGUE}.", + declared.len(), + ); + return Ok(()); + } + + 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); +} + +fn collect(dir: &Path, repo_root: &Path, out: &mut Vec) -> eyre::Result<()> { + for entry in WalkDir::new(dir) + .into_iter() + .filter_map(Result::ok) + .filter(|e| e.file_type().is_file()) + { + let path = entry.path(); + if path.extension().and_then(|e| e.to_str()) != Some("rs") { + continue; + } + let rel = path.strip_prefix(repo_root).unwrap_or(path); + if skip_path(rel) { + continue; + } + let src = + std::fs::read_to_string(path).with_context(|| format!("reading {}", path.display()))?; + // Only files that declare an event are worth parsing; but once a file + // does, a parse failure is loud -- never a silent skip of a metric. + if !src.contains("#[event(") { + continue; + } + let file = syn::parse_file(&src).with_context(|| { + format!( + "{} declares #[event(...)] but the metric-docs gate could not parse it", + rel.display() + ) + })?; + collect_items(&file.items, false, rel, out)?; + } + Ok(()) +} + +/// Walks the module item tree, carrying whether an enclosing module is +/// `#[cfg(test)]`. Production events are module-level structs, so fn bodies and +/// impl blocks (where test fixtures live) are deliberately not descended into. +fn collect_items( + items: &[Item], + in_test: bool, + source: &Path, + out: &mut Vec, +) -> eyre::Result<()> { + for item in items { + match item { + Item::Mod(module) => { + let module_in_test = in_test || has_cfg_test(&module.attrs); + if let Some((_, sub_items)) = &module.content { + collect_items(sub_items, module_in_test, source, out)?; + } + } + Item::Struct(item_struct) => { + if in_test || has_cfg_test(&item_struct.attrs) || !derives_event(&item_struct.attrs) + { + continue; + } + if let Some(metric) = event_metric(item_struct, source)? { + out.push(metric); + } + } + _ => {} + } + } + Ok(()) +} + +/// Reads the `#[event(...)]` of a `#[derive(Event)]` struct. `Ok(None)` when it +/// declares no counter/histogram (metric = none) or is `name_unchecked`; +/// errors (fails closed) when a counter/histogram's name cannot be read. +fn event_metric(item: &ItemStruct, source: &Path) -> eyre::Result> { + let Some(attr) = item.attrs.iter().find(|a| a.path().is_ident("event")) else { + // `#[derive(Event)]` with no `#[event(...)]`; the derive itself rejects + // this, so it is not the gate's job to report. + return Ok(None); + }; + + let mut name = None; + let mut metric = None; + let mut describe = None; + let mut name_unchecked = false; + attr.parse_nested_meta(|meta| { + if meta.path.is_ident("name") { + name = Some(meta.value()?.parse::()?.value()); + } else if meta.path.is_ident("metric") { + metric = Some(meta.value()?.parse::()?.to_string()); + } else if meta.path.is_ident("describe") { + describe = Some(meta.value()?.parse::()?.value()); + } else if meta.path.is_ident("name_unchecked") { + name_unchecked = true; + } else if meta.path.is_ident("describe_unchecked") { + // A flag with no value, and no bearing on documentation. + } else if meta.path.is_ident("component") + || meta.path.is_ident("message") + || meta.path.is_ident("unit") + { + // String-valued keys the gate ignores; consumed to keep parsing. + let _: syn::LitStr = meta.value()?.parse()?; + } else if meta.path.is_ident("log") { + let _: syn::Ident = meta.value()?.parse()?; + } else { + // An event key the derive defines but this gate has not been taught: + // fail closed so the two stay in sync. + return Err(meta.error("metric-docs gate: unrecognized `event` key")); + } + Ok(()) + }) + .with_context(|| format!("parsing #[event(...)] in {}", source.display()))?; + + let kind = match metric.as_deref() { + Some("counter") => "counter", + Some("histogram") => "histogram", + // metric = none, or no metric side: nothing to catalogue. + _ => return Ok(None), + }; + if name_unchecked { + return Ok(None); + } + let Some(name) = name else { + bail!( + "{}: an #[event(...)] with metric = {kind} has no readable name; the metric-docs \ + gate will not skip it silently", + source.display(), + ); + }; + Ok(Some(DeclaredMetric { + name, + kind, + describe, + source: source.to_path_buf(), + })) +} + +/// The instrumentation crate's own `#[event]` sites are doc examples and test +/// fixtures; xtask (this crate) only mentions `#[event(` in scanner strings; +/// `tests`/`benches` trees and `tests.rs` modules are never production metrics. +fn skip_path(rel: &Path) -> bool { + let path = rel.to_string_lossy(); + if path.starts_with("crates/instrument/") + || path.starts_with("crates/instrument-macros/") + || path.starts_with("crates/xtask/") + { + return true; + } + if matches!( + rel.file_stem().and_then(|s| s.to_str()), + Some("tests" | "test") + ) { + return true; + } + rel.components().any(|c| { + matches!( + c.as_os_str().to_str(), + Some("tests") | Some("benches") | Some("target") + ) + }) +} + +/// Whether `#[derive(...)]` names `Event` (bare or path-qualified, e.g. +/// `carbide_instrument::Event`). +fn derives_event(attrs: &[Attribute]) -> bool { + attrs.iter().any(|attr| { + attr.path().is_ident("derive") + && matches!(&attr.meta, Meta::List(list) if tokens_have_ident(list.tokens.clone(), "Event")) + }) +} + +/// Whether an attribute list carries `#[cfg(test)]` (also inside `all(...)` / +/// `any(...)`), ignoring a `not(test)` subtree. +fn has_cfg_test(attrs: &[Attribute]) -> bool { + attrs.iter().any(|attr| { + attr.path().is_ident("cfg") + && matches!(&attr.meta, Meta::List(list) if predicate_mentions_test(list.tokens.clone())) + }) +} + +/// A `test` predicate is active in a `cfg(...)` token stream: recurse into +/// `all(...)`/`any(...)` groups, but skip a `not(...)` subtree (its `test` +/// would negate to non-test). +fn predicate_mentions_test(tokens: TokenStream) -> bool { + let mut iter = tokens.into_iter().peekable(); + while let Some(tt) = iter.next() { + match tt { + TokenTree::Ident(ident) if ident == "not" => { + // Drop the argument group of `not(...)`. + if matches!(iter.peek(), Some(TokenTree::Group(_))) { + iter.next(); + } + } + TokenTree::Ident(ident) if ident == "test" => return true, + TokenTree::Group(group) if predicate_mentions_test(group.stream()) => return true, + _ => {} + } + } + false +} + +/// Whether a token stream contains the identifier `wanted` at any nesting. +fn tokens_have_ident(tokens: TokenStream, wanted: &str) -> bool { + tokens.into_iter().any(|tt| match tt { + TokenTree::Ident(ident) => ident == wanted, + TokenTree::Group(group) => tokens_have_ident(group.stream(), wanted), + _ => false, + }) +} + +/// The `` row a developer should add to the catalogue for a missing metric. +fn suggested_row(m: &DeclaredMetric) -> String { + format!( + "{}{}{}", + m.name, + m.kind, + html_escape(m.describe.as_deref().unwrap_or("")), + ) +} + +/// HTML entity escaping matching the catalogue generator (`askama_escape::Html`). +fn html_escape(s: &str) -> String { + let mut out = String::with_capacity(s.len()); + for c in s.chars() { + match c { + '&' => out.push_str("&"), + '<' => out.push_str("<"), + '>' => out.push_str(">"), + '"' => out.push_str("""), + '\'' => out.push_str("'"), + _ => out.push(c), + } + } + out +} + +#[cfg(test)] +mod tests { + use std::path::Path; + + use super::{DeclaredMetric, collect_items}; + + /// Collect the counter/histogram metrics the gate would require from a + /// snippet of source (the same item walk the real scan uses). + fn declared(src: &str) -> Vec { + let file = syn::parse_file(src).expect("snippet parses"); + let mut out = Vec::new(); + collect_items(&file.items, false, Path::new("snippet.rs"), &mut out) + .expect("no fail-closed error"); + out + } + + /// A raw-string metric name (which the derive accepts) is read, not + /// silently skipped -- the codex P2 hole. + #[test] + fn raw_string_name_is_collected() { + let metrics = declared( + r##" + #[derive(Event)] + #[event(name = r"carbide_raw_total", component = "c", log = off, metric = counter, + describe = "Number of raw things")] + struct RawNamed; + + #[derive(Event)] + #[event(name = r#"carbide_hashed_total"#, component = "c", log = off, metric = counter, + describe = "Number of hashed things")] + struct HashNamed; + "##, + ); + let names: Vec<&str> = metrics.iter().map(|m| m.name.as_str()).collect(); + assert!( + names.contains(&"carbide_raw_total"), + "raw string name r\"...\" must be read, got {names:?}" + ); + assert!( + names.contains(&"carbide_hashed_total"), + "raw string name r#\"...\"# must be read, got {names:?}" + ); + } + + /// A cooked string name still round-trips (escapes handled by LitStr). + #[test] + fn cooked_string_name_is_collected() { + let metrics = declared( + r#" + #[derive(Event)] + #[event(name = "carbide_cooked_total", component = "c", log = off, metric = counter, + describe = "Number of cooked things")] + struct Cooked; + "#, + ); + assert_eq!(metrics.len(), 1); + assert_eq!(metrics[0].name, "carbide_cooked_total"); + } + + /// Events under `#[cfg(test)]` -- on the struct or an enclosing module -- + /// are excluded, so unit-test fixtures do not demand catalogue rows. + #[test] + fn cfg_test_events_are_excluded() { + let metrics = declared( + r#" + #[derive(Event)] + #[event(name = "carbide_prod_total", component = "c", log = off, metric = counter, + describe = "Number of prod things")] + struct Prod; + + #[cfg(test)] + mod tests { + #[derive(Event)] + #[event(name = "carbide_modfixture_total", component = "c", log = off, + metric = counter, describe = "Number of fixtures")] + struct ModFixture; + } + + #[cfg(test)] + #[derive(Event)] + #[event(name = "carbide_structfixture_total", component = "c", log = off, + metric = counter, describe = "Number of fixtures")] + struct StructFixture; + "#, + ); + let names: Vec<&str> = metrics.iter().map(|m| m.name.as_str()).collect(); + assert_eq!( + names, + vec!["carbide_prod_total"], + "only the non-test event should be collected, got {names:?}" + ); + } + + /// `metric = none` and `name_unchecked` events are not catalogue rows. + #[test] + fn non_metric_and_grandfathered_are_skipped() { + let metrics = declared( + r#" + #[derive(Event)] + #[event(name = "carbide_logonly", component = "c", metric = none, message = "hi")] + struct LogOnly; + + #[derive(Event)] + #[event(name = "carbide_legacy_total_total", name_unchecked, component = "c", + log = off, metric = counter, describe = "Total number", describe_unchecked)] + struct Legacy; + "#, + ); + let names: Vec<&str> = metrics.iter().map(|m| m.name.as_str()).collect(); + assert!(names.is_empty(), "got {names:?}"); + } +} diff --git a/docs/observability/core_metrics.md b/docs/observability/core_metrics.md index 60769dfcc0..4ae61000b6 100644 --- a/docs/observability/core_metrics.md +++ b/docs/observability/core_metrics.md @@ -9,8 +9,13 @@ This file contains a list of metrics exported by NVIDIA Infra Controller (NICo). carbide_api_db_span_query_time_millisecondshistogramTotal time the request spent inside a span on database transactions carbide_api_grpc_server_duration_millisecondshistogramProcessing time for a request on the carbide API server carbide_api_readygaugeWhether the NICo API is running +carbide_api_secrets_request_duration_millisecondshistogramDuration of Postgres secrets operations, in milliseconds. +carbide_api_secrets_requests_failed_totalcounterNumber of failed Postgres secrets operations. +carbide_api_secrets_requests_succeeded_totalcounterNumber of successful Postgres secrets operations. +carbide_api_secrets_requests_totalcounterNumber of Postgres secrets operations attempted. carbide_api_tls_cert_refreshes_totalcounterNumber of TLS acceptor refreshes performed by the API listener carbide_api_tls_connection_attempted_totalcounterNumber of inbound TLS connection attempts +carbide_api_tls_connection_fail_totalcounterNumber of failed inbound TLS connection attempts carbide_api_tls_connection_success_totalcounterNumber of successful TLS connections carbide_api_tracing_spans_opengaugeNumber of open logging/tracing spans carbide_api_vault_request_duration_millisecondshistogramDuration of outbound Vault requests, in milliseconds @@ -19,18 +24,34 @@ This file contains a list of metrics exported by NVIDIA Infra Controller (NICo). carbide_api_vault_requests_succeeded_totalcounterNumber of successful Vault requests carbide_api_vault_token_time_until_refresh_secondsgaugeThe amount of time, in seconds, until the Vault token is required to be refreshed carbide_api_versiongaugeVersion (git sha, build date, etc) of this service +carbide_auth_denied_totalcounterNumber of Forge calls denied by the authorizer carbide_authn_client_cert_rejected_totalcounterNumber of client certificates rejected during authentication carbide_available_ips_countgaugeNumber of available IPs per network segment +carbide_bmc_proxy_tls_connection_attempted_totalcounterNumber of inbound TLS connection attempts +carbide_bmc_proxy_tls_connection_fail_totalcounterNumber of failed inbound TCP connections +carbide_bmc_proxy_tls_connection_success_totalcounterNumber of successful TLS connections +carbide_bmc_proxy_upstream_request_duration_millisecondshistogramDuration of requests the proxy forwarded to BMCs, by HTTP method and upstream status class; the _count series, split by status, gives the request and outcome rates. +carbide_certs_renewals_totalcounterNumber of client certificate renewal attempts, by outcome carbide_client_tcp_connect_attempts_totalcounterNumber of outbound TCP connect attempts across all HTTP connectors carbide_client_tcp_connect_errors_totalcounterNumber of failed outbound TCP connect attempts across all HTTP connectors carbide_client_tcp_connect_successes_totalcounterNumber of successful outbound TCP connects across all HTTP connectors carbide_concurrent_machine_updates_availablegaugeNumber of machines in the system that can be updated concurrently. carbide_db_pool_idle_connsgaugeNumber of idle connections in the carbide database pool carbide_db_pool_total_connsgaugeNumber of (active + idle) connections in the carbide database pool +carbide_dhcp_dropped_requests_totalcounterNumber of DHCP packets dropped without a reply, by drop reason. +carbide_dhcp_replies_sent_totalcounterNumber of DHCP replies sent, by reply message type. +carbide_dhcp_requests_totalcounterNumber of DHCP packets received and decoded, by DHCP message type. +carbide_dns_queries_totalcounterNumber of DNS queries received, by query type +carbide_dns_request_duration_millisecondshistogramTime to process a DNS query, by query type and response code +carbide_dns_responses_totalcounterNumber of DNS responses sent, by response code carbide_dpu_agent_version_countgaugeNumber of DPU agents which have reported a certain version. carbide_dpu_firmware_version_countgaugeNumber of DPUs which have reported a certain firmware version. carbide_dpus_healthy_countgaugeNumber of DPUs in the system that have reported healthy in the last report. Healthy does not imply up - the report from the DPU might be outdated. carbide_dpus_up_countgaugeNumber of DPUs in the system that are up. Up means we have received a health report less than 5 minutes ago. +carbide_dsx_exchange_consumer_dedup_skipped_totalcounterNumber of messages skipped due to deduplication +carbide_dsx_exchange_consumer_messages_dropped_totalcounterNumber of messages dropped due to queue overflow +carbide_dsx_exchange_consumer_messages_processed_totalcounterNumber of messages successfully processed +carbide_dsx_exchange_consumer_messages_received_totalcounterNumber of MQTT messages received carbide_endpoint_exploration_duration_millisecondshistogramThe time it took to explore an endpoint carbide_endpoint_exploration_expected_machines_missing_overall_countgaugeNumber of machines expected but not identified carbide_endpoint_exploration_expected_power_shelves_missing_overall_countgaugeNumber of power shelves expected but not identified @@ -41,11 +62,15 @@ This file contains a list of metrics exported by NVIDIA Infra Controller (NICo). carbide_endpoint_explorations_countgaugeNumber of attempted endpoint explorations carbide_exhausted_reprovision_retry_countgaugeNumber of host machines in the system whose host firmware upgrade retry budget is exhausted. carbide_external_call_duration_millisecondshistogramDuration of outbound calls by backend, operation, and outcome; the _count series, split by outcome, gives the request and error rates. +carbide_firmware_download_duration_secondshistogramDuration of background firmware artifact downloads, by outcome; an ok attempt spans fetch, checksum verification, and publish, and the _count series, split by outcome, is the download and failure rate. carbide_firmware_update_failures_totalcounterNumber of firmware update failures, by update target and cause carbide_firmware_updates_totalcounterNumber of firmware updates started and completed, by update target and phase; only the host target emits both phases carbide_gpus_in_use_countgaugeNumber of GPUs actively used by tenants in instances in the NICo deployment carbide_gpus_total_countgaugeNumber of GPUs in the NICo deployment carbide_gpus_usable_countgaugeNumber of remaining GPUs in the NICo deployment available for immediate instance creation +carbide_health_otlp_export_failures_totalcounterNumber of OTLP export batches dropped after a send failure, by signal and gRPC status code. +carbide_health_report_submissions_totalcounterNumber of health report submissions to the NICo API, by report target and outcome. +carbide_host_reprovision_retries_totalcounterNumber of times a failed host firmware upgrade was retried during host reprovisioning carbide_hosts_by_sku_countgaugeNumber of hosts by SKU and device type ('unknown' for hosts without SKU) carbide_hosts_health_overrides_countgaugeNumber of health overrides configured in the site carbide_hosts_health_status_countgaugeNumber of managed hosts in the system that have reported either a healthy or not healthy status - based on the presence of health probe alerts @@ -98,6 +123,8 @@ This file contains a list of metrics exported by NVIDIA Infra Controller (NICo). carbide_measured_boot_machines_totalgaugeNumber of machines reporting measurements. carbide_measured_boot_profiles_totalgaugeNumber of measured boot profiles. carbide_measured_boot_verification_failures_totalcounterNumber of measured boot verification failures, across quote verification and attestation handling, by cause +carbide_mqtt_dispatch_dropped_totalcounterNumber of received MQTT messages dropped because the handler concurrency semaphore could not be acquired +carbide_mqtt_reconnects_totalcounterNumber of times an MQTT client re-established its broker connection after the initial connect carbide_network_segments_enqueuer_iteration_latency_millisecondshistogramThe overall time it took to enqueue state handling tasks for all carbide_network_segments in the system carbide_network_segments_handler_latency_in_state_millisecondshistogramThe amount of time it took to invoke the state handler for objects of type carbide_network_segments in a certain state carbide_network_segments_iteration_latency_millisecondshistogramThe elapsed time in the last state processor iteration to handle objects of type carbide_network_segments @@ -133,6 +160,7 @@ This file contains a list of metrics exported by NVIDIA Infra Controller (NICo). carbide_preingestion_totalgaugeNumber of known machines currently being evaluated prior to ingestion carbide_preingestion_waiting_downloadgaugeNumber of machines that are waiting for firmware downloads on other machines to complete before doing their own carbide_preingestion_waiting_installationgaugeNumber of machines which have had firmware uploaded to them and are currently in the process of installing that firmware +carbide_pxe_boot_outcomes_totalcounterNumber of PXE boot-path outcomes served, by endpoint and reason. carbide_racks_enqueuer_iteration_latency_millisecondshistogramThe overall time it took to enqueue state handling tasks for all carbide_racks in the system carbide_racks_health_overrides_countgaugeNumber of health overrides configured in the site carbide_racks_health_status_countgaugeNumber of racks in the system that have reported either a healthy or not healthy status - based on the presence of health probe alerts @@ -158,6 +186,7 @@ This file contains a list of metrics exported by NVIDIA Infra Controller (NICo). carbide_site_explorer_last_run_statusgaugeThe status of the latest Site Explorer run carbide_site_explorer_phase_latency_millisecondshistogramThe time it took to perform one site explorer iteration phase carbide_site_explorer_update_explored_endpoints_countgaugeCounts from the last update_explored_endpoints phase by kind +carbide_state_handler_wakeup_failures_totalcounterNumber of times a machine's state handler could not be woken after an agent-reported event carbide_switches_enqueuer_iteration_latency_millisecondshistogramThe overall time it took to enqueue state handling tasks for all carbide_switches in the system carbide_switches_health_overrides_countgaugeNumber of health overrides configured in the site carbide_switches_health_status_countgaugeNumber of switches in the system that have reported either a healthy or not healthy status - based on the presence of health probe alerts diff --git a/docs/observability/instrumentation.md b/docs/observability/instrumentation.md index f615807d52..9626e44615 100644 --- a/docs/observability/instrumentation.md +++ b/docs/observability/instrumentation.md @@ -200,7 +200,8 @@ The `name` in the attribute is the exposed Prometheus name, verbatim, and the de enforces the conventions at compile time: - All new metrics use the `carbide_` prefix. -- Counter names have a `_total` suffix. +- Counter names have a `_total` suffix -- and only one: the Prometheus exporter appends `_total`, + so an instrument name that already ends in `_total` (a doubled `_total_total`) is rejected. - Histograms end in their unit: `_seconds`, `_milliseconds`, `_microseconds`, `_bytes`. - Gauge names (existing pattern, not the framework) are mixed legacy forms; follow established neighboring names rather than a single suffix rule. @@ -215,9 +216,18 @@ its frozen name via `name_unchecked` (plus an explicit `unit = "..."` for histog a search for `name_unchecked` finds every grandfathered site, and new metrics cannot use the opt-out silently. -Give every metric a `describe = "..."` attribute -- it becomes the Prometheus HELP text and the -Description column of the [core_metrics.md](core_metrics.md) catalogue, which is -regenerated by `test_integration` and checked in CI. +A counter documents itself: its `describe = "..."` is required and opens with "Number of ..." (the +tech-writer house rule, enforced by the derive). The text becomes the Prometheus HELP and the +Description column of the [core_metrics.md](core_metrics.md) catalogue. A grandfathered describe +keeps its wording with `describe_unchecked` -- the counterpart to `name_unchecked` -- and a search +for either finds every opt-out. A histogram takes any `describe` (or none); a log-only event +(`metric = none`) has no metric to document, so it must omit `describe`. + +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 -- other than a +`name_unchecked` one, whose exposed name is an OTel-sanitized transform (see #3221) -- is missing a +catalogue row, so a new metric cannot land undocumented. ## Derivation outputs and costs