State that exists only to be reported was being written with preprocessor
branches at each site, which put #ifdef through business logic and gave the
owning class a different member set per build.
Document kEnabled, Stopwatch and Counter, and the two constraints that decide
whether they fit a site: a no-op method still evaluates its arguments, and
if constexpr still type-checks the branch it discards.
The table of contents in this file indexes third- and fourth-level headings,
so a new subsection that is absent from it is a gap rather than a style
choice.
Compiled out, compiled in and tracing, compiled in with no active span, and
compiled in but disabled by config all have to produce the right wire bytes,
and only two of them are obvious. Tabulate them, and record why the predicate
reads the context directly instead of calling GetSpan(), which allocates a
DefaultSpan in the no-span case.
mutable_ on a protobuf optional submessage allocates it and sets its has-bit
at the call site, before the helper can decide there is nothing to write. A
caller that dereferences it ships an empty TraceContext whenever nothing is
recorded, and its peers each take a branch to extract nothing.
Document the rule with the right and wrong forms side by side.
The conditional-compilation section promised zero overhead when telemetry is
not wanted. The span disappears, but the arguments passed to it do not: the
compiled-out guards are ordinary inline functions, so a to_string() or a hash
in an argument list still runs and its result is then discarded.
State that, show the guard that does remove the work, and name the opposite
case -- the metric macros, which discard their arguments and need no guard.
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.
The devnet config was missed when the mainnet one was corrected. On the
OTel path prefix is inert, because formatName() ignores it, and
service_instance_id is read and then discarded -- OTelCollector.cpp does
`(void)instanceId`. The service_instance_id label Prometheus shows comes
from [telemetry] service_instance_id, which this file still sets, so
dashboards keep filtering by node.
The comment removed here claimed every insight-backed panel goes empty
without the [insight] copy of the key. That is not the case.
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.
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.
prefix and service_instance_id are read and thrown away on this path, so
the node's identity label comes from [telemetry] instead. The old comment
claimed the insight copy was required or panels would be empty.
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.
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.
