diff --git a/.github/scripts/otel-naming/README.md b/.github/scripts/otel-naming/README.md index d0a6b9f603..c27c8cf5e6 100644 --- a/.github/scripts/otel-naming/README.md +++ b/.github/scripts/otel-naming/README.md @@ -70,6 +70,7 @@ still caught. | ---- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | 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. | | L | A literal metric name in a family that has no `*MetricNames.h` constants yet. Rule I's ratchet defers these instead of failing the build on the whole pre-existing metric surface at once; the warning keeps the outstanding conversion work visible rather than silently accepted. | +| 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 diff --git a/.github/scripts/otel-naming/check_otel_naming.py b/.github/scripts/otel-naming/check_otel_naming.py index 5d8aaf104b..5ca897aea7 100644 --- a/.github/scripts/otel-naming/check_otel_naming.py +++ b/.github/scripts/otel-naming/check_otel_naming.py @@ -127,6 +127,10 @@ Warnings (printed, but do NOT fail the build) 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 outstanding conversion work visible instead of silently accepted. + 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. @@ -137,7 +141,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 @@ -526,6 +530,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 @@ -804,6 +811,173 @@ def iter_calls(text: str): yield name, arglist, lineno +# --------------------------------------------------------------------------- +# Rule M: L1 constants nothing references (the reverse direction) +# --------------------------------------------------------------------------- + +# A scope event: `namespace {`, 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 `::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: `::` 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 `::` 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 + `::`, 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(): @@ -1595,15 +1769,6 @@ def run_rule_i_metric_literals(root: Path, report: Report) -> None: report.ok("I: no string-literal names/label keys in converted metric families") -def iter_sources(root: Path) -> List[Path]: - """Every C++ source/header under src/ and include/.""" - return [ - p - for base in ("src", "include") - for ext in ("*.h", "*.cpp") - for p in (root / base).rglob(ext) - if p.is_file() - ] def instrument_kinds(root: Path, wire_by_symbol: Dict[str, str]) -> Dict[str, Set[str]]: diff --git a/.github/scripts/otel-naming/test_check_otel_naming.py b/.github/scripts/otel-naming/test_check_otel_naming.py index 7942bcba1e..9a2447c8f0 100644 --- a/.github/scripts/otel-naming/test_check_otel_naming.py +++ b/.github/scripts/otel-naming/test_check_otel_naming.py @@ -1234,6 +1234,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()) diff --git a/docker/telemetry/xrpld-telemetry.cfg b/docker/telemetry/xrpld-telemetry.cfg index b362465b7a..325e195b95 100644 --- a/docker/telemetry/xrpld-telemetry.cfg +++ b/docker/telemetry/xrpld-telemetry.cfg @@ -108,15 +108,13 @@ data/logs/devnet/debug.log # --- Insight (native OTel metrics via beast::insight) ----------------------- +# server is the only key that changes behaviour here. No prefix: formatName() +# ignores it, so names stay bare (jobq_job_count). service_instance_id is read +# and discarded (OTelCollector.cpp `(void)instanceId`); the label Prometheus +# shows comes from [telemetry] service_instance_id below. [insight] server=otel endpoint=http://localhost:4318/v1/metrics -prefix=xrpld -# Sets the OTel service.instance.id resource attribute, which Prometheus -# exposes as the `service_instance_id` label. Dashboards filter on it via the -# $node template variable, so without this every insight-backed panel is -# empty. Matches [telemetry] service_instance_id for a single node identity. -service_instance_id=xrpld-devnet # --- OpenTelemetry tracing --------------------------------------------------