From e3635f1e310353a92009c90c315a251eb38e19da Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 28 Jul 2026 12:50:27 +0100 Subject: [PATCH] fix(ci): resolve metric label keys written as named constants Rule D rejected the `handler` and `result` dashboard filters even though both labels are emitted, because the L6 extractor missed them twice over: - It matched only a key written as an inline literal directly after `{{`. A label map is a braced list of braced pairs, so in `Add(1, {{"job_type", a}, {"handler", b}})` only the first key follows `{{` -- every later one was dropped. A key hoisted into a constant (`{{kHandlerLabel, v}}`) was invisible in any position. - It walked *.cpp only, so a constant declared in a header, as kLabelResult is, could never be found. Match the label map first and scan its pairs, resolve `k...Label` constants through their `constexpr char k...[] = "..."` definitions, and read headers too. Matching the map rather than any `{"key",` in the file keeps ordinary brace initializers, such as the `{"http", "https"}` scheme array, out of the label set -- they are not labels and must not license a dashboard filter. Test code is skipped for the same reason Rule F skips it: fixtures pass arbitrary literal pairs. --- .../scripts/otel-naming/check_otel_naming.py | 55 ++++++++++++---- .../otel-naming/test_check_otel_naming.py | 63 +++++++++++++++++++ 2 files changed, 107 insertions(+), 11 deletions(-) diff --git a/.github/scripts/otel-naming/check_otel_naming.py b/.github/scripts/otel-naming/check_otel_naming.py index 35d7d5634d..6bf6a6a266 100644 --- a/.github/scripts/otel-naming/check_otel_naming.py +++ b/.github/scripts/otel-naming/check_otel_naming.py @@ -148,9 +148,22 @@ BLOCK_COMMENT = re.compile(r"/\*.*?\*/", re.DOTALL) # Dashboards reference span attributes in TraceQL as `span.`; the bare # attribute is what must exist in L1, so strip the scope before validating. TRACEQL_SCOPE = re.compile(r"^(?:span|resource|event|link|instrumentation_scope)\.") -# An OTel metric label key as emitted in C++: `Add(.., {{"label", ...}})` / -# `{{"label", value}}` instrument calls in MetricsRegistry. -METRIC_LABEL = re.compile(r'\{\{\s*"([a-z_][a-z0-9_]*)"\s*,') +# An OTel metric label map as emitted in C++: the `{{...}}` argument of an +# instrument call, e.g. `counter->Add(1, {{"job_type", a}, {"handler", b}})`. +# Matching the whole map first, rather than scanning the file for `{"key",`, +# keeps ordinary brace initializers (`{"http", "https", ...}`) out of the +# label set — they are not labels and must not license a dashboard filter. +METRIC_LABEL_MAP = re.compile(r"\{\{(.*?)\}\}", re.DOTALL) +# One key inside such a map: a string literal, or a `kFooLabel` constant name +# resolved through LABEL_CONST_DEF. Inside the map, each pair opens with `{`, +# except the first, whose `{` was consumed by the map's own `{{`. +METRIC_LABEL = re.compile(r'(?:^|\{)\s*"([a-z_][a-z0-9_]*)"\s*,') +METRIC_LABEL_CONST = re.compile(r"(?:^|\{)\s*(k[A-Za-z0-9_]*)\s*,") +# A label-key constant definition, with or without `inline`: +# `constexpr char kHandlerLabel[] = "handler";` +LABEL_CONST_DEF = re.compile( + r'(?:inline\s+)?constexpr\s+char\s+(k[A-Za-z0-9_]*)\s*\[\s*\]\s*=\s*"([a-z_][a-z0-9_]*)"' +) def strip_comments(text: str) -> str: @@ -793,16 +806,36 @@ def run_rule_c_tempo(root: Path, l1_keys: Set[str], report: Report) -> None: def metric_label_names(root: Path) -> Set[str]: """L6: OTel native-metric label keys emitted by the telemetry code, e.g. `counter->Add(1, {{"job_type", value}})` in MetricsRegistry.cpp. These are - a valid source of dashboard labels distinct from span attributes (L1).""" + a valid source of dashboard labels distinct from span attributes (L1). + + A label key may be written inline as a string literal or hoisted into a + named constant (`constexpr char kHandlerLabel[] = "handler";`, then + `{{kHandlerLabel, value}}`). Both forms are collected, and the constants + may be declared in a header, so headers are scanned as well as sources.""" labels: Set[str] = set() + # Constant name -> label string, gathered across every scanned file so a + # `{{kFooLabel, ...}}` call site resolves even when the definition lives in + # a different file from the instrument call. + constants: Dict[str, str] = {} + used_constants: Set[str] = set() for base in ("src", "include"): - for p in (root / base).rglob("*.cpp"): - if not p.is_file(): - continue - text = read_source(p) - if "MetricsRegistry" not in p.name and "metric" not in text.lower(): - continue - labels |= set(METRIC_LABEL.findall(text)) + for pattern in ("*.cpp", "*.h"): + for p in (root / base).rglob(pattern): + if not p.is_file(): + continue + # Test code passes arbitrary literal pairs to exercise APIs + # (`signers("alice", 1, {{"alice", 1}, {"bob", 2}})`). Those are + # not metric labels, and must not license a dashboard filter. + if is_test_path(p): + continue + text = read_source(p) + if "MetricsRegistry" not in p.name and "metric" not in text.lower(): + continue + constants.update(dict(LABEL_CONST_DEF.findall(text))) + for label_map in METRIC_LABEL_MAP.findall(text): + labels |= set(METRIC_LABEL.findall(label_map)) + used_constants |= set(METRIC_LABEL_CONST.findall(label_map)) + labels |= {constants[name] for name in used_constants if name in constants} return labels diff --git a/.github/scripts/otel-naming/test_check_otel_naming.py b/.github/scripts/otel-naming/test_check_otel_naming.py index c24bc063b4..99beec8928 100644 --- a/.github/scripts/otel-naming/test_check_otel_naming.py +++ b/.github/scripts/otel-naming/test_check_otel_naming.py @@ -827,6 +827,69 @@ class MetricLabelExtraction(unittest.TestCase): finally: shutil.rmtree(d) + def test_extracts_second_label_of_a_pair(self): + """A multi-label map opens the first pair with `{{` and later ones with + a single `{`; every key must be collected, not just the first.""" + d = Path(tempfile.mkdtemp()) + try: + _write( + d / "src" / "xrpld" / "telemetry" / "MetricsRegistry.cpp", + 'h->Record(v, {{"job_type", std::string(t)}, {"handler", h2}});\n', + ) + self.assertEqual(chk.metric_label_names(d), {"job_type", "handler"}) + finally: + shutil.rmtree(d) + + def test_resolves_label_key_constants(self): + """A key hoisted into a `constexpr char k...[]` constant resolves to its + string, including when the constant is declared in a header.""" + d = Path(tempfile.mkdtemp()) + try: + _write( + d / "include" / "xrpl" / "telemetry" / "GetObjectMetricNames.h", + "// metric name constants\n" + 'inline constexpr char kLabelResult[] = "result";\n', + ) + _write( + d / "src" / "xrpld" / "telemetry" / "MetricsRegistry.cpp", + 'constexpr char kHandlerLabel[] = "handler";\n' + "counter->Add(1, {{kLabelResult, std::string(r)}});\n" + 'c2->Add(1, {{"job_type", std::string(t)}, {kHandlerLabel, h}});\n', + ) + self.assertEqual( + chk.metric_label_names(d), {"result", "handler", "job_type"} + ) + finally: + shutil.rmtree(d) + + def test_ignores_plain_brace_initializers(self): + """Ordinary brace lists are not label maps: a bare `{"http", "https"}` + array must not license a dashboard filter on `http`.""" + d = Path(tempfile.mkdtemp()) + try: + _write( + d / "src" / "xrpld" / "telemetry" / "MetricsRegistry.cpp", + 'std::array schemes{"http", "https", "ws"};\n' + 'for (auto const* k : {"read_request_bundle", "read_threads"})\n' + " use(k);\n" + 'counter->Add(1, {{"job_type", std::string(t)}});\n', + ) + self.assertEqual(chk.metric_label_names(d), {"job_type"}) + finally: + shutil.rmtree(d) + + def test_ignores_test_code_literals(self): + """Test fixtures pass arbitrary literal pairs; they define no labels.""" + d = Path(tempfile.mkdtemp()) + try: + _write( + d / "src" / "test" / "jtx" / "Env_test.cpp", + "// metric\n" 'env(signers("alice", 1, {{"alice", 1}, {"bob", 2}}));\n', + ) + self.assertEqual(chk.metric_label_names(d), set()) + finally: + shutil.rmtree(d) + class ReportExitContract(unittest.TestCase): @staticmethod