Three conflicts, all between this branch's own sync-diagnostics work and
phase-10's older versions. Resolved to this branch in each case, since it
owns the newer content:
- InboundLedger.h keeps the missing-node and receive-depth gauges and the
fuller acquire-span contract.
- MetricsRegistry.cpp keeps the namespaced label:: constants.
- LedgerMaster.cpp keeps makeLedgerTraceSpan(), which joins the store and
validate spans into one per-ledger trace by hash.
LedgerMaster.cpp needed a second pass. The automatic merge had kept both
sides outside the conflict markers, nesting phase-10's older promotion
block inside this branch's `if (!pubLedger_)` — so setValidated,
setFull and setValidLedger would each have run twice. Taking this
branch's file wholesale removes the duplicate; brace balance and a single
"Advancing accepted ledger" confirm it.
That resolution drops two things phase-10 was carrying into this file:
the storeSpan/validateSpan guard names, and the explicit scope that keeps
the one-in-256 flag-ledger check outside the ledger.validate measurement.
Both are re-applied on this branch in the next commit; the scope needs a
variable-lifetime check that does not belong in a merge.
Eighteen conflict regions across nine files. Resolved by asking, per
region, which side is the better final state rather than by taking a
branch wholesale.
Telemetry.cpp keeps phase-9's two resource builders. phase-8 offered a
single makeResource() with no node identity; phase-9 splits it into
makeTracerResource() and makeMetricsResource() because the metrics
provider is built in the constructor, before setNodeId() runs, so
xrpl.node.id can only be stamped unconditionally on the tracer side.
Collapsing them would have dropped that attribute, which is what keeps
per-node traces from folding into one identity.
Telemetry.h and the config test compose both sides: phase-9's nodeId
member and its assertion, plus the renamed endpoint.
xrpld-telemetry.cfg keeps phase-9's devnet identity and its
metrics_endpoint, renames the traces key, and drops exporter=otlp_http.
Nothing reads an `exporter` key on any branch in the chain: it was a real
Setup member in the first phase-1b implementation, removed when only
OTLP/HTTP was wired up, and already deleted from TESTING.md once on the
same grounds. The cfg line was the last carrier.
The docs keep phase-9's versions, which are both fuller and more
accurate: the incoming runbook listed the consensus strategy values as
"random" where the code compares against "attribute".
OTelCollector.cpp had five comment-only regions in a file phase-7 owns,
so those take the upstream side.
MetricsRegistry.h's usage example named a member that no longer exists
and the wrong arity; it now matches the real three-argument call and says
where the endpoint comes from.
Ten conflict regions in four files, none of them caused by the rename.
phase-8's own commit had rewritten comments in files phase-7 owns
(Unit.h, HistogramBuckets.h, OTelCollector.cpp) and in Telemetry.cpp,
while the same sweep ran independently on phase-7.
Resolved every region to phase-7's side on ownership grounds: those files
belong to phase-7 or earlier, so a downstream branch should not carry
divergent copies. phase-8 changed comments only in all four, verified
against the merge base, so no code was dropped.
The one structural region: phase-7 had refactored addUnitView from an
inline lambda into a member function, and phase-8 still held the lambda.
Keeping phase-8's would have shadowed the member.
Wording phase-8 had that is worth restoring on phase-7 -- the "legacy"
qualifier on the prefix parameter, and the rejected-alternative note on
the bucket ladders -- is recorded outside the tree for a follow-up.
Three conflicts, all composed rather than resolved by taking a side:
- TelemetryConfig.cpp: phase-6 kept networkTypeFromId file-local with
[[nodiscard]]; phase-7 had relocated it to public scope for
Application.cpp. Kept phase-7's relocation, so one definition remains.
The [[nodiscard]] survives on the declaration in Telemetry.h.
- Telemetry.cpp x2: phase-7 added getMeter overrides, phase-6 added
[[nodiscard]] to the startSpan below them. Kept both, and put
[[nodiscard]] on getMeter too.
- TESTING.md: phase-7 had the right metric name (span_calls_total, which
the spanmetrics namespace produces) but the wrong label. Its
xrpl.rpc.command appears nowhere else in the branch; the attribute is
bare `command`, which is what the dashboards query. Took phase-7's
metric with the correct label.
Both signalEndpoint call sites follow the renamed member. signalEndpoint
itself is left in place: removing it and adding metrics_endpoint is a
design change, not part of propagating a rename.
The rename arrived from phase-1b by merge; the runbook still told
operators to set `endpoint`, which the parser no longer reads. Updates
the Quick Start ini block and the Configuration Reference row.
No metrics_endpoint row is added: this branch exports no metrics, so
documenting the key here would describe something the code ignores.
Harness and docs:
- integration-test.sh queried traces_span_metrics_* for spanmetrics, but this
branch sets the connector namespace to "span", so those two checks matched
nothing and failed. The dashboards and runbook had moved; the script had not.
- The same script queried eight native metric names with a product prefix and
capitals that formatName() cannot produce: it lowercases, maps '.' and ' ' to
'_', and prepends nothing. Corrected against the runbook tables.
- TESTING.md carried the same stale spanmetrics names and a jq example reading
a Prometheus label that does not exist.
- The runbook now records where each part of a derived metric name comes from,
since only the namespace is ours to choose.
Collector:
- OTelCounterImpl::increment silently dropped a negative amount. An OTel
counter takes unsigned deltas, so assert and let a release build under-count
rather than wrap.
- OTelGaugeImpl::increment computed current + amount in int64, which is
undefined on overflow, and the clamp ran afterwards so it could not help.
Check the headroom first. set() now clamps rather than casting a uint64 above
INT64_MAX to a negative, which is what made underflow reachable.
- The meter scope was two bare literals. They are constants now, and
Telemetry.cpp static_asserts them equal to kMeterName and kMeterVersion:
beast cannot include the telemetry header, so a build failure is the only way
to catch the copies drifting.
- formatName uses views::transform and ranges::to, as Backend.cpp already does.
- Unused constructor parameters take [[maybe_unused]] instead of (void) casts.
- The destructor logged "shutting down" and "stopped" with nothing between.
initMetrics was 79 lines doing four jobs. The exporter and the histogram views
are separate functions now, addUnitView is a member rather than a lambda
capturing this, and the export interval and timeout are named. It also derived
the metrics URL from the traces URL by suffix swap, which sent metrics to the
traces path whenever the configured URL had any other shape; both URLs now come
from one rule that handles a bare host, a trailing slash and either signal path.
Review feedback on the testing guide:
- rm -rf targeted data/, but this config writes under docker/telemetry/data/,
so teardown did nothing and a second run reused the old NuDB and SQLite
state. Corrected at both sites, including the Test 2 keygen node, which
launches with the same config.
- The standalone span table said consensus.* does not fire. It does:
ledger_accept drives a simulated round, so consensus.round, .phase.open,
.ledger_close, .accept and .accept.apply all appear. Only .establish,
.update_positions, .check, .proposal.* , .validation.receive and
.mode_change cannot. The test intro claimed the same thing and now agrees
with the table.
- Three blocks duplicated content the file already had. Test 1 now points at
the shared Verification Queries section as Test 2 already did, and the
Test 2 submit block checks engine_result like Test 1 does.
- The numbered step list was a copy of the script's own Step N headers and had
drifted by four entries, so it now points at those headers instead.
Also corrects the runbook's ledger and peer span tables against the code:
ledger.build was credited with tx_count and tx_failed, which tx.apply sets,
and was missing its three close-time attributes; peer.validation.receive was
missing ledger_hash and full_validation. The five source line numbers in those
two tables were stale, so they now name the file only, as the other nineteen
rows do.
Notes across the workload contract described earlier versions of themselves, or
cited commits that only exist inside this chain. A squash merge publishes none of
it, so each reference resolves nowhere.
Notes that described their own earlier text:
- expected_spans.json: 'this note previously concluded', 'this note previously
said', 'Un-skipped 2026-08-26', 'the reason had simply gone stale for two
weeks' and 'the claim this entry carried' are replaced by the standing reason
each entry holds. The wildcard pairs now say the validator globs the child via
_span_name_matches(), and state the literal-collapse failure as what a
different validator WOULD do rather than as history.
- regression-thresholds.json: 'an earlier version of this note wrongly claimed',
'the earlier version oversold it' and 'an earlier note called that' become the
cautions themselves -- do not reason from 'every ladder step is at least 2x',
do not oversell the backstop, do not read a false fire as a missing override.
- test_check_regression_bounds.py: the docstring gives the reason a literal is
wrong here, not the story of two tests that once hard-coded one.
Baseline-refresh history rewritten as measurement:
- README.md, baselines/README.md, telemetry-runbook.md and regression-metrics.json
no longer attribute threshold moves to 'the 2026-08-26 refresh'. The evidence
is kept as measurement -- span.tx.apply.p50 has read 0.7917 ms and 0.00597 ms
on the same workload, 132x apart; job.acceptLedger.running.p95 has measured a
5.74x floor on one baseline and 16.28x on another -- which is what supports the
claim that a single-run baseline cannot bound these keys.
Two chain-only commit ids removed, d059f21bf3 and 3860c93db2. Neither is
reachable from develop, so both cease to exist on merge; the second is chain
bookkeeping. The facts they were cited for (the validator globs wildcards; the
span ladder's floor is 0.01 ms) are stated directly instead.
Capture provenance is deliberately kept: baseline-timings.json 'captured_at',
the 2026-08-26 baseline heading, and the 2026-08-24 figures cited as data.
Documentation, JSON note strings and one docstring only, no behaviour change.
The rule's rationale said its three points were 'learned from a dataset that an
earlier version of this table got wrong'. That earlier table exists only in this
branch's intermediate commits, which a squash merge does not publish, so the
sentence points at nothing a reader can check.
The measured figures behind each point are unchanged.
Documentation only.
Comments across the sync-diagnostic work described earlier revisions of the same
change, or cited identifiers a reader of the merged tree cannot resolve.
Prior-state comparisons rewritten in the present tense:
- MetricsRegistry.cpp carried two adjacent paragraphs prescribing opposite
behaviour for a disabled quorum, one publishing int64 max and one omitting
the series. The code omits it; the superseded paragraph is gone and the
surviving reason SIZE_MAX must not be cast is kept.
- MallocTrim, LedgerMaster, LedgerReplayTask, TransactionAcquire, Application:
say what the signal is the only record of, rather than what was 'previously
trace-only', 'not logged at all here' or 'used to sit inside if (debug())'.
- LedgerMaster.h and SpanGuardScope: without an explicit join each ledger's
spans WOULD be separate traces -- not that they were 'before this'.
- Handshake: the message is forwarded byte for byte, not 'byte-identical to the
previous behaviour', and the helper throws rather than 'throws as before'.
- MetricNames: quorum_disabled is a separate boolean rather than a sentinel,
stated without what the state 'used to be encoded by'.
- LedgerMaster.cpp no longer claims to mirror the unl_quorum gauge; it does not.
That gauge omits the series while this stores int64 max.
- 'Split out of' / 'Split from' become 'Kept separate from' in five places.
Plan-internal identifiers removed:
- All 24 WP-Ax / WP-Bx work-package labels across the telemetry tests, the
collector configs, tempo.yaml and the expected_* inventories. They are defined
in no file in the repo, so they resolve nowhere once merged.
- The two references to OpenTelemetryPlan/, which does not reach develop, now
point at docs/telemetry-glossary.md 'Fresh-node sync diagnostics'.
Comments and JSON note strings only, no behaviour change.
Compare-then-store is the recurring shape behind these values: read whether
an incoming value differs from the last, store it, report only on a change.
Document changedTo() as doing all three, and state the two constraints -- it
is not synchronized, so it cannot replace an atomic read from another thread,
and it requires a copyable T so its copy semantics match across builds.
Recording.h declares Mirror beside Stopwatch and Counter, so its overview
diagram lists Mirror too and its thread-safety note covers it.
State that exists only to be reported was being written with preprocessor
branches at each site, which put #ifdef through business logic and gave the
owning class a different member set per build.
Document kEnabled, Stopwatch and Counter, and the two constraints that decide
whether they fit a site: a no-op method still evaluates its arguments, and
if constexpr still type-checks the branch it discards.
The table of contents in this file indexes third- and fourth-level headings,
so a new subsection that is absent from it is a gap rather than a style
choice.
Compiled out, compiled in and tracing, compiled in with no active span, and
compiled in but disabled by config all have to produce the right wire bytes,
and only two of them are obvious. Tabulate them, and record why the predicate
reads the context directly instead of calling GetSpan(), which allocates a
DefaultSpan in the no-span case.
mutable_ on a protobuf optional submessage allocates it and sets its has-bit
at the call site, before the helper can decide there is nothing to write. A
caller that dereferences it ships an empty TraceContext whenever nothing is
recorded, and its peers each take a branch to extract nothing.
Document the rule with the right and wrong forms side by side.
The conditional-compilation section promised zero overhead when telemetry is
not wanted. The span disappears, but the arguments passed to it do not: the
compiled-out guards are ordinary inline functions, so a to_string() or a hash
in an argument list still runs and its result is then discarded.
State that, show the guard that does remove the work, and name the opposite
case -- the metric macros, which discard their arguments and need no guard.
Two conflicts.
RCLConsensus.cpp: upstream restructured makeAcceptSpan so the accept span's
attributes sit behind if (*span). This branch's own contribution there is the
consensus round-duration histogram, which is kept -- placed inside the
telemetry guard but OUTSIDE the span-liveness test, because a metric must
still record when the trace category is disabled or the span was not created.
The duplicated attribute lines on this side are dropped; the guarded block
upstream added supersedes them.
MetricsRegistry.cpp: kept this branch's JobQueue.h include, which it uses.
Its Journal.h include was dropped as a duplicate -- the file already includes
that header higher up, with a comment explaining why it is unguarded, and
readability-duplicate-include is fatal under WarningsAsErrors.
Four conflicts. None was a take-a-side.
xrpld-telemetry.cfg: dropped the incoming [insight] block. This branch already
has one, and duplicate ini sections do not replace each other -- parseIniFile
appends onto the same section, so keys merge last-wins and the effective config
is one that appears nowhere in the file.
src/tests/libxrpl/CMakeLists.txt: kept this branch's else() branch, which
compiles MetricsRegistry.cpp for the telemetry-off build, and dropped only the
ValidationTracker target_sources inside if(telemetry). Upstream relocated that
one out of the guard, so keeping both would have compiled it twice. The
else() branch is this branch's own: the MetricsRegistry test exists only here,
and without its implementation the off build would not link.
RCLConsensus.cpp: union. The metric macros and registry come from this branch,
PropagationHelpers from upstream; all three are used.
PeerImp.cpp: MetricMacros.h stays unguarded, because all seven XRPL_METRIC_*
uses in this file are on unconditional paths and the header is what defines
them. ConsensusReceiveTracing.h takes upstream's guarded placement, and this
branch's guarded GetObjectMetricNames.h is kept beside it.
formatName() never reads prefix, so setting it here does nothing and the
exported names are bare and lowercase. Leaving it invites queries written
against xrpld_jobq_job_count, which match no series.
The StatsD examples keep it, because that path does apply it to the name.
The consensus round-flow diagram marked `acquireTxSet -> gotTxSet` as having no
span, and styled it as a step with no instrumentation. That was true when the
diagram was written and stopped being true on this branch, which added the span.
The fetch is now `txset.acquire`, carrying one `round.request` event per round
that asked for the set. The `gotTxSet` delivery still has no span of its own, so
a set that arrives too late for the round to use it leaves no trace -- worth
stating, because that is the case an operator goes looking for.
Corrected on this branch rather than upstream on purpose: phase-9 and phase-10
carry the same diagram but not the span, so "no span" is accurate there. Moving
the fix upstream would describe instrumentation those branches do not have.
Brings phase-10 up to f13524c93c, one commit: every harness failure now maps to
exit 2 and timings are captured unconditionally.
Merged clean -- git reported no conflicts, so no resolution decisions were made
here. Two incoming files, run-full-validation.sh and the runbook. No runtime C++,
no include changes, no change to either expected_*.json contract.
Relevant to this branch: the previous run failed the job through the regression
gate while the validation suite itself passed 264/264, and the failing step was
the reporter keying on the validation step's outcome rather than anything the
suite reported. A single exit code for every harness failure makes which stage
failed legible from the exit status instead of only from the log.
Two defects raised in review of PR 6519, both about the harness misreporting
its own state.
The script documents exit 2 for an infrastructure failure and routes that
through die(), but eleven commands were unguarded, so under set -euo pipefail a
failure aborted with the tool's own status instead. Measured before the fix:
docker compose exited 125, the key generator 7, a jq read 5, and several others
1 -- which the table defines as "checks failed", so an infrastructure problem
was reported as a validation result. Two of the eleven are worth naming. A
trailing option with no value (--nodes at the end of the command line) exited 1
because set -u aborted on the unset positional, now unified through one
require_value helper. And report_stopped_nodes, which runs immediately before a
die, contained an unguarded pipeline that tripped errexit, so the die never ran
and a crashed cluster reported 1 -- the script failed to report the exact
condition the contract exists for. Commands whose failure is genuinely
tolerated were left alone.
The seed read also gained a value check, because jq prints the string "null" and
exits 0 for a missing key, so testing only the exit status cannot see it.
Step 6 said it "ALWAYS captures timings (so CI always has an artifact from which
to bootstrap/refresh the committed baseline)" while the capture sat inside the
--skip-regression guard. The comment stated the intent and the code was the bug:
that artifact is the only route to a refreshed baseline, and the workflow reads
it unconditionally to print the paste-me block. Capture now always runs and only
the comparison is gated. A capture failure still surfaces, folding into the exit
code only when the gate is active, so --skip-regression cannot start failing
runs that previously passed.
Note a non-zero capture status does not mean the file is absent: capture_timings
writes it and then fails the minimum-ratio check, so the artifact exists but is
incomplete. The messages say incomplete rather than missing, so nobody goes
looking for a file that is already there.
The runbook's matching claims are corrected in the same commit: it said
--skip-regression skips the capture, and its exit-code summary predated the
uniform mapping.
Brings phase-10 up to 29673de531, three commits: a Loki-diagnostic count fix with
Tempo errors made visible, a baseline recapture that stops gating keys whose
run-to-run variance dominates their bound, and WebSocket reply correlation with
silent placeholder data removed.
Merged clean -- git reported no conflicts, so no resolution decisions were made
here. The 14 incoming files are all harness and docs: six .py, three .md, three
.json, two .sh. No runtime C++ and no include-line changes.
The baseline recapture resolves the regression gate that reddened the previous
run on this branch. That run reported span.consensus.ledger_close p95 1.62ms and
p99 7.14ms against a baseline of 0.49 and 0.93; the recaptured baseline puts p95
at 0.783 with a 5ms trip point and p99 at 2.03 with a 10ms trip point, so both
readings now sit inside the gate. That matches the diagnosis recorded at the
time: everything changed between the last green gate and that failure was a .py,
a .md and a GTest file, with zero runtime C++, and p50 had improved while only
the tail moved -- variance, not a slowdown. The incoming commit reaches the same
conclusion from the other side, excluding three p50 keys after measuring spreads
of 391.8x, 20.7x and 6.1x across three runs.
Refreshes baselines/baseline-timings.json from run 32964262700 at 8418d474a7,
byte-identical to the CI artifact. The previous baseline was captured at
6a82fc6f37, before the path-finding load was removed from the workload, so it
described a load shape the harness no longer runs.
Every absolute bound is re-derived, because the rule is hi_next minus baseline
and the baselines moved.
Three more keys stop being gated: span.tx.apply.p50, span.ledger.build.p50 and
span.consensus.ledger_close.p50. This is the rule the previous commit recorded
being applied, not a new exception -- a key is gateable only when its
run-to-run spread fits inside its bound.
The evidence is span.tx.apply.p50, which read 0.7917 ms in the old baseline and
0.00597 ms in this one. That is a 132x move between two runs of the SAME
workload. The old value happened to land mid-distribution, so hi_next minus
baseline gave a 4.21 ms bound that absorbed the spread; the new value lands in
the ladder's first bucket, so the same rule gives 0.0440 ms and cannot survive
one. Whether the gate functioned was decided by where in the distribution the
captured run happened to fall, which is not a threshold in need of tuning.
Measured spreads across four runs agree: 364x, 25.3x and 5.9x respectively.
All five excluded keys share one shape -- a baseline landing in the ladder's
low buckets, where the derived bound is tiny, together with large run-to-run
spread. Single-run baselines cannot support them; a multi-run baseline, or a
spread measurement captured alongside the baseline, is what would let them be
gated again. Not attempted here.
Both runs that would have reddened CI now replay clean, and an injected 10x
regression is still caught on 19 of the 20 remaining keys, 20 of 20 at 20x.
The exception is job.acceptLedger.running.p95, whose baseline fell while its
hi_next did not, moving its floor to 16.28x. It stays gated with that floor
recorded beside the other weak keys.
Also makes the bounds checker report a zero or negative baseline as a named
rule failure instead of dividing by it and raising.
Brings phase-10 up to 8418d474a7, one commit: the span reverse-coverage check
was querying the tag `span.name`, but a span's name is a TraceQL intrinsic
rather than a span-scoped attribute, so Tempo answered 200 with an empty
tagValues list and the check silently never evaluated. It now queries the bare
`name` intrinsic.
That is the same inertness the previous CI round on this branch observed from the
other end -- the run logged "Tempo span names (0 total)" while per-span TraceQL
searches each found traces and a logged trace id resolved to 100 spans. So the
incoming fix converts a check that could only ever pass into one that can
actually fail.
validate_telemetry.py auto-merged: the incoming change and this branch's are in
different regions. Verified afterwards that both survived -- the bare `name`
endpoint is in and the old `span.name` one is gone, alongside this branch's
assert_sync_diagnostics_metrics, its gather-based fan-out, and the
SKIPPED_METRIC_GROUPS exclusion inside _metric_check_targets that keeps the
sync-diagnostics group from being walked twice.
One conflict, in the runbook's validation-coverage table, and it needed both
sides rather than either. This branch's copy carries the counts recomputed from
the real contract during the previous merge (48 span types as 28 required and 20
optional, 145 metric checks across 26 categories, 16 dashboards); phase-10's copy
still carries the pre-merge figures. But phase-10's copy also corrected the
Reverse coverage row's description from the `span.name` attribute to the `name`
intrinsic, which is precisely the bug its commit fixes -- this branch's row still
described the broken query. Resolved as this branch's rows with phase-10's
Reverse coverage row substituted in. Every other cell was byte-identical between
the two sides apart from separator padding.
Verification: no conflict markers repo-wide; two parents; validate_telemetry.py
compiles; both sides' contributions asserted present by name rather than assumed;
otel-naming exits 0 including Rule E over the edited doc; levelization baseline
clean and the incoming diff changes no include lines; pre-commit clean on both
files, prettier having re-padded only the eight table rows, with the recomputed
counts and the corrected intrinsic wording confirmed present afterwards. No C++
changed, so no compile is implicated by this merge.
The span reverse-coverage check has never evaluated. It reported "no span
names were reported (backend unreachable or empty)" on a run where Tempo
demonstrably held data -- the same run resolved a logged trace id to 32
spans.
Root cause: the tag-values query asked for `span.name`. A span's name is a
TraceQL intrinsic, not a span-scoped attribute, so `span.name` resolves to
an attribute nothing sets. Tempo answers 200 with an empty tagValues list,
which is indistinguishable from an empty backend and never raises, so the
surrounding try/except stayed silent.
Verified against tempo 2.9.4 holding exactly one span named
probe.reverse.coverage, with the collector in front of it:
/api/v2/search/tag/span.name/values -> {"tagValues":[]}
/api/v2/search/tag/name/values -> that span's name
/api/v2/search/tag/resource.service.name/values -> xrpld
The third line is the control: the span was in Tempo, so the first line's
emptiness was the wrong tag rather than no data. Cross-checked against a
populated Tempo elsewhere, whose span scope lists real attributes
(command, ledger_seq, tx_hash) and no name tag at all, while the bare
intrinsic returns the whole span inventory.
This is pre-existing, not a regression in the reverse check: the same URL
fed the operations diagnostic before that check existed, and the last
green run before it also logged "Tempo operations (0 total)". The check
faithfully reported an empty input; the input was broken.
The neighbouring resource.service.name query is correctly scoped and is
left alone.
