From ac71480a626aa759598cc8d3991c14b6cb959110 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 17 Aug 2026 19:36:41 +0100 Subject: [PATCH 1/3] fix(telemetry): count mode transitions with tiling buckets, not overlapping ones The transitions panel used increase(...[$__rate_interval]). $__rate_interval is defined as max($__interval + scrape, 4 * scrape), i.e. deliberately one scrape longer than the step so rate() windows overlap and lose no counter increase. That overlap is harmless for rate(), but this panel reads the value as a count of discrete events, and the overlap counts each event in more than one bucket. Measured against a log-derived ground truth of 106 syncing transitions on devnet-otel-usw2-01 over 2026-08-11T11:05Z..2026-08-12T23:04Z, the old query reported 111.3 at a 300s step and 133.7 at a 60s step -- the error grew to +26% as you zoomed in, because the overlap is a larger fraction of a smaller step. Switch to $__interval so the buckets tile exactly, and wrap in round() because increase() extrapolates to the window edges and so reports fractional counts for an integer counter. The same measurement now gives 106 at 300s, 105 at 60s and 107 at 900s. Every state and both nodes land within a few counts of truth at any zoom, and the legend Total is now a meaningful figure. Pin Min step to 1m: the real scrape interval is 60s while the datasource declares 15s, so without a floor $__interval can fall below one sample. Draw as bars with 0 decimals -- the value is a discrete count per bucket, and a line implies interpolation between counts that does not exist. --- .../grafana/dashboards/node-health.json | 31 ++++++++++------ docs/telemetry-runbook.md | 36 +++++++++---------- 2 files changed, 38 insertions(+), 29 deletions(-) diff --git a/docker/telemetry/grafana/dashboards/node-health.json b/docker/telemetry/grafana/dashboards/node-health.json index 35e525d27f..b5231ae7d5 100644 --- a/docker/telemetry/grafana/dashboards/node-health.json +++ b/docker/telemetry/grafana/dashboards/node-health.json @@ -274,7 +274,7 @@ }, { "title": "Operating Mode Transitions", - "description": "**What:** Transitions into each operating mode, per interval.\n**How it's computed:** increase() over the per-mode transition counters, so each point is the number of transitions in that bucket and the series stays correct across an xrpld restart (the counters reset to 0).\n**Reading it:** Few transitions is good; a stable node rarely leaves Full. Brief flaps are visible here even when they are too short to appear on Operating Mode (State Timeline), which can only sample state once per scrape.\n**Healthy range:** workload-dependent; low and infrequent transitions.\n**Watch for:** Frequent transitions out of Full, or any into Disconnected/Syncing (flapping).\n**Source:** src/xrpld/app/misc/NetworkOPs.cpp NetworkOPsImp::Stats ctor", + "description": "**What:** Transitions into each operating mode, per interval.\n**How it's computed:** round(increase(...[$__interval])) over the per-mode transition counters. $__interval tiles the buckets exactly, so each bar is the transitions in that bucket and the legend Total is the true count; $__rate_interval would overlap each bucket by one scrape and inflate it (measured +5% at a 36h range, +26% zoomed in). round() removes increase()'s extrapolation, which otherwise reports fractional counts. The series stays correct across an xrpld restart (the counters reset to 0).\n**Reading it:** Few transitions is good; a stable node rarely leaves Full. Brief flaps are visible here even when they are too short to appear on Operating Mode (State Timeline), which can only sample state once per scrape.\n**Healthy range:** workload-dependent; low and infrequent transitions.\n**Watch for:** Frequent transitions out of Full, or any into Disconnected/Syncing (flapping).\n**Source:** src/xrpld/app/misc/NetworkOPs.cpp NetworkOPsImp::Stats ctor", "type": "timeseries", "gridPos": { "h": 10, @@ -287,6 +287,12 @@ "maxHeight": 600, "mode": "multi", "sort": "desc" + }, + "legend": { + "calcs": ["sum", "max"], + "displayMode": "table", + "placement": "bottom", + "showLegend": true } }, "targets": [ @@ -294,35 +300,35 @@ "datasource": { "type": "prometheus" }, - "expr": "increase(state_accounting_full_transitions{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[$__rate_interval])", + "expr": "round(increase(state_accounting_full_transitions{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[$__interval]))", "legendFormat": "Full [{{service_instance_id}}]" }, { "datasource": { "type": "prometheus" }, - "expr": "increase(state_accounting_tracking_transitions{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[$__rate_interval])", + "expr": "round(increase(state_accounting_tracking_transitions{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[$__interval]))", "legendFormat": "Tracking [{{service_instance_id}}]" }, { "datasource": { "type": "prometheus" }, - "expr": "increase(state_accounting_syncing_transitions{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[$__rate_interval])", + "expr": "round(increase(state_accounting_syncing_transitions{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[$__interval]))", "legendFormat": "Syncing [{{service_instance_id}}]" }, { "datasource": { "type": "prometheus" }, - "expr": "increase(state_accounting_connected_transitions{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[$__rate_interval])", + "expr": "round(increase(state_accounting_connected_transitions{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[$__interval]))", "legendFormat": "Connected [{{service_instance_id}}]" }, { "datasource": { "type": "prometheus" }, - "expr": "increase(state_accounting_disconnected_transitions{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[$__rate_interval])", + "expr": "round(increase(state_accounting_disconnected_transitions{service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\", xrpl_network_type=~\"$xrpl_network_type\", service_name=~\"$service_name\"}[$__interval]))", "legendFormat": "Disconnected [{{service_instance_id}}]" } ], @@ -335,14 +341,17 @@ "insertNulls": false, "showPoints": "auto", "pointSize": 5, - "lineWidth": 1, - "fillOpacity": 0, - "gradientMode": "none" - } + "lineWidth": 0, + "fillOpacity": 70, + "gradientMode": "none", + "drawStyle": "bars" + }, + "decimals": 0 }, "overrides": [] }, - "id": 6 + "id": 6, + "interval": "1m" }, { "title": "I/O Latency", diff --git a/docs/telemetry-runbook.md b/docs/telemetry-runbook.md index e915e8cfd7..93f67fabbd 100644 --- a/docs/telemetry-runbook.md +++ b/docs/telemetry-runbook.md @@ -723,24 +723,24 @@ Requires `trace_peer=1` in the `[telemetry]` config section. ### Node Health -- System Metrics (`node-health`) -| Panel | Type | PromQL | Labels Used | -| -------------------------------------- | ---------- | --------------------------------------------------------------------------------- | ----------- | -| Validated Ledger Age | stat | `ledgermaster_validated_ledger_age` | — | -| Published Ledger Age | stat | `ledgermaster_published_ledger_age` | — | -| Operating Mode (Time Share) | timeseries | `rate(state_accounting_X_duration) / sum(rate(all modes))` | — | -| Operating Mode Transitions | timeseries | `increase(state_accounting_*_transitions[$__rate_interval])` | — | -| I/O Latency | timeseries | `histogram_quantile(0.95, ios_latency_bucket)` | — | -| Job Queue Depth | timeseries | `jobq_job_count` | — | -| Ledger Fetch Rate | stat | `rate(ledger_fetches_total[$__rate_interval])` | — | -| Ledger History Mismatches | stat | `rate(ledger_history_mismatch_total[$__rate_interval])` | — | -| Key Jobs Execution Time | timeseries | `acceptledger{quantile="$quantile"}` (+ 10 more key jobs) | `quantile` | -| Key Jobs Dequeue Wait Time | timeseries | `acceptledger_q{quantile="$quantile"}` (+ 10 more) | `quantile` | -| FullBelowCache Size | timeseries | `node_family_full_below_cache_size` | — | -| FullBelowCache Hit Rate | gauge | `node_family_full_below_cache_hit_rate` | — | -| Ledger Publish Gap | stat | `Published_Ledger_Age - Validated_Ledger_Age` | — | -| State Duration Rate (Full vs Tracking) | timeseries | `rate(state_accounting_full_duration[5m]) / 1000000` | — | -| All Jobs Execution Time (Detail) | timeseries | `histogram_quantile($quantile, rate(job_running_us_bucket[5m])) by job_type` — µs | `quantile` | -| All Jobs Dequeue Wait (Detail) | timeseries | `histogram_quantile($quantile, rate(job_queued_us_bucket[5m])) by job_type` — µs | `quantile` | +| Panel | Type | PromQL | Labels Used | +| -------------------------------------- | ---------- | ---------------------------------------------------------------------------------- | ----------- | +| Validated Ledger Age | stat | `ledgermaster_validated_ledger_age` | — | +| Published Ledger Age | stat | `ledgermaster_published_ledger_age` | — | +| Operating Mode (Time Share) | timeseries | `rate(state_accounting_X_duration) / sum(rate(all modes))` | — | +| Operating Mode Transitions | timeseries | `round(increase(state_accounting_*_transitions[$__interval]))` (bars, Min step 1m) | — | +| I/O Latency | timeseries | `histogram_quantile(0.95, ios_latency_bucket)` | — | +| Job Queue Depth | timeseries | `jobq_job_count` | — | +| Ledger Fetch Rate | stat | `rate(ledger_fetches_total[$__rate_interval])` | — | +| Ledger History Mismatches | stat | `rate(ledger_history_mismatch_total[$__rate_interval])` | — | +| Key Jobs Execution Time | timeseries | `acceptledger{quantile="$quantile"}` (+ 10 more key jobs) | `quantile` | +| Key Jobs Dequeue Wait Time | timeseries | `acceptledger_q{quantile="$quantile"}` (+ 10 more) | `quantile` | +| FullBelowCache Size | timeseries | `node_family_full_below_cache_size` | — | +| FullBelowCache Hit Rate | gauge | `node_family_full_below_cache_hit_rate` | — | +| Ledger Publish Gap | stat | `Published_Ledger_Age - Validated_Ledger_Age` | — | +| State Duration Rate (Full vs Tracking) | timeseries | `rate(state_accounting_full_duration[5m]) / 1000000` | — | +| All Jobs Execution Time (Detail) | timeseries | `histogram_quantile($quantile, rate(job_running_us_bucket[5m])) by job_type` — µs | `quantile` | +| All Jobs Dequeue Wait (Detail) | timeseries | `histogram_quantile($quantile, rate(job_queued_us_bucket[5m])) by job_type` — µs | `quantile` | ### Network Traffic -- System Metrics (`network-traffic`) From 4a361a496daf64af65825d5d1cafe9764733d381 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 19 Aug 2026 15:44:32 +0100 Subject: [PATCH 2/3] fix(telemetry): carry the per-node id on spans, not only the resource Consensus spans share one deterministic, ledger-derived trace_id, so a single trace holds spans from every node and the resource-level node id is not a reliable per-span discriminator in stored traces. Add transform/spanidentity to both collector configs, copying service.instance.id onto every span as service_instance_id so TraceQL can filter per node with the same value the $node dashboard variable already uses on the metrics side. Wired into the traces pipeline locally and into traces/store (after tail_sampling) on the Grafana Cloud variant. --- .../otel-collector-config.grafanacloud.yaml | 22 ++++++++++++++++++- docker/telemetry/otel-collector-config.yaml | 17 +++++++++++++- docs/telemetry-runbook.md | 10 +++++++++ 3 files changed, 47 insertions(+), 2 deletions(-) diff --git a/docker/telemetry/otel-collector-config.grafanacloud.yaml b/docker/telemetry/otel-collector-config.grafanacloud.yaml index 5fe6957c72..2a717213b0 100644 --- a/docker/telemetry/otel-collector-config.grafanacloud.yaml +++ b/docker/telemetry/otel-collector-config.grafanacloud.yaml @@ -123,6 +123,15 @@ processors: - set(attributes["service_instance_id"], resource.attributes["service.instance.id"]) - set(attributes["deployment_environment"], resource.attributes["deployment.environment"]) - set(attributes["xrpl_network_type"], resource.attributes["xrpl.network.type"]) + # Copy the per-node id from the resource onto every span so TraceQL can + # filter by node (span.service_instance_id). The name matches the metric + # label written by transform/cloudlabels, so the dashboards' $node variable + # applies to both signals. + transform/spanidentity: + trace_statements: + - context: span + statements: + - set(attributes["service_instance_id"], resource.attributes["service.instance.id"]) connectors: spanmetrics: @@ -256,9 +265,20 @@ service: exporters: [spanmetrics] # Trace-STORAGE branch: 0.5% probabilistic tail sampling before Tempo # and Grafana Cloud, so stored trace volume is ~1/200 of ingested spans. + # transform/spanidentity runs after tail_sampling so only retained spans + # pay for the copy. traces/metrics does not need it: spanmetrics already + # groups by the service.instance.id resource attribute + # (resource_metrics_key_attributes). traces/store: receivers: [otlp] - processors: [tail_sampling, resource/tier, resource/stripsdk, batch] + processors: + [ + tail_sampling, + resource/tier, + resource/stripsdk, + transform/spanidentity, + batch, + ] exporters: [otlp/tempo, otlphttp/grafanacloud] # The local Prometheus scrape promotes tier/instance resource attrs to # labels via resource_to_telemetry_conversion; Grafana Cloud (OTLP) does diff --git a/docker/telemetry/otel-collector-config.yaml b/docker/telemetry/otel-collector-config.yaml index f595c43855..b5cd059fe5 100644 --- a/docker/telemetry/otel-collector-config.yaml +++ b/docker/telemetry/otel-collector-config.yaml @@ -124,6 +124,14 @@ processors: action: hash - key: pathfind_dest_account action: hash + # Copy the per-node id from the resource onto every span so TraceQL can + # filter by node (span.service_instance_id), matching the service_instance_id + # metric label the dashboards' $node variable already uses. + transform/spanidentity: + trace_statements: + - context: span + statements: + - set(attributes["service_instance_id"], resource.attributes["service.instance.id"]) connectors: spanmetrics: @@ -241,7 +249,14 @@ service: pipelines: traces: receivers: [otlp] - processors: [resource/tier, resource/stripsdk, attributes/hash, batch] + processors: + [ + resource/tier, + resource/stripsdk, + attributes/hash, + transform/spanidentity, + batch, + ] exporters: [debug, otlp/tempo, spanmetrics] metrics: receivers: [otlp, spanmetrics] diff --git a/docs/telemetry-runbook.md b/docs/telemetry-runbook.md index f08b526732..484d439576 100644 --- a/docs/telemetry-runbook.md +++ b/docs/telemetry-runbook.md @@ -144,6 +144,16 @@ curl -s http://localhost:5015 -d '{"method":"server_info"}' | | `tls_client_cert` | (empty) | Client cert (PEM) for mutual TLS; empty = one-way TLS | | `tls_client_key` | (empty) | Private key (PEM) for `tls_client_cert` | +> **`service_instance_id` reaches traces as a span attribute too.** xrpld sends +> it as the `service.instance.id` resource attribute; the collector's +> `transform/spanidentity` processor copies it onto every span as +> `service_instance_id`, so TraceQL can filter per node +> (`{span.service_instance_id="validator-0"}`) with the same value the `$node` +> dashboard variable uses for metrics. Spans recorded before that processor was +> added do not carry it, and a TraceQL regex does **not** match a missing +> attribute — so a `$node` filter on a trace panel returns nothing for older +> data. + > **`consensus_trace_strategy` is not validated.** The parser copies the raw > string through (`TelemetryConfig.cpp:155-156`) and the only equality test in > the code is `strategy == "attribute"` (`RCLConsensus.cpp:1296`). Any other From 070d29b465bfa6f1d61a89bf3072478e93b311c6 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Wed, 19 Aug 2026 19:50:40 +0100 Subject: [PATCH 3/3] feat(telemetry): add the xrpl.node.id resource attribute Node identity reached the OTel resource only as service.instance.id, which is config-overridable and carries a deployment-chosen label rather than the node's own identity. Add xrpl.node.id, set unconditionally from the node public key (base58, TokenType::NodePublic), so traces and metrics share a stable per-node key independent of [telemetry] service_instance_id. Set on the tracer resource via Telemetry::setNodeId(), called from ApplicationImp::setup() once nodeIdentity_ is known, and on the MetricsRegistry resource via an added start() parameter. The beast::insight meter provider is built in TelemetryImpl's constructor, before the wallet DB exists, so its resource cannot carry the value; that path is left for later and the attribute is omitted rather than stamped blank. Also drops the transform/spanidentity collector processor added in 4a361a496d: per-node identity belongs on the resource, not copied onto every span. --- .../otel-collector-config.grafanacloud.yaml | 22 +-- docker/telemetry/otel-collector-config.yaml | 17 +- docs/telemetry-runbook.md | 14 +- include/xrpl/telemetry/SpanNames.h | 11 ++ include/xrpl/telemetry/Telemetry.h | 29 +++- src/libxrpl/telemetry/Telemetry.cpp | 24 +-- .../libxrpl/telemetry/NodeIdResource.cpp | 162 ++++++++++++++++++ .../libxrpl/telemetry/TelemetryConfig.cpp | 1 + src/xrpld/app/main/Application.cpp | 13 +- src/xrpld/telemetry/MetricsRegistry.cpp | 21 ++- src/xrpld/telemetry/MetricsRegistry.h | 14 +- 11 files changed, 262 insertions(+), 66 deletions(-) create mode 100644 src/tests/libxrpl/telemetry/NodeIdResource.cpp diff --git a/docker/telemetry/otel-collector-config.grafanacloud.yaml b/docker/telemetry/otel-collector-config.grafanacloud.yaml index 2a717213b0..5fe6957c72 100644 --- a/docker/telemetry/otel-collector-config.grafanacloud.yaml +++ b/docker/telemetry/otel-collector-config.grafanacloud.yaml @@ -123,15 +123,6 @@ processors: - set(attributes["service_instance_id"], resource.attributes["service.instance.id"]) - set(attributes["deployment_environment"], resource.attributes["deployment.environment"]) - set(attributes["xrpl_network_type"], resource.attributes["xrpl.network.type"]) - # Copy the per-node id from the resource onto every span so TraceQL can - # filter by node (span.service_instance_id). The name matches the metric - # label written by transform/cloudlabels, so the dashboards' $node variable - # applies to both signals. - transform/spanidentity: - trace_statements: - - context: span - statements: - - set(attributes["service_instance_id"], resource.attributes["service.instance.id"]) connectors: spanmetrics: @@ -265,20 +256,9 @@ service: exporters: [spanmetrics] # Trace-STORAGE branch: 0.5% probabilistic tail sampling before Tempo # and Grafana Cloud, so stored trace volume is ~1/200 of ingested spans. - # transform/spanidentity runs after tail_sampling so only retained spans - # pay for the copy. traces/metrics does not need it: spanmetrics already - # groups by the service.instance.id resource attribute - # (resource_metrics_key_attributes). traces/store: receivers: [otlp] - processors: - [ - tail_sampling, - resource/tier, - resource/stripsdk, - transform/spanidentity, - batch, - ] + processors: [tail_sampling, resource/tier, resource/stripsdk, batch] exporters: [otlp/tempo, otlphttp/grafanacloud] # The local Prometheus scrape promotes tier/instance resource attrs to # labels via resource_to_telemetry_conversion; Grafana Cloud (OTLP) does diff --git a/docker/telemetry/otel-collector-config.yaml b/docker/telemetry/otel-collector-config.yaml index b5cd059fe5..f595c43855 100644 --- a/docker/telemetry/otel-collector-config.yaml +++ b/docker/telemetry/otel-collector-config.yaml @@ -124,14 +124,6 @@ processors: action: hash - key: pathfind_dest_account action: hash - # Copy the per-node id from the resource onto every span so TraceQL can - # filter by node (span.service_instance_id), matching the service_instance_id - # metric label the dashboards' $node variable already uses. - transform/spanidentity: - trace_statements: - - context: span - statements: - - set(attributes["service_instance_id"], resource.attributes["service.instance.id"]) connectors: spanmetrics: @@ -249,14 +241,7 @@ service: pipelines: traces: receivers: [otlp] - processors: - [ - resource/tier, - resource/stripsdk, - attributes/hash, - transform/spanidentity, - batch, - ] + processors: [resource/tier, resource/stripsdk, attributes/hash, batch] exporters: [debug, otlp/tempo, spanmetrics] metrics: receivers: [otlp, spanmetrics] diff --git a/docs/telemetry-runbook.md b/docs/telemetry-runbook.md index 484d439576..e8fc6c97a2 100644 --- a/docs/telemetry-runbook.md +++ b/docs/telemetry-runbook.md @@ -144,15 +144,11 @@ curl -s http://localhost:5015 -d '{"method":"server_info"}' | | `tls_client_cert` | (empty) | Client cert (PEM) for mutual TLS; empty = one-way TLS | | `tls_client_key` | (empty) | Private key (PEM) for `tls_client_cert` | -> **`service_instance_id` reaches traces as a span attribute too.** xrpld sends -> it as the `service.instance.id` resource attribute; the collector's -> `transform/spanidentity` processor copies it onto every span as -> `service_instance_id`, so TraceQL can filter per node -> (`{span.service_instance_id="validator-0"}`) with the same value the `$node` -> dashboard variable uses for metrics. Spans recorded before that processor was -> added do not carry it, and a TraceQL regex does **not** match a missing -> attribute — so a `$node` filter on a trace panel returns nothing for older -> data. +> **Traces and metrics also carry `xrpl.node.id`.** xrpld sets it as a resource +> attribute alongside `service.instance.id`; the value is the node public key +> (base58, begins with `n`). It comes from the node identity unconditionally, so +> it is present even when `[telemetry] service_instance_id` is configured. +> TraceQL filters on it as `resource.xrpl.node.id`. > **`consensus_trace_strategy` is not validated.** The parser copies the raw > string through (`TelemetryConfig.cpp:155-156`) and the only equality test in diff --git a/include/xrpl/telemetry/SpanNames.h b/include/xrpl/telemetry/SpanNames.h index b848f96c03..d9f2fb0f0b 100644 --- a/include/xrpl/telemetry/SpanNames.h +++ b/include/xrpl/telemetry/SpanNames.h @@ -108,6 +108,7 @@ inline constexpr auto consensus = makeStr("consensus"); inline constexpr auto peer = makeStr("peer"); inline constexpr auto ledger = makeStr("ledger"); inline constexpr auto network = makeStr("network"); +inline constexpr auto node = makeStr("node"); inline constexpr auto link = makeStr("link"); } // namespace seg @@ -117,6 +118,16 @@ namespace attr { inline constexpr auto networkId = join(join(seg::xrpl, seg::network), makeStr("id")); inline constexpr auto networkType = join(join(seg::xrpl, seg::network), makeStr("type")); +/** + * Resource attribute `xrpl.node.id` — the node's base58 public key. + * + * Dotted form, like its siblings above, because it is a process-identity + * value stamped once on the OTel resource rather than a per-span attribute. + * It gives traces and metrics a stable per-node key alongside + * `service.instance.id`. + */ +inline constexpr auto nodeId = join(join(seg::xrpl, seg::node), makeStr("id")); + /** * Canonical shared attrs (rule 5 — _ underscore form). * diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index b9ca1dca64..3f27767770 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -83,7 +83,8 @@ * * @note Thread safety: The Telemetry interface is safe for concurrent reads * (isEnabled, shouldTrace*, getTracer, startSpan) after start() completes. - * setServiceInstanceId() must be called before start() and is not thread-safe. + * setServiceInstanceId() and setNodeId() must be called before start() and + * are not thread-safe. * The OTel SDK's TracerProvider and Tracer are internally thread-safe. */ @@ -193,6 +194,14 @@ public: */ std::string serviceInstanceId; + /** + * OTel resource attribute `xrpl.node.id`: the node's base58-encoded + * public key. Always the node identity, never config-supplied, so it + * stays a stable per-node key even when serviceInstanceId is + * overridden by [telemetry] service_instance_id. + */ + std::string nodeId; + /** * OTLP/HTTP endpoint URL where spans are sent. */ @@ -313,6 +322,24 @@ public: (void)id; } + /** + * Update the node ID (OTel resource attribute `xrpl.node.id`). + * + * Must be called before start(). A setter is needed for the same reason + * setServiceInstanceId() needs one: the node public key is not available + * when Telemetry is constructed (during the ApplicationImp member + * initializer list), so Application::setup() injects it once + * nodeIdentity_ is known. + * + * @param id The node's base58-encoded public key. + */ + virtual void + setNodeId(std::string const& id) + { + // Default no-op for NullTelemetry implementations. + (void)id; + } + /** * Initialize the tracing pipeline (exporter, processor, provider). * Call after construction. diff --git a/src/libxrpl/telemetry/Telemetry.cpp b/src/libxrpl/telemetry/Telemetry.cpp index 1d3eb13c7f..4a8210cbe1 100644 --- a/src/libxrpl/telemetry/Telemetry.cpp +++ b/src/libxrpl/telemetry/Telemetry.cpp @@ -283,8 +283,8 @@ class TelemetryImpl : public Telemetry { /** * Configuration from the [telemetry] config section. - * Non-const so setServiceInstanceId() can update the instance ID - * before start() creates the OTel resource. + * Non-const so setServiceInstanceId() and setNodeId() can update the + * identity attributes before start() creates the OTel resource. */ Setup setup_; @@ -343,6 +343,12 @@ public: setup_.serviceInstanceId = id; } + void + setNodeId(std::string const& id) override + { + setup_.nodeId = id; + } + void start() override { @@ -384,6 +390,7 @@ public: {std::string(attr::networkId), static_cast(setup_.networkId)}, // LCOV_EXCL_LINE {std::string(attr::networkType), setup_.networkType}, // LCOV_EXCL_LINE + {std::string(attr::nodeId), setup_.nodeId}, // LCOV_EXCL_LINE }); // Configure sampler. Head sampling is fixed at 1.0 (sample everything); @@ -442,6 +449,8 @@ public: * during ApplicationImp's member-init list. The metrics resource uses * setup_.serviceInstanceId from config; it is immutable once the provider * is built, so a later node-key setServiceInstanceId() does not affect it. + * The same applies to setNodeId(): xrpl.node.id reaches this resource only + * if setup_.nodeId is already populated when the constructor runs. */ void initMetrics() @@ -479,16 +488,7 @@ public: auto reader = metrics_sdk::PeriodicExportingMetricReaderFactory::Create( std::move(metricExporter), readerOpts); - // Metrics resource: same attributes as the tracer resource so metrics - // and traces share one identity. Built here (not shared with start()) - // because start() runs later; serviceInstanceId comes from config. - auto resourceAttrs = resource::Resource::Create({ - {opentelemetry::semconv::service::kServiceName, setup_.serviceName}, - {opentelemetry::semconv::service::kServiceVersion, setup_.serviceVersion}, - {opentelemetry::semconv::service::kServiceInstanceId, setup_.serviceInstanceId}, - {std::string(attr::networkId), static_cast(setup_.networkId)}, - {std::string(attr::networkType), setup_.networkType}, - }); + auto resourceAttrs = makeMetricsResource(); // Create MeterProvider with the shared resource, then attach reader. meterProvider_ = metrics_sdk::MeterProviderFactory::Create( diff --git a/src/tests/libxrpl/telemetry/NodeIdResource.cpp b/src/tests/libxrpl/telemetry/NodeIdResource.cpp new file mode 100644 index 0000000000..c0f4387b7e --- /dev/null +++ b/src/tests/libxrpl/telemetry/NodeIdResource.cpp @@ -0,0 +1,162 @@ +#include +#include +#include +#include + +#include + +#include +#include + +#ifdef XRPL_ENABLE_TELEMETRY +#include +#include +#endif + +/** + * Contract tests for the `xrpl.node.id` resource attribute. + * + * `xrpl.node.id` carries the node's base58 public key on both the trace and + * the metric OTel resource, so traces and metrics resolve to one node. The + * key string is a cross-component contract: the collector, TraceQL queries + * and Grafana dashboards all name it literally, and a silent rename would + * break them with no compile error. These tests pin the literal key, the + * Setup default, and the fact that the value can only arrive through + * Telemetry::setNodeId(). + * + * Scope limit: the two production resources are built inside TelemetryImpl + * (trace resource in start(), metric resource in the constructor) and inside + * MetricsRegistry::initExporterAndProvider(). Neither is reachable from this + * binary — TelemetryImpl only exists behind an OTLP/HTTP exporter with + * background export threads, which a unit test must not spin up (see + * GetMeter.cpp), and MetricsRegistry.cpp is not compiled into xrpl_tests in + * the telemetry-enabled build. The resource test below therefore pins the SDK + * contract those three call sites rely on: the exact key, and a std::string + * value landing in the string alternative of the attribute variant rather + * than the bool one. + */ + +using namespace xrpl; +using namespace xrpl::telemetry; + +TEST(NodeIdResource, attribute_key_is_dotted_resource_form) +{ + // The literal the collector, TraceQL and the dashboards all name. + EXPECT_EQ(std::string_view(attr::nodeId), "xrpl.node.id"); + + // Dotted, not the underscore form used for span attributes. + EXPECT_EQ(std::string_view(attr::nodeId).find('_'), std::string_view::npos); + + // Sibling of the other two xrpl.* resource attributes, and distinct + // from both. + EXPECT_EQ(std::string_view(attr::networkId), "xrpl.network.id"); + EXPECT_EQ(std::string_view(attr::networkType), "xrpl.network.type"); + EXPECT_NE(std::string_view(attr::nodeId), std::string_view(attr::networkId)); + EXPECT_NE(std::string_view(attr::nodeId), std::string_view(attr::networkType)); + + // Built from the shared segments, so the segment additions are exercised + // too rather than only the joined result. + EXPECT_EQ(std::string_view(seg::node), "node"); + EXPECT_EQ(std::string_view(seg::xrpl), "xrpl"); +} + +TEST(NodeIdResource, setup_node_id_defaults_to_empty) +{ + // Negative path: nothing has called setNodeId(), so there is no value to + // stamp and the resource builders skip the attribute. + Telemetry::Setup const s; + EXPECT_TRUE(s.nodeId.empty()); + EXPECT_EQ(s.nodeId, ""); +} + +TEST(NodeIdResource, config_parsing_never_populates_node_id) +{ + // nodeId is deliberately not config-driven. Even with an explicit + // service_instance_id and a node public key argument, makeTelemetrySetup() + // must leave nodeId empty: Application::setup() is the only writer, via + // setNodeId(). + Section section; + section.set("enabled", "1"); + section.set("service_instance_id", "custom-id"); + + auto const setup = makeTelemetrySetup(section, "nHUtest123", "2.0.0", 1); + + EXPECT_EQ(setup.serviceInstanceId, "custom-id"); + EXPECT_TRUE(setup.nodeId.empty()); +} + +TEST(NodeIdResource, set_node_id_on_disabled_path_is_inert) +{ + // The disabled build/config path takes the base-class no-op. Calling it + // must be safe and must not change any observable state. + Telemetry::Setup setup; + setup.enabled = false; + + beast::Journal::Sink& sink = beast::Journal::getNullSink(); + beast::Journal const journal(sink); + auto telemetry = makeTelemetry(setup, journal); + ASSERT_NE(telemetry, nullptr); + + telemetry->setNodeId("nHUtest123"); + + EXPECT_FALSE(telemetry->isEnabled()); + EXPECT_FALSE(telemetry->shouldTraceRpc()); + EXPECT_FALSE(telemetry->shouldTraceTransactions()); + EXPECT_FALSE(telemetry->shouldTraceConsensus()); + EXPECT_FALSE(telemetry->shouldTracePeer()); + EXPECT_FALSE(telemetry->shouldTraceLedger()); + EXPECT_EQ(telemetry->getConsensusTraceStrategy(), "deterministic"); +} + +#ifdef XRPL_ENABLE_TELEMETRY + +TEST(NodeIdResource, resource_carries_node_id_as_a_string) +{ + namespace otel_resource = opentelemetry::sdk::resource; + + // A base58 node public key: 'n' prefix, 52 characters. + std::string const nodeId = "n9MozjnGB3tpULewtTsVtuudg5JqYFyV3QFdAtVLzJaxHcBaxuXM"; + ASSERT_EQ(nodeId.size(), 52u); + + otel_resource::ResourceAttributes attrs; + // std::string, never a string literal: the attribute variant's + // char-const* overload binds to bool, which would record `true`. + attrs[std::string(attr::nodeId)] = nodeId; + + auto const resource = otel_resource::Resource::Create(attrs); + auto const& out = resource.GetAttributes(); + + auto const it = out.find("xrpl.node.id"); + ASSERT_NE(it, out.end()); + + // The string alternative, not bool — the pitfall the call sites guard. + ASSERT_TRUE(opentelemetry::nostd::holds_alternative(it->second)); + EXPECT_FALSE(opentelemetry::nostd::holds_alternative(it->second)); + EXPECT_EQ(opentelemetry::nostd::get(it->second), nodeId); +} + +TEST(NodeIdResource, resource_omits_node_id_when_it_was_never_set) +{ + namespace otel_resource = opentelemetry::sdk::resource; + + // Negative path: the call sites only assign when the value is non-empty, + // so an unset node ID leaves the key off the resource entirely rather + // than stamping a blank one. + Telemetry::Setup const setup; + ASSERT_TRUE(setup.nodeId.empty()); + + otel_resource::ResourceAttributes attrs; + if (!setup.nodeId.empty()) + attrs[std::string(attr::nodeId)] = setup.nodeId; + + auto const resource = otel_resource::Resource::Create(attrs); + auto const& out = resource.GetAttributes(); + + EXPECT_EQ(out.find("xrpl.node.id"), out.end()); + + // The SDK still merges in its own defaults, so the absence above is a + // real absence and not an empty map. + EXPECT_FALSE(out.empty()); +} + +#endif // XRPL_ENABLE_TELEMETRY diff --git a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp index bfa0b85149..65e2513348 100644 --- a/src/tests/libxrpl/telemetry/TelemetryConfig.cpp +++ b/src/tests/libxrpl/telemetry/TelemetryConfig.cpp @@ -15,6 +15,7 @@ TEST(TelemetryConfig, setup_defaults) EXPECT_EQ(s.serviceName, "xrpld"); EXPECT_TRUE(s.serviceVersion.empty()); EXPECT_TRUE(s.serviceInstanceId.empty()); + EXPECT_TRUE(s.nodeId.empty()); EXPECT_EQ(s.exporterEndpoint, "http://localhost:4318/v1/traces"); EXPECT_FALSE(s.useTls); EXPECT_TRUE(s.tlsCertPath.empty()); diff --git a/src/xrpld/app/main/Application.cpp b/src/xrpld/app/main/Application.cpp index 736db61d6f..c06fb84c79 100644 --- a/src/xrpld/app/main/Application.cpp +++ b/src/xrpld/app/main/Application.cpp @@ -1322,6 +1322,11 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline) if (!config_->section("telemetry").exists("service_instance_id")) telemetry_->setServiceInstanceId(toBase58(TokenType::NodePublic, nodeIdentity_->first)); + // xrpl.node.id always carries the node public key. Unlike + // service_instance_id it is not configurable, so traces and metrics keep a + // stable per-node key whatever [telemetry] says. + telemetry_->setNodeId(toBase58(TokenType::NodePublic, nodeIdentity_->first)); + // Create the OTel MetricsRegistry for gap-fill metrics (counters, // histograms, observable gauges). It must exist before startTelemetry(), // which starts the metrics half of the pipeline. @@ -1676,7 +1681,13 @@ ApplicationImp::startTelemetry() const if (instanceId.empty() && nodeIdentity_) instanceId = toBase58(TokenType::NodePublic, nodeIdentity_->first); - metricsRegistry_->start(endpoint, instanceId); + // The node public key also goes on its own resource attribute, + // xrpl.node.id, which config cannot override. + std::string nodeId; + if (nodeIdentity_) + nodeId = toBase58(TokenType::NodePublic, nodeIdentity_->first); + + metricsRegistry_->start(endpoint, instanceId, nodeId); } } diff --git a/src/xrpld/telemetry/MetricsRegistry.cpp b/src/xrpld/telemetry/MetricsRegistry.cpp index c07817e41e..69aa120705 100644 --- a/src/xrpld/telemetry/MetricsRegistry.cpp +++ b/src/xrpld/telemetry/MetricsRegistry.cpp @@ -63,6 +63,7 @@ #include #include #include +#include #include #include @@ -240,14 +241,17 @@ MetricsRegistry::~MetricsRegistry() } void -MetricsRegistry::start(std::string const& endpoint, std::string const& instanceId) +MetricsRegistry::start( + std::string const& endpoint, + std::string const& instanceId, + std::string const& nodeId) { #ifdef XRPL_ENABLE_TELEMETRY if (!enabled_) return; JLOG(journal_.info()) << "MetricsRegistry: starting, endpoint=" << endpoint - << ", instanceId=" << instanceId; + << ", instanceId=" << instanceId << ", nodeId=" << nodeId; // Rule for anything added below: this phase may create only instruments // whose recording is PUSHED from app code -- counters and histograms. An @@ -257,13 +261,14 @@ MetricsRegistry::start(std::string const& endpoint, std::string const& instanceI // belongs in startAsyncGauges(), not here. That includes observable // COUNTERS, not just gauges: jq_trans_overflow_total was created here and // its callback read getOverlay(), which asserts overlay_ is non-null. - initExporterAndProvider(endpoint, instanceId); + initExporterAndProvider(endpoint, instanceId, nodeId); initSyncInstruments(); JLOG(journal_.info()) << "MetricsRegistry: provider and instruments ready"; #else (void)endpoint; (void)instanceId; + (void)nodeId; (void)enabled_; #endif // XRPL_ENABLE_TELEMETRY } @@ -294,7 +299,10 @@ MetricsRegistry::startAsyncGauges() #ifdef XRPL_ENABLE_TELEMETRY void -MetricsRegistry::initExporterAndProvider(std::string const& endpoint, std::string const& instanceId) +MetricsRegistry::initExporterAndProvider( + std::string const& endpoint, + std::string const& instanceId, + std::string const& nodeId) { // Configure OTLP/HTTP metric exporter. otlp_http::OtlpHttpMetricExporterOptions exporterOpts; @@ -318,6 +326,11 @@ MetricsRegistry::initExporterAndProvider(std::string const& endpoint, std::strin attrs[opentelemetry::semconv::service::kServiceName] = std::string("xrpld"); if (!instanceId.empty()) attrs[opentelemetry::semconv::service::kServiceInstanceId] = instanceId; + // xrpl.node.id: the same per-node key the trace resource carries, so + // metrics and traces resolve to one node. std::string for the same + // variant reason as service.name above. + if (!nodeId.empty()) + attrs[std::string(attr::nodeId)] = nodeId; auto resourceAttrs = otel_resource::Resource::Create(attrs); // Build a view registry with explicit microsecond buckets for the diff --git a/src/xrpld/telemetry/MetricsRegistry.h b/src/xrpld/telemetry/MetricsRegistry.h index 3662f1960b..7acc6f63ee 100644 --- a/src/xrpld/telemetry/MetricsRegistry.h +++ b/src/xrpld/telemetry/MetricsRegistry.h @@ -285,9 +285,15 @@ public: * attribute. When non-empty, Prometheus metrics * carry a service_instance_id label for per-node * filtering. + * @param nodeId Value for the xrpl.node.id resource attribute (the + * node's base58 public key). When non-empty, metrics + * carry the same per-node key that traces do. */ void - start(std::string const& endpoint, std::string const& instanceId = {}); + start( + std::string const& endpoint, + std::string const& instanceId = {}, + std::string const& nodeId = {}); /** * Register the pull-model observable instruments — the second startup @@ -971,9 +977,13 @@ private: * * @param endpoint OTLP/HTTP metrics endpoint URL. * @param instanceId service.instance.id resource attribute (may be empty). + * @param nodeId xrpl.node.id resource attribute (may be empty). */ void - initExporterAndProvider(std::string const& endpoint, std::string const& instanceId); + initExporterAndProvider( + std::string const& endpoint, + std::string const& instanceId, + std::string const& nodeId); /** * Create the synchronous instruments (RPC and job-queue counters and