Four conflicts, all where this branch's replacement of the StatsD path with
native OTLP met the upstream spanmetrics -> span_metrics rename. This branch's
design wins in every case; the rename is carried into its text rather than
reverting it, so the connector, its pipeline references, the header comment,
the TESTING.md summary and the runbook all use span_metrics while keeping the
native-OTLP wording.
One addition beyond a straight take-a-side: publish the collector's health
check port. This branch restored the health_check extension and its own
TESTING.md polls http://localhost:13133/ to decide the collector is ready, but
the port was never published on this side of docker-compose.yml, so that check
could not pass from the host. Verified the merged config loads with no
deprecation warnings and that 13133 is published exactly once.
No metric name changed: traces_span_metrics_* already read that way before the
rename, which only ever touched component names and prose.
The pinned collector warns on every start that "spanmetrics" is a deprecated
alias for "span_metrics". Rename the connector, its pipeline references and
the prose that names it.
The derived metric names are untouched. They come from the connector's
`namespace` setting rather than its component name, so occurrences inside a
metric name such as traces_spanmetrics_calls_total are deliberately left as
they are; renaming those would break every span panel. The rename is applied
only to the bare word, never where it is joined to a metric name by
underscores.
This branch introduces the connector, so the change belongs here.
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.
requireReadableFile proved a path readable with getFileContents, which
loads the whole file into a std::string and then drops it. One of the
three paths it checks is tls_client_key, so a private key was loaded to
answer a question that does not need its contents. It now stats the
path, rejects anything that is not a regular file, and opens it without
reading. The message shape is unchanged:
"[telemetry] <key> cannot be read: <path> - <reason>".
A path naming a directory used to escape as an ios failure from the
stream buffer, naming neither the config key nor the path. It is now
rejected as "not a regular file" with both named. The new test covers
that case; it fails against the old implementation and against a copy
with the file-type branch removed.
The runbook's quick start and disable sections both told the reader to
run "cmake --preset default". No presets file is tracked, and the only
preset Conan generates is conan-release, so each of those steps failed
on its first command. Replaced with the flow BUILD.md documents, and
noted that telemetry is the current default while still passing the
flags.
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.
