mirror of
https://github.com/XRPLF/rippled.git
synced 2026-08-27 09:10:56 +00:00
fix(telemetry): trigger the workload by what changed, and assert the span tree
Two problems, both about coverage this workflow claims to have and does not. The push trigger gated on branch NAME as well as path, and GitHub ANDs the two. Branch names are not something this repository controls, so a push to any branch outside "pratik/otel-phase*", "feature/otel-*" or "feature/telemetry-*" was never dispatched -- not queued, not skipped, no run to look at. That is not a theoretical gap: two rounds of harness fixes on pratik/otel-sync-diagnostics produced no signal at all before anyone noticed the workflow had never started. The branches filter is removed; the paths already express the real question. The path list was also incomplete in a way that matters more than it looks. The span-name and metric-name headers are the wire contract this harness asserts against by literal string, and the convention colocates each one with the class it serves -- so eight of the ten *SpanNames.h headers live under consensus/, overlay/, app/ledger/, app/main/, app/misc/, rpc/ and tx/, none of which was matched. Renaming a span constant therefore compiled clean, emptied the assertions and triggered nothing. Matched now by filename, "**/*SpanNames.h" and "**/*MetricNames.h", so future headers are covered wherever they land. Added for the same reason: include/xrpl/beast/insight (the interface headers decide what the collector can publish, so they move the metric surface as surely as the implementation), src/tests/libxrpl/telemetry (the GTests pinning those constants), and the two checker directories that gate this surface in CI. Second, the span hierarchy. Each span entry documents its parent, and a separate list holds the pairs the validator actually checks in Tempo. Those had drifted apart: 18 parentings were documented, 7 were checked. A span that stops nesting under its parent -- which is what a detached guard does -- leaves every span and every attribute intact, so no other check in this harness notices; the trace simply stops being readable as one operation. Eleven pairs are added, each one where both ends emitted on a real run: rpc.http_request -> rpc.process, the three txq parentings, and seven consensus ones under consensus.round and consensus.establish. Fourteen of eighteen are now asserted; the four still skipped are the wildcard rpc.command.* families and pathfind.compute. Three notes were also factually wrong, all repeating one mistake. They said rpc.process and rpc.http_request cannot appear because that path is HTTP-only while the load generator is WebSocket-only. The premise is right, the conclusion is not: both appear on every run, five traces each, because run-full-validation.sh polls each node's HTTP port with curl for readiness and validated-ledger progress (:449, :502). Those polls take the HTTP path. A reader acting on the old text would have gone looking for a way to make the harness speak HTTP that it already speaks. The rpc.process -> rpc.command.* skip reason inherited the same error and additionally claimed the WebSocket equivalent is "asserted above instead", which it is not -- that one is skipped for the same wildcard limitation. All three now state the real blocker, which is that _validate_parent_child resolves a wildcard child to a single literal probe. Both HTTP spans stay optional rather than being promoted: the curl polls are harness scaffolding, not workload, and a future change to how the script waits for a node could legitimately remove them. Verification: JSON parses; 18 relationships, no duplicates, every non-wildcard endpoint resolves to a declared span entry; counters still 41 span types and 62 unique attributes; workflow YAML parses, has no branches key, keeps workflow_dispatch, and every new glob was checked against the tracked file list with a matcher that reproduces GitHub's ** semantics; otel-naming exits 0; pre-commit clean on both files. The eleven new assertions are proven only to the extent that both ends emitted on run 32969481032 -- that a child is findable INSIDE the parent's fetched trace is what CI will now decide.
This commit is contained in:
44
.github/workflows/telemetry-validation.yml
vendored
44
.github/workflows/telemetry-validation.yml
vendored
@@ -57,23 +57,53 @@ on:
|
||||
default: false
|
||||
|
||||
push:
|
||||
branches:
|
||||
- "pratik/otel-phase*"
|
||||
- "feature/otel-*"
|
||||
- "feature/telemetry-*"
|
||||
# No branches filter, deliberately. Branch names are not something this
|
||||
# repository controls, so gating on one decides whether telemetry gets
|
||||
# validated by what a branch is CALLED rather than by what it CHANGED. The
|
||||
# previous list ("pratik/otel-phase*", "feature/otel-*",
|
||||
# "feature/telemetry-*") silently excluded every other name, and because
|
||||
# GitHub ANDs the branch and path filters the effect was total: pushes to
|
||||
# pratik/otel-sync-diagnostics matched the paths below but not the branch
|
||||
# glob, so this workflow was never dispatched there at all -- not queued,
|
||||
# not skipped, no run to look at. Two rounds of harness fixes on that branch
|
||||
# produced no signal before anyone noticed. The paths below already express
|
||||
# the real question, which is whether a change can affect telemetry.
|
||||
#
|
||||
# Keep these globs pointing at paths that actually exist. Two earlier
|
||||
# entries (include/xrpl/basics/Telemetry*.h, src/xrpld/app/misc/Telemetry*)
|
||||
# matched zero tracked files, so a pure C++ telemetry change never
|
||||
# triggered this workflow on push — only edits under docker/telemetry/**
|
||||
# or to this file did. The telemetry sources live in the three telemetry
|
||||
# module directories below.
|
||||
# or to this file did.
|
||||
paths:
|
||||
# This workflow, and the harness it runs.
|
||||
- ".github/workflows/telemetry-validation.yml"
|
||||
- "docker/telemetry/**"
|
||||
# The telemetry modules themselves.
|
||||
- "include/xrpl/telemetry/**"
|
||||
- "src/libxrpl/telemetry/**"
|
||||
- "src/libxrpl/beast/insight/**"
|
||||
- "src/xrpld/telemetry/**"
|
||||
# beast::insight, whose gauges and counters this workflow asserts on.
|
||||
# Both halves: the interface headers decide what the OTel collector can
|
||||
# publish, so editing one moves the metric surface as surely as editing
|
||||
# the implementation does.
|
||||
- "include/xrpl/beast/insight/**"
|
||||
- "src/libxrpl/beast/insight/**"
|
||||
# The span-name and metric-name constants. These are the wire contract the
|
||||
# harness asserts against by literal string, so a rename here compiles
|
||||
# cleanly and silently empties the assertions. Matched by filename rather
|
||||
# than directory because the convention colocates each header with the
|
||||
# class it serves: they live under consensus/, overlay/, app/ledger/,
|
||||
# app/main/, app/misc/, rpc/ and tx/, not under a telemetry/ directory.
|
||||
- "**/*SpanNames.h"
|
||||
- "**/*MetricNames.h"
|
||||
# The GTests that pin those same constants.
|
||||
- "src/tests/libxrpl/telemetry/**"
|
||||
# The checkers that gate this surface. otel-naming derives the legal
|
||||
# attribute and metric names from the headers above; the telemetry scripts
|
||||
# enforce the histogram-bucket parity and regression-bound rules the
|
||||
# harness depends on.
|
||||
- ".github/scripts/otel-naming/**"
|
||||
- ".github/scripts/telemetry/**"
|
||||
|
||||
concurrency:
|
||||
group: telemetry-validation-${{ github.ref }}
|
||||
|
||||
@@ -25,7 +25,7 @@
|
||||
"required_attributes": [],
|
||||
"config_flag": "trace_rpc",
|
||||
"optional": true,
|
||||
"note": "HTTP-only. Created solely in ServerHandler::processRequest() (ServerHandler.cpp:705), which is reached only from processSession(Session, coro) (ServerHandler.cpp:646) — the HTTP/JSON-RPC path that roots rpc.http_request at ServerHandler.cpp:640-641. The WebSocket path (processSession(WSSession, coro, jv), ServerHandler.cpp:467) never calls processRequest, so this span cannot appear under the WebSocket-only harness workload."
|
||||
"note": "HTTP-only. Created solely in ServerHandler::processRequest() (ServerHandler.cpp:705), which is reached only from processSession(Session, coro) (ServerHandler.cpp:646) — the HTTP/JSON-RPC path that roots rpc.http_request at ServerHandler.cpp:640-641. The WebSocket path (processSession(WSSession, coro, jv), ServerHandler.cpp:467) never calls processRequest, so this span never appears under a WebSocket request. It does still appear under this harness, 5 traces on a normal run, because run-full-validation.sh polls each node over HTTP with curl (:449, :502) and those requests take the HTTP path. Corrected 2026-08-26; this note previously concluded the span cannot appear at all, which sent a reader looking for a way to make the harness speak HTTP that it already speaks."
|
||||
},
|
||||
{
|
||||
"name": "rpc.command.*",
|
||||
@@ -42,7 +42,7 @@
|
||||
"required_attributes": ["request_payload_size"],
|
||||
"config_flag": "trace_rpc",
|
||||
"optional": true,
|
||||
"note": "HTTP/JSON-RPC root span. The harness load generator is WebSocket-only, so this does not fire."
|
||||
"note": "HTTP/JSON-RPC root span. It DOES fire under this harness, 5 traces on a normal run -- one per node -- even though the load generator is WebSocket-only, because run-full-validation.sh polls each node's HTTP port with curl for readiness and validated-ledger progress (:449, :502). Corrected 2026-08-26; this note previously said it does not fire. Kept optional rather than promoted to required because those polls are harness scaffolding rather than workload: a future change to how the script waits for a node could remove them without anything being wrong with the node."
|
||||
},
|
||||
{
|
||||
"name": "tx.process",
|
||||
@@ -403,7 +403,7 @@
|
||||
"child": "rpc.command.*",
|
||||
"description": "Processing span contains per-command span (HTTP/JSON-RPC path only)",
|
||||
"skip": true,
|
||||
"skip_reason": "Real relationship, but unreachable here: rpc.process only exists on the HTTP/JSON-RPC path and the harness load generator is WebSocket-only, so there are no rpc.process traces to check. The WS-path equivalent (rpc.ws_message -> rpc.command.*) is asserted above instead."
|
||||
"skip_reason": "Real relationship, skipped for a VALIDATOR limitation, not for absence: _validate_parent_child() resolves the wildcard child to one literal probe name, so it cannot assert a family whose members vary per request. Corrected 2026-08-26 -- the earlier reason claimed there are no rpc.process traces to check because that span exists only on the HTTP/JSON-RPC path while the load generator is WebSocket-only. The premise is right and the conclusion is wrong: rpc.process DOES appear, 5 traces on a normal run, because run-full-validation.sh polls each node's HTTP port with curl for readiness and validated-ledger progress (:449, :502). Those curl calls are what produce rpc.http_request and its rpc.process child, entirely independently of the load generator. Anyone acting on the old reason would have gone looking for a way to make the harness speak HTTP, which it already does. That same reason also said the WS-path equivalent is 'asserted above instead'; it is not, rpc.ws_message -> rpc.command.* is skipped for this identical wildcard limitation. To assert either, teach _validate_parent_child to accept a wildcard child by matching any span whose name has the declared prefix."
|
||||
},
|
||||
{
|
||||
"parent": "ledger.build",
|
||||
@@ -426,6 +426,62 @@
|
||||
"description": "Pathfind request contains the compute sub-span",
|
||||
"skip": true,
|
||||
"skip_reason": "Real relationship (pathfind.compute is created inside PathRequest::doUpdate at PathRequest.cpp:749-750, under the pathfind.request scope), but the child never exists on the harness because pathfinding is disabled on every node: Config.cpp:725-726 zeroes pathSearchMax whenever a [validation_seed] or [validator_token] section is present, run-full-validation.sh writes [validation_seed] for all five nodes (:308) with no [path_search*] override, and doRipplePathFind returns rpcNOT_SUPPORTED at RipplePathFind.cpp:48-49 before constructing a PathRequest. The parent used to appear anyway, because its ScopedSpanGuard is created at RipplePathFind.cpp:35, above that guard; since 2026-08-25 not even the parent appears, the ripple_path_find load having been removed from rpc_load_generator.py, so both ends of this relationship are now absent. Liquidity has nothing to do with it — the earlier 'no liquidity, returns before computing' reason was wrong, because no path search is attempted at all. Asserting this relationship needs the load restored AND a [path_search_max] override (or a non-validator node) in run-full-validation.sh."
|
||||
},
|
||||
|
||||
{
|
||||
"parent": "rpc.http_request",
|
||||
"child": "rpc.process",
|
||||
"description": "The HTTP request span contains the request-processing span. ServerHandler::processRequest() opens rpc.process as a scoped child and its own comment names rpc.http_request as the parent; processRequest has exactly one caller. Both ends emit on every run: the harness polls each node's HTTP port with curl, so this is the one rpc hierarchy that is assertable without a wildcard."
|
||||
},
|
||||
{
|
||||
"parent": "tx.process",
|
||||
"child": "txq.enqueue",
|
||||
"description": "Transaction processing contains the queue-admission span."
|
||||
},
|
||||
{
|
||||
"parent": "txq.enqueue",
|
||||
"child": "txq.apply_direct",
|
||||
"description": "Queue admission contains the direct-apply path taken when the transaction is applied straight to the open ledger instead of being queued."
|
||||
},
|
||||
{
|
||||
"parent": "txq.accept",
|
||||
"child": "txq.accept_tx",
|
||||
"description": "The queue's accept pass contains the per-transaction accept span."
|
||||
},
|
||||
{
|
||||
"parent": "consensus.round",
|
||||
"child": "consensus.phase.open",
|
||||
"description": "A consensus round contains its open phase."
|
||||
},
|
||||
{
|
||||
"parent": "consensus.round",
|
||||
"child": "consensus.establish",
|
||||
"description": "A consensus round contains its establish phase."
|
||||
},
|
||||
{
|
||||
"parent": "consensus.round",
|
||||
"child": "consensus.proposal.send",
|
||||
"description": "A consensus round contains the proposals this node sends during it."
|
||||
},
|
||||
{
|
||||
"parent": "consensus.round",
|
||||
"child": "consensus.ledger_close",
|
||||
"description": "A consensus round contains the ledger close it performs."
|
||||
},
|
||||
{
|
||||
"parent": "consensus.round",
|
||||
"child": "consensus.mode_change",
|
||||
"description": "A consensus round contains any mode transition that happens inside it."
|
||||
},
|
||||
{
|
||||
"parent": "consensus.establish",
|
||||
"child": "consensus.update_positions",
|
||||
"description": "The establish phase contains each position-update pass."
|
||||
},
|
||||
{
|
||||
"parent": "consensus.establish",
|
||||
"child": "consensus.check",
|
||||
"description": "The establish phase contains each consensus-reached check."
|
||||
}
|
||||
],
|
||||
"total_span_types": 41,
|
||||
|
||||
Reference in New Issue
Block a user