From ed9250173092a52b2543d74cf4fa6436897e6e9e Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:46:17 +0100 Subject: [PATCH] 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. --- .github/workflows/telemetry-validation.yml | 22 +++------ .../telemetry/workload/validate_telemetry.py | 48 +++++-------------- 2 files changed, 19 insertions(+), 51 deletions(-) diff --git a/.github/workflows/telemetry-validation.yml b/.github/workflows/telemetry-validation.yml index fc8b98efaf..e5b20de529 100644 --- a/.github/workflows/telemetry-validation.yml +++ b/.github/workflows/telemetry-validation.yml @@ -57,23 +57,13 @@ on: default: false push: - # No branches filter, deliberately. Branch names are not something this - # repository controls, so gating on one decides whether telemetry gets - # validated by what a branch is CALLED rather than by what it CHANGED. The - # previous list ("pratik/otel-phase*", "feature/otel-*", - # "feature/telemetry-*") silently excluded every other name, and because - # GitHub ANDs the branch and path filters the effect was total: pushes to - # pratik/otel-sync-diagnostics matched the paths below but not the branch - # glob, so this workflow was never dispatched there at all -- not queued, - # not skipped, no run to look at. Two rounds of harness fixes on that branch - # produced no signal before anyone noticed. The paths below already express - # the real question, which is whether a change can affect telemetry. + # No branches filter, deliberately: GitHub ANDs branches with paths, so a + # branch glob decides validation by what a branch is CALLED rather than by + # what it CHANGED, and a non-matching name gets no run at all. The paths + # below already ask the real question. # - # Keep these globs pointing at paths that actually exist. Two earlier - # entries (include/xrpl/basics/Telemetry*.h, src/xrpld/app/misc/Telemetry*) - # matched zero tracked files, so a pure C++ telemetry change never - # triggered this workflow on push — only edits under docker/telemetry/** - # or to this file did. + # Keep these globs pointing at paths that exist -- two earlier entries + # matched zero tracked files, so C++ telemetry changes never triggered. paths: # This workflow, and the harness it runs. - ".github/workflows/telemetry-validation.yml" diff --git a/docker/telemetry/workload/validate_telemetry.py b/docker/telemetry/workload/validate_telemetry.py index 551c168c3f..3a37e46795 100644 --- a/docker/telemetry/workload/validate_telemetry.py +++ b/docker/telemetry/workload/validate_telemetry.py @@ -427,19 +427,13 @@ def _otlp_span_attr_keys(span: dict[str, Any]) -> set[str]: def _traceql_name_predicate(expected_name: str) -> str: """Build the TraceQL `name` predicate that selects a contract span name. - A literal contract name becomes an equality test. A glob becomes a regex - test, because TraceQL has no glob operator: `rpc.command.*` is sent as + A literal name becomes an equality test. A glob becomes a regex, since + TraceQL has no glob operator: `rpc.command.*` is sent as `name=~"rpc[.]command[.].*"`. - A literal dot is written as the character class `[.]` rather than as `\\.`, - and that is not a style choice. TraceQL's string lexer rejects a backslash - escape it does not recognise, so the re.escape spelling this replaced -- - `name=~"rpc\\.command\\..*"` -- came back as HTTP 400, "invalid TraceQL - query: parse error at line 1, col 68: invalid char escape". `[.]` carries no - backslash, so nothing reaches the lexer that it can refuse, while still - meaning a literal dot to the regex engine behind it. Leaving the dots bare - would parse but match any character in those positions, which is the - looseness _span_name_matches exists to avoid. + Dots use the `[.]` character class, not `\\.`: TraceQL's string lexer + rejects an escape it does not recognise, so `\\.` returns HTTP 400. Bare + dots would parse but match any character there. Args: expected_name: Span name or glob from expected_spans.json. @@ -890,33 +884,17 @@ async def _validate_parent_child( ) return - # Then ask Tempo for traces containing BOTH, and inspect those instead of - # the newest parent traces from the query above. - # - # Sampling the newest N parent traces is wrong whenever the child is - # conditional on a state the workload only sometimes reaches: the parent - # fires constantly, so the newest traces are the ones LEAST likely to - # carry a rare child. Three relationships were skipped as unassertable - # for exactly this and none of them was a missing span -- - # txq.accept -> txq.accept_tx (child needs a queue holding a fee-clearing - # transaction), txq.enqueue -> txq.batch_clear (needs a supersedable - # batch) and ledger.acquire -> ledger.acquire.txtree (opens only when the - # node lacks the tx set, which in a cluster building identical sets is the - # minority case). Each child emitted on its own; it simply was not in the - # three newest parent traces. `{A} && {B}` is a trace-level conjunction, - # so Tempo searches its whole retention for co-occurrence rather than - # leaving it to which traces happen to be newest. + # Ask for traces holding BOTH rather than the newest parent traces. A + # parent that fires on every close has newest traces least likely to + # carry a conditional child. `{A} && {B}` matches at trace level, so + # co-occurrence is found wherever it happened. both_query = query + " && {" + _traceql_name_predicate(child_name) + "}" traces = await _tempo_search(session, tempo_url, both_query, limit=3) - # Verify against the returned traces rather than trusting the query. - # Tempo has already guaranteed co-occurrence, but re-checking the span - # names keeps the glob semantics in one place (_span_name_matches) and - # means a query built wrongly cannot silently pass. Names are matched - # exactly (globs for wildcard contracts) — a substring test let a - # longer emitted name satisfy a shorter contract, so - # consensus.round -> consensus.accept passed on a - # consensus.accept.apply span alone. + # Re-check the names even though Tempo already guaranteed co-occurrence: + # it keeps the glob handling in one place, and a wrongly built query + # cannot then pass silently. Matched exactly (globs for wildcard + # contracts) so a longer emitted name cannot satisfy a shorter contract. found_child = False for trace_summary in traces: trace_id = trace_summary.get("traceID", "")