mirror of
https://github.com/XRPLF/rippled.git
synced 2026-09-27 15:28:03 +00:00
ci(telemetry): warn when a span constant no longer has a reference (rule M)
Every existing rule in this checker runs one way: take a consumer -- a collector dimension, a Tempo tag, a dashboard label, a doc, an asserted metric name -- and require it to resolve to the *SpanNames.h constants. Rule H looks closest to the reverse but is still consumer-side: a constant USED at a call site that no header defines. Nothing looked the other way. So deleting a setAttribute from a .cpp and leaving its constant in the header passed every rule and every compiler, while the telemetry it described stopped being emitted. The workload validation job would eventually notice, but only when it happens to run, and its path filter deliberately does not watch daemon .cpp files -- widening it to 1827 C++ files to catch this would fire a twenty-minute Docker job on nearly every commit. Rule M closes that: an L1 constant no code under src/ or include/ references. It searches all references, not just telemetry call sites, because constants are passed to helpers, stored in locals and used as attribute VALUES -- a call-site-only scan would report false positives. Constants referenced only by test code are reported separately, since a constant exercised by a test but by no production path is still dead in production. A WARNING, not a failure, for two reasons that are both real here. Six constants in this tree are already dead, so failing would redden the branch immediately. And in a stacked chain a constant legitimately lands one commit before its call site, so a failing rule would break intermediate branches for a condition that resolves downstream. What it reports today, all verified unreferenced across the whole repository and not just src/include: ConsensusSpanNames.h val::increased, val::decreased and val::unchanged; SpanNames.h seg::link, attr_val::success and attr_val::error. Placed here rather than upstream on phase-1c, where the checker was introduced, because the gap it closes is a workload-harness concern -- the contract asserting a name nothing emits -- and the harness is this branch's. Putting it on 1c would also mean union-resolving a 1900-line file across ten merge hops, each an opportunity to silently drop the metric rules that live downstream. Ported from a patch written against the sync-diagnostics copy, which carries the metric-side rules I/J/K/L that this branch does not. The rule itself is purely span-side; the only shared dependency it needed was iter_sources, which arrived with those metric rules, so that helper is added here on its own. The rule-L docstring entry and README row that came with the patch context were dropped -- this branch has no rule L. Verification: rule M reports 262 constants checked and 6 unreferenced, exit code still 0 because warnings do not change it; 169 unittest cases pass; the checker compiles; no metric-rule content leaked in from the patch context (0 occurrences of METRIC_MACRO_CALL or run_rule_i_metric_literals). Proven non-vacuous by blanking all three call sites of attr::rpcStatus -- the count went 6 to 7 and named that constant -- then restoring them and watching it return to 6.
This commit is contained in:
Pratik Mankawde
7
.github/scripts/otel-naming/README.md
vendored
7
.github/scripts/otel-naming/README.md
vendored
@@ -55,9 +55,10 @@ still caught.
|
||||
|
||||
### Warnings (printed, never fail the build)
|
||||
|
||||
| Rule | Check |
|
||||
| ---- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
|
||||
| H | A namespace-qualified constant (e.g. `foo::bar::myKey`) used at a telemetry call-site is not defined in any `*SpanNames.h`. The constant should live in the proper header; defining it in-place bypasses rules A/G/F. Warns rather than fails — the argument may be a legitimately dynamic value, and the header may live on a later branch. Bare locals and `std::` names are not warned. |
|
||||
| Rule | Check |
|
||||
| ---- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
|
||||
| H | A namespace-qualified constant (e.g. `foo::bar::myKey`) used at a telemetry call-site is not defined in any `*SpanNames.h`. The constant should live in the proper header; defining it in-place bypasses rules A/G/F. Warns rather than fails — the argument may be a legitimately dynamic value, and the header may live on a later branch. Bare locals and `std::` names are not warned. |
|
||||
| M | A constant defined in a `*SpanNames.h` that no code in `src/**` or `include/**` references — the reverse of every failing rule above, which all start from a consumer and look for its L1 source. Deleting the last `setAttribute(attr::foo, …)` while leaving `attr::foo` in the header otherwise passes every rule and the compiler, and the telemetry silently stops being emitted. Whole files are searched rather than telemetry call sites only, since a constant is also passed to helpers and used as an attribute _value_. Constants only test code references are reported separately. Warns rather than fails: in a stacked chain a constant may legitimately land a commit before its call site. |
|
||||
|
||||
## Presence-gated
|
||||
|
||||
|
||||
176
.github/scripts/otel-naming/check_otel_naming.py
vendored
176
.github/scripts/otel-naming/check_otel_naming.py
vendored
@@ -99,6 +99,10 @@ Warnings (printed, but do NOT fail the build)
|
||||
exemption). Warnings, not failures, because presence-gating must keep
|
||||
working on partial branches -- but the skip is now visible instead of
|
||||
silent.
|
||||
M A constant DEFINED in a *SpanNames.h that no code references — the reverse
|
||||
of every rule above, which all start from a consumer. Constants referenced
|
||||
only by test code are reported separately. A warning because a constant may
|
||||
legitimately land a commit before its call site in a stacked chain.
|
||||
|
||||
Exit code is non-zero if any present-and-enforced rule finds a violation.
|
||||
Warnings never change the exit code.
|
||||
@@ -109,7 +113,7 @@ import re
|
||||
import subprocess
|
||||
import sys
|
||||
from pathlib import Path
|
||||
from typing import Dict, List, Optional, Set, Tuple
|
||||
from typing import Dict, List, NamedTuple, Optional, Set, Tuple
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Repo location
|
||||
@@ -488,6 +492,9 @@ def main() -> None:
|
||||
header_symbols = spanname_symbol_names(headers)
|
||||
run_rule_f(root, report, header_symbols)
|
||||
|
||||
# --- Rule M (warning): L1 constants nothing references ------------------
|
||||
run_rule_m_unreferenced(root, headers, report)
|
||||
|
||||
# --- Cross-layer rules B/C/D/E (each presence-gated) -------------------
|
||||
# L6 native-metric labels: span attributes are not the only valid dashboard
|
||||
# labels — the MetricsRegistry emits OTel metrics whose label keys are an
|
||||
@@ -748,6 +755,173 @@ def iter_calls(text: str):
|
||||
yield name, arglist, lineno
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Rule M: L1 constants nothing references (the reverse direction)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
# A scope event: `namespace <name> {`, a bare `{`, or a `}`. Reuses NS_OPEN's
|
||||
# pattern, keeping its capture as group 1.
|
||||
NS_EVENT = re.compile(NS_OPEN.pattern + r"|\{|\}")
|
||||
IDENTIFIER_TOKEN = re.compile(r"[A-Za-z_]\w*")
|
||||
# A qualified name such as `ledger_span::attr::ledgerHash`, possibly wrapped
|
||||
# across lines. Bare names are useless here: `JSS(aborted)` in jss.h would
|
||||
# vouch for `val::aborted`.
|
||||
QUALIFIED_CHAIN = re.compile(r"[A-Za-z_]\w*(?:\s*::\s*[A-Za-z_]\w*)+")
|
||||
# Wider than the .h/.cpp the other rules parse: a file missed here would make a
|
||||
# live constant look dead.
|
||||
REFERENCE_EXTENSIONS = ("*.h", "*.cpp", "*.ipp", "*.hpp")
|
||||
RULE_M_DEAD = "no reference in src/ or include/"
|
||||
RULE_M_TEST_ONLY = "referenced only by test code"
|
||||
|
||||
|
||||
def iter_sources(
|
||||
root: Path, extensions: Tuple[str, ...] = ("*.h", "*.cpp")
|
||||
) -> List[Path]:
|
||||
"""Every C++ source/header under src/ and include/. Rule M passes a wider
|
||||
`extensions` set; see REFERENCE_EXTENSIONS."""
|
||||
return [
|
||||
p
|
||||
for base in ("src", "include")
|
||||
for ext in extensions
|
||||
for p in (root / base).rglob(ext)
|
||||
if p.is_file()
|
||||
]
|
||||
|
||||
|
||||
def constant_definitions(text: str) -> List[Tuple[str, str]]:
|
||||
"""Every `inline constexpr auto NAME = ...;`, as `(name, qualified)` where
|
||||
`qualified` is `<innermost namespace>::NAME` — the form to grep for.
|
||||
|
||||
String literals are blanked to same-length spacing before the brace scan, so
|
||||
a `{` inside a literal is not read as a scope and offsets stay valid."""
|
||||
masked = STRING_LITERAL.sub(lambda m: " " * len(m.group(0)), text)
|
||||
events: List[Tuple[int, bool, Optional[str]]] = []
|
||||
for m in NS_EVENT.finditer(masked):
|
||||
if m.group(1) is not None:
|
||||
# `namespace a::b {` opens one brace; the innermost name is the
|
||||
# qualifier a call site writes.
|
||||
events.append((m.start(), True, m.group(1).split("::")[-1]))
|
||||
elif m.group(0) == "{":
|
||||
events.append((m.start(), True, None))
|
||||
else:
|
||||
events.append((m.start(), False, None))
|
||||
out: List[Tuple[str, str]] = []
|
||||
stack: List[Optional[str]] = []
|
||||
event_idx = 0
|
||||
for m in CONST_DEF.finditer(text):
|
||||
while event_idx < len(events) and events[event_idx][0] < m.start():
|
||||
_, is_open, ns = events[event_idx]
|
||||
if is_open:
|
||||
stack.append(ns)
|
||||
elif stack:
|
||||
stack.pop()
|
||||
event_idx += 1
|
||||
inner = next((ns for ns in reversed(stack) if ns), None)
|
||||
name = m.group(1)
|
||||
out.append((name, f"{inner}::{name}" if inner else name))
|
||||
return out
|
||||
|
||||
|
||||
class References(NamedTuple):
|
||||
"""Reference forms found in one body of code: `<outer>::<inner>` pairs, plus
|
||||
bare identifiers (needed only for a constant outside any namespace)."""
|
||||
|
||||
pairs: Set[str]
|
||||
bare: Set[str]
|
||||
|
||||
|
||||
def qualified_references(text: str) -> Set[str]:
|
||||
"""Every adjacent `<outer>::<inner>` pair, so a constant is found however
|
||||
deeply its call site qualifies it.
|
||||
|
||||
Whole chains are split rather than matched two segments at a time: a
|
||||
non-overlapping regex over `a::b::c` eats `a::b` and never sees `b::c`."""
|
||||
pairs: Set[str] = set()
|
||||
for chain in QUALIFIED_CHAIN.finditer(text):
|
||||
segments = [s.strip() for s in chain.group(0).split("::")]
|
||||
for outer, inner in zip(segments, segments[1:]):
|
||||
pairs.add(f"{outer}::{inner}")
|
||||
return pairs
|
||||
|
||||
|
||||
def referenced_constants(root: Path) -> Tuple[References, References]:
|
||||
"""References made by production code and by test code, across `src/**` and
|
||||
`include/**`.
|
||||
|
||||
Comments are stripped: a constant a doc comment only mentions is still dead.
|
||||
In a `*SpanNames.h`, `using` re-exports are dropped too — an unused alias
|
||||
must not vouch for the constant it renames."""
|
||||
prod = References(set(), set())
|
||||
tests = References(set(), set())
|
||||
for path in iter_sources(root, REFERENCE_EXTENSIONS):
|
||||
text = strip_comments(read_source(path))
|
||||
if path.name.endswith("SpanNames.h"):
|
||||
text = USING_DECL.sub("", text)
|
||||
target = tests if is_test_path(path) else prod
|
||||
target.pairs.update(qualified_references(text))
|
||||
target.bare.update(IDENTIFIER_TOKEN.findall(text))
|
||||
return prod, tests
|
||||
|
||||
|
||||
def sibling_bare_tokens(text: str) -> Set[str]:
|
||||
"""Bare identifiers on the right-hand side of one header's definitions, so a
|
||||
sibling composed unqualified (`join(prefix, op::x)`) counts as used.
|
||||
|
||||
String literals are removed first, so `makeStr("header")` cannot vouch for a
|
||||
constant named `header`."""
|
||||
tokens: Set[str] = set()
|
||||
for m in CONST_DEF.finditer(text):
|
||||
tokens.update(IDENTIFIER_TOKEN.findall(STRING_LITERAL.sub("", m.group(2))))
|
||||
return tokens
|
||||
|
||||
|
||||
def constant_is_referenced(name: str, qualified: str, refs: References) -> bool:
|
||||
"""True if `refs` names the constant by its qualified form, or by its bare
|
||||
name when it sits outside any namespace and so has no qualified form."""
|
||||
if qualified in refs.pairs:
|
||||
return True
|
||||
return qualified == name and name in refs.bare
|
||||
|
||||
|
||||
def run_rule_m_unreferenced(root: Path, headers: List[Path], report: Report) -> None:
|
||||
"""Rule M (WARN): a `*SpanNames.h` constant that no code references.
|
||||
|
||||
The reverse of the other rules, which start from a consumer and look for its
|
||||
L1 source. Warns rather than fails: in a stacked chain a constant can
|
||||
legitimately land a commit before its call site. A constant only test code
|
||||
names is reported separately, since the fix differs.
|
||||
|
||||
Whole files are searched, not just telemetry call sites, because a constant
|
||||
is also passed to helpers and used as an attribute VALUE. Matching is by
|
||||
`<innermost namespace>::<name>`, so two headers declaring the same qualified
|
||||
form share one key — under-reporting, the safe direction.
|
||||
|
||||
Presence-gated on `*SpanNames.h` existing."""
|
||||
if not headers:
|
||||
report.skip("M", "no *SpanNames.h present")
|
||||
return
|
||||
prod, tests = referenced_constants(root)
|
||||
total = dead = test_only = 0
|
||||
for h in sorted(headers):
|
||||
rel = str(h.relative_to(root))
|
||||
text = strip_comments(read_source(h))
|
||||
siblings = sibling_bare_tokens(text)
|
||||
for name, qualified in constant_definitions(text):
|
||||
total += 1
|
||||
if constant_is_referenced(name, qualified, prod) or name in siblings:
|
||||
continue
|
||||
if constant_is_referenced(name, qualified, tests):
|
||||
test_only += 1
|
||||
report.warning("M", rel, qualified, RULE_M_TEST_ONLY)
|
||||
else:
|
||||
dead += 1
|
||||
report.warning("M", rel, qualified, RULE_M_DEAD)
|
||||
note = f"M: {total} *SpanNames.h constant(s) checked for references"
|
||||
if dead or test_only:
|
||||
note += f" ({dead} unreferenced, {test_only} test-only — see warnings)"
|
||||
report.ok(note)
|
||||
|
||||
|
||||
def run_rule_b_collector(root: Path, l1_keys: Set[str], report: Report) -> None:
|
||||
path = root / "docker" / "telemetry" / "otel-collector-config.yaml"
|
||||
if not path.is_file():
|
||||
|
||||
@@ -1230,6 +1230,196 @@ class RuleFAndH(unittest.TestCase):
|
||||
self.assertEqual(w, [])
|
||||
|
||||
|
||||
class RuleMUnreferencedConstants(unittest.TestCase):
|
||||
"""Rule M: an L1 constant defined in a `*SpanNames.h` that nothing in
|
||||
`src/**` or `include/**` references."""
|
||||
|
||||
# The two notes Rule M distinguishes; they must not be folded together.
|
||||
DEAD = "no reference in src/ or include/"
|
||||
TEST_ONLY = "referenced only by test code"
|
||||
|
||||
def _run(self, files):
|
||||
"""Build a synthetic tree, run Rule M, return (sorted (token, note)
|
||||
pairs, the Report)."""
|
||||
d = Path(tempfile.mkdtemp())
|
||||
try:
|
||||
for rel, text in files.items():
|
||||
_write(d / rel, text)
|
||||
report = chk.Report()
|
||||
chk.run_rule_m_unreferenced(d, chk.find_spanname_headers(d), report)
|
||||
return sorted((w[2], w[3]) for w in report.warnings), report
|
||||
finally:
|
||||
shutil.rmtree(d)
|
||||
|
||||
HEADER = "include/xrpl/telemetry/DemoSpanNames.h"
|
||||
|
||||
def _two_attrs(self):
|
||||
return _header(
|
||||
'inline constexpr auto used = makeStr("used_key");\n'
|
||||
'inline constexpr auto unused = makeStr("unused_key");\n'
|
||||
)
|
||||
|
||||
# ----- positive: a defined-but-unreferenced constant is warned -----
|
||||
def test_unreferenced_constant_warned(self):
|
||||
warnings, _ = self._run(
|
||||
{
|
||||
self.HEADER: self._two_attrs(),
|
||||
"src/xrpld/app/Foo.cpp": "g.setAttribute(demo::attr::used, v);\n",
|
||||
}
|
||||
)
|
||||
self.assertEqual(warnings, [("attr::unused", self.DEAD)])
|
||||
|
||||
def test_warning_location_is_the_defining_header(self):
|
||||
_, report = self._run(
|
||||
{
|
||||
self.HEADER: self._two_attrs(),
|
||||
"src/xrpld/app/Foo.cpp": "g.setAttribute(demo::attr::used, v);\n",
|
||||
}
|
||||
)
|
||||
self.assertEqual(len(report.warnings), 1)
|
||||
self.assertEqual(report.warnings[0][0], "M")
|
||||
self.assertEqual(report.warnings[0][1], self.HEADER)
|
||||
|
||||
def test_lone_header_flags_every_constant(self):
|
||||
# Proves the definition itself is not counted as a reference: with no
|
||||
# other file in the tree, BOTH constants are dead.
|
||||
warnings, _ = self._run({self.HEADER: self._two_attrs()})
|
||||
self.assertEqual(
|
||||
warnings, [("attr::unused", self.DEAD), ("attr::used", self.DEAD)]
|
||||
)
|
||||
|
||||
def test_comment_mention_is_not_a_reference(self):
|
||||
warnings, _ = self._run(
|
||||
{
|
||||
self.HEADER: self._two_attrs(),
|
||||
"src/xrpld/app/Foo.cpp": (
|
||||
"g.setAttribute(demo::attr::used, v);\n"
|
||||
"// see demo::attr::unused for the aborted case\n"
|
||||
"/* demo::attr::unused */\n"
|
||||
),
|
||||
}
|
||||
)
|
||||
self.assertEqual(warnings, [("attr::unused", self.DEAD)])
|
||||
|
||||
def test_using_reexport_is_not_a_reference(self):
|
||||
# An unused `using` re-export must not vouch for the constant it
|
||||
# renames. Fails if referenced_constants stops stripping USING_DECL.
|
||||
warnings, _ = self._run(
|
||||
{
|
||||
self.HEADER: self._two_attrs(),
|
||||
"include/xrpl/telemetry/PeerSpanNames.h": _header(
|
||||
"using ::xrpl::telemetry::demo::span::attr::unused;\n"
|
||||
),
|
||||
"src/xrpld/app/Foo.cpp": "g.setAttribute(demo::attr::used, v);\n",
|
||||
}
|
||||
)
|
||||
self.assertEqual(warnings, [("attr::unused", self.DEAD)])
|
||||
|
||||
# ----- negative: a referenced constant must NOT be warned -----
|
||||
def test_referenced_constant_not_warned(self):
|
||||
warnings, report = self._run(
|
||||
{
|
||||
self.HEADER: _header('inline constexpr auto used = makeStr("k");\n'),
|
||||
"src/xrpld/app/Foo.cpp": "g.setAttribute(demo::attr::used, v);\n",
|
||||
}
|
||||
)
|
||||
self.assertEqual(warnings, [])
|
||||
self.assertTrue(any("M:" in c for c in report.checked))
|
||||
|
||||
def test_reference_as_an_attribute_value_counts(self):
|
||||
# Value constants are referenced in the VALUE position, which a scan of
|
||||
# key positions only would never see.
|
||||
warnings, _ = self._run(
|
||||
{
|
||||
self.HEADER: _header(
|
||||
'inline constexpr auto outcome = makeStr("outcome");\n'
|
||||
'inline constexpr auto complete = makeStr("complete");\n'
|
||||
),
|
||||
"src/xrpld/app/Foo.cpp": (
|
||||
"g.setAttribute(demo::attr::outcome, demo::attr::complete);\n"
|
||||
),
|
||||
}
|
||||
)
|
||||
self.assertEqual(warnings, [])
|
||||
|
||||
def test_reference_through_a_local_counts(self):
|
||||
# Constants are passed to helpers and stored in locals, not only used
|
||||
# inline at a telemetry call.
|
||||
warnings, _ = self._run(
|
||||
{
|
||||
self.HEADER: _header('inline constexpr auto used = makeStr("k");\n'),
|
||||
"src/xrpld/app/Foo.cpp": (
|
||||
"auto key = demo::attr::used;\nrecord(key, v);\n"
|
||||
),
|
||||
}
|
||||
)
|
||||
self.assertEqual(warnings, [])
|
||||
|
||||
def test_reference_from_a_join_in_its_own_header_counts(self):
|
||||
# `op::leaf` is consumed only by a `join(...)` in the same header; the
|
||||
# composed constant is what the call site names. Neither is dead.
|
||||
warnings, _ = self._run(
|
||||
{
|
||||
self.HEADER: (
|
||||
"#pragma once\n"
|
||||
"namespace demo {\n"
|
||||
"namespace seg {\n"
|
||||
'inline constexpr auto demo = makeStr("demo");\n'
|
||||
"}\n"
|
||||
"namespace op {\n"
|
||||
'inline constexpr auto leaf = makeStr("leaf");\n'
|
||||
"}\n"
|
||||
"namespace span {\n"
|
||||
"inline constexpr auto full = join(seg::demo, op::leaf);\n"
|
||||
"}\n"
|
||||
"}\n"
|
||||
),
|
||||
"src/xrpld/app/Foo.cpp": "auto s = g.childSpan(demo::span::full);\n",
|
||||
}
|
||||
)
|
||||
self.assertEqual(warnings, [])
|
||||
|
||||
# ----- test-only references are a separate, named class -----
|
||||
def test_test_only_reference_reported_separately(self):
|
||||
warnings, _ = self._run(
|
||||
{
|
||||
self.HEADER: self._two_attrs(),
|
||||
"src/xrpld/app/Foo.cpp": "g.setAttribute(demo::attr::used, v);\n",
|
||||
"src/tests/libxrpl/telemetry/SpanNames.cpp": (
|
||||
'EXPECT_EQ(demo::attr::unused, "unused_key");\n'
|
||||
),
|
||||
}
|
||||
)
|
||||
self.assertEqual(warnings, [("attr::unused", self.TEST_ONLY)])
|
||||
|
||||
def test_production_reference_wins_over_test_reference(self):
|
||||
warnings, _ = self._run(
|
||||
{
|
||||
self.HEADER: _header('inline constexpr auto used = makeStr("k");\n'),
|
||||
"src/xrpld/app/Foo.cpp": "g.setAttribute(demo::attr::used, v);\n",
|
||||
"src/tests/libxrpl/telemetry/SpanNames.cpp": (
|
||||
'EXPECT_EQ(demo::attr::used, "k");\n'
|
||||
),
|
||||
}
|
||||
)
|
||||
self.assertEqual(warnings, [])
|
||||
|
||||
# ----- boundary -----
|
||||
def test_skips_when_no_headers(self):
|
||||
d = Path(tempfile.mkdtemp())
|
||||
try:
|
||||
report = chk.Report()
|
||||
chk.run_rule_m_unreferenced(d, [], report)
|
||||
self.assertEqual(report.warnings, [])
|
||||
self.assertTrue(any("SKIP: M" in s for s in report.skips))
|
||||
finally:
|
||||
shutil.rmtree(d)
|
||||
|
||||
def test_never_fails_the_build(self):
|
||||
_, report = self._run({self.HEADER: self._two_attrs()})
|
||||
self.assertEqual(report.violations, [])
|
||||
|
||||
|
||||
class RuleBCollector(unittest.TestCase):
|
||||
def _run(self, yaml_text, l1):
|
||||
d = Path(tempfile.mkdtemp())
|
||||
|
||||
Reference in New Issue
Block a user