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_.
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.
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.
callHooks() copied the hook list into a vector of raw pointers, released
mutex_, then dereferenced them. It has to release the lock: a handler may
drop the last reference to a hook, and ~OTelHookImpl re-acquires mutex_,
so invoking handlers under the non-recursive lock would deadlock. That
left a window in which an entry could be freed before it was used.
The window is not reachable today. Every hook belongs to a long-lived
ApplicationImp member, and onCollectionStopping() runs before those
members are destroyed, both from stop() and from the destructor body.
That call disarms each gauge via RemoveCallback, which blocks until an
in-flight callback finishes, because the SDK holds its registry mutex
across the callback. Safety therefore rests on four separate facts, none
of them enforced by a test, one of them internal to a vendored library.
Store weak references instead, so the code is correct by construction:
locking an entry keeps that hook alive for exactly its own handler call,
and a hook destroyed since the snapshot locks to null and is skipped.
Registration moves from the OTelHookImpl constructor to makeHook(),
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.
Add three GTests over the real collector. A test-local MetricReader
drives one synchronous collection pass, since the SDK ships only a
threaded periodic reader. They assert a live hook runs, a destroyed hook
is skipped, and destroying one hook leaves its siblings registered --
the last pairing both directions so neither can pass vacuously.
Not yet run: verifying them needs a telemetry-enabled build of
xrpl_tests. Compile, clang-tidy and the pre-commit gates are clean.
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.
Three CI failures, one cause each.
macOS could not compile the tracker: Apple's libc++ has no std::atomic for a
shared_ptr, so the primary template's trivially-copyable assert fired. Publish
through boost::atomic_shared_ptr instead, which every standard library the
matrix covers can build. boost/smart_ptr is already used in this tree. The
header's note no longer claims libstdc++ as the assumption.
Four tests that predate the metrics_endpoint scheme guard set tls_client_cert
and only put traces_endpoint on https, so the new guard threw before the check
each one asserts. They now set both endpoints. Nine tests in the two files set a
client cert; the other five stay correct because the pairing, use_tls and traces
checks all run ahead of the metrics one.
Eleven clang-tidy findings: redundant member initialisers, two aggregate
initialisations that wanted designated form, two unbraced bodies, three
unparenthesised multiplications, and a reserve before a loop that emplaces.
Dropping std::make_shared also left <memory> unused in both files.
The two writer paths run on the consensus and ledger-publish threads, and both
took a single mutex that the nine window getters also held while scanning their
deques. At a week of 4-second ledgers those deques hold about 151,000 records,
the same record stored three times, so a read walked half a million entries and
the writers waited behind it.
Each writer now owns a 128-slot ring and shares nothing with the other, so a
record is one timestamp read and one store. A full ring drops the event and
bumps droppedEvents() rather than blocking a consensus thread; at 4-second
ledgers that needs about eight and a half minutes with no drain, against a
worst normal drain gap of one export interval plus one export timeout.
The windows become a grid of one-minute buckets with a running agreed and total
per span, so a getter reads a published snapshot instead of counting. Missed is
derived from the two, which leaves a late repair touching only the agreed side.
Steady-state footprint drops from about 8 MB to about 91 KB, and window edges
quantise to a minute.
Whichever thread is inside reconcile() owns the decision state; a second caller
returns instead of waiting. Readers take a shared_ptr to the snapshot, so the
values they read cannot be overwritten underneath them.
The clock is now injected, which is what makes bucket expiry reachable in a
test. The suite covers pairing, single-sided misses, the grace boundary, late
repair across a bucket edge and past the window, each window edge exactly, two
grid lengths of steady traffic, ring overflow, and three real-thread cases.
It no longer sleeps: nine sleep_for calls totalling 81 seconds are gone.
Every public method keeps its name and signature.
MetricsRegistry::start() takes a StartOptions aggregate. Six call sites in
the #ifndef XRPL_ENABLE_TELEMETRY block still passed a std::string, so the
block did not compile. Nothing in CI compiles it: telemetry defaults ON, and
the block is skipped whenever the macro is defined.
Add one shared kTestStartOptions carrying just the endpoint -- the other
fields are never read on the no-op path -- and correct two comments that
still described the old signature and a #else stub that does not exist.
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.
A StatsDCollector polls its metrics only after onCollectionReady(), so
neither test flushed anything: the gauge test timed out and the counter test
passed for the wrong reason. Call it in both, and drop the two includes
whose symbols the file never names.
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.
getConsensusTraceStrategy() returns ConsensusTraceStrategy, so comparing
it to a string literal does not compile.
No coverage is lost: strategyName()'s spelling has its own assertions in
the TelemetryConfig test, in this same binary.
readability-identifier-naming wants lower_case for a namespace, so
clang-tidy failed on this file under warnings-as-errors. All seven use
sites move with the declaration.
requireReadableFile proved a path readable with getFileContents, which
loads the whole file into a std::string and then drops it. One of the
three paths it checks is tls_client_key, so a private key was loaded to
answer a question that does not need its contents. It now stats the
path, rejects anything that is not a regular file, and opens it without
reading. The message shape is unchanged:
"[telemetry] <key> cannot be read: <path> - <reason>".
A path naming a directory used to escape as an ios failure from the
stream buffer, naming neither the config key nor the path. It is now
rejected as "not a regular file" with both named. The new test covers
that case; it fails against the old implementation and against a copy
with the file-type branch removed.
The runbook's quick start and disable sections both told the reader to
run "cmake --preset default". No presets file is tracked, and the only
preset Conan generates is conan-release, so each of those steps failed
on its first command. Replaced with the flow BUILD.md documents, and
noted that telemetry is the current default while still passing the
flags.
check_statsd_metric queried rippled_rpc_requests, which no pipeline
produces: the collector's statsd receiver runs with is_monotonic_counter,
so the Prometheus exporter appends _total. A wrong name returns zero
series rather than an error, so the assertion could not be told apart
from a broken pipeline. All eight assertions were re-derived from how
each metric is created in code; this was the only counter.
Tempo searches carried no start/end, and tempo-data is a named volume
that `docker compose down` preserves under a one-hour block retention, so
the 17 span assertions could pass on an earlier local run's traces. Bound
every search to this run, and tear the stack down with -v before starting
so no earlier data is present to match. The service-name check now
matches a whole line, because the tag-values endpoint ignores start/end.
Add a gtest for the StatsD gauge that publishes its initial zero and for
the counter that must publish nothing. Assert two metrics the harness
never checked: a traffic-category gauge no message reaches, and
io_context latency.
One conflict, in docs/telemetry-runbook.md: both sides had independently
corrected the same consensus_round_id example. This branch kept the pipe form,
which Tempo rejects as a parse error; upstream moved the predicate inside the
braces, which parses and returns data. Upstream's query is kept, with this
branch's note that the value is the previous ledger sequence plus one.
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.
retries_remaining is stamped on the txq.accept_tx span before the
transaction is applied and before the retry counter is decremented, so a
span with txq_status="retried" always shows a non-zero count and exhaustion
shows up as txq_status="failed" with zero. The attribute comment said only
"retries left before discard", which reads as a post-decrement value and
led to a runbook query that could never match.
Also rename the drifted consensus_trace_strategy value in the plan docs
from "attribute" to "random", the spelling the parser accepts, and note that
it is experimental and not used.
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.
