One conflict, in docs/telemetry-runbook.md: both sides had independently
corrected the same consensus_round_id example. This branch kept the pipe form,
which Tempo rejects as a parse error; upstream moved the predicate inside the
braces, which parses and returns data. Upstream's query is kept, with this
branch's note that the value is the previous ledger sequence plus one.
Two conflicts, both additive.
TelemetryConfig.cpp: this branch added requireHttpsEndpoint next to
requireReadableFile; upstream added readConsensusTraceStrategy at the same spot.
Both kept.
05-configuration-reference.md: this branch added the two client-certificate rows
while upstream corrected the consensus strategy value from attribute to random.
Both kept. Also drops the stale "not yet implemented" row for
consensus_trace_strategy, which the merged table now contradicts twice over: the
option is parsed, and its value is no longer spelled attribute.
retries_remaining is stamped on the txq.accept_tx span before the
transaction is applied and before the retry counter is decremented, so a
span with txq_status="retried" always shows a non-zero count and exhaustion
shows up as txq_status="failed" with zero. The attribute comment said only
"retries left before discard", which reads as a post-decrement value and
led to a runbook query that could never match.
Also rename the drifted consensus_trace_strategy value in the plan docs
from "attribute" to "random", the spelling the parser accepts, and note that
it is experimental and not used.
The OTLP/HTTP exporter selects TLS from the endpoint URL scheme alone
(HttpSslOptions in the pinned SDK matches "https:" exactly), so a client
certificate handed to it alongside an http:// traces_endpoint is loaded and
never presented. The parser checked the cert/key pairing, use_tls and file
readability, but never the scheme, and the default traces_endpoint is plain
HTTP. makeTelemetrySetup() now requires traces_endpoint to start with
"https://" whenever tls_client_cert is set, including when the key is left
at its default.
Nothing asserted the client options reaching the exporter, so a swapped
certificate and key would have passed every test. Move the options mapping
into makeTraceExporterOptions() and assert it at that boundary with
distinct certificate and key paths, plus a one-way-TLS control and a
use_tls=0 control. One case runs the whole path from a [telemetry] section.
Runbook and example-config fixes:
- tx.included is emitted per transaction of the agreed consensus set,
before buildLCL() applies anything, so it is a superset of the accepted
ledger rather than proof of inclusion.
- the dispute.resolve query used the descendant operator, but the event is
on the consensus.update_positions span itself, so it matched nothing.
- the exhausted-retries query asked for txq_status="retried" with
retries_remaining=0, which cannot occur: the attribute is stamped before
the attempt and the retried branch only runs while retries are left.
Exhaustion is txq_status="failed" with a zero count.
- consensus_round_id is an int64, so the two queries comparing it to a
quoted string matched nothing.
- note that consensus_trace_strategy=random is experimental and not used.
- note that a trailing "| attr = value" is rejected by current Tempo;
attribute filters belong inside the braces.
consensus_trace_strategy was read as a std::string and compared against the
literal "attribute" in startRoundTracing(), while the runbook documented
"deterministic" and "random". The documented value "random" therefore fell
through to the default and did nothing.
Parse the setting once into ConsensusTraceStrategy, so the consensus code
branches on a type. The accepted spellings are now "deterministic" and
"random"; anything else fails at startup instead of silently defaulting.
The behaviour behind the old "attribute" name is unchanged and is now
reached by "random".
Document consensus_trace_strategy in xrpld-example.cfg, stating that
"random" is experimental and not used: it gives each node its own trace id,
so one round arrives as one trace per node.
Also state on the tx.included event that it covers the agreed consensus set
before the ledger is built, so it is a superset of the accepted ledger.
Two conflicts, both additive on each side.
TelemetryConfig.cpp: include blocks only. This branch added FileUtilities.h for
the certificate readability checks; upstream added <limits> and <optional> for
the bounds parser. Both kept.
The TelemetryConfig test: this branch's mutual-TLS cases and upstream's
batch-bounds cases were added at the same positions, so the file is rebuilt from
both stages and carries all 32 tests. Two shared cases were each edited by one
side only, so the edited side wins in each: upstream asserts the batch defaults
in parse_empty_section, and this branch's parse_full_section writes a real
certificate file, which is now required since the parser opens it.
One conflict, in 06-implementation-phases.md: this branch had rewritten the
phase-4 task table with a Status column, a descoping note and a Spans Produced
section, while upstream corrected the class name in the old plain table. This
branch's section is kept and the name correction re-applied to its 4.1 row.
One conflict, in SpanGuard.h: this branch added struct TraceBytes and upstream
added enum SpanRole at the same position after TraceCategory. Unrelated
declarations, so both are kept.
Guards the validation the parser gained upstream: zero rejected for all three
keys, a non-numeric value raising std::runtime_error rather than leaking
boost::bad_lexical_cast, a negative value rejected instead of wrapping to
4294967295, both bounds accepted exactly, and batch_size held at or below
max_queue_size.
The catch is std::runtime_error, not std::exception, on purpose: if the parser
ever stops wrapping, a bad_cast escapes and the suite fails loudly instead of
swallowing it.
Two conflicts, both resolved by composing the sides rather than taking one.
cfg/xrpld-example.cfg: this branch had moved the batch-processor keys under
their own heading while upstream edited them in place, so a merge-both would
have documented them twice. Upstream's range sentences are applied to the
relocated block and the head-sampling note keeps its position.
02-design-decisions.md: the summary table changed on both sides for different
reasons. Upstream renamed ledger_index to current_ledger_seq and ledger_seq;
this branch had corrected the PathFinding row to the keys it actually emits.
Both are kept.
The category mapped every Rpc span to kServer, so one inbound request emitted
several nested server spans. Per the trace spec, SERVER covers server-side
handling of a remote request the client awaits, while INTERNAL is an operation
with a local parent. rpc.process and both rpc.command sites have a local parent,
so they now pass SpanRole::Internal.
The four transport-edge roots keep the category default: rpc.http_request,
rpc.ws_upgrade, rpc.ws_message and the gRPC span each begin a remote call. This
matters to Tempo's service-graph and span-metrics generators, which pair server
spans with client spans and leave a surplus one unpaired.
Both record entry points reached their record with pending_[ledgerHash], and
operator[] default-constructs on a missing key. An all-zero recordTime was the
only signal for "first time seeing this ledger", so it could not tell a new
ledger apart from one already counted and evicted.
Reconciled entries are removed on purpose once they leave the late-repair
window, and reconcile() adds a ledger to the totals only on its first
reconcile. A validation arriving after its entry was counted and removed
therefore rebuilt a blank record, looked new, and was counted a second time -
in the lifetime totals and in all three rolling windows, so the agreement
percentages skewed too. A re-arrival carrying only one side manufactured a miss
for a ledger first counted as an agreement.
Both entry points now go through pendingEvent(), which looks in pending_
first so a ledger still awaiting late repair keeps updating its record, then
checks tallied_ and records nothing for a ledger already counted. That retires
the all-zero sentinel, which was the defect itself.
tallied_ holds hashes only, since the record is gone by then, and is bounded at
kMaxTalliedEvents with oldest-first eviction to match evictOldPending. A
validation for a ledger counted more than that many ledgers ago is counted
again; the comment says so.
The message counters keep incrementing on every call, including for an ignored
ledger. They count messages rather than ledgers.
kMaxPendingEvents becomes public so the new test sizes its fill from the
production constant instead of copying the number. Its value and meaning are
unchanged.
startRoundTracing runs as an argument to Consensus::startRound, so it creates
consensus.round before startRoundInternal applies the new mode. Reading mode_
there recorded the previous round's value, and a validator switching from
observing to proposing got a round span labelled observing that nothing corrected.
The attribute is now written in onModeChange, from the mode being applied. All
three MonitoredMode::set paths funnel through there, so round start, a wrong-ledger
switch and a bow-out all correct the parent span with one statement. Every path
reaches it under RCLConsensus::mutex_ on the thread that created the span.
The stale write is removed rather than kept alongside: neither Consensus::startRound
nor startRoundInternal has an early return before mode_.set, so every round span is
stamped. If a future path ever skipped it the attribute would be absent, which reads
as a gap, instead of confidently wrong. onClose also sets consensus_mode, from the
engine's own parameter, and is correct as it stands.
Also tests addEvent's attribute overload on a live span, reading the exported event
name and each value back off the in-memory exporter. It was previously only ever
called on a null guard, so a dropped attribute exported nothing and failed nothing.
Review feedback asked for a Histogram rather than a span attribute at these two
places. Both, not either: the attribute answers how big one sampled request was,
which an aggregate cannot, and the histogram answers the distribution across all
requests, which an unsampled trace never reveals. Both attributes stay.
The metrics land here rather than with the attributes because neither
HistogramBuckets.h nor the metric macro exists on the branch that added them.
Both use kObjectCountBuckets. The argument is the floor, not the ceiling: the SDK
default edges start 0,5,10,25, so an ordinary batch of one to five falls in a
single bucket and every quantile becomes an interpolation on one edge. The object
ladder puts five edges over the mass of both distributions. Path count is bounded
at 352 by kMaxPaths times kMaxAutoSrcCur and cannot saturate. Batch size can, at
roughly 333k, but no measured traffic goes near it, so the ladder is not widened
for a range nothing occupies; the runbook records the overflow query and a test
asserts it stays readable.
tryDirectApply returns an engaged optional whenever the fee bar was cleared,
including when xrpl::apply() failed, so testing the optional labelled failures as
applied. TxQ_test's fail-in-preclaim case hits exactly this: the fee clears the
bar and preclaim then rejects with terINSUF_FEE_B.
The stamp now branches on ApplyResult::applied. A failure reports failed rather
than falling through to the default rejected, because rejected means the
transaction got nowhere, while this one cleared the fee bar and ran through
apply(). ter_code is recorded either way, so the failure is diagnosable. Both
values already existed and are used the same way by the queued-apply path in this
file, so the vocabulary is unchanged.
The value set in the phase-3 task list is updated to match, including a ter_code
row for txq.accept_tx that was already emitted but undocumented.
Span kind was derived from TraceCategory alone, so every Rpc-category span was
kServer. A category cannot tell an inbound handler from the internal work under
it, and trace backends pair kServer with kClient, so internal spans left as
kServer become unpaired edges in a service graph and read as extra inbound
requests.
SpanRole is a new xrpl-owned enum, orthogonal to TraceCategory: the category
names the subsystem and gates the span on config, the role says whether the span
handles a remote call. It is a defaulted fourth parameter on span(), freshRoot()
and the ScopedSpanGuard equivalents, defaulting to SpanRole::FromCategory, so no
existing call site changes. resolveSpanKind() applies an explicit role and falls
back to the category map, which keeps its single responsibility. The
telemetry-disabled stubs mirror all four signatures.
No call site passes a role yet. The two that need it are on a later branch.
Also fixes a ScopedSpanGuard example that passed a bare op:: suffix to
childSpan(), which takes the name verbatim. Naming the child rpc.command made it
a child of rpc.command.<cmd>, inverting the hierarchy, so the example's parent is
now rpc.process and the command attribute moved onto the command span.
Two review findings on the telemetry library.
SpanContext::isValid() returned impl_ != nullptr, so it answered true for a
context holding no span. threadLocalContext() wraps whatever GetCurrent()
returns, and that is an empty Context on a thread with no active span, which
contradicted the documented "invalid context if none is active". It now asks the
Context for its span. childSpan(name, ctx) is the one caller whose behaviour
changes: a context with no span used to produce a new root span, and now returns
a null guard as its @return already promised.
The three batch settings went to the OTel BatchSpanProcessor unchecked. Three
ways that failed: zero was accepted for all of them; batch_size could exceed
max_queue_size, which the SDK documents as a precondition and does not enforce;
and a mistyped value let boost::bad_lexical_cast escape, which derives from
std::bad_cast rather than std::runtime_error, so the operator saw a bare "bad
cast" naming no key. Reading unsigned also turned "-1" into 4294967295 instead
of failing, so the value is parsed signed and negatives are rejected.
xrpld-example.cfg now states the ranges.
Nine files conflicted. Resolutions, and why:
Telemetry.cpp - upstream carried its own initMetrics(), makeResource() and
makeMetricExporter(); this branch already has an initMetrics() that builds the
exporter inline and uses makeMetricsResource(), which stamps xrpl.node.id only
when it is already known. Keeping both would have defined initMetrics() twice.
Kept this branch's, then pointed its reader at setup_.metricExportInterval and
setup_.metricExportTimeout: upstream turned those constants into [telemetry]
keys and removed the old ones, so the previous spelling no longer resolves.
OTelCollector.h - kept upstream's parameter docs. This branch's text promised
instanceId, serviceName and networkType become resource attributes; the
constructor marks all three [[maybe_unused]] and the .cpp already says they are
not read.
CollectorManager.cpp - kept upstream's comment for the same reason.
node-health.json - kept this branch's 60 panels. Upstream's only change to the
file was job_count to jobq_job_count, which this branch already had.
cfg/xrpld-example.cfg - composed. Kept this branch's warning that
service_instance_id must be set explicitly for the metrics pipeline, took
upstream's traces_endpoint rename, and removed a duplicate metrics_endpoint
entry along with the claim that metrics derive from the traces URL by rewriting
the signal path. Nothing derives it; both metric exporters read
metrics_endpoint. 17 keys, one entry each.
05-configuration-reference.md - both sides misdescribed the parser. Kept this
branch's fuller text, corrected the endpoint default to traces_endpoint, and
replaced the "resolve their URL differently" table with what the code does now.
09-data-collection-reference.md, 06-implementation-phases.md,
Phase7_taskList.md - kept this branch's versions, which drop a metric that was
never implemented, correct the state encoding to 0-6, and rename nudb_bytes to
stored_object_bytes. Re-applied upstream's rpc_requests_total fix, which taking
this side had reverted.
The native-metrics pipeline built its own OTLP/HTTP exporter and set only the
URL, so an operator who enabled TLS got mutual TLS on the trace exporter and a
plaintext-configured exporter for metrics. cfg/xrpld-example.cfg promises TLS for
"the OTLP exporter connection" with no carve-out, and two exporters exist. The
exporter now reads the same four [telemetry] TLS keys the trace exporter does.
Its resource was also thinner than the trace resource: service.name was
hardcoded, and service.version, xrpl.network.id and xrpl.network.type were
absent. Because the collector promotes resource attributes to labels, an operator
setting service_name split their fleet - spans carried the configured name while
every XRPL_METRIC_ series still said xrpld, blanking native-metric panels in
every dashboard that filters on it. All four now come from config, and
xrpl.network.type is derived inside from network_id through the shared
networkTypeFromId so a caller cannot supply a mismatched pair.
start() and initExporterAndProvider() take a StartOptions aggregate rather than
growing to eleven positional parameters, seven of them same-typed strings where a
swap would compile silently and stamp the wrong label - the defect class this
change exists to remove. It is constructed at one production site, so replacing
it with Telemetry::Setup once that is exposed stays a single-site edit.
service.version, service.instance.id and xrpl.node.id are stamped only when
non-empty, keeping this pipeline's existing behaviour of omitting an attribute
rather than writing it blank.
Also corrects three plan-doc references that cited line numbers rather than
symbols; the reader line moved and the numbers differ per branch.
Addresses review findings on the native-metrics work.
StatsDCollector::onTimer drained the send buffer inside the polling_ gate. That
gate holds back hook handlers until the application's services are built, but
sendBuffers() is socket I/O. StatsDEventImpl derives only from EventImpl, so it
never enters metrics_ and posts straight to the buffer; its |ms timings piled up
before onCollectionReady and were dropped after onCollectionStopping. The drain
now runs every tick, and outside metricsLock_, so onCollectionStopping no longer
waits on a UDP flush.
TelemetryImpl's constructor left meterProvider_ set when initMetrics() threw.
initMetrics publishes globally as its last step, so a throw left getMeter()
callers holding a provider nothing else could reach. Reset it in the catch.
~ApplicationImp caught only std::exception around telemetry shutdown while the
callees reach third-party SDK code, so a foreign exception would have terminated
the process. Added a logging catch-all.
ValidationTracker's hard trim evicted by unordered_map bucket order. It now
evicts oldest-first, so the entry dropped under pressure is the one least likely
to still reconcile.
The GetMeter test restored the global meter provider only on the success path,
and ASSERT_TRUE early-returns past it. Uses xrpl::ScopeExit instead.
The hook debounce window is a named constant rather than a bare 500 in a
comparison, and the metric export cadence becomes operator-configurable through
metric_export_interval_ms and metric_export_timeout_ms. Both are range-checked:
the SDK warns and silently substitutes its own 60s/30s defaults when the timeout
is not below the interval, so an unchecked value would slow export rather than
speed it up. Parsing uses a signed representation because lexical_cast<uint32_t>
accepts a leading minus and wraps it.
Naming corrections: CollectorManager documented exported_instance, which no OTel
dashboard uses; node-health queried job_count where the exported name is
jobq_job_count; network-traffic and overlay-traffic-detail referenced an
undeclared DS_PROMETHEUS variable; the counter table omitted the _total suffix
the Prometheus exporter appends; the plan docs and task list carried an xrpld_
prefix formatName never applies; and OTelCollector::New()'s contract promised its
instanceId, serviceName and networkType arguments were read, contradicting the
definition that marks them unused.
clang-tidy reported six bugprone-unchecked-optional-access errors: the dataflow
analysis does not treat ASSERT_TRUE(x.has_value()) as establishing the
precondition, because the assertion's early return is hidden inside a macro. Three
sites now guard with a plain if + FAIL(), the form already used elsewhere in this
file and one the analysis does model.
The fourth site compares the whole optional instead. That covers both "was it
parsed" and "are the bounds right" in one exact assertion, and because it is
EXPECT rather than ASSERT every row of the table is now checked -- previously one
bad row returned from the test and hid the other five.
parseLedgerRange uses std::errc, which comes from <system_error>; the header
included only <charconv>, which misc-include-cleaner flagged.
kMetricExportInterval and kMetricExportTimeout were declared but the reader
options set the same values as literals, so both constants were unused. constexpr
at namespace scope has internal linkage, so clang reports them under
-Wunused-const-variable, which -Dwerr=ON makes fatal: it failed the compile on
ubuntu-clang-release-amd64 and macos-arm64-release, and clang-tidy as well.
Using them removes two magic numbers and keeps the comment that explains why the
interval is 1 s.
Two conflicts, both additions at the same spot:
NetworkOPs.cpp include block kept develop's rpc/detail/SyntheticFields.h
alongside this branch's telemetry/MetricsRegistry.h; .cspell.config.yaml
kept both new words.
NetworkOPs.cpp include block: develop replaced DeliveredAmount.h,
MPTokenIssuanceID.h and NFTSyntheticSerializer.h with the single
rpc/detail/SyntheticFields.h. Kept that and this branch's two telemetry
includes.
Two different facts were sharing one attribute key. The sync-diagnostics
branch already emits validation_status on consensus.validation.accept,
carrying what the validation store did (ValStatus: current, stale,
bad_seq, multiple, conflicting, unknown) with a mapping function and
tests behind it. This branch then added validation_status on
consensus.validation.receive for which exit the receive path took
(queued, dropped_diverged, dropped_load).
One key with two value domains means any aggregation that does not also
filter on span name mixes them. Merging the two branches also produced a
duplicate constexpr declaration in one header, which is how the compiler
surfaced it.
The established accept-path key keeps its name; this one becomes
validation_receive_status, qualified by the span phase it describes. The
values are unchanged.
phase-4 renamed the shared close_time attribute to
close_time_ripple_epoch_s, naming its unit and epoch. Two consumers here
still referenced the old spelling and broke the build once the rename
merged forward: the re-export in LedgerSpanNames.h and the setAttribute
call in BuildLedger.cpp.
Both are phase-6 content, so they are fixed here rather than upstream.
initMetrics() held its own copy of the suffix-swap: take tracesEndpoint,
replace a trailing /v1/traces with /v1/metrics. Any other URL shape sent
metrics to the traces path. metrics_endpoint is now a config key of its
own, so use it as given.
Application::startTelemetry() still reads the key itself for
MetricsRegistry, which builds its own exporter. Telemetry exposes no
accessor for the Setup it parsed, and re-parsing would re-run the mTLS
validation and cert-file checks at a later point in startup, so the
second read stays. The comment above it no longer claims the URL is
derived from the traces one.
Also corrects 05-configuration-reference.md: the sample configs carry
metrics_endpoint where exporter used to sit, not a comment.
