The $xrpl_work_item variable's description carried a real ticket id as its
example. The filter needs no example, so the id is gone and the wording stays.
unl_expiry_days subtracted two NetClock time points, whose rep is uint32_t, so
the subtraction wrapped before the duration_cast ran. A list expired by one day
read about +49709 days. The panel is green above 30 while its own description
promises red at expiry, so an expired validator list rendered healthy.
daysUntil() widens both endpoints to int64_t first, which makes the wrap
impossible rather than checked for. It deliberately does not clamp at zero: a
negative reading is the signal that expiry has passed. A config-listed list,
which uses time_point::max(), now reports positive infinity, because any finite
sentinel could not be told apart from the wrap this removes. -1 keeps its
existing meaning of no published list fetched.
The sweep counter told a second story it could not support. It counted every
entry the 1-minute sweep evicted, including acquisitions that had already
completed or failed and were merely still in the map. Those were counted when
they ended, so the metric buried the wasteful case in ordinary cleanup while
the runbook, the reference doc and the panel description all described only the
unfinished population. It now counts what those three already claimed.
isComplete()/isFailed() are used rather than isDone(), which is protected on
TimeoutCounter and not callable here.
tx.receive no longer sets it, and it is no longer a spanmetrics dimension, so
the attribute tables, the span-scope list and the Prometheus label list all
named something that is not emitted. The reduce-relay suppressed_peers gauge is
a different thing and stays.
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.
Grafana keys a dashboard by uid, so the Phase 9 and Phase 11 rows both
claiming `validator-health` meant one would silently overwrite the other.
Phase11_taskList.md already requires `validator-health-external`; the
reference table now agrees with it, and says why.
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.
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.
One conflicted file, docs/telemetry-runbook.md, with three spots:
- Build section: both sides added different text at one point. Kept both,
incoming sentence first, then this branch's "Run against a live network".
- Disabling section, first spot: this branch's wording names the config
section and says no rebuild is needed, so it already covers the incoming
sentence.
- Disabling section, second spot: kept this branch's paragraph and folded in
the one point it lacked, that both flags have to be passed.
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.
The rename script rewrites "Ripple epoch" to "XRPL epoch", so the old
spelling in a tracked .md makes the check-rename job fail on a dirty tree.
The attribute keys are left alone: the script's pattern needs a space, and
those keys are a cross-layer contract.
Review feedback asked for a Histogram rather than a span attribute at these two
places. Both, not either: the attribute answers how big one sampled request was,
which an aggregate cannot, and the histogram answers the distribution across all
requests, which an unsampled trace never reveals. Both attributes stay.
The metrics land here rather than with the attributes because neither
HistogramBuckets.h nor the metric macro exists on the branch that added them.
Both use kObjectCountBuckets. The argument is the floor, not the ceiling: the SDK
default edges start 0,5,10,25, so an ordinary batch of one to five falls in a
single bucket and every quantile becomes an interpolation on one edge. The object
ladder puts five edges over the mass of both distributions. Path count is bounded
at 352 by kMaxPaths times kMaxAutoSrcCur and cannot saturate. Batch size can, at
roughly 333k, but no measured traffic goes near it, so the ladder is not widened
for a range nothing occupies; the runbook records the overflow query and a test
asserts it stays readable.
The runbook span table, the phase-4 task list and the data reference all
conflicted: this branch had already expanded them with the open-phase,
avalanche and proposal-prefix attributes. Keep this branch's tables and
apply the close-time rename to their rows.
The close_time entry in ledger_history_mismatch_total{reason} is a metric
label value, not the span attribute, and is deliberately unchanged.
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.
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.
The native-metrics pipeline built its own OTLP/HTTP exporter and set only the
URL, so an operator who enabled TLS got mutual TLS on the trace exporter and a
plaintext-configured exporter for metrics. cfg/xrpld-example.cfg promises TLS for
"the OTLP exporter connection" with no carve-out, and two exporters exist. The
exporter now reads the same four [telemetry] TLS keys the trace exporter does.
Its resource was also thinner than the trace resource: service.name was
hardcoded, and service.version, xrpl.network.id and xrpl.network.type were
absent. Because the collector promotes resource attributes to labels, an operator
setting service_name split their fleet - spans carried the configured name while
every XRPL_METRIC_ series still said xrpld, blanking native-metric panels in
every dashboard that filters on it. All four now come from config, and
xrpl.network.type is derived inside from network_id through the shared
networkTypeFromId so a caller cannot supply a mismatched pair.
start() and initExporterAndProvider() take a StartOptions aggregate rather than
growing to eleven positional parameters, seven of them same-typed strings where a
swap would compile silently and stamp the wrong label - the defect class this
change exists to remove. It is constructed at one production site, so replacing
it with Telemetry::Setup once that is exposed stays a single-site edit.
service.version, service.instance.id and xrpl.node.id are stamped only when
non-empty, keeping this pipeline's existing behaviour of omitting an attribute
rather than writing it blank.
Also corrects three plan-doc references that cited line numbers rather than
symbols; the reader line moved and the numbers differ per branch.
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 Consensus Health template-variable table documented $node as resolving via
exported_instance. That dashboard defines $node as
label_values(target_info, service_instance_id) and its panels filter on
service_instance_id; exported_instance appears in it zero times.
exported_instance is a real label, but it belongs to the StatsD boards shipped
alongside, where Prometheus renames a scraped instance label that collides with
the target's own. Documenting it against an OTel dashboard pointed readers at
the wrong pipeline's label.
Telemetry.cpp conflicted. Phase-9 rewrote the metrics pipeline into
makeTracerResource()/makeMetricsResource()/initMetrics() further down the
class, so its side of the region is empty and phase-8's private helper
block does not apply. Resolved to phase-9's structure; phase-8's own
hunks outside the region (the deleted kTracesPath/kMetricsPath, the
verbatim traces URL, the two-endpoint startup log) merged in.
Phase-9's initMetrics() still derives the metrics URL by suffix-swap.
That is fixed in the next commit, not here.
One [telemetry] key served both OTLP signals, and the metrics URL was
derived from it by suffix-swap: strip a trailing slash, strip a known
signal path if present, append the wanted one. Anything not ending
/v1/traces therefore posted metrics to the traces path, and the OTLP
version was pinned in code where an operator could not reach it.
Adds metrics_endpoint alongside traces_endpoint. Both are full URLs used
verbatim, so traces and metrics can go to different collectors, or to one
whose OTLP paths are not the defaults. signalEndpoint(), kTracesPath and
kMetricsPath are gone; nothing derives an endpoint from another.
The startup log names both URLs, since with two independent endpoints
there was otherwise no way to see where metrics were going.
Also drops exporter=otlp_http from the shipped config and the test
fixture. No branch in the chain reads an `exporter` key: it was a real
Setup member in the first phase-1b implementation, removed when only
OTLP/HTTP was wired up, and already deleted from TESTING.md once on the
same grounds.
Eighteen conflict regions across nine files. Resolved by asking, per
region, which side is the better final state rather than by taking a
branch wholesale.
Telemetry.cpp keeps phase-9's two resource builders. phase-8 offered a
single makeResource() with no node identity; phase-9 splits it into
makeTracerResource() and makeMetricsResource() because the metrics
provider is built in the constructor, before setNodeId() runs, so
xrpl.node.id can only be stamped unconditionally on the tracer side.
Collapsing them would have dropped that attribute, which is what keeps
per-node traces from folding into one identity.
Telemetry.h and the config test compose both sides: phase-9's nodeId
member and its assertion, plus the renamed endpoint.
xrpld-telemetry.cfg keeps phase-9's devnet identity and its
metrics_endpoint, renames the traces key, and drops exporter=otlp_http.
Nothing reads an `exporter` key on any branch in the chain: it was a real
Setup member in the first phase-1b implementation, removed when only
OTLP/HTTP was wired up, and already deleted from TESTING.md once on the
same grounds. The cfg line was the last carrier.
The docs keep phase-9's versions, which are both fuller and more
accurate: the incoming runbook listed the consensus strategy values as
"random" where the code compares against "attribute".
OTelCollector.cpp had five comment-only regions in a file phase-7 owns,
so those take the upstream side.
MetricsRegistry.h's usage example named a member that no longer exists
and the wrong arity; it now matches the real three-argument call and says
where the endpoint comes from.
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. Four files still wrote the old
key, which the parser no longer reads, so each would have silently
fallen back to the default collector URL.
integration-test.sh is the load-bearing one: it generates the node config
the test harness starts, so the stale key would have pointed the node at
localhost regardless of the compose network. xrpld-telemetry.cfg is the
standalone node config; the other two document the key.
Note this cfg has a second, divergent variant on the devnet branches that
needs the same fix there.
Conflict in docker/telemetry/integration-test.sh: the incoming side removes
the inert [insight] prefix, and this branch had added service_instance_id
to the same block. Resolved by taking both -- prefix=rippled dropped,
service_instance_id=Node-${i} kept.
OTelCollector routes every instrument name through a static formatName()
that only lowercases the name and maps '.' and space to '_'. The sole read
of prefix_ is the startup log line at OTelCollector.cpp:802, and all four
instrument factories go through formatName(), so no prefix can ever reach
an exported name. StatsDCollector does prepend it, so the StatsD example
keeps the key and now states why.
Covers the three server=otel blocks in the 09 reference and the config
integration-test.sh generates. This branch introduces OTelCollector, so it
is where the inert examples first appear; phase-6's examples are all
server=statsd and stay as they are.
Eight attributes added on phase 4 were missing from the attribute catalogue.
Seven are on consensus.phase.open, which had no entries at all; the eighth is
the terminal regime on consensus.establish.
Does not touch the neighbouring proposers_agreed row, which names an attribute
the code never sets -- pre-existing and outside this change.
The microsecond ladder's first edge was 100us, which sat ABOVE the mass of
every instrument using it. Measured on devnet: 99.3% of job_queued_us
samples, 92.5% of job_running_us and 90.4% of getobject_lookup_us fell in
that first bucket. histogram_quantile then interpolated inside bucket 0 and
returned `quantile / fraction_in_bucket_0 x first_edge` -- p75/p95/p99 of
job_queued_us read 75.52/95.66/99.69us against a prediction of
75.53/95.67/99.70. Three-decimal agreement: those panels were reporting
arithmetic on the bucket edge, not latency.
The fix was already half-written. kSubMillisecondBoundaries had been parked
in MetricsRegistry.cpp as [[maybe_unused]] with a comment noting exactly this
problem for nodestore reads. Its edges are now folded into kMicrosecondBuckets
rather than deleted, so the parked intent is carried forward: 1..1000us
resolution where the mass is, upper edges unchanged so multi-second stalls
stay measurable.
Also moves the GetObject count and charge ladders into HistogramBuckets.h, so
all five ladders have one owner and one set of invariant tests (29 now).
Adds check_bucket_parity.py, wired into the existing OTel naming workflow.
The C++ millisecond ladder and the collector's spanmetrics ladder are
specified to agree over their shared range; they were identical when shipped,
then the collector side alone was extended and nothing noticed for eleven
phases. The check asserts containment rather than equality, because jobs
outlive spans -- jobq_updatepaths averages ~60s, which no span approaches, so
demanding equality would force a ceiling that censors it. Verified it rejects
a missing collector edge, a bogus in-range edge, and a return to the 5s
ceiling.
ledger-data-sync's "Job Queue Wait p95 By Type" moves off the beast
jobq_*_q_milliseconds pair onto job_queued_us filtered by job_type. Those
beast metrics are ms-quantised at the source (Event rounds up to a whole
millisecond), so 94-100% of their samples sat in the first bucket and no
ladder change could fix them. Note the label values are camelCase
(job_type="ledgerData"), not the lowercase metric-name fragments.
Both histogram-fed alert thresholds re-validated and left unchanged, with the
measured basis recorded so neither gets tuned against the old artefact: only
0.0022% of job_queued_us samples exceed the 1s threshold, and every edge
bracketing the 1000ms ios_latency threshold survived the ladder change.
Docs: the rpc_size "known issue -- tracked separately" notes in the runbook
and 09-data-collection-reference are now resolved notes, the stale 10-edge
span_duration bucket list is corrected to the collector's real 20, and the
runbook gains a "Reading A Histogram Percentile" section covering both
saturation traps and the expected discontinuity after a ladder change.
This branch had already made the same corrections independently, and in
richer form, so the resolution keeps this branch's version nearly throughout:
- 09-data-collection-reference.md: this branch already documents the
state-accounting gauges as cumulative **microseconds** with an explanatory
callout, and already names `jobq_job_count` with its `jobq` group. Kept.
- telemetry-runbook.md: already carries `jobq_job_count` in both tables. Kept,
along with this branch's larger additions.
- OpenTelemetryPlan.md: kept this branch's rewritten section 9 blurb, which
describes the inventory without hardcoding counts and so cannot drift.
- consensus-health.json: kept this branch's rewrite. It deliberately removed
the four TraceQL close-time detail panels and renamed the agreement panel;
the incoming side would have resurrected them. Panel count unchanged at 26.
- integration-test.sh: this branch's unprefixed native metric names were kept,
but it still asserted `job_count`, so the `jobq_job_count` correction was
carried over. That check would otherwise always fail.
Conflict resolution kept this branch's evolution and re-applied phase-6's
fixes on top of it, rather than taking either side wholesale:
- consensus-health.json: kept the native `span_calls_total` metric name and
the `interval: 15s` and point styling from this branch; added phase-6's
`close_time_correct` PromQL filter and the NetClock axis labels. The
TraceQL boolean-regex filter stays removed and the `byRegexp` overrides
carry over. Panel count unchanged at 27.
- 09-data-collection-reference.md: kept this branch's headings, its more
detailed consensus attribute table (which already types
`consensus_round_id` as int64) and its section numbering, including the
deliberate removal of the SpanNames inventory. Carried over only the
correction that the state-accounting duration gauges are cumulative
microseconds, not seconds.
- telemetry-runbook.md: kept this branch's native metric names
(`span_calls_total`, `span_duration_milliseconds_bucket`); carried the
`rpc.request` -> `rpc.http_request` span-name fix and the `jobq_` segment
on the job-queue depth metric.
- integration-test.sh: kept this branch's `check_otel_metric` form and
carried the `jobq_job_count` correction.
Conflict resolution, OpenTelemetryPlan/09-data-collection-reference.md
Known Issues table: kept phase-9's expanded 17-row table rather than the
incoming 4-row version, and re-applied the incoming correction to the one
row it changed. The other three incoming rows are already present in
phase-9's table, and phase-9's version is more current (jobq_job_count,
not job_count).
The rpc_requests row now says [insight] server=otel; server=statsd
reaches a collector with no statsd receiver on this branch. The section 8
Quick Reference and the fallback caveat auto-merged cleanly.
09-data-collection-reference.md contradicted itself. Section 2 presents
server=otel as the recommended transport with StatsD as a fallback, but
the Known Issues table and both Configuration Quick Reference examples
still prescribed server=statsd, which on this branch reaches a collector
with no statsd receiver and an unpublished 8125/udp.
Switch the Known Issues row and the Minimal and Production examples to
server=otel with the OTLP metrics endpoint. Keep the labelled fallback
block, and state what it actually requires: re-adding the statsd
receiver and republishing the port. Also record that StatsDCollector
applies prefix to metric names while OTelCollector does not, so the two
transports do not produce the same series.
The phase-6 copy is left alone; server=statsd is correct there.
Conflict resolutions:
- docker/telemetry/xrpld-telemetry.cfg: relocation conflict. phase-9 had
already moved [insight] to the end of the file with server=otel, so the
incoming block was dropped rather than inserted. Keeping both would have
produced two [insight] sections, which merge last-wins into a single
effective section, silently reviving the bug this branch just fixed.
phase-9's per-branch service_instance_id=xrpld-devnet is preserved.
- OpenTelemetryPlan/06-implementation-phases.md: kept both corrections.
phase-9's "Tempo" is right (no Jaeger anywhere in the stack) and
phase-8's "active, sampled span" is right: Log.cpp:328 injects only
when spanCtx.IsValid() && spanCtx.IsSampled().
- OpenTelemetryPlan/09-data-collection-reference.md and
docs/telemetry-runbook.md: kept phase-9's structured-metadata LogQL.
The collector's filelog regex_parser already extracts partition,
severity, trace_id and span_id, so phase-8's inline regexp forms are
redundant, and a line filter matches the literal text in a message body.
Six findings from the review of #6494 survived independent verification.
Each was checked against the branch tip, and where behaviour was in
question, against a live collector and Loki rather than from the
reviewer's claim or from documentation alone.
Plan-doc section numbering. 06-implementation-phases.md used "## 6.9"
twice: for the new Phase 8 section and for the pre-existing Risk
Assessment. Three references already pointed at 6.8.1 and none at 6.9,
and the later phases are numbered 6.8.2 through 6.8.4, so Phase 8
becomes 6.8.1 and the sequence is monotonic. Renumbering to 6.10, as
suggested on the PR, would have collided with Success Metrics.
filelog read position. The receiver relied on the upstream default
start_at=end, which skips everything a node wrote before the first poll
and reads nothing at all from a log that has stopped being written to.
Read from the beginning instead, paired with a file_storage extension so
a restart resumes at the last offset rather than re-ingesting the file.
The collector image runs as 10001:10001 and ships no writable directory,
and a fresh named volume is root-owned, so a one-shot init service
prepares the volume first. It reuses an image the stack already pulls,
adding no new dependency.
Loki log stream label. The job resource attribute did not become a Loki
index label, so the documented {job="xrpld"} queries matched nothing.
Verified against grafana/loki:3.4.2 with its default config: only
service_name and deployment_environment are indexed, and job arrives as
structured metadata, which a stream selector cannot match. Dropped the
attribute and moved the twelve queries this branch introduced to
{service_name="xrpld"}. Three further occurrences in
07-observability-backends.md originate on the phase-1a branch and are
left for a commit there.
Trace ids on unsampled spans. Logs::format emitted trace_id and span_id
whenever the span context was valid. A span dropped by the
ParentBasedSampler still carries its parent's ids, so log lines
advertised traces that were never exported and the log-to-trace link
resolved to nothing. Require the sampled flag as well, and correct the
task list and the documentation that promised the fields unconditionally.
The remaining two findings were refuted. The reported risk of signing
material reaching Loki does not hold: Logs::format already scrubs seven
sensitive fields, and there is a single write path to the log file, so
every JLOG site is covered. The suggestion to add internalLink to the
Loki derived field is not applicable, because that key is not part of
Grafana's schema.
The integration test's span assertions never actually ran. check_span()
built a Tempo /api/search call with --data-urlencode but no -G, so curl
POSTed the params as a body; Tempo answers 200 and ignores the query, so
every span name looked present. Verified against a live Tempo 2.9.4: the
buggy form returns the store's total trace count for any name, including
"zzz.does.not.exist"; with -G a real name returns 1 and a bogus one 0.
Fixed alongside it: the RPC check asserted "rpc.request", which is never
emitted (ServerHandler.cpp builds "rpc.http_request"). These two had to
change together, since -G turns the bogus name from a silent pass into a
hard failure.
Also in the script: a consensus timeout logged two failures and counted
two, because a post-loop else re-reported what the timeout branch had
already reported; and three unguarded curl calls aborted the whole script
under set -euo pipefail, making the ACCOUNT_ZERO fallback dead code with
no cleanup. Guarded the curls and wired an EXIT trap to the existing
cleanup(). The trap deliberately fires only before the summary, so a
completed run still leaves the stack up as the header documents.
Docs corrections, all re-derived from code:
- span inventory heading 35 -> 38, attribute heading 83 -> 89 rows
(78 unique keys), and the section 6 header table now carries the
missing TxApplySpanNames.h row so its columns sum to the same figures
- two stale paths: ConsensusSpanNames.h is under include/xrpl/consensus/,
TxSpanNames.h under src/xrpld/telemetry/
- consensus_round_id is int64, not string (RCLConsensus.cpp sets
prevLgr.seq() + 1); the runbook's TraceQL examples now use a numeric
literal instead of an unparseable bare <round_id>
- state-accounting duration gauges are cumulative MICROSECONDS, not
seconds (NetworkOPs.cpp declares std::chrono::microseconds and
publishes dur.count() raw)
- sampling_ratio is not a config key; head sampling is fixed at 1.0 and
the shipped collector has no tail sampling, so the caveat was rewritten
- the plan blurb referenced Jaeger; this stack is Tempo
The runbook told operators to read a missing outcome as "never went to the
network", which stopped being true once the abort path started setting it,
and the glossary still described outcome as a two-value split.
Document all three values, where each is written, and why peer_count is
absent on the abort path. Several claims were wrong and are corrected:
- Give-up is reached at about 18s, not 21s. There is no setTimer call, so
the first timer runs immediately and the old derivation counted a wait
that does not happen.
- A live aborted rate does not imply stalled acquisitions. A clean
shutdown clears every in-flight acquire, the admin fetch_info clear does
the same, and a full job lane stops timeouts advancing so give-up cannot
fire. The runbook already said the last of these elsewhere.
- A missing outcome does not mean exactly one thing. tryDB can set failed_
and return before done() runs, exporting a span with no outcome at all.
- failed covers unusable ledger data as well as exhausting the retry
limit, so a failed span can carry timeouts=0.
- The aborted lower bound of one minute holds only on the sweep path.
- The sweep measures time since anything last asked for the ledger, not
since data last arrived.
Also fixes the mainnet verification command, which still used the old RPC
port, and drops an inaccurate claim from the config comment about which
ports the workload scripts use.
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.
The Peer Disconnect Rate By Reason panel anchored its LogQL capture on
"\] ", which only matches a reason logged immediately after the [NNN]
peer-id prefix. PeerImp does not log that way: PeerImp::fail emits
"[NNN] <name> failed: <reason>" and the clean teardown emits
"close: Closed". Only ConnectAttempt::fail, which logs the bare reason,
ever matched. The panel's Timeout series was therefore connect-attempt
timeouts only, Ping Timeout was invisible, and PeerImp's own Closed was
uncounted.
Match all three prefixes and separate Ping Timeout from Connect Timeout.
Recorded as LogQL trap 11 in the runbook, alongside the other silent
failures this dashboard exposed.
Also document six overlay observability gaps found while auditing what
ping/pong and gossip traffic is actually tracked. All are pre-existing
and none is fixed here: the code fixes belong in develop-owned overlay
files (TrafficCount, OverlayImpl, PeerImp, PeerfinderManager), not on a
telemetry branch, and one of them needs a public signature change.
- 09 section 6: six known issues, each marked NOT IMPLEMENTED with
file:line evidence -- mtCLUSTER counted as unknown (overhead_cluster_*
always zero, 8 panels flatline), squelch_ignored byte counts always
zero, inbound/outbound byte-basis asymmetry plus a stale Total header
comment, ping/endpoints instrumentation absent, peer span coverage,
and PeerFinder exporting 2 of ~17 available readings.
- 02 section 2.3.2: add a Status column to the span catalog. Of 36
catalogued spans, 16 are live, 15 were never built, and 5 shipped
under different names (consensus.phase.establish became
consensus.establish, ledger.close became consensus.ledger_close,
rpc.request split into rpc.http_request/rpc.ws_message, txq.apply
became txq.apply_direct/txq.accept_tx). The catalog is a design
inventory; 09 section 1.1 remains authoritative for what emits.
- Phase9_taskList: tasks 9.14-9.17 tracking the deferred work, with
exit criteria checked only for what is actually done.
- Glossary: new Ping / pong keepalive term distinguishing ping timeout
from connect timeout. Correct the Cluster and Squelch entries, which
described behaviour the metrics cannot show.
The glossary header pointed at tasks/telemetry_terms.py as its
generator. That file is in no branch and nowhere on disk -- tasks/ is
gitignored one directory up -- so the header now states the file is
hand-maintained and gives the entry format.
Gates: check_otel_naming.py passes all 9 rules (Rule D over 555
dashboard queries, Rule E over the runbook); 19 doc anchors verified;
dashboard JSON valid with a one-line diff. No C++ changes.
Bumps the Loki pin this branch introduced, and updates the two docs that
name the version so they do not drift from the compose file:
- OpenTelemetryPlan/09-data-collection-reference.md (log backend section)
- OpenTelemetryPlan/Phase8_taskList.md (compose snippet)
The service runs the config bundled in the image rather than one from this
repo; `-verify-config` reports "config is valid" under 3.7.6, and native OTLP
ingestion at /otlp is unchanged across 3.4 -> 3.7.
