From f8e0a19b9fd985691e1372145b067151e33c10a3 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 7 Sep 2026 13:25:22 +0100 Subject: [PATCH] fix(telemetry): correct the childSpan doc examples and make Rule D tests real Review feedback on the RPC integration PR. The childSpan examples could not work as written. childSpan() takes its parent from the ambient context and uses impl_ only as a liveness gate, so an unscoped SpanGuard parent produced two siblings rather than a parent and child. The parent is now a ScopedSpanGuard, the child no longer reuses the parent's name, and the examples pass a full dotted constant because childSpan() takes the name verbatim. Five of the ten Rule D tests could not fail. Four passed an empty L1 key set, which makes the rule skip validation altogether; the fifth asserted an empty result against an escaped-quote selector that extracted no labels at all. Each now passes a nonempty L1 set and carries a known-bad label in the same expression, so it asserts both that the intended labels are accepted and that Rule D ran. Verified by disabling the rule: the old tests stay green, the new ones all fail. Span kind is not fixed here. categoryToSpanKind and the span factories belong to the telemetry library, so the role parameter is routed to that branch, and the two call sites here follow once it exists. --- .../otel-naming/test_check_otel_naming.py | 52 +++++++++++++++---- include/xrpl/telemetry/SpanGuard.h | 2 +- include/xrpl/telemetry/Telemetry.h | 12 +++-- 3 files changed, 52 insertions(+), 14 deletions(-) diff --git a/.github/scripts/otel-naming/test_check_otel_naming.py b/.github/scripts/otel-naming/test_check_otel_naming.py index c24bc063b4..4fd225ad6e 100644 --- a/.github/scripts/otel-naming/test_check_otel_naming.py +++ b/.github/scripts/otel-naming/test_check_otel_naming.py @@ -738,28 +738,54 @@ class RuleDDashboards(unittest.TestCase): ["bogus_label"], ) + # Rule D returns early on an empty L1 set, so a test that passes one asserts + # nothing. Each test below passes a nonempty L1 set and puts `bogus_label` in + # the same expression as the labels under test: the assertion then pins both + # halves at once — the accepted labels are absent from the result, and Rule D + # demonstrably ran because it flagged the bad one. + def test_builtin_labels_not_flagged(self): self.assertEqual( - self._run('"expr": "sum by (le, span_name, exported_instance) (x)"', set()), - [], + self._run( + '"expr": "sum by (le, span_name, exported_instance, bogus_label) (x)"', + {"command"}, + ), + ["bogus_label"], ) def test_external_infra_labels_not_flagged(self): # EXTERNAL_INFRA_LABELS (perf-iac identity labels with no in-tree - # source) must be recognized as valid, distinct from `builtins`. - expr = "sum by (" + ", ".join(sorted(chk.EXTERNAL_INFRA_LABELS)) + ") (x)" - self.assertEqual(self._run(f'"expr": "{expr}"', set()), []) + # source) must be recognized as valid, distinct from `builtins`. The + # names are spelled out rather than joined from chk.EXTERNAL_INFRA_LABELS + # because building the query from the set that validates it passes for + # whatever that set happens to hold — including an empty one. + self.assertEqual( + self._run( + '"expr": "sum by (xrpl_branch, xrpl_node_role, bogus_label) (x)"', + {"command"}, + ), + ["bogus_label"], + ) def test_prometheus_name_label_not_flagged(self): # `__name__` is the Prometheus reserved metric-name label; the renamed # system-*.json dashboards use `sum by (le, __name__)`. self.assertEqual( - self._run('"expr": "sum by (le, __name__) (rate(x[5m]))"', set()), - [], + self._run( + '"expr": "sum by (le, __name__, bogus_label) (rate(x[5m]))"', + {"command"}, + ), + ["bogus_label"], ) def test_l1_label_passes(self): - self.assertEqual(self._run('"q": "{command=\\"x\\"}"', {"command"}), []) + # `by (...)` form, not a `{command="x"}` selector: a dashboard stores the + # query inside a JSON string, so its quotes are escaped on disk and the + # selector branch extracts nothing from them here. + self.assertEqual( + self._run('"expr": "sum by (command, bogus_label) (x)"', {"command"}), + ["bogus_label"], + ) def test_traceql_span_prefix_stripped(self): # `span.establish_count` must validate against the bare L1 key. @@ -772,7 +798,15 @@ class RuleDDashboards(unittest.TestCase): ) def test_traceql_resource_prefix_stripped(self): - self.assertEqual(self._run('"q": "{resource.service_name=\\"x\\"}"', set()), []) + # `resource.service_name` must validate against the bare builtin, same as + # the `span.` case above. + self.assertEqual( + self._run( + '"expr": "count_over_time(x) by (resource.service_name, bogus_label)"', + {"command"}, + ), + ["bogus_label"], + ) def test_native_metric_label_passes(self): # `job_type` / `reason` are emitted by MetricsRegistry, not span attrs. diff --git a/include/xrpl/telemetry/SpanGuard.h b/include/xrpl/telemetry/SpanGuard.h index 9f6d4c0ae9..39b589e655 100644 --- a/include/xrpl/telemetry/SpanGuard.h +++ b/include/xrpl/telemetry/SpanGuard.h @@ -99,7 +99,7 @@ * auto ctx = span.spanContext(); * * // Thread B: create child with captured context - * auto child = SpanGuard::childSpan(rpc_span::op::process, ctx); + * auto child = SpanGuard::childSpan(rpc_span::prefix::command, ctx); * @endcode * * 4. Conditional check (rarely needed — methods are no-ops on null): diff --git a/include/xrpl/telemetry/Telemetry.h b/include/xrpl/telemetry/Telemetry.h index 68efe4965c..ab694abdae 100644 --- a/include/xrpl/telemetry/Telemetry.h +++ b/include/xrpl/telemetry/Telemetry.h @@ -53,14 +53,18 @@ * * 2. Child span for a sub-operation (scoped child): * @code - * auto parent = SpanGuard::span( + * auto parent = ScopedSpanGuard( * TraceCategory::Rpc, rpc_span::prefix::rpc, rpc_span::op::process); * { - * auto child = parent.childSpan(rpc_span::op::process); - * child.setAttribute(rpc_span::attr::version, apiVersion); + * auto child = parent.childSpan(rpc_span::prefix::command); + * child.setAttribute(rpc_span::attr::version, static_cast(apiVersion)); * // child ends here * } * @endcode + * childSpan() parents to the ambient scope, so the parent must be a + * ScopedSpanGuard. A plain SpanGuard is not ambient: pass its spanContext() + * to childSpan(name, ctx) instead. childSpan() takes the name verbatim, so + * pass a full dotted constant, never a bare op:: suffix. * * 3. Unrelated span (cross-scope, same thread): * @code @@ -78,7 +82,7 @@ * auto ctx = parentGuard.spanContext(); * * // Thread B: create child span with explicit parent - * auto child = SpanGuard::childSpan(rpc_span::op::process, ctx); + * auto child = SpanGuard::childSpan(rpc_span::prefix::command, ctx); * @endcode * * @note Thread safety: The Telemetry interface is safe for concurrent reads