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.
Two if constexpr/else pairs had single-statement bodies holding a gtest macro.
ShortStatementLines exempts a one-line body, but these macros expand to
multi-line constructs and some clang-tidy versions report the expansion's
range, which would make readability-braces-around-statements fire under
WarningsAsErrors. Braces also read better beside an else.
The nine counters have no reader outside telemetry, so the record calls were
wrapped in preprocessor branches at six sites. Holding them in Counter makes
the storage and the increments disappear together, so the call sites read as
ordinary code.
The two deferral sites keep a compile-time block, because their argument is
a string compare that a no-op add would still evaluate.
The unit tests now assert both configurations: every expectation of a non-zero
count has a mirror expectation of zero, so neither build is left unasserted.
Two conflicts, both in PeerImp's proposal and validation receive paths, and
both resolved by taking the incoming side: it holds the span in a handle that
stays empty when telemetry is compiled out and moves every attribute behind
if (span && *span), which supersedes the unguarded form on this side.
Taking the incoming text renamed consSpan to span in both blocks, while the
two job-lambda captures further down had merged cleanly and still named
consSpan. Renamed those captures so each names the handle its own function
declares.
This branch's own guard on the inbound-validation ledger_hash attribute is a
different span in a different function; it merged cleanly and is preserved.
The header told callers to declare members [[no_unique_address]]. That
attribute has no other use in this repo, and MSVC ignores the standard
spelling for ABI compatibility, so following the advice would have been a
portability wart for one byte per member.
Say instead that the compiled-out member collapses to padding, and that what
these types buy is work not being done.
Both broadcast paths passed *msg.mutable_trace_context() to the injector,
which allocates the submessage and sets its has-bit before the injector can
decide there is nothing to write. A compile-time guard covered the
telemetry-off build, but a node with telemetry compiled in and no active
span -- a disabled category, telemetry disabled by config, or a round that
is not being traced -- still broadcast an empty TraceContext to every peer,
and every peer took its has_trace_context() branch to extract nothing.
Add SpanGuard::hasCurrentContext(), a predicate that tests the same two
conditions the injector bails out on without allocating, and an
injectCurrentContext(message) helper that uses it to decide whether to
create the submessage at all. Both consensus call sites now call the helper
unguarded.
Four preprocessor branches around one function -- one for the start
timestamp, one for the elapsed computation, one for the call that reports it,
and one around the reporting method itself -- become none. The clock is still
not read when telemetry is compiled out, because Stopwatch holds no state in
that build.
The reporting method is now compiled in both builds so its call site needs no
guard. Every statement in its body is an XRPL_METRIC_* argument, and those
macros discard their arguments when telemetry is off, so the body costs
nothing there. Its four arguments are all values the request already computed.
std::atomic implicitly deletes copy and move, so a class holding a Counter is
non-copyable when telemetry is compiled in. Compiled out, Counter was an empty
type with no such member, which would have made that same owner freely
copyable in one build only -- the per-configuration API difference these
utilities exist to avoid.
Declare copy and move deleted so both builds agree, and assert it
unconditionally in the tests.
Two conflicts, both in the telemetry include blocks.
SpanGuard.h: kept the union. The incoming side moves <memory> inside the
telemetry guard and adds <type_traits>; this branch adds <initializer_list>,
<utility> and the protocol::TraceContext forward declaration. Guarding
<memory> is correct here: the only std::shared_ptr uses are SpanContext's
member and constructor, both inside the guard, and SpanGuardHandle is a
template parameter name rather than a smart-pointer typedef.
NullTelemetry.cpp: took only the incoming guarded Journal.h block. The
incoming hunk also carried <memory> and <utility>, which this branch already
includes below the guarded OpenTelemetry block; taking them as well would
have tripped readability-duplicate-include.
Sites that exist only to be reported currently spell their own gating: an
#ifdef around a member, another around the clock read, another around the
call that reports it. That puts preprocessor branches through business logic
and leaves each class with a different member set per build.
Add kEnabled, Stopwatch and Counter. Each holds real state when telemetry is
compiled in and is an empty type with no-op methods when it is not, while the
member stays declared in both builds so no class's API differs by
configuration. Tests assert both configurations from one file, including that
the compiled-out types are empty.
The existing helper takes the TraceContext submessage, so every caller
writes *msg.mutable_trace_context(). On a protobuf optional field that
allocates the submessage and sets its has-bit before the helper runs, so a
message ships an empty TraceContext whenever nothing is recorded and its
peers take their has_trace_context() branch to extract nothing.
Add an overload taking the parent message, which decides whether to create
the submessage at all, and correct the header note that claimed the old
helper was already free.
NullTelemetry overrides the other OTel virtuals behind the telemetry guard so
it stays concrete in both configurations, but getMeter was added to the base
without a matching override. That leaves the class abstract in a telemetry-on
build, which compiles today only because its sole instantiation sits behind
#ifndef. Anyone constructing one with telemetry on gets an abstract-class
error pointing at the base, not at the missing override.
Return a NoopMeter from a function-local static, mirroring getTracer.
The three helpers whose bodies are compiled out with telemetry read members
only inside the guard, so in a telemetry-off build they touch no member and
readability-convert-member-functions-to-static fires. WarningsAsErrors makes
that fatal.
Suppress it where the body is gated, matching OverlayImpl::reportDnsResolve.
Making them static instead would give the two configurations different
signatures, which is the hazard the gating pattern avoids.
Five sites on the consensus round path did telemetry-only work whether or
not anything could record it.
onClose set four attributes on the ledger-close span. Two cost real work
once per round: OpenLedger::current() takes currentMutex_ and copies a
shared_ptr just to read txCount, and the mode attribute builds a string.
The block now sits behind if (span).
doAccept read the previous close-time resolution and ran a lambda
returning a std::string, feeding the resolution_direction attribute and
nothing else, once per accepted ledger. Now behind if (doAcceptSpan).
makeAcceptSpan, startRoundTracing and createValidationSpan have wholly
telemetry bodies, so each body sits inside XRPL_ENABLE_TELEMETRY.
makeAcceptSpan then allocates no control block per accepted ledger; an
empty handle is safe because doAccept only passes it to activateIfLive(),
which tests it. Its attributes are additionally guarded on the span being
live. startRoundTracing's early return sits after two virtual Telemetry
calls and a strategy string compare, so the whole body is compiled out
rather than reached each round. createValidationSpan yields std::nullopt,
and its two call sites in validate() test the guard as well as the
optional -- an engaged optional holding a dead guard still turned a
32-byte ledger hash into a 64-character string.
Telemetry.h and SpanNames.h are named only by startRoundTracing, so their
includes are guarded the same way to keep misc-include-cleaner satisfied
when telemetry is off.
onPhaseEvent and onOutcomeEvent are left as they are: the generic
Consensus template calls both and the csf simulator implements both, so
they are part of the adaptor surface.
recvValidation paid a virtual registry lookup plus a call into an
out-of-line function with an empty body for every validation it checked.
That is once per unique validation the node accepts, on the check job
PeerImp queues after dropping duplicates. The registry is always
constructed, so the null test never skipped any of it.
incrementValidationsChecked only advances an OTel counter, published as
validations_checked_total. One dashboard panel and one alert rule are its
whole audience; no RPC reply, log line or control decision reads it.
Guard the call. The MetricsRegistry include stays, because
incrementStateChanges in setMode still uses it and that fires only when the
operating mode actually changes.
Five AcquireStats recording calls ran in every build. The two in TimeoutCounter
are the frequent ones: one on every deferred timer tick and one on every
no-progress tick, for every in-flight TimeoutCounter, and each builds its
argument with isLedgerAcquisition(), which compares the job name std::string
against a literal. The other three fire once per acquisition that aborts, gives
up or completes.
Nothing outside telemetry reads any of the nine counters. The only non-test
caller of any accessor is MetricsRegistry::observeAcquireStats, which itself
sits inside that file's XRPL_ENABLE_TELEMETRY block and so does not exist in a
telemetry-off build.
Guard the five calls, and the AcquireStats include with them, since they were
its only users in these three files. The counters and their accessors are left
alone: with every writer guarded and the only reader absent, they are nine
untouched atomics in one process-wide object, so gating them would add many
preprocessor blocks to the header and force a gated test file to save 72 bytes
of a build that never touches them.
TimeoutCounter::timeouts_ stays outside the guard because the give-up test
reads it, and so does InboundLedger's completionCounted_ latch, which is two
branches once per acquisition and would otherwise be left an unused member.
Both inbound peer-message handlers built a receive span and set its
attributes unconditionally. The proposal path turned two 32-byte hashes
into full hex strings and then took a 16-character substring of each --
four heap allocations per message -- and it ran for every inbound
proposal, trusted or untrusted. The validation path did a field lookup,
two flag reads and a sign-time conversion for every inbound validation,
including the ones dropped just below it for peer divergence or local
load.
The handles are declared empty and only the make_shared sits inside
XRPL_ENABLE_TELEMETRY, so with telemetry compiled out neither path
allocates. The attribute blocks sit behind if (span && *span), which
also skips them when telemetry is compiled in but disabled in config,
and when the consensus trace category is off. It does not skip a span
that exists but was sampled out; that span still pays.
Both job bodies only carry the handle to hold the span alive and never
dereference it, so an empty handle is safe there. The validation span is
still built before the drop decision, so a dropped validation is still
traced; only its cost is removed.
ConsensusReceiveTracing.h has no other user in the file, so its include
is guarded the same way to keep misc-include-cleaner satisfied when
telemetry is off.
PeerImp::onMessage(TMValidation) runs once per inbound validation message,
and it reaches these attribute calls before the HashRouter duplicate
check, so every peer's copy of every validation paid for them. Span
setAttribute is a real inline function whose arguments are evaluated even
when telemetry is compiled out, and to_string(val->getLedgerHash())
heap-allocates a 64-character hex string on each call.
Wrap the ledger_hash and full_validation attributes in if (valSpan), so
neither the string build nor the flags lookup behind isFull() runs when
telemetry is compiled out, when it is switched off in the config, or when
the Peer trace category is disabled. A span that exists but was sampled
out still pays; there is no isRecording() to test.
peer_id and validation_trusted stay unguarded: their arguments are an
integer cast and a bool the surrounding logic already computes.
The comments claimed the guard skips work for a span that is "not being
recorded", which reads as sampling awareness. It has none: operator bool() is
impl_ != nullptr, and the span factories return an empty guard only when
telemetry is absent, disabled at runtime, or the trace category is off. A span
that exists but was sampled out still pays.
There is no isRecording() in the telemetry API, so the guard is still the
strongest available; only the justification was overstated.
The comments claimed the guard skips work for a span that is "not being
recorded", which reads as sampling awareness. It has none: operator bool() is
impl_ != nullptr, and the span factories return an empty guard only when
telemetry is absent, disabled at runtime, or the trace category is off. A span
that exists but was sampled out still pays.
There is no isRecording() in the telemetry API, so the guard is still the
strongest available; only the justification was overstated.
updateAll's update_all span is wholly telemetry: the optional guard, the
empty-requests test that decides whether to emit at all, and the two
attributes have no reader outside the span. It runs on every ledger close, so
with telemetry compiled out the function still constructed a stub guard and
discarded pathfind_ledger_index and pathfind_num_requests once a close for
nothing. Everything the rest of updateAll depends on, including the
isNewPathRequest() flag reset, is outside the block and unchanged.
No span object exists to test before it is created, so the guard is an #ifdef
over the whole block. The three includes it was the sole user of --
PathFindSpanNames.h, SpanGuard.h and <optional> -- are gated the same way,
because otherwise they would be unused includes in that build and
clang-tidy's misc-include-cleaner would reject them.
Two pieces of pathfinding telemetry ran regardless of the build.
doUpdate fills pathfind_dest_currency by rendering the destination asset for
the pathfind.compute span. For a non-XRP issue that is a base58 check encode
of the issuer, two SHA-256 rounds, then a SHA-512Half over the result and
three string allocations. It is a call argument, so it ran even where
setAttribute's body is empty. doUpdate is not a cold path: besides once per
pathfinding RPC, PathRequestManager::updateAll calls it once per active
path_find subscription on every ledger close, so a node with N subscriptions
paid N times a close. It now sits inside "if (span)" with the cheap
pathfind_fast flag, so it is skipped with telemetry compiled out and also for
any span that is not being recorded.
findPaths keeps a totalPaths counter across its per-source-asset loop. Its
only reader is the pathfind_num_paths attribute at the end of the same
function, so the counter is maintained only when telemetry is compiled in.
That needs an #ifdef rather than "if (span)": the attribute cannot read a
variable that does not exist, and a counter kept up to date but never read is
an unused variable, which fails the build.
doPathFind and doRipplePathFind fill two span attributes from the request's
source and destination accounts. Both values are call arguments, so they are
built whatever the build: asString() copies the address out of the JSON and
redactAccount() takes a SHA-512Half over it and formats 16 hex characters.
That is two copies and two hashes on every pathfinding RPC, for
pathfind_source_account and pathfind_dest_account, which nothing outside the
span reads.
Wrapping the block in "if (span)" drops that work when telemetry is compiled
out, where the guard's operator bool() is a literal false, and also when
telemetry is on but this span is not being recorded. The const-reference read
of context.params moves inside the guard with the code that needs it, so a
telemetry read still never inserts a null into the request.
resolveCommandSpanName() and the error span in doCommand() exist only to
name and label a telemetry span. With telemetry compiled out the
resolver still ran on every RPC that fillHandler() rejects: up to five
json isMember lookups, up to three string copies, a virtual config()
call and a handler-table lookup, all to build a name nobody records. A
storm of malformed requests paid that cost once per request.
A runtime `if (span)` guard cannot work here. The resolver's result is
the span name itself, passed as the third argument of the
ScopedSpanGuard constructor, so no span object exists yet to test. That
leaves `#ifdef XRPL_ENABLE_TELEMETRY`, matching the house style used
elsewhere.
The guard covers the whole telemetry block at the call site and the
helper definition too, so the file-static helper does not become an
unreferenced function, which the build rejects because warnings are
errors. injectError() and the error return stay outside the guard, so
behaviour on the failure path is unchanged. No include is orphaned:
ErrorCodes.h, SpanGuard.h, RpcSpanNames.h and <string_view> all keep
uses outside the guards.
With telemetry on nothing changes: only comments and the four directives
were added.
TxQ::accept set two of its per-transaction span attributes unconditionally, so
every queued transaction the loop tried to apply to the open ledger paid for a
64-char string built from the transaction id and a transToken() lookup that
builds a string, whether or not anything recorded them. That is once per
candidate clearing the required fee level, on every ledger close.
Guard both on the span being active, as the enqueue path in TxQ::apply and the
apply-pipeline spans already do. The transaction apply itself stays outside the
guard; only the attribute that reads its result is telemetry.
The rpc.ws_message span's command attribute was resolved for every
inbound WebSocket message, whether or not anything was recording it.
The resolver does two JSON member tests plus two subscripts, copies the
command into a std::string, calls getAPIVersionNumber and then looks the
name up in the handler multimap. Because it is a call argument to
setAttribute, it ran even with telemetry compiled out, where
setAttribute's body is empty.
Wrapping the call in "if (span)" drops that work entirely when telemetry
is off, and also when telemetry is on but this span is not being
recorded. Nothing outside the attribute reads the resolved value, so no
other behaviour changes; the real request validation further down
computes its own api version, command string and handler role.
doAccept's canonical-tx-set loop built a 64-character hex string from
every transaction hash and added a span event for it, then reported a
txCount that only the span reads. That ran once per transaction in every
accepted ledger, whether or not anything was recording.
txCount and txHash have no other consumer: txCount is only read by the
tx_count attribute, and txHash only by the tx_included event. buildLCL
takes retriableTxs, not the count.
Both now sit behind if (doAcceptSpan). With telemetry compiled out the
stub's operator bool is a literal false, so the blocks are eliminated;
with telemetry on they are also skipped whenever the span is null
because the consensus trace category is off.
TxQ::apply set its enqueue-span attributes unconditionally, so every transaction
paid for two 64-char hash strings and a TxFormats lookup whether or not anything
recorded them. The open-ledger rebuild replays transactions through this same
path, so it was paid more than once each.
Guard the block on the span being active, as the tx apply-pipeline spans do.
Transactor::operator()() set its apply-stage attributes unconditionally, so
every transaction applied paid for a TxFormats::findByType() lookup and a
64-char string built from the view's parent hash, plus one or two transToken()
lookups in the exit funnel, whether or not anything recorded them.
Guard both blocks on the span being active. This is the pattern the sibling
preflight and preclaim spans already use in applySteps.cpp, with a comment
giving this exact reason -- the apply stage was simply missed.
Behaviour is unchanged where the span is live, and setAttribute on an inactive
guard was already a no-op.
jobQueue, jobStart and jobFinish each called into the metrics registry, so every
job paid for three of them. The recordJob* bodies are compiled out without
telemetry, but the calls were not: each still made a virtual
getMetricsRegistry() call, and each built its argument with JobTypes::name(),
which is a std::map lookup plus an assert. The registry is constructed
unconditionally, so the null test never short-circuited any of it.
Guard the five recording blocks, and the registry include with them: the only
other thing naming that type is the metric macros' expansion, which is also
compiled out. The two in-flight UpDownCounter macros stay unguarded -- they take
only app_, which costs nothing, and the macro drops it.
This is the highest-frequency site in the audit: three calls per job against one
per transaction elsewhere.
The tx.receive and tx.process spans were built with make_shared whatever the
build, so a node without telemetry allocated once per inbound, submitted and
relayed transaction -- duplicates included, since tx.receive runs before the
duplicate check -- to hold an empty object.
Leave the handle null in that build instead. Nothing needed to change to carry
it: both doTransaction* overloads already default the span to nullptr, the job
capture and activateIfLive() accept a null handle, and the apply loop already
tested `e.span && *e.span` before using it. The remaining uses now test the
handle, which they have to do anyway once it can be null.
With telemetry compiled in the behaviour is unchanged, and the attribute block
is still skipped for any span that is not being recorded.
Every node write folded its queue depth into an accumulator, took two
steady_clock samples and updated four atomics, whatever the build. Nothing
consumes any of it without telemetry: the write-stats gauges are its only
reader.
Guard the accounting so a telemetry-off build matches develop exactly. That
includes putting getWriteLoad() back to develop's `return 0`, which matters
beyond cost: instrumenting this turned a hard-coded constant into a live depth,
which quietly armed LedgerMaster's kMaxWriteLoadAcquire history gate and changed
what get_counts reports. Neither should follow from adding telemetry. With
telemetry on, all of it behaves as before.
getWriteStats() now reports absence rather than zeros in that build, which is
the answer the base class already gives for backends that do not measure, and
which readers distinguish from measured-and-idle.
Also drops the depth parameter recordInsert never read, and gates the counters
themselves since their only readers are the two guarded accessors.
The four write-path tests are guarded to match, and the backend and database
tests now derive whether NuDB measures from the build instead of hard-coding it,
so their existing absence paths cover the compiled-out case.
Both the proposal and validation broadcast paths called
injectCurrentContextToProtobuf(*msg.mutable_trace_context()) unconditionally.
The injector is a no-op when telemetry is compiled out, but its argument is not:
mutable_ on an optional submessage allocates it and sets its has-bit. So a node
built without telemetry put an empty TraceContext in every proposal and every
validation it broadcast, and made each receiving peer take its
has_trace_context() branch for nothing.
Guard both calls. The relay path in NetworkOPs already tests its span before
touching mutable_trace_context(), so this brings the two in line.
Both tx.receive and tx.process set their attributes unconditionally, so the
work happened even when nothing consumed it: a 64-char hash string allocation,
a getCurrentLedgerIndex() call that takes the ledger master's lock, a
TxFormats lookup, a peer-version string copy, and in tx.process a fee and
sequence decode. tx.receive runs before the duplicate check, so duplicate
relays paid for it too.
Guard both blocks on the span being live. When telemetry is compiled out the
guard's operator bool() is a literal false and the block is eliminated; when it
is compiled in the block is skipped for any span that is not being recorded,
which the previous code could not do. Behaviour is unchanged where the span is
live, and setAttribute on a null guard was already a no-op.
The remaining attributes at the exit paths keep their unconditional calls: their
arguments are compile-time constants, so there is nothing to save.
ValidationTracker::pending_ was pruned only by evictOldPending(), which is
reachable only from reconcile(), which runs only from the observable-gauge
callbacks. Those callbacks need telemetry compiled in and enabled, so any node
without both recorded one entry per validated ledger and freed none -- roughly
2.4 MB a day, plus a mutex acquisition per ledger. kMaxPendingEvents did not
help: that check lives inside the function that never ran.
This also affected ordinary builds, not just telemetry-off ones, because
startAsyncGauges() returns early when [telemetry] enabled=0 and so never
registers the callbacks.
Bound the map where the bound can actually be guaranteed -- the insert path --
by dropping the oldest entry once it is full. A container has to be bounded by
its writer, not by whoever happens to read it.
Also stop doing the work when nothing will consume it: hold the tracker, its
accessor and its header behind the telemetry guard, and check isEnabled() at
both record sites, which the metric macros do but these direct calls did not.
Adds pendingCount() so the bound is observable, and a test that records 8000
ledgers without ever reconciling and asserts the size settles at a fixed cap.
The metric macros discard their arguments when telemetry is compiled out, so a
file's used-symbol set differs from the one its includes and declarations were
written for. That produced eleven clang-tidy errors here, all from that cause.
- guard <atomic>, <functional> and <memory> in MetricsRegistry.h, and
<algorithm> and <optional> in DatabaseConfig_test.cpp, to match their uses
- include Journal.h directly in MetricsRegistry.cpp, whose constructor takes a
beast::Journal by value in both configurations
- compile out recordGetObjectMetrics, its declaration, its call site and the
two clock reads that feed it, since every statement in it records a metric
- drop two duplicate includes and rename a function-local static constant to
kEmpty
ValidationTracker carries no telemetry guards and neither does its test, so
both are compiled whatever the telemetry setting. Its implementation and the
src/ include path it needs were added only inside if(telemetry), so a build
with telemetry off could not reach the header and left fifteen of its symbols
unresolved at link time on every platform.
Hoist both out of the guard. The else() branch already does this for
MetricsRegistry, so the case was anticipated there and missed here.
std::size_t names the trace-id and span-id hex widths, which are compiled only
when telemetry is enabled, so the include is unused otherwise and clang-tidy
fails the build on warnings-as-errors.
isValidTraceContext and std::uint8_t are named only inside the two
telemetry-enabled branches, so with telemetry compiled out both includes are
unused and clang-tidy fails the build on warnings-as-errors.
isValidSpanId and std::uint8_t are named only inside the telemetry-enabled
branch of txReceiveSpan, so with telemetry compiled out both includes are
unused and clang-tidy fails the build on warnings-as-errors.
With telemetry compiled out, <string_view> in Telemetry.h and <memory> in
SpanGuard.h are named only by declarations that are themselves guarded, so
clang-tidy's include-cleaner reports them unused and warnings-as-errors fails
the build. NullTelemetry.cpp has the mirror problem: it names beast::Journal
only in the compiled-out makeTelemetry(), reaching the type transitively.
Guard each include to match the configuration that uses it, and correct three
comments that overstated what the empty destructors cost.
