From 282aec436789835aae98ee28efa17760e37535fb Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 11 Jun 2026 22:41:08 +0100 Subject: [PATCH 1/3] ci: Fix OTel naming check blind spot for dotted span attrs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rule A silently missed a dotted span attribute (xrpl.ledger.hash) because of two interacting bugs: 1. attr_keys_from_header resolved each constant via a flat global symbol table keyed by bare name, so a later header defining a same-named constant (e.g. consensus attr::ledgerHash = "ledger_hash") clobbered the base header's attr::ledgerHash = "xrpl.ledger.hash", erasing the real dotted key from L1. Now each constant is resolved against its own header (the global table only seeds seg::/join() cross-file references); using-re-exports still resolve globally. 2. derive_dotted_resource_keys allowlisted any dotted key declared in the base SpanNames.h. Now it allowlists 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 never set as a resource attr is a Rule-A violation. Adds 4 regression tests (collision, using-re-export, allowlist scope, brace matching). No allowlist exception is added — the check now catches the violation so the offending code can be fixed. --- .github/scripts/otel-naming/README.md | 15 +- .../scripts/otel-naming/check_otel_naming.py | 115 +++++++++---- .../otel-naming/test_check_otel_naming.py | 156 ++++++++++++++++++ 3 files changed, 247 insertions(+), 39 deletions(-) diff --git a/.github/scripts/otel-naming/README.md b/.github/scripts/otel-naming/README.md index b68e661628..f6a4c3a1ff 100644 --- a/.github/scripts/otel-naming/README.md +++ b/.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::` + 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) diff --git a/.github/scripts/otel-naming/check_otel_naming.py b/.github/scripts/otel-naming/check_otel_naming.py index 3b5ad1fe5b..59f679f5a4 100644 --- a/.github/scripts/otel-naming/check_otel_naming.py +++ b/.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::` + 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::::k -> 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::::k -> 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:: 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 diff --git a/.github/scripts/otel-naming/test_check_otel_naming.py b/.github/scripts/otel-naming/test_check_otel_naming.py index 638e72216e..f23ff5782d 100644 --- a/.github/scripts/otel-naming/test_check_otel_naming.py +++ b/.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) From c3ccde3e3978086c86b0e883b74c5c83e489eb59 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 11 Jun 2026 22:17:31 +0100 Subject: [PATCH 2/3] docs(telemetry): document the span-attribute naming rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit State the rules so they stay consistent across code, collector, Tempo, dashboards, and docs: - Per-span-unique field -> bare name (the span name carries the domain). - Same concept on more than one span -> ONE shared key, reused verbatim and distinguished by span name, never tagged with the emitting workflow (e.g. ledger_hash, full_validation, proposal_trusted/validation_trusted). Defined once in the base SpanNames.h and re-exported by each domain header. - Collision qualifier _ only to separate DIFFERENT concepts that share a word, or the OTel-reserved status key (rpc_status, consensus_state). - Dotted xrpl.<...> is reserved for resource attributes (xrpl.network.*). Updates CONTRIBUTING.md (permanent home) and OpenTelemetryPlan §2.3.3. --- CONTRIBUTING.md | 27 ++++++++++++++++------- OpenTelemetryPlan/02-design-decisions.md | 28 +++++++++++++++--------- 2 files changed, 37 insertions(+), 18 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index bc56d5a6bb..1edb689756 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -307,14 +307,25 @@ across the code, the OTel collector, Tempo, Grafana dashboards, and docs. The constants in the `*SpanNames.h` headers are the single source of truth; every other layer must match them. A CI check enforces this end to end. -1. Per-span unique attribute: bare field name — the span name already carries - the domain (e.g. `command`, `local`, `version` on `rpc.command` / `tx.process`). -2. Collision qualifier: `_` when a bare name would collide across - domains (in the shared spanmetrics label space) or with the OTel-reserved - `status` key (e.g. `rpc_status`, `grpc_status`, `proposal_trusted`, - `validation_trusted`). -3. Shared cross-span attribute: `_` underscore form - (e.g. `tx_hash`, `peer_id`, `ledger_seq`, `consensus_round`). +1. Per-span unique attribute: bare field name — allowed when the field is + recorded by a single span/workflow, so the span name already supplies the + domain (e.g. `command`, `local`, `version` on `rpc.command` / `tx.process`). +2. Shared attribute (same concept on more than one span): ONE key, reused + verbatim on every span that records it — the span name tells the occurrences + apart, so no per-emitter prefix is added. Pick the name by the field's + meaning: a property of a domain object keeps that object's bare field name + (`ledger_hash`, `ledger_seq`, `tx_hash`, `peer_id`, `full_validation`); a + field already qualified by a sub-kind keeps that qualifier on every emitter + (`proposal_trusted` on both `consensus.proposal.receive` and + `peer.proposal.receive`; `validation_trusted` likewise). Define it once in + the base `SpanNames.h` `namespace attr` block and re-export (`using`) it from + each domain header, so all emitters share the exact string. +3. Collision qualifier: `_` — only when a bare name would collide + with a DIFFERENT concept in the shared spanmetrics label space, or with the + OTel-reserved `status` key (e.g. `rpc_status`, `grpc_status`, + `consensus_state`, `consensus_round`). This disambiguates distinct concepts + that share a word; it is NOT used to tag the same concept with the workflow + that emitted it — that is rule 2 (one shared name). 4. Resource attribute: dotted `xrpl..` — reserved ONLY for process/network identity set once at startup (`xrpl.network.id`, `xrpl.network.type`). Never use the dotted `xrpl.` form for span attributes. diff --git a/OpenTelemetryPlan/02-design-decisions.md b/OpenTelemetryPlan/02-design-decisions.md index 732eda3661..64d1da123b 100644 --- a/OpenTelemetryPlan/02-design-decisions.md +++ b/OpenTelemetryPlan/02-design-decisions.md @@ -170,16 +170,24 @@ headers are the single source of truth; the collector, Tempo, the Grafana dashboards, and the runbook all consume these exact keys, so every layer must agree with the code. A CI check enforces this end to end. -1. **Per-span unique attribute** → bare field name. The span name already - carries the domain, so no prefix is needed (e.g. `command`, `version`, - `local` on `rpc.command`). -2. **Collision qualifier** → `_` when a bare name would collide - across domains in the shared spanmetrics label space, or with the - OTel-reserved `status` key (e.g. `rpc_status`, `grpc_status`, - `proposal_trusted`, `validation_trusted`). -3. **Shared cross-span attribute** → `_` underscore form, used - wherever the same field appears on more than one span (e.g. `tx_hash`, - `peer_id`, `ledger_seq`, `consensus_round`, `consensus_mode`). +1. **Per-span unique attribute** → bare field name, allowed when the field is + recorded by a single span/workflow so the span name already supplies the + domain (e.g. `command`, `version`, `local` on `rpc.command`). +2. **Shared attribute (same concept on more than one span)** → ONE key, reused + verbatim on every span that records it; the span name tells the occurrences + apart, so no per-emitter prefix is added. Name it by the field's meaning: a + property of a domain object keeps that object's bare field name (`ledger_hash`, + `ledger_seq`, `tx_hash`, `peer_id`, `full_validation`); a field already + qualified by a sub-kind keeps that qualifier on every emitter (`proposal_trusted` + on both `consensus.proposal.receive` and `peer.proposal.receive`; + `validation_trusted` likewise). Defined once in the base `SpanNames.h` + `namespace attr` block and re-exported (`using`) by each domain header. +3. **Collision qualifier** → `_`, only when a bare name would + collide with a DIFFERENT concept in the shared spanmetrics label space or with + the OTel-reserved `status` key (e.g. `rpc_status`, `grpc_status`, + `consensus_state`, `consensus_round`, `consensus_mode`). This disambiguates + distinct concepts that share a word; it is NOT used to tag the same concept + with its emitting workflow — that is rule 2 (one shared name). 4. **Resource attribute** → dotted `xrpl..`, reserved ONLY for process/network identity set once at startup (`xrpl.network.id`, `xrpl.network.type`). Span attributes are never dotted in the `xrpl.` form — From 43a70551b94485897045ad7144a9cb53be9be3a7 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 11 Jun 2026 23:15:28 +0100 Subject: [PATCH 3/3] docs(telemetry): fix stale txq.accept_tx span name in header diagram The span-hierarchy comment showed txq.accept.tx, but the constant emits txq.accept_tx (op::acceptTx = "accept_tx"). Correct the diagram. --- src/xrpld/app/misc/detail/TxQSpanNames.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/xrpld/app/misc/detail/TxQSpanNames.h b/src/xrpld/app/misc/detail/TxQSpanNames.h index 3f8f86aa30..24c2cfa458 100644 --- a/src/xrpld/app/misc/detail/TxQSpanNames.h +++ b/src/xrpld/app/misc/detail/TxQSpanNames.h @@ -34,7 +34,7 @@ * | attrs: queue_size, ledger_changed | * | | * | +--------------------------------------------------+ | - * | | txq.accept.tx (per queued transaction) | | + * | | txq.accept_tx (per queued transaction) | | * | | attrs: tx_hash, ter_code, retries_remaining | | * | +--------------------------------------------------+ | * +-------------------------------------------------------+