Resolved network-traffic.json. Both branches own a version of the Peer
Disconnects legend: phase-7 had the older "[instance branch role work_item]"
identity form, and this branch rewrote the same line to use xrpl_ident. Kept
this branch's line, then applied the same change to it, so the legend drops
the series token and the query drops the label_replace that set it.
The incoming side's only edit to that file was those two lines, so nothing
else was superseded.
A panel legend drops ${__field.labels.series} where the panel title already
carries that name, and the query drops the label_replace that set the label,
since nothing reads it any more.
Horizontal bar gauges put the series name above the bar.
14 legends, one bar gauge to the LCD style and 19 name placements, all in ledger-sync-health.
A panel legend drops ${__field.labels.series} where the panel title already
carries that name, and the query drops the label_replace that set the label,
since nothing reads it any more.
Horizontal bar gauges put the series name above the bar.
40 legends across 5 dashboards, 2 bar gauges to the LCD style, 5 name placements.
A panel legend drops ${__field.labels.series} where the panel title already
carries that name, and the query drops the label_replace that set the label,
since nothing reads it any more.
Horizontal bar gauges put the series name above the bar.
3 legends across 2 dashboards, and one bar gauge name placement.
Resolved four dashboards. This branch re-exported consensus-health, node-health,
rpc-performance and transaction-overview, so both sides touched the same option
blocks: upstream added the annotations block where this branch's export already
carried it set to false. Rebuilt each file from the clean pre-change merge of the
two branches, then set the option on every panel. Panel sets match that baseline
merge exactly, so the panels this branch's export drops are unchanged by the
resolution.
All 189 timeseries and state-timeline panels on this branch now set multiLane,
and all four state timelines set perPage.
Resolved three dashboards. phase-7 rewrote consensus-health, rpc-performance
and transaction-overview, so both sides added the annotations block next to
lines that had already diverged. Rebuilt each file from the clean pre-change
merge of the two branches, then set the option on every panel, which is what
both sides intended. Panel sets match that baseline merge exactly.
All 107 timeseries and state-timeline panels on this branch now set multiLane,
and both state timelines set perPage.
Every timeseries and state-timeline panel sets
options.annotations.multiLane. The state-timeline panels set
options.perPage to 5.
99 panels across 13 dashboards. Seven of those already carried the option from
this branch's node-health export, so they flip here and not upstream.
Every timeseries and state-timeline panel sets
options.annotations.multiLane. The state-timeline panels set
options.perPage to 5.
32 panels across 2 dashboards.
Every timeseries and state-timeline panel sets
options.annotations.multiLane. The state-timeline panels set
options.perPage to 5.
62 panels across 5 dashboards.
Every timeseries and state-timeline panel sets
options.annotations.multiLane. The state-timeline panels set
options.perPage to 5.
46 panels across 5 dashboards.
The list mirrors the collector's spanmetrics dimensions, and that dimension is
gone. The reduce-relay and squelch counters keep their own names, which are a
different thing.
Alloy and the Grafana Cloud collector still declared a spanmetrics dimension
for an attribute tx.receive no longer sets, which only widens the label set.
The runbook's drop-path diagram still listed two tx_status values that no
longer exist, because the paths that set them now run before the span starts.
transaction-overview.json conflicted only because this branch reordered every
panel, so the diff could not line the two sides up. The incoming side changed
one panel and nothing else, so this keeps every panel and rewrite this branch
made and re-applies that single change in this branch's own style: the receive
panel drops the suppressed grouping, names its series like the other
single-series panels, and says where the duplicate count now lives.
Resolved transaction-overview.json by composing both sides: this branch's
renamed span_calls_total metric and its 15s interval, with phase-6's removal of
the suppressed grouping and its new legend.
tx.receive is created after the duplicate check, so it never carries a
suppressed attribute. The Tempo tag filter and the dashboard panel still
referenced it, and check_otel_naming rules C and D fail on a reference with no
constant behind it.
The panel keeps its remaining signal as a plain receive rate. How many relayed
copies were dropped is the transactions_duplicate traffic category, which does
not depend on a span.
The spanmetrics dimension goes too; a dimension for an attribute nothing sets
only widens the label set.
tx.receive no longer carries a suppressed attribute: the span is created only
once the node has decided to process the transaction, so there is no dropped
copy for the attribute to describe.
The validator fails a span that is missing a required attribute, so leaving it
listed turns the telemetry-validation leg red.
The previous commit switched the generated node config from [ips] to
[ips_fixed] on the grounds that the variable, the comment and the sibling cfg
template all named ips_fixed, and that ips_fixed is the section whose
documented meaning fits a private cluster. Both of those are still true. The
switch is reverted anyway, because it is a workload change rather than a
naming fix.
Measured on CI, parent commit against this branch's previous tip, one
functional config line apart:
span.consensus.ledger_close.p95 0.57 ms -> 6.43 ms (tripped the gate)
span.consensus.ledger_close.p99 0.94 ms -> 9.50 ms
span.consensus.accept.p50 0.97 ms -> 2.63 ms
span.tx.process.p50 0.36 ms -> 0.18 ms (faster)
job.acceptLedger.running.p95 21157 us -> 10938 us (faster)
Every consensus-path span rose and every transaction-path metric fell, which
is the shape a denser always-connected mesh produces and not the shape of
run-to-run variance. [ips_fixed] holds connections open to all four peers
instead of treating the list as a discovery hint, so each node processes
proposals and validations from the full mesh every round. Nothing else in that
commit touches the consensus path: the emitted config differed in exactly
three lines, of which one is a die message and one expands to an identical
string.
The committed baseline describes the [ips] topology. Adopting [ips_fixed]
therefore needs a refreshed baseline and re-derived bounds, which is the
process baselines/README.md already documents for a workload change. Left as
its own work item rather than smuggled in behind a section rename, and the
reason is now recorded beside the line so it is not repeated.
This also falsified a claim the previous commit had written into
baselines/README.md and regression-thresholds.json: that none of the six
weakly-guarded keys fires on any observed run. Corrected in both, and the
measurement above is cited in place of the absolute.
The harness killed and probed node directories named `node<N>`, but the
directories it creates are `validator-<N>` in run-full-validation.sh and
`bench-node-<N>` in benchmark.sh. Verified with pgrep against processes whose
command lines mimic the real ones: the pattern matched nothing either script
produces. Three consequences, all live:
- `--cleanup` deleted the workdir and left the xrpld processes running. They
are host processes, so the compose teardown does not reach them.
- The pre-run cleanup could not free the previous run's RPC, WS and peer
ports, which surfaces much later as a cluster that never reaches consensus.
- The startup crash fast-fail read a pid path that never exists, so its
`stopped > 0` branch was unreachable and a dead node waited out the full
120-attempt window.
Rather than patch four literals, derive every node path, kill pattern and log
glob from one NODE_PREFIX per script. The directory name is also the node's
identity: the collector's file_log receiver lifts that segment into
service.instance.id, so the directory and the [telemetry] service_instance_id
must agree. Deriving both from one value is what stops them drifting again.
Also in the same files, each confirmed by test rather than inspection:
- The collector readiness probe could never fail. curl -w '%{http_code}'
prints 000 on a refused connection and then exits non-zero, so the
`|| echo 000` inside the substitution appended a second 000 and the
"not ready" comparison never matched. Move the fallback outside.
- The generated config wrote [ips], the starter-list section. A loopback mesh
that must reach quorum is the [ips_fixed] case, which is what the variable,
the comment and the sibling cfg template already said.
- benchmark.sh returned exit 1 for a row it could not measure, though the
exit-code table reserves 1 for "every metric was measured and one breached".
Report 2 there instead.
- Five bc computations fell back to 0, which clears every threshold. The
guards beside them already fall back to the inconclusive token; these now
do too.
- A comment claimed a `|| guard` after a heredoc lands in the heredoc, and
that claim had removed a real guard from the config write. It does not: the
guard runs, and fires when cat fails.
- The EXIT trap was installed 88 lines before stop_workload was defined. If it
fired in that window, errexit aborted the handler on "command not found" and
the cluster reap never ran. Install it below both handlers.
- jq exits 5 on malformed JSON, outside this script's documented codes, so
read_metric now routes that through cannot_measure.
- --nodes and --duration were unvalidated, and --nodes 0 made the pid-count
guard compare 0 with 0 and pass, handing the sampler no pids at all.
- --cleanup now passes -v so the named tempo-data volume goes with it.
Otherwise the next run's Tempo still serves the previous run's traces and a
span assertion can be satisfied by them.
- Five messages reported an attempt count as seconds, though each attempt is
a sleep plus every node's probe.
The baselines README and the two regression JSON files had gone stale when the
baseline was refreshed to a three-run median: they described 20 gated keys and
five exclusions, against an actual 19 and six, and cited the superseded run,
date and commit. Re-derive every affected figure from the committed files. The
detection floors are recomputed (2.00x to 7.41x, so a 10x regression is now
caught on all 19 keys), the newly excluded span.ledger.build.p99 is documented,
and figures that no committed artifact can verify are either replaced with
derivable ones or labelled with their numerator.
No baseline value, threshold bound or derivation entry changes.
Brings the MetricsRegistry split onto this branch. The pipeline half is now
xrpl::telemetry::MetricsRegistry in libxrpl; the observable gauges are
xrpl::telemetry::AppMetricGauges in xrpld.
This branch had added its own instrumentation to the pre-split class, so the
merge had to route each addition to the correct half:
- The thirteen gauges added here -- amendment block, cache hit-rate detail,
clock skew, job-queue saturation, ledger quorum publish, peer ledger supply,
rotation state, slot census, stall events, sync acquire, sync state, UNL
quorum, and the cache lock-hold observer -- all land on AppMetricGauges,
reading the core's meter and validation tracker through it.
- The pipeline additions stay in libxrpl: the consensus round-duration and
rotation-phase histogram views, the malloc-trim and dns/dial latency bucket
ladders, the job-stall counter, and the switch from literal metric names to
the MetricNames.h constants.
Git detected the pre-split MetricsRegistry.cpp and .h as renames of the gauge
files, so both sides' pipeline changes initially landed in the gauge half. They
were moved back, and the result was audited by inventory: every method
definition, instrument creation, view registration, and emitted string from
either side is present, with identical multiplicity.
MetricNames.h moves to include/xrpl/telemetry/ alongside the core. It has no
includes of its own and its two sibling name headers already live there, so
keeping it under src/ would leave an xrpld path in libxrpl's dependency
surface. Nineteen files follow it.
incrementStateChanges() stays removed. The labelled state_changes_total{from,to}
counter this branch introduced replaces it, and the test asserting the method is
absent is kept -- an unlabelled instrument alongside the labelled one would give
Prometheus two conflicting versions of one metric name.
Two tests that drove startAsyncGauges() against a mock ServiceRegistry are
dropped: xrpl_tests links only xrpl.libxrpl and cannot reach the gauge class.
Levelization regenerated. Both xrpld.telemetry loops become bidirectional
rather than one-way; neither is new.
MetricsRegistry did two jobs. It owned the OTel metrics pipeline, and it
registered the observable gauges whose callbacks read live application
services. The second job is what made the whole class xrpld-tier, so the
pipeline's lifecycle -- the recording() gate and the stop() teardown that
closes a use-after-free window -- could not be unit-tested in xrpl_tests.
Split it in two:
- xrpl::telemetry::MetricsRegistry (libxrpl) owns the exporter, provider,
meter, the 16 synchronous instruments, recording(), stop(), and the
record*/increment* methods.
- xrpl::telemetry::AppMetricGauges (xrpld) owns the 19 observable gauges
and their callbacks, holding a reference to the core and to the
ServiceRegistry.
MetricMacros.h and ValidationTracker move with the core. The macros need
only recording() and meter(), both core members; the core holds a tracker
by value, and a libxrpl header cannot include one from src/.
ApplicationImp owns both objects and sequences them. The core is built in
the member-init list, so every synchronous instrument exists before any
subsystem can record one. The gauges are armed once overlay_ exists, the
last service their callbacks read. Shutdown detaches the gauge callbacks
before the core drops the provider, and each shutdown step is isolated so
a failure in one cannot skip the others.
That detach call is new. detachCallbacks() had no callers, and the flag it
sets is read by the gauge callbacks but can no longer be written by the
core, so the caller now has to make the ordering explicit.
The telemetry module links xrpl.libxrpl.core and xrpl.libxrpl.protocol
PUBLIC: ValidationTracker.h takes a LedgerIndex and MetricMacros.h takes a
ServiceRegistry, both in interfaces a consumer compiles against.
Adds a MetricsRegistry gtest that drives an enabled core with telemetry on
and pins the recording() gate, stop() leaving the registry inert, and
stop() being idempotent. The libxrpl test tree no longer depends on
xrpld.telemetry at all, and the two CMake workarounds that compiled xrpld
sources into xrpl_tests are gone.
Documentation and dashboard source links follow the code to their new
paths, split between the two classes by which one now defines each metric.
The rule watched state_accounting_full_transitions > 3 per hour, so a node
that flaps once (one full -> syncing -> full round, e.g. per online-delete
rotation) never tripped it. Lower the threshold to > 0 so a single re-entry
into FULL, past the one-hour uptime gate, alerts.
Keep the state_accounting_full_transitions metric: it is a cumulative gauge
every node always reports, so increase() yields a real series (0 when
healthy) and the rule never evaluates to NoData. A sparse counter would
raise a false DatasourceNoData on a healthy node. Set noDataState: OK so a
scrape gap cannot page either.
The reference doc, span-harness notes and histogram-bucket comments
named the internal AWS dev box and dates while explaining why the
rotation phases are timed. Reword to the general mechanism (a
multi-second freeze at the copy-walk to freshen boundary on a populated
node); the specific hosts, dates and trace ids stay in the task notes.
Resolutions:
- MetricsRegistry.cpp: keep both <exception> and <limits>; drop
incrementStateChanges(), which this branch removed on purpose (the
labelled state_changes_total call site in NetworkOPsImp::setMode
replaces it, and a compile-time test guards that).
- tests/MetricsRegistry.cpp: constructor-built pipeline wording from
phase-10, this branch's test list and gauge paragraphs kept; the two
lifecycle tests now call startAsyncGauges() and pass kTestOptions.
- tests/MetricMacros.cpp: comments name the recording() gate.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
CI's clang-tidy leg flagged eight include-cleaner errors and three
misc-const-correctness / readability-convert-member-functions-to-static /
modernize-use-designated-initializers issues, all inside WP-B6's own code.
Fixed as follows:
- `MetricsRegistry.h`: `#include <opentelemetry/metrics/observer_result.h>`
for ObserverResult; `observeCacheLockHoldPeaks` is now `static` because it
touches neither instance state nor telemetry members.
- `SHAMapStoreImp.h`: adds direct includes for `<cstddef>`, `<string_view>`
and `<xrpl/telemetry/SpanNames.h>` (the StaticStr provider). `seconds` in
`RotationPhase::~RotationPhase` is `[[maybe_unused]]` so a
`-DXRPL_ENABLE_TELEMETRY=0` build under `-Werror` keeps compiling.
- `SHAMapStoreImp.cpp`: direct includes for `SHAMapStoreSpanNames.h`,
`SpanGuard.h`, `SpanNames.h`; `RotationPhase` locals that never call
`setAttribute` are declared `const`; `RotationOutcome` uses designated
initialisers.
Final-review findings (WP-B6-rotation-stall-tracing.md, "What to check
when reviewing"):
- Panels 74 and 75 on `ledger-sync-health.json` still carried panel 41's
description, axisLabel, Source and Keywords copy; rewritten to describe
rotation phase duration and cache lock hold respectively.
- `consensus_view_change_total` and the `view.change` round-span event
were emitted but not registered with the harness. Added the counter to
`not_asserted.metrics_excluded` (workload-gated) and annotated the
`consensus.round` span note with the event and its two attribute keys.
Not fixed (parked, see progress ledger):
- The reviewer's second Important finding — a plan/code contradiction on
the consensus counter — was based on a misread of the plan; the plan's
"Rejected alternatives" table lists a new `TraceCategory::Nodestore` and
the getKeys() fix, not the consensus counter. No action.
- The Minor note about `sweep()`'s peak including lock-acquire time and
`getKeys()`'s not: `sweep()` acquires and releases the lock via a
`scoped_lock`, so `noteLockHold` still runs after the release and the
numbers are comparable. No action.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merges pratik/otel-phase9-metric-gap-fill into
pratik/otel-phase10-workload-validation.
Auto-merged. Carries the weak_ptr gauges_ change into OTelCollector, the
Test 1 standalone-store fix in TESTING.md, and the collection-lifecycle
calls in the StatsD test that phase-7 requires.
Merges pratik/otel-phase8-log-correlation into pratik/otel-phase9-metric-gap-fill.
Auto-merged. TESTING.md picked up phase-8's Test 4 rewrite alongside this
branch's Step 2 standalone-store fix. OTelCollector.cpp gained the weak_ptr
gauges_ list; the StatsD test gained its onCollectionReady() calls.
Merges pratik/otel-phase7-native-metrics into pratik/otel-phase8-log-correlation.
Conflict was one TESTING.md hunk under "Nodes not reaching proposing state":
this branch renamed the node directories to Node-N in integration-test.sh,
phase-7 kept nodeN and expanded the [peer_private] explanation. Resolution
keeps this branch's Node-1 path (its own script uses that naming) and
phase-7's fuller prose citing peerfinder/Config.cpp.
Non-conflicting phase-7 changes come through: OTelCollector's gauges_ list
becomes weak_ptr, matching the earlier hooks_ change; the phase-6 revert of
the StatsD-test onCollectionReady() calls resolved against phase-7's version
that keeps them.
Merges pratik/otel-phase6-statsd into pratik/otel-phase7-native-metrics.
Phase-6 dropped the three onCollectionReady() calls that had been added to
its StatsD test, because that method is only declared here on phase-7.
This branch's own copy of the file was unchanged from the merge base, so
the default merge would have silently deleted the calls from here too —
where they are needed, because this branch gates polling behind
onCollectionReady() in OTelCollectorImp::onTimer.
Resolution keeps both sides: phase-6's two new include lines
(Counter.h, Gauge.h) and phase-7's three onCollectionReady() calls plus
their doxygen and inline explanations. The merged file is exactly
phase-7's tip plus those two includes.
TESTING.md auto-merged cleanly; both sides added text under Test 1 in
different regions.
Three new panels on `ledger-sync-health.json`, cloned from existing panels
(id 41 for timeseries, id 27 for bargauge) so template filters, tooltip
mode, `xrpl_ident` legend idiom and `spanNulls` all match the surrounding
dashboard exactly. Every existing panel is untouched.
- id 73 bargauge Job queue row x=12 y=226 Job Stalls >=1 s (Count By Job Type)
- id 74 timeseries Back-fill row x=0 y=429 Rotation Phase Duration (p95 by stage)
- id 75 timeseries Back-fill row x=0 y=439 Cache Lock Hold Peak (us)
Count panels use `increase(...[$__range])` per memory
promql-counting-events-interval-not-rate-interval. Units set explicitly
(short / s / us).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds `nodestore.rotate` and its eight phase children to expected_spans.json,
all `optional: true` because the 5-node localhost harness cluster never reaches
`online_delete`. Their parent-child relationships are asserted but skip-marked
so a run without a rotation stays green.
Adds `cache_metrics{metric="treenode_lock_hold_peak_us"|"fullbelow_lock_hold_peak_us"}`
to the asserted sync_diagnostics group -- both are observable and always emit,
even at zero. Puts `rotation_phase_duration_seconds` and `jobq_stall_total` in
`not_asserted.metrics_excluded`; both are workload-gated.
On the Cloud collector, adds an `ottl_condition` policy that keeps any trace
carrying a span whose name matches `^nodestore\.rotate`, so the 0.5% probabilistic
tail sampler cannot drop a rotation trace. Sampler is OR'd across policies.
Three conflicts, all from both branches editing the same passage:
- Main.cpp: kept phase-9's wording. The metrics registry only exists here, so
"unwinding destroys little: the metrics registry, whose destructor joins its
export thread" is the true statement on this branch.
- TESTING.md: kept both paragraphs. They document different things (the
private [network_id], and the log path plus log_level).
- 05-configuration-reference.md: composed both. The identity is now resolved
before construction and never empty, so every producer stamps the node key
from the start; the only divergence left is a wallet that already holds a
different key, which corrects the tracer alone. Rewrote the earlier
"three producers" blockquote too: its "no fallback", "first boot ... left
off" and "Known issue" claims are what this change removes.
One silent break the merge could not flag: makeMetricsRegistryOptions() took
the std::optional<std::string> node key that used to be a constructor
parameter, and that parameter is now the resolved keypair. It takes the base58
string directly, and the constructor derives it from nodeIdentity_, which is
declared before both telemetry_ and metricsRegistry_.
