Add TODO comments for known issues, update task lists + cspell

Document known issues with inline TODO comments:
- Thread safety of TelemetryImpl::stop() vs startSpan() in
  Telemetry.cpp and Application.cpp shutdown sequence
- Unwired propagation utilities in TraceContextPropagator.h
- Unused trace_state proto field in xrpl.proto

Add "Known Issues / Future Work" sections to Phase 2 and Phase 3
task lists covering: thread safety, macro incompatibility between
XRPL_TRACE_SPAN and XRPL_TRACE_SET_ATTR, unwired P2P propagation,
and unused trace_state field.

Add "reqps" to cspell dictionary and sort trailing entries.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: pratikmankawde

OpenTelemetryPlan/Phase2_taskList.md

@@ -185,3 +185,36 @@
 | 2.6  | Build verification and performance baseline | 0         | 0              | 2.1-2.5    |
 
 **Parallel work**: Tasks 2.1, 2.2, 2.3 can run in parallel. Task 2.4 depends on 2.3. Task 2.5 can run in parallel with 2.4. Task 2.6 depends on all others.
+
+---
+
+## Known Issues / Future Work
+
+### Thread safety of TelemetryImpl::stop() vs startSpan()
+
+`TelemetryImpl::stop()` resets `sdkProvider_` (a `std::shared_ptr`) without
+synchronization. `getTracer()` reads the same member from RPC handler threads.
+This is a data race if any thread calls `startSpan()` concurrently with `stop()`.
+
+**Current mitigation**: `Application::stop()` shuts down `serverHandler_`,
+`overlay_`, and `jobQueue_` before calling `telemetry_->stop()`, so no callers
+remain. See comments in `Telemetry.cpp:stop()` and `Application.cpp`.
+
+**TODO**: Add an `std::atomic<bool> stopped_` flag checked in `getTracer()` to
+make this robust against future shutdown order changes.
+
+### Macro incompatibility: XRPL_TRACE_SPAN vs XRPL_TRACE_SET_ATTR
+
+`XRPL_TRACE_SPAN` and `XRPL_TRACE_SPAN_KIND` declare `_xrpl_guard_` as a bare
+`SpanGuard`, but `XRPL_TRACE_SET_ATTR` and `XRPL_TRACE_EXCEPTION` call
+`_xrpl_guard_.has_value()` which requires `std::optional<SpanGuard>`. Using
+`XRPL_TRACE_SPAN` followed by `XRPL_TRACE_SET_ATTR` in the same scope would
+fail to compile.
+
+**Current mitigation**: No call site currently uses `XRPL_TRACE_SPAN` — all
+production code uses the conditional macros (`XRPL_TRACE_RPC`, `XRPL_TRACE_TX`,
+etc.) which correctly wrap the guard in `std::optional`.
+
+**TODO**: Either make `XRPL_TRACE_SPAN`/`XRPL_TRACE_SPAN_KIND` also wrap in
+`std::optional`, or document that `XRPL_TRACE_SET_ATTR` is only compatible with
+the conditional macros.

OpenTelemetryPlan/Phase3_taskList.md

@@ -236,3 +236,28 @@
 - [ ] Trace context in Protocol Buffer messages
 - [ ] HashRouter deduplication visible in traces
 - [ ] <5% overhead on transaction throughput
+
+---
+
+## Known Issues / Future Work
+
+### Propagation utilities not yet wired into P2P flow
+
+`extractFromProtobuf()` and `injectToProtobuf()` in `TraceContextPropagator.h`
+are implemented and tested but not called from production code. To enable
+cross-node distributed traces:
+
+- Call `injectToProtobuf()` in `PeerImp` when sending `TMTransaction` /
+  `TMProposeSet` messages
+- Call `extractFromProtobuf()` in the corresponding message handlers to
+  reconstruct the parent span context, then pass it to `startSpan()` as the
+  parent
+
+This was deferred to validate single-node tracing performance first.
+
+### Unused trace_state proto field
+
+The `TraceContext.trace_state` field (field 4) in `xrpl.proto` is reserved for
+W3C `tracestate` vendor-specific key-value pairs but is not read or written by
+`TraceContextPropagator`. Wire it when cross-vendor trace propagation is needed.
+No wire cost since proto `optional` fields are zero-cost when absent.

cspell.config.yaml

@@ -210,6 +210,7 @@ words:
   - queuable
   - Raphson
   - replayer
+  - reqps
   - rerere
   - retriable
   - RIPD
@@ -304,6 +305,10 @@ words:
   - xchain
   - ximinez
   - EXPECT_STREQ
+  - Gantt
+  - gantt
+  - otelc
+  - traceql
   - XMACRO
   - xrpkuwait
   - xrpl
@@ -312,8 +317,4 @@ words:
   - xxhash
   - xxhasher
   - xychart
-  - otelc
   - zpages
-  - traceql
-  - Gantt
-  - gantt

include/xrpl/proto/xrpl.proto

@@ -91,6 +91,10 @@ message TraceContext {
   optional bytes trace_id = 1;      // 16-byte trace identifier
   optional bytes span_id = 2;       // 8-byte parent span identifier
   optional uint32 trace_flags = 3;  // bit 0 = sampled
+  // TODO: trace_state is reserved for W3C tracestate vendor-specific
+  // key-value pairs but is not yet read or written by
+  // TraceContextPropagator. Wire it when cross-vendor trace
+  // propagation is needed.
   optional string trace_state = 4;  // W3C tracestate header value
 }
 

include/xrpl/telemetry/TraceContextPropagator.h

@@ -6,6 +6,13 @@
     Protocol Buffer TraceContext messages (P2P cross-node propagation).
 
     Only compiled when XRPL_ENABLE_TELEMETRY is defined.
+
+    TODO: These utilities are not yet wired into the P2P message flow.
+    To enable cross-node distributed traces, call injectToProtobuf() in
+    PeerImp when sending TMTransaction/TMProposeSet messages, and call
+    extractFromProtobuf() in the corresponding message handlers to
+    reconstruct the parent span context before starting a child span.
+    This was deferred to validate single-node tracing performance first.
 */
 
 #ifdef XRPL_ENABLE_TELEMETRY

src/libxrpl/telemetry/Telemetry.cpp

@@ -200,6 +200,13 @@ class TelemetryImpl : public Telemetry
         {
             // Force flush before shutdown
             sdkProvider_->ForceFlush();
+            // TODO: sdkProvider_ is not thread-safe. This reset() races with
+            // getTracer() if any thread is still calling startSpan().
+            // Currently safe because Application::stop() shuts down
+            // serverHandler_, overlay_, and jobQueue_ before calling
+            // telemetry_->stop() — so no callers should remain. If the
+            // shutdown order ever changes, add an std::atomic<bool> stopped_
+            // flag checked in getTracer() to make this robust.
             sdkProvider_.reset();
             trace_api::Provider::SetTracerProvider(
                 opentelemetry::nostd::shared_ptr<trace_api::TracerProvider>(

src/xrpld/app/main/Application.cpp

@@ -1570,6 +1570,10 @@ ApplicationImp::run()
     ledgerCleaner_->stop();
     m_nodeStore->stop();
     perfLog_->stop();
+    // Telemetry must stop last among trace-producing components.
+    // serverHandler_, overlay_, and jobQueue_ are already stopped above,
+    // so no threads should be calling startSpan() at this point.
+    // See TODO in TelemetryImpl::stop() re: thread-safety of sdkProvider_.
     telemetry_->stop();
 
     JLOG(m_journal.info()) << "Done.";