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:
Pratik Mankawde
2026-08-26 16:17:36 +01:00
parent f13524c93c
commit c531ac569b
2 changed files with 96 additions and 10 deletions

View File

@@ -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 }}

View File

@@ -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,