One conflict, in docs/telemetry-runbook.md: both sides had independently
corrected the same consensus_round_id example. This branch kept the pipe form,
which Tempo rejects as a parse error; upstream moved the predicate inside the
braces, which parses and returns data. Upstream's query is kept, with this
branch's note that the value is the previous ledger sequence plus one.
The OTLP/HTTP exporter selects TLS from the endpoint URL scheme alone
(HttpSslOptions in the pinned SDK matches "https:" exactly), so a client
certificate handed to it alongside an http:// traces_endpoint is loaded and
never presented. The parser checked the cert/key pairing, use_tls and file
readability, but never the scheme, and the default traces_endpoint is plain
HTTP. makeTelemetrySetup() now requires traces_endpoint to start with
"https://" whenever tls_client_cert is set, including when the key is left
at its default.
Nothing asserted the client options reaching the exporter, so a swapped
certificate and key would have passed every test. Move the options mapping
into makeTraceExporterOptions() and assert it at that boundary with
distinct certificate and key paths, plus a one-way-TLS control and a
use_tls=0 control. One case runs the whole path from a [telemetry] section.
Runbook and example-config fixes:
- tx.included is emitted per transaction of the agreed consensus set,
before buildLCL() applies anything, so it is a superset of the accepted
ledger rather than proof of inclusion.
- the dispute.resolve query used the descendant operator, but the event is
on the consensus.update_positions span itself, so it matched nothing.
- the exhausted-retries query asked for txq_status="retried" with
retries_remaining=0, which cannot occur: the attribute is stamped before
the attempt and the retried branch only runs while retries are left.
Exhaustion is txq_status="failed" with a zero count.
- consensus_round_id is an int64, so the two queries comparing it to a
quoted string matched nothing.
- note that consensus_trace_strategy=random is experimental and not used.
- note that a trailing "| attr = value" is rejected by current Tempo;
attribute filters belong inside the braces.
The consensus and ledger attribute tables conflicted: this branch had
already rewritten both, adding the open-phase and avalanche attributes and
correcting tx_count/tx_failed to sit on tx.apply alone. Keep this branch's
tables and apply the close-time rename to their rows, rather than taking
either side whole.
The emitted keys carry the unit and epoch suffix. Update the consensus
and ledger attribute tables and the ledger.build span row to match.
The Close Time Drift panel row is left alone: phase-7 removes that whole
table, so editing it here would only conflict on the way forward.
The consensus.accept.apply row listed close_time, parent_close_time and
close_time_self. The emitted keys carry the unit and epoch suffix, so
update the row to match.
Three conflicts, all composed rather than resolved by taking a side:
- TelemetryConfig.cpp: phase-6 kept networkTypeFromId file-local with
[[nodiscard]]; phase-7 had relocated it to public scope for
Application.cpp. Kept phase-7's relocation, so one definition remains.
The [[nodiscard]] survives on the declaration in Telemetry.h.
- Telemetry.cpp x2: phase-7 added getMeter overrides, phase-6 added
[[nodiscard]] to the startSpan below them. Kept both, and put
[[nodiscard]] on getMeter too.
- TESTING.md: phase-7 had the right metric name (span_calls_total, which
the spanmetrics namespace produces) but the wrong label. Its
xrpl.rpc.command appears nowhere else in the branch; the attribute is
bare `command`, which is what the dashboards query. Took phase-7's
metric with the correct label.
Both signalEndpoint call sites follow the renamed member. signalEndpoint
itself is left in place: removing it and adding metrics_endpoint is a
design change, not part of propagating a rename.
The rename arrived from phase-1b by merge; the runbook still told
operators to set `endpoint`, which the parser no longer reads. Updates
the Quick Start ini block and the Configuration Reference row.
No metrics_endpoint row is added: this branch exports no metrics, so
documenting the key here would describe something the code ignores.
Harness and docs:
- integration-test.sh queried traces_span_metrics_* for spanmetrics, but this
branch sets the connector namespace to "span", so those two checks matched
nothing and failed. The dashboards and runbook had moved; the script had not.
- The same script queried eight native metric names with a product prefix and
capitals that formatName() cannot produce: it lowercases, maps '.' and ' ' to
'_', and prepends nothing. Corrected against the runbook tables.
- TESTING.md carried the same stale spanmetrics names and a jq example reading
a Prometheus label that does not exist.
- The runbook now records where each part of a derived metric name comes from,
since only the namespace is ours to choose.
Collector:
- OTelCounterImpl::increment silently dropped a negative amount. An OTel
counter takes unsigned deltas, so assert and let a release build under-count
rather than wrap.
- OTelGaugeImpl::increment computed current + amount in int64, which is
undefined on overflow, and the clamp ran afterwards so it could not help.
Check the headroom first. set() now clamps rather than casting a uint64 above
INT64_MAX to a negative, which is what made underflow reachable.
- The meter scope was two bare literals. They are constants now, and
Telemetry.cpp static_asserts them equal to kMeterName and kMeterVersion:
beast cannot include the telemetry header, so a build failure is the only way
to catch the copies drifting.
- formatName uses views::transform and ranges::to, as Backend.cpp already does.
- Unused constructor parameters take [[maybe_unused]] instead of (void) casts.
- The destructor logged "shutting down" and "stopped" with nothing between.
initMetrics was 79 lines doing four jobs. The exporter and the histogram views
are separate functions now, addUnitView is a member rather than a lambda
capturing this, and the export interval and timeout are named. It also derived
the metrics URL from the traces URL by suffix swap, which sent metrics to the
traces path whenever the configured URL had any other shape; both URLs now come
from one rule that handles a bare host, a trailing slash and either signal path.
Review feedback on the testing guide:
- rm -rf targeted data/, but this config writes under docker/telemetry/data/,
so teardown did nothing and a second run reused the old NuDB and SQLite
state. Corrected at both sites, including the Test 2 keygen node, which
launches with the same config.
- The standalone span table said consensus.* does not fire. It does:
ledger_accept drives a simulated round, so consensus.round, .phase.open,
.ledger_close, .accept and .accept.apply all appear. Only .establish,
.update_positions, .check, .proposal.* , .validation.receive and
.mode_change cannot. The test intro claimed the same thing and now agrees
with the table.
- Three blocks duplicated content the file already had. Test 1 now points at
the shared Verification Queries section as Test 2 already did, and the
Test 2 submit block checks engine_result like Test 1 does.
- The numbered step list was a copy of the script's own Step N headers and had
drifted by four entries, so it now points at those headers instead.
Also corrects the runbook's ledger and peer span tables against the code:
ledger.build was credited with tx_count and tx_failed, which tx.apply sets,
and was missing its three close-time attributes; peer.validation.receive was
missing ledger_hash and full_validation. The five source line numbers in those
two tables were stale, so they now name the file only, as the other nineteen
rows do.
The table of contents in this file indexes third- and fourth-level headings,
so a new subsection that is absent from it is a gap rather than a style
choice.
Compiled out, compiled in and tracing, compiled in with no active span, and
compiled in but disabled by config all have to produce the right wire bytes,
and only two of them are obvious. Tabulate them, and record why the predicate
reads the context directly instead of calling GetSpan(), which allocates a
DefaultSpan in the no-span case.
mutable_ on a protobuf optional submessage allocates it and sets its has-bit
at the call site, before the helper can decide there is nothing to write. A
caller that dereferences it ships an empty TraceContext whenever nothing is
recorded, and its peers each take a branch to extract nothing.
Document the rule with the right and wrong forms side by side.
The conditional-compilation section promised zero overhead when telemetry is
not wanted. The span disappears, but the arguments passed to it do not: the
compiled-out guards are ordinary inline functions, so a to_string() or a hash
in an argument list still runs and its result is then discarded.
State that, show the guard that does remove the work, and name the opposite
case -- the metric macros, which discard their arguments and need no guard.
formatName() never reads prefix, so setting it here does nothing and the
exported names are bare and lowercase. Leaving it invites queries written
against xrpld_jobq_job_count, which match no series.
The StatsD examples keep it, because that path does apply it to the name.
close_time_avalanche_state is new; the row also did not say that the other
three are rewritten on every convergence iteration, so a reader could not tell
that the exported value is the last one rather than a series.
This row is byte-identical on phases 5 through 10, so it is edited here and
merges forward. The consensus.phase.open row is empty until phase 9 and is
updated there instead.
The runbook's metric tables are headed "Prometheus Metric", so they
should carry the series names an operator can paste into Grafana. Five
counter rows carried the code-side name instead: the OTel Prometheus
exporter appends _total, so rpc_requests, ledger_fetches,
ledger_history_mismatch, warn and drop name no series at all. Verified
two ways: every dashboard queries the _total form and never the bare
one, and phase-10's expected_metrics.json lists the _total names. The
same file already used warn_total further down, so it contradicted
itself.
Nine panel rows documented a hardcoded [5m] window where the panel
actually uses $__rate_interval. The RPC Request Rate row had both
faults.
Left alone deliberately:
- four histogram _bucket rows keep [5m], because the dashboards
hardcode the window for bucket queries and the dashboard linter only
flags hardcoded ranges on counter-rate shapes;
- three example queries inside fenced code blocks, which are standalone
snippets rather than panel documentation;
- the All Traffic Categories (Detail) row, which phase-10 has already
rewritten to the label_replace shape.
Prettier re-padded two table separator rows because the longer names
widen the columns.
The dashboard inventory listed "Peer TX Receive Rate", but this branch
deletes that panel from transaction-overview.json, so an operator
following the runbook finds no such chart. The row is stale from
phase-7 onward and is still present at the tip of the chain, so
nothing downstream repairs it.
Remove the row rather than retarget it: the neighbouring
"Transaction Receive vs Suppressed" row already documents the
tx.receive rate and its panel exists on every branch, so retargeting
would have produced two rows for one chart.
Bring the three documentation surfaces in line with the new parse-time check:
- The @throws clause on makeTelemetrySetup now names the third failure
condition and records that an empty path is skipped.
- cfg/xrpld-example.cfg states, under all three TLS keys, that with enabled=1
and use_tls=1 a path that does not exist or cannot be read stops startup. The
tls_ca_cert wording still says that empty selects the system CA store, since
only a path that is set is checked.
- The runbook troubleshooting entry gains a third bullet for the "cannot be
read" message, whose remedy is the path or its permissions rather than the
certificate and key pairing.
Documentation only; no behaviour change.
makeTelemetrySetup() rejects a contradictory [telemetry] mutual-TLS
setup by throwing, but it is called from ApplicationImp's
member-initializer list. A try/catch in the constructor body cannot
reach a throw from there, and nothing further up the stack caught it
either, so a config mistake reached std::terminate: the default handler
printed a terminate dump and raised SIGABRT, leaving a core file
instead of a startup error.
Catch std::exception around makeApplication() in run(), report the
reason on stderr and return -1, so the failure is a clean non-zero exit
with a message an operator can act on. Only the construction is
wrapped. setup() starts subsystems whose shutdown order is delicate and
is left outside deliberately, because unwinding a half-started
Application would skip the normal stop sequence.
Gate both validation guards on enabled. A node with telemetry switched
off previously refused to start over certificate paths that nothing
would read.
Document both throws on makeTelemetrySetup(), state in
cfg/xrpld-example.cfg and the configuration reference that a partial
mutual-TLS setup is fatal and that the checks apply only when
enabled=1, and add a runbook troubleshooting entry keyed on the two
error messages.
Tests cover both guards with the message asserted so the two are told
apart, both enabled=0 paths, and the default plaintext configuration.
The transitions panel used increase(...[$__rate_interval]). $__rate_interval is
defined as max($__interval + scrape, 4 * scrape), i.e. deliberately one scrape
longer than the step so rate() windows overlap and lose no counter increase.
That overlap is harmless for rate(), but this panel reads the value as a count
of discrete events, and the overlap counts each event in more than one bucket.
Measured against a log-derived ground truth of 106 syncing transitions on
devnet-otel-usw2-01 over 2026-08-11T11:05Z..2026-08-12T23:04Z, the old query
reported 111.3 at a 300s step and 133.7 at a 60s step -- the error grew to +26%
as you zoomed in, because the overlap is a larger fraction of a smaller step.
Switch to $__interval so the buckets tile exactly, and wrap in round() because
increase() extrapolates to the window edges and so reports fractional counts for
an integer counter. The same measurement now gives 106 at 300s, 105 at 60s and
107 at 900s. Every state and both nodes land within a few counts of truth at any
zoom, and the legend Total is now a meaningful figure.
Pin Min step to 1m: the real scrape interval is 60s while the datasource
declares 15s, so without a floor $__interval can fall below one sample.
Draw as bars with 0 decimals -- the value is a discrete count per bucket, and a
line implies interpolation between counts that does not exist.
