mirror of
https://github.com/XRPLF/rippled.git
synced 2026-09-27 23:38:08 +00:00
2fad047df0071f4db3346eca22528d1930cf646d
135 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Pratik Mankawde
|
b0cea67aed |
fix(telemetry): address final-review + CI clang-tidy findings
CI's clang-tidy leg flagged eight include-cleaner errors and three misc-const-correctness / readability-convert-member-functions-to-static / modernize-use-designated-initializers issues, all inside WP-B6's own code. Fixed as follows: - `MetricsRegistry.h`: `#include <opentelemetry/metrics/observer_result.h>` for ObserverResult; `observeCacheLockHoldPeaks` is now `static` because it touches neither instance state nor telemetry members. - `SHAMapStoreImp.h`: adds direct includes for `<cstddef>`, `<string_view>` and `<xrpl/telemetry/SpanNames.h>` (the StaticStr provider). `seconds` in `RotationPhase::~RotationPhase` is `[[maybe_unused]]` so a `-DXRPL_ENABLE_TELEMETRY=0` build under `-Werror` keeps compiling. - `SHAMapStoreImp.cpp`: direct includes for `SHAMapStoreSpanNames.h`, `SpanGuard.h`, `SpanNames.h`; `RotationPhase` locals that never call `setAttribute` are declared `const`; `RotationOutcome` uses designated initialisers. Final-review findings (WP-B6-rotation-stall-tracing.md, "What to check when reviewing"): - Panels 74 and 75 on `ledger-sync-health.json` still carried panel 41's description, axisLabel, Source and Keywords copy; rewritten to describe rotation phase duration and cache lock hold respectively. - `consensus_view_change_total` and the `view.change` round-span event were emitted but not registered with the harness. Added the counter to `not_asserted.metrics_excluded` (workload-gated) and annotated the `consensus.round` span note with the event and its two attribute keys. Not fixed (parked, see progress ledger): - The reviewer's second Important finding — a plan/code contradiction on the consensus counter — was based on a misread of the plan; the plan's "Rejected alternatives" table lists a new `TraceCategory::Nodestore` and the getKeys() fix, not the consensus counter. No action. - The Minor note about `sweep()`'s peak including lock-acquire time and `getKeys()`'s not: `sweep()` acquires and releases the lock via a `scoped_lock`, so `noteLockHold` still runs after the release and the numbers are comparable. No action. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Pratik Mankawde
|
9adb6a255d |
test(telemetry): register the rotation spans and stall metrics with the harness
Adds `nodestore.rotate` and its eight phase children to expected_spans.json,
all `optional: true` because the 5-node localhost harness cluster never reaches
`online_delete`. Their parent-child relationships are asserted but skip-marked
so a run without a rotation stays green.
Adds `cache_metrics{metric="treenode_lock_hold_peak_us"|"fullbelow_lock_hold_peak_us"}`
to the asserted sync_diagnostics group -- both are observable and always emit,
even at zero. Puts `rotation_phase_duration_seconds` and `jobq_stall_total` in
`not_asserted.metrics_excluded`; both are workload-gated.
On the Cloud collector, adds an `ottl_condition` policy that keeps any trace
carrying a span whose name matches `^nodestore\.rotate`, so the 0.5% probabilistic
tail sampler cannot drop a rotation trace. Sampler is OR'd across policies.
|
||
|
Pratik Mankawde
|
800662a268 |
corrections
Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> |
||
|
Pratik Mankawde
|
521f00a484 |
merge: bring the lock-free ValidationTracker forward from phase10-workload-validation
Only the workload README conflicted; the tracker, config and test files merged clean, which closes the chain from phase-7. |
||
|
Pratik Mankawde
|
ac07e1345f |
fix(telemetry): name the harness log directories after their instance ids
The collector reads the per-node directory off the log file path and stamps it
as the Loki label service_instance_id, so the directory name has to equal the
node's own [telemetry] service_instance_id or log lines carry a node name that
no trace or metric shares and nothing joins.
Both harness scripts disagreed with themselves: run-full-validation.sh wrote to
node$i while setting validator-${i}, and benchmark.sh wrote to node$i while
setting bench-node-${i}. Rename the directories to match the ids rather than
the reverse, so no existing trace or metric label value moves and no harness
expectation has to be re-checked. Only path references are renamed; the
human-readable "node$i" in log and error messages is left as prose.
The config template is not rendered by any script, so its DATA_DIR
documentation gains a note about the same constraint instead.
Also rename the deprecated otlphttp/filelog collector component names in the
harness scripts and docs.
|
||
|
Pratik Mankawde
|
478b3e4b07 |
docs(telemetry): correct stale claims and citations in the harness docs
The workload README contradicted itself on --skip-loki: one bullet said CI always passes it and so the two log-correlation checks are never exercised, another said the workflow no longer passes it. The workflow mentions the flag nowhere, so the first was the stale half. Other claims checked against the tree and corrected: - both the README and the plan doc described the push trigger as filtered on branch names. The workflow has no branches filter, deliberately, because GitHub ANDs branches with paths - the plan doc printed 6 of the workflow's 12 paths globs, and claimed the workflow was 367 lines against an actual 451. The glob block is now generated from the workflow, and the line count dropped rather than restated - rpcNOT_SUPPORTED does not exist anywhere in the tree. The symbol is RpcNotSupported, and the refusal sites are RipplePathFind.cpp:59-60 and PathFind.cpp:50-51, not :48-49 and :39 - RCLConsensus.cpp:666 and :663 are not log or event lines; the tx.included event is at :720 and the per-transaction debug log at :715 - LedgerMaster.cpp:463 is fixIndex, not the ledger.store span, which is at :470 - ServerHandler.cpp:705 is inside makeJsonError; processRequest is at :718 - file counts: docker/telemetry/workload/ is 25 files, include/xrpl/telemetry/ 13 - the optional-span bullet named five causes covering 10 of 16 entries, omitting the txq.* family and the WebSocket handshake - the /api/v1/series choice was attributed to stale StatsD gauges; this harness runs no StatsD A line number in run-full-validation.sh was cited in five places and drifts on every edit to that file, so those now name the file only. The keygen helper's header records what production does instead -- validator-keys-tool create_keys then create_token, keeping the master key off the node -- and why a disposable cluster does not. |
||
|
Pratik Mankawde
|
ec308c6b00 |
fix(telemetry): poll the parity queries instead of racing one instant query
The four external-parity bounds checks each ran a single Prometheus instant query and failed on an empty result. The metric checks that run earlier poll /api/v1/series, which returns a series regardless of staleness, but a bounds check needs the sample value and so cannot use that endpoint. This file's own docstring records the consequence: a beast::insight gauge that stops changing can fall out of an instant query while /api/v1/series still returns it, so one attempt is not enough to call the series absent. Poll to the same deadline the metric checks use. A Prometheus error is raised rather than retried, because a rejected query never becomes valid and retrying it only burns the full timeout. |
||
|
Pratik Mankawde
|
7f829a5929 |
fix(telemetry): fail the regression gate on a unit change, and report what it gated
compare_to_baseline took the unit from the baseline entry and dropped the current run's, and nothing compared the two, so a us -> ms change was scored as a numeric delta: four keys rewritten to the same physical durations reported 99.9% improvements and the gate exited 0. prom_queries.py says the baseline preserves the unit "so the comparator can sanity-check unit drift"; it never did. A unit mismatch now fails and names both units. The workflow's step summary printed total, regressions and improvements. total is every key in the report -- the union of baseline and current -- so it was neither the baseline count nor what was gated, and missing_in_current was computed and never printed. A run that gated 16 of 20 keys read as a full comparison. The comparator now reports a real "compared" count and the summary prints it beside the not-captured count, with a warning when any key was missed. The table also refused nothing on a truncated report; existence is not readability. check_regression_bounds told the operator to add max_abs_increase while reading max_abs_increase_ms / _us, so following the message added a key nothing reads and the gate kept failing with no explanation. The committed thresholds use only the suffixed spelling, so the message was the defect. Its three JSON inputs were also unchecked: a top-level null, list or number parsed and then died on the first .get, and a string "metrics" survived the placeholder test and reported its own characters as gated keys -- wrong advice rather than a crash. Four tests cover these; all four fail against the previous checker. |
||
|
Pratik Mankawde
|
c8d9d88113 |
fix(telemetry): measure telemetry overhead under load, on this cluster only
The overhead benchmark generated no workload. Each arm was start_cluster -> collect_metrics -> stop_cluster, and collect_metrics only ran the sampler, so the only client traffic was the sampler's own server_info probes at under 1 request/sec. The hottest instrumented paths -- tx.*, txq.*, the transactor stage spans, every rpc.command.* other than server_info -- were never entered, which is where per-operation span cost appears. Both arms now drive rpc_load_generator and tx_submitter at one fixed rate for the whole window, over a [port_ws] listener present in both arms so the listener is not part of the delta. A flat rate rather than a workload profile, because both arms must issue the same work and a profile's phase shaping only adds variance. The sampler also selected xrpld host-wide. run-full-validation.sh leaves its five validation nodes running while the benchmark's three start, so both arms averaged eight processes -- diluting the CPU delta and making memory_rss_mb_peak report a validation node either way. It now takes an optional pid list, and the benchmark passes its own nodes' pids and refuses to measure if it cannot collect them all. consensus_round_mean_ms counted distinct ledger sequences seen by a loop that sampled every 5 s, so it read back 5000 ms for every close time from 2 s to 5 s and a 10% regression measured 0%. Sampling at 2 s -- the close-time floor from ConsensusParms.h:93 -- resolves a 10% regression as at least 9.3%. It also divided by the requested DURATION rather than the measured ELAPSED, which the TPS calculation in the same file already used. Key generation, the workdir setup and the seed read exited 1 under errexit, the code this script reserves for a measured threshold breach, so an infrastructure failure was reported as "telemetry is too expensive". They map to cannot_measure now. No guard is added after the config heredoc: a guard there is read as the heredoc's first line, lands in the generated config and never runs. curl probes across the harness had no --max-time, so a server that accepts the connection and then stops answering blocks forever and the loops' attempt counts stop bounding anything. |
||
|
Pratik Mankawde
|
d2cefa05d9 |
fix(telemetry): make the load generators fail loudly instead of exiting 0
Three ways a run could produce no traffic and still report success: - tx_submitter logged a funding shortfall and returned an empty stats object; main() then printed the summary and exited 0, so the failure only surfaced later as "spans missing", which points nowhere. It now records setup_failed in the summary and exits 1 after the report is written. - --weights was checked for valid JSON but not for a positive sum. An all-zero mapping reached random.choices, which raises ValueError from inside the dispatch loop where only CancelledError is caught. Rejected at parse time now, in both generators. - a profile phase declaring neither rpc nor tx logged a warning and returned no error. Both error rates short-circuit to 0.0 when nothing was sent, so a mistyped key produced zero traffic and still passed the exit gate. That phase is now an error. |
||
|
Pratik Mankawde
|
198207eee4 | merge: bring the close-time attr harness fix forward from phase10-workload-validation | ||
|
Pratik Mankawde
|
f80faea85d |
fix(telemetry): match the renamed close-time attrs in expected_spans.json
The close-time span attributes name their unit and epoch: close_time_ripple_epoch_s, parent_close_time_ripple_epoch_s and close_time_self_ripple_epoch_s. The harness inventory still required the unsuffixed keys, so the attribute checks for consensus.accept.apply and ledger.build failed on every validation run while the spans themselves were correct. Rename the four required_attributes entries to the keys the code emits. |
||
|
Pratik Mankawde
|
3e4b5c71ff |
merge: bring the traces_endpoint rename forward from phase-10
Three conflicts, all between this branch's own sync-diagnostics work and phase-10's older versions. Resolved to this branch in each case, since it owns the newer content: - InboundLedger.h keeps the missing-node and receive-depth gauges and the fuller acquire-span contract. - MetricsRegistry.cpp keeps the namespaced label:: constants. - LedgerMaster.cpp keeps makeLedgerTraceSpan(), which joins the store and validate spans into one per-ledger trace by hash. LedgerMaster.cpp needed a second pass. The automatic merge had kept both sides outside the conflict markers, nesting phase-10's older promotion block inside this branch's `if (!pubLedger_)` — so setValidated, setFull and setValidLedger would each have run twice. Taking this branch's file wholesale removes the duplicate; brace balance and a single "Advancing accepted ledger" confirm it. That resolution drops two things phase-10 was carrying into this file: the storeSpan/validateSpan guard names, and the explicit scope that keeps the one-in-256 flag-ledger check outside the ledger.validate measurement. Both are re-applied on this branch in the next commit; the scope needs a variable-lifetime check that does not belong in a merge. |
||
|
Pratik Mankawde
|
3a63a17548 |
docs(telemetry): stop the harness contract narrating its own revisions
Notes across the workload contract described earlier versions of themselves, or cited commits that only exist inside this chain. A squash merge publishes none of it, so each reference resolves nowhere. Notes that described their own earlier text: - expected_spans.json: 'this note previously concluded', 'this note previously said', 'Un-skipped 2026-08-26', 'the reason had simply gone stale for two weeks' and 'the claim this entry carried' are replaced by the standing reason each entry holds. The wildcard pairs now say the validator globs the child via _span_name_matches(), and state the literal-collapse failure as what a different validator WOULD do rather than as history. - regression-thresholds.json: 'an earlier version of this note wrongly claimed', 'the earlier version oversold it' and 'an earlier note called that' become the cautions themselves -- do not reason from 'every ladder step is at least 2x', do not oversell the backstop, do not read a false fire as a missing override. - test_check_regression_bounds.py: the docstring gives the reason a literal is wrong here, not the story of two tests that once hard-coded one. Baseline-refresh history rewritten as measurement: - README.md, baselines/README.md, telemetry-runbook.md and regression-metrics.json no longer attribute threshold moves to 'the 2026-08-26 refresh'. The evidence is kept as measurement -- span.tx.apply.p50 has read 0.7917 ms and 0.00597 ms on the same workload, 132x apart; job.acceptLedger.running.p95 has measured a 5.74x floor on one baseline and 16.28x on another -- which is what supports the claim that a single-run baseline cannot bound these keys. Two chain-only commit ids removed, |
||
|
Pratik Mankawde
|
fa9f75d4e7 |
docs(telemetry): explain the absent path-finding load without the change story
The harness contract described how path-finding load came to be absent rather than why it is absent. The load exists on no branch before this one, so a squash merge publishes no revision that ever issued it: the 2026-08-25 date resolves nowhere, and 'removing it', 'used to satisfy' and 'has now cleared' compare against a state a reader cannot reach. - README: state that DEFAULT_WEIGHTS carries no ripple_path_find entry, and give the error floor as what WOULD happen if it did, rather than what removing it fixed. 'Putting it back' becomes 'Enabling it'. - expected_spans.json: the pathfind.request and pathfind.compute notes, and both hierarchy skip_reasons, now put the span's presence in the conditional -- the parent would appear if the RPC were issued, because the ScopedSpanGuard at RipplePathFind.cpp:35 sits above the rpcNOT_SUPPORTED guard at :48-49. - expected_metrics.json: the rpc_method_errored_total, pathfind_fast and pathfind_full notes drop the date and keep both independent reasons the metrics stay absent. - regression-thresholds.json: span.ledger.store 'is excluded from' the gated surface rather than 'was removed from' it. The reasoning is unchanged: pathfinding is off because Config.cpp:725-726 zeroes pathSearchMax when [validation_seed] is present, a refused call still exports an error span, and at a 3% weight that is a ~3% STATUS_CODE_ERROR floor. Documentation and JSON note strings only, no behaviour change. |
||
|
Pratik Mankawde
|
a214db3a90 |
docs(telemetry): remove pre-squash and plan-internal references from sync diagnostics
Comments across the sync-diagnostic work described earlier revisions of the same change, or cited identifiers a reader of the merged tree cannot resolve. Prior-state comparisons rewritten in the present tense: - MetricsRegistry.cpp carried two adjacent paragraphs prescribing opposite behaviour for a disabled quorum, one publishing int64 max and one omitting the series. The code omits it; the superseded paragraph is gone and the surviving reason SIZE_MAX must not be cast is kept. - MallocTrim, LedgerMaster, LedgerReplayTask, TransactionAcquire, Application: say what the signal is the only record of, rather than what was 'previously trace-only', 'not logged at all here' or 'used to sit inside if (debug())'. - LedgerMaster.h and SpanGuardScope: without an explicit join each ledger's spans WOULD be separate traces -- not that they were 'before this'. - Handshake: the message is forwarded byte for byte, not 'byte-identical to the previous behaviour', and the helper throws rather than 'throws as before'. - MetricNames: quorum_disabled is a separate boolean rather than a sentinel, stated without what the state 'used to be encoded by'. - LedgerMaster.cpp no longer claims to mirror the unl_quorum gauge; it does not. That gauge omits the series while this stores int64 max. - 'Split out of' / 'Split from' become 'Kept separate from' in five places. Plan-internal identifiers removed: - All 24 WP-Ax / WP-Bx work-package labels across the telemetry tests, the collector configs, tempo.yaml and the expected_* inventories. They are defined in no file in the repo, so they resolve nowhere once merged. - The two references to OpenTelemetryPlan/, which does not reach develop, now point at docs/telemetry-glossary.md 'Fresh-node sync diagnostics'. Comments and JSON note strings only, no behaviour change. |
||
|
Pratik Mankawde
|
22362bc4af |
docs(telemetry): drop pre-squash comparisons from the workload harness comments
Three harness comments described the behaviour this change replaced, which the squash merge does not publish. - run-full-validation.sh: the capture flag means CAPTURE_EXIT is not the only record of capture health; and a gated capture failure is an infrastructure error, stated without 'exactly as before'. - tx_submitter.py: give the reason the first occurrence logs at WARNING (DEBUG is off in CI) rather than what a failed run 'previously produced'. - workload_orchestrator.py: a wedged process cannot stall the profile, rather than 'can no longer'. Comments only, no behaviour change. |
||
|
Pratik Mankawde
|
e9856897ec | Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics | ||
|
Pratik Mankawde
|
0bda9e8953 |
test(telemetry): cover the capture completeness guard and run the harness tests in CI
capture_timings.py decides whether a captured timings file may become a regression baseline. Every way of getting that wrong is silently green: a capture that asked Prometheus for nothing still writes valid JSON, and once accepted it is pasted in as a baseline, still reads as a placeholder, and the regression gate stays off while the workflow reports it as activated. Covered: an empty surface is not complete (0 of 0 is 100% by arithmetic), the minimum ratio is inclusive, null values count as declared but not captured, the threshold is recorded so a rejected capture can be judged later, and the exit code follows the flag rather than recomputing the ratio. The empty case has its own error path because the percentage message divides by the declared count. Neither this file nor test_validate_telemetry.py ran anywhere before: not in CI, not in run-full-validation.sh, not in pre-commit. They now run in the naming job, which is fast and fires on nearly every PR, so a broken harness surfaces in seconds rather than after an xrpld build. They run as plain scripts. unittest discover would collect nothing from them, since they hold bare functions rather than TestCase subclasses, and would exit 0 -- which is why each file fails when it collects no tests. The dependency install is a separate step, placed after every stdlib-only check so those stay reachable if PyPI is unavailable. |
||
|
Pratik Mankawde
|
53cc08aa52 |
fix(telemetry): assert real ancestry in the span hierarchy check
The check reported span.hierarchy.<parent>-><child> and a message reading "Found <child> as child of <parent>" on the strength of both names appearing somewhere in the same trace. A span parented by something unrelated passed, so the one property the check exists to prove was never tested. It now walks the child's parentSpanId chain looking for a span matching the parent name. Ancestry rather than a direct edge, because all 21 declared relationships are worded as the parent containing the child, so a scope appearing in between is a refactor and not a broken relationship. Span ids are compared as opaque strings: both fields come from the same Tempo response and share its encoding, so nothing here depends on whether that is hex or base64. Co-occurrence is still the search filter, which is what lets a conditional child be found in an older trace instead of only the newest ones. Verdicts are separated because they send the reader to different places: a child that is present but not under the parent is a hierarchy bug, a chain running into a span the trace lacks is one that never reached Tempo, and an unusable parent span is neither. A definite negative outranks an indefinite one, and one trace proving ancestry settles the relationship. Tests cover each verdict plus the cross-trace and cyclic-chain cases, and each one was checked against the specific defect it names. The runner now fails when it collects no tests and reports SystemExit, both of which otherwise produce a silent pass. The pathfind.request skip_reason said only the child side handles globs. Both sides do now; the blocker is the literal parent name in the Tempo query, so the skip itself stands. |
||
|
Pratik Mankawde
|
d0eb346ec4 | Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics | ||
|
Pratik Mankawde
|
d423863b82 |
Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics
Two conflicts. RCLConsensus.cpp: upstream restructured makeAcceptSpan so the accept span's attributes sit behind if (*span). This branch's own contribution there is the consensus round-duration histogram, which is kept -- placed inside the telemetry guard but OUTSIDE the span-liveness test, because a metric must still record when the trace category is disabled or the span was not created. The duplicated attribute lines on this side are dropped; the guarded block upstream added supersedes them. MetricsRegistry.cpp: kept this branch's JobQueue.h include, which it uses. Its Journal.h include was dropped as a duplicate -- the file already includes that header higher up, with a comment explaining why it is unguarded, and readability-duplicate-include is fatal under WarningsAsErrors. |
||
|
Pratik Mankawde
|
ed92501730 |
style(telemetry): cut the comments I over-wrote back to the guideline
The comments I added with the hierarchy sampling fix and the trigger change ran to sixteen and twelve lines. The guideline is short and plain English. Rationale, CI run numbers and the list of which relationships were affected belong in the commit message, which is where they already are; inline they push the code apart and go stale as soon as the reasons change. Trimmed the sampling comment from sixteen lines to four, the re-check comment from eight to four, _traceql_name_predicate's docstring from fourteen lines of explanation to three, and the push-trigger comment from twelve to seven. Each keeps what a reader needs at that line -- what the code does and the one non-obvious reason -- and drops the history. Comment-only: 13 insertions against 35 deletions, no statement changed. Left alone deliberately: this file has ten pre-existing comment blocks longer than six lines, including one added recently by another party. Rewriting someone else's comments is not mine to do here, and the guideline is being applied to what I wrote. Verification: 7/7 validator tests pass; validate_telemetry.py compiles; the workflow YAML parses, still carries no branches filter, and still lists 12 paths; otel-naming exits 0. |
||
|
Pratik Mankawde
|
b8cb36ffca |
fix(telemetry): refuse an empty capture, and name a bad baseline entry
An empty metric surface counted as a complete capture. build_query_plan
returns an empty plan without complaining for any config that yields no
gated keys, so pointing --metrics at the wrong file exits 0 and hands the
paste-me path a metrics:{} artifact to offer as the next baseline. Nothing
about such a run is evidence the pipeline works, so declared == 0 is now a
failure rather than vacuously complete.
The bounds checker also raised AttributeError on a baseline entry that is
not an object, instead of naming the key. A validator whose job is to catch
a malformed contract should report it, not crash on it.
Test cleanup is bound to its own temp tree, so a loop no longer leaves five
of six directories behind.
|
||
|
Pratik Mankawde
|
5638cd976e |
fix(telemetry): write the wildcard span predicate without a backslash escape
The conjunction query works. Run 33062418036 proved it on real Tempo: both hierarchies that newest-N sampling made unassertable now PASS -- txq.accept -> txq.accept_tx and ledger.acquire -> ledger.acquire.txtree -- along with every other literal-child pair. Only the two wildcard children failed, and not because of the sampling change. They failed with HTTP 400, "invalid TraceQL query: parse error at line 1, col 68: invalid char escape". _traceql_name_predicate built the pattern with re.escape, giving name=~"rpc\.command\..*", and TraceQL's string lexer refuses a backslash escape it does not recognise -- the query never reached the regex engine at all. A literal dot is now written as the character class [.], which carries no backslash for the lexer to refuse while still meaning a literal dot to the engine behind it. Leaving the dots bare would have parsed, but would match any character in those positions, which is the looseness _span_name_matches exists to avoid. The builder now also rejects a span name containing anything outside lower_snake_case, dots and the glob star, rather than passing it through unescaped. Every name in the contract is of that shape, so this changes nothing today; it exists because the failure mode it guards against is exactly the one above -- a character that means something to one layer and something else to the next, discovered only from a 400 in CI. Worth recording why the tests did not catch this. The stub evaluated the pattern with Python's re, which accepts \. happily, so it modelled the regex engine and not the query lexer sitting in front of it. A stub is only as good as the layer it imitates, and the layer that rejected this was one the stub did not represent. The new test therefore asserts the property the lexer enforces -- that no backslash appears in the predicate at all -- rather than any particular spelling, plus that the pattern still accepts rpc.command.fee and still rejects a near-miss whose separators are not dots. Verification: 7/7 tests pass, and the new one was watched failing first with the exact string Tempo rejected, name=~"rpc\.command\..*"; the full query the check now builds was printed and confirmed backslash-free; validate_telemetry.py compiles. Three unrelated files in this worktree are another party's live work and were left unstaged. |
||
|
Pratik Mankawde
|
39fa18e898 |
test(telemetry): assert the tx-tree acquire hierarchy again
The sampling fix that just merged forward removes the only reason this was skipped. The check no longer inspects the three newest parent traces; it asks Tempo for traces containing both parent and child. Worth recording why this phase was the one that failed while its two siblings passed, because the original assertion treated all three as equivalent and they are not. InboundLedger.cpp opens each phase only when that piece is still needed: header on !haveHeader_ (:672), astree in the else of haveState_ (:689), txtree in the else of haveTransactions_ (:698). A node acquiring a ledger here almost always lacks the account-state tree, so astree opens on essentially every acquire. But it usually already holds the transaction set -- every node sees the same relayed transactions and builds the same set -- so txtree opens on a minority of acquires. The child was always emitting, 5 traces of its own on the run that failed; it just was not in the three most recent acquires. That is now all three sampling-caused skips retired: txq.accept -> txq.accept_tx and this one asserted, and txq.enqueue -> txq.batch_clear narrowed to its real remaining cause, a child that never fires under this workload at all. Contract on this branch: 24 relationships, 19 asserted, 5 skipped, and zero spans declaring a parent without an entry. The five are the two pathfind pairs and the pathfind.request parent (pathfinding disabled and no path-finding RPC issued), rpc.ws_message -> rpc.process (not a code relationship -- rpc.process is a child of rpc.http_request), and txq.batch_clear. None is a sampling artifact. Verification: JSON parses; the four validator tests pass after the merge; 0 unaccounted parentings; counters still 48 span types and 74 unique attributes; otel-naming exits 0. Whether this holds against a live Tempo is what the run this push triggers decides -- the stub proves the query shape, not the corpus. |
||
|
Pratik Mankawde
|
ce18bb3317 |
Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics
Brings in the hierarchy-check sampling fix: the check now asks Tempo for traces containing both parent and child rather than inspecting the three newest parent traces, so a child conditional on a state the workload rarely reaches is found wherever it occurred. Merged clean, no conflicts, no resolution decisions. This unblocks ledger.acquire -> ledger.acquire.txtree on this branch, which was skipped for exactly that sampling problem and is un-skipped in the next commit. |
||
|
Pratik Mankawde
|
a87d772f40 |
fix(telemetry): find a span hierarchy where it happened, not only where it is newest
The hierarchy check searched the parent span and inspected the three newest traces it returned. That is wrong whenever the child is conditional on a state the workload only sometimes reaches: the parent fires constantly, so its newest traces are the ones LEAST likely to carry a rare child. Three relationships had been skipped as unassertable for exactly this, and in none of them was the child missing -- each emitted traces of its own and simply was not in the three most recent parent traces. The check now issues a second query, a TraceQL trace-level conjunction of the parent and child name predicates, and inspects those traces. Tempo searches its whole retention for co-occurrence instead of leaving the answer to which traces happen to be newest. The parent-only query is kept and still runs first, so "the parent stopped being emitted" stays a distinct failure from "the parent is there but the child never co-occurs" -- they mean different things to whoever reads the report, and collapsing them would lose that. The returned traces are still verified with _span_name_matches rather than the query result being trusted on its own. Tempo has already guaranteed co-occurrence, so this is redundant on the happy path; it is kept because it keeps the glob semantics in one place and means a wrongly built query cannot silently pass. _traceql_name_predicate handles the wildcard contracts. TraceQL has no glob operator, so `rpc.command.*` is sent as name=~"rpc\.command\..*" with the dots escaped -- unescaped they would match any character in those positions, which is the looseness _span_name_matches exists to avoid. Two entries follow from the fix. txq.accept -> txq.accept_tx is asserted again: its child is created inside the queued-transaction loop behind `if (feeLevelPaid >= requiredFeeLevel)` (TxQ.cpp:1530) while the parent fires on every close (:1499), which was the whole reason it failed. txq.enqueue -> txq.batch_clear stays skipped but for ONE reason now instead of two -- its child never fires at all under this workload, needing an account with a supersedable batch, so it is purely a workload gap and needs nothing further from the validator. The third, ledger.acquire -> ledger.acquire.txtree, lives on the sync-diagnostics branch and is un-skipped there once this merges forward. Written test-first, and the first test this module has had. The failing test reproduces the exact CI message, "txq.accept_tx not found in txq.accept traces", against a stubbed Tempo whose corpus holds the child only in a trace outside the newest three. Three sibling tests guard the ways this could be "fixed" wrongly: an absent child must still fail, a missing parent must still name the parent rather than the child, and a wildcard child must be satisfied by any family member. The stub records the queries issued, so the conjunction is asserted rather than assumed. A stub rather than a live Tempo because the behaviour under test is which traces the check ASKS FOR -- a passing query against real data proves the data co-operated, not that the query was right. The first run of those tests failed for the wrong reason: my stub's name-predicate regex also matched the resource.service.name="xrpld" term every query carries and so demanded a span literally named "xrpld". Fixed in the stub, with the lookbehind commented as load-bearing, before touching production code. Verification: 4/4 tests pass, and the failing one was watched failing first with the production message; the issued queries were printed and confirmed to contain the conjunction; validate_telemetry.py compiles; expected_spans.json parses; 21 relationships, 16 asserted and 5 skipped; counters still 41 span types; otel-naming exits 0. Three unrelated files in this worktree are another party's live work and were deliberately left unstaged. |
||
|
Pratik Mankawde
|
8521b96d85 |
fix(telemetry): stop an incomplete capture becoming the committed baseline
The regression baseline is bootstrapped by copying a CI artifact. The workflow
tested only that timings.json existed, then printed it verbatim under a heading
inviting the reader to paste it in as the new baseline.
capture_timings.py writes that file and only then enforces --min-capture-ratio,
so an incomplete capture leaves a file that exists but covers fewer keys than
the contract declares. The verdict lived in CAPTURE_EXIT, a shell variable local
to run-full-validation.sh that no other program could read. So on a placeholder
baseline plus a thin capture, CI offered an incomplete artifact as the next
baseline, and pasting it narrowed the gate with nothing reporting that it had.
That is the failure shape this harness keeps producing: a degraded result that
looks exactly like a good one.
The artifact now carries its own completeness, next to metrics:
"capture": { "declared": 20, "captured": 20, "min_ratio": 0.5, "complete": true }
complete is the same condition the producer exits 0 on, computed once with the
exit code read off it, so the flag and the status cannot drift apart. Any
consumer can now tell a complete capture from a thin one, not just CI.
Both paste-me paths refuse rather than warn: the workflow prints the counts and
an error annotation with no JSON, and the comparator explains on stderr while
leaving stdout empty, so a redirect cannot produce a plausible-looking file. A
warning above a copyable block is still a copyable block, and a reader who has
just hit a red gate is already predisposed to re-baseline. A missing capture
block fails closed.
Refusal is scoped to bootstrapping a baseline, not to comparing against one, so
artifacts captured before this change still replay: verified against the run the
current baseline came from, which carries no capture block and still reports 0
regressions. An injected regression is still caught, and the gated surface is
unchanged at 20 keys with 5 excluded.
|
||
|
Pratik Mankawde
|
1f8b69a3f0 |
fix(telemetry): skip the tx-tree acquire hierarchy, which the tx set being present hides
Asserting all three ledger.acquire phase parentings treated them as equally
conditional. They are not. Run 33002568549 failed on
ledger.acquire -> ledger.acquire.txtree -- "ledger.acquire.txtree not found in
ledger.acquire traces", the single failure in 279 checks -- while header and
astree passed. Skipped rather than left red.
Not a missing span: txtree reports 5 traces of its own on that same run, one per
node. InboundLedger.cpp opens each phase only when that piece is still needed:
header on !haveHeader_ (:672), astree in the else of haveState_ (:689), txtree in
the else of haveTransactions_ (:698). A node acquiring a ledger here almost always
lacks the account-state tree, so astree opens on essentially every acquire and its
assertion holds. But it usually already HOLDS the transaction set, because every
node sees the same relayed transactions and builds the same set, so
haveTransactions_ is true and no txtree phase opens at all. It fires only on the
minority of acquires where the set was genuinely missing, and with
_validate_parent_child sampling the 3 newest parent traces
(validate_telemetry.py:803) those are not the ones sampled.
This is the third entry skipped for one underlying cause, after
txq.accept -> txq.accept_tx and txq.enqueue -> txq.batch_clear: a child that is
conditional on a state the harness rarely reaches, met by newest-N sampling of the
parent. Preferring parent traces that CONTAIN the child would retire all three at
once, and that is now the highest-value change left in this harness -- recorded in
each of the three reasons so whoever picks it up finds the whole set.
The rest of the run supports the other changes. Both rpc.command.* hierarchies
un-skipped in
|
||
|
Pratik Mankawde
|
f2cc740c42 |
Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics
Brings phase-10 up to
|
||
|
Pratik Mankawde
|
6d17df083f |
test(telemetry): account for every declared span parenting
Each span entry documents its parent, and a separate list holds the pairs the validator actually checks. Three parentings were declared on the span entries and absent from that list entirely, so they were neither asserted nor recorded as unassertable -- silently missing rather than deliberately skipped. All three are now listed, skipped, each with the reason that actually applies. Every span declaring a parent now has an entry: the count went from 3 unaccounted to 0. txq.enqueue -> txq.batch_clear is conditional and narrowly so. The child is created in TxQ::tryClearAccountQueueUpThruTx (TxQ.cpp:550), which needs one account holding several queued transactions AND an arriving transaction that supersedes the batch. txq-burst produces queueing but arranges no such shape, and it has never been observed on a run. It would also meet the sampling limit that forced the txq.accept_tx skip, so fixing the sampling addresses both at once. rpc.command.* -> pathfind.request is the one skip caused by a wildcard PARENT rather than by a missing span, and the asymmetry is worth recording: _validate_parent_child inserts the parent name literally into its Tempo query (:801), so a wildcard parent matches nothing, while the CHILD side globs through _span_name_matches (:826-828). That is exactly why rpc.ws_message -> rpc.command.* can be asserted and this cannot. pathfind.compute -> pathfind.discover has both ends absent, for the reason the pathfind.compute entry already sets out at length: pathfinding is disabled on every harness node because Config.cpp:725-726 zeroes pathSearchMax when a [validation_seed] is present, and since 2026-08-25 no path-finding RPC is issued either. Listed so the family is fully accounted for rather than partly silent. No assertion is added or removed here -- this is accounting. The plan task that prompted it also assumed the pathfind.compute skip reason was stale and needed correcting; it is not, it already names both blockers and corrects an older liquidity-based reason, so that half of the task was a defect in my plan rather than in the file. Verification: JSON parses; 21 relationships, 15 asserted and 6 skipped; no duplicates; 0 spans declaring a parent without an entry, down from 3; counters still 41 span types; otel-naming exits 0; pre-commit clean. |
||
|
Pratik Mankawde
|
c3e4c4244a |
docs(telemetry): explain why the overlay dial metrics report one fewer series
overlay_connect_total and the three overlay_dial_latency_ms series come back with 4 series per run while sibling families such as dns_resolve_* come back with 5, one per node. That was unexplained, so anyone reading the group had to choose between suspecting the exporter and re-deriving the cause. It is a topology artefact and nothing is wrong. OverlayImpl::connect asks peerFinder().newOutboundSlot for a slot and returns early when it gets a null one (OverlayImpl.cpp:464-470), before it constructs the ConnectAttempt that emits both signals (:472). Every node is seeded to dial every other node -- run-full-validation.sh:324-331 builds IPS_FIXED from all NUM_NODES-1 peers and :373-374 writes it into [ips] -- so all five nodes do try. In a full mesh each pair is dialled from both ends, and the node whose peer got there first is refused an outbound slot for an address it already holds inbound: no ConnectAttempt, so neither the counter nor the histogram. dns_resolve_* reports 5 because reportDnsResolve fires inside the resolver handler (OverlayImpl.cpp:603), which runs before any slot allocation. The note also records why the four entries assert series presence rather than a count: hard-coding 4 would bake today's mesh into the contract and break on any cluster-size change, while gaining nothing -- and it says that fewer than 4 would be worth investigating, since that means a node did not dial at all. Verified in code: the early return and its position relative to the ConnectAttempt, the [ips] construction, and the resolver call site. Not verified against a run: which node is missing on any given run, because dial ordering is not controlled and the identity is not expected to be stable. No assertion changed -- this commit adds documentation only. |
||
|
Pratik Mankawde
|
a215ab7bb1 |
fix(telemetry): assert the rpc.command hierarchies, whose skips described dead code
Both rpc.command.* relationships were skipped on the claim that
_validate_parent_child collapses a wildcard child to one literal name via
child_name.replace("*", "server_info"). That code does not exist.
|
||
|
Pratik Mankawde
|
143abfd8f6 |
fix(telemetry): stop requiring what the harness cannot guarantee
Two entries in the span contract could fail on healthy behaviour. `ledger.serve` is emitted when this node answers another node's request for ledger data, and was mandatory. A node only answers if a peer asks, and a `TMGetLedger` request is constructed in exactly two places -- src/xrpld/app/ledger/detail/InboundLedger.cpp and src/xrpld/app/ledger/detail/TransactionAcquire.cpp -- whose `ledger.acquire` and `txset.acquire` spans are both already marked optional. A mandatory check therefore rested on an optional cause. Marked optional, with the dependency named in the note so it is promoted together with them rather than alone. `peer.dial` covers one outbound connect attempt and required `outcome` and `duration_ms`. Both are set only in `reportOutcome()` (ConnectAttempt.cpp:158-199). The teardown path sets neither on purpose: an attempt destroyed during overlay shutdown, or one whose connect was aborted, ends its span in `~ConnectAttempt` (ConnectAttempt.cpp:89-102), whose own comment states that a span ending with no `outcome` is the honest record of a dial that never concluded. Since `peer.dial` is a freshRoot, each dial is its own trace with exactly one instance of the span, and the validator inspects only the most recent trace -- so one newly-aborted dial fails CI while the node is behaving correctly. `remote_endpoint` stays required; it is set at construction on every path. Verified: both call sites read in current code; `TMGetLedger` construction confined to those two files; the counters recomputed -- 48 span types (matches len(spans)) and 74 unique attributes, unchanged, because `outcome` and `duration_ms` are still required by `txset.acquire` and the `ledger.acquire` family. Not verified: that a run with these entries relaxed still exercises both spans, which only a CI dispatch can show. Neither entry can now fail on healthy behaviour, so a green run proves less than before by design. |
||
|
Pratik Mankawde
|
e5d7b2a4b0 |
fix(telemetry): skip the txq accept-pass hierarchy, which sampling cannot assert
|
||
|
Pratik Mankawde
|
92e988c8b8 |
Merge branch 'pratik/otel-phase10-workload-validation' into pratik/otel-sync-diagnostics
Brings phase-10 up to
|
||
|
Pratik Mankawde
|
504138dd9c |
test(telemetry): assert all three ledger-acquire phase hierarchies
The acquire span opens three phase children -- header, account-state tree and transaction tree -- and the contract documented all three parentings while checking none of them. astree had an entry marked skipped; header and txtree had no entry at all. The skip reason was that the parent is optional, because a healthy 5-node cluster agreeing from genesis rarely back-fills history, so the hierarchy check would fail against a parent with no traces. Run 32969481032 refutes the premise: ledger.acquire reported 5 traces and so did each of the three children, one per node. The parenting was never the uncertain part -- beginPhaseSpan() parents through the acquire span's own captured SpanContext rather than the ambient thread context, so it holds whichever worker opens a phase. Asserting these matters because of what the phases are for. A fresh sync is dominated by the account-state tree, and the flat parent span cannot separate that from the much smaller transaction tree or from the header wait that gates both. If a phase stops nesting under the acquire it still emits, still carries its missing-node count and its timeout flag, and nothing else in this harness notices -- but the trace stops answering which phase the sync is stuck in, which is the whole reason these spans exist. Routed here rather than to phase-10 because phase-10 has no ledger.acquire.header or ledger.acquire.txtree span at all; the phase children were introduced on this branch. Verification: JSON parses; 10 relationships, 6 asserted and 4 skipped, no duplicates, every non-wildcard child resolves to a declared span entry; counters unchanged at 48 span types and 74 unique attributes; otel-naming exits 0; pre-commit clean. Edited by surgical text replacement -- a first attempt used a json.dumps round-trip and reflowed the whole file, 148 insertions against 47 deletions with unrelated compact arrays expanded and unrelated notes rewritten; that was reverted and redone, and the churn is now 11 against 3. Whether a child is findable INSIDE the parent's fetched trace is what CI will decide. |
||
|
Pratik Mankawde
|
c531ac569b |
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. |
||
|
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. |