fix(telemetry): restore injectCurrentContextToProtobuf decl; re-parent open/establish spans under round; correct childSpan names

Post-review fixes for the SpanGuard scope-leak change:
- SpanGuard.h: restore the injectCurrentContextToProtobuf real-class
  declaration, its #else no-op stub, and the protocol::TraceContext
  forward-declaration dropped during the 3->4 union merge (compile break).
- Re-parent consensus.phase.open and consensus.establish under the round
  span via a new RCLConsensus::Adaptor::roundSpanContext() accessor: the
  round span is now detached, so ambient parenting no longer works; the
  phase spans link explicitly to the captured round context. csf::Peer
  gains a matching no-op accessor so the generic engine still compiles.
- Switch five childSpan(op::X, ctx) sites to the full consensus::span::X
  constants: childSpan(name, ctx) takes the name verbatim, so the suffix-
  only op:: constants emitted short, non-dotted span names.
- TestTelemetry mock: add getConsensusTraceStrategy() override (base pure
  virtual introduced on this branch) so the mock is not abstract.
- validate(): rewrite the trace_context comment to drop attack-surface
  framing and the PR-discussion reference.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Pratik Mankawde
2026-07-20 15:25:15 +01:00
parent c3b1f69ba8
commit 58d4e48679
6 changed files with 86 additions and 24 deletions

View File

@@ -170,6 +170,10 @@
#include <string_view>
#include <utility>
namespace protocol {
class TraceContext;
} // namespace protocol
namespace xrpl::telemetry {
/**
@@ -436,6 +440,20 @@ public:
[[nodiscard]] TraceBytes
getTraceBytes() const;
/**
* Inject the calling thread's currently-active OTel context into a
* protobuf TraceContext message for cross-node propagation.
*
* Encapsulates `RuntimeContext::GetCurrent()` + `injectToProtobuf`
* so callers in app-layer code (e.g. RCLConsensus broadcasting
* TMProposeSet / TMValidation) don't depend on any OTel headers.
* No-op if telemetry is disabled or no span is active.
*
* @param proto The protobuf TraceContext to populate.
*/
static void
injectCurrentContextToProtobuf(protocol::TraceContext& proto);
// --- Attribute setters (explicit overloads, no OTel types) ---------
/**
@@ -613,6 +631,11 @@ public:
{
return {};
}
static void
injectCurrentContextToProtobuf(protocol::TraceContext&)
{
}
// NOLINTEND(readability-convert-member-functions-to-static)
void

View File

@@ -24,6 +24,7 @@
#include <xrpl/beast/utility/WrappedSink.h>
#include <xrpl/json/json_value.h>
#include <xrpl/json/json_writer.h>
#include <xrpl/telemetry/SpanGuard.h>
#include <boost/container/flat_map.hpp>
#include <boost/container/flat_set.hpp>
@@ -700,6 +701,15 @@ struct Peer
{
}
// The generic engine parents its phase spans under the round span via
// this context; the simulator runs without telemetry, so return an
// invalid context and no phase span is created.
static telemetry::SpanContext
roundSpanContext()
{
return {};
}
// Share a message by broadcasting to all connected peers
template <class M>
void

View File

@@ -135,6 +135,17 @@ public:
return true;
}
/**
* @return A fixed strategy label; the scope tests do not exercise
* deterministic trace-id correlation, so any stable value works.
*/
[[nodiscard]] std::string const&
getConsensusTraceStrategy() const override
{
static std::string const kStrategy{"none"};
return kStrategy;
}
opentelemetry::nostd::shared_ptr<opentelemetry::trace::Tracer>
getTracer(std::string_view name) override
{

View File

@@ -233,7 +233,7 @@ 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).
auto span = telemetry::SpanGuard::childSpan(
telemetry::consensus::span::op::proposalSend, roundSpanContext_);
telemetry::consensus::span::proposalSend, roundSpanContext_);
span.setAttribute(
telemetry::consensus::span::attr::round, static_cast<int64_t>(proposal.proposeSeq()));
span.setAttribute(telemetry::consensus::span::attr::isBowOut, proposal.isBowOut());
@@ -349,7 +349,7 @@ RCLConsensus::Adaptor::onClose(
// Child of the round span via its captured context (roundSpan_ is detached,
// so it is no longer the thread's ambient parent).
auto span = telemetry::SpanGuard::childSpan(cs::op::ledgerClose, roundSpanContext_);
auto span = telemetry::SpanGuard::childSpan(cs::ledgerClose, roundSpanContext_);
span.setAttribute(cs::attr::ledgerSeq, static_cast<int64_t>(ledger.ledger->header().seq) + 1);
span.setAttribute(cs::attr::mode, toDisplayString(mode).c_str());
span.setAttribute(
@@ -1059,14 +1059,10 @@ RCLConsensus::Adaptor::validate(RCLCxLedger const& ledger, RCLTxSet const& txns,
// Inject the current thread's active span context so receiving
// peers can link their validation.receive span as a child.
//
// TODO(observability/secure-OTel): the trace_context appended below is
// outside the cryptographic signature on `serialized` and is therefore
// unauthenticated. Receivers cannot prove it was not tampered with by
// a relay. A signed trace context (either folded into the validation
// payload or carried by an authenticated trace_state token) is tracked
// as a follow-up — see PR #6425 discussion r3317273388 and
// OpenTelemetryPlan/secure-OTel.md. Until then, downstream consumers
// must treat the validation trace_context as advisory only.
// The trace_context appended below is outside the signature on
// `serialized`, so it is not covered by validation authenticity.
// Downstream consumers treat it as advisory only. A signature-covered
// trace context is a possible future enhancement.
telemetry::SpanGuard::injectCurrentContextToProtobuf(*val.mutable_trace_context());
app_.getOverlay().broadcast(val);
@@ -1082,7 +1078,7 @@ RCLConsensus::Adaptor::onModeChange(ConsensusMode before, ConsensusMode after)
// 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).
auto span = telemetry::SpanGuard::childSpan(cs::op::modeChange, roundSpanContext_);
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());

View File

@@ -285,6 +285,24 @@ class RCLConsensus
void
onOutcomeEvent(std::string_view eventName);
/**
* Captured context of the current round span.
*
* 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.
*
* @return The round span's captured context, or an invalid context.
*/
telemetry::SpanContext
roundSpanContext() const
{
return roundSpanContext_;
}
private:
//---------------------------------------------------------------------
// The following members implement the generic Consensus requirements

View File

@@ -779,14 +779,16 @@ Consensus<Adaptor>::startRoundInternal(
// early-returns when establishSpan_ is populated).
establishSpan_.reset();
establishSpanContext_ = telemetry::SpanContext{};
// Detached: emplaced here on one job worker and reset() on another, so
// strip the thread-local Scope to avoid a wrong-thread Scope pop. No
// same-thread child span nests under openSpan_, so detaching is safe.
// 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_.
openSpan_.emplace(
telemetry::SpanGuard::span(
telemetry::TraceCategory::Consensus,
telemetry::seg::consensus,
telemetry::consensus::span::op::phaseOpen)
telemetry::SpanGuard::childSpan(
telemetry::consensus::span::phaseOpen, adaptor_.roundSpanContext())
.detached());
// On the Recovered path, fire phase.open here because startRoundTracing
// (which fires it for the Initial path) is not called on re-entry. On
@@ -1631,7 +1633,7 @@ Consensus<Adaptor>::updateOurPositions(std::unique_ptr<std::stringstream> const&
// 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.
auto span = SpanGuard::childSpan(consensus::span::op::updatePositions, establishSpanContext_);
auto span = SpanGuard::childSpan(consensus::span::updatePositions, establishSpanContext_);
span.setAttribute(
consensus::span::attr::convergePercent, static_cast<int64_t>(convergePercent_));
span.setAttribute(
@@ -1840,7 +1842,7 @@ Consensus<Adaptor>::haveConsensus(std::unique_ptr<std::stringstream> const& clog
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).
auto span = SpanGuard::childSpan(consensus::span::op::check, establishSpanContext_);
auto span = SpanGuard::childSpan(consensus::span::check, establishSpanContext_);
// CHECKME: should possibly count unacquired TX sets as disagreeing
int agree = 0, disagree = 0;
@@ -2099,11 +2101,13 @@ Consensus<Adaptor>::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
// explicitly under roundSpanContext_. An invalid round context (round span
// not yet created) yields a null guard.
establishSpan_.emplace(
telemetry::SpanGuard::span(
telemetry::TraceCategory::Consensus,
telemetry::seg::consensus,
telemetry::consensus::span::op::establish));
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