diff --git a/docs/build/telemetry.md b/docs/build/telemetry.md index f4681b3309..8be38466c3 100644 --- a/docs/build/telemetry.md +++ b/docs/build/telemetry.md @@ -16,6 +16,10 @@ This document explains how to build xrpld with OpenTelemetry distributed tracing - [CMake target not found](#cmake-target-not-found) - [Conditional compilation](#conditional-compilation) - [Span lifetime and cross-thread handling](#span-lifetime-and-cross-thread-handling) + - [`SpanGuard` versus `ScopedSpanGuard`](#spanguard-versus-scopedspanguard) + - [Coroutine-aware context storage](#coroutine-aware-context-storage) + - [Handing a span to a job](#handing-a-span-to-a-job) + - [Why are unrelated spans in my trace?](#why-are-unrelated-spans-in-my-trace) ## Overview @@ -131,31 +135,96 @@ This two-layer approach ensures zero overhead when telemetry is not wanted. ## Span lifetime and cross-thread handling -`SpanGuard` is an RAII wrapper: it activates a span on the current thread when -constructed and ends it when destroyed. A normal (scoped) guard binds an OTel -`Scope` to the constructing thread's thread-local context stack. Because of -this, a scoped guard must be used and destroyed on the thread that created it. +Telemetry exposes two RAII guards with split responsibilities, plus a +non-owning activation helper. Picking the right one is what keeps a trace's +parent/child nesting and its per-line log correlation correct. -Two entry points cover the cases where the default scoped behaviour is wrong: +### `SpanGuard` versus `ScopedSpanGuard` -- **`SpanGuard::rootSpan(cat, prefix, name)`** starts a span as a fresh trace - root, ignoring whatever span is already active on the current thread. Use it - at an inbound entry point that runs on a shared worker thread (for example a - peer message received while unrelated work is still on the stack), so those - unrelated spans do not become parents of the new trace. +- **`SpanGuard`** owns a span and nothing else. It is _thread-free_: it never + touches the active-context stack, so it carries no thread affinity and may be + moved to and ended on any thread. It is movable and move-assignable. Create + one with `SpanGuard::span(cat, prefix, name)`, or with + `SpanGuard::freshRoot(...)` to start a fresh trace root that ignores whatever + span is currently ambient. Reach for a plain `SpanGuard` whenever the span + must leave the context store that created it — for example when it is handed + into a job. -- **`std::move(guard).detached()`** returns a new guard that holds the same - span with **no** thread-local `Scope`. Call it on the origin thread before - handing the guard to a job that runs on another thread. The returned guard - can be moved freely across threads and ended anywhere; only its final - destruction ends the span. +- **`ScopedSpanGuard`** owns a `SpanGuard` _plus_ an active OTel scope that + pushes the span onto the current context store. While it lives, the span is + the ambient parent for child spans created on that store, and log lines + emitted under it carry its `trace_id`. It is non-copyable and non-movable — + short-lived stack RAII. It offers the same factories (`freshRoot(...)`, + `childSpan(...)`). When the span must outlive the scope, convert it with + `operator SpanGuard() &&`: that pops the scope on the origin store and yields + the bare, thread-free `SpanGuard`. + +Rule of thumb: use `ScopedSpanGuard` for same-thread (or same-coroutine) +nesting and log correlation; use the plain `SpanGuard` whenever the span +crosses out of the store that created it. + +```mermaid +flowchart TD + SG["SpanGuard
(unscoped, thread-free)
owns span only; movable across threads and coroutines"] + SSG["ScopedSpanGuard
(scoped, store-bound)
owns a SpanGuard plus an active scope on the current store"] + SA["ScopedActivation
(non-owning)
activates a borrowed span; never owns or ends it"] + + SSG -->|"handoff: pop scope, yield bare span"| SG + SG -->|"activate / activateIfLive"| SA + SA -.->|"borrows span, no ownership"| SG + + classDef box fill:#e8f0fe,stroke:#3b5bdb,color:#111827; + class SG,SSG,SA box; +``` + +### Coroutine-aware context storage + +The active-context stack is not a plain `thread_local`. At telemetry start +xrpld installs `CoroAwareContextStorage`, which keeps the stack in an +`xrpl::LocalValue`. Because `JobQueue::Coro::resume()` swaps the coroutine's +`LocalValue` store in and out with the coroutine, the ambient context _follows +the coroutine_ across every yield and resume — even when it resumes on a +different worker thread. A `ScopedSpanGuard` held across a coroutine yield is +therefore safe: its scope rides the coroutine and pops on the same store it was +pushed onto, so it never pops the wrong stack. Off a coroutine the `LocalValue` +transparently gives each thread its own store, so behaviour matches OTel's +default thread-local storage. This is what lets the RPC entry, process, and +command spans be scoped — for correct nesting and per-line log-trace +correlation — even though the RPC path yields. + +### Handing a span to a job + +The hand-off pattern is: create a thread-free `SpanGuard` at the origin (or +convert a `ScopedSpanGuard` with `operator SpanGuard() &&`), move it into the +job closure, and inside the worker body activate it non-destructively with +`telemetry::activateIfLive(handle)`. That call takes no ownership and returns a +`ScopedActivation` (a no-op if the handle is empty or the span inactive) which +makes the span the ambient context so log lines in the worker body carry its +`trace_id`. The activation neither owns nor ends the span — the owning +`SpanGuard` still controls its lifetime and ends it when the closure is +destroyed. Keep the activation confined to a synchronous, non-yielding block. +There is no detach step: a `SpanGuard` is already thread-free. ### Why are unrelated spans in my trace? -If a trace shows hundreds of unrelated spans nested under one operation, a -scoped guard was likely moved to another thread (for example stored in a job -and destroyed on a worker thread). Destroying a scoped guard off its origin -thread pops the wrong context stack, leaving the origin thread's active span -in place so later spans inherit it. Fix it by calling `.detached()` on the -origin thread before the hand-off, or by starting the operation with -`rootSpan()` so it never inherits an ambient parent. +Historically a scoped guard destroyed off its origin thread popped the wrong +context stack, leaving a stale ambient span in place that later work inherited. +The current design removes that failure mode in two ways: + +- **Coroutine-aware storage** makes a scope held across a coroutine yield pop on + the same store it was pushed onto, so a coroutine that resumes on another + worker never pops the wrong stack. + +- **A same-store assertion** in `ScopedSpanGuard` (and `ScopedActivation`) + records the `LocalValue` store its scope was pushed onto and checks, in + debug/test/fuzzing builds, that destruction, hand-off, and discard all happen + while that same store is active — turning a genuine cross-store misuse into an + immediate assertion failure rather than a silently corrupted trace. + +If a trace still shows unrelated spans nested under one operation, the usual +cause is an inbound entry point that inherited an ambient parent it should not +have. Start such an operation with `freshRoot()` so it begins a clean trace +root and never adopts whatever span happened to be active. To move a span +across a store boundary, keep it in a thread-free `SpanGuard` (or convert via +`operator SpanGuard() &&`) rather than holding a `ScopedSpanGuard` across the +boundary.