Merge branch 'pratik/otel-phase9-metric-gap-fill' into pratik/otel-phase10-workload-validation

docker/telemetry/otel-collector-config.grafanacloud.yaml

@@ -123,15 +123,6 @@ processors:
           - set(attributes["service_instance_id"], resource.attributes["service.instance.id"])
           - set(attributes["deployment_environment"], resource.attributes["deployment.environment"])
           - set(attributes["xrpl_network_type"], resource.attributes["xrpl.network.type"])
-  # Copy the per-node id from the resource onto every span so TraceQL can
-  # filter by node (span.service_instance_id). The name matches the metric
-  # label written by transform/cloudlabels, so the dashboards' $node variable
-  # applies to both signals.
-  transform/spanidentity:
-    trace_statements:
-      - context: span
-        statements:
-          - set(attributes["service_instance_id"], resource.attributes["service.instance.id"])
 
 connectors:
   spanmetrics:
@@ -265,20 +256,9 @@ service:
       exporters: [spanmetrics]
     # Trace-STORAGE branch: 0.5% probabilistic tail sampling before Tempo
     # and Grafana Cloud, so stored trace volume is ~1/200 of ingested spans.
-    # transform/spanidentity runs after tail_sampling so only retained spans
-    # pay for the copy. traces/metrics does not need it: spanmetrics already
-    # groups by the service.instance.id resource attribute
-    # (resource_metrics_key_attributes).
     traces/store:
       receivers: [otlp]
-      processors:
-        [
-          tail_sampling,
-          resource/tier,
-          resource/stripsdk,
-          transform/spanidentity,
-          batch,
-        ]
+      processors: [tail_sampling, resource/tier, resource/stripsdk, batch]
       exporters: [otlp/tempo, otlphttp/grafanacloud]
     # The local Prometheus scrape promotes tier/instance resource attrs to
     # labels via resource_to_telemetry_conversion; Grafana Cloud (OTLP) does

docker/telemetry/otel-collector-config.yaml

@@ -124,14 +124,6 @@ processors:
         action: hash
       - key: pathfind_dest_account
         action: hash
-  # Copy the per-node id from the resource onto every span so TraceQL can
-  # filter by node (span.service_instance_id), matching the service_instance_id
-  # metric label the dashboards' $node variable already uses.
-  transform/spanidentity:
-    trace_statements:
-      - context: span
-        statements:
-          - set(attributes["service_instance_id"], resource.attributes["service.instance.id"])
 
 connectors:
   spanmetrics:
@@ -249,14 +241,7 @@ service:
   pipelines:
     traces:
       receivers: [otlp]
-      processors:
-        [
-          resource/tier,
-          resource/stripsdk,
-          attributes/hash,
-          transform/spanidentity,
-          batch,
-        ]
+      processors: [resource/tier, resource/stripsdk, attributes/hash, batch]
       exporters: [debug, otlp/tempo, spanmetrics]
     metrics:
       receivers: [otlp, spanmetrics]

docs/telemetry-runbook.md

@@ -146,15 +146,11 @@ curl -s http://localhost:5015 -d '{"method":"server_info"}' |
 | `tls_client_cert`          | (empty)                           | Client cert (PEM) for mutual TLS; empty = one-way TLS        |
 | `tls_client_key`           | (empty)                           | Private key (PEM) for `tls_client_cert`                      |
 
-> **`service_instance_id` reaches traces as a span attribute too.** xrpld sends
-> it as the `service.instance.id` resource attribute; the collector's
-> `transform/spanidentity` processor copies it onto every span as
-> `service_instance_id`, so TraceQL can filter per node
-> (`{span.service_instance_id="validator-0"}`) with the same value the `$node`
-> dashboard variable uses for metrics. Spans recorded before that processor was
-> added do not carry it, and a TraceQL regex does **not** match a missing
-> attribute — so a `$node` filter on a trace panel returns nothing for older
-> data.
+> **Traces and metrics also carry `xrpl.node.id`.** xrpld sets it as a resource
+> attribute alongside `service.instance.id`; the value is the node public key
+> (base58, begins with `n`). It comes from the node identity unconditionally, so
+> it is present even when `[telemetry] service_instance_id` is configured.
+> TraceQL filters on it as `resource.xrpl.node.id`.
 
 > **`consensus_trace_strategy` is not validated.** The parser copies the raw
 > string through (`TelemetryConfig.cpp:155-156`) and the only equality test in

include/xrpl/telemetry/SpanNames.h

@@ -108,6 +108,7 @@ inline constexpr auto consensus = makeStr("consensus");
 inline constexpr auto peer = makeStr("peer");
 inline constexpr auto ledger = makeStr("ledger");
 inline constexpr auto network = makeStr("network");
+inline constexpr auto node = makeStr("node");
 inline constexpr auto link = makeStr("link");
 }  // namespace seg
 
@@ -117,6 +118,16 @@ namespace attr {
 inline constexpr auto networkId = join(join(seg::xrpl, seg::network), makeStr("id"));
 inline constexpr auto networkType = join(join(seg::xrpl, seg::network), makeStr("type"));
 
+/**
+ * Resource attribute `xrpl.node.id` — the node's base58 public key.
+ *
+ * Dotted form, like its siblings above, because it is a process-identity
+ * value stamped once on the OTel resource rather than a per-span attribute.
+ * It gives traces and metrics a stable per-node key alongside
+ * `service.instance.id`.
+ */
+inline constexpr auto nodeId = join(join(seg::xrpl, seg::node), makeStr("id"));
+
 /**
  * Canonical shared attrs (rule 5 — <domain>_<field> underscore form).
  *

include/xrpl/telemetry/Telemetry.h

@@ -83,7 +83,8 @@
  *
  * @note Thread safety: The Telemetry interface is safe for concurrent reads
  * (isEnabled, shouldTrace*, getTracer, startSpan) after start() completes.
- * setServiceInstanceId() must be called before start() and is not thread-safe.
+ * setServiceInstanceId() and setNodeId() must be called before start() and
+ * are not thread-safe.
  * The OTel SDK's TracerProvider and Tracer are internally thread-safe.
  */
 
@@ -193,6 +194,14 @@ public:
          */
         std::string serviceInstanceId;
 
+        /**
+         * OTel resource attribute `xrpl.node.id`: the node's base58-encoded
+         * public key. Always the node identity, never config-supplied, so it
+         * stays a stable per-node key even when serviceInstanceId is
+         * overridden by [telemetry] service_instance_id.
+         */
+        std::string nodeId;
+
         /**
          * OTLP/HTTP endpoint URL where spans are sent.
          */
@@ -313,6 +322,24 @@ public:
         (void)id;
     }
 
+    /**
+     * Update the node ID (OTel resource attribute `xrpl.node.id`).
+     *
+     * Must be called before start(). A setter is needed for the same reason
+     * setServiceInstanceId() needs one: the node public key is not available
+     * when Telemetry is constructed (during the ApplicationImp member
+     * initializer list), so Application::setup() injects it once
+     * nodeIdentity_ is known.
+     *
+     * @param id  The node's base58-encoded public key.
+     */
+    virtual void
+    setNodeId(std::string const& id)
+    {
+        // Default no-op for NullTelemetry implementations.
+        (void)id;
+    }
+
     /**
      * Initialize the tracing pipeline (exporter, processor, provider).
      * Call after construction.

src/libxrpl/telemetry/Telemetry.cpp

@@ -283,8 +283,8 @@ class TelemetryImpl : public Telemetry
 {
     /**
      * Configuration from the [telemetry] config section.
-     * Non-const so setServiceInstanceId() can update the instance ID
-     * before start() creates the OTel resource.
+     * Non-const so setServiceInstanceId() and setNodeId() can update the
+     * identity attributes before start() creates the OTel resource.
      */
     Setup setup_;
 
@@ -343,6 +343,12 @@ public:
         setup_.serviceInstanceId = id;
     }
 
+    void
+    setNodeId(std::string const& id) override
+    {
+        setup_.nodeId = id;
+    }
+
     void
     start() override
     {
@@ -384,6 +390,7 @@ public:
             {std::string(attr::networkId),
              static_cast<int64_t>(setup_.networkId)},              // LCOV_EXCL_LINE
             {std::string(attr::networkType), setup_.networkType},  // LCOV_EXCL_LINE
+            {std::string(attr::nodeId), setup_.nodeId},            // LCOV_EXCL_LINE
         });
 
         // Configure sampler. Head sampling is fixed at 1.0 (sample everything);
@@ -442,6 +449,8 @@ public:
      * during ApplicationImp's member-init list. The metrics resource uses
      * setup_.serviceInstanceId from config; it is immutable once the provider
      * is built, so a later node-key setServiceInstanceId() does not affect it.
+     * The same applies to setNodeId(): xrpl.node.id reaches this resource only
+     * if setup_.nodeId is already populated when the constructor runs.
      */
     void
     initMetrics()
@@ -479,16 +488,7 @@ public:
         auto reader = metrics_sdk::PeriodicExportingMetricReaderFactory::Create(
             std::move(metricExporter), readerOpts);
 
-        // Metrics resource: same attributes as the tracer resource so metrics
-        // and traces share one identity. Built here (not shared with start())
-        // because start() runs later; serviceInstanceId comes from config.
-        auto resourceAttrs = resource::Resource::Create({
-            {opentelemetry::semconv::service::kServiceName, setup_.serviceName},
-            {opentelemetry::semconv::service::kServiceVersion, setup_.serviceVersion},
-            {opentelemetry::semconv::service::kServiceInstanceId, setup_.serviceInstanceId},
-            {std::string(attr::networkId), static_cast<int64_t>(setup_.networkId)},
-            {std::string(attr::networkType), setup_.networkType},
-        });
+        auto resourceAttrs = makeMetricsResource();
 
         // Create MeterProvider with the shared resource, then attach reader.
         meterProvider_ = metrics_sdk::MeterProviderFactory::Create(

src/tests/libxrpl/telemetry/NodeIdResource.cpp

@@ -0,0 +1,162 @@
+#include <xrpl/beast/utility/Journal.h>
+#include <xrpl/config/BasicConfig.h>
+#include <xrpl/telemetry/SpanNames.h>
+#include <xrpl/telemetry/Telemetry.h>
+
+#include <gtest/gtest.h>
+
+#include <string>
+#include <string_view>
+
+#ifdef XRPL_ENABLE_TELEMETRY
+#include <opentelemetry/nostd/variant.h>
+#include <opentelemetry/sdk/resource/resource.h>
+#endif
+
+/**
+ * Contract tests for the `xrpl.node.id` resource attribute.
+ *
+ *  `xrpl.node.id` carries the node's base58 public key on both the trace and
+ *  the metric OTel resource, so traces and metrics resolve to one node. The
+ *  key string is a cross-component contract: the collector, TraceQL queries
+ *  and Grafana dashboards all name it literally, and a silent rename would
+ *  break them with no compile error. These tests pin the literal key, the
+ *  Setup default, and the fact that the value can only arrive through
+ *  Telemetry::setNodeId().
+ *
+ *  Scope limit: the two production resources are built inside TelemetryImpl
+ *  (trace resource in start(), metric resource in the constructor) and inside
+ *  MetricsRegistry::initExporterAndProvider(). Neither is reachable from this
+ *  binary — TelemetryImpl only exists behind an OTLP/HTTP exporter with
+ *  background export threads, which a unit test must not spin up (see
+ *  GetMeter.cpp), and MetricsRegistry.cpp is not compiled into xrpl_tests in
+ *  the telemetry-enabled build. The resource test below therefore pins the SDK
+ *  contract those three call sites rely on: the exact key, and a std::string
+ *  value landing in the string alternative of the attribute variant rather
+ *  than the bool one.
+ */
+
+using namespace xrpl;
+using namespace xrpl::telemetry;
+
+TEST(NodeIdResource, attribute_key_is_dotted_resource_form)
+{
+    // The literal the collector, TraceQL and the dashboards all name.
+    EXPECT_EQ(std::string_view(attr::nodeId), "xrpl.node.id");
+
+    // Dotted, not the underscore form used for span attributes.
+    EXPECT_EQ(std::string_view(attr::nodeId).find('_'), std::string_view::npos);
+
+    // Sibling of the other two xrpl.* resource attributes, and distinct
+    // from both.
+    EXPECT_EQ(std::string_view(attr::networkId), "xrpl.network.id");
+    EXPECT_EQ(std::string_view(attr::networkType), "xrpl.network.type");
+    EXPECT_NE(std::string_view(attr::nodeId), std::string_view(attr::networkId));
+    EXPECT_NE(std::string_view(attr::nodeId), std::string_view(attr::networkType));
+
+    // Built from the shared segments, so the segment additions are exercised
+    // too rather than only the joined result.
+    EXPECT_EQ(std::string_view(seg::node), "node");
+    EXPECT_EQ(std::string_view(seg::xrpl), "xrpl");
+}
+
+TEST(NodeIdResource, setup_node_id_defaults_to_empty)
+{
+    // Negative path: nothing has called setNodeId(), so there is no value to
+    // stamp and the resource builders skip the attribute.
+    Telemetry::Setup const s;
+    EXPECT_TRUE(s.nodeId.empty());
+    EXPECT_EQ(s.nodeId, "");
+}
+
+TEST(NodeIdResource, config_parsing_never_populates_node_id)
+{
+    // nodeId is deliberately not config-driven. Even with an explicit
+    // service_instance_id and a node public key argument, makeTelemetrySetup()
+    // must leave nodeId empty: Application::setup() is the only writer, via
+    // setNodeId().
+    Section section;
+    section.set("enabled", "1");
+    section.set("service_instance_id", "custom-id");
+
+    auto const setup = makeTelemetrySetup(section, "nHUtest123", "2.0.0", 1);
+
+    EXPECT_EQ(setup.serviceInstanceId, "custom-id");
+    EXPECT_TRUE(setup.nodeId.empty());
+}
+
+TEST(NodeIdResource, set_node_id_on_disabled_path_is_inert)
+{
+    // The disabled build/config path takes the base-class no-op. Calling it
+    // must be safe and must not change any observable state.
+    Telemetry::Setup setup;
+    setup.enabled = false;
+
+    beast::Journal::Sink& sink = beast::Journal::getNullSink();
+    beast::Journal const journal(sink);
+    auto telemetry = makeTelemetry(setup, journal);
+    ASSERT_NE(telemetry, nullptr);
+
+    telemetry->setNodeId("nHUtest123");
+
+    EXPECT_FALSE(telemetry->isEnabled());
+    EXPECT_FALSE(telemetry->shouldTraceRpc());
+    EXPECT_FALSE(telemetry->shouldTraceTransactions());
+    EXPECT_FALSE(telemetry->shouldTraceConsensus());
+    EXPECT_FALSE(telemetry->shouldTracePeer());
+    EXPECT_FALSE(telemetry->shouldTraceLedger());
+    EXPECT_EQ(telemetry->getConsensusTraceStrategy(), "deterministic");
+}
+
+#ifdef XRPL_ENABLE_TELEMETRY
+
+TEST(NodeIdResource, resource_carries_node_id_as_a_string)
+{
+    namespace otel_resource = opentelemetry::sdk::resource;
+
+    // A base58 node public key: 'n' prefix, 52 characters.
+    std::string const nodeId = "n9MozjnGB3tpULewtTsVtuudg5JqYFyV3QFdAtVLzJaxHcBaxuXM";
+    ASSERT_EQ(nodeId.size(), 52u);
+
+    otel_resource::ResourceAttributes attrs;
+    // std::string, never a string literal: the attribute variant's
+    // char-const* overload binds to bool, which would record `true`.
+    attrs[std::string(attr::nodeId)] = nodeId;
+
+    auto const resource = otel_resource::Resource::Create(attrs);
+    auto const& out = resource.GetAttributes();
+
+    auto const it = out.find("xrpl.node.id");
+    ASSERT_NE(it, out.end());
+
+    // The string alternative, not bool — the pitfall the call sites guard.
+    ASSERT_TRUE(opentelemetry::nostd::holds_alternative<std::string>(it->second));
+    EXPECT_FALSE(opentelemetry::nostd::holds_alternative<bool>(it->second));
+    EXPECT_EQ(opentelemetry::nostd::get<std::string>(it->second), nodeId);
+}
+
+TEST(NodeIdResource, resource_omits_node_id_when_it_was_never_set)
+{
+    namespace otel_resource = opentelemetry::sdk::resource;
+
+    // Negative path: the call sites only assign when the value is non-empty,
+    // so an unset node ID leaves the key off the resource entirely rather
+    // than stamping a blank one.
+    Telemetry::Setup const setup;
+    ASSERT_TRUE(setup.nodeId.empty());
+
+    otel_resource::ResourceAttributes attrs;
+    if (!setup.nodeId.empty())
+        attrs[std::string(attr::nodeId)] = setup.nodeId;
+
+    auto const resource = otel_resource::Resource::Create(attrs);
+    auto const& out = resource.GetAttributes();
+
+    EXPECT_EQ(out.find("xrpl.node.id"), out.end());
+
+    // The SDK still merges in its own defaults, so the absence above is a
+    // real absence and not an empty map.
+    EXPECT_FALSE(out.empty());
+}
+
+#endif  // XRPL_ENABLE_TELEMETRY

src/tests/libxrpl/telemetry/TelemetryConfig.cpp

@@ -15,6 +15,7 @@ TEST(TelemetryConfig, setup_defaults)
     EXPECT_EQ(s.serviceName, "xrpld");
     EXPECT_TRUE(s.serviceVersion.empty());
     EXPECT_TRUE(s.serviceInstanceId.empty());
+    EXPECT_TRUE(s.nodeId.empty());
     EXPECT_EQ(s.exporterEndpoint, "http://localhost:4318/v1/traces");
     EXPECT_FALSE(s.useTls);
     EXPECT_TRUE(s.tlsCertPath.empty());

src/xrpld/app/main/Application.cpp

@@ -1322,6 +1322,11 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline)
     if (!config_->section("telemetry").exists("service_instance_id"))
         telemetry_->setServiceInstanceId(toBase58(TokenType::NodePublic, nodeIdentity_->first));
 
+    // xrpl.node.id always carries the node public key. Unlike
+    // service_instance_id it is not configurable, so traces and metrics keep a
+    // stable per-node key whatever [telemetry] says.
+    telemetry_->setNodeId(toBase58(TokenType::NodePublic, nodeIdentity_->first));
+
     // Create the OTel MetricsRegistry for gap-fill metrics (counters,
     // histograms, observable gauges). It must exist before startTelemetry(),
     // which starts the metrics half of the pipeline.
@@ -1676,7 +1681,13 @@ ApplicationImp::startTelemetry() const
         if (instanceId.empty() && nodeIdentity_)
             instanceId = toBase58(TokenType::NodePublic, nodeIdentity_->first);
 
-        metricsRegistry_->start(endpoint, instanceId);
+        // The node public key also goes on its own resource attribute,
+        // xrpl.node.id, which config cannot override.
+        std::string nodeId;
+        if (nodeIdentity_)
+            nodeId = toBase58(TokenType::NodePublic, nodeIdentity_->first);
+
+        metricsRegistry_->start(endpoint, instanceId, nodeId);
     }
 }
 

src/xrpld/telemetry/MetricsRegistry.cpp

@@ -63,6 +63,7 @@
 #include <xrpl/server/LoadFeeTrack.h>
 #include <xrpl/server/NetworkOPs.h>
 #include <xrpl/telemetry/GetObjectMetricNames.h>
+#include <xrpl/telemetry/SpanNames.h>
 
 #include <opentelemetry/context/context.h>
 #include <opentelemetry/exporters/otlp/otlp_http_metric_exporter_factory.h>
@@ -240,14 +241,17 @@ MetricsRegistry::~MetricsRegistry()
 }
 
 void
-MetricsRegistry::start(std::string const& endpoint, std::string const& instanceId)
+MetricsRegistry::start(
+    std::string const& endpoint,
+    std::string const& instanceId,
+    std::string const& nodeId)
 {
 #ifdef XRPL_ENABLE_TELEMETRY
     if (!enabled_)
         return;
 
     JLOG(journal_.info()) << "MetricsRegistry: starting, endpoint=" << endpoint
-                          << ", instanceId=" << instanceId;
+                          << ", instanceId=" << instanceId << ", nodeId=" << nodeId;
 
     // Rule for anything added below: this phase may create only instruments
     // whose recording is PUSHED from app code -- counters and histograms. An
@@ -257,13 +261,14 @@ MetricsRegistry::start(std::string const& endpoint, std::string const& instanceI
     // belongs in startAsyncGauges(), not here. That includes observable
     // COUNTERS, not just gauges: jq_trans_overflow_total was created here and
     // its callback read getOverlay(), which asserts overlay_ is non-null.
-    initExporterAndProvider(endpoint, instanceId);
+    initExporterAndProvider(endpoint, instanceId, nodeId);
     initSyncInstruments();
 
     JLOG(journal_.info()) << "MetricsRegistry: provider and instruments ready";
 #else
     (void)endpoint;
     (void)instanceId;
+    (void)nodeId;
     (void)enabled_;
 #endif  // XRPL_ENABLE_TELEMETRY
 }
@@ -294,7 +299,10 @@ MetricsRegistry::startAsyncGauges()
 
 #ifdef XRPL_ENABLE_TELEMETRY
 void
-MetricsRegistry::initExporterAndProvider(std::string const& endpoint, std::string const& instanceId)
+MetricsRegistry::initExporterAndProvider(
+    std::string const& endpoint,
+    std::string const& instanceId,
+    std::string const& nodeId)
 {
     // Configure OTLP/HTTP metric exporter.
     otlp_http::OtlpHttpMetricExporterOptions exporterOpts;
@@ -318,6 +326,11 @@ MetricsRegistry::initExporterAndProvider(std::string const& endpoint, std::strin
     attrs[opentelemetry::semconv::service::kServiceName] = std::string("xrpld");
     if (!instanceId.empty())
         attrs[opentelemetry::semconv::service::kServiceInstanceId] = instanceId;
+    // xrpl.node.id: the same per-node key the trace resource carries, so
+    // metrics and traces resolve to one node. std::string for the same
+    // variant reason as service.name above.
+    if (!nodeId.empty())
+        attrs[std::string(attr::nodeId)] = nodeId;
     auto resourceAttrs = otel_resource::Resource::Create(attrs);
 
     // Build a view registry with explicit microsecond buckets for the

src/xrpld/telemetry/MetricsRegistry.h

@@ -285,9 +285,15 @@ public:
      * attribute. When non-empty, Prometheus metrics
      * carry a service_instance_id label for per-node
      * filtering.
+     * @param nodeId      Value for the xrpl.node.id resource attribute (the
+     * node's base58 public key). When non-empty, metrics
+     * carry the same per-node key that traces do.
      */
     void
-    start(std::string const& endpoint, std::string const& instanceId = {});
+    start(
+        std::string const& endpoint,
+        std::string const& instanceId = {},
+        std::string const& nodeId = {});
 
     /**
      * Register the pull-model observable instruments — the second startup
@@ -971,9 +977,13 @@ private:
      *
      * @param endpoint OTLP/HTTP metrics endpoint URL.
      * @param instanceId service.instance.id resource attribute (may be empty).
+     * @param nodeId xrpl.node.id resource attribute (may be empty).
      */
     void
-    initExporterAndProvider(std::string const& endpoint, std::string const& instanceId);
+    initExporterAndProvider(
+        std::string const& endpoint,
+        std::string const& instanceId,
+        std::string const& nodeId);
 
     /**
      * Create the synchronous instruments (RPC and job-queue counters and