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.
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.
[telemetry] endpoint carried one OTLP signal while its name implied it
covered every signal. That asymmetry is what let the metrics URL be
guessed later by rewriting this one's path suffix, so anything not
ending /v1/traces silently posted metrics to the traces path.
Renames the key to traces_endpoint and Setup::exporterEndpoint to
tracesEndpoint. The default value is unchanged and the URL is still used
verbatim, with no path derived from it. The startup log line and the
compose-file example name the new key, the latter being where an
operator copies it from.
No metrics_endpoint is added here: this branch has no metrics pipeline,
so the key would parse into a member nothing reads.
getInstance(), getTracer(), both startSpan() overloads and networkTypeFromId()
return values that a caller must use. A discarded startSpan() result destroys
the returned span immediately, so the span opens and closes with no content.
Six methods in Telemetry.h already carried the attribute, on the base and on
every override. The new attributes follow that: the overrides in Telemetry.cpp
and NullTelemetry.cpp get it too, because [[nodiscard]] is not inherited and a
call bound to the derived type would otherwise be unchecked.
No caller anywhere in the chain discards any of these results.
The default setServiceInstanceId() body ignores its argument. Use the
attribute rather than a (void) cast: the codebase already uses it 104 times
and the build is C++23.
