diff --git a/docker/telemetry/otel-collector-config.grafanacloud.yaml b/docker/telemetry/otel-collector-config.grafanacloud.yaml index 2a717213b0..5fe6957c72 100644 --- a/docker/telemetry/otel-collector-config.grafanacloud.yaml +++ b/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 diff --git a/docker/telemetry/otel-collector-config.yaml b/docker/telemetry/otel-collector-config.yaml index b5cd059fe5..f595c43855 100644 --- a/docker/telemetry/otel-collector-config.yaml +++ b/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] diff --git a/docs/telemetry-runbook.md b/docs/telemetry-runbook.md index 484d439576..e8fc6c97a2 100644 --- a/docs/telemetry-runbook.md +++ b/docs/telemetry-runbook.md @@ -144,15 +144,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 diff --git a/include/xrpl/telemetry/SpanNames.h b/include/xrpl/telemetry/SpanNames.h index b848f96c03..d9f2fb0f0b 100644 --- a/include/xrpl/telemetry/SpanNames.h +++ b/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 — _ underscore form). * diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index b9ca1dca64..3f27767770 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/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. diff --git a/src/libxrpl/telemetry/Telemetry.cpp b/src/libxrpl/telemetry/Telemetry.cpp index 1d3eb13c7f..4a8210cbe1 100644 --- a/src/libxrpl/telemetry/Telemetry.cpp +++ b/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(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(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( diff --git a/src/tests/libxrpl/telemetry/NodeIdResource.cpp b/src/tests/libxrpl/telemetry/NodeIdResource.cpp new file mode 100644 index 0000000000..c0f4387b7e --- /dev/null +++ b/src/tests/libxrpl/telemetry/NodeIdResource.cpp @@ -0,0 +1,162 @@ +#include +#include +#include +#include + +#include + +#include +#include + +#ifdef XRPL_ENABLE_TELEMETRY +#include +#include +#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(it->second)); + EXPECT_FALSE(opentelemetry::nostd::holds_alternative(it->second)); + EXPECT_EQ(opentelemetry::nostd::get(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 diff --git a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp index bfa0b85149..65e2513348 100644 --- a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp +++ b/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()); diff --git a/src/xrpld/app/main/Application.cpp b/src/xrpld/app/main/Application.cpp index 736db61d6f..c06fb84c79 100644 --- a/src/xrpld/app/main/Application.cpp +++ b/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); } } diff --git a/src/xrpld/telemetry/MetricsRegistry.cpp b/src/xrpld/telemetry/MetricsRegistry.cpp index c07817e41e..69aa120705 100644 --- a/src/xrpld/telemetry/MetricsRegistry.cpp +++ b/src/xrpld/telemetry/MetricsRegistry.cpp @@ -63,6 +63,7 @@ #include #include #include +#include #include #include @@ -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 diff --git a/src/xrpld/telemetry/MetricsRegistry.h b/src/xrpld/telemetry/MetricsRegistry.h index 3662f1960b..7acc6f63ee 100644 --- a/src/xrpld/telemetry/MetricsRegistry.h +++ b/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