From 859aafa3d6fd6279455b5dfeb00a964d9ca88f83 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:39:58 +0100 Subject: [PATCH 01/10] fix(insight): hold collector hooks weakly, and cover the lifetime callHooks() copied the hook list into a vector of raw pointers, released mutex_, then dereferenced them. It has to release the lock: a handler may drop the last reference to a hook, and ~OTelHookImpl re-acquires mutex_, so invoking handlers under the non-recursive lock would deadlock. That left a window in which an entry could be freed before it was used. The window is not reachable today. Every hook belongs to a long-lived ApplicationImp member, and onCollectionStopping() runs before those members are destroyed, both from stop() and from the destructor body. That call disarms each gauge via RemoveCallback, which blocks until an in-flight callback finishes, because the SDK holds its registry mutex across the callback. Safety therefore rests on four separate facts, none of them enforced by a test, one of them internal to a vendored library. Store weak references instead, so the code is correct by construction: locking an entry keeps that hook alive for exactly its own handler call, and a hook destroyed since the snapshot locks to null and is skipped. Registration moves from the OTelHookImpl constructor to makeHook(), because no weak_ptr to the object exists until the owning shared_ptr does, and the destructor now prunes by expiry rather than by address. Add three GTests over the real collector. A test-local MetricReader drives one synchronous collection pass, since the SDK ships only a threaded periodic reader. They assert a live hook runs, a destroyed hook is skipped, and destroying one hook leaves its siblings registered -- the last pairing both directions so neither can pass vacuously. Not yet run: verifying them needs a telemetry-enabled build of xrpl_tests. Compile, clang-tidy and the pre-commit gates are clean. --- src/libxrpl/beast/insight/OTelCollector.cpp | 63 ++++-- .../beast/insight/OTelCollectorHooks.cpp | 208 ++++++++++++++++++ 2 files changed, 253 insertions(+), 18 deletions(-) create mode 100644 src/tests/libxrpl/beast/insight/OTelCollectorHooks.cpp diff --git a/src/libxrpl/beast/insight/OTelCollector.cpp b/src/libxrpl/beast/insight/OTelCollector.cpp index b957bd5f79..aba5e59835 100644 --- a/src/libxrpl/beast/insight/OTelCollector.cpp +++ b/src/libxrpl/beast/insight/OTelCollector.cpp @@ -511,17 +511,25 @@ public: /** * @brief Register a hook for periodic invocation. - * @param hook Pointer to the hook to register. + * + * Takes the owning shared_ptr so the list can store a weak reference. + * Called from makeHook() rather than the hook's constructor, because a + * weak_ptr cannot be formed until the shared_ptr owns the object. + * + * @param hook Owning pointer to the hook to register. */ void - addHook(OTelHookImpl* hook); + addHook(std::shared_ptr const& hook); /** - * @brief Unregister a hook. - * @param hook Pointer to the hook to unregister. + * @brief Drop entries for hooks that have been destroyed. + * + * Called from ~OTelHookImpl. The dying hook's weak_ptr has already + * expired by then, so the entry is identified by expiry rather than by + * address. */ void - removeHook(OTelHookImpl* hook); + removeExpiredHooks(); /** * @brief Invoke all registered hooks. @@ -597,8 +605,16 @@ private: /** * Registered hooks called during observable callbacks. + * + * Weak, not owning, and not raw. callHooks() must invoke handlers with + * mutex_ released, because a handler may drop the last reference to a + * hook and ~OTelHookImpl re-acquires mutex_. A raw pointer copied out of + * this list could therefore be dangling by the time it is dereferenced. + * Locking a weak_ptr instead keeps the hook alive for exactly the + * duration of its own handler call, and an already-destroyed hook is + * skipped rather than followed. */ - std::vector hooks_; + std::vector> hooks_; /** * Registered gauges read during observable callbacks. @@ -634,12 +650,14 @@ private: OTelHookImpl::OTelHookImpl(HandlerType handler, std::shared_ptr impl) : impl_(std::move(impl)), handler_(std::move(handler)) { - impl_->addHook(this); + // Registration happens in OTelCollectorImp::makeHook(), not here: the + // list holds weak references, and no weak_ptr to this object exists + // until the owning shared_ptr does. } OTelHookImpl::~OTelHookImpl() { - impl_->removeHook(this); + impl_->removeExpiredHooks(); } void @@ -849,7 +867,9 @@ OTelCollectorImp::~OTelCollectorImp() Hook OTelCollectorImp::makeHook(HookImpl::HandlerType const& handler) { - return Hook(std::make_shared(handler, shared_from_this())); + auto hook = std::make_shared(handler, shared_from_this()); + addHook(hook); + return Hook(hook); } Counter @@ -883,17 +903,17 @@ OTelCollectorImp::makeMeter(std::string const& name) } void -OTelCollectorImp::addHook(OTelHookImpl* hook) +OTelCollectorImp::addHook(std::shared_ptr const& hook) { std::scoped_lock const lock(mutex_); - hooks_.push_back(hook); + hooks_.emplace_back(hook); } void -OTelCollectorImp::removeHook(OTelHookImpl* hook) +OTelCollectorImp::removeExpiredHooks() { std::scoped_lock const lock(mutex_); - std::erase(hooks_, hook); + std::erase_if(hooks_, [](std::weak_ptr const& hook) { return hook.expired(); }); } void @@ -910,15 +930,22 @@ OTelCollectorImp::callHooks() // Copy the hook list under the lock, then invoke handlers outside it. // A handler may drop the last reference to an OTelHookImpl, whose - // destructor calls removeHook() and re-acquires mutex_; invoking - // handlers while holding the (non-recursive) lock would deadlock. - std::vector hooks; + // destructor re-acquires mutex_; invoking handlers while holding the + // (non-recursive) lock would deadlock. + std::vector> hooks; { std::scoped_lock const lock(mutex_); hooks = hooks_; } - for (auto* hook : hooks) - hook->callHandler(); + + // Locking each entry keeps that hook alive across its own handler call, + // so releasing mutex_ above cannot leave a dangling reference. A hook + // destroyed since the snapshot was taken locks to null and is skipped. + for (auto const& weakHook : hooks) + { + if (auto const hook = weakHook.lock()) + hook->callHandler(); + } } void diff --git a/src/tests/libxrpl/beast/insight/OTelCollectorHooks.cpp b/src/tests/libxrpl/beast/insight/OTelCollectorHooks.cpp new file mode 100644 index 0000000000..18f35e2f8a --- /dev/null +++ b/src/tests/libxrpl/beast/insight/OTelCollectorHooks.cpp @@ -0,0 +1,208 @@ +#ifdef XRPL_ENABLE_TELEMETRY + +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +namespace beast::insight { + +namespace metrics_api = opentelemetry::metrics; +namespace metrics_sdk = opentelemetry::sdk::metrics; + +/** + * A MetricReader that collects only when the test asks it to. + * + * The SDK ships only PeriodicExportingMetricReader, whose background thread + * would make these tests depend on timing. MetricReader::Collect() is public + * and synchronous, so a minimal subclass lets a test drive one collection pass + * on the calling thread. That pass is what invokes an observable gauge's + * callback, which is the only path that reaches the collector's hooks. + * + * @code + * auto reader = std::make_shared(); + * provider->AddMetricReader(reader); + * reader->collectOnce(); // runs every registered observable callback + * @endcode + */ +class ManualMetricReader : public metrics_sdk::MetricReader +{ +public: + /** + * @brief Run exactly one collection pass, discarding the metric data. + * + * The tests assert on hook side effects, not on exported points, so the + * callback returns true without inspecting what it was handed. + */ + void + collectOnce() + { + Collect([](metrics_sdk::ResourceMetrics&) { return true; }); + } + + metrics_sdk::AggregationTemporality + GetAggregationTemporality(metrics_sdk::InstrumentType) const noexcept override + { + return metrics_sdk::AggregationTemporality::kCumulative; + } + + bool + OnForceFlush(std::chrono::microseconds) noexcept override + { + return true; + } + + bool + OnShutDown(std::chrono::microseconds) noexcept override + { + return true; + } +}; + +/** + * Installs a real SDK MeterProvider so observable gauges actually fire. + * + * OTelCollector takes its Meter from the global provider. Under the default + * noop provider an observable gauge's callback is never invoked, so a hook + * test would pass whatever the collector did. The fixture swaps in an SDK + * provider with a ManualMetricReader and restores the previous global provider + * afterwards, so it leaks no state into other telemetry tests in this binary. + */ +class OTelCollectorHooks : public ::testing::Test +{ +protected: + void + SetUp() override + { + previous_ = metrics_api::Provider::GetMeterProvider(); + reader_ = std::make_shared(); + auto provider = metrics_sdk::MeterProviderFactory::Create(); + provider->AddMetricReader(reader_); + provider_ = std::shared_ptr(std::move(provider)); + metrics_api::Provider::SetMeterProvider( + opentelemetry::nostd::shared_ptr(provider_)); + } + + void + TearDown() override + { + metrics_api::Provider::SetMeterProvider(previous_); + provider_.reset(); + reader_.reset(); + } + + /** + * @brief Build a collector, plus the armed gauge that drives its hooks. + * + * A collection pass only reaches the hooks through an observable gauge's + * callback, and a gauge is armed by onCollectionReady(), so every test + * needs both. The gauge is returned because dropping it would unregister + * the callback. + * + * Each test builds its own collector: the hook debounce is keyed to the + * time of the last invocation, which starts unset, so the first collection + * on a fresh collector always runs the hooks. + */ + static std::pair + makeArmedCollector() + { + auto collector = OTelCollector::New( + "http://127.0.0.1:4318/v1/metrics", + "", + "test-instance", + "xrpld", + "test", + Journal(Journal::getNullSink())); + auto gauge = collector->makeGauge("hook_test_gauge"); + collector->onCollectionReady(); + return {std::move(collector), std::move(gauge)}; + } + + opentelemetry::nostd::shared_ptr previous_; + std::shared_ptr reader_; + std::shared_ptr provider_; +}; + +// --------------------------------------------------------------------------- +// 1. A hook that is still alive runs on a collection pass. +// This is the registration path: makeHook() puts the hook on the +// collector's list, and an observable gauge callback invokes it. Without +// this, a hook that is never registered is indistinguishable from one that +// is registered and skipped. +// --------------------------------------------------------------------------- +TEST_F(OTelCollectorHooks, live_hook_runs_once_per_collection) +{ + auto [collector, gauge] = makeArmedCollector(); + + std::size_t calls = 0; + auto const hook = collector->makeHook([&calls] { ++calls; }); + + reader_->collectOnce(); + + EXPECT_EQ(calls, 1u); +} + +// --------------------------------------------------------------------------- +// 2. A hook destroyed before the collection pass is skipped, not called. +// The collector holds weak references, so the destroyed hook's entry locks +// to null. Asserting zero (not "did not crash") is what makes this a real +// check: a stale entry that was still followed would run the handler and +// increment the counter through freed memory. +// --------------------------------------------------------------------------- +TEST_F(OTelCollectorHooks, destroyed_hook_is_skipped) +{ + auto [collector, gauge] = makeArmedCollector(); + + std::size_t calls = 0; + { + auto const hook = collector->makeHook([&calls] { ++calls; }); + } + + reader_->collectOnce(); + + EXPECT_EQ(calls, 0u); +} + +// --------------------------------------------------------------------------- +// 3. Destroying one hook leaves its siblings registered. +// Guards the pruning step: removeExpiredHooks() erases by expiry rather +// than by address, so an over-broad predicate would drop live hooks too and +// silently stop their metrics updating. +// --------------------------------------------------------------------------- +TEST_F(OTelCollectorHooks, destroying_one_hook_keeps_the_others) +{ + auto [collector, gauge] = makeArmedCollector(); + + std::size_t kept = 0; + std::size_t dropped = 0; + auto const keptHook = collector->makeHook([&kept] { ++kept; }); + { + auto const droppedHook = collector->makeHook([&dropped] { ++dropped; }); + } + + reader_->collectOnce(); + + EXPECT_EQ(kept, 1u); + EXPECT_EQ(dropped, 0u); +} + +} // namespace beast::insight + +#endif // XRPL_ENABLE_TELEMETRY From 5f68b22cec50d15f8e444df5b6030851a6dd5aa9 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:42:02 +0100 Subject: [PATCH 02/10] fix(telemetry): stop publishing the Grafana renderer on the host Grafana reaches the image renderer over the compose network at http://renderer:8081, so the host publish gave nothing the stack needs. AUTH_TOKEN is the only guard on the endpoint and its default is a fixed string in this file. Update the service table in the configuration reference to match. --- OpenTelemetryPlan/05-configuration-reference.md | 2 +- docker/telemetry/docker-compose.yml | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/OpenTelemetryPlan/05-configuration-reference.md b/OpenTelemetryPlan/05-configuration-reference.md index 41796b3590..baceb4680f 100644 --- a/OpenTelemetryPlan/05-configuration-reference.md +++ b/OpenTelemetryPlan/05-configuration-reference.md @@ -353,7 +353,7 @@ The authoritative development stack lives in the repo at `docker/telemetry/docke | `loki` | `grafana/loki:3.7.6` | `3100` | Log storage for log↔trace correlation | | `prometheus` | `prom/prometheus:v3.13.2` | `9090` | Scrapes the collector's `:8889` | | `grafana` | `grafana/grafana:13.1.2` | `3000` | Dashboards + provisioned datasources/alerts, anonymous admin | -| `renderer` | `grafana/grafana-image-renderer:v5.12.0` | `8081` | Panel→PNG rendering for image export and alert screenshots | +| `renderer` | `grafana/grafana-image-renderer:v5.12.0` | none | Panel→PNG rendering for image export and alert screenshots | Two corrections to earlier drafts: diff --git a/docker/telemetry/docker-compose.yml b/docker/telemetry/docker-compose.yml index 1cd02c9647..1cb821734d 100644 --- a/docker/telemetry/docker-compose.yml +++ b/docker/telemetry/docker-compose.yml @@ -214,8 +214,10 @@ services: # Shared secret for the JWT-authenticated render requests Grafana 13 # sends. Must match GF_RENDERING_RENDERER_TOKEN on the grafana service. - AUTH_TOKEN=${GF_RENDERING_RENDERER_TOKEN:-xrpld-local-render} - ports: - - "8081:8081" # Renderer HTTP endpoint (called by grafana) + # No `ports:` on purpose. Grafana reaches this over the compose network at + # http://renderer:8081, so publishing 8081 on the host adds nothing the + # stack needs. AUTH_TOKEN above is the only guard on the endpoint, and its + # default is a fixed string in this file, so keep the service off the host. networks: - xrpld-telemetry # Named volume for Tempo trace storage (WAL and compacted blocks). From ddff6019f22b1a25a6e9236c210c024fa3c46a7a Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:42:16 +0100 Subject: [PATCH 03/10] docs(telemetry): give the Phase 11 validator board its own dashboard uid Grafana keys a dashboard by uid, so the Phase 9 and Phase 11 rows both claiming `validator-health` meant one would silently overwrite the other. Phase11_taskList.md already requires `validator-health-external`; the reference table now agrees with it, and says why. --- OpenTelemetryPlan/09-data-collection-reference.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/OpenTelemetryPlan/09-data-collection-reference.md b/OpenTelemetryPlan/09-data-collection-reference.md index 986e7c2d5d..ed69c1d5ad 100644 --- a/OpenTelemetryPlan/09-data-collection-reference.md +++ b/OpenTelemetryPlan/09-data-collection-reference.md @@ -2009,11 +2009,13 @@ query, an alert — matches nothing and should be pointed at the live keys above | Dashboard | UID | Data Source | Key Panels | | ------------------ | --------------------------- | ----------- | ---------------------------------------------------------------------- | -| Validator Health | `validator-health` | Prometheus | Server state timeline, proposer count, converge time, amendment voting | +| Validator Health | `validator-health-external` | Prometheus | Server state timeline, proposer count, converge time, amendment voting | | Network Topology | `xrpld-network-topology` | Prometheus | Peer count, version distribution, latency distribution, diverged peers | | Fee Market (Ext) | `xrpld-fee-market-external` | Prometheus | Fee levels, queue depth, load factor breakdown, escalation timeline | | DEX & AMM Overview | `xrpld-dex-amm` | Prometheus | AMM TVL, order book depth, spread trends, trading fee revenue | +Grafana keys a dashboard by its UID, so two dashboards sharing one UID overwrite each other — whichever the provisioner loads last wins, and it does so silently. Phase 9 already ships `validator-health` (the row above), so the Phase 11 dashboard uses `validator-health-external`, the same way Fee Market is disambiguated as `xrpld-fee-market-external`. `OpenTelemetryPlan/Phase11_taskList.md` § Task 11.9 carries the same rule and the filename that goes with it. + ### Prometheus Alerting Rules (Phase 11) | Alert Name | Severity | Condition | For | From eb76645f69edf3d78907a6ca3fcbd6d8e137686a Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 10 Sep 2026 15:43:00 +0100 Subject: [PATCH 04/10] docs(telemetry): name the collector stanzas instead of citing line numbers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `service_name`, not `job` note pointed at three file:line locations. All three had drifted, because the cited files move on every merge forward and nothing checks the references. Name the `resource/logs` processor and the `loki` service instead — those survive line drift and a rename breaks a grep loudly. --- docker/telemetry/TESTING.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docker/telemetry/TESTING.md b/docker/telemetry/TESTING.md index 738fe71fac..e18b48551d 100644 --- a/docker/telemetry/TESTING.md +++ b/docker/telemetry/TESTING.md @@ -662,17 +662,17 @@ Timestamps are unix nanoseconds, matching `workload/validate_telemetry.py`. Counting `.data.result | length` would count streams, not log lines. > **Use `service_name`, not `job`.** The local stack's `resource/logs` processor -> sets one key, `service.name=xrpld` (`otel-collector-config.yaml:84-86`); its +> sets one key, `service.name=xrpld`, in `otel-collector-config.yaml`; its > comment there explains that a custom `job` attribute is not promoted to a > stream label and tells you to select on `service_name`. Only the Grafana Cloud -> variant also sets `job=xrpld` (`otel-collector-config.grafanacloud.yaml:73-75`). +> variant also sets `job=xrpld`, in `otel-collector-config.grafanacloud.yaml`. > Either way `{job="xrpld"}` does not work as a selector: on OTLP ingest Loki > promotes only an allow-listed set of resource attributes to indexed stream > labels (`service.name` → `service_name`, plus `service.namespace`, > `service.instance.id`, `deployment.environment`, `k8s.*`, `cloud.*`), and `job` > is not on the list. This repo mounts no Loki config override — the `loki` > service runs the image's built-in `/etc/loki/local-config.yaml` -> (`docker-compose.yml:116`) — so `job` lands in **structured metadata**, which +> named in `docker-compose.yml` — so `job` lands in **structured metadata**, which > cannot be a stream selector. `{job="xrpld"}` therefore returns **zero results > with no error**, which reads exactly like "logs are not being ingested". If > this query is empty, check `{service_name="xrpld"}` before debugging the From e1ef6ba18372230d00ccd9ce68c5814ef3621cd7 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:30:17 +0100 Subject: [PATCH 05/10] docs(telemetry): drop the inert insight endpoint from the test config template On the OTel path only [insight] server is load-bearing. CollectorManager reads endpoint and hands it to OTelCollector, which logs it at startup and routes nothing with it; the real export endpoint is [telemetry] metrics_endpoint, which the template already sets. service_instance_id and service_name in that section are read and discarded. Leaving the line invited an operator to reconcile a mismatch that has no effect. integration-test.sh already emits only server=otel with the same explanation, so the two now agree. --- docker/telemetry/TESTING.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docker/telemetry/TESTING.md b/docker/telemetry/TESTING.md index a18baaadc7..d18114d80c 100644 --- a/docker/telemetry/TESTING.md +++ b/docker/telemetry/TESTING.md @@ -266,8 +266,10 @@ trace_peer=1 trace_ledger=1 [insight] +# server=otel is the only load-bearing key here -- it selects OTelCollector. +# The export endpoint comes from [telemetry] metrics_endpoint, and [insight]'s +# own service_instance_id/service_name keys are ignored. server=otel -endpoint=http://localhost:4318/v1/metrics [rpc_startup] { "command": "log_level", "severity": "warning" } From 975238d7d4ccb65e1ab00213d8a1eba4ba248139 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:30:39 +0100 Subject: [PATCH 06/10] docs(telemetry): fix the node log path, log level and Grafana span link MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The config template wrote each node's log to a lowercase node{N} directory while setting service_instance_id=Node-{N}. The collector takes the node name from the log file's parent directory and stamps it as the Loki service_instance_id label, so the logs carried a name no trace or metric shared and nothing joined. Use Node-{N} and state the rule. The template also set log_level to warning. Nothing in the pipeline filters on severity; the constraint is that a log line carries trace context only when it is emitted inside an active span. At warning the only such statements in the consensus accept span are a catch path a healthy round never takes and a periodic censorship warning. At info the CNF Val / CNF buildLCL pair writes one line per accepted ledger, which is what makes this test's Step 1 findable. Grafana 13 offers the link per span, labelled "Logs for this span", in the span's Links row — not per trace. Fix the step and the expected-results row. The example log line quoted a message that does not exist. The real in-span RPC statement logs at debug, so the severity code is DBG; say which line to look for under each test, since Test 2 now logs at info. Drop the reference to workload/validate_telemetry.py: that file is not part of this branch, and its instant-endpoint call uses seconds, so the nanoseconds claim applied only to query_range. --- docker/telemetry/TESTING.md | 52 ++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/docker/telemetry/TESTING.md b/docker/telemetry/TESTING.md index 456a091cd9..9705934aaa 100644 --- a/docker/telemetry/TESTING.md +++ b/docker/telemetry/TESTING.md @@ -237,14 +237,14 @@ protocol = peer [node_db] type=NuDB -path=/tmp/xrpld-integration/node{N}/nudb +path=/tmp/xrpld-integration/Node-{N}/nudb online_delete=256 [database_path] -/tmp/xrpld-integration/node{N}/db +/tmp/xrpld-integration/Node-{N}/db [debug_logfile] -/tmp/xrpld-integration/node{N}/debug.log +/tmp/xrpld-integration/Node-{N}/debug.log [validation_seed] {seed from step 2} @@ -279,12 +279,22 @@ server=otel endpoint=http://localhost:4318/v1/metrics [rpc_startup] -{ "command": "log_level", "severity": "warning" } +{ "command": "log_level", "severity": "info" } [ssl_verify] 0 ``` +The per-node directory name must equal `[telemetry] service_instance_id`: the +collector reads the node name off the log file's path and stamps it as the Loki +label `service_instance_id`, so a mismatch leaves the logs labelled with a node +name that no trace or metric shares. + +`log_level` is `info`, not `warning`. A log line carries trace context only when +it is emitted inside an active span, and the pair that reliably carries it — the +`CNF Val` / `CNF buildLCL` branches inside the consensus accept span, one of +which fires for every accepted ledger — logs at `info`. + #### Step 4: Create validators.txt ```ini @@ -481,9 +491,15 @@ Expected: log lines with `trace_id=<32hex> span_id=<16hex>` between the severity code and the message. Example: ``` -2024-Jan-15 10:30:45.123456789 UTC RPCHandler:NFO trace_id=abc123def456789012345678abcdef01 span_id=0123456789abcdef Calling server_info +2024-Jan-15 10:30:45.123456789 UTC RPCHandler:DBG trace_id=abc123def456789012345678abcdef01 span_id=0123456789abcdef RPC call server_info completed in 0.000123seconds ``` +That example is a Test 1 line. `xrpld-telemetry.cfg` logs at `debug`, so the +in-span RPC statement above appears. Test 2's nodes log at `info`, which +suppresses it — there, look for the `CNF Val` / `CNF buildLCL` lines from the +consensus accept span instead. Either carries trace context; only the message +differs. + Lines emitted outside of an active span (background tasks, startup) will NOT have trace context — this is expected. @@ -524,9 +540,9 @@ Use `query_range`, not `query`. Loki rejects a bare log selector on the instant `/query` endpoint with HTTP 400 and a `text/plain` body ("log queries are not supported as an instant query type"), so `jq` fails to parse it and the step never prints a number — even when ingestion is working. -Only metric queries such as `sum(count_over_time(...))` are allowed there, -which is why the validation scripts can use the instant endpoint. -Timestamps are unix nanoseconds, matching `workload/validate_telemetry.py`. +Only metric queries such as `sum(count_over_time(...))` are allowed there, so a +check that needs a count rather than the lines themselves can use the instant +endpoint. `query_range` timestamps are unix nanoseconds. Counting `.data.result | length` would count streams, not log lines. ### Step 4: Verify Grafana Tempo-to-Loki correlation @@ -534,7 +550,7 @@ Counting `.data.result | length` would count streams, not log lines. 1. Open Grafana at http://localhost:3000 2. Navigate to **Explore** -> select **Tempo** datasource 3. Search for a trace (e.g., operation `rpc.command.server_info`) -4. Click **"Logs for this trace"** in the trace detail view +4. Expand a span and click **"Logs for this span"** in its **Links** row 5. Verify that Loki log lines appear, filtered by the trace's `trace_id` ### Step 5: Verify Grafana Loki-to-Tempo correlation @@ -546,15 +562,15 @@ Counting `.data.result | length` would count streams, not log lines. ### Expected results -| Check | Expected | -| ------------------------------ | ---------------------------------------- | -| `trace_id=` in debug.log | Present in log lines within active spans | -| `span_id=` in debug.log | Present alongside trace_id | -| Logs without active span | No trace_id/span_id fields | -| trace_id in Tempo | Matches a valid trace | -| Loki log ingestion | Logs visible via LogQL | -| Tempo -> Loki "Logs for trace" | Shows correlated log lines | -| Loki -> Tempo TraceID link | Navigates to correct trace | +| Check | Expected | +| --------------------------- | ---------------------------------------- | +| `trace_id=` in debug.log | Present in log lines within active spans | +| `span_id=` in debug.log | Present alongside trace_id | +| Logs without active span | No trace_id/span_id fields | +| trace_id in Tempo | Matches a valid trace | +| Loki log ingestion | Logs visible via LogQL | +| Tempo -> Loki span log link | Shows correlated log lines | +| Loki -> Tempo TraceID link | Navigates to correct trace | --- From 178fc58c3441e71c17e9a08b93f0b627be35310f Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:31:58 +0100 Subject: [PATCH 07/10] docs(telemetry): correct the readiness check, build steps and span triggers The collector readiness note claimed docker-compose.yml publishes only 4317, 4318 and 8889 and that 13133 comes from a workload stack. It publishes 13133, and that stack is not part of this branch. Probe health_check on 13133 and drop the note; the troubleshooting entry now points at the same check instead of carrying a second, weaker copy. Stop restating BUILD.md. The hardcoded conan and cmake lines had drifted from it, -Dtelemetry=ON is redundant because the Conan toolchain carries it, and the conan-release preset resolves only from the repo root, builds into .build/build/Release rather than .build, and sets no -Dxrpld=ON. Defer to BUILD.md and docs/build/telemetry.md. Test 2's keygen step reused the Devnet config with -a --start, which wrote a genesis chain into the Devnet store, took RPC port 5005 from node 1, and was followed by an rm -rf that also destroyed the mainnet node's store and every log. Give it its own config under the test's temp root, as the script does. The manual path also needs XRPLD_LOG_DIR, or the collector tails the wrong root and Test 3 finds nothing without erroring. Neither the template nor the script set [network_id], so a local cluster stamped xrpl.network.type=mainnet and shared dashboard series with real mainnet data. Set a private id in both, and say which label it produces. Also drop a duplicate metrics_endpoint from the generated config. Split the consensus trigger row: six families fire on a standalone ledger_accept, and the remaining seven need the establish phase, a validator key, or a peer. ledger.validate needs peers too, because checkAccept is unreachable in standalone. Correct the trace-id note to 16 bytes, and name the strategy it depends on. The pathfinding bullet said raw account values reach Grafana Cloud. Both accounts are already tokens when they leave the node; what differs is that the base config hashes them a second time, so one account carries two tokens across configs and traces must not be joined across them. Also: the Loki allow-list is a fixed 18 keys on the pinned image with k8s and cloud enumerated rather than wildcarded, the runbook documents 9 of 15 dashboards, and the spanmetrics block now uses one spelling with a note that the cloud config uses the other. --- docker/telemetry/TESTING.md | 218 +++++++++++++------- docker/telemetry/integration-test.sh | 7 +- docker/telemetry/otel-collector-config.yaml | 11 +- 3 files changed, 156 insertions(+), 80 deletions(-) diff --git a/docker/telemetry/TESTING.md b/docker/telemetry/TESTING.md index e18b48551d..8c7ae4c5d6 100644 --- a/docker/telemetry/TESTING.md +++ b/docker/telemetry/TESTING.md @@ -10,17 +10,14 @@ pipeline end-to-end, from span generation through the observability stack ### Build xrpld with telemetry -Follow [BUILD.md](../../BUILD.md) with `-o telemetry=True` added. From a build directory (`.build/`): +Build as [BUILD.md](../../BUILD.md) **§ Steps** describes, adding +`-o telemetry=True` to the `conan install` line. That is the only change: +Conan carries `telemetry=ON` into the generated CMake toolchain, so no extra +CMake flag is needed. For the full telemetry build, including how to turn it +off, see [`docs/build/telemetry.md`](../../docs/build/telemetry.md). -```bash -conan install .. --output-folder . --build missing -o telemetry=True --settings build_type=Release -cmake -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake -DCMAKE_BUILD_TYPE=Release -Dxrpld=ON -Dtelemetry=ON .. -cmake --build . --target xrpld -``` - -Conan also writes a `conan-release` preset, so `cmake --preset conan-release -Dtelemetry=ON` works too. There is no preset named `default`. - -The binary is at `.build/xrpld`. +This document assumes the `.build/` layout, so the binary is at `.build/xrpld` +and every command below runs from the repo root. ### Required tools @@ -61,21 +58,14 @@ XRPLD_UID=$(id -u) XRPLD_GID=$(id -g) \ Wait for services to be ready: ```bash -# otel-collector readiness: any HTTP response on the OTLP/HTTP port means the -# receiver is listening. Do NOT use `curl -sf` here — a GET of / returns 404, -# which -f treats as failure even when the collector is healthy. -[ "$(curl -so /dev/null -w '%{http_code}' http://localhost:4318/)" != "000" ] && - echo "collector ready" +# otel-collector readiness: the health_check extension answers on 13133, which +# docker-compose.yml publishes. +curl -sf http://localhost:13133/ >/dev/null && echo "collector ready" # Tempo readiness curl -sf http://localhost:3200/ready >/dev/null && echo "tempo ready" ``` -> The collector's `health_check` extension listens on **13133**, but -> `docker-compose.yml` publishes only 4317, 4318 and 8889 — so 13133 is not -> reachable from the host with the base stack. It is published only by the -> workload validation stack (`docker-compose.workload.yaml`). - ### Step 2: Start xrpld in standalone mode ```bash @@ -197,34 +187,82 @@ If you prefer to run the steps manually: #### Step 1: Start observability stack ```bash -docker compose -f docker/telemetry/docker-compose.yml up -d +XRPLD_LOG_DIR=/tmp/xrpld-integration \ + docker compose -f docker/telemetry/docker-compose.yml up -d ``` +The override is required here. The collector's log mount defaults to the +repo-relative `docker/telemetry/data/logs`, but this test writes its logs under +`/tmp/xrpld-integration`, so without it the `file_log` receiver tails the wrong +root, no log line reaches Loki, and Test 3 Step 3 finds nothing with no error. + #### Step 2: Generate validator keys -Start a temporary standalone xrpld: +Give the throwaway node a config of its own, under the same temp root the rest +of this test uses: ```bash -.build/xrpld --conf docker/telemetry/xrpld-telemetry.cfg -a --start & +mkdir -p /tmp/xrpld-integration/temp-keygen +cat >/tmp/xrpld-integration/temp-keygen/xrpld.cfg <<'EOCFG' +[server] +port_rpc_temp + +[port_rpc_temp] +port = 5099 +ip = 127.0.0.1 +admin = 127.0.0.1 +protocol = http + +[node_db] +type=NuDB +path=/tmp/xrpld-integration/temp-keygen/nudb +online_delete=256 + +[database_path] +/tmp/xrpld-integration/temp-keygen/db + +[debug_logfile] +/tmp/xrpld-integration/temp-keygen/debug.log + +[ssl_verify] +0 +EOCFG +``` + +Do not point this node at `docker/telemetry/xrpld-telemetry.cfg`. That is a +Devnet config whose `[node_db]`, `[database_path]` and `[debug_logfile]` all +resolve under `docker/telemetry/data`, so `--start` (a fresh-genesis start) +would write a genesis chain into the Devnet store, and deleting that directory +afterwards would also destroy the sibling mainnet node's store and every log +under `data/logs/`. Its RPC port is 5005, which is node 1's port later in this +test. + +Start it and wait for RPC before asking for keys: + +```bash +.build/xrpld --conf /tmp/xrpld-integration/temp-keygen/xrpld.cfg -a --start & TEMP_PID=$! -sleep 5 +until curl -sf http://localhost:5099 -d '{"method":"server_info"}' >/dev/null; do + sleep 1 +done ``` Generate 6 key pairs: ```bash for i in $(seq 1 6); do - curl -s http://localhost:5005 \ + curl -s http://localhost:5099 \ -d '{"method":"validation_create"}' | jq '.result' done ``` Record the `validation_seed` and `validation_public_key` for each. -Kill the temporary node: +Stop the temporary node and remove only its own directory: ```bash kill $TEMP_PID -rm -rf docker/telemetry/data/ +wait $TEMP_PID 2>/dev/null +rm -rf /tmp/xrpld-integration/temp-keygen ``` #### Step 3: Create node configs @@ -247,6 +285,9 @@ port = {51234 + node_number} ip = 0.0.0.0 protocol = peer +[network_id] +1025 + [node_db] type=NuDB path=/tmp/xrpld-integration/node{N}/nudb @@ -297,6 +338,14 @@ endpoint=http://localhost:4318/v1/metrics 0 ``` +`[network_id]` has to be a private id (anything other than 0, 1 or 2), because +the config default is id 0 and the telemetry resource maps that to `mainnet` — +without the stanza every span and metric this local cluster emits is stamped +`xrpl.network.type=mainnet` and lands on the same dashboard series as real +mainnet data. Only 0, 1 and 2 have names, so a private id is stamped +`xrpl.network.type=unknown`. That is the value to select in the dashboards' +Network Type filter when looking at this cluster. + #### Step 4: Create validators.txt ```ini @@ -387,45 +436,51 @@ One hole worth knowing: the runbook's Span Reference tables have no row for `method`, `grpc_role` and `grpc_status`, emitted from `GRPCServer.cpp` with the key constants in `src/xrpld/app/main/GrpcSpanNames.h`. -If you find an older inline span inventory in this file or elsewhere, do not -trust it — the copy that used to live here had drifted badly (18 rows under a -"16 spans" heading, whole families missing, and pre-rename dotted `xrpl.*` -attribute keys the code no longer emits). The code and the runbook are the source -of truth. - ### Span → How to Trigger "Test" is the section of this file that exercises the family. `T1` = Test 1 (standalone), `T2` = Test 2 (6-node network). -| Span family (count) | Config toggle | How to trigger | Test | -| ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------- | -| **RPC** (5 total, 3 here): `rpc.http_request`, `rpc.process`, `rpc.command.` | `trace_rpc=1` | Any HTTP JSON-RPC call: `curl -s http://localhost:5005 -d '{"method":"server_info"}'`. `rpc.command.` is one family — the command name is part of the span name. | T1 | -| **RPC** (cont.): `rpc.ws_message`, `rpc.ws_upgrade` | `trace_rpc=1` | Needs a WebSocket client against `[port_ws_public]` (**6005**) or `[port_ws_admin_local]` (6006). `rpc.ws_upgrade` covers the handshake — force a failure to see its error path. `curl` alone will not do it. | — | -| **gRPC** (1): `grpc.` | `trace_rpc=1` | Call a gRPC method (`GetLedger`, `GetLedgerData`, …). **Requires a `[port_grpc]` stanza — the shipped `xrpld-telemetry*.cfg` files define none**, so add one first. | — | -| **Transaction** (6 total, 4 here): `tx.process`, `tx.preflight`, `tx.preclaim`, `tx.transactor` | `trace_transactions` | Submit any transaction (T1 Step 4). The three apply-stage spans share the tx's deterministic trace id; the `stage` attribute says where a failing tx stopped. | T1 | -| **Transaction** (cont.): `tx.receive` | `trace_transactions` | A **peer** relays a transaction. Never appears in standalone — submit on one node of the cluster and look on another. | T2 | -| **Transaction** (cont.): `tx.apply` | `trace_transactions` | Ledger close with a non-empty transaction set: submit, then `ledger_accept` (T1) or wait for consensus (T2). | T1 / T2 | -| **TxQ** (6): `txq.enqueue`, `txq.apply_direct`, `txq.batch_clear`, `txq.accept`, `txq.accept_tx`, `txq.cleanup` | `trace_transactions` | `txq.enqueue`/`apply_direct` on every submission; `txq.accept`/`accept_tx`/`cleanup` on every ledger close. To force real queueing, submit faster than ledgers close or with a fee below the required fee level. | T1 | -| **Consensus** (13): `consensus.round`, `.phase.open`, `.establish`, `.update_positions`, `.check`, `.proposal.send`, `.ledger_close`, `.accept`, `.accept.apply`, `.validation.send`, `.mode_change`, `.proposal.receive`, `.validation.receive` | `trace_consensus=1` | Requires real consensus — **standalone emits none of these**. Bring up T2 and wait for nodes to reach `proposing`; one `consensus.round` per close. `.mode_change` needs an actual mode transition (stop/start a node). | T2 | -| **Ledger** (4 total, 3 here): `ledger.build`, `ledger.validate`, `ledger.store` | `trace_ledger=1` | Any ledger close: `ledger_accept` in standalone, or consensus in T2. | T1 / T2 | -| **Ledger** (cont.): `ledger.acquire` | `trace_ledger=1` | Node fetches a **missing** ledger from peers. Start a node with no history against a running cluster, or restart one node after the others have advanced. | T2 | -| **Peer** (2): `peer.proposal.receive`, `peer.validation.receive` | `trace_peer=1` | Inbound consensus messages from peers; fresh trace roots. T2 only, and high volume. | T2 | -| **PathFind** (4): `pathfind.request`, `pathfind.compute`, `pathfind.discover`, `pathfind.update_all` | `trace_rpc=1` | `curl -s http://localhost:5005 -d '{"method":"ripple_path_find","params":[{"source_account":"…","destination_account":"…","destination_amount":"100"}]}'`. `pathfind.update_all` fires on ledger close while a request is active. | T1 | +| Span family (count) | Config toggle | How to trigger | Test | +| ------------------------------------------------------------------------------------------------------------------------------- | -------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ------- | +| **RPC** (5 total, 3 here): `rpc.http_request`, `rpc.process`, `rpc.command.` | `trace_rpc=1` | Any HTTP JSON-RPC call: `curl -s http://localhost:5005 -d '{"method":"server_info"}'`. `rpc.command.` is one family — the command name is part of the span name. | T1 | +| **RPC** (cont.): `rpc.ws_message`, `rpc.ws_upgrade` | `trace_rpc=1` | Needs a WebSocket client against `[port_ws_public]` (**6005**) or `[port_ws_admin_local]` (6006). `rpc.ws_upgrade` covers the handshake — force a failure to see its error path. `curl` alone will not do it. | — | +| **gRPC** (1): `grpc.` | `trace_rpc=1` | Call a gRPC method (`GetLedger`, `GetLedgerData`, …). **Requires a `[port_grpc]` stanza — the shipped `xrpld-telemetry*.cfg` files define none**, so add one first. | — | +| **Transaction** (6 total, 4 here): `tx.process`, `tx.preflight`, `tx.preclaim`, `tx.transactor` | `trace_transactions` | Submit any transaction (T1 Step 4). The three apply-stage spans share the tx's deterministic trace id; the `stage` attribute says where a failing tx stopped. | T1 | +| **Transaction** (cont.): `tx.receive` | `trace_transactions` | A **peer** relays a transaction. Never appears in standalone — submit on one node of the cluster and look on another. | T2 | +| **Transaction** (cont.): `tx.apply` | `trace_transactions` | Ledger close with a non-empty transaction set: submit, then `ledger_accept` (T1) or wait for consensus (T2). | T1 / T2 | +| **TxQ** (6): `txq.enqueue`, `txq.apply_direct`, `txq.batch_clear`, `txq.accept`, `txq.accept_tx`, `txq.cleanup` | `trace_transactions` | `txq.enqueue`/`apply_direct` on every submission; `txq.accept`/`accept_tx`/`cleanup` on every ledger close. To force real queueing, submit faster than ledgers close or with a fee below the required fee level. | T1 | +| **Consensus** (13 total, 6 here): `consensus.round`, `.phase.open`, `.mode_change`, `.ledger_close`, `.accept`, `.accept.apply` | `trace_consensus=1` | A standalone `ledger_accept` drives a whole simulated round, so these six fire in T1 as well as on every real close in T2. Note `consensus.round` is ended by the **next** round's start, so a single `ledger_accept` leaves it open and Tempo will not return it. | T1 / T2 | +| **Consensus** (cont., 3): `.establish`, `.update_positions`, `.check` | `trace_consensus=1` | Need the establish phase, which the simulated round skips by jumping straight to `Accepted`. Bring up T2 and wait for a timer-driven round. | T2 | +| **Consensus** (cont., 2): `.proposal.send`, `.validation.send` | `trace_consensus=1` | Need the node to propose, which needs a **validator key** — not peers. The shipped `xrpld-telemetry*.cfg` set no `[validation_seed]`/`[validator_token]`, so a standalone node only observes. | T2 | +| **Consensus** (cont., 2): `.proposal.receive`, `.validation.receive` | `trace_consensus=1` | A peer's consensus message arriving. T2 only. | T2 | +| **Ledger** (4 total, 2 here): `ledger.build`, `ledger.store` | `trace_ledger=1` | Any ledger close: `ledger_accept` in standalone, or consensus in T2. | T1 / T2 | +| **Ledger** (cont.): `ledger.validate` | `trace_ledger=1` | Belongs to `LedgerMaster::checkAccept`, which standalone never reaches — `consensusBuilt` returns early and `switchLCL` takes its standalone branch instead. Needs peers or an inbound validation. | T2 | +| **Ledger** (cont.): `ledger.acquire` | `trace_ledger=1` | Node fetches a **missing** ledger from peers. Start a node with no history against a running cluster, or restart one node after the others have advanced. | T2 | +| **Peer** (2): `peer.proposal.receive`, `peer.validation.receive` | `trace_peer=1` | Inbound consensus messages from peers; fresh trace roots. T2 only, and high volume. | T2 | +| **PathFind** (4): `pathfind.request`, `pathfind.compute`, `pathfind.discover`, `pathfind.update_all` | `trace_rpc=1` | `curl -s http://localhost:5005 -d '{"method":"ripple_path_find","params":[{"source_account":"…","destination_account":"…","destination_amount":"100"}]}'`. `pathfind.update_all` fires on ledger close while a request is active. | T1 | Notes that matter when a span you expect is missing: - **Toggles are per-subsystem and all default to on** (`trace_rpc`, `trace_transactions`, `trace_consensus`, `trace_peer`, `trace_ledger`), but `[telemetry] enabled` defaults to **0** — nothing is emitted until it is `1`. -- **`consensus.*` and `peer.*` cannot be produced in standalone mode.** If Test 1 - shows none, that is correct behaviour, not a regression — see "Expected spans - (standalone mode)" above. +- **`peer.*` cannot be produced in standalone mode.** Both peer spans are created + in inbound message handlers, and `-a` turns peerfinder's `autoConnect` off, so + the node opens no outbound peer connections and receives nothing. If Test 1 + shows none, that is correct behaviour, not a regression. +- **`consensus.*` is only partly absent in standalone.** `consensus.round`, + `.phase.open`, `.mode_change`, `.ledger_close`, `.accept` and `.accept.apply` + all fire on a `ledger_accept`; the other seven need the establish phase, a + validator key, or a peer — see the Consensus rows above. - **`rpc.ws_*` and `grpc.*` need a client and a port the quick tests do not use.** Absence in T1/T2 is expected. -- Trace ids are deterministic for transactions (`txID[0:16]`) and consensus - rounds (`prevLedgerHash[0:16]`), so you can compute the id you expect rather - than searching for it. +- Trace ids are deterministic for transactions (from `txID`) and consensus rounds + (from `prevLedgerHash`): the trace id is the hash's first **16 bytes**, so from + a hex-printed hash take the first **32 characters**. This holds under the + default `consensus_trace_strategy=deterministic`; set it to `random` and each + node gives its round a random trace id instead, joinable only by the + `consensus_ledger_id` attribute. --- @@ -499,8 +554,11 @@ registration. For what each dashboard covers, see [`docs/telemetry-runbook.md`](../../docs/telemetry-runbook.md) **§ Grafana -Dashboards** — the per-dashboard reference. Listing them here would be a second -copy that rots (this section previously named 5 of the 15 provisioned). +Dashboards**. That reference is partial: 9 of the 15 provisioned dashboards have +a section there, and six — `fee-market`, `job-queue`, `ledger-data-sync`, +`overlay-traffic-detail`, `peer-quality` and `validator-health` — do not. For +those, open a panel's info icon in Grafana; the panel descriptions carry the same +reference format. Pre-configured datasources: @@ -566,11 +624,17 @@ Consequences worth knowing before you debug against the cloud stack: pipeline, so `span_*` rates stay exact while only ~1 trace in 200 is retrievable by trace ID. A trace you can see in a metric may not exist in Tempo. -- **Pathfinding account hashing does not happen on the cloud export.** The base - config's `attributes/hash` processor hashes `pathfind_source_account` and - `pathfind_dest_account`. It is absent from every cloud pipeline, so those two - attributes leave for Grafana Cloud (and, on that config, for Tempo) with their - raw account values. +- **The same account carries a different token on each config.** No raw account + address leaves the node: the path-finding handlers under + `src/xrpld/rpc/handlers/orderbook/` pass both accounts through + `redactAccount()` first, which is a prefix of the address's SHA-512Half digest + (contract in `include/xrpl/telemetry/Redaction.h`). The base config's + `attributes/hash` processor then hashes that token a second time; no cloud + pipeline has it. The token is deterministic, so one account stays correlatable + across nodes and restarts — but only within one config. A trace stored while + the collector ran the base config must not be joined against a trace stored + under the cloud config, because the same account appears under two different + tokens. ### Step 4: Verify data reaches Grafana Cloud @@ -579,7 +643,7 @@ Cloud instance and confirm: - **Traces**: Explore → hosted Tempo datasource → search `{resource.service.name="xrpld"}` - **Metrics**: Explore → hosted Prometheus/Mimir → query `span_calls_total` -- **Logs**: Explore → hosted Loki → query `{service_name="xrpld"}` (requires `warning`+ file logging). **Not `{job="xrpld"}`** — see the note under Test 3 Step 3. +- **Logs**: Explore → hosted Loki → query `{service_name="xrpld"}` (requires file logging, at a level low enough to keep the correlated lines — the shipped devnet config's `debug` does, the mainnet config's `warning` suppresses them). **Not `{job="xrpld"}`** — see the note under Test 3 Step 3. If nothing appears, check the collector logs for auth/export errors: @@ -603,10 +667,13 @@ end-to-end log-trace correlation pipeline. ### Step 1: Verify trace_id in log output After running Test 1 or Test 2 (which generate RPC spans), check the -xrpld debug.log for trace context: +xrpld debug.log for trace context. A Test 1 run writes +`docker/telemetry/data/logs/xrpld-devnet/debug.log`; the mainnet config writes +`docker/telemetry/data/logs/mainnet/debug.log` instead. ```bash -grep 'trace_id=[a-f0-9]\{32\} span_id=[a-f0-9]\{16\}' /path/to/debug.log +grep 'trace_id=[a-f0-9]\{32\} span_id=[a-f0-9]\{16\}' \ + docker/telemetry/data/logs/xrpld-devnet/debug.log ``` Expected: log lines with `trace_id=<32hex> span_id=<16hex>` between the @@ -624,7 +691,8 @@ NOT have trace context — this is expected. Extract a `trace_id` from the log and verify it exists in Tempo: ```bash -TRACE_ID=$(grep -m1 -o 'trace_id=[a-f0-9]\{32\}' /path/to/debug.log | cut -d= -f2) +TRACE_ID=$(grep -m1 -o 'trace_id=[a-f0-9]\{32\}' \ + docker/telemetry/data/logs/xrpld-devnet/debug.log | cut -d= -f2) echo "Checking trace: $TRACE_ID" curl -s "http://localhost:3200/api/traces/$TRACE_ID" | jq '.batches | length' ``` @@ -668,9 +736,11 @@ Counting `.data.result | length` would count streams, not log lines. > variant also sets `job=xrpld`, in `otel-collector-config.grafanacloud.yaml`. > Either way `{job="xrpld"}` does not work as a selector: on OTLP ingest Loki > promotes only an allow-listed set of resource attributes to indexed stream -> labels (`service.name` → `service_name`, plus `service.namespace`, -> `service.instance.id`, `deployment.environment`, `k8s.*`, `cloud.*`), and `job` -> is not on the list. This repo mounts no Loki config override — the `loki` +> labels. On the pinned `grafana/loki:3.7.6` that list is a fixed 18 keys, +> including `service.name` → `service_name`, `service.namespace`, +> `service.instance.id`, `deployment.environment` and `container.name`. `k8s.*` +> and `cloud.*` are enumerated key lists (ten and two entries), not wildcards. +> `job` is not on the list. This repo mounts no Loki config override — the `loki` > service runs the image's built-in `/etc/loki/local-config.yaml` > named in `docker-compose.yml` — so `job` lands in **structured metadata**, which > cannot be a stream selector. `{job="xrpld"}` therefore returns **zero results @@ -717,11 +787,9 @@ Counting `.data.result | length` would count streams, not log lines. docker compose -f docker/telemetry/docker-compose.yml logs otel-collector ``` 2. Verify xrpld telemetry config has `enabled=1` and correct endpoint -3. Check that otel-collector port 4318 is accessible (`-f` would fail on the - receiver's 404 for `GET /`, so test for any HTTP status instead): - ```bash - curl -so /dev/null -w '%{http_code}\n' http://localhost:4318/ - ``` +3. Check the collector is up — the readiness check in Test 1 Step 1. Probe + `health_check` on 13133, not the OTLP/HTTP port 4318, which answers 404 to a + `GET /` 4. Increase `batch_delay_ms` or decrease `batch_size` in xrpld config ### Nodes not reaching "proposing" state @@ -802,11 +870,13 @@ Counting `.data.result | length` would count streams, not log lines. processors: [resource/tier, resource/stripsdk, batch] exporters: [prometheus] ``` - Both receivers are required. `spanmetrics` carries the span-derived + Both receivers are required. `span_metrics` carries the span-derived `span_*` series; `otlp` carries the node's native `beast::insight` / MetricsRegistry metrics, which arrive on the same OTLP port. Dropping `otlp` silently removes every native metric while the `span_*` ones keep - working — so the dashboards only half-break. + working — so the dashboards only half-break. (The cloud config, + `otel-collector-config.grafanacloud.yaml`, spells the same connector + `spanmetrics`; both are valid ids for it.) 3. Verify Prometheus can reach collector: ```bash curl -s http://localhost:9090/api/v1/targets | jq '.data.activeTargets' diff --git a/docker/telemetry/integration-test.sh b/docker/telemetry/integration-test.sh index 71747301bb..8efe4388b3 100755 --- a/docker/telemetry/integration-test.sh +++ b/docker/telemetry/integration-test.sh @@ -444,6 +444,12 @@ port = $PEER_PORT ip = 0.0.0.0 protocol = peer +# A private id, so telemetry stamps xrpl.network.type=unknown. The config +# default is id 0, which maps to "mainnet" -- this cluster's spans and metrics +# would then share dashboard series with real mainnet data. +[network_id] +1025 + [node_db] type=NuDB path=$NODE_DIR/nudb @@ -479,7 +485,6 @@ trace_transactions=1 trace_consensus=1 trace_peer=1 trace_ledger=1 -metrics_endpoint=http://localhost:4318/v1/metrics [insight] # server=otel is the only load-bearing key here -- it selects OTelCollector so diff --git a/docker/telemetry/otel-collector-config.yaml b/docker/telemetry/otel-collector-config.yaml index 5d238cdae7..6a81eb443d 100644 --- a/docker/telemetry/otel-collector-config.yaml +++ b/docker/telemetry/otel-collector-config.yaml @@ -92,16 +92,17 @@ processors: # and arrives as the label `service_name`, which is what the LogQL # examples in the runbook and TESTING.md select on. # - # A custom `job` attribute is NOT on that list. Verified against - # grafana/loki:3.4.2 with the default config: after ingesting through - # this pipeline, /loki/api/v1/labels returned only `service_name` and - # `deployment_environment`, `{job="xrpld"}` matched 0 streams, and + # A custom `job` attribute is NOT on that list: the pinned + # grafana/loki:3.7.6 prints an 18-key default list under + # limits_config.otlp_config, and `job` is not one of them. Ingesting + # through this pipeline, /loki/api/v1/labels returned only `service_name` + # and `deployment_environment`, `{job="xrpld"}` matched 0 streams, and # `job` appeared as structured metadata instead — which a `{...}` # stream selector cannot match. Promoting it would mean mounting a Loki # config and adding it to limits_config.otlp_config.resource_attributes # (additive to Loki's defaults unless ignore_defaults is set), which is # not worth a constant value — especially as Loki caps index labels at - # 15 and already promotes ~17 by default. Select on `service_name`. + # 15 and already promotes 18 by default. Select on `service_name`. - key: service.name value: xrpld action: upsert From 31b58e67242617ce3638371fa13d00066a12784a Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 14 Sep 2026 20:12:20 +0100 Subject: [PATCH 08/10] fix(telemetry): build the metrics pipeline at construction, before any producer MetricsRegistry created its provider and synchronous instruments in start(), called from setup() after the node identity was read. Every XRPL_METRIC_* call site creates its instrument on first use, so any site that ran before that point found no meter and never recorded again. The start was moved three times to chase the newest early caller; nothing guaranteed the order. Build the pipeline in the constructor instead. ApplicationImp declares metricsRegistry_ right after telemetry_ and before every subsystem, so declaration order now guarantees the instruments exist before any producer. start() is gone and its config parsing moves to makeMetricsRegistryOptions(). The registry now guarantees a meter whenever it is enabled: the real one, or the OTel no-op meter if the pipeline failed to build. disablePipeline() owns that fallback and its one error log, and the constructor routes both std::exception and a non-std throw through it, because the SDK is third-party code. So the macros shrink to one function-local static built from meter() plus the record call: no once-flag, no null check, and no path for a call that arrives before the meter, because that state no longer exists. The three observable macros drop the same now-dead meter check. The lifecycle is three explicit phases with a Phase enum: Ready at construction, GaugesArmed by startAsyncGauges() once overlay_ exists, and Stopped by stop(). startAsyncGauges() checks the phase before the pipeline, so a second call and a call after stop() are each reported as what they are. run() stops both observers (insight collector, registry) before any service, and ~ApplicationImp repeats the stop for the setup() failure paths that never reach run(). stop() already detaches the callbacks, so it is the only call. Meter name and version come from kMeterName/kMeterVersion, and the endpoint default from Telemetry::Setup, so the two metric pipelines share one source for both. --- .../05-configuration-reference.md | 58 ++- OpenTelemetryPlan/OpenTelemetryPlan.md | 10 +- src/tests/libxrpl/telemetry/MetricMacros.cpp | 4 +- .../libxrpl/telemetry/MetricsRegistry.cpp | 117 +++-- src/xrpld/app/main/Application.cpp | 257 +++++------ src/xrpld/app/main/Main.cpp | 3 +- src/xrpld/telemetry/MetricMacros.h | 405 ++++++++---------- src/xrpld/telemetry/MetricsRegistry.cpp | 106 +++-- src/xrpld/telemetry/MetricsRegistry.h | 192 +++++---- 9 files changed, 574 insertions(+), 578 deletions(-) diff --git a/OpenTelemetryPlan/05-configuration-reference.md b/OpenTelemetryPlan/05-configuration-reference.md index baceb4680f..23affa5c9e 100644 --- a/OpenTelemetryPlan/05-configuration-reference.md +++ b/OpenTelemetryPlan/05-configuration-reference.md @@ -25,32 +25,31 @@ The authoritative `[telemetry]` example lives in `cfg/xrpld-example.cfg`. Teleme > > - **Traces**: the tracer resource is built in `Telemetry::start()` > (`Telemetry.cpp:380-387`), which runs after `ApplicationImp::setup()` has -> called `setServiceInstanceId()` (`Application.cpp:1323`) with the Base58 +> called `setServiceInstanceId()` (in `ApplicationImp::setup()`) with the Base58 > node public key. An unset key therefore still yields the node key. The > `spanmetrics` connector derives `span_calls_total` / > `span_duration_milliseconds_*` from those spans, so span metrics inherit > the correct id too. > - **Native `XRPL_METRIC_*` metrics** build their **own** MeterProvider -> resource in `MetricsRegistry::initExporterAndProvider()` -> (`MetricsRegistry.cpp:280`, `:296-304`, provider created at `:339`), and -> `ApplicationImp::startTelemetry()` supplies the id with an explicit node-key -> fallback (`Application.cpp:1674-1679`: read the config key, and -> `if (instanceId.empty() && nodeIdentity_)` substitute -> `toBase58(TokenType::NodePublic, …)`). By then `setup()` has resolved -> `nodeIdentity_` (`Application.cpp:1315`), so these metrics carry the node -> key even with the config key unset. +> resource in `MetricsRegistry::initExporterAndProvider()`, called from the +> registry's constructor. `makeMetricsRegistryOptions()` in `Application.cpp` +> supplies the id: the config key when set, else the node public key that +> `Main.cpp` resolves before `ApplicationImp` is constructed (the same source +> `Telemetry`'s own metrics resource uses). On a first boot with no node key +> yet, both `service_instance_id` and `xrpl.node.id` are left off until the +> next restart. > - **`beast::insight` metrics** are the exception. They use the **global** > MeterProvider, whose resource is built in the `TelemetryImpl` > **constructor** (`Telemetry.cpp:321-338`, `initMetrics()` at `:447`), > because insight instruments are created eagerly in subsystem constructors -> and would otherwise bind to the noop provider forever. At that point -> `serviceInstanceId` is still `""` (`Application.cpp:348` passes an empty -> node key), and the code comment at `Telemetry.cpp:333-336` states plainly +> and would otherwise bind to the noop provider forever. The constructor +> receives the key `Main.cpp` resolved, which is empty when no key exists +> yet, and the code comment in `TelemetryImpl::initMetrics()` states plainly > that the later setter "cannot change this immutable resource". Worse, -> `initMetrics()` sets the attribute **unconditionally** -> (`Telemetry.cpp:488`), so the resource carries `service.instance.id=""` -> rather than omitting it — whereas `MetricsRegistry` guards the same write -> with `if (!instanceId.empty())` (`MetricsRegistry.cpp:302-303`). +> `initMetrics()` sets the attribute **unconditionally**, so on such a run +> the resource carries `service.instance.id=""` rather than omitting it — +> whereas `MetricsRegistry` guards the same write with +> `if (!options.serviceInstanceId.empty())` in `MetricsRegistry::initExporterAndProvider()`. > > Result: with `service_instance_id` unset, `beast::insight` metrics — and only > those — export with an empty `service.instance.id`. Every shipped Grafana @@ -70,7 +69,7 @@ The authoritative `[telemetry]` example lives in `cfg/xrpld-example.cfg`. Teleme | -------------------------- | ------ | ---------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | `enabled` | 0 or 1 | `0` | Enable/disable telemetry | | `traces_endpoint` | string | `http://localhost:4318/v1/traces` | OTLP/HTTP collector endpoint for **traces** | -| `metrics_endpoint` | string | `http://localhost:4318/v1/metrics` | OTLP/HTTP collector endpoint for the native metrics pipeline (`MetricsRegistry`). Read in `Application.cpp:1670` | +| `metrics_endpoint` | string | `http://localhost:4318/v1/metrics` | OTLP/HTTP collector endpoint for the native metrics pipeline (`MetricsRegistry`). Read by `makeMetricsRegistryOptions()` in `Application.cpp` | | `use_tls` | 0 or 1 | `0` | Enable TLS for exporter connection | | `tls_ca_cert` | string | `""` | Path to CA certificate file | | `tls_client_cert` | string | `""` | Client cert (PEM) for mTLS; empty = one-way; if `enabled=1`, needs key + `use_tls=1` or startup fails | @@ -119,7 +118,7 @@ the corresponding subsystems are instrumented: The parser `makeTelemetrySetup()` in `src/libxrpl/telemetry/TelemetryConfig.cpp` reads the `[telemetry]` `Section` and populates a `Telemetry::Setup` struct, applying the defaults listed in Section 5.1.2 via `section.valueOr(...)`. It takes `serviceInstanceId` from the `nodePublicKey` argument when the key is absent, applies one unconditional `traces_endpoint` default (`dflt::tracesEndpoint`) — the parser has no notion of exporter type — and leaves the sampling ratio at its fixed 1.0 default (a `static constexpr` member, so there is nothing to parse). It also rejects two contradictory mTLS configurations outright (`tls_client_cert` without `tls_client_key`, and either without `use_tls=1`) rather than failing open at handshake time. -`metrics_endpoint` reaches `MetricsRegistry` by a second route: `ApplicationImp::startTelemetry()` reads it from the same `Section` and passes it to `MetricsRegistry::start()`, because `Telemetry` does not expose the `Setup` it parsed. Both metric exporters resolve to that one key: +`metrics_endpoint` reaches `MetricsRegistry` by a second route: `makeMetricsRegistryOptions()` in `Application.cpp` reads it from the same `Section` and passes it to the registry's constructor, because `Telemetry` does not expose the `Setup` it parsed. Both metric exporters resolve to that one key: | Metric source | Exporter built by | URL comes from | | ------------------------------------------ | -------------------------------------------- | -------------------------------------------------------------------- | @@ -134,20 +133,17 @@ Setting `traces_endpoint` therefore moves traces only; both metric pipelines fol ### 5.3.1 ApplicationImp Changes -> **Deferred identity**: The node public key (`nodeIdentity_`) is not -> available during `ApplicationImp`'s member initializer list — it is -> resolved later in `setup()`. The `Telemetry` object is therefore -> constructed with an empty `serviceInstanceId` and patched via -> `setServiceInstanceId()` once `setup()` has called `getNodeIdentity()`. -> **This patch reaches traces only.** The **global** MeterProvider resource — -> the one `beast::insight` metrics use — is already frozen by then (§5.1.1), so -> those metrics keep whatever `service_instance_id` the config supplied (`""` -> if it supplied none). Native `XRPL_METRIC_*` metrics do not go through this -> patch at all: `startTelemetry()` re-reads the config key and applies its own -> node-key fallback when building `MetricsRegistry`'s separate resource -> (`Application.cpp:1674-1679`). +> **Identity at construction**: `Main.cpp` resolves the node public key with +> `resolveNodePublicKey()` before `ApplicationImp` is built and passes it to +> the constructor, so both metric resources — the **global** MeterProvider the +> `beast::insight` metrics use, and `MetricsRegistry`'s separate one — carry it +> from the start. `getNodeIdentity()` in `setup()` stays authoritative; when it +> mints a key that did not exist at construction (a first boot), it patches the +> tracer via `setServiceInstanceId()`. **That patch reaches traces only**: both +> metric resources are frozen once their providers are built, so on that one +> run the metrics report without a node id until the next restart. -`ApplicationImp` (in `src/xrpld/app/main/Application.cpp`) owns a `std::unique_ptr telemetry_`. It is built in the member initializer list via `makeTelemetry(makeTelemetrySetup(...))` with an empty `serviceInstanceId`, then patched in `setup()` by calling `setServiceInstanceId()` with the Base58 node public key (unless the user supplied a custom `service_instance_id`). `start()` and `run()` forward to `telemetry_->start()` / `telemetry_->stop()`, and `getTelemetry()` returns the owned instance. +`ApplicationImp` (in `src/xrpld/app/main/Application.cpp`) owns a `std::unique_ptr telemetry_` and, declared right after it, a `std::unique_ptr metricsRegistry_`. Both are built in the member initializer list, before every subsystem, from the node key `Main.cpp` resolved (empty on a first boot). `setup()` patches the tracer via `setServiceInstanceId()` if `getNodeIdentity()` minted a new key, starts tracing with `startTelemetry()` before the first consensus round, and arms the registry's observable gauges with `startTelemetryGauges()` once `overlay_` exists. `run()` stops both observers before any service, then stops telemetry last; `~ApplicationImp` repeats those stops for the paths that never reach `run()`. `getTelemetry()` and `getMetricsRegistry()` return the owned instances. ### 5.3.2 ServiceRegistry Interface Addition diff --git a/OpenTelemetryPlan/OpenTelemetryPlan.md b/OpenTelemetryPlan/OpenTelemetryPlan.md index f2855643a8..e914e8062c 100644 --- a/OpenTelemetryPlan/OpenTelemetryPlan.md +++ b/OpenTelemetryPlan/OpenTelemetryPlan.md @@ -166,11 +166,11 @@ Configuration is handled through the `[telemetry]` section in `xrpld.cfg` with o Endpoints are spread across **three** keys in two sections, not one "traces and metrics" pair: -| Signal | Key | Default | Source | -| ---------------------------------------------------- | ------------------------------ | ---------------------------------- | --------------------------- | -| Traces | `[telemetry] endpoint` | `http://localhost:4318/v1/traces` | `TelemetryConfig.cpp:36,61` | -| Native metrics (`XRPL_METRIC_*` / `MetricsRegistry`) | `[telemetry] metrics_endpoint` | `http://localhost:4318/v1/metrics` | `Application.cpp:1670` | -| `beast::insight` metrics (`server=otel`) | `[insight] endpoint` | `http://localhost:4318/v1/metrics` | `CollectorManager.cpp:50` | +| Signal | Key | Default | Source | +| ---------------------------------------------------- | ------------------------------ | ---------------------------------- | --------------------------------------------------- | +| Traces | `[telemetry] endpoint` | `http://localhost:4318/v1/traces` | `TelemetryConfig.cpp:36,61` | +| Native metrics (`XRPL_METRIC_*` / `MetricsRegistry`) | `[telemetry] metrics_endpoint` | `http://localhost:4318/v1/metrics` | `makeMetricsRegistryOptions()` in `Application.cpp` | +| `beast::insight` metrics (`server=otel`) | `[insight] endpoint` | `http://localhost:4318/v1/metrics` | `CollectorManager.cpp:50` | `[telemetry]` itself has exactly **one** `endpoint` key, and it is traces-only. diff --git a/src/tests/libxrpl/telemetry/MetricMacros.cpp b/src/tests/libxrpl/telemetry/MetricMacros.cpp index 988cf1f1ba..d29aeb2930 100644 --- a/src/tests/libxrpl/telemetry/MetricMacros.cpp +++ b/src/tests/libxrpl/telemetry/MetricMacros.cpp @@ -65,7 +65,7 @@ public: /** * Number of times meter() has been consulted, so a test can assert the - * create-once (call_once) and disabled-gating behavior exactly. + * create-once (function-local static) and disabled-gating behavior exactly. */ [[nodiscard]] int meterCalls() const noexcept @@ -202,7 +202,7 @@ TEST(MetricMacros, counter_inc_creates_once_and_does_not_crash) app, "test_macro_counter_total", "Test counter for macro unit test"); } - // Create-once proof: std::call_once consults meter() exactly once across + // Create-once proof: the function-local static consults meter() exactly once across // the three calls at this site, then reuses the cached instrument handle. EXPECT_EQ(app.registry().meterCalls(), 1); } diff --git a/src/tests/libxrpl/telemetry/MetricsRegistry.cpp b/src/tests/libxrpl/telemetry/MetricsRegistry.cpp index af3eb68ddf..ba5d62e268 100644 --- a/src/tests/libxrpl/telemetry/MetricsRegistry.cpp +++ b/src/tests/libxrpl/telemetry/MetricsRegistry.cpp @@ -20,9 +20,10 @@ * real producer, xrpl::to_string(RangeSet), rather than restating its * format. * - * 4. The no-op / telemetry-disabled path — construction, the two-phase - * start() / startAsyncGauges() / stop() lifecycle, and the synchronous - * record*() methods. Guarded, because when XRPL_ENABLE_TELEMETRY is + * 4. The no-op / telemetry-disabled path — construction (which is where the + * pipeline and the synchronous instruments are built), startAsyncGauges(), + * stop(), and the synchronous record*() methods. Guarded, because when + * XRPL_ENABLE_TELEMETRY is * defined MetricsRegistry.cpp is not compiled into this binary (see * src/tests/libxrpl/CMakeLists.txt) and its out-of-line symbols are * unresolvable here. @@ -603,22 +604,21 @@ using namespace xrpl; namespace { /** - * OTLP/HTTP endpoint used by every start() call below. Nothing ever dials it + * OTLP/HTTP endpoint given to every registry below. Nothing ever dials it * -- these tests exercise the no-op path -- it just has to be a plausible URL. - * It reaches start() through @ref kTestStartOptions. + * It reaches the constructor through @ref kTestOptions. */ constexpr std::string_view kTestEndpoint{"http://localhost:4318/v1/metrics"}; /** - * The only StartOptions field these tests need. + * The only Options field these tests need. * - * start() takes the StartOptions aggregate, not a string. The other fields -- - * resource identity, network id, TLS paths -- are never read on the no-op - * path, and their defaults already mean "unset". One shared value keeps all - * six call sites on the same endpoint. + * The constructor takes the Options aggregate, not a string. The other fields + * -- resource identity, network id, TLS paths -- are never read on the no-op + * path, and their defaults already mean "unset". One shared value keeps every + * construction on the same endpoint. */ -telemetry::MetricsRegistry::StartOptions const kTestStartOptions{ - .endpoint = std::string{kTestEndpoint}}; +telemetry::MetricsRegistry::Options const kTestOptions{.endpoint = std::string{kTestEndpoint}}; /** * Minimal mock ServiceRegistry for MetricsRegistry testing. @@ -904,16 +904,15 @@ protected: TEST_F(MetricsRegistryTest, disabled_construction) { // Construct with enabled=false; should be a no-op. - telemetry::MetricsRegistry const registry(false, mockApp_, j_); + telemetry::MetricsRegistry const registry(false, mockApp_, j_, kTestOptions); EXPECT_FALSE(registry.isEnabled()); } -TEST_F(MetricsRegistryTest, disabled_start_stop) +TEST_F(MetricsRegistryTest, disabled_construct_stop) { - telemetry::MetricsRegistry registry(false, mockApp_, j_); + telemetry::MetricsRegistry registry(false, mockApp_, j_, kTestOptions); - // start() and stop() should be no-ops when disabled. - registry.start(kTestStartOptions); + // stop() should be a no-op when disabled. registry.stop(); // Double stop should be safe. @@ -921,48 +920,41 @@ TEST_F(MetricsRegistryTest, disabled_start_stop) } // --------------------------------------------------------------------------- -// The two-phase startup split: start() then startAsyncGauges(). +// The two startup phases: construction, then startAsyncGauges(). // -// Why the split exists: start() reads only config strings, while the -// observable-instrument callbacks registered by startAsyncGauges() read live -// Application services (getOverlay() asserts overlay_ is non-null). The split -// lets the meter go live before the first consensus round records its -// mode-transition counter, while the callbacks still wait for the subsystems. +// Why two phases: the constructor needs only config strings, so it can run in +// the Application's member-init list, before any subsystem that records a +// metric exists. The observable-instrument callbacks registered by +// startAsyncGauges() read live Application services (getOverlay() asserts +// overlay_ is non-null), so they wait until those services are built. // // SCOPE OF THESE TESTS -- read before adding to them. MetricsRegistry.cpp is // compiled into this binary ONLY when telemetry is OFF -// (src/tests/libxrpl/CMakeLists.txt:117-126 -- the `else()` branch; when it is -// ON the .cpp needs concrete xrpld types such as LedgerMaster, TxQ, NetworkOPs, +// (src/tests/libxrpl/CMakeLists.txt -- the `else()` branch; when it is ON the +// .cpp needs concrete xrpld types such as LedgerMaster, TxQ, NetworkOPs, // Overlay and node_store::Database, which a standalone GTest binary cannot -// link). Both start() and startAsyncGauges() have a single definition whose -// whole body sits inside #ifdef XRPL_ENABLE_TELEMETRY, so here they compile to -// an empty body with a [[maybe_unused]] parameter. So these tests pin the -// API SURFACE -- that both entry points exist, are callable in either order, -// and leave the object usable -- and NOT the gauge behaviour. Real coverage of -// "gauges observe values only after startAsyncGauges()" is unreachable from -// this target; it needs the enabled path plus an in-memory metric reader. -// -// Two properties the production code does NOT have, so nothing below asserts -// them: startAsyncGauges() has no idempotency guard (a second call on the -// enabled path would create a second set of same-named instruments), and -// callbacksDetached_ is one-way, so detachCallbacks() followed by -// startAsyncGauges() would register permanently-dead instruments. +// link). The constructor body and startAsyncGauges() sit inside +// #ifdef XRPL_ENABLE_TELEMETRY, so here they compile to empty bodies. So these +// tests pin the API SURFACE -- that the entry points exist, are callable in +// the documented order, and leave the object usable -- and NOT the gauge +// behaviour. Real coverage of "gauges observe values only after +// startAsyncGauges()" is unreachable from this target; it needs the enabled +// path plus an in-memory metric reader. // --------------------------------------------------------------------------- -TEST_F(MetricsRegistryTest, async_gauges_start_after_start_is_safe) +TEST_F(MetricsRegistryTest, async_gauges_after_construction_is_safe) { - telemetry::MetricsRegistry registry(false, mockApp_, j_); + telemetry::MetricsRegistry registry(false, mockApp_, j_, kTestOptions); - // The documented order: provider/sync instruments first, gauges second. - registry.start(kTestStartOptions); + // The documented order: instruments at construction, gauges second. registry.startAsyncGauges(); // State: the enable flag is untouched by either phase. Exact value, not // merely "falsy" -- a phase that flipped it would be a real defect. EXPECT_EQ(registry.isEnabled(), false); - // Synchronous recording must work off phase 1 alone. This is the whole - // point of the split: nothing here needs the gauges to be registered. + // Synchronous recording must work off construction alone. Nothing here + // needs the gauges to be registered. registry.recordRpcStarted("server_info"); registry.recordRpcFinished("server_info", 1000); @@ -970,42 +962,35 @@ TEST_F(MetricsRegistryTest, async_gauges_start_after_start_is_safe) EXPECT_EQ(registry.isEnabled(), false); } -TEST_F(MetricsRegistryTest, async_gauges_before_start_does_not_break_start) +TEST_F(MetricsRegistryTest, async_gauges_twice_is_safe) { - telemetry::MetricsRegistry registry(false, mockApp_, j_); + telemetry::MetricsRegistry registry(false, mockApp_, j_, kTestOptions); - // Negative path: the mis-ordered call, gauges before the provider exists. - // In THIS build it reaches the (void)-cast stub, so what is actually - // proven is only that the entry point tolerates being called first and - // leaves the object usable -- not that the enabled path's `if (!meter_)` - // guard works, since that guard is inside #ifdef XRPL_ENABLE_TELEMETRY and - // is not compiled here. + // A second arm must be a no-op, not a second set of instruments. On the + // enabled path the Phase guard logs and returns; here the stub returns. + registry.startAsyncGauges(); registry.startAsyncGauges(); EXPECT_EQ(registry.isEnabled(), false); - // Phase 1 still works afterwards, so the bad call left no state behind. - registry.start(kTestStartOptions); registry.recordJobQueued("ledgerData", "ProcessLData"); - EXPECT_EQ(registry.isEnabled(), false); - registry.stop(); } TEST_F(MetricsRegistryTest, async_gauges_respect_the_compile_time_guard) { - // Constructed with enabled=true, which on the enabled path would register - // instruments for real. In this build XRPL_ENABLE_TELEMETRY is undefined, - // so both phases compile to the (void)-cast stub branch and neither - // touches the mock -- every MockServiceRegistry accessor throws, so a - // callback that actually ran would surface as a thrown exception here. - telemetry::MetricsRegistry registry(true, mockApp_, j_); + // Constructed with enabled=true, which on the enabled path would build the + // pipeline and register instruments for real. In this build + // XRPL_ENABLE_TELEMETRY is undefined, so both phases compile to the stub + // branch and neither touches the mock -- every MockServiceRegistry + // accessor throws, so a callback that actually ran would surface as a + // thrown exception here. + telemetry::MetricsRegistry registry(true, mockApp_, j_, kTestOptions); // Cause, not just state: the flag really is true, so the no-op below is // attributable to the compile-time guard and not to an early enabled_ // return. EXPECT_EQ(registry.isEnabled(), true); - EXPECT_NO_THROW(registry.start(kTestStartOptions)); EXPECT_NO_THROW(registry.startAsyncGauges()); EXPECT_NO_THROW(registry.stop()); @@ -1014,8 +999,7 @@ TEST_F(MetricsRegistryTest, async_gauges_respect_the_compile_time_guard) TEST_F(MetricsRegistryTest, disabled_recording_methods) { - telemetry::MetricsRegistry registry(false, mockApp_, j_); - registry.start(kTestStartOptions); + telemetry::MetricsRegistry registry(false, mockApp_, j_, kTestOptions); // All recording methods should be no-ops (not crash). registry.recordRpcStarted("server_info"); @@ -1032,8 +1016,7 @@ TEST_F(MetricsRegistryTest, destructor_calls_stop) { { // Let the destructor handle cleanup. - telemetry::MetricsRegistry registry(false, mockApp_, j_); - registry.start(kTestStartOptions); + telemetry::MetricsRegistry const registry(false, mockApp_, j_, kTestOptions); } // If we get here without crash, the destructor handled stop. } diff --git a/src/xrpld/app/main/Application.cpp b/src/xrpld/app/main/Application.cpp index 527789f4c9..76fc7cd32c 100644 --- a/src/xrpld/app/main/Application.cpp +++ b/src/xrpld/app/main/Application.cpp @@ -144,6 +144,65 @@ namespace xrpl { static void fixConfigPorts(Config& config, Endpoints const& endpoints); +/** + * Read the native metrics pipeline settings from [telemetry] and [network_id]. + * + * The identity and TLS values must match what makeTelemetrySetup() gives the + * trace pipeline, or this node reports two identities. Telemetry does not + * expose the Setup it parsed, so those keys are read here a second time. The + * export cadence is the registry's own (10 s) and is not read from config. + * + * @param config The loaded server config. + * @param nodePublicKey Node key resolved in Main.cpp; empty on a first boot. + * @return Options for MetricsRegistry's constructor. + */ +static telemetry::MetricsRegistry::Options +makeMetricsRegistryOptions(Config const& config, std::optional const& nodePublicKey) +{ + auto const& section = config.section("telemetry"); + telemetry::MetricsRegistry::Options options; + + // metrics_endpoint is a full URL of its own, not a host to be joined. The + // default is the one Telemetry::Setup carries, so both pipelines fall back + // to the same collector. + options.endpoint = telemetry::Telemetry::Setup{}.metricsEndpoint; + set(options.endpoint, "metrics_endpoint", section); + + // Same default and same key as makeTelemetrySetup(), so traces and + // metrics carry one service.name. systemName() is "xrpld". + options.serviceName = systemName(); + set(options.serviceName, "service_name", section); + + // Not from config: the build's version, the same source the trace + // resource takes it from. + options.serviceVersion = build_info::getVersionString(); + + // service_instance_id is the label every dashboard filters $node on. + // xrpl.node.id carries the same key and cannot be overridden by config. + // Both come from the key Main.cpp resolved before construction, the same + // source Telemetry's own metrics resource uses. + set(options.serviceInstanceId, "service_instance_id", section); + if (options.serviceInstanceId.empty()) + options.serviceInstanceId = nodePublicKey.value_or(""); + options.nodeId = nodePublicKey.value_or(""); + + // xrpl.network.id, and the xrpl.network.type label the registry derives + // from it. Without this the collector's insert rule fills in its own + // default and a devnet node reports mainnet on this pipeline. + options.networkId = config.networkId; + + // The exporter connection reads the same four TLS keys the trace exporter + // does, so one [telemetry] block covers both signals. use_tls is an int + // compared to 0, matching makeTelemetrySetup(). + int useTls = 0; + set(useTls, "use_tls", section); + options.useTls = useTls != 0; + set(options.tlsCaCertPath, "tls_ca_cert", section); + set(options.tlsClientCertPath, "tls_client_cert", section); + set(options.tlsClientKeyPath, "tls_client_key", section); + return options; +} + // VFALCO TODO Move the function definitions into the class declaration class ApplicationImp : public Application, public BasicApp { @@ -225,7 +284,10 @@ public: std::unique_ptr telemetry_; /** * OTel metrics registry for gap-fill metrics (counters, histograms, - * observable gauges). Created after telemetry_ during setup(). + * observable gauges). Its constructor builds the pipeline and every + * synchronous instrument, so it must stay declared after telemetry_ and + * before every subsystem that records a metric. Declaration order is the + * whole guarantee. Gauges are armed later by startTelemetryGauges(). */ std::unique_ptr metricsRegistry_; Application::MutexType masterMutex_; @@ -361,14 +423,16 @@ public: build_info::getVersionString(), config_->networkId), logs_->journal("Telemetry"))) - // Built here, not in setup(): getMetricsRegistry() is read from the job - // queue and io threads, which are already running, so assigning the - // handle later would race with those reads. + // Built here, not in setup(), for two reasons: getMetricsRegistry() is + // read from job-queue and io threads that are already running, and the + // constructor creates every synchronous instrument, which must happen + // before any subsystem below can record one. , metricsRegistry_( std::make_unique( telemetry_->isEnabled(), *this, - logs_->journal("MetricsRegistry"))) + logs_->journal("MetricsRegistry"), + makeMetricsRegistryOptions(*config_, nodePublicKey))) , txMaster_(*this) , collectorManager_(makeCollectorManager( @@ -547,16 +611,16 @@ public: } /** - * Stop observing and stop telemetry before the members are destroyed. + * Stop both observers and stop telemetry before the members are destroyed. * - * The metrics reader thread runs callbacks that read the services member - * destruction is about to tear down. telemetry_ is declared early because - * the collector needs its MeterProvider, so reverse-order member destruction - * would take it down last. + * The insight collector and the metrics registry each own a reader thread + * whose callbacks read the services member destruction is about to tear + * down. Both are declared early, so reverse-order destruction would take + * them down last. * - * run() does both on the normal path; this covers the paths that never - * reach it -- every `return false` in setup(), and the unit tests. Both - * calls are idempotent. + * run() does all of this on the normal path; this covers the paths that + * never reach it -- every `return false` in setup(), and the unit tests. + * Every call here is idempotent. */ ~ApplicationImp() override { @@ -565,6 +629,7 @@ public: try { collectorManager_->collector()->onCollectionStopping(); + stopMetricsRegistry(); telemetry_->stop(); } catch (std::exception const& e) @@ -1235,30 +1300,18 @@ private: startGenesisLedger(); /** - * Start the tracing pipeline and the metrics provider and synchronous - * instruments. First of the two telemetry startup phases. + * Start the tracing pipeline. * - * Called once from setup(), immediately after metricsRegistry_ is - * constructed. Starting here (rather than in start()) guarantees the OTel - * MeterProvider is live before any metric-emitting code runs — including - * the first consensus round, which records a mode-transition counter, and - * the startup RPCs, whose PerfLog instrumentation records a call-site - * metric. A call-site metric macro caches its instrument on first use via - * std::call_once; if that first use happens while the meter is still - * empty, the instrument latches null for the process lifetime and the - * metric silently never records. + * Called once from setup(), after the node identity is known and before + * beginConsensus() emits the first spans. SpanGuard drops a span whenever + * the global Telemetry instance is not yet live, so this cannot wait for + * start(). * - * Rule for keeping this call site valid: only telemetry work that reads - * NO application subsystem may run here. That holds today — this phase - * uses the config strings and creates only push-model counters and - * histograms, which app code records into once it is ready. Anything that - * registers a callback reading a subsystem must go in - * startTelemetryGauges() instead, because a callback registered here can - * fire on the metrics reader thread while the rest of the application is - * still being built. + * The metrics pipeline is not started here: metricsRegistry_'s constructor + * built it, so its instruments exist before any subsystem does. * - * The resource attributes, including service.instance.id, were supplied at - * construction. + * @pre nodeIdentity_ is populated, so setServiceInstanceId() has already + * supplied the tracer's service.instance.id. */ void startTelemetry() const; @@ -1270,21 +1323,27 @@ private: * Registering an observable instrument arms the metrics reader thread to * invoke its callback, and those callbacks read application services — * getOverlay() asserts overlay_ is non-null, and an assert is not caught - * by the callbacks' own try/catch — so this cannot run as early as - * startTelemetry(). + * by the callbacks' own try/catch — so this cannot run at construction. * - * @pre startTelemetry() has run, and every service the callbacks read is - * constructed. overlay_ is the binding one: the rest (networkOPs_, - * ledgerMaster_, openLedger_, txQ_, nodeStore_, nodeFamily_, - * validators_, acceptedLedgerCache_, cachedSLEs_, acquireStats_, - * timeKeeper_, relationalDatabase_, inboundLedgers_, feeTrack_) are - * already live by the time startTelemetry() is callable, and - * overlay_ is the only one built after it. See + * @pre Every service the callbacks read is constructed. overlay_ is the + * binding one: the rest (networkOPs_, ledgerMaster_, openLedger_, txQ_, + * nodeStore_, nodeFamily_, validators_, acceptedLedgerCache_, + * cachedSLEs_, acquireStats_, timeKeeper_, relationalDatabase_, + * inboundLedgers_, feeTrack_) are built earlier in setup(). See * MetricsRegistry::startAsyncGauges() for the full list. */ void startTelemetryGauges() const; + /** + * Stop the metrics registry: detach its gauge callbacks and join its + * reader thread. Idempotent. Called from run() before any observed + * service stops, and again from ~ApplicationImp for the paths that never + * reach run(). + */ + void + stopMetricsRegistry() const; + std::shared_ptr getLastFullLedger(); @@ -1392,21 +1451,20 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline) // stable per-node key whatever [telemetry] says. telemetry_->setNodeId(toBase58(TokenType::NodePublic, nodeIdentity_->first)); - // Start telemetry here, not in start(). Spans and metrics are both emitted - // during the rest of setup() — the first consensus round in - // beginConsensus() below emits spans and records the process's only - // operating-mode transition — and both are dropped unless the pipeline is - // already live. + // Start tracing here, not in start(). Spans are emitted during the rest of + // setup() — the first consensus round in beginConsensus() below — and are + // dropped unless the global Telemetry instance is already live. // // The position is bounded on both sides: // - After initRelationalDatabase(): the wallet DB must exist for the node // identity above, and a DB failure aborts setup(), so starting earlier - // would export a partial stream for a run that never comes up. - // - Before beginConsensus(): that call emits the first consensus spans - // and the only mode-transition counter increment. + // would export a partial trace stream for a run that never comes up. + // - Before beginConsensus(): that call emits the first consensus spans. // - // Only the observable instruments have to wait for their subsystems; they - // are registered separately by startTelemetryGauges() once overlay_ exists. + // Metrics need nothing here: metricsRegistry_'s constructor built the + // pipeline and the synchronous instruments before any subsystem existed. + // Only the observable instruments wait for their subsystems; they are + // registered by startTelemetryGauges() once overlay_ exists. startTelemetry(); if (validatorKeys_.keys) @@ -1723,67 +1781,23 @@ ApplicationImp::start(bool withTimers) void ApplicationImp::startTelemetry() const { - // Start tracing first so subsequent startup/early activity can be traced. telemetry_->start(); - - // Start the metrics pipeline after telemetry. Everything below is read - // from [telemetry] here because Telemetry does not expose the Setup it - // parsed. Every value must match what makeTelemetrySetup() gave the trace - // pipeline above, or this node reports two identities. - if (metricsRegistry_) - { - auto const& section = config_->section("telemetry"); - - telemetry::MetricsRegistry::StartOptions options; - - // metrics_endpoint is a full URL of its own, not a host to be joined. - options.endpoint = "http://localhost:4318/v1/metrics"; - set(options.endpoint, "metrics_endpoint", section); - - // Same default and same key as makeTelemetrySetup(), so traces and - // metrics carry one service.name. systemName() is "xrpld". - options.serviceName = systemName(); - set(options.serviceName, "service_name", section); - - // Not from config: the build's version, the same source the trace - // resource takes it from at construction. - options.serviceVersion = build_info::getVersionString(); - - // The MeterProvider Resource carries service_instance_id, which - // Prometheus turns into the label every dashboard filters $node on. - set(options.serviceInstanceId, "service_instance_id", section); - if (options.serviceInstanceId.empty() && nodeIdentity_) - options.serviceInstanceId = toBase58(TokenType::NodePublic, nodeIdentity_->first); - - // The node public key also goes on its own resource attribute, - // xrpl.node.id, which config cannot override. - if (nodeIdentity_) - options.nodeId = toBase58(TokenType::NodePublic, nodeIdentity_->first); - - // xrpl.network.id, and the xrpl.network.type label the registry - // derives from it. Without this the collector's insert rule fills in - // its own default and a devnet node reports mainnet on this pipeline. - options.networkId = config_->networkId; - - // The exporter connection reads the same four TLS keys the trace - // exporter does, so one [telemetry] block covers both signals. use_tls - // is an int compared to 0, matching makeTelemetrySetup(). - int useTls = 0; - set(useTls, "use_tls", section); - options.useTls = useTls != 0; - set(options.tlsCaCertPath, "tls_ca_cert", section); - set(options.tlsClientCertPath, "tls_client_cert", section); - set(options.tlsClientKeyPath, "tls_client_key", section); - - metricsRegistry_->start(options); - } } void ApplicationImp::startTelemetryGauges() const { - if (metricsRegistry_) - metricsRegistry_->startAsyncGauges(); + metricsRegistry_->startAsyncGauges(); +} + +void +ApplicationImp::stopMetricsRegistry() const +{ + // stop() detaches the callbacks and then shuts the provider down, which + // joins the reader thread, so once it returns no callback is running or + // can start. The cost is that metrics recorded after this point are not + // exported. + metricsRegistry_->stop(); } void @@ -1858,32 +1872,19 @@ ApplicationImp::run() return getValidators().trustedPublisher(pubKey); }); - // Stop observing before any service below is stopped: the collector's gauge - // callbacks run hook handlers that read ledgerMaster_, networkOPs_, the peer - // finder, the job queue and overlay_. Returns once no callback is running. + // Both observers stop before any service below is stopped. The collector's + // gauge callbacks run hook handlers that read ledgerMaster_, networkOPs_, + // the peer finder, the job queue and overlay_; the registry's callbacks + // run on the OTel reader thread and read nodeStore_, overlay_, networkOPs_, + // loadManager_, ledgerMaster, inboundLedgers and more. Each call returns + // once no callback is running or can start. collectorManager_->collector()->onCollectionStopping(); + stopMetricsRegistry(); // The order of these stop calls is delicate. // Re-ordering them risks undefined behavior. loadManager_->stop(); - // Stop the metrics pipeline BEFORE any service its callbacks read. Those - // callbacks run on the OTel reader thread and touch nodeStore_, overlay_, - // networkOPs_, ledgerMaster, inboundLedgers and more, so a tick arriving - // after one of them has stopped would read dangling state. - // - // detachCallbacks() alone would not be enough: it flips a flag that each - // callback checks on entry, which leaves a callback that is already past - // that check running. stop() shuts the provider down, which joins the - // reader thread, so once it returns no callback is running or can start. - // The cost is that metrics recorded during the remaining shutdown steps - // are not exported. - if (metricsRegistry_) - { - metricsRegistry_->detachCallbacks(); - metricsRegistry_->stop(); - } - shaMapStore_->stop(); jobQueue_->stop(); if (overlay_) diff --git a/src/xrpld/app/main/Main.cpp b/src/xrpld/app/main/Main.cpp index fcae528737..79ba6e809a 100644 --- a/src/xrpld/app/main/Main.cpp +++ b/src/xrpld/app/main/Main.cpp @@ -840,7 +840,8 @@ run(int argc, char** argv) // // Only the construction is covered. The [telemetry] section is parsed // near the top of the member list, before the job queue and node store - // are built, so unwinding that throw destroys very little. setup() is + // are built, so unwinding that throw destroys little: the metrics + // registry, whose destructor joins its export thread. setup() is // left outside deliberately: it starts subsystems whose shutdown order // is delicate, and only the normal stop sequence gets that order right. std::unique_ptr app; diff --git a/src/xrpld/telemetry/MetricMacros.h b/src/xrpld/telemetry/MetricMacros.h index a870df3319..4b0f3754e0 100644 --- a/src/xrpld/telemetry/MetricMacros.h +++ b/src/xrpld/telemetry/MetricMacros.h @@ -11,7 +11,7 @@ * field, no init line, no wrapper method in MetricsRegistry. Covers every * instrument kind the OTel Metrics API defines: * - * Synchronous (create-once via std::call_once, then record on every call): + * Synchronous (created once on first use, then record on every call): * Counter XRPL_METRIC_COUNTER_INC / _ADD [+ _LABELED] * UpDownCounter XRPL_METRIC_UPDOWN_ADD [+ _LABELED] * Histogram XRPL_METRIC_HISTOGRAM_RECORD [+ _LABELED] @@ -91,10 +91,14 @@ * MetricsRegistry::initExporterAndProvider() as today; the * histogram-record call itself can still use the macro. * - * @note Only call the SYNCHRONOUS macros (Counter/UpDownCounter/ - * Histogram/Gauge) from code that runs AFTER MetricsRegistry::start() has - * completed (RPC handlers, job callbacks, consensus rounds, tx apply, peer - * message handlers). + * @note The SYNCHRONOUS macros (Counter/UpDownCounter/Histogram/Gauge) + * create their instrument once, on first use, from + * MetricsRegistry::meter(). The registry builds that meter in its + * constructor, before any subsystem exists, and guarantees it is never + * empty while the registry is enabled (a no-op meter stands in if the + * pipeline failed to build). So a call site holds a valid instrument from + * its first call and needs no check of its own; the only branch on the + * hot path is the isEnabled() gate. * * @note The OBSERVABLE registration macros are the opposite: call them * EAGERLY, exactly once, from constructor/init code -- never from a hot @@ -128,82 +132,57 @@ #ifdef XRPL_ENABLE_TELEMETRY #include // IWYU pragma: keep -#include // IWYU pragma: keep -#define XRPL_METRIC_COUNTER_INC(app, name, description) \ - do \ - { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ - { \ - static opentelemetry::nostd::unique_ptr> \ - xrpl_counter_; \ - static std::once_flag xrpl_once_; \ - std::call_once(xrpl_once_, [&] { \ - if (auto xrpl_m_ = xrpl_mr_->meter()) \ - xrpl_counter_ = xrpl_m_->CreateUInt64Counter((name), (description)); \ - }); \ - if (xrpl_counter_) \ - xrpl_counter_->Add(1); \ - } \ +#define XRPL_METRIC_COUNTER_INC(app, name, description) \ + do \ + { \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + { \ + static auto const xrpl_counter_ = \ + xrpl_mr_->meter()->CreateUInt64Counter((name), (description)); \ + xrpl_counter_->Add(1); \ + } \ } while (false) // The label set is passed as trailing variadic arguments so a // brace-enclosed initializer list (e.g. {{"reason", std::string("x")}}), // which contains a top-level comma, survives preprocessing as a single // logical argument. __VA_ARGS__ re-joins it verbatim into the Add() call. -#define XRPL_METRIC_COUNTER_INC_LABELED(app, name, description, ...) \ - do \ - { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ - { \ - static opentelemetry::nostd::unique_ptr> \ - xrpl_counter_; \ - static std::once_flag xrpl_once_; \ - std::call_once(xrpl_once_, [&] { \ - if (auto xrpl_m_ = xrpl_mr_->meter()) \ - xrpl_counter_ = xrpl_m_->CreateUInt64Counter((name), (description)); \ - }); \ - if (xrpl_counter_) \ - xrpl_counter_->Add(1, __VA_ARGS__); \ - } \ +#define XRPL_METRIC_COUNTER_INC_LABELED(app, name, description, ...) \ + do \ + { \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + { \ + static auto const xrpl_counter_ = \ + xrpl_mr_->meter()->CreateUInt64Counter((name), (description)); \ + xrpl_counter_->Add(1, __VA_ARGS__); \ + } \ } while (false) // Same as XRPL_METRIC_COUNTER_INC, but increments by a caller-supplied amount // instead of a fixed 1 (e.g. bytes transferred, batch sizes). -#define XRPL_METRIC_COUNTER_ADD(app, name, description, amount) \ - do \ - { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ - { \ - static opentelemetry::nostd::unique_ptr> \ - xrpl_counter_; \ - static std::once_flag xrpl_once_; \ - std::call_once(xrpl_once_, [&] { \ - if (auto xrpl_m_ = xrpl_mr_->meter()) \ - xrpl_counter_ = xrpl_m_->CreateUInt64Counter((name), (description)); \ - }); \ - if (xrpl_counter_) \ - xrpl_counter_->Add(amount); \ - } \ +#define XRPL_METRIC_COUNTER_ADD(app, name, description, amount) \ + do \ + { \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + { \ + static auto const xrpl_counter_ = \ + xrpl_mr_->meter()->CreateUInt64Counter((name), (description)); \ + xrpl_counter_->Add(amount); \ + } \ } while (false) // amount is fixed; the trailing variadic args carry the label set (see the // note on XRPL_METRIC_COUNTER_INC_LABELED for why labels are variadic). -#define XRPL_METRIC_COUNTER_ADD_LABELED(app, name, description, amount, ...) \ - do \ - { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ - { \ - static opentelemetry::nostd::unique_ptr> \ - xrpl_counter_; \ - static std::once_flag xrpl_once_; \ - std::call_once(xrpl_once_, [&] { \ - if (auto xrpl_m_ = xrpl_mr_->meter()) \ - xrpl_counter_ = xrpl_m_->CreateUInt64Counter((name), (description)); \ - }); \ - if (xrpl_counter_) \ - xrpl_counter_->Add(amount, __VA_ARGS__); \ - } \ +#define XRPL_METRIC_COUNTER_ADD_LABELED(app, name, description, amount, ...) \ + do \ + { \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + { \ + static auto const xrpl_counter_ = \ + xrpl_mr_->meter()->CreateUInt64Counter((name), (description)); \ + xrpl_counter_->Add(amount, __VA_ARGS__); \ + } \ } while (false) // UpDownCounter: like COUNTER_ADD, but the underlying instrument permits a @@ -212,79 +191,53 @@ // A plain Counter's Add() must never see a negative value per the OTel // API contract; use this macro, not COUNTER_ADD, whenever the value can // decrease. -#define XRPL_METRIC_UPDOWN_ADD(app, name, description, amount) \ - do \ - { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ - { \ - static opentelemetry::nostd::unique_ptr< \ - opentelemetry::metrics::UpDownCounter> \ - xrpl_updown_; \ - static std::once_flag xrpl_once_; \ - std::call_once(xrpl_once_, [&] { \ - if (auto xrpl_m_ = xrpl_mr_->meter()) \ - xrpl_updown_ = xrpl_m_->CreateInt64UpDownCounter((name), (description)); \ - }); \ - if (xrpl_updown_) \ - xrpl_updown_->Add(amount); \ - } \ +#define XRPL_METRIC_UPDOWN_ADD(app, name, description, amount) \ + do \ + { \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + { \ + static auto const xrpl_updown_ = \ + xrpl_mr_->meter()->CreateInt64UpDownCounter((name), (description)); \ + xrpl_updown_->Add(amount); \ + } \ } while (false) // amount may be negative; the trailing variadic args carry the label set // (see the note on XRPL_METRIC_COUNTER_INC_LABELED for why labels are variadic). -#define XRPL_METRIC_UPDOWN_ADD_LABELED(app, name, description, amount, ...) \ - do \ - { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ - { \ - static opentelemetry::nostd::unique_ptr< \ - opentelemetry::metrics::UpDownCounter> \ - xrpl_updown_; \ - static std::once_flag xrpl_once_; \ - std::call_once(xrpl_once_, [&] { \ - if (auto xrpl_m_ = xrpl_mr_->meter()) \ - xrpl_updown_ = xrpl_m_->CreateInt64UpDownCounter((name), (description)); \ - }); \ - if (xrpl_updown_) \ - xrpl_updown_->Add(amount, __VA_ARGS__); \ - } \ +#define XRPL_METRIC_UPDOWN_ADD_LABELED(app, name, description, amount, ...) \ + do \ + { \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + { \ + static auto const xrpl_updown_ = \ + xrpl_mr_->meter()->CreateInt64UpDownCounter((name), (description)); \ + xrpl_updown_->Add(amount, __VA_ARGS__); \ + } \ } while (false) -#define XRPL_METRIC_HISTOGRAM_RECORD(app, name, description, value) \ - do \ - { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ - { \ - static opentelemetry::nostd::unique_ptr> \ - xrpl_hist_; \ - static std::once_flag xrpl_once_; \ - std::call_once(xrpl_once_, [&] { \ - if (auto xrpl_m_ = xrpl_mr_->meter()) \ - xrpl_hist_ = xrpl_m_->CreateDoubleHistogram((name), (description)); \ - }); \ - if (xrpl_hist_) \ - xrpl_hist_->Record(static_cast(value), opentelemetry::context::Context{}); \ - } \ +#define XRPL_METRIC_HISTOGRAM_RECORD(app, name, description, value) \ + do \ + { \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + { \ + static auto const xrpl_hist_ = \ + xrpl_mr_->meter()->CreateDoubleHistogram((name), (description)); \ + xrpl_hist_->Record(static_cast(value), opentelemetry::context::Context{}); \ + } \ } while (false) // value is fixed; the trailing variadic args carry the label set (see the // note on XRPL_METRIC_COUNTER_INC_LABELED for why labels are variadic). -#define XRPL_METRIC_HISTOGRAM_RECORD_LABELED(app, name, description, value, ...) \ - do \ - { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ - { \ - static opentelemetry::nostd::unique_ptr> \ - xrpl_hist_; \ - static std::once_flag xrpl_once_; \ - std::call_once(xrpl_once_, [&] { \ - if (auto xrpl_m_ = xrpl_mr_->meter()) \ - xrpl_hist_ = xrpl_m_->CreateDoubleHistogram((name), (description)); \ - }); \ - if (xrpl_hist_) \ - xrpl_hist_->Record( \ - static_cast(value), __VA_ARGS__, opentelemetry::context::Context{}); \ - } \ +#define XRPL_METRIC_HISTOGRAM_RECORD_LABELED(app, name, description, value, ...) \ + do \ + { \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + { \ + static auto const xrpl_hist_ = \ + xrpl_mr_->meter()->CreateDoubleHistogram((name), (description)); \ + xrpl_hist_->Record( \ + static_cast(value), __VA_ARGS__, opentelemetry::context::Context{}); \ + } \ } while (false) // Synchronous Gauge: last-value snapshot, not a distribution (contrast @@ -301,41 +254,28 @@ // instrument kind (misusing Histogram or UpDownCounter as a gauge // substitute is explicitly discouraged -- see Design/taxonomy section). #if OPENTELEMETRY_ABI_VERSION_NO >= 2 -#define XRPL_METRIC_GAUGE_RECORD(app, name, description, value) \ - do \ - { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ - { \ - static opentelemetry::nostd::unique_ptr> \ - xrpl_gauge_; \ - static std::once_flag xrpl_once_; \ - std::call_once(xrpl_once_, [&] { \ - if (auto xrpl_m_ = xrpl_mr_->meter()) \ - xrpl_gauge_ = xrpl_m_->CreateDoubleGauge((name), (description)); \ - }); \ - if (xrpl_gauge_) \ - xrpl_gauge_->Record( \ - static_cast(value), opentelemetry::context::Context{}); \ - } \ +#define XRPL_METRIC_GAUGE_RECORD(app, name, description, value) \ + do \ + { \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + { \ + static auto const xrpl_gauge_ = \ + xrpl_mr_->meter()->CreateDoubleGauge((name), (description)); \ + xrpl_gauge_->Record(static_cast(value), opentelemetry::context::Context{}); \ + } \ } while (false) // value is fixed; the trailing variadic args carry the label set (see the // note on XRPL_METRIC_COUNTER_INC_LABELED for why labels are variadic). -#define XRPL_METRIC_GAUGE_RECORD_LABELED(app, name, description, value, ...) \ - do \ - { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ - { \ - static opentelemetry::nostd::unique_ptr> \ - xrpl_gauge_; \ - static std::once_flag xrpl_once_; \ - std::call_once(xrpl_once_, [&] { \ - if (auto xrpl_m_ = xrpl_mr_->meter()) \ - xrpl_gauge_ = xrpl_m_->CreateDoubleGauge((name), (description)); \ - }); \ - if (xrpl_gauge_) \ - xrpl_gauge_->Record( \ - static_cast(value), __VA_ARGS__, opentelemetry::context::Context{}); \ - } \ +#define XRPL_METRIC_GAUGE_RECORD_LABELED(app, name, description, value, ...) \ + do \ + { \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + { \ + static auto const xrpl_gauge_ = \ + xrpl_mr_->meter()->CreateDoubleGauge((name), (description)); \ + xrpl_gauge_->Record( \ + static_cast(value), __VA_ARGS__, opentelemetry::context::Context{}); \ + } \ } while (false) #else #define XRPL_METRIC_GAUGE_RECORD(app, name, description, value) \ @@ -374,88 +314,81 @@ // the registry itself) -- do not "fix" this with a smart pointer that // frees before the reader thread's last collection tick. // ----------------------------------------------------------------- -#define XRPL_METRIC_OBSERVABLE_GAUGE_REGISTER(app, name, description, valueFn) \ - do \ - { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ - { \ - if (auto xrpl_m_ = xrpl_mr_->meter()) \ - { \ - auto* xrpl_fn_ = new std::function(valueFn); \ - auto xrpl_inst_ = xrpl_m_->CreateInt64ObservableGauge((name), (description)); \ - xrpl_inst_->AddCallback( \ - [](opentelemetry::metrics::ObserverResult result, void* state) { \ - auto* fn = static_cast*>(state); \ - try \ - { \ - opentelemetry::nostd::get>>(result) \ - ->Observe((*fn)()); \ - } \ - catch (...) \ - { \ - } \ - }, \ - xrpl_fn_); \ - } \ - } \ - } while (false) - -#define XRPL_METRIC_OBSERVABLE_COUNTER_REGISTER(app, name, description, valueFn) \ - do \ - { \ - if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ - { \ - if (auto xrpl_m_ = xrpl_mr_->meter()) \ - { \ - auto* xrpl_fn_ = new std::function(valueFn); \ - auto xrpl_inst_ = xrpl_m_->CreateInt64ObservableCounter((name), (description)); \ - xrpl_inst_->AddCallback( \ - [](opentelemetry::metrics::ObserverResult result, void* state) { \ - auto* fn = static_cast*>(state); \ - try \ - { \ - opentelemetry::nostd::get>>(result) \ - ->Observe((*fn)()); \ - } \ - catch (...) \ - { \ - } \ - }, \ - xrpl_fn_); \ - } \ - } \ - } while (false) - -#define XRPL_METRIC_OBSERVABLE_UPDOWN_REGISTER(app, name, description, valueFn) \ +#define XRPL_METRIC_OBSERVABLE_GAUGE_REGISTER(app, name, description, valueFn) \ do \ { \ if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ { \ - if (auto xrpl_m_ = xrpl_mr_->meter()) \ - { \ - auto* xrpl_fn_ = new std::function(valueFn); \ - auto xrpl_inst_ = \ - xrpl_m_->CreateInt64ObservableUpDownCounter((name), (description)); \ - xrpl_inst_->AddCallback( \ - [](opentelemetry::metrics::ObserverResult result, void* state) { \ - auto* fn = static_cast*>(state); \ - try \ - { \ - opentelemetry::nostd::get>>(result) \ - ->Observe((*fn)()); \ - } \ - catch (...) \ - { \ - } \ - }, \ - xrpl_fn_); \ - } \ + auto xrpl_m_ = xrpl_mr_->meter(); \ + auto* xrpl_fn_ = new std::function(valueFn); \ + auto xrpl_inst_ = xrpl_m_->CreateInt64ObservableGauge((name), (description)); \ + xrpl_inst_->AddCallback( \ + [](opentelemetry::metrics::ObserverResult result, void* state) { \ + auto* fn = static_cast*>(state); \ + try \ + { \ + opentelemetry::nostd::get>>(result) \ + ->Observe((*fn)()); \ + } \ + catch (...) \ + { \ + } \ + }, \ + xrpl_fn_); \ } \ } while (false) +#define XRPL_METRIC_OBSERVABLE_COUNTER_REGISTER(app, name, description, valueFn) \ + do \ + { \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + { \ + auto xrpl_m_ = xrpl_mr_->meter(); \ + auto* xrpl_fn_ = new std::function(valueFn); \ + auto xrpl_inst_ = xrpl_m_->CreateInt64ObservableCounter((name), (description)); \ + xrpl_inst_->AddCallback( \ + [](opentelemetry::metrics::ObserverResult result, void* state) { \ + auto* fn = static_cast*>(state); \ + try \ + { \ + opentelemetry::nostd::get>>(result) \ + ->Observe((*fn)()); \ + } \ + catch (...) \ + { \ + } \ + }, \ + xrpl_fn_); \ + } \ + } while (false) + +#define XRPL_METRIC_OBSERVABLE_UPDOWN_REGISTER(app, name, description, valueFn) \ + do \ + { \ + if (auto* xrpl_mr_ = (app).getMetricsRegistry(); xrpl_mr_ && xrpl_mr_->isEnabled()) \ + { \ + auto xrpl_m_ = xrpl_mr_->meter(); \ + auto* xrpl_fn_ = new std::function(valueFn); \ + auto xrpl_inst_ = xrpl_m_->CreateInt64ObservableUpDownCounter((name), (description)); \ + xrpl_inst_->AddCallback( \ + [](opentelemetry::metrics::ObserverResult result, void* state) { \ + auto* fn = static_cast*>(state); \ + try \ + { \ + opentelemetry::nostd::get>>(result) \ + ->Observe((*fn)()); \ + } \ + catch (...) \ + { \ + } \ + }, \ + xrpl_fn_); \ + } \ + } while (false) + #else // !XRPL_ENABLE_TELEMETRY #define XRPL_METRIC_COUNTER_INC(app, name, description) \ diff --git a/src/xrpld/telemetry/MetricsRegistry.cpp b/src/xrpld/telemetry/MetricsRegistry.cpp index f335e26925..08c1f596c5 100644 --- a/src/xrpld/telemetry/MetricsRegistry.cpp +++ b/src/xrpld/telemetry/MetricsRegistry.cpp @@ -78,6 +78,7 @@ #include #include #include +#include #include #include #include @@ -154,7 +155,8 @@ addHistogramView( auto selector = metric_sdk::InstrumentSelectorFactory::Create( metric_sdk::InstrumentType::kHistogram, name, ""); - auto meterSelector = metric_sdk::MeterSelectorFactory::Create("xrpld", "1.0.0", ""); + auto meterSelector = metric_sdk::MeterSelectorFactory::Create( + std::string(xrpl::telemetry::kMeterName), std::string(xrpl::telemetry::kMeterVersion), ""); auto view = metric_sdk::ViewFactory::Create(name, "", metric_sdk::AggregationType::kHistogram, config); @@ -188,23 +190,14 @@ namespace xrpl::telemetry { MetricsRegistry::MetricsRegistry( [[maybe_unused]] bool enabled, [[maybe_unused]] ServiceRegistry& app, - [[maybe_unused]] beast::Journal journal) + [[maybe_unused]] beast::Journal journal, + [[maybe_unused]] Options const& options) : enabled_(enabled) #ifdef XRPL_ENABLE_TELEMETRY , app_(app) , journal_(journal) #endif { -} - -MetricsRegistry::~MetricsRegistry() -{ - stop(); -} - -void -MetricsRegistry::start([[maybe_unused]] StartOptions const& options) -{ #ifdef XRPL_ENABLE_TELEMETRY if (!enabled_) return; @@ -218,21 +211,62 @@ MetricsRegistry::start([[maybe_unused]] StartOptions const& options) << ", nodeId=" << options.nodeId << ", networkId=" << options.networkId << ", useTls=" << options.useTls; - // Rule for anything added below: this phase may create only instruments - // whose recording is PUSHED from app code -- counters and histograms. An - // instrument registered here is live immediately, and the reader thread - // may invoke a registered callback before the rest of the Application is - // built, so any observable whose callback reads an Application service - // 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(options); - initSyncInstruments(); + // A broken pipeline must not stop the node. The SDK is third-party code, + // so the catch-all is deliberate, as in ~ApplicationImp. + try + { + initExporterAndProvider(options); + + // Rule for anything added below: the constructor may create only + // instruments whose recording is PUSHED from app code -- counters and + // histograms. An instrument registered here is live immediately, and + // the reader thread may invoke a registered callback before the rest + // of the Application is built, so any observable whose callback reads + // an Application service 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. + initSyncInstruments(); + } + catch (std::exception const& e) + { + disablePipeline(e.what()); + return; + } + catch (...) + { + disablePipeline("unknown exception"); + return; + } JLOG(journal_.info()) << "MetricsRegistry: provider and instruments ready"; #endif // XRPL_ENABLE_TELEMETRY } +#ifdef XRPL_ENABLE_TELEMETRY +void +MetricsRegistry::disablePipeline(std::string_view reason) +{ + provider_.reset(); + // meter_ becomes a no-op meter, which keeps the invariant the + // XRPL_METRIC_* macros rely on: an enabled registry always has a meter, + // so every call site gets an instrument (a no-op one here) with no check + // of its own. Through the base pointer, as Telemetry::getMeter() does: + // the no-op provider's override hides the base class's defaulted overload. + opentelemetry::nostd::shared_ptr const noop( + new opentelemetry::metrics::NoopMeterProvider()); + meter_ = noop->GetMeter(std::string(kMeterName), std::string(kMeterVersion)); + JLOG(journal_.error()) << "MetricsRegistry: metrics pipeline failed to initialise, " + "continuing without native metrics: " + << reason; +} +#endif // XRPL_ENABLE_TELEMETRY + +MetricsRegistry::~MetricsRegistry() +{ + stop(); +} + void MetricsRegistry::startAsyncGauges() { @@ -240,15 +274,28 @@ MetricsRegistry::startAsyncGauges() if (!enabled_) return; - // A mis-ordered call must not crash: without a meter there is nothing to - // create instruments on, so registration is skipped entirely. - if (!meter_) + // One arm per life. A second call would create a second set of + // same-named instruments, and a call after stop() would register on a + // provider that is gone. Checked before the pipeline, so a call after + // stop() is reported as what it is and not as a build failure. + if (phase_ != Phase::Ready) { JLOG(journal_.warn()) << "MetricsRegistry: startAsyncGauges() called " - "before start(); no gauges registered"; + << (phase_ == Phase::Stopped ? "after stop()" : "twice") + << "; ignored"; return; } + // The pipeline failed to build: the meter is a no-op, so registering + // gauges on it would only log a success that is not one. + if (!provider_) + { + JLOG(journal_.warn()) << "MetricsRegistry: startAsyncGauges() without a pipeline; " + "no gauges registered"; + return; + } + phase_ = Phase::GaugesArmed; + registerAsyncGauges(); JLOG(journal_.info()) << "MetricsRegistry: started successfully"; @@ -257,7 +304,7 @@ MetricsRegistry::startAsyncGauges() #ifdef XRPL_ENABLE_TELEMETRY void -MetricsRegistry::initExporterAndProvider(StartOptions const& options) +MetricsRegistry::initExporterAndProvider(Options const& options) { // Configure OTLP/HTTP metric exporter. The TLS settings come from the one // [telemetry] block that also drives the trace exporter in Telemetry.cpp, @@ -357,7 +404,7 @@ MetricsRegistry::initExporterAndProvider(StartOptions const& options) provider_->AddMetricReader(std::move(reader)); // Get a meter for all xrpld instruments. - meter_ = provider_->GetMeter("xrpld", "1.0.0"); + meter_ = provider_->GetMeter(std::string(kMeterName), std::string(kMeterVersion)); } void @@ -415,6 +462,9 @@ void MetricsRegistry::stop() { #ifdef XRPL_ENABLE_TELEMETRY + // Idempotent: the destructor calls this after run() or the Application + // destructor already did. + phase_ = Phase::Stopped; if (!provider_) return; diff --git a/src/xrpld/telemetry/MetricsRegistry.h b/src/xrpld/telemetry/MetricsRegistry.h index abde25eafb..c65c8bb646 100644 --- a/src/xrpld/telemetry/MetricsRegistry.h +++ b/src/xrpld/telemetry/MetricsRegistry.h @@ -94,17 +94,16 @@ * Example usage: * * @code - * // In Application::setup(), after telemetry_ is created. Phase 1 needs - * // only the config strings, so it runs immediately and the meter is live - * // before any metric-emitting code: - * metricsRegistry_ = std::make_unique( - * telemetry_->isEnabled(), app, journal); - * // The endpoint, the TLS settings and the resource identity come from - * // [telemetry] and [network_id], read directly in Application::setup() - * // rather than through Telemetry::Setup. - * metricsRegistry_->start(startOptions); + * // In ApplicationImp's member-init list, right after telemetry_ and before + * // every subsystem. The constructor builds the pipeline and every + * // synchronous instrument, so no producer can exist before they do. The + * // endpoint, the TLS settings and the resource identity come from + * // [telemetry] and [network_id], read by Application.cpp rather than + * // through Telemetry::Setup. + * metricsRegistry_(std::make_unique( + * telemetry_->isEnabled(), *this, journal, options)) * - * // Later in setup(), once overlay_ exists (the last of the services the + * // Later, in setup(), once overlay_ exists (the last of the services the * // callbacks read). Phase 2 registers the observable instruments: * metricsRegistry_->startAsyncGauges(); * @@ -124,13 +123,16 @@ * if (auto* mr = app_.getMetricsRegistry()) * mr->recordJobQueued("ledgerData", "ProcessLData"); * - * // Shutdown: + * // Shutdown, before any service the callbacks read is stopped. Idempotent, + * // so run() and ~ApplicationImp both call it: * metricsRegistry_->stop(); * @endcode * * Caveats: * - The MetricsRegistry must be created AFTER the Telemetry object because - * it reads isEnabled() to decide whether to initialize the OTel SDK. + * it reads isEnabled() to decide whether to initialize the OTel SDK, and + * BEFORE every subsystem that records a metric. Declaration order in + * ApplicationImp is the guarantee; keep the member where it is. * - Observable gauge callbacks capture a reference to the Application; the * Application must outlive the MetricsRegistry (guaranteed because * MetricsRegistry is stopped before Application teardown). @@ -227,15 +229,19 @@ namespace telemetry { * catch-all try block so a transient failure never crashes * the reader thread. * - ValidationTracker protects its rolling windows internally. - * - start(), startAsyncGauges() and stop() are NOT thread-safe + * - The constructor, startAsyncGauges() and stop() are NOT thread-safe * with each other and must all be called, in that order, from * the single Application lifecycle thread. * - * @note Lifetime: - * - Must be constructed AFTER telemetry_ (reads isEnabled()). - * - Must be stopped BEFORE Application services it observes are - * destroyed; the Application owns it via unique_ptr so normal - * teardown guarantees this. + * @note Lifetime, in three phases (see Phase): + * - Ready: the constructor built the pipeline and the synchronous + * instruments. Runs in ApplicationImp's member-init list, so it precedes + * every subsystem that could record. + * - GaugesArmed: startAsyncGauges() registered the observable callbacks. + * Runs once overlay_ exists, the last service those callbacks read. + * - Stopped: stop() joined the reader thread. Runs before any observed + * service stops, from run() and again from ~ApplicationImp for the + * paths that never reach run(). * * @note Extending: * - Adding a new CountedObject type is auto-picked up by the @@ -254,61 +260,42 @@ class MetricsRegistry { public: /** - * Construct a MetricsRegistry. - * - * @param enabled Whether OTel metric export is active. When false, - * all methods become no-ops. - * @param app Reference to the ServiceRegistry (Application) for - * reading current metric values in gauge callbacks. - * @param journal Journal for log output. - */ - MetricsRegistry(bool enabled, ServiceRegistry& app, beast::Journal journal); - - ~MetricsRegistry(); - - /** - * Non-copyable, non-movable. - */ - MetricsRegistry(MetricsRegistry const&) = delete; - MetricsRegistry& - operator=(MetricsRegistry const&) = delete; - - /** - * Everything `start()` needs from config: where to export, how to secure - * the connection, and the process identity stamped on the OTel resource. + * Everything the constructor needs from config: where to export, how to + * secure the connection, and the process identity stamped on the OTel + * resource. * * The values come from the `[telemetry]` section plus `[network_id]`, read - * in `ApplicationImp::startTelemetry()`. They must match what - * `makeTelemetrySetup()` gives the trace pipeline, or one node reports two - * identities and a dashboard filter shows half its series. + * by `makeMetricsRegistryOptions()` in `Application.cpp`. They must match + * what `makeTelemetrySetup()` gives the trace pipeline, or one node reports + * two identities and a dashboard filter shows half its series. * * A struct rather than ten positional parameters: seven of them are * strings, so a swapped pair would compile and silently stamp the wrong * label. Designated initializers name every value at the call site. * * @code - * MetricsRegistry::StartOptions opts{ + * MetricsRegistry::Options opts{ * .endpoint = "http://localhost:4318/v1/metrics", * .serviceName = "xrpld", * .serviceVersion = build_info::getVersionString(), * .serviceInstanceId = nodePublicKey, * .nodeId = nodePublicKey, * .networkId = 2}; - * registry.start(opts); + * MetricsRegistry registry(enabled, app, journal, opts); * * // Edge case: mutual TLS to a collector that requires it. * opts.useTls = true; * opts.tlsCaCertPath = "/etc/xrpld/otel-ca.pem"; * opts.tlsClientCertPath = "/etc/xrpld/node.pem"; * opts.tlsClientKeyPath = "/etc/xrpld/node.key"; - * registry.start(opts); + * MetricsRegistry secure(enabled, app, journal, opts); * @endcode * * @note Plain aggregate, no invariants enforced. `networkType` is not a - * field: it is derived from @ref networkId inside `start()` so the - * two can never disagree. + * field: it is derived from @ref networkId inside the constructor so + * the two can never disagree. */ - struct StartOptions + struct Options { /** * OTLP/HTTP endpoint URL for metric export, from @@ -374,17 +361,17 @@ public: }; /** - * Initialize the OTel metrics pipeline and create the SYNCHRONOUS - * instruments (counters and histograms). + * Construct the registry and, when enabled, build the whole metrics + * pipeline: OTLP exporter, periodic reader, MeterProvider and every + * SYNCHRONOUS instrument (counters and histograms). * - * This is the first of two startup phases, and it can be called as soon - * as the registry is constructed — which is what makes the meter live - * before the first metric-emitting code runs. Startup RPCs and the first - * consensus round both record metrics; a call-site metric macro caches - * its instrument on first use, so a first use before the meter exists - * latches null for the process lifetime. + * Doing this in the constructor is what fixes the init order. The + * Application declares its registry before every subsystem, so no + * producer can exist before the instruments do. A failure to build the + * pipeline is logged and leaves the registry a no-op; it never stops the + * node. * - * @note Invariant for future changes: this phase may create only + * @note Invariant for future changes: the constructor may create only * instruments with NO Application-reading callback. Push-model * counters and histograms qualify; app code records into them * when it is ready. Any observable instrument whose callback @@ -393,34 +380,51 @@ public: * that callback against a half-built Application. This applies * to observable COUNTERS as well as gauges. * + * @param enabled False makes every method a no-op (telemetry disabled). + * @param app Services the observable-gauge callbacks read. + * @param journal Log output. * @param options Endpoint, TLS settings and resource identity, all read - * from config by the caller. See @ref StartOptions. + * from config by the caller. See @ref Options. */ - void - start(StartOptions const& options); + MetricsRegistry( + bool enabled, + ServiceRegistry& app, + beast::Journal journal, + Options const& options); + + /** + * Stops the pipeline if run() or ~ApplicationImp did not already. + */ + ~MetricsRegistry(); + + /** + * Non-copyable, non-movable. + */ + MetricsRegistry(MetricsRegistry const&) = delete; + MetricsRegistry& + operator=(MetricsRegistry const&) = delete; /** * Register the pull-model observable instruments — the second startup * phase. Mostly ObservableGauges, plus the ObservableCounters whose * source value is already cumulative. * - * A separate entry point from `start()` because the two halves have - * different prerequisites. `start()` needs only config strings; these - * callbacks read live Application services, so this half must run later. - * Registering an observable also arms the reader thread to invoke its - * callback on the next tick, which is why the separation is about ordering - * and not just tidiness. + * A separate entry point from the constructor because the two halves have + * different prerequisites. The constructor needs only config strings; + * these callbacks read live Application services, so this half must run + * later. Registering an observable also arms the reader thread to invoke + * its callback on the next tick, which is why the separation is about + * ordering and not just tidiness. + * + * Calling it twice, or after stop(), logs a warning and does nothing. * - * @pre `start()` has already run (the meter exists). If it has not, - * this is a logged no-op rather than a crash. * @pre Every service the callbacks read is constructed. The full set, * from the `app.get*()` calls in the registration helpers, is: * Overlay, OPs (NetworkOPs), LedgerMaster, OpenLedger, TxQ, * NodeStore, NodeFamily, Validators, AcceptedLedgerCache, * CachedSLEs, AcquireStats, TimeKeeper, RelationalDatabase, * InboundLedgers and FeeTrack. - * All but Overlay already exist by the time `start()` is - * callable, so Overlay is what fixes this call's position: + * Overlay is built last, so it fixes this call's position: * `ServiceRegistry::getOverlay()` `XRPL_ASSERT`s that * `overlay_` is non-null, and a reader-thread tick before the * overlay exists aborts a Debug build. The callbacks' catch-all @@ -843,9 +847,15 @@ public: * Access the shared OTel Meter for call-site instrument creation. * Used by the XRPL_METRIC_* macros (MetricMacros.h) so new synchronous * counters/histograms can be declared at their call site instead of as - * MetricsRegistry members. Returns an empty (falsy) shared_ptr before - * start() has run or when disabled. - * @return The shared Meter, or empty if not yet started. + * MetricsRegistry members. + * + * Invariant: never empty while isEnabled() is true. The constructor sets + * it to the real meter, or to a no-op meter when the pipeline failed to + * build, so a call site creates its instrument with no check of its own. + * Empty only when the registry is disabled, which the macros gate on + * first. + * + * @return The shared Meter. */ [[nodiscard]] opentelemetry::nostd::shared_ptr meter() const noexcept @@ -936,6 +946,18 @@ private: */ beast::Journal const journal_; + /** + * Where the registry is in its life. Construction ends in `Ready`; + * startAsyncGauges() moves to `GaugesArmed`; stop() to `Stopped`. A call + * that does not fit the current phase logs a warning and does nothing. + */ + enum class Phase { Ready, GaugesArmed, Stopped }; + + /** + * Current phase; written only from the Application lifecycle thread. + */ + Phase phase_{Phase::Ready}; + /** * Set by detachCallbacks() during shutdown so every ObservableGauge * callback returns early before reading Application services that @@ -1150,29 +1172,39 @@ private: /** * Build the OTLP/HTTP exporter, periodic reader, resource attributes and * histogram views, then create the MeterProvider and meter. Extracted - * from start() to keep each function under the 80-line limit. + * from the constructor to keep each function under the 80-line limit. * * @param options Endpoint, TLS settings and resource identity, forwarded - * unchanged from `start()`. See @ref StartOptions. + * unchanged from the constructor. See @ref Options. */ void - initExporterAndProvider(StartOptions const& options); + initExporterAndProvider(Options const& options); /** * Create the synchronous instruments (RPC and job-queue counters and * histograms, plus the external dashboard parity counters). Extracted - * from start() to keep each function under the 80-line limit. + * from the constructor to keep each function under the 80-line limit. */ void initSyncInstruments(); + /** + * Give up the pipeline after a build failure: drop the provider, hand + * out a no-op meter so every call site still gets an instrument, and log + * why. The registry stays enabled and inert for the process. + * + * @param reason What failed, for the log line. + */ + void + disablePipeline(std::string_view reason); + /** * Register all observable gauge callbacks with the OTel SDK. * Dispatches to one helper per metric domain so that each helper * stays well under the 80-line-per-function limit. * - * Called only from `startAsyncGauges()`, which owns the enabled_ and - * meter_ guards and the Application-state precondition. + * Called only from `startAsyncGauges()`, which owns the enabled_, + * phase_ and provider_ guards and the Application-state precondition. */ void registerAsyncGauges(); From 4aaac039e8518da5ee0ee7d27e59461b83d01964 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 14 Sep 2026 20:21:19 +0100 Subject: [PATCH 09/10] docs(telemetry): give Test 1 its own store instead of the Devnet one Test 1 pointed standalone at docker/telemetry/xrpld-telemetry.cfg, whose [node_db], [database_path] and [debug_logfile] all resolve under docker/telemetry/data. Standalone builds its own private chain, so that left one NuDB holding two unrelated chains. This file already states the rule for the key generation node in Test 2, and the sibling mainnet config keeps its store under data/mainnet/ for the same reason. Derive a standalone config with the three paths redirected under data/standalone/, and run from that. Note why the flag is not the problem: --start selects StartUpType::Fresh, but the default Normal reaches startGenesisLedger() through the same branch chain in ApplicationImp::setup, so any standalone run writes a genesis ledger into whichever store the config names. --- docker/telemetry/TESTING.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/docker/telemetry/TESTING.md b/docker/telemetry/TESTING.md index 8c7ae4c5d6..78c0724bfe 100644 --- a/docker/telemetry/TESTING.md +++ b/docker/telemetry/TESTING.md @@ -68,12 +68,21 @@ curl -sf http://localhost:3200/ready >/dev/null && echo "tempo ready" ### Step 2: Start xrpld in standalone mode +`xrpld-telemetry.cfg` is a Devnet config whose `[node_db]`, `[database_path]` and `[debug_logfile]` all resolve under `docker/telemetry/data`. Standalone builds its own private chain, so pointing it at that store leaves one NuDB holding two unrelated chains. This is the same rule stated for the key-generation node in Test 2, and the reason the sibling mainnet config keeps its store under `data/mainnet/`. Give standalone its own prefix: + ```bash -.build/xrpld --conf docker/telemetry/xrpld-telemetry.cfg -a --start +sed -e 's|^path=docker/telemetry/data/nudb$|path=docker/telemetry/data/standalone/nudb|' \ + -e 's|^docker/telemetry/data$|docker/telemetry/data/standalone|' \ + -e 's|^data/logs/xrpld-devnet/debug.log$|data/logs/xrpld-standalone/debug.log|' \ + docker/telemetry/xrpld-telemetry.cfg >/tmp/xrpld-standalone.cfg + +.build/xrpld --conf /tmp/xrpld-standalone.cfg -a --start ``` Wait a few seconds for the node to initialize. +> Separating the store is required whether or not `--start` is passed. `--start` selects `StartUpType::Fresh`, but the default `Normal` reaches `startGenesisLedger()` through the same branch chain in `ApplicationImp::setup`, so every standalone run writes a genesis ledger into whichever store the config names. Dropping the flag does not avoid it; only a separate path does. `--start` additionally seeds the amendments this build desires into that genesis ledger. + ### Step 3: Exercise RPC spans ```bash From bec9e1c8a9ab4a8a4a74a603dfb0684fe40f9959 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 14 Sep 2026 20:34:31 +0100 Subject: [PATCH 10/10] fix(telemetry): resolve the node identity before the Application is built resolveNodePublicKey() returned std::nullopt in three real cases: a first boot with no wallet database, a standalone run (its wallet is a private temporary database), and --newnodeid. Telemetry's resources are built during ApplicationImp's member-init list and are immutable, so on those runs the node reported an empty service.instance.id and no xrpl.node.id for the whole run, while setup() minted a key moments later and patched only the tracer. Replace it with resolveNodeIdentity(), which always returns a keypair: derived from a configured seed, else read from an existing wallet database, else minted. Main.cpp passes that pair to makeApplication(), ApplicationImp stores it in nodeIdentity_ -- now declared before telemetry_ and no longer an optional, because it is always set -- and builds the telemetry resource from it. setup() calls getNodeIdentity(), which now persists rather than mints: it stores the resolved pair when the wallet holds no identity, adopts the stored one when it does, and clears first for --newnodeid. The write stays in setup() because that is where the database exists; a standalone run has no persistent wallet to write to, which is why the pair has to be decided before construction rather than read back afterwards. Wallet gains storeNodeIdentity() for that write, and getNodeIdentity(session) now uses it instead of repeating the insert. The three-argument makeApplication() mints a keypair, so jtx::Env and any other test Application behave as a standalone run always did. Also fold the three hand-rolled "meter from a NoopMeterProvider" copies into telemetry::noopMeter(): the base-pointer call and the kMeterVersion argument are both easy to get wrong alone, and the meter identity has to match the one the histogram views select on. The new gtest covers the wallet half: store-then-read, store not replacing an existing identity, clear-then-store, and that the mint path persists. It adds the tests.libxrpl > xrpl.rdb levelization edge, regenerated here. --- .../scripts/levelization/results/ordering.txt | 1 + .../05-configuration-reference.md | 16 +- include/xrpl/server/Wallet.h | 16 ++ include/xrpl/telemetry/Telemetry.h | 19 ++ src/libxrpl/server/Wallet.cpp | 22 ++- src/libxrpl/telemetry/Telemetry.cpp | 17 +- src/tests/libxrpl/server/NodeIdentity.cpp | 158 ++++++++++++++++ .../libxrpl/telemetry/SpanGuardScope.cpp | 5 +- src/xrpld/app/main/Application.cpp | 43 +++-- src/xrpld/app/main/Application.h | 13 +- src/xrpld/app/main/Main.cpp | 23 +-- src/xrpld/app/main/NodeIdentity.cpp | 172 +++++++++++------- src/xrpld/app/main/NodeIdentity.h | 51 ++++-- 13 files changed, 412 insertions(+), 144 deletions(-) create mode 100644 src/tests/libxrpl/server/NodeIdentity.cpp diff --git a/.github/scripts/levelization/results/ordering.txt b/.github/scripts/levelization/results/ordering.txt index 51db8661c8..49eb71d8e0 100644 --- a/.github/scripts/levelization/results/ordering.txt +++ b/.github/scripts/levelization/results/ordering.txt @@ -196,6 +196,7 @@ tests.libxrpl > xrpl.nodestore tests.libxrpl > xrpl.peerfinder tests.libxrpl > xrpl.protocol tests.libxrpl > xrpl.protocol_autogen +tests.libxrpl > xrpl.rdb tests.libxrpl > xrpl.resource tests.libxrpl > xrpl.server tests.libxrpl > xrpl.shamap diff --git a/OpenTelemetryPlan/05-configuration-reference.md b/OpenTelemetryPlan/05-configuration-reference.md index 057ab34a3e..ddd05cfdf0 100644 --- a/OpenTelemetryPlan/05-configuration-reference.md +++ b/OpenTelemetryPlan/05-configuration-reference.md @@ -62,13 +62,17 @@ The parser `makeTelemetrySetup()` in `src/libxrpl/telemetry/TelemetryConfig.cpp` ### 5.3.1 ApplicationImp Changes -> **Deferred identity**: The node public key (`nodeIdentity_`) is not -> available during `ApplicationImp`'s member initializer list — it is -> resolved later in `setup()`. The `Telemetry` object is therefore -> constructed with an empty `serviceInstanceId` and patched via -> `setServiceInstanceId()` once `setup()` has called `getNodeIdentity()`. +> **Identity before construction**: telemetry stamps the node public key into +> resources that are immutable once built, and it builds them during +> `ApplicationImp`'s member initializer list. So `Main.cpp` calls +> `resolveNodeIdentity()` first, from the config and command line alone, and +> passes the keypair to `makeApplication()`. It never comes back empty: a +> configured `[node_seed]` decides it, else the wallet database supplies it if +> one already exists, else it is minted. `ApplicationImp::setup()` then calls +> `getNodeIdentity()`, which stores that keypair when the wallet holds none and +> otherwise adopts what the wallet holds. -`ApplicationImp` (in `src/xrpld/app/main/Application.cpp`) owns a `std::unique_ptr telemetry_`. It is built in the member initializer list via `makeTelemetry(makeTelemetrySetup(...))` with an empty `serviceInstanceId`, then patched in `setup()` by calling `setServiceInstanceId()` with the Base58 node public key (unless the user supplied a custom `service_instance_id`). `start()` and `run()` forward to `telemetry_->start()` / `telemetry_->stop()`, and `getTelemetry()` returns the owned instance. +`ApplicationImp` (in `src/xrpld/app/main/Application.cpp`) owns a `std::pair nodeIdentity_`, declared before `std::unique_ptr telemetry_` so the resource can be built from it. `telemetry_` is built in the member initializer list via `makeTelemetry(makeTelemetrySetup(...))` with that key as `serviceInstanceId` (unless the user supplied a custom `service_instance_id`). `setup()` still calls `setServiceInstanceId()`, which now matters only where the stored key differs from the resolved one, and reaches the tracer resource alone. `start()` and `run()` forward to `telemetry_->start()` / `telemetry_->stop()`, and `getTelemetry()` returns the owned instance. ### 5.3.2 ServiceRegistry Interface Addition diff --git a/include/xrpl/server/Wallet.h b/include/xrpl/server/Wallet.h index af6c92b83d..e549a1305e 100644 --- a/include/xrpl/server/Wallet.h +++ b/include/xrpl/server/Wallet.h @@ -102,6 +102,22 @@ clearNodeIdentity(soci::session& session); std::optional> readNodeIdentity(soci::session& session); +/** + * Persist a keypair as this node's identity. + * + * Write-only counterpart of readNodeIdentity(). The caller must have found the + * table empty: this inserts a row without clearing, so storing twice leaves two + * and readNodeIdentity() then returns whichever the query yields first. + * + * Exists because xrpld resolves its identity before the Application, and so + * before any database, is built; setup() persists that keypair here. + * + * @param session Session with the database. + * @param keys The keypair to store. + */ +void +storeNodeIdentity(soci::session& session, std::pair const& keys); + /** * Returns a stable public and private key for this node. * diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index a0f56387b0..335b2ae7b8 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -133,6 +133,25 @@ inline constexpr std::string_view kMeterName{"xrpld"}; * OTel instrumentation scope version reported for the meter. */ inline constexpr std::string_view kMeterVersion{"1.0.0"}; + +/** + * A meter whose instruments record nothing. + * + * For every path that must hand out a usable meter without a pipeline behind + * it: telemetry disabled, or an exporter that failed to build. Callers then + * need no null check, because an instrument always comes back. + * + * Two details are easy to get wrong alone, which is why this is shared: the + * provider must be reached through a base `MeterProvider` pointer, because + * `NoopMeterProvider`'s override hides the base class's defaulted overload; + * and the version must be @ref kMeterVersion, or the meter identity differs + * from the one the histogram views select on. + * + * @param name Instrumentation scope name to report. + * @return An inert meter. Never empty. + */ +[[nodiscard]] opentelemetry::nostd::shared_ptr +noopMeter(std::string_view name = kMeterName); #endif /** diff --git a/src/libxrpl/server/Wallet.cpp b/src/libxrpl/server/Wallet.cpp index 92317d40f6..ec9f1fd3b5 100644 --- a/src/libxrpl/server/Wallet.cpp +++ b/src/libxrpl/server/Wallet.cpp @@ -171,6 +171,16 @@ readNodeIdentity(soci::session& session) return std::nullopt; } +void +storeNodeIdentity(soci::session& session, std::pair const& keys) +{ + session << std::format( + "INSERT INTO NodeIdentity (PublicKey,PrivateKey) " + "VALUES ('{}','{}');", + toBase58(TokenType::NodePublic, keys.first), + toBase58(TokenType::NodePrivate, keys.second)); +} + std::pair getNodeIdentity(soci::session& session) { @@ -178,15 +188,9 @@ getNodeIdentity(soci::session& session) return *stored; // If a valid identity wasn't found, we randomly generate a new one: - auto [newpublicKey, newsecretKey] = randomKeyPair(KeyType::Secp256k1); - - session << std::format( - "INSERT INTO NodeIdentity (PublicKey,PrivateKey) " - "VALUES ('{}','{}');", - toBase58(TokenType::NodePublic, newpublicKey), - toBase58(TokenType::NodePrivate, newsecretKey)); - - return {newpublicKey, newsecretKey}; + auto const keys = randomKeyPair(KeyType::Secp256k1); + storeNodeIdentity(session, keys); + return keys; } std::unordered_set, KeyEqual> diff --git a/src/libxrpl/telemetry/Telemetry.cpp b/src/libxrpl/telemetry/Telemetry.cpp index ad9bc1479e..6ff5993f93 100644 --- a/src/libxrpl/telemetry/Telemetry.cpp +++ b/src/libxrpl/telemetry/Telemetry.cpp @@ -261,11 +261,8 @@ public: [[nodiscard]] opentelemetry::nostd::shared_ptr getMeter(std::string_view name) override { - // Serve a meter from a process-wide noop provider, mirroring the - // noop tracer above. Instruments created from it are inert. - static auto noopProvider = opentelemetry::nostd::shared_ptr( - new metrics_api::NoopMeterProvider()); - return noopProvider->GetMeter(std::string(name), std::string(kMeterVersion)); + // Mirrors the noop tracer above: instruments created from it are inert. + return noopMeter(name); } [[nodiscard]] opentelemetry::nostd::shared_ptr @@ -703,6 +700,16 @@ public: } // namespace +opentelemetry::nostd::shared_ptr +noopMeter(std::string_view name) +{ + // One provider for the process: it holds a single inert meter, so nothing + // is gained by building another. + static auto const provider = opentelemetry::nostd::shared_ptr( + new metrics_api::NoopMeterProvider()); + return provider->GetMeter(std::string(name), std::string(kMeterVersion)); +} + opentelemetry::exporter::otlp::OtlpHttpExporterOptions makeTraceExporterOptions(Telemetry::Setup const& setup) { diff --git a/src/tests/libxrpl/server/NodeIdentity.cpp b/src/tests/libxrpl/server/NodeIdentity.cpp new file mode 100644 index 0000000000..22dc9d2948 --- /dev/null +++ b/src/tests/libxrpl/server/NodeIdentity.cpp @@ -0,0 +1,158 @@ +/** + * @file NodeIdentity.cpp + * GTest unit tests for the wallet database's node-identity storage. + * + * Three functions share one table, `NodeIdentity`, and the split between them + * is what the telemetry startup order depends on: `readNodeIdentity()` only + * reads, `storeNodeIdentity()` only writes, and `getNodeIdentity()` reads then + * writes a fresh key when the table is empty. `xrpld` resolves its identity + * before the Application exists and persists it later, so the store step has + * to be callable on its own and has to be idempotent-by-read: a second run + * must return the first run's key, not a new one. + * + * Each test gets its own database file in a temporary directory, so nothing + * here depends on order or on the developer's data directory. + */ + +#include +#include +#include +#include +#include +#include +#include + +#include + +#include +#include +#include +#include + +using namespace xrpl; + +namespace { + +/** + * A wallet database in its own temporary directory, removed on destruction. + * + * `makeTestWalletDB()` creates the schema, so every fixture starts with an + * empty `NodeIdentity` table. + */ +class TempWalletDb +{ +public: + explicit TempWalletDb(std::string const& name) + : dir_(std::filesystem::temp_directory_path() / ("xrpl-node-identity-" + name)) + { + std::filesystem::remove_all(dir_); + std::filesystem::create_directories(dir_); + + DatabaseCon::Setup setup; + setup.dataDir = dir_; + db_ = makeTestWalletDB(setup, "wallet.db", beast::Journal{beast::Journal::getNullSink()}); + } + + ~TempWalletDb() + { + db_.reset(); + std::error_code ec; + std::filesystem::remove_all(dir_, ec); + } + + TempWalletDb(TempWalletDb const&) = delete; + TempWalletDb& + operator=(TempWalletDb const&) = delete; + + [[nodiscard]] DatabaseCon& + operator*() const noexcept + { + return *db_; + } + +private: + std::filesystem::path dir_; + std::unique_ptr db_; +}; + +} // namespace + +TEST(WalletNodeIdentity, store_then_read_returns_the_same_pair) +{ + // The store step exists so a key minted before the Application is built + // can be persisted afterwards. Reading it back must give the same pair, or + // the two halves of one run report two identities. + TempWalletDb wallet("store-then-read"); + auto const minted = randomKeyPair(KeyType::Secp256k1); + + { + auto db = (*wallet).checkoutDb(); + ASSERT_FALSE(readNodeIdentity(*db).has_value()) << "a fresh wallet must hold no identity"; + storeNodeIdentity(*db, minted); + } + + auto db = (*wallet).checkoutDb(); + auto const stored = readNodeIdentity(*db); + ASSERT_TRUE(stored.has_value()); + EXPECT_EQ(stored->first, minted.first); + EXPECT_EQ(stored->second, minted.second); +} + +TEST(WalletNodeIdentity, store_does_not_replace_an_existing_identity) +{ + // getNodeIdentity() is the read-or-mint path and must keep the first key, + // so a restart does not change the node's identity on the network. The + // stored pair wins over anything a later caller offers. + TempWalletDb wallet("no-replace"); + auto db = (*wallet).checkoutDb(); + + auto const first = getNodeIdentity(*db); + auto const other = randomKeyPair(KeyType::Secp256k1); + ASSERT_NE(first.first, other.first) + << "the two pairs must differ for this test to mean anything"; + + storeNodeIdentity(*db, other); + + auto const stored = readNodeIdentity(*db); + ASSERT_TRUE(stored.has_value()); + EXPECT_EQ(stored->first, first.first); + EXPECT_EQ(getNodeIdentity(*db).first, first.first); +} + +TEST(WalletNodeIdentity, clear_then_store_installs_the_new_pair) +{ + // --newnodeid clears the row and then persists the freshly minted pair. + // Both steps are needed: clearing alone would leave the node with no + // stored identity at all. + TempWalletDb wallet("clear-then-store"); + auto db = (*wallet).checkoutDb(); + + auto const first = getNodeIdentity(*db); + auto const replacement = randomKeyPair(KeyType::Secp256k1); + ASSERT_NE(first.first, replacement.first); + + clearNodeIdentity(*db); + EXPECT_FALSE(readNodeIdentity(*db).has_value()) << "clear must leave the table empty"; + + storeNodeIdentity(*db, replacement); + auto const stored = readNodeIdentity(*db); + ASSERT_TRUE(stored.has_value()); + EXPECT_EQ(stored->first, replacement.first); + EXPECT_EQ(stored->second, replacement.second); +} + +TEST(WalletNodeIdentity, get_mints_and_persists_when_the_table_is_empty) +{ + // The mint path must persist, not just return: a second call has to give + // the same key. This is the property --newnodeid relies on to be + // meaningful, and the one a caller that only reads would break. + TempWalletDb wallet("mint-and-persist"); + auto db = (*wallet).checkoutDb(); + + auto const minted = getNodeIdentity(*db); + + auto const stored = readNodeIdentity(*db); + ASSERT_TRUE(stored.has_value()) << "getNodeIdentity() must persist what it mints"; + EXPECT_EQ(stored->first, minted.first); + EXPECT_EQ(getNodeIdentity(*db).first, minted.first); +} diff --git a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp index 78a5a04983..b46bbcc90f 100644 --- a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp +++ b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp @@ -193,10 +193,7 @@ public: opentelemetry::nostd::shared_ptr getMeter(std::string_view name) override { - static auto noopProvider = - opentelemetry::nostd::shared_ptr( - new opentelemetry::metrics::NoopMeterProvider()); - return noopProvider->GetMeter(std::string(name), std::string(kMeterVersion)); + return noopMeter(name); } opentelemetry::nostd::shared_ptr diff --git a/src/xrpld/app/main/Application.cpp b/src/xrpld/app/main/Application.cpp index 412a4b4cd5..13afb18c1d 100644 --- a/src/xrpld/app/main/Application.cpp +++ b/src/xrpld/app/main/Application.cpp @@ -80,9 +80,11 @@ #include #include #include // IWYU pragma: keep +#include #include #include #include +#include #include #include // IWYU pragma: keep #include @@ -220,6 +222,13 @@ public: beast::Journal journal_; std::unique_ptr perfLog_; + /** + * This node's keypair, resolved before construction by + * resolveNodeIdentity() and persisted by setup(). Declared before + * telemetry_ because that builds resource attributes from it, and they are + * immutable once built. + */ + std::pair nodeIdentity_; std::unique_ptr telemetry_; Application::MutexType masterMutex_; @@ -236,7 +245,6 @@ public: NodeCache tempNodeCache_; CachedSLEs cachedSLEs_; std::unique_ptr networkIDService_; - std::optional> nodeIdentity_; ValidatorKeys const validatorKeys_; std::unique_ptr resourceManager_; @@ -317,7 +325,7 @@ public: std::unique_ptr config, std::unique_ptr logs, std::unique_ptr timeKeeper, - std::optional const& nodePublicKey) + std::pair const& resolvedIdentity) : BasicApp(numberOfThreads(*config)) , config_(std::move(config)) , logs_(std::move(logs)) @@ -331,15 +339,16 @@ public: *this, logs_->journal("PerfLog"), [this] { signalStop("PerfLog"); })) + , nodeIdentity_(resolvedIdentity) // Telemetry publishes the MeterProvider on construction, so it must // precede collectorManager_ below and every subsystem that creates an // instrument. Its resource is immutable, so the instance id has to be - // supplied now; empty means this run reports none. + // supplied now, from the identity resolved above. , telemetry_( telemetry::makeTelemetry( telemetry::makeTelemetrySetup( config_->section("telemetry"), - nodePublicKey.value_or(""), + toBase58(TokenType::NodePublic, nodeIdentity_.first), build_info::getVersionString(), config_->networkId), logs_->journal("Telemetry"))) @@ -619,10 +628,7 @@ public: std::pair const& nodeIdentity() override { - if (nodeIdentity_) - return *nodeIdentity_; - - logicError("Accessing Application::nodeIdentity() before it is initialized."); + return nodeIdentity_; } std::optional @@ -1306,12 +1312,15 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline) return false; } - nodeIdentity_ = getNodeIdentity(*this, cmdline); + // Persist the identity resolved before construction, or adopt the one the + // wallet already holds. Telemetry is already reporting the resolved key. + nodeIdentity_ = getNodeIdentity(*this, cmdline, nodeIdentity_); // The metrics resource was fixed at construction, but the tracer resource is - // built by start() below, so a key minted just now can still reach spans. + // built by start() below, so the stored key still reaches spans if it + // differs from the resolved one. if (!config_->section("telemetry").exists("service_instance_id")) - telemetry_->setServiceInstanceId(toBase58(TokenType::NodePublic, nodeIdentity_->first)); + telemetry_->setServiceInstanceId(toBase58(TokenType::NodePublic, nodeIdentity_.first)); // Start telemetry here, not in start(). Spans are emitted during the rest // of setup() — the first consensus round in beginConsensus() below — and @@ -2298,7 +2307,13 @@ makeApplication( std::unique_ptr logs, std::unique_ptr timeKeeper) { - return makeApplication(std::move(config), std::move(logs), std::move(timeKeeper), std::nullopt); + // No identity supplied, so mint one. setup() stores it if the wallet holds + // none, which is what a standalone run and a test Application do anyway. + return makeApplication( + std::move(config), + std::move(logs), + std::move(timeKeeper), + randomKeyPair(KeyType::Secp256k1)); } std::unique_ptr @@ -2306,10 +2321,10 @@ makeApplication( std::unique_ptr config, std::unique_ptr logs, std::unique_ptr timeKeeper, - std::optional const& nodePublicKey) + std::pair const& nodeIdentity) { return std::make_unique( - std::move(config), std::move(logs), std::move(timeKeeper), nodePublicKey); + std::move(config), std::move(logs), std::move(timeKeeper), nodeIdentity); } void diff --git a/src/xrpld/app/main/Application.h b/src/xrpld/app/main/Application.h index 1d7125cd64..8791a6ec8f 100644 --- a/src/xrpld/app/main/Application.h +++ b/src/xrpld/app/main/Application.h @@ -175,18 +175,21 @@ makeApplication( std::unique_ptr timeKeeper); /** - * Construct the application with a known node public key. + * Construct the application with a known node identity. * * Telemetry builds its resource attributes during construction and they are - * immutable, so the base58 node public key must be supplied here. Pass - * std::nullopt when it is unknown; that run reports no instance id. See - * resolveNodePublicKey(). + * immutable, so the node keypair must be supplied here. See + * resolveNodeIdentity(), which decides it from the config and command line + * alone; setup() then persists it. + * + * The three-argument overload above mints a keypair, which is what a test + * Application and a standalone run get anyway. */ std::unique_ptr makeApplication( std::unique_ptr config, std::unique_ptr logs, std::unique_ptr timeKeeper, - std::optional const& nodePublicKey); + std::pair const& resolvedIdentity); } // namespace xrpl diff --git a/src/xrpld/app/main/Main.cpp b/src/xrpld/app/main/Main.cpp index fcae528737..d72fbf5c77 100644 --- a/src/xrpld/app/main/Main.cpp +++ b/src/xrpld/app/main/Main.cpp @@ -807,13 +807,14 @@ run(int argc, char** argv) if (vm.contains("debug")) setDebugLogSink(logs->makeSink("Debug", beast::Severity::Trace)); - // Telemetry needs the node public key at construction, so read it here - // where a config error can still be reported and the process can exit - // cleanly. getNodeIdentity() in setup() stays authoritative. - std::optional nodePublicKey; + // Telemetry stamps the node public key into resources it builds during + // construction, so the identity is decided here, where a malformed + // [node_seed] can still be reported and the process can exit cleanly. + // setup() persists it; see getNodeIdentity(). + std::optional> nodeIdentity; try { - nodePublicKey = resolveNodePublicKey(*config, vm, logs->journal("Application")); + nodeIdentity = resolveNodeIdentity(*config, vm, logs->journal("Application")); } catch (std::exception const& e) { @@ -821,14 +822,6 @@ run(int argc, char** argv) return -1; } - if (!nodePublicKey) - { - JLOG(logs->journal("Application").warn()) - << "Telemetry: no node identity available yet, so this run reports an empty " - "service.instance.id. Set [telemetry] service_instance_id, or restart once " - "the node key exists."; - } - // Application construction runs member initializers that validate // config (for example the [telemetry] section) and can throw. A throw // from a member-initializer list cannot be recovered inside the @@ -840,14 +833,14 @@ run(int argc, char** argv) // // Only the construction is covered. The [telemetry] section is parsed // near the top of the member list, before the job queue and node store - // are built, so unwinding that throw destroys very little. setup() is + // are built, so unwinding that throw destroys little. setup() is // left outside deliberately: it starts subsystems whose shutdown order // is delicate, and only the normal stop sequence gets that order right. std::unique_ptr app; try { app = makeApplication( - std::move(config), std::move(logs), std::make_unique(), nodePublicKey); + std::move(config), std::move(logs), std::make_unique(), *nodeIdentity); } catch (std::exception const& e) { diff --git a/src/xrpld/app/main/NodeIdentity.cpp b/src/xrpld/app/main/NodeIdentity.cpp index 3860bbe3c6..bb62c99cf4 100644 --- a/src/xrpld/app/main/NodeIdentity.cpp +++ b/src/xrpld/app/main/NodeIdentity.cpp @@ -30,104 +30,88 @@ namespace xrpl { -std::pair -getNodeIdentity(Application& app, boost::program_options::variables_map const& cmdline) -{ - std::optional seed; +namespace { +/** + * The seed a configured `[node_seed]` or `--nodeid` names. + * + * @param config The server configuration. + * @param cmdline The command line parameters passed into the application. + * @return The seed, or std::nullopt when neither is configured. + * @throws std::runtime_error if the configured value is malformed. + */ +std::optional +configuredSeed(Config const& config, boost::program_options::variables_map const& cmdline) +{ if (cmdline.contains("nodeid")) { - seed = parseGenericSeed(cmdline["nodeid"].as(), false); - + auto seed = parseGenericSeed(cmdline["nodeid"].as(), false); if (!seed) Throw("Invalid 'nodeid' in command line"); + return seed; } - else if (app.config().exists(Sections::kNodeSeed)) - { - seed = parseBase58(app.config().section(Sections::kNodeSeed).lines().front()); + if (config.exists(Sections::kNodeSeed)) + { + auto const& lines = config.section(Sections::kNodeSeed).lines(); + auto seed = lines.empty() ? std::nullopt : parseBase58(lines.front()); if (!seed) { Throw( std::string("Invalid [") + Sections::kNodeSeed + "] in configuration file"); } + return seed; } - if (seed) - { - auto secretKey = generateSecretKey(KeyType::Secp256k1, *seed); - auto publicKey = derivePublicKey(KeyType::Secp256k1, secretKey); - - return {publicKey, secretKey}; - } - - auto db = app.getWalletDB().checkoutDb(); - - if (cmdline.contains("newnodeid")) - clearNodeIdentity(*db); - - return getNodeIdentity(*db); + return std::nullopt; } -std::optional -resolveNodePublicKey( - Config const& config, - boost::program_options::variables_map const& cmdline, - beast::Journal journal) +/** + * The keypair a seed defines. + * + * @param seed The configured seed. + * @return The derived secp256k1 keypair. + */ +std::pair +keysFromSeed(Seed const& seed) { - std::optional seed; - bool seedConfigured = false; - - if (cmdline.contains("nodeid")) - { - seedConfigured = true; - seed = parseGenericSeed(cmdline["nodeid"].as(), false); - } - else if (config.exists(Sections::kNodeSeed)) - { - seedConfigured = true; - if (auto const& lines = config.section(Sections::kNodeSeed).lines(); !lines.empty()) - seed = parseBase58(lines.front()); - } - - // A configured seed decides the identity outright. A malformed or missing - // one is reported by getNodeIdentity(), which runs later. - if (seedConfigured) - { - if (!seed) - return std::nullopt; - - auto const secretKey = generateSecretKey(KeyType::Secp256k1, *seed); - return toBase58(TokenType::NodePublic, derivePublicKey(KeyType::Secp256k1, secretKey)); - } - - // --newnodeid discards whatever is stored. - if (cmdline.contains("newnodeid")) - return std::nullopt; + auto const secretKey = generateSecretKey(KeyType::Secp256k1, seed); + return {derivePublicKey(KeyType::Secp256k1, secretKey), secretKey}; +} +/** + * The stored identity, read without creating or modifying anything. + * + * Runs before the Application, so it opens the wallet itself rather than going + * through getWalletDB(). Three things keep that safe: the file must already + * exist, the init SQL is empty so the schema is never created, and the global + * pragmas are off because they include journal_mode, which rewrites the + * database header. The connection closes before this returns. + * + * @param config The server configuration. + * @param journal Journal for reporting an unreadable database. + * @return The stored keypair, or std::nullopt when there is none to read. + */ +std::optional> +storedIdentity(Config const& config, beast::Journal journal) +{ try { auto setup = setupDatabaseCon(config, journal); - // Standalone uses a temporary database, so nothing is persisted and this - // run will mint a fresh key. + // Standalone gets a private temporary database, so there is nothing + // persisted to read and nothing setup() could read back either. if (setup.standAlone && setup.startUp != StartUpType::Load && setup.startUp != StartUpType::LoadFile && setup.startUp != StartUpType::Replay) { return std::nullopt; } - // The global pragmas include journal_mode, which rewrites the database - // header. The wallet is opened without them everywhere else. setup.useGlobalPragma = false; - // Only read an existing file: SQLite would otherwise create one. if (std::error_code ec; !std::filesystem::exists(setup.dataDir / kWalletDbName, ec)) - { return std::nullopt; - } - // Empty init SQL: open the existing schema, never create it. DatabaseCon walletDb{ setup, kWalletDbName, @@ -136,8 +120,7 @@ resolveNodePublicKey( journal}; auto db = walletDb.checkoutDb(); - if (auto const stored = readNodeIdentity(*db)) - return toBase58(TokenType::NodePublic, stored->first); + return readNodeIdentity(*db); } catch (std::exception const& e) { @@ -147,4 +130,59 @@ resolveNodePublicKey( return std::nullopt; } +} // namespace + +std::pair +resolveNodeIdentity( + Config const& config, + boost::program_options::variables_map const& cmdline, + beast::Journal journal) +{ + // A configured seed decides the identity outright, and nothing is stored. + if (auto const seed = configuredSeed(config, cmdline)) + return keysFromSeed(*seed); + + // --newnodeid discards whatever is stored, so mint now; getNodeIdentity() + // clears the old row and stores this pair. + if (!cmdline.contains("newnodeid")) + { + if (auto const stored = storedIdentity(config, journal)) + return *stored; + } + + // Nothing to read: a first boot, or a standalone run's temporary database. + // Mint here so telemetry has an identity from construction; setup() + // persists this pair if there is a database to hold it. + return randomKeyPair(KeyType::Secp256k1); +} + +std::pair +getNodeIdentity( + Application& app, + boost::program_options::variables_map const& cmdline, + std::pair const& resolved) +{ + // A configured seed reaches neither the reader nor the writer. + if (cmdline.contains("nodeid") || app.config().exists(Sections::kNodeSeed)) + return resolved; + + auto db = app.getWalletDB().checkoutDb(); + + if (cmdline.contains("newnodeid")) + clearNodeIdentity(*db); + + // What is stored wins, so a restart keeps the node's identity even if + // another process wrote one between construction and here. Telemetry's + // resources are already built from `resolved`, so on that one run the two + // would disagree; it needs a restart to line up, as the configuration + // reference records. + if (auto const stored = readNodeIdentity(*db)) + return *stored; + + // Nothing stored, or --newnodeid just cleared it. Persist the pair + // telemetry is already reporting, so both agree from now on. + storeNodeIdentity(*db, resolved); + return resolved; +} + } // namespace xrpl diff --git a/src/xrpld/app/main/NodeIdentity.h b/src/xrpld/app/main/NodeIdentity.h index 7309f6007a..837d07184b 100644 --- a/src/xrpld/app/main/NodeIdentity.h +++ b/src/xrpld/app/main/NodeIdentity.h @@ -16,34 +16,47 @@ namespace xrpl { /** - * The cryptographic credentials identifying this server instance. + * This server's identity, resolved before the Application exists. * - * @param app The application object - * @param cmdline The command line parameters passed into the application. - */ -std::pair -getNodeIdentity(Application& app, boost::program_options::variables_map const& cmdline); - -/** - * This server's public key, read without creating or modifying anything. + * Telemetry stamps the node public key into resource attributes that are + * immutable once built, and those resources are built in ApplicationImp's + * member-init list. So the identity has to be decided before construction, + * from the config and the command line alone. * - * For callers that need the identity before the Application exists, such as - * telemetry building its resource attributes in the member-init list. Derives - * from a configured seed when there is one, otherwise reads the wallet database - * only if it already exists. - * - * getNodeIdentity() remains authoritative and mints a key when none exists. + * Always returns a keypair. It derives one from a configured seed, else reads + * the wallet database if it already exists, else mints one. Nothing is created + * or written here: getNodeIdentity() persists the result once setup() has + * opened the database. * * @param config The server configuration. * @param cmdline The command line parameters passed into the application. * @param journal Journal for reporting an unreadable database. - * @return The base58-encoded node public key, or std::nullopt if none can be - * read. + * @return This node's keypair. + * @throws std::runtime_error if a configured seed is malformed. */ -std::optional -resolveNodePublicKey( +std::pair +resolveNodeIdentity( Config const& config, boost::program_options::variables_map const& cmdline, beast::Journal journal); +/** + * The cryptographic credentials identifying this server instance, persisted. + * + * Called from setup(), once the wallet database is open. Stores @p resolved + * when the database holds no identity, and returns whatever the database holds + * when it does. + * + * @param app The application object + * @param cmdline The command line parameters passed into the application. + * @param resolved The keypair resolveNodeIdentity() decided before + * construction, which telemetry is already reporting. + * @return This node's keypair. + */ +std::pair +getNodeIdentity( + Application& app, + boost::program_options::variables_map const& cmdline, + std::pair const& resolved); + } // namespace xrpl