diff --git a/OpenTelemetryPlan/06-implementation-phases.md b/OpenTelemetryPlan/06-implementation-phases.md index 7ce13eceb8..fab7fe5e3f 100644 --- a/OpenTelemetryPlan/06-implementation-phases.md +++ b/OpenTelemetryPlan/06-implementation-phases.md @@ -906,7 +906,7 @@ flowchart TB | 9.11 | Validator Health dashboard | βœ… Done | | 9.12 | Peer Quality dashboard | βœ… Done | | 9.13 | Ledger Economy row on `node-health` | βœ… Done | -| 9.14 | Overlay traffic accounting defects (documentation only) | πŸ“„ Documented, not fixed | +| 9.14 | Overlay traffic accounting defects (documentation only) | πŸ“„ Documented, 1 of 4 fixed | | 9.15 | Peer keepalive / discovery instrumentation | ❌ Not implemented | | 9.16 | PeerFinder slot and cache metrics | ❌ Not implemented | | 9.17 | Peer span coverage (`peer.connect` / `peer.message.*`) | ❌ Not implemented (deferred) | diff --git a/OpenTelemetryPlan/09-data-collection-reference.md b/OpenTelemetryPlan/09-data-collection-reference.md index 4cd23e33d0..d759e79c9b 100644 --- a/OpenTelemetryPlan/09-data-collection-reference.md +++ b/OpenTelemetryPlan/09-data-collection-reference.md @@ -2091,7 +2091,7 @@ Grafana keys a dashboard by its UID, so two dashboards sharing one UID overwrite | `rpc_requests_total` depends on `[insight]` config | Zero series if `[insight]` is absent or unset | Requires `[insight] server=otel` 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) | +| `overhead_cluster_*` counts cluster members only | Zero on a node with no `[cluster_nodes]`; cluster messages from non-member peers count as `unknown` | By design β€” see [Β§6.0](#60-cluster-traffic-is-counted-for-cluster-members-only) | | `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) | @@ -2102,29 +2102,16 @@ Grafana keys a dashboard by its UID, so two dashboards sharing one UID overwrite | Nine dotted `xrpl..*` span attributes never shipped | TraceQL filters and harness assertions on the dotted keys match nothing | **NOT IMPLEMENTED** β€” renamed to bare keys; see [Β§Span Attribute Enrichments](#span-attribute-enrichments-phases-2-4-removed) | | `node_writes_duration_us` has no dashboard panel | Cumulative write latency is exported and linted, but never charted | Open follow-up β€” see [Β§Extended NodeStore Metrics](#extended-nodestore-metrics-additions-to-existing-nodestore_state) | -### 6.0 `mtCLUSTER` is counted as `unknown`: NOT IMPLEMENTED +### 6.0 Cluster traffic is counted for cluster members only -`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). +`kTypeLookup` in `src/xrpld/overlay/detail/TrafficCount.cpp` maps `mtCLUSTER` to `Category::Cluster`. That category is exported as `overhead_cluster_bytes_in/out` and `overhead_cluster_messages_in/out`. -This also degrades `unknown_*` as an anomaly signal: on a clustered node it mixes -genuinely unrecognized wire types with routine `mtCLUSTER` traffic. +- **Outbound:** the `Message` constructor sets the category. `NetworkOPsImp::processClusterTimer` sends cluster reports only to cluster members (`PeerInCluster`), so all outbound cluster traffic is member traffic. +- **Inbound:** `PeerImp::onMessageBegin` passes the category through `TrafficCount::attribute`. A cluster message from a cluster member stays in the cluster category. From any other peer it is counted as `unknown`, and the node ignores it. -**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. +A cluster member is a peer in this node's cluster list, which `[cluster_nodes]` sets up. A node with no `[cluster_nodes]` has no members, so its `overhead_cluster_*` series read zero. That zero is the true value, not missing data. The local telemetry stack configures no cluster, so these series read zero there. + +Routine cluster traffic does not land in `unknown_*`. On a clustered node, a sustained non-zero `unknown_*` therefore points at an unrecognized wire type or at a cluster message from a peer outside the cluster. See the [Cluster](../docs/telemetry-glossary.md#cluster) glossary entry. ### 6.1 `squelch_ignored` byte counts: NOT IMPLEMENTED diff --git a/OpenTelemetryPlan/Phase9_taskList.md b/OpenTelemetryPlan/Phase9_taskList.md index 17f7c4e6b4..0ea18ab836 100644 --- a/OpenTelemetryPlan/Phase9_taskList.md +++ b/OpenTelemetryPlan/Phase9_taskList.md @@ -596,22 +596,22 @@ dropped the `xrpld-` prefix). ## 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) +> **Status**: Defect 1 FIXED in `develop`. Defects 2-4 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` | +| # | Defect | Effect | Fix location (NOT this branch) | +| --- | -------------------------------------- | ------------------------------------------------------------------------------------------------- | ------------------------------------------------ | +| 1 | `mtCLUSTER` missing from `kTypeLookup` | **Fixed in `develop`.** Cluster messages from members count as cluster; others count as `unknown` | `TrafficCount.cpp`: `kTypeLookup`, `attribute()` | +| 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 and 4 need `TrafficCount.cpp` and `PeerImp.cpp` +shared overlay code. Defect 4 needs `PeerImp.cpp` edits that are 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. @@ -620,10 +620,10 @@ merge timeline. > `TrafficCount.{h,cpp}`" no longer holds for the header: the telemetry chain > already edits `TrafficCount.h` β€” Phase 6's `77f35c03db` fixed the > `Category::GetFetchPack` label from `"getobject_Fetch Pack_get"` to -> `"getobject_Fetch_Pack_get"` at `TrafficCount.h:285`, the sole difference from +> `"getobject_Fetch_Pack_get"` in its category-name table, the sole difference from > `develop`. Defect 2 (the stale `Total` header comment, `TrafficCount.h:28-31`) -> is therefore **unblocked** and can land here. Defects **1, 3 and 4** stay -> blocked: defect 1 needs `TrafficCount.cpp`'s `kTypeLookup`, defect 3 needs the +> is therefore **unblocked** and can land here. Defect 1 is fixed in `develop`. +> Defects **3 and 4** stay blocked: defect 3 needs the > `OverlayImpl` signature change, and defect 4 needs `PeerImp.cpp:1079` vs `:313` > to agree on a byte basis (compressed vs uncompressed) β€” a change to overlay > accounting semantics, not telemetry. @@ -633,13 +633,13 @@ merge timeline. **Exit Criteria**: - [x] Each defect documented with file:line evidence in `09` Β§6 -- [x] `overhead_cluster_*` documented as "no data", not "no cluster traffic" +- [x] Defect 1 fixed in `develop`'s `TrafficCount` code; `09` Β§6.0 says what `overhead_cluster_*` counts - [ ] Defect 2 (stale `Total` header comment, `TrafficCount.h:28-31`) fixed on this branch β€” it is **unblocked** (the chain already edits `TrafficCount.h`) but the comment is still uncorrected -- [ ] Follow-up overlay-owned branch raised for the three still-blocked code - fixes (defects 1, 3, 4) -- [ ] Re-baseline any threshold keyed on `unknown_bytes_in` when defect 1 lands +- [ ] Follow-up overlay-owned branch raised for the two still-blocked code + fixes (defects 3, 4) +- [x] No re-baseline needed for defect 1: no alert rule in this repo reads `unknown_*` --- @@ -676,9 +676,7 @@ merge timeline. `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 still blocked with Task 9.14 defect 1. (The `.h` half of that - constraint no longer applies β€” see Task 9.14.) +- Splitting `mtPING` out of `Category::Base` is a `TrafficCount.cpp` change, so it needs its own overlay change, like the one in `develop` that fixed Task 9.14 defect 1. (The `.h` half of that constraint no longer applies β€” see Task 9.14.) - Per the runbook's "Adding a New Metric" contract, `_total` is reserved for monotonic counters; a histogram takes no suffix. @@ -789,8 +787,8 @@ before commitment. "Ledger Economy" row) - [x] Provisioned Grafana alerting: 14 rules / 5 groups, 2 contact points, nested notification policy -- [ ] Tasks 9.14-9.17 closed β€” **open by design**: 9.14 documented-not-fixed - (defects 1, 3 and 4 still blocked; defect 2 unblocked but not yet fixed), +- [ ] Tasks 9.14-9.17 closed β€” **open by design**: 9.14 partly fixed + (defect 1 fixed in `develop`; defects 3 and 4 still blocked; defect 2 unblocked but not yet fixed), 9.15 and 9.16 not implemented, 9.17 deferred pending approval and volume measurement diff --git a/docs/telemetry-glossary.md b/docs/telemetry-glossary.md index 993321cdb8..409a5fa23c 100644 --- a/docs/telemetry-glossary.md +++ b/docs/telemetry-glossary.md @@ -1061,7 +1061,7 @@ 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. -**What is observable:** cluster overhead is **not** measurable today, and this is a gap in the instrumentation rather than a display problem. The cluster message type is not in the overlay's message-to-category lookup table and none of the fallback branches match it, so every cluster message falls through to the `unknown` category (`src/xrpld/overlay/detail/TrafficCount.cpp`); no code path ever reports the cluster category, even though the `overhead_cluster` name is defined. Consequences: 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, and `unknown_*` is a weaker anomaly signal on a clustered node because it mixes genuinely unrecognized wire types with routine cluster traffic. If this is ever instrumented, volume moves out of `unknown_bytes_in`, so any alert threshold set against that series will need re-baselining. +**What is observable:** the `overhead_cluster_*` series measure cluster overhead in bytes and messages, in and out. The overlay's message-to-category lookup table maps the cluster message type to the cluster category (`src/xrpld/overlay/detail/TrafficCount.cpp`). Only traffic with cluster members counts. A node sends cluster messages only to its members. A cluster message from any other peer is counted as `unknown` instead. A node with no `[cluster_nodes]` has no members, so these series read zero there, and that zero is the true value. **See also:** [Cluster on xrpl.org](https://xrpl.org/docs/concepts/networks-and-servers/clustering)