From 295ee1aa3611792868f6c4e6f865c48cd238f53b Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Mon, 27 Jul 2026 12:20:01 +0100 Subject: [PATCH] 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) --- .github/scripts/otel-naming/README.md | 30 +- .../scripts/otel-naming/check_otel_naming.py | 562 +++++++++++++- .../otel-naming/test_check_otel_naming.py | 470 ++++++++++++ CONTRIBUTING.md | 58 +- docs/telemetry-runbook.md | 35 +- src/tests/libxrpl/telemetry/MetricMacros.cpp | 221 +++--- src/xrpld/app/consensus/RCLConsensus.cpp | 3 +- src/xrpld/app/ledger/detail/InboundLedger.cpp | 20 +- .../app/ledger/detail/LedgerDeltaAcquire.cpp | 6 +- src/xrpld/app/ledger/detail/LedgerMaster.cpp | 5 +- .../app/ledger/detail/LedgerReplayTask.cpp | 13 +- .../app/ledger/detail/SkipListAcquire.cpp | 6 +- src/xrpld/app/misc/NetworkOPs.cpp | 8 +- src/xrpld/app/misc/detail/ValidatorSite.cpp | 15 +- src/xrpld/overlay/detail/ConnectAttempt.cpp | 7 +- src/xrpld/overlay/detail/Handshake.cpp | 76 +- src/xrpld/overlay/detail/OverlayImpl.cpp | 32 +- src/xrpld/overlay/detail/PeerImp.cpp | 75 +- src/xrpld/overlay/detail/PeerImp.h | 5 +- src/xrpld/telemetry/MetricNames.h | 695 ++++++++++++++++++ src/xrpld/telemetry/MetricsRegistry.cpp | 155 ++-- 21 files changed, 2219 insertions(+), 278 deletions(-) create mode 100644 src/xrpld/telemetry/MetricNames.h diff --git a/.github/scripts/otel-naming/README.md b/.github/scripts/otel-naming/README.md index 660b574ed0..50051e8e31 100644 --- a/.github/scripts/otel-naming/README.md +++ b/.github/scripts/otel-naming/README.md @@ -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..` 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..` 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 diff --git a/.github/scripts/otel-naming/check_otel_naming.py b/.github/scripts/otel-naming/check_otel_naming.py index 2df6c43790..2847397afd 100644 --- a/.github/scripts/otel-naming/check_otel_naming.py +++ b/.github/scripts/otel-naming/check_otel_naming.py @@ -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 { ... }` 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() diff --git a/.github/scripts/otel-naming/test_check_otel_naming.py b/.github/scripts/otel-naming/test_check_otel_naming.py index 890648cefe..451d4eff97 100644 --- a/.github/scripts/otel-naming/test_check_otel_naming.py +++ b/.github/scripts/otel-naming/test_check_otel_naming.py @@ -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) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d15548e239..778babf012 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -414,13 +414,65 @@ python .github/scripts/otel-naming/check_otel_naming.py See [.github/scripts/otel-naming/README.md](.github/scripts/otel-naming/README.md) for the full rule list. +## Telemetry metric naming + +The metric-side counterpart of the span rules above. Metric instrument names and +metric label keys are duplicated across the emit site, the instrument +registration, the unit test, `expected_metrics.json`, the dashboard PromQL and +the runbook, so a rename touches six places and a typo in any one of them fails +silently at runtime — a metric that never appears, or a label that never joins. +The constants in the `*MetricNames.h` headers are the single source of truth for +the C++ layers; a CI check validates the layers that cannot reference a constant. + +1. Instrument names are bare `lower_snake_case` with **no `xrpld_` prefix**. The + Prometheus exporter adds the namespace itself, so a name carrying it emits + `xrpld_xrpld_*` on the wire. +2. A monotonic counter ends in `_total`, so `rate()` over it reads correctly and + a reader can tell it from a gauge at a glance. +3. A duration carries its unit as the suffix — `_us`, `_ms` or `_seconds`. The + unit belongs in the name because the OTel `unit` argument is not surfaced on + the Prometheus metric name. +4. A gauge that snapshots current state takes no suffix (`jobq_backlog`, + `sync_state`), and never `_total`. +5. Label keys are `lower_snake_case` and must have **bounded** cardinality. A + multi-series gauge discriminates its readings with the `metric` label rather + than minting one instrument per reading. +6. Label **values** are declared as constants only when the code picks them from + a fixed set (`namespace lval`). A value derived from runtime data — a peer + address, a ledger hash — must never become a label on a metric. + +Always reference the `*MetricNames.h` constants for instrument names and label +keys — never pass a string literal. (Label _values_ may be runtime data.) Note +that these headers use `constexpr char[]`, not the `makeStr`/`StaticStr` DSL the +`*SpanNames.h` headers use: the OTel C++ API takes `nostd::string_view`, which +constructs from `char const*` but has no constructor from `std::string_view`, so +`StaticStr` does not compile in an instrument-name or label-key position. + +Enforcement is by the same script as the span rules, whose metric rules are: + +- **I** — no string literal as an instrument name or label key at an emit site + (the mirror of Rule F). Scoped by metric _family_ (the first underscore + segment) so conversion can proceed subsystem by subsystem: declaring a + constant opts that family in. An unconverted family is reported as a + non-fatal **L** warning, keeping the remaining work visible. +- **J** — the suffix conventions above. The instrument _kind_ is read from the + emit site, not guessed from the name, so a multi-series gauge whose units live + in its label values is not mistaken for a mis-suffixed duration. +- **K** — every metric named in `docker/telemetry/workload/expected_metrics.json` + resolves to a declared constant. This is the check that catches a metric + renamed in code while the workload validator still asserts the old name. + Groups whose names come from a different emit path (`statsd_gauges`, + `statsd_counters` from `beast::insight`, and collector-derived `spanmetrics`) + are out of scope by design. + ## Adding a new OTel metric See `src/xrpld/telemetry/MetricMacros.h` for the call-site macros covering every OTel instrument kind (Counter, UpDownCounter, Histogram, Gauge, and their -Observable/async counterparts) and the "Adding a New Metric" section in -[docs/telemetry-runbook.md](docs/telemetry-runbook.md) for the walkthrough and a -need-to-macro lookup table. +Observable/async counterparts), `src/xrpld/telemetry/MetricNames.h` for the name +and label constants to reference (and the rules above), and the "Adding a New +Metric" section in [docs/telemetry-runbook.md](docs/telemetry-runbook.md) for the +walkthrough and a need-to-macro lookup table. ## Contracts and instrumentation diff --git a/docs/telemetry-runbook.md b/docs/telemetry-runbook.md index 378083dddd..8e80edd981 100644 --- a/docs/telemetry-runbook.md +++ b/docs/telemetry-runbook.md @@ -1508,23 +1508,48 @@ Use the call-site macros in `src/xrpld/telemetry/MetricMacros.h` -- no | Last-value snapshot (not a distribution) | `XRPL_METRIC_GAUGE_RECORD` [+ `_LABELED`] -- requires an ABI v2 opentelemetry-cpp build; this repo currently builds ABI v1, so use the observable-gauge row below instead | | Value your own code already tracks, sampled on a timer | `XRPL_METRIC_OBSERVABLE_GAUGE_REGISTER` / `_COUNTER_REGISTER` / `_UPDOWN_REGISTER` | +First declare the name in `src/xrpld/telemetry/MetricNames.h` -- the emit site +must reference a constant, never a string literal, and CI Rule I enforces that +for any metric family that already has constants: + +```cpp +// in src/xrpld/telemetry/MetricNames.h, namespace metric: +inline constexpr char myNewThingTotal[] = "my_new_thing_total"; +inline constexpr char myInFlightRequests[] = "my_in_flight_requests"; +inline constexpr char myThingSize[] = "my_thing_size"; +``` + +Then emit against it: + ```cpp #include +#include // Monotonic counter: -XRPL_METRIC_COUNTER_INC(app_, "my_new_thing_total", "Description of what this counts"); +XRPL_METRIC_COUNTER_INC( + app_, metric::myNewThingTotal, "Description of what this counts"); // Value that can go up and down, e.g. in-flight work (no _total suffix -- that // is reserved for monotonic counters; an UpDownCounter is a current value): -XRPL_METRIC_UPDOWN_ADD(app_, "my_in_flight_requests", "Currently executing", 1); // on start -XRPL_METRIC_UPDOWN_ADD(app_, "my_in_flight_requests", "Currently executing", -1); // on finish +XRPL_METRIC_UPDOWN_ADD(app_, metric::myInFlightRequests, "Currently executing", 1); +XRPL_METRIC_UPDOWN_ADD(app_, metric::myInFlightRequests, "Currently executing", -1); + +// Labelled: the KEY is a constant too, and so is the VALUE when it comes from a +// fixed set. Only runtime data stays a plain expression. +XRPL_METRIC_COUNTER_INC_LABELED( + app_, + metric::myNewThingTotal, + "Description of what this counts", + {{label::outcome, std::string(lval::dns_resolve::resolved)}}); // Sampled from your own state, on the OTel export timer (register ONCE, in init code): -XRPL_METRIC_OBSERVABLE_GAUGE_REGISTER(app_, "my_thing_size", "Current size", +XRPL_METRIC_OBSERVABLE_GAUGE_REGISTER(app_, metric::myThingSize, "Current size", [this] { return static_cast(myThing_.size()); }); ``` -Counters use a `_total` suffix by convention. A histogram whose values can +Naming rules (counter `_total`, duration `_us`/`_ms`/`_seconds`, no `xrpld_` +prefix, bounded label cardinality) are listed in CONTRIBUTING.md -> +"Telemetry metric naming" and enforced by CI Rules I/J/K. A histogram whose values can exceed ~10,000 units (e.g. a microsecond duration beyond 10ms) still needs one line added to `addMicrosecondHistogramView()` in `MetricsRegistry.cpp` -- the only case that still touches a central file. There is no way to read a metric's diff --git a/src/tests/libxrpl/telemetry/MetricMacros.cpp b/src/tests/libxrpl/telemetry/MetricMacros.cpp index d29b784994..9b661186d1 100644 --- a/src/tests/libxrpl/telemetry/MetricMacros.cpp +++ b/src/tests/libxrpl/telemetry/MetricMacros.cpp @@ -27,6 +27,7 @@ #include #include +#include #include #include @@ -533,7 +534,7 @@ TEST(MetricMacros, counter_inc_labeled_does_not_crash) app, "test_macro_labeled_counter_total", "Test labeled counter for macro unit test", - {{"reason", std::string("unit_test")}}); + {{telemetry::label::reason, std::string("unit_test")}}); // Instrument was created exactly once at this single call site. EXPECT_EQ(app.registry().meterCalls(), 1); @@ -582,7 +583,7 @@ TEST(MetricMacros, updown_add_labeled_does_not_crash) "test_macro_updown_labeled_total", "Test labeled updown for macro unit test", -1, - {{"reason", std::string("unit_test")}}); + {{telemetry::label::reason, std::string("unit_test")}}); EXPECT_EQ(app.registry().meterCalls(), 1); } @@ -688,9 +689,9 @@ TEST(MetricMacros, dns_resolve_records_exact_counts_and_latency) { XRPL_METRIC_COUNTER_INC_LABELED( app, - "dns_resolve_total", + telemetry::metric::dnsResolveTotal, "Peer hostname resolutions, by outcome", - {{"outcome", std::string(resolved ? "resolved" : "empty")}}); + {{telemetry::label::outcome, std::string(resolved ? "resolved" : "empty")}}); } // Two latency samples with known values: 1.5 ms + 2.5 ms = 4.0 ms. @@ -698,7 +699,7 @@ TEST(MetricMacros, dns_resolve_records_exact_counts_and_latency) { XRPL_METRIC_HISTOGRAM_RECORD( app, - "dns_resolve_latency_ms", + telemetry::metric::dnsResolveLatencyMs, "Time taken to resolve a configured peer hostname, in milliseconds", ms); } @@ -740,9 +741,9 @@ TEST(MetricMacros, overlay_connect_records_exact_counts_per_outcome) auto const bump = [&app](char const* outcome) { XRPL_METRIC_COUNTER_INC_LABELED( app, - "overlay_connect_total", + telemetry::metric::overlayConnectTotal, "Outbound peer connection attempts, by terminal outcome", - {{"outcome", std::string(outcome)}}); + {{telemetry::label::outcome, std::string(outcome)}}); }; bump("connected"); bump("tcp_fail"); @@ -756,7 +757,7 @@ TEST(MetricMacros, overlay_connect_records_exact_counts_per_outcome) { XRPL_METRIC_HISTOGRAM_RECORD( app, - "overlay_dial_latency_ms", + telemetry::metric::overlayDialLatencyMs, "Time from starting an outbound peer dial to its terminal outcome, in milliseconds", ms); } @@ -792,9 +793,9 @@ TEST(MetricMacros, handshake_negotiation_fail_keeps_reasons_distinct) { XRPL_METRIC_COUNTER_INC_LABELED( app, - "handshake_negotiation_fail_total", + telemetry::metric::handshakeNegotiationFailTotal, "Peer handshake negotiations rejected, by reason", - {{"reason", std::string(reason)}}); + {{telemetry::label::reason, std::string(reason)}}); } auto const data = provider.collect(); @@ -832,9 +833,10 @@ TEST(MetricMacros, unl_fetch_total_keys_series_on_site_and_outcome_pair) auto const bump = [&app](char const* site, char const* outcome) { XRPL_METRIC_COUNTER_INC_LABELED( app, - "unl_fetch_total", + telemetry::metric::unlFetchTotal, "Validator list fetch attempts, by site and outcome", - {{"site", std::string(site)}, {"outcome", std::string(outcome)}}); + {{telemetry::label::site, std::string(site)}, + {telemetry::label::outcome, std::string(outcome)}}); }; constexpr char const* kSiteA = "https://a.example.com/vl.json"; @@ -901,7 +903,7 @@ TEST(MetricMacros, unl_quorum_gauge_observes_exact_trusted_keys_and_quorum) // deregisters the callback (ObservableInstrument's destructor calls // CleanupCallback), which is why the real registry holds it in a member. auto gauge = provider.meter()->CreateInt64ObservableGauge( - "unl_quorum", "Trusted UNL key count vs required quorum"); + telemetry::metric::unlQuorum, "Trusted UNL key count vs required quorum"); gauge->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto const* self = static_cast(state); @@ -909,7 +911,7 @@ TEST(MetricMacros, unl_quorum_gauge_observes_exact_trusted_keys_and_quorum) auto observe = [&](char const* name, std::int64_t value) { opentelemetry::nostd::get>>(result) - ->Observe(value, {{"metric", name}}); + ->Observe(value, {{telemetry::label::metric, name}}); }; observe("trusted_keys", self->trustedKeys); observe("quorum", self->quorum); @@ -941,14 +943,15 @@ TEST(MetricMacros, clock_skew_gauge_observes_exact_negative_offset) std::int64_t offsetSeconds = -3; auto gauge = provider.meter()->CreateInt64ObservableGauge( - "clock_close_offset_seconds", "Network close time offset from the local clock, in seconds"); + telemetry::metric::clockCloseOffsetSeconds, + "Network close time offset from the local clock, in seconds"); gauge->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto const* value = static_cast(state); auto observe = [&](char const* name, std::int64_t v) { opentelemetry::nostd::get>>(result) - ->Observe(v, {{"metric", name}}); + ->Observe(v, {{telemetry::label::metric, name}}); }; observe("offset", *value); }, @@ -979,35 +982,35 @@ TEST(MetricMacros, sync_diagnostics_metrics_emit_nothing_when_registry_disabled) XRPL_METRIC_COUNTER_INC_LABELED( app, - "dns_resolve_total", + telemetry::metric::dnsResolveTotal, "Peer hostname resolutions, by outcome", - {{"outcome", std::string("resolved")}}); + {{telemetry::label::outcome, std::string("resolved")}}); XRPL_METRIC_HISTOGRAM_RECORD( app, - "dns_resolve_latency_ms", + telemetry::metric::dnsResolveLatencyMs, "Time taken to resolve a configured peer hostname, in milliseconds", 1.5); XRPL_METRIC_COUNTER_INC_LABELED( app, - "overlay_connect_total", + telemetry::metric::overlayConnectTotal, "Outbound peer connection attempts, by terminal outcome", - {{"outcome", std::string("connected")}}); + {{telemetry::label::outcome, std::string("connected")}}); XRPL_METRIC_HISTOGRAM_RECORD( app, - "overlay_dial_latency_ms", + telemetry::metric::overlayDialLatencyMs, "Time from starting an outbound peer dial to its terminal outcome, in milliseconds", 10.0); XRPL_METRIC_COUNTER_INC_LABELED( app, - "handshake_negotiation_fail_total", + telemetry::metric::handshakeNegotiationFailTotal, "Peer handshake negotiations rejected, by reason", - {{"reason", std::string("wrong_network")}}); + {{telemetry::label::reason, std::string("wrong_network")}}); XRPL_METRIC_COUNTER_INC_LABELED( app, - "unl_fetch_total", + telemetry::metric::unlFetchTotal, "Validator list fetch attempts, by site and outcome", - {{"site", std::string("https://a.example.com/vl.json")}, - {"outcome", std::string("accepted")}}); + {{telemetry::label::site, std::string("https://a.example.com/vl.json")}, + {telemetry::label::outcome, std::string("accepted")}}); auto const data = provider.collect(); @@ -1061,9 +1064,9 @@ TEST(MetricMacros, state_changes_total_keys_series_on_from_to_pair) auto const transition = [&app](char const* from, char const* to) { XRPL_METRIC_COUNTER_INC_LABELED( app, - "state_changes_total", + telemetry::metric::stateChangesTotal, "Total operating mode changes", - {{"from", std::string(from)}, {"to", std::string(to)}}); + {{telemetry::label::from, std::string(from)}, {telemetry::label::to, std::string(to)}}); }; // A clean climb: disconnected -> connected -> syncing -> full, once each. @@ -1142,8 +1145,8 @@ TEST(MetricMacros, sync_state_gauge_observes_exact_stuck_node_values) // Keep the instrument alive for the whole test: destroying the handle // deregisters the callback, which is why the real registry holds a member. - auto gauge = - provider.meter()->CreateInt64ObservableGauge("sync_state", "Sync-pipeline health signals"); + auto gauge = provider.meter()->CreateInt64ObservableGauge( + telemetry::metric::syncState, "Sync-pipeline health signals"); gauge->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto const* self = static_cast(state); @@ -1151,7 +1154,7 @@ TEST(MetricMacros, sync_state_gauge_observes_exact_stuck_node_values) auto observe = [&](char const* name, std::int64_t value) { opentelemetry::nostd::get>>(result) - ->Observe(value, {{"metric", name}}); + ->Observe(value, {{telemetry::label::metric, name}}); }; observe("initial_full_duration_us", self->initialFullDurationUs); observe("network_ledger_gate", self->networkLedgerGate); @@ -1211,7 +1214,7 @@ TEST(MetricMacros, stall_events_counter_observes_exact_cumulative_count) std::int64_t stallEpisodes = 3; auto counter = provider.meter()->CreateInt64ObservableCounter( - "server_stall_events_total", "Total server main-loop stall episodes"); + telemetry::metric::serverStallEventsTotal, "Total server main-loop stall episodes"); counter->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto const* value = static_cast(state); @@ -1250,9 +1253,10 @@ TEST(MetricMacros, state_changes_total_emits_nothing_when_registry_disabled) XRPL_METRIC_COUNTER_INC_LABELED( app, - "state_changes_total", + telemetry::metric::stateChangesTotal, "Total operating mode changes", - {{"from", std::string("connected")}, {"to", std::string("full")}}); + {{telemetry::label::from, std::string("connected")}, + {telemetry::label::to, std::string("full")}}); auto const data = provider.collect(); @@ -1299,9 +1303,9 @@ TEST(MetricMacros, acquire_source_splits_local_and_network) auto const acquire = [&app](bool localComplete) { XRPL_METRIC_COUNTER_INC_LABELED( app, - "sync_acquire_source_total", + telemetry::metric::syncAcquireSourceTotal, "Ledger acquires by where the data came from", - {{"source", std::string(localComplete ? "local" : "network")}}); + {{telemetry::label::source, std::string(localComplete ? "local" : "network")}}); }; // One satisfied locally, two needing the network. acquire(true); @@ -1341,7 +1345,7 @@ TEST(MetricMacros, acquire_no_progress_counts_only_stalled_timeouts) { XRPL_METRIC_COUNTER_INC( app, - "sync_acquire_no_progress_total", + telemetry::metric::syncAcquireNoProgressTotal, "Ledger-acquire timeouts where no new node arrived"); } }; @@ -1390,10 +1394,10 @@ TEST(MetricMacros, addnode_outcomes_record_exact_batch_tallies) return; XRPL_METRIC_COUNTER_ADD_LABELED( app, - "sync_addnode_total", + telemetry::metric::syncAddnodeTotal, "SHAMap nodes received during ledger acquire, by outcome", static_cast(count), - {{"outcome", std::string(outcome)}}); + {{telemetry::label::outcome, std::string(outcome)}}); }; emit("good", good); emit("duplicate", duplicate); @@ -1460,7 +1464,8 @@ TEST(MetricMacros, sync_acquire_gauge_observes_exact_stuck_acquire_values) // Keep the instrument alive for the whole test: destroying the handle // deregisters the callback, which is why the real registry holds a member. auto gauge = provider.meter()->CreateInt64ObservableGauge( - "sync_acquire", "Aggregate ledger-acquire progress across in-flight acquires"); + telemetry::metric::syncAcquire, + "Aggregate ledger-acquire progress across in-flight acquires"); gauge->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto const* self = static_cast(state); @@ -1468,7 +1473,7 @@ TEST(MetricMacros, sync_acquire_gauge_observes_exact_stuck_acquire_values) auto observe = [&](char const* name, std::int64_t value) { opentelemetry::nostd::get>>(result) - ->Observe(value, {{"metric", name}}); + ->Observe(value, {{telemetry::label::metric, name}}); }; observe("missing_state_nodes_max", self->maxMissingStateNodes); observe("missing_tx_nodes_max", self->maxMissingTxNodes); @@ -1527,7 +1532,8 @@ TEST(MetricMacros, shamap_cache_hit_rate_gauge_normalizes_to_unit_fraction) float rawHitRatePercent = 90.0F; auto gauge = provider.meter()->CreateDoubleObservableGauge( - "shamap_cache_hit_rate", "SHAMap tree-node cache hit rate (0.0-1.0), by cache"); + telemetry::metric::shamapCacheHitRate, + "SHAMap tree-node cache hit rate (0.0-1.0), by cache"); gauge->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto const* raw = static_cast(state); @@ -1535,7 +1541,8 @@ TEST(MetricMacros, shamap_cache_hit_rate_gauge_normalizes_to_unit_fraction) opentelemetry::nostd::get< opentelemetry::nostd::shared_ptr>>( result) - ->Observe(static_cast(*raw / 100.0F), {{"metric", "treenode"}}); + ->Observe( + static_cast(*raw / 100.0F), {{telemetry::label::metric, "treenode"}}); }, &rawHitRatePercent); @@ -1580,17 +1587,19 @@ TEST(MetricMacros, acquire_counters_emit_nothing_when_registry_disabled) XRPL_METRIC_COUNTER_INC_LABELED( app, - "sync_acquire_source_total", + telemetry::metric::syncAcquireSourceTotal, "Ledger acquires by where the data came from", - {{"source", std::string("network")}}); + {{telemetry::label::source, std::string("network")}}); XRPL_METRIC_COUNTER_INC( - app, "sync_acquire_no_progress_total", "Ledger-acquire timeouts where no new node arrived"); + app, + telemetry::metric::syncAcquireNoProgressTotal, + "Ledger-acquire timeouts where no new node arrived"); XRPL_METRIC_COUNTER_ADD_LABELED( app, - "sync_addnode_total", + telemetry::metric::syncAddnodeTotal, "SHAMap nodes received during ledger acquire, by outcome", static_cast(5), - {{"outcome", std::string("good")}}); + {{telemetry::label::outcome, std::string("good")}}); auto const data = provider.collect(); @@ -1643,7 +1652,8 @@ TEST(MetricMacros, jobq_backlog_gauge_separates_waiting_running_and_deferred) // Keep the instrument alive for the whole test: destroying the handle // deregisters the callback, which is why the real registry holds a member. auto gauge = provider.meter()->CreateInt64ObservableGauge( - "jobq_backlog", "JobQueue occupancy per job type (waiting/running/deferred)"); + telemetry::metric::jobqBacklog, + "JobQueue occupancy per job type (waiting/running/deferred)"); gauge->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto const* counts = static_cast const*>(state); @@ -1651,7 +1661,9 @@ TEST(MetricMacros, jobq_backlog_gauge_separates_waiting_running_and_deferred) auto observe = [&](char const* field, std::string const& jobType, std::int64_t value) { opentelemetry::nostd::get>>(result) - ->Observe(value, {{"metric", field}, {"job_type", jobType}}); + ->Observe( + value, + {{telemetry::label::metric, field}, {telemetry::label::jobType, jobType}}); }; for (auto const& count : *counts) { @@ -1746,14 +1758,15 @@ TEST(MetricMacros, jobq_saturation_gauge_observes_exact_pool_exhaustion_values) JobQueue::WorkerSaturation observed{.runningTasks = 6, .workerThreads = 6, .totalWaiting = 12}; auto gauge = provider.meter()->CreateInt64ObservableGauge( - "jobq_saturation", "Worker-pool saturation: tasks in flight, worker threads, jobs queued"); + telemetry::metric::jobqSaturation, + "Worker-pool saturation: tasks in flight, worker threads, jobs queued"); gauge->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto const* self = static_cast(state); auto observe = [&](char const* field, std::int64_t value) { opentelemetry::nostd::get>>(result) - ->Observe(value, {{"metric", field}}); + ->Observe(value, {{telemetry::label::metric, field}}); }; observe("running_tasks", self->runningTasks); observe("worker_threads", self->workerThreads); @@ -1847,7 +1860,8 @@ TEST(MetricMacros, peer_ledger_supply_gauge_names_a_gap_no_peer_can_fill) // Keep the instrument alive for the whole test: destroying the handle // deregisters the callback, which is why the real registry holds a member. auto gauge = provider.meter()->CreateInt64ObservableGauge( - "peer_ledger_supply", "Peer coverage of the ledger sequence this node needs"); + telemetry::metric::peerLedgerSupply, + "Peer coverage of the ledger sequence this node needs"); gauge->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto const* self = static_cast(state); @@ -1855,7 +1869,7 @@ TEST(MetricMacros, peer_ledger_supply_gauge_names_a_gap_no_peer_can_fill) auto observe = [&](char const* field, std::int64_t value) { opentelemetry::nostd::get>>(result) - ->Observe(value, {{"metric", field}}); + ->Observe(value, {{telemetry::label::metric, field}}); }; observe("peers_reporting", self->peersReporting); observe("peers_serving_validated", self->peersServingValidated); @@ -1919,14 +1933,15 @@ TEST(MetricMacros, peer_ledger_supply_gauge_reads_zero_window_as_unknown) .supplyMaxSeq = 5000}; auto gauge = provider.meter()->CreateInt64ObservableGauge( - "peer_ledger_supply", "Peer coverage of the ledger sequence this node needs"); + telemetry::metric::peerLedgerSupply, + "Peer coverage of the ledger sequence this node needs"); gauge->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto const* self = static_cast(state); auto observe = [&](char const* field, std::int64_t value) { opentelemetry::nostd::get>>(result) - ->Observe(value, {{"metric", field}}); + ->Observe(value, {{telemetry::label::metric, field}}); }; observe("peers_reporting", self->peersReporting); observe("peers_serving_validated", self->peersServingValidated); @@ -1996,7 +2011,8 @@ TEST(MetricMacros, slot_census_gauge_names_each_bootstrap_fault_exactly) .livecache = 12}; auto gauge = provider.meter()->CreateInt64ObservableGauge( - "peerfinder_slot_census", "PeerFinder slots, connection attempts and address caches"); + telemetry::metric::peerfinderSlotCensus, + "PeerFinder slots, connection attempts and address caches"); gauge->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto const* self = static_cast(state); @@ -2004,7 +2020,7 @@ TEST(MetricMacros, slot_census_gauge_names_each_bootstrap_fault_exactly) auto observe = [&](char const* field, std::int64_t value) { opentelemetry::nostd::get>>(result) - ->Observe(value, {{"metric", field}}); + ->Observe(value, {{telemetry::label::metric, field}}); }; observe("out_active", self->outActive); observe("out_max", self->outMax); @@ -2079,14 +2095,15 @@ TEST(MetricMacros, slot_census_gauge_reports_every_field_even_when_idle) .livecache = 0}; auto gauge = provider.meter()->CreateInt64ObservableGauge( - "peerfinder_slot_census", "PeerFinder slots, connection attempts and address caches"); + telemetry::metric::peerfinderSlotCensus, + "PeerFinder slots, connection attempts and address caches"); gauge->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto const* self = static_cast(state); auto observe = [&](char const* field, std::int64_t value) { opentelemetry::nostd::get>>(result) - ->Observe(value, {{"metric", field}}); + ->Observe(value, {{telemetry::label::metric, field}}); }; observe("out_active", self->outActive); observe("out_max", self->outMax); @@ -2170,14 +2187,15 @@ TEST(MetricMacros, amendment_block_gauge_observes_exact_countdown_and_sentinel) .warned = false, .expectedEpochSeconds = {}, .nowEpochSeconds = kNowEpochSeconds}; auto gauge = provider.meter()->CreateInt64ObservableGauge( - "amendment_block", "Amendment-block warning and seconds until the node stops validating"); + telemetry::metric::amendmentBlock, + "Amendment-block warning and seconds until the node stops validating"); gauge->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto const* self = static_cast(state); auto observe = [&](char const* field, std::int64_t value) { opentelemetry::nostd::get>>(result) - ->Observe(value, {{"metric", field}}); + ->Observe(value, {{telemetry::label::metric, field}}); }; observe("warned", self->warned ? 1 : 0); @@ -2246,14 +2264,15 @@ TEST(MetricMacros, amendment_block_gauge_clamps_past_due_and_carries_no_amendmen .nowEpochSeconds = kNowEpochSeconds}; auto gauge = provider.meter()->CreateInt64ObservableGauge( - "amendment_block", "Amendment-block warning and seconds until the node stops validating"); + telemetry::metric::amendmentBlock, + "Amendment-block warning and seconds until the node stops validating"); gauge->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto const* self = static_cast(state); auto observe = [&](char const* field, std::int64_t value) { opentelemetry::nostd::get>>(result) - ->Observe(value, {{"metric", field}}); + ->Observe(value, {{telemetry::label::metric, field}}); }; observe("warned", self->warned ? 1 : 0); std::int64_t secondsToBlock = -1; @@ -2315,9 +2334,10 @@ TEST(MetricMacros, peer_disconnect_total_keys_series_on_reason_and_direction_pai auto const bump = [&app](char const* reason, char const* direction) { XRPL_METRIC_COUNTER_INC_LABELED( app, - "peer_disconnect_total", + telemetry::metric::peerDisconnectTotal, "Peer disconnects, by cause and connection direction", - {{"reason", std::string(reason)}, {"direction", std::string(direction)}}); + {{telemetry::label::reason, std::string(reason)}, + {telemetry::label::direction, std::string(direction)}}); }; // Two OUR-FAULT reasons: this node could not keep up with what it owed the @@ -2394,9 +2414,10 @@ TEST(MetricMacros, peer_disconnect_total_does_not_merge_directions_for_one_reaso auto const bump = [&app](char const* reason, char const* direction) { XRPL_METRIC_COUNTER_INC_LABELED( app, - "peer_disconnect_total", + telemetry::metric::peerDisconnectTotal, "Peer disconnects, by cause and connection direction", - {{"reason", std::string(reason)}, {"direction", std::string(direction)}}); + {{telemetry::label::reason, std::string(reason)}, + {telemetry::label::direction, std::string(direction)}}); }; // One read_error each way. If direction did not key the series, this would @@ -2463,9 +2484,9 @@ TEST(MetricMacros, peer_accept_total_keys_series_on_outcome) auto const bump = [&app](char const* outcome) { XRPL_METRIC_COUNTER_INC_LABELED( app, - "peer_accept_total", + telemetry::metric::peerAcceptTotal, "Inbound peer connection attempts, by terminal outcome", - {{"outcome", std::string(outcome)}}); + {{telemetry::label::outcome, std::string(outcome)}}); }; // accepted x2, no_slot x3, handshake_error x1 -- three distinct @@ -2520,9 +2541,10 @@ TEST(MetricMacros, serve_refused_total_keys_series_on_request_and_reason_pair) auto const bump = [&app](char const* request, char const* reason) { XRPL_METRIC_COUNTER_INC_LABELED( app, - "serve_refused_total", + telemetry::metric::serveRefusedTotal, "Peer data requests this node declined to serve, by request kind and cause", - {{"request", std::string(request)}, {"reason", std::string(reason)}}); + {{telemetry::label::request, std::string(request)}, + {telemetry::label::reason, std::string(reason)}}); }; // Same request kind, two different reasons -> two series. @@ -2605,7 +2627,7 @@ TEST(MetricMacros, ledger_jump_total_accumulates_on_one_unlabelled_series) { XRPL_METRIC_COUNTER_INC( app, - "ledger_jump_total", + telemetry::metric::ledgerJumpTotal, "Forced jumps of the last closed ledger to a divergent chain"); } @@ -2631,7 +2653,9 @@ TEST(MetricMacros, ledger_jump_total_accumulates_on_one_unlabelled_series) // A fourth jump advances the SAME series to exactly 4 rather than creating a // second one, which is what "no labels" has to mean over time. XRPL_METRIC_COUNTER_INC( - app, "ledger_jump_total", "Forced jumps of the last closed ledger to a divergent chain"); + app, + telemetry::metric::ledgerJumpTotal, + "Forced jumps of the last closed ledger to a divergent chain"); auto const fourth = provider.collect(); ASSERT_EQ(fourth.at("ledger_jump_total").size(), 1u); EXPECT_EQ(counterValue(fourth, "ledger_jump_total", otel_sdk::PointAttributes{}), 4); @@ -2650,21 +2674,25 @@ TEST(MetricMacros, sync_supply_counters_emit_nothing_when_registry_disabled) XRPL_METRIC_COUNTER_INC_LABELED( app, - "peer_disconnect_total", + telemetry::metric::peerDisconnectTotal, "Peer disconnects, by cause and connection direction", - {{"reason", std::string("large_sendq")}, {"direction", std::string("outbound")}}); + {{telemetry::label::reason, std::string("large_sendq")}, + {telemetry::label::direction, std::string("outbound")}}); XRPL_METRIC_COUNTER_INC_LABELED( app, - "peer_accept_total", + telemetry::metric::peerAcceptTotal, "Inbound peer connection attempts, by terminal outcome", - {{"outcome", std::string("accepted")}}); + {{telemetry::label::outcome, std::string("accepted")}}); XRPL_METRIC_COUNTER_INC_LABELED( app, - "serve_refused_total", + telemetry::metric::serveRefusedTotal, "Peer data requests this node declined to serve, by request kind and cause", - {{"request", std::string("ledger")}, {"reason", std::string("sendq_full")}}); + {{telemetry::label::request, std::string("ledger")}, + {telemetry::label::reason, std::string("sendq_full")}}); XRPL_METRIC_COUNTER_INC( - app, "ledger_jump_total", "Forced jumps of the last closed ledger to a divergent chain"); + app, + telemetry::metric::ledgerJumpTotal, + "Forced jumps of the last closed ledger to a divergent chain"); auto const data = provider.collect(); @@ -2699,15 +2727,15 @@ TEST(MetricMacros, ledger_replay_fallback_counter_separates_stages_by_exact_coun { XRPL_METRIC_COUNTER_INC_LABELED( app, - "ledger_replay_fallback_total", + telemetry::metric::ledgerReplayFallbackTotal, "Replay sub-acquires that fell back to a full ledger acquire", - {{"stage", std::string("skiplist")}}); + {{telemetry::label::stage, std::string("skiplist")}}); } XRPL_METRIC_COUNTER_INC_LABELED( app, - "ledger_replay_fallback_total", + telemetry::metric::ledgerReplayFallbackTotal, "Replay sub-acquires that fell back to a full ledger acquire", - {{"stage", std::string("delta")}}); + {{telemetry::label::stage, std::string("delta")}}); auto const data = provider.collect(); @@ -2743,9 +2771,9 @@ TEST(MetricMacros, ledger_replay_outcome_counter_records_each_terminal_state_exa { XRPL_METRIC_COUNTER_INC_LABELED( app, - "ledger_replay_outcome_total", + telemetry::metric::ledgerReplayOutcomeTotal, "Ledger replay tasks by terminal outcome", - {{"outcome", std::string(outcome)}}); + {{telemetry::label::outcome, std::string(outcome)}}); } }; record("success", 3); @@ -2782,14 +2810,14 @@ TEST(MetricMacros, ledger_replay_counters_emit_nothing_when_disabled) XRPL_METRIC_COUNTER_INC_LABELED( app, - "ledger_replay_fallback_total", + telemetry::metric::ledgerReplayFallbackTotal, "Replay sub-acquires that fell back to a full ledger acquire", - {{"stage", std::string("skiplist")}}); + {{telemetry::label::stage, std::string("skiplist")}}); XRPL_METRIC_COUNTER_INC_LABELED( app, - "ledger_replay_outcome_total", + telemetry::metric::ledgerReplayOutcomeTotal, "Ledger replay tasks by terminal outcome", - {{"outcome", std::string("timeout")}}); + {{telemetry::label::outcome, std::string("timeout")}}); auto const data = provider.collect(); @@ -2829,14 +2857,15 @@ TEST(MetricMacros, nodestore_latency_gauge_observes_exact_derived_means) .fetchDurationUs = 1'000'000}; // 1 s over 1000 fetches -> 1000 us auto gauge = provider.meter()->CreateInt64ObservableGauge( - "nodestore_latency", "NodeStore mean store/fetch latency in microseconds, with counts"); + telemetry::metric::nodestoreLatency, + "NodeStore mean store/fetch latency in microseconds, with counts"); gauge->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto const* self = static_cast(state); auto observe = [&](char const* field, std::int64_t value) { opentelemetry::nostd::get>>(result) - ->Observe(value, {{"metric", field}}); + ->Observe(value, {{telemetry::label::metric, field}}); }; observe("write_count", static_cast(self->storeCount)); observe("read_count", static_cast(self->fetchCount)); @@ -2942,7 +2971,7 @@ TEST(MetricMacros, consensus_round_duration_records_exact_values) { XRPL_METRIC_HISTOGRAM_RECORD( app, - "consensus_round_duration_ms", + telemetry::metric::consensusRoundDurationMs, "Wall-clock duration of a completed consensus round in milliseconds", ms); } @@ -2976,7 +3005,7 @@ TEST(MetricMacros, consensus_round_duration_emits_nothing_when_registry_disabled XRPL_METRIC_HISTOGRAM_RECORD( app, - "consensus_round_duration_ms", + telemetry::metric::consensusRoundDurationMs, "Wall-clock duration of a completed consensus round in milliseconds", 3100); diff --git a/src/xrpld/app/consensus/RCLConsensus.cpp b/src/xrpld/app/consensus/RCLConsensus.cpp index bab81168fd..aaf7e17c83 100644 --- a/src/xrpld/app/consensus/RCLConsensus.cpp +++ b/src/xrpld/app/consensus/RCLConsensus.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include @@ -533,7 +534,7 @@ RCLConsensus::Adaptor::makeAcceptSpan(Result const& result) // transaction. XRPL_METRIC_HISTOGRAM_RECORD( app_, - "consensus_round_duration_ms", + telemetry::metric::consensusRoundDurationMs, "Wall-clock duration of a completed consensus round in milliseconds", result.roundTime.read().count()); span->setAttribute(cs::attr::quorum, static_cast(app_.getValidators().quorum())); diff --git a/src/xrpld/app/ledger/detail/InboundLedger.cpp b/src/xrpld/app/ledger/detail/InboundLedger.cpp index 5da372f319..dd50e14ff6 100644 --- a/src/xrpld/app/ledger/detail/InboundLedger.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedger.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -160,9 +161,12 @@ InboundLedger::init(ScopedLockType& collectionLock) // ("nothing was local, every node must come over the wire"). XRPL_METRIC_COUNTER_INC_LABELED( app_, - "sync_acquire_source_total", + telemetry::metric::syncAcquireSourceTotal, "Ledger acquires by where the data came from", - {{"source", std::string(complete_ ? "local" : "network")}}); + {{telemetry::label::source, + std::string( + complete_ ? telemetry::lval::acquire_source::local + : telemetry::lval::acquire_source::network)}}); if (!complete_) { @@ -500,7 +504,7 @@ InboundLedger::onTimer(bool wasProgress, ScopedLockType&) // signature of a sync that will never complete. XRPL_METRIC_COUNTER_INC( app_, - "sync_acquire_no_progress_total", + telemetry::metric::syncAcquireNoProgressTotal, "Ledger-acquire timeouts where no new node arrived"); // addPeers triggers if the reason is not HISTORY @@ -1517,14 +1521,14 @@ InboundLedger::recordBatchOutcome(SHAMapAddNode const& san) return; XRPL_METRIC_COUNTER_ADD_LABELED( app_, - "sync_addnode_total", + telemetry::metric::syncAddnodeTotal, "SHAMap nodes received during ledger acquire, by outcome", static_cast(count), - {{"outcome", std::string(outcome)}}); + {{telemetry::label::outcome, std::string(outcome)}}); }; - emit("good", san.getGood()); - emit("duplicate", san.getDuplicate()); - emit("invalid", san.getBad()); + emit(telemetry::lval::addnode::good, san.getGood()); + emit(telemetry::lval::addnode::duplicate, san.getDuplicate()); + emit(telemetry::lval::addnode::invalid, san.getBad()); return san.getGood(); } diff --git a/src/xrpld/app/ledger/detail/LedgerDeltaAcquire.cpp b/src/xrpld/app/ledger/detail/LedgerDeltaAcquire.cpp index 9dc8cf5339..4cb449a08f 100644 --- a/src/xrpld/app/ledger/detail/LedgerDeltaAcquire.cpp +++ b/src/xrpld/app/ledger/detail/LedgerDeltaAcquire.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -115,9 +116,10 @@ LedgerDeltaAcquire::trigger(std::size_t limit, ScopedLockType& sl) // Emitted once, on the transition into fallback. XRPL_METRIC_COUNTER_INC_LABELED( app_, - "ledger_replay_fallback_total", + telemetry::metric::ledgerReplayFallbackTotal, "Replay sub-acquires that fell back to a full ledger acquire", - {{"stage", std::string("delta")}}); + {{telemetry::label::stage, + std::string(telemetry::lval::replay_fallback::delta)}}); fallBack_ = true; } diff --git a/src/xrpld/app/ledger/detail/LedgerMaster.cpp b/src/xrpld/app/ledger/detail/LedgerMaster.cpp index e6432c10af..b238bbbffc 100644 --- a/src/xrpld/app/ledger/detail/LedgerMaster.cpp +++ b/src/xrpld/app/ledger/detail/LedgerMaster.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -1061,9 +1062,9 @@ LedgerMaster::checkAccept(std::shared_ptr const& ledger) // OTel callback ever acquires mutex_. XRPL_METRIC_COUNTER_INC_LABELED( app_, - "ledger_quorum_shortfall_total", + telemetry::metric::ledgerQuorumShortfallTotal, "Pre-accept gate rejections because trusted validations were below quorum", - {{"stage", std::string("pre_accept")}}); + {{telemetry::label::stage, std::string(telemetry::lval::quorum_shortfall::preAccept)}}); return; } diff --git a/src/xrpld/app/ledger/detail/LedgerReplayTask.cpp b/src/xrpld/app/ledger/detail/LedgerReplayTask.cpp index 59d713bbec..98f08ca1d5 100644 --- a/src/xrpld/app/ledger/detail/LedgerReplayTask.cpp +++ b/src/xrpld/app/ledger/detail/LedgerReplayTask.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -213,7 +214,7 @@ LedgerReplayTask::tryAdvance(ScopedLockType& sl) // Terminal success. Emitted once per task: the loop above is guarded by // isDone() at every entry point, so a completed task cannot re-enter // and double-count. - recordOutcome("success"); + recordOutcome(telemetry::lval::replay_outcome::success); } catch (std::runtime_error const&) { @@ -222,7 +223,7 @@ LedgerReplayTask::tryAdvance(ScopedLockType& sl) // A delta failed to build on top of its parent, so the replayed range // cannot be reconstructed. Previously not logged at all here, only // reflected in failed_. - recordOutcome("build_failed"); + recordOutcome(telemetry::lval::replay_outcome::buildFailed); } } @@ -235,9 +236,9 @@ LedgerReplayTask::recordOutcome(char const* outcome) const // indistinguishable from one that was never attempted. XRPL_METRIC_COUNTER_INC_LABELED( app_, - "ledger_replay_outcome_total", + telemetry::metric::ledgerReplayOutcomeTotal, "Ledger replay tasks by terminal outcome", - {{"outcome", std::string(outcome)}}); + {{telemetry::label::outcome, std::string(outcome)}}); } void @@ -259,7 +260,7 @@ LedgerReplayTask::updateSkipList( // so the task is abandoned before any delta is fetched. A distinct // outcome from a timeout: this one indicates a peer served an // inconsistent skip list, not a slow or absent peer. - recordOutcome("parameter_failed"); + recordOutcome(telemetry::lval::replay_outcome::parameterFailed); return; } } @@ -282,7 +283,7 @@ LedgerReplayTask::onTimer(bool progress, ScopedLockType& sl) // The task ran out of retries waiting for its deltas. This is the // outcome that pairs with the fallback counters: the sub-acquires gave // up, and so did the task above them. - recordOutcome("timeout"); + recordOutcome(telemetry::lval::timeout); } else { diff --git a/src/xrpld/app/ledger/detail/SkipListAcquire.cpp b/src/xrpld/app/ledger/detail/SkipListAcquire.cpp index cca54d5fcb..c69ae82d76 100644 --- a/src/xrpld/app/ledger/detail/SkipListAcquire.cpp +++ b/src/xrpld/app/ledger/detail/SkipListAcquire.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -111,9 +112,10 @@ SkipListAcquire::trigger(std::size_t limit, ScopedLockType& sl) // call below, which re-runs on every later trigger. XRPL_METRIC_COUNTER_INC_LABELED( app_, - "ledger_replay_fallback_total", + telemetry::metric::ledgerReplayFallbackTotal, "Replay sub-acquires that fell back to a full ledger acquire", - {{"stage", std::string("skiplist")}}); + {{telemetry::label::stage, + std::string(telemetry::lval::replay_fallback::skiplist)}}); fallBack_ = true; } diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index 15a1da17d9..796d6816d9 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -34,6 +34,7 @@ #include #include #include +#include #include #include #include @@ -2172,7 +2173,7 @@ NetworkOPsImp::switchLastClosedLedger(std::shared_ptr const& newLC // unbounded as label values, and the log line above already carries them. XRPL_METRIC_COUNTER_INC( registry_.get(), - "ledger_jump_total", + telemetry::metric::ledgerJumpTotal, "Forced jumps of the last closed ledger to a divergent chain"); clearNeedNetworkLedger(); @@ -2739,9 +2740,10 @@ NetworkOPsImp::setMode(OperatingMode om) // names, which keeps them identical to the ones server_info reports. XRPL_METRIC_COUNTER_INC_LABELED( registry_.get(), - "state_changes_total", + telemetry::metric::stateChangesTotal, "Total operating mode changes", - {{"from", strOperatingMode(prevMode, false)}, {"to", strOperatingMode(om, false)}}); + {{telemetry::label::from, strOperatingMode(prevMode, false)}, + {telemetry::label::to, strOperatingMode(om, false)}}); JLOG(journal_.info()) << "STATE->" << strOperatingMode(); pubServer(); diff --git a/src/xrpld/app/misc/detail/ValidatorSite.cpp b/src/xrpld/app/misc/detail/ValidatorSite.cpp index c40b00042c..f7667e7f90 100644 --- a/src/xrpld/app/misc/detail/ValidatorSite.cpp +++ b/src/xrpld/app/misc/detail/ValidatorSite.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -394,10 +395,10 @@ ValidatorSite::reportFetchOutcome( // the time series stable when a site redirects. XRPL_METRIC_COUNTER_INC_LABELED( app_, - "unl_fetch_total", + telemetry::metric::unlFetchTotal, "Validator list fetch attempts, by site and outcome", - {{"site", std::string(sites_[siteIdx].loadedResource->uri)}, - {"outcome", std::string(outcome)}}); + {{telemetry::label::site, std::string(sites_[siteIdx].loadedResource->uri)}, + {telemetry::label::outcome, std::string(outcome)}}); } void @@ -593,7 +594,7 @@ ValidatorSite::onSiteFetch( { JLOG(j_.warn()) << "Problem retrieving from " << sites_[siteIdx].activeResource->uri << " " << endpoint << " " << ec.value() << ":" << ec.message(); - onError("fetch error", true, "fetch_error"); + onError("fetch error", true, telemetry::lval::unl_fetch::fetchError); } else { @@ -629,7 +630,7 @@ ValidatorSite::onSiteFetch( JLOG(j_.warn()) << "Request for validator list at " << sites_[siteIdx].activeResource->uri << " " << endpoint << " returned bad status: " << res.result_int(); - onError("bad result code", true, "bad_status"); + onError("bad result code", true, telemetry::lval::unl_fetch::badStatus); } } } @@ -658,14 +659,14 @@ ValidatorSite::onTextFetch( std::scoped_lock const lockSites{sitesMutex_}; { // Both failures share one catch, so the label is set where detected. - std::string_view outcome = "parse_error"; + std::string_view outcome = telemetry::lval::unl_fetch::parseError; try { if (ec) { JLOG(j_.warn()) << "Problem retrieving from " << sites_[siteIdx].activeResource->uri << " " << ec.value() << ": " << ec.message(); - outcome = "fetch_error"; + outcome = telemetry::lval::unl_fetch::fetchError; throw std::runtime_error{"fetch error"}; } diff --git a/src/xrpld/overlay/detail/ConnectAttempt.cpp b/src/xrpld/overlay/detail/ConnectAttempt.cpp index 08084c7148..6b3877b079 100644 --- a/src/xrpld/overlay/detail/ConnectAttempt.cpp +++ b/src/xrpld/overlay/detail/ConnectAttempt.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -166,7 +167,7 @@ ConnectAttempt::reportOutcome(std::string_view outcome) // comma in a non-variadic macro argument, which the preprocessor splits. XRPL_METRIC_HISTOGRAM_RECORD( app_, - "overlay_dial_latency_ms", + telemetry::metric::overlayDialLatencyMs, "Time from starting an outbound peer dial to its terminal outcome, in milliseconds", std::chrono::duration_cast( std::chrono::steady_clock::now() - dialStart_) @@ -175,9 +176,9 @@ ConnectAttempt::reportOutcome(std::string_view outcome) XRPL_METRIC_COUNTER_INC_LABELED( app_, - "overlay_connect_total", + telemetry::metric::overlayConnectTotal, "Outbound peer connection attempts, by terminal outcome", - {{"outcome", std::string(outcome)}}); + {{telemetry::label::outcome, std::string(outcome)}}); // End the span with the SAME outcome value the counter just recorded, from // the same funnel, so the two can never disagree. The first-call-wins guard diff --git a/src/xrpld/overlay/detail/Handshake.cpp b/src/xrpld/overlay/detail/Handshake.cpp index fcd359d158..74e4bb0b38 100644 --- a/src/xrpld/overlay/detail/Handshake.cpp +++ b/src/xrpld/overlay/detail/Handshake.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -263,9 +264,9 @@ throwNegotiationFailure(Application& app, char const* reason, std::string const& { XRPL_METRIC_COUNTER_INC_LABELED( app, - "handshake_negotiation_fail_total", + telemetry::metric::handshakeNegotiationFailTotal, "Peer handshake negotiations rejected, by reason", - {{"reason", std::string(reason)}}); + {{telemetry::label::reason, std::string(reason)}}); throw std::runtime_error(message); } @@ -284,7 +285,10 @@ verifyHandshake( if (auto const iter = headers.find("Server-Domain"); iter != headers.end()) { if (!isProperlyFormedTomlDomain(iter->value())) - throwNegotiationFailure(app, "invalid_server_domain", "Invalid server domain"); + { + throwNegotiationFailure( + app, telemetry::lval::handshake_fail::invalidServerDomain, "Invalid server domain"); + } } if (auto const iter = headers.find("Network-ID"); iter != headers.end()) @@ -292,10 +296,20 @@ verifyHandshake( std::uint32_t nid = 0; if (!beast::lexicalCastChecked(nid, iter->value())) - throwNegotiationFailure(app, "invalid_network_id", "Invalid peer network identifier"); + { + throwNegotiationFailure( + app, + telemetry::lval::handshake_fail::invalidNetworkId, + "Invalid peer network identifier"); + } if (networkID && nid != *networkID) - throwNegotiationFailure(app, "wrong_network", "Peer is on a different network"); + { + throwNegotiationFailure( + app, + telemetry::lval::handshake_fail::wrongNetwork, + "Peer is on a different network"); + } } if (auto const iter = headers.find("Network-Time"); iter != headers.end()) @@ -308,7 +322,10 @@ verifyHandshake( // It's not an error for the header field to not be present but if // it is present and it contains junk data, that is an error. - throwNegotiationFailure(app, "invalid_clock_timestamp", "Invalid peer clock timestamp"); + throwNegotiationFailure( + app, + telemetry::lval::handshake_fail::invalidClockTimestamp, + "Invalid peer clock timestamp"); }(); using namespace std::chrono; @@ -328,7 +345,10 @@ verifyHandshake( auto const offset = calculateOffset(netTime, ourTime); if (abs(offset) > tolerance) - throwNegotiationFailure(app, "clock_skew", "Peer clock is too far off"); + { + throwNegotiationFailure( + app, telemetry::lval::handshake_fail::clockSkew, "Peer clock is too far off"); + } } PublicKey const publicKey = [&headers, &app] { @@ -339,14 +359,19 @@ verifyHandshake( if (pk) { if (publicKeyType(*pk) != KeyType::Secp256k1) + { throwNegotiationFailure( - app, "unsupported_key_type", "Unsupported public key type"); + app, + telemetry::lval::handshake_fail::unsupportedKeyType, + "Unsupported public key type"); + } return *pk; } } - throwNegotiationFailure(app, "bad_public_key", "Bad node public key"); + throwNegotiationFailure( + app, telemetry::lval::handshake_fail::badPublicKey, "Bad node public key"); }(); // This check gets two birds with one stone: @@ -359,16 +384,29 @@ verifyHandshake( auto const iter = headers.find("Session-Signature"); if (iter == headers.end()) - throwNegotiationFailure(app, "no_session_signature", "No session signature specified"); + { + throwNegotiationFailure( + app, + telemetry::lval::handshake_fail::noSessionSignature, + "No session signature specified"); + } auto sig = base64Decode(iter->value()); if (!verifyDigest(publicKey, sharedValue, makeSlice(sig), false)) - throwNegotiationFailure(app, "session_verify_failed", "Failed to verify session"); + { + throwNegotiationFailure( + app, + telemetry::lval::handshake_fail::sessionVerifyFailed, + "Failed to verify session"); + } } if (publicKey == app.nodeIdentity().first) - throwNegotiationFailure(app, "self_connection", "Self connection"); + { + throwNegotiationFailure( + app, telemetry::lval::handshake_fail::selfConnection, "Self connection"); + } if (auto const iter = headers.find("Local-IP"); iter != headers.end()) { @@ -376,13 +414,16 @@ verifyHandshake( auto const localIp = boost::asio::ip::make_address(std::string_view(iter->value()), ec); if (ec) - throwNegotiationFailure(app, "invalid_local_ip", "Invalid Local-IP"); + { + throwNegotiationFailure( + app, telemetry::lval::handshake_fail::invalidLocalIp, "Invalid Local-IP"); + } if (beast::IP::isPublic(remote) && remote != localIp) { throwNegotiationFailure( app, - "local_ip_mismatch", + telemetry::lval::handshake_fail::localIpMismatch, "Incorrect Local-IP: " + remote.to_string() + " instead of " + localIp.to_string()); } } @@ -393,7 +434,10 @@ verifyHandshake( auto const remoteIp = boost::asio::ip::make_address(std::string_view(iter->value()), ec); if (ec) - throwNegotiationFailure(app, "invalid_remote_ip", "Invalid Remote-IP"); + { + throwNegotiationFailure( + app, telemetry::lval::handshake_fail::invalidRemoteIp, "Invalid Remote-IP"); + } if (beast::IP::isPublic(remote) && !beast::IP::isUnspecified(publicIp)) { @@ -403,7 +447,7 @@ verifyHandshake( { throwNegotiationFailure( app, - "remote_ip_mismatch", + telemetry::lval::handshake_fail::remoteIpMismatch, "Incorrect Remote-IP: " + publicIp.to_string() + " instead of " + remoteIp.to_string()); } diff --git a/src/xrpld/overlay/detail/OverlayImpl.cpp b/src/xrpld/overlay/detail/OverlayImpl.cpp index e33baddb0c..6a1a2760bd 100644 --- a/src/xrpld/overlay/detail/OverlayImpl.cpp +++ b/src/xrpld/overlay/detail/OverlayImpl.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -241,7 +242,7 @@ OverlayImpl::onHandoff( if (ec) { JLOG(journal.debug()) << remoteEndpoint << " failed: " << ec.message(); - reportAcceptOutcome("local_endpoint_fail"); + reportAcceptOutcome(telemetry::lval::peer_accept::localEndpointFail); return handoff; } @@ -249,7 +250,7 @@ OverlayImpl::onHandoff( resourceManager_.newInboundEndpoint(beast::IPAddressConversion::fromAsio(remoteEndpoint)); if (consumer.disconnect(journal)) { - reportAcceptOutcome("resource_limit"); + reportAcceptOutcome(telemetry::lval::peer_accept::resourceLimit); return handoff; } @@ -262,7 +263,7 @@ OverlayImpl::onHandoff( // connection refused either IP limit exceeded or self-connect handoff.moved = false; JLOG(journal.debug()) << "Peer " << remoteEndpoint << " refused, " << to_string(result); - reportAcceptOutcome("no_slot"); + reportAcceptOutcome(telemetry::lval::peer_accept::noSlot); return handoff; } @@ -277,7 +278,7 @@ OverlayImpl::onHandoff( handoff.moved = false; handoff.response = makeRedirectResponse(slot, request, remoteEndpoint.address()); handoff.keepAlive = beast::rfc2616::isKeepAlive(request); - reportAcceptOutcome("not_peer_request"); + reportAcceptOutcome(telemetry::lval::peer_accept::notPeerRequest); return handoff; } } @@ -290,7 +291,7 @@ OverlayImpl::onHandoff( handoff.response = makeErrorResponse( slot, request, remoteEndpoint.address(), "Unable to agree on a protocol version"); handoff.keepAlive = false; - reportAcceptOutcome("protocol_mismatch"); + reportAcceptOutcome(telemetry::lval::peer_accept::protocolMismatch); return handoff; } @@ -302,7 +303,7 @@ OverlayImpl::onHandoff( handoff.response = makeErrorResponse(slot, request, remoteEndpoint.address(), "Incorrect security cookie"); handoff.keepAlive = false; - reportAcceptOutcome("bad_cookie"); + reportAcceptOutcome(telemetry::lval::peer_accept::badCookie); return handoff; } @@ -332,7 +333,7 @@ OverlayImpl::onHandoff( handoff.moved = false; handoff.response = makeRedirectResponse(slot, request, remoteEndpoint.address()); handoff.keepAlive = false; - reportAcceptOutcome("slot_refused"); + reportAcceptOutcome(telemetry::lval::peer_accept::slotRefused); return handoff; } } @@ -365,7 +366,7 @@ OverlayImpl::onHandoff( // Only after run() is the peer genuinely accepted. Anything that threw // above is reported as a handshake error by the catch below instead. - reportAcceptOutcome("accepted"); + reportAcceptOutcome(telemetry::lval::peer_accept::accepted); return handoff; } catch (std::exception const& e) @@ -377,7 +378,7 @@ OverlayImpl::onHandoff( handoff.moved = false; handoff.response = makeErrorResponse(slot, request, remoteEndpoint.address(), e.what()); handoff.keepAlive = false; - reportAcceptOutcome("handshake_error"); + reportAcceptOutcome(telemetry::lval::peer_accept::handshakeError); return handoff; } } @@ -636,7 +637,7 @@ OverlayImpl::reportDnsResolve(std::chrono::steady_clock::time_point start, bool // comma in a non-variadic macro argument, which the preprocessor splits. XRPL_METRIC_HISTOGRAM_RECORD( app_, - "dns_resolve_latency_ms", + telemetry::metric::dnsResolveLatencyMs, "Time taken to resolve a configured peer hostname, in milliseconds", std::chrono::duration_cast( std::chrono::steady_clock::now() - start) @@ -645,9 +646,12 @@ OverlayImpl::reportDnsResolve(std::chrono::steady_clock::time_point start, bool XRPL_METRIC_COUNTER_INC_LABELED( app_, - "dns_resolve_total", + telemetry::metric::dnsResolveTotal, "Peer hostname resolutions, by outcome", - {{"outcome", std::string(resolved ? "resolved" : "empty")}}); + {{telemetry::label::outcome, + std::string( + resolved ? telemetry::lval::dns_resolve::resolved + : telemetry::lval::dns_resolve::empty)}}); } void @@ -655,9 +659,9 @@ OverlayImpl::reportAcceptOutcome(char const* outcome) { XRPL_METRIC_COUNTER_INC_LABELED( app_, - "peer_accept_total", + telemetry::metric::peerAcceptTotal, "Inbound peer connection attempts, by terminal outcome", - {{"outcome", std::string(outcome)}}); + {{telemetry::label::outcome, std::string(outcome)}}); } PeerLedgerSupply diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 3f7320370e..98132100be 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -232,7 +233,7 @@ PeerImp::run() if (!closed) { - self->setDisconnectReason("malformed_handshake"); + self->setDisconnectReason(telemetry::lval::disconnect::malformedHandshake); self->fail("Malformed handshake data (1)"); } } @@ -243,14 +244,14 @@ PeerImp::run() if (!previous) { - self->setDisconnectReason("malformed_handshake"); + self->setDisconnectReason(telemetry::lval::disconnect::malformedHandshake); self->fail("Malformed handshake data (2)"); } } if (previous && !closed) { - self->setDisconnectReason("malformed_handshake"); + self->setDisconnectReason(telemetry::lval::disconnect::malformedHandshake); self->fail("Malformed handshake data (3)"); } @@ -285,7 +286,7 @@ PeerImp::stop() // Overlay-wide shutdown, not a fault with this peer. Distinguished so // a clean restart does not look like a wave of peer failures. - self->setDisconnectReason("stopping"); + self->setDisconnectReason(telemetry::lval::disconnect::stopping); self->close(); }); } @@ -415,7 +416,7 @@ PeerImp::charge(Resource::Charge const& fee, std::string const& context) // Set inside the latch, so only the one worker that wins the // exchange writes it. This is the node's own backpressure, not // a peer or network fault. - self->setDisconnectReason("charge_resources"); + self->setDisconnectReason(telemetry::lval::disconnect::chargeResources); self->fail("charge: Resources"); } } @@ -651,10 +652,11 @@ PeerImp::close() // "ping_timeout", "read_error"), and the two need opposite responses. XRPL_METRIC_COUNTER_INC_LABELED( app_, - "peer_disconnect_total", + telemetry::metric::peerDisconnectTotal, "Peer disconnects, by cause and connection direction", - {{"reason", std::string(disconnectReason_)}, - {"direction", std::string(inbound_ ? "inbound" : "outbound")}}); + {{telemetry::label::reason, std::string(disconnectReason_)}, + {telemetry::label::direction, + std::string(inbound_ ? telemetry::lval::inbound : telemetry::lval::outbound)}}); JLOG((inbound_ ? journal_.debug() : journal_.info())) << "close: Closed"; } @@ -664,9 +666,10 @@ PeerImp::reportServeRefusal(char const* request, char const* reason) { XRPL_METRIC_COUNTER_INC_LABELED( app_, - "serve_refused_total", + telemetry::metric::serveRefusedTotal, "Peer data requests this node declined to serve, by request kind and cause", - {{"request", std::string(request)}, {"reason", std::string(reason)}}); + {{telemetry::label::request, std::string(request)}, + {telemetry::label::reason, std::string(reason)}}); } void @@ -762,7 +765,7 @@ PeerImp::onTimer(error_code const& ec) // This should never happen JLOG(journal_.error()) << "onTimer: " << ec.message(); - setDisconnectReason("timer_error"); + setDisconnectReason(telemetry::lval::disconnect::timerError); close(); return; } @@ -771,7 +774,7 @@ PeerImp::onTimer(error_code const& ec) { // Our own send queue never drained: this node could not keep up with // what it owed the peer, so it is local backpressure, not a peer fault. - setDisconnectReason("large_sendq"); + setDisconnectReason(telemetry::lval::disconnect::largeSendq); fail("Large send queue"); return; } @@ -791,7 +794,7 @@ PeerImp::onTimer(error_code const& ec) overlay_.peerFinder().onFailure(slot_); // The peer is on a different chain, or we cannot tell: a topology // signal, not a fault on either side. - setDisconnectReason("not_useful"); + setDisconnectReason(telemetry::lval::disconnect::notUseful); fail("Not useful"); return; } @@ -800,7 +803,7 @@ PeerImp::onTimer(error_code const& ec) // Already waiting for PONG if (lastPingSeq_) { - setDisconnectReason("ping_timeout"); + setDisconnectReason(telemetry::lval::disconnect::pingTimeout); fail("Ping Timeout"); return; } @@ -842,7 +845,7 @@ PeerImp::onShutdown(error_code ec) // The TLS shutdown handshake finished. First-wins means the reason set by // whoever asked for the graceful close is kept; "shutdown" only lands when // the teardown started here, i.e. a clean close with no earlier cause. - setDisconnectReason("shutdown"); + setDisconnectReason(telemetry::lval::disconnect::shutdown); close(); } @@ -858,7 +861,7 @@ PeerImp::doAccept() // the shared value successfully in OverlayImpl if (!sharedValue) { - setDisconnectReason("shared_value"); + setDisconnectReason(telemetry::lval::disconnect::sharedValue); fail("makeSharedValue: Unexpected failure"); return; } @@ -908,7 +911,7 @@ PeerImp::doAccept() if (ec == boost::asio::error::operation_aborted) return; - setDisconnectReason("write_error"); + setDisconnectReason(telemetry::lval::disconnect::writeError); fail("onWriteResponse", ec); return; } @@ -918,7 +921,7 @@ PeerImp::doAccept() doProtocolStart(); return; } - setDisconnectReason("write_error"); + setDisconnectReason(telemetry::lval::disconnect::writeError); fail("Failed to write header"); return; })); @@ -995,12 +998,12 @@ PeerImp::onReadMessage(error_code ec, std::size_t bytesTransferred) JLOG(journal_.info()) << "EOF"; // The peer closed its side cleanly. Counted apart from a read // error because it is normal peer churn, not a fault. - setDisconnectReason("graceful"); + setDisconnectReason(telemetry::lval::disconnect::graceful); gracefulClose(); return; } - setDisconnectReason("read_error"); + setDisconnectReason(telemetry::lval::disconnect::readError); fail("onReadMessage", ec); return; } @@ -1030,7 +1033,7 @@ PeerImp::onReadMessage(error_code ec, std::size_t bytesTransferred) if (ec) { - setDisconnectReason("read_error"); + setDisconnectReason(telemetry::lval::disconnect::readError); fail("onReadMessage", ec); return; } @@ -1067,7 +1070,7 @@ PeerImp::onWriteMessage(error_code ec, std::size_t bytesTransferred) if (ec == boost::asio::error::operation_aborted) return; - setDisconnectReason("write_error"); + setDisconnectReason(telemetry::lval::disconnect::writeError); fail("onWriteMessage", ec); return; } @@ -2601,7 +2604,8 @@ PeerImp::onMessage(std::shared_ptr const& m) if (sendQueue_.size() >= Tuning::kDropSendQueue) { JLOG(pJournal_.debug()) << "GetObject: Large send queue"; - reportServeRefusal("object", "sendq_full"); + reportServeRefusal( + telemetry::lval::serve_request::object, telemetry::lval::serve_refused::sendqFull); return; } @@ -2960,7 +2964,8 @@ PeerImp::doFetchPack(std::shared_ptr const& packet) // A fetch pack is how a syncing peer catches up in bulk, so refusing // one directly slows that peer's sync. Counted separately from the // ledger path because the shed threshold is a different one. - reportServeRefusal("fetchpack", "load_shed"); + reportServeRefusal( + telemetry::lval::serve_request::fetchpack, telemetry::lval::serve_refused::loadShed); return; } @@ -3537,7 +3542,7 @@ PeerImp::processLedgerRequest(std::shared_ptr const& m) { if (sharedMap = getTxSet(m); !sharedMap) { - reportServeRefusal("txset", "not_found"); + reportServeRefusal(telemetry::lval::serve_request::txset, telemetry::lval::notFound); return; } map = sharedMap.get(); @@ -3557,19 +3562,21 @@ PeerImp::processLedgerRequest(std::shared_ptr const& m) if (sendQueue_.size() >= Tuning::kDropSendQueue) { JLOG(pJournal_.debug()) << "processLedgerRequest: Large send queue"; - reportServeRefusal("ledger", "sendq_full"); + reportServeRefusal( + telemetry::lval::serve_request::ledger, telemetry::lval::serve_refused::sendqFull); return; } if (app_.getFeeTrack().isLoadedLocal() && !cluster()) { JLOG(pJournal_.debug()) << "processLedgerRequest: Too busy"; - reportServeRefusal("ledger", "load_shed"); + reportServeRefusal( + telemetry::lval::serve_request::ledger, telemetry::lval::serve_refused::loadShed); return; } if (ledger = getLedger(m); !ledger) { - reportServeRefusal("ledger", "not_found"); + reportServeRefusal(telemetry::lval::serve_request::ledger, telemetry::lval::notFound); return; } @@ -3602,7 +3609,9 @@ PeerImp::processLedgerRequest(std::shared_ptr const& m) default: // This case should not be possible here JLOG(pJournal_.error()) << "processLedgerRequest: Invalid ledger info type"; - reportServeRefusal("ledger", "bad_type"); + reportServeRefusal( + telemetry::lval::serve_request::ledger, + telemetry::lval::serve_refused::badType); return; } } @@ -3610,7 +3619,8 @@ PeerImp::processLedgerRequest(std::shared_ptr const& m) if (map == nullptr) { JLOG(pJournal_.warn()) << "processLedgerRequest: Unable to find map"; - reportServeRefusal("ledger", "no_map"); + reportServeRefusal( + telemetry::lval::serve_request::ledger, telemetry::lval::serve_refused::noMap); return; } @@ -3699,7 +3709,10 @@ PeerImp::processLedgerRequest(std::shared_ptr const& m) // The map was found but produced no nodes to return, so the requester // gets nothing back and will have to ask someone else. Emitted here, // after the node loop, rather than inside it -- one call per request. - reportServeRefusal(itype == protocol::liTS_CANDIDATE ? "txset" : "ledger", "empty_reply"); + reportServeRefusal( + itype == protocol::liTS_CANDIDATE ? telemetry::lval::serve_request::txset + : telemetry::lval::serve_request::ledger, + telemetry::lval::serve_refused::emptyReply); return; } diff --git a/src/xrpld/overlay/detail/PeerImp.h b/src/xrpld/overlay/detail/PeerImp.h index c486e38954..67db379055 100644 --- a/src/xrpld/overlay/detail/PeerImp.h +++ b/src/xrpld/overlay/detail/PeerImp.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -210,7 +211,7 @@ private: * never peer-supplied data -- so the label's cardinality is bounded * by the code. */ - char const* disconnectReason_{"unknown"}; + char const* disconnectReason_{telemetry::lval::disconnect::unknown}; std::shared_ptr const slot_; boost::beast::multi_buffer readBuffer_; @@ -510,7 +511,7 @@ private: { // Only the first cause is kept: fail() sites frequently run before // close(), and a later generic reason must not mask the real one. - if (disconnectReason_ == std::string_view{"unknown"}) + if (disconnectReason_ == std::string_view{telemetry::lval::disconnect::unknown}) disconnectReason_ = reason; } diff --git a/src/xrpld/telemetry/MetricNames.h b/src/xrpld/telemetry/MetricNames.h new file mode 100644 index 0000000000..ad2ecbf1d1 --- /dev/null +++ b/src/xrpld/telemetry/MetricNames.h @@ -0,0 +1,695 @@ +#pragma once + +/** + * Compile-time OTel metric name constants for the sync-diagnostics signals. + * + * The metric-side counterpart of the `*SpanNames.h` headers: one constant per + * emitted string, so a rename is one edit and a typo is a compile error rather + * than a metric that silently never appears. Each instrument name, label key + * and bounded label value added by the sync-diagnostics work is declared here + * exactly once and referenced from every C++ user of it -- the emit site, the + * gauge registration in MetricsRegistry.cpp, and the unit test. + * + * Layer map -- who references these constants: + * + * +-------------------------------------------------------------+ + * | MetricNames.h (this file, L1-metrics) | + * | namespace metric namespace label namespace lval | + * +-------------------------------------------------------------+ + * ^ ^ ^ + * | | | + * +--------------+ +-------------------+ +---------------------+ + * | emit sites | | MetricsRegistry | | unit test | + * | XRPL_METRIC_ | | Create*Gauge + | | tests/libxrpl/ | + * | macros under | | AddCallback | | telemetry/ | + * | src/xrpld/ | | observe(...) | | MetricMacros.cpp | + * +--------------+ +-------------------+ +---------------------+ + * + * Layers that CANNOT reference a C++ constant -- the collector config, the + * dashboard PromQL, `expected_metrics.json` and the runbook -- are held to + * these same strings by `.github/scripts/otel-naming/check_otel_naming.py` + * instead. + * + * Where this is documented: + * - CONTRIBUTING.md -> "Telemetry metric naming" is the authoritative rule + * list, alongside the sibling span-attribute convention. + * - `.github/scripts/otel-naming/README.md` describes the enforcing rules + * I (no literals), J (suffix conventions) and K (expected_metrics.json). + * - docs/telemetry-runbook.md -> "Adding a New Metric" is the walkthrough for + * adding one, and shows the declare-then-emit pattern. + * + * Naming rules (enforced by the checker's Rule J): + * - Bare `lower_snake_case`. No `xrpld_` prefix in code: the Prometheus + * exporter adds the namespace prefix itself, so writing it here would + * produce `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 at a glance. + * - A duration carries its unit as the suffix: `_us`, `_ms` or `_seconds`. + * The unit belongs in the name because the OTel `unit` argument is not + * surfaced on the Prometheus metric name. + * - A gauge that is a snapshot of current state takes no suffix + * (`jobq_backlog`, `sync_state`). + * - Label VALUES are declared here only when they come from a fixed set that + * the code itself writes (`namespace lval`), which is what keeps series + * cardinality bounded. A value derived from runtime data -- a site URI, a + * peer address, a ledger hash -- is deliberately NOT declared here and must + * never become a label on a metric. + * + * Why `constexpr char[]` and not the `makeStr`/`StaticStr` DSL that + * `SpanNames.h` uses: the OTel C++ API takes `nostd::string_view`, which in + * this build (`OPENTELEMETRY_STL_VERSION` unset, so the back-ported class is + * used) constructs only from `char const*`, `std::string` or + * `(char const*, size)`. It has NO constructor from `std::string_view`, so + * neither `StaticStr` (which converts to `std::string_view`) nor a + * `constexpr std::string_view` compiles in an instrument-name or label-key + * position -- both were tried and both fail with "no viable conversion". + * A `constexpr char[]` decays to `char const*` and binds directly. This also + * matches the precedent already in MetricsRegistry.cpp + * (`kJobQueuedDurationUs`), which this header absorbs. + * + * Example usage -- a labelled counter at an emit site: + * @code + * XRPL_METRIC_COUNTER_INC_LABELED( + * app_, + * metric::dnsResolveTotal, + * "Peer hostname resolutions, by outcome", + * {{label::outcome, + * std::string( + * resolved ? lval::dns_resolve::resolved : lval::dns_resolve::empty)}}); + * @endcode + * + * Example usage -- an observable gauge and its sub-metric discriminators: + * @code + * syncStateGauge_ = meter_->CreateInt64ObservableGauge( + * metric::syncState, "Sync-pipeline health signals"); + * // ... inside the callback: + * observe(lval::sync_state::ledgersBehind, ops.getLedgersBehindNetwork()); + * @endcode + * + * Example usage -- edge case: a value that must NOT be a constant. The site + * URI is runtime data, so only the KEY is named here; declaring the value + * would imply a bounded set that does not exist: + * @code + * XRPL_METRIC_COUNTER_INC_LABELED( + * app_, metric::unlFetchTotal, "...", + * {{label::site, std::string(sites_[siteIdx].loadedResource->uri)}, + * {label::outcome, std::string(outcome)}}); + * @endcode + * + * @note Header-only and dependency-free: nothing here includes an OTel or an + * xrpld header, so `src/tests/libxrpl/telemetry/MetricMacros.cpp` can + * include it even though `xrpl_tests` links only `xrpl.libxrpl`. The + * constants are `inline constexpr`, so they contribute no symbol to link + * against. + * @note Not guarded by `XRPL_ENABLE_TELEMETRY`, for the same reason + * `SpanNames.h` is not: call sites name these constants even where the + * macros expand to no-ops, and the compiler elides any constant whose + * only uses are in dead code. + * @note Every constant is a compile-time value with no mutable state, so all + * of them are safe to read concurrently from any thread. + */ + +namespace xrpl::telemetry { + +/** + * Instrument names -- the metric name as it reaches the OTel meter. + * + * Grouped by the subsystem that emits them, matching how the sync-diagnostics + * work was staged. A name is declared here whether it is created lazily by an + * `XRPL_METRIC_*` macro at a call site or eagerly by a `meter_->Create*` call + * in MetricsRegistry.cpp, because the dashboards cannot tell the two apart. + */ +namespace metric { + +// ===== Bootstrap: getting a fresh node its first peers and its first UNL ===== + +/** + * Time to resolve one configured peer hostname. + */ +inline constexpr char dnsResolveLatencyMs[] = "dns_resolve_latency_ms"; +/** + * Peer hostname resolutions, split by whether any address came back. + */ +inline constexpr char dnsResolveTotal[] = "dns_resolve_total"; +/** + * Time from starting an outbound dial to its terminal outcome. + */ +inline constexpr char overlayDialLatencyMs[] = "overlay_dial_latency_ms"; +/** + * Outbound peer connection attempts, by terminal outcome. + */ +inline constexpr char overlayConnectTotal[] = "overlay_connect_total"; +/** + * Peer handshakes this node rejected, by reason. + */ +inline constexpr char handshakeNegotiationFailTotal[] = "handshake_negotiation_fail_total"; +/** + * Validator-list fetch attempts, by site and outcome. + */ +inline constexpr char unlFetchTotal[] = "unl_fetch_total"; +/** + * Trusted UNL key count against the required quorum. + * + * A gauge, not a counter: both series are current state, and the useful read + * is the difference between them. + */ +inline constexpr char unlQuorum[] = "unl_quorum"; +/** + * Network close-time offset from the local clock. + */ +inline constexpr char clockCloseOffsetSeconds[] = "clock_close_offset_seconds"; + +// ===== Sync state: why this node is not FULL yet ============================= + +/** + * Operating-mode transitions, labelled with the mode pair. + */ +inline constexpr char stateChangesTotal[] = "state_changes_total"; +/** + * Four sync-pipeline health signals, split by the `metric` label. + */ +inline constexpr char syncState[] = "sync_state"; +/** + * Main-loop stall episodes. Cumulative, so `rate()` gives episodes/sec. + */ +inline constexpr char serverStallEventsTotal[] = "server_stall_events_total"; + +// ===== Acquire / SHAMap: is ledger data actually arriving? =================== + +/** + * Aggregate ledger-acquire progress across all in-flight acquires. + */ +inline constexpr char syncAcquire[] = "sync_acquire"; +/** + * SHAMap tree-node cache hit rate, 0.0-1.0. + */ +inline constexpr char shamapCacheHitRate[] = "shamap_cache_hit_rate"; +/** + * Ledger acquires by where the data came from (local store vs network). + */ +inline constexpr char syncAcquireSourceTotal[] = "sync_acquire_source_total"; +/** + * Acquire timeouts where not one new node arrived. + */ +inline constexpr char syncAcquireNoProgressTotal[] = "sync_acquire_no_progress_total"; +/** + * SHAMap nodes received during an acquire, by per-node outcome. + */ +inline constexpr char syncAddnodeTotal[] = "sync_addnode_total"; + +// ===== JobQueue: is the worker pool the bottleneck? ========================== + +/** + * Instantaneous JobQueue occupancy, per job type and per state. + */ +inline constexpr char jobqBacklog[] = "jobq_backlog"; +/** + * Worker-pool saturation: tasks in flight, threads, and jobs queued. + */ +inline constexpr char jobqSaturation[] = "jobq_saturation"; + +// ===== Quorum and publish: can this node accept and publish a ledger? ======= + +/** + * Pre-accept quorum gate and publish lag, split by the `metric` label. + */ +inline constexpr char ledgerQuorumPublish[] = "ledger_quorum_publish"; +/** + * Pre-accept gate rejections for being below quorum. + */ +inline constexpr char ledgerQuorumShortfallTotal[] = "ledger_quorum_shortfall_total"; + +// ===== Back-fill persistence: is history repair making progress? ============= + +/** + * Replay sub-acquires that fell back to a full ledger acquire, by stage. + */ +inline constexpr char ledgerReplayFallbackTotal[] = "ledger_replay_fallback_total"; +/** + * Ledger replay tasks by terminal outcome. + */ +inline constexpr char ledgerReplayOutcomeTotal[] = "ledger_replay_outcome_total"; +/** + * Forced jumps of the last closed ledger to a divergent chain. + */ +inline constexpr char ledgerJumpTotal[] = "ledger_jump_total"; + +// ===== Peer supply: what this node's peers can and will serve =============== + +/** + * Peer coverage of the ledger sequence range this node still needs. + */ +inline constexpr char peerLedgerSupply[] = "peer_ledger_supply"; +/** + * Inbound peer connection attempts, by terminal outcome. + */ +inline constexpr char peerAcceptTotal[] = "peer_accept_total"; +/** + * Peer disconnects, by cause and connection direction. + */ +inline constexpr char peerDisconnectTotal[] = "peer_disconnect_total"; +/** + * Peer data requests this node declined to serve, by kind and cause. + */ +inline constexpr char serveRefusedTotal[] = "serve_refused_total"; +/** + * PeerFinder slots, dials in flight, and address-cache sizes. + */ +inline constexpr char peerfinderSlotCensus[] = "peerfinder_slot_census"; +/** + * Amendment-block warning and the countdown to this node ceasing to validate. + */ +inline constexpr char amendmentBlock[] = "amendment_block"; +/** + * NodeStore mean store/fetch latency, with the operation counts. + */ +inline constexpr char nodestoreLatency[] = "nodestore_latency"; + +// ===== Consensus ============================================================= + +/** + * Wall-clock duration of a completed consensus round. + */ +inline constexpr char consensusRoundDurationMs[] = "consensus_round_duration_ms"; + +// ===== Pre-existing instruments pulled in by the family ratchet ============== +// +// These predate the sync-diagnostics work. They are declared here because the +// checker's Rule I enforces literal-freedom per metric FAMILY (first +// underscore segment), and each of these shares a family with a name above -- +// `ledger_`, `nodestore_`, `server_`, `peer_`, `state_`. Leaving them as +// literals would either weaken the rule to per-name (letting a typo'd sibling +// through) or require an exemption list. Declaring them is the honest option: +// no behaviour changes, and the next author editing these families finds the +// constant rather than inventing a second spelling. +// +// The remaining unconverted families are reported as Rule L warnings, so the +// outstanding work stays visible rather than silently accepted. + +/** + * Built-vs-validated ledger mismatches, by reason. + */ +inline constexpr char ledgerHistoryMismatchTotal[] = "ledger_history_mismatch_total"; +/** + * Ledger fee and economy readings. + */ +inline constexpr char ledgerEconomy[] = "ledger_economy"; +/** + * NodeStore I/O counters, queue depth and write load. + */ +inline constexpr char nodestoreState[] = "nodestore_state"; +/** + * Server-level health readings. + */ +inline constexpr char serverInfo[] = "server_info"; +/** + * Peer-network quality readings. + */ +inline constexpr char peerQuality[] = "peer_quality"; +/** + * Node state and operating-mode tracking. + */ +inline constexpr char stateTracking[] = "state_tracking"; + +} // namespace metric + +/** + * Label keys -- the dimension names attached to a metric datapoint. + * + * Every key here is bounded by design: the values it can take are either a + * fixed set declared in `namespace lval` below, or a small enumeration the + * code derives (an operating mode, a job type). A key whose values are + * unbounded runtime data would mint one time series per distinct value, so no + * such key is declared. + */ +namespace label { + +/** + * Sub-metric discriminator on a multi-series gauge. + * + * The pattern every observable gauge in MetricsRegistry.cpp already uses: one + * instrument carries several related readings, told apart by this label rather + * than by being separate instruments. Pre-dates the sync-diagnostics work; + * named here because the new gauges are its heaviest users. + */ +inline constexpr char metric[] = "metric"; +/** + * Job type, as produced by `JobTypes::name()`. + */ +inline constexpr char jobType[] = "job_type"; +/** + * Terminal result of a bounded operation. + */ +inline constexpr char outcome[] = "outcome"; +/** + * Cause of a rejection, refusal or teardown. + */ +inline constexpr char reason[] = "reason"; +/** + * Configured validator-list site URI. The one runtime-valued key here. + */ +inline constexpr char site[] = "site"; +/** + * Operating mode a transition started from. + */ +inline constexpr char from[] = "from"; +/** + * Operating mode a transition ended at. + */ +inline constexpr char to[] = "to"; +/** + * Where acquired ledger data came from. + */ +inline constexpr char source[] = "source"; +/** + * Which stage of a multi-step pipeline the event belongs to. + */ +inline constexpr char stage[] = "stage"; +/** + * Connection direction, inbound or outbound. + */ +inline constexpr char direction[] = "direction"; +/** + * Which kind of peer data request is being described. + */ +inline constexpr char request[] = "request"; + +} // namespace label + +/** + * Bounded label values -- the fixed value sets the code itself writes. + * + * Nested by the instrument (or the gauge) that owns the set, because the same + * word means different things in different sets and a flat namespace would let + * two of them collide. A value is declared here only when the code chooses it + * from a fixed list; anything derived from runtime data stays out. + */ +namespace lval { + +// ===== Shared outcome/direction slugs ======================================= + +/** + * Values shared by more than one instrument. Declared once so two instruments + * that mean the same thing cannot spell it differently. + */ +inline constexpr char timeout[] = "timeout"; +inline constexpr char notFound[] = "not_found"; +inline constexpr char inbound[] = "inbound"; +inline constexpr char outbound[] = "outbound"; + +/** + * `dns_resolve_total` outcomes: did the resolver return any address? + */ +namespace dns_resolve { +inline constexpr char resolved[] = "resolved"; +inline constexpr char empty[] = "empty"; +} // namespace dns_resolve + +/** + * `peer_accept_total` outcomes -- the nine exits of the inbound-accept path. + * + * Every exit records one of these, so the counter's total equals the number of + * inbound attempts and an unexplained gap is impossible. + */ +namespace peer_accept { +inline constexpr char localEndpointFail[] = "local_endpoint_fail"; +inline constexpr char resourceLimit[] = "resource_limit"; +inline constexpr char noSlot[] = "no_slot"; +inline constexpr char notPeerRequest[] = "not_peer_request"; +inline constexpr char protocolMismatch[] = "protocol_mismatch"; +inline constexpr char badCookie[] = "bad_cookie"; +inline constexpr char slotRefused[] = "slot_refused"; +inline constexpr char accepted[] = "accepted"; +inline constexpr char handshakeError[] = "handshake_error"; +} // namespace peer_accept + +/** + * `handshake_negotiation_fail_total` reasons -- one per rejection point in + * the handshake verifier. + * + * These separate a peer misconfiguration this node should tolerate + * (`wrong_network`, `self_connection`) from a local misconfiguration an + * operator must fix (`clock_skew`, `local_ip_mismatch`), which is the whole + * point of splitting the counter by reason. + */ +namespace handshake_fail { +inline constexpr char invalidServerDomain[] = "invalid_server_domain"; +inline constexpr char invalidNetworkId[] = "invalid_network_id"; +inline constexpr char wrongNetwork[] = "wrong_network"; +inline constexpr char invalidClockTimestamp[] = "invalid_clock_timestamp"; +inline constexpr char clockSkew[] = "clock_skew"; +inline constexpr char unsupportedKeyType[] = "unsupported_key_type"; +inline constexpr char badPublicKey[] = "bad_public_key"; +inline constexpr char noSessionSignature[] = "no_session_signature"; +inline constexpr char sessionVerifyFailed[] = "session_verify_failed"; +inline constexpr char selfConnection[] = "self_connection"; +inline constexpr char invalidLocalIp[] = "invalid_local_ip"; +inline constexpr char localIpMismatch[] = "local_ip_mismatch"; +inline constexpr char invalidRemoteIp[] = "invalid_remote_ip"; +inline constexpr char remoteIpMismatch[] = "remote_ip_mismatch"; +} // namespace handshake_fail + +/** + * `unl_fetch_total` outcomes for the transport-level failures. + * + * The success path instead labels with `to_string(ListDisposition)`, whose + * values are owned by the protocol enum and are therefore not restated here -- + * duplicating them would create a second place to update when a disposition is + * added. + */ +namespace unl_fetch { +inline constexpr char fetchError[] = "fetch_error"; +inline constexpr char badStatus[] = "bad_status"; +inline constexpr char parseError[] = "parse_error"; +} // namespace unl_fetch + +/** + * `unl_quorum` sub-metrics: the trusted-key count and the bar it must clear. + */ +namespace unl_quorum { +inline constexpr char trustedKeys[] = "trusted_keys"; +inline constexpr char quorum[] = "quorum"; +} // namespace unl_quorum + +/** + * `clock_close_offset_seconds` sub-metric. + */ +namespace clock_offset { +inline constexpr char offset[] = "offset"; +} // namespace clock_offset + +/** + * `sync_state` sub-metrics -- the four "why am I not FULL" signals. + * + * `initial_full_duration_us` reads zero until the node first reaches FULL, + * which is the state this gauge exists to make visible rather than a missing + * value. + */ +namespace sync_state { +inline constexpr char initialFullDurationUs[] = "initial_full_duration_us"; +inline constexpr char networkLedgerGate[] = "network_ledger_gate"; +inline constexpr char serverStallSeconds[] = "server_stall_seconds"; +inline constexpr char ledgersBehind[] = "ledgers_behind"; +} // namespace sync_state + +/** + * `sync_acquire` sub-metrics -- aggregate acquire progress. + * + * The two `missing_*_max` series are the stuck-detector: flat and non-zero + * across collection ticks means the acquire will never finish, while shrinking + * means slow but alive. `in_flight` is the context that tells idle from stuck. + */ +namespace sync_acquire { +inline constexpr char missingStateNodesMax[] = "missing_state_nodes_max"; +inline constexpr char missingTxNodesMax[] = "missing_tx_nodes_max"; +inline constexpr char receivedDataDepth[] = "received_data_depth"; +inline constexpr char inFlight[] = "in_flight"; +} // namespace sync_acquire + +/** + * `shamap_cache_hit_rate` sub-metric: which cache the rate describes. + */ +namespace shamap_cache { +inline constexpr char treenode[] = "treenode"; +} // namespace shamap_cache + +/** + * `sync_acquire_source_total` sources: served locally or fetched from peers. + */ +namespace acquire_source { +inline constexpr char local[] = "local"; +inline constexpr char network[] = "network"; +} // namespace acquire_source + +/** + * `sync_addnode_total` outcomes -- the per-node verdict on received SHAMap data. + * + * The split is what separates real progress (`good`) from wasted bandwidth + * (`duplicate`) and a misbehaving peer (`invalid`); traffic-level metrics show + * all three as healthy throughput. + */ +namespace addnode { +inline constexpr char good[] = "good"; +inline constexpr char duplicate[] = "duplicate"; +inline constexpr char invalid[] = "invalid"; +} // namespace addnode + +/** + * `jobq_backlog` sub-metrics -- the three occupancy states of a job type. + * + * `deferred` has no other exposure anywhere: a job held back by its type's + * concurrency limit counts as neither waiting nor running. + */ +namespace jobq_backlog { +inline constexpr char waiting[] = "waiting"; +inline constexpr char running[] = "running"; +inline constexpr char deferred[] = "deferred"; +} // namespace jobq_backlog + +/** + * `jobq_saturation` sub-metrics: the numerator, denominator and the backlog. + */ +namespace jobq_saturation { +inline constexpr char runningTasks[] = "running_tasks"; +inline constexpr char workerThreads[] = "worker_threads"; +inline constexpr char totalWaiting[] = "total_waiting"; +} // namespace jobq_saturation + +/** + * `peer_ledger_supply` sub-metrics: who can serve what this node needs. + */ +namespace peer_supply { +inline constexpr char peersReporting[] = "peers_reporting"; +inline constexpr char peersServingValidated[] = "peers_serving_validated"; +inline constexpr char peersServingNext[] = "peers_serving_next"; +inline constexpr char supplyMinSeq[] = "supply_min_seq"; +inline constexpr char supplyMaxSeq[] = "supply_max_seq"; +} // namespace peer_supply + +/** + * `peerfinder_slot_census` sub-metrics -- slots, dials and address caches. + * + * `connecting` non-zero while `out_active` stays under `out_max` is the + * "starting dials and never completing them" case; both caches at zero on a + * fresh node means there is nothing left to dial at all. + */ +namespace slot_census { +inline constexpr char outActive[] = "out_active"; +inline constexpr char outMax[] = "out_max"; +inline constexpr char inActive[] = "in_active"; +inline constexpr char inMax[] = "in_max"; +inline constexpr char connecting[] = "connecting"; +inline constexpr char fixedConfigured[] = "fixed_configured"; +inline constexpr char fixedActive[] = "fixed_active"; +inline constexpr char bootcache[] = "bootcache"; +inline constexpr char livecache[] = "livecache"; +} // namespace slot_census + +/** + * `amendment_block` sub-metrics: the warning flag and the countdown. + */ +namespace amendment_block { +inline constexpr char warned[] = "warned"; +inline constexpr char secondsToBlock[] = "seconds_to_block"; +} // namespace amendment_block + +/** + * `nodestore_latency` sub-metrics: mean latency per direction, with counts. + */ +namespace nodestore_latency { +inline constexpr char writeCount[] = "write_count"; +inline constexpr char readCount[] = "read_count"; +inline constexpr char writeMeanUs[] = "write_mean_us"; +inline constexpr char readMeanUs[] = "read_mean_us"; +} // namespace nodestore_latency + +/** + * `ledger_quorum_publish` sub-metrics: the gate, and how late publish is. + */ +namespace quorum_publish { +inline constexpr char trustedValidationTally[] = "trusted_validation_tally"; +inline constexpr char quorumTarget[] = "quorum_target"; +inline constexpr char timeToFirstValidatedUs[] = "time_to_first_validated_us"; +inline constexpr char publishLag[] = "publish_lag"; +} // namespace quorum_publish + +/** + * `ledger_quorum_shortfall_total` stage: which gate did the rejecting. + */ +namespace quorum_shortfall { +inline constexpr char preAccept[] = "pre_accept"; +} // namespace quorum_shortfall + +/** + * `ledger_replay_fallback_total` stages: which sub-acquire gave up. + */ +namespace replay_fallback { +inline constexpr char skiplist[] = "skiplist"; +inline constexpr char delta[] = "delta"; +} // namespace replay_fallback + +/** + * `ledger_replay_outcome_total` outcomes -- the four terminal states of a + * replay task. `timeout` is the shared slug above. + */ +namespace replay_outcome { +inline constexpr char success[] = "success"; +inline constexpr char buildFailed[] = "build_failed"; +inline constexpr char parameterFailed[] = "parameter_failed"; +} // namespace replay_outcome + +/** + * `peer_disconnect_total` reasons -- why a peer connection closed. + * + * The split separates our-fault backpressure (`large_sendq`, + * `charge_resources`) from a topology or network fault (`not_useful`, + * `ping_timeout`, `read_error`); the two call for opposite responses. + * `unknown` is the initial value and appears when a teardown path set no + * cause, so an unattributed disconnect is visible rather than absent. + */ +namespace disconnect { +inline constexpr char unknown[] = "unknown"; +inline constexpr char malformedHandshake[] = "malformed_handshake"; +inline constexpr char stopping[] = "stopping"; +inline constexpr char chargeResources[] = "charge_resources"; +inline constexpr char timerError[] = "timer_error"; +inline constexpr char largeSendq[] = "large_sendq"; +inline constexpr char notUseful[] = "not_useful"; +inline constexpr char pingTimeout[] = "ping_timeout"; +inline constexpr char shutdown[] = "shutdown"; +inline constexpr char sharedValue[] = "shared_value"; +inline constexpr char writeError[] = "write_error"; +inline constexpr char graceful[] = "graceful"; +inline constexpr char readError[] = "read_error"; +} // namespace disconnect + +/** + * `serve_refused_total` request kinds: what the peer had asked for. + */ +namespace serve_request { +inline constexpr char object[] = "object"; +inline constexpr char fetchpack[] = "fetchpack"; +inline constexpr char txset[] = "txset"; +inline constexpr char ledger[] = "ledger"; +} // namespace serve_request + +/** + * `serve_refused_total` reasons: why this node would not answer. + * `not_found` is the shared slug above. + * + * `empty_reply` is the subtle one: the map WAS found, but the reply loop + * produced no nodes, so the requester still gets nothing and must ask another + * peer. Counting it as served would make a node that answers every request + * with an empty payload look healthy. + */ +namespace serve_refused { +inline constexpr char sendqFull[] = "sendq_full"; +inline constexpr char loadShed[] = "load_shed"; +inline constexpr char badType[] = "bad_type"; +inline constexpr char noMap[] = "no_map"; +inline constexpr char emptyReply[] = "empty_reply"; +} // namespace serve_refused + +} // namespace lval + +} // namespace xrpl::telemetry diff --git a/src/xrpld/telemetry/MetricsRegistry.cpp b/src/xrpld/telemetry/MetricsRegistry.cpp index aba1a313dc..27b3e35c2b 100644 --- a/src/xrpld/telemetry/MetricsRegistry.cpp +++ b/src/xrpld/telemetry/MetricsRegistry.cpp @@ -36,6 +36,7 @@ #include #include #include +#include #include #include @@ -369,7 +370,7 @@ MetricsRegistry::initSyncInstruments() }, this); ledgerHistoryMismatchCounter_ = meter_->CreateUInt64Counter( - "ledger_history_mismatch_total", "Total built-vs-validated ledger mismatches by reason"); + metric::ledgerHistoryMismatchTotal, "Total built-vs-validated ledger mismatches by reason"); txqExpiredCounter_ = meter_->CreateUInt64Counter( "txq_expired_total", "Total transactions expired out of the transaction queue"); txqDroppedCounter_ = meter_->CreateUInt64Counter( @@ -815,7 +816,7 @@ MetricsRegistry::registerNodeStoreGauge() // libxrpl nodestore code — the MetricsRegistry reads the existing atomic // counters from Database via its public accessors. nodeStoreGauge_ = meter_->CreateInt64ObservableGauge( - "nodestore_state", "NodeStore I/O counters, queue depth, and write load"); + metric::nodestoreState, "NodeStore I/O counters, queue depth, and write load"); nodeStoreGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto* self = static_cast(state); @@ -894,7 +895,7 @@ MetricsRegistry::registerServerInfoGauge() { // --- Task 9.7a: Server info gauges --- serverInfoGauge_ = - meter_->CreateInt64ObservableGauge("server_info", "Server-level health metrics"); + meter_->CreateInt64ObservableGauge(metric::serverInfo, "Server-level health metrics"); serverInfoGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto* self = static_cast(state); @@ -1146,7 +1147,7 @@ MetricsRegistry::registerPeerQualityGauge() // Uses Peer::json() to read latency and version since those accessors // are not on the abstract Peer interface (they live on PeerImp). peerQualityGauge_ = - meter_->CreateDoubleObservableGauge("peer_quality", "Peer network quality metrics"); + meter_->CreateDoubleObservableGauge(metric::peerQuality, "Peer network quality metrics"); peerQualityGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto* self = static_cast(state); @@ -1297,8 +1298,8 @@ void MetricsRegistry::registerLedgerEconomyGauge() { // --- Task 7.11: Ledger economy gauges --- - ledgerEconomyGauge_ = - meter_->CreateDoubleObservableGauge("ledger_economy", "Ledger fee and economy metrics"); + ledgerEconomyGauge_ = meter_->CreateDoubleObservableGauge( + metric::ledgerEconomy, "Ledger fee and economy metrics"); ledgerEconomyGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto* self = static_cast(state); @@ -1364,7 +1365,7 @@ MetricsRegistry::registerStateTrackingGauge() { // --- Task 7.12: State tracking gauges --- stateTrackingGauge_ = - meter_->CreateDoubleObservableGauge("state_tracking", "Node state and mode tracking"); + meter_->CreateDoubleObservableGauge(metric::stateTracking, "Node state and mode tracking"); stateTrackingGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto* self = static_cast(state); @@ -1567,7 +1568,7 @@ MetricsRegistry::registerUnlQuorumGauge() // with the trusted-key count in one instrument is what makes the // "can this node ever validate?" comparison a single query. unlQuorumGauge_ = meter_->CreateInt64ObservableGauge( - "unl_quorum", "Trusted UNL key count vs required quorum"); + metric::unlQuorum, "Trusted UNL key count vs required quorum"); unlQuorumGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto* self = static_cast(state); @@ -1587,7 +1588,9 @@ MetricsRegistry::registerUnlQuorumGauge() // Trusted master keys currently in effect. Zero means no // usable UNL: quorum can never be met. - observe("trusted_keys", static_cast(validators.trustedKeyCount())); + observe( + lval::unl_quorum::trustedKeys, + static_cast(validators.trustedKeyCount())); // Validations required for a ledger to be fully validated. // ValidatorList disables quorum by returning SIZE_MAX when too @@ -1599,7 +1602,7 @@ MetricsRegistry::registerUnlQuorumGauge() // truthful signal. auto const quorum = validators.quorum(); observe( - "quorum", + lval::unl_quorum::quorum, quorum == std::numeric_limits::max() ? std::numeric_limits::max() : static_cast(quorum)); @@ -1620,7 +1623,8 @@ MetricsRegistry::registerClockSkewGauge() // network, which delays consensus participation. server_info hides // this below 60 s, so export it continuously instead. clockSkewGauge_ = meter_->CreateInt64ObservableGauge( - "clock_close_offset_seconds", "Network close time offset from the local clock, in seconds"); + metric::clockCloseOffsetSeconds, + "Network close time offset from the local clock, in seconds"); clockSkewGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto* self = static_cast(state); @@ -1637,7 +1641,9 @@ MetricsRegistry::registerClockSkewGauge() }; // Negative when the local clock runs ahead of the network. - observe("offset", static_cast(app.getTimeKeeper().closeOffset().count())); + observe( + lval::clock_offset::offset, + static_cast(app.getTimeKeeper().closeOffset().count())); } catch (...) // NOLINT(bugprone-empty-catch) { @@ -1654,7 +1660,7 @@ MetricsRegistry::registerSyncStateGauge() // Four values that previously lived only in a log line or in server_info // JSON. All four are cheap reads pulled on the ~10 s reader tick. syncStateGauge_ = - meter_->CreateInt64ObservableGauge("sync_state", "Sync-pipeline health signals"); + meter_->CreateInt64ObservableGauge(metric::syncState, "Sync-pipeline health signals"); syncStateGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto* self = static_cast(state); @@ -1675,21 +1681,23 @@ MetricsRegistry::registerSyncStateGauge() // Time to first FULL. Zero means the node has not synced yet, // which is exactly the case this signal exists to expose. observe( - "initial_full_duration_us", + lval::sync_state::initialFullDurationUs, static_cast(ops.getInitialSyncDurationUs())); // 1 = still waiting for a full network ledger. While this is // set the node refuses transactions and cannot reach FULL. - observe("network_ledger_gate", ops.isNeedNetworkLedger() ? 1 : 0); + observe(lval::sync_state::networkLedgerGate, ops.isNeedNetworkLedger() ? 1 : 0); // Current main-loop stall duration; 0 when healthy. observe( - "server_stall_seconds", + lval::sync_state::serverStallSeconds, static_cast(app.getLoadManager().getCurrentStallSeconds())); // Distance from the network tip, floored at zero by the // accessor. - observe("ledgers_behind", static_cast(ops.getLedgersBehindNetwork())); + observe( + lval::sync_state::ledgersBehind, + static_cast(ops.getLedgersBehindNetwork())); } catch (...) // NOLINT(bugprone-empty-catch) { @@ -1709,7 +1717,7 @@ MetricsRegistry::registerStallEventsCounter() // Kept out of the sync_state gauge because a cumulative total needs // counter aggregation for rate() to be meaningful. stallEventsObservable_ = meter_->CreateInt64ObservableCounter( - "server_stall_events_total", "Total server main-loop stall episodes"); + metric::serverStallEventsTotal, "Total server main-loop stall episodes"); stallEventsObservable_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto* self = static_cast(state); @@ -1738,7 +1746,7 @@ MetricsRegistry::registerSyncAcquireGauge() // acquired, which is unbounded. The per-ledger view lives on the // ledger.acquire span instead. syncAcquireGauge_ = meter_->CreateInt64ObservableGauge( - "sync_acquire", "Aggregate ledger-acquire progress across in-flight acquires"); + metric::syncAcquire, "Aggregate ledger-acquire progress across in-flight acquires"); syncAcquireGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto* self = static_cast(state); @@ -1761,15 +1769,20 @@ MetricsRegistry::registerSyncAcquireGauge() // Flat and non-zero across ticks = this acquire will never // finish. Shrinking = slow but alive. observe( - "missing_state_nodes_max", static_cast(progress.maxMissingStateNodes)); - observe("missing_tx_nodes_max", static_cast(progress.maxMissingTxNodes)); + lval::sync_acquire::missingStateNodesMax, + static_cast(progress.maxMissingStateNodes)); + observe( + lval::sync_acquire::missingTxNodesMax, + static_cast(progress.maxMissingTxNodes)); // Deep stash = arriving data outpaces processing. - observe("received_data_depth", static_cast(progress.receivedDataDepth)); + observe( + lval::sync_acquire::receivedDataDepth, + static_cast(progress.receivedDataDepth)); // Context for the three above: zero everywhere with zero // in-flight acquires is idle, not healthy. - observe("in_flight", static_cast(progress.inFlight)); + observe(lval::sync_acquire::inFlight, static_cast(progress.inFlight)); } catch (...) // NOLINT(bugprone-empty-catch) { @@ -1786,7 +1799,7 @@ MetricsRegistry::registerCacheHitRateDetailGauge() // The memory layer above the node store: a miss here is what causes a // node-store read, which the NuDB hit-ratio panel then measures. shamapCacheHitRateGauge_ = meter_->CreateDoubleObservableGauge( - "shamap_cache_hit_rate", "SHAMap tree-node cache hit rate (0.0-1.0), by cache"); + metric::shamapCacheHitRate, "SHAMap tree-node cache hit rate (0.0-1.0), by cache"); shamapCacheHitRateGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto* self = static_cast(state); @@ -1802,7 +1815,8 @@ MetricsRegistry::registerCacheHitRateDetailGauge() auto const rate = app.getNodeFamily().getTreeNodeCache()->getHitRate() / 100.0F; opentelemetry::nostd::get>>(result) - ->Observe(static_cast(rate), {{"metric", "treenode"}}); + ->Observe( + static_cast(rate), {{label::metric, lval::shamap_cache::treenode}}); } catch (...) // NOLINT(bugprone-empty-catch) { @@ -1821,7 +1835,7 @@ MetricsRegistry::registerJobQueueBacklogGauge() // has no other exposure: a job held back by its type's concurrency limit // counts as neither waiting nor running anywhere else. jobQueueBacklogGauge_ = meter_->CreateInt64ObservableGauge( - "jobq_backlog", "JobQueue occupancy per job type (waiting/running/deferred)"); + metric::jobqBacklog, "JobQueue occupancy per job type (waiting/running/deferred)"); jobQueueBacklogGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto* self = static_cast(state); @@ -1834,7 +1848,7 @@ MetricsRegistry::registerJobQueueBacklogGauge() auto observe = [&](char const* field, std::string const& jobType, int64_t value) { opentelemetry::nostd::get>>(result) - ->Observe(value, {{"metric", field}, {"job_type", jobType}}); + ->Observe(value, {{label::metric, field}, {label::jobType, jobType}}); }; // One snapshot under one lock acquire, so the three fields of @@ -1846,9 +1860,9 @@ MetricsRegistry::registerJobQueueBacklogGauge() // the same one the job_*_total counters already use, so the // two label sets join. auto const& jobType = JobTypes::name(count.type); - observe("waiting", jobType, count.waiting); - observe("running", jobType, count.running); - observe("deferred", jobType, count.deferred); + observe(lval::jobq_backlog::waiting, jobType, count.waiting); + observe(lval::jobq_backlog::running, jobType, count.running); + observe(lval::jobq_backlog::deferred, jobType, count.deferred); } } catch (...) // NOLINT(bugprone-empty-catch) @@ -1867,7 +1881,8 @@ MetricsRegistry::registerJobQueueSaturationGauge() // leaving it to look like an independent fault in every subsystem whose // jobs are queued behind it. jobQueueSaturationGauge_ = meter_->CreateInt64ObservableGauge( - "jobq_saturation", "Worker-pool saturation: tasks in flight, worker threads, jobs queued"); + metric::jobqSaturation, + "Worker-pool saturation: tasks in flight, worker threads, jobs queued"); jobQueueSaturationGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto* self = static_cast(state); @@ -1886,16 +1901,16 @@ MetricsRegistry::registerJobQueueSaturationGauge() // One reading feeds all three series so the ratio and the // backlog describe the same instant. auto const saturation = app.getJobQueue().getWorkerSaturation(); - observe("running_tasks", saturation.runningTasks); + observe(lval::jobq_saturation::runningTasks, saturation.runningTasks); // The denominator for the ratio panel. Derived at startup from // [workers], node size and hardware concurrency, so it cannot // be hardcoded in a dashboard. - observe("worker_threads", saturation.workerThreads); + observe(lval::jobq_saturation::workerThreads, saturation.workerThreads); // Ratio at 1.0 alone is a busy pool; ratio at 1.0 with a // non-zero backlog is an exhausted one. - observe("total_waiting", saturation.totalWaiting); + observe(lval::jobq_saturation::totalWaiting, saturation.totalWaiting); } catch (...) // NOLINT(bugprone-empty-catch) { @@ -1914,7 +1929,7 @@ MetricsRegistry::registerPeerLedgerSupplyGauge() // want" looked exactly like "my peers are slow" -- two faults with // completely different fixes. peerLedgerSupplyGauge_ = meter_->CreateInt64ObservableGauge( - "peer_ledger_supply", "Peer coverage of the ledger sequence this node needs"); + metric::peerLedgerSupply, "Peer coverage of the ledger sequence this node needs"); peerLedgerSupplyGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto* self = static_cast(state); @@ -1937,17 +1952,17 @@ MetricsRegistry::registerPeerLedgerSupplyGauge() // The denominator. Zero serving out of zero reporting is // silence; zero out of many is a real supply gap. - observe("peers_reporting", supply.peersReporting); - observe("peers_serving_validated", supply.peersServingValidated); + observe(lval::peer_supply::peersReporting, supply.peersReporting); + observe(lval::peer_supply::peersServingValidated, supply.peersServingValidated); // The verdict: zero here while peers_reporting is non-zero // means waiting cannot finish the sync. - observe("peers_serving_next", supply.peersServingNext); + observe(lval::peer_supply::peersServingNext, supply.peersServingNext); // The window the peer set covers, so an operator can tell a // request for discarded history from one for an unreached tip. - observe("supply_min_seq", supply.supplyMinSeq); - observe("supply_max_seq", supply.supplyMaxSeq); + observe(lval::peer_supply::supplyMinSeq, supply.supplyMinSeq); + observe(lval::peer_supply::supplyMaxSeq, supply.supplyMaxSeq); } catch (...) // NOLINT(bugprone-empty-catch) { @@ -1965,7 +1980,7 @@ MetricsRegistry::registerSlotCensusGauge() // counts are exported today, which cannot distinguish "not dialling", // "dialling and failing" and "nothing to dial". slotCensusGauge_ = meter_->CreateInt64ObservableGauge( - "peerfinder_slot_census", "PeerFinder slots, connection attempts and address caches"); + metric::peerfinderSlotCensus, "PeerFinder slots, connection attempts and address caches"); slotCensusGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto* self = static_cast(state); @@ -1985,23 +2000,23 @@ MetricsRegistry::registerSlotCensusGauge() // and capacity can be compared against each other. auto const census = app.getOverlay().getSlotCensus(); - observe("out_active", census.outActive); - observe("out_max", census.outMax); - observe("in_active", census.inActive); - observe("in_max", census.inMax); + observe(lval::slot_census::outActive, census.outActive); + observe(lval::slot_census::outMax, census.outMax); + observe(lval::slot_census::inActive, census.inActive); + observe(lval::slot_census::inMax, census.inMax); // Dials in flight. Non-zero while out_active stays under // out_max is the "starting and never completing" case. - observe("connecting", census.connecting); + observe(lval::slot_census::connecting, census.connecting); // fixed_active below fixed_configured names a configured peer // that cannot be reached. - observe("fixed_configured", census.fixedConfigured); - observe("fixed_active", census.fixedActive); + observe(lval::slot_census::fixedConfigured, census.fixedConfigured); + observe(lval::slot_census::fixedActive, census.fixedActive); // Both at zero on a fresh node means there is nothing to dial. - observe("bootcache", census.bootcache); - observe("livecache", census.livecache); + observe(lval::slot_census::bootcache, census.bootcache); + observe(lval::slot_census::livecache, census.livecache); } catch (...) // NOLINT(bugprone-empty-catch) { @@ -2018,7 +2033,8 @@ MetricsRegistry::registerAmendmentBlockGauge() // The existing validator_health{metric="amendment_blocked"} reports the // terminal state, when nothing can be done. This is the window before it. amendmentBlockGauge_ = meter_->CreateInt64ObservableGauge( - "amendment_block", "Amendment-block warning and seconds until the node stops validating"); + metric::amendmentBlock, + "Amendment-block warning and seconds until the node stops validating"); amendmentBlockGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto* self = static_cast(state); @@ -2036,7 +2052,7 @@ MetricsRegistry::registerAmendmentBlockGauge() // An unsupported amendment has reached majority. Until now this // only surfaced as an admin-only server_info warning. - observe("warned", app.getOPs().isAmendmentWarned() ? 1 : 0); + observe(lval::amendment_block::warned, app.getOPs().isAmendmentWarned() ? 1 : 0); // Seconds until that amendment activates. -1 means nothing is // pending: a distinct healthy value rather than an absent @@ -2056,7 +2072,7 @@ MetricsRegistry::registerAmendmentBlockGauge() // overdue by an amount worth charting. secondsToBlock = std::max(expectedSecs - nowSecs, 0); } - observe("seconds_to_block", secondsToBlock); + observe(lval::amendment_block::secondsToBlock, secondsToBlock); } catch (...) // NOLINT(bugprone-empty-catch) { @@ -2082,7 +2098,8 @@ MetricsRegistry::registerNodeStoreLatencyGauge() // ledger write. This reads four atomics per ~10 s tick instead. The // trade-off is that percentiles are unavailable -- see the header comment. nodeStoreLatencyGauge_ = meter_->CreateInt64ObservableGauge( - "nodestore_latency", "NodeStore mean store/fetch latency in microseconds, with counts"); + metric::nodestoreLatency, + "NodeStore mean store/fetch latency in microseconds, with counts"); nodeStoreLatencyGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { auto* self = static_cast(state); @@ -2109,8 +2126,8 @@ MetricsRegistry::registerNodeStoreLatencyGauge() // Counts are always observed, including zero: that is what // separates "nothing written yet" from "writes are instant". - observe("write_count", static_cast(storeCount)); - observe("read_count", static_cast(fetchCount)); + observe(lval::nodestore_latency::writeCount, static_cast(storeCount)); + observe(lval::nodestore_latency::readCount, static_cast(fetchCount)); // A mean needs a non-zero denominator, and it needs a // numerator that was actually measured. Both are required, and @@ -2128,9 +2145,17 @@ MetricsRegistry::registerNodeStoreLatencyGauge() // stays 0. Omitting the mean makes that a visible data gap // instead of a false "writes take 0 us" line on the panel. if (storeCount > 0 && storeDurationUs > 0) - observe("write_mean_us", static_cast(storeDurationUs / storeCount)); + { + observe( + lval::nodestore_latency::writeMeanUs, + static_cast(storeDurationUs / storeCount)); + } if (fetchCount > 0 && fetchDurationUs > 0) - observe("read_mean_us", static_cast(fetchDurationUs / fetchCount)); + { + observe( + lval::nodestore_latency::readMeanUs, + static_cast(fetchDurationUs / fetchCount)); + } } catch (...) // NOLINT(bugprone-empty-catch) { @@ -2149,7 +2174,7 @@ MetricsRegistry::registerLedgerQuorumPublishGauge() // one validated (quorum short), or validate correctly and never publish // (pipeline behind). Both used to be trace-log-only or not derivable at all. ledgerQuorumPublishGauge_ = meter_->CreateInt64ObservableGauge( - "ledger_quorum_publish", + metric::ledgerQuorumPublish, "Pre-accept quorum gate and publish lag (tally vs quorum, first-validated, lag)"); ledgerQuorumPublishGauge_->AddCallback( [](opentelemetry::metrics::ObserverResult result, void* state) { @@ -2171,21 +2196,25 @@ MetricsRegistry::registerLedgerQuorumPublishGauge() // The pair that separates "slow" from "stuck". A tally climbing // toward the target will get there; a tally flat below it never // will, and no acquire or peer panel says which is happening. - observe("trusted_validation_tally", ledgerMaster.getTrustedValidationTally()); + observe( + lval::quorum_publish::trustedValidationTally, + ledgerMaster.getTrustedValidationTally()); // What the last gate evaluation actually required, as opposed to // unl_quorum{quorum} which is what the trusted list configures. // Already clamped against the SIZE_MAX "quorum disabled" // sentinel by LedgerMaster, so this never wraps negative. - observe("quorum_target", ledgerMaster.getQuorumTarget()); + observe(lval::quorum_publish::quorumTarget, ledgerMaster.getQuorumTarget()); // One-shot: a value is the time the first ledger took to pass // the gate, and 0 means it never has. Not a trend. - observe("time_to_first_validated_us", ledgerMaster.getTimeToFirstValidatedUs()); + observe( + lval::quorum_publish::timeToFirstValidatedUs, + ledgerMaster.getTimeToFirstValidatedUs()); // Validated but not yet published. pubLedgerSeq_ was never // exported, so this gap was not derivable from any other series. - observe("publish_lag", ledgerMaster.getPublishLag()); + observe(lval::quorum_publish::publishLag, ledgerMaster.getPublishLag()); } catch (...) // NOLINT(bugprone-empty-catch) {