CMakeLists declared option(telemetry) with a default of its own, so the setting
had two homes and they were free to disagree. The Conan option already reaches
CMake without it: conanfile.py forwards the option into the generated toolchain,
which sets the variable this file reads, and every build here goes through Conan.
Drop the option() call and keep the if(telemetry) test. docs/build/telemetry.md
told readers a CMake option had to be set as well, so that goes with it.
Two conflicts, both where this branch's log-pipeline additions sat next to the
upstream spanmetrics -> span_metrics rename: the config header comment, which
this branch extended with a logs line, and the integration test, where the
log-correlation step precedes the span-metrics step. This branch's content is
kept in both and the rename carried into it.
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.
Addresses the open review findings on this branch.
The log root was never delivered at all. Docker creates a missing bind-mount
source as root, Config::getDebugLogFile() only warns when it cannot create the
network subdirectory inside it, and Application carries on. The node therefore
looked healthy while writing no debug.log, and Loki stayed empty with no error
at any layer. docker/telemetry/data/logs has in fact been root-owned in a
working checkout since it was first created. A one-shot xrpld-logdir-init
service now creates the directory and hands it to XRPLD_UID/XRPLD_GID,
following the pattern the storage-init service already uses.
Ingested logs carried no node identity, so a multi-node stack collapsed into
one indistinguishable stream while every dashboard filters on
service_instance_id. The receiver now sets include_file_path and lifts the
per-node directory onto the resource attribute service.instance.id, which is
on the allow-list Loki promotes to an indexed stream label. A record attribute
would only become structured metadata and could not be used in a selector.
For that to join anything the directory name has to equal the emitter's
service_instance_id, so the node directories are renamed to match: node$i
becomes Node-$i, and the standalone config writes to logs/xrpld-standalone.
The integration test aborted before reporting. Under set -o pipefail the
grep | head -1 pipeline is killed by SIGPIPE once the log exceeds the pipe
buffer, so the run exited 141 somewhere past a few hundred matching lines and
read as a flaky test. grep -m1 stops on its own. The test also verified the
local file and Tempo but never that a line reached Loki, which is the one hop
this branch adds, so a bounded Loki assertion is added alongside a readiness
wait.
Documentation fixes: the Tempo cross-check counted .data, but Tempo returns
OTLP shape so the array is batches and one trace can span several; the Loki
step used the instant /query endpoint, which rejects a bare log selector with
HTTP 400 and a text/plain body, so jq could never parse it and the step never
printed a number even when ingestion worked. The filelog comment claimed six
fractional digits where the node always emits nine. The two flowcharts used
<br/>, carried no legend, and advertised GetSpan(), which Log.cpp deliberately
avoids in favour of reading the thread-local context directly.
Finally, rename the deprecated collector component names: the pinned
collector warns on every start that otlphttp and filelog are aliases for
otlp_http and file_log. Alloy's otelcol.exporter.otlphttp and
otelcol.receiver.filelog are that product's own component names and are not
deprecated, so they are left alone.
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.
Ten conflict regions in four files, none of them caused by the rename.
phase-8's own commit had rewritten comments in files phase-7 owns
(Unit.h, HistogramBuckets.h, OTelCollector.cpp) and in Telemetry.cpp,
while the same sweep ran independently on phase-7.
Resolved every region to phase-7's side on ownership grounds: those files
belong to phase-7 or earlier, so a downstream branch should not carry
divergent copies. phase-8 changed comments only in all four, verified
against the merge base, so no code was dropped.
The one structural region: phase-7 had refactored addUnitView from an
inline lambda into a member function, and phase-8 still held the lambda.
Keeping phase-8's would have shadowed the member.
Wording phase-8 had that is worth restoring on phase-7 -- the "legacy"
qualifier on the prefix parameter, and the rejected-alternative note on
the bucket ladders -- is recorded outside the tree for a follow-up.
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.
