Six related defects in the RPC/gRPC span surface, all cases where a failure
was recorded as success or an attribute was missing on an error path.
GRPCServer: the non-exception branch set the span Ok unconditionally, then
sent a possibly-failed grpc::Status. The handler can return a non-OK status
without throwing, so every failed call traced as successful. Status now
follows result.second, with the error message as the span description.
ServerHandler: eight per-item error branches appended an error reply without
recording that the request failed. Batch responses and ripplerpc < 3.0 always
carry HTTP 200, so those failures were invisible and an entirely failed batch
ended its span as successful. Added an appendItemError() helper next to the
existing httpReplyError() lambda and routed all eight sites through it, so the
flag cannot be forgotten at a new call site.
ServerHandler: the early-return validation paths set the span error but not the
rpc_status attribute. Added it to httpReplyError() so every such path gets it.
RPCHandler: the fillHandler error path set only command and rpc_status, while
callMethod sets command, version and rpc_role. Error spans were therefore not
filterable by API version or role. The error path now mirrors that set.
RPCHandler: resolveCommandSpanName() checked only that command/method were
present, not that they agreed, while fillHandler rejects a mismatch as
rpcUNKNOWN_COMMAND. A request supplying both with different values was labelled
with one of the two names, misattributing the error to a command that never
dispatched. It now mirrors fillHandler's rule and collapses to "unknown".
ServerHandler: processRequest returned bool solely so the caller could set its
span status. Telemetry should read state, not shape the signature of the code it
observes, so the signature returns to void and rpc.process sets its own status
from spanHadError. The enclosing rpc.http_request span now leaves status unset:
the OTel spec has instrumentation leave status unset unless the operation itself
errored, and reserves Ok for an operator asserting verified success.
SpanGuard::span() and freshRoot() joined the prefix and suffix by hand with
reserve() + three append() calls. std::format expresses the same join in one
line; libstdc++ has shipped <format> since GCC 13 and the project floor is
GCC 15.2 (BUILD.md), so it is available on every supported compiler.
Both callers are noexcept and std::format can throw (std::bad_alloc, or
std::format_error on a malformed spec), so an escaping exception would
terminate the process. The call is wrapped in a joinSpanName() helper that
catches and returns std::nullopt; the caller then returns a null guard, which
is the same degrade-to-no-op path already taken when telemetry is disabled.
Telemetry must never bring the node down.
One helper rather than a try/catch at each site keeps both call sites a single
line and puts the exception-safety requirement in one documented place.
Addresses a review comment on PR #6437.
Conflict in OpenTelemetryPlan/03-implementation-strategy.md §3.9: both
branches independently fixed the same defect (stale hand-maintained line
counts) in different ways.
- 1b (0ff947454c) stripped the Lines Added / Lines Changed columns but kept
the §3.9.1 and §3.9.2 tables.
- 1a (3ad525a48a) removed both sections outright.
Resolved in favour of 1a's deletion. The tables were pre-implementation
estimates with no source of truth, so trimming the columns only defers the
next drift; the file/component lists duplicated §3.1's directory tree, which
1b already keeps current (DiscardFlag.h, FilteringSpanProcessor). Keeping the
upstream resolution also stops the same conflict recurring on 1c..10.
1b-specific content verified intact after the merge: §3.1 retains the
DiscardFlag.h entry, the FilteringSpanProcessor and discard() annotations,
and the TracingInstrumentation removal. No references to the deleted
sections remain in any plan doc.
The §3.9.1 "Files Modified Summary" and §3.9.2 "Detailed File Impact"
tables carried hand-maintained per-component line counts that had drifted
from the plan: the Lines Added column summed to 1,565 while the Total row
claimed ~1,670. Files (34) and Lines Changed (120) reconciled, so only the
Added total was stale — residue from expanding Core Telemetry 5 -> 11 files.
Rather than patch one cell, remove both tables. They were pre-implementation
estimates with no source of truth, so any figure in them drifts again on the
next phase. §3.1's directory tree remains the canonical list of the 11 new
telemetry files, and §3.9.3-3.9.7 (risk, architectural impact, backward
compatibility, rollback) carry the assessment without inventing line counts.
No references to the removed sections exist elsewhere in the plan docs.
M1: ~PendingTraceId now increments a process-wide atomic counter when a
pinned deterministic trace_id is destroyed unconsumed, so the silent drop
stays observable in release builds where the existing XRPL_ASSERT is a
no-op. Exposed via unconsumedDeterministicIdDrops() for a future metric.
The deterministic trace_id bytes, the GenerateTraceId() consume logic, and
the unconditional reset() are all unchanged.
M2: tighten the Doxygen on SpanGuard::childSpan(std::string_view) to state
it parents to the current ambient context of this store (meaningful only
when a scope/ScopedSpanGuard/ScopedActivation is active) and to point
callers at childSpan(name, ctx) for explicit cross-store parenting.
Doc-only; no behavior change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rpc.command.* becomes a scoped child so it nests under rpc.process, correlates
its log lines, and parents pathfind.request. No RPC::Context plumbing needed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Brings coroutine-aware OTel context storage forward: CoroAwareContextStorage,
its install in Telemetry, ScopedSpanGuard same-store assertion, and the
non-owning ScopedActivation helper.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add SpanGuard::activate() returning a ScopedActivation RAII helper that
activates an already-owned span (from a thread-free SpanGuard) as the
current context WITHOUT taking ownership. The activation pushes the span
onto the current LocalValue context store on construction and pops it on
destruction; it never ends the span (its owning SpanGuard does).
This lets a job-handoff span be made ambient for the duration of a
synchronous, non-yielding worker body so log lines there carry the
span's trace_id. Non-copyable and non-movable, mirroring ScopedSpanGuard.
owner is captured after the Scope push via declaration-order member
initialization (scope declared before owner), matching the A3
capture-after-materialization invariant. A #else no-op stub keeps the
API zero-overhead when telemetry is compiled out.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Coroutine-aware storage lets a scope resume on another worker within the same
coroutine store; store-identity is the correct pop-safety invariant. Same-store
equals same-thread for synchronous code, so no safety is lost.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Install CoroAwareContextStorage as the process-wide OTel runtime context
storage in TelemetryImpl::start(), before SetTracerProvider and before any
span is created, so the ambient context follows JobQueue coroutines across
yield/resume. Hold it in a member for the process lifetime; not reset in
stop() because resetting while spans may exist is SDK undefined behaviour.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Backs the OTel active-context stack with xrpl::LocalValue so the ambient
context follows a JobQueue::Coro across yield/resume. Not yet installed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
