ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-04-29 15:37:57 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(telemetry): address Phase 1c code review findings

Browse Source 
TracingInstrumentation.h:
- Unify all span-creation macros to use std::optional<SpanGuard>
  (fixes type mismatch between XRPL_TRACE_SPAN and SET_ATTR)
- Wrap XRPL_TRACE_SET_ATTR/EXCEPTION in do-while(0) (dangling-else)
- Move macros outside namespace blocks (macros are global)
- Cache telemetry reference to avoid double-evaluation
- Remove leaked _xrpl_span_ intermediate variable
- Add @note tags for thread safety, scope, and usage constraints
- Add 3 usage examples per CLAUDE.md requirements

ServerHandler.cpp:
- Remove misleading rpc.request span from onRequest() (span ended
  before coroutine runs, producing orphan spans)
- Add rpc.http_request span to HTTP processSession() (runs inside
  the coroutine, correct parent for rpc.process/rpc.command spans)
- Add XRPL_TRACE_EXCEPTION and error status in both catch blocks
  (WS processSession and processRequest)

SpanGuard.h:
- Add null guards to all mutating methods (setOk, setStatus,
  setAttribute, addEvent, recordException) for safety after discard()

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Pratik Mankawde
2026-04-17 13:33:35 +01:00
parent 9ee9e566d4
commit 025a8a344b
2 changed files with 81 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
src/xrpld/rpc/detail/ServerHandler.cpp
View File

@@ -303,8 +303,6 @@ buffers_to_string(ConstBufferSequence const& bs)
void
ServerHandler::onRequest(Session& session)
{
XRPL_TRACE_RPC(app_.getTelemetry(), "rpc.request");
// Make sure RPC is enabled on the port
if (session.port().protocol.count("http") == 0 && session.port().protocol.count("https") == 0)
{
@@ -504,6 +502,8 @@ ServerHandler::processSession(
jr[jss::result] = RPC::make_error(rpcINTERNAL);
JLOG(m_journal.error()) << "Exception while processing WS: " << ex.what() << "\n"
<< "Input JSON: " << Json::Compact{Json::Value{jv}};
XRPL_TRACE_EXCEPTION(ex);
XRPL_TRACE_SET_ATTR("xrpl.rpc.status", "error");
// LCOV_EXCL_STOP
}
@@ -563,6 +563,8 @@ ServerHandler::processSession(
std::shared_ptr<Session> const& session,
std::shared_ptr<JobQueue::Coro> coro)
{
XRPL_TRACE_RPC(app_.getTelemetry(), "rpc.http_request");
processRequest(
session->port(),
buffers_to_string(session->request().body().data()),
@@ -890,6 +892,8 @@ ServerHandler::processRequest(
JLOG(m_journal.error())
<< "Internal error : " << ex.what()
<< " when processing request: " << Json::Compact{Json::Value{params}};
XRPL_TRACE_EXCEPTION(ex);
XRPL_TRACE_SET_ATTR("xrpl.rpc.status", "error");
// LCOV_EXCL_STOP
}

118
src/xrpld/telemetry/TracingInstrumentation.h
View File

@@ -6,16 +6,47 @@
that manage span lifetime via RAII. When not defined, all macros expand to
((void)0) with zero overhead.
Usage in instrumented code:
All span-creation macros produce a std::optional<SpanGuard> named
_xrpl_guard_. The accessor macros (XRPL_TRACE_SET_ATTR,
XRPL_TRACE_EXCEPTION) reference this variable by name, so they must
appear in the same scope after exactly one span-creation macro.
@note Only one XRPL_TRACE_* span-creation macro may appear per scope,
because they all declare a variable named _xrpl_guard_. Nested spans
across function boundaries are fine (each function has its own scope).
@note These macros must not be used in single-statement if/else without
braces. The span-creation macros expand to multiple statements that
declare variables needed by the accessor macros.
@note Thread safety: Each SpanGuard binds to the constructing thread's
OTel context stack via Scope. Do not move a guard across threads.
Usage examples:
1. Basic RPC tracing:
@code
XRPL_TRACE_RPC(app.getTelemetry(), "rpc.command." + name);
XRPL_TRACE_SET_ATTR("xrpl.rpc.command", name);
XRPL_TRACE_SET_ATTR("xrpl.rpc.status", "success");
@endcode
@note Macro parameter names use leading/trailing underscores
(e.g. _tel_obj_) to avoid colliding with identifiers in the macro body,
specifically the ::xrpl::telemetry:: namespace qualifier.
2. Exception recording:
@code
XRPL_TRACE_RPC(telemetry, "rpc.process");
try {
doWork();
} catch (std::exception const& e) {
XRPL_TRACE_EXCEPTION(e);
XRPL_TRACE_SET_ATTR("xrpl.rpc.status", "error");
}
@endcode
3. Unconditional span:
@code
XRPL_TRACE_SPAN(telemetry, "tx.apply");
XRPL_TRACE_SET_ATTR("xrpl.tx.hash", txHash);
@endcode
*/
#ifdef XRPL_ENABLE_TELEMETRY
@@ -25,36 +56,34 @@
#include <optional>
namespace xrpl {
namespace telemetry {
/** Start an unconditional span, ended when the guard goes out of scope.
@param _tel_obj_ Telemetry instance reference.
@param _span_name_ Span name string.
*/
#define XRPL_TRACE_SPAN(_tel_obj_, _span_name_) \
auto _xrpl_span_ = (_tel_obj_).startSpan(_span_name_); \
::xrpl::telemetry::SpanGuard _xrpl_guard_(_xrpl_span_)
#define XRPL_TRACE_SPAN(_tel_obj_, _span_name_) \
std::optional<::xrpl::telemetry::SpanGuard> _xrpl_guard_( \
std::in_place, (_tel_obj_).startSpan(_span_name_))
/** Start an unconditional span with a specific SpanKind.
@param _tel_obj_ Telemetry instance reference.
@param _span_name_ Span name string.
@param _span_kind_ opentelemetry::trace::SpanKind value.
*/
#define XRPL_TRACE_SPAN_KIND(_tel_obj_, _span_name_, _span_kind_) \
auto _xrpl_span_ = (_tel_obj_).startSpan(_span_name_, _span_kind_); \
::xrpl::telemetry::SpanGuard _xrpl_guard_(_xrpl_span_)
#define XRPL_TRACE_SPAN_KIND(_tel_obj_, _span_name_, _span_kind_) \
std::optional<::xrpl::telemetry::SpanGuard> _xrpl_guard_( \
std::in_place, (_tel_obj_).startSpan(_span_name_, _span_kind_))
/** Conditionally start a span for RPC tracing.
The span is only created if shouldTraceRpc() returns true.
@param _tel_obj_ Telemetry instance reference.
@param _span_name_ Span name string.
*/
#define XRPL_TRACE_RPC(_tel_obj_, _span_name_) \
std::optional<::xrpl::telemetry::SpanGuard> _xrpl_guard_; \
if ((_tel_obj_).shouldTraceRpc()) \
{ \
_xrpl_guard_.emplace((_tel_obj_).startSpan(_span_name_)); \
#define XRPL_TRACE_RPC(_tel_obj_, _span_name_) \
auto& _xrpl_tel_ = (_tel_obj_); \
std::optional<::xrpl::telemetry::SpanGuard> _xrpl_guard_; \
if (_xrpl_tel_.shouldTraceRpc()) \
{ \
_xrpl_guard_.emplace(_xrpl_tel_.startSpan(_span_name_)); \
}
/** Conditionally start a span for transaction tracing.
@@ -62,11 +91,12 @@ namespace telemetry {
@param _tel_obj_ Telemetry instance reference.
@param _span_name_ Span name string.
*/
#define XRPL_TRACE_TX(_tel_obj_, _span_name_) \
std::optional<::xrpl::telemetry::SpanGuard> _xrpl_guard_; \
if ((_tel_obj_).shouldTraceTransactions()) \
{ \
_xrpl_guard_.emplace((_tel_obj_).startSpan(_span_name_)); \
#define XRPL_TRACE_TX(_tel_obj_, _span_name_) \
auto& _xrpl_tel_ = (_tel_obj_); \
std::optional<::xrpl::telemetry::SpanGuard> _xrpl_guard_; \
if (_xrpl_tel_.shouldTraceTransactions()) \
{ \
_xrpl_guard_.emplace(_xrpl_tel_.startSpan(_span_name_)); \
}
/** Conditionally start a span for consensus tracing.
@@ -74,33 +104,35 @@ namespace telemetry {
@param _tel_obj_ Telemetry instance reference.
@param _span_name_ Span name string.
*/
#define XRPL_TRACE_CONSENSUS(_tel_obj_, _span_name_) \
std::optional<::xrpl::telemetry::SpanGuard> _xrpl_guard_; \
if ((_tel_obj_).shouldTraceConsensus()) \
{ \
_xrpl_guard_.emplace((_tel_obj_).startSpan(_span_name_)); \
#define XRPL_TRACE_CONSENSUS(_tel_obj_, _span_name_) \
auto& _xrpl_tel_ = (_tel_obj_); \
std::optional<::xrpl::telemetry::SpanGuard> _xrpl_guard_; \
if (_xrpl_tel_.shouldTraceConsensus()) \
{ \
_xrpl_guard_.emplace(_xrpl_tel_.startSpan(_span_name_)); \
}
/** Set a key-value attribute on the current span (if it exists).
Must be used after one of the XRPL_TRACE_* span macros.
Must be used after one of the XRPL_TRACE_* span-creation macros
in the same scope.
*/
#define XRPL_TRACE_SET_ATTR(key, value) \
if (_xrpl_guard_.has_value()) \
{ \
_xrpl_guard_->setAttribute(key, value); \
}
#define XRPL_TRACE_SET_ATTR(key, value) \
do \
{ \
if (_xrpl_guard_.has_value()) \
_xrpl_guard_->setAttribute(key, value); \
} while (0)
/** Record an exception on the current span and mark it as error.
Must be used after one of the XRPL_TRACE_* span macros.
Must be used after one of the XRPL_TRACE_* span-creation macros
in the same scope.
*/
#define XRPL_TRACE_EXCEPTION(e) \
if (_xrpl_guard_.has_value()) \
{ \
_xrpl_guard_->recordException(e); \
}
} // namespace telemetry
} // namespace xrpl
#define XRPL_TRACE_EXCEPTION(e) \
do \
{ \
if (_xrpl_guard_.has_value()) \
_xrpl_guard_->recordException(e); \
} while (0)
#else // XRPL_ENABLE_TELEMETRY not defined