The three tracker files conflicted because both sides had rewritten them. Took
the incoming lock-free class, then put this branch's two monotonic accessors
back on top of it: totalAgreementsEver() and totalMissedEver(), backed by a
gross pair incremented at first classification and left alone by the repair
branch. MetricsRegistry reads both, so dropping them would not compile.
The test file kept the incoming suite, which renames every case, and gained
this branch's two gross-counter cases adapted to the injected clock.
With tls_client_cert set, only traces_endpoint was checked for an https
scheme. Telemetry::makeMetricExporter() attaches the client certificate and
key to the metric exporter whenever use_tls=1, and metrics_endpoint defaults
to a plain http URL, so an operator who set up mTLS and overrode only
traces_endpoint exported every metric in the clear with the configured client
identity unused.
Check both endpoints, and state the requirement under metrics_endpoint and
tls_client_cert in the example config. Four config tests cover an explicit
http metrics endpoint, the omitted-key default, both endpoints on https, and
a one-way-TLS control that must stay accepted.
Two conflicts, both additive.
TelemetryConfig.cpp: this branch added requireHttpsEndpoint next to
requireReadableFile; upstream added readConsensusTraceStrategy at the same spot.
Both kept.
05-configuration-reference.md: this branch added the two client-certificate rows
while upstream corrected the consensus strategy value from attribute to random.
Both kept. Also drops the stale "not yet implemented" row for
consensus_trace_strategy, which the merged table now contradicts twice over: the
option is parsed, and its value is no longer spelled attribute.
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.
consensus_trace_strategy was read as a std::string and compared against the
literal "attribute" in startRoundTracing(), while the runbook documented
"deterministic" and "random". The documented value "random" therefore fell
through to the default and did nothing.
Parse the setting once into ConsensusTraceStrategy, so the consensus code
branches on a type. The accepted spellings are now "deterministic" and
"random"; anything else fails at startup instead of silently defaulting.
The behaviour behind the old "attribute" name is unchanged and is now
reached by "random".
Document consensus_trace_strategy in xrpld-example.cfg, stating that
"random" is experimental and not used: it gives each node its own trace id,
so one round arrives as one trace per node.
Also state on the tx.included event that it covers the agreed consensus set
before the ledger is built, so it is a superset of the accepted ledger.
Two conflicts, both additive on each side.
TelemetryConfig.cpp: include blocks only. This branch added FileUtilities.h for
the certificate readability checks; upstream added <limits> and <optional> for
the bounds parser. Both kept.
The TelemetryConfig test: this branch's mutual-TLS cases and upstream's
batch-bounds cases were added at the same positions, so the file is rebuilt from
both stages and carries all 32 tests. Two shared cases were each edited by one
side only, so the edited side wins in each: upstream asserts the batch defaults
in parse_empty_section, and this branch's parse_full_section writes a real
certificate file, which is now required since the parser opens it.
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.
Two review findings on the telemetry library.
SpanContext::isValid() returned impl_ != nullptr, so it answered true for a
context holding no span. threadLocalContext() wraps whatever GetCurrent()
returns, and that is an empty Context on a thread with no active span, which
contradicted the documented "invalid context if none is active". It now asks the
Context for its span. childSpan(name, ctx) is the one caller whose behaviour
changes: a context with no span used to produce a new root span, and now returns
a null guard as its @return already promised.
The three batch settings went to the OTel BatchSpanProcessor unchecked. Three
ways that failed: zero was accepted for all of them; batch_size could exceed
max_queue_size, which the SDK documents as a precondition and does not enforce;
and a mistyped value let boost::bad_lexical_cast escape, which derives from
std::bad_cast rather than std::runtime_error, so the operator saw a bare "bad
cast" naming no key. Reading unsigned also turned "-1" into 4294967295 instead
of failing, so the value is parsed signed and negatives are rejected.
xrpld-example.cfg now states the ranges.
Nine files conflicted. Resolutions, and why:
Telemetry.cpp - upstream carried its own initMetrics(), makeResource() and
makeMetricExporter(); this branch already has an initMetrics() that builds the
exporter inline and uses makeMetricsResource(), which stamps xrpl.node.id only
when it is already known. Keeping both would have defined initMetrics() twice.
Kept this branch's, then pointed its reader at setup_.metricExportInterval and
setup_.metricExportTimeout: upstream turned those constants into [telemetry]
keys and removed the old ones, so the previous spelling no longer resolves.
OTelCollector.h - kept upstream's parameter docs. This branch's text promised
instanceId, serviceName and networkType become resource attributes; the
constructor marks all three [[maybe_unused]] and the .cpp already says they are
not read.
CollectorManager.cpp - kept upstream's comment for the same reason.
node-health.json - kept this branch's 60 panels. Upstream's only change to the
file was job_count to jobq_job_count, which this branch already had.
cfg/xrpld-example.cfg - composed. Kept this branch's warning that
service_instance_id must be set explicitly for the metrics pipeline, took
upstream's traces_endpoint rename, and removed a duplicate metrics_endpoint
entry along with the claim that metrics derive from the traces URL by rewriting
the signal path. Nothing derives it; both metric exporters read
metrics_endpoint. 17 keys, one entry each.
05-configuration-reference.md - both sides misdescribed the parser. Kept this
branch's fuller text, corrected the endpoint default to traces_endpoint, and
replaced the "resolve their URL differently" table with what the code does now.
09-data-collection-reference.md, 06-implementation-phases.md,
Phase7_taskList.md - kept this branch's versions, which drop a metric that was
never implemented, correct the state encoding to 0-6, and rename nudb_bytes to
stored_object_bytes. Re-applied upstream's rpc_requests_total fix, which taking
this side had reverted.
phase-6 corrected the Consensus Health template-variable table to name
service_instance_id, the label that dashboard actually filters on. This branch
removes that table entirely - the section is restructured around a
Prometheus-label reference and a pointer to the runbook - so the corrected row
has nothing to land in. Resolved by keeping the restructured section; phase-6's
fix remains correct for phase-6, where the table still exists.
The [telemetry] cfg block merged without conflict: the composed 14-key block
from upstream and this branch's metrics_endpoint, metric_export_interval_ms and
metric_export_timeout_ms entries coexist, 17 keys with one entry each.
Both sides documented the same six [telemetry] keys, so the automatic merge
duplicated all of them. Resolved by keeping this branch's structure - which
already covers all 14 keys and groups them under TLS and batch-processor
headings - and folding in the corrections from the upstream side:
- endpoint is renamed to traces_endpoint, which is what the parser reads, and
described as used verbatim including its signal path.
- use_tls no longer claims to enable TLS. The exporter's URL scheme selects
TLS; this key only decides whether tls_ca_cert reaches it as a CA bundle.
- tls_ca_cert records that the path is not opened while the config is parsed,
so an unreadable file shows up as an export failure rather than at startup.
- service_instance_id explains that it is normally left unset and filled in
from the node public key during startup.
Section::value_or in 05-configuration-reference.md becomes Section::valueOr;
that member does not exist under the other spelling.
Addresses review findings on the native-metrics work.
StatsDCollector::onTimer drained the send buffer inside the polling_ gate. That
gate holds back hook handlers until the application's services are built, but
sendBuffers() is socket I/O. StatsDEventImpl derives only from EventImpl, so it
never enters metrics_ and posts straight to the buffer; its |ms timings piled up
before onCollectionReady and were dropped after onCollectionStopping. The drain
now runs every tick, and outside metricsLock_, so onCollectionStopping no longer
waits on a UDP flush.
TelemetryImpl's constructor left meterProvider_ set when initMetrics() threw.
initMetrics publishes globally as its last step, so a throw left getMeter()
callers holding a provider nothing else could reach. Reset it in the catch.
~ApplicationImp caught only std::exception around telemetry shutdown while the
callees reach third-party SDK code, so a foreign exception would have terminated
the process. Added a logging catch-all.
ValidationTracker's hard trim evicted by unordered_map bucket order. It now
evicts oldest-first, so the entry dropped under pressure is the one least likely
to still reconcile.
The GetMeter test restored the global meter provider only on the success path,
and ASSERT_TRUE early-returns past it. Uses xrpl::ScopeExit instead.
The hook debounce window is a named constant rather than a bare 500 in a
comparison, and the metric export cadence becomes operator-configurable through
metric_export_interval_ms and metric_export_timeout_ms. Both are range-checked:
the SDK warns and silently substitutes its own 60s/30s defaults when the timeout
is not below the interval, so an unchecked value would slow export rather than
speed it up. Parsing uses a signed representation because lexical_cast<uint32_t>
accepts a leading minus and wraps it.
Naming corrections: CollectorManager documented exported_instance, which no OTel
dashboard uses; node-health queried job_count where the exported name is
jobq_job_count; network-traffic and overlay-traffic-detail referenced an
undeclared DS_PROMETHEUS variable; the counter table omitted the _total suffix
the Prometheus exporter appends; the plan docs and task list carried an xrpld_
prefix formatName never applies; and OTelCollector::New()'s contract promised its
instanceId, serviceName and networkType arguments were read, contradicting the
definition that marks them unused.
The commented [telemetry] block in cfg/xrpld-example.cfg documented 8 of the 14
keys the parser accepts. Add the six that were missing - service_instance_id,
use_tls, tls_ca_cert, batch_size, batch_delay_ms and max_queue_size - each with
the unit and default read from the parser, and rename the documented endpoint
key to traces_endpoint so it matches what makeTelemetrySetup reads.
use_tls is documented for what it does rather than what its name suggests: it
gates whether tls_ca_cert reaches the exporter as a CA bundle, while the scheme
of traces_endpoint is what selects TLS. The path is not opened during parsing,
so an unreadable file surfaces as an export failure at runtime.
05-configuration-reference.md named three symbols that do not exist:
setup_Telemetry, make_Telemetry and Section::value_or. Correct them to
makeTelemetrySetup, makeTelemetry and Section::valueOr.
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 reference docs had drifted from the code in ways that break the reader
rather than merely misinform: PromQL examples that return no data, a rollback
flag that is a no-op, a sampling knob that does not exist, and two span parents
that moved. Code is treated as the truth throughout; where the code is the
defective side, the doc now records it as a known issue instead of describing
the bug as intent.
Renames the docs missed: histogram names gain the exporter's unit suffix
(ios_latency_milliseconds_bucket and four siblings), ledger_history_mismatch
gains _total, the StatsD-era quantile label gives way to le buckets,
rpc.request becomes rpc.http_request, traces_spanmetrics_calls_total becomes
span_calls_total, and the nine dotted xrpl.* span attributes are recorded as
renamed rather than left as live keys.
Re-parenting: consensus.update_positions and consensus.check are children of
consensus.establish, not of consensus.round.
Units and labels: state_accounting_*_duration is microseconds, not seconds;
cache_metrics label values are case-sensitive; object_count carries demangled
C++ type names. Nodestore read and write latency stays microseconds -- the
nanosecond accumulator change did not move the exported unit.
Adds what shipped but was undocumented: the ledger.acquire span, seven
consensus.round events, twelve span attributes, node_writes_duration_us, the
7-day validation-agreement window, the TxQ admission and reduce-relay metric
families, metrics_endpoint, and the phase-10 validation workflow.
Corrects claims that never held: 10% head sampling (it is fixed at 100%),
configurable redaction (it is unconditional), -DXRPL_ENABLE_TELEMETRY=OFF
(the flag is -Dtelemetry=OFF, default ON), FindOpenTelemetry.cmake and the
xrpl_telemetry target (neither exists), Promtail and a StatsD exporter in the
pipeline (neither exists), and Loki stream selection on job= (only
service_name is a stream label).
Phase 9 is marked complete, its provisioned alerting is attributed to the
branch that shipped it, and Phase 11 stays at zero except the one prerequisite
its code closes. Counts are reconciled repo-wide: 41 emitted span families,
15 dashboards on disk with 14 asserted, 13 alert rules in 5 groups.
Hardens the gate that let this drift through: Rule E of the naming check now
covers the reference docs, its allow-dotted marker is key-scoped and warns on
stale or empty use, a missing checked file is reported instead of silently
skipped, the test suite runs in CI, and doc paths trigger the check.
C++ and CMake changes are comment-only: three MetricsRegistry instrument names,
eight OTelCollector claims of a metric-name prefix that formatName never adds,
and the telemetry option's inverted default.
Flip the tracePeer Setup default and the trace_peer config parser
default from off to on, and update the example config and build doc to
match. Peer spans record only peer_id (a node-local numeric connection
id) plus trust/ledger metadata — no IP addresses or public keys — so
the privacy concern behind disabling it does not apply. The high-volume
characteristic is retained in the docs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
