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] 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; } //------------------------------------------------------------------------------