mirror of
https://github.com/XRPLF/rippled.git
synced 2026-09-27 15:28:03 +00:00
493475a9d4
Brings phase-10 up to 3836078a78 (74 commits). Seven conflicts, resolved
per-hunk; no side was taken wholesale.
validate_telemetry.py, four hunks. The module docstring keeps both category
lists, renumbered. _log_prometheus_metric_names takes phase-10's version: it
returns the family list that the new reverse-coverage check consumes, and
_log_name_list prints every family sorted one per line, which supersedes the
hand-maintained prefix filter this branch had been extending -- that filter
existed only to keep the log readable and listed strictly less. validate_metrics
takes phase-10's _metric_check_targets call. assert_sync_diagnostics_metrics and
phase-10's _check_metric_label both landed at the same place; both are kept.
That third hunk carried the hazard this branch had flagged in advance.
_metric_check_targets selects every group satisfying isinstance(dict) and had no
equivalent of SKIPPED_METRIC_GROUPS, because on phase-10 there was no group that
needed excluding. Merged as-is it would have walked sync_diagnostics while
assert_sync_diagnostics_metrics also walks it, polling and reporting all 61
metrics twice. The exclusion is reinstated inside that function, and its
docstring claim that the isinstance test "selects exactly the same groups the
previous name-based exclusion list did" is corrected -- true on phase-10, false
here, and the reason is ownership, which no structural test can express.
expected_metrics.json: both sides appended to metrics_excluded, so both sets are
kept, 29 entries. expected_spans.json: phase-10's fuller pathfind.compute
skip_reason replaces this branch's, and this branch's ledger.acquire ->
ledger.acquire.astree relationship is kept.
Both docs carried stale counts, and the two sides disagreed with each other --
15 dashboards against 16, and both claiming 41 span types when the contract holds
48. Rather than pick a stale side, every figure is recomputed from the resolved
contract: 48 span types as 28 required and 20 optional, 145 metric checks across
26 asserting categories as 140 names plus 5 required_labels, of which 61 are the
sync_diagnostics names, and 16 dashboards, which matches both the uid list and
the files on disk. The runbook keeps phase-10's table, which adds the reverse-
coverage row.
ConsensusSpanNames.h: both sides added different constants to namespace val;
both kept. Confirmed no identifier is redefined -- the merged file's duplicate
set is identical to this branch's, and those duplicates are distinct namespaces
(op::round against the enclosing span::round), not redefinitions.
ConsensusSpanNames.cpp was an add/add: both branches wrote this file
independently, 9 tests here and 8 on phase-10, with no name in common. All 17
are kept. The guard is dropped rather than applied to the union: SpanNames.h
documents that its constants are deliberately NOT guarded by
XRPL_ENABLE_TELEMETRY, ConsensusSpanNames.h has no guard, and the tests this
branch contributed reference ValStatus only in comments while calling
validationStatusValue with plain ints. So they compile without telemetry, and
unguarding them gains coverage in a -Dtelemetry=OFF build rather than losing it.
One defect belongs to the merge itself, appearing on neither parent. phase-10
added a job_queue_per_type_gauges group holding six jobq_<type>_running/_waiting
names; this branch declared jobq_saturation in MetricNames.h. Rule K checks a
name only when its family is owned, so declaring that constant made jobq_ owned
and turned phase-10's six entries into violations. They are beast::insight gauges
created per job type by JobTypeData's constructor, so no constant can exist for
them -- one triple per job type, minted at runtime. The group joins
NON_OTEL_METRIC_GROUPS alongside statsd_gauges for the same reason.
Verification: no conflict markers repo-wide and no unmerged index entries; both
JSON contracts parse; validate_telemetry.py compiles; every count written into
the docs re-derived from the resolved files and matching; span counters still 48
and 74; check_otel_naming.py exits 0, and Rule K proven still able to fail by
injecting a bogus name in an owned family; levelization baseline clean after
regeneration, with 17 incoming include changes; doxygen style clean across all
tracked C++ at CI scope; pre-commit --all-files clean except cargo-fmt, which
reports "Executable `cargo` not found" and touches none of the 0 Rust files here.
NOT compiled -- no approval to build, so the incoming C++ is unverified by a
compiler on this branch.
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. - L1-metrics — instrument names, label keys and bounded label values come
from the
namespace metric/namespace label/namespace lvalblocks of every*MetricNames.h, read asinline constexpr char NAME[] = "wire";. These headers deliberately do not use themakeStr/StaticStrDSL the span headers use: the OTel C++ API takesnostd::string_view, which constructs fromchar const*but has no constructor fromstd::string_view, so aStaticStrwill not compile in an instrument-name or label-key position.
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. |
| I | No string literals as metric instrument names or label keys — the mirror of Rule F. Applies to the name passed to an XRPL_METRIC_* macro or a meter->Create* factory and to the label keys in its label set. Label values, descriptions, *MetricNames.h, MetricMacros.h and test files are exempt. Scoped by metric family (first underscore segment): declaring a constant opts that family in, so the metric surface can be converted subsystem by subsystem. Unconverted families warn as Rule L. |
| J | Metric instrument names follow the suffix conventions: lower_snake_case, no xrpld_/xrpl_ prefix (the exporter adds it), a counter ends _total, a histogram ends _us/_ms/_seconds, a gauge does not end _total. The instrument kind is read from the emit site, never guessed from words in the name — so a multi-series gauge carrying units in its label values (e.g. nodestore_state observing write_mean_us) is not a violation. A name created through two different factories is itself reported as a kind conflict, since no suffix can be correct for both. |
| K | Every metric named in docker/telemetry/workload/expected_metrics.json resolves to a declared constant, so a rename in code cannot leave the workload validator asserting a name nothing emits. PromQL selectors (m{label="v"}) and exporter-appended histogram suffixes (_bucket/_count/_sum) are normalized away first; groups fed by another emit path (statsd_gauges, statsd_counters, spanmetrics) are out of scope by design. |
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. |
| L | A literal metric name in a family that has no *MetricNames.h constants yet. Rule I's ratchet defers these instead of failing the build on the whole pre-existing metric surface at once; the warning keeps the outstanding conversion work visible rather than silently accepted. |
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.