The plan doc offered `"attribute"` as the alternative to `"deterministic"`
for consensus_trace_strategy. The parser accepts `"random"`; "attribute"
described the correlation mechanism rather than the setting's value. Note
also that the alternative is experimental and not used.
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.
childSpan() takes the span name verbatim, so a bare op:: suffix names the span
"process" rather than "rpc.process". The same defect was corrected in the
SpanGuard and Telemetry examples; this is the last copy.
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.
Review feedback on the RPC integration PR.
The childSpan examples could not work as written. childSpan() takes its parent
from the ambient context and uses impl_ only as a liveness gate, so an unscoped
SpanGuard parent produced two siblings rather than a parent and child. The parent
is now a ScopedSpanGuard, the child no longer reuses the parent's name, and the
examples pass a full dotted constant because childSpan() takes the name verbatim.
Five of the ten Rule D tests could not fail. Four passed an empty L1 key set,
which makes the rule skip validation altogether; the fifth asserted an empty
result against an escaped-quote selector that extracted no labels at all. Each
now passes a nonempty L1 set and carries a known-bad label in the same
expression, so it asserts both that the intended labels are accepted and that
Rule D ran. Verified by disabling the rule: the old tests stay green, the new
ones all fail.
Span kind is not fixed here. categoryToSpanKind and the span factories belong to
the telemetry library, so the role parameter is routed to that branch, and the
two call sites here follow once it exists.
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.
Review feedback on the plan documents. Four kinds of error:
- Symbols that do not exist: ConsensusProposal::prevLedger_ (it is
previousLedger_), RCLConsensusAdaptor (it is RCLConsensus::Adaptor, and
startRound() is on RCLConsensus itself), and RPCHandler::doCommand (a free
function, xrpl::rpc::doCommand).
- Attribute keys: the tables used ledger_index, which no telemetry code emits.
Same concept as ledger_seq but a different referent, so the code disambiguates
by prefix: current_ledger_seq for the open ledger a transaction targeted,
ledger_seq for a closed or validated one. A note now states which is which.
- TraceQL that does not parse: span-field predicates need braces, status.code
is not an intrinsic (status = error), and avg(duration) does not take a by
clause (avg_over_time does). All five re-tested against Tempo.
- The StatsD comparison omitted the Histogram instrument, which aggregates at
the point of measure, and the when-to-use table had no row for a metric that
spans cannot afford to carry.
The emitted key is close_time_ripple_epoch_s, which names its unit and
epoch. Update the ledger attribute table to match.
ledger_index and ledger_tx_count in the same table belong to a separate
rename and are left as they are.
Both sides documented the same six [telemetry] keys, so the automatic merge
duplicated all of them. Resolved by keeping this branch's structure - which
already covers all 14 keys and groups them under TLS and batch-processor
headings - and folding in the corrections from the upstream side:
- endpoint is renamed to traces_endpoint, which is what the parser reads, and
described as used verbatim including its signal path.
- use_tls no longer claims to enable TLS. The exporter's URL scheme selects
TLS; this key only decides whether tls_ca_cert reaches it as a CA bundle.
- tls_ca_cert records that the path is not opened while the config is parsed,
so an unreadable file shows up as an export failure rather than at startup.
- service_instance_id explains that it is normally left unset and filled in
from the node public key during startup.
Section::value_or in 05-configuration-reference.md becomes Section::valueOr;
that member does not exist under the other spelling.
The commented [telemetry] block in cfg/xrpld-example.cfg documented 8 of the 14
keys the parser accepts. Add the six that were missing - service_instance_id,
use_tls, tls_ca_cert, batch_size, batch_delay_ms and max_queue_size - each with
the unit and default read from the parser, and rename the documented endpoint
key to traces_endpoint so it matches what makeTelemetrySetup reads.
use_tls is documented for what it does rather than what its name suggests: it
gates whether tls_ca_cert reaches the exporter as a CA bundle, while the scheme
of traces_endpoint is what selects TLS. The path is not opened during parsing,
so an unreadable file surfaces as an export failure at runtime.
05-configuration-reference.md named three symbols that do not exist:
setup_Telemetry, make_Telemetry and Section::value_or. Correct them to
makeTelemetrySetup, makeTelemetry and Section::valueOr.
The [telemetry] option table documents the config key operators copy.
The key is now traces_endpoint, named for the one OTLP signal it
carries, so the old row pointed at a key the parser no longer reads.
Two prose mentions of "endpoint" further down describe the concept
rather than naming the key, and are left alone.
The rename arrived from phase-1b by merge, which left this test naming a
member that no longer exists. Updates both assertions to tracesEndpoint
and the section key to traces_endpoint.
The key matters as much as the member: had only the member been renamed,
the parse would have fallen back to the default and the test would have
compared the collector URL against localhost.
