From f1b7104bb82fa32dd7bb13ed3ae1281b96454bfd Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 22 Jul 2026 16:56:16 +0100 Subject: [PATCH 1/7] feat(telemetry): coroutine-aware OTel context storage Backs the OTel active-context stack with xrpl::LocalValue so the ambient context follows a JobQueue::Coro across yield/resume. Not yet installed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../scripts/levelization/results/ordering.txt | 1 + .../xrpl/telemetry/CoroAwareContextStorage.h | 121 ++++++++++++++++++ .../telemetry/CoroAwareContextStorage.cpp | 68 ++++++++++ 3 files changed, 190 insertions(+) create mode 100644 include/xrpl/telemetry/CoroAwareContextStorage.h create mode 100644 src/libxrpl/telemetry/CoroAwareContextStorage.cpp diff --git a/.github/scripts/levelization/results/ordering.txt b/.github/scripts/levelization/results/ordering.txt index b38c125c7c..9538b32802 100644 --- a/.github/scripts/levelization/results/ordering.txt +++ b/.github/scripts/levelization/results/ordering.txt @@ -243,6 +243,7 @@ xrpl.server > xrpl.resource xrpl.shamap > xrpl.basics xrpl.shamap > xrpl.nodestore xrpl.shamap > xrpl.protocol +xrpl.telemetry > xrpl.basics xrpl.telemetry > xrpl.config xrpl.tx > xrpl.basics xrpl.tx > xrpl.core diff --git a/include/xrpl/telemetry/CoroAwareContextStorage.h b/include/xrpl/telemetry/CoroAwareContextStorage.h new file mode 100644 index 0000000000..ee982ace6c --- /dev/null +++ b/include/xrpl/telemetry/CoroAwareContextStorage.h @@ -0,0 +1,121 @@ +#pragma once + +/** + * A coroutine-aware OpenTelemetry runtime-context storage. + * + * OpenTelemetry's default ThreadLocalContextStorage keeps the active-context + * stack in a plain static thread_local, so the ambient span does NOT follow a + * JobQueue::Coro across yield/resume — a scope pushed on one worker is stranded + * when the coroutine resumes on another. This storage instead keeps the stack + * in an xrpl::LocalValue, which JobQueue::Coro::resume() swaps in and out with + * the coroutine (see Coro.ipp). The active context therefore rides the + * coroutine: scopes held across yield are safe, and per-line log-trace + * correlation (Log.cpp reads RuntimeContext::GetCurrent()) is retained. + * + * Off a coroutine, LocalValue transparently provides a per-thread store, so + * behaviour is identical to the default thread-local storage. + * + * +-------------------------------------------------+ + * | CoroAwareContextStorage | + * | (opentelemetry RuntimeContextStorage) | + * +-------------------------------------------------+ + * | - stack_ : LocalValue> | + * +-------------------------------------------------+ + * | + GetCurrent() : Context | + * | + Attach(ctx) : unique_ptr | + * | + Detach(token): bool | + * +-------------------------------------------------+ + * | backed by (coro-aware) + * +------------------------+ + * | xrpl::LocalValue store | + * | (swapped by Coro) | + * +------------------------+ + * + * Install once at telemetry start via + * opentelemetry::context::RuntimeContext::SetRuntimeContextStorage(), BEFORE + * any span is created (SDK requirement). + * + * @note Thread-safety: each thread/coroutine sees its own LocalValue store, so + * the stack is never shared across threads — no locking needed. The storage + * object itself is stateless apart from the LocalValue handle. + * @note Known limitation: install once, before the first span; resetting the + * storage while spans exist is undefined behaviour (SDK). A span created on a + * raw worker thread and later moved onto a coroutine does not retro-attach to + * the coroutine's store — not a pattern here (spans are created inside their + * own coro/job body). + * + * Example 1 — install at telemetry start (primary use): + * @code + * using opentelemetry::context::RuntimeContext; + * RuntimeContext::SetRuntimeContextStorage( + * opentelemetry::nostd::shared_ptr< + * opentelemetry::context::RuntimeContextStorage>( + * new xrpl::telemetry::CoroAwareContextStorage())); + * @endcode + * + * Example 2 — a scope held across a coroutine yield stays correct: + * @code + * // Inside a JobQueue::Coro body: + * auto span = ScopedSpanGuard(TraceCategory::Rpc, "rpc", "process"); + * context.coro->yield(); // may resume on another worker + * // span is still the ambient context here — the storage rode the coro. + * @endcode + * + * Example 3 — edge case: off a coroutine, behaves like thread-local storage: + * @code + * // On a plain worker thread (no coro): each thread has its own stack, + * // identical to opentelemetry's default ThreadLocalContextStorage. + * auto span = ScopedSpanGuard(TraceCategory::Ledger, "ledger", "build"); + * @endcode + */ + +#ifdef XRPL_ENABLE_TELEMETRY + +#include + +#include +#include +#include + +#include + +namespace xrpl::telemetry { + +class CoroAwareContextStorage : public opentelemetry::context::RuntimeContextStorage +{ +public: + CoroAwareContextStorage() = default; + + /** + * @return the current (top-of-stack) context for this coro/thread. + */ + opentelemetry::context::Context + GetCurrent() noexcept override; + + /** + * Push a context frame onto this coro/thread's stack. + * @param context the context to make current. + * @return a token that Detach() uses to pop back to the prior frame. + */ + opentelemetry::nostd::unique_ptr + Attach(opentelemetry::context::Context const& context) noexcept override; + + /** + * Pop the stack back through the frame the token refers to. + * @param token a token returned by Attach(). + * @return true if the frame was found and detached. + */ + bool + Detach(opentelemetry::context::Token& token) noexcept override; + +private: + /** + * The active-context stack, stored coro-locally. LocalValue hands back the + * stack for whichever store (coro or thread) is currently installed. + */ + LocalValue> stack_; +}; + +} // namespace xrpl::telemetry + +#endif // XRPL_ENABLE_TELEMETRY diff --git a/src/libxrpl/telemetry/CoroAwareContextStorage.cpp b/src/libxrpl/telemetry/CoroAwareContextStorage.cpp new file mode 100644 index 0000000000..acd0dc77c3 --- /dev/null +++ b/src/libxrpl/telemetry/CoroAwareContextStorage.cpp @@ -0,0 +1,68 @@ +/** + * Implementation of CoroAwareContextStorage. + * + * Mirrors opentelemetry's ThreadLocalContextStorage stack semantics, but the + * stack lives in an xrpl::LocalValue so it follows a JobQueue::Coro across + * yield/resume. All OpenTelemetry types stay confined to this translation unit. + * + * @see CoroAwareContextStorage (CoroAwareContextStorage.h) + */ + +#ifdef XRPL_ENABLE_TELEMETRY + +#include + +#include +#include +#include + +namespace xrpl::telemetry { + +opentelemetry::context::Context +CoroAwareContextStorage::GetCurrent() noexcept +{ + auto& stack = *stack_; + return stack.empty() ? opentelemetry::context::Context{} : stack.back(); +} + +opentelemetry::nostd::unique_ptr +CoroAwareContextStorage::Attach(opentelemetry::context::Context const& context) noexcept +{ + stack_->push_back(context); + return CreateToken(context); +} + +bool +CoroAwareContextStorage::Detach(opentelemetry::context::Token& token) noexcept +{ + auto& stack = *stack_; + // Fast path: the token's frame is on top (the common LIFO case). + if (!stack.empty() && token == stack.back()) + { + stack.pop_back(); + return true; + } + // Fallback: token not on top — verify it is present, then pop down to and + // including it (mirrors ThreadLocalContextStorage::Detach; also detaches + // any child frames left above it). + bool found = false; + for (auto const& frame : stack) + { + if (token == frame) + { + found = true; + break; + } + } + if (!found) + return false; + while (!stack.empty() && !(token == stack.back())) + stack.pop_back(); + if (!stack.empty()) + stack.pop_back(); + return true; +} + +} // namespace xrpl::telemetry + +#endif // XRPL_ENABLE_TELEMETRY From 9fa58067fa34c66a100556f4b3c4a6077ff35653 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 22 Jul 2026 17:12:11 +0100 Subject: [PATCH 2/7] feat(telemetry): install coroutine-aware context storage at start Install CoroAwareContextStorage as the process-wide OTel runtime context storage in TelemetryImpl::start(), before SetTracerProvider and before any span is created, so the ambient context follows JobQueue coroutines across yield/resume. Hold it in a member for the process lifetime; not reset in stop() because resetting while spans may exist is SDK undefined behaviour. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/libxrpl/telemetry/Telemetry.cpp | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/libxrpl/telemetry/Telemetry.cpp b/src/libxrpl/telemetry/Telemetry.cpp index a7ba3a4a11..8076146a9c 100644 --- a/src/libxrpl/telemetry/Telemetry.cpp +++ b/src/libxrpl/telemetry/Telemetry.cpp @@ -20,10 +20,12 @@ #include #include +#include #include #include #include +#include #include #include #include @@ -263,6 +265,13 @@ class TelemetryImpl : public Telemetry */ std::shared_ptr sdkProvider_; + /** + * Coroutine-aware runtime-context storage, installed globally so the OTel + * ambient context follows JobQueue coroutines. Held for the process + * lifetime because it must outlive every span (SDK requirement). + */ + opentelemetry::nostd::shared_ptr contextStorage_; + public: TelemetryImpl(Setup setup, beast::Journal journal) : setup_(std::move(setup)), journal_(journal) { @@ -332,6 +341,18 @@ public: std::move(sampler), std::make_unique()); + // Install coroutine-aware context storage BEFORE any span is created + // so the OTel ambient context follows JobQueue coroutines across + // yield/resume (fixes wrong-thread scope pop; keeps log-trace + // correlation). Must precede SetTracerProvider and the first span. + // Not reset in stop(): resetting the storage while spans may still + // exist is undefined behaviour (SDK), and by stop() all spans are + // gone, so the storage is simply left installed for process lifetime. + contextStorage_ = + opentelemetry::nostd::shared_ptr( + new CoroAwareContextStorage()); + opentelemetry::context::RuntimeContext::SetRuntimeContextStorage(contextStorage_); + // Set as global provider trace_api::Provider::SetTracerProvider( opentelemetry::nostd::shared_ptr(sdkProvider_)); From 7dbbf95c72cb56eebd5f00f69b0246363d3a0bbd Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 22 Jul 2026 18:52:48 +0100 Subject: [PATCH 3/7] refactor(telemetry): ScopedSpanGuard asserts same context store, not thread Coroutine-aware storage lets a scope resume on another worker within the same coroutine store; store-identity is the correct pop-safety invariant. Same-store equals same-thread for synchronous code, so no safety is lost. Co-Authored-By: Claude Opus 4.8 (1M context) --- include/xrpl/telemetry/SpanGuard.h | 47 ++++++++------- src/libxrpl/telemetry/SpanGuard.cpp | 88 ++++++++++++++++++----------- 2 files changed, 82 insertions(+), 53 deletions(-) diff --git a/include/xrpl/telemetry/SpanGuard.h b/include/xrpl/telemetry/SpanGuard.h index 341dc9fe71..d0abaa3769 100644 --- a/include/xrpl/telemetry/SpanGuard.h +++ b/include/xrpl/telemetry/SpanGuard.h @@ -11,9 +11,11 @@ * 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 + * pushes the span onto the constructing context store's + * stack. Store-bound: it must be created and destroyed + * while the same LocalValue context store is active (the + * same thread, or the same JobQueue coroutine even if it + * resumes on another worker). Non-copyable and * non-movable. * * Both wrap all OpenTelemetry types behind the pimpl idiom so no @@ -446,26 +448,26 @@ public: }; /** - * RAII guard that activates a span on the current thread (scoped). + * RAII guard that activates a span on the current context store (scoped). * * 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 + * pushes the span onto the active context store's stack for the + * guard's lifetime, so child spans created under that store 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(...)`. * - * When the span must outlive the current thread (e.g. handed to a job + * When the span must outlive the current store (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 + * that pops the Scope eagerly under the constructing store and yields a * thread-free guard. * * ScopedSpanGuard dependency diagram: * * +--------------------------------------------+ * | ScopedSpanGuard | - * | (scoped, thread-bound) | + * | (scoped, store-bound) | * +--------------------------------------------+ * | - impl_ : unique_ptr (pimpl) | * +--------------------------------------------+ @@ -483,7 +485,7 @@ public: * | | * +----------+ +-----------------------------+ * | SpanGuard| | optional | - * | (span) | | (OTel, thread-bound; | + * | (span) | | (OTel, store-bound; | * | | | present : span active) | * +----------+ +-----------------------------+ * @@ -513,11 +515,14 @@ public: * @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. + * destroyed while the same LocalValue context store is active — its + * Scope binds to that store's context stack. This means the same + * thread, or the same JobQueue coroutine even if it resumes on another + * worker (coroutine-aware storage keeps the same store across the + * resume). `operator SpanGuard() &&` and the destructor must both run + * under the constructing store; 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 @@ -609,9 +614,10 @@ public: // --- 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. + * Pop the active Scope under the constructing context store and yield + * the span as a thread-free SpanGuard. Use to move a span into a job or + * onto another thread. Must be called while the constructing context + * store is active. * @return The unscoped SpanGuard that now owns the span. */ operator SpanGuard() &&; @@ -692,8 +698,9 @@ public: /** * 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. + * scope right away (under the constructing context store) and discards + * the span. After discard() the guard is inert and its destructor is a + * no-op. */ void discard(); diff --git a/src/libxrpl/telemetry/SpanGuard.cpp b/src/libxrpl/telemetry/SpanGuard.cpp index 8c71e55046..744f93a35c 100644 --- a/src/libxrpl/telemetry/SpanGuard.cpp +++ b/src/libxrpl/telemetry/SpanGuard.cpp @@ -10,10 +10,12 @@ * 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 + * Scope. The Scope pushes the span onto the constructing context + * store's 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. + * store-bound: construct and destroy while the same LocalValue + * context store is active (the same thread, or the same JobQueue + * coroutine even if it resumes on another worker). * * Static factory methods access the global Telemetry instance via * Telemetry::getInstance(), check whether the requested TraceCategory @@ -28,6 +30,7 @@ #include +#include #include #include #include @@ -48,7 +51,6 @@ #include #include #include -#include #include #include @@ -428,21 +430,35 @@ struct ScopedSpanGuard::ScopedImpl 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. + * Active OTel Scope binding the span to the active context store's + * stack. Present while the guard is active; reset() (via operator + * SpanGuard) pops it eagerly under the constructing store. 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. + * Identity of the LocalValue store the Scope was pushed onto (the + * coroutine's store on a coro, else the thread's own store). Popping + * the Scope (via ~Scope or reset()) must happen while the SAME store + * is active, else a different stack is corrupted. Coroutine-aware + * storage lets a coro legitimately resume on another worker while + * keeping the same store, so store-identity — not thread-id — is the + * correct invariant. Checked in the destructor and operator + * SpanGuard() to turn a silent cross-store pop into an assertion + * failure in debug/test/fuzzing builds. + * + * Assigned in the constructor body AFTER the Scope push, not as a + * member initializer: the push is the first LocalValue touch on a + * fresh thread for a root or explicit-parent span (the OTel SDK skips + * the ambient-context read in those cases), so it materializes the + * store. Capturing before the push would record nullptr while the + * destructor sees the now-materialized store. For a null guard no + * Scope is pushed and the assertions short-circuit, so this holds + * whatever store is active then. */ - std::thread::id owner{std::this_thread::get_id()}; + void const* owner = nullptr; /** * Wrap a SpanGuard and activate its span on this thread. If the @@ -454,6 +470,9 @@ struct ScopedSpanGuard::ScopedImpl { if (guard) scope.emplace(guard.impl_->span); + // Capture after the push so `owner` names the store the Scope + // actually lives on, even when the push materialized it. + owner = detail::getLocalValues().get(); } }; @@ -466,13 +485,14 @@ ScopedSpanGuard::ScopedSpanGuard(SpanGuard&& 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. + // A live Scope must be popped while its constructing context store is + // active; destroying the guard under a different store corrupts that + // store'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"); + !impl_ || !impl_->scope.has_value() || impl_->owner == detail::getLocalValues().get(), + "xrpl::telemetry::ScopedSpanGuard::~ScopedSpanGuard : destroyed on the " + "constructing context store"); } // ===== ScopedSpanGuard factory methods ===================================== @@ -517,13 +537,14 @@ ScopedSpanGuard::linkedSpan(std::string_view name, SpanContext const& linkCtx) ScopedSpanGuard:: operator SpanGuard() && { - // The scope must be popped on the thread that pushed it; handing off - // elsewhere would pop the wrong stack. + // The scope must be popped while its constructing context store is + // active; handing off under a different store would pop the wrong stack. + // A null guard holds no Scope, so the check is skipped. 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 + !impl_->scope.has_value() || impl_->owner == detail::getLocalValues().get(), + "xrpl::telemetry::ScopedSpanGuard::operator SpanGuard : handoff on the " + "constructing context store"); + impl_->scope.reset(); // eager pop, on the origin store return std::move(impl_->guard); } @@ -594,14 +615,15 @@ ScopedSpanGuard::recordException(std::exception const& 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. + // A live Scope must be popped while its constructing context store is + // active; discarding under a different store corrupts that store'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_ || !impl_->scope.has_value() || impl_->owner == detail::getLocalValues().get(), + "xrpl::telemetry::ScopedSpanGuard::discard : discarded on the " + "constructing context store"); + // Pop the scope first (under the constructing store) so no span stays + // active on the stack, then discard the span via the owned guard. impl_->scope.reset(); impl_->guard.discard(); } From e3c724423f34a603aee12dc89eb2f99decfb3888 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 22 Jul 2026 19:32:18 +0100 Subject: [PATCH 4/7] feat(telemetry): add ScopedActivation for non-owning span activation Add SpanGuard::activate() returning a ScopedActivation RAII helper that activates an already-owned span (from a thread-free SpanGuard) as the current context WITHOUT taking ownership. The activation pushes the span onto the current LocalValue context store on construction and pops it on destruction; it never ends the span (its owning SpanGuard does). This lets a job-handoff span be made ambient for the duration of a synchronous, non-yielding worker body so log lines there carry the span's trace_id. Non-copyable and non-movable, mirroring ScopedSpanGuard. owner is captured after the Scope push via declaration-order member initialization (scope declared before owner), matching the A3 capture-after-materialization invariant. A #else no-op stub keeps the API zero-overhead when telemetry is compiled out. Co-Authored-By: Claude Opus 4.8 (1M context) --- include/xrpl/telemetry/SpanGuard.h | 120 ++++++++++++++++++++++++++++ src/libxrpl/telemetry/SpanGuard.cpp | 58 ++++++++++++++ 2 files changed, 178 insertions(+) diff --git a/include/xrpl/telemetry/SpanGuard.h b/include/xrpl/telemetry/SpanGuard.h index d0abaa3769..d4543e145b 100644 --- a/include/xrpl/telemetry/SpanGuard.h +++ b/include/xrpl/telemetry/SpanGuard.h @@ -42,6 +42,7 @@ * | + addEvent(name) | * | + recordException(e) | * | + discard() | + * | + activate() : ScopedActivation | * | + operator bool() | * +--------------------------------------------+ * | hides (pimpl) @@ -233,6 +234,11 @@ public: // --------------------------------------------------------------------------- #ifdef XRPL_ENABLE_TELEMETRY +// SpanGuard::activate() returns a ScopedActivation by value; the full class is +// defined after ScopedSpanGuard below. Forward-declared here so the return type +// is named before its definition. +class ScopedActivation; + /** * RAII owner of an OTel span (unscoped, thread-free). * @@ -440,6 +446,17 @@ public: void discard(); + // --- Activation (non-owning) --------------------------------------- + + /** + * Activate this guard's span as the current context for the returned + * activation's lifetime, without transferring ownership. Returns a no-op + * activation if this guard is null. See ScopedActivation. + * @return an RAII activation; drop it to restore the prior context. + */ + [[nodiscard]] ScopedActivation + activate() const; + /** * @return true if this guard holds an active span. */ @@ -712,11 +729,106 @@ public: operator bool() const; }; +/** + * RAII activation of an already-owned span as the current context, WITHOUT + * taking ownership. Use to give a thread-free SpanGuard (e.g. one handed into + * a JobQueue job) ambient status for the duration of a synchronous, non-yielding + * worker body, so log lines emitted there carry the span's trace_id. The span + * is neither owned nor ended here — its owning SpanGuard still controls its + * lifetime. Non-copyable, non-movable; must be created and destroyed while the + * same context store is active (see ScopedSpanGuard). + * + * Dependency diagram: + * + * +---------------------------------------------+ + * | ScopedActivation | + * | (non-owning RAII span activation) | + * +---------------------------------------------+ + * | - impl_ : unique_ptr | + * | Impl { scope : otel::Scope; owner } | + * +---------------------------------------------+ + * | + (ctor) pushes span onto current store | + * | + (dtor) pops; does NOT end the span | + * +---------------------------------------------+ + * | activates (borrows, no ownership) + * +-----------+ ends the span + * | SpanGuard |----------------------------> (owner) + * +-----------+ + * + * @note Thread-safety: like ScopedSpanGuard, it pushes/pops the current + * LocalValue context store; construct and destroy it while the same store is + * active (asserted in debug). Must NOT outlive the SpanGuard whose span it + * activates. Intended as a short-lived stack local inside a worker body that + * runs to completion on one thread (no coro yield inside the activation). + * + * Example 1 — correlate a handed-off span's worker logs (primary use): + * @code + * // span is std::shared_ptr handed into this job body: + * auto activation = (span && *span) ? span->activate() : ScopedActivation{}; + * JLOG(j.info()) << "applied"; // carries span's trace_id + * // span is still owned/ended elsewhere; activation only pops here. + * @endcode + * + * Example 2 — edge case: a null guard yields a no-op activation: + * @code + * SpanGuard g; // null (telemetry off, or disabled category) + * auto a = g.activate(); // no-op; GetCurrent() unchanged + * @endcode + */ +class ScopedActivation +{ + struct Impl; + std::unique_ptr impl_; + friend class SpanGuard; + explicit ScopedActivation(std::unique_ptr impl); + +public: + /** + * Construct a null / no-op activation. Its destructor does nothing and + * leaves the current context unchanged. Returned by SpanGuard::activate() + * for a null guard. + */ + ScopedActivation(); + + /** + * Pop the activated span off the current context store, restoring the + * prior context. Does NOT end the span (the owning SpanGuard does that). + * A null activation is a no-op. + */ + ~ScopedActivation(); + + ScopedActivation(ScopedActivation&&) = delete; + ScopedActivation& + operator=(ScopedActivation&&) = delete; + ScopedActivation(ScopedActivation const&) = delete; + ScopedActivation& + operator=(ScopedActivation const&) = delete; +}; + // --------------------------------------------------------------------------- // No-op stub (all inline, zero overhead, no OTel dependency) // --------------------------------------------------------------------------- #else // XRPL_ENABLE_TELEMETRY not defined +/** + * No-op activation used when telemetry is disabled at compile time. Holds no + * state and does nothing on construction or destruction. Declared before + * SpanGuard so its inline activate() can return one by value. Non-copyable and + * non-movable to match the real ScopedActivation. + */ +class ScopedActivation +{ +public: + ScopedActivation() = default; + ~ScopedActivation() = default; + ScopedActivation(ScopedActivation&&) = delete; + ScopedActivation& + operator=(ScopedActivation&&) = delete; + ScopedActivation(ScopedActivation const&) = delete; + ScopedActivation& + operator=(ScopedActivation const&) = delete; +}; + class SpanGuard { public: @@ -818,6 +930,14 @@ public: { } + // NOLINTBEGIN(readability-convert-member-functions-to-static) + [[nodiscard]] ScopedActivation + activate() const + { + return {}; + } + // NOLINTEND(readability-convert-member-functions-to-static) + explicit operator bool() const { diff --git a/src/libxrpl/telemetry/SpanGuard.cpp b/src/libxrpl/telemetry/SpanGuard.cpp index 744f93a35c..c7b021c357 100644 --- a/src/libxrpl/telemetry/SpanGuard.cpp +++ b/src/libxrpl/telemetry/SpanGuard.cpp @@ -634,6 +634,64 @@ operator bool() const return impl_ && static_cast(impl_->guard); } +// ===== ScopedActivation ==================================================== + +struct ScopedActivation::Impl +{ + /** + * Active OTel Scope binding an externally-owned span to the current + * context store. Popped on destruction; never ends the span. + */ + otel_trace::Scope scope; + + /** + * Identity of the LocalValue store the Scope was pushed onto. The Scope + * must pop while the SAME store is active (see + * ScopedSpanGuard::ScopedImpl::owner). Initialized AFTER `scope` in the + * member init list: members initialize in declaration order, so the + * Scope's push (the first LocalValue touch on a fresh worker, which + * materializes the store) runs first, then this captures the + * now-materialized store. Capturing before the push would record + * nullptr while the destructor sees the materialized store. + */ + void const* owner; + + /** + * Push an externally-owned span onto the current context store. + * @param span The already-owned span to activate (not owned here). + */ + explicit Impl(opentelemetry::nostd::shared_ptr const& span) + : scope(span), owner(detail::getLocalValues().get()) + { + } +}; + +ScopedActivation::ScopedActivation() = default; + +ScopedActivation::ScopedActivation(std::unique_ptr impl) : impl_(std::move(impl)) +{ +} + +ScopedActivation::~ScopedActivation() +{ + // A live Scope must be popped while its constructing context store is + // active; destroying the activation under a different store corrupts + // that store's context stack. A null activation holds no Scope, so the + // check is skipped. + XRPL_ASSERT( + !impl_ || impl_->owner == detail::getLocalValues().get(), + "xrpl::telemetry::ScopedActivation::~ScopedActivation : destroyed on the " + "constructing context store"); +} + +ScopedActivation +SpanGuard::activate() const +{ + if (!impl_ || !impl_->span) + return {}; + return ScopedActivation(std::make_unique(impl_->span)); +} + } // namespace xrpl::telemetry #endif // XRPL_ENABLE_TELEMETRY From 47a29187fd0e4045b29fdff114600e52c57ece0e Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 22 Jul 2026 19:57:27 +0100 Subject: [PATCH 5/7] refactor(rpc): scope rpc.command spans; coro-aware storage makes RPC scopes yield-safe rpc.command.* becomes a scoped child so it nests under rpc.process, correlates its log lines, and parents pathfind.request. No RPC::Context plumbing needed. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/xrpld/rpc/detail/RPCHandler.cpp | 7 +++++-- src/xrpld/rpc/detail/ServerHandler.cpp | 5 ++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/xrpld/rpc/detail/RPCHandler.cpp b/src/xrpld/rpc/detail/RPCHandler.cpp index 7a9d85748a..27dacdc560 100644 --- a/src/xrpld/rpc/detail/RPCHandler.cpp +++ b/src/xrpld/rpc/detail/RPCHandler.cpp @@ -162,7 +162,10 @@ template Status callMethod(JsonContext& context, Method method, std::string const& name, Object& result) { - auto span = SpanGuard::span(TraceCategory::Rpc, rpc_span::prefix::command, name); + // Scoped so this command nests under rpc.process and becomes the ambient + // parent of any command-internal spans (e.g. pathfind.request). Coro-aware + // storage keeps the scope correct across doRipplePathFind's yield. + auto span = ScopedSpanGuard(TraceCategory::Rpc, rpc_span::prefix::command, name); span.setAttribute(rpc_span::attr::command, name.c_str()); span.setAttribute(rpc_span::attr::version, static_cast(context.apiVersion)); span.setAttribute( @@ -256,7 +259,7 @@ doCommand(RPC::JsonContext& context, json::Value& result) // registered handler names (plus "unknown") — see the helper for why // raw request input must not reach the telemetry pipeline. auto const cmdName = resolveCommandSpanName(context); - auto span = SpanGuard::span(TraceCategory::Rpc, rpc_span::prefix::command, cmdName); + auto span = ScopedSpanGuard(TraceCategory::Rpc, rpc_span::prefix::command, cmdName); span.setAttribute(rpc_span::attr::command, cmdName); span.setAttribute(rpc_span::attr::rpcStatus, rpc_span::val::error); span.setError(getErrorInfo(error).token.cStr()); diff --git a/src/xrpld/rpc/detail/ServerHandler.cpp b/src/xrpld/rpc/detail/ServerHandler.cpp index 9f221a3823..4abee4c79f 100644 --- a/src/xrpld/rpc/detail/ServerHandler.cpp +++ b/src/xrpld/rpc/detail/ServerHandler.cpp @@ -652,7 +652,10 @@ ServerHandler::processRequest( std::string_view forwardedFor, std::string_view user) { - // Scoped child: nests under the httpRequest span active on this thread. + // Scoped child of rpc.http_request. Safe to hold across the coroutine + // yield in doRipplePathFind: the coro-aware context storage moves this + // scope with the coroutine on resume (it is never stranded on a worker's + // thread-local stack), so nesting and log-trace correlation both hold. auto span = ScopedSpanGuard(TraceCategory::Rpc, rpc_span::prefix::rpc, rpc_span::op::process); auto rpcJ = app_.getJournal("RPC"); From 8192da52439bd400abbcd5fffd575810e0916b82 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 22 Jul 2026 21:35:27 +0100 Subject: [PATCH 6/7] test(telemetry): coro-store-swap + activate tests; revert pathfind.request to scoped Add two GTests on the SpanGuardScopeTest fixture and install a CoroAwareContextStorage so the tests exercise coro-aware ambient context: - scopedGuard_survives_localvalue_store_swap: a ScopedSpanGuard's scope is hidden when its LocalValue store is swapped out and visible again when swapped back in, and pops cleanly under the owning store. - activate_sets_ambient_without_owning: SpanGuard::activate() makes the span ambient for the activation's lifetime without ending it; the owning guard ends it exactly once. Fix the cross-store death test to match the store-identity assertion message ("constructing context store", not "constructing thread") after the A3 refactor; the fixture's storage install is what lets the assert fire at all. Revert RipplePathFind's pathfind.request from freshRoot SpanGuard back to ScopedSpanGuard: the coro-aware storage moves the scope with the coroutine across yield, so it nests under rpc.command and stays trace-correlated. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../libxrpl/telemetry/SpanGuardScope.cpp | 184 +++++++++++++++--- .../rpc/handlers/orderbook/RipplePathFind.cpp | 16 +- 2 files changed, 168 insertions(+), 32 deletions(-) diff --git a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp index 91999f2352..3fa35bd7c5 100644 --- a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp +++ b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp @@ -6,11 +6,12 @@ // 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. +// - ScopedSpanGuard is scoped and store-bound: it also pushes an OTel Scope +// so the span is the ambient parent on the constructing context store (the +// thread's own store, or a coroutine's store). Its `operator SpanGuard() &&` +// pops that Scope eagerly on the origin store and yields a thread-free +// SpanGuard; destroying it while a different store is active trips an +// owner-store 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. @@ -21,6 +22,8 @@ #ifdef XRPL_ENABLE_TELEMETRY +#include +#include #include #include #include @@ -252,6 +255,17 @@ makeTraceIdBytes() /** * Installs a TestTelemetry as the global instance for each test and clears it * afterwards so the singleton never dangles between cases. + * + * The fixture also installs a CoroAwareContextStorage as the process-global + * OTel runtime-context storage. That storage backs the ambient-context stack + * with an xrpl::LocalValue, which is what makes a ScopedSpanGuard's Scope live + * in the ACTIVE store (thread or coroutine) rather than a plain thread_local. + * The store-swap and activate() tests depend on this to observe the ambient + * context move with the store. + * + * @note SetRuntimeContextStorage is process-global. Re-installing it in every + * SetUp is idempotent (last writer wins) and safe because GTests run serially; + * a fresh storage per test keeps the coro/thread context stacks isolated. */ class SpanGuardScopeTest : public ::testing::Test { @@ -261,6 +275,13 @@ protected: { telemetry_ = std::make_unique(); Telemetry::setInstance(telemetry_.get()); + + // Back the OTel ambient context with an xrpl::LocalValue store so a + // scope follows the active store, not a bare thread_local. Installed + // before any span is created in the test body (SDK requirement). + storage_ = opentelemetry::nostd::shared_ptr( + new CoroAwareContextStorage()); + opentelemetry::context::RuntimeContext::SetRuntimeContextStorage(storage_); } void @@ -283,6 +304,13 @@ protected: * The in-memory Telemetry installed for the duration of a test. */ std::unique_ptr telemetry_; + + /** + * The coro-aware runtime-context storage installed for the test. Kept + * alive by the fixture so the process-global storage pointer stays valid + * for the test body; replaced each SetUp (see class note). + */ + opentelemetry::nostd::shared_ptr storage_; }; // freshRoot() must ignore the ambient active span and start a brand-new trace. @@ -418,6 +446,115 @@ TEST_F(SpanGuardScopeTest, scopedGuard_conversion_result_ends_span_once) EXPECT_EQ(countSpans(spans, "ledger.build"), 1u); } +// A ScopedSpanGuard's scope lives in the ACTIVE LocalValue store, and the +// coro-aware storage makes the ambient context follow that store. This +// simulates a coroutine that yields (its store swapped out for the worker's +// own) then resumes on another worker (store swapped back in): the span must be +// hidden while off-store, visible again on resume, and pop cleanly under the +// same store it was pushed on -- so the owner-store assertion never trips. +// +// Fails without CoroAwareContextStorage: OpenTelemetry's default thread-local +// stack ignores the LocalValue swap, so the span would stay visible off-store +// (the EXPECT_NE below would fail). Passes only because the fixture installs the +// coro-aware storage that binds the ambient stack to the active store. +TEST_F(SpanGuardScopeTest, scopedGuard_survives_localvalue_store_swap) +{ + namespace ctx = opentelemetry::context; + namespace trc = opentelemetry::trace; + + // Stand-in stores for the coroutine and a worker thread's own store. Both + // are onCoro=true so LocalValues::cleanup() never deletes these stack + // objects. Keeping a stack store active across every GetCurrent() call below + // stops LocalValue from materializing (then leaking) a heap thread-store. + xrpl::detail::LocalValues coroStore; + xrpl::detail::LocalValues workerStore; + + // Detach (do NOT delete) the fixture's active store, run on the coro store, + // and remember the original so teardown gets it back. + auto* saved = xrpl::detail::getLocalValues().release(); + xrpl::detail::getLocalValues().reset(&coroStore); + + trc::SpanContext captured = trc::SpanContext::GetInvalid(); + { + ScopedSpanGuard const span(TraceCategory::Rpc, "rpc", "process"); + ASSERT_TRUE(static_cast(span)); + auto active = trc::GetSpan(ctx::RuntimeContext::GetCurrent())->GetContext(); + ASSERT_TRUE(active.IsValid()); + captured = active; + + // Yield: swap the coro store OUT, the worker's own store IN. + xrpl::detail::getLocalValues().release(); + xrpl::detail::getLocalValues().reset(&workerStore); + auto offCoro = trc::GetSpan(ctx::RuntimeContext::GetCurrent())->GetContext(); + EXPECT_FALSE(offCoro.IsValid()); // no ambient span off-coro + EXPECT_NE(offCoro.span_id(), captured.span_id()); // span NOT visible + + // Resume on another worker: swap the coro store back IN. + xrpl::detail::getLocalValues().release(); + xrpl::detail::getLocalValues().reset(&coroStore); + auto resumed = trc::GetSpan(ctx::RuntimeContext::GetCurrent())->GetContext(); + EXPECT_TRUE(resumed.IsValid()); // ambient again + EXPECT_EQ(resumed.span_id(), captured.span_id()); // same span visible + } // ~ScopedSpanGuard pops from coroStore (its owner store active) -- no assert + + // The scope popped cleanly: the coro store's stack is empty again. + auto afterPop = trc::GetSpan(ctx::RuntimeContext::GetCurrent()); + EXPECT_FALSE(afterPop->GetContext().IsValid()); + + // Restore (re-own) the fixture's store for teardown before any stack store + // leaves scope, so the thread pointer never dangles. + xrpl::detail::getLocalValues().release(); + xrpl::detail::getLocalValues().reset(saved); + + // The span ended exactly once, when the scope popped on resume. + EXPECT_EQ(countSpans(spanData()->GetSpans(), "rpc.process"), 1u); +} + +// SpanGuard::activate() makes an already-owned span the ambient context for the +// activation's lifetime WITHOUT owning or ending it: before activate() the span +// is not ambient, during it the span is the current context, and after it the +// prior (empty) context is restored. ~ScopedActivation must NOT end the span -- +// the owning guard ends it exactly once. Identity is proved by matching the +// ambient span_id seen during activation to the single exported span. +TEST_F(SpanGuardScopeTest, activate_sets_ambient_without_owning) +{ + namespace ctx = opentelemetry::context; + namespace trc = opentelemetry::trace; + + auto guard = SpanGuard::freshRoot(TraceCategory::Transactions, "tx", "process"); + ASSERT_TRUE(static_cast(guard)); + + // Unscoped: the guard's span is NOT ambient before activate(). + EXPECT_FALSE(trc::GetSpan(ctx::RuntimeContext::GetCurrent())->GetContext().IsValid()); + + trc::SpanId activeId; + { + auto activation = guard.activate(); + auto active = trc::GetSpan(ctx::RuntimeContext::GetCurrent())->GetContext(); + EXPECT_TRUE(active.IsValid()); // now ambient + activeId = active.span_id(); + } // ~ScopedActivation pops the scope; it does NOT end the span. + + // A valid ambient span was observed while the activation was live. + EXPECT_TRUE(activeId.IsValid()); + + // Prior context restored: no ambient span after the activation drops. + EXPECT_FALSE(trc::GetSpan(ctx::RuntimeContext::GetCurrent())->GetContext().IsValid()); + + // Non-owning: the activation did NOT end the span, so nothing is exported. + EXPECT_EQ(countSpans(spanData()->GetSpans(), "tx.process"), 0u); + + // Ending the OWNING guard exports the span exactly once. + guard = SpanGuard{}; + auto spans = spanData()->GetSpans(); + EXPECT_EQ(countSpans(spans, "tx.process"), 1u); + + // The span made ambient during activation WAS the guard's own span. + auto* txSpan = findSpan(spans, "tx.process"); + ASSERT_NE(txSpan, nullptr); + EXPECT_EQ(txSpan->GetSpanId(), activeId); +} + // 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) @@ -514,15 +651,16 @@ TEST_F(SpanGuardScopeTest, deterministicIdGenerator_ambient_child_ignores_pendin EXPECT_FALSE(root->GetParentSpanId().IsValid()); } -// Death test guarding the cross-thread scope-leak bug, now on ScopedSpanGuard. +// Death test guarding the cross-store scope-leak bug on ScopedSpanGuard. // -// 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(). +// A ScopedSpanGuard's Scope is bound to the LocalValue context store that was +// active when it was constructed, so it must be destroyed while that same store +// is active. A different thread has a different LocalValue store, so destroying +// the guard on another thread pops the Scope against a foreign context stack -- +// ~ScopedSpanGuard's owner-store XRPL_ASSERT turns that silent corruption into a +// loud abort(). 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, whose store differs from this thread's. // // 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() @@ -531,38 +669,40 @@ TEST_F(SpanGuardScopeTest, deterministicIdGenerator_ambient_child_ignores_pendin // 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". +// between "the" 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, scopedGuard_cross_thread_death_asserts_at_wrong_thread_destroy) +TEST_F(SpanGuardScopeTest, scopedGuard_cross_thread_death_asserts_at_wrong_store_destroy) { #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."; + "cross-store scope-leak assertion this test exercises does not fire."; #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."; + "the cross-store scope-leak assertion this test exercises does not crash."; #else EXPECT_DEATH( { - // Scoped guard constructed on THIS thread; its Scope binds here. + // Scoped guard constructed on THIS thread; its Scope binds to this + // thread's LocalValue store. auto scoped = std::make_unique(TraceCategory::Ledger, "ledger", "build"); // 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(). + // the worker when the lambda's captures are destroyed; the worker's + // store differs from the constructing store, tripping the owner-store + // 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; assert() stringifies the expression source via the // preprocessor '#' operator, keeping both quoted literals with the - // "\" \"" gap between "on" and "constructing". Match across that gap. - ".*destroyed on.*constructing thread.*"); + // "\" \"" gap between "the" and "constructing". Match across that gap. + ".*destroyed on.*constructing context store.*"); #endif } diff --git a/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp b/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp index b1e2bc64b2..f65a41a911 100644 --- a/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp +++ b/src/xrpld/rpc/handlers/orderbook/RipplePathFind.cpp @@ -27,16 +27,12 @@ json::Value doRipplePathFind(RPC::JsonContext& context) { using namespace telemetry; - // 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. - // - 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( + // pathfind.request nests under rpc.command. This scope is held across + // context.coro->yield() below; the coro-aware OTel context storage moves it + // with the coroutine on resume (it is never stranded on a worker's + // thread-local stack), so the scope pops on the correct store and the + // span's log lines stay trace-correlated. + 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 93c9ec672c36fef4b14c2526ccbfcb3520225c93 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 22 Jul 2026 22:05:44 +0100 Subject: [PATCH 7/7] feat(telemetry): correlate tx apply + receive logs via non-owning span activation Co-Authored-By: Claude Opus 4.8 (1M context) --- src/xrpld/app/misc/NetworkOPs.cpp | 8 ++++++++ src/xrpld/overlay/detail/PeerImp.cpp | 4 ++++ 2 files changed, 12 insertions(+) diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index aa5a483119..f14e9c51a7 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -1619,6 +1619,14 @@ NetworkOPsImp::apply(std::unique_lock& batchLock) auto newOL = registry_.get().getOpenLedger().current(); for (TransactionStatus const& e : transactions) { + // Make this transaction's span ambient for the duration of its + // apply so the per-tx log lines below carry its trace_id. + // Non-owning: the span is still owned/ended by e.span. Scoped to + // one loop iteration, so each tx's span is ambient only for its + // own processing. No yield in this loop. + auto txActivation = + (e.span && *e.span) ? e.span->activate() : telemetry::ScopedActivation{}; + if (e.span && *e.span) { e.span->setAttribute( diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index fcf07046e4..fda6b09dda 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -1431,6 +1431,10 @@ PeerImp::handleTransaction( batch, stx, sp = std::move(span)]() { + // Activate the tx.receive span so checkTransaction's log + // lines carry its trace_id. Non-owning; sp still owns/ends + // the span. This job body runs to completion on one worker. + auto activation = (sp && *sp) ? sp->activate() : telemetry::ScopedActivation{}; if (auto peer = weak.lock()) peer->checkTransaction(flags, checkSignature, stx, batch); });