From 733af97ce35e878b1018250f951b1a2a6b7373a7 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 11 Aug 2026 16:05:21 +0100 Subject: [PATCH] docs(telemetry): fix peer disconnect panel regex; document overlay gaps The Peer Disconnect Rate By Reason panel anchored its LogQL capture on "\] ", which only matches a reason logged immediately after the [NNN] peer-id prefix. PeerImp does not log that way: PeerImp::fail emits "[NNN] failed: " and the clean teardown emits "close: Closed". Only ConnectAttempt::fail, which logs the bare reason, ever matched. The panel's Timeout series was therefore connect-attempt timeouts only, Ping Timeout was invisible, and PeerImp's own Closed was uncounted. Match all three prefixes and separate Ping Timeout from Connect Timeout. Recorded as LogQL trap 11 in the runbook, alongside the other silent failures this dashboard exposed. Also document six overlay observability gaps found while auditing what ping/pong and gossip traffic is actually tracked. All are pre-existing and none is fixed here: the code fixes belong in develop-owned overlay files (TrafficCount, OverlayImpl, PeerImp, PeerfinderManager), not on a telemetry branch, and one of them needs a public signature change. - 09 section 6: six known issues, each marked NOT IMPLEMENTED with file:line evidence -- mtCLUSTER counted as unknown (overhead_cluster_* always zero, 8 panels flatline), squelch_ignored byte counts always zero, inbound/outbound byte-basis asymmetry plus a stale Total header comment, ping/endpoints instrumentation absent, peer span coverage, and PeerFinder exporting 2 of ~17 available readings. - 02 section 2.3.2: add a Status column to the span catalog. Of 36 catalogued spans, 16 are live, 15 were never built, and 5 shipped under different names (consensus.phase.establish became consensus.establish, ledger.close became consensus.ledger_close, rpc.request split into rpc.http_request/rpc.ws_message, txq.apply became txq.apply_direct/txq.accept_tx). The catalog is a design inventory; 09 section 1.1 remains authoritative for what emits. - Phase9_taskList: tasks 9.14-9.17 tracking the deferred work, with exit criteria checked only for what is actually done. - Glossary: new Ping / pong keepalive term distinguishing ping timeout from connect timeout. Correct the Cluster and Squelch entries, which described behaviour the metrics cannot show. The glossary header pointed at tasks/telemetry_terms.py as its generator. That file is in no branch and nowhere on disk -- tasks/ is gitignored one directory up -- so the header now states the file is hand-maintained and gives the entry format. Gates: check_otel_naming.py passes all 9 rules (Rule D over 555 dashboard queries, Rule E over the runbook); 19 doc anchors verified; dashboard JSON valid with a one-line diff. No C++ changes. --- OpenTelemetryPlan/02-design-decisions.md | 88 +++++----- .../09-data-collection-reference.md | 159 +++++++++++++++++- OpenTelemetryPlan/Phase9_taskList.md | 131 +++++++++++++++ .../dashboards/log-derived-insights.json | 4 +- docs/telemetry-glossary.md | 27 ++- docs/telemetry-runbook.md | 16 +- 6 files changed, 375 insertions(+), 50 deletions(-) diff --git a/OpenTelemetryPlan/02-design-decisions.md b/OpenTelemetryPlan/02-design-decisions.md index 4b9dcf993e..ff71e44a80 100644 --- a/OpenTelemetryPlan/02-design-decisions.md +++ b/OpenTelemetryPlan/02-design-decisions.md @@ -123,44 +123,56 @@ path in Phase 1b through Phase 5. ### 2.3.2 Complete Span Catalog -| Span name | Description | -| ------------------------------ | --------------------------------------- | -| `tx.receive` | Transaction received from network | -| `tx.validate` | Transaction signature/format validation | -| `tx.process` | Full transaction processing | -| `tx.relay` | Transaction relay to peers | -| `tx.apply` | Apply transaction to ledger | -| `consensus.round` | Complete consensus round | -| `consensus.phase.open` | Open phase - collecting transactions | -| `consensus.phase.establish` | Establish phase - reaching agreement | -| `consensus.phase.accept` | Accept phase - applying consensus | -| `consensus.proposal.receive` | Receive peer proposal | -| `consensus.proposal.send` | Send our proposal | -| `consensus.validation.receive` | Receive peer validation | -| `consensus.validation.send` | Send our validation | -| `rpc.request` | HTTP/WebSocket request handling | -| `rpc.command.*` | Specific RPC command (dynamic) | -| `peer.connect` | Peer connection establishment | -| `peer.disconnect` | Peer disconnection | -| `peer.message.send` | Send protocol message | -| `peer.message.receive` | Receive protocol message | -| `ledger.acquire` | Ledger acquisition from network | -| `ledger.build` | Build new ledger | -| `ledger.validate` | Ledger validation | -| `ledger.close` | Close ledger | -| `ledger.replay` | Ledger replay executed | -| `ledger.delta` | Delta-based ledger acquired | -| `pathfind.request` | Path request initiated | -| `pathfind.compute` | Path computation executed | -| `txq.enqueue` | Transaction queued | -| `txq.apply` | Queued transaction applied | -| `fee.escalate` | Fee escalation triggered | -| `validator.list.fetch` | UNL list fetched | -| `validator.manifest` | Manifest update processed | -| `amendment.vote` | Amendment voting executed | -| `shamap.sync` | State tree synchronization | -| `job.enqueue` | Job added to queue | -| `job.execute` | Job execution | +> **Status column.** This catalog is the design inventory; it is not a +> statement of what currently emits. `Live` means the span is present in the +> implemented inventory ([09-data-collection-reference.md §1.1](./09-data-collection-reference.md#11-complete-span-inventory-37-spans)), +> which is the authoritative list. `Renamed`/`Split` means the concept shipped +> under a different name than planned here. **Not built** means no span is +> emitted for it today. +> +> The four `peer.*` entries are the peer-span coverage gap: only +> `peer.proposal.receive` and `peer.validation.receive` exist, so protocol +> message send/receive and connection lifecycle are untraced. See +> [09 §6.4](./09-data-collection-reference.md#64-peer-span-coverage-gap-not-implemented). + +| Span name | Description | Status | +| ------------------------------ | --------------------------------------- | ------------------------------------------------ | +| `tx.receive` | Transaction received from network | Live | +| `tx.validate` | Transaction signature/format validation | **Not built** | +| `tx.process` | Full transaction processing | Live | +| `tx.relay` | Transaction relay to peers | **Not built** | +| `tx.apply` | Apply transaction to ledger | Live | +| `consensus.round` | Complete consensus round | Live | +| `consensus.phase.open` | Open phase - collecting transactions | Live | +| `consensus.phase.establish` | Establish phase - reaching agreement | Renamed `consensus.establish` | +| `consensus.phase.accept` | Accept phase - applying consensus | Renamed `consensus.accept` | +| `consensus.proposal.receive` | Receive peer proposal | Live | +| `consensus.proposal.send` | Send our proposal | Live | +| `consensus.validation.receive` | Receive peer validation | Live | +| `consensus.validation.send` | Send our validation | Live | +| `rpc.request` | HTTP/WebSocket request handling | Split into `rpc.http_request` / `rpc.ws_message` | +| `rpc.command.*` | Specific RPC command (dynamic) | Live | +| `peer.connect` | Peer connection establishment | **Not built** | +| `peer.disconnect` | Peer disconnection | **Not built** | +| `peer.message.send` | Send protocol message | **Not built** | +| `peer.message.receive` | Receive protocol message | **Not built** | +| `ledger.acquire` | Ledger acquisition from network | Live | +| `ledger.build` | Build new ledger | Live | +| `ledger.validate` | Ledger validation | Live | +| `ledger.close` | Close ledger | Renamed `consensus.ledger_close` | +| `ledger.replay` | Ledger replay executed | **Not built** | +| `ledger.delta` | Delta-based ledger acquired | **Not built** | +| `pathfind.request` | Path request initiated | Live | +| `pathfind.compute` | Path computation executed | Live | +| `txq.enqueue` | Transaction queued | Live | +| `txq.apply` | Queued transaction applied | Renamed `txq.apply_direct` / `txq.accept_tx` | +| `fee.escalate` | Fee escalation triggered | **Not built** | +| `validator.list.fetch` | UNL list fetched | **Not built** | +| `validator.manifest` | Manifest update processed | **Not built** | +| `amendment.vote` | Amendment voting executed | **Not built** | +| `shamap.sync` | State tree synchronization | **Not built** | +| `job.enqueue` | Job added to queue | **Not built** | +| `job.execute` | Job execution | **Not built** | ### 2.3.3 Attribute Naming Conventions diff --git a/OpenTelemetryPlan/09-data-collection-reference.md b/OpenTelemetryPlan/09-data-collection-reference.md index 926fb09e06..1c67d275b2 100644 --- a/OpenTelemetryPlan/09-data-collection-reference.md +++ b/OpenTelemetryPlan/09-data-collection-reference.md @@ -1663,13 +1663,158 @@ counters), observed from an existing cumulative source each collection cycle: ## 6. Known Issues -| Issue | Impact | Status | -| ------------------------------------------------------------------ | ------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------- | -| `warn` and `drop` metrics use non-standard StatsD `\|m` meter type | Metrics silently dropped by OTel StatsD receiver | Phase 6 Task 6.1 — needs `\|m` → `\|c` change in StatsDCollector.cpp | -| `jobq_job_count` may not emit in standalone mode | Missing from Prometheus in some test configs | Requires active job queue activity | -| `rpc_requests` depends on `[insight]` config | Zero series if StatsD not configured | Requires `[insight] server=statsd` in xrpld.cfg | -| Peer tracing enabled by default | `peer.*` spans emit unless `trace_peer=0` | High volume — set `trace_peer=0` to opt out on busy mainnet nodes | -| `handler="other"` mixes several producers | Cannot separate `GetConsL1` from `GetConsL2` | By design — the cardinality bound; see [§Per-Job-Type Metrics](#per-job-type-metrics-synchronous-countershistogram) | +| Issue | Impact | Status | +| --------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------- | +| `warn` and `drop` metrics use non-standard StatsD `\|m` meter type | Metrics silently dropped by OTel StatsD receiver | Phase 6 Task 6.1 — needs `\|m` → `\|c` change in StatsDCollector.cpp | +| `jobq_job_count` may not emit in standalone mode | Missing from Prometheus in some test configs | Requires active job queue activity | +| `rpc_requests` depends on `[insight]` config | Zero series if StatsD not configured | Requires `[insight] server=statsd` in xrpld.cfg | +| Peer tracing enabled by default | `peer.*` spans emit unless `trace_peer=0` | High volume — set `trace_peer=0` to opt out on busy mainnet nodes | +| `handler="other"` mixes several producers | Cannot separate `GetConsL1` from `GetConsL2` | By design — the cardinality bound; see [§Per-Job-Type Metrics](#per-job-type-metrics-synchronous-countershistogram) | +| `overhead_cluster_*` is always zero | 8 dashboard panel references are flatlines by construction; cluster traffic is counted as `unknown` | **NOT IMPLEMENTED** — see [§6.0](#60-mtcluster-is-counted-as-unknown-not-implemented) | +| `squelch_ignored_bytes_in/out` always read zero | Only the `_messages_*` pair carries signal for this category | **NOT IMPLEMENTED** — see [§6.1](#61-squelch_ignored-byte-counts-not-implemented) | +| `total_bytes_in` and `total_bytes_out` use different size bases | In/out byte totals are not directly comparable when compression is on | **NOT IMPLEMENTED** — see [§6.2](#62-inboundoutbound-byte-basis-asymmetry-not-implemented) | +| `overhead` conflates `mtPING` with `mtSTATUS_CHANGE` | Keepalive traffic cannot be isolated from status-change traffic | **NOT IMPLEMENTED** — needs a new category; see [§6.3](#63-peer-keepalive-and-discovery-traffic-gaps-not-implemented) | +| No metrics for ping RTT distribution, ping timeouts, or `mtENDPOINTS` | Peer keepalive and discovery health are not observable | **NOT IMPLEMENTED** — see [§6.3](#63-peer-keepalive-and-discovery-traffic-gaps-not-implemented) | +| 11 of 13 peer message families have no spans | `02` §2.3.2 catalogs `peer.message.*`, `peer.connect`, `peer.disconnect` that were never built | **NOT IMPLEMENTED** — see [§6.4](#64-peer-span-coverage-gap-not-implemented) | +| PeerFinder exports 2 of ~17 available slot/cache readings | Slot pressure, connection churn and discovery-cache health are not observable | **NOT IMPLEMENTED** — see [§6.5](#65-peerfinder-slot-and-cache-metrics-not-implemented) | + +### 6.0 `mtCLUSTER` is counted as `unknown`: NOT IMPLEMENTED + +`mtCLUSTER` is absent from `kTypeLookup` +(`src/xrpld/overlay/detail/TrafficCount.cpp:11-27`), and `categorize()`'s +fallback chain only inspects `TMLedgerData`, `TMGetLedger` and +`TMGetObjectByHash` before returning `Category::Unknown` (`:135`). No call site +ever passes `Category::Cluster`. Cluster traffic is therefore counted as +`unknown`, and `overhead_cluster_bytes_in/out` and +`overhead_cluster_messages_in/out` are always zero — including the 8 panel +references across `network-traffic` and `overlay-traffic-detail` (both the local +and grafanacloud copies). + +This also degrades `unknown_*` as an anomaly signal: on a clustered node it mixes +genuinely unrecognized wire types with routine `mtCLUSTER` traffic. + +**Status**: Planned, not yet implemented. The fix is a one-line addition to +`kTypeLookup`, but `TrafficCount.cpp` is shared overlay code rather than a +telemetry-owned file, so it is scoped as a separate overlay change. Note that +landing it moves volume out of `unknown_bytes_in`, so any threshold measured +against that series needs re-baselining. Until then, treat `overhead_cluster_*` +as "no data" rather than "no cluster traffic", and read the +[Cluster](../docs/telemetry-glossary.md#cluster) glossary entry's guidance on +sustained cluster overhead as not yet observable. + +### 6.1 `squelch_ignored` byte counts: NOT IMPLEMENTED + +`OverlayImpl::updateSlotAndSquelch` reports the `SquelchIgnored` category with a +hardcoded size of `0` (`src/xrpld/overlay/detail/OverlayImpl.cpp:1460` and +`:1489`), so `squelch_ignored_bytes_in` and `squelch_ignored_bytes_out` are +always zero. Only `squelch_ignored_messages_in/out` carry signal. This is +inconsistent with `SquelchSuppressed`, which passes the real wire size +(`src/xrpld/overlay/detail/PeerImp.cpp:302`) — so the two squelch categories are +not comparable on bytes. + +The message size is available at all four call sites (each holds the protobuf +message and could call `Message::messageSize()`), but plumbing it through would +require widening the two `OverlayImpl::updateSlotAndSquelch` overloads. + +**Status**: Deferred as a separate change — a public signature change on +`OverlayImpl` is out of scope for the telemetry chain, since `OverlayImpl.h` is +shared overlay code rather than a telemetry-owned file. Until it lands, read +`squelch_ignored` on the `_messages_*` series only and do not build a +bytes-per-message ratio from this category. + +### 6.2 Inbound/outbound byte-basis asymmetry: NOT IMPLEMENTED + +Inbound traffic is counted with the raw wire size as received +(`src/xrpld/overlay/detail/PeerImp.cpp:1079`), while outbound traffic is counted +from the possibly-compressed send buffer +(`getBuffer(compressionEnabled_).size()`, `PeerImp.cpp:313`). When compression is +enabled the two directions measure different things, so `total_bytes_in` versus +`total_bytes_out` is not a like-for-like comparison, and neither is any +`{category}_bytes_in` / `_bytes_out` pair. + +A related documentation defect sits in the same class: the `TrafficCount` header +comment states that "messages whose category is not in `TrafficCount::categorize` +are not included in the total" (`src/xrpld/overlay/detail/TrafficCount.h:28-31`), +but `Category::Total` is incremented unconditionally at +`src/xrpld/overlay/detail/PeerImp.cpp:1079`, _before_ the per-category split. The +total does include uncategorized traffic; the comment is stale. + +**Status**: Planned, not yet implemented — neither the metric change nor the +header-comment correction has landed, because `TrafficCount.h` is shared overlay +code rather than a telemetry-owned file. Normalizing one direction would in any +case silently redefine an existing series, so the likely resolution is to document +the asymmetry at the class and leave both readings intact. Until then, compare +`_bytes_in` against `_bytes_out` only when compression is known to be off. + +### 6.3 Peer keepalive and discovery traffic gaps: NOT IMPLEMENTED + +Three related gaps on the peer keepalive and discovery paths. All are byte/message +counters only — none has a dedicated instrument, and none is traced. + +| Gap | Current state | What is missing | +| --------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------- | +| `mtPING` / `mtSTATUS_CHANGE` share `Category::Base` (`TrafficCount.cpp:12-13`) → `overhead_*` | Both message types land in one bucket | A distinct category for one of them, plus name-map, `counts_` and dashboard entries | +| Ping RTT | An 8-sample EWMA per peer (`PeerImp.cpp:1150-1163`), exported only as the single `peer_quality{metric="peer_latency_p90_ms"}` gauge | A histogram; the lone p90 hides a bimodal peer set | +| Ping failures | `fail("Ping Timeout")` (`PeerImp.cpp:762`) logs only; a wrong-cookie PONG (`PeerImp.cpp:1146`) is silently ignored | A counter for each | +| `mtENDPOINTS` | `overhead_overlay_*` bytes only | Counters for endpoints received / handed out / malformed (`PeerImp.cpp:1265-1270` charges a fee but records no metric) | + +**Status**: Planned, not yet implemented. Adding these means a new metric family +plus matching rows in this document, in +[docs/telemetry-runbook.md](../docs/telemetry-runbook.md) § Metric Reference, and +in `docker/telemetry/workload/expected_metrics.json` (Phase 10 branch — see the +Cross-Phase Dependency Chain in +[06-implementation-phases.md](./06-implementation-phases.md)), and dashboard +panels following the conventions in `06` § Branch-to-Change Mapping. + +### 6.4 Peer span coverage gap: NOT IMPLEMENTED + +[02-design-decisions.md §2.3.2](./02-design-decisions.md#232-complete-span-catalog) +catalogs `peer.connect`, `peer.disconnect`, `peer.message.send` and +`peer.message.receive`. None was ever built: the implemented peer surface is the +two spans in [§Peer Spans](#peer-spans) above (`peer.proposal.receive`, +`peer.validation.receive`). Of the 13 protocol message families, only +`mtGET_OBJECTS` has native instrumentation, and only transactions and consensus +messages are traced. + +**Status**: NOT IMPLEMENTED. The span catalog in `02` §2.3.2 is a design +inventory, not a statement of what emits; §2.3.2 now marks which entries are +live. Instrumenting the remaining families would change the "~37 spans" count +asserted in [§1.1](#11-complete-span-inventory-37-spans) and in +`docker/telemetry/workload/expected_spans.json`, so it is scoped as its own +change rather than folded into a metric task. + +### 6.5 PeerFinder slot and cache metrics: NOT IMPLEMENTED + +`peer_finder::Manager` registers exactly two instruments — +`peer_finder_active_inbound_peers` and `peer_finder_active_outbound_peers` +(`src/libxrpl/peerfinder/PeerfinderManager.cpp:229-230`), listed in +[§2.1](#21-gauges). The `Counts` class exposes roughly fifteen further readings +that are never exported (`include/xrpl/peerfinder/detail/Counts.h`), and neither +discovery cache has any instrument at all. + +| Reading | Source | Why it matters | +| --------------------------------------------------- | ---------------------- | ---------------------------------------------------------------------------------------------------------------------- | +| `attempts()`, `attemptsNeeded()` | `Counts.h:68,79` | Outbound connection churn; distinguishes "not trying" from "trying and failing" | +| `outMax()`, `outActive()`, `outboundSlotsFree()` | `Counts.h:88,98,205` | Outbound slot saturation | +| `inMax()`, `inboundActive()`, `inboundSlotsFree()` | `Counts.h:165,174,193` | Inbound slot saturation — the two exported gauges give the actives but not the caps, so utilization cannot be computed | +| `acceptCount()`, `connectCount()`, `closingCount()` | `Counts.h:138,147,156` | Handshake pipeline depth; `closingCount()` rising is teardown backpressure | +| `fixed()`, `fixedActive()` | `Counts.h:107,116` | Whether configured fixed peers are actually connected | +| `isConnectedToNetwork()` | `Counts.h:218` | Binary reachability | +| `Livecache::size()` | `Livecache.h:365` | Size of the live endpoint pool used to answer `mtENDPOINTS` | +| `Bootcache::size()` | `Bootcache.h:121` | Bootstrap-address pool; an empty bootcache is why a fresh node cannot find peers | + +The two exported actives are also the only inputs to the "Inbound vs Outbound" +panel specified for the Peer Quality dashboard +([06 § Branch-to-Change Mapping, Task 9.12](./06-implementation-phases.md)), so +that panel cannot show slot utilization as a percentage. + +**Status**: Planned, not yet implemented. These would extend the existing +`beast::insight` registration in `PeerfinderManager.cpp` (arrow **B** in the +[Data Flow Overview](#data-flow-overview)) rather than use the `XRPL_METRIC_*` +macros, because `libxrpl` code cannot use those macros — see the pipeline note in +[§2.5](#25-per-job-type-queue-gauges). `Livecache`/`Bootcache` currently receive +no collector reference, so exporting their sizes needs one plumbed in or the +values read via the existing `Manager` hook. --- diff --git a/OpenTelemetryPlan/Phase9_taskList.md b/OpenTelemetryPlan/Phase9_taskList.md index 818530ef93..5c67ec1097 100644 --- a/OpenTelemetryPlan/Phase9_taskList.md +++ b/OpenTelemetryPlan/Phase9_taskList.md @@ -439,6 +439,137 @@ These metrics serve multiple external consumer categories identified during rese --- +## Task 9.14: Overlay Traffic Accounting Defects (Documentation Only) + +> **Status**: DOCUMENTED, NOT FIXED. Reference: [09 §6.0-§6.2](./09-data-collection-reference.md#6-known-issues) + +**Objective**: Record four pre-existing overlay traffic-accounting defects so +dashboard readers are not misled. All four originate in `develop`-owned overlay +files, so **no code fix lands on this branch**. + +| # | Defect | Effect | Fix location (NOT this branch) | +| --- | -------------------------------------- | ----------------------------------------------------------------------------------------- | ------------------------------------------------ | +| 1 | `mtCLUSTER` missing from `kTypeLookup` | `overhead_cluster_*` always zero; 8 panels flatline; cluster traffic counted as `unknown` | `TrafficCount.cpp:11-27` | +| 2 | Stale `Total` header comment | Claims uncategorized traffic is excluded; it is included | `TrafficCount.h:28-31` | +| 3 | `SquelchIgnored` reported with size 0 | `squelch_ignored_bytes_*` always zero, inconsistent with `SquelchSuppressed` | `OverlayImpl.cpp:1460,1489` (+ signature change) | +| 4 | In/out byte-basis asymmetry | `_bytes_in` vs `_bytes_out` not comparable under compression | `PeerImp.cpp:1079` vs `:313` | + +**Why deferred**: Defect 3 requires widening the two +`OverlayImpl::updateSlotAndSquelch` overloads — a public signature change on +shared overlay code. Defects 1, 2 and 4 sit in `TrafficCount.{h,cpp}`, likewise +not telemetry-owned. Routing them through the telemetry chain would hide overlay +changes from overlay reviewers and couple them to a 12-PR merge timeline. + +**Key modified files**: `OpenTelemetryPlan/09-data-collection-reference.md` only. + +**Exit Criteria**: + +- [x] Each defect documented with file:line evidence in `09` §6 +- [x] `overhead_cluster_*` documented as "no data", not "no cluster traffic" +- [ ] Follow-up overlay-owned branch raised for the four code fixes +- [ ] Re-baseline any threshold keyed on `unknown_bytes_in` when defect 1 lands + +--- + +## Task 9.15: Peer Keepalive and Discovery Instrumentation + +> **Status**: NOT IMPLEMENTED — awaiting a decision on whether `XRPL_METRIC_*` +> call sites may be added to `src/xrpld/overlay/detail/PeerImp.cpp` from this +> branch. Reference: [09 §6.3](./09-data-collection-reference.md#63-peer-keepalive-and-discovery-traffic-gaps-not-implemented) + +**Objective**: Make peer keepalive and peer-discovery health observable. Today +`mtPING`, `mtSTATUS_CHANGE` and `mtENDPOINTS` are byte counters only. + +| Proposed metric | Type | Labels | Record site | +| ------------------------------- | --------- | -------------------------------- | --------------------------------------------------- | +| `peer_ping_rtt_ms` | Histogram | none (see note) | `PeerImp.cpp:1150-1163`, where the EWMA is computed | +| `peer_ping_timeouts_total` | Counter | `reason="timeout"\|"bad_cookie"` | `PeerImp.cpp:762` and `:1146` | +| `peer_endpoints_received_total` | Counter | `result="accepted"\|"malformed"` | `PeerImp.cpp:1265-1270` | + +**Design notes / open questions**: + +- A histogram needs an explicit bucket view: the SDK default tops out at 10000, + and these are milliseconds. Follow the µs-ladder precedent in + `MetricsRegistry.cpp` (see [09 § GetObject Request Path](./09-data-collection-reference.md#getobject-request-path-synchronous-countershistograms)). +- `peer_id` as a label is unbounded cardinality — rejected. A bounded + `peer_role`-style label is the alternative if per-peer attribution is needed. +- Splitting `mtPING` out of `Category::Base` is a `TrafficCount.cpp` change and + therefore blocked with Task 9.14. +- Per the runbook's "Adding a New Metric" contract, `_total` is reserved for + monotonic counters; a histogram takes no suffix. + +**Key files (if approved)**: `src/xrpld/overlay/detail/PeerImp.cpp`, +`09-data-collection-reference.md`, `docs/telemetry-runbook.md` § Metric Reference, +`docker/telemetry/grafana/dashboards/peer-quality.json`, and +`docker/telemetry/workload/expected_metrics.json` (**Phase 10 branch**). + +**Exit Criteria**: + +- [ ] Decision recorded on editing `PeerImp.cpp` from the telemetry chain +- [ ] Three instruments emitting, with an explicit histogram bucket view +- [ ] Rows added to `09` §5b, runbook § Metric Reference, and `expected_metrics.json` +- [ ] Peer Quality dashboard panels follow the Task 9.12 conventions (`$node`, Title Case, legend dimensions) +- [ ] `check_otel_naming.py` passes (Rules D and E cover the new labels) + +--- + +## Task 9.16: PeerFinder Slot and Cache Metrics + +> **Status**: NOT IMPLEMENTED. Reference: [09 §6.5](./09-data-collection-reference.md#65-peerfinder-slot-and-cache-metrics-not-implemented) + +**Objective**: Export the PeerFinder slot counts and discovery-cache sizes. +Only 2 of ~17 available readings are exported today. + +**What to do**: Extend the existing `Stats` struct in +`src/libxrpl/peerfinder/PeerfinderManager.cpp:227-236` with gauges for the +`Counts` accessors listed in [09 §6.5](./09-data-collection-reference.md#65-peerfinder-slot-and-cache-metrics-not-implemented) +(slot caps and frees, attempt counts, handshake pipeline depth, fixed-peer state, +network reachability), plus `Livecache::size()` and `Bootcache::size()`. + +**Pipeline constraint**: `PeerfinderManager.cpp` is in `libxrpl`, which **cannot** +use the `XRPL_METRIC_*` macros. These must go through `beast::insight` — +arrow **B**, not **C**. Naming follows `GroupImp::makeName()` + +`OTelCollectorImp::formatName()`, so the `"Peer_Finder"` group yields +`peer_finder_` lowercased. + +**Known obstacle**: `Livecache` and `Bootcache` hold no collector reference, so +their sizes must either be read through the existing `Manager` hook or have a +collector plumbed in. + +**Exit Criteria**: + +- [ ] Slot caps exported so utilization (`active / max`) is computable +- [ ] Both cache sizes exported +- [ ] "Inbound vs Outbound" panel on `peer-quality` extended to show utilization % +- [ ] Rows added to `09` §2.1, runbook § Metric Reference, `expected_metrics.json` (Phase 10) + +--- + +## Task 9.17: Peer Span Coverage (Deferred to Phase 11) + +> **Status**: NOT IMPLEMENTED — design only, pending approval. Reference: +> [09 §6.4](./09-data-collection-reference.md#64-peer-span-coverage-gap-not-implemented) +> and [02 §2.3.2](./02-design-decisions.md#232-complete-span-catalog) + +**Objective**: Close the gap between the `02` §2.3.2 span catalog and what +actually emits. `peer.connect`, `peer.disconnect`, `peer.message.send` and +`peer.message.receive` were catalogued from the start and never built; 11 of 13 +protocol message families have no spans. + +**Scope warning**: This is larger than Tasks 9.14-9.16 combined and changes the +"~37 spans" figure asserted in `09` §1.1 and in +`docker/telemetry/workload/expected_spans.json`. `trace_peer` is also **on by +default** and already flagged as high-volume, so adding per-message spans has a +volume cost that needs measuring before commitment. + +**Exit Criteria**: + +- [x] `02` §2.3.2 marked Live / Not built / Renamed against the real inventory +- [ ] User approval to proceed with span implementation +- [ ] Volume impact measured under `trace_peer=1` before any span is added + +--- + ## Exit Criteria - [ ] All ~50 new metrics visible in Prometheus via OTLP pipeline diff --git a/docker/telemetry/grafana/dashboards/log-derived-insights.json b/docker/telemetry/grafana/dashboards/log-derived-insights.json index dc2d967c1d..6acda66c69 100644 --- a/docker/telemetry/grafana/dashboards/log-derived-insights.json +++ b/docker/telemetry/grafana/dashboards/log-derived-insights.json @@ -1858,7 +1858,7 @@ { "type": "timeseries", "title": "Peer Disconnect Rate By Reason", - "description": "###### What this is:\n*Rate of peer connection endings, split by the reason recorded in the log.*\n\n###### How it's computed:\n*Per-second count of Peer log lines matching Timeout, Closed, or a refused connection attempt.*\n\n###### Reading it:\n*Distinguishes clean teardown from failure. Closed is a normal ending; Timeout and Connection refused are not.*\n\n###### Healthy range:\n*Closed dominant with a low, steady background of the others.*\n\n###### Watch for:\n*A Timeout rate approaching the Closed rate, which points at network trouble or unresponsive peers.*\n\n###### Keywords:\n- **Closed** *(per peer)* — a clean connection teardown; the normal ending.\n- **Timeout** *(per peer)* — the peer stopped responding.\n- **Connection refused** *(per attempt)* — an outbound attempt the remote rejected.\n\n###### Computation boundary:\n*Result: Per node per outcome — a count of peer lifecycle events.*\n*Derived in the Grafana query. Note `overlay_peer_disconnects` exists as a metric but carries no reason breakdown, which is what this panel adds.*\n\n###### Source:\n[PeerImp.cpp](https://github.com/XRPLF/rippled/blob/develop/src/xrpld/overlay/detail/PeerImp.cpp)\n\n###### Function:\n`PeerImp::close / onTimer`\n\n###### References:\n[Peer protocol](https://xrpl.org/docs/concepts/networks-and-servers/peer-protocol)", + "description": "###### What this is:\n*Rate of peer connection endings, split by the reason recorded in the log.*\n\n###### How it's computed:\n*Per-second count of Peer log lines matching a clean close, a ping-keepalive timeout, a connect timeout, or a refused connection attempt.*\n\n###### Reading it:\n*Distinguishes clean teardown from failure. Closed is a normal ending; Timeout and Connection refused are not.*\n\n###### Healthy range:\n*Closed dominant with a low, steady background of the others.*\n\n###### Watch for:\n*A Ping Timeout rate approaching the Closed rate, which points at network trouble or unresponsive peers.*\n\n###### Keywords:\n- **Closed** *(per peer)* — a clean connection teardown; the normal ending.\n- **Connect Timeout** *(per attempt)* — an outbound connect attempt timed out.\n- **Ping Timeout** *(per peer)* — an established peer missed its keepalive PONG.\n- **Connection refused** *(per attempt)* — an outbound attempt the remote rejected.\n\n###### Computation boundary:\n*Result: Per node per outcome — a count of peer lifecycle events.*\n*Derived in the Grafana query. Note `overlay_peer_disconnects` exists as a metric but carries no reason breakdown, which is what this panel adds.*\n\n###### Source:\n[PeerImp.cpp](https://github.com/XRPLF/rippled/blob/develop/src/xrpld/overlay/detail/PeerImp.cpp)\n\n###### Function:\n`PeerImp::close / onTimer`\n\n###### References:\n[Peer protocol](https://xrpl.org/docs/concepts/networks-and-servers/peer-protocol)", "gridPos": { "h": 10, "w": 12, @@ -1906,7 +1906,7 @@ "uid": "${DS_LOKI}" }, "refId": "A", - "expr": "sum by (outcome, service_instance_id) (rate({service_name=~\"$service_name\", service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\"} | xrpl_network_type =~ \"$xrpl_network_type\" | partition = `Peer` | severity =~ \"$severity\" | regexp `\\] (?PTimeout|Closed|onConnect: Connection refused)` | outcome != `` | label_format outcome=`{{if eq .outcome \"Closed\"}}Closed{{else if eq .outcome \"Timeout\"}}Timeout{{else if eq .outcome \"onConnect: Connection refused\"}}Connection Refused{{else}}{{.outcome}}{{end}}` [$__auto]))" + "expr": "sum by (outcome, service_instance_id) (rate({service_name=~\"$service_name\", service_instance_id=~\"$node\", deployment_environment=~\"$deployment_environment\"} | xrpl_network_type =~ \"$xrpl_network_type\" | partition = `Peer` | severity =~ \"$severity\" | regexp `(?:\\] |failed: |close: )(?PPing Timeout|Timeout|Closed|onConnect: Connection refused)` | outcome != `` | label_format outcome=`{{if eq .outcome \"Closed\"}}Closed{{else if eq .outcome \"Ping Timeout\"}}Ping Timeout{{else if eq .outcome \"Timeout\"}}Connect Timeout{{else if eq .outcome \"onConnect: Connection refused\"}}Connection Refused{{else}}{{.outcome}}{{end}}` [$__auto]))" } ], "id": 29, diff --git a/docs/telemetry-glossary.md b/docs/telemetry-glossary.md index 62e0e42160..fa38be7a3d 100644 --- a/docs/telemetry-glossary.md +++ b/docs/telemetry-glossary.md @@ -9,7 +9,12 @@ documentation. > **Related docs**: > [docs/telemetry-runbook.md](./telemetry-runbook.md) (operator runbook). - + ## Contents @@ -574,7 +579,9 @@ A cluster is a set of servers run by the same operator that trust each other, ex **Scope:** cluster-wide — shared across a co-operated cluster of nodes run by one operator. -**See also:** [Cluster on xrpl.org](https://xrpl.org/docs/concepts/networks-and-servers/clustering) +**What is observable:** cluster overhead is **not** measurable today. Cluster messages are counted under `unknown` rather than `overhead_cluster`, so the `overhead_cluster_*` series read zero on a clustered node — treat them as "no data", not "no cluster traffic". The churn guidance above cannot yet be acted on. + +**See also:** [Cluster on xrpl.org](https://xrpl.org/docs/concepts/networks-and-servers/clustering) · [Data collection reference §6.0](../OpenTelemetryPlan/09-data-collection-reference.md#60-mtcluster-is-counted-as-unknown-not-implemented) @@ -634,6 +641,18 @@ The overlay is xrpld's peer-to-peer messaging layer connecting nodes. All inter- **See also:** [Overlay on xrpl.org](https://xrpl.org/docs/concepts/networks-and-servers/peer-protocol) + + +### Ping / pong keepalive + +Each peer connection is probed on a timer: the node sends a ping carrying a random cookie and expects a pong echoing it back. The round-trip is smoothed into a per-peer latency estimate that feeds peer scoring, and a peer that leaves a ping unanswered before the next probe is dropped as a ping timeout. A pong bearing the wrong cookie is ignored, so a peer answering incorrectly eventually times out too. Ping timeouts are distinct from connect timeouts, which happen while an outbound connection is still being established and so involve no established peer. + +**Scope:** per node — measured on and specific to this individual server. + +**What is observable:** only the p90 of the smoothed per-peer latency (`peer_quality{metric="peer_latency_p90_ms"}`) — there is no distribution, ping timeouts and wrong-cookie pongs have no counter, and ping bytes are not separable from status-change bytes because both share the `overhead` traffic category. + +**See also:** [Data collection reference §6.3](../OpenTelemetryPlan/09-data-collection-reference.md#63-peer-keepalive-and-discovery-traffic-gaps-not-implemented) + ### Proof path @@ -690,6 +709,10 @@ Squelching is a relay-control mechanism: a node tells peers to stop sending it a **Scope:** per node — measured on and specific to this individual server. +**What is observable:** read ignored directives on `squelch_ignored_messages_in/out` only. The paired `squelch_ignored_bytes_*` series are always zero because the ignored-squelch callback records no size, so bandwidth wasted by peers ignoring squelch cannot be quantified — and `squelch_ignored` is therefore not comparable on bytes against `squelch_suppressed`, which does record real sizes. + +**See also:** [Data collection reference §6.1](../OpenTelemetryPlan/09-data-collection-reference.md#61-squelch_ignored-byte-counts-not-implemented) + ### Trusted / untrusted / duplicate diff --git a/docs/telemetry-runbook.md b/docs/telemetry-runbook.md index d1a8c25152..20ad2b7bcd 100644 --- a/docs/telemetry-runbook.md +++ b/docs/telemetry-runbook.md @@ -2480,7 +2480,7 @@ timeseries, 2 table, 1 state-timeline, 1 logs, 1 text, across 35 queries. | Manifests — Disposition & Producers | `[DBG]` | Disposition rate; accept-vs-reject; top-N master keys | | Resource Fee Charges — Load Attribution | `[DBG]` | Charge rate by reason; fee-weighted load; top-N peers by IP and public key | | Ledger Acquisition Efficiency | `[DBG]` | Duplicate ratio; good vs duplicate vs timeout | -| Peer Lifecycle & Disconnects | `[DBG]` | Disconnect reason breakdown; handshake and accept rate | +| Peer Lifecycle & Disconnects | `[DBG]` | Disconnect reason breakdown (Closed / Ping Timeout / Connect Timeout / Connection Refused); handshake and accept rate | | Consensus Phase & Mode | `[DEFAULT OK]` | Phase transitions; operating-mode proxy; quorum and trusted-set size | | Slow Job Latency Breaches | `[DEFAULT OK]` | Run p99, wait p99, breach rate by job (`LoadMonitor`, >500ms only) | | Error & Warning Stream | `[DEFAULT OK]` | WRN/ERR/FTL rate by partition; live log tail | @@ -2550,6 +2550,20 @@ Stream labels are only `service_name`, `service_instance_id`, target as a range query even with `instant: true`, so `lastNotNull` reads only the final bucket — a window total shows as a single-bucket count. Aggregate over `$__range` and reduce with `max`. +11. **A `regexp` anchored on the log prefix silently drops most matches.** The + _Peer Disconnect Rate By Reason_ panel anchored its capture on `\] `, which + only matches a reason emitted immediately after the `[NNN] ` peer-id prefix. + `PeerImp` does not log that way: `PeerImp::fail` emits + `[NNN] failed: ` (`src/xrpld/overlay/detail/PeerImp.cpp:645`) + and the clean teardown emits `close: Closed` (`:635`). Only + `ConnectAttempt::fail`, which logs the bare reason + (`src/xrpld/overlay/detail/ConnectAttempt.cpp:136`), ever matched — so the + panel's `Timeout` series was connect-attempt timeouts only, `Ping Timeout` + (`PeerImp.cpp:762`) was invisible, and `PeerImp`'s own `Closed` was + uncounted. The panel now matches all three prefixes + (`(?:\] |failed: |close: )`) and distinguishes `Ping Timeout` from + `Connect Timeout`. When adding a log-derived panel, enumerate every producer + of the string being captured rather than sampling one. Also worth knowing: the **Grafana Cloud image renderer cannot query Loki** in this stack. A minimal probe dashboard with a hardcoded datasource uid, a literal