initMetrics() held its own copy of the suffix-swap: take tracesEndpoint,
replace a trailing /v1/traces with /v1/metrics. Any other URL shape sent
metrics to the traces path. metrics_endpoint is now a config key of its
own, so use it as given.
Application::startTelemetry() still reads the key itself for
MetricsRegistry, which builds its own exporter. Telemetry exposes no
accessor for the Setup it parsed, and re-parsing would re-run the mTLS
validation and cert-file checks at a later point in startup, so the
second read stays. The comment above it no longer claims the URL is
derived from the traces one.
Also corrects 05-configuration-reference.md: the sample configs carry
metrics_endpoint where exporter used to sit, not a comment.
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 OpenTelemetryPlan/05-configuration-reference.md: phase-5 had
corrected the enabled and use_tls types to 0 or 1 and added the
tls_client_cert and tls_client_key rows, while the incoming side renamed
the endpoint option.
Composed both — phase-5's type corrections and its two mTLS rows are
kept, with the endpoint row renamed to traces_endpoint.
Conflict in OpenTelemetryPlan/05-configuration-reference.md: phase-3 had
widened the options table and added the tx_trace_strategy and
consensus_trace_strategy rows, while the incoming side renamed the
endpoint option.
Composed both — phase-3's wider layout and its two extra rows are kept,
with the endpoint row renamed to traces_endpoint.
The [telemetry] option table documents the config key operators copy.
The key is now traces_endpoint, named for the one OTLP signal it
carries, so the old row pointed at a key the parser no longer reads.
Two prose mentions of "endpoint" further down describe the concept
rather than naming the key, and are left alone.
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.
Four conflicts. None was a take-a-side.
xrpld-telemetry.cfg: dropped the incoming [insight] block. This branch already
has one, and duplicate ini sections do not replace each other -- parseIniFile
appends onto the same section, so keys merge last-wins and the effective config
is one that appears nowhere in the file.
src/tests/libxrpl/CMakeLists.txt: kept this branch's else() branch, which
compiles MetricsRegistry.cpp for the telemetry-off build, and dropped only the
ValidationTracker target_sources inside if(telemetry). Upstream relocated that
one out of the guard, so keeping both would have compiled it twice. The
else() branch is this branch's own: the MetricsRegistry test exists only here,
and without its implementation the off build would not link.
RCLConsensus.cpp: union. The metric macros and registry come from this branch,
PropagationHelpers from upstream; all three are used.
PeerImp.cpp: MetricMacros.h stays unguarded, because all seven XRPL_METRIC_*
uses in this file are on unconditional paths and the header is what defines
them. ConsensusReceiveTracing.h takes upstream's guarded placement, and this
branch's guarded GetObjectMetricNames.h is kept beside it.
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.
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 Phase 4a span table had no consensus.phase.open row at all and listed
only three attributes for consensus.establish. Add the row, the eight
attributes added on this branch, and the two label-valued ones to the
attribute inventory.
Leaves the pre-existing dotted names in the consensus.round row alone; they
predate the underscore convention and are not part of 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 is the change that actually lifts the 5 s ceiling. Until now the
millisecond ladder and the Unit type existed but nothing consumed them.
Telemetry.cpp registered ONE histogram view: instrument name pattern "*",
unit exactly "ms", boundaries {1, 5, ..., 1000, 5000}. Verified against the
installed SDK, "*" matches every name and "ms" matches exactly, so that view
governed every beast::insight Event -- all 54 of them, whatever they measure.
Measured on devnet: 24.9% of rpc_size samples and 100% of jobq_updatepaths
samples fell above 5000. A quantile landing in the `+Inf` bucket reads back
as the second-highest edge, so those p95s reported a flat 5000 rather than a
measurement, and the 1 s to 5 s span was a single four-second-wide bucket
that any quantile inside it had to interpolate across.
Replaces it with one view per unit, keyed on the unit an instrument declares:
- `ms` gets kMillisecondBuckets: every representable edge of the collector's
spanmetrics ladder, plus 60 s and 120 s. The extensions are deliberate --
jobq_updatepaths was measured averaging 59,956 ms, which no span
approaches, so parity alone would still censor it.
- `By` gets kByteBuckets, placed from the measured response distribution
(mean 2131 B, half under 1 kB, tail mean bounded at 7538 B).
OTelEventImpl now derives its declared unit AND its description from unit()
instead of hardcoding "Duration in ms"/"ms", so rpc_size exports as
rpc_size_bytes on the byte ladder. rpc-pathfinding's "RPC Response Size"
panel follows the rename; its unit was already decbytes and is now truthful.
Also corrects Phase7_taskList.md, which still specified the 5000 ladder as
"matching SpanMetrics". That was true when written and became false when the
collector ladder was extended on its own -- implementing the plan as written
reproduced the bug, so the spec is where the defect had come to live. The
edges now have exactly one owner and the plan points at it.
The configuration reference typed the five trace_* switches as bool with
default true. An xrpld config section carries integers, and these keys are
read with an integer cast, so a literal "true" fails to convert rather
than enabling the switch.
Type them as 0 or 1 with default 1, matching the other integer-valued
keys in the same table.
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.
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.
