mirror of
https://github.com/XRPLF/rippled.git
synced 2026-09-27 23:38:08 +00:00
44fd31f7cda9f96b74d7debdf87b66c566c679cb
96 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Pratik Mankawde
|
44fd31f7cd |
Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics
Brings phase-10 up to
|
||
|
Pratik Mankawde
|
f13524c93c |
fix(telemetry): map every harness failure to exit 2, and always capture timings
Two defects raised in review of PR 6519, both about the harness misreporting its own state. The script documents exit 2 for an infrastructure failure and routes that through die(), but eleven commands were unguarded, so under set -euo pipefail a failure aborted with the tool's own status instead. Measured before the fix: docker compose exited 125, the key generator 7, a jq read 5, and several others 1 -- which the table defines as "checks failed", so an infrastructure problem was reported as a validation result. Two of the eleven are worth naming. A trailing option with no value (--nodes at the end of the command line) exited 1 because set -u aborted on the unset positional, now unified through one require_value helper. And report_stopped_nodes, which runs immediately before a die, contained an unguarded pipeline that tripped errexit, so the die never ran and a crashed cluster reported 1 -- the script failed to report the exact condition the contract exists for. Commands whose failure is genuinely tolerated were left alone. The seed read also gained a value check, because jq prints the string "null" and exits 0 for a missing key, so testing only the exit status cannot see it. Step 6 said it "ALWAYS captures timings (so CI always has an artifact from which to bootstrap/refresh the committed baseline)" while the capture sat inside the --skip-regression guard. The comment stated the intent and the code was the bug: that artifact is the only route to a refreshed baseline, and the workflow reads it unconditionally to print the paste-me block. Capture now always runs and only the comparison is gated. A capture failure still surfaces, folding into the exit code only when the gate is active, so --skip-regression cannot start failing runs that previously passed. Note a non-zero capture status does not mean the file is absent: capture_timings writes it and then fails the minimum-ratio check, so the artifact exists but is incomplete. The messages say incomplete rather than missing, so nobody goes looking for a file that is already there. The runbook's matching claims are corrected in the same commit: it said --skip-regression skips the capture, and its exit-code summary predated the uniform mapping. |
||
|
Pratik Mankawde
|
a4fedeceed |
Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics
Brings phase-10 up to
|
||
|
Pratik Mankawde
|
29673de531 |
fix(telemetry): correlate WebSocket replies, and stop silent placeholder data
Eight defects found in review of PR 6519. Twenty-four review threads reported them; ten were duplicates of one another and four were wrong about the code. The two that corrupt data. Both WebSocket clients reuse the socket after a recv timeout, and the library queues the late reply, so the NEXT request reads the previous response. In rpc_load_generator that misattributes latency, and the skew is permanent rather than one-off. In tx_submitter it is worse: a submit that reads an account_info reply freezes that account's sequence number and every later transaction for it fails. Both now correlate replies by request id under a single overall deadline, with a counter rather than a wall clock, since time.time() is not monotonic and collides within a tick. workload_orchestrator never cleared its fixed report paths, so a run that produced no report silently adopted the previous run's totals -- breaking the invariant evaluate_exit_gate documents. Reproduced by planting a stale total and watching it appear in a later summary. collect_system_metrics reported placeholders as if they were measurements. The consensus mean used bc with a || echo 0 fallback that neither warned nor cleared METRICS_COMPLETE, unlike every sibling path; it now uses awk, already a hard dependency here, which removes the failure mode instead of reporting it. Note this moves the mean from truncation to rounding, at most 1 ms on a value of about 45 s. Unmeasurable TPS now warns and clears the flag too. All four curl probes gained a timeout, not just the one the review named -- an unresponsive node could hang any of them. The orchestrator's help text claimed 18-dashboard coverage; there are 15 on disk, 15 uids in the contract, and the profile already said 15. The tx_submitter docstring listed twelve transaction types where ten exist, and claimed issued-currency payments that build_payment never sends. Two suggested patches were deliberately not taken. A recursive delete of the report parent sits in a per-task function and would delete earlier phases' reports mid-run, and recursively remove a caller-supplied --report-dir. The stale microsecond axis label on ledger-data-sync is real but belongs to phase 9, which carries a byte-identical copy of that dashboard, so fixing it here would leave that PR wrong and guarantee a conflict. |
||
|
Pratik Mankawde
|
a734da8b33 |
test(telemetry): recapture the baseline and stop gating what variance dominates
Refreshes baselines/baseline-timings.json from run 32964262700 at |
||
|
Pratik Mankawde
|
6cb02a1b40 |
fix(telemetry): make the Loki diagnostic count real, and Tempo errors visible
Three defects in the harness's own instrumentation, all of the same shape: a failure that reads as an absence. The Loki diagnostic reported "unavailable entries" rather than a count. It issued an unaggregated count_over_time, and because the filelog regex_parser leaves message and timestamp as log-record attributes, Loki's OTLP path turns those into structured metadata, which joins a metric query's label set. The query therefore produced one series per log line and Loki answered HTTP 400, maximum number of series reached. A second bug hid the first: the JSON helper never checked resp.status, so Loki's own explanation arrived as a mimetype complaint instead. Both fixed, in the Python and the shell twin, and verified against a real loki 3.7.6 including a genuine-zero control so that zero stays distinguishable from unavailable. _tempo_search and _tempo_get_trace called resp.json() with no status check, so any non-2xx became "0 traces" or "0 spans" -- the same class of bug as the span.name tag returning 200 with an empty list. A 404 on /api/traces/<id> legitimately means "not indexed yet", so that stays an absence and every other non-200 now raises. log.trace_id_cross_reference queried Tempo once, with no retry, while the metric checks share a poll deadline for exactly this race. It now polls on the existing METRIC_POLL_TIMEOUT_SEC/INTERVAL, so a trace that has not yet been indexed is retried rather than reported missing. The window stays at 4 hours and the assertion is unchanged. |
||
|
Pratik Mankawde
|
3d61ceae6e |
Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics
Brings phase-10 up to
|
||
|
Pratik Mankawde
|
8418d474a7 |
fix(telemetry): read span names from the Tempo name intrinsic
The span reverse-coverage check has never evaluated. It reported "no span
names were reported (backend unreachable or empty)" on a run where Tempo
demonstrably held data -- the same run resolved a logged trace id to 32
spans.
Root cause: the tag-values query asked for `span.name`. A span's name is a
TraceQL intrinsic, not a span-scoped attribute, so `span.name` resolves to
an attribute nothing sets. Tempo answers 200 with an empty tagValues list,
which is indistinguishable from an empty backend and never raises, so the
surrounding try/except stayed silent.
Verified against tempo 2.9.4 holding exactly one span named
probe.reverse.coverage, with the collector in front of it:
/api/v2/search/tag/span.name/values -> {"tagValues":[]}
/api/v2/search/tag/name/values -> that span's name
/api/v2/search/tag/resource.service.name/values -> xrpld
The third line is the control: the span was in Tempo, so the first line's
emptiness was the wrong tag rather than no data. Cross-checked against a
populated Tempo elsewhere, whose span scope lists real attributes
(command, ledger_seq, tx_hash) and no name tag at all, while the bare
intrinsic returns the whole span inventory.
This is pre-existing, not a regression in the reverse check: the same URL
fed the operations diagnostic before that check existed, and the last
green run before it also logged "Tempo operations (0 total)". The check
faithfully reported an empty input; the input was broken.
The neighbouring resource.service.name query is correctly scoped and is
left alone.
|
||
|
Pratik Mankawde
|
493475a9d4 |
Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics
Brings phase-10 up to
|
||
|
Pratik Mankawde
|
394ed2cbc0 |
fix(telemetry): correct the sync-diagnostics harness rationales and gaps
Review of |
||
|
Pratik Mankawde
|
42a72863bb |
fix(telemetry): make the sync-diagnostics metric gate actually assert
The sync_diagnostics group asserted nothing. assert_sync_diagnostics_metrics called _check_prometheus_metric with five positional arguments against a six-parameter signature: `report` landed in `deadline` and `sem` was omitted entirely, so the call raised TypeError before a single metric was queried. Neither run_validation nor main catches anything, so the traceback propagated, run-full-validation.sh recorded the non-zero exit as a validation failure, and the four phases ordered after it -- dashboards, both parity checks and log-trace correlation -- never ran at all. Reproduced directly: TypeError, zero checks recorded. Even with the arity corrected the group would still have passed silently, because _check_prometheus_metric RETURNS its CheckResult rather than recording it and the value was discarded. Both halves are fixed by adopting the fan-out validate_metrics already uses: one shared deadline, a concurrency semaphore, gather, then report.add per result. The same call now records 55 checks where it previously recorded none. With the gate live, the inventory it guards had to be made honest. Two metrics could never have passed it. unl_fetch_total is emitted only from ValidatorSite::reportFetchOutcome, which indexes sites_[siteIdx]; sites_ comes from [validator_list_sites], and the harness writes a static [validators] file with no list site anywhere, so no fetch outcome is ever reported. handshake_negotiation_fail_total needs a rejected handshake, and no reject path was found to be reachable between identical localhost nodes. Both move to not_asserted.metrics_excluded, which is where the file's own description says workload-gated names belong. Eleven further conditional metrics -- the acquire, replay, disconnect, serve, jump and sweep counters -- were documented only inside free-text notes; they move to the same map. That matters beyond tidiness: _accounted_metric_names harvests metrics_excluded keys, so a name recorded only in prose is reported as unaccounted, and a prose note cannot be linted at all. Two metrics were wrongly excluded. rotation_state's callback gates only on dynamic_cast<DatabaseRotating*>, and online_delete=256 is set by both the cfg template and run-full-validation.sh, so SHAMapStoreImp builds a DatabaseRotatingImp, the cast succeeds, and both sub-series are observed on every collection tick. The note claiming the harness could not produce them conflated "no rotation runs" with "no series published"; the first is true and bounds the values, the second is false. Both are now asserted at value 0, where absence rather than the zero is the regression, and the note is corrected. The four new histograms listed only _bucket, or _bucket and _count. Each now lists _sum as well, matching the rpc_method_us and job_queued_us convention, so an exporter regression that drops one series cannot pass. On the span side, ledger.validate and ledger.store are the two ends of the per_ledger trace-join group, and the join is computed by hashing ledger_hash -- yet neither required it. Both spans take it unconditionally from makeLedgerTraceSpan, so requiring it is free, and without it a lost join key surfaces only as "spans landed in separate traces", naming the consequence instead of the cause. Deliberately unchanged: ledger.serve stays required and peer.dial keeps its current required attributes, though both look unsafe -- ledger.serve can only fire if an optional span fires first, and peer.dial's destructor exit sets neither outcome nor duration_ms. Those weaken assertions rather than add coverage, so they are reported rather than changed here. Verification: TypeError reproduced before the fix and absent after, with 55 checks recorded; both JSON files parse; no name is both asserted and excluded and none is duplicated; the declared span counters remain consistent at 48 and 74, proven by injecting an extra attribute and watching the check fail; check_otel_naming.py exits 0, and Rule K was proven to read these entries by injecting a bogus name in an owned family and observing exit 1; pre-commit passes on all three files; the levelization baseline is unchanged. NOT compiled -- no C++ changed. |
||
|
Pratik Mankawde
|
3836078a78 |
fix(telemetry): stop gating ledger.validate p95 and p99, which vary too much
The regression gate has been red on runs with no code change. Only two of the 25 gated keys ever tripped, both on the same span and never together: run 32862589645 failed p99 at 25.8750 ms against a 1.0600 ms baseline (+2341%), run 32867433073 failed p95 at 0.7500 ms against 0.2404 ms (+212%), and in each run the other quantile sat well inside its own bound. A real slowdown would move both. This is variance, not a defect. Measured across four CI runs: span.ledger.validate.p50 0.0484 to 0.0778 ms 1.6x spread kept span.ledger.validate.p95 0.1281 to 0.7500 ms 5.9x spread excluded span.ledger.validate.p99 0.3875 to 25.8750 ms 66.8x spread excluded Both excluded quantiles reach past their trip point on a healthy run. The mechanism is arrival timing, not slow code: the span opens only once a quorum-completing validation arrives (LedgerMaster.cpp:987, inside checkAccept, past the early return) and wraps the promotion work that follows, so one slow consensus round dominates the tail of a 3m rate window and which round that is differs every run. Widening is not available and must not be attempted later: tolerating 25.8750 ms against a 1.0600 ms baseline needs a bound of about 24.8 ms, which gates nothing. A bound admitting every healthy run's worst case admits every regression too. p50 stays gated; it is stable. THE GENERAL RULE, recorded so this does not recur: an absolute bound derived as hi_next minus baseline comes from the histogram ladder, so it budgets for quantization noise and for nothing else. It knows nothing about how far a metric moves between runs on identical code. Before gating any key, check its observed maximum across several runs against its trip point and gate it only with margin. Spread alone proves nothing: tx.apply.p50 swings 364x and never fires, because its 5 ms trip point absorbs the range. Of the 23 keys still gated the worst reaches 0.67 of its trip point. Mechanism: spans.names lists span names while _quantiles is shared, so dropping two quantiles of one span cannot be expressed by deleting a name. regression-metrics.json gains an excluded_keys map from a flat key to the reason it is not gated, subtracted by both prom_queries.py (so the key is never queried) and check_regression_bounds.py rule A. A per-name quantile override was rejected: a typo there leaves the key gating, whereas a typo in an exclusion subtracts nothing and new rule F rejects it, along with an empty reason, a leftover threshold override and a leftover baseline value. Derived figures recomputed from the committed baseline: 25 gated keys to 23, detection floor 2.02x-9.43x to 2.02x-9.42x, weakly guarded keys ten to nine, bound over baseline 102%-843% to 102%-842%. The baseline edit is a deletion of two entries only, with no value rewritten. Verified: both previously failing runs replay to zero regressions and exit 0; a tenfold increase injected into each of the 23 remaining keys in turn is still caught in all 23 cases; rule F was confirmed load-bearing by stubbing it out, which lets a stale exclusion pass. |
||
|
Pratik Mankawde
|
c65cb0e2a8 |
feat(telemetry): gate log-trace correlation in CI with per-leg diagnostics
The two log-correlation checks have never executed in CI: the workflow
hardcoded --skip-loki, so validate_telemetry.py never constructed
log.trace_id_present or log.trace_id_cross_reference. A green Telemetry
Validation therefore carried no evidence that a log line reaches Loki with
trace context. Drop the flag so both checks run and can fail the job.
Correlation spans four independent legs and a failed check names none of
them, so run-full-validation.sh now prints a per-leg diagnostic after the
suite whenever the checks are enabled:
node per-node debug.log line count, the count matching the injected
trace_id/span_id shape, one sample line, and the severity mix,
so "no log at all", "log level too high" and "no active sampled
span" are distinguishable
mount the container-side listing of /var/log/xrpld, taken with the
collector's own mounts and uid. That image is built from
scratch and carries no shell, so the listing runs in a
throwaway container with --volumes-from, not via docker exec
collector the receiver's watched files, logs-pipeline warnings, and the
internal log-record counters, read from inside the container's
network namespace because that endpoint binds to the
container's own localhost and its port is not published
loki the exact query used, the label inventory, and entry counts for
the stream selector with and without the line filter, so "Loki
has nothing" and "Loki has lines but none carry a trace id" are
distinguishable
The diagnostics are non-fatal by construction: every leg runs in its own
subshell with errexit off, each docker and curl call is guarded, and the
coordinator always returns success. Verified with no containers and no Loki
reachable, with an emptied PATH, and with a leg forced to exit non-zero.
validate_telemetry.py gains a matching diagnostic beside the checks,
following _log_prometheus_metric_names: warnings only, never a check
result. Its stream selector and line filter move into module constants
that the shell diagnostic reads back, so the two cannot drift into
describing different queries.
No check was widened or auto-passed, and LOG_QUERY_WINDOW_SECONDS stays at
four hours; a wider window would let a check pass on a previous run's logs.
|
||
|
Pratik Mankawde
|
59a0595a6e |
fix(telemetry): stop the workload harness issuing refused path-finding RPC
Every node the harness starts is a validator, and validators disable pathfinding: Config.cpp:725-726 zeroes pathSearchMax whenever a [validation_seed] or [validator_token] section is present, and run-full-validation.sh writes [validation_seed] into every generated node cfg (:308) with no [path_search] section to put the default back. So doRipplePathFind refused every call at RipplePathFind.cpp:48-49 and the 3% ripple_path_find weight bought no coverage at all. It was not free either. The pathfind.request guard is constructed at RipplePathFind.cpp:35, above that refusal, so each refused call still exported a span, and the enclosing rpc.command.ripple_path_find span carried rpc_status=error. That put a steady 3% error floor into span_calls_total for STATUS_CODE_ERROR: any error-rate threshold derived from harness data before this change was measuring the harness rather than xrpld, and needs re-deriving. Removing the load makes pathfind.request unreachable, so it moves from required to optional in expected_spans.json; without that the span check would fail on every run. Three notes in that file and three in expected_metrics.json made claims that are now false, two of them citing line numbers this commit deletes; all six are corrected. The runbook required/optional count moves 26/15 to 25/16. Two facts a future reader needs. First, the weights previously summed to 103, not 100, so every percentage the docstring stated was wrong: health checks were really 38.8%, not 40%. Dropping the 3 makes the sum exactly 100 and every stated percentage correct for the first time. expected_spans.json also carried live arithmetic off the old total, "25/103 ... roughly 43%", now 25/100 and 42%. Second, baselines/baseline-timings.json was captured WITH this load. Only span.rpc.ws_message p50/p95/p99 of the 25 gated keys sees the RPC mix, and their trip points sit 3.1x to 5.9x above baseline, so the gate will not fire. But a timing baseline is workload-specific and its profile field still reads full-validation, so nothing will flag the drift: refresh it from the next CI run's timings artifact. Pathfinding now has no coverage in this harness at all. The workload README section "Pathfinding is not exercised" records that cost, the manual verification route, and a four-step restore recipe in which steps 1 and 2 alone only reinstate the error floor. |
||
|
Pratik Mankawde
|
c863b83a1c |
feat(telemetry): warn on telemetry the harness contract does not account for
validate_metrics and validate_spans only ever run one direction: read the contract, ask the backend whether each listed name exists. Nothing looked the other way, so a metric family or span name the contract omitted was invisible by construction. Both emitted inventories were already being fetched for the CI log and neither was compared back, which is how a 345 family metric gap and 7 unknown span names went unnoticed. Add two reverse checks, metric.reverse_coverage and span.reverse_coverage. Each names every emitted family the contract never mentions, sorted, one per line, with counts in the report details. Warn only, by design. passed is hardcoded True in a single shared builder, so an unaccounted name cannot turn CI red: downstream branches legitimately add telemetry an upstream contract has not seen yet, and a hard failure would redden all of them for doing the right thing. Bulk families are accounted for declaratively. A new top level accounted_patterns list in expected_metrics.json holds anchored regexes with a written reason each, covering the 105 per job type queue gauges, the 70 per job type histogram families, the 228 overlay per category traffic families, and the Prometheus scrape plumbing that is not xrpld telemetry. Job type shapes are reduced structurally because every job type name lowercases to letters only; traffic categories are enumerated instead, because they contain underscores and a structural pattern there would swallow unrelated names. Anything outside these shapes still surfaces. Exporter shapes are folded before matching, so a histogram triple is accounted for by an entry written for its base family and is never reported as three separate gaps. Spans need no pattern list: the reverse check reuses the same matcher the forward check uses, so a glob such as rpc.command.* covers every command it expands to, and an optional entry still counts as known. Also fix the diagnostic these checks feed on: both emitted lists were logged as a single Python list repr, about 15 kB on one line for 422 families, unreadable and impossible to compare between runs. Both now print one name per line. _metric_check_targets now selects groups by testing that the value is an object, rather than by excluding two key names, so a non group top level key cannot break it. Output is byte identical: 79 metric plus 5 label checks, same names in the same order. |
||
|
Pratik Mankawde
|
7d35eb872a |
docs(telemetry): correct the harness contract's pathfinding and gauge reasons
The assert / do-not-assert decisions in expected_metrics.json and expected_spans.json were all correct, but several recorded reasons were not. Pathfinding is disabled outright on every harness 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 every node and has no [path_search] override, and both handlers return rpcNOT_SUPPORTED before constructing a PathRequest. - pathfind_full_milliseconds no longer claims a probabilistic path, nor prescribes an explicit ledger index, which cannot help: the config gate fires before the ledger parameter is read. - pathfind_fast_milliseconds keeps its hasCompletion argument but now leads with the config gate, which is the operative blocker. - The pathfind.compute and pathfind.discover notes and the pathfind.request to pathfind.compute skip reason no longer blame missing liquidity. pathfind.update_all now records why its request list stays empty. - statsd_gauges states the arming precondition: a beast gauge is only as safe as an observable gauge when its object exists before Application.cpp:1570, where onCollectionReady arms the registered gauges exactly once. - Alert wiring claims softened: every rule in rules.yaml is paused. - The per job type gauge group loses its bogus poll bandwidth reason, and its regex claim is corrected: there is no running state regex, so 30 of those gauges have no consumer at all. - overlay_peer_disconnects has one query consumer, not two. - Cloud dashboard copies dropped from consumer counts: that tree is ignored by git and has no tracked files. - rpc_method_errored_total explains that a refused RPC is a normal return, not a throw, so the refusals above do not make it fire. - 09-data-collection-reference.md no longer claims a Prometheus name query in expected_metrics.json. No behaviour change: the flattened check name list is byte identical. |
||
|
Pratik Mankawde
|
638ab2f4f5 |
test(telemetry): log every emitted metric family, not 19 prefixes
_log_prometheus_metric_names exists to make name mismatches between expected_metrics.json and actual emissions visible in CI logs, but it kept only names matching 19 hard-coded prefixes. On the last CI run that showed 147 of 422 families, and none of the prefixes covered state_accounting_*, node_family_*, overlay_peer_disconnects or the pathfind_* histograms, so the coverage gap the preceding commit closes could not be seen through it at all. An allow-list can only ever surface names someone already thought to look for, which is the opposite of what a discovery aid has to do, so the filter is removed rather than extended. The whole list is a few kilobytes of CI log. Sorted, so two runs' output can be diffed directly; the Prometheus API promises no order. |
||
|
Pratik Mankawde
|
8547adb5c1 |
test(telemetry): close the 20-metric harness coverage gap
Dashboards and alert rules reference 186 metrics; the harness asserted 57. Excluding the 107 per-category overlay-traffic expansions, the meaningful gap was 20 names. This closes it under the contract file's own doctrine: assert only what the workload guarantees, and record the rest with a precise reason. Asserted 18, taking the metric checks from 61 to 79 and the whole metric phase from 66 to 84. No pre-existing check name or position changes. statsd_gauges gains the nine state_accounting_* siblings of the one member already asserted, plus the two NodeFamily full-below-cache gauges and overlay_peer_disconnects. All twelve rest on one mechanism the group description now spells out: on the OTel path a beast gauge is an Int64ObservableGauge, every instance self-registers in its constructor, onCollectionReady arms all of them unconditionally, and the armed callback Observes on every export cycle whether or not set was ever called, so the series exist at 0. The state_accounting family is set in one unconditional block in NetworkOPsImp::collectMetrics, and full_transitions is the input to the NodeStateFlapping alert rule, so the alert's own signal had been going unverified. A new job_queue_per_type_gauges group asserts the six per-job-type gauges that a panel or a rule names literally, jobq_manifest_waiting among them as the ManifestJobQueueConvoy rule's input. The description records why those six and not all 105: the guarantee is identical for every non-special job type, so the discriminator is consumer coverage, and the remaining names are only reached through topk queries over the family that do not depend on any single type being present. Recorded four more in not_asserted rather than asserting them. pathfind_fast_milliseconds is unreachable for this workload, not merely rare: reportFast fires only from the doCreate fast pass, which is guarded by !hasCompletion(), and both ripple_path_find entry points construct the request with a completion function. Only the path_find subscription reaches it, and the generator does not use it. pathfind_full_milliseconds is reachable but only one ledger close after the request, through PathRequestManager::updateAll, and nothing in the harness arranges or checks that, so the guarantee is probabilistic. warn_total and drop_total are resource-manager meters gated on a consumer crossing the warn or drop threshold; their rpc-pathfinding panels are correct and render empty only because the condition has not occurred, which is worth stating because both were briefly mis-read as phantoms. Runbook check counts updated to match. |
||
|
Pratik Mankawde
|
e4926f55be |
fix(telemetry): derive workload gate bounds from the bucket above the baseline
The gate could not catch a regression on any sub-millisecond span. compare_to_baseline.py requires both the percentage and the absolute bound to breach, and every span shared one flat absolute bound of 10 ms (15 ms for p99) calibrated for a 5-25 ms band the spans do not occupy. Against the baseline captured on 2026-08-24, where 18 of the 28 quantiles gated at the time sat below 1 ms, that bound sat 1.15x to 2000x above the metric it guarded, so the AND never fired: a 100x regression injected into span.ledger.store.p95 reported 0 regressions and exit 0. Injecting a 10x regression into each key in turn was caught on only 5 of 28. Give every gated key its own absolute bound, equal to the distance from its baseline to hi_next, the edge above the top of the bucket the baseline sits in. The trip point is then exactly hi_next, so the gate fires only once the reading clears the bucket above the baseline's own. That is the property a multiple of the enclosing bucket width cannot provide: after the quantile crosses hi, the interpolation happens across the next bucket, which on this ladder is up to eight times wider, so no multiple of the enclosing width bounds the excursion. Measured with a model-free reachability test, a single bucket crossing can produce a false regression on 2 of 25 keys under the old flat bound and 0 of 25 under this rule. The smallest catchable regression is 2.02x to 9.43x per key. The job queue bound had the same shape of problem on three of its four keys (42x, 47x, 220x before). Defaults now sit at each ladder floor, leaving the percentage bound operative for a metric that somehow reaches them. Drop span.ledger.store from the gated surface. Its captured quantiles were 0.005, 0.0095 and 0.0099 ms, which is the ladder's 0.01 ms floor times the quantile: every sample lands under 10 us, so the reported value does not move even if each store slows from 2 us to 9 us. No bound can gate it. Presence is still asserted by expected_spans.json and the integration test, and the rate is still on the ledger-operations dashboard. Add check_regression_bounds.py, wired into the same workflow step as the bucket parity check. It fails when a bound is not the one its own baseline implies, when a gated key has no override, when the baseline and metric surface disagree, when the percentage bound would become operative, and when a baseline carries the ladder floor signature. This gate has now broken three times through the same drift between ladder, baseline and bounds, so documentation alone is not enough. compare_to_baseline.py is unchanged: its existing per-metric override mechanism already expresses all of this. A missing, unreadable or malformed input makes that check exit 1 naming the input, rather than reporting success without having checked anything; only a placeholder baseline, the documented bootstrap state, still exits 0. Its own tests cover both halves of that contract plus one case per rule, and run in the workflow before the check so a broken rule reads as a broken rule. |
||
|
Pratik Mankawde
|
c4b8df9de1 |
test(telemetry): recapture span baselines on the current ladder
Copied verbatim from the timings.json produced by the telemetry-validation
run at
|
||
|
Pratik Mankawde
|
6a82fc6f37 |
docs(telemetry): qualify the log-correlation guarantee and record the CI gap
The runbook said a correlated log line was "guaranteed" at info severity. Info is necessary but not sufficient. Replaced the flat claim with the four real preconditions, each with the code that enforces it and the failure mode it produces: telemetry enabled, trace_consensus=1, a valid roundSpanContext_ (SpanGuard::childSpan returns a null guard on an invalid parent), and a valid plus sampled span context (Log.cpp gates injection on IsValid and IsSampled). Also noted which harness cfgs satisfy them -- run-full-validation.sh and integration-test.sh set all three config keys; benchmark.sh deliberately stays at warning and runs no correlation check. Second, the two checks this work exists to make pass are not exercised by CI. The workflow hardcodes --skip-loki, and validate_telemetry.py builds log.trace_id_present and log.trace_id_cross_reference only inside an "if not skip_loki" branch, so they are never constructed rather than merely skipped, and never appear in the report. No workflow runs integration-test.sh either, so its own check_log_correlation() never runs in CI. Recorded that in the runbook's CI workflow section and in the workload README, with the local command that does cover it: run-full-validation.sh without --skip-loki. The workflow itself is unchanged on purpose. Dropping the flag would make CI exercise Loki ingestion and filelog mounting for the first time on the same run that must produce a clean regression baseline, so a red result would not be attributable. |
||
|
Pratik Mankawde
|
9c89419929 |
docs(telemetry): correct two miscounted facts in dashboard and metric docs
RPC Response Size on rpc-pathfinding said its p95 was computed "over the dashboard rate interval", but the query hardcodes [5m]. Every other panel on that dashboard with a hardcoded [5m] -- RPC Response Time, RPC Response Time Distribution, both Pathfinding duration panels, the gRPC latency panel and Pathfinding Compute Duration -- says "over 5 minutes". Matched the clause to the query. The same edit was applied to the local grafanacloud copy so the two stay identical; that tree is gitignored, so it is not in this commit. The io_latency group in expected_metrics.json claimed "all 6 panels that query it". That 6 was a raw string-occurrence count over the dashboards and included two panel descriptions. Verified truth: two distinct panels query the metric, ledger-data-sync "I/O Scheduler Latency p95" and node-health "I/O Latency", each mirrored in a grafanacloud copy, plus one alert rule in grafana/provisioning/alerting/rules.yaml. Stated that instead of a count. |
||
|
Pratik Mankawde
|
bca2b35bf0 |
docs(telemetry): stop showing inert [insight] prefix on the OTel path
|
||
|
Pratik Mankawde
|
4c33ffb9ca | docs(telemetry): retire the rpc_size instrument-mismatch warning | ||
|
Pratik Mankawde
|
98ba282854 |
fix(telemetry): make the integration test correlate by construction, record the baseline log-level coupling
Three related follow-ups to running the workload at info. integration-test.sh has its own log-trace correlation check that the workload validator knows nothing about: check_log_correlation() greps each node's debug.log for "trace_id=<hex> span_id=<hex>" and fails when it finds none, then cross-checks a sample id against Tempo. At warning it had no guaranteed source. The only warn-or-worse statement inside the activated accept scope is RCLConsensus.cpp:671, which fires solely when a transaction throws, so the check was passing incidentally -- helped by scanning whole files with no time window. Raising it to info gives it the same guarantee the workload now has: the consensus accept pair, one branch of which fires every accepted round. Safe here because this script captures no latency baseline, so there is nothing for the extra log I/O to contaminate. baselines/README.md now records that the committed baseline is only valid at the log level the harness generates. Logging is synchronous and several gated spans contain log statements -- ledger.build has BuildLedger.cpp:81, and consensus.accept has RCLConsensus.cpp:655/663/686 with :663 logging once per transaction -- so the configured level is part of the measurement. Moving it inflates or deflates the quantiles the gate reads without ever reporting a regression, because the baseline moves with it. Changing the level therefore requires re-capturing the baseline. benchmark.sh keeps warning and keeps prefix=xrpld, and now says why. It measures telemetry overhead as a delta between a telemetry-off and a telemetry-on arm, so extra synchronous log I/O would inflate both arms and the thresholds gate the result. The comment exists to stop a future reader "aligning" it with the workload harness and quietly degrading the measurement. |
||
|
Pratik Mankawde
|
ff8629bb11 |
fix(telemetry): drop the inert insight prefix and its false comment
Every generated node cfg carried prefix=xrpld under a comment claiming it
"matches the OTel resource service name and the metric names the dashboards
query". Both halves are false.
Verified inert before removing: CollectorManager.cpp reads the key on the OTel
path and passes it to OTelCollector::New, but the only use of prefix_ anywhere
in OTelCollector.cpp is the startup log line. formatName() -- the single funnel
for every instrument name -- only lowercases the name and turns dots and
spaces into underscores; it never reads prefix_. So exported names carry no
prefix at all. expected_metrics.json's own description records this ("Metric
names have no prefix (the xrpld_ prefix was removed)") and 488 live metric
names confirmed it: jobq_job_count, rpc_requests_total, total_bytes_in.
A reader trusting the comment would look for xrpld_jobq_job_count and find
nothing.
The replacement comment states what is true and checkable: the collector
declares no statsd receiver (its metrics pipeline is [otlp, spanmetrics],
confirmed in otel-collector-config.yaml), so beast::insight must export over
OTLP for system metrics to reach Prometheus at all; server=otel is the only
load-bearing key; exported names carry no prefix.
Metric names, series and dashboards are unchanged. The one observable
difference is the OTelCollector startup log line, which now prints an empty
prefix.
Also updated workload/README.md, which repeated the same prefix=xrpld claim
and would have been left describing a cfg key that no longer exists, and made
the template header state the sync obligation explicitly -- nothing reads that
file, so nothing catches it drifting from the cfg the runner generates.
|
||
|
Pratik Mankawde
|
2097293e8f |
fix(telemetry): run the workload at info so log-trace correlation is testable
The two log.trace_id_* checks have failed on every run -- they were the only failures in the 2026-08-20 run (158/160). The workload never satisfied their precondition, because warning suppressed the one line that is correlated by construction. trace_id is injected in Log.cpp from RuntimeContext::GetCurrent(). Severity does not affect injection, but JLOG filters on severity before format() runs, so what matters is which severity emits a line while a span is current. A span becomes current in either of two ways: as a ScopedSpanGuard, or by activating a plain SpanGuard via activate() / activateIfLive(). activate() returns a ScopedActivation holding an otel_trace::Scope built from the span, which pushes onto the same RuntimeContext store Log.cpp reads. A plain SpanGuard that is never activated makes no span current. The guaranteed correlated line at info is the consensus accept pair at RCLConsensus.cpp:736/740 -- an if/else, so exactly one fires on every accepted round. doAccept activates the accept span as ambient over its whole body at :565 via activateIfLive(acceptSpan), and that activation lives to the end of the function, so both branches are inside it. At roughly one round every 4 s this gives dozens of correlated lines per run, well inside the validator's 4 h window. LOG_QUERY_WINDOW_SECONDS stays at 4 h deliberately -- a wider window would let the check pass on logs from a previous run. info is the minimum that works, which is what the task asked for. debug would correlate strictly more, additionally covering BuildLedger.cpp:81 and RPCHandler.cpp:188, but it is the wrong default: it puts synchronous log I/O inside ledger.build, consensus.accept (RCLConsensus.cpp:663 logs per transaction) and tx.apply, which are exactly the spans whose latency regression-metrics.json gates. The next run reprints the voided baseline, so capturing at debug would bake log I/O into the latency numbers permanently -- the same class of defect this plan exists to remove. The runbook records how to get the broader coverage per partition, after a baseline exists. |
||
|
Pratik Mankawde
|
c5829df68f |
feat(telemetry): record which consensus rounds requested a tx-set fetch
A tx-set fetch carried no key tying it to the consensus round that needed the set, so attributing a stalled fetch to a round meant guessing from timestamps. One fetch is wanted by many rounds -- it is keyed by set hash, survives the round sweep, and the round never blocks on it -- so a single parent, link or attribute cannot describe the relationship. Instead the fetch span records one timestamped event per requesting round, carrying the round's parent-ledger hash and the ledger it is building. Both attribute keys already existed in the shared telemetry namespace with exactly this meaning, and the existing addEvent API is used as-is, so no new telemetry surface is added and the whole feature compiles out with telemetry disabled. The event fires once per round rather than once per peer proposal, keyed on the round's parent-ledger hash: that distinguishes rounds started on different forks at the same height, which a ledger-height compare cannot. A mid-round wrong-ledger recovery re-enters consensus without re-caching the round identity, so a fetch begun after that switch is attributed to the pre-switch round; the limitation is documented where the values are cached. Also fixes the fetch span's end time, which depended on when the C++ object was destroyed. Three of the four exits that stop pursuing a fetch -- the set arriving from elsewhere, the round sweep, and shutdown -- ended the span only via the destructor, so the recorded duration included however long any reference happened to be held. Each exit now ends the span itself, plus cancel() and container teardown, and the destructor asserts the span is already closed rather than closing it: a fallback that can never legitimately fire should fail loudly instead of hiding a missed exit. An abandoned fetch also no longer asks peers for a set nobody wants, which previously led to charging those peers for answering our own request. Verification: pre-commit and TIDY=1 clang-tidy pass; levelization is unchanged. NOT compiled -- the branch is blocked by a gcc-15 internal compiler error in the unrelated xrpl.libxrpl.rdb unity translation unit. Runtime behaviour is unasserted: xrpl_tests links only xrpl.libxrpl, so TransactionAcquire is unreachable from GTest; the added tests cover the new span-name and attribute constants only. |
||
|
Pratik Mankawde
|
2ae47c66aa | fix(telemetry): assert ios_latency and correct the rpc_size rename attribution | ||
|
Pratik Mankawde
|
2afae6655d | test(telemetry): assert xrpl_node_id reaches the metric series | ||
|
Pratik Mankawde
|
14badbfdd7 | docs(telemetry): record rpc_size_bytes and the other unasserted histograms | ||
|
Pratik Mankawde
|
d1b80e47a2 | test(telemetry): void span baselines captured on the old span ladder | ||
|
Pratik Mankawde
|
7c70e142e9 |
Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics
# Conflicts: # src/xrpld/telemetry/MetricsRegistry.cpp |
||
|
Pratik Mankawde
|
1282645289 |
test(telemetry): invalidate job-queue baselines captured on the old ladder
The workload harness gates regressions on histogram_quantile over job_queued_us / job_running_us, so re-cutting the microsecond ladder changes what those queries return and the stored baselines no longer describe the same measurement. baseline-timings.json's job.acceptLedger.queued.p95 was 96.79us, which is 0.95 / 0.9926 x 100 -- the old 100us bucket edge scaled by the quantile, with 99.3% of samples beneath it. It was never a latency. Keeping it would make the gate LESS sensitive rather than more: a genuine regression from a real 40us to 90us would still sit under 96.79us + 50% and pass. Removes the four job.* entries and records why, including their values. The comparer reports a metric absent from the baseline as "new metric (not in baseline)" and skips it, so the span baselines stay live and gating continues for everything unaffected. is_placeholder() still returns False, so this does not disable the gate wholesale. Recapture the job.* numbers on a node running the re-cut ladder. Also corrects _bucket_note in regression-thresholds.json. It described the spanmetrics ladder as 15 edges starting at 1ms; the collector config has 20, including five sub-millisecond edges. The note's own reasoning was void too -- it justified the 10ms absolute span bound as "~2 low-end bucket widths", but the low-end bucket width is 0.01ms, not 5ms. The bound is kept and justified on the band where span quantiles actually sit, rather than on a derivation from a ladder that no longer exists. |
||
|
Pratik Mankawde
|
3a458f2873 | Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics | ||
|
Pratik Mankawde
|
4b017dbade |
fix(telemetry): make the log-trace correlation checks meaningful
Both checks selected on {job="xrpld"}. Loki's OTLP ingestion promotes
service.name to the label `service_name` and keeps a `job` attribute as
structured metadata, which a stream selector cannot match, so the selector
returned zero streams whatever had been ingested. The collector config and
TESTING.md already say to select on `service_name`.
Invert the cross-reference. Picking an arbitrary trace from Tempo and
expecting it in Loki fails even when correlation works, because a log line
carries a trace_id only when emitted inside a sampled span and most spans
log nothing at `warning` level. Start from a logged trace_id instead and
resolve it in Tempo, which is the invariant worth asserting, and try every
id found so one unexported trace does not fail the check.
Bound the log queries in time. Nothing here set start/end, so every query
relied on Loki's one-hour default and returned nothing when re-run later to
investigate a result.
|
||
|
Pratik Mankawde
|
cb88a12883 |
Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics
Nine conflicts, resolved as follows. src/xrpld/app/ledger/detail/InboundLedger.cpp -- kept this branch's version. phase10 sets the span's outcome/timeouts/peer_count attributes inline at each exit; this branch replaced that with the idempotent finalizeAcquireSpan(), called on all four exits (init, done, give-up, destructor). Taking phase10's blocks would have set the outcome twice against a helper documented as not overwriting what the real exit recorded. phase10's comment explains why peer_count must not be read in a destructor; the helper solves that structurally by taking std::optional<std::size_t> and being passed std::nullopt from there. src/xrpld/telemetry/MetricsRegistry.cpp -- kept metric::ledgerEconomy over phase10's "ledger_economy" literal. This branch added the naming check that requires constants for converted families, so the literal would regress it. Took phase10's comment cleanup. src/xrpld/telemetry/MetricsRegistry.h -- kept registerRotationStateGauge(), which only exists here, and took phase10's removal of the stale task-number comment. validate_telemetry.py -- combined both. phase10 replaced serial metric polling with a concurrent fan-out on one shared deadline, because 58 metrics x 45 s of additive timeout overran the CI budget; that is kept. Its target list filters on SKIPPED_METRIC_GROUPS rather than the two literals it hardcoded, so the sync_diagnostics group stays owned by assert_sync_diagnostics_metrics() instead of being polled and reported twice. Both SYNC_DIAGNOSTICS_GROUP and METRIC_POLL_CONCURRENCY are needed and both are kept. check_otel_naming.py -- both sides extend the rule docstring. Took phase10's fuller Rule E text (doc discovery, allow-dotted markers) and re-appended rules I/J/K/L, which exist only here. expected_metrics.json -- the two sides add disjoint sibling groups, so both are kept: sync_diagnostics alongside node_health_gauges, overlay_reduce_relay, overlay_overflow, validation_lifetime_counters and not_asserted. Both dashboard uids are kept, giving 16 asserted uids against 16 dashboards on disk. expected_spans.json -- kept this branch's span set, a superset that adds the acquire phase spans, ledger.serve, txset.acquire and peer.dial, and expands ledger.acquire's required attributes. Took phase10's description, which documents what the totals mean, and its note on how the RPC wildcard span is created. total_span_types and total_unique_attributes are recomputed for the union: 48 and 74, since each side's figure counted only its own spans. Docs: took phase10's more accurate wording on what the dashboard check actually covers, and corrected the dashboard count from 15 to 16 where the merge made it stale. Verified: no conflict markers remain, both JSON contracts parse, both Python files compile, asserted dashboard uids match the dashboards on disk exactly, and the OTel naming check reports all layers consistent. |
||
|
Pratik Mankawde
|
b7167e5568 |
fix(telemetry): emit valid JSON for sub-1 TPS, and stop double-reporting a span
Two defects reported against the harness, both confirmed. The TPS field was computed with `bc` at scale=2, and bc omits the leading zero: it prints ".25", not "0.25". A bare ".25" is not valid JSON, and this was the normal case rather than an edge case — ledgers close every few seconds, so ledger-advance over elapsed-seconds is well under 1 for any realistic window. It survived earlier checks because those piped the file through jq, which accepts the malformed form; Python's json rejects the whole file. awk's %.2f always pads, so the field is now produced with awk. Audited the other numeric fields at the same time: CPU average and memory peak already used awk, and the p99, sample count and consensus mean are integers, so TPS was the only one affected. Separately, a failing attribute fetch was reported under the span's own check name, which had already recorded the trace as found. That produced two entries for one name, one passing and one failing, inflating the check total and blaming the trace-existence check for a failure in a later network call. The fetch now carries its own error handling and reports under `span.attrs.<span>`, matching where its successful counterpart reports. It moved into a helper rather than growing `validate_spans`, which was already well over the line limit. |
||
|
Pratik Mankawde
|
040a75dc46 |
ci(telemetry): report why a node stopped instead of waiting on a corpse
Three consecutive validation runs timed out at Step 3 with nodes stuck at "unreachable", and the reason was not recoverable from the logs. The node logs showed the failing nodes stopping at an identical point, immediately after JobQueue initialisation and before the debug log is opened, with no error text at all. The harness knew each node's pid and never used it, so a crashed node was indistinguishable from a slow one. The readiness loop now checks whether each node process is still alive and fails as soon as one is not, instead of waiting out the remaining window and burying the cause under two minutes of progress output. Liveness is not a bare `kill -0`: an exited-but-unreaped child keeps its pid, so a zombie answers `kill -0` and reads as alive for the whole window, which is exactly how a crashed node came to look like a slow one. On failure each stopped node reports its wait status and the tail of its stdout. The status is the discriminator that was missing: 137 for a SIGKILL, 139 for a segfault, 134 for an abort, anything below 128 for a deliberate exit. stdout is printed inline rather than left to the artifact upload, because a node that dies before its debug log opens writes nothing else and a cancelled run uploads nothing at all. This is instrumentation, not a fix. The failure is not attributable to the recent changes on this branch: the first red run touched only the two Python files used at Steps 4 and 5, both of which run after this gate, and the same harness passed 5/5 twice before that. |
||
|
Pratik Mankawde
|
bf5c3e328c |
ci(telemetry): make a cluster bring-up failure diagnosable
A validation run timed out at Step 3 with only 4 of 5 nodes proposing, and the reason was unrecoverable afterwards. Two gaps caused that. The node-log artifact collected `node*/debug.log` but not `node*/stdout.log`. A node that dies before its log sink opens never writes a debug.log at all, so stdout is the only place its reason survives — and that file is written by the harness and read by nothing, so it went to the runner and was discarded. The failing node's log was simply absent from the artifact. The readiness loop also fetched each node's `server_state` and threw it away, reporting only a count. "4/5 nodes proposing" says a node is missing but not which one, so there is nothing to grep for even once the logs are kept. The timeout now names each node that is not proposing along with the state it last reported, distinguishing a node that answered with a non-proposing state from one whose RPC port did not answer at all. Neither change affects a healthy run: the accumulator resets each attempt and stays empty while every node is proposing. |
||
|
Pratik Mankawde
|
aea0422562 |
fix(telemetry): count only rendered panels, and drop no-reply latencies
Two defects reported against the validation harness. Both premises were correct, but neither suggested fix was, so the remedies differ. Dashboard panel count: `len(dashboard["panels"])` treated Grafana row objects as panels and skipped the panels nested inside collapsed rows, so every dashboard was over-reported by between 1 and 10 (`log-derived-insights` read 41 against a true 31). The check also passed unconditionally on HTTP 200, so a dashboard that renders nothing would still pass. `_leaf_panel_count` now walks row children and the result gates the verdict. Gating on the old top-level length, as suggested, would not have caught the case it was aimed at: a dashboard made only of collapsed rows counts its rows and reports a positive number while rendering nothing. RPC latency percentiles: `LoadStats.record` appended a latency for every outcome, including requests that never got a reply, where the value is a time-to-failure rather than a round trip. A timeout contributed the full receive timeout, and at the error rate a real run shows this reported p95 and p99 of 10000 ms where the true figure was 5 ms. `record` now takes an optional latency and the timeout path passes none. The suggestion to append only on success was not adopted: a reply carrying `status: error` is a completed, timely round trip whose latency is a genuine measurement, and discarding it would throw away real data. `per_command` is now keyed off the request counts rather than the latency map, so a command whose every request timed out still appears in the report instead of vanishing from it, and each entry carries a `latency_samples` count. |
||
|
Pratik Mankawde
|
2770dbbf11 |
docs(telemetry): drop the legacy daemon name from the workload README
The rename check rewrites a bare pre-rename binary name in any processed doc, which turned the sampler's selector description into "against xrpld or xrpld". Describe the fallback without spelling the legacy token. |
||
|
Pratik Mankawde
|
d059f21bf3 |
fix(telemetry): address review findings in the workload validation harness
Fixes the review findings on this PR that belong to files it owns, plus several defects found while verifying those fixes. Findings in files owned by upstream branches are routed there and left untouched here. Correctness: - tx_submitter: advance the account sequence only on results that actually consume one (tes*, tec*, terQUEUED). tem*/tef*/tel* never reach the ledger, so advancing left a permanent gap that every later submit from that account inherited. Add a re-fetch hatch so a repeated non-consuming failure cannot livelock on the same sequence, and gate the account check on funded-ness rather than list length. - validate_telemetry: filter spans by name before collecting attributes, so a per-span attribute contract can no longer be satisfied by a sibling span; require exact name equality for non-wildcard children and glob matching for wildcards; bounds-check every returned series instead of only the first. - collect_system_metrics: select xrpld by argv[0] rather than a substring match on the whole command line, which averaged in unrelated processes and reported their RSS as xrpld's. Count genuine 0.0 CPU readings, use a clamped nearest-rank p99 index, and record RPC latency only on success. - benchmark: return each verdict through a named variable instead of a command substitution, so the pass/fail counters survive and the exit gate can fire. Scale before dividing in the percentage math, which truncated a 1.26% impact to 1.00% and cleared a 1% threshold. - compare_to_baseline: fall back to the absolute bound when the baseline is not positive, so a 0 -> 500 ms jump is no longer "within bounds". - rpc_load_generator: bound each connection to one in-flight recv(), drain in-flight requests before closing, use a nearest-rank percentile, and report delivery shortfall so an under-delivered run cannot pass with a 0% error rate. Fail loudly instead of silently: - run-full-validation: treat a consensus timeout and a missing validated ledger as fatal infrastructure errors, and fold the orchestrator and benchmark exit codes into the final status. A degraded cluster previously ran a full validation pass and reported misleading downstream failures. - collect_system_metrics: warn per empty measurement source, emit metrics_complete, and exit non-zero instead of substituting zeros that pass every threshold. Require GNU date with %N rather than falling back to a per-sample python3 fork that costs more than the threshold it is measured against. - benchmark: distinguish "could not measure" from "exceeded thresholds", install a cleanup trap so a failure cannot leak nodes and ports, and report an unusable baseline as inconclusive. - workload_orchestrator: bound subprocess communicate() and fail the exit gate on per-phase errors. Also pins the workload compose images to the versions the sibling stack already uses, hash-pins the Python dependencies, restricts the validator config template to loopback, corrects the dashboard and metric counts in the reference docs, drops a span from the regression gate that cannot fire under a WebSocket-only workload, and narrows the teardown pkill pattern so it no longer matches processes that merely mention the work directory. Verified with a full harness run against a local five-node cluster: 158 of 158 checks passed with no regressions detected. |
||
|
Pratik Mankawde
|
d3ff79121d |
fix(telemetry): assert histogram metrics by their exported names
The Telemetry Validation workflow failed with three "0 series" checks: rpc_method_us, job_queued_us and job_running_us. All three are Histograms, and the Prometheus exporter emits a histogram only as the _bucket/_count/_sum triple -- the bare instrument name is never a series, so validate_metrics() could never match it. Evidence from the failing run (31804450127): its own metric-name dump lists rpc_method_us_bucket/_count/_sum and no bare rpc_method_us, while the sibling counters recorded in the same function bodies passed with 100 and 67 series. capture_timings.py, which queries job_queued_us_bucket and job_running_us_bucket, returned real values for the acceptLedger job type in that same run. Every one of the 10 histograms present exposes the full triple, so all three suffixes are safe to assert. Name them the way the exporter does, matching what the spanmetrics group above already does for span_duration_milliseconds and what regression-metrics.json and the job-queue dashboard already query. The metrics stay in their asserted groups because they are genuinely unconditional, so `not_asserted` would be wrong. |
||
|
Pratik Mankawde
|
22e440aee1 |
fix(telemetry): correct the phase-10 validation harness against the code
The harness manifests asserted things the code cannot produce and missed most of what it does. Two assertions were failing every run, and the metric set covered 16 of the ~41 emitted names. expected_spans.json: rpc.process was required with rpc.ws_message as its parent, but it is created only in ServerHandler::processRequest() on the HTTP path, so a WebSocket-only workload never produces it -- it is now optional and parented to rpc.http_request, and the rpc.process -> rpc.command.* edge is skipped with the real reason instead of a coroutine-context-loss diagnosis that was never the cause. Adds the missing rpc.ws_upgrade span, corrects four parents (consensus.mode_change, pathfind.request, and update_positions/check, which are children of consensus.establish rather than consensus.round), and demotes conditionally-set attributes out of required_attributes so a healthy run stops failing. Counts recomputed from the file: 41 span types, 62 unique required attributes. expected_metrics.json: 16 -> 52 asserted entries across the job-queue, RPC method, reduce-relay, overflow and validation families, plus the fifteenth dashboard uid. Metrics the harness workload cannot exercise -- erroring RPC, ledger-mismatch, TxQ overflow, and the lazily-created getobject_* instruments -- are listed in a not_asserted group the validator skips, rather than as assertions that would fail on a healthy node. The workflow's push trigger listed two globs matching nothing (include/xrpl/basics/Telemetry*.h, src/xrpld/app/misc/Telemetry*), so no C++ telemetry change ever triggered validation. Replaced with the paths the code actually lives in, including src/libxrpl/beast/insight/** for the insight export path the harness depends on. The four inert workflow_dispatch inputs are now labelled UNUSED rather than looking like working knobs. Docs: the workload README described a StatsD dirty-flag mechanism under a member name that does not exist, on a code path the harness never uses -- it sets [insight] server=otel, so gauges export through an observable-gauge callback every cycle. Adds the missing txq-burst phase, reconciles three different dashboard counts, and drops "posts summary to PR", which the workflow has no permission to do. The runbook's phase-10 section loses the last sampling_ratio reference (not a config key), gains a Regression Gate and CI subsection covering the gate that can fail CI, and its compose-logs command now names the workload compose file. cmake --preset default is left for a separate change: no CMakePresets.json is tracked, so it is wrong everywhere it appears. Also drops the dead exporter=otlp_http key the harness wrote into every node config, and stops capture_timings.py defaulting --profile to a profile that does not exist. |
||
|
Pratik Mankawde
|
de6b20f44c |
feat(telemetry): render peer disconnect reason and rate on Peer Quality
Add two Peer Quality panels reading peer_disconnect_total: Peer Disconnect Rate, the per-second teardown rate per node, and Peer Disconnects By Reason & Direction, the per-interval increase split by cause and by which side opened the connection. Both sit in the existing Disconnects & Connection Mix row beside Resource Disconnects, which counts only the resource-charge subset and carries no reason label. The Ledger Sync Health board already shows the same split as a window total, so it says how much of each reason but not when. These give the time-shaped view, letting a reason spike be lined up against a stall. Add disconnect_reason and disconnect_direction template variables for the two new label dimensions and wire both queries to them, so the panels filter on every dimension their series carry. Update the 09 reference panel column and the _a7_note panel list to name the panels that now render this counter. |
||
|
Pratik Mankawde
|
de3c9725a5 | Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics | ||
|
Pratik Mankawde
|
817c773162 |
fix(telemetry): make a failed validation run explain itself
Two gaps meant the last failure produced no evidence of its cause. The node-log upload was gated on `if: failure()`, but the validation step sets continue-on-error, so the job is not failing at that point and the condition never fired. Every failed run silently skipped the one artifact that records why a node did not reach consensus. It now keys on the validation step's own outcome, and also collects the harness logs. Transaction failures were logged at DEBUG, which CI does not enable, so a run where all 3052 submissions failed on a refused connection reported nothing about it. The first occurrence of each distinct failure kind is now a warning and repeats stay at DEBUG, so one refused connection says so once instead of 3052 times. |
||
|
Pratik Mankawde
|
b9497d05da |
fix(telemetry): correct the dial-outcome diagnosis and harden the site label
Adversarial validation of the previous commit found one of its two code fixes
was diagnosed wrongly and the other incomplete. Both are corrected here, along
with the layers the first pass missed.
1. The new dial outcome was named for the wrong condition. It was added as
`duplicate` on the belief that PeerFinder had already granted a slot for the
address. It has not: `Logic::onConnected` contains exactly ONE false-returning
path and it is the self-connect check, which logs "Logic dropping as self
connect" (include/xrpl/peerfinder/detail/Logic.h). The duplicate check lives
in `newOutboundSlot`, evaluated before a ConnectAttempt exists, so a real
duplicate can never reach this branch.
That mattered beyond the name: the previous commit told operators the outcome
was benign churn to ignore, when it actually reports a local misconfiguration
-- this node has its own address in [ips_fixed] or behind its advertised
endpoint, and every dial to it is wasted. Renamed to `self_connection`,
reusing the slug `handshake_negotiation_fail_total` already publishes for the
same fault so it reads identically on both signals, and every description
corrected to say so. The fail() string now reads "Self connection" too.
The first pass also missed three enforcement and contract sites: the
ConnectAttempt.h Doxygen state machine (which still mapped the slot branch
onto tls_fail), the LedgerSpanNames unit test (which pinned exactly five
values over a std::array<..., 5> and so left the new member untested), and the
span-derived twin panel plus two reference docs that still published the old
five-value domain.
2. The credential-free site label was incomplete twice over.
- It appended the port, and `Resource::Resource` DEFAULTS that to 443/https
and 80/http when the config omits one. The label would have become
`https://vl.ripple.com:443/` where Grafana Cloud currently holds
`https://vl.ripple.com`, silently renaming the series for every deployment
already scraping this metric. Verified against live label values before and
after; the port is now omitted.
- parseUrl's path group is `(/.*)?`, greedy to end of string, so a query or
fragment lands inside `path`. A list URL authenticated by `?token=...` would
have leaked exactly as userinfo did. The path is now truncated at the first
'?' or '#'.
Also updated the MetricNames.h usage example, which still taught the raw-URI
pattern to the next author, and the 09-doc row that described the label as the
configured URI.
3. Rule J hardening from the same review: `classify_instrument_kind` returns an
`other` sentinel for a non-factory macro, and storing it in the kind set could
render a future conflict as "created as counter and other". The sentinel is
now skipped, keeping it doing what it already did -- matching no shape rule.
Added a second regression test whose input the pre-fix code reported as CLEAN
(gauge-then-histogram on a `_us` name), so the guard is proven by a 0-vs-1
difference and not only by a changed message. Both new tests were run against
a reconstructed last-wins implementation and both fail against it.
Documented the conflict class in the Rule J rows of the checker README and
CONTRIBUTING, which previously described only the suffix conventions.
Verified: naming checker exits 0 with Rule J passing all 40 real names; 140
checker tests pass; 15 dashboards validate; both workload JSON files parse;
clang-tidy over the full compile database reports no finding on any changed line
of ConnectAttempt.cpp or ValidatorSite.cpp; pre-commit passes.
Not verified: not compiled. The label change adds string truncation and the
outcome rename touches a constexpr used across three translation units, so CI's
build remains the first real check on both.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
||
|
Pratik Mankawde
|
f649670ef7 |
fix(telemetry): address the PR review findings
Four defects from the automated review on PR #7875, each verified against the current tree before fixing (one further comment, the row-63 dashboard overlap, was already fixed by an earlier commit and needed nothing). 1. Rule J could not detect an instrument-kind mismatch. instrument_kinds() wrote `kinds[wire] = ...`, so a wire name created through two different factories kept only the kind visited last and whichever emit site the file walk reached last silently decided the verdict. It now collects a set per name and reports the conflict itself -- one name exporting two instruments is the defect, and no suffix can be correct for both. Added a regression test that builds a name as both a counter and an observable gauge and asserts the message names both. 2. A duplicate connection was reported as `tls_fail`. The TLS handshake had in fact succeeded; PeerFinder simply already held a slot for that address, which is ordinary churn on a healthy node. Conflating the two made a rising `tls_fail` unreadable -- it could mean unreachable peers or merely a busy PeerFinder, and those need opposite responses. Added a distinct `duplicate` outcome and carried the widened vocabulary through every place that enumerates it: the panel description, both filter descriptions, the runbook branch table, the runbook outcome list and the expected_spans note. The `dial_outcome` template variable is a label_values() query, so it picks the new value up on its own. 3. ConnectAttempt::onShutdown had no `operation_aborted` guard, unlike the five other handlers in the same file. A clean teardown was therefore counted as `upgrade_fail`, inflating that outcome on any node shutting down with dials in flight. 4. ValidatorSite used the raw configured URI as a Prometheus label. [validator_list_sites] accepts credentials in the URI and ParsedUrl keeps them in username/password, so a configured `https://user:pass@host` would have copied the secret into a metric label and on into the collector, Prometheus and every dashboard. The label is now rebuilt from scheme, host, port and path -- everything needed to tell one site apart, and nothing more. Verified: naming checker exits 0 with Rule J still passing all 40 real instrument names; its unit tests now number 139 and all pass; 15 dashboards validate; both workload JSON files parse; clang-tidy over the full compile database reports no finding on either changed .cpp; pre-commit passes. Not verified: not compiled. Item 4 introduces string concatenation and item 2 a new constexpr, so CI's build is the first real check on both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |