Review feedback on the testing guide:
- rm -rf targeted data/, but this config writes under docker/telemetry/data/,
so teardown did nothing and a second run reused the old NuDB and SQLite
state. Corrected at both sites, including the Test 2 keygen node, which
launches with the same config.
- The standalone span table said consensus.* does not fire. It does:
ledger_accept drives a simulated round, so consensus.round, .phase.open,
.ledger_close, .accept and .accept.apply all appear. Only .establish,
.update_positions, .check, .proposal.* , .validation.receive and
.mode_change cannot. The test intro claimed the same thing and now agrees
with the table.
- Three blocks duplicated content the file already had. Test 1 now points at
the shared Verification Queries section as Test 2 already did, and the
Test 2 submit block checks engine_result like Test 1 does.
- The numbered step list was a copy of the script's own Step N headers and had
drifted by four entries, so it now points at those headers instead.
Also corrects the runbook's ledger and peer span tables against the code:
ledger.build was credited with tx_count and tx_failed, which tx.apply sets,
and was missing its three close-time attributes; peer.validation.receive was
missing ledger_hash and full_validation. The five source line numbers in those
two tables were stale, so they now name the file only, as the other nineteen
rows do.
Review feedback, plus a sweep of the branch for the same defects elsewhere.
Scope: the ledger.validate guard was a plain local, so it stayed alive until
checkAccept returned and the flag-ledger upgrade check ran inside the measured
span. That check reads every trusted validation of the parent, so one span in
256 became a duration outlier for work unrelated to promoting a ledger. The
span is now scoped to the promotion. tryAdvance stays inside it because it only
sets a flag and posts a job.
Attributes: tx.apply now carries ledger_seq, which the runbook already
documented. The parent ledger.build span has it, but a child cannot be selected
by its parent's attributes, so the span could not be found by ledger.
Guard names: each span guard is now named after the span it holds, so
proposalReceiveSpan, validationReceiveSpan, storeSpan and validateSpan. The
name "span" previously meant the trace root in one inbound-message handler and
the job-queue handle in its sibling, which taught a reader the opposite of the
truth in the next function.
Comments: the StatsD gauge rationale now sits with the initialiser it explains
rather than in the constructor. The peer span header described its trust flags
as shared when they are in fact re-declared to match the consensus keys; the
duplication is intentional and the wording was not.
Docs: the ledger and peer span tables disagreed with the code, crediting
ledger.build with attributes that are set on tx.apply and omitting several that
it does set, and all five source-file line numbers in them were stale. The
testing guide listed attribute keys that exist nowhere in the code, so its
catalog now points at the runbook instead of keeping a second copy that drifts.
Both sides added a guarded include block at the same point in
PeerImp.cpp: this branch's consensus receive-span header, and phase-3's
tx-tracing header. Kept both in one guard with a comment covering the two
reasons, rather than taking a side and dropping an include the file needs.
TxTracing.h declares txReceiveSpan() and txProcessSpan(). Both call sites
sit inside an XRPL_ENABLE_TELEMETRY guard, so with telemetry off the header
is included but nothing from it is used, which clang-tidy reports as an
unused header. Guard the include the same way so it is still there for the
default build.
startRoundTracing() is the only reader and writer of prevRoundSpanContext_,
and it is compiled out with telemetry, so a telemetry-off build sees an unused
private field and -Werror rejects it.
Marked rather than guarded: the two sibling span contexts are declared in both
configurations -- one of them escapes the warning only because an unguarded
accessor returns it -- and guarding this one alone would make the member set
depend on the build.
onMessage(TMProposeSet) already owns a ScopedSpanGuard called span, the root
for the inbound peer message. The thread-free handle for the proposal receive
span was declared with the same name in the same scope, so the second
declaration conflicted with the first and every use of it -- the assignment,
the liveness test, the attribute writes and the job capture -- resolved
against the wrong type.
Call it proposalSpan. The validation handler already keeps its two apart the
same way, with valSpan for the root.
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.
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.
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.
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.
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.
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.
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.
