Compare-then-store is the recurring shape behind these values: read whether
an incoming value differs from the last, store it, report only on a change.
Document changedTo() as doing all three, and state the two constraints -- it
is not synchronized, so it cannot replace an atomic read from another thread,
and it requires a copyable T so its copy semantics match across builds.
Recording.h declares Mirror beside Stopwatch and Counter, so its overview
diagram lists Mirror too and its thread-safety note covers it.
The class documented a name format it does not produce. Eleven comments said
the [insight] prefix is prepended, and gave examples like "xrpld_rpc_size" and
"xrpld_LedgerMaster_Validated_Ledger_Age" that also kept the original casing.
formatName() lowercases the raw instrument name and maps dots and spaces to
underscores, and applies no prefix. All four instrument factories go through
it, so "RPC.Size" exports as "rpc_size". prefix_ is written in the constructor
and read in one place, the startup log line, so nothing it holds can reach an
exported name. The service is identified by the service.name resource
attribute.
Comments only, in both the header and the implementation. The ASCII diagram
still lists prefix_ as a member, which it is.
The overview diagram and the thread-safety note named Mirror<T>, which this
header does not declare. Drop both mentions so the file documents what it
ships.
Mirror holds a plain T when telemetry is compiled in and nothing at all when
it is not. For a non-copyable T that means the class inherits T's deleted copy
in one build and is an empty, freely copyable type in the other -- a
per-configuration difference in the class's own interface, which is what these
types exist to avoid, and a compile-time one rather than merely semantic.
store() assigns to the member, so a copyable T is required regardless. Assert
it in the class body so the requirement is enforced in both builds instead of
being stated only in the @tparam text.
Compare-then-store is the recurring shape: read whether a value differs from
the last one, store it, report only on a change. Mirror does all three in one
call, holds no storage when telemetry is compiled out, and answers false
there so the reporting branch is never taken.
Two conflicts.
RCLConsensus.cpp: upstream restructured makeAcceptSpan so the accept span's
attributes sit behind if (*span). This branch's own contribution there is the
consensus round-duration histogram, which is kept -- placed inside the
telemetry guard but OUTSIDE the span-liveness test, because a metric must
still record when the trace category is disabled or the span was not created.
The duplicated attribute lines on this side are dropped; the guarded block
upstream added supersedes them.
MetricsRegistry.cpp: kept this branch's JobQueue.h include, which it uses.
Its Journal.h include was dropped as a duplicate -- the file already includes
that header higher up, with a comment explaining why it is unguarded, and
readability-duplicate-include is fatal under WarningsAsErrors.
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 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.
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.
copyForwardTotal_ is a second atomic increment beside copyForwardCount_ on the
same event, kept only so a metric never goes backwards: rotate() zeroes the
per-rotation tally for its log line, which leaves that counter unusable as a
rate. Its only reader is MetricsRegistry.cpp:1153, through copyForwardTotal().
Guard the increment. During a rotation window every archive-served
non-duplicate read pays for it, and with telemetry compiled out there is
nothing to read it back.
copyForwardCount_ is untouched: rotate() exchanges it for the "copied forward N
archive-served reads" warning, which is real logging, not instrumentation. The
virtual and the member stay declared unconditionally, so the nodestore
interface has the same shape in every configuration.
updateOurPositions built a 64-character hex string from the transaction
id plus two std::to_string number conversions, then attached them to a
span event. That ran for every dispute that flipped position, on every
establish tick of every consensus round, whether or not anything was
recording.
The three strings and the event have no other consumer; the vote change
itself (mutableSet insert/erase) is untouched.
The block now sits behind if (span). With telemetry compiled out the
stub's operator bool is a literal false, so it is eliminated; with
telemetry on it is also skipped when the span is null because the
establish context was never captured. The guard is inside the function
body, so Consensus<Adaptor>'s adaptor interface is unchanged and the
csf::Peer simulator is unaffected.
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.
The compiled-out ScopedActivation, SpanGuard and ScopedSpanGuard each used a
defaulted destructor. A defaulted destructor on an empty class is trivial, so
compilers report any guard held only for its scope as an unused variable. With
telemetry compiled out that produced seven -Wunused-variable errors under
-Werror, across the ledger acquire, consensus, ledger master and overlay paths.
Write the three destructors by hand so destruction is not trivial, matching the
telemetry-enabled types, and assert that property so it cannot quietly regress
to `= default`. The bodies are empty, so no code is generated either way.
Also move <memory> inside the telemetry guard: only the telemetry-enabled types
hold a unique_ptr, so the include is unused when telemetry is compiled out.
Brings phase-10 up to 3836078a78 (74 commits). Seven conflicts, resolved
per-hunk; no side was taken wholesale.
validate_telemetry.py, four hunks. The module docstring keeps both category
lists, renumbered. _log_prometheus_metric_names takes phase-10's version: it
returns the family list that the new reverse-coverage check consumes, and
_log_name_list prints every family sorted one per line, which supersedes the
hand-maintained prefix filter this branch had been extending -- that filter
existed only to keep the log readable and listed strictly less. validate_metrics
takes phase-10's _metric_check_targets call. assert_sync_diagnostics_metrics and
phase-10's _check_metric_label both landed at the same place; both are kept.
That third hunk carried the hazard this branch had flagged in advance.
_metric_check_targets selects every group satisfying isinstance(dict) and had no
equivalent of SKIPPED_METRIC_GROUPS, because on phase-10 there was no group that
needed excluding. Merged as-is it would have walked sync_diagnostics while
assert_sync_diagnostics_metrics also walks it, polling and reporting all 61
metrics twice. The exclusion is reinstated inside that function, and its
docstring claim that the isinstance test "selects exactly the same groups the
previous name-based exclusion list did" is corrected -- true on phase-10, false
here, and the reason is ownership, which no structural test can express.
expected_metrics.json: both sides appended to metrics_excluded, so both sets are
kept, 29 entries. expected_spans.json: phase-10's fuller pathfind.compute
skip_reason replaces this branch's, and this branch's ledger.acquire ->
ledger.acquire.astree relationship is kept.
Both docs carried stale counts, and the two sides disagreed with each other --
15 dashboards against 16, and both claiming 41 span types when the contract holds
48. Rather than pick a stale side, every figure is recomputed from the resolved
contract: 48 span types as 28 required and 20 optional, 145 metric checks across
26 asserting categories as 140 names plus 5 required_labels, of which 61 are the
sync_diagnostics names, and 16 dashboards, which matches both the uid list and
the files on disk. The runbook keeps phase-10's table, which adds the reverse-
coverage row.
ConsensusSpanNames.h: both sides added different constants to namespace val;
both kept. Confirmed no identifier is redefined -- the merged file's duplicate
set is identical to this branch's, and those duplicates are distinct namespaces
(op::round against the enclosing span::round), not redefinitions.
ConsensusSpanNames.cpp was an add/add: both branches wrote this file
independently, 9 tests here and 8 on phase-10, with no name in common. All 17
are kept. The guard is dropped rather than applied to the union: SpanNames.h
documents that its constants are deliberately NOT guarded by
XRPL_ENABLE_TELEMETRY, ConsensusSpanNames.h has no guard, and the tests this
branch contributed reference ValStatus only in comments while calling
validationStatusValue with plain ints. So they compile without telemetry, and
unguarding them gains coverage in a -Dtelemetry=OFF build rather than losing it.
One defect belongs to the merge itself, appearing on neither parent. phase-10
added a job_queue_per_type_gauges group holding six jobq_<type>_running/_waiting
names; this branch declared jobq_saturation in MetricNames.h. Rule K checks a
name only when its family is owned, so declaring that constant made jobq_ owned
and turned phase-10's six entries into violations. They are beast::insight gauges
created per job type by JobTypeData's constructor, so no constant can exist for
them -- one triple per job type, minted at runtime. The group joins
NON_OTEL_METRIC_GROUPS alongside statsd_gauges for the same reason.
Verification: no conflict markers repo-wide and no unmerged index entries; both
JSON contracts parse; validate_telemetry.py compiles; every count written into
the docs re-derived from the resolved files and matching; span counters still 48
and 74; check_otel_naming.py exits 0, and Rule K proven still able to fail by
injecting a bogus name in an owned family; levelization baseline clean after
regeneration, with 17 incoming include changes; doxygen style clean across all
tracked C++ at CI scope; pre-commit --all-files clean except cargo-fmt, which
reports "Executable `cargo` not found" and touches none of the 0 Rust files here.
NOT compiled -- no approval to build, so the incoming C++ is unverified by a
compiler on this branch.
Splitting the consensus span labels into their own header moved both
constexpr std::string_view helpers out of ConsensusSpanNames.h, but left
the include they needed behind. The header names string_view nowhere now,
and SpanNames.h already provides the type for the conversion operator.
clang-tidy misc-include-cleaner reports this as an error under
-warnings-as-errors, which fails the clang-tidy job for the whole chain.
Event::notify takes std::uint64_t but the header never included <cstdint>,
relying on it arriving through another include. clang-tidy's include-cleaner
flags it, which fails CI on any branch where this file lands in the
changed-file set.
Fixed here, on the branch that introduced the std::uint64_t parameter, rather
than only downstream where it happened to surface.
CI's clang-tidy job failed on this branch. clang-tidy itself succeeded; the
step that failed was the gate that fails the job when findings exist, and the
uploaded diff named exactly two missing includes.
Event.h calls notify(std::uint64_t) but never included <cstdint>, relying on
it arriving transitively. This is pre-existing on the branch rather than new,
surfaced now because clang-tidy only inspects changed files and this one has
not been touched since.
InboundTransactions.cpp uses uint256 throughout and had no direct include for
it either. The round-request work added further direct uses, which is what
brought the file into clang-tidy's changed-file set.
Both fixes are the ones clang-tidy generated itself, applied with the repo's
angle-bracket include style; the include-style and clang-format hooks accept
the placement unchanged.
Note on why this was not caught before pushing: TIDY=1 clang-tidy was run
locally and passed, but against the compile database under
~/sourceCode/.clangdcache/<worktree>/, which was stale -- generated before
these edits. A stale database makes a local clang-tidy pass meaningless, as
project-map.md warns. CI holds the only current one.
