Merge branch 'pratik/otel-phase1c-rpc-integration' into pratik/otel-phase2-rpc-tracing

RPCHandler.cpp composed both sides: phase-1c's null-guard and reply-aware
status logic, keeping this branch's load_type attribute inside that guard
because its argument allocates.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

src/test/rpc/RPCHandler_test.cpp

@@ -0,0 +1,176 @@
+#include <test/jtx/Env.h>
+
+#include <xrpld/app/main/Application.h>
+#include <xrpld/rpc/Context.h>
+#include <xrpld/rpc/RPCHandler.h>
+#include <xrpld/rpc/Role.h>
+#include <xrpld/rpc/Status.h>
+#include <xrpld/rpc/detail/Tuning.h>
+
+#include <xrpl/basics/scope.h>
+#include <xrpl/beast/unit_test/suite.h>
+#include <xrpl/core/Job.h>
+#include <xrpl/core/JobQueue.h>
+#include <xrpl/core/ServiceRegistry.h>
+#include <xrpl/json/json_value.h>
+#include <xrpl/protocol/ApiVersion.h>
+#include <xrpl/protocol/ErrorCodes.h>
+#include <xrpl/protocol/jss.h>
+#include <xrpl/resource/Charge.h>
+#include <xrpl/resource/Consumer.h>
+#include <xrpl/resource/Fees.h>
+
+#include <exception>
+#include <future>
+#include <string>
+
+namespace xrpl::test {
+
+/**
+ * Checks the error a busy server reports for a request it never dispatches.
+ *
+ *   RPCHandler_test ──doCommand()──> RPC::fillHandler()
+ *          │                                │
+ *          └── fills ──> JobQueue <── reads ─┘
+ *
+ * An overloaded server answers rpcTOO_BUSY before it reads the command name, so
+ * the request fields still hold whatever json type the client sent. Anything
+ * that runs afterwards to describe the error has to cope with that and leave
+ * the answer alone.
+ *
+ * @note Each testcase keeps one job-queue worker blocked for as long as it
+ * runs, and releases it before returning.
+ */
+class RPCHandler_test : public beast::unit_test::Suite
+{
+    /**
+     * How many jobs to queue to hold the server over its overload threshold.
+     * One job is dispatched straight away, so one spare keeps the waiting
+     * count above the limit.
+     */
+    static constexpr int kOverloadJobs = rpc::tuning::kMaxJobQueueClients + 2;
+
+    /**
+     * Dispatches one request on an overloaded server and checks the client is
+     * told the server is busy.
+     *
+     * @param params Request fields, in the form fillHandler() reads them.
+     */
+    void
+    expectTooBusy(json::Value const& params)
+    {
+        using namespace jtx;
+        Env env{*this};
+        auto& app = env.app();
+
+        // Only one job of this type runs at a time, so every job after the
+        // first stays queued until the gate opens. They also sort above
+        // JtClient, the priority the overload check counts from.
+        std::promise<void> gate;
+        std::shared_future<void> const open = gate.get_future().share();
+        ScopeExit const openGate{[&gate]() { gate.set_value(); }};
+
+        int queued = 0;
+        for (int i = 0; i < kOverloadJobs; ++i)
+        {
+            if (app.getJobQueue().addJob(JtSweep, "overload", [open]() { open.wait(); }))
+                ++queued;
+        }
+        BEAST_EXPECT(queued == kOverloadJobs);
+        BEAST_EXPECT(app.getJobQueue().getJobCountGE(JtClient) > rpc::tuning::kMaxJobQueueClients);
+
+        resource::Charge loadType = resource::kFeeReferenceRpc;
+        resource::Consumer consumer;
+        rpc::JsonContext context{
+            {.j = env.journal,
+             .app = app,
+             .loadType = loadType,
+             .netOps = app.getOPs(),
+             .ledgerMaster = app.getLedgerMaster(),
+             .consumer = consumer,
+             .role = Role::USER,
+             .coro = {},
+             .infoSub = {},
+             .apiVersion = rpc::kApiVersionIfUnspecified},
+            params,
+            {}};
+
+        json::Value result;
+        rpc::Status status;
+        std::string thrown;
+        try
+        {
+            status = rpc::doCommand(context, result);
+        }
+        catch (std::exception const& e)
+        {
+            thrown = e.what();
+        }
+
+        if (BEAST_EXPECTS(thrown.empty(), "doCommand threw: " + thrown))
+        {
+            BEAST_EXPECT(status.type() == rpc::Status::Type::ErrorCodeI);
+            BEAST_EXPECT(status.toErrorCode() == RpcTooBusy);
+            BEAST_EXPECT(result[jss::error].asString() == "tooBusy");
+            BEAST_EXPECT(result[jss::error_code].asInt() == static_cast<int>(RpcTooBusy));
+        }
+    }
+
+    /**
+     * Checks a well-formed request on an overloaded server. This is the control
+     * for the two cases below: it shares their fixture and their assertions,
+     * and differs only in that every field it sends is a string.
+     */
+    void
+    testRegisteredCommand()
+    {
+        testcase("Busy server, registered command");
+
+        json::Value params = json::ValueType::Object;
+        params[jss::command] = "ping";
+        expectTooBusy(params);
+    }
+
+    /**
+     * Checks a request whose "method" field is not a string.
+     */
+    void
+    testNonStringMethod()
+    {
+        testcase("Busy server, method field is not a string");
+
+        // The HTTP path sets "command" from the outer method name it has
+        // already checked, and passes the inner request object through
+        // untouched, so "method" can arrive holding any json type.
+        json::Value params = json::ValueType::Object;
+        params[jss::command] = "ping";
+        params[jss::method] = json::ValueType::Array;
+        expectTooBusy(params);
+    }
+
+    /**
+     * Checks a request whose "command" field is not a string.
+     */
+    void
+    testNonStringCommand()
+    {
+        testcase("Busy server, command field is not a string");
+
+        json::Value params = json::ValueType::Object;
+        params[jss::command] = json::ValueType::Object;
+        expectTooBusy(params);
+    }
+
+public:
+    void
+    run() override
+    {
+        testRegisteredCommand();
+        testNonStringMethod();
+        testNonStringCommand();
+    }
+};
+
+BEAST_DEFINE_TESTSUITE(RPCHandler, rpc, xrpl);
+
+}  // namespace xrpl::test

src/xrpld/rpc/detail/RPCHandler.cpp

@@ -158,6 +158,42 @@ fillHandler(JsonContext& context, Handler const*& result)
     return RpcSuccess;
 }
 
+/**
+ * Names the reason a command failed, for the span's error description.
+ *
+ * jss::error holds the error token, and an old-style handler reports it there
+ * and nowhere else. A failure the reply does not name is described by the
+ * status's own error code, which is the only reason left to report. Every
+ * token is a compile-time string, so no request text reaches the description.
+ *
+ * @param status What the handler returned.
+ * @param result The reply the handler filled in. The returned view can point
+ * into it, so result must outlive the view.
+ * @param replyHasError The caller's containsError(result), passed in so the
+ * reply is not searched twice.
+ * @return The error token, or "error" where neither source carries one.
+ */
+std::string_view
+errorDescription(Status const& status, json::Value const& result, bool replyHasError)
+{
+    if (replyHasError && result[jss::error].isString())
+    {
+        // asCString() asserts the type, then hands back the stored pointer
+        // unchecked, and a string-typed json::Value may hold a null one. Both
+        // checks are needed before that pointer becomes a view.
+        if (char const* const token = result[jss::error].asCString(); token != nullptr)
+            return token;
+    }
+
+    // A TER or a bare integer code has no token in the error registry, so
+    // reading one would name an unrelated error. getErrorInfo() returns a
+    // reference into a static table, so its token outlives this call.
+    if (status.type() == Status::Type::ErrorCodeI)
+        return getErrorInfo(status.toErrorCode()).token.cStr();
+
+    return rpc_span::val::error;
+}
+
 Status
 callMethod(JsonContext& context, Handler::Method method, std::string_view name, json::Value& result)
 {
@@ -190,23 +226,33 @@ callMethod(JsonContext& context, Handler::Method method, std::string_view name,
         JLOG(context.j.debug()) << "RPC call " << name << " completed in "
                                 << ((end - start).count() / 1000000000.0) << "seconds";
         perfLog.rpcFinish(name, curId);
-        span.setAttribute(rpc_span::attr::loadType, context.loadType.label().c_str());
-        // Status::operator bool() returns true when there IS an error
-        // (code_ != OK), so the ternary correctly maps error->error, ok->success.
-        span.setAttribute(
-            rpc_span::attr::rpcStatus,
-            ret ? std::string_view{rpc_span::val::error}
-                : std::string_view{rpc_span::val::success});
-        // Reflect the result in the OTel span status, not just the attribute,
-        // so non-exception RPC errors (rpcTOO_BUSY, rpcNO_PERMISSION, ...) are
-        // visible to {status.code=error} queries.
-        if (ret)
+        // Everything in here only feeds the span, and searching the reply is
+        // not free, so a null guard pays for none of it. setError() and
+        // setAttribute() are no-ops on a null guard, but their arguments are
+        // not: with telemetry compiled out operator bool() is a constant false.
+        if (span)
         {
-            span.setError(rpc_span::val::error);
-        }
-        else
-        {
-            span.setOk();
+            // Read after the handler ran, because a handler may raise its own
+            // load type (pathfind charges a heavy burden).
+            span.setAttribute(rpc_span::attr::loadType, context.loadType.label().c_str());
+            // An old-style handler reports its error in the reply, not in the
+            // Status: byRef() returns a default Status whatever happened.
+            // Reading both covers every handler. Status::operator bool() is
+            // true when there IS an error.
+            bool const replyHasError = containsError(result);
+            bool const failed = static_cast<bool>(ret) || replyHasError;
+            // Two values only. rpc_status is a spanmetrics dimension, so every
+            // value it can take becomes a Prometheus label and a metric series.
+            span.setAttribute(
+                rpc_span::attr::rpcStatus,
+                failed ? std::string_view{rpc_span::val::error}
+                       : std::string_view{rpc_span::val::success});
+            // Error so a failed call answers {status.code=error}, for the codes
+            // that never throw (rpcTOO_BUSY, rpcNO_PERMISSION, ...). Success
+            // stays Unset: the spec reserves Ok for an operator asserting
+            // verified success, and a tool may read it as suppressing errors.
+            if (failed)
+                span.setError(errorDescription(ret, result, replyHasError));
         }
         return ret;
     }
@@ -234,32 +280,37 @@ callMethod(JsonContext& context, Handler::Method method, std::string_view name,
 #ifdef XRPL_ENABLE_TELEMETRY
 
 // Resolve the span suffix / command attribute for a request that failed in
-// fillHandler. Returns the canonical handler name for a recognized command
-// (a finite, bounded set) or the literal "unknown" for a request that omits
-// both fields or names an unregistered command. The raw request value is
-// deliberately NOT used: the command attribute is promoted to a Prometheus
-// label by the spanmetrics connector, so an attacker-controlled string would
-// let arbitrary request input drive unbounded span-name / label cardinality.
-// Resolving against the registry keeps per-command error attribution for real
-// commands (e.g. a submit rejected with rpcTOO_BUSY stays rpc.command.submit)
-// while collapsing garbage input to a single series.
+// fillHandler. The name comes from the handler registry, so only a registered
+// handler name or the "unknown" label can reach the span; request text never
+// does. That bounded set also bounds the Prometheus label the spanmetrics
+// connector derives from it, and a real command still keeps its own error
+// attribution: a submit rejected with rpcTOO_BUSY stays rpc.command.submit.
 std::string_view
 resolveCommandSpanName(JsonContext const& context)
 {
-    if (!context.params.isMember(jss::command) && !context.params.isMember(jss::method))
+    bool const hasCommand = context.params.isMember(jss::command);
+    bool const hasMethod = context.params.isMember(jss::method);
+
+    if (!hasCommand && !hasMethod)
+        return rpc_span::val::unknownCommand;
+
+    // A json array or object throws when asked for its string value, and no
+    // non-string field names a handler. The reply's error code is already
+    // decided, so naming the span must not be able to change it.
+    if ((hasCommand && !context.params[jss::command].isString()) ||
+        (hasMethod && !context.params[jss::method].isString()))
         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) &&
+    if (hasCommand && hasMethod &&
         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();
+    std::string const cmd = hasCommand ? context.params[jss::command].asString()
+                                       : context.params[jss::method].asString();
 
     auto const* handler = getHandler(context.apiVersion, context.app.config().betaRpcApi, cmd);
     return (handler != nullptr) ? std::string_view{handler->name}

src/xrpld/rpc/detail/ServerHandler.cpp

@@ -391,6 +391,10 @@ ServerHandler::onWSMessage(
         // Fresh root so each WS message is its own trace.
         auto span = ScopedSpanGuard::freshRoot(
             TraceCategory::Rpc, rpc_span::prefix::rpc, rpc_span::op::wsMessage);
+        // rpc_status is a span-metrics dimension, so leaving it unset emits a
+        // series with a blank label and hides this failure from any query that
+        // selects on error.
+        span.setAttribute(rpc_span::attr::rpcStatus, rpc_span::val::error);
         span.setError(rpc_span::val::invalidJson);
 
         json::Value jvResult(json::ValueType::Object);