mirror of
https://github.com/XRPLF/rippled.git
synced 2026-08-21 14:20:56 +00:00
refactor(telemetry): name metrics with constants, and make CI require it (WP-A8)
Metric names and label keys were bare string literals, repeated across the emit site, the gauge registration, the unit test, the workload manifest, the dashboard queries and the reference table. A rename touched six places and a typo in any one of them failed silently: a metric that never appears, or a label that never joins. The span side already had this right, with names and attribute keys declared once in the *SpanNames.h headers and a CI rule rejecting literals at call sites. That rule only ever covered spans, so the metric side had no equivalent and no suffix convention was enforced by anything. - Adds MetricNames.h declaring every instrument name, label key and bounded label value this story emits, grouped by subsystem, following the existing span-name header layout. - Converts the call sites subsystem by subsystem. The emitted strings are unchanged: 75 names before, the same 75 after, verified by extracting the wire strings from both trees and diffing the sets. - Extends the naming check with three rules: no literal instrument name or label key at an emit site, the duration and counter suffix conventions, and every name in the workload manifest resolving to a constant. The first rule is ratcheted per metric family so the pre-existing families warn rather than block, keeping the remaining work visible instead of forcing one unreviewable change. Constants are character arrays rather than the span headers' StaticStr, because the metrics API takes a string view that will not construct from it. Two things the conversion exposed: a serve-refusal reason that the original inventory missed because it is passed through a ternary, and a label whose constant made it invisible to the checker's literal scan, which would have failed a dashboard rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Pratik Mankawde
30
.github/scripts/otel-naming/README.md
vendored
30
.github/scripts/otel-naming/README.md
vendored
@@ -35,18 +35,29 @@ hardcoded allowlist:
|
||||
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`.
|
||||
- **L1-metrics** — instrument names, label keys and bounded label values come
|
||||
from the `namespace metric` / `namespace label` / `namespace lval` blocks of
|
||||
every `*MetricNames.h`, read as `inline constexpr char NAME[] = "wire";`.
|
||||
These headers deliberately do **not** use the `makeStr`/`StaticStr` DSL the
|
||||
span headers use: the OTel C++ API takes `nostd::string_view`, which
|
||||
constructs from `char const*` but has no constructor from
|
||||
`std::string_view`, so a `StaticStr` will not compile in an instrument-name
|
||||
or label-key position.
|
||||
|
||||
### 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`/`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 | 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. |
|
||||
| I | No string literals as **metric** instrument names or label keys — the mirror of Rule F. Applies to the name passed to an `XRPL_METRIC_*` macro or a `meter->Create*` factory and to the label _keys_ in its label set. Label _values_, descriptions, `*MetricNames.h`, `MetricMacros.h` and test files are exempt. Scoped by metric **family** (first underscore segment): declaring a constant opts that family in, so the metric surface can be converted subsystem by subsystem. Unconverted families warn as Rule L. |
|
||||
| J | Metric instrument names follow the suffix conventions: `lower_snake_case`, no `xrpld_`/`xrpl_` prefix (the exporter adds it), a counter ends `_total`, a histogram ends `_us`/`_ms`/`_seconds`, a gauge does not end `_total`. The instrument **kind** is read from the emit site, never guessed from words in the name — so a multi-series gauge carrying units in its label values (e.g. `nodestore_latency` observing `write_mean_us`) is not a violation. |
|
||||
| K | Every metric named in `docker/telemetry/workload/expected_metrics.json` resolves to a declared constant, so a rename in code cannot leave the workload validator asserting a name nothing emits. PromQL selectors (`m{label="v"}`) and exporter-appended histogram suffixes (`_bucket`/`_count`/`_sum`) are normalized away first; groups fed by another emit path (`statsd_gauges`, `statsd_counters`, `spanmetrics`) are out of scope by design. |
|
||||
|
||||
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
|
||||
@@ -58,6 +69,7 @@ still caught.
|
||||
| Rule | Check |
|
||||
| ---- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
|
||||
| H | A namespace-qualified constant (e.g. `foo::bar::myKey`) used at a telemetry call-site is not defined in any `*SpanNames.h`. The constant should live in the proper header; defining it in-place bypasses rules A/G/F. Warns rather than fails — the argument may be a legitimately dynamic value, and the header may live on a later branch. Bare locals and `std::` names are not warned. |
|
||||
| L | A literal metric name in a family that has no `*MetricNames.h` constants yet. Rule I's ratchet defers these instead of failing the build on the whole pre-existing metric surface at once; the warning keeps the outstanding conversion work visible rather than silently accepted. |
|
||||
|
||||
## Presence-gated
|
||||
|
||||
|
||||
562
.github/scripts/otel-naming/check_otel_naming.py
vendored
562
.github/scripts/otel-naming/check_otel_naming.py
vendored
@@ -5,10 +5,12 @@ Usage: check_otel_naming.py
|
||||
This script takes no parameters and can be called from any directory inside the
|
||||
repository (it locates the repo root via `git rev-parse`).
|
||||
|
||||
Enforces the OpenTelemetry span-attribute naming convention documented in
|
||||
CONTRIBUTING.md ("Telemetry span attribute naming") across every layer of the
|
||||
telemetry pipeline. The `*SpanNames.h` constants are the single source of truth
|
||||
(L1); every other layer must agree with them.
|
||||
Enforces the OpenTelemetry naming conventions documented in CONTRIBUTING.md
|
||||
("Telemetry span attribute naming" and "Telemetry metric naming") across every
|
||||
layer of the telemetry pipeline. The `*SpanNames.h` constants are the single
|
||||
source of truth for span attributes (L1) and the `*MetricNames.h` constants for
|
||||
metric names and label keys (L1-metrics); every other layer must agree with
|
||||
them.
|
||||
|
||||
Design principles
|
||||
-----------------
|
||||
@@ -49,6 +51,10 @@ Layers
|
||||
L5 runbook : docs/telemetry-runbook.md (attr tables)
|
||||
L6 metrics : MetricsRegistry.cpp instrument labels (native-metric
|
||||
label keys, a valid dashboard-label source besides L1)
|
||||
L1-metrics : src/**/*MetricNames.h, include/**/*MetricNames.h (ground truth
|
||||
for metric instrument names, label keys and bounded values)
|
||||
L7 workload : docker/telemetry/workload/expected_metrics.json (asserted
|
||||
metric names)
|
||||
|
||||
Rules (each FAILS the build, when its inputs are present)
|
||||
---------------------------------------------------------
|
||||
@@ -71,6 +77,24 @@ Rules (each FAILS the build, when its inputs are present)
|
||||
form -- xrpl.work.item/.branch/.node.role -- may be dotted). Span names,
|
||||
filenames,
|
||||
OTel-standard keys, and metric labels are not flagged.
|
||||
I No string literals as METRIC instrument names or label keys. The mirror of
|
||||
Rule F for metrics: the name passed to an XRPL_METRIC_* macro or to a
|
||||
meter->Create* factory, and the label KEYS in its label set, must reference
|
||||
a *MetricNames.h constant. Label VALUES are exempt (runtime data), as are
|
||||
the descriptions, *MetricNames.h itself, MetricMacros.h, and test files.
|
||||
Scoped by metric FAMILY (first underscore segment) so the metric surface
|
||||
can be converted subsystem by subsystem -- declaring a constant for a
|
||||
family opts that family into enforcement. See Rule L.
|
||||
J Metric instrument names follow the naming and suffix conventions:
|
||||
lower_snake_case, no xrpld_/xrpl_ prefix (the exporter adds it), a counter
|
||||
ends in _total, a histogram ends in _us/_ms/_seconds, and a gauge does not
|
||||
end in _total. The instrument KIND is read from the emit site, never
|
||||
guessed from words in the name.
|
||||
K Every metric named in expected_metrics.json resolves to a *MetricNames.h
|
||||
constant, so a rename in code cannot leave the workload validator
|
||||
asserting a name nothing emits. PromQL selectors and exporter-appended
|
||||
histogram suffixes are normalized away first; groups fed by another emit
|
||||
path (statsd_*, spanmetrics) are out of scope.
|
||||
|
||||
Warnings (printed, but do NOT fail the build)
|
||||
----------------------------------------------
|
||||
@@ -79,11 +103,16 @@ Warnings (printed, but do NOT fail the build)
|
||||
*SpanNames.h (single source of truth); defining one in-place bypasses the
|
||||
naming rules. A warning (not a failure) because the argument may instead
|
||||
be a legitimately dynamic local (e.g. a computed span-name leaf).
|
||||
L A literal metric name in a family that has no *MetricNames.h constants
|
||||
yet. Rule I's ratchet defers these rather than failing the build on the
|
||||
whole pre-existing metric surface at once; the warning is what keeps the
|
||||
outstanding conversion work visible instead of silently accepted.
|
||||
|
||||
Exit code is non-zero if any present-and-enforced rule finds a violation.
|
||||
Warnings never change the exit code.
|
||||
"""
|
||||
|
||||
import json
|
||||
import re
|
||||
import subprocess
|
||||
import sys
|
||||
@@ -474,6 +503,24 @@ def main() -> None:
|
||||
run_rule_d_dashboards(root, l1_keys, metric_labels, report)
|
||||
run_rule_e_runbook(root, l1_keys, report)
|
||||
|
||||
# --- Metric rules I/J/K --------------------------------------------------
|
||||
# The metric-name counterpart of the span rules above. Rule I is the mirror
|
||||
# of Rule F (no literals at an emit site) and, like it, runs
|
||||
# unconditionally because it is purely syntactic. Rules J and K are
|
||||
# presence-gated on *MetricNames.h existing.
|
||||
l1_metric_names, l1_metric_labels, _ = metric_constants(root)
|
||||
if find_metricname_headers(root):
|
||||
report.ok(
|
||||
f"L1-metrics: {len(l1_metric_names)} instrument name(s) and "
|
||||
f"{len(l1_metric_labels)} label key(s) from "
|
||||
f"{len(find_metricname_headers(root))} *MetricNames.h header(s)"
|
||||
)
|
||||
else:
|
||||
report.skip("L1-metrics", "no *MetricNames.h present")
|
||||
run_rule_i_metric_literals(root, report)
|
||||
run_rule_j_metric_suffixes(root, report)
|
||||
run_rule_k_expected_metrics(root, report)
|
||||
|
||||
report.render_and_exit()
|
||||
|
||||
|
||||
@@ -812,7 +859,13 @@ def metric_label_names(root: Path) -> Set[str]:
|
||||
|
||||
Collects both the first label of a set and every subsequent one, so a
|
||||
multi-label instrument such as `{{"from", a}, {"to", b}}` contributes ALL
|
||||
of its keys."""
|
||||
of its keys.
|
||||
|
||||
Includes the keys declared as constants in `*MetricNames.h` (L1-metrics),
|
||||
because Rule I requires call sites to reference a constant rather than a
|
||||
literal — so once a subsystem is converted, its label keys no longer appear
|
||||
as literals anywhere and a literal-only scan would under-derive the set and
|
||||
make Rule D reject a dashboard querying a label the code really emits."""
|
||||
labels: Set[str] = set()
|
||||
for base in ("src", "include"):
|
||||
for p in (root / base).rglob("*.cpp"):
|
||||
@@ -823,9 +876,84 @@ def metric_label_names(root: Path) -> Set[str]:
|
||||
continue
|
||||
labels |= set(METRIC_LABEL.findall(text))
|
||||
labels |= set(METRIC_LABEL_NEXT.findall(text))
|
||||
labels |= metric_constants(root)[1]
|
||||
return labels
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# L1-metrics: parse *MetricNames.h into the authoritative metric-name/label set
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
# A metric-name constant definition: `inline constexpr char NAME[] = "wire";`.
|
||||
# The metric headers use `constexpr char[]` rather than the `makeStr`/StaticStr
|
||||
# DSL the span headers use, because the OTel C++ API takes nostd::string_view,
|
||||
# which constructs from `char const*` but NOT from std::string_view.
|
||||
METRIC_CONST_DEF = re.compile(
|
||||
r"inline\s+constexpr\s+char\s+(\w+)\s*\[\s*\]\s*=\s*\"([^\"]*)\"\s*;"
|
||||
)
|
||||
|
||||
|
||||
def find_metricname_headers(root: Path) -> List[Path]:
|
||||
"""Every `*MetricNames.h` in the tree (the metric-side counterpart of
|
||||
`find_spanname_headers`)."""
|
||||
return sorted(
|
||||
p
|
||||
for p in list((root / "src").rglob("*MetricNames.h"))
|
||||
+ list((root / "include").rglob("*MetricNames.h"))
|
||||
if p.is_file()
|
||||
)
|
||||
|
||||
|
||||
def metric_constants(root: Path) -> Tuple[Set[str], Set[str], Set[str]]:
|
||||
"""Return (instrument_names, label_keys, label_values) declared across the
|
||||
`*MetricNames.h` headers, split by which namespace block each constant sits
|
||||
in: `namespace metric` -> instrument names, `namespace label` -> label keys,
|
||||
`namespace lval` -> bounded label values.
|
||||
|
||||
Comments are stripped first, so an illustrative `@code` example in a doc
|
||||
comment cannot seed the authoritative set (same reasoning as
|
||||
`strip_comments` for L1 spans).
|
||||
|
||||
A constant in none of the three namespaces is ignored rather than guessed
|
||||
at, keeping the derivation conservative in the same direction as L1."""
|
||||
names: Set[str] = set()
|
||||
keys: Set[str] = set()
|
||||
values: Set[str] = set()
|
||||
for h in find_metricname_headers(root):
|
||||
text = strip_comments(read_source(h))
|
||||
for ns, bucket in (
|
||||
("metric", names),
|
||||
("label", keys),
|
||||
("lval", values),
|
||||
):
|
||||
for block in namespace_spans(text, ns):
|
||||
for m in METRIC_CONST_DEF.finditer(block):
|
||||
bucket.add(m.group(2))
|
||||
return names, keys, values
|
||||
|
||||
|
||||
def namespace_spans(text: str, want: str) -> List[str]:
|
||||
"""Return the body text of each `namespace <want> { ... }` block, brace
|
||||
matched so nested namespaces (e.g. `lval::dns_resolve`) are contained in
|
||||
their parent's span. Generalizes `attr_namespace_spans`, which is hardcoded
|
||||
to `attr`."""
|
||||
spans: List[str] = []
|
||||
for opener in NS_OPEN.finditer(text):
|
||||
if opener.group(1).split("::")[-1] != want:
|
||||
continue
|
||||
i = opener.end()
|
||||
depth, start = 1, i
|
||||
while i < len(text) and depth > 0:
|
||||
c = text[i]
|
||||
if c == "{":
|
||||
depth += 1
|
||||
elif c == "}":
|
||||
depth -= 1
|
||||
i += 1
|
||||
spans.append(text[start : i - 1])
|
||||
return spans
|
||||
|
||||
|
||||
# Identity labels stamped by EXTERNAL infrastructure the OTel pipeline in this
|
||||
# repo does not own: the perf-iac harness attaches these to every metric it
|
||||
# scrapes so dashboards can filter by which build/role produced a series. They
|
||||
@@ -942,5 +1070,429 @@ def run_rule_e_runbook(root: Path, l1_keys: Set[str], report: Report) -> None:
|
||||
report.ok("E: runbook attribute references consistent with L1")
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Metric rules I / J / K
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
# A metric emit site whose name argument must be a constant. Two families:
|
||||
# * the XRPL_METRIC_* call-site macros, where arg0 is the app/registry and
|
||||
# arg1 is the instrument name;
|
||||
# * the OTel meter factories, where arg0 is the instrument name.
|
||||
# Matching the macro by name (rather than by receiver, as CALLSITE does) is
|
||||
# sufficient because the XRPL_METRIC_ prefix is unambiguous in this repo.
|
||||
METRIC_MACRO_CALL = re.compile(r"\bXRPL_METRIC_([A-Z_]+)\s*\(")
|
||||
METRIC_FACTORY_CALL = re.compile(
|
||||
r"\bCreate(?:UInt64|Int64|Double)"
|
||||
r"(?:Counter|UpDownCounter|Histogram|Gauge|ObservableGauge|"
|
||||
r"ObservableCounter|ObservableUpDownCounter)\s*\("
|
||||
)
|
||||
# A `{ KEY , ...` label pair opener inside an already-isolated argument list.
|
||||
# KEY is what Rule I checks; the VALUE after the comma is exempt (runtime data).
|
||||
METRIC_PAIR_KEY = re.compile(r"\{\s*(\"(?:[^\"\\]|\\.)*\"|[A-Za-z_][\w:]*)\s*,")
|
||||
|
||||
# The unit/kind suffix convention every instrument name must satisfy (Rule J).
|
||||
# A cumulative counter reads correctly under rate() only if a reader can tell it
|
||||
# from a gauge, and a duration is ambiguous unless it carries its unit -- the
|
||||
# OTel `unit` argument is not surfaced on the Prometheus metric name.
|
||||
METRIC_COUNTER_SUFFIX = "_total"
|
||||
METRIC_DURATION_SUFFIXES = ("_us", "_ms", "_seconds")
|
||||
|
||||
|
||||
def metric_calls(text: str):
|
||||
"""Yield (kind, name_index, arglist, lineno) for every metric emit site.
|
||||
|
||||
`kind` is the macro suffix (e.g. `COUNTER_INC_LABELED`) for a macro call, or
|
||||
the factory method name for a `meter->Create*` call. `name_index` is which
|
||||
top-level argument holds the instrument name."""
|
||||
for m in METRIC_MACRO_CALL.finditer(text):
|
||||
arglist = balanced_arglist(text, m.end())
|
||||
yield m.group(1), 1, arglist, text.count("\n", 0, m.start()) + 1
|
||||
for m in METRIC_FACTORY_CALL.finditer(text):
|
||||
arglist = balanced_arglist(text, m.end())
|
||||
name = m.group(0).rstrip("( \t\n")
|
||||
yield name, 0, arglist, text.count("\n", 0, m.start()) + 1
|
||||
|
||||
|
||||
def balanced_arglist(text: str, start: int) -> str:
|
||||
"""Return the argument-list text of a call whose '(' ends at `start`-1,
|
||||
balancing nesting and ignoring parens inside string literals."""
|
||||
i, depth, in_str, esc = start, 1, False, False
|
||||
while i < len(text) and depth > 0:
|
||||
c = text[i]
|
||||
if in_str:
|
||||
if esc:
|
||||
esc = False
|
||||
elif c == "\\":
|
||||
esc = True
|
||||
elif c == '"':
|
||||
in_str = False
|
||||
elif c == '"':
|
||||
in_str = True
|
||||
elif c == "(":
|
||||
depth += 1
|
||||
elif c == ")":
|
||||
depth -= 1
|
||||
i += 1
|
||||
return text[start : i - 1]
|
||||
|
||||
|
||||
def run_rule_i_metric_literals(root: Path, report: Report) -> None:
|
||||
"""Rule I: no string literal as a metric instrument NAME or label KEY.
|
||||
|
||||
The metric-side mirror of Rule F. An instrument name or label key passed to
|
||||
an `XRPL_METRIC_*` macro or to a `meter->Create*` factory must reference a
|
||||
`*MetricNames.h` constant, so a rename is one edit instead of six and a typo
|
||||
is a compile error instead of a metric that silently never appears.
|
||||
|
||||
Exempt, for the same reasons Rule F exempts them:
|
||||
* label VALUES -- the argument after a label key is runtime data;
|
||||
* the instrument DESCRIPTION -- prose, not naming surface;
|
||||
* `*MetricNames.h` itself -- that is where the strings are defined;
|
||||
* `MetricMacros.h` -- the macro definitions, whose `(name)` is a parameter;
|
||||
* test code -- tests pass arbitrary literals to exercise the API, and
|
||||
asserting on a literal is what proves a constant's value is unchanged.
|
||||
|
||||
Scope -- a RATCHET, not a flag day. The rule fires only on a metric whose
|
||||
FAMILY already has constants declared in a `*MetricNames.h` (matched on the
|
||||
name's first underscore segment, e.g. `sync_`, `jobq_`, `unl_`). The metric
|
||||
surface predates this rule by many phases, and failing the build on ~27
|
||||
pre-existing instruments at once would force one un-reviewable mega-change.
|
||||
Declaring a constant for a family is what opts that family in, so:
|
||||
* a NEW metric in an already-converted family is caught immediately;
|
||||
* a literal left behind in a family being converted is caught;
|
||||
* an untouched legacy family stays green until someone converts it, at
|
||||
which point the whole family is enforced.
|
||||
A literal whose family has no constants at all is reported as a Rule L
|
||||
warning instead (non-fatal), so the remaining work stays visible.
|
||||
|
||||
Presence-gated on `*MetricNames.h` existing at all; with no metric header
|
||||
in the tree the rule has no families to enforce and is skipped."""
|
||||
if not find_metricname_headers(root):
|
||||
report.skip("I", "no *MetricNames.h present")
|
||||
return
|
||||
names, keys, _ = metric_constants(root)
|
||||
owned = metric_prefixes(names)
|
||||
found = False
|
||||
deferred: List[Tuple[str, str]] = []
|
||||
|
||||
def report_literal(rel: object, lineno: int, token: str, wire: str) -> None:
|
||||
nonlocal found
|
||||
# A label key is enforced once ANY label-key constant exists, because
|
||||
# the label vocabulary is shared across families -- `outcome` means the
|
||||
# same thing everywhere, so there is no per-family ratchet for it.
|
||||
in_scope = (
|
||||
any(wire.startswith(p) for p in owned) if token.startswith("name") else True
|
||||
)
|
||||
if in_scope:
|
||||
found = True
|
||||
report.violation(
|
||||
"I", f"{rel}:{lineno}", token, "use a *MetricNames.h constant"
|
||||
)
|
||||
else:
|
||||
deferred.append((f"{rel}:{lineno}", token))
|
||||
|
||||
for path in sorted(iter_sources(root)):
|
||||
if path.name.endswith("MetricNames.h") or path.name == "MetricMacros.h":
|
||||
continue
|
||||
if is_test_path(path):
|
||||
continue
|
||||
text = strip_comments(read_source(path))
|
||||
rel = path.relative_to(root)
|
||||
for kind, name_idx, arglist, lineno in metric_calls(text):
|
||||
args = split_top_level_args(arglist)
|
||||
if len(args) > name_idx:
|
||||
lit = STRING_LITERAL.search(args[name_idx])
|
||||
if lit:
|
||||
report_literal(
|
||||
rel, lineno, f'name "{lit.group(1)}" ({kind})', lit.group(1)
|
||||
)
|
||||
# Label KEYS live in the label-set initializer, e.g.
|
||||
# `{{"outcome", v}, {"reason", w}}`. Scanned over the WHOLE argument
|
||||
# list rather than per split argument, because
|
||||
# `split_top_level_args` balances only parentheses -- a brace-
|
||||
# enclosed label set contains top-level commas and would be split
|
||||
# apart mid-pair, hiding every key from the pattern.
|
||||
#
|
||||
# Scanning the whole list is safe: METRIC_PAIR_KEY requires an
|
||||
# opening `{` before the key, which the instrument name and the
|
||||
# description never have. Only the key position is checked; the
|
||||
# VALUE after it is runtime data and exempt.
|
||||
if keys:
|
||||
for pm in METRIC_PAIR_KEY.finditer(arglist):
|
||||
key = pm.group(1)
|
||||
if not key.startswith('"'):
|
||||
continue
|
||||
report_literal(rel, lineno, f"label {key} ({kind})", key.strip('"'))
|
||||
for loc, token in deferred:
|
||||
report.warning("L", loc, token, "metric family not yet converted")
|
||||
if not found:
|
||||
report.ok("I: no string-literal names/label keys in converted metric families")
|
||||
|
||||
|
||||
def iter_sources(root: Path) -> List[Path]:
|
||||
"""Every C++ source/header under src/ and include/."""
|
||||
return [
|
||||
p
|
||||
for base in ("src", "include")
|
||||
for ext in ("*.h", "*.cpp")
|
||||
for p in (root / base).rglob(ext)
|
||||
if p.is_file()
|
||||
]
|
||||
|
||||
|
||||
def instrument_kinds(root: Path, wire_by_symbol: Dict[str, str]) -> Dict[str, str]:
|
||||
"""Map each declared instrument's WIRE name to its OTel instrument kind, by
|
||||
looking at how the emit sites actually create it.
|
||||
|
||||
The kind is what decides which suffix is correct, so it must be read from
|
||||
the emit site rather than guessed from the name -- guessing from words like
|
||||
"latency" mislabels a multi-series gauge whose units live in its label
|
||||
VALUES (e.g. `nodestore_latency` observing `write_mean_us`), which is a
|
||||
legitimate shape, not a violation.
|
||||
|
||||
Returns one of `counter`, `histogram`, `gauge`, `updown` per wire name.
|
||||
A name whose emit site is not found is absent from the result, so Rule J
|
||||
checks only the shape-independent rules for it."""
|
||||
kinds: Dict[str, str] = {}
|
||||
for path in iter_sources(root):
|
||||
if path.name == "MetricMacros.h" or path.name.endswith("MetricNames.h"):
|
||||
continue
|
||||
if is_test_path(path):
|
||||
continue
|
||||
text = strip_comments(read_source(path))
|
||||
for kind, name_idx, arglist, _ in metric_calls(text):
|
||||
args = split_top_level_args(arglist)
|
||||
if len(args) <= name_idx:
|
||||
continue
|
||||
arg = args[name_idx].strip()
|
||||
lit = re.fullmatch(r'"((?:[^"\\]|\\.)*)"', arg)
|
||||
if lit:
|
||||
wire: Optional[str] = lit.group(1)
|
||||
else:
|
||||
ident = IDENTIFIER_ARG.match(arg)
|
||||
wire = (
|
||||
wire_by_symbol.get(ident.group(1).split("::")[-1])
|
||||
if ident
|
||||
else None
|
||||
)
|
||||
if wire is None:
|
||||
continue
|
||||
kinds[wire] = classify_instrument_kind(kind)
|
||||
return kinds
|
||||
|
||||
|
||||
def classify_instrument_kind(kind: str) -> str:
|
||||
"""Normalise a macro suffix (`COUNTER_INC_LABELED`) or a factory method
|
||||
name (`CreateInt64ObservableGauge`) to one of counter/histogram/gauge/
|
||||
updown. Order matters: `UpDown` and `Observable` are checked before the
|
||||
bare `Counter` substring they both contain."""
|
||||
if "HISTOGRAM" in kind or "Histogram" in kind:
|
||||
return "histogram"
|
||||
if "UPDOWN" in kind or "UpDown" in kind:
|
||||
return "updown"
|
||||
if "GAUGE" in kind or "Gauge" in kind:
|
||||
return "gauge"
|
||||
if "COUNTER" in kind or "Counter" in kind:
|
||||
return "counter"
|
||||
return "other"
|
||||
|
||||
|
||||
def symbol_wire_names(root: Path) -> Dict[str, str]:
|
||||
"""Map each `*MetricNames.h` constant's C++ symbol name to its wire string,
|
||||
so an emit site referencing `metric::dnsResolveTotal` can be resolved back
|
||||
to `dns_resolve_total`."""
|
||||
out: Dict[str, str] = {}
|
||||
for h in find_metricname_headers(root):
|
||||
for m in METRIC_CONST_DEF.finditer(strip_comments(read_source(h))):
|
||||
out[m.group(1)] = m.group(2)
|
||||
return out
|
||||
|
||||
|
||||
def run_rule_j_metric_suffixes(root: Path, report: Report) -> None:
|
||||
"""Rule J: instrument names follow the naming and unit/kind suffix rules.
|
||||
|
||||
Checked against the `namespace metric` constants in `*MetricNames.h`, which
|
||||
Rule I makes the only place an instrument name can be spelled. Enforced:
|
||||
|
||||
* lower_snake_case (same shape as Rule G for span attributes);
|
||||
* no `xrpld_`/`xrpl_` prefix -- the Prometheus exporter adds the namespace
|
||||
itself, so a name carrying it would emit `xrpld_xrpld_*` on the wire;
|
||||
* a monotonic COUNTER ends in `_total`, so `rate()` over it reads
|
||||
correctly and a reader can tell it from a gauge;
|
||||
* a HISTOGRAM ends in `_us`, `_ms` or `_seconds`: histograms in this repo
|
||||
are all durations, and the OTel `unit` argument is not surfaced on the
|
||||
Prometheus metric name, so an unlabelled duration is ambiguous;
|
||||
* a GAUGE does not end in `_total`, which is reserved for counters.
|
||||
|
||||
The instrument KIND comes from the emit site (see `instrument_kinds`), not
|
||||
from words in the name, so a multi-series gauge that carries its units in
|
||||
its label values is not mistaken for a mis-suffixed duration.
|
||||
|
||||
Presence-gated: skipped when no `*MetricNames.h` exists."""
|
||||
if not find_metricname_headers(root):
|
||||
report.skip("J", "no *MetricNames.h present")
|
||||
return
|
||||
names, _, _ = metric_constants(root)
|
||||
if not names:
|
||||
report.skip("J", "no metric instrument-name constants declared")
|
||||
return
|
||||
kinds = instrument_kinds(root, symbol_wire_names(root))
|
||||
found = False
|
||||
|
||||
def flag(name: str, expected: str) -> None:
|
||||
nonlocal found
|
||||
found = True
|
||||
report.violation("J", "*MetricNames.h", name, expected)
|
||||
|
||||
for name in sorted(names):
|
||||
if not SNAKE_SEGMENT.match(name):
|
||||
flag(name, "must be lower_snake_case")
|
||||
continue
|
||||
if name.startswith(("xrpld_", "xrpl_")):
|
||||
flag(name, "drop the prefix; the exporter adds it")
|
||||
continue
|
||||
kind = kinds.get(name)
|
||||
if kind == "counter" and not name.endswith(METRIC_COUNTER_SUFFIX):
|
||||
flag(name, "counter must end in _total")
|
||||
elif kind == "histogram" and not name.endswith(METRIC_DURATION_SUFFIXES):
|
||||
flag(name, "histogram needs _us/_ms/_seconds suffix")
|
||||
elif kind == "gauge" and name.endswith(METRIC_COUNTER_SUFFIX):
|
||||
flag(name, "_total is reserved for counters")
|
||||
if not found:
|
||||
report.ok(f"J: {len(names)} metric instrument name(s) follow the naming rules")
|
||||
|
||||
|
||||
def run_rule_k_expected_metrics(root: Path, report: Report) -> None:
|
||||
"""Rule K: every metric named in the workload's `expected_metrics.json`
|
||||
exists as a `namespace metric` constant.
|
||||
|
||||
The layer that cannot reference a C++ constant, so it is validated against
|
||||
one instead. This is the check that catches the failure mode this rule set
|
||||
exists for: a metric renamed in code while the workload validator still
|
||||
asserts on the old name, which fails as "metric never appeared" at runtime
|
||||
rather than at review time.
|
||||
|
||||
Scope. Only groups whose metrics come from the OTel instrument API are
|
||||
checked. The file also inventories `beast::insight` statsd gauges/counters
|
||||
(`statsd_gauges`, `statsd_counters`) and collector-derived spanmetrics, whose
|
||||
names are minted by a different emit path -- `formatName()` lowercasing an
|
||||
insight metric, or the spanmetrics connector -- and so have no
|
||||
`*MetricNames.h` constant to resolve to by design. Checking them would fail
|
||||
the build on metrics this convention does not govern. Within the OTel groups
|
||||
the family ratchet still applies, so an unconverted family is out of scope
|
||||
too. Presence-gated on both layers."""
|
||||
path = root / "docker" / "telemetry" / "workload" / "expected_metrics.json"
|
||||
if not path.is_file():
|
||||
report.skip("K", "expected_metrics.json not present")
|
||||
return
|
||||
if not find_metricname_headers(root):
|
||||
report.skip("K", "no *MetricNames.h to validate against")
|
||||
return
|
||||
names, _, _ = metric_constants(root)
|
||||
try:
|
||||
doc = json.loads(read_source(path))
|
||||
except json.JSONDecodeError as exc:
|
||||
report.violation("K", str(path.relative_to(root)), str(exc), "valid JSON")
|
||||
return
|
||||
declared = {
|
||||
base_metric_name(e) for e in expected_metric_names(doc, NON_OTEL_METRIC_GROUPS)
|
||||
}
|
||||
# Only entries inside a metric FAMILY the constants already own are checked.
|
||||
# Anything else in the file predates *MetricNames.h, so demanding a constant
|
||||
# for it would fail the build on unrelated pre-existing metrics rather than
|
||||
# on the drift this rule targets.
|
||||
owned = metric_prefixes(names)
|
||||
unknown = sorted(
|
||||
n for n in declared if n not in names and any(n.startswith(p) for p in owned)
|
||||
)
|
||||
for n in unknown:
|
||||
report.violation(
|
||||
"K",
|
||||
str(path.relative_to(root)),
|
||||
n,
|
||||
"no matching *MetricNames.h constant",
|
||||
)
|
||||
if not unknown:
|
||||
checked = sum(1 for n in declared if n in names)
|
||||
report.ok(f"K: {checked} expected-metric name(s) resolve to constants")
|
||||
|
||||
|
||||
# Suffixes the Prometheus exporter appends to a histogram instrument. The
|
||||
# instrument name declared in code is the stem, so they are stripped before the
|
||||
# constant lookup.
|
||||
HISTOGRAM_SUFFIXES = ("_bucket", "_count", "_sum")
|
||||
|
||||
|
||||
def base_metric_name(entry: str) -> str:
|
||||
"""Reduce an `expected_metrics.json` entry to the bare instrument name.
|
||||
|
||||
Entries are written the way an operator would query them, not the way the
|
||||
code declares them, so two forms have to be normalized away:
|
||||
* a PromQL label selector -- `sync_state{metric="ledgers_behind"}`;
|
||||
* an exporter-appended histogram suffix -- `..._ms_bucket`/`_count`/`_sum`,
|
||||
which the SDK adds and the code never names.
|
||||
Without this the rule would reject every entry in the file, which is a
|
||||
checker bug rather than a naming violation."""
|
||||
name = entry.split("{", 1)[0].strip()
|
||||
for suffix in HISTOGRAM_SUFFIXES:
|
||||
if name.endswith(suffix):
|
||||
return name[: -len(suffix)]
|
||||
return name
|
||||
|
||||
|
||||
def metric_prefixes(names: Set[str]) -> Set[str]:
|
||||
"""The first underscore-delimited segment of each declared instrument name,
|
||||
e.g. {`sync_`, `jobq_`, `unl_`}. Used by Rule K to decide whether an entry
|
||||
in `expected_metrics.json` belongs to a family this repo's constants own --
|
||||
so the rule flags a stale name inside a converted family without demanding a
|
||||
constant for every unrelated metric in the file."""
|
||||
return {n.split("_", 1)[0] + "_" for n in names if "_" in n}
|
||||
|
||||
|
||||
# Groups in expected_metrics.json whose names are NOT minted by the OTel
|
||||
# instrument API, and therefore have no *MetricNames.h constant by design:
|
||||
# statsd_gauges / statsd_counters -- beast::insight metrics, whose wire names
|
||||
# come from formatName() lowercasing an insight metric path;
|
||||
# spanmetrics -- synthesised by the collector's spanmetrics connector from
|
||||
# span names, not declared in C++ at all.
|
||||
NON_OTEL_METRIC_GROUPS = frozenset({"statsd_gauges", "statsd_counters", "spanmetrics"})
|
||||
|
||||
|
||||
def expected_metric_names(
|
||||
doc: object, skip_groups: frozenset = frozenset()
|
||||
) -> Set[str]:
|
||||
"""Collect metric names from `expected_metrics.json`.
|
||||
|
||||
The file groups metrics by source (`spanmetrics`, `statsd_gauges`,
|
||||
`phase9_nodestore`, ...), each group holding a `metrics` LIST OF STRINGS
|
||||
alongside a prose `description`. Both that shape and a
|
||||
`[{"metric": "..."}]` shape are accepted, and the walk is generic, so a
|
||||
layout change degrades to "checks fewer names" rather than silently
|
||||
checking none. `description` is skipped explicitly -- it is prose, and
|
||||
letting it through would feed whole sentences to the family match.
|
||||
|
||||
`skip_groups` drops whole top-level groups (see NON_OTEL_METRIC_GROUPS)."""
|
||||
out: Set[str] = set()
|
||||
|
||||
def walk(node: object, in_metrics: bool = False) -> None:
|
||||
if isinstance(node, dict):
|
||||
for key, val in node.items():
|
||||
if key == "description" or key in skip_groups:
|
||||
continue
|
||||
if key in ("metric", "name") and isinstance(val, str):
|
||||
out.add(val)
|
||||
else:
|
||||
walk(val, in_metrics or key == "metrics")
|
||||
elif isinstance(node, list):
|
||||
for item in node:
|
||||
if in_metrics and isinstance(item, str):
|
||||
out.add(item)
|
||||
else:
|
||||
walk(item, in_metrics)
|
||||
|
||||
walk(doc)
|
||||
return out
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
main()
|
||||
|
||||
@@ -1,5 +1,9 @@
|
||||
#!/usr/bin/env python3
|
||||
|
||||
# cspell:ignore ISTOGRAM
|
||||
# The all-caps macro name XRPL_METRIC_HISTOGRAM_RECORD trips cspell's
|
||||
# compound-word splitter, which emits the subword "ISTOGRAM"; ignore it here.
|
||||
|
||||
"""Unit tests for check_otel_naming.py.
|
||||
|
||||
Stdlib-only (unittest), matching the dependency-free policy of the check itself.
|
||||
@@ -897,5 +901,471 @@ class RuleEReportTuple(unittest.TestCase):
|
||||
shutil.rmtree(d)
|
||||
|
||||
|
||||
# A minimal *MetricNames.h body: the three namespaces the metric rules read.
|
||||
def _metric_header(
|
||||
metric_body: str = "", label_body: str = "", lval_body: str = ""
|
||||
) -> str:
|
||||
return (
|
||||
"#pragma once\n"
|
||||
"namespace xrpl::telemetry {\n"
|
||||
f"namespace metric {{\n{metric_body}\n}}\n"
|
||||
f"namespace label {{\n{label_body}\n}}\n"
|
||||
f"namespace lval {{\n{lval_body}\n}}\n"
|
||||
"}\n"
|
||||
)
|
||||
|
||||
|
||||
def _mc(symbol: str, wire: str) -> str:
|
||||
"""One `inline constexpr char SYM[] = "wire";` declaration."""
|
||||
return f'inline constexpr char {symbol}[] = "{wire}";'
|
||||
|
||||
|
||||
class MetricConstantExtraction(unittest.TestCase):
|
||||
"""L1-metrics: instrument names / label keys / label values are read from
|
||||
the right namespace, and comments never seed the set."""
|
||||
|
||||
def _run(self, header_text):
|
||||
d = Path(tempfile.mkdtemp())
|
||||
try:
|
||||
_write(d / "src" / "xrpld" / "telemetry" / "MetricNames.h", header_text)
|
||||
return chk.metric_constants(d)
|
||||
finally:
|
||||
shutil.rmtree(d)
|
||||
|
||||
def test_splits_by_namespace(self):
|
||||
names, keys, vals = self._run(
|
||||
_metric_header(
|
||||
_mc("dnsResolveTotal", "dns_resolve_total"),
|
||||
_mc("outcome", "outcome"),
|
||||
_mc("resolved", "resolved"),
|
||||
)
|
||||
)
|
||||
self.assertEqual(names, {"dns_resolve_total"})
|
||||
self.assertEqual(keys, {"outcome"})
|
||||
self.assertEqual(vals, {"resolved"})
|
||||
|
||||
def test_nested_lval_namespace_included(self):
|
||||
# `namespace lval { namespace dns_resolve { ... } }` — the inner block is
|
||||
# inside the outer's brace-matched span, so its values must be collected.
|
||||
_, _, vals = self._run(
|
||||
_metric_header(
|
||||
lval_body="namespace dns_resolve {\n"
|
||||
+ _mc("resolved", "resolved")
|
||||
+ "\n}\n"
|
||||
)
|
||||
)
|
||||
self.assertEqual(vals, {"resolved"})
|
||||
|
||||
def test_commented_constant_not_collected(self):
|
||||
names, _, _ = self._run(
|
||||
_metric_header(
|
||||
"// " + _mc("ghost", "ghost_total") + "\n" + _mc("real", "real_total")
|
||||
)
|
||||
)
|
||||
self.assertEqual(names, {"real_total"})
|
||||
|
||||
def test_block_commented_constant_not_collected(self):
|
||||
names, _, _ = self._run(
|
||||
_metric_header("/* " + _mc("ghost", "ghost_total") + " */\n")
|
||||
)
|
||||
self.assertEqual(names, set())
|
||||
|
||||
def test_no_header_empty_sets(self):
|
||||
d = Path(tempfile.mkdtemp())
|
||||
try:
|
||||
(d / "src").mkdir()
|
||||
self.assertEqual(chk.metric_constants(d), (set(), set(), set()))
|
||||
finally:
|
||||
shutil.rmtree(d)
|
||||
|
||||
|
||||
class RuleIMetricLiterals(unittest.TestCase):
|
||||
"""Rule I: literal instrument names / label keys at a metric emit site are
|
||||
flagged, but only inside a metric FAMILY that already has constants."""
|
||||
|
||||
def _run(self, rel_path, source, header=None):
|
||||
d = Path(tempfile.mkdtemp())
|
||||
try:
|
||||
_write(
|
||||
d / "src" / "xrpld" / "telemetry" / "MetricNames.h",
|
||||
(
|
||||
header
|
||||
if header is not None
|
||||
else _metric_header(
|
||||
_mc("syncState", "sync_state"), _mc("outcome", "outcome")
|
||||
)
|
||||
),
|
||||
)
|
||||
_write(d / rel_path, source)
|
||||
report = chk.Report()
|
||||
chk.run_rule_i_metric_literals(d, report)
|
||||
return (
|
||||
sorted(v[2] for v in report.violations),
|
||||
sorted(w[2] for w in report.warnings),
|
||||
)
|
||||
finally:
|
||||
shutil.rmtree(d)
|
||||
|
||||
# ----- positive: a literal in a converted family must FAIL -----
|
||||
def test_literal_macro_name_flagged(self):
|
||||
v, _ = self._run(
|
||||
"src/xrpld/Foo.cpp",
|
||||
'XRPL_METRIC_COUNTER_INC(app, "sync_acquire_total", "d");\n',
|
||||
)
|
||||
self.assertEqual(v, ['name "sync_acquire_total" (COUNTER_INC)'])
|
||||
|
||||
def test_literal_factory_name_flagged(self):
|
||||
v, _ = self._run(
|
||||
"src/xrpld/Foo.cpp",
|
||||
'meter_->CreateInt64ObservableGauge("sync_thing", "d");\n',
|
||||
)
|
||||
self.assertEqual(v, ['name "sync_thing" (CreateInt64ObservableGauge)'])
|
||||
|
||||
def test_literal_label_key_flagged(self):
|
||||
v, _ = self._run(
|
||||
"src/xrpld/Foo.cpp",
|
||||
'XRPL_METRIC_COUNTER_INC_LABELED(app, metric::syncState, "d", '
|
||||
'{{"outcome", std::string(x)}});\n',
|
||||
)
|
||||
self.assertEqual(v, ['label "outcome" (COUNTER_INC_LABELED)'])
|
||||
|
||||
def test_multiline_call_flagged(self):
|
||||
v, _ = self._run(
|
||||
"src/xrpld/Foo.cpp",
|
||||
'XRPL_METRIC_COUNTER_INC(\n app,\n "sync_x_total",\n "d");\n',
|
||||
)
|
||||
self.assertEqual(v, ['name "sync_x_total" (COUNTER_INC)'])
|
||||
|
||||
# ----- negative: things Rule I must NOT flag -----
|
||||
def test_constant_name_accepted(self):
|
||||
v, _ = self._run(
|
||||
"src/xrpld/Foo.cpp",
|
||||
'XRPL_METRIC_COUNTER_INC(app, metric::syncState, "d");\n',
|
||||
)
|
||||
self.assertEqual(v, [])
|
||||
|
||||
def test_label_value_exempt(self):
|
||||
# The VALUE after a constant key is runtime data and must not be flagged.
|
||||
v, _ = self._run(
|
||||
"src/xrpld/Foo.cpp",
|
||||
'XRPL_METRIC_COUNTER_INC_LABELED(app, metric::syncState, "d", '
|
||||
'{{label::outcome, std::string("resolved")}});\n',
|
||||
)
|
||||
self.assertEqual(v, [])
|
||||
|
||||
def test_description_exempt(self):
|
||||
# arg2 is prose, not naming surface.
|
||||
v, _ = self._run(
|
||||
"src/xrpld/Foo.cpp",
|
||||
'XRPL_METRIC_COUNTER_INC(app, metric::syncState, "Some description");\n',
|
||||
)
|
||||
self.assertEqual(v, [])
|
||||
|
||||
def test_test_path_exempt(self):
|
||||
v, _ = self._run(
|
||||
"src/tests/libxrpl/telemetry/MetricMacros.cpp",
|
||||
'XRPL_METRIC_COUNTER_INC(app, "sync_lit_total", "d");\n',
|
||||
)
|
||||
self.assertEqual(v, [])
|
||||
|
||||
def test_metricnames_header_exempt(self):
|
||||
v, _ = self._run(
|
||||
"src/xrpld/telemetry/OtherMetricNames.h",
|
||||
'XRPL_METRIC_COUNTER_INC(app, "sync_lit_total", "d");\n',
|
||||
)
|
||||
self.assertEqual(v, [])
|
||||
|
||||
def test_macro_definition_header_exempt(self):
|
||||
v, _ = self._run(
|
||||
"src/xrpld/telemetry/MetricMacros.h",
|
||||
'XRPL_METRIC_COUNTER_INC(app, "sync_lit_total", "d");\n',
|
||||
)
|
||||
self.assertEqual(v, [])
|
||||
|
||||
# ----- the ratchet: an unconverted family warns (L) instead of failing -----
|
||||
def test_unconverted_family_warns_not_fails(self):
|
||||
v, w = self._run(
|
||||
"src/xrpld/Foo.cpp",
|
||||
'meter_->CreateDoubleObservableGauge("txq_metrics", "d");\n',
|
||||
)
|
||||
self.assertEqual(v, [])
|
||||
self.assertEqual(w, ['name "txq_metrics" (CreateDoubleObservableGauge)'])
|
||||
|
||||
def test_skip_when_no_metric_header(self):
|
||||
d = Path(tempfile.mkdtemp())
|
||||
try:
|
||||
_write(
|
||||
d / "src" / "xrpld" / "Foo.cpp",
|
||||
'XRPL_METRIC_COUNTER_INC(app, "x_total", "d");\n',
|
||||
)
|
||||
report = chk.Report()
|
||||
chk.run_rule_i_metric_literals(d, report)
|
||||
self.assertEqual(report.violations, [])
|
||||
self.assertTrue(any("I" in s for s in report.skips))
|
||||
finally:
|
||||
shutil.rmtree(d)
|
||||
|
||||
|
||||
class RuleJMetricSuffixes(unittest.TestCase):
|
||||
"""Rule J: instrument names follow the kind-appropriate suffix rules, with
|
||||
the KIND read from the emit site rather than guessed from the name."""
|
||||
|
||||
def _run(self, metric_body, emit_source=""):
|
||||
d = Path(tempfile.mkdtemp())
|
||||
try:
|
||||
_write(
|
||||
d / "src" / "xrpld" / "telemetry" / "MetricNames.h",
|
||||
_metric_header(metric_body),
|
||||
)
|
||||
if emit_source:
|
||||
_write(d / "src" / "xrpld" / "Foo.cpp", emit_source)
|
||||
report = chk.Report()
|
||||
chk.run_rule_j_metric_suffixes(d, report)
|
||||
return sorted((v[2], v[3]) for v in report.violations)
|
||||
finally:
|
||||
shutil.rmtree(d)
|
||||
|
||||
# ----- positive -----
|
||||
def test_counter_without_total_flagged(self):
|
||||
self.assertEqual(
|
||||
self._run(
|
||||
_mc("dnsResolve", "dns_resolve"),
|
||||
'XRPL_METRIC_COUNTER_INC(app, metric::dnsResolve, "d");\n',
|
||||
),
|
||||
[("dns_resolve", "counter must end in _total")],
|
||||
)
|
||||
|
||||
def test_histogram_without_unit_flagged(self):
|
||||
self.assertEqual(
|
||||
self._run(
|
||||
_mc("dialLatency", "overlay_dial_latency"),
|
||||
'XRPL_METRIC_HISTOGRAM_RECORD(app, metric::dialLatency, "d", v);\n',
|
||||
),
|
||||
[("overlay_dial_latency", "histogram needs _us/_ms/_seconds suffix")],
|
||||
)
|
||||
|
||||
def test_gauge_with_total_flagged(self):
|
||||
self.assertEqual(
|
||||
self._run(
|
||||
_mc("syncTotal", "sync_total"),
|
||||
'meter_->CreateInt64ObservableGauge(metric::syncTotal, "d");\n',
|
||||
),
|
||||
[("sync_total", "_total is reserved for counters")],
|
||||
)
|
||||
|
||||
def test_prefixed_name_flagged(self):
|
||||
self.assertEqual(
|
||||
self._run(_mc("prefixed", "xrpld_sync_state")),
|
||||
[("xrpld_sync_state", "drop the prefix; the exporter adds it")],
|
||||
)
|
||||
|
||||
def test_non_snake_case_flagged(self):
|
||||
self.assertEqual(
|
||||
self._run(_mc("camel", "syncState")),
|
||||
[("syncState", "must be lower_snake_case")],
|
||||
)
|
||||
|
||||
# ----- negative -----
|
||||
def test_conforming_counter_accepted(self):
|
||||
self.assertEqual(
|
||||
self._run(
|
||||
_mc("dnsResolveTotal", "dns_resolve_total"),
|
||||
'XRPL_METRIC_COUNTER_INC(app, metric::dnsResolveTotal, "d");\n',
|
||||
),
|
||||
[],
|
||||
)
|
||||
|
||||
def test_conforming_histogram_accepted(self):
|
||||
self.assertEqual(
|
||||
self._run(
|
||||
_mc("dialMs", "overlay_dial_latency_ms"),
|
||||
'XRPL_METRIC_HISTOGRAM_RECORD(app, metric::dialMs, "d", v);\n',
|
||||
),
|
||||
[],
|
||||
)
|
||||
|
||||
def test_gauge_named_latency_is_not_flagged(self):
|
||||
# The regression this rule's kind-awareness exists for: a multi-series
|
||||
# GAUGE whose units live in its label VALUES (nodestore_latency
|
||||
# observing write_mean_us) must not be read as a mis-suffixed duration.
|
||||
self.assertEqual(
|
||||
self._run(
|
||||
_mc("nodestoreLatency", "nodestore_latency"),
|
||||
'meter_->CreateInt64ObservableGauge(metric::nodestoreLatency, "d");\n',
|
||||
),
|
||||
[],
|
||||
)
|
||||
|
||||
def test_unknown_kind_only_shape_checked(self):
|
||||
# With no emit site found, the kind is unknown, so only the
|
||||
# shape-independent rules apply -- a bare gauge-ish name is fine.
|
||||
self.assertEqual(self._run(_mc("syncState", "sync_state")), [])
|
||||
|
||||
def test_skip_when_no_header(self):
|
||||
d = Path(tempfile.mkdtemp())
|
||||
try:
|
||||
(d / "src").mkdir()
|
||||
report = chk.Report()
|
||||
chk.run_rule_j_metric_suffixes(d, report)
|
||||
self.assertEqual(report.violations, [])
|
||||
self.assertTrue(any("J" in s for s in report.skips))
|
||||
finally:
|
||||
shutil.rmtree(d)
|
||||
|
||||
|
||||
class RuleKExpectedMetrics(unittest.TestCase):
|
||||
"""Rule K: a name in expected_metrics.json inside a converted family must
|
||||
resolve to a *MetricNames.h constant."""
|
||||
|
||||
def _run(self, json_text, metric_body):
|
||||
d = Path(tempfile.mkdtemp())
|
||||
try:
|
||||
_write(
|
||||
d / "src" / "xrpld" / "telemetry" / "MetricNames.h",
|
||||
_metric_header(metric_body),
|
||||
)
|
||||
_write(
|
||||
d / "docker" / "telemetry" / "workload" / "expected_metrics.json",
|
||||
json_text,
|
||||
)
|
||||
report = chk.Report()
|
||||
chk.run_rule_k_expected_metrics(d, report)
|
||||
return sorted(v[2] for v in report.violations), report.skips
|
||||
finally:
|
||||
shutil.rmtree(d)
|
||||
|
||||
def test_stale_name_in_converted_family_flagged(self):
|
||||
# `sync_` is owned (sync_state is declared), so a sibling that resolves
|
||||
# to nothing is the rename-drift this rule exists to catch.
|
||||
v, _ = self._run(
|
||||
'{"metrics": [{"metric": "sync_stale_total"}]}',
|
||||
_mc("syncState", "sync_state"),
|
||||
)
|
||||
self.assertEqual(v, ["sync_stale_total"])
|
||||
|
||||
def test_declared_name_accepted(self):
|
||||
v, _ = self._run(
|
||||
'{"metrics": [{"metric": "sync_state"}]}', _mc("syncState", "sync_state")
|
||||
)
|
||||
self.assertEqual(v, [])
|
||||
|
||||
def test_unowned_family_not_flagged(self):
|
||||
# `txq_` has no constants, so its entries are out of scope rather than
|
||||
# a failure -- the ratchet again.
|
||||
v, _ = self._run(
|
||||
'{"metrics": [{"metric": "txq_metrics"}]}', _mc("syncState", "sync_state")
|
||||
)
|
||||
self.assertEqual(v, [])
|
||||
|
||||
def test_name_key_also_read(self):
|
||||
v, _ = self._run(
|
||||
'{"metrics": [{"name": "sync_stale_total"}]}',
|
||||
_mc("syncState", "sync_state"),
|
||||
)
|
||||
self.assertEqual(v, ["sync_stale_total"])
|
||||
|
||||
def test_nested_structure_walked(self):
|
||||
v, _ = self._run(
|
||||
'{"groups": {"a": {"items": [{"metric": "sync_stale_total"}]}}}',
|
||||
_mc("syncState", "sync_state"),
|
||||
)
|
||||
self.assertEqual(v, ["sync_stale_total"])
|
||||
|
||||
def test_promql_selector_stripped(self):
|
||||
# Entries are written as an operator would query them; the selector must
|
||||
# be stripped before the constant lookup or every entry would fail.
|
||||
v, _ = self._run(
|
||||
'{"g": {"metrics": ["sync_state{metric=\\"ledgers_behind\\"}"]}}',
|
||||
_mc("syncState", "sync_state"),
|
||||
)
|
||||
self.assertEqual(v, [])
|
||||
|
||||
def test_histogram_suffix_stripped(self):
|
||||
# _bucket/_count/_sum are appended by the exporter, never named in code.
|
||||
v, _ = self._run(
|
||||
'{"g": {"metrics": ["overlay_dial_latency_ms_bucket"]}}',
|
||||
_mc("dialMs", "overlay_dial_latency_ms"),
|
||||
)
|
||||
self.assertEqual(v, [])
|
||||
|
||||
def test_statsd_group_skipped(self):
|
||||
# beast::insight names come from formatName(), not the OTel API, so a
|
||||
# statsd group must not be held to a *MetricNames.h constant.
|
||||
v, _ = self._run(
|
||||
'{"statsd_gauges": {"metrics": ["sync_not_a_constant"]}}',
|
||||
_mc("syncState", "sync_state"),
|
||||
)
|
||||
self.assertEqual(v, [])
|
||||
|
||||
def test_spanmetrics_group_skipped(self):
|
||||
v, _ = self._run(
|
||||
'{"spanmetrics": {"metrics": ["sync_span_calls_total"]}}',
|
||||
_mc("syncState", "sync_state"),
|
||||
)
|
||||
self.assertEqual(v, [])
|
||||
|
||||
def test_description_prose_not_treated_as_metric(self):
|
||||
v, _ = self._run(
|
||||
'{"g": {"description": "sync_ stuff about metrics", "metrics": []}}',
|
||||
_mc("syncState", "sync_state"),
|
||||
)
|
||||
self.assertEqual(v, [])
|
||||
|
||||
def test_malformed_json_reported(self):
|
||||
v, _ = self._run("{not json", _mc("syncState", "sync_state"))
|
||||
self.assertEqual(len(v), 1)
|
||||
|
||||
def test_skip_when_file_absent(self):
|
||||
d = Path(tempfile.mkdtemp())
|
||||
try:
|
||||
_write(
|
||||
d / "src" / "xrpld" / "telemetry" / "MetricNames.h",
|
||||
_metric_header(_mc("syncState", "sync_state")),
|
||||
)
|
||||
report = chk.Report()
|
||||
chk.run_rule_k_expected_metrics(d, report)
|
||||
self.assertEqual(report.violations, [])
|
||||
self.assertTrue(any("K" in s for s in report.skips))
|
||||
finally:
|
||||
shutil.rmtree(d)
|
||||
|
||||
|
||||
class InstrumentKindClassification(unittest.TestCase):
|
||||
"""classify_instrument_kind: `UpDown`/`Observable` are checked before the
|
||||
bare `Counter` substring they both contain."""
|
||||
|
||||
def test_macro_kinds(self):
|
||||
for kind, want in (
|
||||
("COUNTER_INC", "counter"),
|
||||
("COUNTER_ADD_LABELED", "counter"),
|
||||
("HISTOGRAM_RECORD", "histogram"),
|
||||
("UPDOWN_ADD", "updown"),
|
||||
("OBSERVABLE_GAUGE_REGISTER", "gauge"),
|
||||
):
|
||||
self.assertEqual(chk.classify_instrument_kind(kind), want, kind)
|
||||
|
||||
def test_factory_kinds(self):
|
||||
for kind, want in (
|
||||
("CreateUInt64Counter", "counter"),
|
||||
("CreateDoubleHistogram", "histogram"),
|
||||
("CreateInt64UpDownCounter", "updown"),
|
||||
("CreateInt64ObservableGauge", "gauge"),
|
||||
("CreateInt64ObservableCounter", "counter"),
|
||||
("CreateInt64ObservableUpDownCounter", "updown"),
|
||||
):
|
||||
self.assertEqual(chk.classify_instrument_kind(kind), want, kind)
|
||||
|
||||
|
||||
class MetricPrefixFamilies(unittest.TestCase):
|
||||
def test_first_segment_is_the_family(self):
|
||||
self.assertEqual(
|
||||
chk.metric_prefixes({"sync_state", "jobq_backlog", "unl_quorum"}),
|
||||
{"sync_", "jobq_", "unl_"},
|
||||
)
|
||||
|
||||
def test_name_without_underscore_has_no_family(self):
|
||||
self.assertEqual(chk.metric_prefixes({"uptime"}), set())
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main(verbosity=2)
|
||||
|
||||
Reference in New Issue
Block a user