mirror of
https://github.com/XRPLF/rippled.git
synced 2026-09-27 15:28:03 +00:00
83b003df69
Every existing rule in this checker runs one way: take a consumer -- a collector dimension, a Tempo tag, a dashboard label, a doc, an asserted metric name -- and require it to resolve to the *SpanNames.h constants. Rule H looks closest to the reverse but is still consumer-side: a constant USED at a call site that no header defines. Nothing looked the other way. So deleting a setAttribute from a .cpp and leaving its constant in the header passed every rule and every compiler, while the telemetry it described stopped being emitted. The workload validation job would eventually notice, but only when it happens to run, and its path filter deliberately does not watch daemon .cpp files -- widening it to 1827 C++ files to catch this would fire a twenty-minute Docker job on nearly every commit. Rule M closes that: an L1 constant no code under src/ or include/ references. It searches all references, not just telemetry call sites, because constants are passed to helpers, stored in locals and used as attribute VALUES -- a call-site-only scan would report false positives. Constants referenced only by test code are reported separately, since a constant exercised by a test but by no production path is still dead in production. A WARNING, not a failure, for two reasons that are both real here. Six constants in this tree are already dead, so failing would redden the branch immediately. And in a stacked chain a constant legitimately lands one commit before its call site, so a failing rule would break intermediate branches for a condition that resolves downstream. What it reports today, all verified unreferenced across the whole repository and not just src/include: ConsensusSpanNames.h val::increased, val::decreased and val::unchanged; SpanNames.h seg::link, attr_val::success and attr_val::error. Placed here rather than upstream on phase-1c, where the checker was introduced, because the gap it closes is a workload-harness concern -- the contract asserting a name nothing emits -- and the harness is this branch's. Putting it on 1c would also mean union-resolving a 1900-line file across ten merge hops, each an opportunity to silently drop the metric rules that live downstream. Ported from a patch written against the sync-diagnostics copy, which carries the metric-side rules I/J/K/L that this branch does not. The rule itself is purely span-side; the only shared dependency it needed was iter_sources, which arrived with those metric rules, so that helper is added here on its own. The rule-L docstring entry and README row that came with the patch context were dropped -- this branch has no rule L. Verification: rule M reports 262 constants checked and 6 unreferenced, exit code still 0 because warnings do not change it; 169 unittest cases pass; the checker compiles; no metric-rule content leaked in from the patch context (0 occurrences of METRIC_MACRO_CALL or run_rule_i_metric_literals). Proven non-vacuous by blanking all three call sites of attr::rpcStatus -- the count went 6 to 7 and named that constant -- then restoring them and watching it return to 6.
OTel naming-consistency check
check_otel_naming.py enforces the OpenTelemetry span-attribute naming
convention documented in
CONTRIBUTING.md
across every layer of the telemetry pipeline. The *SpanNames.h constants are
the single source of truth (L1); every other layer must agree with them.
Running locally
python .github/scripts/otel-naming/check_otel_naming.py
It takes no arguments, can be run from any directory inside the repo, and uses
only the Python standard library (no pip install, matching the levelization
check). A non-zero exit code means a violation was found; the output lists each
violation as RULE | location | token | expected.
What it checks
The valid key set is derived dynamically from the OTel code — there is no hardcoded allowlist:
- L1 keys come from the
namespace attr { ... }blocks of every*SpanNames.h, resolving themakeStr("x")/join(seg::a, seg::b)DSL (cross-file, sojoin(seg::rpc, ...)resolvesseg::rpcfrom the baseSpanNames.h). Each constant is resolved against its own header, so two headers that define a same-named constant (e.g. a baseattr::ledgerHashand a domainattr::ledgerHash) each contribute their real wire key — a later header cannot clobber an earlier one's value in a flat table. - Legitimate dotted keys = ONLY the keys the code actually sets as resource
attributes, i.e. the entries inside
Telemetry.cpp'sResource::Create({...})call: thesemconv::service::*keys (service.*) plus anyattr::<name>constants passed there (xrpl.network.*). A dotted key that is declared in a header but never set as a resource attr is a span attribute in resource clothing — a Rule-A violation, even if it lives in the baseSpanNames.h.
Rules (each fails the build, when its inputs are present)
| Rule | Check |
|---|---|
| A | No stray dotted span-attribute key (only the derived resource keys may be dotted). |
| G | Attribute keys are lower_snake_case (^[a-z][a-z0-9_]*$ per dot-segment) — no camelCase, UPPERCASE, or spaces. |
| F | No string literals as attribute keys or span-name arguments in setAttribute/addEvent/span/rootSpan/childSpan (rootSpan shares span's (cat, prefix, name) signature). Attribute values are exempt (runtime data); *SpanNames.h definitions and test files are exempt. |
| B | Every collector spanmetrics.dimensions name exists in the L1 key set. |
| C | Every Tempo span-filter tag exists in the L1 key set. |
| D | Every dashboard label resolves to an L1 span attribute, a native-metric label (L6, emitted by MetricsRegistry), or a Prometheus/Grafana builtin. TraceQL scope prefixes (span./resource./…) are stripped before the L1 lookup. |
| E | No dotted xrpl.<domain>.<field> attribute key in the runbook (only the L1 resource attrs xrpl.network.* may be dotted). Span names, filenames, OTel-standard keys, and metric labels are not flagged. |
Rule F runs unconditionally (it is a purely syntactic check on the
call-sites and needs no *SpanNames.h), so a code path that calls
SpanGuard::span/setAttribute directly without ever defining a header is
still caught.
Warnings (printed, never fail the build)
| Rule | Check |
|---|---|
| H | A namespace-qualified constant (e.g. foo::bar::myKey) used at a telemetry call-site is not defined in any *SpanNames.h. The constant should live in the proper header; defining it in-place bypasses rules A/G/F. Warns rather than fails — the argument may be a legitimately dynamic value, and the header may live on a later branch. Bare locals and std:: names are not warned. |
| M | A constant defined in a *SpanNames.h that no code in src/** or include/** references — the reverse of every failing rule above, which all start from a consumer and look for its L1 source. Deleting the last setAttribute(attr::foo, …) while leaving attr::foo in the header otherwise passes every rule and the compiler, and the telemetry silently stops being emitted. Whole files are searched rather than telemetry call sites only, since a constant is also passed to helpers and used as an attribute value. Constants only test code references are reported separately. Warns rather than fails: in a stacked chain a constant may legitimately land a commit before its call site. |
Presence-gated
Every rule runs only when the source files it needs are present in the tree
and is otherwise skipped (printed as SKIP: <rule> — <reason>), never failed.
This keeps the check correct no matter how telemetry work is split across PRs —
a stacked chain, one large PR, or independent per-stage PRs where (for example)
the collector config lands before the dashboards. The collector/Tempo/dashboard/
runbook layers are introduced in later phases; on a branch without them, only
the L1-intrinsic rules (A, G, F) run.