Three harness comments described the behaviour this change replaced, which the
squash merge does not publish.
- run-full-validation.sh: the capture flag means CAPTURE_EXIT is not the only
record of capture health; and a gated capture failure is an infrastructure
error, stated without 'exactly as before'.
- tx_submitter.py: give the reason the first occurrence logs at WARNING (DEBUG
is off in CI) rather than what a failed run 'previously produced'.
- workload_orchestrator.py: a wedged process cannot stall the profile, rather
than 'can no longer'.
Comments only, no behaviour change.
capture_timings.py decides whether a captured timings file may become a
regression baseline. Every way of getting that wrong is silently green: a
capture that asked Prometheus for nothing still writes valid JSON, and once
accepted it is pasted in as a baseline, still reads as a placeholder, and the
regression gate stays off while the workflow reports it as activated.
Covered: an empty surface is not complete (0 of 0 is 100% by arithmetic), the
minimum ratio is inclusive, null values count as declared but not captured, the
threshold is recorded so a rejected capture can be judged later, and the exit
code follows the flag rather than recomputing the ratio. The empty case has its
own error path because the percentage message divides by the declared count.
Neither this file nor test_validate_telemetry.py ran anywhere before: not in
CI, not in run-full-validation.sh, not in pre-commit. They now run in the
naming job, which is fast and fires on nearly every PR, so a broken harness
surfaces in seconds rather than after an xrpld build.
They run as plain scripts. unittest discover would collect nothing from them,
since they hold bare functions rather than TestCase subclasses, and would exit
0 -- which is why each file fails when it collects no tests. The dependency
install is a separate step, placed after every stdlib-only check so those stay
reachable if PyPI is unavailable.
The check reported span.hierarchy.<parent>-><child> and a message reading
"Found <child> as child of <parent>" on the strength of both names appearing
somewhere in the same trace. A span parented by something unrelated passed, so
the one property the check exists to prove was never tested.
It now walks the child's parentSpanId chain looking for a span matching the
parent name. Ancestry rather than a direct edge, because all 21 declared
relationships are worded as the parent containing the child, so a scope
appearing in between is a refactor and not a broken relationship. Span ids are
compared as opaque strings: both fields come from the same Tempo response and
share its encoding, so nothing here depends on whether that is hex or base64.
Co-occurrence is still the search filter, which is what lets a conditional
child be found in an older trace instead of only the newest ones.
Verdicts are separated because they send the reader to different places: a
child that is present but not under the parent is a hierarchy bug, a chain
running into a span the trace lacks is one that never reached Tempo, and an
unusable parent span is neither. A definite negative outranks an indefinite
one, and one trace proving ancestry settles the relationship.
Tests cover each verdict plus the cross-trace and cyclic-chain cases, and each
one was checked against the specific defect it names. The runner now fails when
it collects no tests and reports SystemExit, both of which otherwise produce a
silent pass.
The pathfind.request skip_reason said only the child side handles globs. Both
sides do now; the blocker is the literal parent name in the Tempo query, so the
skip itself stands.
The comments I added with the hierarchy sampling fix and the trigger change ran
to sixteen and twelve lines. The guideline is short and plain English. Rationale,
CI run numbers and the list of which relationships were affected belong in the
commit message, which is where they already are; inline they push the code apart
and go stale as soon as the reasons change.
Trimmed the sampling comment from sixteen lines to four, the re-check comment
from eight to four, _traceql_name_predicate's docstring from fourteen lines of
explanation to three, and the push-trigger comment from twelve to seven. Each
keeps what a reader needs at that line -- what the code does and the one
non-obvious reason -- and drops the history.
Comment-only: 13 insertions against 35 deletions, no statement changed.
Left alone deliberately: this file has ten pre-existing comment blocks longer
than six lines, including one added recently by another party. Rewriting someone
else's comments is not mine to do here, and the guideline is being applied to what
I wrote.
Verification: 7/7 validator tests pass; validate_telemetry.py compiles; the
workflow YAML parses, still carries no branches filter, and still lists 12 paths;
otel-naming exits 0.
An empty metric surface counted as a complete capture. build_query_plan
returns an empty plan without complaining for any config that yields no
gated keys, so pointing --metrics at the wrong file exits 0 and hands the
paste-me path a metrics:{} artifact to offer as the next baseline. Nothing
about such a run is evidence the pipeline works, so declared == 0 is now a
failure rather than vacuously complete.
The bounds checker also raised AttributeError on a baseline entry that is
not an object, instead of naming the key. A validator whose job is to catch
a malformed contract should report it, not crash on it.
Test cleanup is bound to its own temp tree, so a loop no longer leaves five
of six directories behind.
The conjunction query works. Run 33062418036 proved it on real Tempo: both
hierarchies that newest-N sampling made unassertable now PASS --
txq.accept -> txq.accept_tx and ledger.acquire -> ledger.acquire.txtree -- along
with every other literal-child pair. Only the two wildcard children failed, and
not because of the sampling change.
They failed with HTTP 400, "invalid TraceQL query: parse error at line 1, col 68:
invalid char escape". _traceql_name_predicate built the pattern with re.escape,
giving name=~"rpc\.command\..*", and TraceQL's string lexer refuses a backslash
escape it does not recognise -- the query never reached the regex engine at all. A
literal dot is now written as the character class [.], which carries no backslash
for the lexer to refuse while still meaning a literal dot to the engine behind it.
Leaving the dots bare would have parsed, but would match any character in those
positions, which is the looseness _span_name_matches exists to avoid.
The builder now also rejects a span name containing anything outside
lower_snake_case, dots and the glob star, rather than passing it through
unescaped. Every name in the contract is of that shape, so this changes nothing
today; it exists because the failure mode it guards against is exactly the one
above -- a character that means something to one layer and something else to the
next, discovered only from a 400 in CI.
Worth recording why the tests did not catch this. The stub evaluated the pattern
with Python's re, which accepts \. happily, so it modelled the regex engine and
not the query lexer sitting in front of it. A stub is only as good as the layer it
imitates, and the layer that rejected this was one the stub did not represent. The
new test therefore asserts the property the lexer enforces -- that no backslash
appears in the predicate at all -- rather than any particular spelling, plus that
the pattern still accepts rpc.command.fee and still rejects a near-miss whose
separators are not dots.
Verification: 7/7 tests pass, and the new one was watched failing first with the
exact string Tempo rejected, name=~"rpc\.command\..*"; the full query the check
now builds was printed and confirmed backslash-free; validate_telemetry.py
compiles. Three unrelated files in this worktree are another party's live work and
were left unstaged.
The hierarchy check searched the parent span and inspected the three newest
traces it returned. That is wrong whenever the child is conditional on a state
the workload only sometimes reaches: the parent fires constantly, so its newest
traces are the ones LEAST likely to carry a rare child. Three relationships had
been skipped as unassertable for exactly this, and in none of them was the child
missing -- each emitted traces of its own and simply was not in the three most
recent parent traces.
The check now issues a second query, a TraceQL trace-level conjunction of the
parent and child name predicates, and inspects those traces. Tempo searches its
whole retention for co-occurrence instead of leaving the answer to which traces
happen to be newest. The parent-only query is kept and still runs first, so "the
parent stopped being emitted" stays a distinct failure from "the parent is there
but the child never co-occurs" -- they mean different things to whoever reads the
report, and collapsing them would lose that.
The returned traces are still verified with _span_name_matches rather than the
query result being trusted on its own. Tempo has already guaranteed
co-occurrence, so this is redundant on the happy path; it is kept because it
keeps the glob semantics in one place and means a wrongly built query cannot
silently pass.
_traceql_name_predicate handles the wildcard contracts. TraceQL has no glob
operator, so `rpc.command.*` is sent as name=~"rpc\.command\..*" with the dots
escaped -- unescaped they would match any character in those positions, which is
the looseness _span_name_matches exists to avoid.
Two entries follow from the fix. txq.accept -> txq.accept_tx is asserted again:
its child is created inside the queued-transaction loop behind
`if (feeLevelPaid >= requiredFeeLevel)` (TxQ.cpp:1530) while the parent fires on
every close (:1499), which was the whole reason it failed. txq.enqueue ->
txq.batch_clear stays skipped but for ONE reason now instead of two -- its child
never fires at all under this workload, needing an account with a supersedable
batch, so it is purely a workload gap and needs nothing further from the
validator. The third, ledger.acquire -> ledger.acquire.txtree, lives on the
sync-diagnostics branch and is un-skipped there once this merges forward.
Written test-first, and the first test this module has had. The failing test
reproduces the exact CI message, "txq.accept_tx not found in txq.accept traces",
against a stubbed Tempo whose corpus holds the child only in a trace outside the
newest three. Three sibling tests guard the ways this could be "fixed" wrongly: an
absent child must still fail, a missing parent must still name the parent rather
than the child, and a wildcard child must be satisfied by any family member. The
stub records the queries issued, so the conjunction is asserted rather than
assumed. A stub rather than a live Tempo because the behaviour under test is which
traces the check ASKS FOR -- a passing query against real data proves the data
co-operated, not that the query was right.
The first run of those tests failed for the wrong reason: my stub's name-predicate
regex also matched the resource.service.name="xrpld" term every query carries and
so demanded a span literally named "xrpld". Fixed in the stub, with the lookbehind
commented as load-bearing, before touching production code.
Verification: 4/4 tests pass, and the failing one was watched failing first with
the production message; the issued queries were printed and confirmed to contain
the conjunction; validate_telemetry.py compiles; expected_spans.json parses;
21 relationships, 16 asserted and 5 skipped; counters still 41 span types;
otel-naming exits 0. Three unrelated files in this worktree are another party's
live work and were deliberately left unstaged.
The regression baseline is bootstrapped by copying a CI artifact. The workflow
tested only that timings.json existed, then printed it verbatim under a heading
inviting the reader to paste it in as the new baseline.
capture_timings.py writes that file and only then enforces --min-capture-ratio,
so an incomplete capture leaves a file that exists but covers fewer keys than
the contract declares. The verdict lived in CAPTURE_EXIT, a shell variable local
to run-full-validation.sh that no other program could read. So on a placeholder
baseline plus a thin capture, CI offered an incomplete artifact as the next
baseline, and pasting it narrowed the gate with nothing reporting that it had.
That is the failure shape this harness keeps producing: a degraded result that
looks exactly like a good one.
The artifact now carries its own completeness, next to metrics:
"capture": { "declared": 20, "captured": 20, "min_ratio": 0.5, "complete": true }
complete is the same condition the producer exits 0 on, computed once with the
exit code read off it, so the flag and the status cannot drift apart. Any
consumer can now tell a complete capture from a thin one, not just CI.
Both paste-me paths refuse rather than warn: the workflow prints the counts and
an error annotation with no JSON, and the comparator explains on stderr while
leaving stdout empty, so a redirect cannot produce a plausible-looking file. A
warning above a copyable block is still a copyable block, and a reader who has
just hit a red gate is already predisposed to re-baseline. A missing capture
block fails closed.
Refusal is scoped to bootstrapping a baseline, not to comparing against one, so
artifacts captured before this change still replay: verified against the run the
current baseline came from, which carries no capture block and still reports 0
regressions. An injected regression is still caught, and the gated surface is
unchanged at 20 keys with 5 excluded.
Each span entry documents its parent, and a separate list holds the pairs the
validator actually checks. Three parentings were declared on the span entries and
absent from that list entirely, so they were neither asserted nor recorded as
unassertable -- silently missing rather than deliberately skipped. All three are
now listed, skipped, each with the reason that actually applies. Every span
declaring a parent now has an entry: the count went from 3 unaccounted to 0.
txq.enqueue -> txq.batch_clear is conditional and narrowly so. The child is
created in TxQ::tryClearAccountQueueUpThruTx (TxQ.cpp:550), which needs one
account holding several queued transactions AND an arriving transaction that
supersedes the batch. txq-burst produces queueing but arranges no such shape, and
it has never been observed on a run. It would also meet the sampling limit that
forced the txq.accept_tx skip, so fixing the sampling addresses both at once.
rpc.command.* -> pathfind.request is the one skip caused by a wildcard PARENT
rather than by a missing span, and the asymmetry is worth recording:
_validate_parent_child inserts the parent name literally into its Tempo query
(:801), so a wildcard parent matches nothing, while the CHILD side globs through
_span_name_matches (:826-828). That is exactly why rpc.ws_message ->
rpc.command.* can be asserted and this cannot.
pathfind.compute -> pathfind.discover has both ends absent, for the reason the
pathfind.compute entry already sets out at length: pathfinding is disabled on
every harness node because Config.cpp:725-726 zeroes pathSearchMax when a
[validation_seed] is present, and since 2026-08-25 no path-finding RPC is issued
either. Listed so the family is fully accounted for rather than partly silent.
No assertion is added or removed here -- this is accounting. The plan task that
prompted it also assumed the pathfind.compute skip reason was stale and needed
correcting; it is not, it already names both blockers and corrects an older
liquidity-based reason, so that half of the task was a defect in my plan rather
than in the file.
Verification: JSON parses; 21 relationships, 15 asserted and 6 skipped; no
duplicates; 0 spans declaring a parent without an entry, down from 3; counters
still 41 span types; otel-naming exits 0; pre-commit clean.
Both rpc.command.* relationships were skipped on the claim that
_validate_parent_child collapses a wildcard child to one literal name via
child_name.replace("*", "server_info"). That code does not exist. d059f21bf3
removed it on 2026-08-14 and replaced it with _span_name_matches(), which globs
through fnmatch.fnmatchcase; the check's own comment now reads "globs for
wildcard contracts". So any rpc.command.<anything> under the parent satisfies the
contract, and the command mix the sampled traces happen to carry no longer
matters -- which was the entire basis of the skip. The wildcard_probes map that
does still substitute a literal name belongs to the span-EXISTENCE check
(validate_telemetry.py:545, :554), not to the hierarchy check.
The WebSocket entry's reason went stale the day that code was deleted. The
rpc.process entry's is worse and is mine: c531ac569b rewrote that reason to fix a
different error in it -- it had claimed rpc.process cannot appear under a
WebSocket-only harness, when it appears on every run because
run-full-validation.sh polls each node over HTTP with curl -- and while fixing
that I copied the wildcard claim across from the stale WS entry without checking
it. Correcting one false statement in a note is not a licence to inherit the
next one.
Both are now asserted. Both parents emit on a normal run: rpc.ws_message is the
WebSocket root the load generator drives, and rpc.process reports 5 traces from
the curl readiness and validated-ledger polls, every one of which runs a command.
This also retires the plan's Task 5 without writing any validator code. The task
was scoped as "teach the validator to match a wildcard child"; it already does,
and had for two weeks. Checking the code before writing the feature turned a code
change into a data change.
Verification: JSON parses; 18 relationships, 15 asserted and 3 skipped, up from
13 asserted; the three remaining skips are txq.accept_tx (newest-N sampling of a
conditional child), rpc.ws_message -> rpc.process (genuinely not a code
relationship) and pathfind.compute (child never fires); counters still 41 span
types; churn 2/6; otel-naming exits 0; pre-commit clean. Whether these two hold in
a real trace is what the next run decides -- both ends emitting is necessary, not
sufficient.
c531ac569b asserted txq.accept -> txq.accept_tx. Run 32990348089 failed it:
"txq.accept_tx not found in txq.accept traces", the only failure in 278 checks.
Skipped rather than left red.
Not a missing span, and not an xrpld defect. Both ends emit on that same run, 5
traces each with all their attributes. The assertion was simply stronger than the
check can evaluate, and the reason is a conditional child meeting newest-N
sampling.
The parent is created once per accept pass, so every ledger close (TxQ.cpp:1499).
The child is created inside the loop over queued transactions and behind
`if (feeLevelPaid >= requiredFeeLevel)` (TxQ.cpp:1530), so it exists only for a
close where the queue actually held a transaction whose fee cleared the level.
_validate_parent_child searches the parent with limit=3
(validate_telemetry.py:803). Queue pressure comes from workload phase 5 of 7,
txq-burst, and mixed-peak (60s) then cooldown (30s) run after it -- so by the time
validation queries, the three newest txq.accept traces are quiet closes with an
empty queue and no child to find.
That is the same shape as the rpc.command.* skips already in this file: sampling
the newest traces of the parent is wrong whenever the child is conditional on load
that has since stopped. Recorded in the reason, with the two real fixes in
preference order -- prefer parent traces that contain the child via a TraceQL
child filter instead of newest-N, or move txq-burst to the final workload phase.
Raising the limit alone only shifts the odds, which would make the check flaky
rather than correct, so it is named and rejected there.
The other 13 assertions added in c531ac569b all PASS, including the three
consensus.round children, the two consensus.establish children,
rpc.http_request -> rpc.process and the three ledger.acquire phases. The
regression gate is clean at 0 regressions now that phase-10 recaptured the
baseline, and both reverse-coverage checks pass.
Verification: JSON parses; 18 relationships, 13 asserted and 5 skipped; counters
still 41 span types; churn 3/1, surgical; otel-naming exits 0; pre-commit clean.
Two problems, both about coverage this workflow claims to have and does not.
The push trigger gated on branch NAME as well as path, and GitHub ANDs the two.
Branch names are not something this repository controls, so a push to any branch
outside "pratik/otel-phase*", "feature/otel-*" or "feature/telemetry-*" was never
dispatched -- not queued, not skipped, no run to look at. That is not a
theoretical gap: two rounds of harness fixes on pratik/otel-sync-diagnostics
produced no signal at all before anyone noticed the workflow had never started.
The branches filter is removed; the paths already express the real question.
The path list was also incomplete in a way that matters more than it looks. The
span-name and metric-name headers are the wire contract this harness asserts
against by literal string, and the convention colocates each one with the class
it serves -- so eight of the ten *SpanNames.h headers live under consensus/,
overlay/, app/ledger/, app/main/, app/misc/, rpc/ and tx/, none of which was
matched. Renaming a span constant therefore compiled clean, emptied the
assertions and triggered nothing. Matched now by filename, "**/*SpanNames.h" and
"**/*MetricNames.h", so future headers are covered wherever they land. Added for
the same reason: include/xrpl/beast/insight (the interface headers decide what
the collector can publish, so they move the metric surface as surely as the
implementation), src/tests/libxrpl/telemetry (the GTests pinning those
constants), and the two checker directories that gate this surface in CI.
Second, the span hierarchy. Each span entry documents its parent, and a separate
list holds the pairs the validator actually checks in Tempo. Those had drifted
apart: 18 parentings were documented, 7 were checked. A span that stops nesting
under its parent -- which is what a detached guard does -- leaves every span and
every attribute intact, so no other check in this harness notices; the trace
simply stops being readable as one operation. Eleven pairs are added, each one
where both ends emitted on a real run: rpc.http_request -> rpc.process, the three
txq parentings, and seven consensus ones under consensus.round and
consensus.establish. Fourteen of eighteen are now asserted; the four still
skipped are the wildcard rpc.command.* families and pathfind.compute.
Three notes were also factually wrong, all repeating one mistake. They said
rpc.process and rpc.http_request cannot appear because that path is HTTP-only
while the load generator is WebSocket-only. The premise is right, the conclusion
is not: both appear on every run, five traces each, because
run-full-validation.sh polls each node's HTTP port with curl for readiness and
validated-ledger progress (:449, :502). Those polls take the HTTP path. A reader
acting on the old text would have gone looking for a way to make the harness
speak HTTP that it already speaks. The rpc.process -> rpc.command.* skip reason
inherited the same error and additionally claimed the WebSocket equivalent is
"asserted above instead", which it is not -- that one is skipped for the same
wildcard limitation. All three now state the real blocker, which is that
_validate_parent_child resolves a wildcard child to a single literal probe.
Both HTTP spans stay optional rather than being promoted: the curl polls are
harness scaffolding, not workload, and a future change to how the script waits
for a node could legitimately remove them.
Verification: JSON parses; 18 relationships, no duplicates, every non-wildcard
endpoint resolves to a declared span entry; counters still 41 span types and 62
unique attributes; workflow YAML parses, has no branches key, keeps
workflow_dispatch, and every new glob was checked against the tracked file list
with a matcher that reproduces GitHub's ** semantics; otel-naming exits 0;
pre-commit clean on both files. The eleven new assertions are proven only to the
extent that both ends emitted on run 32969481032 -- that a child is findable
INSIDE the parent's fetched trace is what CI will now decide.
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.
Eight defects found in review of PR 6519. Twenty-four review threads reported
them; ten were duplicates of one another and four were wrong about the code.
The two that corrupt data. Both WebSocket clients reuse the socket after a recv
timeout, and the library queues the late reply, so the NEXT request reads the
previous response. In rpc_load_generator that misattributes latency, and the
skew is permanent rather than one-off. In tx_submitter it is worse: a submit
that reads an account_info reply freezes that account's sequence number and
every later transaction for it fails. Both now correlate replies by request id
under a single overall deadline, with a counter rather than a wall clock, since
time.time() is not monotonic and collides within a tick.
workload_orchestrator never cleared its fixed report paths, so a run that
produced no report silently adopted the previous run's totals -- breaking the
invariant evaluate_exit_gate documents. Reproduced by planting a stale total
and watching it appear in a later summary.
collect_system_metrics reported placeholders as if they were measurements. The
consensus mean used bc with a || echo 0 fallback that neither warned nor
cleared METRICS_COMPLETE, unlike every sibling path; it now uses awk, already a
hard dependency here, which removes the failure mode instead of reporting it.
Note this moves the mean from truncation to rounding, at most 1 ms on a value
of about 45 s. Unmeasurable TPS now warns and clears the flag too. All four
curl probes gained a timeout, not just the one the review named -- an
unresponsive node could hang any of them.
The orchestrator's help text claimed 18-dashboard coverage; there are 15 on
disk, 15 uids in the contract, and the profile already said 15. The
tx_submitter docstring listed twelve transaction types where ten exist, and
claimed issued-currency payments that build_payment never sends.
Two suggested patches were deliberately not taken. A recursive delete of the report
parent sits in a per-task function and would delete earlier phases' reports
mid-run, and recursively remove a caller-supplied --report-dir.
The stale microsecond axis label on ledger-data-sync is real but belongs to
phase 9, which carries a byte-identical copy of that dashboard, so fixing it
here would leave that PR wrong and guarantee a conflict.
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.
Three defects in the harness's own instrumentation, all of the same shape: a
failure that reads as an absence.
The Loki diagnostic reported "unavailable entries" rather than a count. It
issued an unaggregated count_over_time, and because the filelog regex_parser
leaves message and timestamp as log-record attributes, Loki's OTLP path turns
those into structured metadata, which joins a metric query's label set. The
query therefore produced one series per log line and Loki answered HTTP 400,
maximum number of series reached. A second bug hid the first: the JSON helper
never checked resp.status, so Loki's own explanation arrived as a mimetype
complaint instead. Both fixed, in the Python and the shell twin, and verified
against a real loki 3.7.6 including a genuine-zero control so that zero stays
distinguishable from unavailable.
_tempo_search and _tempo_get_trace called resp.json() with no status check, so
any non-2xx became "0 traces" or "0 spans" -- the same class of bug as the
span.name tag returning 200 with an empty list. A 404 on /api/traces/<id>
legitimately means "not indexed yet", so that stays an absence and every other
non-200 now raises.
log.trace_id_cross_reference queried Tempo once, with no retry, while the
metric checks share a poll deadline for exactly this race. It now polls on the
existing METRIC_POLL_TIMEOUT_SEC/INTERVAL, so a trace that has not yet been
indexed is retried rather than reported missing. The window stays at 4 hours
and the assertion is unchanged.
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.
The regression gate has been red on runs with no code change. Only two of
the 25 gated keys ever tripped, both on the same span and never together:
run 32862589645 failed p99 at 25.8750 ms against a 1.0600 ms baseline
(+2341%), run 32867433073 failed p95 at 0.7500 ms against 0.2404 ms
(+212%), and in each run the other quantile sat well inside its own bound.
A real slowdown would move both. This is variance, not a defect.
Measured across four CI runs:
span.ledger.validate.p50 0.0484 to 0.0778 ms 1.6x spread kept
span.ledger.validate.p95 0.1281 to 0.7500 ms 5.9x spread excluded
span.ledger.validate.p99 0.3875 to 25.8750 ms 66.8x spread excluded
Both excluded quantiles reach past their trip point on a healthy run. The
mechanism is arrival timing, not slow code: the span opens only once a
quorum-completing validation arrives (LedgerMaster.cpp:987, inside
checkAccept, past the early return) and wraps the promotion work that
follows, so one slow consensus round dominates the tail of a 3m rate
window and which round that is differs every run.
Widening is not available and must not be attempted later: tolerating
25.8750 ms against a 1.0600 ms baseline needs a bound of about 24.8 ms,
which gates nothing. A bound admitting every healthy run's worst case
admits every regression too. p50 stays gated; it is stable.
THE GENERAL RULE, recorded so this does not recur: an absolute bound
derived as hi_next minus baseline comes from the histogram ladder, so it
budgets for quantization noise and for nothing else. It knows nothing about
how far a metric moves between runs on identical code. Before gating any
key, check its observed maximum across several runs against its trip point
and gate it only with margin. Spread alone proves nothing: tx.apply.p50
swings 364x and never fires, because its 5 ms trip point absorbs the range.
Of the 23 keys still gated the worst reaches 0.67 of its trip point.
Mechanism: spans.names lists span names while _quantiles is shared, so
dropping two quantiles of one span cannot be expressed by deleting a name.
regression-metrics.json gains an excluded_keys map from a flat key to the
reason it is not gated, subtracted by both prom_queries.py (so the key is
never queried) and check_regression_bounds.py rule A. A per-name quantile
override was rejected: a typo there leaves the key gating, whereas a typo
in an exclusion subtracts nothing and new rule F rejects it, along with an
empty reason, a leftover threshold override and a leftover baseline value.
Derived figures recomputed from the committed baseline: 25 gated keys to
23, detection floor 2.02x-9.43x to 2.02x-9.42x, weakly guarded keys ten to
nine, bound over baseline 102%-843% to 102%-842%. The baseline edit is a
deletion of two entries only, with no value rewritten.
Verified: both previously failing runs replay to zero regressions and exit
0; a tenfold increase injected into each of the 23 remaining keys in turn
is still caught in all 23 cases; rule F was confirmed load-bearing by
stubbing it out, which lets a stale exclusion pass.
The two log-correlation checks have never executed in CI: the workflow
hardcoded --skip-loki, so validate_telemetry.py never constructed
log.trace_id_present or log.trace_id_cross_reference. A green Telemetry
Validation therefore carried no evidence that a log line reaches Loki with
trace context. Drop the flag so both checks run and can fail the job.
Correlation spans four independent legs and a failed check names none of
them, so run-full-validation.sh now prints a per-leg diagnostic after the
suite whenever the checks are enabled:
node per-node debug.log line count, the count matching the injected
trace_id/span_id shape, one sample line, and the severity mix,
so "no log at all", "log level too high" and "no active sampled
span" are distinguishable
mount the container-side listing of /var/log/xrpld, taken with the
collector's own mounts and uid. That image is built from
scratch and carries no shell, so the listing runs in a
throwaway container with --volumes-from, not via docker exec
collector the receiver's watched files, logs-pipeline warnings, and the
internal log-record counters, read from inside the container's
network namespace because that endpoint binds to the
container's own localhost and its port is not published
loki the exact query used, the label inventory, and entry counts for
the stream selector with and without the line filter, so "Loki
has nothing" and "Loki has lines but none carry a trace id" are
distinguishable
The diagnostics are non-fatal by construction: every leg runs in its own
subshell with errexit off, each docker and curl call is guarded, and the
coordinator always returns success. Verified with no containers and no Loki
reachable, with an emptied PATH, and with a leg forced to exit non-zero.
validate_telemetry.py gains a matching diagnostic beside the checks,
following _log_prometheus_metric_names: warnings only, never a check
result. Its stream selector and line filter move into module constants
that the shell diagnostic reads back, so the two cannot drift into
describing different queries.
No check was widened or auto-passed, and LOG_QUERY_WINDOW_SECONDS stays at
four hours; a wider window would let a check pass on a previous run's logs.
Every node the harness starts is a validator, and validators disable
pathfinding: Config.cpp:725-726 zeroes pathSearchMax whenever a
[validation_seed] or [validator_token] section is present, and
run-full-validation.sh writes [validation_seed] into every generated node
cfg (:308) with no [path_search] section to put the default back. So
doRipplePathFind refused every call at RipplePathFind.cpp:48-49 and the
3% ripple_path_find weight bought no coverage at all.
It was not free either. The pathfind.request guard is constructed at
RipplePathFind.cpp:35, above that refusal, so each refused call still
exported a span, and the enclosing rpc.command.ripple_path_find span
carried rpc_status=error. That put a steady 3% error floor into
span_calls_total for STATUS_CODE_ERROR: any error-rate threshold derived
from harness data before this change was measuring the harness rather
than xrpld, and needs re-deriving.
Removing the load makes pathfind.request unreachable, so it moves from
required to optional in expected_spans.json; without that the span check
would fail on every run. Three notes in that file and three in
expected_metrics.json made claims that are now false, two of them citing
line numbers this commit deletes; all six are corrected. The runbook
required/optional count moves 26/15 to 25/16.
Two facts a future reader needs.
First, the weights previously summed to 103, not 100, so every percentage
the docstring stated was wrong: health checks were really 38.8%, not 40%.
Dropping the 3 makes the sum exactly 100 and every stated percentage
correct for the first time. expected_spans.json also carried live
arithmetic off the old total, "25/103 ... roughly 43%", now 25/100 and
42%.
Second, baselines/baseline-timings.json was captured WITH this load. Only
span.rpc.ws_message p50/p95/p99 of the 25 gated keys sees the RPC mix,
and their trip points sit 3.1x to 5.9x above baseline, so the gate will
not fire. But a timing baseline is workload-specific and its profile
field still reads full-validation, so nothing will flag the drift:
refresh it from the next CI run's timings artifact.
Pathfinding now has no coverage in this harness at all. The workload
README section "Pathfinding is not exercised" records that cost, the
manual verification route, and a four-step restore recipe in which steps
1 and 2 alone only reinstate the error floor.
validate_metrics and validate_spans only ever run one direction: read the
contract, ask the backend whether each listed name exists. Nothing looked the
other way, so a metric family or span name the contract omitted was invisible
by construction. Both emitted inventories were already being fetched for the
CI log and neither was compared back, which is how a 345 family metric gap and
7 unknown span names went unnoticed.
Add two reverse checks, metric.reverse_coverage and span.reverse_coverage.
Each names every emitted family the contract never mentions, sorted, one per
line, with counts in the report details.
Warn only, by design. passed is hardcoded True in a single shared builder, so
an unaccounted name cannot turn CI red: downstream branches legitimately add
telemetry an upstream contract has not seen yet, and a hard failure would
redden all of them for doing the right thing.
Bulk families are accounted for declaratively. A new top level
accounted_patterns list in expected_metrics.json holds anchored regexes with a
written reason each, covering the 105 per job type queue gauges, the 70 per job
type histogram families, the 228 overlay per category traffic families, and the
Prometheus scrape plumbing that is not xrpld telemetry. Job type shapes are
reduced structurally because every job type name lowercases to letters only;
traffic categories are enumerated instead, because they contain underscores and
a structural pattern there would swallow unrelated names. Anything outside
these shapes still surfaces.
Exporter shapes are folded before matching, so a histogram triple is accounted
for by an entry written for its base family and is never reported as three
separate gaps. Spans need no pattern list: the reverse check reuses the same
matcher the forward check uses, so a glob such as rpc.command.* covers every
command it expands to, and an optional entry still counts as known.
Also fix the diagnostic these checks feed on: both emitted lists were logged as
a single Python list repr, about 15 kB on one line for 422 families, unreadable
and impossible to compare between runs. Both now print one name per line.
_metric_check_targets now selects groups by testing that the value is an
object, rather than by excluding two key names, so a non group top level key
cannot break it. Output is byte identical: 79 metric plus 5 label checks, same
names in the same order.
The assert / do-not-assert decisions in expected_metrics.json and
expected_spans.json were all correct, but several recorded reasons were not.
Pathfinding is disabled outright on every harness node: Config.cpp:725-726
zeroes pathSearchMax whenever a [validation_seed] or [validator_token] section
is present, run-full-validation.sh writes [validation_seed] for every node and
has no [path_search] override, and both handlers return rpcNOT_SUPPORTED
before constructing a PathRequest.
- pathfind_full_milliseconds no longer claims a probabilistic path, nor
prescribes an explicit ledger index, which cannot help: the config gate
fires before the ledger parameter is read.
- pathfind_fast_milliseconds keeps its hasCompletion argument but now leads
with the config gate, which is the operative blocker.
- The pathfind.compute and pathfind.discover notes and the
pathfind.request to pathfind.compute skip reason no longer blame missing
liquidity. pathfind.update_all now records why its request list stays empty.
- statsd_gauges states the arming precondition: a beast gauge is only as safe
as an observable gauge when its object exists before Application.cpp:1570,
where onCollectionReady arms the registered gauges exactly once.
- Alert wiring claims softened: every rule in rules.yaml is paused.
- The per job type gauge group loses its bogus poll bandwidth reason, and its
regex claim is corrected: there is no running state regex, so 30 of those
gauges have no consumer at all.
- overlay_peer_disconnects has one query consumer, not two.
- Cloud dashboard copies dropped from consumer counts: that tree is ignored by
git and has no tracked files.
- rpc_method_errored_total explains that a refused RPC is a normal return, not
a throw, so the refusals above do not make it fire.
- 09-data-collection-reference.md no longer claims a Prometheus name query in
expected_metrics.json.
No behaviour change: the flattened check name list is byte identical.
_log_prometheus_metric_names exists to make name mismatches between
expected_metrics.json and actual emissions visible in CI logs, but it kept
only names matching 19 hard-coded prefixes. On the last CI run that showed
147 of 422 families, and none of the prefixes covered state_accounting_*,
node_family_*, overlay_peer_disconnects or the pathfind_* histograms, so
the coverage gap the preceding commit closes could not be seen through it
at all.
An allow-list can only ever surface names someone already thought to look
for, which is the opposite of what a discovery aid has to do, so the
filter is removed rather than extended. The whole list is a few kilobytes
of CI log. Sorted, so two runs' output can be diffed directly; the
Prometheus API promises no order.
Dashboards and alert rules reference 186 metrics; the harness asserted 57.
Excluding the 107 per-category overlay-traffic expansions, the meaningful
gap was 20 names. This closes it under the contract file's own doctrine:
assert only what the workload guarantees, and record the rest with a
precise reason.
Asserted 18, taking the metric checks from 61 to 79 and the whole metric
phase from 66 to 84. No pre-existing check name or position changes.
statsd_gauges gains the nine state_accounting_* siblings of the one member
already asserted, plus the two NodeFamily full-below-cache gauges and
overlay_peer_disconnects. All twelve rest on one mechanism the group
description now spells out: on the OTel path a beast gauge is an
Int64ObservableGauge, every instance self-registers in its constructor,
onCollectionReady arms all of them unconditionally, and the armed callback
Observes on every export cycle whether or not set was ever called, so the
series exist at 0. The state_accounting family is set in one unconditional
block in NetworkOPsImp::collectMetrics, and full_transitions is the input
to the NodeStateFlapping alert rule, so the alert's own signal had been
going unverified.
A new job_queue_per_type_gauges group asserts the six per-job-type gauges
that a panel or a rule names literally, jobq_manifest_waiting among them
as the ManifestJobQueueConvoy rule's input. The description records why
those six and not all 105: the guarantee is identical for every
non-special job type, so the discriminator is consumer coverage, and the
remaining names are only reached through topk queries over the family that
do not depend on any single type being present.
Recorded four more in not_asserted rather than asserting them.
pathfind_fast_milliseconds is unreachable for this workload, not merely
rare: reportFast fires only from the doCreate fast pass, which is guarded
by !hasCompletion(), and both ripple_path_find entry points construct the
request with a completion function. Only the path_find subscription
reaches it, and the generator does not use it.
pathfind_full_milliseconds is reachable but only one ledger close after
the request, through PathRequestManager::updateAll, and nothing in the
harness arranges or checks that, so the guarantee is probabilistic.
warn_total and drop_total are resource-manager meters gated on a consumer
crossing the warn or drop threshold; their rpc-pathfinding panels are
correct and render empty only because the condition has not occurred,
which is worth stating because both were briefly mis-read as phantoms.
Runbook check counts updated to match.
The gate could not catch a regression on any sub-millisecond span.
compare_to_baseline.py requires both the percentage and the absolute bound to
breach, and every span shared one flat absolute bound of 10 ms (15 ms for p99)
calibrated for a 5-25 ms band the spans do not occupy. Against the baseline
captured on 2026-08-24, where 18 of the 28 quantiles gated at the time sat
below 1 ms, that bound sat 1.15x to 2000x above the metric it guarded, so the
AND never fired: a 100x regression injected into span.ledger.store.p95 reported
0 regressions and exit 0. Injecting a 10x regression into each key in turn was
caught on only 5 of 28.
Give every gated key its own absolute bound, equal to the distance from its
baseline to hi_next, the edge above the top of the bucket the baseline sits in.
The trip point is then exactly hi_next, so the gate fires only once the reading
clears the bucket above the baseline's own. That is the property a multiple of
the enclosing bucket width cannot provide: after the quantile crosses hi, the
interpolation happens across the next bucket, which on this ladder is up to
eight times wider, so no multiple of the enclosing width bounds the excursion.
Measured with a model-free reachability test, a single bucket crossing can
produce a false regression on 2 of 25 keys under the old flat bound and 0 of 25
under this rule. The smallest catchable regression is 2.02x to 9.43x per key.
The job queue bound had the same shape of problem on three of its four keys
(42x, 47x, 220x before). Defaults now sit at each ladder floor, leaving the
percentage bound operative for a metric that somehow reaches them.
Drop span.ledger.store from the gated surface. Its captured quantiles were
0.005, 0.0095 and 0.0099 ms, which is the ladder's 0.01 ms floor times the
quantile: every sample lands under 10 us, so the reported value does not move
even if each store slows from 2 us to 9 us. No bound can gate it. Presence is
still asserted by expected_spans.json and the integration test, and the rate is
still on the ledger-operations dashboard.
Add check_regression_bounds.py, wired into the same workflow step as the bucket
parity check. It fails when a bound is not the one its own baseline implies,
when a gated key has no override, when the baseline and metric surface disagree,
when the percentage bound would become operative, and when a baseline carries
the ladder floor signature. This gate has now broken three times through the
same drift between ladder, baseline and bounds, so documentation alone is not
enough.
compare_to_baseline.py is unchanged: its existing per-metric override mechanism
already expresses all of this.
A missing, unreadable or malformed input makes that check exit 1 naming the
input, rather than reporting success without having checked anything; only a
placeholder baseline, the documented bootstrap state, still exits 0. Its own
tests cover both halves of that contract plus one case per rule, and run in the
workflow before the check so a broken rule reads as a broken rule.
Copied verbatim from the timings.json produced by the telemetry-validation
run at 6a82fc6f37 (166/166 checks passed), which is the hand-off the
workflow prints for a placeholder baseline.
The numbers confirm why the previous baseline had to be voided. It was
captured 2026-06-05, before the collector's spanmetrics ladder gained
sub-millisecond edges, and its sub-1ms entries were arithmetic on the old
1ms first edge rather than latencies:
span.ledger.store p50/p95/p99 0.5 / 0.95 / 0.99 -> 0.005 / 0.0095 / 0.0099
Exactly 100x, because the old values were quantile x 1ms and the real ones
are quantile x 0.01ms. Since the gate only trips on increases, every
sub-millisecond span was unguarded against a 100x regression.
The job.* pair is back too, recaptured on the re-cut microsecond ladder
(floor 1us): job.acceptLedger.queued.p95 now reads 91.1us as a measurement,
where the voided value of 96.79us was 0.95/0.9926 x 100.
The runbook said a correlated log line was "guaranteed" at info severity.
Info is necessary but not sufficient. Replaced the flat claim with the four
real preconditions, each with the code that enforces it and the failure mode
it produces: telemetry enabled, trace_consensus=1, a valid roundSpanContext_
(SpanGuard::childSpan returns a null guard on an invalid parent), and a valid
plus sampled span context (Log.cpp gates injection on IsValid and IsSampled).
Also noted which harness cfgs satisfy them -- run-full-validation.sh and
integration-test.sh set all three config keys; benchmark.sh deliberately
stays at warning and runs no correlation check.
Second, the two checks this work exists to make pass are not exercised by
CI. The workflow hardcodes --skip-loki, and validate_telemetry.py builds
log.trace_id_present and log.trace_id_cross_reference only inside an
"if not skip_loki" branch, so they are never constructed rather than merely
skipped, and never appear in the report. No workflow runs integration-test.sh
either, so its own check_log_correlation() never runs in CI. Recorded that in
the runbook's CI workflow section and in the workload README, with the local
command that does cover it: run-full-validation.sh without --skip-loki.
The workflow itself is unchanged on purpose. Dropping the flag would make CI
exercise Loki ingestion and filelog mounting for the first time on the same
run that must produce a clean regression baseline, so a red result would not
be attributable.
RPC Response Size on rpc-pathfinding said its p95 was computed "over the
dashboard rate interval", but the query hardcodes [5m]. Every other panel on
that dashboard with a hardcoded [5m] -- RPC Response Time, RPC Response Time
Distribution, both Pathfinding duration panels, the gRPC latency panel and
Pathfinding Compute Duration -- says "over 5 minutes". Matched the clause to
the query. The same edit was applied to the local grafanacloud copy so the
two stay identical; that tree is gitignored, so it is not in this commit.
The io_latency group in expected_metrics.json claimed "all 6 panels that
query it". That 6 was a raw string-occurrence count over the dashboards and
included two panel descriptions. Verified truth: two distinct panels query
the metric, ledger-data-sync "I/O Scheduler Latency p95" and node-health
"I/O Latency", each mirrored in a grafanacloud copy, plus one alert rule in
grafana/provisioning/alerting/rules.yaml. Stated that instead of a count.
ff8629bb11 dropped prefix=xrpld as inert and misleading, but four OTel-path
sites still set it, so the branch contradicted itself.
OTelCollector routes every instrument name through a static formatName()
that only lowercases and maps '.'/space to '_'; the sole read of prefix_ is
the startup log line at OTelCollector.cpp:810. All four instrument factories
funnel through formatName(), so no prefix can reach an exported name.
StatsDCollector does prepend it (StatsDCollector.cpp:551/592/640/715), so the
StatsD example legitimately keeps it.
Removed from the 09 reference's OTel config block and from both
quick-reference setups, and from the cfg integration-test.sh generates. The
StatsD example is unchanged and now states why it keeps the key.
Also corrected run-full-validation.sh: [insight] endpoint was described as
"already matches the built-in default", implying it would matter if it
differed. CollectorManager reads it and hands it to OTelCollector, which also
only logs it; the exporter URL is built in Telemetry::initMetrics() from
[telemetry] endpoint. It is as inert as prefix was.
Three related follow-ups to running the workload at info.
integration-test.sh has its own log-trace correlation check that the workload
validator knows nothing about: check_log_correlation() greps each node's
debug.log for "trace_id=<hex> span_id=<hex>" and fails when it finds none, then
cross-checks a sample id against Tempo. At warning it had no guaranteed source.
The only warn-or-worse statement inside the activated accept scope is
RCLConsensus.cpp:671, which fires solely when a transaction throws, so the
check was passing incidentally -- helped by scanning whole files with no time
window. Raising it to info gives it the same guarantee the workload now has:
the consensus accept pair, one branch of which fires every accepted round.
Safe here because this script captures no latency baseline, so there is nothing
for the extra log I/O to contaminate.
baselines/README.md now records that the committed baseline is only valid at
the log level the harness generates. Logging is synchronous and several gated
spans contain log statements -- ledger.build has BuildLedger.cpp:81, and
consensus.accept has RCLConsensus.cpp:655/663/686 with :663 logging once per
transaction -- so the configured level is part of the measurement. Moving it
inflates or deflates the quantiles the gate reads without ever reporting a
regression, because the baseline moves with it. Changing the level therefore
requires re-capturing the baseline.
benchmark.sh keeps warning and keeps prefix=xrpld, and now says why. It
measures telemetry overhead as a delta between a telemetry-off and a
telemetry-on arm, so extra synchronous log I/O would inflate both arms and the
thresholds gate the result. The comment exists to stop a future reader
"aligning" it with the workload harness and quietly degrading the measurement.
Every generated node cfg carried prefix=xrpld under a comment claiming it
"matches the OTel resource service name and the metric names the dashboards
query". Both halves are false.
Verified inert before removing: CollectorManager.cpp reads the key on the OTel
path and passes it to OTelCollector::New, but the only use of prefix_ anywhere
in OTelCollector.cpp is the startup log line. formatName() -- the single funnel
for every instrument name -- only lowercases the name and turns dots and
spaces into underscores; it never reads prefix_. So exported names carry no
prefix at all. expected_metrics.json's own description records this ("Metric
names have no prefix (the xrpld_ prefix was removed)") and 488 live metric
names confirmed it: jobq_job_count, rpc_requests_total, total_bytes_in.
A reader trusting the comment would look for xrpld_jobq_job_count and find
nothing.
The replacement comment states what is true and checkable: the collector
declares no statsd receiver (its metrics pipeline is [otlp, spanmetrics],
confirmed in otel-collector-config.yaml), so beast::insight must export over
OTLP for system metrics to reach Prometheus at all; server=otel is the only
load-bearing key; exported names carry no prefix.
Metric names, series and dashboards are unchanged. The one observable
difference is the OTelCollector startup log line, which now prints an empty
prefix.
Also updated workload/README.md, which repeated the same prefix=xrpld claim
and would have been left describing a cfg key that no longer exists, and made
the template header state the sync obligation explicitly -- nothing reads that
file, so nothing catches it drifting from the cfg the runner generates.
The two log.trace_id_* checks have failed on every run -- they were the only
failures in the 2026-08-20 run (158/160). The workload never satisfied their
precondition, because warning suppressed the one line that is correlated by
construction.
trace_id is injected in Log.cpp from RuntimeContext::GetCurrent(). Severity
does not affect injection, but JLOG filters on severity before format() runs,
so what matters is which severity emits a line while a span is current.
A span becomes current in either of two ways: as a ScopedSpanGuard, or by
activating a plain SpanGuard via activate() / activateIfLive(). activate()
returns a ScopedActivation holding an otel_trace::Scope built from the span,
which pushes onto the same RuntimeContext store Log.cpp reads. A plain
SpanGuard that is never activated makes no span current.
The guaranteed correlated line at info is the consensus accept pair at
RCLConsensus.cpp:736/740 -- an if/else, so exactly one fires on every accepted
round. doAccept activates the accept span as ambient over its whole body at
:565 via activateIfLive(acceptSpan), and that activation lives to the end of
the function, so both branches are inside it. At roughly one round every 4 s
this gives dozens of correlated lines per run, well inside the validator's 4 h
window. LOG_QUERY_WINDOW_SECONDS stays at 4 h deliberately -- a wider window
would let the check pass on logs from a previous run.
info is the minimum that works, which is what the task asked for. debug would
correlate strictly more, additionally covering BuildLedger.cpp:81 and
RPCHandler.cpp:188, but it is the wrong default: it puts synchronous log I/O
inside ledger.build, consensus.accept (RCLConsensus.cpp:663 logs per
transaction) and tx.apply, which are exactly the spans whose latency
regression-metrics.json gates. The next run reprints the voided baseline, so
capturing at debug would bake log I/O into the latency numbers permanently --
the same class of defect this plan exists to remove. The runbook records how to
get the broader coverage per partition, after a baseline exists.
The workload harness gates regressions on histogram_quantile over
job_queued_us / job_running_us, so re-cutting the microsecond ladder changes
what those queries return and the stored baselines no longer describe the
same measurement.
baseline-timings.json's job.acceptLedger.queued.p95 was 96.79us, which is
0.95 / 0.9926 x 100 -- the old 100us bucket edge scaled by the quantile, with
99.3% of samples beneath it. It was never a latency. Keeping it would make the
gate LESS sensitive rather than more: a genuine regression from a real 40us to
90us would still sit under 96.79us + 50% and pass.
Removes the four job.* entries and records why, including their values. The
comparer reports a metric absent from the baseline as "new metric (not in
baseline)" and skips it, so the span baselines stay live and gating continues
for everything unaffected. is_placeholder() still returns False, so this does
not disable the gate wholesale. Recapture the job.* numbers on a node running
the re-cut ladder.
Also corrects _bucket_note in regression-thresholds.json. It described the
spanmetrics ladder as 15 edges starting at 1ms; the collector config has 20,
including five sub-millisecond edges. The note's own reasoning was void too --
it justified the 10ms absolute span bound as "~2 low-end bucket widths", but
the low-end bucket width is 0.01ms, not 5ms. The bound is kept and justified
on the band where span quantiles actually sit, rather than on a derivation
from a ladder that no longer exists.
Both checks selected on {job="xrpld"}. Loki's OTLP ingestion promotes
service.name to the label `service_name` and keeps a `job` attribute as
structured metadata, which a stream selector cannot match, so the selector
returned zero streams whatever had been ingested. The collector config and
TESTING.md already say to select on `service_name`.
Invert the cross-reference. Picking an arbitrary trace from Tempo and
expecting it in Loki fails even when correlation works, because a log line
carries a trace_id only when emitted inside a sampled span and most spans
log nothing at `warning` level. Start from a logged trace_id instead and
resolve it in Tempo, which is the invariant worth asserting, and try every
id found so one unexported trace does not fail the check.
Bound the log queries in time. Nothing here set start/end, so every query
relied on Loki's one-hour default and returned nothing when re-run later to
investigate a result.
Two defects reported against the harness, both confirmed.
The TPS field was computed with `bc` at scale=2, and bc omits the leading
zero: it prints ".25", not "0.25". A bare ".25" is not valid JSON, and this
was the normal case rather than an edge case — ledgers close every few
seconds, so ledger-advance over elapsed-seconds is well under 1 for any
realistic window. It survived earlier checks because those piped the file
through jq, which accepts the malformed form; Python's json rejects the whole
file. awk's %.2f always pads, so the field is now produced with awk. Audited
the other numeric fields at the same time: CPU average and memory peak
already used awk, and the p99, sample count and consensus mean are integers,
so TPS was the only one affected.
Separately, a failing attribute fetch was reported under the span's own check
name, which had already recorded the trace as found. That produced two
entries for one name, one passing and one failing, inflating the check total
and blaming the trace-existence check for a failure in a later network call.
The fetch now carries its own error handling and reports under
`span.attrs.<span>`, matching where its successful counterpart reports. It
moved into a helper rather than growing `validate_spans`, which was already
well over the line limit.
Three consecutive validation runs timed out at Step 3 with nodes stuck at
"unreachable", and the reason was not recoverable from the logs. The node
logs showed the failing nodes stopping at an identical point, immediately
after JobQueue initialisation and before the debug log is opened, with no
error text at all. The harness knew each node's pid and never used it, so a
crashed node was indistinguishable from a slow one.
The readiness loop now checks whether each node process is still alive and
fails as soon as one is not, instead of waiting out the remaining window and
burying the cause under two minutes of progress output. Liveness is not a
bare `kill -0`: an exited-but-unreaped child keeps its pid, so a zombie
answers `kill -0` and reads as alive for the whole window, which is exactly
how a crashed node came to look like a slow one.
On failure each stopped node reports its wait status and the tail of its
stdout. The status is the discriminator that was missing: 137 for a SIGKILL,
139 for a segfault, 134 for an abort, anything below 128 for a deliberate
exit. stdout is printed inline rather than left to the artifact upload,
because a node that dies before its debug log opens writes nothing else and
a cancelled run uploads nothing at all.
This is instrumentation, not a fix. The failure is not attributable to the
recent changes on this branch: the first red run touched only the two Python
files used at Steps 4 and 5, both of which run after this gate, and the same
harness passed 5/5 twice before that.
A validation run timed out at Step 3 with only 4 of 5 nodes proposing, and
the reason was unrecoverable afterwards. Two gaps caused that.
The node-log artifact collected `node*/debug.log` but not `node*/stdout.log`.
A node that dies before its log sink opens never writes a debug.log at all,
so stdout is the only place its reason survives — and that file is written by
the harness and read by nothing, so it went to the runner and was discarded.
The failing node's log was simply absent from the artifact.
The readiness loop also fetched each node's `server_state` and threw it away,
reporting only a count. "4/5 nodes proposing" says a node is missing but not
which one, so there is nothing to grep for even once the logs are kept. The
timeout now names each node that is not proposing along with the state it
last reported, distinguishing a node that answered with a non-proposing
state from one whose RPC port did not answer at all.
Neither change affects a healthy run: the accumulator resets each attempt and
stays empty while every node is proposing.
Two defects reported against the validation harness. Both premises were
correct, but neither suggested fix was, so the remedies differ.
Dashboard panel count: `len(dashboard["panels"])` treated Grafana row
objects as panels and skipped the panels nested inside collapsed rows, so
every dashboard was over-reported by between 1 and 10 (`log-derived-insights`
read 41 against a true 31). The check also passed unconditionally on HTTP
200, so a dashboard that renders nothing would still pass. `_leaf_panel_count`
now walks row children and the result gates the verdict. Gating on the old
top-level length, as suggested, would not have caught the case it was aimed
at: a dashboard made only of collapsed rows counts its rows and reports a
positive number while rendering nothing.
RPC latency percentiles: `LoadStats.record` appended a latency for every
outcome, including requests that never got a reply, where the value is a
time-to-failure rather than a round trip. A timeout contributed the full
receive timeout, and at the error rate a real run shows this reported p95 and
p99 of 10000 ms where the true figure was 5 ms. `record` now takes an
optional latency and the timeout path passes none. The suggestion to append
only on success was not adopted: a reply carrying `status: error` is a
completed, timely round trip whose latency is a genuine measurement, and
discarding it would throw away real data. `per_command` is now keyed off the
request counts rather than the latency map, so a command whose every request
timed out still appears in the report instead of vanishing from it, and each
entry carries a `latency_samples` count.
The rename check rewrites a bare pre-rename binary name in any processed
doc, which turned the sampler's selector description into "against xrpld
or xrpld". Describe the fallback without spelling the legacy token.
Fixes the review findings on this PR that belong to files it owns, plus
several defects found while verifying those fixes. Findings in files owned
by upstream branches are routed there and left untouched here.
Correctness:
- tx_submitter: advance the account sequence only on results that actually
consume one (tes*, tec*, terQUEUED). tem*/tef*/tel* never reach the
ledger, so advancing left a permanent gap that every later submit from
that account inherited. Add a re-fetch hatch so a repeated non-consuming
failure cannot livelock on the same sequence, and gate the account check
on funded-ness rather than list length.
- validate_telemetry: filter spans by name before collecting attributes, so
a per-span attribute contract can no longer be satisfied by a sibling
span; require exact name equality for non-wildcard children and glob
matching for wildcards; bounds-check every returned series instead of
only the first.
- collect_system_metrics: select xrpld by argv[0] rather than a substring
match on the whole command line, which averaged in unrelated processes
and reported their RSS as xrpld's. Count genuine 0.0 CPU readings, use a
clamped nearest-rank p99 index, and record RPC latency only on success.
- benchmark: return each verdict through a named variable instead of a
command substitution, so the pass/fail counters survive and the exit gate
can fire. Scale before dividing in the percentage math, which truncated a
1.26% impact to 1.00% and cleared a 1% threshold.
- compare_to_baseline: fall back to the absolute bound when the baseline is
not positive, so a 0 -> 500 ms jump is no longer "within bounds".
- rpc_load_generator: bound each connection to one in-flight recv(), drain
in-flight requests before closing, use a nearest-rank percentile, and
report delivery shortfall so an under-delivered run cannot pass with a 0%
error rate.
Fail loudly instead of silently:
- run-full-validation: treat a consensus timeout and a missing validated
ledger as fatal infrastructure errors, and fold the orchestrator and
benchmark exit codes into the final status. A degraded cluster previously
ran a full validation pass and reported misleading downstream failures.
- collect_system_metrics: warn per empty measurement source, emit
metrics_complete, and exit non-zero instead of substituting zeros that
pass every threshold. Require GNU date with %N rather than falling back
to a per-sample python3 fork that costs more than the threshold it is
measured against.
- benchmark: distinguish "could not measure" from "exceeded thresholds",
install a cleanup trap so a failure cannot leak nodes and ports, and
report an unusable baseline as inconclusive.
- workload_orchestrator: bound subprocess communicate() and fail the exit
gate on per-phase errors.
Also pins the workload compose images to the versions the sibling stack
already uses, hash-pins the Python dependencies, restricts the validator
config template to loopback, corrects the dashboard and metric counts in
the reference docs, drops a span from the regression gate that cannot fire
under a WebSocket-only workload, and narrows the teardown pkill pattern so
it no longer matches processes that merely mention the work directory.
Verified with a full harness run against a local five-node cluster:
158 of 158 checks passed with no regressions detected.
The Telemetry Validation workflow failed with three "0 series" checks:
rpc_method_us, job_queued_us and job_running_us. All three are Histograms,
and the Prometheus exporter emits a histogram only as the
_bucket/_count/_sum triple -- the bare instrument name is never a series,
so validate_metrics() could never match it.
Evidence from the failing run (31804450127): its own metric-name dump
lists rpc_method_us_bucket/_count/_sum and no bare rpc_method_us, while
the sibling counters recorded in the same function bodies passed with 100
and 67 series. capture_timings.py, which queries job_queued_us_bucket and
job_running_us_bucket, returned real values for the acceptLedger job type
in that same run. Every one of the 10 histograms present exposes the full
triple, so all three suffixes are safe to assert.
Name them the way the exporter does, matching what the spanmetrics group
above already does for span_duration_milliseconds and what
regression-metrics.json and the job-queue dashboard already query. The
metrics stay in their asserted groups because they are genuinely
unconditional, so `not_asserted` would be wrong.
The harness manifests asserted things the code cannot produce and missed most
of what it does. Two assertions were failing every run, and the metric set
covered 16 of the ~41 emitted names.
expected_spans.json: rpc.process was required with rpc.ws_message as its
parent, but it is created only in ServerHandler::processRequest() on the HTTP
path, so a WebSocket-only workload never produces it -- it is now optional and
parented to rpc.http_request, and the rpc.process -> rpc.command.* edge is
skipped with the real reason instead of a coroutine-context-loss diagnosis that
was never the cause. Adds the missing rpc.ws_upgrade span, corrects four
parents (consensus.mode_change, pathfind.request, and update_positions/check,
which are children of consensus.establish rather than consensus.round), and
demotes conditionally-set attributes out of required_attributes so a healthy
run stops failing. Counts recomputed from the file: 41 span types, 62 unique
required attributes.
expected_metrics.json: 16 -> 52 asserted entries across the job-queue, RPC
method, reduce-relay, overflow and validation families, plus the fifteenth
dashboard uid. Metrics the harness workload cannot exercise -- erroring RPC,
ledger-mismatch, TxQ overflow, and the lazily-created getobject_* instruments
-- are listed in a not_asserted group the validator skips, rather than as
assertions that would fail on a healthy node.
The workflow's push trigger listed two globs matching nothing
(include/xrpl/basics/Telemetry*.h, src/xrpld/app/misc/Telemetry*), so no C++
telemetry change ever triggered validation. Replaced with the paths the code
actually lives in, including src/libxrpl/beast/insight/** for the insight
export path the harness depends on. The four inert workflow_dispatch inputs are
now labelled UNUSED rather than looking like working knobs.
Docs: the workload README described a StatsD dirty-flag mechanism under a
member name that does not exist, on a code path the harness never uses -- it
sets [insight] server=otel, so gauges export through an observable-gauge
callback every cycle. Adds the missing txq-burst phase, reconciles three
different dashboard counts, and drops "posts summary to PR", which the workflow
has no permission to do. The runbook's phase-10 section loses the last
sampling_ratio reference (not a config key), gains a Regression Gate and CI
subsection covering the gate that can fail CI, and its compose-logs command now
names the workload compose file. cmake --preset default is left for a separate
change: no CMakePresets.json is tracked, so it is wrong everywhere it appears.
Also drops the dead exporter=otlp_http key the harness wrote into every node
config, and stops capture_timings.py defaulting --profile to a profile that
does not exist.
Two gaps meant the last failure produced no evidence of its cause.
The node-log upload was gated on `if: failure()`, but the validation step
sets continue-on-error, so the job is not failing at that point and the
condition never fired. Every failed run silently skipped the one artifact
that records why a node did not reach consensus. It now keys on the
validation step's own outcome, and also collects the harness logs.
Transaction failures were logged at DEBUG, which CI does not enable, so a
run where all 3052 submissions failed on a refused connection reported
nothing about it. The first occurrence of each distinct failure kind is now
a warning and repeats stay at DEBUG, so one refused connection says so once
instead of 3052 times.
P95 of second-scale spans was a meaningless interpolation. The spanmetrics
histogram topped out at [.. 1s, 5s], so consensus.round (~3.9s) and
consensus.establish (~1.9s) all fell into one 1s-5s bucket and
histogram_quantile interpolated linearly across that 4s-wide gap — the
"Build vs Close" / "Ledger Close Duration" panels' P95 read ~4800ms purely
as an artifact (verified: sum/count avg = 3824ms). ledger.acquire was worse:
~17% of samples exceeded the 5s ceiling, so its p95/p99 were unmeasurable.
Add 2s, 3s, 4s (resolve the 1-5s pile-up) and 10s, 30s (give the
ledger.acquire catch-up tail a measurable home). All ten existing boundaries
are preserved and the list stays strictly ascending (the connector
binary-searches buckets and silently misbuckets otherwise). Pin unit=ms so a
future collector default-unit flip can't rename the metric to _seconds.
Buckets chosen from the live mainnet distribution, not guessed. Native
beast::insight histograms (ms-scale RPC/IO timers in Telemetry.cpp) are 100%
under 5s, so they keep the original buckets — this is collector-only.
Applies on collector restart (cumulative series reset once, handled by
rate()). Runbook and regression-threshold bucket notes updated to match.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
