A -DXRPL_ENABLE_TELEMETRY=0 build expands every XRPL_METRIC_* macro to
nothing, so anything whose only use sits inside one looks unused.
- MetricsRegistry::hasPipeline touches no member in that build, so
clang-tidy asks for it to be static. Making it static would give the two
builds different signatures, so suppress it the way Recording.h already
does for the same reason.
- PathRequest.cpp's only use of MetricMacros.h is such a macro, so mark the
include as kept.
- AppMetricGauges.cpp names MetricsRegistry and beast::Journal in a
constructor signature that is compiled either way, so their includes move
outside the telemetry guard.
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.
The rule watched state_accounting_full_transitions > 3 per hour, so a node
that flaps once (one full -> syncing -> full round, e.g. per online-delete
rotation) never tripped it. Lower the threshold to > 0 so a single re-entry
into FULL, past the one-hour uptime gate, alerts.
Keep the state_accounting_full_transitions metric: it is a cumulative gauge
every node always reports, so increase() yields a real series (0 when
healthy) and the rule never evaluates to NoData. A sparse counter would
raise a false DatasourceNoData on a healthy node. Set noDataState: OK so a
scrape gap cannot page either.
The runbook provenance paragraph named the internal AWS dev box and a
build hash and dates. State what was measured (one mainnet node, same
host and binary, differing only in store state) without the deployment
detail, which belongs in an internal runbook, not the public repo.
clang-tidy's bugprone-unchecked-optional-access does not model GTest's
ASSERT_TRUE(x.has_value()), so it flags every deref that follows one.
The reads are guarded; mark them NOLINT, matching the same suppression
in src/tests/libxrpl/consensus/LedgerTrie.cpp. .value() does not help --
the checker treats it as an unchecked access too.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
OTelCollectorHooks.cpp named nothing from Hook.h, took ResourceMetrics
without a direct include, and left the overriding
GetAggregationTemporality() unmarked. NodeIdentity.h carried <optional>
and <string> it no longer uses. All fail CI under warnings-as-errors.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SecretKey deletes operator==, so EXPECT_EQ on the pair's second member
does not compile; compare the byte ranges instead. Application.cpp no
longer calls logicError(), so clang-tidy's include-cleaner rejects
<xrpl/basics/contract.h> there.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Static constants take the k prefix (readability-identifier-naming), and
SpanGuardScope.cpp no longer names anything from <opentelemetry/metrics/noop.h>
since it calls noopMeter(). Both fail CI under warnings-as-errors.
The helper's docstring also claimed NoopMeterProvider hides the base
two-argument GetMeter; it declares that overload itself, so the only
detail worth sharing is the version.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
clang-tidy's include-cleaner flags Counter.h and Gauge.h here: the test
names neither type, so the includes fail CI under warnings-as-errors.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
stop() destroys the MeterProvider, and with it every View's
AggregationConfig. The SDK's SyncMetricStorage keeps a raw pointer to
that config, and the call-site statics keep the storage alive, so a
histogram record with a first-seen attribute set during the shutdown
drain would dereference freed memory. Application::run() stops the
registry before the job queue and server handler, so that window is
real.
phase_ is now atomic and stop() stores Stopped before tearing down.
Every XRPL_METRIC_* macro and every record*/increment* method checks
recording() (enabled and not stopped) instead of isEnabled(). meter_ is
never written after construction, so record threads read it without a
lock.
Also: an empty [telemetry] service_instance_id now falls back to the
node key on both the trace and the metrics side, so one node reports one
identity; disablePipeline() uses telemetry::noopMeter(); comments
corrected.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Lift the seed parsing and the stored-vs-mint choice into two libxrpl
helpers, parseNodeIdentitySeed() and selectNodeIdentity(), so xrpl_tests
can drive each branch without an xrpld Config. resolveNodeIdentity() now
marshals Config and the cmdline into them; behaviour is unchanged.
Also pin that storeNodeIdentity() appends (row count, not SQLite row
order), fix the test header that described getNodeIdentity()'s property
as the store's, and route NullTelemetry::getMeter() through noopMeter().
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Merges pratik/otel-phase8-log-correlation into pratik/otel-phase9-metric-gap-fill.
Auto-merged. TESTING.md picked up phase-8's Test 4 rewrite alongside this
branch's Step 2 standalone-store fix. OTelCollector.cpp gained the weak_ptr
gauges_ list; the StatsD test gained its onCollectionReady() calls.
Merges pratik/otel-phase7-native-metrics into pratik/otel-phase8-log-correlation.
Conflict was one TESTING.md hunk under "Nodes not reaching proposing state":
this branch renamed the node directories to Node-N in integration-test.sh,
phase-7 kept nodeN and expanded the [peer_private] explanation. Resolution
keeps this branch's Node-1 path (its own script uses that naming) and
phase-7's fuller prose citing peerfinder/Config.cpp.
Non-conflicting phase-7 changes come through: OTelCollector's gauges_ list
becomes weak_ptr, matching the earlier hooks_ change; the phase-6 revert of
the StatsD-test onCollectionReady() calls resolved against phase-7's version
that keeps them.
Merges pratik/otel-phase6-statsd into pratik/otel-phase7-native-metrics.
Phase-6 dropped the three onCollectionReady() calls that had been added to
its StatsD test, because that method is only declared here on phase-7.
This branch's own copy of the file was unchanged from the merge base, so
the default merge would have silently deleted the calls from here too —
where they are needed, because this branch gates polling behind
onCollectionReady() in OTelCollectorImp::onTimer.
Resolution keeps both sides: phase-6's two new include lines
(Counter.h, Gauge.h) and phase-7's three onCollectionReady() calls plus
their doxygen and inline explanations. The merged file is exactly
phase-7's tip plus those two includes.
TESTING.md auto-merged cleanly; both sides added text under Test 1 in
different regions.
nodeIdentity_ is no longer a std::optional, so setNodeId() must read it
directly. The merge could not flag this: phase-8 changed the member's type and
this line lives only on phase-9, so neither side of the merge touched the same
file region.
Three conflicts, all from both branches editing the same passage:
- Main.cpp: kept phase-9's wording. The metrics registry only exists here, so
"unwinding destroys little: the metrics registry, whose destructor joins its
export thread" is the true statement on this branch.
- TESTING.md: kept both paragraphs. They document different things (the
private [network_id], and the log path plus log_level).
- 05-configuration-reference.md: composed both. The identity is now resolved
before construction and never empty, so every producer stamps the node key
from the start; the only divergence left is a wallet that already holds a
different key, which corrects the tracer alone. Rewrote the earlier
"three producers" blockquote too: its "no fallback", "first boot ... left
off" and "Known issue" claims are what this change removes.
One silent break the merge could not flag: makeMetricsRegistryOptions() took
the std::optional<std::string> node key that used to be a constructor
parameter, and that parameter is now the resolved keypair. It takes the base58
string directly, and the constructor derives it from nodeIdentity_, which is
declared before both telemetry_ and metricsRegistry_.
gauges_ was still a vector of raw pointers after the hook list moved to
weak references. onCollectionReady() and onCollectionStopping() copy that
list under mutex_ and then call arm()/disarm() with the lock released,
because both enter the SDK's observable registry lock. ~OTelGaugeImpl only
re-acquires mutex_ to erase its own entry, so nothing stopped a gauge from
being destroyed between the snapshot and the dereference.
Store weak references, for the reason the hook list already does: locking
an entry keeps that gauge alive for exactly its own arm or disarm call, and
one destroyed since the snapshot locks to null and is skipped. Registration
moves from the OTelGaugeImpl constructor to makeGauge(), because no
weak_ptr to the object exists until the owning shared_ptr does, and the
destructor now prunes by expiry rather than by address.
The startup log line counts the gauges that were still live rather than the
snapshot length, so a skipped entry is not reported as a failed
registration.
The SDK callback path is unchanged and does not need this: gaugeCallback
takes a void* and is guarded by RemoveCallback being synchronous, as
~OTelGaugeImpl already documents.
resolveNodePublicKey() returned std::nullopt in three real cases: a first boot
with no wallet database, a standalone run (its wallet is a private temporary
database), and --newnodeid. Telemetry's resources are built during
ApplicationImp's member-init list and are immutable, so on those runs the node
reported an empty service.instance.id and no xrpl.node.id for the whole run,
while setup() minted a key moments later and patched only the tracer.
Replace it with resolveNodeIdentity(), which always returns a keypair: derived
from a configured seed, else read from an existing wallet database, else
minted. Main.cpp passes that pair to makeApplication(), ApplicationImp stores
it in nodeIdentity_ -- now declared before telemetry_ and no longer an optional,
because it is always set -- and builds the telemetry resource from it.
setup() calls getNodeIdentity(), which now persists rather than mints: it
stores the resolved pair when the wallet holds no identity, adopts the stored
one when it does, and clears first for --newnodeid. The write stays in setup()
because that is where the database exists; a standalone run has no persistent
wallet to write to, which is why the pair has to be decided before
construction rather than read back afterwards. Wallet gains storeNodeIdentity()
for that write, and getNodeIdentity(session) now uses it instead of repeating
the insert.
The three-argument makeApplication() mints a keypair, so jtx::Env and any other
test Application behave as a standalone run always did.
Also fold the three hand-rolled "meter from a NoopMeterProvider" copies into
telemetry::noopMeter(): the base-pointer call and the kMeterVersion argument are
both easy to get wrong alone, and the meter identity has to match the one the
histogram views select on.
The new gtest covers the wallet half: store-then-read, store not replacing an
existing identity, clear-then-store, and that the mint path persists. It adds
the tests.libxrpl > xrpl.rdb levelization edge, regenerated here.
Test 1 pointed standalone at docker/telemetry/xrpld-telemetry.cfg, whose
[node_db], [database_path] and [debug_logfile] all resolve under
docker/telemetry/data. Standalone builds its own private chain, so that
left one NuDB holding two unrelated chains. This file already states the
rule for the key generation node in Test 2, and the sibling mainnet config
keeps its store under data/mainnet/ for the same reason.
Derive a standalone config with the three paths redirected under
data/standalone/, and run from that.
Note why the flag is not the problem: --start selects StartUpType::Fresh,
but the default Normal reaches startGenesisLedger() through the same branch
chain in ApplicationImp::setup, so any standalone run writes a genesis
ledger into whichever store the config names.
MetricsRegistry created its provider and synchronous instruments in start(),
called from setup() after the node identity was read. Every XRPL_METRIC_*
call site creates its instrument on first use, so any site that ran before
that point found no meter and never recorded again. The start was moved three
times to chase the newest early caller; nothing guaranteed the order.
Build the pipeline in the constructor instead. ApplicationImp declares
metricsRegistry_ right after telemetry_ and before every subsystem, so
declaration order now guarantees the instruments exist before any producer.
start() is gone and its config parsing moves to makeMetricsRegistryOptions().
The registry now guarantees a meter whenever it is enabled: the real one, or
the OTel no-op meter if the pipeline failed to build. disablePipeline() owns
that fallback and its one error log, and the constructor routes both
std::exception and a non-std throw through it, because the SDK is third-party
code. So the macros shrink to one function-local static built from meter()
plus the record call: no once-flag, no null check, and no path for a call
that arrives before the meter, because that state no longer exists. The three
observable macros drop the same now-dead meter check.
The lifecycle is three explicit phases with a Phase enum: Ready at
construction, GaugesArmed by startAsyncGauges() once overlay_ exists, and
Stopped by stop(). startAsyncGauges() checks the phase before the pipeline,
so a second call and a call after stop() are each reported as what they are.
run() stops both observers (insight collector, registry) before any service,
and ~ApplicationImp repeats the stop for the setup() failure paths that never
reach run(). stop() already detaches the callbacks, so it is the only call.
Meter name and version come from kMeterName/kMeterVersion, and the endpoint
default from Telemetry::Setup, so the two metric pipelines share one source
for both.
0eb4291688 added collector->onCollectionReady() to both StatsD tests. That
method does not exist on this branch: it is introduced later, alongside the
polling gate it belongs to, so all four build legs and clang-tidy failed.
Nothing gates polling here. The StatsDCollectorImp constructor starts its
thread, run() calls setTimer() unconditionally, and onTimer polls metrics_
and drains the buffers every second. The gauge test's expectation already
holds without any lifecycle call, because StatsDGaugeImpl starts dirty so a
untouched gauge emits its zero on the first flush.
Restores the two includes that commit also dropped.
The collector readiness note claimed docker-compose.yml publishes only 4317,
4318 and 8889 and that 13133 comes from a workload stack. It publishes 13133,
and that stack is not part of this branch. Probe health_check on 13133 and drop
the note; the troubleshooting entry now points at the same check instead of
carrying a second, weaker copy.
Stop restating BUILD.md. The hardcoded conan and cmake lines had drifted from
it, -Dtelemetry=ON is redundant because the Conan toolchain carries it, and the
conan-release preset resolves only from the repo root, builds into
.build/build/Release rather than .build, and sets no -Dxrpld=ON. Defer to
BUILD.md and docs/build/telemetry.md.
Test 2's keygen step reused the Devnet config with -a --start, which wrote a
genesis chain into the Devnet store, took RPC port 5005 from node 1, and was
followed by an rm -rf that also destroyed the mainnet node's store and every
log. Give it its own config under the test's temp root, as the script does.
The manual path also needs XRPLD_LOG_DIR, or the collector tails the wrong root
and Test 3 finds nothing without erroring.
Neither the template nor the script set [network_id], so a local cluster
stamped xrpl.network.type=mainnet and shared dashboard series with real mainnet
data. Set a private id in both, and say which label it produces. Also drop a
duplicate metrics_endpoint from the generated config.
Split the consensus trigger row: six families fire on a standalone
ledger_accept, and the remaining seven need the establish phase, a validator
key, or a peer. ledger.validate needs peers too, because checkAccept is
unreachable in standalone. Correct the trace-id note to 16 bytes, and name the
strategy it depends on.
The pathfinding bullet said raw account values reach Grafana Cloud. Both
accounts are already tokens when they leave the node; what differs is that the
base config hashes them a second time, so one account carries two tokens across
configs and traces must not be joined across them.
Also: the Loki allow-list is a fixed 18 keys on the pinned image with k8s and
cloud enumerated rather than wildcarded, the runbook documents 9 of 15
dashboards, and the spanmetrics block now uses one spelling with a note that
the cloud config uses the other.
The config template wrote each node's log to a lowercase node{N} directory
while setting service_instance_id=Node-{N}. The collector takes the node name
from the log file's parent directory and stamps it as the Loki
service_instance_id label, so the logs carried a name no trace or metric
shared and nothing joined. Use Node-{N} and state the rule.
The template also set log_level to warning. Nothing in the pipeline filters on
severity; the constraint is that a log line carries trace context only when it
is emitted inside an active span. At warning the only such statements in the
consensus accept span are a catch path a healthy round never takes and a
periodic censorship warning. At info the CNF Val / CNF buildLCL pair writes one
line per accepted ledger, which is what makes this test's Step 1 findable.
Grafana 13 offers the link per span, labelled "Logs for this span", in the
span's Links row — not per trace. Fix the step and the expected-results row.
The example log line quoted a message that does not exist. The real in-span
RPC statement logs at debug, so the severity code is DBG; say which line to
look for under each test, since Test 2 now logs at info.
Drop the reference to workload/validate_telemetry.py: that file is not part of
this branch, and its instant-endpoint call uses seconds, so the nanoseconds
claim applied only to query_range.
On the OTel path only [insight] server is load-bearing. CollectorManager reads
endpoint and hands it to OTelCollector, which logs it at startup and routes
nothing with it; the real export endpoint is [telemetry] metrics_endpoint,
which the template already sets. service_instance_id and service_name in that
section are read and discarded.
Leaving the line invited an operator to reconcile a mismatch that has no
effect. integration-test.sh already emits only server=otel with the same
explanation, so the two now agree.
