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.
Three conflicts, all resolved by keeping this branch's rewrite and
re-applying the incoming change onto it:
- 09-data-collection-reference.md: phase-7 rewrote both attribute tables,
so the incoming table would have reverted them. Kept phase-7's and
re-applied the two "XRPL epoch" spellings.
- integration-test.sh: phase-7 moved these checks from StatsD to OTel and
no longer defines check_statsd_metric, so only this side compiles.
- TelemetryConfig.cpp: the incoming side carried networkTypeFromId(), which
this branch already has. Kept one definition and took the incoming
doc wording, which the auto-merged body below it already matches.
One conflict, in 06-implementation-phases.md: this branch had rewritten the
phase-4 task table with a Status column, a descoping note and a Spans Produced
section, while upstream corrected the class name in the old plain table. This
branch's section is kept and the name correction re-applied to its 4.1 row.
One conflict, in SpanGuard.h: this branch added struct TraceBytes and upstream
added enum SpanRole at the same position after TraceCategory. Unrelated
declarations, so both are kept.
Two conflicts, both resolved by composing the sides rather than taking one.
cfg/xrpld-example.cfg: this branch had moved the batch-processor keys under
their own heading while upstream edited them in place, so a merge-both would
have documented them twice. Upstream's range sentences are applied to the
relocated block and the head-sampling note keeps its position.
02-design-decisions.md: the summary table changed on both sides for different
reasons. Upstream renamed ledger_index to current_ledger_seq and ledger_seq;
this branch had corrected the PathFinding row to the keys it actually emits.
Both are kept.
Review feedback asked for a Histogram rather than a span attribute at these two
places. Both, not either: the attribute answers how big one sampled request was,
which an aggregate cannot, and the histogram answers the distribution across all
requests, which an unsampled trace never reveals. Both attributes stay.
The metrics land here rather than with the attributes because neither
HistogramBuckets.h nor the metric macro exists on the branch that added them.
Both use kObjectCountBuckets. The argument is the floor, not the ceiling: the SDK
default edges start 0,5,10,25, so an ordinary batch of one to five falls in a
single bucket and every quantile becomes an interpolation on one edge. The object
ladder puts five edges over the mass of both distributions. Path count is bounded
at 352 by kMaxPaths times kMaxAutoSrcCur and cannot saturate. Batch size can, at
roughly 333k, but no measured traffic goes near it, so the ladder is not widened
for a range nothing occupies; the runbook records the overflow query and a test
asserts it stays readable.
Review feedback on the RPC integration PR.
The childSpan examples could not work as written. childSpan() takes its parent
from the ambient context and uses impl_ only as a liveness gate, so an unscoped
SpanGuard parent produced two siblings rather than a parent and child. The parent
is now a ScopedSpanGuard, the child no longer reuses the parent's name, and the
examples pass a full dotted constant because childSpan() takes the name verbatim.
Five of the ten Rule D tests could not fail. Four passed an empty L1 key set,
which makes the rule skip validation altogether; the fifth asserted an empty
result against an escaped-quote selector that extracted no labels at all. Each
now passes a nonempty L1 set and carries a known-bad label in the same
expression, so it asserts both that the intended labels are accepted and that
Rule D ran. Verified by disabling the rule: the old tests stay green, the new
ones all fail.
Span kind is not fixed here. categoryToSpanKind and the span factories belong to
the telemetry library, so the role parameter is routed to that branch, and the
two call sites here follow once it exists.
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.
NetworkOPs.cpp include block: develop replaced DeliveredAmount.h,
MPTokenIssuanceID.h and NFTSyntheticSerializer.h with the single
rpc/detail/SyntheticFields.h. Kept that and this branch's two telemetry
includes.
Notes across the workload contract described earlier versions of themselves, or
cited commits that only exist inside this chain. A squash merge publishes none of
it, so each reference resolves nowhere.
Notes that described their own earlier text:
- expected_spans.json: 'this note previously concluded', 'this note previously
said', 'Un-skipped 2026-08-26', 'the reason had simply gone stale for two
weeks' and 'the claim this entry carried' are replaced by the standing reason
each entry holds. The wildcard pairs now say the validator globs the child via
_span_name_matches(), and state the literal-collapse failure as what a
different validator WOULD do rather than as history.
- regression-thresholds.json: 'an earlier version of this note wrongly claimed',
'the earlier version oversold it' and 'an earlier note called that' become the
cautions themselves -- do not reason from 'every ladder step is at least 2x',
do not oversell the backstop, do not read a false fire as a missing override.
- test_check_regression_bounds.py: the docstring gives the reason a literal is
wrong here, not the story of two tests that once hard-coded one.
Baseline-refresh history rewritten as measurement:
- README.md, baselines/README.md, telemetry-runbook.md and regression-metrics.json
no longer attribute threshold moves to 'the 2026-08-26 refresh'. The evidence
is kept as measurement -- span.tx.apply.p50 has read 0.7917 ms and 0.00597 ms
on the same workload, 132x apart; job.acceptLedger.running.p95 has measured a
5.74x floor on one baseline and 16.28x on another -- which is what supports the
claim that a single-run baseline cannot bound these keys.
Two chain-only commit ids removed, d059f21bf3 and 3860c93db2. Neither is
reachable from develop, so both cease to exist on merge; the second is chain
bookkeeping. The facts they were cited for (the validator globs wildcards; the
span ladder's floor is 0.01 ms) are stated directly instead.
Capture provenance is deliberately kept: baseline-timings.json 'captured_at',
the 2026-08-26 baseline heading, and the 2026-08-24 figures cited as data.
Documentation, JSON note strings and one docstring only, no behaviour change.
These comments dated themselves against this branch or against an earlier
revision of the same change, neither of which survives a squash merge.
- 'as of this branch' in the pricing-case doc and the pass-through static_assert
becomes a statement about what the constants currently produce.
- 'Extracted from processGetObjectByHash()' and 'Split from start()' describe
edits internal to this change; both now say why the method stands alone.
- Recording.h: the mock-abstract mismatch is a standing consequence of a member
set that differs between builds, not something that 'has previously' happened.
- MetricsRegistry.cpp: describe the loops.txt entry as recording two cycles
rather than as what ordering.txt 'previously had'.
- InboundLedger.h: the acquire span is the only signal for back-fill cost; it
did not 'previously emit' nothing.
- check_bucket_parity.py: replace the eleven-phase drift story with the reason
the check exists, and point the failure message at HistogramBuckets.h and the
collector config instead of OpenTelemetryPlan/, which does not reach develop.
Comments and one error message 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.
Every existing rule in this checker runs one way: take a consumer -- a collector
dimension, a Tempo tag, a dashboard label, a doc, an asserted metric name -- and
require it to resolve to the *SpanNames.h constants. Rule H looks closest to the
reverse but is still consumer-side: a constant USED at a call site that no header
defines. Nothing looked the other way.
So deleting a setAttribute from a .cpp and leaving its constant in the header
passed every rule and every compiler, while the telemetry it described stopped
being emitted. The workload validation job would eventually notice, but only when
it happens to run, and its path filter deliberately does not watch daemon .cpp
files -- widening it to 1827 C++ files to catch this would fire a twenty-minute
Docker job on nearly every commit.
Rule M closes that: an L1 constant no code under src/ or include/ references. It
searches all references, not just telemetry call sites, because constants are
passed to helpers, stored in locals and used as attribute VALUES -- a
call-site-only scan would report false positives. Constants referenced only by
test code are reported separately, since a constant exercised by a test but by no
production path is still dead in production.
A WARNING, not a failure, for two reasons that are both real here. Six constants
in this tree are already dead, so failing would redden the branch immediately. And
in a stacked chain a constant legitimately lands one commit before its call site,
so a failing rule would break intermediate branches for a condition that resolves
downstream.
What it reports today, all verified unreferenced across the whole repository and
not just src/include: ConsensusSpanNames.h val::increased, val::decreased and
val::unchanged; SpanNames.h seg::link, attr_val::success and attr_val::error.
Placed here rather than upstream on phase-1c, where the checker was introduced,
because the gap it closes is a workload-harness concern -- the contract asserting
a name nothing emits -- and the harness is this branch's. Putting it on 1c would
also mean union-resolving a 1900-line file across ten merge hops, each an
opportunity to silently drop the metric rules that live downstream.
Ported from a patch written against the sync-diagnostics copy, which carries the
metric-side rules I/J/K/L that this branch does not. The rule itself is purely
span-side; the only shared dependency it needed was iter_sources, which arrived
with those metric rules, so that helper is added here on its own. The rule-L
docstring entry and README row that came with the patch context were dropped --
this branch has no rule L.
Verification: rule M reports 262 constants checked and 6 unreferenced, exit code
still 0 because warnings do not change it; 169 unittest cases pass; the checker
compiles; no metric-rule content leaked in from the patch context (0 occurrences
of METRIC_MACRO_CALL or run_rule_i_metric_literals). Proven non-vacuous by
blanking all three call sites of attr::rpcStatus -- the count went 6 to 7 and
named that constant -- then restoring them and watching it return to 6.
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 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.
