mirror of
https://github.com/XRPLF/rippled.git
synced 2026-08-21 22:30:57 +00:00
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.
This commit is contained in:
215
.github/scripts/otel-naming/check_otel_naming.py
vendored
215
.github/scripts/otel-naming/check_otel_naming.py
vendored
@@ -49,7 +49,9 @@ Layers
|
||||
L3 tempo : docker/telemetry/tempo.yaml (span filter tags)
|
||||
L4 dashboards: docker/telemetry/grafana/dashboards/*.json (PromQL labels;
|
||||
Loki/LogQL queries exempt -- see LOG_QUERY_DATASOURCES)
|
||||
L5 runbook : docs/telemetry-runbook.md (attr tables)
|
||||
L5 docs : every *.md under docs/ and docker/telemetry/ (attr tables;
|
||||
the set is DISCOVERED, never enumerated -- see
|
||||
RULE_E_DOC_ROOTS)
|
||||
L6 metrics : MetricsRegistry.cpp instrument labels (native-metric
|
||||
label keys, a valid dashboard-label source besides L1)
|
||||
L1-metrics : src/**/*MetricNames.h, include/**/*MetricNames.h (ground truth
|
||||
@@ -77,11 +79,17 @@ Rules (each FAILS the build, when its inputs are present)
|
||||
captures or an in-query `| regexp` stage, so they have no L1/L6 source to
|
||||
resolve against (see LOG_QUERY_DATASOURCES). The exemption is per QUERY,
|
||||
not per file, so a mixed dashboard still has its PromQL panels checked.
|
||||
E No dotted `xrpl.<domain>.<field>` attribute key in the runbook (only the
|
||||
L1 resource attrs xrpl.network.* and the EXTERNAL_INFRA_LABELS dotted
|
||||
form -- xrpl.work.item/.branch/.node.role -- may be dotted). Span names,
|
||||
filenames,
|
||||
OTel-standard keys, and metric labels are not flagged.
|
||||
E No dotted `xrpl.<domain>.<field>` attribute key in any L5 doc. The doc set
|
||||
is DISCOVERED, not listed: every `*.md` under RULE_E_DOC_ROOTS, so a doc
|
||||
added or renamed inside a root is covered without editing this script.
|
||||
Only the L1 resource
|
||||
attrs xrpl.network.* and the EXTERNAL_INFRA_LABELS dotted form --
|
||||
xrpl.work.item/.branch/.node.role -- may be dotted. Span names, filenames,
|
||||
OTel-standard keys, and metric labels are not flagged. A doc that names the
|
||||
wrong form as a deliberate counter-example opts that mention out with a
|
||||
key-scoped marker (RULE_E_MARKER_SYNTAX), which exempts ONLY the keys it
|
||||
lists on the line it appears on -- any other dotted token on that same line
|
||||
still fails.
|
||||
I No string literals as METRIC instrument names or label keys. The mirror of
|
||||
Rule F for metrics: the name passed to an XRPL_METRIC_* macro or to a
|
||||
meter->Create* factory, and the label KEYS in its label set, must reference
|
||||
@@ -108,6 +116,13 @@ Warnings (printed, but do NOT fail the build)
|
||||
*SpanNames.h (single source of truth); defining one in-place bypasses the
|
||||
naming rules. A warning (not a failure) because the argument may instead
|
||||
be a legitimately dynamic local (e.g. a computed span-name leaf).
|
||||
E A RULE_E_REQUIRED_DOCS anchor is not present in the tree (renamed, moved,
|
||||
or deleted -- so doc discovery can no longer be trusted to have found the
|
||||
docs that matter), an allow-dotted marker names no key (so it exempts
|
||||
nothing), or it names a key the line no longer mentions (a stale
|
||||
exemption). Warnings, not failures, because presence-gating must keep
|
||||
working on partial branches -- but the skip is now visible instead of
|
||||
silent.
|
||||
L A literal metric name in a family that has no *MetricNames.h constants
|
||||
yet. Rule I's ratchet defers these rather than failing the build on the
|
||||
whole pre-existing metric surface at once; the warning is what keeps the
|
||||
@@ -520,7 +535,7 @@ def main() -> None:
|
||||
run_rule_b_collector(root, l1_keys, report)
|
||||
run_rule_c_tempo(root, l1_keys, report)
|
||||
run_rule_d_dashboards(root, l1_keys, metric_labels, report)
|
||||
run_rule_e_runbook(root, l1_keys, report)
|
||||
run_rule_e_docs(root, l1_keys, report)
|
||||
|
||||
# --- Metric rules I/J/K --------------------------------------------------
|
||||
# The metric-name counterpart of the span rules above. Rule I is the mirror
|
||||
@@ -1038,6 +1053,107 @@ EXTERNAL_INFRA_LABELS = {
|
||||
}
|
||||
|
||||
|
||||
# L5 doc layer (Rule E): the doc trees whose markdown publishes span-attribute
|
||||
# keys a reader is expected to copy into a TraceQL/PromQL query. A dotted
|
||||
# `xrpl.*` key left in any of them hands out a query that silently matches
|
||||
# nothing, so the whole tree is checked, not just the operator runbook:
|
||||
# * docs/ operator runbook, telemetry glossary, and the
|
||||
# build/telemetry guides (recursively)
|
||||
# * docker/telemetry/ stack bring-up guide; its TraceQL/PromQL examples
|
||||
# are meant to be pasted verbatim
|
||||
#
|
||||
# The set is DISCOVERED (every `*.md` under these roots), never enumerated. Two
|
||||
# reasons this beats a hardcoded path list:
|
||||
# * it cannot go stale -- a telemetry doc added, renamed, or moved within a
|
||||
# root is picked up with no edit here, which is the same "derive it, do not
|
||||
# hardcode it" principle the rest of this script follows (design
|
||||
# principle 1), and coverage therefore grows with the docs; and
|
||||
# * it names only doc ROOTS that ship on the default branch, so the rule can
|
||||
# never be wired to a path that does not exist there.
|
||||
# Each root is presence-gated (design principle 2): a branch that carries only
|
||||
# one of them still gets that one checked. Both roots are also CI path-triggers
|
||||
# for this check -- see `.github/workflows/on-pr.yml`.
|
||||
RULE_E_DOC_ROOTS = (
|
||||
Path("docs"),
|
||||
Path("docker") / "telemetry",
|
||||
)
|
||||
|
||||
# The anchor doc(s) discovery is REQUIRED to find. Discovery on its own can pass
|
||||
# vacuously: if a root were renamed away, or emptied of markdown, Rule E would
|
||||
# report a clean zero/near-zero-file run and nobody would notice the layer had
|
||||
# stopped being checked. Demanding the one doc that is the whole reason Rule E
|
||||
# exists turns that silent green into a visible warning (plus a count on the OK
|
||||
# line). Deliberately tiny: this is a tripwire, not a second doc list. Every
|
||||
# entry must live under RULE_E_DOC_ROOTS, or discovery could never find it.
|
||||
RULE_E_REQUIRED_DOCS = (Path("docs") / "telemetry-runbook.md",)
|
||||
|
||||
|
||||
def rule_e_docs(root: Path) -> List[Path]:
|
||||
"""Discover the Rule E doc set: every `*.md` under RULE_E_DOC_ROOTS.
|
||||
|
||||
Returns repo-relative paths, sorted for stable reporting and de-duplicated
|
||||
so overlapping roots (say `docs` and `docs/build`) cannot check one file
|
||||
twice. A root absent from the tree is skipped rather than treated as an
|
||||
error, so presence gating applies to the roots as it does to every other
|
||||
layer."""
|
||||
found: Set[Path] = set()
|
||||
for rel_root in RULE_E_DOC_ROOTS:
|
||||
base = root / rel_root
|
||||
if not base.is_dir():
|
||||
continue
|
||||
found.update(p.relative_to(root) for p in base.rglob("*.md") if p.is_file())
|
||||
return sorted(found)
|
||||
|
||||
|
||||
# Line-scoped AND key-scoped opt-out for Rule E. Put it on a line that names a
|
||||
# dotted key as a deliberate counter-example ("`tx_hash`, not `xrpl.tx.hash`") —
|
||||
# a mention, not a published attribute key — and list exactly the keys that line
|
||||
# is allowed to mention:
|
||||
#
|
||||
# ... use `tx_hash`, not `xrpl.tx.hash`.
|
||||
# <!-- otel-naming:allow-dotted: xrpl.tx.hash -->
|
||||
#
|
||||
# The keys are part of the marker so an exemption cannot widen silently: a dotted
|
||||
# token added to an already-marked line later, and not named in the marker, still
|
||||
# fails. A marker with an empty key list therefore exempts NOTHING (and warns) —
|
||||
# it is not a blanket line opt-out. NEVER use the marker to keep a real attribute
|
||||
# table dotted; fix the table instead.
|
||||
RULE_E_MARKER_SYNTAX = "<!-- otel-naming:allow-dotted: <key>[, <key>...] -->"
|
||||
# The marker itself; group(1) is the raw key list (empty for the bare form).
|
||||
# `[^>]` keeps the match inside one comment, so a marker cannot swallow the rest
|
||||
# of the line, and the whole thing is matched per line (never across lines).
|
||||
RULE_E_MARKER = re.compile(r"<!--\s*otel-naming:allow-dotted\s*:?\s*([^>]*?)\s*-->")
|
||||
# Keys inside the marker are separated by commas and/or whitespace.
|
||||
RULE_E_MARKER_KEY_SEP = re.compile(r"[,\s]+")
|
||||
# The only doc form Rule E flags: a backticked dotted `xrpl.<domain>.<field>`.
|
||||
RULE_E_DOTTED_TOKEN = re.compile(r"`(xrpl\.[a-z][a-z0-9_.]*)`")
|
||||
|
||||
|
||||
def rule_e_allowed_keys(line: str) -> Tuple[Set[str], int]:
|
||||
"""Parse every Rule-E allow-dotted marker on one line.
|
||||
|
||||
Returns `(keys named by the markers, number of markers seen)`. The count is
|
||||
returned separately so the caller can tell "no marker" (enforce everything,
|
||||
silently) from "a marker that names no key" (enforce everything, and warn
|
||||
that the marker does nothing).
|
||||
|
||||
Keys may be written bare or backticked and separated by commas and/or
|
||||
spaces, so both of these parse to the same two keys::
|
||||
|
||||
<!-- otel-naming:allow-dotted: xrpl.tx.hash, xrpl.peer.id -->
|
||||
<!-- otel-naming:allow-dotted: `xrpl.tx.hash` `xrpl.peer.id` -->
|
||||
"""
|
||||
keys: Set[str] = set()
|
||||
count = 0
|
||||
for m in RULE_E_MARKER.finditer(line):
|
||||
count += 1
|
||||
for raw in RULE_E_MARKER_KEY_SEP.split(m.group(1)):
|
||||
token = raw.strip().strip("`")
|
||||
if token:
|
||||
keys.add(token)
|
||||
return keys, count
|
||||
|
||||
|
||||
# Datasource types whose query language draws its label names from the log
|
||||
# stream at query time rather than from anything this repo's OTel code emits.
|
||||
#
|
||||
@@ -1228,20 +1344,33 @@ def run_rule_d_dashboards(
|
||||
report.ok(note + ")")
|
||||
|
||||
|
||||
def run_rule_e_runbook(root: Path, l1_keys: Set[str], report: Report) -> None:
|
||||
path = root / "docs" / "telemetry-runbook.md"
|
||||
if not path.is_file():
|
||||
report.skip("E", "runbook not present")
|
||||
def run_rule_e_docs(root: Path, l1_keys: Set[str], report: Report) -> None:
|
||||
discovered = rule_e_docs(root)
|
||||
if not discovered:
|
||||
roots = ", ".join(str(rel) for rel in RULE_E_DOC_ROOTS)
|
||||
report.skip("E", f"no doc-layer file present (no *.md under {roots})")
|
||||
return
|
||||
# Presence-gating (design principle 2) must stay VISIBLE. Discovery cannot go
|
||||
# stale, but it can go QUIET: a root renamed away, or stripped of its
|
||||
# telemetry docs, still yields a clean run over whatever markdown is left, so
|
||||
# the layer would stop being checked while the rule reported green. Warn per
|
||||
# missing anchor — non-fatal, so a partial branch still passes — and name the
|
||||
# count on the OK line, so the doc set cannot quietly shrink.
|
||||
missing_required = [
|
||||
rel for rel in RULE_E_REQUIRED_DOCS if not (root / rel).is_file()
|
||||
]
|
||||
for rel in missing_required:
|
||||
report.warning(
|
||||
"E", str(rel), "absent", "required Rule E doc not in tree (renamed?)"
|
||||
)
|
||||
if not l1_keys:
|
||||
report.skip("E", "no L1 key set to validate against")
|
||||
return
|
||||
text = read_source(path)
|
||||
found = False
|
||||
# Only the dotted `xrpl.<domain>.<field>` attribute form is a violation. The
|
||||
# `xrpl.`-with-trailing-dot anchor is the discriminator: it matches the old
|
||||
# dotted attribute convention being migrated away from, while everything
|
||||
# else legitimately dotted in the runbook does NOT match it —
|
||||
# else legitimately dotted in these docs does NOT match it —
|
||||
# * span names (`consensus.round`, `tx.process`) no `xrpl.` prefix
|
||||
# * filenames (`xrpld.cfg`, `RCLConsensus.cpp`) `xrpld.`/`.cpp`, not `xrpl.`
|
||||
# * OTel-standard (`service.name`, `http.method`) no `xrpl.` prefix
|
||||
@@ -1254,19 +1383,57 @@ def run_rule_e_runbook(root: Path, l1_keys: Set[str], report: Report) -> None:
|
||||
# identities dotted (xrpl.work.item/.branch/.node.role -- see the alloy
|
||||
# pipeline that owns them), so also skip a token whose dotted-to-underscore
|
||||
# form is in that set.
|
||||
# A doc that TEACHES the convention has to be able to name the wrong form as
|
||||
# a counter-example ("`tx_hash`, not `xrpl.tx.hash`"). The allow-dotted
|
||||
# marker is the opt-out for exactly that: the token is a mention, not a
|
||||
# published attribute key. It follows the repo's existing inline-marker
|
||||
# precedent (`<!-- cspell:ignore ... -->`), and is scoped BOTH to the line
|
||||
# and to the keys it names, so it can neither exempt a whole table nor grow
|
||||
# to cover a violation appended to an already-marked line.
|
||||
external_infra_dotted = {lbl.replace("_", ".") for lbl in EXTERNAL_INFRA_LABELS}
|
||||
for m in re.finditer(r"`(xrpl\.[a-z][a-z0-9_.]*)`", text):
|
||||
token = m.group(1)
|
||||
if token in l1_keys: # legitimate dotted resource attr (xrpl.network.*)
|
||||
continue
|
||||
if token in external_infra_dotted: # perf-iac resource-attribute layer
|
||||
continue
|
||||
found = True
|
||||
report.violation(
|
||||
"E", str(path.relative_to(root)), token, "underscore, not dotted"
|
||||
)
|
||||
for rel in discovered:
|
||||
for lineno, line in enumerate(read_source(root / rel).splitlines(), start=1):
|
||||
allowed, markers = rule_e_allowed_keys(line)
|
||||
if markers and not allowed:
|
||||
report.warning(
|
||||
"E",
|
||||
f"{rel}:{lineno}",
|
||||
"allow-dotted",
|
||||
f"marker names no key: exempts nothing. Use {RULE_E_MARKER_SYNTAX}",
|
||||
)
|
||||
tokens = [m.group(1) for m in RULE_E_DOTTED_TOKEN.finditer(line)]
|
||||
for token in tokens:
|
||||
if token in l1_keys: # legitimate dotted resource attr (xrpl.network.*)
|
||||
continue
|
||||
if token in external_infra_dotted: # perf-iac resource-attr layer
|
||||
continue
|
||||
if token in allowed: # named counter-example on this line
|
||||
continue
|
||||
found = True
|
||||
report.violation(
|
||||
"E",
|
||||
f"{rel}:{lineno}",
|
||||
token,
|
||||
"underscore, not dotted",
|
||||
)
|
||||
# A key the marker names but the line no longer mentions is a stale
|
||||
# exemption: harmless today, but it is how a marker silently starts
|
||||
# covering more than the author reviewed. Flagged, never fatal.
|
||||
for stale in sorted(allowed.difference(tokens)):
|
||||
report.warning(
|
||||
"E", f"{rel}:{lineno}", stale, "allow-dotted key not on this line"
|
||||
)
|
||||
if not found:
|
||||
report.ok("E: runbook attribute references consistent with L1")
|
||||
note = (
|
||||
"E: doc attribute references consistent with L1 "
|
||||
f"({len(discovered)} file(s) checked"
|
||||
)
|
||||
if missing_required:
|
||||
note += (
|
||||
f", {len(missing_required)} of {len(RULE_E_REQUIRED_DOCS)} "
|
||||
"required doc(s) absent"
|
||||
)
|
||||
report.ok(note + ")")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -13,7 +13,7 @@ Run from anywhere:
|
||||
|
||||
Each rule is exercised in isolation against a synthetic tree / synthetic L1 key
|
||||
set, covering positive (must flag), negative (must not flag), and boundary
|
||||
cases. Rule E (runbook dotted-attribute detection) has the densest coverage
|
||||
cases. Rule E (doc-layer dotted-attribute detection) has the densest coverage
|
||||
because its discriminator — the `xrpl.<domain>.` prefix vs span names,
|
||||
filenames, OTel-standard keys, and metric labels — is the subtlest.
|
||||
"""
|
||||
@@ -23,6 +23,7 @@ import importlib.util
|
||||
import io
|
||||
import json
|
||||
import shutil
|
||||
import subprocess
|
||||
import tempfile
|
||||
import unittest
|
||||
from pathlib import Path
|
||||
@@ -51,13 +52,25 @@ L1 = {
|
||||
|
||||
def _run_rule_e(runbook_text: str):
|
||||
"""Run Rule E against a synthetic runbook; return the flagged tokens."""
|
||||
return _run_rule_e_docs({"docs/telemetry-runbook.md": runbook_text})[0]
|
||||
|
||||
|
||||
def _run_rule_e_docs(docs, l1_keys=None):
|
||||
"""Run Rule E against a synthetic doc set.
|
||||
|
||||
`docs` maps a repo-relative path to that file's text; only the listed files
|
||||
are created, so per-file presence gating can be exercised. Returns
|
||||
(sorted flagged tokens, the Report) so location and skip/ok lines can be
|
||||
asserted as well as the tokens."""
|
||||
d = Path(tempfile.mkdtemp())
|
||||
try:
|
||||
(d / "docs").mkdir()
|
||||
(d / "docs" / "telemetry-runbook.md").write_text(runbook_text)
|
||||
for rel, text in docs.items():
|
||||
path = d / rel
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
path.write_text(text)
|
||||
report = chk.Report()
|
||||
chk.run_rule_e_runbook(d, set(L1), report)
|
||||
return sorted(v[2] for v in report.violations)
|
||||
chk.run_rule_e_docs(d, set(L1) if l1_keys is None else set(l1_keys), report)
|
||||
return sorted(v[2] for v in report.violations), report
|
||||
finally:
|
||||
shutil.rmtree(d)
|
||||
|
||||
@@ -170,7 +183,7 @@ class RuleERunbook(unittest.TestCase):
|
||||
d = Path(tempfile.mkdtemp())
|
||||
try:
|
||||
report = chk.Report()
|
||||
chk.run_rule_e_runbook(d, set(L1), report)
|
||||
chk.run_rule_e_docs(d, set(L1), report)
|
||||
self.assertEqual(report.violations, [])
|
||||
self.assertTrue(any("SKIP: E" in s for s in report.skips))
|
||||
finally:
|
||||
@@ -182,13 +195,593 @@ class RuleERunbook(unittest.TestCase):
|
||||
(d / "docs").mkdir()
|
||||
(d / "docs" / "telemetry-runbook.md").write_text("`xrpl.tx.hash`")
|
||||
report = chk.Report()
|
||||
chk.run_rule_e_runbook(d, set(), report)
|
||||
chk.run_rule_e_docs(d, set(), report)
|
||||
self.assertEqual(report.violations, [])
|
||||
self.assertTrue(any("SKIP: E" in s for s in report.skips))
|
||||
finally:
|
||||
shutil.rmtree(d)
|
||||
|
||||
|
||||
# The doc paths the Rule E tests build synthetic trees from. All four are real,
|
||||
# committable files that live under RULE_E_DOC_ROOTS, so a test tree has the same
|
||||
# shape discovery meets in the repo. GLOSSARY and BUILD_GUIDE additionally prove
|
||||
# discovery is not runbook-shaped: BUILD_GUIDE sits one directory deeper, so a
|
||||
# non-recursive glob would miss it.
|
||||
RUNBOOK_DOC = "docs/telemetry-runbook.md"
|
||||
GLOSSARY_DOC = "docs/telemetry-glossary.md"
|
||||
BUILD_GUIDE_DOC = "docs/build/telemetry.md"
|
||||
TESTING_DOC = "docker/telemetry/TESTING.md"
|
||||
ALL_DOCS = (RUNBOOK_DOC, GLOSSARY_DOC, BUILD_GUIDE_DOC, TESTING_DOC)
|
||||
|
||||
# Top-level trees that ship to the default branch, so anything the check is
|
||||
# configured to read must live inside one of them. A configured path outside
|
||||
# these would name a tree that is not part of the shipped repository: the rule
|
||||
# would warn forever about a doc that can never appear, and the matching CI
|
||||
# path-trigger would be dead weight.
|
||||
COMMITTABLE_ROOTS = (Path("docs"), Path("docker"))
|
||||
|
||||
|
||||
def _inside_any(rel: Path, roots) -> bool:
|
||||
"""True if `rel` is one of `roots` or lives underneath one of them."""
|
||||
return any(root == rel or root in rel.parents for root in roots)
|
||||
|
||||
|
||||
def _git_tracks(root: Path, rel: Path) -> bool:
|
||||
"""True if git tracks `rel` (a file) or anything under it (a directory).
|
||||
|
||||
Evidence that a configured path is genuinely part of the repository rather
|
||||
than a local scratch or ignored directory."""
|
||||
out = subprocess.run(
|
||||
["git", "ls-files", "--", str(rel)],
|
||||
cwd=str(root),
|
||||
capture_output=True,
|
||||
text=True,
|
||||
)
|
||||
return bool(out.stdout.strip())
|
||||
|
||||
|
||||
class RuleEDocSet(unittest.TestCase):
|
||||
"""Rule E scans every discovered doc, not just the runbook. The glossary, the
|
||||
build/telemetry guide and the stack testing guide all publish attribute keys
|
||||
readers copy queries from, so a dotted key left in any of them is as
|
||||
operator-breaking as one in the runbook."""
|
||||
|
||||
RUNBOOK = RUNBOOK_DOC
|
||||
GLOSSARY = GLOSSARY_DOC
|
||||
BUILD_GUIDE = BUILD_GUIDE_DOC
|
||||
TESTING = TESTING_DOC
|
||||
|
||||
def test_flags_dotted_attr_in_testing_guide(self):
|
||||
# TESTING.md publishes copy-paste TraceQL/PromQL, so a dotted key there
|
||||
# hands the reader a query that silently matches nothing.
|
||||
tokens, report = _run_rule_e_docs(
|
||||
{self.TESTING: '{name="tx.process" && span.`xrpl.tx.hash`="AB"}'}
|
||||
)
|
||||
self.assertEqual(tokens, ["xrpl.tx.hash"])
|
||||
self.assertEqual(report.violations[0][1], f"{self.TESTING}:1")
|
||||
self.assertEqual(report.violations[0][3], "underscore, not dotted")
|
||||
|
||||
def test_testing_guide_checked_when_every_other_doc_absent(self):
|
||||
# Per-file presence gating: TESTING.md alone must not skip the rule.
|
||||
tokens, report = _run_rule_e_docs({self.TESTING: "`xrpl.peer.version`"})
|
||||
self.assertEqual(tokens, ["xrpl.peer.version"])
|
||||
self.assertEqual(report.skips, [])
|
||||
self.assertEqual(report.violations[0][1], f"{self.TESTING}:1")
|
||||
# A violating run emits the violation, not an "ok" line.
|
||||
self.assertEqual(report.checked, [])
|
||||
|
||||
def test_testing_guide_alone_reports_one_clean_file(self):
|
||||
tokens, report = _run_rule_e_docs({self.TESTING: "`tx_hash`"})
|
||||
self.assertEqual(tokens, [])
|
||||
self.assertTrue(any("E:" in c and "1 file(s)" in c for c in report.checked))
|
||||
|
||||
def test_clean_testing_guide_passes_with_bare_keys(self):
|
||||
# The real docker/telemetry/TESTING.md shape: bare/underscore attr keys
|
||||
# plus dotted SPAN names, which must not be flagged.
|
||||
tokens, report = _run_rule_e_docs(
|
||||
{self.TESTING: '{name="tx.process" && span.tx_hash!=""} `ledger_seq`'}
|
||||
)
|
||||
self.assertEqual(tokens, [])
|
||||
self.assertEqual(report.violations, [])
|
||||
|
||||
def test_exemptions_apply_in_testing_guide_too(self):
|
||||
tokens, _ = _run_rule_e_docs(
|
||||
{self.TESTING: "`xrpl.network.type` `xrpl.work.item`"}
|
||||
)
|
||||
self.assertEqual(tokens, [])
|
||||
|
||||
def test_flags_dotted_attr_in_glossary(self):
|
||||
tokens, report = _run_rule_e_docs({self.GLOSSARY: "| `xrpl.tx.hash` | h |"})
|
||||
self.assertEqual(tokens, ["xrpl.tx.hash"])
|
||||
self.assertEqual(report.violations[0][1], f"{self.GLOSSARY}:1")
|
||||
|
||||
def test_flags_dotted_attr_in_nested_build_guide(self):
|
||||
# One directory below the root: proves discovery recurses rather than
|
||||
# globbing only the root's own children.
|
||||
tokens, report = _run_rule_e_docs({self.BUILD_GUIDE: "`xrpl.consensus.mode`"})
|
||||
self.assertEqual(tokens, ["xrpl.consensus.mode"])
|
||||
self.assertEqual(report.violations[0][1], f"{self.BUILD_GUIDE}:1")
|
||||
|
||||
def test_location_carries_the_offending_line_number(self):
|
||||
tokens, report = _run_rule_e_docs(
|
||||
{self.GLOSSARY: "clean\nstill clean\n| `xrpl.tx.hash` |"}
|
||||
)
|
||||
self.assertEqual(tokens, ["xrpl.tx.hash"])
|
||||
self.assertEqual(report.violations[0][1], f"{self.GLOSSARY}:3")
|
||||
|
||||
def test_flags_across_all_four_docs(self):
|
||||
tokens, report = _run_rule_e_docs(
|
||||
{
|
||||
self.RUNBOOK: "`xrpl.tx.hash`",
|
||||
self.GLOSSARY: "`xrpl.peer.id`",
|
||||
self.BUILD_GUIDE: "`xrpl.ledger.seq`",
|
||||
self.TESTING: "`xrpl.rpc.command`",
|
||||
}
|
||||
)
|
||||
self.assertEqual(
|
||||
tokens,
|
||||
[
|
||||
"xrpl.ledger.seq",
|
||||
"xrpl.peer.id",
|
||||
"xrpl.rpc.command",
|
||||
"xrpl.tx.hash",
|
||||
],
|
||||
)
|
||||
self.assertEqual(
|
||||
sorted(v[1] for v in report.violations),
|
||||
sorted(
|
||||
[
|
||||
f"{self.RUNBOOK}:1",
|
||||
f"{self.GLOSSARY}:1",
|
||||
f"{self.BUILD_GUIDE}:1",
|
||||
f"{self.TESTING}:1",
|
||||
]
|
||||
),
|
||||
)
|
||||
|
||||
def test_other_doc_checked_when_runbook_absent(self):
|
||||
# Presence gating is per discovered file: a branch without the runbook
|
||||
# still gets the docs it does carry checked, instead of the rule skipping.
|
||||
tokens, report = _run_rule_e_docs({self.GLOSSARY: "`xrpl.tx.hash`"})
|
||||
self.assertEqual(tokens, ["xrpl.tx.hash"])
|
||||
self.assertEqual(report.skips, [])
|
||||
|
||||
def test_absent_doc_is_not_an_error(self):
|
||||
tokens, report = _run_rule_e_docs({self.RUNBOOK: "`tx_hash`"})
|
||||
self.assertEqual(tokens, [])
|
||||
self.assertTrue(any("1 file(s)" in c for c in report.checked))
|
||||
|
||||
def test_clean_doc_set_reports_file_count(self):
|
||||
tokens, report = _run_rule_e_docs(
|
||||
{
|
||||
self.RUNBOOK: "`tx_hash` `consensus.round`",
|
||||
self.GLOSSARY: "`xrpl.network.id` `ledger_seq`",
|
||||
self.BUILD_GUIDE: "`service.name` `peer_id`",
|
||||
self.TESTING: "`rpc_command` `tx.process`",
|
||||
}
|
||||
)
|
||||
self.assertEqual(tokens, [])
|
||||
self.assertEqual(report.violations, [])
|
||||
self.assertTrue(any("E:" in c and "4 file(s)" in c for c in report.checked))
|
||||
|
||||
def test_skips_when_no_doc_present(self):
|
||||
tokens, report = _run_rule_e_docs({})
|
||||
self.assertEqual(tokens, [])
|
||||
self.assertEqual(report.checked, [])
|
||||
self.assertTrue(any("SKIP: E" in s for s in report.skips))
|
||||
|
||||
def test_exemptions_apply_in_every_discovered_doc(self):
|
||||
# The L1 resource attrs and perf-iac dotted identities must stay exempt
|
||||
# in every discovered doc, exactly as in the runbook.
|
||||
tokens, _ = _run_rule_e_docs(
|
||||
{
|
||||
self.GLOSSARY: "`xrpl.network.id` `xrpl.network.type`",
|
||||
self.BUILD_GUIDE: "`xrpl.work.item` `xrpl.branch` `xrpl.node.role`",
|
||||
}
|
||||
)
|
||||
self.assertEqual(tokens, [])
|
||||
|
||||
|
||||
def _marker(*keys: str) -> str:
|
||||
"""Build an allow-dotted marker naming `keys` (no key = the bare form)."""
|
||||
body = ": " + ", ".join(keys) if keys else ""
|
||||
return f"<!-- otel-naming:allow-dotted{body} -->"
|
||||
|
||||
|
||||
def _marker_warnings(report):
|
||||
"""The marker-related warnings only.
|
||||
|
||||
A synthetic doc set that does not create the runbook leaves that required
|
||||
anchor absent, which is warned about in its own right (see
|
||||
RuleERequiredDocPresence). Filtering those out keeps the marker tests
|
||||
asserting the marker's own behaviour."""
|
||||
return [tuple(w) for w in report.warnings if w[2] != "absent"]
|
||||
|
||||
|
||||
class RuleEAllowDottedMarkerParsing(unittest.TestCase):
|
||||
"""`rule_e_allowed_keys` — the marker parser. The key list is what bounds the
|
||||
exemption, so its parsing (and the marker COUNT, which distinguishes "no
|
||||
marker" from "a marker that names nothing") is asserted directly."""
|
||||
|
||||
def test_no_marker_yields_no_keys_and_no_marker_count(self):
|
||||
self.assertEqual(
|
||||
chk.rule_e_allowed_keys("plain `xrpl.tx.hash` line"), (set(), 0)
|
||||
)
|
||||
|
||||
def test_bare_marker_counts_but_names_no_key(self):
|
||||
self.assertEqual(chk.rule_e_allowed_keys(_marker()), (set(), 1))
|
||||
|
||||
def test_single_key(self):
|
||||
self.assertEqual(
|
||||
chk.rule_e_allowed_keys(_marker("xrpl.tx.hash")), ({"xrpl.tx.hash"}, 1)
|
||||
)
|
||||
|
||||
def test_comma_separated_keys(self):
|
||||
self.assertEqual(
|
||||
chk.rule_e_allowed_keys(_marker("xrpl.tx.hash", "xrpl.node.server_state")),
|
||||
({"xrpl.tx.hash", "xrpl.node.server_state"}, 1),
|
||||
)
|
||||
|
||||
def test_space_separated_and_backticked_keys(self):
|
||||
self.assertEqual(
|
||||
chk.rule_e_allowed_keys(
|
||||
"<!-- otel-naming:allow-dotted: `xrpl.tx.hash` `xrpl.peer.id` -->"
|
||||
),
|
||||
({"xrpl.tx.hash", "xrpl.peer.id"}, 1),
|
||||
)
|
||||
|
||||
def test_no_surrounding_whitespace(self):
|
||||
self.assertEqual(
|
||||
chk.rule_e_allowed_keys("<!--otel-naming:allow-dotted:xrpl.tx.hash-->"),
|
||||
({"xrpl.tx.hash"}, 1),
|
||||
)
|
||||
|
||||
def test_two_markers_on_one_line_union(self):
|
||||
line = _marker("xrpl.tx.hash") + " text " + _marker("xrpl.peer.id")
|
||||
self.assertEqual(
|
||||
chk.rule_e_allowed_keys(line), ({"xrpl.tx.hash", "xrpl.peer.id"}, 2)
|
||||
)
|
||||
|
||||
def test_prose_around_marker_is_not_absorbed_as_a_key(self):
|
||||
line = "use `tx_hash`, not `xrpl.tx.hash`. " + _marker("xrpl.tx.hash") + " ok"
|
||||
self.assertEqual(chk.rule_e_allowed_keys(line), ({"xrpl.tx.hash"}, 1))
|
||||
|
||||
def test_trailing_comma_does_not_yield_empty_key(self):
|
||||
self.assertEqual(
|
||||
chk.rule_e_allowed_keys("<!-- otel-naming:allow-dotted: xrpl.tx.hash, -->"),
|
||||
({"xrpl.tx.hash"}, 1),
|
||||
)
|
||||
|
||||
|
||||
class RuleEAllowDottedMarkerEnforcement(unittest.TestCase):
|
||||
"""The marker exempts ONLY the keys it names, only on its own line. A
|
||||
blanket line opt-out would let a genuine violation appended to any marked
|
||||
line ride in silently, which is exactly what these tests forbid."""
|
||||
|
||||
REFERENCE = GLOSSARY_DOC
|
||||
|
||||
def test_named_key_is_exempt(self):
|
||||
tokens, report = _run_rule_e_docs(
|
||||
{
|
||||
self.REFERENCE: "use `tx_hash`, not `xrpl.tx.hash`. "
|
||||
+ _marker("xrpl.tx.hash")
|
||||
}
|
||||
)
|
||||
self.assertEqual(tokens, [])
|
||||
self.assertEqual(report.violations, [])
|
||||
self.assertEqual(_marker_warnings(report), [])
|
||||
|
||||
def test_unlisted_key_on_marked_line_still_fails(self):
|
||||
# The hole this closes: one marker must not cover a second dotted key
|
||||
# someone appends to the line later.
|
||||
tokens, report = _run_rule_e_docs(
|
||||
{
|
||||
self.REFERENCE: "not `xrpl.tx.hash` and also `xrpl.peer.id` "
|
||||
+ _marker("xrpl.tx.hash")
|
||||
}
|
||||
)
|
||||
self.assertEqual(tokens, ["xrpl.peer.id"])
|
||||
self.assertEqual(report.violations[0][0], "E")
|
||||
self.assertEqual(report.violations[0][1], f"{self.REFERENCE}:1")
|
||||
self.assertEqual(report.violations[0][3], "underscore, not dotted")
|
||||
|
||||
def test_bare_marker_exempts_nothing_and_warns(self):
|
||||
tokens, report = _run_rule_e_docs(
|
||||
{self.REFERENCE: "not `xrpl.tx.hash`. " + _marker()}
|
||||
)
|
||||
self.assertEqual(tokens, ["xrpl.tx.hash"])
|
||||
self.assertEqual(report.violations[0][1], f"{self.REFERENCE}:1")
|
||||
warnings = _marker_warnings(report)
|
||||
self.assertEqual(
|
||||
[w[:3] for w in warnings], [("E", f"{self.REFERENCE}:1", "allow-dotted")]
|
||||
)
|
||||
self.assertIn(chk.RULE_E_MARKER_SYNTAX, warnings[0][3])
|
||||
|
||||
def test_marker_does_not_leak_to_adjacent_lines(self):
|
||||
tokens, report = _run_rule_e_docs(
|
||||
{
|
||||
self.REFERENCE: "| `xrpl.tx.hash` | before |\n"
|
||||
+ "not `xrpl.tx.hash`. "
|
||||
+ _marker("xrpl.tx.hash")
|
||||
+ "\n| `xrpl.tx.hash` | after |"
|
||||
}
|
||||
)
|
||||
self.assertEqual(tokens, ["xrpl.tx.hash", "xrpl.tx.hash"])
|
||||
self.assertEqual(
|
||||
sorted(v[1] for v in report.violations),
|
||||
[f"{self.REFERENCE}:1", f"{self.REFERENCE}:3"],
|
||||
)
|
||||
|
||||
def test_all_named_keys_exempt_on_a_table_row(self):
|
||||
# The real-world shape: a "was -> is now" row naming two old
|
||||
# dotted keys, with both listed in the marker.
|
||||
tokens, _ = _run_rule_e_docs(
|
||||
{
|
||||
self.REFERENCE: "| `xrpl.validation.full`, `xrpl.peer.validation.full`"
|
||||
" | one bare `full_validation` | "
|
||||
+ _marker("xrpl.validation.full", "xrpl.peer.validation.full")
|
||||
}
|
||||
)
|
||||
self.assertEqual(tokens, [])
|
||||
|
||||
def test_partially_listed_table_row_fails_on_the_unlisted_key(self):
|
||||
tokens, report = _run_rule_e_docs(
|
||||
{
|
||||
self.REFERENCE: "| `xrpl.validation.full`, `xrpl.peer.validation.full`"
|
||||
" | one bare `full_validation` | " + _marker("xrpl.validation.full")
|
||||
}
|
||||
)
|
||||
self.assertEqual(tokens, ["xrpl.peer.validation.full"])
|
||||
self.assertEqual(report.violations[0][1], f"{self.REFERENCE}:1")
|
||||
|
||||
def test_key_prefix_does_not_exempt_a_longer_key(self):
|
||||
# Exemption is an exact token match, not a prefix match.
|
||||
tokens, _ = _run_rule_e_docs(
|
||||
{self.REFERENCE: "`xrpl.tx.hash` " + _marker("xrpl.tx")}
|
||||
)
|
||||
self.assertEqual(tokens, ["xrpl.tx.hash"])
|
||||
|
||||
def test_dotted_prefix_token_can_be_listed_verbatim(self):
|
||||
# The docs grep for prefixes such as `xrpl.node.` (trailing dot); the
|
||||
# marker must accept that exact token.
|
||||
tokens, report = _run_rule_e_docs(
|
||||
{
|
||||
self.REFERENCE: "a grep for `xrpl.node.` and `xrpl.peer.` returns "
|
||||
"nothing " + _marker("xrpl.node.", "xrpl.peer.")
|
||||
}
|
||||
)
|
||||
self.assertEqual(tokens, [])
|
||||
self.assertEqual(_marker_warnings(report), [])
|
||||
|
||||
def test_stale_key_warns_but_does_not_fail(self):
|
||||
tokens, report = _run_rule_e_docs(
|
||||
{self.REFERENCE: "all clean now. " + _marker("xrpl.tx.hash")}
|
||||
)
|
||||
self.assertEqual(tokens, [])
|
||||
self.assertEqual(report.violations, [])
|
||||
self.assertEqual(
|
||||
_marker_warnings(report),
|
||||
[
|
||||
(
|
||||
"E",
|
||||
f"{self.REFERENCE}:1",
|
||||
"xrpl.tx.hash",
|
||||
"allow-dotted key not on this line",
|
||||
)
|
||||
],
|
||||
)
|
||||
|
||||
def test_marker_naming_an_l1_key_is_not_reported_stale(self):
|
||||
# `xrpl.network.id` is exempt via L1, so a marker naming it is redundant
|
||||
# but not stale — the key IS on the line.
|
||||
tokens, report = _run_rule_e_docs(
|
||||
{self.REFERENCE: "`xrpl.network.id` " + _marker("xrpl.network.id")}
|
||||
)
|
||||
self.assertEqual(tokens, [])
|
||||
self.assertEqual(_marker_warnings(report), [])
|
||||
|
||||
def test_marker_works_in_every_doc_of_the_set(self):
|
||||
docs = {
|
||||
rel: "not `xrpl.tx.hash`. " + _marker("xrpl.tx.hash") for rel in ALL_DOCS
|
||||
}
|
||||
tokens, report = _run_rule_e_docs(docs)
|
||||
self.assertEqual(tokens, [])
|
||||
self.assertEqual(report.violations, [])
|
||||
|
||||
|
||||
class RuleEDocDiscovery(unittest.TestCase):
|
||||
"""`rule_e_docs` — the discovery that replaced a hardcoded doc list. What has
|
||||
to hold: it finds the real docs in this repo (a rule scanning an empty set
|
||||
passes without checking anything), it recurses into subdirectories, it never
|
||||
checks a file twice, and every path it is configured with lies inside a tree
|
||||
that actually ships."""
|
||||
|
||||
def test_discovery_finds_more_than_one_doc_in_this_repo(self):
|
||||
# The vacuous-pass guard, measured against the real tree: one file (or
|
||||
# none) would mean discovery had collapsed and Rule E was reporting green
|
||||
# over almost nothing.
|
||||
found = chk.rule_e_docs(chk.repo_root())
|
||||
self.assertGreater(len(found), 1, found)
|
||||
|
||||
def test_discovery_returns_no_duplicates(self):
|
||||
# Roots may overlap (a root and a subdirectory of it); a file must still
|
||||
# be checked once, so line numbers are not reported twice.
|
||||
found = chk.rule_e_docs(chk.repo_root())
|
||||
self.assertEqual(len(found), len(set(found)))
|
||||
|
||||
def test_discovery_finds_the_known_telemetry_docs(self):
|
||||
# Named here as an expectation of DISCOVERY, not as the checker's config:
|
||||
# if any of these is renamed, discovery still covers it under its new
|
||||
# name and only this assertion needs updating.
|
||||
found = set(chk.rule_e_docs(chk.repo_root()))
|
||||
for rel in (RUNBOOK_DOC, GLOSSARY_DOC, TESTING_DOC):
|
||||
self.assertIn(Path(rel), found)
|
||||
|
||||
def test_discovery_recurses_into_subdirectories(self):
|
||||
d = Path(tempfile.mkdtemp())
|
||||
try:
|
||||
nested = d / "docs" / "build" / "deep"
|
||||
nested.mkdir(parents=True)
|
||||
(nested / "telemetry.md").write_text("`tx_hash`")
|
||||
self.assertEqual(
|
||||
chk.rule_e_docs(d), [Path("docs") / "build" / "deep" / "telemetry.md"]
|
||||
)
|
||||
finally:
|
||||
shutil.rmtree(d)
|
||||
|
||||
def test_discovery_ignores_non_markdown(self):
|
||||
d = Path(tempfile.mkdtemp())
|
||||
try:
|
||||
(d / "docs").mkdir()
|
||||
(d / "docs" / "notes.txt").write_text("`xrpl.tx.hash`")
|
||||
(d / "docs" / "runbook.md").write_text("`tx_hash`")
|
||||
self.assertEqual(chk.rule_e_docs(d), [Path("docs") / "runbook.md"])
|
||||
finally:
|
||||
shutil.rmtree(d)
|
||||
|
||||
def test_discovery_tolerates_an_absent_root(self):
|
||||
# Presence gating applies to the roots too: a tree carrying only one of
|
||||
# them must not raise, and must still discover that one.
|
||||
d = Path(tempfile.mkdtemp())
|
||||
try:
|
||||
(d / "docker" / "telemetry").mkdir(parents=True)
|
||||
(d / "docker" / "telemetry" / "TESTING.md").write_text("`tx_hash`")
|
||||
self.assertEqual(
|
||||
chk.rule_e_docs(d), [Path("docker") / "telemetry" / "TESTING.md"]
|
||||
)
|
||||
finally:
|
||||
shutil.rmtree(d)
|
||||
|
||||
def test_no_configured_path_lies_outside_the_committable_roots(self):
|
||||
# The check may only be wired to trees that ship. A configured path
|
||||
# outside them would make Rule E warn about a doc that can never appear
|
||||
# and leave the matching CI path-trigger dead.
|
||||
for rel in chk.RULE_E_DOC_ROOTS + chk.RULE_E_REQUIRED_DOCS:
|
||||
self.assertTrue(_inside_any(rel, COMMITTABLE_ROOTS), rel)
|
||||
|
||||
def test_configured_paths_exist_and_are_version_controlled(self):
|
||||
# "Committable" is not just a naming claim: git must actually track the
|
||||
# contents of every configured path in this repo.
|
||||
root = chk.repo_root()
|
||||
for rel in chk.RULE_E_DOC_ROOTS:
|
||||
self.assertTrue((root / rel).is_dir(), rel)
|
||||
self.assertTrue(_git_tracks(root, rel), rel)
|
||||
for rel in chk.RULE_E_REQUIRED_DOCS:
|
||||
self.assertTrue((root / rel).is_file(), rel)
|
||||
self.assertTrue(_git_tracks(root, rel), rel)
|
||||
|
||||
def test_every_required_doc_is_discoverable(self):
|
||||
# An anchor outside every root could never be found, so the tripwire
|
||||
# would fire on every run and stop meaning anything.
|
||||
for rel in chk.RULE_E_REQUIRED_DOCS:
|
||||
self.assertTrue(_inside_any(rel, chk.RULE_E_DOC_ROOTS), rel)
|
||||
self.assertIn(rel, chk.rule_e_docs(chk.repo_root()))
|
||||
|
||||
|
||||
class RuleERequiredDocPresence(unittest.TestCase):
|
||||
"""A RULE_E_REQUIRED_DOCS anchor that is not in the tree must be VISIBLE.
|
||||
Discovery cannot go stale, but it can go quiet — a root emptied of telemetry
|
||||
docs still yields a clean run — so the anchor's absence is warned about and
|
||||
counted, while staying non-fatal so a partial branch still passes."""
|
||||
|
||||
RUNBOOK = RUNBOOK_DOC
|
||||
|
||||
def test_every_required_doc_resolves_in_this_repo(self):
|
||||
# If the runbook is renamed and RULE_E_REQUIRED_DOCS is not updated, the
|
||||
# anchor warning would fire on every run; catch it here instead.
|
||||
root = chk.repo_root()
|
||||
self.assertEqual(
|
||||
[
|
||||
str(rel)
|
||||
for rel in chk.RULE_E_REQUIRED_DOCS
|
||||
if not (root / rel).is_file()
|
||||
],
|
||||
[],
|
||||
)
|
||||
|
||||
def test_missing_anchor_is_warned_once(self):
|
||||
# Discovery finds the glossary, so the rule runs — but the anchor it is
|
||||
# required to find is gone, which must be said out loud.
|
||||
tokens, report = _run_rule_e_docs({GLOSSARY_DOC: "`tx_hash`"})
|
||||
self.assertEqual(tokens, [])
|
||||
self.assertEqual(
|
||||
[tuple(w) for w in report.warnings],
|
||||
[
|
||||
(
|
||||
"E",
|
||||
self.RUNBOOK,
|
||||
"absent",
|
||||
"required Rule E doc not in tree (renamed?)",
|
||||
)
|
||||
],
|
||||
)
|
||||
|
||||
def test_ok_line_names_the_absent_anchor_count(self):
|
||||
tokens, report = _run_rule_e_docs({GLOSSARY_DOC: "`tx_hash`"})
|
||||
self.assertEqual(tokens, [])
|
||||
self.assertTrue(
|
||||
any(
|
||||
"1 file(s) checked" in c
|
||||
and f"1 of {len(chk.RULE_E_REQUIRED_DOCS)} required doc(s) absent" in c
|
||||
for c in report.checked
|
||||
),
|
||||
report.checked,
|
||||
)
|
||||
|
||||
def test_full_doc_set_warns_nothing_and_reports_the_full_count(self):
|
||||
docs = {rel: "`tx_hash`" for rel in ALL_DOCS}
|
||||
tokens, report = _run_rule_e_docs(docs)
|
||||
self.assertEqual(tokens, [])
|
||||
self.assertEqual(report.warnings, [])
|
||||
self.assertTrue(
|
||||
any(
|
||||
f"{len(ALL_DOCS)} file(s) checked" in c and "absent" not in c
|
||||
for c in report.checked
|
||||
),
|
||||
report.checked,
|
||||
)
|
||||
|
||||
def test_anchor_present_warns_nothing_even_with_other_docs_absent(self):
|
||||
# Only the ANCHOR is required. A tree carrying just the runbook is a
|
||||
# legitimate partial branch, not a shrinking doc set.
|
||||
tokens, report = _run_rule_e_docs({self.RUNBOOK: "`tx_hash`"})
|
||||
self.assertEqual(tokens, [])
|
||||
self.assertEqual(report.warnings, [])
|
||||
self.assertTrue(
|
||||
any("1 file(s) checked" in c and "absent" not in c for c in report.checked),
|
||||
report.checked,
|
||||
)
|
||||
|
||||
def test_absent_anchor_is_warned_even_when_l1_is_empty(self):
|
||||
# The anchor can be verified without an L1 key set, so the visibility
|
||||
# signal must not depend on the (separately gated) key comparison.
|
||||
tokens, report = _run_rule_e_docs({GLOSSARY_DOC: "`xrpl.tx.hash`"}, l1_keys=[])
|
||||
self.assertEqual(tokens, [])
|
||||
self.assertEqual(len(report.warnings), len(chk.RULE_E_REQUIRED_DOCS))
|
||||
self.assertTrue(any("SKIP: E" in s for s in report.skips))
|
||||
|
||||
def test_no_anchor_warning_when_discovery_finds_nothing(self):
|
||||
# Nothing to check at all is already reported by the SKIP line, which
|
||||
# names the roots it searched, so an anchor warning would be noise.
|
||||
tokens, report = _run_rule_e_docs({})
|
||||
self.assertEqual(tokens, [])
|
||||
self.assertEqual(report.warnings, [])
|
||||
roots = ", ".join(str(rel) for rel in chk.RULE_E_DOC_ROOTS)
|
||||
self.assertEqual(
|
||||
report.skips,
|
||||
[f"SKIP: E — no doc-layer file present (no *.md under {roots})"],
|
||||
)
|
||||
|
||||
def test_doc_outside_every_root_is_not_discovered(self):
|
||||
# The counterpart of the roots being committable: markdown parked outside
|
||||
# them is not part of the L5 layer, so a dotted key there cannot fail the
|
||||
# build — and equally cannot make the rule look like it checked something.
|
||||
tokens, report = _run_rule_e_docs({"elsewhere/notes.md": "`xrpl.tx.hash`"})
|
||||
self.assertEqual(tokens, [])
|
||||
self.assertEqual(report.checked, [])
|
||||
self.assertTrue(any("SKIP: E" in s for s in report.skips))
|
||||
|
||||
|
||||
class DslParser(unittest.TestCase):
|
||||
"""The makeStr/join/seg:: constexpr DSL resolver — the foundation of the
|
||||
L1 key set. Covers flat, nested, cross-file, alias, and multi-line forms."""
|
||||
@@ -1068,7 +1661,7 @@ class RuleEReportTuple(unittest.TestCase):
|
||||
(d / "docs").mkdir()
|
||||
(d / "docs" / "telemetry-runbook.md").write_text("`xrpl.tx.hash`")
|
||||
report = chk.Report()
|
||||
chk.run_rule_e_runbook(d, {"xrpl.network.id"}, report)
|
||||
chk.run_rule_e_docs(d, {"xrpl.network.id"}, report)
|
||||
self.assertEqual(len(report.violations), 1)
|
||||
rule, _loc, token, expected = report.violations[0]
|
||||
self.assertEqual(rule, "E")
|
||||
@@ -1085,7 +1678,7 @@ class RuleEReportTuple(unittest.TestCase):
|
||||
"`tx_hash` `consensus.round`"
|
||||
)
|
||||
report = chk.Report()
|
||||
chk.run_rule_e_runbook(d, {"tx_hash"}, report)
|
||||
chk.run_rule_e_docs(d, {"tx_hash"}, report)
|
||||
self.assertEqual(report.violations, [])
|
||||
self.assertTrue(any("E:" in c for c in report.checked))
|
||||
finally:
|
||||
|
||||
10
.github/workflows/on-pr.yml
vendored
10
.github/workflows/on-pr.yml
vendored
@@ -77,6 +77,16 @@ jobs:
|
||||
.github/workflows/reusable-check-rename.yml
|
||||
.github/workflows/on-pr.yml
|
||||
|
||||
# The non-code layers the OTel naming check validates: the docs that
|
||||
# publish attribute tables (Rule E) and the telemetry stack config —
|
||||
# collector, Tempo, dashboards (Rules B, C, D). Without these paths a
|
||||
# docs-only or dashboard-only pull request sets `go=false`, so the
|
||||
# very layers those rules exist to police would never be checked.
|
||||
# As with `README.md` below, matching one of these also switches on
|
||||
# the rest of the workflow; there is a single `go` gate.
|
||||
docs/**
|
||||
docker/telemetry/**
|
||||
|
||||
# Keep the paths below in sync with those in `on-trigger.yml`.
|
||||
.github/actions/build-deps/**
|
||||
.github/actions/generate-version/**
|
||||
|
||||
@@ -21,6 +21,13 @@ jobs:
|
||||
steps:
|
||||
- name: Checkout repository
|
||||
uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3
|
||||
- name: Test the OTel naming checker
|
||||
# The checker's own unit tests, run before the check itself so a broken
|
||||
# rule is reported as a broken rule rather than as a naming violation
|
||||
# (or, worse, as a rule that silently stops flagging anything).
|
||||
# stdlib `unittest` only: the repo installs no third-party test runner
|
||||
# for CI, and the checker itself is deliberately dependency-free.
|
||||
run: python -m unittest discover -s .github/scripts/otel-naming -p 'test_*.py' --verbose
|
||||
- name: Check OTel naming
|
||||
# The script is stdlib-only and reads only files already in the tree;
|
||||
# it enforces each rule only when the layer it needs is present, so it
|
||||
|
||||
48
.github/workflows/telemetry-validation.yml
vendored
48
.github/workflows/telemetry-validation.yml
vendored
@@ -26,25 +26,32 @@ name: Telemetry Validation
|
||||
|
||||
on:
|
||||
workflow_dispatch:
|
||||
# NOTE: rpc_rate / rpc_duration / tx_tps / tx_duration have NO effect.
|
||||
# They are forwarded to run-full-validation.sh, which parses them into
|
||||
# shell variables and never reads them again — load shape comes entirely
|
||||
# from --profile and docker/telemetry/workload/workload-profiles.json.
|
||||
# They are kept (and labelled) rather than removed so existing dispatch
|
||||
# bookmarks and any saved input sets do not break. To change the load,
|
||||
# edit or add a profile in workload-profiles.json.
|
||||
inputs:
|
||||
rpc_rate:
|
||||
description: "RPC load rate (requests per second)"
|
||||
description: "UNUSED — has no effect. Load shape comes from the workload profile."
|
||||
required: false
|
||||
default: "50"
|
||||
rpc_duration:
|
||||
description: "RPC load duration (seconds)"
|
||||
description: "UNUSED — has no effect. Load shape comes from the workload profile."
|
||||
required: false
|
||||
default: "120"
|
||||
tx_tps:
|
||||
description: "Transaction submit rate (TPS)"
|
||||
description: "UNUSED — has no effect. Load shape comes from the workload profile."
|
||||
required: false
|
||||
default: "5"
|
||||
tx_duration:
|
||||
description: "Transaction submit duration (seconds)"
|
||||
description: "UNUSED — has no effect. Load shape comes from the workload profile."
|
||||
required: false
|
||||
default: "120"
|
||||
run_benchmark:
|
||||
description: "Run performance benchmarks"
|
||||
description: "Run performance benchmarks (the only input that changes behaviour)"
|
||||
required: false
|
||||
type: boolean
|
||||
default: false
|
||||
@@ -54,11 +61,19 @@ on:
|
||||
- "pratik/otel-phase*"
|
||||
- "feature/otel-*"
|
||||
- "feature/telemetry-*"
|
||||
# Keep these globs pointing at paths that actually exist. Two earlier
|
||||
# entries (include/xrpl/basics/Telemetry*.h, src/xrpld/app/misc/Telemetry*)
|
||||
# matched zero tracked files, so a pure C++ telemetry change never
|
||||
# triggered this workflow on push — only edits under docker/telemetry/**
|
||||
# or to this file did. The telemetry sources live in the three telemetry
|
||||
# module directories below.
|
||||
paths:
|
||||
- ".github/workflows/telemetry-validation.yml"
|
||||
- "docker/telemetry/**"
|
||||
- "include/xrpl/basics/Telemetry*.h"
|
||||
- "src/xrpld/app/misc/Telemetry*"
|
||||
- "include/xrpl/telemetry/**"
|
||||
- "src/libxrpl/telemetry/**"
|
||||
- "src/libxrpl/beast/insight/**"
|
||||
- "src/xrpld/telemetry/**"
|
||||
|
||||
concurrency:
|
||||
group: telemetry-validation-${{ github.ref }}
|
||||
@@ -141,6 +156,13 @@ jobs:
|
||||
build_type: Release
|
||||
log_verbosity: verbose
|
||||
|
||||
# telemetry is passed explicitly even though the CMake option and the
|
||||
# Conan recipe both default it on. The whole point of this workflow is to
|
||||
# exercise telemetry, so it should not silently depend on a default it
|
||||
# does not control: if that default ever flips, every span and metric
|
||||
# assertion would fail for a reason no log names. Stated here, a build
|
||||
# without the dependency fails loudly instead, because CMakeLists.txt
|
||||
# does find_package(opentelemetry-cpp CONFIG REQUIRED) under this option.
|
||||
- name: Configure CMake
|
||||
working-directory: ${{ env.BUILD_DIR }}
|
||||
run: |
|
||||
@@ -148,6 +170,7 @@ jobs:
|
||||
-G Ninja \
|
||||
-DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake \
|
||||
-DCMAKE_BUILD_TYPE=Release \
|
||||
-Dtelemetry=ON \
|
||||
..
|
||||
|
||||
- name: Build xrpld
|
||||
@@ -206,6 +229,11 @@ jobs:
|
||||
TX_DURATION: ${{ github.event.inputs.tx_duration || '120' }}
|
||||
RUN_BENCHMARK: ${{ github.event.inputs.run_benchmark }}
|
||||
run: |
|
||||
# The four rate/duration flags below are inert (see the
|
||||
# workflow_dispatch inputs note): run-full-validation.sh parses them
|
||||
# and never reads them. Load shape comes from the default
|
||||
# --profile full-validation. They are still passed so the flags stay
|
||||
# exercised if they are ever wired up.
|
||||
ARGS="--xrpld ${{ env.BUILD_DIR }}/xrpld --skip-loki"
|
||||
ARGS="$ARGS --rpc-rate $RPC_RATE"
|
||||
ARGS="$ARGS --rpc-duration $RPC_DURATION"
|
||||
@@ -234,6 +262,11 @@ jobs:
|
||||
# and `if: failure()` never fires -- which silently skipped these logs on
|
||||
# every failed run, and they are the only record of why a node did not
|
||||
# reach consensus.
|
||||
#
|
||||
# stdout.log matters as much as debug.log: a node that dies before its
|
||||
# log sink opens writes no debug.log at all, so stdout is the only place
|
||||
# its reason survives. A run that timed out at 4/5 nodes was left
|
||||
# undiagnosable because that file was not collected.
|
||||
- name: Upload node logs
|
||||
if: always() && steps.validation.outcome != 'success'
|
||||
uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
|
||||
@@ -241,6 +274,7 @@ jobs:
|
||||
name: xrpld-node-logs
|
||||
path: |
|
||||
/tmp/xrpld-validation/node*/debug.log
|
||||
/tmp/xrpld-validation/node*/stdout.log
|
||||
/tmp/xrpld-validation/*.log
|
||||
retention-days: 7
|
||||
if-no-files-found: warn
|
||||
|
||||
Reference in New Issue
Block a user