From 508c91d36caa6e9903553ddcbbe368e245ed60fe Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 21 Jul 2026 18:03:20 +0100 Subject: [PATCH 01/19] refactor(telemetry): split SpanGuard into unscoped SpanGuard + scoped ScopedSpanGuard Separate the two responsibilities the old SpanGuard fused: span ownership (thread-free) and scope activation (thread-bound TLS push). - SpanGuard now owns ONLY the span. Its Impl drops the optional, the owner thread-id, the Detached tag and the scope-less ctor; ~Impl is just `if (span) span->End()`. The guard never binds a thread-local context stack, so it may be moved to and destroyed on any thread. - ScopedSpanGuard is a new pimpl type that wraps a SpanGuard plus an optional. Member order (guard first, scope second) pops the scope before the span ends. It is non-copyable and non-movable; factories return unnamed temporaries so guaranteed copy elision covers `auto s = ScopedSpanGuard::freshRoot(...)`. - `operator SpanGuard() &&` replaces detached()/detachInPlace(): it pops the scope eagerly on the origin thread and yields a thread-free SpanGuard for handoff to a job or another thread. detached(), detachInPlace() (both overloads) and the Detached apparatus are deleted. - Move-assignment is re-enabled on SpanGuard (no Scope to re-bind), in both the real class and the no-op stub. - rootSpan() renamed to freshRoot() in both classes (behavior unchanged: still forces kIsRootSpanKey). - hashSpan behavior is intentionally unchanged (only builds the now unscoped Impl); the true-root IdGenerator fix is a separate later task. Co-Authored-By: Claude Opus 4.8 (1M context) --- include/xrpl/telemetry/SpanGuard.h | 545 +++++++++++++++++++++------- src/libxrpl/telemetry/SpanGuard.cpp | 314 +++++++++++----- 2 files changed, 639 insertions(+), 220 deletions(-) diff --git a/include/xrpl/telemetry/SpanGuard.h b/include/xrpl/telemetry/SpanGuard.h index 88814bfcb5..9ec4733473 100644 --- a/include/xrpl/telemetry/SpanGuard.h +++ b/include/xrpl/telemetry/SpanGuard.h @@ -1,25 +1,38 @@ #pragma once /** - * RAII guard for OpenTelemetry trace spans. + * RAII guards for OpenTelemetry trace spans. * - * Wraps an OTel Span and Scope behind the pimpl idiom so that no - * opentelemetry headers are exposed in this public header. When - * XRPL_ENABLE_TELEMETRY is not defined, SpanGuard is an empty class - * with all-inline no-op methods — zero overhead, zero dependencies. + * This header declares two complementary span guards, split by + * responsibility: * - * Dependency diagram: + * - SpanGuard — owns a span only. Thread-free: it never touches the + * OTel thread-local context stack, so it may be moved + * to and destroyed on any thread. Movable AND + * move-assignable. + * - ScopedSpanGuard — owns a SpanGuard plus an active OTel Scope that + * pushes the span onto the constructing thread's + * context stack. Thread-bound: it must be created and + * destroyed on the same thread. Non-copyable and + * non-movable. + * + * Both wrap all OpenTelemetry types behind the pimpl idiom so no + * opentelemetry headers are exposed here. When XRPL_ENABLE_TELEMETRY + * is not defined, both are empty classes with all-inline no-op methods + * — zero overhead, zero dependencies. + * + * SpanGuard dependency diagram: * * +--------------------------------------------+ * | SpanGuard | + * | (unscoped, thread-free) | * +--------------------------------------------+ * | - impl_ : unique_ptr (pimpl) | * +--------------------------------------------+ * | + span(cat, prefix, name) [static] | - * | + rootSpan(cat, prefix, name) [static] | + * | + freshRoot(cat, prefix, name) [static] | * | + childSpan(name) : SpanGuard | * | + linkedSpan(name) : SpanGuard | - * | + detached() : SpanGuard | * | + captureContext() : SpanContext | * | + setAttribute(key, value) | * | + setOk() / setError(desc) | @@ -29,14 +42,10 @@ * | + operator bool() | * +--------------------------------------------+ * | hides (pimpl) - * +-------+-------------+ - * | | - * +--------+ +---------------------------+ - * | Span | | optional | - * | (OTel) | | (OTel, non-movable) | - * | | | present : scoped guard | - * | | | nullopt : detached guard | - * +--------+ +---------------------------+ + * +--------+ + * | Span | + * | (OTel) | + * +--------+ * * Static factory methods access the global Telemetry instance * internally (via Telemetry::getInstance()), check whether tracing @@ -60,7 +69,7 @@ * TraceCategory::Rpc, rpc_span::prefix::command, commandName); * span.setAttribute(rpc_span::attr::command, commandName); * span.setAttribute(rpc_span::attr::rpcStatus, rpc_span::val::success); - * // span ended automatically on scope exit + * // span ended automatically when the guard is destroyed * @endcode * * 2. Error recording: @@ -110,7 +119,7 @@ * } * @endcode * - * 6. Fresh trace root at an inbound entry point (primary rootSpan use): + * 6. Fresh trace root at an inbound entry point (primary freshRoot use): * @code * #include * using namespace xrpl::telemetry; @@ -119,56 +128,50 @@ * // already have unrelated spans active — start a clean root so * // those do not become parents of this trace. Names come from a * // *SpanNames.h header, never raw literals. - * auto span = SpanGuard::rootSpan( + * auto span = SpanGuard::freshRoot( * TraceCategory::Peer, seg::peer, peer_span::op::validationReceive); * span.setAttribute(peer_span::attr::ledgerHash, hashStr); * @endcode * - * 7. Hand a span to a job on another thread (edge case, detached): + * 7. Hand a span to a job on another thread (thread-free — just move): * @code * #include * using namespace xrpl::telemetry; * - * // Build the guard on THIS thread, then strip its thread-local - * // Scope so it can be safely moved into a job and ended there. + * // SpanGuard owns no thread-local Scope, so it is safe to move + * // into a job and end on the worker thread. No detach step is + * // needed — just move the guard in. * auto span = SpanGuard::span( * TraceCategory::Ledger, seg::ledger, ledger_span::op::build); * jobQueue.addJob( - * [g = std::move(span).detached()]() mutable { + * [g = std::move(span)]() mutable { * doWork(); * // g's span ends when the job's lambda is destroyed, - * // on the worker thread — no origin-stack corruption. + * // on the worker thread. * }); * @endcode * - * @note Thread safety: A SCOPED guard (from span(), rootSpan(), - * childSpan(), linkedSpan()) must only be used on the thread where it - * was constructed — its internal Scope binds to that thread's - * thread-local context stack, and destroying it elsewhere would pop - * the wrong stack. A guard returned by detached() holds no Scope, so - * it may be moved to and destroyed on another thread; detached() - * itself must be called on the origin (constructing) thread. Use - * captureContext() to propagate the trace context to other threads. - * Violating this rule is enforced (not just documented): a scoped - * guard destroyed on a foreign thread, or detached() called from one, - * trips an XRPL_ASSERT in debug/test/fuzzing builds instead of - * silently corrupting the other thread's context stack. + * @note Thread safety: SpanGuard is thread-free. It holds only the + * span (no Scope), so it never binds to a thread-local context stack + * and may be moved to and destroyed on any thread. To make a span the + * ambient parent for child spans on a thread, wrap it in a + * ScopedSpanGuard. Use captureContext() to propagate the trace + * context across threads explicitly. * - * @note Move semantics: Move construction transfers ownership of - * the pimpl pointer — no double-Scope issues. Move assignment is - * deleted to prevent re-scoping mid-flight. + * @note Move semantics: Move construction and move assignment both + * transfer ownership of the pimpl pointer. There is no Scope to + * re-bind, so move assignment is safe and enabled. Copy is deleted. * * @note Known limitations: * - Attributes cannot be removed per the OTel spec; use * setAttribute with an empty value as a convention. - * - SpanGuard::span() (raw Span access) is intentionally not - * exposed — all interaction goes through the public methods. + * - The raw OTel Span is intentionally not exposed — all interaction + * goes through the public methods. */ #include #include #include -#include #include namespace xrpl::telemetry { @@ -225,9 +228,16 @@ public: #ifdef XRPL_ENABLE_TELEMETRY /** - * RAII wrapper that activates a span on construction and ends it on - * destruction. All OTel types are hidden behind the Impl pointer. - * Non-copyable, move-constructible. + * RAII owner of an OTel span (unscoped, thread-free). + * + * Holds only the span behind the Impl pointer — it never pushes the + * span onto the thread-local context stack, so it carries no + * thread-affinity and may be moved to and destroyed on any thread. + * The span is ended when the guard is destroyed (unless discard() + * ended it earlier). Non-copyable; movable and move-assignable. + * + * To activate a span as the ambient parent for child spans on a + * thread, wrap the guard in a ScopedSpanGuard. */ class SpanGuard { @@ -236,6 +246,10 @@ class SpanGuard explicit SpanGuard(std::unique_ptr impl); + // ScopedSpanGuard wraps a SpanGuard and needs the raw span to build + // its Scope; it reads impl_ directly so no OTel type leaks here. + friend class ScopedSpanGuard; + public: /** * Construct a null (no-op) guard. All methods are safe to call. @@ -245,7 +259,7 @@ public: SpanGuard(SpanGuard&& other) noexcept; SpanGuard& - operator=(SpanGuard&&) = delete; + operator=(SpanGuard&&) noexcept; SpanGuard(SpanGuard const&) = delete; SpanGuard& operator=(SpanGuard const&) = delete; @@ -277,16 +291,17 @@ public: * @param prefix Span name prefix (e.g. "peer"). * @param name Span name suffix (e.g. "validation.receive"). * @return An active root-span guard, or a null guard if disabled. - * @note Must be called on the thread that will own the span, like - * span(); the returned guard is scoped to that thread. */ [[nodiscard]] static SpanGuard - rootSpan(TraceCategory cat, std::string_view prefix, std::string_view name); + freshRoot(TraceCategory cat, std::string_view prefix, std::string_view name); // --- Child / linked span creation ---------------------------------- /** - * Create a child span parented to this guard's active context. + * Create a child span parented to the current thread's active + * context. This is meaningful when this guard's span is active on + * the thread (e.g. wrapped in a ScopedSpanGuard); otherwise the + * child inherits whatever context is currently on the stack. * @param name Span name for the child. * @return A new guard, or null if this guard is inactive. */ @@ -321,30 +336,6 @@ public: [[nodiscard]] static SpanGuard linkedSpan(std::string_view name, SpanContext const& linkCtx); - /** - * Detach this guard's span from the current thread's context stack. - * - * A scoped guard holds an OTel Scope bound to the constructing - * thread's context stack. Moving such a guard to another thread - * (e.g. into a job queue) and destroying it there would pop the - * wrong stack, leaving the origin thread's stack corrupted so - * later spans inherit a stale parent. detached() pops the Scope - * now, on the origin thread, and returns a new guard that holds - * the same span with no thread-local binding. - * - * Consumes this guard (rvalue-qualified): after the call this - * guard is null and the returned guard owns the span. - * - * @return A scope-less guard safe to move to and destroy on - * another thread, or a null guard if this guard was null. - * @note Must be called on the origin (constructing) thread; this is - * checked by an XRPL_ASSERT in debug/test/fuzzing builds. The - * returned guard may be freely moved across threads; only - * its final destruction ends the span. - */ - [[nodiscard]] SpanGuard - detached() &&; - // --- Context capture ----------------------------------------------- /** @@ -432,42 +423,260 @@ public: operator bool() const; }; -// --- Detach-in-place helpers ----------------------------------------------- - /** - * Detach an active `std::optional` member in place. + * RAII guard that activates a span on the current thread (scoped). * - * Equivalent to `guard.emplace(std::move(*guard).detached())`, which is the - * required idiom for detaching a live guard stored in an `optional` (move - * assignment is deleted because the underlying Scope cannot be re-scoped in - * place). No-op if `guard` is empty or already null. + * Wraps a SpanGuard (which owns the span) plus an OTel Scope that + * pushes the span onto this thread's thread-local context stack for + * the guard's lifetime, so child spans created on the thread inherit + * it as parent. On destruction the Scope pops BEFORE the span ends + * (member order: guard first, scope second). Non-copyable and + * non-movable — factories return unnamed temporaries, so guaranteed + * copy elision (C++17) covers `auto s = ScopedSpanGuard::freshRoot(...)`. * - * @param guard The optional guard to detach. Must be called on the thread - * that constructed the active guard inside it (same rule as - * `SpanGuard::detached()`). - * @note Must run BEFORE any `captureContext()` snapshot is taken if the - * caller also needs the pre-detach context — capture first, then call - * this helper (same ordering rule `detached()` itself has). + * When the span must outlive the current thread (e.g. handed to a job + * queue), convert to a plain SpanGuard with `operator SpanGuard() &&`: + * that pops the Scope eagerly on the origin thread and yields a + * thread-free guard. + * + * ScopedSpanGuard dependency diagram: + * + * +--------------------------------------------+ + * | ScopedSpanGuard | + * | (scoped, thread-bound) | + * +--------------------------------------------+ + * | - impl_ : unique_ptr (pimpl) | + * +--------------------------------------------+ + * | + (cat, prefix, name) [ctor] | + * | + freshRoot(cat, prefix, name) [static] | + * | + childSpan(name) : ScopedSpanGuard | + * | + linkedSpan(name) : ScopedSpanGuard | + * | + operator SpanGuard() && (handoff) | + * | + setAttribute / setOk / setError / ... | + * | + captureContext() / discard() | + * | + operator bool() | + * +--------------------------------------------+ + * | hides (pimpl) + * +------+--------------------+ + * | | + * +----------+ +-----------------------------+ + * | SpanGuard| | optional | + * | (span) | | (OTel, thread-bound; | + * | | | present : span active) | + * +----------+ +-----------------------------+ + * + * Usage examples: + * + * 1. Same-thread scoped tracing (child inherits parent automatically): + * @code + * using namespace xrpl::telemetry; + * + * ScopedSpanGuard span( + * TraceCategory::Rpc, rpc_span::prefix::command, commandName); + * span.setAttribute(rpc_span::attr::command, commandName); + * // childSpan parents to `span` because it is active on this thread + * auto child = span.childSpan(rpc_span::op::dispatch); + * @endcode + * + * 2. Capture on this thread, hand off to another (edge case): + * @code + * using namespace xrpl::telemetry; + * + * auto scoped = ScopedSpanGuard::freshRoot( + * TraceCategory::Ledger, seg::ledger, ledger_span::op::build); + * // Pop the scope now, on this thread, and move a thread-free + * // SpanGuard into the job. + * SpanGuard span = std::move(scoped); + * jobQueue.addJob([g = std::move(span)]() mutable { doWork(); }); + * @endcode + * + * @note Thread safety: A ScopedSpanGuard must be constructed AND + * destroyed on the same thread — its Scope binds to that thread's + * context stack. `operator SpanGuard() &&` and the destructor must + * both run on the owning thread; both check this with an XRPL_ASSERT + * in debug/test/fuzzing builds. `operator SpanGuard() &&` pops the + * scope eagerly, so the resulting SpanGuard is thread-free. + * + * @note Known limitations: + * - Non-movable: cannot be stored in a container that relocates, and + * cannot be returned then move-assigned into an existing variable. + * Store the thread-free SpanGuard (via `operator SpanGuard() &&`) + * instead when a movable handle is needed. + * - Same attribute-removal limitation as SpanGuard. */ -void -detachInPlace(std::optional& guard); +class ScopedSpanGuard +{ + struct ScopedImpl; + std::unique_ptr impl_; -/** - * Detach an active `std::shared_ptr`, returning the detached - * guard as a new `shared_ptr`. - * - * Equivalent to - * `guard = std::make_shared(std::move(*guard).detached())`. - * No-op (returns the input unchanged) if `guard` is null or points at a - * null guard. - * - * @param guard The guard to detach, taken by value (the caller's pointer - * is consumed; assign the return value back). - * @return A `shared_ptr` to the detached guard (new allocation), or the - * input pointer unchanged if it was null/inactive. - */ -[[nodiscard]] std::shared_ptr -detachInPlace(std::shared_ptr guard); + explicit ScopedSpanGuard(SpanGuard&& guard); + +public: + /** + * Create a scoped span guarded by a TraceCategory flag and activate + * it on this thread. Equivalent to SpanGuard::span() wrapped in an + * active Scope. The span name is built as "prefix.name". + * @param cat Trace subsystem category. + * @param prefix Span name prefix (e.g. "rpc.command"). + * @param name Span name suffix (e.g. "submit"). + */ + ScopedSpanGuard(TraceCategory cat, std::string_view prefix, std::string_view name); + + ~ScopedSpanGuard(); + + ScopedSpanGuard(ScopedSpanGuard&&) = delete; + ScopedSpanGuard& + operator=(ScopedSpanGuard&&) = delete; + ScopedSpanGuard(ScopedSpanGuard const&) = delete; + ScopedSpanGuard& + operator=(ScopedSpanGuard const&) = delete; + + // --- Static factory methods ---------------------------------------- + + /** + * Create a scoped span that always starts a fresh trace root and + * activate it on this thread. See SpanGuard::freshRoot(). + * @param cat Trace subsystem category. + * @param prefix Span name prefix. + * @param name Span name suffix. + * @return An active scoped root-span guard, or a null one if disabled. + */ + [[nodiscard]] static ScopedSpanGuard + freshRoot(TraceCategory cat, std::string_view prefix, std::string_view name); + + // --- Child / linked span creation ---------------------------------- + + /** + * Create a scoped child span parented to this guard's active span + * and activate it on this thread. + * @param name Span name for the child. + * @return A new scoped guard, or a null one if this guard is inactive. + */ + [[nodiscard]] ScopedSpanGuard + childSpan(std::string_view name) const; + + /** + * Create a scoped child span parented to an explicit captured + * context and activate it on this thread. + * @param name Span name for the child. + * @param parentCtx Context captured via captureContext(). + * @return A new scoped guard, or a null one if parentCtx is invalid. + */ + [[nodiscard]] static ScopedSpanGuard + childSpan(std::string_view name, SpanContext const& parentCtx); + + /** + * Create a scoped span linked (follows-from) to this guard's span + * and activate it on this thread. + * @param name Span name for the linked span. + * @return A new scoped guard, or a null one if this guard is inactive. + */ + [[nodiscard]] ScopedSpanGuard + linkedSpan(std::string_view name) const; + + /** + * Create a scoped span linked to an explicit captured context and + * activate it on this thread. + * @param name Span name for the linked span. + * @param linkCtx Context to link from. + * @return A new scoped guard, or a null one if linkCtx is invalid. + */ + [[nodiscard]] static ScopedSpanGuard + linkedSpan(std::string_view name, SpanContext const& linkCtx); + + // --- Handoff bridge ------------------------------------------------- + + /** + * Pop the active Scope on the origin thread and yield the span as a + * thread-free SpanGuard. Use to move a span into a job or onto + * another thread. Must be called on the constructing thread. + * @return The unscoped SpanGuard that now owns the span. + */ + operator SpanGuard() &&; + + // --- Context capture ----------------------------------------------- + + /** + * Snapshot the current thread's OTel context for cross-thread use. + * @return An opaque SpanContext, or an invalid one if null guard. + */ + [[nodiscard]] SpanContext + captureContext() const; + + // --- Forwarding methods (delegate to the owned SpanGuard) ---------- + + /** + * Set a string attribute. No-op on a null guard. + */ + void + setAttribute(std::string_view key, std::string_view value); + + /** + * Set a string attribute (C-string overload). No-op on a null guard. + */ + void + setAttribute(std::string_view key, char const* value); + + /** + * Set an integer attribute. No-op on a null guard. + */ + void + setAttribute(std::string_view key, std::int64_t value); + + /** + * Set a floating-point attribute. No-op on a null guard. + */ + void + setAttribute(std::string_view key, double value); + + /** + * Set a boolean attribute. No-op on a null guard. + */ + void + setAttribute(std::string_view key, bool value); + + /** + * Mark the span status as OK. No-op on a null guard. + */ + void + setOk(); + + /** + * Mark the span status as error. No-op on a null guard. + * @param description Optional human-readable error description. + */ + void + setError(std::string_view description = ""); + + /** + * Add a named event to the span's timeline. No-op on a null guard. + * @param name Event name. + */ + void + addEvent(std::string_view name); + + /** + * Record an exception as a span event and mark status as error. + * No-op on a null guard. + * @param e The exception to record. + */ + void + recordException(std::exception const& e); + + /** + * Mark this span for discard and end it immediately. discard() pops the + * thread-local scope right away (on the owning thread) and discards the + * span. After discard() the guard is inert and its destructor is a no-op. + */ + void + discard(); + + /** + * @return true if this guard holds an active span. + */ + explicit + operator bool() const; +}; // --------------------------------------------------------------------------- // No-op stub (all inline, zero overhead, no OTel dependency) @@ -481,7 +690,7 @@ public: ~SpanGuard() = default; SpanGuard(SpanGuard&&) noexcept = default; SpanGuard& - operator=(SpanGuard&&) = delete; + operator=(SpanGuard&&) noexcept = default; SpanGuard(SpanGuard const&) = delete; SpanGuard& operator=(SpanGuard const&) = delete; @@ -493,7 +702,7 @@ public: } [[nodiscard]] static SpanGuard - rootSpan(TraceCategory, std::string_view, std::string_view) + freshRoot(TraceCategory, std::string_view, std::string_view) { return {}; } @@ -520,12 +729,6 @@ public: return {}; } - [[nodiscard]] SpanGuard - detached() && - { - return {}; - } - [[nodiscard]] SpanContext captureContext() const { @@ -582,18 +785,114 @@ public: } }; -// --- Detach-in-place helpers (no-op stubs) --------------------------------- - -inline void -detachInPlace(std::optional&) +class ScopedSpanGuard { -} + // Private default ctor lets the inline factories/child methods build + // a no-op instance without exposing a public default constructor + // (mirrors the real class, which has no public default ctor). + ScopedSpanGuard() = default; -[[nodiscard]] inline std::shared_ptr -detachInPlace(std::shared_ptr guard) -{ - return guard; -} +public: + ScopedSpanGuard(TraceCategory, std::string_view, std::string_view) + { + } + ~ScopedSpanGuard() = default; + + ScopedSpanGuard(ScopedSpanGuard&&) = delete; + ScopedSpanGuard& + operator=(ScopedSpanGuard&&) = delete; + ScopedSpanGuard(ScopedSpanGuard const&) = delete; + ScopedSpanGuard& + operator=(ScopedSpanGuard const&) = delete; + + [[nodiscard]] static ScopedSpanGuard + freshRoot(TraceCategory, std::string_view, std::string_view) + { + return {}; + } + + // NOLINTBEGIN(readability-convert-member-functions-to-static) + [[nodiscard]] ScopedSpanGuard + childSpan(std::string_view) const + { + return {}; + } + [[nodiscard]] static ScopedSpanGuard + childSpan(std::string_view, SpanContext const&) + { + return {}; + } + [[nodiscard]] ScopedSpanGuard + linkedSpan(std::string_view) const + { + return {}; + } + [[nodiscard]] static ScopedSpanGuard + linkedSpan(std::string_view, SpanContext const&) + { + return {}; + } + + [[nodiscard]] SpanContext + captureContext() const + { + return {}; + } + // NOLINTEND(readability-convert-member-functions-to-static) + + operator SpanGuard() && + { + return {}; + } + + void + setAttribute(std::string_view, std::string_view) + { + } + void + setAttribute(std::string_view, char const*) + { + } + void + setAttribute(std::string_view, std::int64_t) + { + } + void + setAttribute(std::string_view, double) + { + } + void + setAttribute(std::string_view, bool) + { + } + + void + setOk() + { + } + void + setError(std::string_view = "") + { + } + void + addEvent(std::string_view) + { + } + void + recordException(std::exception const&) + { + } + void + discard() + { + } + + explicit + operator bool() const + { + return false; + } +}; #endif // XRPL_ENABLE_TELEMETRY diff --git a/src/libxrpl/telemetry/SpanGuard.cpp b/src/libxrpl/telemetry/SpanGuard.cpp index 3b920e51b3..825402ff63 100644 --- a/src/libxrpl/telemetry/SpanGuard.cpp +++ b/src/libxrpl/telemetry/SpanGuard.cpp @@ -1,23 +1,26 @@ /** - * Pimpl implementation for SpanGuard and SpanContext. + * Pimpl implementation for SpanGuard, ScopedSpanGuard and SpanContext. * * All OpenTelemetry SDK types are confined to this translation unit. * The public SpanGuard.h header contains only standard-library types - * and forward-declares the Impl struct. + * and forward-declares the Impl / ScopedImpl structs. + * + * Two guards with split responsibilities live here: + * - SpanGuard::Impl holds ONLY the OTel Span (shared_ptr). It never + * touches the thread-local context stack, so a SpanGuard is + * thread-free and may be moved to and destroyed on any thread. + * - ScopedSpanGuard::ScopedImpl holds a SpanGuard plus an optional + * Scope. The Scope pushes the span onto the constructing thread's + * context stack; member order (guard first, scope second) ensures + * the Scope pops BEFORE the span ends on destruction. It is + * thread-bound: construct and destroy on the same thread. * * Static factory methods access the global Telemetry instance via * Telemetry::getInstance(), check whether the requested TraceCategory - * is enabled, and return either an active guard with a real Span+Scope - * or a null guard whose methods are all no-ops. + * is enabled, and return either an active guard with a real Span or a + * null guard whose methods are all no-ops. * - * The Impl struct holds the OTel Span (shared_ptr) and an optional - * Scope. Scope is non-movable, but since Impl lives behind a - * unique_ptr, SpanGuard's move constructor simply transfers the - * pointer — no double-Scope issues. A scoped guard holds the Scope; - * detached() produces an Impl with no Scope (nullopt) so the guard - * carries no thread-local binding and is safe to move across threads. - * - * @see SpanGuard (SpanGuard.h), Telemetry (Telemetry.h), + * @see SpanGuard, ScopedSpanGuard (SpanGuard.h), Telemetry (Telemetry.h), * FilteringSpanProcessor (Telemetry.cpp) */ @@ -79,65 +82,22 @@ SpanContext::isValid() const struct SpanGuard::Impl { /** - * The OTel span being guarded. Set to nullptr after discard() or - * once detached() moves it into a new guard, so ~Impl skips End(). + * The OTel span being guarded. Set to nullptr after discard() so + * ~Impl skips End(). */ opentelemetry::nostd::shared_ptr span; /** - * Scope that activates span on the current thread's context stack. - * nullopt for a detached guard, which holds the span with no - * thread-local binding and is therefore safe to move across threads. - */ - std::optional scope; - - /** - * Thread that constructed this Impl. Meaningful only when `scope` - * holds a value: a Scope is bound to the thread-local context stack - * of the thread that pushed it, so popping it (via ~Scope, when - * ~Impl runs) on a different thread corrupts that thread's stack. - * Checked in ~Impl() and detached() to turn a silent cross-thread - * corruption into an assertion failure in debug/test/fuzzing builds. - */ - std::thread::id owner{std::this_thread::get_id()}; - - /** - * Construct a scoped guard: the span is pushed onto this thread's - * active-context stack for the lifetime of the guard. + * Construct an unscoped guard: the span is owned but NOT pushed + * onto any thread's context stack. The guard is thread-free. * @param s The span to guard. */ explicit Impl(opentelemetry::nostd::shared_ptr s) : span(std::move(s)) - { - scope.emplace(span); - } - - /** - * Tag type selecting the scope-less (detached) constructor. - */ - struct Detached - { - }; - - /** - * Construct a detached guard: the span is held with NO thread-local - * Scope, so the guard carries no context-stack binding and is safe - * to move to and destroy on another thread. - * @param s The span to guard. - */ - Impl(opentelemetry::nostd::shared_ptr s, Detached) : span(std::move(s)) { } ~Impl() { - // A live Scope (scope engaged) must be popped on the thread that - // pushed it; ending on any other thread corrupts that thread's - // context stack. A detached guard (scope == nullopt) carries no - // binding and may be destroyed anywhere, so the check is skipped. - XRPL_ASSERT( - !scope.has_value() || owner == std::this_thread::get_id(), - "xrpl::telemetry::SpanGuard::Impl::~Impl : scoped guard destroyed on " - "constructing thread"); if (span) span->End(); } @@ -155,6 +115,8 @@ struct SpanGuard::Impl SpanGuard::SpanGuard() = default; SpanGuard::~SpanGuard() = default; SpanGuard::SpanGuard(SpanGuard&&) noexcept = default; +SpanGuard& +SpanGuard::operator=(SpanGuard&&) noexcept = default; SpanGuard::SpanGuard(std::unique_ptr impl) : impl_(std::move(impl)) { @@ -237,7 +199,7 @@ SpanGuard::span(TraceCategory cat, std::string_view prefix, std::string_view nam } SpanGuard -SpanGuard::rootSpan(TraceCategory cat, std::string_view prefix, std::string_view name) +SpanGuard::freshRoot(TraceCategory cat, std::string_view prefix, std::string_view name) { auto* tel = Telemetry::getInstance(); if ((tel == nullptr) || !tel->isEnabled() || !isCategoryEnabled(*tel, cat)) @@ -327,43 +289,6 @@ SpanGuard::linkedSpan(std::string_view name, SpanContext const& linkCtx) opts))); } -SpanGuard -SpanGuard::detached() && -{ - if (!impl_) - return {}; - // Popping the Scope (below, via impl_.reset()) must happen on the thread - // that pushed it; calling detached() elsewhere would pop the wrong stack. - XRPL_ASSERT( - !impl_->scope.has_value() || impl_->owner == std::this_thread::get_id(), - "xrpl::telemetry::SpanGuard::detached : called on constructing thread"); - // Take the span out; the old Impl.span is now null so ~Impl won't End(). - auto s = std::move(impl_->span); - // Resetting the old Impl destroys its Scope HERE, on the origin thread, - // popping this thread's context stack correctly. The returned guard holds - // the span with no Scope, so it is safe to move to another thread. - impl_.reset(); - return SpanGuard(std::make_unique(std::move(s), Impl::Detached{})); -} - -// ===== Detach-in-place helpers ============================================= - -void -detachInPlace(std::optional& guard) -{ - if (!guard || !*guard) - return; - guard.emplace(std::move(*guard).detached()); -} - -std::shared_ptr -detachInPlace(std::shared_ptr guard) -{ - if (!guard || !*guard) - return guard; - return std::make_shared(std::move(*guard).detached()); -} - // ===== Context capture ===================================================== SpanContext @@ -476,6 +401,201 @@ SpanGuard::discard() } } +// ===== ScopedSpanGuard::ScopedImpl ========================================= + +struct ScopedSpanGuard::ScopedImpl +{ + /** + * The unscoped guard that owns the span. Declared FIRST so it is + * destroyed AFTER `scope`: the Scope must pop before the span ends. + */ + SpanGuard guard; + + /** + * Active OTel Scope binding the span to this thread's context stack. + * Present while the guard is active; reset() (via operator SpanGuard) + * pops it eagerly on the origin thread. Declared AFTER `guard` so + * destruction order pops the scope before the span ends. + */ + std::optional scope; + + /** + * Thread that constructed this ScopedImpl. The Scope is bound to + * this thread's context stack, so popping it (via ~Scope or reset()) + * on a different thread corrupts that thread's stack. Checked in the + * destructor and operator SpanGuard() to turn a silent cross-thread + * corruption into an assertion failure in debug/test/fuzzing builds. + */ + std::thread::id owner{std::this_thread::get_id()}; + + /** + * Wrap a SpanGuard and activate its span on this thread. If the + * guard is active, push its span onto the thread-local context + * stack; a null guard leaves `scope` empty. + * @param g The span-owning guard to activate. + */ + explicit ScopedImpl(SpanGuard g) : guard(std::move(g)) + { + if (guard) + scope.emplace(guard.impl_->span); + } +}; + +// ===== ScopedSpanGuard core lifecycle ====================================== + +ScopedSpanGuard::ScopedSpanGuard(SpanGuard&& guard) + : impl_(std::make_unique(std::move(guard))) +{ +} + +ScopedSpanGuard::~ScopedSpanGuard() +{ + // A live Scope must be popped on the thread that pushed it; destroying + // the guard on any other thread corrupts that thread's context stack. + // A null guard holds no Scope, so the check is skipped. + XRPL_ASSERT( + !impl_ || !impl_->scope.has_value() || impl_->owner == std::this_thread::get_id(), + "xrpl::telemetry::ScopedSpanGuard::~ScopedSpanGuard : destroyed on " + "constructing thread"); +} + +// ===== ScopedSpanGuard factory methods ===================================== + +ScopedSpanGuard::ScopedSpanGuard(TraceCategory cat, std::string_view prefix, std::string_view name) + : ScopedSpanGuard(SpanGuard::span(cat, prefix, name)) +{ +} + +ScopedSpanGuard +ScopedSpanGuard::freshRoot(TraceCategory cat, std::string_view prefix, std::string_view name) +{ + return ScopedSpanGuard(SpanGuard::freshRoot(cat, prefix, name)); +} + +ScopedSpanGuard +ScopedSpanGuard::childSpan(std::string_view name) const +{ + return ScopedSpanGuard(impl_->guard.childSpan(name)); +} + +ScopedSpanGuard +ScopedSpanGuard::childSpan(std::string_view name, SpanContext const& parentCtx) +{ + return ScopedSpanGuard(SpanGuard::childSpan(name, parentCtx)); +} + +ScopedSpanGuard +ScopedSpanGuard::linkedSpan(std::string_view name) const +{ + return ScopedSpanGuard(impl_->guard.linkedSpan(name)); +} + +ScopedSpanGuard +ScopedSpanGuard::linkedSpan(std::string_view name, SpanContext const& linkCtx) +{ + return ScopedSpanGuard(SpanGuard::linkedSpan(name, linkCtx)); +} + +// ===== ScopedSpanGuard handoff bridge ====================================== + +ScopedSpanGuard:: +operator SpanGuard() && +{ + // The scope must be popped on the thread that pushed it; handing off + // elsewhere would pop the wrong stack. + XRPL_ASSERT( + impl_->owner == std::this_thread::get_id(), + "xrpl::telemetry::ScopedSpanGuard::operator SpanGuard : handoff on " + "constructing thread"); + impl_->scope.reset(); // eager pop, on the origin thread + return std::move(impl_->guard); +} + +// ===== ScopedSpanGuard context capture ===================================== + +SpanContext +ScopedSpanGuard::captureContext() const +{ + return impl_->guard.captureContext(); +} + +// ===== ScopedSpanGuard forwarding methods ================================== + +void +ScopedSpanGuard::setAttribute(std::string_view key, std::string_view value) +{ + impl_->guard.setAttribute(key, value); +} + +void +ScopedSpanGuard::setAttribute(std::string_view key, char const* value) +{ + impl_->guard.setAttribute(key, value); +} + +void +ScopedSpanGuard::setAttribute(std::string_view key, std::int64_t value) +{ + impl_->guard.setAttribute(key, value); +} + +void +ScopedSpanGuard::setAttribute(std::string_view key, double value) +{ + impl_->guard.setAttribute(key, value); +} + +void +ScopedSpanGuard::setAttribute(std::string_view key, bool value) +{ + impl_->guard.setAttribute(key, value); +} + +void +ScopedSpanGuard::setOk() +{ + impl_->guard.setOk(); +} + +void +ScopedSpanGuard::setError(std::string_view description) +{ + impl_->guard.setError(description); +} + +void +ScopedSpanGuard::addEvent(std::string_view name) +{ + impl_->guard.addEvent(name); +} + +void +ScopedSpanGuard::recordException(std::exception const& e) +{ + impl_->guard.recordException(e); +} + +void +ScopedSpanGuard::discard() +{ + // A live Scope must be popped on the thread that pushed it; discarding + // on any other thread corrupts that thread's context stack. A null guard + // holds no Scope, so the check is skipped. + XRPL_ASSERT( + !impl_ || !impl_->scope.has_value() || impl_->owner == std::this_thread::get_id(), + "xrpl::telemetry::ScopedSpanGuard::discard : discarded on constructing thread"); + // Pop the scope first (on this thread) so no span stays active on the + // stack, then discard the span via the owned guard. + impl_->scope.reset(); + impl_->guard.discard(); +} + +ScopedSpanGuard:: +operator bool() const +{ + return impl_ && static_cast(impl_->guard); +} + } // namespace xrpl::telemetry #endif // XRPL_ENABLE_TELEMETRY From 4d95f455f4a7efeb95a42ada1b257e0e162c2399 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 21 Jul 2026 18:35:00 +0100 Subject: [PATCH 02/19] feat(telemetry): add DeterministicIdGenerator + PendingTraceId for true-root deterministic trace_ids Add a custom OTel IdGenerator that returns a thread-local pending trace_id on the SDK no-parent (root) branch and a random one otherwise, plus a PendingTraceId RAII guard that pins that id for the next forced-root span and asserts on destruction that it was consumed. Wire the generator into TracerProviderFactory::Create via its 4-arg overload. This lets hash-derived spans become true trace roots so they line up into one trace across nodes. It is installed but dormant on this branch: the caller (hashSpan) arrives on a later branch (phase-3). GenerateSpanId is always random and is_random_ is false so the W3C random-trace-id flag is not set on deterministic ids. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../xrpl/telemetry/DeterministicIdGenerator.h | 170 ++++++++++++++++++ .../telemetry/DeterministicIdGenerator.cpp | 92 ++++++++++ src/libxrpl/telemetry/Telemetry.cpp | 11 +- 3 files changed, 271 insertions(+), 2 deletions(-) create mode 100644 include/xrpl/telemetry/DeterministicIdGenerator.h create mode 100644 src/libxrpl/telemetry/DeterministicIdGenerator.cpp diff --git a/include/xrpl/telemetry/DeterministicIdGenerator.h b/include/xrpl/telemetry/DeterministicIdGenerator.h new file mode 100644 index 0000000000..6e83054797 --- /dev/null +++ b/include/xrpl/telemetry/DeterministicIdGenerator.h @@ -0,0 +1,170 @@ +#pragma once + +#ifdef XRPL_ENABLE_TELEMETRY + +#include + +#include +#include +#include + +namespace xrpl::telemetry { + +/** + * OTel IdGenerator that can mint a deterministic (hash-derived) trace_id. + * + * By default the OTel SDK generates random trace_ids, so a span derived from + * a stable hash (e.g. a transaction id) cannot become a real trace root: the + * SDK either inherits the active span as parent or invents a random root id. + * This generator lets a caller pin the trace_id of the next forced-root span + * to a chosen 16-byte value, so hash-derived spans line up across nodes into + * one trace. When no value is pinned it behaves exactly like the default + * random generator. + * + * The pinned value lives in a thread-local slot set by PendingTraceId (below). + * Only GenerateTraceId() consults that slot, and only on the SDK's no-parent + * (root) branch; span_ids are always random. is_random_ is false so the W3C + * random-trace-id flag is not set on these deterministic ids. + * + * Dependency / data-flow diagram: + * + * +-----------------------------------------------------------+ + * | DeterministicIdGenerator | + * | (IdGenerator) | + * +-----------------------------------------------------------+ + * | - random_ : unique_ptr (random delegate) | + * +-----------------------------------------------------------+ + * | GenerateTraceId(): | + * | thread-local pending id set? --yes--> return that id | + * | --no --> random_ ... | + * | GenerateSpanId(): always ---------------> random_ ... | + * +-----------------------------------------------------------+ + * ^ | + * sets/clears| delegates| + * | v + * +----------------+ +--------------------+ + * | PendingTraceId | | RandomIdGenerator | + * | (RAII guard) | | (delegate) | + * +----------------+ +--------------------+ + * + * @note Thread safety: the pending trace_id is a file-local thread_local, so + * each thread sees only its own pinned value and no synchronization is needed. + * The pending id is consumed ONLY by GenerateTraceId(), which the SDK calls + * solely when a span has no valid parent; GenerateSpanId() never reads it and + * is always random. + * @note Limitation: minting a deterministic root only works when the caller + * forces the SDK's root branch (e.g. by starting the span with a + * Context{kIsRootSpanKey, true}); otherwise the SDK reuses the parent's + * trace_id and never calls GenerateTraceId(). That caller (hashSpan) lands on + * a later branch, so this generator is installed but dormant here. + * + * Example 1 - primary use, inside a forced-root hash span (arrives on a later + * branch; shown as pseudocode): + * @code + * // std::array id = deriveTraceIdFromHash(txHash); + * // PendingTraceId const pending{id}; // pin id for this thread + * // auto root = Context{kIsRootSpanKey, true}; // force the no-parent branch + * // auto guard = telemetry.startSpan("tx.process", root); + * // // GenerateTraceId() returns `id`; the guard's trace_id == id. + * @endcode + * + * Example 2 - edge case, a normal child span never consults the pending id: + * @code + * // With an active parent span, startSpan() inherits the parent's trace_id + * // and the SDK does NOT call GenerateTraceId(), so no PendingTraceId is used. + * // auto child = parentGuard.childSpan("subtask"); // random/parent trace_id + * @endcode + */ +class DeterministicIdGenerator final : public opentelemetry::sdk::trace::IdGenerator +{ + /** + * Random generator the deterministic path falls back to. Used for every + * span_id and for any trace_id when no PendingTraceId is active. + */ + std::unique_ptr random_; + +public: + /** + * Build a generator with is_random_ = false and a random delegate. + */ + DeterministicIdGenerator(); + + /** + * @return the thread's pending trace_id if a PendingTraceId is active, + * otherwise a fresh random trace_id from the delegate. Consuming the + * pending id clears it so it applies to exactly one root span. + */ + opentelemetry::trace::TraceId + GenerateTraceId() noexcept override; + + /** + * @return a fresh random span_id. Never uses the pending trace_id. + */ + opentelemetry::trace::SpanId + GenerateSpanId() noexcept override; +}; + +/** + * RAII guard that pins a deterministic trace_id for the next forced-root span + * started on this thread. + * + * Mirrors DiscardScope: it sets a thread-local pending trace_id on + * construction and clears it on destruction, so the pinned id stays confined + * to the guard's scope and cannot leak onto a later span. On destruction it + * asserts that the id was actually consumed by GenerateTraceId() — if it was + * not, the SDK took a branch other than the root branch the caller intended, + * which is a bug worth catching in debug/test builds. + * + * Wrap ONLY the single forced-root startSpan() call that must receive the + * deterministic id. Non-copyable and non-movable: its sole purpose is the + * scoped lifetime of the pending id. + * + * @note Thread safety: the pending id is thread-local, so a guard on one + * thread never affects another. Construct and destroy the guard on the same + * thread as the startSpan() call it wraps. + * @note Limitation: the consumed-assert only holds when the wrapped span is + * started on the SDK root branch (Context{kIsRootSpanKey, true}); wrapping a + * child span would leave the id unconsumed and trip the assert. Nesting two + * guards on one thread is unsupported: the inner guard consumes/clears first, + * so the outer id is silently dropped and its destructor trips the assert. + * + * Example 1 - primary use, wrapping a forced-root span start (pseudocode; the + * caller lands on a later branch): + * @code + * // { + * // PendingTraceId const pending{id}; // pin for this scope + * // auto root = Context{kIsRootSpanKey, true}; + * // auto guard = telemetry.startSpan(name, root);// consumes the pinned id + * // } // ~PendingTraceId asserts the id was consumed, then clears it + * @endcode + * + * Example 2 - edge case, misuse detection: wrapping a non-root span leaves the + * id unconsumed, so ~PendingTraceId trips XRPL_ASSERT in debug/test builds. + */ +class PendingTraceId +{ +public: + /** + * Pin @p id as the pending trace_id for this thread. + * @param id The 16-byte trace_id the next forced-root span should adopt. + */ + explicit PendingTraceId(std::array const& id) noexcept; + + /** + * Assert the pinned id was consumed by a root span, then clear it so it + * never leaks onto the next span (even in release, where the assert is a + * no-op). + */ + ~PendingTraceId() noexcept; + + PendingTraceId(PendingTraceId const&) = delete; + PendingTraceId& + operator=(PendingTraceId const&) = delete; + PendingTraceId(PendingTraceId&&) = delete; + PendingTraceId& + operator=(PendingTraceId&&) = delete; +}; + +} // namespace xrpl::telemetry + +#endif // XRPL_ENABLE_TELEMETRY diff --git a/src/libxrpl/telemetry/DeterministicIdGenerator.cpp b/src/libxrpl/telemetry/DeterministicIdGenerator.cpp new file mode 100644 index 0000000000..2a2c8c65ec --- /dev/null +++ b/src/libxrpl/telemetry/DeterministicIdGenerator.cpp @@ -0,0 +1,92 @@ +/** + * Implementation of DeterministicIdGenerator and PendingTraceId. + * + * The pending trace_id lives in file-local thread_locals shared between the + * generator and the RAII guard. GenerateTraceId() consumes the pending id on + * the SDK's no-parent branch; PendingTraceId sets it and asserts consumption. + * All OpenTelemetry SDK types stay confined to telemetry translation units. + * + * @see DeterministicIdGenerator, PendingTraceId (DeterministicIdGenerator.h) + */ + +#ifdef XRPL_ENABLE_TELEMETRY + +#include + +#include + +#include +#include +#include +#include + +#include +#include +#include + +namespace xrpl::telemetry { + +namespace { + +/** + * Trace_id pinned by PendingTraceId, consumed by GenerateTraceId(). File-local + * and thread-local, so it is set and read on the same thread with no locking. + */ +thread_local std::optional> tlsPendingTraceId; + +/** + * True once GenerateTraceId() has consumed the pending id. ~PendingTraceId + * asserts on it to catch a forced-root span that never reached the SDK root + * branch. + */ +thread_local bool tlsPendingConsumed = false; + +} // namespace + +DeterministicIdGenerator::DeterministicIdGenerator() + : opentelemetry::sdk::trace::IdGenerator(/*is_random=*/false) + , random_(opentelemetry::sdk::trace::RandomIdGeneratorFactory::Create()) +{ +} + +opentelemetry::trace::TraceId +DeterministicIdGenerator::GenerateTraceId() noexcept +{ + if (tlsPendingTraceId) + { + auto const id = *tlsPendingTraceId; + tlsPendingTraceId.reset(); + tlsPendingConsumed = true; + return opentelemetry::trace::TraceId( + opentelemetry::nostd::span(id.data(), 16)); + } + return random_->GenerateTraceId(); +} + +opentelemetry::trace::SpanId +DeterministicIdGenerator::GenerateSpanId() noexcept +{ + return random_->GenerateSpanId(); // ALWAYS random — never uses the pending id +} + +PendingTraceId::PendingTraceId(std::array const& id) noexcept +{ + tlsPendingTraceId = id; + tlsPendingConsumed = false; +} + +PendingTraceId::~PendingTraceId() noexcept +{ + // The forced no-parent (root) span-start MUST have consumed the pending id + // via GenerateTraceId(). If not, the SDK took a different branch than the + // caller forced — a bug — so fail loudly in debug/test builds. + XRPL_ASSERT( + tlsPendingConsumed, + "xrpl::telemetry::PendingTraceId : deterministic trace_id was not consumed"); + tlsPendingTraceId.reset(); // never leak, even in release + tlsPendingConsumed = false; +} + +} // namespace xrpl::telemetry + +#endif // XRPL_ENABLE_TELEMETRY diff --git a/src/libxrpl/telemetry/Telemetry.cpp b/src/libxrpl/telemetry/Telemetry.cpp index 33c5036315..a7ba3a4a11 100644 --- a/src/libxrpl/telemetry/Telemetry.cpp +++ b/src/libxrpl/telemetry/Telemetry.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include @@ -321,9 +322,15 @@ public: std::make_shared(setup_.samplingRatio); auto sampler = trace_sdk::ParentBasedSamplerFactory::Create(std::move(rootSampler)); - // Create TracerProvider + // Create TracerProvider with a DeterministicIdGenerator. It returns a + // deterministic trace_id when a PendingTraceId is active on the thread, + // else a random one — letting hash-derived roots (introduced on a later + // branch) become true trace roots. Dormant until such a caller exists. sdkProvider_ = trace_sdk::TracerProviderFactory::Create( - std::move(processor), resourceAttrs, std::move(sampler)); + std::move(processor), + resourceAttrs, + std::move(sampler), + std::make_unique()); // Set as global provider trace_api::Provider::SetTracerProvider( From 4f68c97627a2d2a85228714b81fcd212e75e9306 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 21 Jul 2026 18:47:55 +0100 Subject: [PATCH 03/19] fix(telemetry): root RPC entry spans so each request is its own trace Entry points (http_request/ws_message/ws_upgrade) were inheriting a leaked ambient span on reused coro workers; now scoped fresh roots via ScopedSpanGuard::freshRoot. rpc.process stays a scoped child. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/xrpld/rpc/detail/ServerHandler.cpp | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/xrpld/rpc/detail/ServerHandler.cpp b/src/xrpld/rpc/detail/ServerHandler.cpp index cc9f19d06d..9f221a3823 100644 --- a/src/xrpld/rpc/detail/ServerHandler.cpp +++ b/src/xrpld/rpc/detail/ServerHandler.cpp @@ -228,8 +228,10 @@ ServerHandler::onHandoff( if (!isWs) return statusRequestResponse(request, http::status::unauthorized); - auto span = - SpanGuard::span(TraceCategory::Rpc, rpc_span::prefix::rpc, rpc_span::op::wsUpgrade); + // Fresh root so each WS upgrade is its own trace, not nested under a + // leaked ambient span on a reused coro worker. + auto span = ScopedSpanGuard::freshRoot( + TraceCategory::Rpc, rpc_span::prefix::rpc, rpc_span::op::wsUpgrade); std::shared_ptr ws; try { @@ -347,8 +349,9 @@ ServerHandler::onWSMessage( auto const size = boost::asio::buffer_size(buffers); if (size > RPC::Tuning::kMaxRequestSize || !json::Reader{}.parse(jv, buffers) || !jv.isObject()) { - auto span = - SpanGuard::span(TraceCategory::Rpc, rpc_span::prefix::rpc, rpc_span::op::wsMessage); + // Fresh root so each WS message is its own trace. + auto span = ScopedSpanGuard::freshRoot( + TraceCategory::Rpc, rpc_span::prefix::rpc, rpc_span::op::wsMessage); span.setError(rpc_span::val::invalidJson); json::Value jvResult(json::ValueType::Object); @@ -427,7 +430,9 @@ ServerHandler::processSession( std::shared_ptr const& coro, json::Value const& jv) { - auto span = SpanGuard::span(TraceCategory::Rpc, rpc_span::prefix::rpc, rpc_span::op::wsMessage); + // Fresh root so each WS message is its own trace. + auto span = ScopedSpanGuard::freshRoot( + TraceCategory::Rpc, rpc_span::prefix::rpc, rpc_span::op::wsMessage); auto is = std::static_pointer_cast(session->appDefined); if (is->getConsumer().disconnect(journal_)) { @@ -583,8 +588,10 @@ ServerHandler::processSession( std::shared_ptr const& session, std::shared_ptr coro) { - auto span = - SpanGuard::span(TraceCategory::Rpc, rpc_span::prefix::rpc, rpc_span::op::httpRequest); + // Fresh root so each HTTP request is its own trace, not nested under a + // leaked ambient span on a reused coro worker. + auto span = ScopedSpanGuard::freshRoot( + TraceCategory::Rpc, rpc_span::prefix::rpc, rpc_span::op::httpRequest); bool const ok = processRequest( session->port(), @@ -645,7 +652,8 @@ ServerHandler::processRequest( std::string_view forwardedFor, std::string_view user) { - auto span = SpanGuard::span(TraceCategory::Rpc, rpc_span::prefix::rpc, rpc_span::op::process); + // Scoped child: nests under the httpRequest span active on this thread. + auto span = ScopedSpanGuard(TraceCategory::Rpc, rpc_span::prefix::rpc, rpc_span::op::process); auto rpcJ = app_.getJournal("RPC"); // Tracks whether any failure occurred. Set on every error path (early From e4d02904ad01e747151ebd421afc2666b11a372c Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 21 Jul 2026 19:25:12 +0100 Subject: [PATCH 04/19] test(telemetry): rewrite SpanGuard/ScopedSpanGuard + deterministic-root tests; fix RipplePathFind Rewrite SpanGuardScope.cpp for the unscoped SpanGuard / scoped ScopedSpanGuard split and the DeterministicIdGenerator: install the generator in the TestTelemetry mock (4-arg TracerProviderFactory), drop the obsolete detached()/detachInPlace() tests, and add freshRoot, scoped-ambient, scope-handoff, ends-once, deterministic-root, and cross-thread death tests with exact assertions. Fix the last detached() caller: RipplePathFind now uses SpanGuard::freshRoot (SpanGuard is thread-free, so it is held across the coroutine yield and ended on resume with no scope to strip). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../libxrpl/telemetry/SpanGuardScope.cpp | 495 +++++++++--------- .../rpc/handlers/orderbook/RipplePathFind.cpp | 20 +- 2 files changed, 266 insertions(+), 249 deletions(-) diff --git a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp index 9295086a3a..91999f2352 100644 --- a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp +++ b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp @@ -1,10 +1,19 @@ -// Tests for SpanGuard::rootSpan() and SpanGuard::detached(). +// Tests for SpanGuard, ScopedSpanGuard and DeterministicIdGenerator. // -// These verify the cross-thread scope-leak fix at the trace level using an -// in-memory span exporter: rootSpan() must start a fresh trace root (ignoring -// the ambient active span), and detached() must strip the thread-local Scope -// on the origin thread so the guard can be moved to and ended on another -// thread without corrupting the origin thread's context stack. +// These verify the span-guard split and the deterministic-root fix at the +// trace level using an in-memory span exporter: +// - SpanGuard is unscoped and thread-free: it owns only the span, never the +// OTel thread-local context stack, so it may be moved to and ended on any +// thread. SpanGuard::freshRoot() starts a brand-new trace root, ignoring +// the ambient active span. +// - ScopedSpanGuard is scoped and thread-bound: it also pushes an OTel Scope +// so the span is the ambient parent on the constructing thread. Its +// `operator SpanGuard() &&` pops that Scope eagerly on the origin thread and +// yields a thread-free SpanGuard; destroying it on another thread trips an +// owner-thread assertion. +// - DeterministicIdGenerator (installed by the test TracerProvider) mints a +// caller-pinned trace_id for a forced-root span. PendingTraceId pins the id +// for one root span; an ambient child under a live parent never adopts it. // // The whole file is telemetry-only: when XRPL_ENABLE_TELEMETRY is not defined // SpanGuard is a no-op stub and the OpenTelemetry SDK headers are unavailable, @@ -12,11 +21,13 @@ #ifdef XRPL_ENABLE_TELEMETRY +#include #include #include #include #include +#include #include #include #include @@ -26,16 +37,20 @@ #include #include #include +#include #include +#include #include #include #include #include #include +#include #include +#include +#include #include -#include #include #include #include @@ -54,7 +69,9 @@ namespace otel_memory = opentelemetry::exporter::memory; * Reports every trace category as enabled and creates spans through an SDK * TracerProvider whose SimpleSpanProcessor forwards ended spans to an * InMemorySpanExporter, so a test can read the exact exported SpanData - * (trace id, span id, parent id, name). + * (trace id, span id, parent id, name). The provider is built with a + * DeterministicIdGenerator so PendingTraceId can pin the trace_id of a + * forced-root span. * * Inheritance: * @@ -81,16 +98,19 @@ public: // Factory populates spanData_ with the exporter's shared buffer. auto exporter = otel_memory::InMemorySpanExporterFactory::Create(spanData_); auto processor = otel_sdk_trace::SimpleSpanProcessorFactory::Create(std::move(exporter)); + // Install the DeterministicIdGenerator (4-arg overload) so a + // PendingTraceId can pin a forced-root span's trace_id in tests. provider_ = otel_sdk_trace::TracerProviderFactory::Create( std::move(processor), opentelemetry::sdk::resource::Resource::Create({}), - otel_sdk_trace::AlwaysOnSamplerFactory::Create()); + otel_sdk_trace::AlwaysOnSamplerFactory::Create(), + std::make_unique()); } /** * @return The exporter's span buffer (drained by GetSpans()). */ - std::shared_ptr + [[nodiscard]] std::shared_ptr spanData() const { return spanData_; @@ -215,6 +235,20 @@ countSpans( return count; } +/** + * Build the 16-byte deterministic trace_id used by the generator tests + * (bytes 1..16). Kept out of line so every generator test pins the same id. + * @return The fixed 16-byte trace_id {1, 2, ..., 16}. + */ +std::array +makeTraceIdBytes() +{ + std::array h{}; + for (int i = 0; i < 16; ++i) + h[i] = static_cast(i + 1); + return h; +} + /** * Installs a TestTelemetry as the global instance for each test and clears it * afterwards so the singleton never dangles between cases. @@ -239,7 +273,7 @@ protected: /** * @return The exporter's span buffer for the active TestTelemetry. */ - std::shared_ptr + [[nodiscard]] std::shared_ptr spanData() const { return telemetry_->spanData(); @@ -251,18 +285,18 @@ protected: std::unique_ptr telemetry_; }; -// rootSpan() must ignore the ambient active span and start a brand-new trace. -TEST_F(SpanGuardScopeTest, rootSpan_ignores_ambient_parent) +// freshRoot() must ignore the ambient active span and start a brand-new trace. +TEST_F(SpanGuardScopeTest, spanGuard_freshRoot_is_true_root_ignoring_ambient) { { // Ambient span becomes the active span on this thread. - auto ambient = SpanGuard::span(TraceCategory::Rpc, "rpc", "command"); + ScopedSpanGuard const ambient(TraceCategory::Rpc, "rpc", "command"); ASSERT_TRUE(static_cast(ambient)); - // rootSpan must NOT inherit the ambient span as its parent. - auto root = SpanGuard::rootSpan(TraceCategory::Peer, "peer", "validation.receive"); - ASSERT_TRUE(static_cast(root)); - } // root ends first, then ambient -- both on this thread. + // freshRoot must NOT inherit the ambient span as its parent. + auto r = SpanGuard::freshRoot(TraceCategory::Peer, "peer", "validation.receive"); + ASSERT_TRUE(static_cast(r)); + } // r ends first, then ambient's scope pops and ambient span ends. auto spans = spanData()->GetSpans(); auto* ambient = findSpan(spans, "rpc.command"); @@ -274,34 +308,76 @@ TEST_F(SpanGuardScopeTest, rootSpan_ignores_ambient_parent) EXPECT_FALSE(ambient->GetParentSpanId().IsValid()); EXPECT_TRUE(ambient->GetTraceId().IsValid()); - // The rootSpan has NO parent and lives in a DIFFERENT trace than ambient. + // The freshRoot span has NO parent and lives in a DIFFERENT trace. EXPECT_FALSE(root->GetParentSpanId().IsValid()); EXPECT_TRUE(root->GetTraceId().IsValid()); EXPECT_NE(root->GetTraceId(), ambient->GetTraceId()); } -// detached() pops the Scope on the origin thread, so ending the guard on -// another thread leaves the origin thread's context stack clean. -TEST_F(SpanGuardScopeTest, detached_does_not_corrupt_origin_stack) +// A ScopedSpanGuard is the ambient active span on its thread the moment it is +// constructed: a child created while it is alive parents to its span. +TEST_F(SpanGuardScopeTest, scopedGuard_is_ambient_on_construct) { - // Scoped guard is active on this (origin) thread. - auto guard = SpanGuard::span(TraceCategory::Ledger, "ledger", "build"); - ASSERT_TRUE(static_cast(guard)); - - // Strip the thread-local Scope here on the origin thread. - auto detached = std::move(guard).detached(); - ASSERT_TRUE(static_cast(detached)); - - // End the detached span on a worker thread. - std::thread worker([d = std::move(detached)]() mutable {}); - worker.join(); - - // Back on the origin thread: a new span must be a fresh root, proving the - // origin stack top was restored by detached() (not left holding - // ledger.build). + opentelemetry::trace::SpanId activeId; { - auto after = SpanGuard::span(TraceCategory::Rpc, "rpc", "command"); - ASSERT_TRUE(static_cast(after)); + ScopedSpanGuard const s(TraceCategory::Rpc, "rpc", "process"); + ASSERT_TRUE(static_cast(s)); + + // While s is alive it is the active span on this thread's context. + auto active = + opentelemetry::trace::GetSpan(opentelemetry::context::RuntimeContext::GetCurrent()) + ->GetContext(); + ASSERT_TRUE(active.IsValid()); + activeId = active.span_id(); + + // A child created here must parent to s's span, proving s is ambient. + auto child = s.childSpan("rpc.dispatch"); + ASSERT_TRUE(static_cast(child)); + } // child ends first, then s pops its scope and ends. + + auto spans = spanData()->GetSpans(); + auto* parent = findSpan(spans, "rpc.process"); + auto* child = findSpan(spans, "rpc.dispatch"); + ASSERT_NE(parent, nullptr); + ASSERT_NE(child, nullptr); + + // The active span observed while s was alive WAS s's span. + EXPECT_TRUE(activeId.IsValid()); + EXPECT_EQ(parent->GetSpanId(), activeId); + + // The child nested under s: same trace, parent = s's span. + EXPECT_EQ(child->GetParentSpanId(), parent->GetSpanId()); + EXPECT_EQ(child->GetTraceId(), parent->GetTraceId()); +} + +// operator SpanGuard() && pops the Scope eagerly on the origin thread, so the +// span is no longer ambient here and the resulting thread-free guard can be +// ended on a worker thread without corrupting this thread's context stack. +TEST_F(SpanGuardScopeTest, scopedGuard_conversion_pops_scope_on_this_thread) +{ + { + ScopedSpanGuard s(TraceCategory::Ledger, "ledger", "build"); + ASSERT_TRUE(static_cast(s)); + + // Convert to a bare SpanGuard: the scope is popped here, on this thread. + SpanGuard bare = std::move(s); + ASSERT_TRUE(static_cast(bare)); + + // The scope is gone: no span is active on this thread now. + auto active = + opentelemetry::trace::GetSpan(opentelemetry::context::RuntimeContext::GetCurrent()) + ->GetContext(); + EXPECT_FALSE(active.IsValid()); + + // A new ambient span here is a fresh root, NOT nested under build. + { + ScopedSpanGuard const after(TraceCategory::Rpc, "rpc", "command"); + ASSERT_TRUE(static_cast(after)); + } + + // End the thread-free guard on a worker thread -- no crash. + std::thread worker([g = std::move(bare)]() mutable {}); + worker.join(); } auto spans = spanData()->GetSpans(); @@ -310,243 +386,186 @@ TEST_F(SpanGuardScopeTest, detached_does_not_corrupt_origin_stack) ASSERT_NE(build, nullptr); ASSERT_NE(after, nullptr); - // 'after' is a new root: zero parent and a different trace than - // ledger.build. + // The span was exported exactly once, by the worker thread. + EXPECT_EQ(countSpans(spans, "ledger.build"), 1u); + + // 'after' did NOT nest under build: fresh root, different trace. EXPECT_FALSE(after->GetParentSpanId().IsValid()); EXPECT_NE(after->GetTraceId(), build->GetTraceId()); } -// A detached span is ended exactly once, by the worker thread that owns it. -TEST_F(SpanGuardScopeTest, detached_span_ends_once) -{ - auto guard = SpanGuard::span(TraceCategory::Ledger, "ledger", "build"); - auto detached = std::move(guard).detached(); - ASSERT_TRUE(static_cast(detached)); - - // Before the worker ends it, the span is still open: nothing exported yet. - auto before = spanData()->GetSpans(); - EXPECT_EQ(countSpans(before, "ledger.build"), 0u); - - // Ending happens once, on the worker thread, when the guard is destroyed. - { - std::thread worker([d = std::move(detached)]() mutable {}); - worker.join(); - } - - auto after = spanData()->GetSpans(); - EXPECT_EQ(countSpans(after, "ledger.build"), 1u); -} - -// rootSpan().detached() is the combination used at RPC coroutine entry points -// (e.g. RipplePathFind) that hold a span across a coro yield: the span must be -// a fresh root AND must not leave a Scope on the worker's context stack. -TEST_F(SpanGuardScopeTest, root_then_detached_is_root_and_leaves_stack_clean) +// The SpanGuard produced by the conversion ends the span exactly once: the +// moved-from ScopedSpanGuard must not re-end it on destruction. +TEST_F(SpanGuardScopeTest, scopedGuard_conversion_result_ends_span_once) { { - // Ambient span active on this (origin) thread. - auto ambient = SpanGuard::span(TraceCategory::Rpc, "rpc", "command"); - ASSERT_TRUE(static_cast(ambient)); + ScopedSpanGuard scoped(TraceCategory::Ledger, "ledger", "build"); + ASSERT_TRUE(static_cast(scoped)); - // Fresh root, then detach the Scope on the origin thread. - auto req = SpanGuard::rootSpan(TraceCategory::Rpc, "pathfind", "request").detached(); - ASSERT_TRUE(static_cast(req)); + SpanGuard const bare = std::move(scoped); + ASSERT_TRUE(static_cast(bare)); - // End the detached root span on a worker thread (mimics coro resume). - std::thread worker([r = std::move(req)]() mutable {}); - worker.join(); + // Nothing exported yet: the span is still open. + EXPECT_EQ(countSpans(spanData()->GetSpans(), "ledger.build"), 0u); - // Origin stack must be clean: a new span here is still parented by the - // ambient span (proving the detached root did not linger on the stack). - { - auto child = SpanGuard::span(TraceCategory::Rpc, "rpc", "sub"); - ASSERT_TRUE(static_cast(child)); - } + // 'bare' ends the span here on destruction; the moved-from 'scoped' + // guard is destroyed too but must NOT end it a second time. } auto spans = spanData()->GetSpans(); - auto* ambient = findSpan(spans, "rpc.command"); - auto* req = findSpan(spans, "pathfind.request"); - auto* child = findSpan(spans, "rpc.sub"); - ASSERT_NE(ambient, nullptr); - ASSERT_NE(req, nullptr); - ASSERT_NE(child, nullptr); - - // The rooted-then-detached span has NO parent and a DIFFERENT trace. - EXPECT_FALSE(req->GetParentSpanId().IsValid()); - EXPECT_NE(req->GetTraceId(), ambient->GetTraceId()); - - // The detached root did not corrupt the origin stack: 'child' still nests - // under the ambient span (same trace, parent = ambient), NOT under req. - EXPECT_EQ(child->GetParentSpanId(), ambient->GetSpanId()); - EXPECT_EQ(child->GetTraceId(), ambient->GetTraceId()); + // Exactly one export: not zero (the guard still owns the span) and not two + // (the moved-from scoped guard does not re-end it). + EXPECT_EQ(countSpans(spans, "ledger.build"), 1u); } -// Death test guarding the cross-thread scope-leak bug. +// A forced-root span started while a PendingTraceId is active adopts that +// pinned 16-byte trace_id and remains a true root (no parent). +TEST_F(SpanGuardScopeTest, deterministicIdGenerator_forced_root_gets_pending_trace_id) +{ + auto const h = makeTraceIdBytes(); + { + PendingTraceId const pending{h}; + auto rootCtx = opentelemetry::context::Context{opentelemetry::trace::kIsRootSpanKey, true}; + auto span = + telemetry_->startSpan("tx.receive", rootCtx, opentelemetry::trace::SpanKind::kInternal); + span->End(); + } // ~PendingTraceId asserts the id was consumed. + + auto spans = spanData()->GetSpans(); + ASSERT_EQ(spans.size(), 1u); + // trace_id == the pinned hash. + EXPECT_EQ(std::memcmp(spans[0]->GetTraceId().Id().data(), h.data(), 16), 0); + // TRUE ROOT: no parent. + EXPECT_FALSE(spans[0]->GetParentSpanId().IsValid()); +} + +// A forced-root span with NO PendingTraceId gets a random (non-zero) trace_id, +// never the deterministic hash -- the safety property when no id is pinned. +TEST_F(SpanGuardScopeTest, deterministicIdGenerator_no_pending_gives_random_root) +{ + auto const h = makeTraceIdBytes(); + { + auto rootCtx = opentelemetry::context::Context{opentelemetry::trace::kIsRootSpanKey, true}; + auto span = + telemetry_->startSpan("tx.receive", rootCtx, opentelemetry::trace::SpanKind::kInternal); + span->End(); + } + + auto spans = spanData()->GetSpans(); + ASSERT_EQ(spans.size(), 1u); + // Random root: valid (non-zero) trace_id... + EXPECT_TRUE(spans[0]->GetTraceId().IsValid()); + // ...and NOT the deterministic hash (no pending id leaked in). + EXPECT_NE(std::memcmp(spans[0]->GetTraceId().Id().data(), h.data(), 16), 0); + EXPECT_FALSE(spans[0]->GetParentSpanId().IsValid()); +} + +// SAFETY: an ambient child under a live parent never adopts a pending id. The +// SDK inherits the parent's trace_id and never calls GenerateTraceId() for the +// child, so the pinned id stays available -- proven here by a trailing +// forced-root span that DOES adopt it (which also consumes the id so +// ~PendingTraceId's consumed-assert holds; see the report for this choice). +TEST_F(SpanGuardScopeTest, deterministicIdGenerator_ambient_child_ignores_pending) +{ + auto const h = makeTraceIdBytes(); + { + // Ambient parent (a random-id root) active on this thread FIRST, before + // any id is pinned, so the parent itself does not consume it. + ScopedSpanGuard const parent(TraceCategory::Rpc, "rpc", "process"); + ASSERT_TRUE(static_cast(parent)); + + // Pin the id, then start an ambient child under the live parent. + PendingTraceId const pending{h}; + + // The child has a valid parent, so the SDK inherits the parent's + // trace_id and never calls GenerateTraceId(): the pending id is ignored. + { + auto child = parent.childSpan("rpc.dispatch"); + ASSERT_TRUE(static_cast(child)); + } + + // Consume the pinned id with a real forced-root span (its intended use), + // so ~PendingTraceId sees the id as consumed. Its trace_id == h. + { + auto rootCtx = + opentelemetry::context::Context{opentelemetry::trace::kIsRootSpanKey, true}; + auto root = telemetry_->startSpan( + "tx.receive", rootCtx, opentelemetry::trace::SpanKind::kInternal); + root->End(); + } + } // ~PendingTraceId: consumed == true, assert holds; then parent ends. + + auto spans = spanData()->GetSpans(); + auto* parent = findSpan(spans, "rpc.process"); + auto* child = findSpan(spans, "rpc.dispatch"); + auto* root = findSpan(spans, "tx.receive"); + ASSERT_NE(parent, nullptr); + ASSERT_NE(child, nullptr); + ASSERT_NE(root, nullptr); + + // The ambient child inherited the parent's trace and did NOT adopt h. + EXPECT_EQ(child->GetTraceId(), parent->GetTraceId()); + EXPECT_EQ(child->GetParentSpanId(), parent->GetSpanId()); + EXPECT_NE(std::memcmp(child->GetTraceId().Id().data(), h.data(), 16), 0); + + // The forced-root span DID adopt the pinned id: proof the id was available + // the whole time -- the ambient child simply never requested it. + EXPECT_EQ(std::memcmp(root->GetTraceId().Id().data(), h.data(), 16), 0); + EXPECT_FALSE(root->GetParentSpanId().IsValid()); +} + +// Death test guarding the cross-thread scope-leak bug, now on ScopedSpanGuard. // -// This used to be a "negative control": a scoped guard was NOT detached, moved -// to and destroyed on a worker thread (popping the WRONG thread's stack), and -// the test then asserted that a later span on the origin thread had SILENTLY -// inherited the stale span -- proving the corruption happened undetected. +// A ScopedSpanGuard's Scope is bound to the thread that constructed it, so it +// must be destroyed on that same thread. ScopedSpanGuard is non-movable, so it +// cannot be moved into a worker directly; instead we own it through a +// unique_ptr and move only that pointer to a worker thread. Destroying the +// pointer there runs ~ScopedSpanGuard on the WRONG thread, which pops the Scope +// on a foreign context stack -- ~ScopedSpanGuard's owner-thread XRPL_ASSERT +// turns that silent corruption into a loud abort(). // -// SpanGuard::Impl::~Impl now enforces thread affinity with an XRPL_ASSERT: a -// live scope must be destroyed on the thread that created it. In debug/test -// builds (NDEBUG unset) XRPL_ASSERT expands to assert(), so the exact sequence -// above now aborts the process inside the worker thread's destructor instead of -// corrupting the origin stack. The bug therefore moved from "silently wrong" to -// "loudly crashes early" -- the intended tradeoff, since a crash is far easier -// to catch than a corrupted trace hierarchy. -// -// The death happens on the WORKER thread (the moved-in guard is destroyed when -// the worker lambda returns, before worker.join() completes). A failed assert() -// calls abort(), which raises SIGABRT process-wide regardless of which thread -// hit it, so EXPECT_DEATH -- which runs the statement in a forked child and -// checks it dies -- observes the crash. The regex matches the assert message -// substring rather than the whole line, because the file:line/function prefix -// glibc prints around it is platform and compiler dependent. +// The death happens on the WORKER thread (the moved-in unique_ptr is destroyed +// when the worker lambda's captures are torn down, before worker.join() +// completes). A failed assert() calls abort(), which raises SIGABRT +// process-wide regardless of thread, so EXPECT_DEATH -- which runs the +// statement in a forked child and checks it dies -- observes the crash. The +// regex matches the assert message substring; the assert message is written as +// two adjacent string literals in SpanGuard.cpp, so the ".*" bridges the gap +// between "on" and "constructing". // // The test is skipped where the assertion cannot fire: under NDEBUG (Release // builds) XRPL_ASSERT is a no-op, and under ENABLE_VOIDSTAR a failed assert // continues instead of aborting -- in both cases the worker would not crash and // EXPECT_DEATH would report a spurious failure. -TEST_F(SpanGuardScopeTest, non_detached_cross_thread_death_asserts_at_wrong_thread_destroy) +TEST_F(SpanGuardScopeTest, scopedGuard_cross_thread_death_asserts_at_wrong_thread_destroy) { - // XRPL_ASSERT only aborts when assertions are live. Under NDEBUG (Release - // builds) it expands to assert(), which the C standard strips to a no-op, - // so the worker thread destroys the guard without crashing. Under - // ENABLE_VOIDSTAR (Antithesis fuzzing) a failed XRPL_ASSERT continues - // execution instead of aborting. In either case the process does not die - // and EXPECT_DEATH would fail, so skip this test there. The two macros are - // mutually exclusive (instrumentation.h #errors on ENABLE_VOIDSTAR+NDEBUG), - // but both break the death check, so guard against each. #ifdef NDEBUG GTEST_SKIP() << "XRPL_ASSERT compiles to a no-op under NDEBUG (Release builds), so the " "cross-thread scope-leak assertion this test exercises does not fire."; -#elif defined(ENABLE_VOIDSTAR) +#elifdef ENABLE_VOIDSTAR GTEST_SKIP() << "ENABLE_VOIDSTAR continues past a failed XRPL_ASSERT instead of aborting, so " "the cross-thread scope-leak assertion this test exercises does not crash."; #else EXPECT_DEATH( { - // Scoped guard is active on this (origin) thread; its Scope is - // bound to this thread. - auto guard = SpanGuard::span(TraceCategory::Ledger, "ledger", "build"); + // Scoped guard constructed on THIS thread; its Scope binds here. + auto scoped = + std::make_unique(TraceCategory::Ledger, "ledger", "build"); - // Move the still-scoped guard to a worker thread WITHOUT detaching. - // ~SpanGuard::Impl runs on the worker when the lambda returns and - // trips the thread-affinity assertion -> abort(). - std::thread worker([d = std::move(guard)]() mutable {}); + // Move only the owning pointer to a worker. ~ScopedSpanGuard runs on + // the worker when the lambda's captures are destroyed and trips the + // owner-thread assertion -> abort(). + std::thread worker([s = std::move(scoped)]() mutable {}); worker.join(); }, // The assert message is written as two adjacent string literals in - // SpanGuard.cpp; glibc's assert() stringifies the expression source via - // the preprocessor '#' operator, keeping both quoted literals with the + // SpanGuard.cpp; assert() stringifies the expression source via the + // preprocessor '#' operator, keeping both quoted literals with the // "\" \"" gap between "on" and "constructing". Match across that gap. - ".*scoped guard destroyed on.*constructing thread.*"); + ".*destroyed on.*constructing thread.*"); #endif } -// detachInPlace(optional&) must detach the live guard the same way the -// hand-rolled emplace(std::move(*opt).detached()) idiom does: after the call -// the guard can be moved to and ended on a worker thread without leaving the -// origin thread's context stack holding it. -TEST_F(SpanGuardScopeTest, detach_in_place_optional_detaches_active_guard) -{ - // Scoped optional guard is active on this (origin) thread. - std::optional opt = SpanGuard::span(TraceCategory::Ledger, "ledger", "build"); - ASSERT_TRUE(opt.has_value()); - ASSERT_TRUE(static_cast(*opt)); - - // Strip the thread-local Scope here on the origin thread via the helper. - detachInPlace(opt); - ASSERT_TRUE(opt.has_value()); - ASSERT_TRUE(static_cast(*opt)); - - // End the detached span on a worker thread. - std::thread worker([d = std::move(*opt)]() mutable {}); - worker.join(); - - // Back on the origin thread: a new span must be a fresh root, proving the - // origin stack top was restored by detachInPlace() (not left holding - // ledger.build). - { - auto after = SpanGuard::span(TraceCategory::Rpc, "rpc", "command"); - ASSERT_TRUE(static_cast(after)); - } - - auto spans = spanData()->GetSpans(); - auto* build = findSpan(spans, "ledger.build"); - auto* after = findSpan(spans, "rpc.command"); - ASSERT_NE(build, nullptr); - ASSERT_NE(after, nullptr); - - // 'after' is a new root: zero parent and a different trace than - // ledger.build. - EXPECT_FALSE(after->GetParentSpanId().IsValid()); - EXPECT_NE(after->GetTraceId(), build->GetTraceId()); -} - -// detachInPlace(optional&) on an empty optional is a no-op: it must not crash, -// must leave the optional empty, and must export nothing. -TEST_F(SpanGuardScopeTest, detach_in_place_optional_noop_on_empty) -{ - std::optional opt; - ASSERT_FALSE(opt.has_value()); - - detachInPlace(opt); - - // Still empty, no span was created or exported. - EXPECT_FALSE(opt.has_value()); - EXPECT_TRUE(spanData()->GetSpans().empty()); -} - -// detachInPlace(shared_ptr) must detach the live guard and return it as a new -// shared_ptr, so the returned guard can be moved to and ended on a worker -// thread without corrupting the origin thread's context stack. -TEST_F(SpanGuardScopeTest, detach_in_place_shared_detaches_active_guard) -{ - // Scoped shared guard is active on this (origin) thread. - auto span = - std::make_shared(SpanGuard::span(TraceCategory::Ledger, "ledger", "build")); - ASSERT_NE(span, nullptr); - ASSERT_TRUE(static_cast(*span)); - - // Strip the thread-local Scope here on the origin thread via the helper. - span = detachInPlace(std::move(span)); - ASSERT_NE(span, nullptr); - ASSERT_TRUE(static_cast(*span)); - - // End the detached span on a worker thread. - std::thread worker([d = std::move(span)]() mutable {}); - worker.join(); - - // Back on the origin thread: a new span must be a fresh root, proving the - // origin stack top was restored by detachInPlace() (not left holding - // ledger.build). - { - auto after = SpanGuard::span(TraceCategory::Rpc, "rpc", "command"); - ASSERT_TRUE(static_cast(after)); - } - - auto spans = spanData()->GetSpans(); - auto* build = findSpan(spans, "ledger.build"); - auto* after = findSpan(spans, "rpc.command"); - ASSERT_NE(build, nullptr); - ASSERT_NE(after, nullptr); - - // 'after' is a new root: zero parent and a different trace than - // ledger.build. - EXPECT_FALSE(after->GetParentSpanId().IsValid()); - EXPECT_NE(after->GetTraceId(), build->GetTraceId()); -} - -// detachInPlace(shared_ptr) on a null pointer is a no-op: it must not crash and -// must return null unchanged. -TEST_F(SpanGuardScopeTest, detach_in_place_shared_noop_on_null) -{ - auto result = detachInPlace(std::shared_ptr{}); - EXPECT_EQ(result, nullptr); -} - } // namespace } // namespace xrpl::telemetry diff --git a/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp b/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp index 24373a677f..b1e2bc64b2 100644 --- a/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp +++ b/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp @@ -27,19 +27,17 @@ json::Value doRipplePathFind(RPC::JsonContext& context) { using namespace telemetry; - // This RPC span is held live across context.coro->yield() below, so it is - // both rooted and detached: - // - rootSpan: on resume the coroutine may run on a different JobQueue + // This RPC span is held live across context.coro->yield() below, so it must + // be both a fresh root and thread-free: + // - freshRoot: on resume the coroutine may run on a different JobQueue // worker whose thread-local context stack holds unrelated spans; a fresh // root avoids inheriting a stale ambient parent at creation. - // - detached(): strips the thread-local Scope so the guard does not sit on - // the worker's context stack across the yield (which would adopt other - // spans run on that thread) and is not popped on the wrong thread when - // the coroutine resumes elsewhere. The span still ends when this frame - // unwinds after resume. - auto span = SpanGuard::rootSpan( - TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::request) - .detached(); + // - SpanGuard is thread-free: it holds no thread-local Scope, so it never + // sits on the worker's context stack across the yield and is never popped + // on the wrong thread when the coroutine resumes elsewhere. No scope to + // strip. The span still ends when this frame unwinds after resume. + auto span = SpanGuard::freshRoot( + TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::request); // Addresses are hashed before emission for privacy. if (auto const& src = context.params[jss::source_account]; src.isString()) span.setAttribute(pathfind_span::attr::sourceAccount, redactAccount(src.asString())); From d63d5bd45e851b3a3bb75db3cd4a4e97cd4982c5 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 21 Jul 2026 20:10:10 +0100 Subject: [PATCH 05/19] fix(telemetry): hashSpan standalone spans are true roots via DeterministicIdGenerator The single-arg hashSpan fabricated a synthetic DefaultSpan parent (random span_id) to carry the deterministic trace_id. That left every standalone hash span with a phantom parent_span_id pointing at a span that is never exported, so Tempo reported "root span not yet received". Inject the deterministic trace_id through the DeterministicIdGenerator instead: start the span on the SDK root branch (Context{kIsRootSpanKey,true}) with a PendingTraceId pinning hashData[0:16]. The SDK's no-valid-parent branch calls GenerateTraceId(), which returns the pinned id, yielding a TRUE root (empty parent_span_id) whose trace_id == hashData[0:16]. The deterministic bytes are unchanged (still raw hashData[0:16]). The cross-node overload (real remote parent, remote=true) is untouched and must keep producing a child of the sender's span. Drop the now-unused include; add and DeterministicIdGenerator.h. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/libxrpl/telemetry/SpanGuard.cpp | 36 ++++++++++++++--------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/src/libxrpl/telemetry/SpanGuard.cpp b/src/libxrpl/telemetry/SpanGuard.cpp index ba837bd607..242a565817 100644 --- a/src/libxrpl/telemetry/SpanGuard.cpp +++ b/src/libxrpl/telemetry/SpanGuard.cpp @@ -28,8 +28,8 @@ #include -#include #include +#include #include #include @@ -49,6 +49,7 @@ #include #include +#include #include #include #include @@ -303,6 +304,7 @@ SpanGuard::linkedSpan(std::string_view name, SpanContext const& linkCtx) // ===== Hash-derived span (category-gated) ================================== +// Standalone hash-derived span: a TRUE root whose trace_id == hashData[0:16]. SpanGuard SpanGuard::hashSpan( TraceCategory const cat, @@ -316,26 +318,22 @@ SpanGuard::hashSpan( if ((tel == nullptr) || !tel->isEnabled() || !isCategoryEnabled(*tel, cat)) return {}; - otel_trace::TraceId const traceId( - opentelemetry::nostd::span(hashData, 16)); - - auto const rval = defaultPrng()(); - std::uint8_t spanIdBytes[8]; - std::memcpy(spanIdBytes, &rval, sizeof(spanIdBytes)); - otel_trace::SpanId const spanId( - opentelemetry::nostd::span(spanIdBytes, 8)); - - otel_trace::SpanContext const syntheticCtx( - traceId, spanId, otel_trace::TraceFlags(1), /* remote = */ false); - - auto parentCtx = opentelemetry::context::Context{}.SetValue( - otel_trace::kSpanKey, - opentelemetry::nostd::shared_ptr( - new otel_trace::DefaultSpan(syntheticCtx))); - - return SpanGuard(std::make_unique(tel->startSpan(std::string(name), parentCtx))); + // Force the deterministic trace_id via the DeterministicIdGenerator, on the + // SDK root branch, so the span is a TRUE root (empty parent_span_id) whose + // trace_id == hashData[0:16]. The kIsRootSpanKey context forces the SDK's + // no-valid-parent branch, where GenerateTraceId() is called and returns the + // pending id. The PendingTraceId guard scopes that id to this one startSpan. + std::array tid{}; + std::memcpy(tid.data(), hashData, 16); + auto rootCtx = opentelemetry::context::Context{otel_trace::kIsRootSpanKey, true}; + PendingTraceId const pending{tid}; + return SpanGuard( + std::make_unique( + tel->startSpan(std::string(name), rootCtx, categoryToSpanKind(cat)))); } +// Cross-node hash-derived span: a CHILD of the sender's real remote span +// (remote=true), not a root — so it must NOT use PendingTraceId / rootCtx. SpanGuard SpanGuard::hashSpan( TraceCategory const cat, From deecae0f0199a131d2245f34366660a055add609 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 21 Jul 2026 20:15:31 +0100 Subject: [PATCH 06/19] refactor(telemetry): tx spans thread-free (no detach); txq spans scoped for nesting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SpanGuard is now thread-free (holds no Scope), so the tx.receive (PeerImp) and tx.process (NetworkOPs) handoff sites no longer need .detached() before being stored and ended on a worker thread — just construct the guard. The stale "Scope leak" comments are replaced accordingly. Make the six txq.* spans ScopedSpanGuard so their sub-spans nest via the ambient context: txq.apply_direct/batch_clear under txq.enqueue, and txq.accept_tx under txq.accept. All six are verified synchronous, ended at scope, and never moved/handed off, so scoping is safe. applyDirect's span is pure RAII (no method calls), so it is declared const. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/xrpld/app/misc/NetworkOPs.cpp | 9 ++++----- src/xrpld/app/misc/detail/TxQ.cpp | 15 ++++++--------- src/xrpld/overlay/detail/PeerImp.cpp | 7 +++---- 3 files changed, 13 insertions(+), 18 deletions(-) diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index bffe8c5c9f..aa5a483119 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1384,11 +1384,10 @@ NetworkOPsImp::processTransaction( FailHard failType) { using namespace telemetry; - // Detached: this span is stored in TransactionStatus and applied on a - // batch worker thread, so it must not leave its Scope bound to this - // thread's context stack (that leak would adopt later work into this - // transaction's trace). - auto span = std::make_shared(txProcessSpan(transaction->getID()).detached()); + // SpanGuard is thread-free (holds no Scope), so it is safe to store here + // and end on the batch worker thread that later applies this transaction — + // no detach step is needed. + auto span = std::make_shared(txProcessSpan(transaction->getID())); span->setAttribute(tx_span::attr::txHash, to_string(transaction->getID()).c_str()); span->setAttribute(tx_span::attr::local, bLocal); if (auto const& stx = transaction->getSTransaction()) diff --git a/src/xrpld/app/misc/detail/TxQ.cpp b/src/xrpld/app/misc/detail/TxQ.cpp index d3a6b3290d..e0c988a418 100644 --- a/src/xrpld/app/misc/detail/TxQ.cpp +++ b/src/xrpld/app/misc/detail/TxQ.cpp @@ -545,7 +545,7 @@ TxQ::tryClearAccountQueueUpThruTx( beast::Journal j) { using namespace telemetry; - [[maybe_unused]] auto span = SpanGuard::span( + [[maybe_unused]] ScopedSpanGuard span( TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::batchClear); SeqProxy const tSeqProx{tx.getSeqProxy()}; @@ -752,8 +752,7 @@ TxQ::apply( beast::Journal j) { using namespace telemetry; - auto span = - SpanGuard::span(TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::enqueue); + ScopedSpanGuard span(TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::enqueue); span.setAttribute(txq_span::attr::txHash, to_string(tx->getTransactionID()).c_str()); if (auto const* fmt = TxFormats::getInstance().findByType(tx->getTxnType())) span.setAttribute(txq_span::attr::txType, fmt->getName().c_str()); @@ -1374,8 +1373,7 @@ void TxQ::processClosedLedger(Application& app, ReadView const& view, bool timeLeap) { using namespace telemetry; - auto span = - SpanGuard::span(TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::cleanup); + ScopedSpanGuard span(TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::cleanup); span.setAttribute(txq_span::attr::ledgerSeq, static_cast(view.header().seq)); std::scoped_lock const lock(mutex_); @@ -1466,8 +1464,7 @@ TxQ::accept(Application& app, OpenView& view) // Create the span and read byFee_.size() only after taking the lock, since // byFee_ is guarded by mutex_. - auto span = - SpanGuard::span(TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::accept); + ScopedSpanGuard span(TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::accept); span.setAttribute(txq_span::attr::queueSize, static_cast(byFee_.size())); auto const metricsSnapshot = feeMetrics_.getSnapshot(); @@ -1497,7 +1494,7 @@ TxQ::accept(Application& app, OpenView& view) JLOG(j_.trace()) << "Applying queued transaction " << candidateIter->txID << " to open ledger."; - auto txSpan = SpanGuard::span( + ScopedSpanGuard txSpan( TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::acceptTx); txSpan.setAttribute(txq_span::attr::txHash, to_string(candidateIter->txID).c_str()); txSpan.setAttribute( @@ -1718,7 +1715,7 @@ TxQ::tryDirectApply( beast::Journal j) { using namespace telemetry; - [[maybe_unused]] auto span = SpanGuard::span( + [[maybe_unused]] ScopedSpanGuard const span( TraceCategory::Transactions, txq_span::prefix::txq, txq_span::op::applyDirect); auto const account = (*tx)[sfAccount]; diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index c0fb9c6c56..fcf07046e4 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1311,10 +1311,9 @@ PeerImp::handleTransaction( uint256 const txID = stx->getTransactionID(); using namespace telemetry; - // Detached: this span is handed to a job-queue worker and must not - // leave its Scope bound to this peer thread's context stack (that - // leak would adopt later peer messages into this transaction's trace). - auto span = std::make_shared(txReceiveSpan(txID, *m).detached()); + // SpanGuard is thread-free (holds no Scope), so it is safe to hand to + // a job-queue worker and end on that thread — no detach step is needed. + auto span = std::make_shared(txReceiveSpan(txID, *m)); span->setAttribute(tx_span::attr::txHash, to_string(txID).c_str()); span->setAttribute(tx_span::attr::peerId, static_cast(id_)); if (auto const* fmt = TxFormats::getInstance().findByType(stx->getTxnType())) From f6f0e722241ad931d16485e520ddaa1497aa14aa Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 21 Jul 2026 20:39:29 +0100 Subject: [PATCH 07/19] docs(telemetry): DeterministicIdGenerator is active via hashSpan, not dormant The generator's doc said it was 'dormant, caller lands on a later branch'. Reworded to branch-agnostic language: hashSpan() is the primary caller that forces the root branch to mint deterministic trace roots. Co-Authored-By: Claude Opus 4.8 (1M context) --- include/xrpl/telemetry/DeterministicIdGenerator.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/include/xrpl/telemetry/DeterministicIdGenerator.h b/include/xrpl/telemetry/DeterministicIdGenerator.h index 6e83054797..d284a2d4b4 100644 --- a/include/xrpl/telemetry/DeterministicIdGenerator.h +++ b/include/xrpl/telemetry/DeterministicIdGenerator.h @@ -55,11 +55,12 @@ namespace xrpl::telemetry { * @note Limitation: minting a deterministic root only works when the caller * forces the SDK's root branch (e.g. by starting the span with a * Context{kIsRootSpanKey, true}); otherwise the SDK reuses the parent's - * trace_id and never calls GenerateTraceId(). That caller (hashSpan) lands on - * a later branch, so this generator is installed but dormant here. + * trace_id and never calls GenerateTraceId(). The primary such caller is + * SpanGuard::hashSpan(), which forces the root branch to mint deterministic + * per-object trace roots. * - * Example 1 - primary use, inside a forced-root hash span (arrives on a later - * branch; shown as pseudocode): + * Example 1 - primary use, inside a forced-root hash span (shown as + * pseudocode): * @code * // std::array id = deriveTraceIdFromHash(txHash); * // PendingTraceId const pending{id}; // pin id for this thread @@ -128,8 +129,7 @@ public: * guards on one thread is unsupported: the inner guard consumes/clears first, * so the outer id is silently dropped and its destructor trips the assert. * - * Example 1 - primary use, wrapping a forced-root span start (pseudocode; the - * caller lands on a later branch): + * Example 1 - primary use, wrapping a forced-root span start (pseudocode): * @code * // { * // PendingTraceId const pending{id}; // pin for this scope From 97e6586f71c0d1320c2e069d4274f8c6c1fd6d0c Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 21 Jul 2026 21:07:20 +0100 Subject: [PATCH 08/19] fix(telemetry): captureContext() captures the guard's own span, not ambient MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SpanGuard is unscoped (never pushed onto the thread-local context stack), so RuntimeContext::GetCurrent() returned an unrelated ambient span. Build the captured context from impl_->span so captureContext() is correct for every caller regardless of scoping — childSpan(name, ctx) then parents explicitly to this span across threads. Matches getTraceBytes()'s own-context behavior. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/libxrpl/telemetry/SpanGuard.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/libxrpl/telemetry/SpanGuard.cpp b/src/libxrpl/telemetry/SpanGuard.cpp index 825402ff63..0b3a35d170 100644 --- a/src/libxrpl/telemetry/SpanGuard.cpp +++ b/src/libxrpl/telemetry/SpanGuard.cpp @@ -294,9 +294,15 @@ SpanGuard::linkedSpan(std::string_view name, SpanContext const& linkCtx) SpanContext SpanGuard::captureContext() const { - if (!impl_) + if (!impl_ || !impl_->span) return {}; - auto ctx = opentelemetry::context::RuntimeContext::GetCurrent(); + // Capture THIS guard's own span, not the thread's ambient span. A + // SpanGuard is unscoped (never pushed onto the thread-local stack), so + // RuntimeContext::GetCurrent() would return an unrelated ambient span. + // Building the context from impl_->span makes captureContext() correct + // for every caller regardless of scoping, and lets childSpan(name, ctx) + // parent explicitly to this span across threads. + auto ctx = opentelemetry::context::Context{}.SetValue(otel_trace::kSpanKey, impl_->span); return SpanContext(std::make_shared(std::move(ctx))); } From 0e7ca13cf3b3cd769b453b0f95884195f8e20e5b Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 21 Jul 2026 21:47:01 +0100 Subject: [PATCH 09/19] refactor(telemetry): split context capture into spanContext() (own span) + threadLocalContext() (current) OTel distinguishes a span's own context from the thread's current/ambient context; SpanGuard is unscoped so spanContext() (own span, default) is what cross-thread childSpan parenting needs, and threadLocalContext() (static) snapshots RuntimeContext::GetCurrent() for propagation. Renames captureContext. Co-Authored-By: Claude Opus 4.8 (1M context) --- include/xrpl/telemetry/SpanGuard.h | 69 +++++++++++++++++++++-------- include/xrpl/telemetry/Telemetry.h | 4 +- src/libxrpl/telemetry/SpanGuard.cpp | 20 ++++++--- 3 files changed, 68 insertions(+), 25 deletions(-) diff --git a/include/xrpl/telemetry/SpanGuard.h b/include/xrpl/telemetry/SpanGuard.h index 9ec4733473..4ae3a03cf2 100644 --- a/include/xrpl/telemetry/SpanGuard.h +++ b/include/xrpl/telemetry/SpanGuard.h @@ -33,7 +33,8 @@ * | + freshRoot(cat, prefix, name) [static] | * | + childSpan(name) : SpanGuard | * | + linkedSpan(name) : SpanGuard | - * | + captureContext() : SpanContext | + * | + spanContext() : SpanContext | + * | + threadLocalContext() [static] | * | + setAttribute(key, value) | * | + setOk() / setError(desc) | * | + addEvent(name) | @@ -89,10 +90,10 @@ * #include * using namespace xrpl::telemetry; * - * // Thread A: create span and capture context + * // Thread A: create span and capture its own context as parent * auto span = SpanGuard::span( * TraceCategory::Consensus, seg::consensus, consensus::span::op::round); - * auto ctx = span.captureContext(); + * auto ctx = span.spanContext(); * * // Thread B: create child with captured context * auto child = SpanGuard::childSpan(consensus::span::accept, ctx); @@ -155,8 +156,9 @@ * span (no Scope), so it never binds to a thread-local context stack * and may be moved to and destroyed on any thread. To make a span the * ambient parent for child spans on a thread, wrap it in a - * ScopedSpanGuard. Use captureContext() to propagate the trace - * context across threads explicitly. + * ScopedSpanGuard. Use spanContext() to propagate this span's own + * context across threads explicitly, or threadLocalContext() to + * snapshot the thread's currently-active context. * * @note Move semantics: Move construction and move assignment both * transfer ownership of the pimpl pointer. There is no Scope to @@ -189,8 +191,10 @@ enum class TraceCategory { Rpc, Transactions, Consensus, Peer, Ledger }; * Opaque wrapper for an OTel context snapshot. * * Used to propagate trace context across threads. Created by - * SpanGuard::captureContext(), consumed by SpanGuard::childSpan() - * or SpanGuard::linkedSpan() with an explicit parent/link context. + * SpanGuard::spanContext() (the guard's own span) or + * SpanGuard::threadLocalContext() (the thread's current context), + * consumed by SpanGuard::childSpan() or SpanGuard::linkedSpan() + * with an explicit parent/link context. */ class SpanContext { @@ -311,7 +315,7 @@ public: /** * Create a child span parented to an explicit captured context. * @param name Span name for the child. - * @param parentCtx Context captured via captureContext(). + * @param parentCtx Context captured via spanContext(). * @return A new guard, or null if parentCtx is invalid. */ [[nodiscard]] static SpanGuard @@ -339,11 +343,29 @@ public: // --- Context capture ----------------------------------------------- /** - * Snapshot the current thread's OTel context for cross-thread use. - * @return An opaque SpanContext, or an invalid one if null guard. + * Return a SpanContext referring to THIS guard's own span, for use + * as an explicit parent in childSpan(name, ctx) across threads. + * Independent of the thread's active span (a SpanGuard is unscoped). + * Returns an invalid context if this guard is null. + * @return A SpanContext holding this guard's own span. + * @note This is NOT the thread's ambient context — use + * threadLocalContext() for that. */ [[nodiscard]] SpanContext - captureContext() const; + spanContext() const; + + /** + * Snapshot the calling thread's CURRENTLY-ACTIVE OTel context (the + * ambient span on this thread's context stack), for cross-thread + * propagation. This is the OpenTelemetry "capture current context" + * operation: capture -> pass to another thread -> childSpan(name, + * ctx). Unlike spanContext() (which refers to a specific guard's own + * span), this reflects whatever span is active on THIS thread right + * now, or an invalid context if none is active. + * @return A SpanContext holding the thread's current context. + */ + [[nodiscard]] static SpanContext + threadLocalContext(); // --- Attribute setters (explicit overloads, no OTel types) --------- @@ -453,7 +475,7 @@ public: * | + linkedSpan(name) : ScopedSpanGuard | * | + operator SpanGuard() && (handoff) | * | + setAttribute / setOk / setError / ... | - * | + captureContext() / discard() | + * | + spanContext() / discard() | * | + operator bool() | * +--------------------------------------------+ * | hides (pimpl) @@ -559,7 +581,7 @@ public: * Create a scoped child span parented to an explicit captured * context and activate it on this thread. * @param name Span name for the child. - * @param parentCtx Context captured via captureContext(). + * @param parentCtx Context captured via spanContext(). * @return A new scoped guard, or a null one if parentCtx is invalid. */ [[nodiscard]] static ScopedSpanGuard @@ -597,11 +619,16 @@ public: // --- Context capture ----------------------------------------------- /** - * Snapshot the current thread's OTel context for cross-thread use. - * @return An opaque SpanContext, or an invalid one if null guard. + * Return a SpanContext referring to the owned guard's own span, for + * use as an explicit parent in childSpan(name, ctx) across threads. + * Forwards to the owned SpanGuard's spanContext(). Returns an invalid + * context if this guard is null. + * @return A SpanContext holding the owned guard's own span. + * @note This is NOT the thread's ambient context — use + * SpanGuard::threadLocalContext() for that. */ [[nodiscard]] SpanContext - captureContext() const; + spanContext() const; // --- Forwarding methods (delegate to the owned SpanGuard) ---------- @@ -730,12 +757,18 @@ public: } [[nodiscard]] SpanContext - captureContext() const + spanContext() const { return {}; } // NOLINTEND(readability-convert-member-functions-to-static) + [[nodiscard]] static SpanContext + threadLocalContext() + { + return {}; + } + void setAttribute(std::string_view, std::string_view) { @@ -834,7 +867,7 @@ public: } [[nodiscard]] SpanContext - captureContext() const + spanContext() const { return {}; } diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index 38aa06c840..f915818d48 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -58,8 +58,8 @@ * * 3. Cross-thread context propagation: * @code - * // Thread A: capture the active context while span is in scope - * auto ctx = parentGuard.captureContext(); + * // Thread A: take a handle to the parent span's own context + * auto ctx = parentGuard.spanContext(); * * // Thread B: create child span with explicit parent * auto child = SpanGuard::childSpan("async.work", ctx); diff --git a/src/libxrpl/telemetry/SpanGuard.cpp b/src/libxrpl/telemetry/SpanGuard.cpp index 0b3a35d170..8c71e55046 100644 --- a/src/libxrpl/telemetry/SpanGuard.cpp +++ b/src/libxrpl/telemetry/SpanGuard.cpp @@ -292,20 +292,30 @@ SpanGuard::linkedSpan(std::string_view name, SpanContext const& linkCtx) // ===== Context capture ===================================================== SpanContext -SpanGuard::captureContext() const +SpanGuard::spanContext() const { if (!impl_ || !impl_->span) return {}; - // Capture THIS guard's own span, not the thread's ambient span. A + // Refer to THIS guard's own span, not the thread's ambient span. A // SpanGuard is unscoped (never pushed onto the thread-local stack), so // RuntimeContext::GetCurrent() would return an unrelated ambient span. - // Building the context from impl_->span makes captureContext() correct + // Building the context from impl_->span makes spanContext() correct // for every caller regardless of scoping, and lets childSpan(name, ctx) // parent explicitly to this span across threads. auto ctx = opentelemetry::context::Context{}.SetValue(otel_trace::kSpanKey, impl_->span); return SpanContext(std::make_shared(std::move(ctx))); } +SpanContext +SpanGuard::threadLocalContext() +{ + // Snapshot whatever span is currently active on THIS thread's context + // stack (the ambient context), independent of any guard. This is the + // OTel "capture current context" operation used for propagation. + auto ctx = opentelemetry::context::RuntimeContext::GetCurrent(); + return SpanContext(std::make_shared(std::move(ctx))); +} + // ===== Attribute setters =================================================== void @@ -520,9 +530,9 @@ operator SpanGuard() && // ===== ScopedSpanGuard context capture ===================================== SpanContext -ScopedSpanGuard::captureContext() const +ScopedSpanGuard::spanContext() const { - return impl_->guard.captureContext(); + return impl_->guard.spanContext(); } // ===== ScopedSpanGuard forwarding methods ================================== From 64a3c93188a7a59ec9cee3f20afa36cbd81723a0 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 21 Jul 2026 21:57:22 +0100 Subject: [PATCH 10/19] test(telemetry): SpanGuardFactory uses spanContext() (captureContext renamed) Co-Authored-By: Claude Opus 4.8 (1M context) --- src/tests/libxrpl/telemetry/SpanGuardFactory.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp b/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp index 4025a0b1ef..1447543667 100644 --- a/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp +++ b/src/tests/libxrpl/telemetry/SpanGuardFactory.cpp @@ -46,10 +46,10 @@ TEST(SpanGuardFactory, linked_span_null_when_no_context) EXPECT_FALSE(linked); } -TEST(SpanGuardFactory, capture_context_returns_invalid_on_null) +TEST(SpanGuardFactory, span_context_returns_invalid_on_null) { auto span = SpanGuard::span(TraceCategory::Rpc, "rpc", "ctx"); - auto ctx = span.captureContext(); + auto ctx = span.spanContext(); EXPECT_FALSE(ctx.isValid()); } From 7fc145ebd2150a30f0794b35e21e903b197391be Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 21 Jul 2026 22:09:36 +0100 Subject: [PATCH 11/19] refactor(telemetry): consensus + peer-receive spans use spanContext()/freshRoot, drop detach With unscoped SpanGuard + spanContext() (own-span capture), the consensus round/establish/accept sites just capture and drop the detach dance; openSpan + peer proposal/validation receive are thread-free handoffs (no detach); rootSpan->freshRoot. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/xrpld/app/consensus/RCLConsensus.cpp | 51 +++++++++---------- src/xrpld/consensus/Consensus.h | 49 ++++++++---------- src/xrpld/overlay/detail/PeerImp.cpp | 14 +++-- src/xrpld/telemetry/ConsensusReceiveTracing.h | 6 +-- 4 files changed, 52 insertions(+), 68 deletions(-) diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index f301c4608f..b10e709b67 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -230,8 +230,8 @@ RCLConsensus::Adaptor::share(RCLCxTx const& tx) void RCLConsensus::Adaptor::propose(RCLCxPeerPos::Proposal const& proposal) { - // Child of the round span via its captured context (roundSpan_ is detached, - // so it is no longer the thread's ambient parent). + // Child of the round span via its captured context (roundSpan_ is a + // thread-free SpanGuard, so parent explicitly via its context). auto span = telemetry::SpanGuard::childSpan( telemetry::consensus::span::proposalSend, roundSpanContext_); span.setAttribute( @@ -347,8 +347,8 @@ RCLConsensus::Adaptor::onClose( { namespace cs = telemetry::consensus::span; - // Child of the round span via its captured context (roundSpan_ is detached, - // so it is no longer the thread's ambient parent). + // Child of the round span via its captured context (roundSpan_ is a + // thread-free SpanGuard, so parent explicitly via its context). auto span = telemetry::SpanGuard::childSpan(cs::ledgerClose, roundSpanContext_); span.setAttribute(cs::attr::ledgerSeq, static_cast(ledger.ledger->header().seq) + 1); span.setAttribute(cs::attr::mode, toDisplayString(mode).c_str()); @@ -534,14 +534,12 @@ RCLConsensus::Adaptor::makeAcceptSpan(Result const& result) // "validation follows acceptance" causal model). if (*span) { - acceptSpanContext_ = span->captureContext(); - // The accept span is moved into the JtAccept worker (onAccept) and - // ended there. Detach its Scope now, on this thread, AFTER the capture - // above, so it is not popped on the wrong thread. accept.apply parents - // via acceptSpanContext_ (captured above), so no ambient child is - // orphaned on either the sync (onForceAccept) or async (onAccept) path. - // detachInPlace rebuilds the shared_ptr (move-assign is deleted). - span = telemetry::detachInPlace(std::move(span)); + // span is a thread-free SpanGuard handed to the JtAccept worker + // (onAccept), which ends it there. spanContext() captures the guard's + // own span, so accept.apply parents via acceptSpanContext_ regardless + // of which thread runs the sync (onForceAccept) or async (onAccept) + // path -- no scope work is needed. + acceptSpanContext_ = span->spanContext(); } return span; } @@ -584,11 +582,10 @@ RCLConsensus::Adaptor::doAccept( closeTimeCorrect = true; } - // Parent accept.apply via the captured accept context (acceptSpanContext_) - // rather than the accept guard's ambient Scope: the accept span is detached - // (its Scope no longer sits on this thread), so an explicit context is used - // for both the sync (onForceAccept) and async (onAccept) paths. Falls back - // to the round context if the accept span was null. + // Parent accept.apply via the captured accept context (acceptSpanContext_): + // the accept span is a thread-free SpanGuard, so an explicit context is + // used for both the sync (onForceAccept) and async (onAccept) paths. Falls + // back to the round context if the accept span was null. auto doAcceptSpan = acceptSpanContext_.isValid() ? telemetry::SpanGuard::childSpan(cs::acceptApply, acceptSpanContext_) : telemetry::SpanGuard::childSpan(cs::acceptApply, roundSpanContext_); @@ -1075,9 +1072,10 @@ RCLConsensus::Adaptor::onModeChange(ConsensusMode before, ConsensusMode after) { namespace cs = telemetry::consensus::span; - // Child of the round span via its captured context (roundSpan_ is detached, - // so it is no longer the thread's ambient parent). A mode change outside a - // round leaves roundSpanContext_ invalid, yielding a null guard (no-op). + // Child of the round span via its captured context (roundSpan_ is a + // thread-free SpanGuard, so parent explicitly via its context). A mode + // change outside a round leaves roundSpanContext_ invalid, yielding a null + // guard (no-op). auto span = telemetry::SpanGuard::childSpan(cs::modeChange, roundSpanContext_); span.setAttribute(cs::attr::modeOld, toDisplayString(before).c_str()); span.setAttribute(cs::attr::modeNew, toDisplayString(after).c_str()); @@ -1323,14 +1321,11 @@ RCLConsensus::Adaptor::startRoundTracing(RCLCxLedger const& prevLgr) roundSpan_->addEvent(cs::event::phaseOpen); - roundSpanContext_ = roundSpan_->captureContext(); - - // roundSpanContext_ (captured above) is the durable handle that child - // spans on other threads link to. The guard itself is reset() on a - // different worker than it was emplaced on, so detach its Scope now -- - // AFTER the context capture -- to avoid a wrong-thread Scope pop. - // detachInPlace re-emplaces it (move-assignment is deleted). - telemetry::detachInPlace(roundSpan_); + // roundSpanContext_ is the durable handle that child spans on other + // threads parent to. roundSpan_ is a thread-free SpanGuard that is + // reset() on a different worker than it was emplaced on, so spanContext() + // captures its own span and no scope work is needed. + roundSpanContext_ = roundSpan_->spanContext(); } std::optional diff --git a/src/xrpld/consensus/Consensus.h b/src/xrpld/consensus/Consensus.h index df81c7702a..86987d9bc0 100644 --- a/src/xrpld/consensus/Consensus.h +++ b/src/xrpld/consensus/Consensus.h @@ -650,24 +650,21 @@ private: /** * Span for the establish phase of consensus. * Created when the ledger closes and we enter phaseEstablish; - * cleared (ended) when consensus is reached. Stored detached (its Scope - * is stripped right after its context is captured) because it is emplaced - * and reset on different job workers. + * cleared (ended) when consensus is reached. A thread-free SpanGuard, + * emplaced and reset() on different job workers. */ std::optional establishSpan_; /** - * Captured context of establishSpan_, snapshotted while it was still - * scoped. Same-thread children (update_positions, check) build from this - * explicit context instead of the (now detached) ambient establish span. + * Captured context of establishSpan_ (its own span context). Children + * (update_positions, check) build from this explicit context. */ xrpl::telemetry::SpanContext establishSpanContext_; /** * Span for the open phase of consensus. * Created in startRoundInternal(); cleared (ended) in closeLedger(). - * Stored detached: emplaced and reset on different job workers; - * detached() prevents wrong-thread Scope pop. + * A thread-free SpanGuard, emplaced and reset() on different job workers. */ std::optional openSpan_; @@ -779,17 +776,14 @@ Consensus::startRoundInternal( // early-returns when establishSpan_ is populated). establishSpan_.reset(); establishSpanContext_ = telemetry::SpanContext{}; - // Child of the round span via its captured context: the round span is - // detached (no longer the thread's ambient parent), so parent phase.open - // explicitly under roundSpanContext_ rather than the ambient stack. An - // invalid round context (round span not yet created) yields a null guard. - // Detached in turn: emplaced here on one job worker and reset() on - // another, so strip the thread-local Scope to avoid a wrong-thread pop. - // No same-thread child span nests under openSpan_. + // Child of the round span via its captured context: parent phase.open + // explicitly under roundSpanContext_. An invalid round context (round span + // not yet created) yields a null guard. openSpan_ is a thread-free + // SpanGuard emplaced here on one job worker and reset() on another; no + // scope to strip. openSpan_.emplace( telemetry::SpanGuard::childSpan( - telemetry::consensus::span::phaseOpen, adaptor_.roundSpanContext()) - .detached()); + telemetry::consensus::span::phaseOpen, adaptor_.roundSpanContext())); // On the Recovered path, fire phase.open here because startRoundTracing // (which fires it for the Initial path) is not called on re-entry. On // the Initial path this is a no-op because the round span hasn't been @@ -1631,8 +1625,8 @@ Consensus::updateOurPositions(std::unique_ptr const& // NOLINTBEGIN(bugprone-unchecked-optional-access) assert above using namespace telemetry; // Child of the establish span via its captured context (establishSpan_ is - // detached, so it is no longer the thread's ambient parent). Null context - // (establish not started) yields a null guard, same as before. + // a thread-free SpanGuard, so parent explicitly via its context). Null + // context (establish not started) yields a null guard, same as before. auto span = SpanGuard::childSpan(consensus::span::updatePositions, establishSpanContext_); span.setAttribute( consensus::span::attr::convergePercent, static_cast(convergePercent_)); @@ -1841,7 +1835,7 @@ Consensus::haveConsensus(std::unique_ptr const& clog // NOLINTBEGIN(bugprone-unchecked-optional-access) assert above using namespace telemetry; // Child of the establish span via its captured context (establishSpan_ is - // detached, so it is no longer the thread's ambient parent). + // a thread-free SpanGuard, so parent explicitly via its context). auto span = SpanGuard::childSpan(consensus::span::check, establishSpanContext_); // CHECKME: should possibly count unacquired TX sets as disagreeing @@ -2101,22 +2095,19 @@ Consensus::startEstablishTracing() { if (establishSpan_) return; - // Child of the round span via its captured context: the round span is - // detached (no longer the thread's ambient parent), so parent establish + // Child of the round span via its captured context: parent establish // explicitly under roundSpanContext_. An invalid round context (round span // not yet created) yields a null guard. establishSpan_.emplace( telemetry::SpanGuard::childSpan( telemetry::consensus::span::establish, adaptor_.roundSpanContext())); - // Capture the establish context while the guard is still scoped, then - // detach. Same-thread children (update_positions, check) link to this - // captured context explicitly instead of relying on establishSpan_ being - // the ambient parent -- the guard is reset() on a different worker than it - // is emplaced on, so it must not keep a thread-local Scope. + // Capture the establish context; children (update_positions, check) parent + // to it explicitly via establishSpanContext_. establishSpan_ is a + // thread-free SpanGuard reset() on a different worker than it is emplaced + // on -- no scope work. if (*establishSpan_) { - establishSpanContext_ = establishSpan_->captureContext(); - telemetry::detachInPlace(establishSpan_); + establishSpanContext_ = establishSpan_->spanContext(); } } diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index ed64edb30d..699f8651be 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1849,10 +1849,9 @@ PeerImp::onMessage(std::shared_ptr const& m) // Create a receive span that links to the sender's trace context // (if propagated). shared_ptr keeps it alive across the job boundary. - // Detach the guard's Scope on this peer thread so it is not popped on the - // job worker thread (which would leak this thread's context stack). - auto span = - std::make_shared(telemetry::proposalReceiveSpan(set).detached()); + // The receive span is a thread-free SpanGuard handed to the job worker; + // no scope to strip. + auto span = std::make_shared(telemetry::proposalReceiveSpan(set)); span->setAttribute(telemetry::consensus::span::attr::proposalTrusted, isTrusted); span->setAttribute( telemetry::consensus::span::attr::round, static_cast(set.proposeseq())); @@ -2447,10 +2446,9 @@ PeerImp::onMessage(std::shared_ptr const& m) // Create a receive span that links to the sender's trace context // (if propagated). shared_ptr keeps it alive across the job boundary. - // Detach the guard's Scope on this peer thread so it is not popped on - // the job worker thread (which would leak this thread's context stack). - auto span = - std::make_shared(telemetry::validationReceiveSpan(*m).detached()); + // The receive span is a thread-free SpanGuard handed to the job worker; + // no scope to strip. + auto span = std::make_shared(telemetry::validationReceiveSpan(*m)); span->setAttribute(telemetry::consensus::span::attr::validationTrusted, isTrusted); if (val->isFieldPresent(sfLedgerSequence)) { diff --git a/src/xrpld/telemetry/ConsensusReceiveTracing.h b/src/xrpld/telemetry/ConsensusReceiveTracing.h index 83e6db0774..5b7582e13b 100644 --- a/src/xrpld/telemetry/ConsensusReceiveTracing.h +++ b/src/xrpld/telemetry/ConsensusReceiveTracing.h @@ -19,7 +19,7 @@ * +--- has trace_context? ----+ * | yes | no * v v - * SpanGuard::hashSpan() with SpanGuard::rootSpan() + * SpanGuard::hashSpan() with SpanGuard::freshRoot() * extracted parent context (fresh trace root) * * When XRPL_ENABLE_TELEMETRY is not defined, the functions return @@ -85,7 +85,7 @@ proposalReceiveSpan([[maybe_unused]] protocol::TMProposeSet const& msg) } #endif // No propagated context — start a fresh trace root (not an ambient child). - return SpanGuard::rootSpan( + return SpanGuard::freshRoot( TraceCategory::Consensus, seg::consensus, consensus::span::op::proposalReceive); } @@ -123,7 +123,7 @@ validationReceiveSpan([[maybe_unused]] protocol::TMValidation const& msg) } #endif // No propagated context — start a fresh trace root (not an ambient child). - return SpanGuard::rootSpan( + return SpanGuard::freshRoot( TraceCategory::Consensus, seg::consensus, consensus::span::op::validationReceive); } From 9725f5b48fde82f8a09e1769b930a245daf6fe07 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 22 Jul 2026 10:56:47 +0100 Subject: [PATCH 12/19] fix(telemetry): phase-6 peer.proposal/validation.receive use ScopedSpanGuard::freshRoot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These peer-level entry spans (introduced on phase-6) used the removed SpanGuard::rootSpan(). They are scoped on the peer thread and end there, so use ScopedSpanGuard::freshRoot() — a scoped fresh trace root that does not inherit any span left active on the peer thread. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/xrpld/overlay/detail/PeerImp.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index ec268fb570..9f4c03851b 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1758,7 +1758,8 @@ PeerImp::onMessage(std::shared_ptr const& m) using namespace telemetry; // root: inbound peer message entry point (kConsumer); must not inherit // any span left active on this peer thread. - auto span = SpanGuard::rootSpan(TraceCategory::Peer, seg::peer, peer_span::op::proposalReceive); + auto span = + ScopedSpanGuard::freshRoot(TraceCategory::Peer, seg::peer, peer_span::op::proposalReceive); span.setAttribute(peer_span::attr::peerId, static_cast(id_)); protocol::TMProposeSet const& set = *m; @@ -2381,8 +2382,8 @@ PeerImp::onMessage(std::shared_ptr const& m) using namespace telemetry; // root: inbound peer message entry point (kConsumer); must not inherit // any span left active on this peer thread. - auto valSpan = - SpanGuard::rootSpan(TraceCategory::Peer, seg::peer, peer_span::op::validationReceive); + auto valSpan = ScopedSpanGuard::freshRoot( + TraceCategory::Peer, seg::peer, peer_span::op::validationReceive); valSpan.setAttribute(peer_span::attr::peerId, static_cast(id_)); if (m->validation().size() < 50) From 7bdc14eee5d2843a3fc11eb00e5f9f15f9c608a5 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 22 Jul 2026 10:58:35 +0100 Subject: [PATCH 13/19] =?UTF-8?q?docs(telemetry):=2009-doc=20=E2=80=94=20d?= =?UTF-8?q?eterministic=20roots=20are=20true=20roots;=20rootSpan->freshRoo?= =?UTF-8?q?t?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Document that deterministic-trace_id spans (tx.* apply pipeline, tx.process, tx.receive, consensus.round) are now genuine trace roots with empty parent_span_id via the custom DeterministicIdGenerator, superseding the old synthetic-parent behavior that showed 'root span not yet received' in Tempo. Also update the fresh-root note: peer entry spans use ScopedSpanGuard::freshRoot(). Co-Authored-By: Claude Opus 4.8 (1M context) --- OpenTelemetryPlan/09-data-collection-reference.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/OpenTelemetryPlan/09-data-collection-reference.md b/OpenTelemetryPlan/09-data-collection-reference.md index d911cf88ff..710a5a4d17 100644 --- a/OpenTelemetryPlan/09-data-collection-reference.md +++ b/OpenTelemetryPlan/09-data-collection-reference.md @@ -117,6 +117,14 @@ under a single trace even though they run sequentially and often on different threads. A transaction that hard-fails preflight or preclaim never reaches the later spans — the `stage` attribute identifies where it stopped. +> **Deterministic roots are true roots.** Spans with a deterministic `trace_id` +> (the `tx.*` apply pipeline, `tx.process`, `tx.receive`, and `consensus.round`) +> are emitted as genuine trace roots with an empty `parent_span_id`. The chosen +> `trace_id` is injected through a custom `DeterministicIdGenerator` on the SDK's +> no-parent branch, so there is no synthetic placeholder parent — Tempo shows a +> clean root, not a "root span not yet received" warning. Cross-node correlation +> still works because every node derives the same `trace_id` from the shared hash. + **Where to find**: Tempo → TraceQL: `{resource.service.name="xrpld" && name=~"tx.process|tx.receive"}` or, for the apply pipeline: `{resource.service.name="xrpld" && name=~"tx.preflight|tx.preclaim|tx.transactor"}` @@ -212,7 +220,7 @@ Controlled by `trace_peer` in `[telemetry]` config. **Enabled by default** (high | `peer.validation.receive` | — | PeerImp.cpp | Validation message received from peer | A `—` parent means the span is a fresh trace root (`kConsumer`): it is started -via `SpanGuard::rootSpan()` at the inbound-message entry point and never +via `ScopedSpanGuard::freshRoot()` at the inbound-message entry point and never inherits an ambient span left active on the peer thread, so it does not nest under an unrelated transaction's trace. From b1f1e30450e8d088ba0957feab5a82f8728097bf Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 22 Jul 2026 13:10:21 +0100 Subject: [PATCH 14/19] fix(telemetry): scope pathfind.request/compute so children nest pathfind.request (PathFind.cpp) and pathfind.compute (PathRequest.cpp doUpdate) were left as unscoped SpanGuard after the type split, so pathfind.compute and pathfind.discover no longer nested under them. Both handlers run synchronously with no coroutine yield, so ScopedSpanGuard is safe and restores the request -> compute -> discover sub-tree. rpc.command.* stays unscoped (callMethod wraps doRipplePathFind which holds across a yield), so pathfind.request parents to rpc.process. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/xrpld/rpc/detail/PathRequest.cpp | 4 +++- src/xrpld/rpc/handlers/orderbook/PathFind.cpp | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/xrpld/rpc/detail/PathRequest.cpp b/src/xrpld/rpc/detail/PathRequest.cpp index 4fb11ab10c..28a6ad3c5c 100644 --- a/src/xrpld/rpc/detail/PathRequest.cpp +++ b/src/xrpld/rpc/detail/PathRequest.cpp @@ -739,7 +739,9 @@ PathRequest::doUpdate( { using namespace std::chrono; using namespace telemetry; - auto span = SpanGuard::span( + // Scoped so pathfind.discover (created synchronously below via findPaths) + // nests under it. doUpdate does not yield, so scoping is safe. + auto span = ScopedSpanGuard( TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::compute); span.setAttribute(pathfind_span::attr::fast, fast); span.setAttribute(pathfind_span::attr::destCurrency, to_string(saDstAmount_.asset()).c_str()); diff --git a/src/xrpld/rpc/handlers/orderbook/PathFind.cpp b/src/xrpld/rpc/handlers/orderbook/PathFind.cpp index f04756be15..bf7fdfe264 100644 --- a/src/xrpld/rpc/handlers/orderbook/PathFind.cpp +++ b/src/xrpld/rpc/handlers/orderbook/PathFind.cpp @@ -19,7 +19,9 @@ json::Value doPathFind(RPC::JsonContext& context) { using namespace telemetry; - auto span = SpanGuard::span( + // Scoped so pathfind.compute/discover (created synchronously below on this + // thread) nest under it. doPathFind does not yield, so scoping is safe. + auto span = ScopedSpanGuard( TraceCategory::Rpc, pathfind_span::prefix::pathfind, pathfind_span::op::request); // Addresses are hashed before emission for privacy. if (auto const& src = context.params[jss::source_account]; src.isString()) From beb5b2dddc1429669f9bfe91e13ecaf5cc2105dc Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 22 Jul 2026 13:11:53 +0100 Subject: [PATCH 15/19] fix(telemetry): scope ledger.build so tx.apply nests under it ledger.build was left as unscoped SpanGuard after the type split, so tx.apply (created synchronously during applyTxs on the same JtAccept worker) no longer nested under it. buildLedgerImpl runs synchronously with no yield, so ScopedSpanGuard is safe and restores the ledger.build -> tx.apply edge. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/xrpld/app/ledger/detail/BuildLedger.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/xrpld/app/ledger/detail/BuildLedger.cpp b/src/xrpld/app/ledger/detail/BuildLedger.cpp index d0e8967c09..482721bf6a 100644 --- a/src/xrpld/app/ledger/detail/BuildLedger.cpp +++ b/src/xrpld/app/ledger/detail/BuildLedger.cpp @@ -50,7 +50,9 @@ buildLedgerImpl( ApplyTxs&& applyTxs) { using namespace telemetry; - auto buildSpan = SpanGuard::span(TraceCategory::Ledger, seg::ledger, ledger_span::op::build); + // Scoped so tx.apply (created synchronously below during applyTxs on this + // thread) nests under it. buildLedgerImpl runs synchronously with no yield. + auto buildSpan = ScopedSpanGuard(TraceCategory::Ledger, seg::ledger, ledger_span::op::build); auto built = std::make_shared(*parent, closeTime); From 2f627e8e8e0b64e9b4dedd906b87df8eb5a7702f Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 22 Jul 2026 13:12:55 +0100 Subject: [PATCH 16/19] =?UTF-8?q?docs(telemetry):=2009-doc=20=E2=80=94=20p?= =?UTF-8?q?athfind.request=20parents=20to=20rpc.process,=20not=20rpc.comma?= =?UTF-8?q?nd?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reflects the C1 fix: rpc.command.* stays unscoped (its dispatch wraps doRipplePathFind which yields), so pathfind.request nests under rpc.process. The request -> compute -> discover sub-tree nests correctly via ScopedSpanGuard. Co-Authored-By: Claude Opus 4.8 (1M context) --- OpenTelemetryPlan/09-data-collection-reference.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/OpenTelemetryPlan/09-data-collection-reference.md b/OpenTelemetryPlan/09-data-collection-reference.md index 710a5a4d17..87f6e97666 100644 --- a/OpenTelemetryPlan/09-data-collection-reference.md +++ b/OpenTelemetryPlan/09-data-collection-reference.md @@ -136,12 +136,19 @@ Controlled by `trace_rpc=1` in `[telemetry]` config (pathfinding spans fire with | Span Name | Parent | Source File | Description | | --------------------- | ------------------ | ---------------- | -------------------------------------------------------- | -| `pathfind.request` | `rpc.command.*` | PathRequests.cpp | RPC entry for path_find / ripple_path_find | +| `pathfind.request` | `rpc.process` | PathFind.cpp | RPC entry for path_find (doPathFind) | | `pathfind.compute` | `pathfind.request` | PathRequest.cpp | Single path computation (doUpdate) | | `pathfind.update_all` | — | PathRequests.cpp | Async recomputation of all active path requests on close | | `pathfind.discover` | `pathfind.compute` | Pathfinder.cpp | Graph exploration phase (Pathfinder::find) | | `pathfind.rank` | `pathfind.compute` | Pathfinder.cpp | Path ranking and selection phase | +> **Note**: `pathfind.request` parents to `rpc.process`, not `rpc.command.*`. +> `rpc.command.*` is intentionally unscoped: its generic dispatch (`callMethod`) +> also wraps `doRipplePathFind`, whose span is held across a coroutine +> `yield()` — scoping it would risk a wrong-thread scope pop on resume. The +> `pathfind.request → compute → discover` sub-tree nests correctly because those +> handlers run synchronously (no yield) and use `ScopedSpanGuard`. + **Where to find**: Tempo → TraceQL: `{resource.service.name="xrpld" && name=~"pathfind.*"}` **Grafana dashboard**: _RPC & Pathfinding (StatsD)_ (`xrpld-statsd-rpc`) for StatsD timers; span-derived metrics via _RPC Performance_ (`rpc-performance`) From 5150159ca2165ce62223066781bb8a1240e84252 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 22 Jul 2026 13:30:50 +0100 Subject: [PATCH 17/19] refactor(telemetry): ledger.acquire uses thread-free SpanGuard (no detach) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit acquireSpan_ is emplaced on the acquiring thread and reset() on a JtLedgerData worker. SpanGuard is now thread-free (owns no thread-local Scope), so it can be created here and destroyed on the worker with no scope to strip — dropping the .detached() call the old scoped SpanGuard required. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/xrpld/app/ledger/detail/InboundLedger.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index 84f77e5d95..bb0b2ee8e0 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -103,12 +103,11 @@ InboundLedger::init(ScopedLockType& collectionLock) // observable. Finalized in done() with the outcome and timeout count. { using namespace telemetry; - // Detach the Scope: this guard is emplaced here but reset() on a - // JtLedgerData worker thread. Detaching strips the thread-local Scope - // so its destruction there does not corrupt this thread's context stack. + // acquireSpan_ is emplaced here but reset() on a JtLedgerData worker + // thread. A SpanGuard is thread-free (owns no thread-local Scope), so it + // can be created here and destroyed on the worker with no scope to strip. acquireSpan_.emplace( - SpanGuard::span(TraceCategory::Ledger, seg::ledger, ledger_span::op::acquire) - .detached()); + SpanGuard::span(TraceCategory::Ledger, seg::ledger, ledger_span::op::acquire)); if (*acquireSpan_) { acquireSpan_->setAttribute(ledger_span::attr::ledgerSeq, static_cast(seq_)); From b0b174b94d2254b0cc1099d7496a71c47b00cd23 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 22 Jul 2026 13:43:07 +0100 Subject: [PATCH 18/19] docs(telemetry): RCLConsensus roundSpan_ is thread-free, not detached The roundSpan_ / roundSpanContext_ comments still described the old 'detached, Scope stripped' model. roundSpan_ is now a thread-free SpanGuard (no Scope); children link via the captured roundSpanContext_. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/xrpld/app/consensus/RCLConsensus.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/xrpld/app/consensus/RCLConsensus.h b/src/xrpld/app/consensus/RCLConsensus.h index 39a584194c..c3bb1edea0 100644 --- a/src/xrpld/app/consensus/RCLConsensus.h +++ b/src/xrpld/app/consensus/RCLConsensus.h @@ -96,9 +96,9 @@ class RCLConsensus * the trace_id is derived from previousLedger.id() so that all * validators in the same round share the same trace_id. * - * Stored detached: its Scope is stripped right after roundSpanContext_ - * is captured, because it is emplaced and reset on different job - * workers. Child spans link via roundSpanContext_, not the ambient span. + * Thread-free: a SpanGuard owns no thread-local Scope, so it can be + * emplaced and reset on different job workers safely. Child spans link + * via roundSpanContext_ (captured once), not an ambient parent span. */ std::optional roundSpan_; @@ -290,10 +290,10 @@ class RCLConsensus * * The generic engine uses this to parent the phase spans it owns * (consensus.phase.open, consensus.establish) under the round span - * without touching roundSpan_ across threads. roundSpan_ is detached - * after this context is captured, so it is no longer the thread's - * ambient parent; child spans must link via this context. Returns an - * invalid context before the round span is created. + * without touching roundSpan_ across threads. roundSpan_ is a + * thread-free SpanGuard (never an ambient parent), so child spans must + * link via this captured context. Returns an invalid context before the + * round span is created. * * @return The round span's captured context, or an invalid context. */ From d64b744540282eb578dc3f9616a8beeb79e32519 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 22 Jul 2026 13:43:12 +0100 Subject: [PATCH 19/19] docs(telemetry): InboundLedger acquireSpan_ is thread-free, not detached Co-Authored-By: Claude Opus 4.8 (1M context) --- src/xrpld/app/ledger/InboundLedger.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/xrpld/app/ledger/InboundLedger.h b/src/xrpld/app/ledger/InboundLedger.h index f5009d4b21..b7c61f9bf0 100644 --- a/src/xrpld/app/ledger/InboundLedger.h +++ b/src/xrpld/app/ledger/InboundLedger.h @@ -201,10 +201,9 @@ private: * with the outcome (complete/failed), timeout count, and peer count. * Gives operators visibility into back-fill / fork-recovery cost, which * previously emitted no span or metric. - * Stored detached: emplaced by the acquiring thread, reset on a - * JtLedgerData worker. detached() strips the thread-local Scope so the - * guard can be destroyed on the worker without corrupting the origin - * thread's context stack. + * Thread-free: emplaced by the acquiring thread, reset on a JtLedgerData + * worker. A SpanGuard owns no thread-local Scope, so it can be destroyed + * on the worker without corrupting the origin thread's context stack. */ std::optional acquireSpan_; };