mirror of
https://github.com/XRPLF/rippled.git
synced 2026-07-30 10:30:22 +00:00
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.
This commit is contained in:
55
.github/scripts/otel-naming/check_otel_naming.py
vendored
55
.github/scripts/otel-naming/check_otel_naming.py
vendored
@@ -148,9 +148,22 @@ BLOCK_COMMENT = re.compile(r"/\*.*?\*/", re.DOTALL)
|
||||
# Dashboards reference span attributes in TraceQL as `span.<attr>`; 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
|
||||
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user