From 3ad525a48a77df3241eb3e38d13601a4f66d68a9 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 29 Jul 2026 14:33:05 +0100 Subject: [PATCH 1/3] =?UTF-8?q?docs(telemetry):=20drop=20speculative=20fil?= =?UTF-8?q?e/line=20estimate=20tables=20from=20=C2=A73.9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The §3.9.1 "Files Modified Summary" and §3.9.2 "Detailed File Impact" tables carried hand-maintained per-component line counts that had drifted from the plan: the Lines Added column summed to 1,565 while the Total row claimed ~1,670. Files (34) and Lines Changed (120) reconciled, so only the Added total was stale — residue from expanding Core Telemetry 5 -> 11 files. Rather than patch one cell, remove both tables. They were pre-implementation estimates with no source of truth, so any figure in them drifts again on the next phase. §3.1's directory tree remains the canonical list of the 11 new telemetry files, and §3.9.3-3.9.7 (risk, architectural impact, backward compatibility, rollback) carry the assessment without inventing line counts. No references to the removed sections exist elsewhere in the plan docs. --- .../03-implementation-strategy.md | 73 ------------------- 1 file changed, 73 deletions(-) diff --git a/OpenTelemetryPlan/03-implementation-strategy.md b/OpenTelemetryPlan/03-implementation-strategy.md index 4edcef6194..7cf0f9d445 100644 --- a/OpenTelemetryPlan/03-implementation-strategy.md +++ b/OpenTelemetryPlan/03-implementation-strategy.md @@ -339,79 +339,6 @@ Instrumentation is gated on two levels. A compile-time feature flag (`XRPL_ENABL This section provides a detailed assessment of how intrusive the OpenTelemetry integration is to the existing xrpld codebase. -### 3.9.1 Files Modified Summary - -| Component | Files Modified | Lines Added | Lines Changed | Architectural Impact | -| --------------------- | -------------- | ----------- | ------------- | -------------------- | -| **Core Telemetry** | 11 new files | ~980 | 0 | None (new module) | -| **Application Init** | 2 files | ~30 | ~5 | Minimal | -| **RPC Layer** | 3 files | ~80 | ~20 | Minimal | -| **Transaction Relay** | 4 files | ~120 | ~40 | Low | -| **Consensus** | 3 files | ~100 | ~30 | Low-Medium | -| **Protocol Buffers** | 1 file | ~25 | 0 | Low | -| **CMake/Build** | 3 files | ~50 | ~10 | Minimal | -| **PathFinding** | 2 | ~80 | ~5 | Minimal | -| **TxQ/Fee** | 2 | ~60 | ~5 | Minimal | -| **Validator/Amend** | 3 | ~40 | ~5 | Minimal | -| **Total** | **~34 files** | **~1,670** | **~120** | **Low** | - -### 3.9.2 Detailed File Impact - -```mermaid -pie title Code Changes by Component - "New Telemetry Module" : 800 - "Transaction Relay" : 160 - "Consensus" : 130 - "RPC Layer" : 100 - "PathFinding" : 80 - "TxQ/Fee" : 60 - "Validator/Amendment" : 40 - "Application Init" : 35 - "Protocol Buffers" : 25 - "Build System" : 60 -``` - -#### New Files (No Impact on Existing Code) - -| File | Lines | Purpose | -| ------------------------------------------------ | ----- | ------------------------ | -| `include/xrpl/telemetry/Telemetry.h` | ~160 | Main interface | -| `include/xrpl/telemetry/TelemetryConfig.h` | ~80 | Configuration structures | -| `include/xrpl/telemetry/TraceContext.h` | ~80 | Context propagation | -| `include/xrpl/telemetry/SpanGuard.h` | ~120 | RAII wrapper | -| `include/xrpl/telemetry/SpanAttributes.h` | ~60 | Attribute helpers | -| `src/libxrpl/telemetry/Telemetry.cpp` | ~200 | Implementation | -| `src/libxrpl/telemetry/TelemetryConfig.cpp` | ~60 | Config parsing | -| `src/libxrpl/telemetry/TraceContext.cpp` | ~80 | Context serialization | -| `src/libxrpl/telemetry/NullTelemetry.cpp` | ~40 | No-op implementation | -| `src/xrpld/telemetry/TracingInstrumentation.h` | ~60 | Macros | -| `src/xrpld/telemetry/TracingInstrumentation.cpp` | ~40 | Instrumentation impl | - -#### Modified Files (Existing Xrpld Code) - -| File | Lines Added | Lines Changed | Risk Level | -| ------------------------------------------------- | ----------- | ------------- | ---------- | -| `src/xrpld/app/main/Application.cpp` | ~15 | ~3 | Low | -| `include/xrpl/core/ServiceRegistry.h` | ~5 | ~2 | Low | -| `src/xrpld/rpc/detail/ServerHandler.cpp` | ~40 | ~10 | Low | -| `src/xrpld/rpc/handlers/*.cpp` | ~30 | ~8 | Low | -| `src/xrpld/overlay/detail/PeerImp.cpp` | ~60 | ~15 | Medium | -| `src/xrpld/overlay/detail/OverlayImpl.cpp` | ~30 | ~10 | Medium | -| `src/xrpld/app/consensus/RCLConsensus.cpp` | ~50 | ~15 | Medium | -| `src/xrpld/app/consensus/RCLConsensusAdaptor.cpp` | ~40 | ~12 | Medium | -| `src/xrpld/core/JobQueue.cpp` | ~20 | ~5 | Low | -| `src/xrpld/app/paths/PathRequest.cpp` | ~40 | ~3 | Low | -| `src/xrpld/app/paths/Pathfinder.cpp` | ~40 | ~2 | Low | -| `src/xrpld/app/misc/TxQ.cpp` | ~40 | ~3 | Low | -| `src/xrpld/app/main/LoadManager.cpp` | ~20 | ~2 | Low | -| `src/xrpld/app/misc/ValidatorList.cpp` | ~20 | ~2 | Low | -| `src/xrpld/app/misc/AmendmentTable.cpp` | ~10 | ~2 | Low | -| `src/xrpld/app/misc/Manifest.cpp` | ~10 | ~1 | Low | -| `src/xrpld/shamap/SHAMap.cpp` | ~20 | ~3 | Low | -| `src/xrpld/overlay/detail/ripple.proto` | ~25 | 0 | Low | -| `CMakeLists.txt` | ~40 | ~8 | Low | -| `cmake/FindOpenTelemetry.cmake` | ~50 | 0 | None (new) | - ### 3.9.3 Risk Assessment by Component
From 17ff797633db4b7eb006dedac8c566dfe93f3af8 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 29 Jul 2026 15:06:31 +0100 Subject: [PATCH 2/3] refactor(telemetry): build span names with std::format SpanGuard::span() and freshRoot() joined the prefix and suffix by hand with reserve() + three append() calls. std::format expresses the same join in one line; libstdc++ has shipped since GCC 13 and the project floor is GCC 15.2 (BUILD.md), so it is available on every supported compiler. Both callers are noexcept and std::format can throw (std::bad_alloc, or std::format_error on a malformed spec), so an escaping exception would terminate the process. The call is wrapped in a joinSpanName() helper that catches and returns std::nullopt; the caller then returns a null guard, which is the same degrade-to-no-op path already taken when telemetry is disabled. Telemetry must never bring the node down. One helper rather than a try/catch at each site keeps both call sites a single line and puts the exception-safety requirement in one documented place. Addresses a review comment on PR #6437. --- src/libxrpl/telemetry/SpanGuard.cpp | 45 ++++++++++++++++++++++++----- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/src/libxrpl/telemetry/SpanGuard.cpp b/src/libxrpl/telemetry/SpanGuard.cpp index 84b23f44cd..a165d0ad5a 100644 --- a/src/libxrpl/telemetry/SpanGuard.cpp +++ b/src/libxrpl/telemetry/SpanGuard.cpp @@ -47,6 +47,7 @@ #include #include +#include #include #include #include @@ -186,6 +187,34 @@ categoryToSpanKind(TraceCategory cat) return otel_trace::SpanKind::kInternal; // unreachable } +/** + * Join a span-name prefix and suffix into the dotted full name. + * + * Wraps std::format because the callers are noexcept: std::format can throw + * (std::bad_alloc, or std::format_error on a malformed spec) and an escaping + * exception would terminate the process. Telemetry must never take the node + * down, so a failure yields std::nullopt and the caller returns a null guard — + * the same degrade-to-no-op path already used when telemetry is disabled. + * + * @param prefix Segment before the dot (e.g. "consensus"). + * @param name Segment after the dot (e.g. "round"). + * @return The joined name, or std::nullopt if formatting failed. + */ +[[nodiscard]] std::optional +joinSpanName(std::string_view prefix, std::string_view name) noexcept +{ + try + { + return std::format("{}.{}", prefix, name); + } + catch (std::exception const&) + { + // Out of memory or a bad format spec. Drop the span rather than + // propagate out of a noexcept factory. + return std::nullopt; + } +} + } // namespace SpanGuard @@ -194,10 +223,10 @@ SpanGuard::span(TraceCategory cat, std::string_view prefix, std::string_view nam auto* tel = Telemetry::getInstance(); if ((tel == nullptr) || !tel->isEnabled() || !isCategoryEnabled(*tel, cat)) return {}; - std::string fullName; - fullName.reserve(prefix.size() + 1 + name.size()); - fullName.append(prefix).append(1, '.').append(name); - return SpanGuard(std::make_unique(tel->startSpan(fullName, categoryToSpanKind(cat)))); + auto const fullName = joinSpanName(prefix, name); + if (!fullName) + return {}; + return SpanGuard(std::make_unique(tel->startSpan(*fullName, categoryToSpanKind(cat)))); } SpanGuard @@ -206,13 +235,13 @@ SpanGuard::freshRoot(TraceCategory cat, std::string_view prefix, std::string_vie auto* tel = Telemetry::getInstance(); if ((tel == nullptr) || !tel->isEnabled() || !isCategoryEnabled(*tel, cat)) return {}; - std::string fullName; - fullName.reserve(prefix.size() + 1 + name.size()); - fullName.append(prefix).append(1, '.').append(name); + auto const fullName = joinSpanName(prefix, name); + if (!fullName) + return {}; // Force a fresh trace root: do NOT inherit this thread's active span. auto rootCtx = opentelemetry::context::Context{otel_trace::kIsRootSpanKey, true}; return SpanGuard( - std::make_unique(tel->startSpan(fullName, rootCtx, categoryToSpanKind(cat)))); + std::make_unique(tel->startSpan(*fullName, rootCtx, categoryToSpanKind(cat)))); } // ===== Child / linked span creation ======================================== From 3715b7a2a3318078e4154bf4cab045378ee97fd7 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 29 Jul 2026 16:02:19 +0100 Subject: [PATCH 3/3] fix(telemetry): correct RPC and gRPC span status reporting Six related defects in the RPC/gRPC span surface, all cases where a failure was recorded as success or an attribute was missing on an error path. GRPCServer: the non-exception branch set the span Ok unconditionally, then sent a possibly-failed grpc::Status. The handler can return a non-OK status without throwing, so every failed call traced as successful. Status now follows result.second, with the error message as the span description. ServerHandler: eight per-item error branches appended an error reply without recording that the request failed. Batch responses and ripplerpc < 3.0 always carry HTTP 200, so those failures were invisible and an entirely failed batch ended its span as successful. Added an appendItemError() helper next to the existing httpReplyError() lambda and routed all eight sites through it, so the flag cannot be forgotten at a new call site. ServerHandler: the early-return validation paths set the span error but not the rpc_status attribute. Added it to httpReplyError() so every such path gets it. RPCHandler: the fillHandler error path set only command and rpc_status, while callMethod sets command, version and rpc_role. Error spans were therefore not filterable by API version or role. The error path now mirrors that set. RPCHandler: resolveCommandSpanName() checked only that command/method were present, not that they agreed, while fillHandler rejects a mismatch as rpcUNKNOWN_COMMAND. A request supplying both with different values was labelled with one of the two names, misattributing the error to a command that never dispatched. It now mirrors fillHandler's rule and collapses to "unknown". ServerHandler: processRequest returned bool solely so the caller could set its span status. Telemetry should read state, not shape the signature of the code it observes, so the signature returns to void and rpc.process sets its own status from spanHadError. The enclosing rpc.http_request span now leaves status unset: the OTel spec has instrumentation leave status unset unless the operation itself errored, and reserves Ok for an operator asserting verified success. --- src/xrpld/app/main/GRPCServer.cpp | 15 +++++- src/xrpld/rpc/ServerHandler.h | 4 +- src/xrpld/rpc/detail/RPCHandler.cpp | 15 ++++++ src/xrpld/rpc/detail/ServerHandler.cpp | 69 ++++++++++++++------------ 4 files changed, 67 insertions(+), 36 deletions(-) diff --git a/src/xrpld/app/main/GRPCServer.cpp b/src/xrpld/app/main/GRPCServer.cpp index 566362bb0f..f217771025 100644 --- a/src/xrpld/app/main/GRPCServer.cpp +++ b/src/xrpld/app/main/GRPCServer.cpp @@ -243,8 +243,19 @@ GRPCServerImpl::CallData::process(std::shared_ptr result = handler_(context); setIsUnlimited(result.first, isUnlimited); - span.setAttribute(grpc_span::attr::grpcStatus, grpc_span::val::success); - span.setOk(); + // The handler can return a non-OK status without throwing, so + // the span status must follow result.second rather than assume + // success — otherwise every failed call traces as OK. + if (result.second.ok()) + { + span.setAttribute(grpc_span::attr::grpcStatus, grpc_span::val::success); + span.setOk(); + } + else + { + span.setAttribute(grpc_span::attr::grpcStatus, grpc_span::val::error); + span.setError(result.second.error_message()); + } responder_.Finish(result.first, result.second, this); } } diff --git a/src/xrpld/rpc/ServerHandler.h b/src/xrpld/rpc/ServerHandler.h index fbe487654e..5dfd2476da 100644 --- a/src/xrpld/rpc/ServerHandler.h +++ b/src/xrpld/rpc/ServerHandler.h @@ -194,10 +194,8 @@ private: /** * Process an RPC request and write the reply to `output`. - * @return false if the request resulted in an error response, true - * otherwise. Lets the caller's enclosing span reflect the outcome. */ - bool + void processRequest( Port const& port, std::string const& request, diff --git a/src/xrpld/rpc/detail/RPCHandler.cpp b/src/xrpld/rpc/detail/RPCHandler.cpp index 27dacdc560..8efe957435 100644 --- a/src/xrpld/rpc/detail/RPCHandler.cpp +++ b/src/xrpld/rpc/detail/RPCHandler.cpp @@ -238,6 +238,14 @@ resolveCommandSpanName(JsonContext const& context) if (!context.params.isMember(jss::command) && !context.params.isMember(jss::method)) return rpc_span::val::unknownCommand; + // fillHandler() rejects a request that supplies both fields with differing + // values as rpcUNKNOWN_COMMAND. Mirror that here, or the span would be + // labelled with one of the two names and misattribute the error to a + // command that was never dispatched. + if (context.params.isMember(jss::command) && context.params.isMember(jss::method) && + context.params[jss::command].asString() != context.params[jss::method].asString()) + return rpc_span::val::unknownCommand; + std::string const cmd = context.params.isMember(jss::command) ? context.params[jss::command].asString() : context.params[jss::method].asString(); @@ -261,6 +269,13 @@ doCommand(RPC::JsonContext& context, json::Value& result) auto const cmdName = resolveCommandSpanName(context); auto span = ScopedSpanGuard(TraceCategory::Rpc, rpc_span::prefix::command, cmdName); span.setAttribute(rpc_span::attr::command, cmdName); + // Mirror the attribute set callMethod() puts on a successful command + // span, so error spans stay filterable by API version and role. + span.setAttribute(rpc_span::attr::version, static_cast(context.apiVersion)); + span.setAttribute( + rpc_span::attr::rpcRole, + context.role == Role::ADMIN ? std::string_view(rpc_span::val::admin) + : std::string_view(rpc_span::val::user)); 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 4abee4c79f..0661d78826 100644 --- a/src/xrpld/rpc/detail/ServerHandler.cpp +++ b/src/xrpld/rpc/detail/ServerHandler.cpp @@ -593,7 +593,7 @@ ServerHandler::processSession( auto span = ScopedSpanGuard::freshRoot( TraceCategory::Rpc, rpc_span::prefix::rpc, rpc_span::op::httpRequest); - bool const ok = processRequest( + processRequest( session->port(), buffersToString(session->request().body().data()), session->remoteAddress().atPort(0), @@ -615,15 +615,11 @@ ServerHandler::processSession( { session->close(true); } - // Reflect the request outcome on the wrapper span instead of always OK. - if (ok) - { - span.setOk(); - } - else - { - span.setError(rpc_span::val::error); - } + // Status is left unset: the OTel spec says instrumentation should leave it + // unset unless the operation itself errored, and reserves Ok for an + // operator asserting success. This span only delimits the HTTP request and + // has no error of its own to report — the outcome is determined inside + // rpc.process, which sets its own status. } static json::Value @@ -642,7 +638,7 @@ constexpr json::Int kServerOverloaded = -32604; constexpr json::Int kForbidden = -32605; constexpr json::Int kWrongVersion = -32606; -bool +void ServerHandler::processRequest( Port const& port, std::string const& request, @@ -671,6 +667,7 @@ ServerHandler::processRequest( // (the span would otherwise end UNSET, invisible to {status.code=error}). auto httpReplyError = [&](int status, std::string const& message) { spanHadError = true; + span.setAttribute(rpc_span::attr::rpcStatus, rpc_span::val::error); span.setError(message); httpReply(status, message, output, rpcJ); }; @@ -682,7 +679,7 @@ ServerHandler::processRequest( !jsonOrig || !jsonOrig.isObject()) { httpReplyError(400, "Unable to parse request: " + reader.getFormattedErrorMessages()); - return false; + return; } } @@ -694,12 +691,23 @@ ServerHandler::processRequest( if (!jsonOrig.isMember(jss::params) || !jsonOrig[jss::params].isArray()) { httpReplyError(400, "Malformed batch request"); - return false; + return; } size = jsonOrig[jss::params].size(); } json::Value reply(batch ? json::ValueType::Array : json::ValueType::Object); + + // Append a per-request error item and record that the request failed. + // Batch responses (and ripplerpc < 3.0) always carry HTTP 200, so the + // span status can only learn about these failures through spanHadError. + // Every per-item error path must go through here, or an entirely failed + // batch would end its span as successful. + auto appendItemError = [&](json::Value&& item) { + spanHadError = true; + reply.append(std::move(item)); + }; + auto const start(std::chrono::high_resolution_clock::now()); for (unsigned i = 0; i < size; ++i) { @@ -710,7 +718,7 @@ ServerHandler::processRequest( json::Value r(json::ValueType::Object); r[jss::request] = jsonRPC; r[jss::error] = makeJsonError(kMethodNotFound, "Method not found"); - reply.append(r); + appendItemError(std::move(r)); continue; } @@ -733,12 +741,12 @@ ServerHandler::processRequest( if (!batch) { httpReplyError(400, jss::invalid_API_version.cStr()); - return false; + return; } json::Value r(json::ValueType::Object); r[jss::request] = jsonRPC; r[jss::error] = makeJsonError(kWrongVersion, jss::invalid_API_version.cStr()); - reply.append(r); + appendItemError(std::move(r)); continue; } @@ -776,11 +784,11 @@ ServerHandler::processRequest( if (!batch) { httpReplyError(503, "Server is overloaded"); - return false; + return; } json::Value r = jsonRPC; r[jss::error] = makeJsonError(kServerOverloaded, "Server is overloaded"); - reply.append(r); + appendItemError(std::move(r)); continue; } } @@ -791,11 +799,11 @@ ServerHandler::processRequest( if (!batch) { httpReplyError(403, "Forbidden"); - return false; + return; } json::Value r = jsonRPC; r[jss::error] = makeJsonError(kForbidden, "Forbidden"); - reply.append(r); + appendItemError(std::move(r)); continue; } @@ -805,11 +813,11 @@ ServerHandler::processRequest( if (!batch) { httpReplyError(400, "Null method"); - return false; + return; } json::Value r = jsonRPC; r[jss::error] = makeJsonError(kMethodNotFound, "Null method"); - reply.append(r); + appendItemError(std::move(r)); continue; } @@ -820,11 +828,11 @@ ServerHandler::processRequest( if (!batch) { httpReplyError(400, "method is not string"); - return false; + return; } json::Value r = jsonRPC; r[jss::error] = makeJsonError(kMethodNotFound, "method is not string"); - reply.append(r); + appendItemError(std::move(r)); continue; } @@ -835,11 +843,11 @@ ServerHandler::processRequest( if (!batch) { httpReplyError(400, "method is empty"); - return false; + return; } json::Value r = jsonRPC; r[jss::error] = makeJsonError(kMethodNotFound, "method is empty"); - reply.append(r); + appendItemError(std::move(r)); continue; } @@ -861,7 +869,7 @@ ServerHandler::processRequest( { usage.charge(Resource::kFeeMalformedRpc); httpReplyError(400, "params unparsable"); - return false; + return; } else { @@ -870,7 +878,7 @@ ServerHandler::processRequest( { usage.charge(Resource::kFeeMalformedRpc); httpReplyError(400, "params unparsable"); - return false; + return; } } } @@ -888,12 +896,12 @@ ServerHandler::processRequest( if (!batch) { httpReplyError(400, "ripplerpc is not a string"); - return false; + return; } json::Value r = jsonRPC; r[jss::error] = makeJsonError(kMethodNotFound, "ripplerpc is not a string"); - reply.append(r); + appendItemError(std::move(r)); continue; } ripplerpc = params[jss::ripplerpc].asString(); @@ -1094,7 +1102,6 @@ ServerHandler::processRequest( span.setOk(); } httpReply(httpStatus, response, output, rpcJ); - return !spanHadError; } //------------------------------------------------------------------------------