Resolved transaction-overview.json by composing both sides: this branch's
renamed span_calls_total metric and its 15s interval, with phase-6's removal of
the suppressed grouping and its new legend.
tx.receive is created after the duplicate check, so it never carries a
suppressed attribute. The Tempo tag filter and the dashboard panel still
referenced it, and check_otel_naming rules C and D fail on a reference with no
constant behind it.
The panel keeps its remaining signal as a plain receive rate. How many relayed
copies were dropped is the transactions_duplicate traffic category, which does
not depend on a span.
The spanmetrics dimension goes too; a dimension for an attribute nothing sets
only widens the label set.
A peer relays every transaction it hears, so most inbound copies are ones
handleTransaction() drops. The tx.receive span was created before those checks
ran, so every dropped copy paid for a span, a hex transaction id, and an
open-ledger index read that takes the mutex the apply path needs.
Measured over a four-hour payment run: 83.2M of 101.1M tx.receive spans
described a copy the node dropped. That is 82% of this span and 23% of every
span the node emitted.
Move the span and its attributes below the duplicate check. How many copies
were dropped is already reported as the transactions_duplicate traffic
category, which costs no span. Why a copy was dropped is no longer recorded;
a labelled counter restores it on the branch that carries the metric registry.
The suppressed attribute and the suppressed and rejected_inner_batch status
values go with it. This function was their only user.
CMakeLists conflicted: this branch had rewritten the description to name
SpanGuard, phase-1b removed the option() call. Kept both — this branch's wording
with phase-1b's structure.
CMakeLists declared option(telemetry) with a default of its own, so the setting
had two homes and they were free to disagree. The Conan option already reaches
CMake without it: conanfile.py forwards the option into the generated toolchain,
which sets the variable this file reads, and every build here goes through Conan.
Drop the option() call and keep the if(telemetry) test. docs/build/telemetry.md
told readers a CMake option had to be set as well, so that goes with it.
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>
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-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.
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.
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.
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.
The Payment destination was not a valid XRPL address — its base58 checksum
does not match — so Test 1 Step 4 and Test 2 Step 7 could never have returned
the tesSUCCESS they claim. Use a valid one and note that the destination does
not need to exist.
The Tempo search loop had no -G, so curl posted the parameters as a body,
Tempo answered 200 while ignoring the query, and every span name came back
non-zero. It also had no time bound, and Tempo keeps blocks for an hour, so a
re-run was answered by the previous run's traces. Add -G, RUN_START, and
start/end, matching what integration-test.sh already does.
Split the query list in two: 35 names that should be present, and 8 that need
a trigger neither test performs, where zero is the expected answer. Previously
two of the latter sat in the pass/fail list and read as failures.
Correct the standalone span table. consensus.mode_change fires once per round
start whether or not the mode changes, ledger.validate cannot fire because
checkAccept is unreachable in standalone, and the apply-stage, TxQ and ledger
families were missing rows. Give each "No" row the reason that actually
applies: the establish phase, a missing validator key, or no peers.
Also: ledger_accept is not required before submit, the teardown pgrep matched
more than this node, [peer_private] also disables the inbound listener, and
the 15-second wait covers Tempo but not Prometheus.
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.
Four conflicts, all where this branch's replacement of the StatsD path with
native OTLP met the upstream spanmetrics -> span_metrics rename. This branch's
design wins in every case; the rename is carried into its text rather than
reverting it, so the connector, its pipeline references, the header comment,
the TESTING.md summary and the runbook all use span_metrics while keeping the
native-OTLP wording.
One addition beyond a straight take-a-side: publish the collector's health
check port. This branch restored the health_check extension and its own
TESTING.md polls http://localhost:13133/ to decide the collector is ready, but
the port was never published on this side of docker-compose.yml, so that check
could not pass from the host. Verified the merged config loads with no
deprecation warnings and that 13133 is published exactly once.
No metric name changed: traces_span_metrics_* already read that way before the
rename, which only ever touched component names and prose.
Conflict in docker/telemetry/otel-collector-config.yaml, in the service
pipelines: this branch renamed the deprecated spanmetrics connector to
span_metrics and adds the statsd metrics pipeline, while upstream renamed the
deprecated otlp exporter to otlp_grpc. Both kept.
With both renames present the collector now starts with no deprecation
warnings at all, which was the point of the pair.
