Merge branch 'pratik/otel-phase3-tx-tracing' into pratik/otel-phase4-consensus-tracing

.github/scripts/otel-naming/README.md

@@ -25,11 +25,16 @@ hardcoded allowlist:
 - **L1 keys** come from the `namespace attr { ... }` blocks of every
   `*SpanNames.h`, resolving the `makeStr("x")` / `join(seg::a, seg::b)` DSL
   (cross-file, so `join(seg::rpc, ...)` resolves `seg::rpc` from the base
-  `SpanNames.h`).
-- **Legitimate dotted keys** = the resource attrs declared in the base
-  `SpanNames.h` (`xrpl.network.*`) plus the `semconv::service::*` keys the code
-  passes to `Resource::Create()` in `Telemetry.cpp` (`service.*`). A dotted key
-  declared in any other header is a violation.
+  `SpanNames.h`). Each constant is resolved against **its own** header, so two
+  headers that define a same-named constant (e.g. a base `attr::ledgerHash` and
+  a domain `attr::ledgerHash`) each contribute their real wire key — a later
+  header cannot clobber an earlier one's value in a flat table.
+- **Legitimate dotted keys** = ONLY the keys the code actually sets as resource
+  attributes, i.e. the entries inside `Telemetry.cpp`'s `Resource::Create({...})`
+  call: the `semconv::service::*` keys (`service.*`) plus any `attr::<name>`
+  constants passed there (`xrpl.network.*`). A dotted key that is _declared_ in a
+  header but never set as a resource attr is a span attribute in resource
+  clothing — a Rule-A violation, even if it lives in the base `SpanNames.h`.
 
 ### Rules (each fails the build, when its inputs are present)
 

.github/scripts/otel-naming/check_otel_naming.py

@@ -297,22 +297,39 @@ def attr_namespace_spans(text: str) -> List[str]:
 def attr_keys_from_header(path: Path, symbols: Dict[str, str]) -> Set[str]:
     """Return the set of attribute-key strings declared in a header's
     `namespace attr { ... }` block(s). `symbols` is the global cross-file
-    table so `join(seg::rpc, ...)` style dotted attrs resolve correctly.
+    table, used ONLY to seed `seg::`/segment references for `join(...)`
+    resolution — never to look up an attr constant's value.
+
+    A constant DEFINED in this header is resolved against this header's OWN
+    text, so two headers that each define a same-named constant (e.g. the base
+    `attr::ledgerHash = xrpl.ledger.hash` and consensus
+    `attr::ledgerHash = ledger_hash`) each report their real wire key. The
+    global table is keyed by bare name and would otherwise let a later header
+    clobber an earlier one, erasing the real key from L1 (a Rule-A blind spot).
+    A `using`-re-export, by contrast, imports a constant defined elsewhere, so
+    it is resolved against the global table.
 
     Comments are stripped first (a commented constant must not enter L1), and
     each attr block is brace-matched over the whole text so multi-line
     `inline constexpr auto NAME = join(\\n  ...);` definitions are captured."""
     text = strip_comments(read_source(path))
+    # Local table: the global segments/symbols seed cross-file `join` parts,
+    # then this header's own definitions overwrite any same-named global entry
+    # so a locally-defined attr resolves to ITS value, not another header's.
+    local = dict(symbols)
+    resolve_constants(text, local)
     keys: Set[str] = set()
     for block in attr_namespace_spans(text):
         for md in CONST_DEF.finditer(block):
-            # Resolve the named constant against the global symbol table; this
-            # captures makeStr("x"), join(seg::y, ...), and `using ::a::b` forms.
-            val = symbols.get(md.group(1))
+            # Resolve a locally-defined constant against the LOCAL table; this
+            # captures makeStr("x") and join(seg::y, ...) with the header's own
+            # value, immune to cross-header bare-name collisions.
+            val = local.get(md.group(1))
             if val is not None:
                 keys.add(val)
-        # `using ::ns::attr::field;` re-exports a base constant into this attr
-        # block (e.g. RpcSpanNames imports txHash). Resolve the imported name.
+        # `using ::ns::attr::field;` re-exports a constant defined in ANOTHER
+        # header (e.g. PeerSpanNames imports the base ledgerHash). Resolve the
+        # imported name against the global table.
         for um in USING_DECL.finditer(block):
             val = symbols.get(um.group(1))
             if val is not None:
@@ -400,10 +417,12 @@ def main() -> None:
         keys_by_header = {}
 
     # --- Derive the legitimate dotted (resource) keys dynamically ----------
-    # ONLY the base SpanNames.h resource attrs + the semconv keys Telemetry.cpp
-    # passes to Resource::Create(). A dotted key from any other header is a
-    # violation, not an allowlist entry.
-    dotted_allow = derive_dotted_resource_keys(root, keys_by_header, report)
+    # ONLY the keys actually passed to Resource::Create() in Telemetry.cpp
+    # (semconv service.* + the attr:: constants set there, e.g. xrpl.network.*).
+    # A dotted key declared in a header but NOT set as a resource attr is a
+    # Rule-A violation, not an allowlist entry.
+    resource_symbols = symbols if headers else {}
+    dotted_allow = derive_dotted_resource_keys(root, resource_symbols, report)
 
     # --- Rule A: no stray dotted span-attribute keys -----------------------
     if l1_keys:
@@ -434,35 +453,63 @@ def main() -> None:
     report.render_and_exit()
 
 
+def resource_create_block(text: str) -> str:
+    """Return the text inside the first `Resource::Create({ ... })` argument
+    list, brace-matched so nested `{key, value}` initializers are contained.
+    Empty string if the call is absent."""
+    m = re.search(r"Resource::Create\(\s*\{", text)
+    if not m:
+        return ""
+    i = m.end()  # one char past the opening `{`
+    depth, start = 1, i
+    while i < len(text) and depth > 0:
+        c = text[i]
+        if c == "{":
+            depth += 1
+        elif c == "}":
+            depth -= 1
+        i += 1
+    return text[start : i - 1]
+
+
 def derive_dotted_resource_keys(
-    root: Path, keys_by_header: Dict[Path, Set[str]], report: Report
+    root: Path, symbols: Dict[str, str], report: Report
 ) -> Set[str]:
-    """Legitimate dotted keys = dotted attrs declared ONLY in the base
-    SpanNames.h (xrpl.network.*) plus the standard semconv keys the code
-    passes to Resource::Create() in Telemetry.cpp (service.*). Dotted keys
-    declared in any OTHER header are NOT allowlisted — they are Rule-A
-    violations."""
+    """Legitimate dotted keys = ONLY the keys the code actually sets as RESOURCE
+    attributes, i.e. the entries inside Telemetry.cpp's `Resource::Create({...})`
+    call: the standard semconv keys (`service.*`) plus any `attr::<name>`
+    constants passed there (resolved to their wire key via the global symbol
+    table, e.g. `attr::networkId` -> `xrpl.network.id`).
+
+    A dotted key DECLARED in a `*SpanNames.h` header but NOT passed to
+    Resource::Create() is a span attribute wearing the resource form — a Rule-A
+    violation, never allowlisted. Deriving the allowlist from the actual
+    resource call (not from "any dotted key in the base header") is what lets
+    Rule A catch a stray dotted span attr such as `xrpl.ledger.hash`."""
     allow: Set[str] = set()
-    for h, keys in keys_by_header.items():
-        if h.name == "SpanNames.h":  # the base header holds resource attrs
-            allow |= {k for k in keys if "." in k}
     tele = root / "src" / "libxrpl" / "telemetry" / "Telemetry.cpp"
-    if tele.is_file():
-        text = read_source(tele)
-        # semconv::<group>::k<CamelKey> -> the dotted OTel-standard key. The
-        # CamelKey already embeds the group, e.g. service::kServiceInstanceId
-        # -> service.instance.id. Split the CamelCase name into dotted lowercase
-        # segments; if it does not lead with the group, prepend the group.
-        for m in re.finditer(r"semconv::(\w+)::k(\w+)", text):
-            group, camel = m.group(1), m.group(2)
-            segments = camel_to_dotsegments(camel)
-            if segments and segments[0] == group:
-                allow.add(".".join(segments))
-            else:
-                allow.add(group + "." + ".".join(segments))
-        report.ok(f"resource dotted-key allowlist derived: {sorted(allow)}")
-    else:
+    if not tele.is_file():
         report.skip("resource-derive", "Telemetry.cpp not present")
+        return allow
+    block = resource_create_block(read_source(tele))
+    # semconv::<group>::k<CamelKey> -> the dotted OTel-standard key. The
+    # CamelKey already embeds the group, e.g. service::kServiceInstanceId
+    # -> service.instance.id. Split the CamelCase name into dotted lowercase
+    # segments; if it does not lead with the group, prepend the group.
+    for m in re.finditer(r"semconv::(\w+)::k(\w+)", block):
+        group, camel = m.group(1), m.group(2)
+        segments = camel_to_dotsegments(camel)
+        if segments and segments[0] == group:
+            allow.add(".".join(segments))
+        else:
+            allow.add(group + "." + ".".join(segments))
+    # attr::<name> constants set as resource attrs (e.g. networkId/networkType);
+    # resolve each to its wire key and allowlist only the dotted ones.
+    for m in re.finditer(r"attr::(\w+)", block):
+        val = symbols.get(m.group(1))
+        if val is not None and "." in val:
+            allow.add(val)
+    report.ok(f"resource dotted-key allowlist derived: {sorted(allow)}")
     return allow
 
 

.github/scripts/otel-naming/test_check_otel_naming.py

@@ -326,6 +326,162 @@ class CamelToDotSegments(unittest.TestCase):
             shutil.rmtree(d)
 
 
+class SymbolCollision(unittest.TestCase):
+    """attr_keys_from_header must resolve a constant against ITS OWN header, so
+    two headers defining a same-named constant each report their real wire key.
+    Regression for the flat-symbol-table collision that let a later header
+    clobber an earlier one and erased a dotted key from L1 (a Rule-A blind
+    spot)."""
+
+    def _build(self, files):
+        d = Path(tempfile.mkdtemp())
+        paths = {}
+        for rel, text in files.items():
+            p = d / rel
+            _write(p, text)
+            paths[rel] = p
+        return d, paths
+
+    def test_same_named_const_not_clobbered_across_headers(self):
+        base = (
+            "#pragma once\n"
+            "namespace xrpl::telemetry {\n"
+            'namespace seg { inline constexpr auto xrpl = makeStr("xrpl");\n'
+            'inline constexpr auto ledger = makeStr("ledger"); }\n'
+            "namespace attr {\n"
+            "inline constexpr auto ledgerHash = "
+            'join(join(seg::xrpl, seg::ledger), makeStr("hash"));\n'
+            "}\n}\n"
+        )
+        cons = (
+            "#pragma once\n"
+            "namespace xrpl::telemetry::consensus::span {\n"
+            "namespace attr { inline constexpr auto ledgerHash = "
+            'makeStr("ledger_hash"); }\n}\n'
+        )
+        d, paths = self._build(
+            {
+                "include/xrpl/telemetry/SpanNames.h": base,
+                "src/xrpld/consensus/ConsensusSpanNames.h": cons,
+            }
+        )
+        try:
+            headers = chk.find_spanname_headers(d)
+            syms = chk.build_global_symbols(headers)
+            by_name = {p.name: chk.attr_keys_from_header(p, syms) for p in headers}
+            # The base header keeps its dotted key; consensus keeps the bare one.
+            self.assertIn("xrpl.ledger.hash", by_name["SpanNames.h"])
+            self.assertEqual(by_name["ConsensusSpanNames.h"], {"ledger_hash"})
+        finally:
+            shutil.rmtree(d)
+
+    def test_using_reexport_still_resolves_globally(self):
+        # A `using`-re-export imports a constant defined elsewhere; it must
+        # resolve against the global table, not the local header.
+        base = (
+            "#pragma once\n"
+            "namespace xrpl::telemetry {\n"
+            "namespace attr { inline constexpr auto txHash = "
+            'makeStr("tx_hash"); }\n}\n'
+        )
+        dom = (
+            "#pragma once\n"
+            "namespace xrpl::telemetry::tx::span {\n"
+            "namespace attr { using ::xrpl::telemetry::attr::txHash; }\n}\n"
+        )
+        d, paths = self._build(
+            {
+                "include/xrpl/telemetry/SpanNames.h": base,
+                "src/xrpld/app/misc/TxSpanNames.h": dom,
+            }
+        )
+        try:
+            headers = chk.find_spanname_headers(d)
+            syms = chk.build_global_symbols(headers)
+            keys = chk.attr_keys_from_header(
+                paths["src/xrpld/app/misc/TxSpanNames.h"], syms
+            )
+            self.assertEqual(keys, {"tx_hash"})
+        finally:
+            shutil.rmtree(d)
+
+
+class ResourceAllowlistScope(unittest.TestCase):
+    """derive_dotted_resource_keys must allowlist ONLY the dotted keys actually
+    passed to Resource::Create() — not every dotted key in the base header. A
+    dotted attr declared in a header but not set as a resource attr is a Rule-A
+    violation."""
+
+    def _derive(self, tele_text, span_text):
+        d = Path(tempfile.mkdtemp())
+        try:
+            _write(d / "src" / "libxrpl" / "telemetry" / "Telemetry.cpp", tele_text)
+            _write(d / "include" / "xrpl" / "telemetry" / "SpanNames.h", span_text)
+            headers = chk.find_spanname_headers(d)
+            syms = chk.build_global_symbols(headers)
+            allow = chk.derive_dotted_resource_keys(d, syms, chk.Report())
+            return allow, syms, headers, d
+        except Exception:
+            shutil.rmtree(d)
+            raise
+
+    def test_dotted_span_attr_not_allowlisted_and_flagged(self):
+        span = (
+            "#pragma once\n"
+            "namespace xrpl::telemetry {\n"
+            'namespace seg { inline constexpr auto xrpl = makeStr("xrpl");\n'
+            'inline constexpr auto ledger = makeStr("ledger");\n'
+            'inline constexpr auto network = makeStr("network"); }\n'
+            "namespace attr {\n"
+            "inline constexpr auto networkId = "
+            'join(join(seg::xrpl, seg::network), makeStr("id"));\n'
+            "inline constexpr auto ledgerHash = "
+            'join(join(seg::xrpl, seg::ledger), makeStr("hash"));\n'
+            "}\n}\n"
+        )
+        tele = (
+            "auto r = resource::Resource::Create({\n"
+            "  {semconv::service::kServiceName, x},\n"
+            "  {std::string(attr::networkId), n},\n"
+            "});\n"
+        )
+        allow, syms, headers, d = self._derive(tele, span)
+        try:
+            # networkId IS a resource attr; ledgerHash is NOT, despite living in
+            # the base header.
+            self.assertIn("xrpl.network.id", allow)
+            self.assertNotIn("xrpl.ledger.hash", allow)
+            kbh = {h: chk.attr_keys_from_header(h, syms) for h in headers}
+            report = chk.Report()
+            chk.run_rule_a(kbh, allow, report)
+            self.assertEqual([v[2] for v in report.violations], ["xrpl.ledger.hash"])
+        finally:
+            shutil.rmtree(d)
+
+    def test_resource_block_brace_matched(self):
+        # A nested {key,value} initializer must not truncate the block scan.
+        tele = (
+            "auto r = resource::Resource::Create({\n"
+            "  {semconv::service::kServiceName, x},\n"
+            "  {std::string(attr::networkType), t},\n"
+            "});\n"
+        )
+        span = (
+            "#pragma once\n"
+            "namespace xrpl::telemetry {\n"
+            'namespace seg { inline constexpr auto xrpl = makeStr("xrpl");\n'
+            'inline constexpr auto network = makeStr("network"); }\n'
+            "namespace attr { inline constexpr auto networkType = "
+            'join(join(seg::xrpl, seg::network), makeStr("type")); }\n}\n'
+        )
+        allow, _syms, _headers, d = self._derive(tele, span)
+        try:
+            self.assertIn("xrpl.network.type", allow)
+            self.assertIn("service.name", allow)
+        finally:
+            shutil.rmtree(d)
+
+
 def _run_rule_a(keys_by_header, allow):
     report = chk.Report()
     chk.run_rule_a(keys_by_header, allow, report)