Nine conflicts, resolved as follows.
src/xrpld/app/ledger/detail/InboundLedger.cpp -- kept this branch's version.
phase10 sets the span's outcome/timeouts/peer_count attributes inline at each
exit; this branch replaced that with the idempotent finalizeAcquireSpan(), called
on all four exits (init, done, give-up, destructor). Taking phase10's blocks
would have set the outcome twice against a helper documented as not overwriting
what the real exit recorded. phase10's comment explains why peer_count must not
be read in a destructor; the helper solves that structurally by taking
std::optional<std::size_t> and being passed std::nullopt from there.
src/xrpld/telemetry/MetricsRegistry.cpp -- kept metric::ledgerEconomy over
phase10's "ledger_economy" literal. This branch added the naming check that
requires constants for converted families, so the literal would regress it. Took
phase10's comment cleanup.
src/xrpld/telemetry/MetricsRegistry.h -- kept registerRotationStateGauge(), which
only exists here, and took phase10's removal of the stale task-number comment.
validate_telemetry.py -- combined both. phase10 replaced serial metric polling
with a concurrent fan-out on one shared deadline, because 58 metrics x 45 s of
additive timeout overran the CI budget; that is kept. Its target list filters on
SKIPPED_METRIC_GROUPS rather than the two literals it hardcoded, so the
sync_diagnostics group stays owned by assert_sync_diagnostics_metrics() instead
of being polled and reported twice. Both SYNC_DIAGNOSTICS_GROUP and
METRIC_POLL_CONCURRENCY are needed and both are kept.
check_otel_naming.py -- both sides extend the rule docstring. Took phase10's
fuller Rule E text (doc discovery, allow-dotted markers) and re-appended rules
I/J/K/L, which exist only here.
expected_metrics.json -- the two sides add disjoint sibling groups, so both are
kept: sync_diagnostics alongside node_health_gauges, overlay_reduce_relay,
overlay_overflow, validation_lifetime_counters and not_asserted. Both dashboard
uids are kept, giving 16 asserted uids against 16 dashboards on disk.
expected_spans.json -- kept this branch's span set, a superset that adds the
acquire phase spans, ledger.serve, txset.acquire and peer.dial, and expands
ledger.acquire's required attributes. Took phase10's description, which documents
what the totals mean, and its note on how the RPC wildcard span is created.
total_span_types and total_unique_attributes are recomputed for the union: 48 and
74, since each side's figure counted only its own spans.
Docs: took phase10's more accurate wording on what the dashboard check actually
covers, and corrected the dashboard count from 15 to 16 where the merge made it
stale.
Verified: no conflict markers remain, both JSON contracts parse, both Python
files compile, asserted dashboard uids match the dashboards on disk exactly, and
the OTel naming check reports all layers consistent.
The reference docs had drifted from the code in ways that break the reader
rather than merely misinform: PromQL examples that return no data, a rollback
flag that is a no-op, a sampling knob that does not exist, and two span parents
that moved. Code is treated as the truth throughout; where the code is the
defective side, the doc now records it as a known issue instead of describing
the bug as intent.
Renames the docs missed: histogram names gain the exporter's unit suffix
(ios_latency_milliseconds_bucket and four siblings), ledger_history_mismatch
gains _total, the StatsD-era quantile label gives way to le buckets,
rpc.request becomes rpc.http_request, traces_spanmetrics_calls_total becomes
span_calls_total, and the nine dotted xrpl.* span attributes are recorded as
renamed rather than left as live keys.
Re-parenting: consensus.update_positions and consensus.check are children of
consensus.establish, not of consensus.round.
Units and labels: state_accounting_*_duration is microseconds, not seconds;
cache_metrics label values are case-sensitive; object_count carries demangled
C++ type names. Nodestore read and write latency stays microseconds -- the
nanosecond accumulator change did not move the exported unit.
Adds what shipped but was undocumented: the ledger.acquire span, seven
consensus.round events, twelve span attributes, node_writes_duration_us, the
7-day validation-agreement window, the TxQ admission and reduce-relay metric
families, metrics_endpoint, and the phase-10 validation workflow.
Corrects claims that never held: 10% head sampling (it is fixed at 100%),
configurable redaction (it is unconditional), -DXRPL_ENABLE_TELEMETRY=OFF
(the flag is -Dtelemetry=OFF, default ON), FindOpenTelemetry.cmake and the
xrpl_telemetry target (neither exists), Promtail and a StatsD exporter in the
pipeline (neither exists), and Loki stream selection on job= (only
service_name is a stream label).
Phase 9 is marked complete, its provisioned alerting is attributed to the
branch that shipped it, and Phase 11 stays at zero except the one prerequisite
its code closes. Counts are reconciled repo-wide: 41 emitted span families,
15 dashboards on disk with 14 asserted, 13 alert rules in 5 groups.
Hardens the gate that let this drift through: Rule E of the naming check now
covers the reference docs, its allow-dotted marker is key-scoped and warns on
stale or empty use, a missing checked file is reported instead of silently
skipped, the test suite runs in CI, and doc paths trigger the check.
C++ and CMake changes are comment-only: three MetricsRegistry instrument names,
eight OTelCollector claims of a metric-name prefix that formatName never adds,
and the telemetry option's inverted default.
Resolves the nodestore latency-accumulator conflict by keeping this
branch's API surface and applying the incoming nanosecond fix to it.
This branch had renamed storeDurationStats() to recordStoreDuration() and
given it noexcept, an explicit relaxed fetch_add, and call-frequency and
thread-safety notes. The incoming change fixed the accumulators to hold
nanoseconds so sub-microsecond backend calls stop rounding to zero. Both
are kept: the name, signature and documentation from here, the nanosecond
accumulation from there.
The incoming duplicate getFetchDurationUs()/getStoreDurationUs() pair is
dropped in favour of this branch's noexcept versions, which now read the
nanosecond members and convert on read. recordStoreDuration()'s `if (us >
0)` guard is gone: it discarded every sub-microsecond store, which is the
same rounding the incoming change removes one level up.
Also updates a MetricMacros comment that explained the zero write mean in
terms of that removed guard; the injected totals it asserts on are
unchanged.
The fetch and store duration counters converted each sample to
microseconds before adding it, so any backend call finishing in under a
microsecond contributed zero. A warm nudb read answers in a few hundred
nanoseconds, so on fast hardware every read floored and the totals stayed
at zero no matter how many reads happened -- the same loss of resolution
the microsecond report was introduced to avoid, one decade lower.
Both accumulators now hold nanoseconds, the clock's own resolution, and
convert once in getFetchDurationUs() and getStoreDurationUs(). The public
accessors, the node_reads_duration_us and node_writes_duration_us JSON
fields, and the metrics that read them all keep microseconds, so nothing
downstream changes unit. storeDurationStats() takes the raw duration
instead of a pre-converted integer so no caller can round early, and
updateFetchMetrics() scales its microsecond input to match.
FetchReport::elapsed stays microseconds: it carries one fetch, not a
total, and that is the unit it declares. The reported sum is therefore the
accumulated total minus a sub-microsecond remainder per fetch, so the two
tests that asserted exact equality between them now assert that bound.
Both had assertions that depended on how fast the host reads; the bound
holds on any hardware.
develop renamed CamelCase namespaces to snake_case with no compatibility
aliases, so lines this branch added still referenced the old names. The
merge was textually clean because develop never touched those lines, which
left the breakage invisible to git and visible only at compile time.
PeerFinder:: -> peer_finder:: Overlay.h, OverlayImpl.h, MetricMacros.cpp
Tuning:: -> tuning:: PeerImp.cpp (the other call sites in this
file already used the lowercase form)
Two further merge artifacts:
json_value.h develop added `using value_type = Value const` to
ValueConstIterator, and this branch had added
`using value_type = Value` for the same reason, so the merge
kept both and they redefined the alias with different types.
Keep develop's const-qualified type -- correct for an
iterator whose reference is `Value const&` -- and keep only
this branch's iterator_category addition.
InboundLedger.h <string> is no longer used directly by the header, but
five translation units that include it use std::string
without including <string> themselves. Mark it
`IWYU pragma: keep` rather than remove it, matching
suite_list.h.
Resolves src/tests/libxrpl/CMakeLists.txt test_modules by keeping both
sides: develop's beast/nodestore/protocol additions (nodestore moved into
alphabetical position) and this branch's ledger module.
The round built each thread's batch inside the thread, before arriving at
the latch, so a throw there left the remaining threads waiting on an
arrival that never came -- the test hung instead of failing. A spawn loop
that ended early did the same.
Batches are now built before any thread starts, so nothing between spawn
and arrival can throw, and a guard counts down the shortfall for threads
that were never spawned before joining the ones that were.
The depth accounting having moved to insert entry, depthSamples is now
the denominator of the mean depth, so it gets its own assertions: equal
to insertCount once every thread has been joined, and moving with the
duplicate-key round. The overlap assertions are unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
HashRouter.h declared `class Config;` and never referred to it: the only
other mentions of the name in the file are two prose comments. Nothing
that includes this header depends on it either -- of the eighteen files
that do, the only one that names the type needs the full definition and
includes it itself. The declaration is vestigial, left behind when
setupHashRouter() moved to setup_HashRouter.h, which carries its own.
Left alone it is now an error rather than dead code. This branch's
SlotCensus work made Overlay.h include PeerfinderManager.h, because
Overlay::slotCensus() returns a SlotCensus by value. ValidatorList.cpp
includes both Overlay.h and HashRouter.h, and is the one translation unit
that reaches them without also pulling in xrpld/core/Config.h -- so
PeerFinder::Config and this declaration meet there, and
bugprone-forward-declaration-namespace reports a declaration with no
definition beside a same-named class in another namespace.
Removing it rather than including the real header: xrpl::Config lives in
xrpld, so an include would point the xrpl.libxrpl.core layer at the
layer above it.
Depth was observed at insert entry but its sample was only counted at
insert exit, so an insert still in flight contributed nothing to the
mean while completed inserts -- disproportionately the fast, shallow
ones -- all did. The reported mean understated queueing exactly when
queueing was worst: with every writer inside its first insert the gauge
was omitted entirely, while writers-in-flight correctly showed them all.
Depth and its sample count are now both folded in at entry, so the mean
is taken over one population. Mean depth is the L in Little's Law, so a
biased L understated the derived queueing share of each insert.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Brings in the phase-10 revert of the nodestore read-latency histogram plus the
nudb_bytes -> stored_object_bytes rename.
Conflicts in MetricsRegistry.{h,cpp} resolved keeping both intents:
- MetricsRegistry.cpp: dropped everything that existed only to serve the
reverted nodestore_read_us histogram -- the addSubMillisecondHistogramView()
helper, its call site, the kSubMillisecondBoundaries array and the
NodeStoreMetricNames.h include. Kept every view this branch registers
(consensus round duration, sweep_malloc_trim_us, dns_resolve_latency_ms,
overlay_dial_latency_ms) and the shared addHistogramView() base helper.
Took the rename at the storage_detail observe() call site.
- MetricsRegistry.h: took phase-10's move of the four nodestore_state observe
helpers and their ObserveFn sink from private to public, while keeping this
branch's enriched Doxygen on observeNodeStoreTotals().
Also corrected the registered-view count in the 09 reference doc: neither side's
arithmetic survives the merge, since this branch adds four views phase-10 never
saw and the revert removes one. Ten views are registered now, not six or seven.
Drops nodestore_read_us and everything added to reach it. read_mean_us already
carries microsecond precision and separated the two sync failure modes cleanly
in live testing -- 8.8 us on a clean store against a 223 us cold-store peak --
so the distribution added no signal that changed a diagnosis.
The cost of getting it was disproportionate. NodeStoreScheduler had no path to
the metrics registry, so its production constructor grew a ServiceRegistry
parameter: a metric addition changing a production signature. That in turn
forced an edit to a pre-existing test, src/test/app/SHAMapStore_test.cpp, whose
only stake in this is that it constructs a scheduler. Worse, the scheduler is
built in Application's member initializer list, long before metricsRegistry_
exists, so the registry could not be captured once and had to be re-resolved on
every fetch -- a lookup on a path that runs millions of times per sync.
The constructor returns to taking JobQueue& alone and SHAMapStore_test.cpp
returns to the single-argument call, leaving that file differing from its
pre-change form only by the NodeStore:: to node_store:: rename it picked up from
develop.
FetchReport::elapsed stays microseconds and onFetch keeps its explicit
duration_cast to milliseconds for addLoadEvents, which takes milliseconds. That
widening was a separate fix and is what makes read latency measurable at all.
kSubMillisecondBoundaries loses its only consumer and regains [[maybe_unused]],
which is the state the commit that introduced it left it in; without the
attribute an unused constant is an error under wextra with werr.
Also removes the ledger-data-sync panel that charted the histogram and the
fetch_type and found template variables, which filtered on labels no metric
emits any more, plus the runbook and reference-doc sections and the two
instrument and view counts that named it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
The phase-10 merge relocated PeerFinder's Manager interface from
src/xrpld/peerfinder/ to include/xrpl/peerfinder/, and SlotCensus moved with it
so libxrpl could see it. Logic.h uses SlotCensus as the return type of
getSlotCensus() but was never given the include, so every build failed:
build/modules/xrpl.libxrpl.peerfinder/xrpl/peerfinder/detail/Logic.h:192:5:
error: unknown type name 'SlotCensus'
...:197:16: error: use of undeclared identifier 'SlotCensus'; did you mean
'getSlotCensus'?
That single missing declaration was the whole failure. The 34 further errors in
the log were cascade: PeerFinderTest could not compile, so every TEST() in
src/tests/libxrpl/peerfinder/PeerFinder.cpp failed to instantiate against
gtest-internal.h. All four platforms (ubuntu-clang, ubuntu-gcc, macos-arm64,
windows-amd64) and clang-tidy reported the same root cause.
No include cycle: PeerfinderManager.h does not include detail/Logic.h, directly
or transitively. Levelization is unchanged because both headers are already in
xrpl.libxrpl.peerfinder, so generate.py produces no diff.
This is the risk called out when the merge landed -- moving SlotCensus into the
public header was the one resolution a static check could not confirm, and only
a compile would prove it. CI is that compile, and it found this.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
