The validation owned its own push trigger and paths filter, which made it the one
build-and-test workflow outside the on-pr.yml plus reusable-*.yml pair the rest of
CI uses. Rename it to reusable-telemetry-validation.yml, take workflow_call, and
let on-pr.yml decide when it runs.
on-pr.yml gains a second changed-files list for the telemetry paths. Its existing
`go` flag is true for nearly every pull request, so gating on `go` alone would run
a self-hosted build and a 30-minute cluster on all of them.
The configure step no longer repeats the telemetry option. A command-line define
is written to the cache before the toolchain runs and wins over it, so it could
turn telemetry on with the package never fetched. build-xrpld reads the value the
build actually configured out of CMakeCache.txt and publishes it as a job output;
validate-telemetry runs only when that says telemetry is on, and every value CMake
does not accept as a boolean fails the job rather than skipping silently.
Account setup submitted the funding Payments, slept a flat 10 seconds, then
read each sequence once. The txq-burst and mixed-peak phases escalate the
open-ledger fee on purpose, so the funding transactions were queued, every
account read Sequence 0, and the phase aborted with "only 0 of 8 created
accounts were funded". The run then reddened on a workload gate rather than on
anything telemetry had done.
Poll the ledger until each account has a sequence, with a deadline, so a late
confirmation is still seen and a healthy cluster pays no waiting cost. Pay a
multiple of the current open-ledger fee, so funding is not queued behind the
load a phase creates deliberately. terQUEUED no longer marks an account funded:
only a ledger read does.
Retry the accounts that never confirmed, once, after re-reading the genesis
sequence from the ledger. consumes_sequence advances the local counter on
terQUEUED, so a dropped funding transaction leaves it ahead of the ledger and
every resubmit would otherwise land on a future sequence.
The funding wait can run twice, so raise the orchestrator's grace above twice
the timeout. A test pins that relationship, since the two constants live in
different files.
Also save each generator's full stdout and stderr beside its JSON report. Only
the last 200 characters of stderr reached the phase error and stdout was
dropped, so none of the per-account funding results appeared in CI.
compare_to_baseline took the unit from the baseline entry and dropped the current
run's, and nothing compared the two, so a us -> ms change was scored as a numeric
delta: four keys rewritten to the same physical durations reported 99.9%
improvements and the gate exited 0. prom_queries.py says the baseline preserves
the unit "so the comparator can sanity-check unit drift"; it never did. A unit
mismatch now fails and names both units.
The workflow's step summary printed total, regressions and improvements. total is
every key in the report -- the union of baseline and current -- so it was neither
the baseline count nor what was gated, and missing_in_current was computed and
never printed. A run that gated 16 of 20 keys read as a full comparison. The
comparator now reports a real "compared" count and the summary prints it beside
the not-captured count, with a warning when any key was missed. The table also
refused nothing on a truncated report; existence is not readability.
check_regression_bounds told the operator to add max_abs_increase while reading
max_abs_increase_ms / _us, so following the message added a key nothing reads and
the gate kept failing with no explanation. The committed thresholds use only the
suffixed spelling, so the message was the defect. Its three JSON inputs were also
unchecked: a top-level null, list or number parsed and then died on the first
.get, and a string "metrics" survived the placeholder test and reported its own
characters as gated keys -- wrong advice rather than a crash.
Four tests cover these; all four fail against the previous checker.
One conflicted file, docs/telemetry-runbook.md, with three spots:
- Build section: both sides added different text at one point. Kept both,
incoming sentence first, then this branch's "Run against a live network".
- Disabling section, first spot: this branch's wording names the config
section and says no rebuild is needed, so it already covers the incoming
sentence.
- Disabling section, second spot: kept this branch's paragraph and folded in
the one point it lacked, that both flags have to be passed.
Two conflicts, both additions at the same spot:
NetworkOPs.cpp include block kept develop's rpc/detail/SyntheticFields.h
alongside this branch's telemetry/MetricsRegistry.h; .cspell.config.yaml
kept both new words.
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 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.
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.
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.
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.
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.
The microsecond ladder's first edge was 100us, which sat ABOVE the mass of
every instrument using it. Measured on devnet: 99.3% of job_queued_us
samples, 92.5% of job_running_us and 90.4% of getobject_lookup_us fell in
that first bucket. histogram_quantile then interpolated inside bucket 0 and
returned `quantile / fraction_in_bucket_0 x first_edge` -- p75/p95/p99 of
job_queued_us read 75.52/95.66/99.69us against a prediction of
75.53/95.67/99.70. Three-decimal agreement: those panels were reporting
arithmetic on the bucket edge, not latency.
The fix was already half-written. kSubMillisecondBoundaries had been parked
in MetricsRegistry.cpp as [[maybe_unused]] with a comment noting exactly this
problem for nodestore reads. Its edges are now folded into kMicrosecondBuckets
rather than deleted, so the parked intent is carried forward: 1..1000us
resolution where the mass is, upper edges unchanged so multi-second stalls
stay measurable.
Also moves the GetObject count and charge ladders into HistogramBuckets.h, so
all five ladders have one owner and one set of invariant tests (29 now).
Adds check_bucket_parity.py, wired into the existing OTel naming workflow.
The C++ millisecond ladder and the collector's spanmetrics ladder are
specified to agree over their shared range; they were identical when shipped,
then the collector side alone was extended and nothing noticed for eleven
phases. The check asserts containment rather than equality, because jobs
outlive spans -- jobq_updatepaths averages ~60s, which no span approaches, so
demanding equality would force a ceiling that censors it. Verified it rejects
a missing collector edge, a bogus in-range edge, and a return to the 5s
ceiling.
ledger-data-sync's "Job Queue Wait p95 By Type" moves off the beast
jobq_*_q_milliseconds pair onto job_queued_us filtered by job_type. Those
beast metrics are ms-quantised at the source (Event rounds up to a whole
millisecond), so 94-100% of their samples sat in the first bucket and no
ladder change could fix them. Note the label values are camelCase
(job_type="ledgerData"), not the lowercase metric-name fragments.
Both histogram-fed alert thresholds re-validated and left unchanged, with the
measured basis recorded so neither gets tuned against the old artefact: only
0.0022% of job_queued_us samples exceed the 1s threshold, and every edge
bracketing the 1000ms ios_latency threshold survived the ladder change.
Docs: the rpc_size "known issue -- tracked separately" notes in the runbook
and 09-data-collection-reference are now resolved notes, the stale 10-edge
span_duration bucket list is corrected to the collector's real 20, and the
runbook gains a "Reading A Histogram Percentile" section covering both
saturation traps and the expected discontinuity after a ladder change.
Both the CMake option and the Conan recipe default telemetry on, so the
build worked, but this workflow exists to exercise telemetry and should not
depend on a default it does not control. If that default ever flipped, the
binary would build cleanly and then every span and metric assertion would
fail for a reason no log names.
Stated explicitly, the failure mode inverts: CMakeLists.txt does
find_package(opentelemetry-cpp CONFIG REQUIRED) under this option, so a
dependency graph without it fails at configure time with a clear message.
The shared build-deps action is left alone deliberately — forcing the Conan
option there would change every other workflow that uses it.
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.
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.
