Merge branch 'pratik/otel-phase3-tx-tracing' into pratik/otel-phase4-consensus-tracing

src/xrpld/app/main/GRPCServer.cpp

@@ -243,8 +243,19 @@ GRPCServerImpl::CallData<Request, Response>::process(std::shared_ptr<JobQueue::C
             {
                 std::pair<Response, grpc::Status> 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);
             }
         }

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,

src/xrpld/rpc/detail/RPCHandler.cpp

@@ -239,6 +239,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();
@@ -262,6 +270,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<int64_t>(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());
 

src/xrpld/rpc/detail/ServerHandler.cpp

@@ -606,7 +606,7 @@ ServerHandler::processSession(
     auto const requestBody = ::xrpl::buffersToString(session->request().body().data());
     span.setAttribute(rpc_span::attr::requestPayloadSize, static_cast<int64_t>(requestBody.size()));
 
-    bool const ok = processRequest(
+    processRequest(
         session->port(),
         requestBody,
         session->remoteAddress().atPort(0),
@@ -628,15 +628,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
@@ -655,7 +651,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,
@@ -684,6 +680,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);
     };
@@ -695,7 +692,7 @@ ServerHandler::processRequest(
             !jsonOrig || !jsonOrig.isObject())
         {
             httpReplyError(400, "Unable to parse request: " + reader.getFormattedErrorMessages());
-            return false;
+            return;
         }
     }
 
@@ -707,7 +704,7 @@ ServerHandler::processRequest(
         if (!jsonOrig.isMember(jss::params) || !jsonOrig[jss::params].isArray())
         {
             httpReplyError(400, "Malformed batch request");
-            return false;
+            return;
         }
         size = jsonOrig[jss::params].size();
     }
@@ -716,6 +713,17 @@ ServerHandler::processRequest(
         span.setAttribute(rpc_span::attr::batchSize, static_cast<int64_t>(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)
     {
@@ -726,7 +734,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;
         }
 
@@ -749,12 +757,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;
         }
 
@@ -792,11 +800,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;
             }
         }
@@ -807,11 +815,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;
         }
 
@@ -821,11 +829,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;
         }
 
@@ -836,11 +844,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;
         }
 
@@ -851,11 +859,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;
         }
 
@@ -877,7 +885,7 @@ ServerHandler::processRequest(
             {
                 usage.charge(Resource::kFeeMalformedRpc);
                 httpReplyError(400, "params unparsable");
-                return false;
+                return;
             }
             else
             {
@@ -886,7 +894,7 @@ ServerHandler::processRequest(
                 {
                     usage.charge(Resource::kFeeMalformedRpc);
                     httpReplyError(400, "params unparsable");
-                    return false;
+                    return;
                 }
             }
         }
@@ -904,12 +912,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();
@@ -1110,7 +1118,6 @@ ServerHandler::processRequest(
         span.setOk();
     }
     httpReply(httpStatus, response, output, rpcJ);
-    return !spanHadError;
 }
 
 //------------------------------------------------------------------------------