mirror of
https://github.com/XRPLF/rippled.git
synced 2026-07-23 23:20:33 +00:00
ci(telemetry): teach otel-naming checker the SpanGuard::rootSpan factory
rootSpan has the same (cat, prefix, name) signature as span(), so its
prefix/name arguments must be *SpanNames.h constants under Rule F/H. Add
rootSpan to the CALLSITE regex and CONSTANT_ARG_POSITIONS ({1,2}), update
the enumerating docstrings/README, and add two regression tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Pratik Mankawde
18
.github/scripts/otel-naming/README.md
vendored
18
.github/scripts/otel-naming/README.md
vendored
@@ -38,15 +38,15 @@ hardcoded allowlist:
|
||||
|
||||
### Rules (each fails the build, when its inputs are present)
|
||||
|
||||
| Rule | Check |
|
||||
| ---- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| A | No stray dotted span-attribute key (only the derived resource keys may be dotted). |
|
||||
| G | Attribute keys are `lower_snake_case` (`^[a-z][a-z0-9_]*$` per dot-segment) — no camelCase, UPPERCASE, or spaces. |
|
||||
| F | No string literals as attribute keys or span-name arguments in `setAttribute`/`addEvent`/`span`/`childSpan`. Attribute _values_ are exempt (runtime data); `*SpanNames.h` definitions and test files are exempt. |
|
||||
| B | Every collector `spanmetrics.dimensions` name exists in the L1 key set. |
|
||||
| C | Every Tempo span-filter tag exists in the L1 key set. |
|
||||
| D | Every dashboard label resolves to an L1 span attribute, a native-metric label (L6, emitted by MetricsRegistry), or a Prometheus/Grafana builtin. TraceQL scope prefixes (`span.`/`resource.`/…) are stripped before the L1 lookup. |
|
||||
| E | No dotted `xrpl.<domain>.<field>` attribute key in the runbook (only the L1 resource attrs `xrpl.network.*` may be dotted). Span names, filenames, OTel-standard keys, and metric labels are not flagged. |
|
||||
| Rule | Check |
|
||||
| ---- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| A | No stray dotted span-attribute key (only the derived resource keys may be dotted). |
|
||||
| G | Attribute keys are `lower_snake_case` (`^[a-z][a-z0-9_]*$` per dot-segment) — no camelCase, UPPERCASE, or spaces. |
|
||||
| F | No string literals as attribute keys or span-name arguments in `setAttribute`/`addEvent`/`span`/`rootSpan`/`childSpan` (`rootSpan` shares `span`'s `(cat, prefix, name)` signature). Attribute _values_ are exempt (runtime data); `*SpanNames.h` definitions and test files are exempt. |
|
||||
| B | Every collector `spanmetrics.dimensions` name exists in the L1 key set. |
|
||||
| C | Every Tempo span-filter tag exists in the L1 key set. |
|
||||
| D | Every dashboard label resolves to an L1 span attribute, a native-metric label (L6, emitted by MetricsRegistry), or a Prometheus/Grafana builtin. TraceQL scope prefixes (`span.`/`resource.`/…) are stripped before the L1 lookup. |
|
||||
| E | No dotted `xrpl.<domain>.<field>` attribute key in the runbook (only the L1 resource attrs `xrpl.network.*` may be dotted). Span names, filenames, OTel-standard keys, and metric labels are not flagged. |
|
||||
|
||||
Rule F runs **unconditionally** (it is a purely syntactic check on the
|
||||
call-sites and needs no `*SpanNames.h`), so a code path that calls
|
||||
|
||||
27
.github/scripts/otel-naming/check_otel_naming.py
vendored
27
.github/scripts/otel-naming/check_otel_naming.py
vendored
@@ -31,7 +31,8 @@ Layers
|
||||
------
|
||||
L1 code : src/**/*SpanNames.h, include/**/*SpanNames.h (ground truth)
|
||||
L1 resource : src/libxrpl/telemetry/Telemetry.cpp (dotted allowlist)
|
||||
L1 callsites : setAttribute/addEvent/span/childSpan in src/**, include/**
|
||||
L1 callsites : setAttribute/addEvent/span/rootSpan/childSpan in src/**,
|
||||
include/**
|
||||
L2 collector : docker/telemetry/otel-collector-config.yaml (spanmetrics dims)
|
||||
L3 tempo : docker/telemetry/tempo.yaml (span filter tags)
|
||||
L4 dashboards: docker/telemetry/grafana/dashboards/*.json (PromQL labels)
|
||||
@@ -46,9 +47,9 @@ Rules (each FAILS the build, when its inputs are present)
|
||||
G Attribute keys must be lower_snake_case (^[a-z][a-z0-9_]*$ per segment).
|
||||
Flags camelCase, UPPERCASE, spaces, and other stray characters.
|
||||
F No string literals as attribute keys or span-name arguments. The
|
||||
setAttribute/addEvent key and the span/childSpan prefix/name args must
|
||||
reference a *SpanNames.h constant, never a "literal". Attribute VALUES are
|
||||
exempt (runtime data). Definitions inside *SpanNames.h are exempt, and
|
||||
setAttribute/addEvent key and the span/rootSpan/childSpan prefix/name args
|
||||
must reference a *SpanNames.h constant, never a "literal". Attribute VALUES
|
||||
are exempt (runtime data). Definitions inside *SpanNames.h are exempt, and
|
||||
test files are exempt (they pass arbitrary literals to exercise the API).
|
||||
B Every collector spanmetrics dimension exists in the L1 key set.
|
||||
C Every tempo span-filter tag exists in the L1 key set.
|
||||
@@ -123,12 +124,14 @@ USING_DECL = re.compile(r"using\s+(?:::)?[\w:]*::(\w+)\s*;")
|
||||
# Telemetry call-sites whose string arguments must be constants, not literals.
|
||||
# Require a receiver so we match real SpanGuard calls, not std::span / a math
|
||||
# `span(...)` / a bare method declaration:
|
||||
# - `SpanGuard::span(` / `SpanGuard::childSpan(` (static factory)
|
||||
# - `SpanGuard::span(` / `SpanGuard::rootSpan(` / `SpanGuard::childSpan(`
|
||||
# (static factories)
|
||||
# - `<obj>.span(` / `<obj>->setAttribute(` etc. (member call)
|
||||
# `span`/`childSpan` additionally require the `SpanGuard`/`.`/`->` receiver;
|
||||
# `setAttribute`/`addEvent` only ever exist on a guard, so a `.`/`->` suffices.
|
||||
# `span`/`rootSpan`/`childSpan` additionally require the `SpanGuard`/`.`/`->`
|
||||
# receiver; `setAttribute`/`addEvent` only ever exist on a guard, so a `.`/`->`
|
||||
# suffices. `rootSpan` shares `span`'s (cat, prefix, name) signature.
|
||||
CALLSITE = re.compile(
|
||||
r"(?:SpanGuard::|\.|->)\s*(setAttribute|addEvent|span|childSpan)\s*\("
|
||||
r"(?:SpanGuard::|\.|->)\s*(setAttribute|addEvent|span|rootSpan|childSpan)\s*\("
|
||||
)
|
||||
# A C++ string literal (used to flag literals inside call-site argument lists).
|
||||
STRING_LITERAL = re.compile(r'"((?:[^"\\]|\\.)*)"')
|
||||
@@ -562,11 +565,13 @@ def run_rule_g(keys_by_header: Dict[Path, Set[str]], report: Report) -> None:
|
||||
# setAttribute(key, value) -> check arg 0 (key); value (arg 1) exempt
|
||||
# addEvent(name[, attrs]) -> check arg 0 (event name)
|
||||
# span(category, prefix, name) -> check args 1,2 (prefix + span-name leaf)
|
||||
# rootSpan(category, prefix, name)-> check args 1,2 (same signature as span)
|
||||
# childSpan(name[, parentCtx]) -> check arg 0 (span-name leaf)
|
||||
CONSTANT_ARG_POSITIONS: Dict[str, Set[int]] = {
|
||||
"setAttribute": {0},
|
||||
"addEvent": {0},
|
||||
"span": {1, 2},
|
||||
"rootSpan": {1, 2}, # same signature as span(cat, prefix, name)
|
||||
"childSpan": {0},
|
||||
}
|
||||
|
||||
@@ -597,7 +602,8 @@ def spanname_symbol_names(headers: List[Path]) -> Set[str]:
|
||||
|
||||
def run_rule_f(root: Path, report: Report, header_symbols: Set[str]) -> None:
|
||||
"""Walk every telemetry call-site (non-test, non-*SpanNames.h) and check the
|
||||
constant-only argument positions of setAttribute/addEvent/span/childSpan:
|
||||
constant-only argument positions of
|
||||
setAttribute/addEvent/span/rootSpan/childSpan:
|
||||
|
||||
Rule F (FAIL): a string literal in a key / span-name position. Attribute
|
||||
VALUES are exempt (runtime data).
|
||||
@@ -664,7 +670,8 @@ def run_rule_f(root: Path, report: Report, header_symbols: Set[str]) -> None:
|
||||
|
||||
def iter_calls(text: str):
|
||||
"""Yield (call_name, raw_arglist, lineno) for each setAttribute/addEvent/
|
||||
span/childSpan invocation, spanning multiple physical lines if needed."""
|
||||
span/rootSpan/childSpan invocation, spanning multiple physical lines if
|
||||
needed."""
|
||||
for m in CALLSITE.finditer(text):
|
||||
name = m.group(1)
|
||||
# Walk from the opening paren, balancing nesting to find the close.
|
||||
|
||||
@@ -559,6 +559,29 @@ class RuleFAndH(unittest.TestCase):
|
||||
v, _ = self._run("src/Foo.cpp", 'SpanGuard::span(cat, "rpc", "command");\n')
|
||||
self.assertEqual(v, ['span arg1 "rpc"', 'span arg2 "command"'])
|
||||
|
||||
def test_rootspan_literal_flagged_by_rule_f(self):
|
||||
# rootSpan(cat, prefix, name) shares span()'s signature, so a string
|
||||
# literal in the prefix/name position must FAIL rule F exactly as it
|
||||
# does for span() — otherwise a call switched to rootSpan silently
|
||||
# escapes span-name validation.
|
||||
v, _ = self._run(
|
||||
"src/Foo.cpp",
|
||||
'SpanGuard::rootSpan(cat, "peer", "validation.receive");\n',
|
||||
)
|
||||
self.assertEqual(
|
||||
v, ['rootSpan arg1 "peer"', 'rootSpan arg2 "validation.receive"']
|
||||
)
|
||||
|
||||
def test_rootspan_constant_args_accepted(self):
|
||||
# Constant references in the prefix/name position are accepted (no
|
||||
# rule F), mirroring span()'s constant-arg handling.
|
||||
v, _ = self._run(
|
||||
"src/Foo.cpp",
|
||||
"SpanGuard::rootSpan(TraceCategory::Peer, seg::peer, "
|
||||
"peer_span::op::validationReceive);\n",
|
||||
)
|
||||
self.assertEqual(v, [])
|
||||
|
||||
def test_test_path_exempt(self):
|
||||
v, _ = self._run("src/test/Foo.cpp", 'g.setAttribute("lit_key", v);\n')
|
||||
self.assertEqual(v, [])
|
||||
|
||||
Reference in New Issue
Block a user