Two conflicts, both additive-vs-additive; each resolution keeps both sides.
check_otel_naming.py -- phase-10 taught the L6 label extractor to match the
label MAP first and to resolve a key hoisted into a `k...Label` constant,
scanning headers as well as sources. Our side had added the two-regex
first/subsequent literal scan and the `metric_constants(root)[1]` union that
covers the `namespace label` header style.
Kept phase-10's mechanism whole: METRIC_LABEL_MAP + the `(?:^|\{)` key regex
already subsumes what METRIC_LABEL_NEXT did, since matching inside the map body
makes every pair after the first open with a single `{`. So METRIC_LABEL_NEXT is
dropped as genuinely redundant rather than kept as a duplicate scan, and the
reason it existed is folded into METRIC_LABEL's comment. Re-added our
`metric_constants(root)[1]` union on top: LABEL_CONST_DEF only matches
`k`-prefixed identifiers, so it cannot see MetricNames.h's `label::jobType`
style, and without that union Rule D would reject dashboards querying labels
Rule I forced into constants. The two derivations are complementary and both
are now documented as such.
MetricsRegistry.cpp -- both sides added a new sibling view-registration helper
next to addMicrosecondHistogramView, and both added a registration call in
initExporterAndProvider(). Kept all four helpers
(addHistogramView/Microsecond/RoundDuration/SubMillisecond) and every
registration: phase-10's addSubMillisecondHistogramView + kNodeStoreReadUs
alongside our addRoundDurationHistogramView, sweepMallocTrimUs and the two
millisecond dial/resolve ladders.
phase-10's nodestore_read_us histogram does not duplicate our work. The
nodestore_latency gauge that would have overlapped it was retired in c4e434d520
before this merge, and the surviving nodestore_state gauge is complementary
rather than duplicative: both read the same fetch measurement, but the gauge
publishes only a since-boot mean via scaledMean() and cannot yield a
percentile -- the consequence observeNodeStoreTotals' own docs state plainly --
while the histogram buckets each fetch and can. The histogram also splits by
fetch_type and found, which the gauge cannot. phase-10 registered its
explicit-bucket View, so it does not inherit the SDK default ladder.
Each file keeps its own existing naming style: phase-10's k-prefixed constants
are left as-is, ours stay namespaced.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rule D rejected the `handler` and `result` dashboard filters even though
both labels are emitted, because the L6 extractor missed them twice over:
- It matched only a key written as an inline literal directly after `{{`.
A label map is a braced list of braced pairs, so in
`Add(1, {{"job_type", a}, {"handler", b}})` only the first key follows
`{{` -- every later one was dropped. A key hoisted into a constant
(`{{kHandlerLabel, v}}`) was invisible in any position.
- It walked *.cpp only, so a constant declared in a header, as
kLabelResult is, could never be found.
Match the label map first and scan its pairs, resolve `k...Label`
constants through their `constexpr char k...[] = "..."` definitions, and
read headers too. Matching the map rather than any `{"key",` in the file
keeps ordinary brace initializers, such as the `{"http", "https"}` scheme
array, out of the label set -- they are not labels and must not license a
dashboard filter. Test code is skipped for the same reason Rule F skips
it: fixtures pass arbitrary literal pairs.
nodestore_latency published six values that nodestore_state already
publishes from the same Database accessors, so the two gauges were
duplicate readings of the same atomics:
write_count -> node_writes getStoreCount()
read_count -> node_reads_total getFetchTotalCount()
write_duration_us -> node_writes_duration_us getStoreDurationUs()
read_duration_us -> node_reads_duration_us getFetchDurationUs()
write_mean_us -> write_mean_us store duration / count
read_mean_us -> read_mean_us fetch duration / count
nodestore_state is kept because its means go through scaledMean(), which
saturates at INT64_MAX instead of wrapping and omits a mean when the
denominator is zero rather than reporting a misleading 0 us.
Removes registerNodeStoreLatencyGauge, its instrument member, the
metric::nodestoreLatency constant and the lval::nodestore_latency label
namespace. The gauge-over-histogram rationale and the "p99 is not
obtainable" consequence are folded into observeNodeStoreTotals' docs.
Retargets the gauge-contract test onto nodestore_state rather than
deleting it: the scaledMean arithmetic is covered by the static_asserts
in tests/libxrpl/telemetry/MetricsRegistry.cpp, but nothing else asserts
that these named series multiplex onto one instrument keyed by `metric`.
The test now calls the production scaledMean instead of a copy of the
division, and its sub-microsecond case asserts scaledMean's actual
behaviour (a genuine mean of 0 on a zero numerator with a non-zero
count), which differs from the retired gauge's extra numerator guard.
Rewrites both ledger-sync-health copies' panel 38/39 queries and drops
the obsolete claim that the write numerator was never written: all three
concrete store paths call recordStoreDuration, so write_mean_us is live
on an ordinary node. The same stale [import_db] caveat is removed from
the runbook, the 09 reference row and the workload validator's note.
Phase-10 independently instrumented the peer object-fetch path while this
branch instrumented fresh-node sync, so the two overlapped in three places.
Resolved by keeping each side's stronger implementation rather than shipping
both.
Per-job-type waiting/running/deferred existed twice. Phase-10's version
survives: it publishes per-type gauges from JobQueue::collect(), which
snapshots under the queue lock and publishes after releasing it, a
deliberate lock-order fix against the collector's own lock. This branch's
jobq_backlog gauge and the JobQueue::getJobTypeCounts() accessor that fed it
are removed, along with their panels, assertions and reference rows.
jobq_saturation stays: it reports the whole worker pool, which phase-10 has
no equivalent for.
The histogram view helper also existed twice with identical bodies under two
names; one survives, and the microsecond ladder is now the named array
rather than boundaries repeated inline. The job_type label was declared
twice, once as a file-local constant invisible to the naming check; both it
and handler now come from the constants header.
Two things phase-10 adds are complementary, not duplicates, and are kept as
they are: the handler label, which separates the two request kinds that both
report as the same job type, and getobject_rejected_total, which counts
malformed requests where this branch's serve_refused_total counts requests
this node declined to serve.
Also fixes two naming-check failures that pre-date this merge on phase-10.
The check derived label keys only from namespaced constants, so it could not
see the per-subsystem headers' flat k-prefixed style and rejected dashboards
querying labels the code really emits. It now reads both styles, with the
enforcement rules unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Metric names and label keys were bare string literals, repeated across the
emit site, the gauge registration, the unit test, the workload manifest, the
dashboard queries and the reference table. A rename touched six places and a
typo in any one of them failed silently: a metric that never appears, or a
label that never joins.
The span side already had this right, with names and attribute keys declared
once in the *SpanNames.h headers and a CI rule rejecting literals at call
sites. That rule only ever covered spans, so the metric side had no
equivalent and no suffix convention was enforced by anything.
- Adds MetricNames.h declaring every instrument name, label key and bounded
label value this story emits, grouped by subsystem, following the existing
span-name header layout.
- Converts the call sites subsystem by subsystem. The emitted strings are
unchanged: 75 names before, the same 75 after, verified by extracting the
wire strings from both trees and diffing the sets.
- Extends the naming check with three rules: no literal instrument name or
label key at an emit site, the duration and counter suffix conventions,
and every name in the workload manifest resolving to a constant. The
first rule is ratcheted per metric family so the pre-existing families
warn rather than block, keeping the remaining work visible instead of
forcing one unreviewable change.
Constants are character arrays rather than the span headers' StaticStr,
because the metrics API takes a string view that will not construct from it.
Two things the conversion exposed: a serve-refusal reason that the original
inventory missed because it is passed through a ternary, and a label whose
constant made it invisible to the checker's literal scan, which would have
failed a dashboard rule.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five signals that explain why a node is not advancing toward full, none of
which were observable before:
- state_changes_total now carries {from,to} mode labels, emitted at
setMode using the existing strOperatingMode helper. A bare count could
not distinguish a healthy climb from a node flapping between tracking
and connected. Removes the now-unused incrementStateChanges wrapper.
- sync_state{initial_full_duration_us}: time to first reach full, which
StateAccounting already computed but exposed only in server_info.
- sync_state{network_ledger_gate}: whether the node is still refusing to
build ledgers because it has no network ledger.
- sync_state{server_stall_seconds} and server_stall_events_total: how
long the main thread has been unresponsive. LoadManager computed this
and only logged it, so a stall was invisible until the fatal threshold.
The episode rule is a pure function so it can be tested without adding
a test-only mutator to LoadManager.
- sync_state{ledgers_behind}: how far our validated sequence trails the
best sequence any peer advertises, read from already-cached peer ranges
so no extra network traffic is added.
Also fixes the naming checker: it derived only the first label of a
multi-label instrument, so a dashboard querying the second label was
wrongly rejected.
Note: the clang-tidy hook cannot run in this worktree (no build
directory); the remaining pre-commit hooks, the naming check, dashboard
schema and harness syntax all pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
xrpl_branch and xrpl_node_role are stamped by the perf-iac harness --
infrastructure outside this repo's OTel code -- so they have no L1
(*SpanNames.h), L2 (collector config), or L6 (MetricsRegistry.cpp) source
Rule D can derive them from. Unlike the generic builtins set (Prometheus/
Grafana mechanics every OTel setup has), these are repo-specific and
narrow, so they get their own EXTERNAL_INFRA_LABELS constant: a visible,
documented, deliberately narrow exception to the 'no hardcoded allowlist'
design principle, not a silent workaround. Add test coverage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
xrpl_branch and xrpl_node_role are collector/infra-injected resource
labels used by perf-iac dashboards to identify the build under test and
its role in the perf cluster. They have no L1 span-attribute source or
L6 native-metric-label source (like the existing job/instance/job_type
builtins), so Rule D flagged them across every perf-iac dashboard.
Register them in the builtins set, matching the existing pattern for
infra-level dashboard labels. Add test coverage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rootSpan has the same (cat, prefix, name) signature as span(), so its
prefix/name arguments must be *SpanNames.h constants under Rule F/H. Add
rootSpan to the CALLSITE regex and CONSTANT_ARG_POSITIONS ({1,2}), update
the enumerating docstrings/README, and add two regression tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Add job_type to otel-naming builtins (standard Prometheus label for
job-queue metrics used in node-health dashboard)
- Add OpenTelemetry SDK to UBSAN suppressions (intentional unsigned
integer overflow in attributemap_hash.h hash computation)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Rule A silently missed a dotted span attribute (xrpl.ledger.hash) because of
two interacting bugs:
1. attr_keys_from_header resolved each constant via a flat global symbol table
keyed by bare name, so a later header defining a same-named constant (e.g.
consensus attr::ledgerHash = "ledger_hash") clobbered the base header's
attr::ledgerHash = "xrpl.ledger.hash", erasing the real dotted key from L1.
Now each constant is resolved against its own header (the global table only
seeds seg::/join() cross-file references); using-re-exports still resolve
globally.
2. derive_dotted_resource_keys allowlisted any dotted key declared in the base
SpanNames.h. Now it allowlists only the keys actually passed to
Resource::Create() in Telemetry.cpp (semconv service.* + the attr:: constants
set there, e.g. xrpl.network.*). A dotted key declared in a header but never
set as a resource attr is a Rule-A violation.
Adds 4 regression tests (collision, using-re-export, allowlist scope, brace
matching). No allowlist exception is added — the check now catches the
violation so the offending code can be fixed.
Rule C was reading docker/telemetry/tempo.yaml (the Tempo server config), which
has no filter tags, so it always SKIPped — L3 was silently unenforced. The
trace-search filter tags actually live in the Grafana datasource provisioning
file (docker/telemetry/grafana/provisioning/datasources/tempo.yaml) as
search.filters[].{tag,scope}. Point Rule C there (server file as fallback),
pair each tag with its scope, validate only span-scope tags against L1 (resource/
intrinsic tags like service.*/name/status/duration are exempt), and strip the
TraceQL span. prefix.
On phase-9 this turns "SKIP: C" into "OK: C: 24 tempo span-filter tags all in
L1" — L3 is now genuinely guarded. Adds a RuleCTempo test class (4 cases:
span-tag-not-in-L1 flagged, span-tags-pass, resource/intrinsic ignored, skip
when datasource absent). 83 tests total.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Phase 9 surfaced two Rule D gaps (false positives, not data errors):
- TraceQL `span.<attr>` / `resource.<attr>` references: the bare attribute is
in L1, but the scope-prefixed form was flagged. Now strip the
span./resource./event./link/instrumentation_scope. prefix before the L1
lookup.
- Native OTel metric labels (e.g. `job_type`, `reason`) emitted by
MetricsRegistry are valid dashboard labels but are not span attributes. Add
an L6 source: parse `Add(.., {{"label", ...}})` instrument calls and accept
those label keys alongside L1 and builtins.
Verified against phase-9's real dashboards: 6 prior false positives -> 0.
79 tests (7 new for span-prefix stripping and metric-label extraction).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rule D (dashboard PromQL labels must exist in L1) flagged `__name__` once the
phase-7 system-*.json dashboards started using `sum by (le, __name__)`.
`__name__` is the Prometheus reserved label for the metric name itself — a
builtin, not a span attribute. Add it to the builtin allowlist and cover it
with a test. (Earlier dashboards only used `__name__` inside `{__name__=~...}`
matchers, which the label regex did not extract, so this surfaced only now.)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Three robustness fixes to check_otel_naming.py, all on phase-1c where the
script lives:
- Rule F now runs UNCONDITIONALLY. It is a purely syntactic check on the
call-sites and does not need the L1 key set, so code that calls
SpanGuard::span/setAttribute directly without ever defining a *SpanNames.h
is still caught (previously it was silently skipped when no header existed).
- Exempt test files from Rule F (tests pass arbitrary literal keys to exercise
the API). The call-site matcher now requires a SpanGuard/`.`/`->` receiver,
so std::span and bare declarations no longer false-positive.
- Add Rule H (warning, non-fatal): a namespace-qualified constant used at a
telemetry call-site but not defined in any *SpanNames.h is flagged, catching
constants defined in-place instead of in the proper header. Bare locals and
std:: names are not warned to avoid noise.
SpanGuard.h / Telemetry.h @code examples updated to reference constants that
exist on this branch. README documents the new behavior.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add Rule G to check_otel_naming.py: every span-attribute key must be
lower_snake_case (^[a-z][a-z0-9_]*$ per dot-separated segment). This catches
camelCase, UPPERCASE, and spaces in keys, which the structural (dotted) and
source (literal) rules did not. Document it in the script README and
CONTRIBUTING.md.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add check_otel_naming.py and wire it into on-pr.yml so every PR validates
that span-attribute names stay consistent across the code, collector, Tempo,
dashboards, and docs.
- The valid key set is derived dynamically from the *SpanNames.h constants and
the resource attributes the code registers in Telemetry.cpp — no hardcoded
allowlist to drift.
- Each rule is presence-gated: it runs only when the file it needs is in the
tree, so the check is correct whether telemetry changes land in one PR or
several (the collector/Tempo/dashboard/runbook layers arrive in later phases).
- Rule A flags dotted span-attribute keys; Rule F flags string-literal
attribute keys and span-name arguments (values may be runtime data).
- stdlib-only, mirroring the levelization check (bare `python`, no pip step).
- Telemetry.h / SpanGuard.h @code examples now use *SpanNames.h constants so
the strict literal check passes.
- CONTRIBUTING.md documents the check and how to run it locally.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
