Five review findings.
The validation_accept span started inside the "outcome is Current"
branch, so validation_status could only ever read "current" and the four
rejected values were unreachable. That defeated the attribute: a node
whose trusted validations are all rejected emitted no span at all, so it
looked the same as a quiet node. The span now starts before the outcome
is checked, still for trusted validations only, so a rejected one is
recorded with its real status.
The rest are consistency fixes: ValidatorSite passes the parse_error
constant instead of the literal, the 28 gauge callbacks that spelled the
`metric` label key as a literal now use label::metric like the other two,
a stray blank first line is gone from six files, and MetricsRegistry.cpp
loses a duplicate Doxygen block that documented addHistogramView but sat
above an unrelated constant.
Eleven local phase-10 commits, mostly documentation and dashboard-description
work, plus one real include fix. No conflicts (git merge-tree reported 0).
Reviewed for interaction with this branch:
- b50aa17aee adds <xrpl/consensus/ConsensusSpanNames.h> to RCLConsensus.cpp and
PeerImp.cpp. This is the same misc-include-cleaner finding my local clang-tidy
run reported on those two files, so the merge resolves two of the 74
pre-existing findings rather than adding any.
- 8ade2b43d9 / 7da5ac5992 / d18f713f91 rename the node-health found-ratio and
stored-bytes panels and correct the nudb_bytes and NuDB found-ratio
descriptions, then repoint the runbook at the new names. These are the same
correction phase-10 made earlier to node_reads_hit -- 'found an object', not a
cache hit -- carried through to the panels that render it.
- ledger-sync-health.json is untouched by phase-10, so none of this branch's
panel work is affected.
Verified after merging: naming checker exits 0, its 138 unit tests pass, all 15
dashboards validate, levelization produces no diff, and this branch's own work is
intact (4 touch() call sites, the ledgers_behind guard, 54 panels / 18 bargauges).
Phase-10 brought in the upstream nodestore/peerfinder/consensus reorganisation
along with its own write-path telemetry, which collided with the sync-diagnostic
signals on this branch. Twelve files conflicted; every resolution keeps both
intents rather than picking a side.
The nodestore write timing was implemented twice, independently. Both sides
added getStoreDurationUs()/getFetchDurationUs() to Database and both timed the
backend call in each concrete store(). Keeping both would have added twice to
storeDurationUs_ per store while storeStats() still counted one, so the mean
write latency would have read double on every dashboard -- silently, since no
test on either side asserts an exact microsecond figure. Resolved to one
accumulator API: recordStoreDuration(), which takes a duration, clamps a
sub-microsecond sample to zero and uses a relaxed atomic add. Phase-10's
storeDurationStats() is gone and its two call sites now use the survivor, so
all three store paths -- both store() overrides and importInternal() -- add
exactly once.
SlotCensus and its pure virtual moved from src/xrpld/peerfinder/ to
include/xrpl/peerfinder/PeerfinderManager.h, following the Manager interface
upstream relocated. The xrpld header is now phase-10's makeConfig shim, and
Overlay.h, MetricMacros.cpp and the getSlotCensus() override chain point at the
new location. ConsensusSpanNames.h and peerfinder Slot.h/Config.h include paths
followed their headers into libxrpl the same way.
InboundLedger gained phase-10's AcquireStats counters next to this branch's
span activations in both the destructor abort path and done(); neither
displaces the other. nodestore_state keeps the constant-based name this branch
requires of it and phase-10's fuller description.
Upstream #7292 deleted src/test/nodestore/Database_test.cpp, which held this
branch's testDurationAccessors. Phase-10 restored the per-store half of that
coverage in DatabaseConfig_test, but nothing covered importInternal -- it writes
through storeBatch() and never through store(), so it is a third store path that
has to time itself. That half is ported to a GTest in
src/tests/libxrpl/nodestore/Database.cpp, keeping the exact zero-before and
accumulate-after assertions and the per-instance negative check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both files use telemetry::consensus::span::* but reached the declarations
only transitively, which misc-include-cleaner rejects. Add the direct
include now that the header lives under xrpl/consensus/.
The consensus headers moved into the isolated xrpl/consensus module and
took a dependency on xrpl/telemetry for the tracing span constants, but
two things were left behind:
- Four includes still pointed at the old src/xrpld/consensus/ location,
which no longer exists, so the build failed with
"fatal error: 'xrpld/consensus/ConsensusParms.h' file not found".
- xrpl.libxrpl.consensus never linked xrpl.libxrpl.telemetry. add_module
isolates each module's headers, so xrpl/telemetry/SpanNames.h was not
on the include path even once the include was repointed.
Repoint the stale includes at xrpl/consensus/, and declare the telemetry
module before consensus so consensus can link it. Regenerate ordering.txt
for the resulting edge.
Two suspects from the 3.3.0 slowdown investigation had no signal. Both were
already computing the numbers and throwing them away, so this exposes them
rather than adding measurement.
Per-sweep heap trim. The trim runs after every cache sweep, and its cost
scales with resident heap, so it is the leading explanation for a node with
a populated database syncing slower than a fresh one. The report already
carried duration, fault deltas and reclaimed pages, but the whole
measurement sat behind a debug-journal check, so an ordinary node measured
nothing, and the call site discarded the result. The measurement now always
runs and only the log line stays gated. Records trim duration, minor faults
and reclaimed kilobytes. Measured cost of the always-on path is about six
microseconds per sweep against a trim costing milliseconds, at a cadence of
ten to a hundred and twenty seconds.
Honest limit, stated in the runbook: the fault delta spans only the trim
call, so it shows the trim itself faulting but not the faults that follow as
caches refill. The duration is the signal to correlate against sweep-job
queueing.
Rotation writes. Rotation copies archive-served reads forward and re-stores
nodes missing from both backends, both of which compete with sync I/O and
only happen on a populated online_delete database. The copy-forward count
existed but was reset by the rotation's own log line, so a metric reading it
would drop to zero on every swap; a never-reset total sits beside it now.
The re-store count was not measured at all. Rotation duration is
deliberately not recorded: the health throttle sleeps at eight points inside
the sequence and dominates exactly when the node is unhealthy, so the number
would conflate work with waiting.
Nothing added for the other two suspects. Get-object serving is already
covered by the handler label, the lookup histogram and the deferred and
saturation gauges; peer churn by the disconnect-reason counter.
Also replaces nine per-file cspell ignores with one ignoreRegExpList entry
for the telemetry macro names, and picks up the levelization baseline for the
consensus span-name test.
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>
A slow fresh-sync ledger produced spans scattered across threads with no
way to relate them. They now share a trace id derived from the ledger's own
hash, the one value every participating site already holds, so nothing new
is plumbed across threads. This is the pattern the transaction pipeline
already uses for its tx id.
Joined: ledger.validate, ledger.store, and a new
consensus.validation.accept recorded when a trusted validation arrives. In
Tempo, searching one ledger hash returns them together, so an operator can
tell whether the ledger was slow to arrive, slow to be accepted, or slow to
be stored. They are siblings rather than a chain because the accept gate is
entered from three different threads, so no fixed parent order exists.
consensus.validation.accept also records why an arriving validation did or
did not advance the gate, which makes "validations arrive but are all
rejected" visible for the first time.
consensus_round_duration_ms turns the existing round-time span attribute
into a histogram, so a fleet trend needs a metric query rather than raw
trace inspection. An explicit bucket view is required, not optional: the
SDK default tops out at ten seconds while consensus abandons a round at two
minutes, so slow rounds would all fall in one bucket and every quantile
would read exactly ten seconds. Cost is one record per round.
Record layer: the histogram is native and needs no collector change. The
two new bounded attributes are added as span-metric dimensions to both
collector configs. The ledger hash stays out of them, since a per-ledger
dimension mints a series per ledger; it is indexed in Tempo as the join key.
The ledger.acquire span is not joined yet, because that file was being
changed concurrently. It is registered as an optional member of the join
group so nothing fails, and switching it is a one-line follow-up.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Activate the accept span as ambient context at the top of doAccept using the
non-owning ScopedActivation, so doAccept's log lines (and any spans created in
the body) carry the accept span's trace_id. doAccept runs to completion on the
JtAccept worker with no coroutine yield, so the activation is thread-local and
safe; the acceptSpan still owns and ends the span. The accept.apply child span
keeps its explicit childSpan(acceptApply, acceptSpanContext_) parentage --
activation only adds log correlation and does not alter parent linkage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
With unscoped SpanGuard + spanContext() (own-span capture), the consensus
round/establish/accept sites just capture and drop the detach dance;
openSpan + peer proposal/validation receive are thread-free handoffs (no
detach); rootSpan->freshRoot.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Post-review fixes for the SpanGuard scope-leak change:
- SpanGuard.h: restore the injectCurrentContextToProtobuf real-class
declaration, its #else no-op stub, and the protocol::TraceContext
forward-declaration dropped during the 3->4 union merge (compile break).
- Re-parent consensus.phase.open and consensus.establish under the round
span via a new RCLConsensus::Adaptor::roundSpanContext() accessor: the
round span is now detached, so ambient parenting no longer works; the
phase spans link explicitly to the captured round context. csf::Peer
gains a matching no-op accessor so the generic engine still compiles.
- Switch five childSpan(op::X, ctx) sites to the full consensus::span::X
constants: childSpan(name, ctx) takes the name verbatim, so the suffix-
only op:: constants emitted short, non-dotted span names.
- TestTelemetry mock: add getConsensusTraceStrategy() override (base pure
virtual introduced on this branch) so the mock is not abstract.
- validate(): rewrite the trace_context comment to drop attack-surface
framing and the PR-discussion reference.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
roundSpan_, establishSpan_, openSpan_ and the accept span are emplaced on
one job worker and reset()/moved to another, leaking their thread-local OTel
Scope. Each is now detached AFTER its context is captured. Spans that
previously nested under them via the ambient thread context are re-pointed to
the captured SpanContext explicitly:
- round children (proposal.send, ledger_close, mode_change) -> roundSpanContext_
- establish children (update_positions, check) -> establishSpanContext_
- accept.apply -> acceptSpanContext_
onForceAccept (synchronous) keeps the accept span's behaviour; only onAccept
(moved into the JtAccept job) needs the detach. Member move-assignment is
deleted, so re-assignment uses emplace().
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add missing <string_view> include in Consensus.h and RCLConsensus.h.
Remove unused SHAMap.h include from RCLConsensus.h.
Add braces around single-line if/else bodies in RCLConsensus.cpp.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Bring phase-8 (log correlation) and the naming-check + BasicConfig fixes forward
into phase 9 (native-metrics gap fill). Phase 9 adds MetricsRegistry and
LedgerSpanNames.h.
Conflict resolution:
- presentation.md: took phase-9's restructured deck — sampling now lives in
Slide 8 (Head vs Tail) and the Phase 10/11/12 future-phase slides; phase-8's
older standalone sampling section is superseded (no content lost).
- ordering.txt/loops.txt: regenerated via generate.py.
Naming check: A/B/E/F/G green. Rule D reports 6 findings that are all check
gaps, not data errors (verified every dashboard label traces to code):
- 3 are TraceQL `span.<attr>` references (bare attr is in L1); Rule D should
strip the span./resource. scope prefix.
- 2 (`reason`, `job_type`) are native OTel metric labels emitted by
MetricsRegistry.cpp — a new source of truth the check does not yet model.
The Rule D enhancement lands on phase-1c and merges forward.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- tempo.yaml: align consensus filter tags with emitted keys
(consensus_mode, consensus_round, ledger_seq) instead of dotted form
- haveConsensus(): set span attributes before early-return paths so the
consensus.check span carries diagnostics even when consensus is not reached
- replace hardcoded consensus phase/result/vote literals with
ConsensusSpanNames.h val constants; add val::phaseOpen/Establish/Accepted
- ConsensusReceiveTracing.h: use canonical consensus::span constants instead
of duplicate inline detail:: names
- SpanGuardFactory test: use rpc_span / consensus::span constants now that
levelization permits the dependency
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds workflow-critical attributes to consensus spans:
- consensus.proposal.send: is_bow_out (identifies resignation proposals)
- consensus.accept: consensus_state (yes/moved_on/expired), disputes_count
- consensus.validation.send: ledger_hash (correlates validation to ledger)
Enables answering: "Did we reach consensus or time out?", "How many
disputes existed at acceptance?", "Which ledger did we validate?"
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
